Merge pull request #8582 from Marcono1234/marcono1234/JumpStmt-superclass

Java: Make `JumpStmt` a proper superclass

Author: aschackmull

java/ql/lib/change-notes/2022-03-28-JumpStmt-as-superclass.md

@@ -0,0 +1,4 @@
+---
+category: fix
+---
+* The QL class `JumpStmt` has been made the superclass of `BreakStmt`, `ContinueStmt` and `YieldStmt`. This allows directly using its inherited predicates without having to explicitly cast to `JumpStmt` first.

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

@@ -1399,7 +1399,7 @@ class SwitchExpr extends Expr, StmtParent, @switchexpr {
   Expr getAResult() {
     result = this.getACase().getRuleExpression()
     or
-    exists(YieldStmt yield | yield.(JumpStmt).getTarget() = this and result = yield.getValue())
+    exists(YieldStmt yield | yield.getTarget() = this and result = yield.getValue())
   }
 
   /** Gets a printable representation of this expression. */

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

@@ -570,14 +570,10 @@ class ThrowStmt extends Stmt, @throwstmt {
   override string getAPrimaryQlClass() { result = "ThrowStmt" }
 }
 
-/** A `break`, `yield` or `continue` statement. */
-class JumpStmt extends Stmt {
-  JumpStmt() {
-    this instanceof BreakStmt or
-    this instanceof YieldStmt or
-    this instanceof ContinueStmt
-  }
+private class JumpStmt_ = @breakstmt or @yieldstmt or @continuestmt;
 
+/** A `break`, `yield` or `continue` statement. */
+class JumpStmt extends Stmt, JumpStmt_ {
   /**
    * Gets the labeled statement that this `break` or
    * `continue` statement refers to, if any.
@@ -598,12 +594,7 @@ class JumpStmt extends Stmt {
     )
   }
 
-  private SwitchExpr getSwitchExprTarget() { result = this.(YieldStmt).getParent+() }
-
   private StmtParent getEnclosingTarget() {
-    result = this.getSwitchExprTarget()
-    or
-    not exists(this.getSwitchExprTarget()) and
     result = this.getAPotentialTarget() and
     not exists(Stmt other | other = this.getAPotentialTarget() | other.getEnclosingStmt+() = result)
   }
@@ -612,14 +603,15 @@ class JumpStmt extends Stmt {
    * Gets the statement or `switch` expression that this `break`, `yield` or `continue` jumps to.
    */
   StmtParent getTarget() {
+    // Note: This implementation only considers `break` and `continue`; YieldStmt overrides this predicate
     result = this.getLabelTarget()
     or
     not exists(this.getLabelTarget()) and result = this.getEnclosingTarget()
   }
 }
 
 /** A `break` statement. */
-class BreakStmt extends Stmt, @breakstmt {
+class BreakStmt extends JumpStmt, @breakstmt {
   /** Gets the label targeted by this `break` statement, if any. */
   string getLabel() { namestrings(result, _, this) }
 
@@ -640,12 +632,21 @@ class BreakStmt extends Stmt, @breakstmt {
 /**
  * A `yield` statement.
  */
-class YieldStmt extends Stmt, @yieldstmt {
+class YieldStmt extends JumpStmt, @yieldstmt {
   /**
    * Gets the value of this `yield` statement.
    */
   Expr getValue() { result.getParent() = this }
 
+  /**
+   * Gets the `switch` expression target of this `yield` statement.
+   */
+  override SwitchExpr getTarget() {
+    // Get the innermost enclosing SwitchExpr; this works because getParent() is defined for Stmt and
+    // therefore won't proceed after the innermost SwitchExpr (due to it being an Expr)
+    result = this.getParent+()
+  }
+
   override string pp() { result = "yield ..." }
 
   override string toString() { result = "yield ..." }
@@ -656,7 +657,7 @@ class YieldStmt extends Stmt, @yieldstmt {
 }
 
 /** A `continue` statement. */
-class ContinueStmt extends Stmt, @continuestmt {
+class ContinueStmt extends JumpStmt, @continuestmt {
   /** Gets the label targeted by this `continue` statement, if any. */
   string getLabel() { namestrings(result, _, this) }
 

java/ql/src/Likely Bugs/Statements/ContinueInFalseLoop.ql

@@ -16,6 +16,6 @@ import java
 from DoStmt do, ContinueStmt continue
 where
   do.getCondition().(BooleanLiteral).getBooleanValue() = false and
-  continue.(JumpStmt).getTarget() = do
+  continue.getTarget() = do
 select continue, "This 'continue' never re-runs the loop - the $@ is always false.",
   do.getCondition(), "loop condition"

java/ql/src/Likely Bugs/Termination/ConstantLoopCondition.ql

@@ -32,7 +32,7 @@ predicate loopExit(LoopStmt loop, Stmt exit) {
   exit.getEnclosingStmt*() = loop.getBody() and
   (
     exit instanceof ReturnStmt or
-    exit.(BreakStmt).(JumpStmt).getTarget() = loop.getEnclosingStmt*()
+    exit.(BreakStmt).getTarget() = loop.getEnclosingStmt*()
   )
 }
 

java/ql/src/Security/CWE/CWE-129/ArraySizing.qll

@@ -44,11 +44,11 @@ class PointlessLoop extends WhileStmt {
   PointlessLoop() {
     this.getCondition().(BooleanLiteral).getBooleanValue() = true and
     // The only `break` must be the last statement.
-    forall(BreakStmt break | break.(JumpStmt).getTarget() = this |
+    forall(BreakStmt break | break.getTarget() = this |
       this.getStmt().(BlockStmt).getLastStmt() = break
     ) and
     // No `continue` statements.
-    not exists(ContinueStmt continue | continue.(JumpStmt).getTarget() = this)
+    not exists(ContinueStmt continue | continue.getTarget() = this)
   }
 }
 

java/ql/src/Security/CWE/CWE-835/InfiniteLoop.ql

@@ -26,7 +26,7 @@ predicate loopCondition(LoopStmt loop, Expr cond, boolean polarity) {
     ifstmt.getEnclosingStmt*() = loop.getBody() and
     ifstmt.getCondition() = cond and
     (
-      exit.(BreakStmt).(JumpStmt).getTarget() = loop or
+      exit.(BreakStmt).getTarget() = loop or
       exit.(ReturnStmt).getEnclosingStmt*() = loop.getBody()
     ) and
     (

java/ql/test/library-tests/stmts/JumpTargets.expected

@@ -1,3 +1,8 @@
+| stmts/A.java:18:5:18:12 | yield ... | stmts/A.java:16:10:16:18 | switch (...) |
+| stmts/A.java:22:6:22:13 | yield ... | stmts/A.java:20:14:20:22 | switch (...) |
+| stmts/A.java:25:6:25:13 | yield ... | stmts/A.java:20:14:20:22 | switch (...) |
+| stmts/A.java:34:7:34:14 | yield ... | stmts/A.java:16:10:16:18 | switch (...) |
+| stmts/A.java:37:5:37:13 | yield ... | stmts/A.java:16:10:16:18 | switch (...) |
 | stmts/B.java:8:5:8:10 | break | stmts/B.java:6:3:6:26 | for (...;...;...) |
 | stmts/B.java:10:5:10:13 | continue | stmts/B.java:6:3:6:26 | for (...;...;...) |
 | stmts/B.java:13:6:13:11 | break | stmts/B.java:11:4:11:17 | while (...) |

java/ql/test/library-tests/stmts/SwitchCases.expected

@@ -1,6 +1,14 @@
 | stmts/A.java:6:3:6:10 | case ... |
 | stmts/A.java:8:3:8:10 | case ... |
 | stmts/A.java:10:3:10:10 | default |
+| stmts/A.java:17:4:17:12 | case ... |
+| stmts/A.java:20:4:20:12 | case ... |
+| stmts/A.java:21:5:21:13 | case ... |
+| stmts/A.java:24:5:24:14 | default |
+| stmts/A.java:28:4:28:12 | case ... |
+| stmts/A.java:29:4:29:13 | default |
+| stmts/A.java:32:6:32:14 | case ... |
+| stmts/A.java:33:6:33:14 | case ... |
 | stmts/B.java:21:5:21:12 | case ... |
 | stmts/B.java:23:5:23:12 | case ... |
 | stmts/B.java:25:5:25:12 | case ... |

java/ql/test/library-tests/stmts/options

@@ -0,0 +1 @@
+//semmle-extractor-options: --javac-args -source 14 -target 14

java/ql/test/library-tests/stmts/stmts/A.java

@@ -11,4 +11,31 @@ public int foo(int x) {
 			return x;
 		}
 	}
+
+	public int nestedSwitchExpr(int x, int y) {
+		return switch(x) {
+			case 1 -> {
+				yield 1;
+			}
+			case 2 -> switch(y) {
+				case 0 -> {
+					yield 0;
+				}
+				default -> {
+					yield 1;
+				}
+			};
+			case 3 -> 3;
+			default -> {
+				// SwitchStmt inside SwitchExpr
+				switch (y) {
+					case 1 -> System.out.println("1");
+					case 2 -> {
+						yield 2;
+					}
+				}
+				yield -1;
+			}
+		};
+	}
 }