-
Notifications
You must be signed in to change notification settings - Fork 723
SONARJAVA-6703 Implement new rule S9133 #5862
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Draft
romainbrenguier
wants to merge
3
commits into
master
Choose a base branch
from
new-rule/SONARJAVA-6703-S9133
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
111 changes: 111 additions & 0 deletions
111
java-checks-test-sources/default/src/main/java/checks/HardcodedMathConstantCheckSample.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,111 @@ | ||
| package checks; | ||
|
|
||
| class HardcodedMathConstantCheckSample { | ||
|
|
||
| // Pi approximations at various precisions (4+ significant digits) | ||
| double pi2 = 3.14159; // Noncompliant {{Use "Math.PI" instead of this approximation of pi.}} | ||
| double pi3 = 3.14159265; // Noncompliant {{Use "Math.PI" instead of this approximation of pi.}} | ||
| double pi4 = 3.14159265358979; // Noncompliant {{Use "Math.PI" instead of this approximation of pi.}} | ||
| double pi5 = 3.141; // Noncompliant {{Use "Math.PI" instead of this approximation of pi.}} | ||
|
|
||
| // E approximations | ||
| double e1 = 2.718; // Noncompliant {{Use "Math.E" instead of this approximation of Euler's number.}} | ||
| double e2 = 2.71828; // Noncompliant {{Use "Math.E" instead of this approximation of Euler's number.}} | ||
| double e3 = 2.71828182845; // Noncompliant {{Use "Math.E" instead of this approximation of Euler's number.}} | ||
|
|
||
| // sqrt(2) approximations | ||
| double sqrt2_1 = 1.414; // Noncompliant {{Use "Math.sqrt(2)" instead of this approximation of the square root of 2.}} | ||
| double sqrt2_2 = 1.41421; // Noncompliant {{Use "Math.sqrt(2)" instead of this approximation of the square root of 2.}} | ||
| double sqrt2_3 = 1.4142135; // Noncompliant {{Use "Math.sqrt(2)" instead of this approximation of the square root of 2.}} | ||
|
|
||
| // ln(2) approximations (4+ significant digits) | ||
| double ln2_2 = 0.6931; // Noncompliant {{Use "Math.log(2)" instead of this approximation of the natural logarithm of 2.}} | ||
| double ln2_3 = 0.69314; // Noncompliant {{Use "Math.log(2)" instead of this approximation of the natural logarithm of 2.}} | ||
| double ln2_4 = 0.693147; // Noncompliant {{Use "Math.log(2)" instead of this approximation of the natural logarithm of 2.}} | ||
|
|
||
| // Float literals | ||
| float piFloat = 3.14159f; // Noncompliant {{Use "Math.PI" instead of this approximation of pi.}} | ||
| float eFloat = 2.718f; // Noncompliant {{Use "Math.E" instead of this approximation of Euler's number.}} | ||
|
|
||
| // Underscore-separated literal (normalize strips underscores) | ||
| double piUnderscore = 3.14_159; // Noncompliant {{Use "Math.PI" instead of this approximation of pi.}} | ||
|
|
||
| // D-suffix double literal (normalize strips suffix) | ||
| double piDsuffix = 3.14159d; // Noncompliant {{Use "Math.PI" instead of this approximation of pi.}} | ||
| double eDsuffix = 2.71828D; // Noncompliant {{Use "Math.E" instead of this approximation of Euler's number.}} | ||
|
|
||
| // Static final field | ||
| private static final double MY_PI = 3.14159265358979; // Noncompliant | ||
|
|
||
| // In arithmetic expressions | ||
| double area(double r) { | ||
| return 3.14159 * r * r; // Noncompliant | ||
| } | ||
|
|
||
| double circumference(double r) { | ||
| return 2 * 3.14159 * r; // Noncompliant | ||
| } | ||
|
|
||
| double volume(double r) { | ||
| return (4.0 / 3.0) * 3.14159 * r * r * r; // Noncompliant | ||
| } | ||
|
|
||
| // Method arguments | ||
| double shifted(double x) { | ||
| return Math.sin(x + 3.14159); // Noncompliant | ||
| } | ||
|
|
||
| // Ternary expression | ||
| double pick(boolean b, double x) { | ||
| return b ? 3.14159 : x; // Noncompliant | ||
| } | ||
|
|
||
| // Compliant - standard library constants | ||
| double compliantPi = Math.PI; | ||
| double compliantE = Math.E; | ||
| double compliantSqrt2 = Math.sqrt(2); | ||
| double compliantLn2 = Math.log(2); | ||
| double compliantStrictPi = StrictMath.PI; | ||
| double compliantStrictE = StrictMath.E; | ||
|
|
||
| // Compliant - unrelated values | ||
| double unrelated1 = 3.0; | ||
| double unrelated2 = 2.0; | ||
| double unrelated3 = 1.5; | ||
| double unrelated4 = 0.5; | ||
| double unrelated5 = 100.0; | ||
| double unrelated6 = 0.001; | ||
|
|
||
| // Compliant - zero value (covers absoluteValue == 0.0 branch) | ||
| double zero = 0.0; | ||
|
|
||
| // Compliant - too few significant digits (fewer than 4) | ||
| double tooImprecise1 = 3.14; | ||
| double tooImprecise2 = 3.1; | ||
| double tooImprecise3 = 2.7; | ||
| double tooImprecise4 = 1.4; | ||
| double tooImprecise5 = 0.69; | ||
| double tooImprecise6 = 0.693; | ||
|
|
||
| // Compliant - outside tolerance | ||
| double outsideTolerance1 = 3.15; | ||
| double outsideTolerance2 = 1.41; | ||
|
|
||
| // Compliant - scientific notation (skipped) | ||
| double sci1 = 3.14e0; | ||
| double sci2 = 314E-2; | ||
|
|
||
| // Compliant - hex float literals (skipped) | ||
| double hex1 = 0x1.0p0; | ||
| double hex2 = 0X1.0p0; | ||
|
|
||
| // Compliant - leading-dot literal with too few significant digits | ||
| double leadingDot = .693; | ||
|
|
||
| // Float literal with F suffix | ||
| float piFloatF = 3.14159F; // Noncompliant {{Use "Math.PI" instead of this approximation of pi.}} | ||
|
|
||
| // Leading-dot literal with enough significant digits | ||
| double leadingDotLn2 = .6931; // Noncompliant {{Use "Math.log(2)" instead of this approximation of the natural logarithm of 2.}} | ||
|
|
||
| } |
127 changes: 127 additions & 0 deletions
127
java-checks/src/main/java/org/sonar/java/checks/HardcodedMathConstantCheck.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,127 @@ | ||
| /* | ||
| * SonarQube Java | ||
| * Copyright (C) SonarSource Sàrl | ||
| * mailto:info AT sonarsource DOT com | ||
| * | ||
| * You can redistribute and/or modify this program under the terms of | ||
| * the Sonar Source-Available License Version 1, as published by SonarSource Sàrl. | ||
| * | ||
| * This program is distributed in the hope that it will be useful, | ||
| * but WITHOUT ANY WARRANTY; without even the implied warranty of | ||
| * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. | ||
| * See the Sonar Source-Available License for more details. | ||
| * | ||
| * You should have received a copy of the Sonar Source-Available License | ||
| * along with this program; if not, see https://sonarsource.com/license/ssal/ | ||
| */ | ||
| package org.sonar.java.checks; | ||
|
|
||
| import java.util.List; | ||
| import org.sonar.check.Rule; | ||
| import org.sonar.plugins.java.api.IssuableSubscriptionVisitor; | ||
| import org.sonar.plugins.java.api.tree.LiteralTree; | ||
| import org.sonar.plugins.java.api.tree.Tree; | ||
|
|
||
| @Rule(key = "S9133") | ||
| public class HardcodedMathConstantCheck extends IssuableSubscriptionVisitor { | ||
|
|
||
| private static final int MIN_SIGNIFICANT_DIGITS = 4; | ||
|
|
||
| private enum MathConstant { | ||
| PI(Math.PI, "Math.PI", "pi"), | ||
| E(Math.E, "Math.E", "Euler's number"), | ||
| SQRT2(Math.sqrt(2), "Math.sqrt(2)", "the square root of 2"), | ||
| LN2(Math.log(2), "Math.log(2)", "the natural logarithm of 2"); | ||
|
|
||
| final double value; | ||
| final String replacement; | ||
| final String description; | ||
|
|
||
| MathConstant(double value, String replacement, String description) { | ||
| this.value = value; | ||
| this.replacement = replacement; | ||
| this.description = description; | ||
| } | ||
| } | ||
|
|
||
| @Override | ||
| public List<Tree.Kind> nodesToVisit() { | ||
| return List.of(Tree.Kind.DOUBLE_LITERAL, Tree.Kind.FLOAT_LITERAL); | ||
| } | ||
|
|
||
| @Override | ||
| public void visitNode(Tree tree) { | ||
| LiteralTree literalTree = (LiteralTree) tree; | ||
| String rawValue = literalTree.value(); | ||
|
|
||
| String normalized = normalize(rawValue); | ||
| if (normalized == null) { | ||
| return; | ||
| } | ||
|
|
||
| double parsedValue; | ||
| try { | ||
| parsedValue = Double.parseDouble(normalized); | ||
| } catch (NumberFormatException e) { | ||
| return; | ||
| } | ||
|
|
||
| double absoluteValue = Math.abs(parsedValue); | ||
| if (absoluteValue == 0.0) { | ||
| return; | ||
| } | ||
|
|
||
| int significantDigits = countSignificantDigits(normalized); | ||
| if (significantDigits < MIN_SIGNIFICANT_DIGITS) { | ||
| return; | ||
| } | ||
|
|
||
| // Tolerance is based on the literal's own precision: half a unit in the last significant digit. | ||
| // This ensures we only flag values that match the constant across all their significant digits. | ||
| double relativeTolerance = 5.0 * Math.pow(10, -significantDigits); | ||
|
|
||
| for (MathConstant constant : MathConstant.values()) { | ||
| double relativeError = Math.abs(absoluteValue - constant.value) / constant.value; | ||
| if (relativeError < relativeTolerance) { | ||
| reportIssue(tree, "Use \"" + constant.replacement + "\" instead of this approximation of " + constant.description + "."); | ||
| return; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| private static String normalize(String rawValue) { | ||
| String value = rawValue.replace("_", ""); | ||
| // Strip type suffix | ||
| char last = value.charAt(value.length() - 1); | ||
| if (last == 'f' || last == 'F' || last == 'd' || last == 'D') { | ||
| value = value.substring(0, value.length() - 1); | ||
| } | ||
| // Skip hex float literals | ||
| if (value.startsWith("0x") || value.startsWith("0X")) { | ||
| return null; | ||
| } | ||
| // Skip scientific notation | ||
| if (value.indexOf('e') >= 0 || value.indexOf('E') >= 0) { | ||
| return null; | ||
| } | ||
| return value; | ||
| } | ||
|
|
||
| private static int countSignificantDigits(String normalized) { | ||
| boolean foundNonZero = false; | ||
| int count = 0; | ||
| for (int i = 0; i < normalized.length(); i++) { | ||
| char c = normalized.charAt(i); | ||
| if (c == '.') { | ||
| continue; | ||
| } | ||
| if (c != '0') { | ||
| foundNonZero = true; | ||
| } | ||
| if (foundNonZero) { | ||
| count++; | ||
| } | ||
| } | ||
| return count; | ||
| } | ||
| } | ||
34 changes: 34 additions & 0 deletions
34
java-checks/src/test/java/org/sonar/java/checks/HardcodedMathConstantCheckTest.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,34 @@ | ||
| /* | ||
| * SonarQube Java | ||
| * Copyright (C) SonarSource Sàrl | ||
| * mailto:info AT sonarsource DOT com | ||
| * | ||
| * You can redistribute and/or modify this program under the terms of | ||
| * the Sonar Source-Available License Version 1, as published by SonarSource Sàrl. | ||
| * | ||
| * This program is distributed in the hope that it will be useful, | ||
| * but WITHOUT ANY WARRANTY; without even the implied warranty of | ||
| * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. | ||
| * See the Sonar Source-Available License for more details. | ||
| * | ||
| * You should have received a copy of the Sonar Source-Available License | ||
| * along with this program; if not, see https://sonarsource.com/license/ssal/ | ||
| */ | ||
| package org.sonar.java.checks; | ||
|
|
||
| import org.junit.jupiter.api.Test; | ||
| import org.sonar.java.checks.verifier.CheckVerifier; | ||
|
|
||
| import static org.sonar.java.checks.verifier.TestUtils.mainCodeSourcesPath; | ||
|
|
||
| class HardcodedMathConstantCheckTest { | ||
|
|
||
| @Test | ||
| void test() { | ||
| CheckVerifier.newVerifier() | ||
| .onFile(mainCodeSourcesPath("checks/HardcodedMathConstantCheckSample.java")) | ||
| .withCheck(new HardcodedMathConstantCheck()) | ||
| .verifyIssues(); | ||
| } | ||
|
|
||
| } |
51 changes: 51 additions & 0 deletions
51
sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S9133.html
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,51 @@ | ||
| <h2>Why is this an issue?</h2> | ||
| <p>Hard-coding approximate values of well-known mathematical constants creates several problems in your codebase.</p> | ||
| <p>First, these hard-coded values often lack the full precision available in predefined library constants. For example, writing <code>3.14</code> instead of | ||
| using a library-provided constant for π loses significant decimal places, potentially leading to calculation errors in scientific or engineering | ||
| applications.</p> | ||
| <p>Second, hard-coded values make your code less readable. When other developers see <code>3.14159</code>, they must recognize it as π themselves. | ||
| Using named constants from standard libraries makes the intent immediately clear.</p> | ||
| <p>Third, maintenance becomes harder. If you need to change the precision or update the value across your codebase, you must find and modify multiple | ||
| literal values. With named constants, the meaning is centralized and clear.</p> | ||
| <p>In Java, these constants and functions are available in the <code>Math</code> class: <code>Math.PI</code>, <code>Math.E</code>, | ||
| <code>Math.sqrt(2)</code>, and <code>Math.log(2)</code>.</p> | ||
| <h2>How to fix it</h2> | ||
| <p>Replace hard-coded numeric approximations with the appropriate predefined constant from the <code>Math</code> class. For π, use | ||
| <code>Math.PI</code>. For Euler's number, use <code>Math.E</code>. For other common constants that don't have direct equivalents, use mathematical | ||
| expressions like <code>Math.sqrt(2)</code> or <code>Math.log(2)</code>.</p> | ||
| <h3>Noncompliant code example</h3> | ||
| <pre> | ||
| public class CircleCalculator { | ||
| public double calculateArea(double radius) { | ||
| return 3.14 * radius * radius; // Noncompliant | ||
| } | ||
|
|
||
| public double calculateCircumference(double radius) { | ||
| return 2 * 3.14159 * radius; // Noncompliant | ||
| } | ||
| } | ||
| </pre> | ||
| <h3>Compliant solution</h3> | ||
| <pre> | ||
| public class CircleCalculator { | ||
| public double calculateArea(double radius) { | ||
| return Math.PI * radius * radius; | ||
| } | ||
|
|
||
| public double calculateCircumference(double radius) { | ||
| return 2 * Math.PI * radius; | ||
| } | ||
| } | ||
| </pre> | ||
| <h2>Resources</h2> | ||
| <h3>Documentation</h3> | ||
| <ul> | ||
| <li>Java Documentation - <a href="https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/lang/Math.html">Math (Java SE 17 & JDK | ||
| 17)</a></li> | ||
| <li>Java Documentation - <a href="https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/lang/StrictMath.html">StrictMath (Java SE 17 | ||
| & JDK 17)</a></li> | ||
| </ul> | ||
| <h3>Standards</h3> | ||
| <ul> | ||
| <li>CWE - <a href="https://cwe.mitre.org/data/definitions/1106.html">CWE-1106: Insufficient Use of Symbolic Constants</a></li> | ||
| </ul> |
28 changes: 28 additions & 0 deletions
28
sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S9133.json
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,28 @@ | ||
| { | ||
| "title": "Predefined mathematical constants should be used instead of hard-coded approximations", | ||
| "type": "CODE_SMELL", | ||
| "status": "ready", | ||
| "remediation": { | ||
| "func": "Constant\/Issue", | ||
| "constantCost": "2min" | ||
| }, | ||
| "tags": [ | ||
| "cwe" | ||
| ], | ||
| "defaultSeverity": "Major", | ||
| "ruleSpecification": "RSPEC-9133", | ||
| "sqKey": "S9133", | ||
| "scope": "All", | ||
| "quickfix": "unknown", | ||
| "code": { | ||
| "impacts": { | ||
| "MAINTAINABILITY": "MEDIUM" | ||
| }, | ||
| "attribute": "IDENTIFIABLE" | ||
| }, | ||
| "securityStandards": { | ||
| "CWE": [ | ||
| 1106 | ||
| ] | ||
| } | ||
| } |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.