Cfg: Rework CFG for switch case patterns.

Author: aschackmull

csharp/ql/lib/semmle/code/csharp/controlflow/internal/ControlFlowGraph.qll

@@ -229,7 +229,7 @@ module Ast implements AstSig<Location> {
   final private class FinalCase = CS::Case;
 
   class Case extends FinalCase {
-    AstNode getAPattern() { result = this.getPattern() }
+    AstNode getPattern(int index) { result = this.getPattern() and index = 0 }
 
     Expr getGuard() { result = this.getCondition() }
 

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

@@ -153,10 +153,10 @@ private module Ast implements AstSig<Location> {
   }
 
   class Case extends AstNode instanceof J::SwitchCase {
-    /** Gets a pattern being matched by this case. */
-    AstNode getAPattern() {
-      result = this.(PatternCase).getAPattern() or
-      result = this.(ConstCase).getValue(_)
+    /** Gets the pattern being matched by this case at the specified (zero-based) `index`. */
+    AstNode getPattern(int index) {
+      result = this.(PatternCase).getPattern(index) or
+      result = this.(ConstCase).getValue(index)
     }
 
     /** Gets the guard expression of this case, if any. */

shared/controlflow/codeql/controlflow/ControlFlowGraph.qll

@@ -255,8 +255,8 @@ signature module AstSig<LocationSig Location> {
 
   /** A case in a switch. */
   class Case extends AstNode {
-    /** Gets a pattern being matched by this case. */
-    AstNode getAPattern();
+    /** Gets the pattern being matched by this case at the specified (zero-based) `index`. */
+    AstNode getPattern(int index);
 
     /** Gets the guard expression of this case, if any. */
     Expr getGuard();
@@ -489,6 +489,18 @@ module Make0<LocationSig Location, AstSig<Location> Ast> {
      * need to be consecutive nor start from a specific index.
      */
     default Parameter callableGetParameter(Callable c, CallableContext ctx, int index) { none() }
+
+    /** Holds if this catch clause catches all exceptions. */
+    default predicate catchAll(CatchClause catch) { none() }
+
+    /**
+     * Holds if this case matches all possible values, for example, if it is a
+     * `default` case or a match-all pattern like `Object o` or if it is the
+     * final case in a switch that is known to be exhaustive.
+     *
+     * A match-all case can still ultimately fail to match if it has a guard.
+     */
+    default predicate matchAll(Case c) { c instanceof DefaultCase }
   }
 
   /**
@@ -612,6 +624,8 @@ module Make0<LocationSig Location, AstSig<Location> Ast> {
         or
         n instanceof Case
         or
+        n = any(Case case).getPattern(_)
+        or
         exists(n.(Parameter).getDefaultValue())
       )
     }
@@ -772,12 +786,25 @@ module Make0<LocationSig Location, AstSig<Location> Ast> {
       result = DenseRank2<ParameterCtxDenseRankInput>::denseRank(c, ctx.asSome(), rnk)
     }
 
+    private predicate constantCondition(AstNode n, ConditionalSuccessor t) {
+      n.(BooleanLiteral).getValue() = t.(BooleanSuccessor).getValue()
+      or
+      exists(Case c, int i |
+        Input1::matchAll(c) and
+        c.getPattern(i) = n and
+        not exists(c.getPattern(i + 1)) and
+        t.(MatchingSuccessor).getValue() = true
+      )
+    }
+
     cached
     private newtype TNode =
       TBeforeNode(AstNode n) { Input1::cfgCachedStageRef() and exists(getEnclosingCallable(n)) } or
       TAstNode(AstNode n) { postOrInOrder(n) and exists(getEnclosingCallable(n)) } or
       TAfterValueNode(AstNode n, ConditionalSuccessor t) {
-        inConditionalContext(n, t.getKind()) and exists(getEnclosingCallable(n))
+        inConditionalContext(n, t.getKind()) and
+        exists(getEnclosingCallable(n)) and
+        not constantCondition(n, t.getDual())
       } or
       TAfterNode(AstNode n) {
         exists(getEnclosingCallable(n)) and
@@ -1105,18 +1132,6 @@ module Make0<LocationSig Location, AstSig<Location> Ast> {
     }
 
     signature module InputSig2 {
-      /** Holds if this catch clause catches all exceptions. */
-      default predicate catchAll(CatchClause catch) { none() }
-
-      /**
-       * Holds if this case matches all possible values, for example, if it is a
-       * `default` case or a match-all pattern like `Object o` or if it is the
-       * final case in a switch that is known to be exhaustive.
-       *
-       * A match-all case can still ultimately fail to match if it has a guard.
-       */
-      default predicate matchAll(Case c) { c instanceof DefaultCase }
-
       /**
        * Holds if `ast` may result in an abrupt completion `c` originating at
        * `n`. The boolean `always`  indicates whether the abrupt completion
@@ -1471,12 +1486,6 @@ module Make0<LocationSig Location, AstSig<Location> Ast> {
           n2.isBefore(condexpr.getElse())
         )
         or
-        exists(BooleanLiteral boollit |
-          inConditionalContext(boollit, _) and
-          n1.isBefore(boollit) and
-          n2.isAfterValue(boollit, any(BooleanSuccessor t | t.getValue() = boollit.getValue()))
-        )
-        or
         exists(PatternMatchExpr pme |
           n1.isBefore(pme) and
           n2.isBefore(pme.getExpr())
@@ -1662,7 +1671,7 @@ module Make0<LocationSig Location, AstSig<Location> Ast> {
           exists(MatchingSuccessor t |
             n1.isBefore(catchclause) and
             n2.isAfterValue(catchclause, t) and
-            if Input2::catchAll(catchclause) then t.getValue() = true else any()
+            if Input1::catchAll(catchclause) then t.getValue() = true else any()
           )
           or
           exists(PreControlFlowNode beforeVar, PreControlFlowNode beforeCond |
@@ -1720,21 +1729,22 @@ module Make0<LocationSig Location, AstSig<Location> Ast> {
         )
         or
         exists(Case case |
-          exists(MatchingSuccessor t |
-            n1.isBefore(case) and
-            n2.isAfterValue(case, t) and
-            if Input2::matchAll(case) then t.getValue() = true else any()
+          n1.isBefore(case) and
+          (
+            if exists(case.getPattern(_))
+            then n2.isBefore(case.getPattern(0))
+            else n2.isAfterValue(case, any(MatchingSuccessor t | t.getValue() = true))
           )
           or
-          exists(
-            PreControlFlowNode beforePattern, PreControlFlowNode beforeGuard,
-            PreControlFlowNode beforeBody
-          |
-            (
-              beforePattern.isBefore(case.getAPattern())
-              or
-              not exists(case.getAPattern()) and beforePattern = beforeGuard
-            ) and
+          exists(int i, MatchingSuccessor ms | n1.isAfterValue(case.getPattern(i), ms) |
+            ms.getValue() = false and
+            n2.isBefore(case.getPattern(i + 1))
+            or
+            (ms.getValue() = true or not exists(case.getPattern(i + 1))) and
+            n2.isAfterValue(case, ms)
+          )
+          or
+          exists(PreControlFlowNode beforeGuard, PreControlFlowNode beforeBody |
             (
               beforeGuard.isBefore(case.getGuard())
               or
@@ -1748,9 +1758,6 @@ module Make0<LocationSig Location, AstSig<Location> Ast> {
             )
           |
             n1.isAfterValue(case, any(MatchingSuccessor t | t.getValue() = true)) and
-            n2 = beforePattern
-            or
-            n1.isAfter(case.getAPattern()) and
             n2 = beforeGuard
             or
             n1.isAfterTrue(case.getGuard()) and
@@ -2225,12 +2232,6 @@ module Make0<LocationSig Location, AstSig<Location> Ast> {
               t instanceof DirectSuccessor and
               node.isAdditional(any(ForeachStmt foreach), loopHeaderTag())
             ) and
-            // allow for disjunctive patterns (e.g. `case "foo", "bar":`)
-            not (
-              t instanceof DirectSuccessor and
-              node.isAfterValue(any(Case c | 2 <= strictcount(c.getAPattern())),
-                any(MatchingSuccessor m | m.getValue() = true))
-            ) and
             // allow for functions with multiple bodies
             not exists(Callable c |
               successor.getAstNode() = getBodyEntry(c, _) and