SONARJAVA-6780: Implement S9354: Comparable.compareTo() and Comparator.compare() should not use subtraction on numerical fields

[nathsou]

Summary

• Implement S9354 as a native SonarJava check that flags int/long subtraction inside Comparable.compareTo(), Comparator.compare(), and Comparator lambdas.
• Report a parameterized message on the - operator, for example Subtracting numeric values in compareTo can overflow; use Long.compare instead.
• Leave floating-point subtraction to S9148. Byte/short/char subtraction is ignored because the promoted int difference cannot overflow.

Links

• Jira: https://sonarsource.atlassian.net/browse/SONARJAVA-6780
• RSPEC PR: https://github.com/SonarSource/rspec/pull/7915

AI disclosure

• LLM model used for implementation: cursor-grok-4.6-high

[hashicorp-vault-sonar-prod]

SONARJAVA-6780

[gitar-bot]

💡 Quality: S9354.json declares quickfix "targeted" but no quick fix is implemented

S9354.json sets "quickfix": "targeted", yet IntegerSubtractionInComparisonCheck implements no quick fix (only reportIssue on the operator token). Other checks without an implemented quick fix declare "infeasible" or "covered" (e.g. S9351.json uses "infeasible"). While no automated test currently enforces this, the declared value is misleading and inconsistent with repo conventions; either implement the targeted quick fix (replace the subtraction with Integer.compare/Long.compare) or change the metadata to the accurate value.

_{Was this helpful? React with 👍 / 👎}

[github-actions]

❌ Ruling needs updating. A fix PR has been created: #5961

Please review and merge it into your branch.

[github-actions]

Ruling Diff Summary

Detected changes in 2 rule files: 0 issues removed, 16 issues added.

S9354 (java) on guava - 0 issues removed, 14 issues added - new ruling file

Added src/com/google/common/collect/ExplicitOrdering.java (line 41)

        36 |     this.rankMap = rankMap;
        37 |   }
        38 | 
        39 |   @Override
        40 |   public int compare(T left, T right) {
>>>     41 |     return rank(left) - rank(right); // safe because both are nonnegative
        42 |   }
        43 | 
        44 |   private int rank(T value) {
        45 |     Integer rank = rankMap.get(value);
        46 |     if (rank == null) {

Added src/com/google/common/primitives/Booleans.java (line 297)

       292 |         int result = Booleans.compare(left[i], right[i]);
       293 |         if (result != 0) {
       294 |           return result;
       295 |         }
       296 |       }
>>>    297 |       return left.length - right.length;
       298 |     }
       299 |   }
       300 | 
       301 |   /**
       302 |    * Copies a collection of {@code Boolean} instances into a new array of

Added src/com/google/common/primitives/Chars.java (line 414)

       409 |         int result = Chars.compare(left[i], right[i]);
       410 |         if (result != 0) {
       411 |           return result;
       412 |         }
       413 |       }
>>>    414 |       return left.length - right.length;
       415 |     }
       416 |   }
       417 | 
       418 |   /**
       419 |    * Copies a collection of {@code Character} instances into a new array of

Added src/com/google/common/primitives/Doubles.java (line 401)

       396 |         int result = Double.compare(left[i], right[i]);
       397 |         if (result != 0) {
       398 |           return result;
       399 |         }
       400 |       }
>>>    401 |       return left.length - right.length;
       402 |     }
       403 |   }
       404 | 
       405 |   /**
       406 |    * Returns an array containing each value of {@code collection}, converted to

Added src/com/google/common/primitives/Floats.java (line 397)

       392 |         int result = Float.compare(left[i], right[i]);
       393 |         if (result != 0) {
       394 |           return result;
       395 |         }
       396 |       }
>>>    397 |       return left.length - right.length;
       398 |     }
       399 |   }
       400 | 
       401 |   /**
       402 |    * Returns an array containing each value of {@code collection}, converted to

Added src/com/google/common/primitives/Ints.java (line 462)

       457 |         int result = Ints.compare(left[i], right[i]);
       458 |         if (result != 0) {
       459 |           return result;
       460 |         }
       461 |       }
>>>    462 |       return left.length - right.length;
       463 |     }
       464 |   }
       465 | 
       466 |   /**
       467 |    * Returns an array containing each value of {@code collection}, converted to

Added src/com/google/common/primitives/Longs.java (line 498)

       493 |         int result = Longs.compare(left[i], right[i]);
       494 |         if (result != 0) {
       495 |           return result;
       496 |         }
       497 |       }
>>>    498 |       return left.length - right.length;
       499 |     }
       500 |   }
       501 | 
       502 |   /**
       503 |    * Returns an array containing each value of {@code collection}, converted to

Added src/com/google/common/primitives/Shorts.java (line 461)

       456 |         int result = Shorts.compare(left[i], right[i]);
       457 |         if (result != 0) {
       458 |           return result;
       459 |         }
       460 |       }
>>>    461 |       return left.length - right.length;
       462 |     }
       463 |   }
       464 | 
       465 |   /**
       466 |    * Returns an array containing each value of {@code collection}, converted to

Added src/com/google/common/primitives/SignedBytes.java (line 202)

       197 |         int result = SignedBytes.compare(left[i], right[i]);
       198 |         if (result != 0) {
       199 |           return result;
       200 |         }
       201 |       }
>>>    202 |       return left.length - right.length;
       203 |     }
       204 |   }
       205 | }

Added src/com/google/common/primitives/UnsignedBytes.java (line 409)

       404 |              * and rw are little-endian.  Long.numberOfTrailingZeros(diff) tells us the least
       405 |              * significant nonzero bit, and zeroing out the first three bits of L.nTZ gives us the
       406 |              * shift to get that least significant nonzero byte.
       407 |              */
       408 |             int n = Long.numberOfTrailingZeros(lw ^ rw) & ~0x7;
