Java: Add StmtExpr

Author: Marcono1234

java/ql/lib/change-notes/2022-03-27-statement-expression.md

@@ -0,0 +1,4 @@
+---
+category: feature
+---
+* The QL class `StmtExpr` has been added to model statement expressions, that is, expressions whose result is discarded.

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

@@ -84,7 +84,7 @@ class CollectionMutation extends MethodAccess {
   CollectionMutation() { this.getMethod() instanceof CollectionMutator }
 
   /** Holds if the result of this call is not immediately discarded. */
-  predicate resultIsChecked() { not this.getParent() instanceof ExprStmt }
+  predicate resultIsChecked() { not this instanceof StmtExpr }
 }
 
 /** A method that queries the contents of a collection without mutating it. */

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

@@ -2133,3 +2133,41 @@ class Argument extends Expr {
     )
   }
 }
+
+/**
+ * A statement expression, as specified by JLS 17 section 14.8.
+ * The result of a statement expression, if any, is discarded.
+ *
+ * Not to be confused with `ExprStmt`; while the child of an `ExprStmt` is always
+ * a `StmtExpr`, the opposite is not true. A `StmtExpr` occurs for example also
+ * as 'init' of a `for` statement.
+ */
+class StmtExpr extends Expr {
+  StmtExpr() {
+    this = any(ExprStmt s).getExpr()
+    or
+    this = any(ForStmt s).getAnInit() and not this instanceof LocalVariableDeclExpr
+    or
+    this = any(ForStmt s).getAnUpdate()
+    or
+    // Only applies to SwitchStmt, but not to SwitchExpr, see JLS 17 section 14.11.2
+    // TODO: Possibly redundant depending on how https://github.com/github/codeql/issues/8570 is resolved
+    this = any(SwitchStmt s).getACase().getRuleExpression()
+    or
+    // TODO: Workarounds for https://github.com/github/codeql/issues/3605
+    exists(LambdaExpr lambda |
+      this = lambda.getExprBody() and
+      lambda.asMethod().getReturnType() instanceof VoidType
+    )
+    or
+    exists(MemberRefExpr memberRef, Method implicitMethod, Method overridden |
+      implicitMethod = memberRef.asMethod()
+    |
+      this.getParent().(ReturnStmt).getEnclosingCallable() = implicitMethod and
+      // asMethod() has bogus method with wrong return type as result, e.g. `run(): String` (overriding `Runnable.run(): void`)
+      // Therefore need to check the overridden method
+      implicitMethod.getSourceDeclaration().overridesOrInstantiates*(overridden) and
+      overridden.getReturnType() instanceof VoidType
+    )
+  }
+}

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

@@ -53,7 +53,7 @@ class MapMutation extends MethodAccess {
   MapMutation() { this.getMethod() instanceof MapMutator }
 
   /** Holds if the result of this call is not immediately discarded. */
-  predicate resultIsChecked() { not this.getParent() instanceof ExprStmt }
+  predicate resultIsChecked() { not this instanceof StmtExpr }
 }
 
 /** A method that queries the contents of the map it belongs to without mutating it. */

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

@@ -18,7 +18,7 @@ import Chaining
 
 predicate checkedMethodCall(MethodAccess ma) {
   relevantMethodCall(ma, _) and
-  not ma.getParent() instanceof ExprStmt
+  not ma instanceof StmtExpr
 }
 
 /**

java/ql/src/Violations of Best Practice/Exception Handling/IgnoreExceptionalReturn.ql

@@ -45,7 +45,7 @@ predicate unboundedQueue(RefType t) {
 
 from MethodAccess ma, SpecialMethod m
 where
-  ma.getParent() instanceof ExprStmt and
+  ma instanceof StmtExpr and
   m = ma.getMethod() and
   (
     m.isMethod("java.util", "Queue", "offer", 1) and not unboundedQueue(m.getDeclaringType())

java/ql/src/experimental/Security/CWE/CWE-297/IgnoredHostnameVerification.ql

@@ -21,7 +21,7 @@ private class HostnameVerificationCall extends MethodAccess {
   }
 
   /** Holds if the result of the call is not used. */
-  predicate isIgnored() { this = any(ExprStmt es).getExpr() }
+  predicate isIgnored() { this instanceof StmtExpr }
 }
 
 from HostnameVerificationCall verification

java/ql/test/library-tests/StmtExpr/StmtExpr.expected

@@ -0,0 +1,14 @@
+| StmtExpr.java:7:9:7:18 | toString(...) |
+| StmtExpr.java:13:9:13:13 | ...=... |
+| StmtExpr.java:14:9:14:11 | ...++ |
+| StmtExpr.java:15:9:15:11 | ++... |
+| StmtExpr.java:16:9:16:11 | ...-- |
+| StmtExpr.java:17:9:17:11 | --... |
+| StmtExpr.java:19:9:19:20 | new Object(...) |
+| StmtExpr.java:22:9:22:28 | clone(...) |
+| StmtExpr.java:25:14:25:39 | println(...) |
+| StmtExpr.java:30:17:30:44 | println(...) |
+| StmtExpr.java:45:24:45:33 | toString(...) |
+| StmtExpr.java:58:28:58:37 | toString(...) |
+| StmtExpr.java:60:13:60:22 | toString(...) |
+| StmtExpr.java:66:23:66:36 | toString(...) |

java/ql/test/library-tests/StmtExpr/StmtExpr.java

@@ -0,0 +1,68 @@
+package StmtExpr;
+
+import java.util.function.Supplier;
+
+class StmtExpr {
+    void test() {
+        toString();
+
+        // LocalVariableDeclarationStatement with init is not a StatementExpression
+        String s = toString();
+
+        int i;
+        i = 0;
+        i++;
+        ++i;
+        i--;
+        --i;
+
+        new Object();
+        // ArrayCreationExpression cannot be a StatementExpression, but a method access
+        // on it can be
+        new int[] {}.clone();
+
+        // for statement init can be StatementExpression
+        for (System.out.println("init");;) {
+            break;
+        }
+
+        // for statement update is StatementExpression
+        for (;; System.out.println("update")) {
+            break;
+        }
+
+        // variable declaration and condition are not StatementExpressions
+        for (int i1 = 0; i1 < 10;) { }
+        for (int i1, i2 = 0; i2 < 10;) { }
+        for (;;) {
+            break;
+        }
+
+        // Not a StatementExpression
+        for (int i2 : new int[] {1}) { }
+
+        switch(1) {
+            default -> toString(); // StatementExpression
+        }
+        // SwitchExpression has no StatementExpression
+        String s2 = switch(1) {
+            default -> toString();
+        };
+
+        // Lambda with non-void return type has no StatementExpression
+        Supplier<Object> supplier1 = () -> toString();
+        Supplier<Object> supplier2 = () -> {
+            return toString();
+        };
+        // Lambda with void return type has StatementExpression
+        Runnable r = () -> toString();
+        Runnable r2 = () -> {
+            toString();
+        };
+
+        // Method reference with non-void return type has no StatementExpression
+        Supplier<Object> supplier3 = StmtExpr::new;
+        // Method reference with void return type has StatementExpression in implicit method body
+        Runnable r3 = this::toString;
+    }
+}

java/ql/test/library-tests/StmtExpr/StmtExpr.ql

@@ -0,0 +1,4 @@
+import java
+
+from StmtExpr e
+select e