Move TypeTestGuard's logic into Guard.appliesTypeTest

smowton · smowton · commit 77b1721542c5 · 2023-11-30T11:24:06.000Z
diff --git a/java/ql/lib/semmle/code/java/controlflow/Guards.qll b/java/ql/lib/semmle/code/java/controlflow/Guards.qll
@@ -164,6 +164,39 @@ class Guard extends ExprParent {
     )
   }
 
+  /**
+   * Holds if this guard tests whether `testedExpr` has type `testedType`.
+   *
+   * `restricted` is true if the test applies additional restrictions on top of just `testedType`, and so
+   * this guard failing does not guarantee `testedExpr` is *not* a `testedType`-- for example,
+   * matching `record R(Object o)` with `case R(String s)` is a guard with an additional restriction on the
+   * type of field `o`, so the guard passing guarantees `testedExpr` is an `R`, but it failing does not
+   * guarantee `testedExpr` is not an `R`.
+   */
+  predicate appliesTypeTest(Expr testedExpr, Type testedType, boolean restricted) {
+    (
+      exists(InstanceOfExpr ioe | this = ioe |
+        testedExpr = ioe.getExpr() and
+        testedType = ioe.getSyntacticCheckedType()
+      )
+      or
+      exists(PatternCase pc | this = pc |
+        pc.getSelectorExpr() = testedExpr and
+        testedType = pc.getPattern().getType()
+      )
+    ) and
+    (
+      if
+        exists(RecordPatternExpr rpe |
+          rpe = [this.(InstanceOfExpr).getPattern(), this.(PatternCase).getPattern()]
+        |
+          not rpe.isUnrestricted()
+        )
+      then restricted = true
+      else restricted = false
+    )
+  }
+
   /**
    * Holds if the evaluation of this guard to `branch` corresponds to the edge
    * from `bb1` to `bb2`.
@@ -223,60 +256,6 @@ class Guard extends ExprParent {
   }
 }
 
-/**
- * A `Guard` that tests an expression's type -- that is, an `instanceof T` or a
- * `case T varname` pattern case.
- */
-class TypeTestGuard extends Guard {
-  Expr testedExpr;
-  Type testedType;
-
-  TypeTestGuard() {
-    exists(InstanceOfExpr ioe | this = ioe |
-      testedExpr = ioe.getExpr() and
-      testedType = ioe.getSyntacticCheckedType()
-    )
-    or
-    exists(PatternCase pc | this = pc |
-      pc.getSelectorExpr() = testedExpr and
-      testedType = pc.getPattern().getType()
-    )
-  }
-
-  /**
-   * Gets the record pattern this type test binds to, if any.
-   */
-  PatternExpr getPattern() {
-    result = this.(InstanceOfExpr).getPattern()
-    or
-    result = this.(PatternCase).getPattern()
-  }
-
-  /**
-   * Holds if this guard tests whether `e` has type `t` on `testedBranch`.
-   *
-   * Note that record patterns that make at least one tighter restriction than the record's definition
-   * (e.g. matching `record R(Object)` with `case R(String)`) means this only guarantees the tested type
-   * on the true branch (i.e., entering such a case guarantees `testedExpr` is a `testedType`, but failing
-   * the type test could mean a nested record or binding pattern didn't match but `testedExpr` is still
-   * of type `testedType`.)
-   */
-  predicate appliesTypeTest(Expr e, Type t, boolean testedBranch) {
-    e = testedExpr and
-    t = testedType and
-    (
-      testedBranch = true
-      or
-      testedBranch = false and
-      (
-        this.getPattern().asRecordPattern().isUnrestricted()
-        or
-        not this.getPattern() instanceof RecordPatternExpr
-      )
-    )
-  }
-}
-
 private predicate switchCaseControls(SwitchCase sc, BasicBlock bb) {
   exists(BasicBlock caseblock |
     caseblock.getFirstNode() = sc.getControlFlowNode() and
diff --git a/java/ql/lib/semmle/code/java/dataflow/TypeFlow.qll b/java/ql/lib/semmle/code/java/dataflow/TypeFlow.qll
@@ -417,8 +417,8 @@ private predicate downcastSuccessor(VarAccess va, RefType t) {
  * Holds if `va` is an access to a value that is guarded by `instanceof t` or `case e t`.
  */
 private predicate typeTestGuarded(VarAccess va, RefType t) {
-  exists(TypeTestGuard typeTest, BaseSsaVariable v |
-    typeTest.appliesTypeTest(v.getAUse(), t, true) and
+  exists(Guard typeTest, BaseSsaVariable v |
+    typeTest.appliesTypeTest(v.getAUse(), t, _) and
     va = v.getAUse() and
     guardControls_v1(typeTest, va.getBasicBlock(), true)
   )
@@ -428,8 +428,8 @@ private predicate typeTestGuarded(VarAccess va, RefType t) {
  * Holds if `aa` is an access to a value that is guarded by `instanceof t` or `case e t`.
  */
 predicate arrayTypeTestGuarded(ArrayAccess aa, RefType t) {
-  exists(TypeTestGuard typeTest, BaseSsaVariable v1, BaseSsaVariable v2, ArrayAccess aa1 |
-    typeTest.appliesTypeTest(aa1, t, true) and
+  exists(Guard typeTest, BaseSsaVariable v1, BaseSsaVariable v2, ArrayAccess aa1 |
+    typeTest.appliesTypeTest(aa1, t, _) and
     aa1.getArray() = v1.getAUse() and
     aa1.getIndexExpr() = v2.getAUse() and
     aa.getArray() = v1.getAUse() and
diff --git a/java/ql/lib/semmle/code/java/dispatch/VirtualDispatch.qll b/java/ql/lib/semmle/code/java/dispatch/VirtualDispatch.qll
@@ -194,7 +194,7 @@ private module Dispatch {
    */
   private predicate impossibleDispatchTarget(MethodCall source, Method tgt) {
     tgt = viableImpl_v1_cand(source) and
-    exists(TypeTestGuard typeTest, BaseSsaVariable v, Expr q, RefType t |
+    exists(Guard typeTest, BaseSsaVariable v, Expr q, RefType t |
       source.getQualifier() = q and
       v.getAUse() = q and
       typeTest.appliesTypeTest(v.getAUse(), t, false) and
