SONARJAVA-6744 Implement new rule S9148: "Float.compare" or "Double.compare" should be used for floating-point comparisons - #5910
Conversation
Detect incorrect floating-point comparisons (subtraction and relational operators on float/double) inside compareTo, Comparator.compare methods, and Comparator lambdas. Users should use Double.compare or Float.compare instead to correctly handle NaN and negative zero.
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
… classes - Add null-check for MethodTree.block() to skip abstract/interface methods - Restrict isCompareMethod to classes implementing java.util.Comparator - Fix column indicator in test sample - Set quickfix to "infeasible" since no quickfixes are implemented - Add test cases for abstract methods and non-Comparator utility methods Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Merge the identical MINUS and relational branches in FloatingPointComparisonVisitor into a single condition (fixes S1871). - Add the autoscan expected baseline for S9148 (hasTP, no FP/FN), taken from the failing CI run's artifact, and the matching entry in autoscan-diff-by-rules.json. - Add test samples covering the previously uncovered paths: nested non-Comparator lambda, local class inside compareTo, a single floating-point operand, and methods that only look like comparison methods. Coverage of the check goes from 85.7% to ~98.7% (all lines covered, 43 of 44 conditions). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
| void test() { | ||
| CheckVerifier.newVerifier() | ||
| .onFile(mainCodeSourcesPath("checks/FloatingPointComparisonCheckSample.java")) | ||
| .withCheck(new FloatingPointComparisonCheck()) |
There was a problem hiding this comment.
We should use withoutSemantics
There was a problem hiding this comment.
Yes, as this rule shouldn't work without semantic (if implemented correctly using matchers), it could be good to add a no-semantic test
| private static boolean isCompareToMethod(MethodTree tree) { | ||
| return "compareTo".equals(tree.simpleName().name()) | ||
| && returnsInt(tree) | ||
| && hasOneNonPrimitiveParameter(tree); | ||
| } | ||
|
|
||
| private static boolean isCompareMethod(MethodTree tree) { | ||
| return "compare".equals(tree.simpleName().name()) | ||
| && returnsInt(tree) | ||
| && tree.parameters().size() == 2 | ||
| && tree.symbol().owner().type().isSubtypeOf("java.util.Comparator"); | ||
| } |
There was a problem hiding this comment.
Please use org.sonar.plugins.java.api.semantic.MethodMatchers here instead. A single matcher (with MethodMatchers.or) should work
Note: This is a recurring issue across new sonar-java rule PRs. If you used a skill to implement this, please consider updating the skill instructions to include MethodMatchers as the standard approach.
There was a problem hiding this comment.
I'm using nigel with guidance that contains that:
### MethodMatchers
Use MethodMatchers to match method calls by type, name, and signature:
private static final MethodMatchers MY_MATCHER = MethodMatchers.create()
.ofTypes("java.util.List")
.names("add")
.withAnyParameters()
.build();
// In visitNode:
if (MY_MATCHER.matches(methodInvocationTree)) {
reportIssue(methodInvocationTree, "Message.");
}
But that doesn't seem to work. I'll try to debug that for the next rule.
| void test() { | ||
| CheckVerifier.newVerifier() | ||
| .onFile(mainCodeSourcesPath("checks/FloatingPointComparisonCheckSample.java")) | ||
| .withCheck(new FloatingPointComparisonCheck()) |
There was a problem hiding this comment.
Yes, as this rule shouldn't work without semantic (if implemented correctly using matchers), it could be good to add a no-semantic test
…e duplicate branches - Refactor to use MethodMatchers.or() instead of manual method matching - Remove helper methods: isCompareToMethod, isCompareMethod, returnsInt, hasOneNonPrimitiveParameter - Merge duplicate S1871 branches in visitBinaryExpression into single condition - Add withoutSemantic test to verify no issues without semantic analysis - Guard visitNode with semantic model null check Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Code Review ✅ Approved 2 resolved / 2 findingsImplements new rule S9148 to detect incorrect floating-point comparisons inside comparison methods and lambdas, addressing null pointer exceptions on abstract methods and false positives on non-Comparator classes. ✅ 2 resolved✅ Bug: NPE on abstract compareTo/compare methods
✅ Bug: 'compare' detection does not require Comparator type
Implementation Status 🟡 0 / 1 issues implemented⬜ SONARJAVA-6744 — 0 / 1 objectivesThe PR does not contain any code changes implementing rule S9148.
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 |
2 participants
Detect incorrect floating-point comparisons (subtraction and relational operators on float/double) inside compareTo, Comparator.compare methods, and Comparator lambdas. Users should use Double.compare or Float.compare instead to correctly handle NaN and negative zero.