diff --git a/its/ruling/src/test/resources/guava/java-S9354.json b/its/ruling/src/test/resources/guava/java-S9354.json new file mode 100644 index 00000000000..5bd8aea52dc --- /dev/null +++ b/its/ruling/src/test/resources/guava/java-S9354.json @@ -0,0 +1,40 @@ +{ +"com.google.guava:guava:src/com/google/common/collect/ExplicitOrdering.java": [ +41 +], +"com.google.guava:guava:src/com/google/common/primitives/Booleans.java": [ +297 +], +"com.google.guava:guava:src/com/google/common/primitives/Chars.java": [ +414 +], +"com.google.guava:guava:src/com/google/common/primitives/Doubles.java": [ +401 +], +"com.google.guava:guava:src/com/google/common/primitives/Floats.java": [ +397 +], +"com.google.guava:guava:src/com/google/common/primitives/Ints.java": [ +462 +], +"com.google.guava:guava:src/com/google/common/primitives/Longs.java": [ +498 +], +"com.google.guava:guava:src/com/google/common/primitives/Shorts.java": [ +461 +], +"com.google.guava:guava:src/com/google/common/primitives/SignedBytes.java": [ +202 +], +"com.google.guava:guava:src/com/google/common/primitives/UnsignedBytes.java": [ +409, +420, +436 +], +"com.google.guava:guava:src/com/google/common/primitives/UnsignedInts.java": [ +176 +], +"com.google.guava:guava:src/com/google/common/primitives/UnsignedLongs.java": [ +177 +] +} diff --git a/its/ruling/src/test/resources/sonar-server/java-S9354.json b/its/ruling/src/test/resources/sonar-server/java-S9354.json new file mode 100644 index 00000000000..81d1a692495 --- /dev/null +++ b/its/ruling/src/test/resources/sonar-server/java-S9354.json @@ -0,0 +1,8 @@ +{ +"org.sonarsource.sonarqube:sonar-server:src/main/java/org/sonar/server/computation/task/projectanalysis/duplication/Duplication.java": [ +119 +], +"org.sonarsource.sonarqube:sonar-server:src/main/java/org/sonar/server/computation/task/projectanalysis/duplication/TextBlock.java": [ +61 +] +} diff --git a/java-checks-test-sources/default/src/main/java/checks/IntegerSubtractionInComparisonCheckSample.java b/java-checks-test-sources/default/src/main/java/checks/IntegerSubtractionInComparisonCheckSample.java new file mode 100644 index 00000000000..dc7a7f46c3d --- /dev/null +++ b/java-checks-test-sources/default/src/main/java/checks/IntegerSubtractionInComparisonCheckSample.java @@ -0,0 +1,304 @@ +package checks; + +import java.io.File; +import java.util.Comparator; +import java.util.List; +import java.util.function.IntSupplier; + +class IntegerSubtractionInComparisonCheckSample { + + static class TimestampedEvent implements Comparable { + private long timestamp; + + @Override + public int compareTo(TimestampedEvent other) { + return (int) (this.timestamp - other.timestamp); // Noncompliant {{Subtracting numeric values in compareTo can overflow; use Long.compare instead.}} +// ^ + } + } + + static class IntHolder implements Comparable { + private int value; + + @Override + public int compareTo(IntHolder other) { + return this.value - other.value; // Noncompliant {{Subtracting numeric values in compareTo can overflow; use Integer.compare instead.}} + } + } + + static class BoxedIntHolder implements Comparable { + private Integer value; + + @Override + public int compareTo(BoxedIntHolder other) { + return this.value - other.value; // Noncompliant {{Subtracting numeric values in compareTo can overflow; use Integer.compare instead.}} + } + } + + static class BoxedLongHolder implements Comparable { + private Long value; + + @Override + public int compareTo(BoxedLongHolder other) { + return (int) (this.value - other.value); // Noncompliant {{Subtracting numeric values in compareTo can overflow; use Long.compare instead.}} + } + } + + static class MixedOperands implements Comparable { + private long longValue; + private int intValue; + + @Override + public int compareTo(MixedOperands other) { + return (int) (this.longValue - other.intValue); // Noncompliant {{Subtracting numeric values in compareTo can overflow; use Long.compare instead.}} + } + } + + static class IndexArithmetic implements Comparable { + private int[] parts; + + @Override + public int compareTo(IndexArithmetic other) { + for (int i = 0; i < parts.length - 1; i++) { // Compliant - subtraction is not the comparison result + int cmp = Integer.compare(this.parts[i], other.parts[i]); + if (cmp != 0) { + return cmp; + } + } + return 0; + } + } + + static class IntermediateDiff implements Comparable { + private int age; + + @Override + public int compareTo(IntermediateDiff other) { + int diff = this.age - other.age; // Compliant - only a returned subtraction is reported + if (diff != 0) { + return Integer.compare(this.age, other.age); + } + return 0; + } + } + + static class HashCodeCompare implements Comparable { + @Override + public int compareTo(HashCodeCompare other) { + return this.hashCode() - other.hashCode(); // Noncompliant {{Subtracting numeric values in compareTo can overflow; use Integer.compare instead.}} + } + } + + static class NonIntCastCompareTo implements Comparable { + private int value; + + @Override + public int compareTo(NonIntCastCompareTo other) { + return (int) (short) (this.value - other.value); // Compliant - the subtraction is wrapped in a non-int cast + } + } + + static class AgeComparator implements Comparator { + @Override + public int compare(IntHolder left, IntHolder right) { + return left.value - right.value; // Noncompliant {{Subtracting numeric values in compare can overflow; use Integer.compare instead.}} + } + } + + static class LongArrayComparator implements Comparator { + @Override + public int compare(long[] a, long[] b) { + return (int) (a[0] - b[0]); // Noncompliant {{Subtracting numeric values in compare can overflow; use Long.compare instead.}} + } + } + + static final Comparator COMPARATOR_UNBOXED_INT_CAST = new Comparator() { + @Override + public int compare(Number n1, Number n2) { + return (int) (n1.longValue() - n2.longValue()); // Noncompliant {{Subtracting numeric values in compare can overflow; use Long.compare instead.}} + } + }; + + static final Comparator COMPARATOR_BOXED_INT_CAST = new Comparator() { + @Override + public int compare(Long n1, Long n2) { + return (int) (n1 - n2); // Noncompliant {{Subtracting numeric values in compare can overflow; use Long.compare instead.}} + } + }; + + static final Comparator COMPARATOR_FILE_INT_CAST = new Comparator() { + @Override + public int compare(File lhs, File rhs) { + return (int) (rhs.lastModified() - lhs.lastModified()); // Noncompliant {{Subtracting numeric values in compare can overflow; use Long.compare instead.}} + } + }; + + void lambdaSubtraction(List list, List events) { + list.sort((a, b) -> a.value - b.value); // Noncompliant {{Subtracting numeric values in compare can overflow; use Integer.compare instead.}} + events.sort((left, right) -> (int) (left.timestamp - right.timestamp)); // Noncompliant {{Subtracting numeric values in compare can overflow; use Long.compare instead.}} + list.sort((a, b) -> { + return a.value - b.value; // Noncompliant {{Subtracting numeric values in compare can overflow; use Integer.compare instead.}} + }); + } + + static class CorrectLongCompareTo implements Comparable { + private long timestamp; + + @Override + public int compareTo(CorrectLongCompareTo other) { + return Long.compare(this.timestamp, other.timestamp); // Compliant + } + } + + static class CorrectIntCompareTo implements Comparable { + private int value; + + @Override + public int compareTo(CorrectIntCompareTo other) { + return Integer.compare(this.value, other.value); // Compliant + } + } + + static class CorrectBoxedLongCompareTo implements Comparable { + private Long value; + + @Override + public int compareTo(CorrectBoxedLongCompareTo other) { + return value.compareTo(other.value); // Compliant + } + } + + static class IntegerRelational implements Comparable { + private int value; + + @Override + public int compareTo(IntegerRelational other) { + if (this.value < other.value) { // Compliant + return -1; + } + return this.value > other.value ? 1 : 0; // Compliant + } + } + + static class FloatingPointCompareTo implements Comparable { + private double latitude; + + @Override + public int compareTo(FloatingPointCompareTo other) { + return (int) (this.latitude - other.latitude); // Compliant - handled by S9148 + } + } + + static class ByteCompareTo implements Comparable { + private byte value; + + @Override + public int compareTo(ByteCompareTo other) { + return this.value - other.value; // Compliant - difference fits in int + } + } + + static class ShortCompareTo implements Comparable { + private short value; + + @Override + public int compareTo(ShortCompareTo other) { + return this.value - other.value; // Compliant - difference fits in int + } + } + + static class CharCompareTo implements Comparable { + private char value; + + @Override + public int compareTo(CharCompareTo other) { + return this.value - other.value; // Compliant - difference fits in int + } + } + + static class BoxedShortCompareTo implements Comparable { + private Short value; + + @Override + public int compareTo(BoxedShortCompareTo other) { + return this.value - other.value; // Compliant - difference fits in int + } + } + + int subtract(int a, int b) { + return a - b; // Compliant - not in a comparison method + } + + static class DoubleUtils { + int compare(int a, int b) { + return a - b; // Compliant - not in a Comparator + } + } + + static class NonComparableTest { + private final long value = 0; + + public int compareTo(NonComparableTest other) { + return (int) (this.value - other.value); // Compliant - class is not Comparable + } + } + + static final Object COMPARATOR_LIKE_INT_CAST = new Object() { + public int compare(Long n1, Long n2) { + return (int) (n1 - n2); // Compliant - not a Comparator + } + }; + + static class LookAlikeMethods { + double compareTo(Object other) { + return other.hashCode() - 1; // Compliant - does not return an int + } + + int compareTo(Object a, Object b) { + return a.hashCode() - b.hashCode(); // Compliant - compareTo takes exactly one parameter + } + + int compare(IntHolder a) { + return a.value - 1; // Compliant - compare takes exactly two parameters + } + } + + static class NestedLambda implements Comparable { + private int value; + + @Override + public int compareTo(NestedLambda other) { + IntSupplier difference = () -> this.value - other.value; // Compliant - not a Comparator + return Integer.compare(difference.getAsInt(), 0); // Compliant + } + } + + static class LocalClassInCompareTo implements Comparable { + private int value; + + @Override + public int compareTo(LocalClassInCompareTo other) { + class Difference { + int between(int a, int b) { + return a - b; // Compliant - not in a comparison method + } + } + return Integer.compare(new Difference().between(this.value, other.value), 0); // Compliant + } + } + + interface CustomComparable { + int compareTo(T other); // Compliant - abstract method, no body + } + + abstract static class AbstractIntComparator implements Comparator { + @Override + public abstract int compare(Integer a, Integer b); // Compliant - abstract method, no body + } + + void lambdaCorrect(List list, List events) { + list.sort((a, b) -> Integer.compare(a.value, b.value)); // Compliant + events.sort((left, right) -> Long.compare(left.timestamp, right.timestamp)); // Compliant + } + +} diff --git a/java-checks/src/main/java/org/sonar/java/checks/FloatingPointComparisonCheck.java b/java-checks/src/main/java/org/sonar/java/checks/FloatingPointComparisonCheck.java index 236eee80506..2d597c3559e 100644 --- a/java-checks/src/main/java/org/sonar/java/checks/FloatingPointComparisonCheck.java +++ b/java-checks/src/main/java/org/sonar/java/checks/FloatingPointComparisonCheck.java @@ -16,18 +16,13 @@ */ package org.sonar.java.checks; -import java.util.Arrays; import java.util.List; import org.sonar.check.Rule; +import org.sonar.java.checks.helpers.ComparisonMethodUtils; 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.BaseTreeVisitor; import org.sonar.plugins.java.api.tree.BinaryExpressionTree; -import org.sonar.plugins.java.api.tree.ClassTree; import org.sonar.plugins.java.api.tree.ExpressionTree; -import org.sonar.plugins.java.api.tree.LambdaExpressionTree; -import org.sonar.plugins.java.api.tree.MethodTree; import org.sonar.plugins.java.api.tree.Tree; @Rule(key = "S9148") @@ -35,39 +30,16 @@ public class FloatingPointComparisonCheck extends IssuableSubscriptionVisitor { private static final String MESSAGE = "Use \"Double.compare\" or \"Float.compare\" to compare floating-point values."; - private static final MethodMatchers COMPARE_METHODS = MethodMatchers.or( - MethodMatchers.create() - .ofSubTypes("java.lang.Comparable") - .names("compareTo") - .addParametersMatcher(MethodMatchers.ANY) - .build(), - MethodMatchers.create() - .ofSubTypes("java.util.Comparator") - .names("compare") - .addParametersMatcher(MethodMatchers.ANY, MethodMatchers.ANY) - .build()); - @Override public List nodesToVisit() { - return Arrays.asList(Tree.Kind.METHOD, Tree.Kind.LAMBDA_EXPRESSION); + return ComparisonMethodUtils.nodesToVisit(); } @Override public void visitNode(Tree tree) { - if (context.getSemanticModel() == null) { - return; - } - if (tree.is(Tree.Kind.METHOD)) { - MethodTree methodTree = (MethodTree) tree; - if (COMPARE_METHODS.matches(methodTree) && methodTree.block() != null) { - methodTree.block().accept(new FloatingPointComparisonVisitor()); - } - } else { - LambdaExpressionTree lambda = (LambdaExpressionTree) tree; - if (lambda.symbolType().isSubtypeOf("java.util.Comparator")) { - lambda.body().accept(new FloatingPointComparisonVisitor()); - } - } + ComparisonMethodUtils.visitComparisonNode(context, tree, + methodTree -> methodTree.block().accept(new FloatingPointComparisonVisitor()), + lambda -> lambda.body().accept(new FloatingPointComparisonVisitor())); } private static boolean hasFloatingType(ExpressionTree tree) { @@ -75,7 +47,7 @@ private static boolean hasFloatingType(ExpressionTree tree) { || tree.symbolType().isPrimitive(Type.Primitives.DOUBLE); } - private class FloatingPointComparisonVisitor extends BaseTreeVisitor { + private class FloatingPointComparisonVisitor extends ComparisonMethodUtils.SkipNestedTypesVisitor { @Override public void visitBinaryExpression(BinaryExpressionTree tree) { @@ -90,15 +62,5 @@ && hasFloatingOperand(tree)) { private boolean hasFloatingOperand(BinaryExpressionTree tree) { return hasFloatingType(tree.leftOperand()) || hasFloatingType(tree.rightOperand()); } - - @Override - public void visitClass(ClassTree tree) { - // Do not visit inner classes - } - - @Override - public void visitLambdaExpression(LambdaExpressionTree tree) { - // Do not visit nested lambdas - } } } diff --git a/java-checks/src/main/java/org/sonar/java/checks/IntegerSubtractionInComparisonCheck.java b/java-checks/src/main/java/org/sonar/java/checks/IntegerSubtractionInComparisonCheck.java new file mode 100644 index 00000000000..951742668a8 --- /dev/null +++ b/java-checks/src/main/java/org/sonar/java/checks/IntegerSubtractionInComparisonCheck.java @@ -0,0 +1,132 @@ +/* + * 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.java.checks.helpers.ComparisonMethodUtils; +import org.sonar.java.model.ExpressionUtils; +import org.sonar.plugins.java.api.IssuableSubscriptionVisitor; +import org.sonar.plugins.java.api.semantic.Type; +import org.sonar.plugins.java.api.tree.BinaryExpressionTree; +import org.sonar.plugins.java.api.tree.ExpressionTree; +import org.sonar.plugins.java.api.tree.ReturnStatementTree; +import org.sonar.plugins.java.api.tree.Tree; +import org.sonar.plugins.java.api.tree.TypeCastTree; + +@Rule(key = "S9354") +public class IntegerSubtractionInComparisonCheck extends IssuableSubscriptionVisitor { + + private static final String MESSAGE = "Subtracting numeric values in %s can overflow; use %s instead."; + + @Override + public List nodesToVisit() { + return ComparisonMethodUtils.nodesToVisit(); + } + + @Override + public void visitNode(Tree tree) { + ComparisonMethodUtils.visitComparisonNode(context, tree, + methodTree -> methodTree.block().accept(new ComparisonResultVisitor(methodTree.simpleName().name())), + lambda -> { + Tree body = lambda.body(); + ComparisonResultVisitor visitor = new ComparisonResultVisitor("compare"); + if (body.is(Tree.Kind.BLOCK)) { + body.accept(visitor); + } else { + visitor.checkComparisonResult((ExpressionTree) body); + } + }); + } + + private class ComparisonResultVisitor extends ComparisonMethodUtils.SkipNestedTypesVisitor { + + private final String enclosingMethodName; + + private ComparisonResultVisitor(String enclosingMethodName) { + this.enclosingMethodName = enclosingMethodName; + } + + @Override + public void visitReturnStatement(ReturnStatementTree tree) { + ExpressionTree expression = tree.expression(); + if (expression != null) { + checkComparisonResult(expression); + } + } + + private void checkComparisonResult(ExpressionTree expression) { + ExpressionTree unwrapped = skipParenthesesAndIntCasts(expression); + if (!unwrapped.is(Tree.Kind.MINUS)) { + return; + } + String replacement = replacementFor((BinaryExpressionTree) unwrapped); + if (replacement != null) { + reportIssue(((BinaryExpressionTree) unwrapped).operatorToken(), String.format(MESSAGE, enclosingMethodName, replacement)); + } + } + } + + private static ExpressionTree skipParenthesesAndIntCasts(ExpressionTree expression) { + ExpressionTree current = ExpressionUtils.skipParentheses(expression); + while (current.is(Tree.Kind.TYPE_CAST)) { + TypeCastTree cast = (TypeCastTree) current; + if (!cast.type().symbolType().isPrimitive(Type.Primitives.INT)) { + return current; + } + current = ExpressionUtils.skipParentheses(cast.expression()); + } + return current; + } + + private static String replacementFor(BinaryExpressionTree tree) { + Type left = primitiveOrSelf(tree.leftOperand()); + Type right = primitiveOrSelf(tree.rightOperand()); + if (left.isUnknown() || right.isUnknown() || isFloating(left) || isFloating(right)) { + return null; + } + if (isLong(left) || isLong(right)) { + return "Long.compare"; + } + if (isInt(left) || isInt(right)) { + return "Integer.compare"; + } + return null; + } + + private static Type primitiveOrSelf(ExpressionTree tree) { + Type type = tree.symbolType(); + if (type.isPrimitive()) { + return type; + } + Type primitive = type.primitiveType(); + return primitive != null ? primitive : type; + } + + private static boolean isFloating(Type type) { + return type.isPrimitive(Type.Primitives.FLOAT) || type.isPrimitive(Type.Primitives.DOUBLE); + } + + private static boolean isLong(Type type) { + return type.isPrimitive(Type.Primitives.LONG); + } + + private static boolean isInt(Type type) { + return type.isPrimitive(Type.Primitives.INT); + } + +} diff --git a/java-checks/src/main/java/org/sonar/java/checks/helpers/ComparisonMethodUtils.java b/java-checks/src/main/java/org/sonar/java/checks/helpers/ComparisonMethodUtils.java new file mode 100644 index 00000000000..4716de263e2 --- /dev/null +++ b/java-checks/src/main/java/org/sonar/java/checks/helpers/ComparisonMethodUtils.java @@ -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.helpers; + +import java.util.Arrays; +import java.util.List; +import java.util.function.Consumer; +import org.sonar.plugins.java.api.JavaFileScannerContext; +import org.sonar.plugins.java.api.semantic.MethodMatchers; +import org.sonar.plugins.java.api.tree.BaseTreeVisitor; +import org.sonar.plugins.java.api.tree.ClassTree; +import org.sonar.plugins.java.api.tree.LambdaExpressionTree; +import org.sonar.plugins.java.api.tree.MethodTree; +import org.sonar.plugins.java.api.tree.Tree; + +public final class ComparisonMethodUtils { + + private static final MethodMatchers COMPARE_METHODS = MethodMatchers.or( + MethodMatchers.create() + .ofSubTypes("java.lang.Comparable") + .names("compareTo") + .addParametersMatcher(MethodMatchers.ANY) + .build(), + MethodMatchers.create() + .ofSubTypes("java.util.Comparator") + .names("compare") + .addParametersMatcher(MethodMatchers.ANY, MethodMatchers.ANY) + .build()); + + private ComparisonMethodUtils() { + } + + public static List nodesToVisit() { + return Arrays.asList(Tree.Kind.METHOD, Tree.Kind.LAMBDA_EXPRESSION); + } + + public static void visitComparisonNode(JavaFileScannerContext context, Tree tree, + Consumer onCompareMethod, Consumer onComparatorLambda) { + if (context.getSemanticModel() == null) { + return; + } + if (tree.is(Tree.Kind.METHOD)) { + MethodTree methodTree = (MethodTree) tree; + if (isCompareMethod(methodTree)) { + onCompareMethod.accept(methodTree); + } + } else { + LambdaExpressionTree lambda = (LambdaExpressionTree) tree; + if (isComparatorLambda(lambda)) { + onComparatorLambda.accept(lambda); + } + } + } + + public static boolean isCompareMethod(MethodTree methodTree) { + return methodTree.block() != null && COMPARE_METHODS.matches(methodTree); + } + + public static boolean isComparatorLambda(LambdaExpressionTree lambda) { + return lambda.symbolType().isSubtypeOf("java.util.Comparator"); + } + + public static class SkipNestedTypesVisitor extends BaseTreeVisitor { + @Override + public void visitClass(ClassTree tree) { + // Do not visit inner classes + } + + @Override + public void visitLambdaExpression(LambdaExpressionTree tree) { + // Do not visit nested lambdas + } + } +} diff --git a/java-checks/src/test/java/org/sonar/java/checks/IntegerSubtractionInComparisonCheckTest.java b/java-checks/src/test/java/org/sonar/java/checks/IntegerSubtractionInComparisonCheckTest.java new file mode 100644 index 00000000000..d11b3e54905 --- /dev/null +++ b/java-checks/src/test/java/org/sonar/java/checks/IntegerSubtractionInComparisonCheckTest.java @@ -0,0 +1,43 @@ +/* + * 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 IntegerSubtractionInComparisonCheckTest { + + @Test + void test() { + CheckVerifier.newVerifier() + .onFile(mainCodeSourcesPath("checks/IntegerSubtractionInComparisonCheckSample.java")) + .withCheck(new IntegerSubtractionInComparisonCheck()) + .verifyIssues(); + } + + @Test + void test_without_semantic() { + CheckVerifier.newVerifier() + .onFile(mainCodeSourcesPath("checks/IntegerSubtractionInComparisonCheckSample.java")) + .withCheck(new IntegerSubtractionInComparisonCheck()) + .withoutSemantic() + .verifyNoIssues(); + } + +} diff --git a/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S9354.html b/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S9354.html new file mode 100644 index 00000000000..9bc09eb5407 --- /dev/null +++ b/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S9354.html @@ -0,0 +1,104 @@ +

This rule raises an issue when compareTo() or compare() orders values by subtracting integers. Integer subtraction can +overflow and reverse the comparison result.

+

Why is this an issue?

+

Comparable.compareTo() and Comparator.compare() must return a negative integer, zero, or a positive integer as the first +value is less than, equal to, or greater than the second. The magnitude of that result does not matter, only its sign.

+

A common shortcut is to return the difference of two numeric fields:

+
+return this.value - other.value;
+
+

Java integer operators wrap on overflow instead of throwing. When the true difference is larger than Integer.MAX_VALUE or smaller than +Integer.MIN_VALUE, the wrapped result can have the wrong sign. For example, (int) (1_000_000_000L - (-1_500_000_000L)) is +negative even though the first value is greater.

+

The same problem appears when a long difference is narrowed to int:

+
+return (int) (this.timestamp - other.timestamp);
+
+

That broken ordering violates the comparison contract. Sorting with Arrays.sort() or Collections.sort() can throw +IllegalArgumentException: Comparison method violates its general contract!. Ordered collections such as TreeSet and +TreeMap can become corrupted.

+

Use Integer.compare() for int values and Long.compare() for long values. Those methods compare +without computing a difference that can overflow.

+

This rule does not flag floating-point subtraction in ordering methods. See {rule:java:S9148}.

+

How to fix it

+

Replace the subtraction with Integer.compare() or Long.compare(), matching the operand type.

+

Code examples

+

Noncompliant code example

+
+class TimestampedEvent implements Comparable<TimestampedEvent> {
+  private long timestamp;
+
+  @Override
+  public int compareTo(TimestampedEvent other) {
+    return (int) (this.timestamp - other.timestamp); // Noncompliant
+  }
+}
+
+

Compliant solution

+
+class TimestampedEvent implements Comparable<TimestampedEvent> {
+  private long timestamp;
+
+  @Override
+  public int compareTo(TimestampedEvent other) {
+    return Long.compare(this.timestamp, other.timestamp);
+  }
+}
+
+

Noncompliant code example

+
+import java.util.Comparator;
+
+class AgeComparator implements Comparator<Person> {
+  @Override
+  public int compare(Person left, Person right) {
+    return left.age - right.age; // Noncompliant
+  }
+}
+
+

Compliant solution

+
+import java.util.Comparator;
+
+class AgeComparator implements Comparator<Person> {
+  @Override
+  public int compare(Person left, Person right) {
+    return Integer.compare(left.age, right.age);
+  }
+}
+
+

Noncompliant code example

+
+import java.util.List;
+
+void sortById(List<Event> events) {
+  events.sort((left, right) -> (int) (left.id - right.id)); // Noncompliant
+}
+
+

Compliant solution

+
+import java.util.List;
+
+void sortById(List<Event> events) {
+  events.sort((left, right) -> Long.compare(left.id, right.id));
+}
+
+

Resources

+

Documentation

+ +

Related rules

+
    +
  • {rule:java:S9148} - "Float.compare" or "Double.compare" should be used for floating-point comparisons
    • +
+ diff --git a/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S9354.json b/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S9354.json new file mode 100644 index 00000000000..08a41d76adf --- /dev/null +++ b/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S9354.json @@ -0,0 +1,23 @@ +{ + "title": "\"Comparable.compareTo()\" and \"Comparator.compare()\" should not use subtraction on numerical fields", + "type": "BUG", + "status": "ready", + "remediation": { + "func": "Constant\/Issue", + "constantCost": "5min" + }, + "tags": [ + "pitfall" + ], + "defaultSeverity": "Major", + "ruleSpecification": "RSPEC-9354", + "sqKey": "S9354", + "scope": "All", + "quickfix": "targeted", + "code": { + "impacts": { + "RELIABILITY": "MEDIUM" + }, + "attribute": "LOGICAL" + } +} diff --git a/sonar-java-plugin/src/main/resources/profiles/Sonar_way/S9354 b/sonar-java-plugin/src/main/resources/profiles/Sonar_way/S9354 new file mode 100644 index 00000000000..e69de29bb2d