Repository navigation
SONARJAVA-7238: Improve S9395 and resolve conflicts with S1905 and S2184 - #6397
romainbrenguier wants to merge 1 commit into
Conversation
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
- S9395 no longer reports integer arithmetic in the shapes S2184 reports: non-parenthesized arithmetic in initializers, plain assignments, returns and non-varargs arguments, plus integer divisions nested in a floating-point expression. Other arithmetic operands are reported with a message advising to cast one of the operands. - S1905 still reports redundant casts of integer arithmetic results, such as (float) (a * b): the precision-loss suppression from SONARJAVA-7227 no longer applies to them. - Reuse NumericConversionUtils from SONARJAVA-7227 in S9395. - Update S9395 description with alternative fixes and mark quickfix as infeasible. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
d91d4a6 to
dfefe0b
Compare
|
Code Review ✅ Approved 2 closed / 2 findings🟡 Medium risk · Changing S9395/S1905 arithmetic handling may suppress or add precision-loss findings for Java numeric conversions, altering which code issues developers see. Refines S9395 to skip ✅ 2 closed✅ Bug: S9395 arithmetic exclusion covers cases S2184 never reports
✅ Edge Case: S1905 now hides redundant casts on int arithmetic results
Verification Rules 2/2 met Repos 4 inspectedFunctional validation No results Rules 2 passed 1 not applicable
Broader impact Repos 4 inspectedCross-repo 4 repositories 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
Builds on SONARJAVA-7227 (#6391, already on master), which makes S1905 skip casts documenting a potentially lossy
int/long→floatorlong→doubleconversion and addsNumericConversionUtils.+ - * /) in the shapes S2184 already reports: non-parenthesized arithmetic in initializers, plain=assignments, returns and non-varargs arguments, plus integer divisions nested in a floating-point expression. Casting the result ((float) (a / b)) would not fix the integer division, S2184's fix of casting an operand is the right one.+ - * /, so redundant casts such as(float) (a * b)are reported again.BigDecimal, or cast only when the precision loss is acceptable) and references S2184. Quickfix metadata is set toinfeasible, since adding a cast automatically could hide a bug.The RSPEC repository needs the matching description and metadata update.
Ruling results for S9395 and S1905 are expected to change. CI will produce the ruling update.
🤖 Generated with Claude Code
Agent workflow
Tool link: https://github.com/SonarSource/languages-experimental-tooling/tree/romain/my-tickets/personal/romain-brenguier
PR created using
uv run create_with_claude.pyPR updated using
uv run update_with_claude.py --prompt "Implement the action plan described in the document Code Review: Improves S9395 and resolves its conflicts with S1905 and S2184 by excluding int/long-to-float casts from S1905 and skipping+ - * /` arithmetic in S9395. Changes are requested because the S9395 exclusion currently hides cases S2184 never reports (parenthesized operands, compound assignments, varargs calls), leaving them unflagged by either rule.S9395Check.checkExpressionskips all+ - * /expressions, but S2184 misses parenthesized operands, compound assignments, and varargs calls; limit the exclusion or extend S2184 and add test cases for these shapes.RedundantTypeCastChecksuppresses S1905 for lossy int arithmetic casts such as(float) (a * b), so no rule reports this redundant cast; apply the same arithmetic exclusion there for consistency.Files: java-checks/src/main/java/org/sonar/java/checks/S9395Check.java:130-133, java-checks-test-sources/default/src/main/java/checks/S9395CheckSample.java:164-173
S9395Check.checkExpressionnow skips every+ - * /expression afterskipParentheses, on the assumption that S2184 reports them.CastArithmeticOperandCheck(S2184) does not report several of these cases:expressionIsOperationToIntOrLong(expr)runs on the raw expression and does not skip parentheses. Sofloat f = (a * b);orfloat f = (a + b);is not reported. Only DIVIDE is still caught, throughvisitBinaryExpression.visitAssignmentExpressiononly handlesTree.Kind.ASSIGNMENT, sofloatVar += a * bordoubleVar -= longA + longBis not reported.arguments.size() == parametersTypes.size(), sotakeFloatVarargs(a * b, "x", "y")is skipped. S9395 does check fixed varargs parameters.S9395 flagged all of these before this PR, and now neither rule reports them. Either limit the S9395 exclusion to the shapes S2184 actually handles, or extend S2184 to cover them. Please also add test cases for these shapes; the sample only covers the parenthesized DIVIDE case.
Fix (Only skip arithmetic in plain (non-parenthesized) form, as S2184 does. Division is the exception, since S2184's visitBinaryExpression catches it anywhere inside a floating-point expression. Compound assignments and varargs need separate handling, e.g. passing a flag from visitAssignmentExpression and checkArguments.):
ExpressionTree unwrapped = ExpressionUtils.skipParentheses(expr);
if (unwrapped.is(Tree.Kind.TYPE_CAST) || unwrapped.is(Tree.Kind.DIVIDE)
|| (coveredByS2184 && expr.is(Tree.Kind.PLUS, Tree.Kind.MINUS, Tree.Kind.MULTIPLY))) {
return;
}
💡 Edge Case: S1905 now hides redundant casts on int arithmetic results
Files: java-checks/src/main/java/org/sonar/java/checks/RedundantTypeCastCheck.java:93-95, java-checks/src/main/java/org/sonar/java/checks/S9395Check.java:131
RedundantTypeCastChecknow ignores any cast whereisPotentiallyLossyConversion(skipParentheses(expr), castType)is true, whatever the shape of the expression. Takefloat f = (float) (a * b);or(float) (a + b)with int operands. S9395 no longer asks for this cast, because it skips+ - * /. S2184 also stays silent, since the initializer is a TYPE_CAST and not a binary expression, and it only detects nested arithmetic for DIVIDE. Before this PR, S1905 reported the redundant cast. Now no rule reports this pattern, even though the PR description says S1905 still reports truly redundant casts. Applying the same arithmetic exclusion here (do not suppress when the unwrapped operand is+ - * /) would keep the two rules consistent.Fix (Only suppress casts that S9395 would actually ask for):
ExpressionTree castOperand = ExpressionUtils.skipParentheses(typeCastTree.expression());
if (!castOperand.is(Tree.Kind.PLUS, Tree.Kind.MINUS, Tree.Kind.MULTIPLY, Tree.Kind.DIVIDE)
&& NumericConversionUtils.isPotentiallyLossyConversion(castOperand, typeCastTree.type().symbolType())) {
return false;
}." -a "Code Review: Improves S9395 and resolves its conflicts with S1905 and S2184 by excluding int/long-to-float casts from S1905 and skipping
+ - * /arithmetic in S9395. Changes are requested because the S9395 exclusion currently hides cases S2184 never reports (parenthesized operands, compound assignments, varargs calls), leaving them unflagged by either rule.S9395Check.checkExpressionskips all+ - * /expressions, but S2184 misses parenthesized operands, compound assignments, and varargs calls; limit the exclusion or extend S2184 and add test cases for these shapes.RedundantTypeCastChecksuppresses S1905 for lossy int arithmetic casts such as(float) (a * b), so no rule reports this redundant cast; apply the same arithmetic exclusion there for consistency.Files: java-checks/src/main/java/org/sonar/java/checks/S9395Check.java:130-133, java-checks-test-sources/default/src/main/java/checks/S9395CheckSample.java:164-173
S9395Check.checkExpressionnow skips every+ - * /expression afterskipParentheses, on the assumption that S2184 reports them.CastArithmeticOperandCheck(S2184) does not report several of these cases:expressionIsOperationToIntOrLong(expr)runs on the raw expression and does not skip parentheses. Sofloat f = (a * b);orfloat f = (a + b);is not reported. Only DIVIDE is still caught, throughvisitBinaryExpression.visitAssignmentExpressiononly handlesTree.Kind.ASSIGNMENT, sofloatVar += a * bordoubleVar -= longA + longBis not reported.arguments.size() == parametersTypes.size(), sotakeFloatVarargs(a * b, "x", "y")is skipped. S9395 does check fixed varargs parameters.S9395 flagged all of these before this PR, and now neither rule reports them. Either limit the S9395 exclusion to the shapes S2184 actually handles, or extend S2184 to cover them. Please also add test cases for these shapes; the sample only covers the parenthesized DIVIDE case.
Fix (Only skip arithmetic in plain (non-parenthesized) form, as S2184 does. Division is the exception, since S2184's visitBinaryExpression catches it anywhere inside a floating-point expression. Compound assignments and varargs need separate handling, e.g. passing a flag from visitAssignmentExpression and checkArguments.):
ExpressionTree unwrapped = ExpressionUtils.skipParentheses(expr);
if (unwrapped.is(Tree.Kind.TYPE_CAST) || unwrapped.is(Tree.Kind.DIVIDE)
|| (coveredByS2184 && expr.is(Tree.Kind.PLUS, Tree.Kind.MINUS, Tree.Kind.MULTIPLY))) {
return;
}
💡 Edge Case: S1905 now hides redundant casts on int arithmetic results
Files: java-checks/src/main/java/org/sonar/java/checks/RedundantTypeCastCheck.java:93-95, java-checks/src/main/java/org/sonar/java/checks/S9395Check.java:131
RedundantTypeCastChecknow ignores any cast whereisPotentiallyLossyConversion(skipParentheses(expr), castType)is true, whatever the shape of the expression. Takefloat f = (float) (a * b);or(float) (a + b)with int operands. S9395 no longer asks for this cast, because it skips+ - * /. S2184 also stays silent, since the initializer is a TYPE_CAST and not a binary expression, and it only detects nested arithmetic for DIVIDE. Before this PR, S1905 reported the redundant cast. Now no rule reports this pattern, even though the PR description says S1905 still reports truly redundant casts. Applying the same arithmetic exclusion here (do not suppress when the unwrapped operand is+ - * /) would keep the two rules consistent.Fix (Only suppress casts that S9395 would actually ask for):
ExpressionTree castOperand = ExpressionUtils.skipParentheses(typeCastTree.expression());
if (!castOperand.is(Tree.Kind.PLUS, Tree.Kind.MINUS, Tree.Kind.MULTIPLY, Tree.Kind.DIVIDE)
&& NumericConversionUtils.isPotentiallyLossyConversion(castOperand, typeCastTree.type().symbolType())) {
return false;
}" -g "claude"`
PR updated using
uv run update_with_claude.py --prompt "Implement the action plan described in the document /tmp/action_plan_romain/sonarjava-7238.txt." -a "/tmp/action_plan_romain/sonarjava-7238.txt" -g "claude"Iterated on the PR with
uv run ci_loop.pyfor 1 iterations.✔️ The PR is now ready for review.