Guards: Improve support for wrapped guards

java/ql/lib/change-notes/2025-07-28-guardwrappers.md

@@ -0,0 +1,4 @@
+---
+category: minorAnalysis
+---
+* Guard implication logic involving wrapper methods has been improved. In particular, this means fewer false positives for `java/dereferenced-value-may-be-null`.

java/ql/lib/semmle/code/java/controlflow/Guards.qll

@@ -146,6 +146,8 @@ private module GuardsInput implements SharedGuards::InputSig<Location> {
 
   class ControlFlowNode = J::ControlFlowNode;
 
+  class NormalExitNode = ControlFlow::NormalExitNode;
+
   class BasicBlock = J::BasicBlock;
 
   predicate dominatingEdge(BasicBlock bb1, BasicBlock bb2) { J::dominatingEdge(bb1, bb2) }
@@ -322,6 +324,55 @@ private module GuardsInput implements SharedGuards::InputSig<Location> {
 
     Expr getElse() { result = super.getFalseExpr() }
   }
+
+  class Parameter = J::Parameter;
+
+  private int parameterPosition() { result in [-1, any(Parameter p).getPosition()] }
+
+  /** A parameter position represented by an integer. */
+  class ParameterPosition extends int {
+    ParameterPosition() { this = parameterPosition() }
+  }
+
+  /** An argument position represented by an integer. */
+  class ArgumentPosition extends int {
+    ArgumentPosition() { this = parameterPosition() }
+  }
+
+  /** Holds if arguments at position `apos` match parameters at position `ppos`. */
+  overlay[caller?]
+  pragma[inline]
+  predicate parameterMatch(ParameterPosition ppos, ArgumentPosition apos) { ppos = apos }
+
+  final private class FinalMethod = Method;
+
+  class NonOverridableMethod extends FinalMethod {
+    NonOverridableMethod() { not super.isOverridable() }
+
+    Parameter getParameter(ParameterPosition ppos) {
+      super.getParameter(ppos) = result and
+      not result.isVarargs()
+    }
+
+    GuardsInput::Expr getAReturnExpr() {
+      exists(ReturnStmt ret |
+        this = ret.getEnclosingCallable() and
+        ret.getResult() = result
+      )
+    }
+  }
+
+  private predicate nonOverridableMethodCall(MethodCall call, NonOverridableMethod m) {
+    call.getMethod().getSourceDeclaration() = m
+  }
+
+  class NonOverridableMethodCall extends GuardsInput::Expr instanceof MethodCall {
+    NonOverridableMethodCall() { nonOverridableMethodCall(this, _) }
+
+    NonOverridableMethod getMethod() { nonOverridableMethodCall(this, result) }
+
+    GuardsInput::Expr getArgument(ArgumentPosition apos) { result = super.getArgument(apos) }
+  }
 }
 
 private module GuardsImpl = SharedGuards::Make<Location, GuardsInput>;
@@ -340,6 +391,17 @@ private module LogicInputCommon {
       NullGuards::nullCheckMethod(call.getMethod(), val.asBooleanValue(), isNull)
     )
   }
+
+  predicate additionalImpliesStep(
+    GuardsImpl::PreGuard g1, GuardValue v1, GuardsImpl::PreGuard g2, GuardValue v2
+  ) {
+    exists(MethodCall check, int argIndex |
+      g1 = check and
+      v1.getDualValue().isThrowsException() and
+      conditionCheckArgument(check, argIndex, v2.asBooleanValue()) and
+      g2 = check.getArgument(argIndex)
+    )
+  }
 }
 
 private module LogicInput_v1 implements GuardsImpl::LogicInputSig {
@@ -364,18 +426,13 @@ private module LogicInput_v1 implements GuardsImpl::LogicInputSig {
     }
   }
 
+  predicate parameterDefinition(Parameter p, SsaDefinition def) {
+    def.(BaseSsaImplicitInit).isParameterDefinition(p)
+  }
+
   predicate additionalNullCheck = LogicInputCommon::additionalNullCheck/4;
 
