Java: Rename StmtExpr to ValueDiscardingExpr

As mentioned by aschackmull during review, StatementExpression as defined
by the JLS only lists possible types of expressions, it does _not_ specify
that their value is discarded. Therefore, for example any method call could
be considered a StatementExpression.

The name ValueDiscardingExpr was chosen as replacement because the JLS uses
the phrase "if the expression has a value, the value is discarded" multiple
times.

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

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

java/ql/lib/change-notes/2022-05-09-value-discarding-expression.md

@@ -0,0 +1,4 @@
+---
+category: feature
+---
+* The QL class `ValueDiscardingExpr` has been added, representing expressions for which the value of the expression as a whole 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 instanceof StmtExpr }
+  predicate resultIsChecked() { not this instanceof ValueDiscardingExpr }
 }
 
 /** A method that queries the contents of a collection without mutating it. */

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

@@ -2135,38 +2135,45 @@ 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.
+ * An expression for which the value of the expression as a whole is discarded. Only cases
+ * of discarded values at the language level (as described by the JLS) are considered;
+ * data flow, for example to determine if an assigned variable value is ever read, is not
+ * considered. Such expressions can for example appear as part of an `ExprStmt` or as
+ * initializer of a `for` loop.
  *
- * 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.
+ * For example, for the statement `i++;` the value of the increment expression, that is the
+ * old value of variable `i`, is discarded. Whereas for the statement `println(i++);` the
+ * value of the increment expression is not discarded but used as argument for the method call.
  */
-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
-    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
-    )
+class ValueDiscardingExpr extends Expr {
+  ValueDiscardingExpr() {
+    (
+      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
+      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
+      )
+    ) and
+    // Ignore if this expression is a method call with `void` as return type
+    not getType() 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 instanceof StmtExpr }
+  predicate resultIsChecked() { not this instanceof ValueDiscardingExpr }
 }
 
 /** A method that queries the contents of the map it belongs to without mutating it. */