Merge pull request #333 from aschackmull/java/useless-comp-concurrent

Approved by yh-semmle

Author: semmle-qlci

change-notes/1.19/analysis-java.md

@@ -13,6 +13,7 @@
 |----------------------------|------------------------|------------------------------------------------------------------|
 | Array index out of bounds (`java/index-out-of-bounds`) | Fewer false positive results | False positives involving arrays with a length evenly divisible by 3 or some greater number and an index being increased with a similar stride length are no longer reported. |
 | Unreachable catch clause (`java/unreachable-catch-clause`) | Fewer false positive results | This rule now accounts for calls to generic methods that throw generic exceptions. |
+| Useless comparison test (`java/constant-comparison`) | Fewer false positive results | Constant comparisons guarding `java.util.ConcurrentModificationException` are no longer reported, as they are intended to always be false in the absence of API misuse. |
 
 ## Changes to QL libraries
 

java/ql/src/Likely Bugs/Comparison/UselessComparisonTest.ql

@@ -167,6 +167,15 @@ predicate overFlowTest(ComparisonExpr comp) {
   comp.getGreaterOperand().(IntegerLiteral).getIntValue() = 0
 }
 
+predicate concurrentModificationTest(BinaryExpr test) {
+  exists(IfStmt ifstmt, ThrowStmt throw, RefType exc |
+    ifstmt.getCondition() = test and
+    (ifstmt.getThen() = throw or ifstmt.getThen().(SingletonBlock).getStmt() = throw) and
+    throw.getExpr().(ClassInstanceExpr).getConstructedType() = exc and
+    exc.hasQualifiedName("java.util", "ConcurrentModificationException")
+  )
+}
+
 /**
  * Holds if `test` and `guard` are equality tests of the same integral variable v with constants `c1` and `c2`.
  */
@@ -202,13 +211,13 @@ where
       )
     else
       if constCondSimple(test, _)
-      then (
-        constCondSimple(test, testIsTrue) and reason = "" and reasonElem = test // dummy reason element
-      ) else
+      then constCondSimple(test, testIsTrue) and reason = "" and reasonElem = test // dummy reason element
+      else
         exists(CondReason r |
           constCond(test, testIsTrue, r) and reason = ", because of $@" and reasonElem = r.getCond()
         )
   ) and
   not overFlowTest(test) and
+  not concurrentModificationTest(test) and
   not exists(AssertStmt assert | assert.getExpr() = test.getParent*())
 select test, "Test is always " + testIsTrue + reason + ".", reasonElem, "this condition"

java/ql/test/query-tests/UselessComparisonTest/B.java

@@ -0,0 +1,22 @@
+import java.util.*;
+import java.util.function.*;
+
+public class B {
+  int modcount = 0;
+
+  int[] arr;
+
+  public void modify(IntUnaryOperator func) {
+    int mc = modcount;
+    for (int i = 0; i < arr.length; i++) {
+      arr[i] = func.applyAsInt(arr[i]);
+    }
+    // Always false unless there is a call path from func.applyAsInt(..) to
+    // this method, but should not be reported, as checks guarding
+    // ConcurrentModificationExceptions are expected to always be false in the
+    // absence of API misuse.
+    if (mc != modcount)
+      throw new ConcurrentModificationException();
+    modcount++;
+  }
+}