Implement multiple pattern case and fall-through pattern case support

Author: smowton

java/ql/lib/semmle/code/java/ControlFlowGraph.qll

@@ -489,14 +489,14 @@ private module ControlFlowGraphImpl {
   private Stmt getSwitchStatement(SwitchBlock switch, int i) { result.isNthChildOf(switch, i) }
 
   /**
-   * Holds if `last` is the last node in a pattern case `pc`'s succeeding bind-and-test operation,
+   * Holds if `last` is the last node in any of pattern case `pc`'s succeeding bind-and-test operations,
    * immediately before either falling through to execute successor statements or execute a rule body
    * if present. `completion` is the completion kind of the last operation.
    */
   private predicate lastPatternCaseMatchingOp(
     PatternCase pc, ControlFlowNode last, Completion completion
   ) {
-    last(pc.getPattern(), last, completion) and
+    last(pc.getAPattern(), last, completion) and
     completion = NormalCompletion() and
     not exists(pc.getGuard())
     or
@@ -776,6 +776,19 @@ private module ControlFlowGraphImpl {
     last(try.getFinally(), last, NormalCompletion())
   }
 
+  private predicate isNextNormalSwitchStmt(SwitchBlock switch, Stmt pred, Stmt succ) {
+    exists(int i, Stmt immediateSucc |
+      getSwitchStatement(switch, i) = pred and
+      getSwitchStatement(switch, i + 1) = immediateSucc and
+      (
+        succ = immediateSucc and
+        not immediateSucc instanceof PatternCase
+        or
+        isNextNormalSwitchStmt(switch, immediateSucc, succ)
+      )
+    )
+  }
+
   /**
    * Bind `last` to a cfg node nested inside `n` (or, indeed, `n` itself) such
    * that `last` may be the last node during an execution of `n` and finish
@@ -927,9 +940,15 @@ private module ControlFlowGraphImpl {
       completion != anonymousBreakCompletion() and
       not completion instanceof NormalOrBooleanCompletion
       or
-      // if the last case completes normally, then so does the switch
-      last(switch.getStmt(strictcount(switch.getAStmt()) - 1), last, NormalCompletion()) and
-      completion = NormalCompletion()
+      // if a statement without a non-pattern-case successor completes normally (or for a pattern case
+      // the guard succeeds) then the switch completes normally.
+      exists(Stmt lastNormalStmt, Completion stmtCompletion |
+        lastNormalStmt = getSwitchStatement(switch, _) and
+        not isNextNormalSwitchStmt(switch, lastNormalStmt, _) and
+        last(lastNormalStmt, last, stmtCompletion) and
+        (stmtCompletion = NormalCompletion() or stmtCompletion = BooleanCompletion(true, _)) and
+        completion = NormalCompletion()
+      )
       or
       // if no default case exists, then normal completion of the expression may terminate the switch
       // Note this can't happen if there are pattern cases or a null literal, as
@@ -973,9 +992,9 @@ private module ControlFlowGraphImpl {
     )
     or
     // A pattern case statement can complete:
-    // * On failure of its type test (boolean false)
+    // * On failure of its final type test (boolean false)
     // * On failure of its guard test if any (boolean false)
-    // * On completion of its variable declarations, if it is not a rule and has no guard (normal completion)
+    // * On completion of one of its pattern variable declarations, if it is not a rule and has no guard (normal completion)
     // * On success of its guard test, if it is not a rule (boolean true)
     // (the latter two cases are accounted for by lastPatternCaseMatchingOp)
     exists(PatternCase pc | n = pc |
@@ -1315,9 +1334,13 @@ private module ControlFlowGraphImpl {
       // Note this includes non-rule case statements and the successful pattern match successor
       // of a non-rule pattern case statement. Rule case statements do not complete normally
       // (they always break or yield).
-      exists(int i |
-        last(getSwitchStatement(switch, i), n, completion) and
-        result = first(getSwitchStatement(switch, i + 1)) and
+      // Exception: falling through into a pattern case statement (which necessarily does not
+      // declare any named variables) must skip one or more such statements, otherwise we would
+      // incorrectly apply their type test and/or guard.
+      exists(Stmt pred, Stmt succ |
+        isNextNormalSwitchStmt(switch, pred, succ) and
+        last(pred, n, completion) and
+        result = first(succ) and
         (completion = NormalCompletion() or completion = BooleanCompletion(true, _))
       )
       or
@@ -1328,16 +1351,19 @@ private module ControlFlowGraphImpl {
     )
     or
     // Pattern cases have internal edges:
-    // * Type test success -true-> variable declarations
+    // * Type test success -true-> one of the possible sets of variable declarations
+    //     n.b. for unnamed patterns (e.g. case A _, B _) this means that *one* of the
+    //          type tests has succeeded. There aren't enough nodes in the AST to describe
+    //          a sequential test in detail, so CFG consumers have to watch out for this case.
     // * Variable declarations -normal-> guard evaluation
     // * Variable declarations -normal-> rule execution (when there is no guard)
     // * Guard success -true-> rule execution
     exists(PatternCase pc |
       n = pc and
       completion = basicBooleanCompletion(true) and
-      result = first(pc.getPattern())
+      result = first(pc.getAPattern())
       or
-      last(pc.getPattern(), n, completion) and
+      last(pc.getAPattern(), n, completion) and
       completion = NormalCompletion() and
       result = first(pc.getGuard())
       or

java/ql/lib/semmle/code/java/Dependency.qll

@@ -84,7 +84,7 @@ predicate depends(RefType t, RefType dep) {
     or
     // A type accessed in a pattern-switch case statement in `t`.
     exists(PatternCase pc | t = pc.getEnclosingCallable().getDeclaringType() |
-      usesType(pc.getPattern().getAChildExpr*().getType(), dep)
+      usesType(pc.getAPattern().getAChildExpr*().getType(), dep)
     )
   )
 }

java/ql/lib/semmle/code/java/DependencyCounts.qll

@@ -107,7 +107,7 @@ predicate numDepends(RefType t, RefType dep, int value) {
       or
       // the type accessed in a pattern-switch case statement in `t`.
       exists(PatternCase pc | elem = pc and t = pc.getEnclosingCallable().getDeclaringType() |
-        usesType(pc.getPattern().getAChildExpr*().getType(), dep)
+        usesType(pc.getAPattern().getAChildExpr*().getType(), dep)
       )
     )
 }

java/ql/lib/semmle/code/java/Expr.qll

@@ -1590,7 +1590,9 @@ class InstanceOfExpr extends Expr, @instanceofexpr {
    * Note that this won't get anything when record pattern matching is used-- for more general patterns,
    * use `getPattern`.
    */
-  LocalVariableDeclExpr getLocalVariableDeclExpr() { result = this.getPattern().asBindingPattern() }
+  LocalVariableDeclExpr getLocalVariableDeclExpr() {
+    result = this.getPattern().asBindingOrUnnamedPattern()
+  }
 
   /**
    * Gets the access to the type on the right-hand side of the `instanceof` operator.
@@ -1681,7 +1683,10 @@ class LocalVariableDeclExpr extends Expr, @localvariabledeclexpr {
     or
     exists(InstanceOfExpr ioe | this.getParent() = ioe | result.isNthChildOf(ioe, 1))
     or
-    exists(PatternCase pc | this.getParent() = pc | result.isNthChildOf(pc, -2))
+    exists(PatternCase pc, int index, int typeAccessIdx | this.isNthChildOf(pc, index) |
+      (if index = 0 then typeAccessIdx = -4 else typeAccessIdx = (-3 - index)) and
+      result.isNthChildOf(pc, typeAccessIdx)
+    )
     or
     exists(RecordPatternExpr rpe, int index |
       this.isNthChildOf(rpe, index) and result.isNthChildOf(rpe, -(index + 1))
@@ -1742,17 +1747,17 @@ class LocalVariableDeclExpr extends Expr, @localvariabledeclexpr {
     or
     exists(SwitchStmt switch |
       result = switch.getExpr() and
-      this = switch.getAPatternCase().getPattern().asBindingPattern()
+      this = switch.getAPatternCase().getAPattern().asBindingOrUnnamedPattern()
     )
     or
     exists(SwitchExpr switch |
       result = switch.getExpr() and
-      this = switch.getAPatternCase().getPattern().asBindingPattern()
+      this = switch.getAPatternCase().getAPattern().asBindingOrUnnamedPattern()
     )
     or
     exists(InstanceOfExpr ioe |
       result = ioe.getExpr() and
-      this = ioe.getPattern().asBindingPattern()
+      this = ioe.getPattern().asBindingOrUnnamedPattern()
     )
   }
 
@@ -2676,9 +2681,9 @@ class NotNullExpr extends UnaryExpr, @notnullexpr {
 }
 
 /**
- * A binding or record pattern.
+ * A binding, unnamed or record pattern.
  *
- * Note binding patterns are represented as `LocalVariableDeclExpr`s.
+ * Note binding and unnamed patterns are represented as `LocalVariableDeclExpr`s.
  */
 class PatternExpr extends Expr {
   PatternExpr() {
@@ -2691,9 +2696,14 @@ class PatternExpr extends Expr {
   }
 
   /**
-   * Gets this pattern cast to a binding pattern.
+   * Gets this pattern cast to a binding or unnamed pattern.
+   */
+  LocalVariableDeclExpr asBindingOrUnnamedPattern() { result = this }
+
+  /**
+   * DEPRECATED: alias for `asBindingOrUnnamedPattern`.
    */
-  LocalVariableDeclExpr asBindingPattern() { result = this }
+  deprecated LocalVariableDeclExpr asBindingPattern() { result = this.asBindingOrUnnamedPattern() }
 
   /**
    * Gets this pattern cast to a record pattern.

java/ql/lib/semmle/code/java/PrettyPrintAst.qll

@@ -386,7 +386,7 @@ private class PpInstanceOfExpr extends PpAst, InstanceOfExpr {
     i = 3 and result = " " and this.getPattern() instanceof LocalVariableDeclExpr
     or
     i = 4 and
-    result = this.getPattern().asBindingPattern().getName()
+    result = this.getPattern().asBindingOrUnnamedPattern().getName()
   }
 
   override PpAst getChild(int i) {
@@ -782,28 +782,53 @@ private class PpSwitchCase extends PpAst, SwitchCase {
 }
 
 private class PpPatternCase extends PpAst, PatternCase {
+  private TypeAccess getPatternTypeAccess(int n) {
+    result = this.getPatternAtIndex(n).asBindingOrUnnamedPattern().getTypeAccess()
+  }
+
+  private predicate isAnonymousPattern(int n) {
+    this.getPatternAtIndex(n).asBindingOrUnnamedPattern().isAnonymous()
+  }
+
   override string getPart(int i) {
-    i = 0 and result = "case "
-    or
-    i = 2 and this.getPattern() instanceof LocalVariableDeclExpr and result = " "
-    or
-    i = 3 and result = this.getPattern().asBindingPattern().getName()
-    or
-    i = 4 and result = ":" and not this.isRule()
-    or
-    i = 4 and result = " -> " and this.isRule()
+    exists(int n, int base | exists(this.getPatternAtIndex(n)) and base = n * 4 |
+      i = base and
+      (if n = 0 then result = "case " else result = ", ")
+      or
+      i = base + 2 and
+      this.getPatternAtIndex(n) instanceof LocalVariableDeclExpr and
+      not this.isAnonymousPattern(n) and
+      result = " "
+      or
+      i = base + 3 and
+      (
+        if this.isAnonymousPattern(n)
+        then result = "_"
+        else result = this.getPatternAtIndex(n).asBindingOrUnnamedPattern().getName()
+      )
+    )
     or
-    i = 6 and result = ";" and exists(this.getRuleExpression())
+    exists(int base | base = (max(int n | exists(this.getPatternAtIndex(n))) + 1) * 4 |
+      i = base and result = ":" and not this.isRule()
+      or
+      i = base and result = " -> " and this.isRule()
+      or
+      i = base + 2 and result = ";" and exists(this.getRuleExpression())
+    )
   }
 
   override PpAst getChild(int i) {
-    i = 1 and result = this.getPattern().asBindingPattern().getTypeAccess()
-    or
-    i = 1 and result = this.getPattern().asRecordPattern()
-    or
-    i = 5 and result = this.getRuleExpression()
+    exists(int n, int base | exists(this.getPatternAtIndex(n)) and base = n * 4 |
+      i = 1 and result = this.getPatternAtIndex(n).asBindingOrUnnamedPattern().getTypeAccess()
+      or
+      i = 1 and result = this.getPatternAtIndex(n).asRecordPattern()
+    )
     or
-    i = 5 and result = this.getRuleStatement()
+    exists(int base | base = (max(int n | exists(this.getPatternAtIndex(n))) + 1) * 4 |
+      i = base + 1 and result = this.getRuleExpression()
+      or
+      i = base + 1 and result = this.getRuleStatement()
+    )
   }
 }
 

java/ql/lib/semmle/code/java/PrintAst.qll

@@ -117,7 +117,11 @@ private newtype TPrintAstNode =
   TElementNode(Element el) { shouldPrint(el, _) } or
   TForInitNode(ForStmt fs) { shouldPrint(fs, _) and exists(fs.getAnInit()) } or
   TLocalVarDeclNode(LocalVariableDeclExpr lvde) {
-    shouldPrint(lvde, _) and lvde.getParent() instanceof SingleLocalVarDeclParent
+    shouldPrint(lvde, _) and
+    (
+      lvde.getParent() instanceof SingleLocalVarDeclParent or
+      lvde.getParent() instanceof PatternCase
+    )
   } or
   TAnnotationsNode(Annotatable ann) {
     shouldPrint(ann, _) and
@@ -415,6 +419,23 @@ final class ForStmtNode extends ExprStmtNode {
   }
 }
 
+/**
+ * A node representing a `PatternCase`.
+ */
+final class PatternCaseNode extends ExprStmtNode {
+  PatternCase pc;
+
+  PatternCaseNode() { pc = element }
+
+  override PrintAstNode getChild(int childIndex) {
+    result = super.getChild(childIndex) and
+    not result.(ElementNode).getElement() instanceof LocalVariableDeclExpr and
+    not result.(ElementNode).getElement() instanceof TypeAccess
+    or
+    result = TLocalVarDeclNode(pc.getPatternAtIndex(childIndex))
+  }
+}
+
 /**
  * An element that can be the parent of up to one `LocalVariableDeclExpr` for which we want
  * to use a synthetic node to hold the variable declaration and its `TypeAccess`.
@@ -423,8 +444,7 @@ private class SingleLocalVarDeclParent extends ExprOrStmt {
   SingleLocalVarDeclParent() {
     this instanceof EnhancedForStmt or
     this instanceof CatchClause or
-    this.(InstanceOfExpr).isPattern() or
-    this instanceof PatternCase
+    this.(InstanceOfExpr).isPattern()
   }
 
   /** Gets the variable declaration that this element contains */
@@ -643,7 +663,11 @@ final class LocalVarDeclSynthNode extends PrintAstNode, TLocalVarDeclNode {
 
   LocalVarDeclSynthNode() { this = TLocalVarDeclNode(lvde) }
 
-  override string toString() { result = "(Single Local Variable Declaration)" }
+  override string toString() {
+    if lvde.getParent() instanceof PatternCase
+    then result = "(Pattern case declaration)"
+    else result = "(Single Local Variable Declaration)"
+  }
 
   override ElementNode getChild(int childIndex) {
     childIndex = 0 and

java/ql/lib/semmle/code/java/Statement.qll

@@ -539,12 +539,19 @@ class ConstCase extends SwitchCase {
 
 /** A pattern case of a `switch` statement */
 class PatternCase extends SwitchCase {
-  PatternExpr pattern;
+  PatternCase() { exists(PatternExpr pe | pe.isNthChildOf(this, _)) }
 
-  PatternCase() { pattern.isNthChildOf(this, 0) }
+  /**
+   * DEPRECATED: alias for getPatternAtIndex(0)
+   */
+  deprecated PatternExpr getPattern() { result = this.getPatternAtIndex(0) }
+
+  /**
+   * Gets this case's `n`th pattern.
+   */
+  PatternExpr getPatternAtIndex(int n) { result.isNthChildOf(this, n) }
 
-  /** Gets this case's pattern. */
-  PatternExpr getPattern() { result = pattern }
+  PatternExpr getAPattern() { result = this.getPatternAtIndex(_) }
 
   /** Gets the guard applicable to this pattern case, if any. */
   Expr getGuard() { result.isNthChildOf(this, -3) }

java/ql/lib/semmle/code/java/Variable.qll

@@ -58,7 +58,13 @@ class LocalVariableDecl extends @localvar, LocalScopeVariable {
   /** Gets the callable in which this declaration occurs. */
   Callable getEnclosingCallable() { result = this.getCallable() }
 
-  override string toString() { result = this.getType().getName() + " " + this.getName() }
+  override string toString() {
+    exists(string sourceName |
+      if this.getName() = "" then sourceName = "_" else sourceName = this.getName()
+    |
+      result = this.getType().getName() + " " + sourceName
+    )
+  }
 
   /** Gets the initializer expression of this local variable declaration. */
   override Expr getInitializer() { result = this.getDeclExpr().getInit() }

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

@@ -115,8 +115,20 @@ private predicate isNonFallThroughPredecessor(SwitchCase sc, ControlFlowNode pre
   (
     pred.(Expr).getParent*() = sc.getSelectorExpr()
     or
-    pred.(Expr).getParent*() = getClosestPrecedingPatternCase(sc).getGuard()
+    // Ambiguous: in the case of `case String _ when x: case "SomeConstant":`, the guard `x`
+    // passing edge will fall through into the constant case, and the guard failing edge
+    // will test if the selector equals `"SomeConstant"` and if so branch to the same
+    // case statement. Therefore don't label this a non-fall-through predecessor.
+    exists(PatternCase previousPatternCase |
+      previousPatternCase = getClosestPrecedingPatternCase(sc)
+    |
+      pred.(Expr).getParent*() = previousPatternCase.getGuard() and
+      // Check there is any statement in between the previous pattern case and this one.
+      not previousPatternCase.getIndex() = sc.getIndex() - 1
+    )
     or
+    // Unambigious: on the test-passing edge there must be at least one intervening
+    // declaration node, including anonymous `_` declarations.
     pred = getClosestPrecedingPatternCase(sc)
   )
 }