SONARJAVA-6703 Implement new rule S9133 - #5862
Conversation
Detect hard-coded floating-point literals that approximate well-known mathematical constants (pi, e, sqrt(2), ln(2)) and suggest using the corresponding Math class constants or expressions instead.
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
|
❌ Ruling needs updating. A fix PR has been created: #5863 Please review and merge it into your branch. |
Ruling Diff SummaryDetected changes in 1 rule files: 0 issues removed, 18 issues added. S9133 (
|
Replace fixed relative tolerance with dynamic precision-based tolerance derived from the literal's significant digit count. Raise minimum significant digits from 3 to 4 to avoid false positives on common domain values like 0.693. Add test cases for underscore-separated literals, D/d suffixes, leading-dot literals, and zero value. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
4925f8b to
0c2cf07
Compare
Remove unreachable '-' and '+' handling from countSignificantDigits since Java literal tokens never contain sign characters. Add test cases for uppercase 'E' scientific notation, '0X' hex prefix, 'F' float suffix, and leading-dot noncompliant literal. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
rombirli
left a comment
rombirli
left a comment
There was a problem hiding this comment.
The rule looks good, but a few things should be improved :
- support octal/binary bases
- what happens with negative numbers?
- some functions could be rewritten in an easier way, less verbose
| @Rule(key = "S9133") | ||
| public class HardcodedMathConstantCheck extends IssuableSubscriptionVisitor { | ||
|
|
||
| private static final int MIN_SIGNIFICANT_DIGITS = 4; |
There was a problem hiding this comment.
Most commonly used approximation of pi is 3.14, which has only 3 significant digits, i would set it to 3
| value = value.substring(0, value.length() - 1); | ||
| } | ||
| // Skip hex float literals | ||
| if (value.startsWith("0x") || value.startsWith("0X")) { |
There was a problem hiding this comment.
We should extend to binary values (0b) and octal values (leading 0)
| } | ||
| return count; | ||
| } | ||
| } |
There was a problem hiding this comment.
Looks to be incorrect on negative numbers at first sight, the reproducer should contain a negative nubmer case to verify
There was a problem hiding this comment.
it could also be rewritten in a easier-to-understand way :
private static int countSignificantDigits(String normalized) {
String sig = normalized.replace("-", "").replace(".", "").replaceFirst("^0+", "");
return sig.isEmpty() ? 1 : sig.length();
}
| String value = rawValue.replace("_", ""); | ||
| // Strip type suffix | ||
| char last = value.charAt(value.length() - 1); | ||
| if (last == 'f' || last == 'F' || last == 'd' || last == 'D') { |
There was a problem hiding this comment.
what about
if ("fd".contains(last.toLowerCase())) { ... - Lower MIN_SIGNIFICANT_DIGITS from 4 to 3 to catch common approximations like 3.14 - Add support for skipping binary (0b) and octal (leading 0) literals in normalize() - Simplify suffix stripping using "fd".indexOf(Character.toLowerCase()) - Rewrite countSignificantDigits with simpler string operations - Add negative number test cases to verify unary minus handling - Update test expectations for new 3-digit threshold Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
❌ Ruling needs updating. A fix PR has been created: #5876 Please review and merge it into your branch. |
Octal and binary notation do not exist for Java floating-point literals, so the octal/binary guards in normalize() were dead code that could cause false negatives for leading-zero decimal float literals like 03.14159. Keep only the hex float literal guard. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
❌ Ruling needs updating. A fix PR has been created: #5876 Please review and merge it into your branch. |
Code Review ✅ Approved 3 resolved / 3 findingsImplements new rule S9133 to detect hard-coded floating-point approximations of mathematical constants, addressing looseness in tolerance, missing normalize() edge case tests, and incorrect octal/binary skip checks. No issues found. ✅ 3 resolved✅ Edge Case: Loose tolerance may flag legitimate 3-digit domain values
✅ Quality: No tests for normalize() edge cases (underscores, D suffix, leading dot)
✅ Edge Case: Octal/binary skip check wrongly ignores leading-zero float literals
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 hard-coded floating-point literals that approximate well-known mathematical constants (pi, e, sqrt(2), ln(2)) and suggest using the corresponding Math class constants or expressions instead.
Part of