SONARJAVA-6825: Implemented rule S9360 - Comparisons should not use Yoda conditions

[romainbrenguier]

This PR implements rule S9360 'Constant expressions and comparisons should be simplified'.

The rule detects Yoda conditions where a constant literal appears on the left side of a comparison operator.

What the rule detects

• 0 == count → should be count == 0
• null == obj → should be obj == null
• true == flag → should be flag == true
• 5 != x → should be x != 5
• 0 < count → should be count > 0
• 5 > x → should be x < 5

Supported operators

• == (equality)
• != (inequality)
• < (less than)
• > (greater than)

Supported literal types

• Integer literals (0, 5, 42)
• Long literals (0L, 5L)
• Floating-point literals (0.0, 3.14)
• Boolean literals (true, false)
• Character literals ('a')
• String literals ("hello", "")
• Null literal (null)

Edge cases handled

• Nested parentheses: ((0)) == count is detected
• Both operands are literals: 0 == 0 is compliant (nothing to swap)
• Both operands are variables: count == otherCount is compliant
• Non-comparison contexts: assignments, arithmetic, method calls are ignored

Implementation details

• Uses IssuableSubscriptionVisitor pattern
• Subscribes to EQUAL_TO, NOT_EQUAL_TO, LESS_THAN, GREATER_THAN tree kinds
• Uses ExpressionUtils.skipParentheses() to handle nested parentheses
• Reports issue on the literal (left operand) with message "Put the variable on the left side of this comparison."

Test coverage

• All literal types with == and != operators
• Less than and greater than operators
• Nested parentheses handling
• Non-comparison contexts (assignments, arithmetic, method calls)
• Edge cases: both literals, both variables, ternary operators

[hashicorp-vault-sonar-prod]

SONARJAVA-6825

[github-actions]

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

Please review and merge it into your branch.

[romainbrenguier]

Changes addressing review comments

1. Fixed compilation error - null == count where count is int (primitive) now uses an Object variable instead
2. Narrowed rule description - Removed "Constant Function Calls" section (Math.max, Math.sqrt, etc.) since the implementation only detects Yoda conditions
3. Fixed compliant example - Changed flag == true to flag (avoids conflict with S1125) and 4.0 + 5 to 9.0
4. Improved relational operator message - For <, >, <=, >=, the message now says "Put the variable on the left side of this comparison and reverse the operator"
5. Added <= and >= support - LESS_THAN_OR_EQUAL_TO and GREATER_THAN_OR_EQUAL_TO are now detected
6. Merged ruling fix PR Update ruling results for PR #6022 #6023 - Ruling expectations for the original findings are included

Note: The addition of <= and >= operators may produce new ruling differences that will require a follow-up ruling update.

✅ All code review findings resolved.

_{Configure merge blocking}

[github-actions]

Ruling Diff Summary

Detected changes in 4 rule files: 0 issues removed, 109 issues added.

S9360 (java) on commons-beanutils - 0 issues removed, 4 issues added - new ruling file

Added src/main/java/org/apache/commons/beanutils2/ConstructorUtils.java (line 106)

       101 |             NoSuchMethodException,
       102 |             IllegalAccessException,
       103 |             InvocationTargetException,
       104 |             InstantiationException {
       105 | 
>>>    106 |         if (null == args) {
       107 |             args = EMPTY_OBJECT_ARRAY;
       108 |         }
       109 |         final int arguments = args.length;
       110 |         final Class<?> parameterTypes[] = new Class<?>[arguments];
       111 |         for (int i = 0; i < arguments; i++) {

Added src/main/java/org/apache/commons/beanutils2/ConstructorUtils.java (line 154)

       149 |             args = EMPTY_OBJECT_ARRAY;
       150 |         }
       151 | 
       152 |         final Constructor<T> ctor =
       153 |             getMatchingAccessibleConstructor(klass, parameterTypes);
>>>    154 |         if (null == ctor) {
       155 |             throw new NoSuchMethodException(
       156 |                 "No such accessible constructor on object: " + klass.getName());
       157 |         }
       158 |         return ctor.newInstance(args);
       159 |     }