-  predicate additionalImpliesStep(
-    GuardsImpl::PreGuard g1, GuardValue v1, GuardsImpl::PreGuard g2, GuardValue v2
-  ) {
-    exists(MethodCall check, int argIndex |
-      g1 = check and
-      v1.getDualValue().isThrowsException() and
-      conditionCheckArgument(check, argIndex, v2.asBooleanValue()) and
-      g2 = check.getArgument(argIndex)
-    )
-  }
+  predicate additionalImpliesStep = LogicInputCommon::additionalImpliesStep/4;
 }
 
 private module LogicInput_v2 implements GuardsImpl::LogicInputSig {
@@ -400,15 +457,13 @@ private module LogicInput_v2 implements GuardsImpl::LogicInputSig {
     }
   }
 
+  predicate parameterDefinition(Parameter p, SsaDefinition def) {
+    def.(SSA::SsaImplicitInit).isParameterDefinition(p)
+  }
+
   predicate additionalNullCheck = LogicInputCommon::additionalNullCheck/4;
 
-  predicate additionalImpliesStep(
-    GuardsImpl::PreGuard g1, GuardValue v1, GuardsImpl::PreGuard g2, GuardValue v2
-  ) {
-    LogicInput_v1::additionalImpliesStep(g1, v1, g2, v2)
-    or
-    CustomGuard::additionalImpliesStep(g1, v1, g2, v2)
-  }
+  predicate additionalImpliesStep = LogicInputCommon::additionalImpliesStep/4;
 }
 
 private module LogicInput_v3 implements GuardsImpl::LogicInputSig {
@@ -421,70 +476,11 @@ private module LogicInput_v3 implements GuardsImpl::LogicInputSig {
 
   predicate additionalNullCheck = LogicInputCommon::additionalNullCheck/4;
 
-  predicate additionalImpliesStep = LogicInput_v2::additionalImpliesStep/4;
-}
-
-private module CustomGuardInput implements Guards_v2::CustomGuardInputSig {
-  private import semmle.code.java.dataflow.SSA
-
-  private int parameterPosition() { result in [-1, any(Parameter p).getPosition()] }
-
-  /** A parameter position represented by an integer. */
-  class ParameterPosition extends int {
-    ParameterPosition() { this = parameterPosition() }
-  }
-
-  /** An argument position represented by an integer. */
-  class ArgumentPosition extends int {
-    ArgumentPosition() { this = parameterPosition() }
-  }
-
-  /** Holds if arguments at position `apos` match parameters at position `ppos`. */
-  overlay[caller?]
-  pragma[inline]
-  predicate parameterMatch(ParameterPosition ppos, ArgumentPosition apos) { ppos = apos }
-
-  final private class FinalMethod = Method;
-
-  class BooleanMethod extends FinalMethod {
-    BooleanMethod() {
-      super.getReturnType().(PrimitiveType).hasName("boolean") and
-      not super.isOverridable()
-    }
-
-    LogicInput_v2::SsaDefinition getParameter(ParameterPosition ppos) {
-      exists(Parameter p |
-        super.getParameter(ppos) = p and
-        not p.isVarargs() and
-        result.(SsaImplicitInit).isParameterDefinition(p)
-      )
-    }
-
-    GuardsInput::Expr getAReturnExpr() {
-      exists(ReturnStmt ret |
-        this = ret.getEnclosingCallable() and
-        ret.getResult() = result
-      )
-    }
-  }
-
-  private predicate booleanMethodCall(MethodCall call, BooleanMethod m) {
-    call.getMethod().getSourceDeclaration() = m
-  }
-
-  class BooleanMethodCall extends GuardsInput::Expr instanceof MethodCall {
-    BooleanMethodCall() { booleanMethodCall(this, _) }
-
-    BooleanMethod getMethod() { booleanMethodCall(this, result) }
-
-    GuardsInput::Expr getArgument(ArgumentPosition apos) { result = super.getArgument(apos) }
-  }
+  predicate additionalImpliesStep = LogicInputCommon::additionalImpliesStep/4;
 }
 
 class GuardValue = GuardsImpl::GuardValue;
 
-private module CustomGuard = Guards_v2::CustomGuard<CustomGuardInput>;
-
 /** INTERNAL: Don't use. */
 module Guards_v1 = GuardsImpl::Logic<LogicInput_v1>;
 

java/ql/test/library-tests/guards/Guards.java

@@ -143,4 +143,73 @@ void t7(int[] a) {
       chk(); // $ guarded=found:true guarded='i < a.length:false'
     }
   }
