Java: Add ValueDiscardingExpr

[Marcono1234]

Adds the CodeQL class StmtExpr which models statement expressions, that is, expressions whose result is discarded. This covers more cases than the existing ExprStmt.

The tests are currently placed under library-test/StmtExpr, that is probably not ideal but I did not find an existing well matching library-test subdirectory; any recommendations are welcome.

The newly covered cases by this pull request will probably not occur that often in code, but maybe it is useful to cover them nonetheless.
I have not modified the control flow library, but possibly it could be changed (or simplified) by using StmtExpr.

[smowton]

Seems reasonable to me (paging @aschackmull to double check)

[aschackmull]

As I read JLS 17 section 14.8 a statement expression is the set of expressions that could occur in an ExprStmt, which means that e.g. method invocations and assignments are statement expressions regardless of whether their results are discarded or not. So the JLS definition seems to be broader than what's defined here.
Now, I don't think the JLS definition of statement expressions is going to be particularly useful to have in QL, so I won't ask for that. On the contrary, I think this more restricted version that only includes those statement expressions that actually have their result discarded is a lot more useful, as the other changes in this PR also indicates. But I wonder whether we should perhaps use a different name for this class, since it doesn't exactly match the JLS definition? Maybe something like DiscardedStmtExpr?

[Marcono1234]

method invocations and assignments are statement expressions regardless of whether their results are discarded or not

But if they appear as ExprStmt.getExpr(), then their result is always discarded, isn't it? Because (if I understand it correctly) CodeQL's ExprStmt is exactly the same as JLS' ExpressionStatement. And an ExpressionStatement always discards the result, e.g.:

getValue();
// vs.
consume(getValue());

int i;
i = 1;
// vs.
int i;
int j = (i = 1);

And a few lines below (line 2147 currently) this = any(ExprStmt s).getExpr() is included in the results.

[aschackmull]

Yes. I don't believe I wrote anything that indicated otherwise. I still think we should use a different name for this QL class.

[Marcono1234]

I might have understood your "regardless of whether their results are discarded or not" and "the JLS definition seems to be broader than what's defined here" then incorrectly. To me that sounded as if in some cases the result of a statement expression is not discarded.
This is also why I don't completely understand the need to use a different QL class name, except if there are some general concerns about the name, regardless of functionality.

Which specific case are you thinking of where this QL class would behave differently that defined by the JLS (maybe I am just completely overlooking it at the moment)?

[aschackmull]

To me that sounded as if in some cases the result of a statement expression is not discarded.

Yes, when taking "statement expression" expression to mean the thing that's defined in the JLS, then I believe that's the case. Indeed it looks to me like a JLS-defined "statement expression" includes among other things all method calls, regardless of whether they occur in an ExprStmt and thereby have their result discarded.

This discrepancy between the JLS definition of "statement expression" and the QL class defined here is what leads me to suggest a different name.

[Marcono1234]

Oh, you are right, I did not consider that. Because "statement expression" was defined in the context of expression statement, I assumed that it is also similarly restricted. But it appears there is no such restriction and as you mentioned it simply is a set of expression types.

Maybe similar to your suggested alternative name, what do you think about the following names?

• DiscardingStmtExpr ("discarding" instead of "discarded" because the expression itself is not discarded)
• ValueDiscardingExpr (similar to how the JLS mentions multiple times "if the expression has a value, the value is discarded")
• ResultDiscardingExpr

The CodeQL class could then also be refined to exclude method calls with void as return type.

The documentation for the CodeQL class could then for example be:

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 a ExprStmt or as initializer of a for loop.

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.

[aschackmull]

Of those I think I like ValueDiscardingExpr the best. And your suggested qldoc looks good!

[github-advanced-security]

Found 1 vulnerability.