Added src/main/java/org/apache/commons/beanutils2/ConstructorUtils.java (line 218)

       213 |             NoSuchMethodException,
       214 |             IllegalAccessException,
       215 |             InvocationTargetException,
       216 |             InstantiationException {
       217 | 
>>>    218 |         if (null == args) {
       219 |             args = EMPTY_OBJECT_ARRAY;
       220 |         }
       221 |         final int arguments = args.length;
       222 |         final Class<?> parameterTypes[] = new Class[arguments];
       223 |         for (int i = 0; i < arguments; i++) {

S9360 (java) on eclipse-jetty - 0 issues removed, 55 issues added - new ruling file

Added jetty-http/src/main/java/org/eclipse/jetty/http/HttpStatus.java (line 343)

(source file not found at this revision: jetty-http/src/main/java/org/eclipse/jetty/http/HttpStatus.java)

Added jetty-http/src/main/java/org/eclipse/jetty/http/HttpStatus.java (line 358)

(source file not found at this revision: jetty-http/src/main/java/org/eclipse/jetty/http/HttpStatus.java)

Added jetty-http/src/main/java/org/eclipse/jetty/http/HttpStatus.java (line 373)

(source file not found at this revision: jetty-http/src/main/java/org/eclipse/jetty/http/HttpStatus.java)

Added jetty-http/src/main/java/org/eclipse/jetty/http/HttpURI.java (line 1028)

(source file not found at this revision: jetty-http/src/main/java/org/eclipse/jetty/http/HttpURI.java)

Added jetty-http/src/main/java/org/eclipse/jetty/http/MimeTypes.java (line 431)

(source file not found at this revision: jetty-http/src/main/java/org/eclipse/jetty/http/MimeTypes.java)

Added jetty-http/src/main/java/org/eclipse/jetty/http/MimeTypes.java (line 436)

(source file not found at this revision: jetty-http/src/main/java/org/eclipse/jetty/http/MimeTypes.java)

Added jetty-http/src/main/java/org/eclipse/jetty/http/MimeTypes.java (line 445)

(source file not found at this revision: jetty-http/src/main/java/org/eclipse/jetty/http/MimeTypes.java)

Added jetty-http/src/main/java/org/eclipse/jetty/http/pathmap/ServletPathSpec.java (line 93)

(source file not found at this revision: jetty-http/src/main/java/org/eclipse/jetty/http/pathmap/ServletPathSpec.java)

Added jetty-http/src/main/java/org/eclipse/jetty/http/pathmap/ServletPathSpec.java (line 414)

(source file not found at this revision: jetty-http/src/main/java/org/eclipse/jetty/http/pathmap/ServletPathSpec.java)

Added jetty-io/src/test/java/org/eclipse/jetty/io/ArrayByteBufferPoolTest.java (line 94)

(source file not found at this revision: jetty-io/src/test/java/org/eclipse/jetty/io/ArrayByteBufferPoolTest.java)

Added jetty-server/src/test/java/org/eclipse/jetty/server/HttpServerTestBase.java (line 744)

(source file not found at this revision: jetty-server/src/test/java/org/eclipse/jetty/server/HttpServerTestBase.java)

Added jetty-server/src/test/java/org/eclipse/jetty/server/HttpServerTestBase.java (line 835)

(source file not found at this revision: jetty-server/src/test/java/org/eclipse/jetty/server/HttpServerTestBase.java)

Added jetty-util-ajax/src/main/java/org/eclipse/jetty/util/ajax/AsyncJSON.java (line 1275)

(source file not found at this revision: jetty-util-ajax/src/main/java/org/eclipse/jetty/util/ajax/AsyncJSON.java)

Added jetty-util/src/main/java/org/eclipse/jetty/util/StringUtil.java (line 901)

(source file not found at this revision: jetty-util/src/main/java/org/eclipse/jetty/util/StringUtil.java)

Added jetty-util/src/main/java/org/eclipse/jetty/util/StringUtil.java (line 907)

(source file not found at this revision: jetty-util/src/main/java/org/eclipse/jetty/util/StringUtil.java)

S9360 (java) on eclipse-jetty-similar-to-main - 0 issues removed, 45 issues added - new ruling file

Added jetty-http/src/main/java/org/eclipse/jetty/http/HttpStatus.java (line 343)

(source file not found at this revision: jetty-http/src/main/java/org/eclipse/jetty/http/HttpStatus.java)

Added jetty-http/src/main/java/org/eclipse/jetty/http/HttpStatus.java (line 358)

(source file not found at this revision: jetty-http/src/main/java/org/eclipse/jetty/http/HttpStatus.java)

Added jetty-http/src/main/java/org/eclipse/jetty/http/HttpStatus.java (line 373)

(source file not found at this revision: jetty-http/src/main/java/org/eclipse/jetty/http/HttpStatus.java)

Added jetty-http/src/main/java/org/eclipse/jetty/http/HttpURI.java (line 1028)

(source file not found at this revision: jetty-http/src/main/java/org/eclipse/jetty/http/HttpURI.java)

Added jetty-http/src/main/java/org/eclipse/jetty/http/MimeTypes.java (line 431)

(source file not found at this revision: jetty-http/src/main/java/org/eclipse/jetty/http/MimeTypes.java)

Added jetty-http/src/main/java/org/eclipse/jetty/http/MimeTypes.java (line 436)

(source file not found at this revision: jetty-http/src/main/java/org/eclipse/jetty/http/MimeTypes.java)

Added jetty-http/src/main/java/org/eclipse/jetty/http/MimeTypes.java (line 445)

(source file not found at this revision: jetty-http/src/main/java/org/eclipse/jetty/http/MimeTypes.java)

Added jetty-http/src/main/java/org/eclipse/jetty/http/pathmap/ServletPathSpec.java (line 93)

(source file not found at this revision: jetty-http/src/main/java/org/eclipse/jetty/http/pathmap/ServletPathSpec.java)

Added jetty-http/src/main/java/org/eclipse/jetty/http/pathmap/ServletPathSpec.java (line 414)

(source file not found at this revision: jetty-http/src/main/java/org/eclipse/jetty/http/pathmap/ServletPathSpec.java)

Added jetty-io/src/test/java/org/eclipse/jetty/io/ArrayByteBufferPoolTest.java (line 94)

(source file not found at this revision: jetty-io/src/test/java/org/eclipse/jetty/io/ArrayByteBufferPoolTest.java)

Added jetty-server/src/test/java/org/eclipse/jetty/server/HttpServerTestBase.java (line 744)

(source file not found at this revision: jetty-server/src/test/java/org/eclipse/jetty/server/HttpServerTestBase.java)

Added jetty-server/src/test/java/org/eclipse/jetty/server/HttpServerTestBase.java (line 835)

(source file not found at this revision: jetty-server/src/test/java/org/eclipse/jetty/server/HttpServerTestBase.java)

S9360 (java) on guava - 0 issues removed, 5 issues added - new ruling file

Added src/com/google/common/base/SmallCharMatcher.java (line 61)

        56 |   static int smear(int hashCode) {
        57 |     return C2 * Integer.rotateLeft(hashCode * C1, 15);
        58 |   }
        59 | 
        60 |   private boolean checkFilter(int c) {
>>>     61 |     return 1 == (1 & (filter >> c));
        62 |   }
        63 | 
        64 |   // This is all essentially copied from ImmutableSet, but we have to duplicate because
        65 |   // of dependencies.
        66 | 

Added src/com/google/common/io/LittleEndianDataInputStream.java (line 82)

        77 |   }
        78 | 
        79 |   @Override
        80 |   public int readUnsignedByte() throws IOException {
        81 |     int b1 = in.read();
>>>     82 |     if (0 > b1) {
        83 |       throw new EOFException();
        84 |     }
        85 |     
        86 |     return b1;
        87 |   }

Added src/com/google/common/net/MediaType.java (line 628)

       623 |         tokenizer.consumeCharacter(';');
       624 |         tokenizer.consumeTokenIfPresent(LINEAR_WHITE_SPACE);
       625 |         String attribute = tokenizer.consumeToken(TOKEN_MATCHER);
       626 |         tokenizer.consumeCharacter('=');
       627 |         final String value;
>>>    628 |         if ('"' == tokenizer.previewChar()) {
       629 |           tokenizer.consumeCharacter('"');
       630 |           StringBuilder valueBuilder = new StringBuilder();
       631 |           while ('"' != tokenizer.previewChar()) {
       632 |             if ('\\' == tokenizer.previewChar()) {
       633 |               tokenizer.consumeCharacter('\\');

Added src/com/google/common/net/MediaType.java (line 631)

       626 |         tokenizer.consumeCharacter('=');
       627 |         final String value;
       628 |         if ('"' == tokenizer.previewChar()) {
       629 |           tokenizer.consumeCharacter('"');
       630 |           StringBuilder valueBuilder = new StringBuilder();
>>>    631 |           while ('"' != tokenizer.previewChar()) {
       632 |             if ('\\' == tokenizer.previewChar()) {
       633 |               tokenizer.consumeCharacter('\\');
       634 |               valueBuilder.append(tokenizer.consumeCharacter(ASCII));
       635 |             } else {
       636 |               valueBuilder.append(tokenizer.consumeToken(QUOTED_TEXT_MATCHER));

Added src/com/google/common/net/MediaType.java (line 632)

       627 |         final String value;
       628 |         if ('"' == tokenizer.previewChar()) {
       629 |           tokenizer.consumeCharacter('"');
       630 |           StringBuilder valueBuilder = new StringBuilder();
       631 |           while ('"' != tokenizer.previewChar()) {
>>>    632 |             if ('\\' == tokenizer.previewChar()) {
       633 |               tokenizer.consumeCharacter('\\');
       634 |               valueBuilder.append(tokenizer.consumeCharacter(ASCII));
       635 |             } else {
       636 |               valueBuilder.append(tokenizer.consumeToken(QUOTED_TEXT_MATCHER));
       637 |             }

[github-actions]

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

Please review and merge it into your branch.

[romainbrenguier]

gitar unblock

[github-actions]

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

Please review and merge it into your branch.

[github-actions]

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

Please review and merge it into your branch.

[gitar-bot]

Code Review ✅ Approved 5 resolved / 5 findings

Implements rule S9360 to detect Yoda conditions with constant expressions and comparisons, addressing the previous compilation, rule description, compliant example, error message, and operator coverage findings.

✅ 5 resolved

✅ Bug: Test sample does not compile: null == int breaks module build

📄 java-checks-test-sources/default/src/main/java/checks/YodaConditionCheckSample.java:64-69
Line 67 of the compiling sample writes if (((null)) == count) where count is declared int on line 65. javac rejects this (bad operand types for binary operator '==' first type: <null> second type: int), and java-checks-test-sources/default compiles src/main/java with the maven-compiler-plugin (release 26), so this file is a hard build break — files that legitimately fail to compile belong under src/main/files/non-compiling/. Compare the null literal against a reference-typed variable instead (marker columns stay valid since null remains at columns 11-14).

✅ Bug: Rule description promises constant precomputation the check never raises

📄 sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S9360.html:1-11 📄 sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S9360.html:34-38 📄 sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S9360.html:56-58 📄 java-checks/src/main/java/org/sonar/java/checks/YodaConditionCheck.java:30-44 📄 java-checks-test-sources/default/src/main/java/checks/YodaConditionCheckSample.java:82-90
The rule title, JSON and the first half of S9360.html describe two behaviours — precomputing constant function calls (Math.max(5, 10) is shown as Noncompliant) and Yoda conditions — but YodaConditionCheck only subscribes to EQUAL_TO/NOT_EQUAL_TO/LESS_THAN/GREATER_THAN and never inspects method invocations. The shipped sample even asserts the opposite of the documentation (Object obj = Math.max(5, 10); // Compliant - method call), so users who see the Math.max example in the description will never get that issue (constant math is a different rule, S2185/ConstantMathCheck). Either drop the constant-function-call sections and the Math resource link from the description so it matches the implemented Yoda-condition scope, or implement the missing part.

✅ Quality: Compliant example recommends code that rule S1125 flags

📄 sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S9360.html:53-67 📄 java-checks/src/main/java/org/sonar/java/checks/YodaConditionCheck.java:47
The "Compliant solution" block recommends if (obj == null && flag == true). flag == true is exactly what S1125 (BooleanLiteralCheck, active in Sonar way) reports as a redundant boolean literal, so following this rule's guidance produces an issue from another default-active rule. Additionally double result = 4.0 + 5; is presented as the precomputed form while still being an unevaluated constant expression, contradicting the section it illustrates. Use if (obj == null && flag) and a fully computed literal (9.0).

✅ Bug: Message for < and > tells users to make a semantics-changing swap

📄 java-checks/src/main/java/org/sonar/java/checks/YodaConditionCheck.java:41-49 📄 java-checks-test-sources/default/src/main/java/checks/YodaConditionCheckSample.java:71-80
For 0 < count and 5 > x the check emits "Put the variable on the left side of this comparison." Taken literally that yields count < 0 / x > 5, which inverts the condition; the correct rewrite also flips the operator (count > 0, x < 5), as the PR description itself states. Neither the message nor S9360.html mentions operator flipping for relational operators, so the guidance is actively misleading. Emit an operator-aware message for LESS_THAN/GREATER_THAN and document the relational case in the description.

✅ Edge Case: <= and >= Yoda conditions are not detected

📄 java-checks/src/main/java/org/sonar/java/checks/YodaConditionCheck.java:31-38
nodesToVisit() covers only EQUAL_TO, NOT_EQUAL_TO, LESS_THAN and GREATER_THAN, so 0 <= count and 5 >= x — the same Yoda pattern with the other two relational operators, whose kinds LESS_THAN_OR_EQUAL_TO/GREATER_THAN_OR_EQUAL_TO exist in the API and are used by other checks — are silently accepted. Add both kinds (and sample cases) so the rule is consistent across relational operators.

Implementation Status ✅ 1 of 1 objectives covered

✅ SONARJAVA-6825 - 1 of 1 objectives covered

This PR implements rule S9360 to detect and report Yoda conditions as part of the SONARJAVA-6825 objective.

✅ 1 covered here

• ✅ Create rule S9360: Constant expressions and comparisons should be simplified

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
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube

[nathsou]

Requesting changes for three blocking scope issues: verified duplicate findings with existing Java rules, divergence from the authoritative draft RSPEC, and incomplete handling of constants. The duplicate-report cases were reproduced with focused CheckVerifier tests on this exact head.

[nathsou]

These literal kinds cause verified duplicate findings for equality operators. I ran focused CheckVerifier tests on this head with exactly each relevant rule pair enabled: true == flag produced S9360 + S1125, "hello" == text produced S9360 + S4973, and 0.0 == value produced S9360 + S1244. S1125 and S4973 are both in Sonar Way, so the first two duplicates occur in the default profile. Please make equality handling operator/type-aware: defer boolean equality to S1125, String equality to S4973, and floating-point equality to S1244, while retaining non-overlapping relational comparisons.

[nathsou]

This recognizes only bare literal AST nodes, not Java constant expressions. For example, -1 == result is a unary-minus expression and is silently missed; named constants such as Integer.MAX_VALUE == result and expressions such as 1 + 1 == result are missed as well. That conflicts with the rule's user-facing constant/Yoda-condition scope. Please either recognize constant expressions (at minimum unary +/- numeric literals) or explicitly narrow the specification and title to bare literals, with tests documenting the boundary.

[romainbrenguier]

superseeded by #6036