+
+  public static boolean testNotNull1(String input) {
+    return input != null && input.length() > 0;
+  }
+
+  public static boolean testNotNull2(String input) {
+    if (input == null) return false;
+    return input.length() > 0;
+  }
+
+  public static int getNumOrDefault(Integer number) {
+    return number == null ? 0 : number;
+  }
+
+  public static String concatNonNull(String s1, String s2) {
+    if (s1 == null || s2 == null) return null;
+    return s1 + s2;
+  }
+
+  public static Status testEnumWrapper(boolean flag) {
+    return flag ? Status.SUCCESS : Status.FAILURE;
+  }
+
+  enum Status { SUCCESS, FAILURE }
+
+  void testWrappers(String s, Integer i) {
+    if (testNotNull1(s)) {
+      chk(); // $ guarded='s:not null' guarded=testNotNull1(...):true
+    } else {
+      chk(); // $ guarded=testNotNull1(...):false
+    }
+
+    if (testNotNull2(s)) {
+      chk(); // $ guarded='s:not null' guarded=testNotNull2(...):true
+    } else {
+      chk(); // $ guarded=testNotNull2(...):false
+    }
+
+    if (0 == getNumOrDefault(i)) {
+      chk(); // $ guarded='0 == getNumOrDefault(...):true' guarded='getNumOrDefault(...):0'
+    } else {
+      chk(); // $ guarded='0 == getNumOrDefault(...):false' guarded='getNumOrDefault(...):not 0' guarded='i:not 0' guarded='i:not null'
+    }
+
+    if (null == concatNonNull(s, "suffix")) {
+      chk(); // $ guarded='concatNonNull(...):null' guarded='null == concatNonNull(...):true'
+    } else {
+      chk(); // $ guarded='concatNonNull(...):not null' guarded='null == concatNonNull(...):false' guarded='s:not null'
+    }
+
+    switch (testEnumWrapper(g(1))) {
+      case SUCCESS:
+        chk(); // $ guarded='testEnumWrapper(...):SUCCESS' guarded='testEnumWrapper(...):match SUCCESS' guarded=g(1):true
+        break;
+      case FAILURE:
+        chk(); // $ guarded='testEnumWrapper(...):FAILURE' guarded='testEnumWrapper(...):match FAILURE' guarded=g(1):false
+        break;
+    }
+  }
+
+  static void ensureNotNull(Object o) throws Exception {
+    if (o == null) throw new Exception();
+  }
+
+  void testExceptionWrapper(String s) throws Exception {
+    chk(); // nothing guards here
+    ensureNotNull(s);
+    chk(); // $ guarded='ensureNotNull(...):no exception' guarded='s:not null'
+  }
 }

java/ql/test/library-tests/guards/GuardsInline.expected

@@ -89,3 +89,28 @@
 | Guards.java:139:9:139:13 | chk(...) | found:true |
 | Guards.java:143:7:143:11 | chk(...) | 'i < a.length:false' |
 | Guards.java:143:7:143:11 | chk(...) | found:true |
