Merge pull request #21441 from aschackmull/cfg/switch-sharing

Cfg: Share more code for switch statements.

Author: aschackmull

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

@@ -132,27 +132,11 @@ private module Ast implements AstSig<Location> {
       result = this.(SwitchStmt).getCase(index) or
       result = this.(SwitchExpr).getCase(index)
     }
-  }
-
-  int getCaseControlFlowOrder(Switch s, Case c) {
-    exists(int pos | s.getCase(pos) = c |
-      // if a default case is not last in the AST, move it last in the CFG order
-      if c instanceof DefaultCase and exists(s.getCase(pos + 1))
-      then result = strictcount(s.getCase(_))
-      else result = pos
-    )
-  }
-
-  private Stmt getSwitchStmt(Switch s, int i) {
-    result = s.(SwitchStmt).getStmt(i) or
-    result = s.(SwitchExpr).getStmt(i)
-  }
 
-  private int numberOfStmts(Switch s) { result = strictcount(getSwitchStmt(s, _)) }
-
-  private predicate caseIndex(Switch s, Case c, int caseIdx, int caseStmtPos) {
-    c = s.getCase(caseIdx) and
-    c = getSwitchStmt(s, caseStmtPos)
+    Stmt getStmt(int index) {
+      result = this.(SwitchStmt).getStmt(index) or
+      result = this.(SwitchExpr).getStmt(index)
+    }
   }
 
   class Case extends AstNode instanceof J::SwitchCase {
@@ -166,32 +150,20 @@ private module Ast implements AstSig<Location> {
     Expr getGuard() { result = this.(PatternCase).getGuard() }
 
     /**
-     * Gets the body element of this case at the specified (zero-based) `index`.
+     * Gets the body of this case, if any.
      *
-     * This is either unique when the case has a single right-hand side, or it
-     * is the sequence of statements between this case and the next case.
+     * A case can either have a body as a single child AST node given by this
+     * predicate, or it can have an implicit body given by the sequence of
+     * statements between this case and the next case.
      */
-    AstNode getBodyElement(int index) {
-      result = this.(J::SwitchCase).getRuleExpression() and index = 0
-      or
-      result = this.(J::SwitchCase).getRuleStatement() and index = 0
+    AstNode getBody() {
+      result = this.(J::SwitchCase).getRuleExpression()
       or
-      not this.(J::SwitchCase).isRule() and
-      exists(Switch s, int caseIdx, int caseStmtPos, int nextCaseStmtPos |
-        caseIndex(pragma[only_bind_into](s), this, caseIdx, caseStmtPos) and
-        (
-          caseIndex(pragma[only_bind_into](s), _, caseIdx + 1, nextCaseStmtPos)
-          or
-          not exists(s.getCase(caseIdx + 1)) and
-          nextCaseStmtPos = numberOfStmts(s)
-        ) and
-        index = [0 .. nextCaseStmtPos - caseStmtPos - 2] and
-        result = getSwitchStmt(pragma[only_bind_into](s), caseStmtPos + 1 + index)
-      )
+      result = this.(J::SwitchCase).getRuleStatement()
     }
   }
 
-  predicate fallsThrough(Case c) { not c.(J::SwitchCase).isRule() }
+  class DefaultCase extends Case instanceof J::DefaultCase { }
 
   class ConditionalExpr = J::ConditionalExpr;
 

shared/controlflow/codeql/controlflow/ControlFlowGraph.qll

@@ -224,17 +224,18 @@ signature module AstSig<LocationSig Location> {
 
     /** Gets the case at the specified (zero-based) `index`. */
     Case getCase(int index);
-  }
 
-  /**
-   * Gets an integer indicating the control flow order of a case within a switch.
-   * This is most often the same as the AST order, but can be different in some
-   * languages if the language allows a default case to appear before other
-   * cases.
-   *
-   * The values do not need to be contiguous; only the relative ordering matters.
-   */
-  default int getCaseControlFlowOrder(Switch s, Case c) { s.getCase(result) = c }
+    /**
+     * Gets the statement at the specified (zero-based) `index`, if any.
+     *
+     * Depending on the language, switches may have their case bodies nested
+     * under the case nodes, which may or may not be statements themselves, or
+     * the switches may have a flat structure where the cases are just labels
+     * and the case bodies are sequences of statements between case statements.
+     * This predicate accommodates the latter.
+     */
+    Stmt getStmt(int index);
+  }
 
   /** A case in a switch. */
   class Case extends AstNode {
@@ -245,19 +246,22 @@ signature module AstSig<LocationSig Location> {
     Expr getGuard();
 
     /**
-     * Gets the body element of this case at the specified (zero-based) `index`.
+     * Gets the body of this case, if any.
      *
-     * This is either unique when the case has a single right-hand side, or it
-     * is the sequence of statements between this case and the next case.
+     * A case can either have a body as a single child AST node given by this
+     * predicate, or it can have an implicit body given by the sequence of
+     * statements between this case and the next case.
      */
-    AstNode getBodyElement(int index);
+    AstNode getBody();
   }
 
+  class DefaultCase extends Case;
+
   /**
    * Holds if this case can fall through to the next case if it is not
    * otherwise prevented with a `break` or similar.
    */
-  default predicate fallsThrough(Case c) { none() }
+  default predicate fallsThrough(Case c) { not exists(c.getBody()) }
 
   /** A ternary conditional expression. */
   class ConditionalExpr extends Expr {
@@ -938,7 +942,7 @@ module Make0<LocationSig Location, AstSig<Location> Ast> {
        *
        * A match-all case can still ultimately fail to match if it has a guard.
        */
-      default predicate matchAll(Case c) { none() }
+      default predicate matchAll(Case c) { c instanceof DefaultCase }
 
       /**
        * Holds if `ast` may result in an abrupt completion `c` originating at
@@ -1083,7 +1087,7 @@ module Make0<LocationSig Location, AstSig<Location> Ast> {
         )
         or
         exists(Switch switch |
-          ast = switch.getCase(_).getBodyElement(_) and
+          ast = getCaseBodyElement(switch.getCase(_), _) and
           n.isAfter(switch) and
           c.getSuccessorType() instanceof BreakSuccessor
         |
@@ -1096,14 +1100,59 @@ module Make0<LocationSig Location, AstSig<Location> Ast> {
         )
       }
 
+      /**
+       * Gets an integer indicating the control flow order of a case within a
+       * switch. This is equal to the AST order, except that default cases are
+       * always last in control flow order, even if some languages allow them
+       * to appear before other cases in the AST.
+       */
+      private int getCaseControlFlowOrder(Switch s, Case c) {
+        exists(int pos | s.getCase(pos) = c |
+          // if a default case is not last in the AST, move it last in the CFG order
+          if c instanceof DefaultCase and exists(s.getCase(pos + 1))
+          then result = strictcount(s.getCase(_))
+          else result = pos
+        )
+      }
+
       private Case getRankedCaseCfgOrder(Switch s, int rnk) {
         result = rank[rnk](Case c, int i | getCaseControlFlowOrder(s, c) = i | c order by i)
       }
 
+      private int numberOfStmts(Switch s) { result = strictcount(s.getStmt(_)) }
+
+      private predicate caseIndex(Switch s, Case c, int caseIdx, int caseStmtPos) {
+        c = s.getCase(caseIdx) and
+        c = s.getStmt(caseStmtPos)
+      }
+
+      /**
+       * Gets the body element of `case` at the specified (zero-based) `index`.
+       *
+       * This is either unique when the case has a single right-hand side, or it
+       * is the sequence of statements between this case and the next case.
+       */
+      private AstNode getCaseBodyElement(Case case, int index) {
+        result = case.getBody() and index = 0
+        or
+        not exists(case.getBody()) and
+        exists(Switch s, int caseIdx, int caseStmtPos, int nextCaseStmtPos |
+          caseIndex(pragma[only_bind_into](s), case, caseIdx, caseStmtPos) and
+          (
+            caseIndex(pragma[only_bind_into](s), _, caseIdx + 1, nextCaseStmtPos)
+            or
+            not exists(s.getCase(caseIdx + 1)) and
+            nextCaseStmtPos = numberOfStmts(s)
+          ) and
+          index = [0 .. nextCaseStmtPos - caseStmtPos - 2] and
+          result = pragma[only_bind_into](s).getStmt(caseStmtPos + 1 + index)
+        )
+      }
+
       private AstNode getFirstCaseBodyElement(Case case) {
-        result = case.getBodyElement(0)
+        result = getCaseBodyElement(case, 0)
         or
-        not exists(case.getBodyElement(0)) and
+        not exists(getCaseBodyElement(case, 0)) and
         exists(Switch s, int i |
           fallsThrough(case) and
           // fall-through follows AST order, not case control flow order:
@@ -1113,10 +1162,10 @@ module Make0<LocationSig Location, AstSig<Location> Ast> {
       }
 
       private AstNode getNextCaseBodyElement(AstNode bodyElement) {
-        exists(Case case, int i | case.getBodyElement(i) = bodyElement |
-          result = case.getBodyElement(i + 1)
+        exists(Case case, int i | getCaseBodyElement(case, i) = bodyElement |
+          result = getCaseBodyElement(case, i + 1)
           or
-          not exists(case.getBodyElement(i + 1)) and
+          not exists(getCaseBodyElement(case, i + 1)) and
           exists(Switch s, int j |
             fallsThrough(case) and
             // fall-through follows AST order, not case control flow order:
@@ -1467,7 +1516,7 @@ module Make0<LocationSig Location, AstSig<Location> Ast> {
           or
           n1.isAfter(caseBodyElement) and
           not exists(getNextCaseBodyElement(caseBodyElement)) and
-          n2.isAfter(any(Switch s | s.getCase(_).getBodyElement(_) = caseBodyElement))
+          n2.isAfter(any(Switch s | getCaseBodyElement(s.getCase(_), _) = caseBodyElement))
         )
       }