Repository navigation
SONARJAVA-7290 False negative java:S1854: a dead store inside a lambda body is not reported - #6462
david-schroeter wants to merge 7 commits into
Conversation
|
/pvf |
3edef44 to
ce3a381
Compare
|
📊 Performance Validation complete Projects with unexpected PVF results:
|
rombirli
left a comment
rombirli
left a comment
There was a problem hiding this comment.
I would remove the exception when the return value of the assignation is used, but the assignation itself is not. The rspec doesn't mention any such exception
| private static boolean isVoidLambda(Tree tree) { | ||
| // A void expression lambda discards its body's value without an enclosing expression statement. | ||
| return tree instanceof LambdaExpressionTree lambda && lambda.symbol().returnType().type().isVoid(); | ||
| } |
There was a problem hiding this comment.
i struggle to understand why only void lambdas are considered.
There was a problem hiding this comment.
isExpressionValueDiscarded checks whether the expression’s result is discarded.
For a standalone statement such as ++x;, the parent is an EXPRESSION_STATEMENT, so the result is discarded.
In an expression-bodied lambda such as IntConsumer consumer = x -> ++x, the parent of ++x is the lambda itself. There is no expression-statement wrapper, so we inspect the lambda’s return type:
- For a void lambda, the result is discarded.
- For a value-returning lambda, such as
IntUnaryOperator operator = x -> ++x, the result is returned and therefore used.
That is why this helper considers only void lambdas. Reporting still depends on the other checks, including whether the updated local variable is read afterward.
|
/pvf |
|
@david-schroeter ❌ Your
|
This comment has been minimized.
This comment has been minimized.
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
|
⚖️ Ruling update ready for review: #6466 |
|
⚖️ Ruling update ready for review: #6466 |
|
|
⚖️ Ruling update ready for review: #6466 |
|
After testing chained assignments, I broadened the reporting to make ordinary assignments, compound assignments, and prefix updates consistent: we now report when the updated local variable is never subsequently read, even if the expression’s result is consumed. Preserving existing behavior means continuing to report: int foo(int x) {
return x = x + 2;
}The same unused-store reasoning applies to: int foo(int x) {
return x += 2;
}Both expressions return a useful result, but storing that result in Exempting assignments whose results are consumed would also remove existing findings such as: void example(BufferedReader reader) throws IOException {
String line;
if ((line = reader.readLine()) != null) {
System.out.println("Line available");
}
}The comparison uses the assignment expression’s result, but the store to This extension therefore seems aligned with S1854’s description of unused assignments, although it deliberately adds findings beyond the lambda fix. These equivalent forms still remain compliant because they explicitly read the updated variable: x += 2;
return x;x = x + 2;
return x;Suggesting Does this consistency improvement make sense within this PR, or would you prefer preserving the existing reporting policy and handling the extension separately? |
Code Review ✅ Approved 2 closed / 2 findings🟡 Medium risk · Extending S1854 dead-store analysis to lambdas, constructors, and discarded assignments could produce new false-positive findings in analyzed Java code. Extends S1854 dead store detection to lambda bodies and adds CONSTRUCTOR visiting so dead stores in constructors are analyzed. Plain assignments used as expressions are no longer reported in methods, and the missing constructor test coverage finding is addressed by a reproducer example. ✅ 2 closed✅ Bug: Plain assignments used as expressions are no longer reported in methods
✅ Quality: New CONSTRUCTOR visiting has no test coverage
Verification Rules 1/1 met Repos 4 inspected · PRs 1 inspectedFunctional validation No results Rules 1 passed 2 not applicable
Broader impact Repos 4 inspected PRs 1 inspectedCross-repo 4 repositories inspected 0 open impacts
Cross-PR 1 PR inspected 0 open impacts
Auto-approval Not enabledFor more verification details, comment OptionsAuto-apply is off → Gitar will not commit updates to this branch. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Gitar |
Summary by Gitar
DeadStoreCheckto analyze lambda expression bodies, not only methodsisExpressionValueDiscardedDeadStoreCheckSample.javaTryVisitorfrom descending into lambda bodies so nested try-finally blocks don't exclude the enclosing scopeCONSTRUCTORto visited nodes so dead stores in constructors are analyzedThis will update automatically on new commits.