Repository navigation
Conversation
|
❌ ErrorsYour PR has failed checks. Please review the issues below and take necessary action before merging. 🚦 4 Pipeline jobs failed
Useful? React with 👍 / 👎 This comment will be updated automatically if new data arrives.🔗 Commit SHA: bb825b8 | Docs | View more details | Give us feedback! |
|
⚖️ Ruling update ready for review: #6392 |
Ruling Diff SummaryDetected changes in 2 rule files: 4 issues removed, 0 issues added. S1905 (
|
CI failed: Ruling QA jobs report S1905 ruling diffs versus expected baselines, likely caused by this PR's preserved precision-loss casts; the stacked PR #6392 updates the expected results, but the logs shown do not include the actual mismatch entries.OverviewFour unique log templates were analyzed (4 logs total) for commit bb825b8. All visible failure signals come from Ruling QA jobs that generate a diff between expected and actual S1905 ruling results. The excerpts are truncated and do not include the comparison result itself, so the mismatch is inferred from job context and the stacked PR rather than confirmed by an explicit error line. FailuresS1905 ruling results differ from expected baseline (confidence: medium)
Summary
Code Review 👍 Approved with suggestions 0 closed / 1 findings🟡 Medium risk · Preserving these casts may leave unnecessary conversions unreported by S1905, though the exception is limited to three widening pairs. Preserves explicit casts from 💡 Edge Case: Exemption also hides redundant casts of exactly representable constants📄 java-checks/src/main/java/org/sonar/java/checks/RedundantTypeCastCheck.java:98-104
Skip the exemption for constants that convert exactly, matching S9395🤖 Prompt for agentsVerification Rules 1/2 met Repos 5 inspectedRules 1 failed 1 passed 1 not applicable
Functional validation No results Broader impact Repos 5 inspectedCross-repo 5 repositories inspected 0 open impacts
Auto-approval Not enabledFor more verification details, comment Tip 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 |
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
| private static boolean isPotentiallyLossyWideningConversion(TypeCastTree tree) { | ||
| Type sourceType = tree.expression().symbolType(); | ||
| Type targetType = tree.type().symbolType(); | ||
| return (sourceType.isPrimitive(Type.Primitives.INT) && targetType.isPrimitive(Type.Primitives.FLOAT)) | ||
| || (sourceType.isPrimitive(Type.Primitives.LONG) | ||
| && (targetType.isPrimitive(Type.Primitives.FLOAT) || targetType.isPrimitive(Type.Primitives.DOUBLE))); | ||
| } |
There was a problem hiding this comment.
💡 Edge Case: Exemption also hides redundant casts of exactly representable constants
isPotentiallyLossyWideningConversion only looks at the operand and target types. That means any int→float or long→float/double cast is now exempt, including casts of compile-time constants whose values are exactly representable, such as float f = (float) 0; or double d = (double) 1L;. S9395 never asks for those casts: S9395Check.isSafeConstant (S9395Check.java:156-171) skips integer constants with |v| ≤ 2^24 for float and long constants with |v| ≤ 2^53 for double. So these casts are still truly redundant, and S1905 stops reporting them without any rule conflict to justify it. The conversion can't lose precision there, so the cast documents nothing. To fix this, apply the same safe-constant exclusion here: keep reporting when ExpressionUtils.resolveAsConstant(tree.expression()) gives a value inside the exact range. The simplest way is to share S9395's isLossyWideningConversion/isSafeConstant logic. Also add a Noncompliant sample like float f = (float) 1;.
Skip the exemption for constants that convert exactly, matching S9395:
private static boolean isPotentiallyLossyWideningConversion(TypeCastTree tree) {
Type sourceType = tree.expression().symbolType();
Type targetType = tree.type().symbolType();
boolean lossyTypes = (sourceType.isPrimitive(Type.Primitives.INT) && targetType.isPrimitive(Type.Primitives.FLOAT))
|| (sourceType.isPrimitive(Type.Primitives.LONG)
&& (targetType.isPrimitive(Type.Primitives.FLOAT) || targetType.isPrimitive(Type.Primitives.DOUBLE)));
if (!lossyTypes) {
return false;
}
Object constant = ExpressionUtils.resolveAsConstant(ExpressionUtils.skipParentheses(tree.expression()));
if (constant instanceof Number n) {
long abs = Math.abs(n.longValue());
return targetType.isPrimitive(Type.Primitives.FLOAT) ? abs > (1L << 24) : abs > (1L << 53);
}
return true;
}
- Apply fix
Check the box to apply the fix or reply for a change | Was this helpful? React with 👍 / 👎
There was a problem hiding this comment.
I checked RedundantTypeCastCheck.java:98-104 against S9395Check.isSafeConstant (lines 156-172), and the finding still holds.
isPotentiallyLossyWideningConversion looks only at the source and target types. A cast like (float) 0 or (double) 1L is therefore exempted from S1905, even though S9395 would not ask for it: isSafeConstant treats those constants as exactly representable (|v| ≤ 2^24 for float, ≤ 2^53 for double). So S1905 stops reporting casts that really are redundant and S9395 doesn't require.
If you want to keep this as is, the trade-off is losing those true positives. If you'd rather fix it, I can apply the same constant check here and add a float f = (float) 1; // Noncompliant sample. Reply "fix this" and I'll make the change.
BTW i would find it better if some parts of S9335 were refactored into helpers to avoid code duplication |
This branch has not been deployed
2 participants
S1905 reports the explicit casts recommended by S9395, so following one rule's remediation produces an issue from the other. Preserve casts from
inttofloat,longtofloat, andlongtodouble: these document potentially lossy widening conversions even when the compiler considers the casts redundant.The exception uses the operand and cast types independently of whether S9395 is enabled. Regression samples cover initializers, assignments, compound assignments, method and constructor arguments, returns, and parentheses, while retaining reports for identity and lossless widening casts.
Community report: https://community.sonarsource.com/t/java-s9395-clashes-with-java-s1905/189375
RSPEC: https://github.com/SonarSource/rspec/pull/8386
Validation: the new samples produce 21 unwanted findings before the fix. After the fix, all 5 focused tests pass, including the existing runs without semantic analysis:
mvn -B -pl java-checks -am test -Dtest=RedundantTypeCastCheckTest,S9395CheckTest -Dsurefire.failIfNoSpecifiedTests=false