From ad8997c41d5e2c287541aeb1a04a5da8cd4ec1c8 Mon Sep 17 00:00:00 2001 From: nathsou Date: Mon, 24 Aug 2026 11:31:18 +0200 Subject: [PATCH 1/5] SONARJAVA-6838 Fix FP on S9354: ignore subtraction of operands with a provably bounded range --- ...gerSubtractionInComparisonCheckSample.java | 96 ++++++++++ .../IntegerSubtractionInComparisonCheck.java | 6 + .../checks/helpers/BoundedIntegerRange.java | 176 ++++++++++++++++++ 3 files changed, 278 insertions(+) create mode 100644 java-checks/src/main/java/org/sonar/java/checks/helpers/BoundedIntegerRange.java 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 index dc7a7f46c3d..b49d946a905 100644 --- a/java-checks-test-sources/default/src/main/java/checks/IntegerSubtractionInComparisonCheckSample.java +++ b/java-checks-test-sources/default/src/main/java/checks/IntegerSubtractionInComparisonCheckSample.java @@ -3,6 +3,7 @@ import java.io.File; import java.util.Comparator; import java.util.List; +import java.util.Map; import java.util.function.IntSupplier; class IntegerSubtractionInComparisonCheckSample { @@ -225,6 +226,101 @@ public int compareTo(BoxedShortCompareTo other) { } } + static class ArrayLengthComparator implements Comparator { + @Override + public int compare(int[] left, int[] right) { + return left.length - right.length; // Compliant - both operands are array lengths, bounded in [0, Integer.MAX_VALUE] + } + } + + static class StringLengthComparator implements Comparator { + @Override + public int compare(String left, String right) { + return left.length() - right.length(); // Compliant - both operands are String.length(), bounded in [0, Integer.MAX_VALUE] + } + } + + static class CollectionSizeComparator implements Comparator> { + @Override + public int compare(List left, List right) { + return left.size() - right.size(); // Compliant - both operands are Collection.size(), bounded in [0, Integer.MAX_VALUE] + } + } + + static class MapSizeComparator implements Comparator> { + @Override + public int compare(Map left, Map right) { + return left.size() - right.size(); // Compliant - both operands are Map.size(), bounded in [0, Integer.MAX_VALUE] + } + } + + enum Suit { + CLUBS, DIAMONDS, HEARTS, SPADES + } + + static class OrdinalComparator implements Comparator { + @Override + public int compare(Suit left, Suit right) { + return left.ordinal() - right.ordinal(); // Compliant - both operands are Enum.ordinal(), bounded in [0, Integer.MAX_VALUE] + } + } + + static class BitCountComparator implements Comparator { + @Override + public int compare(Integer left, Integer right) { + return Integer.bitCount(left) - Integer.bitCount(right); // Compliant - both operands are bounded in [0, 32] + } + } + + static class MaskedByteComparator implements Comparator { + @Override + public int compare(byte[] left, byte[] right) { + int a = left[0] & 0xff; + int b = right[0] & 0xff; + return a - b; // Compliant - both operands are masked to [0, 255] + } + } + + static class DirectMaskedByteComparator implements Comparator { + @Override + public int compare(byte[] left, byte[] right) { + return (left[0] & 0xff) - (right[0] & 0xff); // Compliant - both operands are masked to [0, 255] + } + } + + static class IndirectLengthComparator implements Comparator { + @Override + public int compare(String left, String right) { + int len1 = left.length(); + int len2 = right.length(); + return len1 - len2; // Compliant - len1 and len2 are single-write locals holding String.length() + } + } + + static class LiteralConstantComparator implements Comparator { + @Override + public int compare(Object left, Object right) { + return 0 - 1; // Compliant - both operands are compile-time constants + } + } + + static class MixedBoundedAndUnboundedComparator implements Comparable { + private int value; + + @Override + public int compareTo(MixedBoundedAndUnboundedComparator other) { + // Noncompliant@+1 {{Subtracting numeric values in compareTo can overflow; use Integer.compare instead.}} + return this.value - other.getClass().getName().length(); + } + } + + static class UnknownRangeComparator implements Comparable { + @Override + public int compareTo(UnknownRangeComparator other) { + return this.hashCode() - other.hashCode(); // Noncompliant {{Subtracting numeric values in compareTo can overflow; use Integer.compare instead.}} + } + } + int subtract(int a, int b) { return a - b; // Compliant - not in a comparison method } 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 index 951742668a8..86eefdb8692 100644 --- a/java-checks/src/main/java/org/sonar/java/checks/IntegerSubtractionInComparisonCheck.java +++ b/java-checks/src/main/java/org/sonar/java/checks/IntegerSubtractionInComparisonCheck.java @@ -18,6 +18,7 @@ import java.util.List; import org.sonar.check.Rule; +import org.sonar.java.checks.helpers.BoundedIntegerRange; import org.sonar.java.checks.helpers.ComparisonMethodUtils; import org.sonar.java.model.ExpressionUtils; import org.sonar.plugins.java.api.IssuableSubscriptionVisitor; @@ -103,6 +104,11 @@ private static String replacementFor(BinaryExpressionTree tree) { return "Long.compare"; } if (isInt(left) || isInt(right)) { + if (BoundedIntegerRange.subtractionCannotOverflow(tree.leftOperand(), tree.rightOperand())) { + // Both operands have a provable range whose difference is guaranteed to fit in an int, + // e.g. lengths, sizes, ordinals, or masked values: no overflow is possible. + return null; + } return "Integer.compare"; } return null; diff --git a/java-checks/src/main/java/org/sonar/java/checks/helpers/BoundedIntegerRange.java b/java-checks/src/main/java/org/sonar/java/checks/helpers/BoundedIntegerRange.java new file mode 100644 index 00000000000..e1da997f5fa --- /dev/null +++ b/java-checks/src/main/java/org/sonar/java/checks/helpers/BoundedIntegerRange.java @@ -0,0 +1,176 @@ +/* + * 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 javax.annotation.CheckForNull; +import org.sonar.java.model.ExpressionUtils; +import org.sonar.java.model.LiteralUtils; +import org.sonar.plugins.java.api.semantic.MethodMatchers; +import org.sonar.plugins.java.api.semantic.Symbol; +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.IdentifierTree; +import org.sonar.plugins.java.api.tree.MemberSelectExpressionTree; +import org.sonar.plugins.java.api.tree.MethodInvocationTree; +import org.sonar.plugins.java.api.tree.Tree; + +/** + * Computes a best-effort [lo, hi] bound for an integer-typed expression, used to prove that + * subtracting two such expressions cannot produce a 32-bit overflow. + *

+ * This is intentionally conservative: it only recognizes a small set of JDK APIs and operations + * with a documented or structurally guaranteed range. Any expression it cannot classify is + * treated as unbounded, which keeps every unrecognized case reported as before. + */ +public final class BoundedIntegerRange { + + private static final int MAX_SINGLE_WRITE_DEPTH = 3; + + private static final Range NON_NEGATIVE_INT = new Range(0, Integer.MAX_VALUE); + + private static final MethodMatchers CHAR_SEQUENCE_LENGTH = MethodMatchers.create() + .ofSubTypes("java.lang.CharSequence").names("length").addWithoutParametersMatcher().build(); + + private static final MethodMatchers COLLECTION_SIZE = MethodMatchers.create() + .ofSubTypes("java.util.Collection").names("size").addWithoutParametersMatcher().build(); + + private static final MethodMatchers MAP_SIZE = MethodMatchers.create() + .ofSubTypes("java.util.Map").names("size").addWithoutParametersMatcher().build(); + + private static final MethodMatchers ENUM_ORDINAL = MethodMatchers.create() + .ofSubTypes("java.lang.Enum").names("ordinal").addWithoutParametersMatcher().build(); + + private static final MethodMatchers BIT_COUNT_32 = MethodMatchers.create() + .ofTypes("java.lang.Integer").names("bitCount", "numberOfLeadingZeros", "numberOfTrailingZeros").addParametersMatcher("int").build(); + + private static final MethodMatchers BIT_COUNT_64 = MethodMatchers.create() + .ofTypes("java.lang.Long").names("bitCount", "numberOfLeadingZeros", "numberOfTrailingZeros").addParametersMatcher("long").build(); + + private BoundedIntegerRange() { + } + + /** + * @return true when both operands have a provable range whose difference is guaranteed to fit in an int, + * i.e. cannot overflow regardless of the actual runtime values. + */ + public static boolean subtractionCannotOverflow(ExpressionTree left, ExpressionTree right) { + Range leftRange = rangeOf(left, 0); + if (leftRange == null) { + return false; + } + Range rightRange = rangeOf(right, 0); + return rightRange != null && leftRange.fitsIntSubtraction(rightRange); + } + + @CheckForNull + private static Range rangeOf(ExpressionTree expression, int depth) { + if (depth > MAX_SINGLE_WRITE_DEPTH) { + return null; + } + ExpressionTree tree = ExpressionUtils.skipParentheses(expression); + Range constant = constantRange(tree); + if (constant != null) { + return constant; + } + if (tree.is(Tree.Kind.MEMBER_SELECT)) { + return arrayLengthRange((MemberSelectExpressionTree) tree); + } + if (tree.is(Tree.Kind.METHOD_INVOCATION)) { + return methodInvocationRange((MethodInvocationTree) tree); + } + if (tree.is(Tree.Kind.AND)) { + return bitwiseAndRange((BinaryExpressionTree) tree, depth); + } + if (tree.is(Tree.Kind.IDENTIFIER)) { + return identifierRange((IdentifierTree) tree, depth); + } + return null; + } + + @CheckForNull + private static Range constantRange(ExpressionTree tree) { + Long value = LiteralUtils.longLiteralValue(tree); + if (value == null) { + return null; + } + return new Range(value, value); + } + + @CheckForNull + private static Range arrayLengthRange(MemberSelectExpressionTree memberSelect) { + Type ownerType = memberSelect.expression().symbolType(); + String memberName = memberSelect.identifier().name(); + if (ownerType.isArray() && "length".equals(memberName)) { + return NON_NEGATIVE_INT; + } + return null; + } + + @CheckForNull + private static Range methodInvocationRange(MethodInvocationTree invocation) { + if (CHAR_SEQUENCE_LENGTH.matches(invocation) || COLLECTION_SIZE.matches(invocation) + || MAP_SIZE.matches(invocation) || ENUM_ORDINAL.matches(invocation)) { + return NON_NEGATIVE_INT; + } + if (BIT_COUNT_32.matches(invocation)) { + return new Range(0, 32); + } + if (BIT_COUNT_64.matches(invocation)) { + return new Range(0, 64); + } + return null; + } + + @CheckForNull + private static Range bitwiseAndRange(BinaryExpressionTree and, int depth) { + Long nonNegativeConstant = nonNegativeConstant(and.leftOperand(), depth); + if (nonNegativeConstant == null) { + nonNegativeConstant = nonNegativeConstant(and.rightOperand(), depth); + } + if (nonNegativeConstant == null) { + return null; + } + return new Range(0, nonNegativeConstant); + } + + @CheckForNull + private static Long nonNegativeConstant(ExpressionTree expression, int depth) { + Range range = rangeOf(expression, depth + 1); + if (range != null && range.lo() == range.hi() && range.lo() >= 0) { + return range.lo(); + } + return null; + } + + @CheckForNull + private static Range identifierRange(IdentifierTree identifier, int depth) { + Symbol symbol = identifier.symbol(); + if (symbol.isUnknown()) { + return null; + } + ExpressionTree singleWriteUsage = ExpressionsHelper.getSingleWriteUsage(symbol); + return singleWriteUsage == null ? null : rangeOf(singleWriteUsage, depth + 1); + } + + private record Range(long lo, long hi) { + boolean fitsIntSubtraction(Range other) { + return hi - other.lo() <= Integer.MAX_VALUE && lo - other.hi() >= Integer.MIN_VALUE; + } + } + +} From 454618e15c04731e550af29b363852c1d6cc40d9 Mon Sep 17 00:00:00 2001 From: nathsou Date: Mon, 24 Aug 2026 11:43:49 +0200 Subject: [PATCH 2/5] SONARJAVA-6838 Address review: fix sign-bit constants and ++/-- resolution, add coverage, sync rule HTML --- ...gerSubtractionInComparisonCheckSample.java | 24 +++++++++++++++++++ .../checks/helpers/BoundedIntegerRange.java | 19 ++++++++++----- .../org/sonar/l10n/java/rules/java/S9354.html | 3 +++ 3 files changed, 40 insertions(+), 6 deletions(-) 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 index b49d946a905..253e49c2e5d 100644 --- a/java-checks-test-sources/default/src/main/java/checks/IntegerSubtractionInComparisonCheckSample.java +++ b/java-checks-test-sources/default/src/main/java/checks/IntegerSubtractionInComparisonCheckSample.java @@ -272,6 +272,13 @@ public int compare(Integer left, Integer right) { } } + static class LongBitCountComparator implements Comparator { + @Override + public int compare(Long left, Long right) { + return Long.bitCount(left) - Long.bitCount(right); // Compliant - both operands are bounded in [0, 64] + } + } + static class MaskedByteComparator implements Comparator { @Override public int compare(byte[] left, byte[] right) { @@ -314,6 +321,23 @@ public int compareTo(MixedBoundedAndUnboundedComparator other) { } } + static class MixedUnboundedAndBoundedComparator implements Comparator { + @Override + public int compare(String left, String right) { + // Noncompliant@+1 {{Subtracting numeric values in compare can overflow; use Integer.compare instead.}} + return left.length() - right.hashCode(); + } + } + + static class BoundedButUnsafeComparator implements Comparator { + @Override + public int compare(byte[] left, byte[] right) { + int a = left[0] & 0xff; + // Noncompliant@+1 {{Subtracting numeric values in compare can overflow; use Integer.compare instead.}} + return a - Integer.MIN_VALUE; + } + } + static class UnknownRangeComparator implements Comparable { @Override public int compareTo(UnknownRangeComparator other) { diff --git a/java-checks/src/main/java/org/sonar/java/checks/helpers/BoundedIntegerRange.java b/java-checks/src/main/java/org/sonar/java/checks/helpers/BoundedIntegerRange.java index e1da997f5fa..8686624f1a0 100644 --- a/java-checks/src/main/java/org/sonar/java/checks/helpers/BoundedIntegerRange.java +++ b/java-checks/src/main/java/org/sonar/java/checks/helpers/BoundedIntegerRange.java @@ -18,7 +18,6 @@ import javax.annotation.CheckForNull; import org.sonar.java.model.ExpressionUtils; -import org.sonar.java.model.LiteralUtils; import org.sonar.plugins.java.api.semantic.MethodMatchers; import org.sonar.plugins.java.api.semantic.Symbol; import org.sonar.plugins.java.api.semantic.Type; @@ -104,11 +103,17 @@ private static Range rangeOf(ExpressionTree expression, int depth) { @CheckForNull private static Range constantRange(ExpressionTree tree) { - Long value = LiteralUtils.longLiteralValue(tree); - if (value == null) { - return null; + // Resolve through the compiler's own constant folding rather than re-parsing literal text, so the + // value respects the expression's static type: an int-typed 0x80000000 is -2147483648, not +2147483648. + Integer intValue = tree.asConstant(Integer.class).orElse(null); + if (intValue != null) { + return new Range(intValue, intValue); + } + Long longValue = tree.asConstant(Long.class).orElse(null); + if (longValue != null) { + return new Range(longValue, longValue); } - return new Range(value, value); + return null; } @CheckForNull @@ -160,7 +165,9 @@ private static Long nonNegativeConstant(ExpressionTree expression, int depth) { @CheckForNull private static Range identifierRange(IdentifierTree identifier, int depth) { Symbol symbol = identifier.symbol(); - if (symbol.isUnknown()) { + if (symbol.isUnknown() || !ExpressionsHelper.isNotReassigned(symbol)) { + // getSingleWriteUsage only looks at AssignmentExpressionTree reassignments, so a variable + // mutated by ++/-- would otherwise look like a single-write local pinned to its initializer. return null; } ExpressionTree singleWriteUsage = ExpressionsHelper.getSingleWriteUsage(symbol); 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 index 9bc09eb5407..fe4a0cf95ef 100644 --- 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 @@ -20,6 +20,9 @@

Why is this an issue?

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}.

+

This rule also does not flag subtraction of two values with a provably bounded range, such as array lengths, String.length(), +Collection.size(), Enum.ordinal(), or a value masked with a non-negative bitwise AND constant. Their difference cannot +overflow, so the subtraction is safe.

How to fix it

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

Code examples

From abbd694285a7e37e5f6be9d62a2427acc02e4457 Mon Sep 17 00:00:00 2001 From: nathsou Date: Mon, 24 Aug 2026 12:00:32 +0200 Subject: [PATCH 3/5] SONARJAVA-6838 Update guava ruling expectations for S9354 bounded-range fix --- .../src/test/resources/guava/java-S9354.json | 34 +------------------ 1 file changed, 1 insertion(+), 33 deletions(-) diff --git a/its/ruling/src/test/resources/guava/java-S9354.json b/its/ruling/src/test/resources/guava/java-S9354.json index 5bd8aea52dc..aaba47fcf61 100644 --- a/its/ruling/src/test/resources/guava/java-S9354.json +++ b/its/ruling/src/test/resources/guava/java-S9354.json @@ -2,39 +2,7 @@ "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 +409 ] } From f9adc529a81ecda1f727de831a6d7a16d775f481 Mon Sep 17 00:00:00 2001 From: nathsou Date: Mon, 24 Aug 2026 12:26:01 +0200 Subject: [PATCH 4/5] SONARJAVA-6838 Add direct unit tests for BoundedIntegerRange to satisfy new-code coverage --- .../helpers/BoundedIntegerRangeTest.java | 122 ++++++++++++++++++ 1 file changed, 122 insertions(+) create mode 100644 java-checks/src/test/java/org/sonar/java/checks/helpers/BoundedIntegerRangeTest.java diff --git a/java-checks/src/test/java/org/sonar/java/checks/helpers/BoundedIntegerRangeTest.java b/java-checks/src/test/java/org/sonar/java/checks/helpers/BoundedIntegerRangeTest.java new file mode 100644 index 00000000000..54c091e2896 --- /dev/null +++ b/java-checks/src/test/java/org/sonar/java/checks/helpers/BoundedIntegerRangeTest.java @@ -0,0 +1,122 @@ +/* + * 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.lang.reflect.Constructor; +import java.util.List; +import org.junit.jupiter.api.Test; +import org.sonar.plugins.java.api.tree.BinaryExpressionTree; +import org.sonar.plugins.java.api.tree.ExpressionTree; +import org.sonar.plugins.java.api.tree.MethodTree; +import org.sonar.plugins.java.api.tree.ReturnStatementTree; +import org.sonar.plugins.java.api.tree.StatementTree; + +import static org.assertj.core.api.Assertions.assertThat; + +class BoundedIntegerRangeTest { + + @Test + void private_constructor() throws Exception { + Constructor constructor = BoundedIntegerRange.class.getDeclaredConstructor(); + assertThat(constructor.isAccessible()).isFalse(); + constructor.setAccessible(true); + constructor.newInstance(); + } + + @Test + void both_operands_bounded_cannot_overflow() { + assertThat(subtractionCannotOverflow( + "int compare(String left, String right) { return left.length() - right.length(); }")).isTrue(); + } + + @Test + void unbounded_right_operand_can_overflow() { + assertThat(subtractionCannotOverflow( + "int compare(String left, Object right) { return left.length() - right.hashCode(); }")).isFalse(); + } + + @Test + void unbounded_left_operand_can_overflow() { + assertThat(subtractionCannotOverflow( + "int compare(Object left, String right) { return left.hashCode() - right.length(); }")).isFalse(); + } + + @Test + void masked_byte_operands_cannot_overflow() { + assertThat(subtractionCannotOverflow( + "int compare(byte[] left, byte[] right) { return (left[0] & 0xff) - (right[0] & 0xff); }")).isTrue(); + } + + @Test + void masking_with_no_constant_operand_is_not_resolved() { + assertThat(subtractionCannotOverflow( + "int compare(int left, int mask, int right) { return (left & mask) - right; }")).isFalse(); + } + + @Test + void sign_bit_hex_literal_operand_can_overflow() { + // left.length() is [0, MAX_VALUE], but MIN_VALUE (0x80000000 as an int literal) makes the subtraction overflow. + assertThat(subtractionCannotOverflow( + "int compare(String left, String right) { return left.length() - 0x80000000; }")).isFalse(); + } + + @Test + void long_constant_operands_cannot_overflow() { + // Reached directly, bypassing the check's int/long dispatch, to exercise the long-constant branch of constantRange. + List statements = JParserTestUtils.methodBody(JParserTestUtils.newCode("void m() { long x = 5L - 3L; }")); + ExpressionTree initializer = JParserTestUtils.initializerFromVariableDeclarationStatement(statements.get(0)); + BinaryExpressionTree binary = (BinaryExpressionTree) initializer; + assertThat(BoundedIntegerRange.subtractionCannotOverflow(binary.leftOperand(), binary.rightOperand())).isTrue(); + } + + @Test + void deeply_chained_single_write_locals_are_not_resolved() { + // The resolution depth cap keeps this conservative: a long chain of single-write locals is treated as unbounded + // rather than resolved all the way back to the bounded String.length() call. + assertThat(subtractionCannotOverflow( + "int compare(String left, String right) {", + " int a = left.length();", + " int b = a;", + " int c = b;", + " int d = c;", + " int e = d;", + " return e - right.length();", + "}")).isFalse(); + } + + @Test + void reassignment_by_increment_is_not_a_single_write() { + // i++ is not seen as a reassignment by getSingleWriteUsage, so without the isNotReassigned guard this would + // incorrectly resolve i to its initializer value 0. + assertThat(subtractionCannotOverflow( + "int compare(Node left, Node right) {", + " int i = 0;", + " for (Node n = left; n != null; n = n.parent) { i++; }", + " return i - right.hashCode();", + "}", + "static class Node { Node parent; }")).isFalse(); + } + + private static boolean subtractionCannotOverflow(String... classMembers) { + MethodTree method = JParserTestUtils.methodTree(JParserTestUtils.newCode(classMembers)); + List statements = method.block().body(); + ReturnStatementTree returnStatement = (ReturnStatementTree) statements.get(statements.size() - 1); + BinaryExpressionTree binary = (BinaryExpressionTree) returnStatement.expression(); + return BoundedIntegerRange.subtractionCannotOverflow(binary.leftOperand(), binary.rightOperand()); + } + +} From 01fc1bcfa59505beb0d4f3151351fc742a83ae26 Mon Sep 17 00:00:00 2001 From: nathsou Date: Mon, 24 Aug 2026 16:04:05 +0200 Subject: [PATCH 5/5] SONARJAVA-6838 Move BoundedIntegerRange coverage into the check's sample file Per review feedback, most scenarios are expressible as real compare()/compareTo() bodies and are now tested end-to-end like the rest of the check, instead of via direct AST construction. Only the long-constant case remains a direct unit test, since it is unreachable from any real check call site. --- ...gerSubtractionInComparisonCheckSample.java | 56 +++++++++++++ .../helpers/BoundedIntegerRangeTest.java | 83 ++----------------- 2 files changed, 63 insertions(+), 76 deletions(-) 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 index 253e49c2e5d..071ac30cddc 100644 --- a/java-checks-test-sources/default/src/main/java/checks/IntegerSubtractionInComparisonCheckSample.java +++ b/java-checks-test-sources/default/src/main/java/checks/IntegerSubtractionInComparisonCheckSample.java @@ -338,6 +338,62 @@ public int compare(byte[] left, byte[] right) { } } + static class SignBitLiteralComparator implements Comparator { + @Override + public int compare(String left, String right) { + // 0x80000000 is the int literal for Integer.MIN_VALUE; left.length() is bounded but this still overflows. + // Noncompliant@+1 {{Subtracting numeric values in compare can overflow; use Integer.compare instead.}} + return left.length() - 0x80000000; + } + } + + static class MaskedByUnresolvedMaskComparator implements Comparator { + @Override + public int compare(Integer left, Integer right) { + int mask = computeMask(left); + // mask does not resolve to a constant, so the mask range - and therefore this subtraction - is unknown. + // Noncompliant@+1 {{Subtracting numeric values in compare can overflow; use Integer.compare instead.}} + return (left & mask) - 0; + } + + private static int computeMask(int seed) { + return seed > 0 ? 0xff : 0x0f; + } + } + + static class DeeplyChainedLocalsComparator implements Comparator { + @Override + public int compare(String left, String right) { + int a = left.length(); + int b = a; + int c = b; + int d = c; + int e = d; + // The single-write resolution depth cap keeps this conservative: e is not traced all the way back to + // left.length(), so it is treated as unbounded even though it provably isn't. + // Noncompliant@+1 {{Subtracting numeric values in compare can overflow; use Integer.compare instead.}} + return e - right.length(); + } + } + + static class IncrementedCounterComparator implements Comparator { + @Override + public int compare(String[] left, String[] right) { + int leftCount = 0; + for (String s : left) { + leftCount++; + } + int rightCount = 0; + for (String s : right) { + rightCount++; + } + // leftCount and rightCount are mutated with ++, not a single assignment, so they are not resolved to a + // bounded range even though both loops only ever increment their counter. + // Noncompliant@+1 {{Subtracting numeric values in compare can overflow; use Integer.compare instead.}} + return leftCount - rightCount; + } + } + static class UnknownRangeComparator implements Comparable { @Override public int compareTo(UnknownRangeComparator other) { diff --git a/java-checks/src/test/java/org/sonar/java/checks/helpers/BoundedIntegerRangeTest.java b/java-checks/src/test/java/org/sonar/java/checks/helpers/BoundedIntegerRangeTest.java index 54c091e2896..5da4ad2b733 100644 --- a/java-checks/src/test/java/org/sonar/java/checks/helpers/BoundedIntegerRangeTest.java +++ b/java-checks/src/test/java/org/sonar/java/checks/helpers/BoundedIntegerRangeTest.java @@ -21,12 +21,17 @@ import org.junit.jupiter.api.Test; import org.sonar.plugins.java.api.tree.BinaryExpressionTree; import org.sonar.plugins.java.api.tree.ExpressionTree; -import org.sonar.plugins.java.api.tree.MethodTree; -import org.sonar.plugins.java.api.tree.ReturnStatementTree; import org.sonar.plugins.java.api.tree.StatementTree; import static org.assertj.core.api.Assertions.assertThat; +/** + * Most of {@link BoundedIntegerRange}'s behaviour is exercised end-to-end through + * {@code IntegerSubtractionInComparisonCheckSample}, matching how the rest of this codebase tests checks. This class + * only covers the one scenario that cannot be expressed as a real {@code compare()}/{@code compareTo()} method body: + * {@link BoundedIntegerRange#subtractionCannotOverflow} is only ever called by the check on {@code int}-typed + * operands, so its {@code long} constant handling has no reachable call site and needs a direct test. + */ class BoundedIntegerRangeTest { @Test @@ -37,86 +42,12 @@ void private_constructor() throws Exception { constructor.newInstance(); } - @Test - void both_operands_bounded_cannot_overflow() { - assertThat(subtractionCannotOverflow( - "int compare(String left, String right) { return left.length() - right.length(); }")).isTrue(); - } - - @Test - void unbounded_right_operand_can_overflow() { - assertThat(subtractionCannotOverflow( - "int compare(String left, Object right) { return left.length() - right.hashCode(); }")).isFalse(); - } - - @Test - void unbounded_left_operand_can_overflow() { - assertThat(subtractionCannotOverflow( - "int compare(Object left, String right) { return left.hashCode() - right.length(); }")).isFalse(); - } - - @Test - void masked_byte_operands_cannot_overflow() { - assertThat(subtractionCannotOverflow( - "int compare(byte[] left, byte[] right) { return (left[0] & 0xff) - (right[0] & 0xff); }")).isTrue(); - } - - @Test - void masking_with_no_constant_operand_is_not_resolved() { - assertThat(subtractionCannotOverflow( - "int compare(int left, int mask, int right) { return (left & mask) - right; }")).isFalse(); - } - - @Test - void sign_bit_hex_literal_operand_can_overflow() { - // left.length() is [0, MAX_VALUE], but MIN_VALUE (0x80000000 as an int literal) makes the subtraction overflow. - assertThat(subtractionCannotOverflow( - "int compare(String left, String right) { return left.length() - 0x80000000; }")).isFalse(); - } - @Test void long_constant_operands_cannot_overflow() { - // Reached directly, bypassing the check's int/long dispatch, to exercise the long-constant branch of constantRange. List statements = JParserTestUtils.methodBody(JParserTestUtils.newCode("void m() { long x = 5L - 3L; }")); ExpressionTree initializer = JParserTestUtils.initializerFromVariableDeclarationStatement(statements.get(0)); BinaryExpressionTree binary = (BinaryExpressionTree) initializer; assertThat(BoundedIntegerRange.subtractionCannotOverflow(binary.leftOperand(), binary.rightOperand())).isTrue(); } - @Test - void deeply_chained_single_write_locals_are_not_resolved() { - // The resolution depth cap keeps this conservative: a long chain of single-write locals is treated as unbounded - // rather than resolved all the way back to the bounded String.length() call. - assertThat(subtractionCannotOverflow( - "int compare(String left, String right) {", - " int a = left.length();", - " int b = a;", - " int c = b;", - " int d = c;", - " int e = d;", - " return e - right.length();", - "}")).isFalse(); - } - - @Test - void reassignment_by_increment_is_not_a_single_write() { - // i++ is not seen as a reassignment by getSingleWriteUsage, so without the isNotReassigned guard this would - // incorrectly resolve i to its initializer value 0. - assertThat(subtractionCannotOverflow( - "int compare(Node left, Node right) {", - " int i = 0;", - " for (Node n = left; n != null; n = n.parent) { i++; }", - " return i - right.hashCode();", - "}", - "static class Node { Node parent; }")).isFalse(); - } - - private static boolean subtractionCannotOverflow(String... classMembers) { - MethodTree method = JParserTestUtils.methodTree(JParserTestUtils.newCode(classMembers)); - List statements = method.block().body(); - ReturnStatementTree returnStatement = (ReturnStatementTree) statements.get(statements.size() - 1); - BinaryExpressionTree binary = (BinaryExpressionTree) returnStatement.expression(); - return BoundedIntegerRange.subtractionCannotOverflow(binary.leftOperand(), binary.rightOperand()); - } - }