-
Notifications
You must be signed in to change notification settings - Fork 724
SONARJAVA-6787 Implement new rule S9351: "BigDecimal.compareTo()" should be used instead of "equals()" for numerical comparison #5945
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
base: master
Are you sure you want to change the base?
Changes from all commits
4aee5ef
1fe33ca
106f248
c520554
e5cbc34
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,34 @@ | ||
| package checks; | ||
|
|
||
| import java.math.BigDecimal; | ||
|
|
||
| class BigDecimalEqualsCheckGuavaSample { | ||
|
|
||
| void method(BigDecimal a, BigDecimal b, Object o, String s) { | ||
| boolean res; | ||
|
|
||
| res = com.google.common.base.Objects.equal(a, b); // Noncompliant [["BigDecimal.equals()" compares scale as well as value; use "compareTo() == 0" for numerical comparison.]] | ||
| // ^^^^^ | ||
| res = com.google.common.base.Objects.equal(a, o); // Noncompliant | ||
| res = com.google.common.base.Objects.equal(o, a); // Noncompliant | ||
|
|
||
| // Compliant | ||
| res = com.google.common.base.Objects.equal(s, "hello"); | ||
| } | ||
|
|
||
| static class AccountWithGuavaEquals { | ||
| private BigDecimal balance; | ||
|
|
||
| @Override | ||
| public boolean equals(Object obj) { | ||
| if (this == obj) return true; | ||
| if (!(obj instanceof AccountWithGuavaEquals other)) return false; | ||
| return com.google.common.base.Objects.equal(balance, other.balance); // Compliant: inside equals method override | ||
| } | ||
|
|
||
| @Override | ||
| public int hashCode() { | ||
| return com.google.common.base.Objects.hashCode(balance); | ||
| } | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,72 @@ | ||
| package checks; | ||
|
|
||
| import java.math.BigDecimal; | ||
| import java.util.Objects; | ||
|
|
||
| class BigDecimalEqualsCheckSample { | ||
|
|
||
| void method(BigDecimal a, BigDecimal b, Object o, String s) { | ||
| boolean res; | ||
|
|
||
| res = a.equals(b); // Noncompliant [["BigDecimal.equals()" compares scale as well as value; use "compareTo() == 0" for numerical comparison.]] | ||
| // ^^^^^^ | ||
| res = !a.equals(b); // Noncompliant | ||
| // ^^^^^^ | ||
| res = a.equals(o); // Noncompliant | ||
|
|
||
| res = Objects.equals(a, b); // Noncompliant | ||
| // ^^^^^^ | ||
| res = Objects.equals(a, o); // Noncompliant | ||
| res = Objects.equals(o, a); // Noncompliant | ||
|
|
||
| // Compliant | ||
| res = a.compareTo(b) == 0; | ||
| res = a.compareTo(b) != 0; | ||
| res = o.equals(a); | ||
| res = s.equals(a); | ||
| res = s.equals("hello"); | ||
| res = Objects.equals(s, "hello"); | ||
| } | ||
|
|
||
| static class Account { | ||
| private BigDecimal balance; | ||
|
|
||
| @Override | ||
| public boolean equals(Object obj) { | ||
| if (this == obj) return true; | ||
| if (!(obj instanceof Account other)) return false; | ||
| return balance != null && balance.equals(other.balance); // Compliant: inside equals method override | ||
| } | ||
|
|
||
| @Override | ||
| public int hashCode() { | ||
| return Objects.hashCode(balance); | ||
| } | ||
| } | ||
|
|
||
| static class AccountWithStaticEquals { | ||
| private BigDecimal balance; | ||
|
|
||
| @Override | ||
| public boolean equals(Object obj) { | ||
| if (this == obj) return true; | ||
| if (!(obj instanceof AccountWithStaticEquals other)) return false; | ||
| return Objects.equals(balance, other.balance); // Compliant: inside equals method override | ||
| } | ||
|
|
||
| @Override | ||
| public int hashCode() { | ||
| return Objects.hashCode(balance); | ||
| } | ||
| } | ||
|
|
||
| static class MyBigDecimal extends BigDecimal { | ||
| public MyBigDecimal(String val) { | ||
| super(val); | ||
| } | ||
|
|
||
| void testCustom(MyBigDecimal other) { | ||
| boolean r = this.equals(other); // Noncompliant | ||
| } | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,88 @@ | ||
| /* | ||
| * 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.Collections; | ||
| import java.util.List; | ||
| import org.sonar.check.Rule; | ||
| import org.sonar.java.checks.helpers.MethodTreeUtils; | ||
| import org.sonar.java.model.ExpressionUtils; | ||
| import org.sonar.plugins.java.api.IssuableSubscriptionVisitor; | ||
| import org.sonar.plugins.java.api.semantic.MethodMatchers; | ||
| import org.sonar.plugins.java.api.semantic.Type; | ||
| import org.sonar.plugins.java.api.tree.Arguments; | ||
| import org.sonar.plugins.java.api.tree.MethodInvocationTree; | ||
| import org.sonar.plugins.java.api.tree.MethodTree; | ||
| import org.sonar.plugins.java.api.tree.Tree; | ||
|
|
||
| @Rule(key = "S9351") | ||
| public class BigDecimalEqualsCheck extends IssuableSubscriptionVisitor { | ||
|
|
||
| private static final String MESSAGE = "\"BigDecimal.equals()\" compares scale as well as value; use \"compareTo() == 0\" for numerical comparison."; | ||
| private static final String BIG_DECIMAL = "java.math.BigDecimal"; | ||
| private static final String JAVA_LANG_OBJECT = "java.lang.Object"; | ||
|
|
||
| private static final MethodMatchers INSTANCE_EQUALS = MethodMatchers.create() | ||
| .ofSubTypes(BIG_DECIMAL) | ||
| .names("equals") | ||
| .addParametersMatcher(JAVA_LANG_OBJECT) | ||
| .build(); | ||
|
|
||
| private static final MethodMatchers STATIC_EQUALS = MethodMatchers.create() | ||
| .ofTypes("java.util.Objects", "com.google.common.base.Objects") | ||
| .names("equals", "equal") | ||
| .addParametersMatcher(JAVA_LANG_OBJECT, JAVA_LANG_OBJECT) | ||
| .build(); | ||
|
|
||
| @Override | ||
| public List<Tree.Kind> nodesToVisit() { | ||
| return Collections.singletonList(Tree.Kind.METHOD_INVOCATION); | ||
| } | ||
|
|
||
| @Override | ||
| public void visitNode(Tree tree) { | ||
| MethodInvocationTree mit = (MethodInvocationTree) tree; | ||
| if (isInsideEqualsMethod(mit)) { | ||
| return; | ||
| } | ||
| if (INSTANCE_EQUALS.matches(mit)) { | ||
| reportIssue(ExpressionUtils.methodName(mit), MESSAGE); | ||
| } else if (STATIC_EQUALS.matches(mit)) { | ||
| Arguments arguments = mit.arguments(); | ||
| Type firstType = arguments.get(0).symbolType(); | ||
| Type secondType = arguments.get(1).symbolType(); | ||
| if (isBigDecimal(firstType) || isBigDecimal(secondType)) { | ||
| reportIssue(ExpressionUtils.methodName(mit), MESSAGE); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| private static boolean isBigDecimal(Type type) { | ||
| return !type.isUnknown() && type.isSubtypeOf(BIG_DECIMAL); | ||
| } | ||
|
|
||
| private static boolean isInsideEqualsMethod(Tree tree) { | ||
| Tree parent = tree.parent(); | ||
| while (parent != null && !parent.is(Tree.Kind.CLASS, Tree.Kind.RECORD, Tree.Kind.INTERFACE, Tree.Kind.ENUM)) { | ||
| if (parent.is(Tree.Kind.METHOD)) { | ||
| return MethodTreeUtils.isEqualsMethod((MethodTree) parent); | ||
| } | ||
| parent = parent.parent(); | ||
| } | ||
| return false; | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,59 @@ | ||
| /* | ||
| * 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 BigDecimalEqualsCheckTest { | ||
|
|
||
| @Test | ||
| void test() { | ||
| CheckVerifier.newVerifier() | ||
| .onFile(mainCodeSourcesPath("checks/BigDecimalEqualsCheckSample.java")) | ||
| .withCheck(new BigDecimalEqualsCheck()) | ||
| .verifyIssues(); | ||
| } | ||
|
|
||
| @Test | ||
| void test_without_semantic() { | ||
| CheckVerifier.newVerifier() | ||
| .onFile(mainCodeSourcesPath("checks/BigDecimalEqualsCheckSample.java")) | ||
| .withCheck(new BigDecimalEqualsCheck()) | ||
| .withoutSemantic() | ||
| .verifyIssues(); | ||
| } | ||
|
Comment on lines
+34
to
+41
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
|
|
||
| @Test | ||
| void test_guava() { | ||
| CheckVerifier.newVerifier() | ||
| .onFile(mainCodeSourcesPath("checks/BigDecimalEqualsCheckGuavaSample.java")) | ||
| .withCheck(new BigDecimalEqualsCheck()) | ||
| .verifyIssues(); | ||
| } | ||
|
|
||
| @Test | ||
| void test_guava_without_semantic() { | ||
| CheckVerifier.newVerifier() | ||
| .onFile(mainCodeSourcesPath("checks/BigDecimalEqualsCheckGuavaSample.java")) | ||
| .withCheck(new BigDecimalEqualsCheck()) | ||
| .withoutSemantic() | ||
| .verifyNoIssues(); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,42 @@ | ||
| <p><code>BigDecimal.equals(Object)</code> compares both the numerical value and the scale of the <code>BigDecimal</code> objects.</p> | ||
| <h2>Why is this an issue?</h2> | ||
| <p>In Java, <code>BigDecimal.equals(Object)</code> returns <code>true</code> only if two <code>BigDecimal</code> objects have the same numerical value | ||
| and the same scale (number of digits to the right of the decimal point). Consequently, <code>new BigDecimal("2.0").equals(new | ||
| BigDecimal("2.00"))</code> evaluates to <code>false</code> even though both instances represent the same numerical value.</p> | ||
| <p>In financial, commerce, and scientific applications, comparisons usually intend to verify numerical equivalence regardless of representation | ||
| differences. Using <code>equals()</code> or <code>Objects.equals()</code> can introduce subtle bugs when values are formatted, serialized, or computed | ||
| with different scale factors. To compare <code>BigDecimal</code> values for numerical equality, use <code>compareTo(other) == 0</code> instead.</p> | ||
| <h3>Code examples</h3> | ||
| <h4>Noncompliant code example</h4> | ||
| <pre data-diff-id="1" data-diff-type="noncompliant"> | ||
| BigDecimal priceA = new BigDecimal("10.0"); | ||
| BigDecimal priceB = new BigDecimal("10.00"); | ||
|
|
||
| if (priceA.equals(priceB)) { // Noncompliant: "BigDecimal.equals()" compares scale as well as value; use "compareTo() == 0" for numerical comparison | ||
| applyDiscount(); | ||
| } | ||
| </pre> | ||
| <h4>Compliant solution</h4> | ||
| <pre data-diff-id="1" data-diff-type="compliant"> | ||
| BigDecimal priceA = new BigDecimal("10.0"); | ||
| BigDecimal priceB = new BigDecimal("10.00"); | ||
|
|
||
| if (priceA.compareTo(priceB) == 0) { | ||
| applyDiscount(); | ||
| } | ||
| </pre> | ||
| <h2>Exceptions</h2> | ||
| <p>This rule ignores <code>BigDecimal.equals()</code> calls inside <code>equals(Object)</code> method declarations. Classes implementing | ||
| <code>equals(Object)</code> and <code>hashCode()</code> often require scale-sensitive comparison to satisfy the <code>hashCode</code> contract.</p> | ||
| <p>If scale-sensitive equality is intentionally desired in other contexts, suppress the issue with an inline <code>// NOSONAR</code> comment and | ||
| provide a short rationale explaining why scale sensitivity is necessary (e.g., <code>// NOSONAR: scale-sensitive comparison is | ||
| intentional</code>).</p> | ||
| <h2>Resources</h2> | ||
| <h3>Documentation</h3> | ||
| <ul> | ||
| <li>Oracle Documentation - <a | ||
| href="https://docs.oracle.com/en/java/javase/21/docs/api/java.base/java/math/BigDecimal.html#equals(java.lang.Object)">BigDecimal.equals(Object)</a></li> | ||
| <li>Oracle Documentation - <a | ||
| href="https://docs.oracle.com/en/java/javase/21/docs/api/java.base/java/math/BigDecimal.html#compareTo(java.math.BigDecimal)">BigDecimal.compareTo(BigDecimal)</a></li> | ||
| </ul> | ||
|
|
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,24 @@ | ||
| { | ||
| "title": "\"BigDecimal.compareTo()\" should be used instead of \"equals()\" for numerical comparison", | ||
| "type": "BUG", | ||
| "status": "ready", | ||
| "remediation": { | ||
| "func": "Constant\/Issue", | ||
| "constantCost": "5min" | ||
| }, | ||
| "tags": [ | ||
| "unpredictable", | ||
| "bad-practice" | ||
| ], | ||
| "defaultSeverity": "Major", | ||
| "ruleSpecification": "RSPEC-9351", | ||
| "sqKey": "S9351", | ||
| "scope": "Main", | ||
| "quickfix": "infeasible", | ||
| "code": { | ||
| "impacts": { | ||
| "RELIABILITY": "HIGH" | ||
| }, | ||
| "attribute": "LOGICAL" | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we need a test
withoutSemantic