+| Guards.java:173:7:173:11 | chk(...) | 's:not null' |
+| Guards.java:173:7:173:11 | chk(...) | testNotNull1(...):true |
+| Guards.java:175:7:175:11 | chk(...) | testNotNull1(...):false |
+| Guards.java:179:7:179:11 | chk(...) | 's:not null' |
+| Guards.java:179:7:179:11 | chk(...) | testNotNull2(...):true |
+| Guards.java:181:7:181:11 | chk(...) | testNotNull2(...):false |
+| Guards.java:185:7:185:11 | chk(...) | '0 == getNumOrDefault(...):true' |
+| Guards.java:185:7:185:11 | chk(...) | 'getNumOrDefault(...):0' |
+| Guards.java:187:7:187:11 | chk(...) | '0 == getNumOrDefault(...):false' |
+| Guards.java:187:7:187:11 | chk(...) | 'getNumOrDefault(...):not 0' |
+| Guards.java:187:7:187:11 | chk(...) | 'i:not 0' |
+| Guards.java:187:7:187:11 | chk(...) | 'i:not null' |
+| Guards.java:191:7:191:11 | chk(...) | 'concatNonNull(...):null' |
+| Guards.java:191:7:191:11 | chk(...) | 'null == concatNonNull(...):true' |
+| Guards.java:193:7:193:11 | chk(...) | 'concatNonNull(...):not null' |
+| Guards.java:193:7:193:11 | chk(...) | 'null == concatNonNull(...):false' |
+| Guards.java:193:7:193:11 | chk(...) | 's:not null' |
+| Guards.java:198:9:198:13 | chk(...) | 'testEnumWrapper(...):SUCCESS' |
+| Guards.java:198:9:198:13 | chk(...) | 'testEnumWrapper(...):match SUCCESS' |
+| Guards.java:198:9:198:13 | chk(...) | g(1):true |
+| Guards.java:201:9:201:13 | chk(...) | 'testEnumWrapper(...):FAILURE' |
+| Guards.java:201:9:201:13 | chk(...) | 'testEnumWrapper(...):match FAILURE' |
+| Guards.java:201:9:201:13 | chk(...) | g(1):false |
+| Guards.java:213:5:213:9 | chk(...) | 'ensureNotNull(...):no exception' |
+| Guards.java:213:5:213:9 | chk(...) | 's:not null' |

java/ql/test/query-tests/Nullness/C.java

@@ -204,7 +204,7 @@ public void ex14(int[] a) {
         obj = new Object();
       } else if (a[i] == 2) {
         verifyNotNull(obj);
-        obj.hashCode(); // NPE - false positive
+        obj.hashCode(); // OK
       }
     }
   }

java/ql/test/query-tests/Nullness/NullMaybe.expected

@@ -29,7 +29,6 @@
 | C.java:137:7:137:10 | obj2 | Variable $@ may be null at this access as suggested by $@ null guard. | C.java:131:5:131:23 | Object obj2 | obj2 | C.java:132:9:132:20 | ... != ... | this |
 | C.java:144:15:144:15 | a | Variable $@ may be null at this access as suggested by $@ null guard. | C.java:141:20:141:26 | a | a | C.java:142:13:142:21 | ... == ... | this |
 | C.java:188:9:188:11 | obj | Variable $@ may be null at this access because of $@ assignment. | C.java:181:5:181:22 | Object obj | obj | C.java:181:12:181:21 | obj | this |
-| C.java:207:9:207:11 | obj | Variable $@ may be null at this access because of $@ assignment. | C.java:201:5:201:22 | Object obj | obj | C.java:201:12:201:21 | obj | this |
 | C.java:219:9:219:10 | o1 | Variable $@ may be null at this access as suggested by $@ null guard. | C.java:212:20:212:28 | o1 | o1 | C.java:213:9:213:18 | ... == ... | this |
 | C.java:233:7:233:8 | xs | Variable $@ may be null at this access because of $@ assignment. | C.java:231:5:231:56 | int[] xs | xs | C.java:231:11:231:55 | xs | this |
 | F.java:11:5:11:7 | obj | Variable $@ may be null at this access as suggested by $@ null guard. | F.java:8:18:8:27 | obj | obj | F.java:9:9:9:19 | ... == ... | this |