>>>    409 |             return (int) (((lw >>> n) & UNSIGNED_MASK) - ((rw >>> n) & UNSIGNED_MASK));
       410 |           }
       411 |         }
       412 | 
       413 |         // The epilogue to cover the last (minLength % 8) elements.
       414 |         for (int i = minWords * Longs.BYTES; i < minLength; i++) {

Added src/com/google/common/primitives/UnsignedBytes.java (line 420)

       415 |           int result = UnsignedBytes.compare(left[i], right[i]);
       416 |           if (result != 0) {
       417 |             return result;
       418 |           }
       419 |         }
>>>    420 |         return left.length - right.length;
       421 |       }
       422 |     }
       423 | 
       424 |     enum PureJavaComparator implements Comparator<byte[]> {
       425 |       INSTANCE;

Added src/com/google/common/primitives/UnsignedBytes.java (line 436)

       431 |           int result = UnsignedBytes.compare(left[i], right[i]);
       432 |           if (result != 0) {
       433 |             return result;
       434 |           }
       435 |         }
>>>    436 |         return left.length - right.length;
       437 |       }
       438 |     }
       439 | 
       440 |     /**
       441 |      * Returns the Unsafe-using Comparator, or falls back to the pure-Java

Added src/com/google/common/primitives/UnsignedInts.java (line 176)

       171 |       for (int i = 0; i < minLength; i++) {
       172 |         if (left[i] != right[i]) {
       173 |           return UnsignedInts.compare(left[i], right[i]);
       174 |         }
       175 |       }
>>>    176 |       return left.length - right.length;
       177 |     }
       178 |   }
       179 | 
       180 |   /**
       181 |    * Returns dividend / divisor, where the dividend and divisor are treated as unsigned 32-bit

Added src/com/google/common/primitives/UnsignedLongs.java (line 177)

       172 |       for (int i = 0; i < minLength; i++) {
       173 |         if (left[i] != right[i]) {
       174 |           return UnsignedLongs.compare(left[i], right[i]);
       175 |         }
       176 |       }
>>>    177 |       return left.length - right.length;
       178 |     }
       179 |   }
       180 | 
       181 |   /**
       182 |    * Returns dividend / divisor, where the dividend and divisor are treated as unsigned 64-bit

S9354 (java) on sonar-server - 0 issues removed, 2 issues added - new ruling file

Added src/main/java/org/sonar/server/computation/task/projectanalysis/duplication/Duplication.java (line 119)

(source file not found at this revision: src/main/java/org/sonar/server/computation/task/projectanalysis/duplication/Duplication.java)

Added src/main/java/org/sonar/server/computation/task/projectanalysis/duplication/TextBlock.java (line 61)

(source file not found at this revision: src/main/java/org/sonar/server/computation/task/projectanalysis/duplication/TextBlock.java)

[github-actions]

❌ Ruling needs updating. A fix PR has been created: #5963

Please review and merge it into your branch.

[romainbrenguier]

I don't think adding an abstract check class is a good idea.

[romainbrenguier]

This doesn't seem coherent with the "defaultSeverity". I think it should be "MEDIUM" here. (To be fixed on the rspec PR).

[nathsou]

Fixed. defaultSeverity Major maps to a MEDIUM impact (same as S9350); HIGH is the Critical mapping used by S9148.

RSPEC: https://github.com/SonarSource/rspec/pull/7915 (37ad141ec9)
Analyzer metadata updated to match in 66c7c48b2d.

[romainbrenguier]

Why is it blacklisted? I don't understand what this test is.

[nathsou]

count() asserts that every on-disk *Check.java is a registered rule in GeneratedCheckList. Abstract bases without @Rule have to be excluded (same list as AbstractRegexCheck).

That blacklist entry only existed because of AbstractComparisonMethodCheck. The abstract class is gone in 66c7c48b2d, so the entry is gone too.

[romainbrenguier]

I would stay away from creating abstract classes for checks. That creates coupling between rules, which we always want to avoid. If we need to avoid duplication, utility methods may be a solution.

[nathsou]

Agreed, sharing an abstract check coupled S9148 and S9354.

66c7c48b2d deletes AbstractComparisonMethodCheck. Each rule is a standalone IssuableSubscriptionVisitor again, and they only share ComparisonMethodUtils for the compareTo / Comparator.compare matchers.

[gitar-bot]

Code Review 👍 Approved with suggestions 1 resolved / 2 findings

Implements the S9354 check to flag integer and long subtraction in comparison methods and lambdas, restricting reports to direct comparison results. Ensure the S9354.json quickfix metadata aligns with the lack of an implemented quick fix.

💡 Quality: S9354.json declares quickfix "targeted" but no quick fix is implemented

📄 sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S9354.json:16 📄 java-checks/src/main/java/org/sonar/java/checks/IntegerSubtractionInComparisonCheck.java:82-88

S9354.json sets "quickfix": "targeted", yet IntegerSubtractionInComparisonCheck implements no quick fix (only reportIssue on the operator token). Other checks without an implemented quick fix declare "infeasible" or "covered" (e.g. S9351.json uses "infeasible"). While no automated test currently enforces this, the declared value is misleading and inconsistent with repo conventions; either implement the targeted quick fix (replace the subtraction with Integer.compare/Long.compare) or change the metadata to the accurate value.

✅ 1 resolved

✅ Bug: Flags any int/long subtraction in comparison body, not just the result

📄 java-checks/src/main/java/org/sonar/java/checks/IntegerSubtractionInComparisonCheck.java:82-90 📄 java-checks-test-sources/default/src/main/java/checks/IntegerSubtractionInComparisonCheckSample.java:1-15
SubtractionInComparisonVisitor walks the entire compareTo/compare body and reports every int/long MINUS expression, regardless of whether it contributes to the returned ordering value. This causes false positives for ordinary arithmetic inside these methods that cannot affect the contract — e.g. loop bounds (for (int i = arr.length - 1; ...)), array index math (data[i - 1]), or offset/length computations used while iterating element-by-element. None of these unrelated-subtraction scenarios are covered as Compliant in the sample, so the noise is untested. Consider restricting reporting to subtractions that flow into the method's return value (e.g. subtraction that is the returned expression, or narrowed via (int) cast and returned), and add negative test cases for loop/index arithmetic.

🤖 Prompt for agents

Code Review: Implements the S9354 check to flag integer and long subtraction in comparison methods and lambdas, restricting reports to direct comparison results. Ensure the S9354.json quickfix metadata aligns with the lack of an implemented quick fix.

1. 💡 Quality: S9354.json declares quickfix "targeted" but no quick fix is implemented
   Files: sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S9354.json:16, java-checks/src/main/java/org/sonar/java/checks/IntegerSubtractionInComparisonCheck.java:82-88

   S9354.json sets `"quickfix": "targeted"`, yet IntegerSubtractionInComparisonCheck implements no quick fix (only reportIssue on the operator token). Other checks without an implemented quick fix declare `"infeasible"` or `"covered"` (e.g. S9351.json uses "infeasible"). While no automated test currently enforces this, the declared value is misleading and inconsistent with repo conventions; either implement the targeted quick fix (replace the subtraction with Integer.compare/Long.compare) or change the metadata to the accurate value.

Implementation Status ✅ 1 / 1 issues implemented

✅ SONARJAVA-6780 — 1 / 1 objectives

The PR successfully implements rule S9354 to detect and report numerical field subtraction inside Comparable.compareTo() and Comparator.compare() methods and lambdas.

✅ 1 complete

• ✅ Implement rule S9354 to detect when Comparable.compareTo() or Comparator.compare() use subtraction on numerical fields

Options

Auto-apply is off → Gitar will not commit updates to this branch.
Display: compact → Showing less information.

Comment with these commands to change the behavior for this request:

Auto-apply	Compact
gitar auto-apply:on	gitar display:verbose

_{Was this helpful? React with 👍 / 👎 | Gitar}

[sonarqube-next]

Quality Gate passed

Issues
0 New issues
0 Fixed issues
0 Accepted issues

Measures
0 Security Hotspots
0 Dependency risks
93.9% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube