SONARJAVA-6108 ScopedValue where result is ignored#5441
Conversation
|
I tried to cover as many edge cases as possible, but let's look at the following example : void A() {
var carrier = ScopedValue.where(SCOPED, "hello"); // FN carrier is not used
identity(carrier); // the rule consider carrier escapes from the function A with this call
}
ScopedValue.Carrier identity(ScopedValue.Carrier carrier) {
return carrier; // the rule consider carrier escapes from the function identity with this return
}I think the only way to implement this rule properly would be with CFG/symbolic execution |
asya-vorobeva
left a comment
asya-vorobeva
left a comment
There was a problem hiding this comment.
Sorry, but I can't validate such PR, too AI-generated. Common things that I'd recommend:
- try to reduce Sample files (a lot of things to test are redundant)
- try to restructure Check file, now it's too much boilerplate and very hard to maintain.
| } | ||
|
|
||
| void escapedCarrierFunctionCall() { | ||
| var carrier = ScopedValue.where(myScopedValue, "hello"); // ccompliant - the result escapes |
| ScopedValue.where(myScopedValue, "hello").get(myScopedValue); // Noncompliant | ||
| ScopedValue.where(myScopedValue, "hello").run(() -> { | ||
| }); // Compliant, the result is used immediately | ||
| ScopedValue.where(myScopedValue, "hello").where(ScopedValue.newInstance(), "hello").run(() -> { |
There was a problem hiding this comment.
I don't like this example. The second chain call .where(ScopedValue.newInstance(), "hello") looks weird and should raise an issue (probably another rule?). We can't use such binding if it's unassigned. The same applies for myUnused2LevelCarrier below.
rombirli
changed the title
rombirli
force-pushed
the
rombirli/SONARJAVA-6078-scopedvalue-where-result-ignored
branch
from
87729b6 to
8576a8a
Compare
asya-vorobeva
left a comment
asya-vorobeva
left a comment
There was a problem hiding this comment.
A few improvements, generally LGTM
| Tree parent = mit.parent(); | ||
|
|
||
| // Special case: if there is no parent, consider it consumed to avoid false positives | ||
| if (parent == null) { |
There was a problem hiding this comment.
I think parent is never null for such case
| Tree parent = usage.parent(); | ||
|
|
||
| // Special case: if there is no parent, consider it valid to avoid false positives | ||
| if (parent == null) { |
There was a problem hiding this comment.
The same here: we can't have null parent, I guess?
| Tree parent = tree.parent(); | ||
|
|
||
| // Special case: if there is no parent, consider it escaping to avoid false positives | ||
| if (parent == null) { |
There was a problem hiding this comment.
This condition never met because currently you're calling this method only after explicit check for non-nullable parent.
There was a problem hiding this comment.
I’ve removed the null checks for the parent objects. Since these references are guaranteed to exist in any valid, compiling code snippet, these checks were redundant.
… lambdas, variable reassignment, multiple var same carrier...
…edValueWhereResultEdgeCasesSample.java
…ng, decompose isUsageValid
# Conflicts: # sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/Sonar_way_profile.json
rombirli
force-pushed
the
rombirli/SONARJAVA-6078-scopedvalue-where-result-ignored
branch
from
ef0740f to
f5032ec
Compare
|
rombirli
deleted the
rombirli/SONARJAVA-6078-scopedvalue-where-result-ignored
branch
2 participants
Blocked by : https://github.com/SonarSource/rspec/pull/6136