SONARJAVA-6950: Implemented rule S9392 - Redundant range checks should be removed - #6135
romainbrenguier wants to merge 6 commits into
Conversation
…d be removed This rule detects redundant range checks on the same variable within a single boolean AND expression. When multiple range comparisons (>, <, >=, <=) reference the same variable, one comparison may be logically implied by another. For example, x >= 0 && x >= 5 contains a redundant check because x >= 5 already implies x >= 0.
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
|
❌ Ruling needs updating. A fix PR has been created: #6136 Please review and merge it into your branch. |
- Fix double-counting comparisons when sibling is not a range check - Flip operator for literal-on-left comparisons (e.g. 5 < x) - Add unknown symbol guard to prevent false positives without semantic - Skip nested CONDITIONAL_AND nodes to prevent duplicate issues - Add mutual-implication tie-break for identical comparisons - Use long instead of int for constant values to prevent truncation - Remove planning artifact new_rule_proposal_S9392.txt - Add test cases for literal-on-left, identical comparisons, long literals, mixed operators, and non-range operands Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Fix S3776/S135: Extract helper methods to reduce cognitive complexity - Fix S2325: Make tryAddComparison static - Fix S1132: Use string-literal-on-left style in equals() comparisons - Skip identical comparisons to avoid overlap with S1764 - Use secondary location for implying check instead of normalized text - Guard against side effects (method calls, assignments) in && chains Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
| @Nullable | ||
| private static Comparison createComparison(Symbol variable, String operator, @Nullable Long constant, BinaryExpressionTree tree) { | ||
| if (!variable.isUnknown() && constant != null) { | ||
| return new Comparison(variable, operator, constant, tree); | ||
| } | ||
| return null; | ||
| } |
There was a problem hiding this comment.
⚠️ Bug: test_without_semantic expects issues the check cannot raise
createComparison returns null whenever variable.isUnknown(), and in no-semantic mode VisitorsBridgeForTests passes a null semantic model so IdentifierTreeImpl.symbol() yields Symbol.UNKNOWN_SYMBOL even for method parameters — the check therefore raises zero issues on RedundantRangeCheckCheckSample.java. RedundantRangeCheckCheckTest.test_without_semantic() nevertheless calls verifyIssues(), and since the sample declares ~20 // Noncompliant expectations, expectations.expectNoIssues() is false and assertMultipleIssues fails the build. Switch that test to verifyNoIssues().
The check needs symbols, so no issues can be raised without semantics.:
@Test
void test_without_semantic() {
CheckVerifier.newVerifier()
.onFile(mainCodeSourcesPath("checks/RedundantRangeCheckCheckSample.java"))
.withCheck(new RedundantRangeCheckCheck())
.withoutSemantic()
.verifyNoIssues();
}
- Apply fix
Check the box to apply the fix or reply for a change | Was this helpful? React with 👍 / 👎
- Make hasPotentialSideEffects recursive to detect nested side effects like !refresh(), x++ > 3, and assignment expressions within subtrees - Replace Comparison.isIdenticalTo with SyntacticEquivalence.areEquivalent to properly defer to S1764 only for syntactically identical comparisons - Remove dead isIdenticalTo method from Comparison class - Add test cases for nested side effects and non-syntactically-identical but logically equivalent comparisons (x >= 5 && 5 <= x) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Simplify if-then-else to single return statement to resolve SonarQube QG failure. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
nathsou
left a comment
nathsou
left a comment
There was a problem hiding this comment.
The core implication logic looks sound. I left three non-blocking edge cases for follow-up.
|
|
||
| String operator = comparison.operatorToken().text(); | ||
|
|
||
| if (left.is(Kind.IDENTIFIER) && right.is(Kind.INT_LITERAL, Kind.LONG_LITERAL)) { |
There was a problem hiding this comment.
Signed constants never reach LiteralUtils: Java represents -10 and +10 as unary expressions, so this kind guard rejects them even though the literal helpers can resolve them. For example, x >= -10 && x >= -5 should report the first comparison but currently does not. Could we accept unary plus/minus constant operands and add regression tests?
| } | ||
|
|
||
| @Override | ||
| public void visitMethodReference(MethodReferenceTree tree) { |
There was a problem hiding this comment.
This safety check misses expressions that can throw without having conventional side effects. For example, in x >= 0 && array[0] > 0 && x >= 5, reporting/removing x >= 0 makes the array access execute for negative x, potentially introducing an NPE or AIOOBE. Division, casts, and unboxing have the same concern. Could we require intervening operands to be safely evaluable, or conservatively split/skip such chains?
| } | ||
| if (hasPotentialSideEffects(operand)) { | ||
| comparisonsByVariable.clear(); | ||
| return false; |
There was a problem hiding this comment.
Clearing and aborting the whole chain also loses safe findings that precede a side-effect boundary. In x >= 0 && x >= 5 && refresh(), the first comparison is redundant and removing it does not change whether refresh() runs, but this reports nothing. Could we segment analysis at unsafe operands instead of discarding comparisons already collected before them?
…Check Handle unary plus/minus constant operands (e.g. -10, +5) in range check comparisons, as Java represents these as unary expressions rather than negative literals. Added regression tests for signed constant scenarios. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
CI failed: CI failed due to a build number fetch error causing an unexpected end of JSON input, and a missing ruling artifacts download in the notification job.OverviewTwo distinct errors occurred across the CI run: a build configuration failure where fetching the build number returned malformed or empty JSON, and a dependent ruling update job failure due to missing workflow artifacts. FailuresBuild Number Fetch Error (confidence: high)
Missing Ruling Artifacts (confidence: high)
Summary
Code Review
|
| Auto-apply | Compact | Unblock |
|
|
|
Was this helpful? React with 👍 / 👎 | Gitar
2 participants
This rule detects redundant range checks on the same variable within a single boolean AND expression. When multiple range comparisons (>, <, >=, <=) reference the same variable, one comparison may be logically implied by another. For example,
x >= 0 && x >= 5contains a redundant check becausex >= 5already impliesx >= 0.