SONARJAVA-6824: Implemented rule S9358 - Conditional expressions should not duplicate operations in both branches

[romainbrenguier]

This PR implements rule S9358 which detects when a ternary operator applies the same operation (method invocation, object creation, or array access) to different arguments in both branches.

Such patterns can be refactored by moving the condition inside the operation for better readability:

The rule handles:

• Method invocations with same method name and different arguments
• Object creation ( vs )
• Array access ( vs )

Test cases cover both compliant and non-compliant scenarios including edge cases like nested ternaries, chained method calls, and generic type arguments.

[hashicorp-vault-sonar-prod]

SONARJAVA-6824

[github-actions]

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

Please review and merge it into your branch.

[github-actions]

Ruling Diff Summary

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

S9358 (java) on eclipse-jetty - 0 issues removed, 5 issues added - new ruling file

Added jetty-http/src/main/java/org/eclipse/jetty/http/QuotedQualityCSV.java (line 129)

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

Added jetty-jmx/src/main/java/org/eclipse/jetty/jmx/MBeanContainer.java (line 362)

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

Added jetty-jmx/src/main/java/org/eclipse/jetty/jmx/MBeanContainer.java (line 373)

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

Added jetty-util/src/main/java/org/eclipse/jetty/util/JavaVersion.java (line 58)

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

Added jetty-util/src/test/java/org/eclipse/jetty/util/statistic/CounterStatisticTest.java (line 91)

(source file not found at this revision: jetty-util/src/test/java/org/eclipse/jetty/util/statistic/CounterStatisticTest.java)

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

Added jetty-http/src/main/java/org/eclipse/jetty/http/QuotedQualityCSV.java (line 129)

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

Added jetty-jmx/src/main/java/org/eclipse/jetty/jmx/MBeanContainer.java (line 362)

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

Added jetty-jmx/src/main/java/org/eclipse/jetty/jmx/MBeanContainer.java (line 373)

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

S9358 (java) on guava - 0 issues removed, 4 issues added - new ruling file

Added src/com/google/common/collect/RegularImmutableTable.java (line 151)

       146 |     for (Cell<R, C, V> cell : cells) {
       147 |       rowSpaceBuilder.add(cell.getRowKey());
       148 |       columnSpaceBuilder.add(cell.getColumnKey());
       149 |     }
       150 | 
>>>    151 |     ImmutableSet<R> rowSpace = (rowComparator == null)
       152 |         ? ImmutableSet.copyOf(rowSpaceBuilder)
       153 |         : ImmutableSet.copyOf(
       154 |             Ordering.from(rowComparator).immutableSortedCopy(rowSpaceBuilder));
       155 |     ImmutableSet<C> columnSpace = (columnComparator == null)
       156 |         ? ImmutableSet.copyOf(columnSpaceBuilder)

Added src/com/google/common/collect/RegularImmutableTable.java (line 155)

       150 | 
       151 |     ImmutableSet<R> rowSpace = (rowComparator == null)
       152 |         ? ImmutableSet.copyOf(rowSpaceBuilder)
       153 |         : ImmutableSet.copyOf(
       154 |             Ordering.from(rowComparator).immutableSortedCopy(rowSpaceBuilder));
>>>    155 |     ImmutableSet<C> columnSpace = (columnComparator == null)
       156 |         ? ImmutableSet.copyOf(columnSpaceBuilder)
       157 |         : ImmutableSet.copyOf(
       158 |             Ordering.from(columnComparator).immutableSortedCopy(columnSpaceBuilder));
       159 | 
       160 |     // use a dense table if more than half of the cells have values

Added src/com/google/common/collect/TreeMultiset.java (line 91)

        86 |    *          the comparator that will be used to sort this multiset. A null value indicates that
        87 |    *          the elements' <i>natural ordering</i> should be used.
        88 |    */
        89 |   @SuppressWarnings("unchecked")
        90 |   public static <E> TreeMultiset<E> create(@Nullable Comparator<? super E> comparator) {
>>>     91 |     return (comparator == null)
        92 |         ? new TreeMultiset<E>((Comparator) Ordering.natural())
        93 |         : new TreeMultiset<E>(comparator);
        94 |   }
        95 | 
        96 |   /**

Added src/com/google/common/hash/MessageDigestHashFunction.java (line 156)

       151 | 
       152 |     @Override
       153 |     public HashCode hash() {
       154 |       checkNotDone();
       155 |       done = true;
>>>    156 |       return (bytes == digest.getDigestLength())
       157 |           ? HashCode.fromBytesNoCopy(digest.digest())
       158 |           : HashCode.fromBytesNoCopy(Arrays.copyOf(digest.digest(), bytes));
       159 |     }
       160 |   }
       161 | }

S9358 (java) on sonar-server - 0 issues removed, 4 issues added - new ruling file

Added src/main/java/org/sonar/server/computation/task/projectanalysis/qualitygate/ConditionEvaluator.java (line 113)

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

Added src/main/java/org/sonar/server/qualitygate/QualityGateConditionsUpdater.java (line 175)

(source file not found at this revision: src/main/java/org/sonar/server/qualitygate/QualityGateConditionsUpdater.java)

Added src/main/java/org/sonar/server/qualityprofile/RuleActivator.java (line 200)

(source file not found at this revision: src/main/java/org/sonar/server/qualityprofile/RuleActivator.java)

Added src/main/java/org/sonar/server/setting/ws/ValuesAction.java (line 247)

(source file not found at this revision: src/main/java/org/sonar/server/setting/ws/ValuesAction.java)

[github-actions]

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

Please review and merge it into your branch.

[github-actions]

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

Please review and merge it into your branch.

[github-actions]

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

Please review and merge it into your branch.

[github-actions]

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

Please review and merge it into your branch.

[nathsou]

Approved with 2 findings I think are worth addressing:

[nathsou]

Method names do not identify the invoked operation. For example, condition ? overloaded(1) : overloaded("x") invokes distinct overloads but is reported, while condition ? foo(a) : this.foo(b) invokes the same method but is missed. The suggested refactoring for the overload case may not compile. Please compare resolved method symbols when semantic information is available, and avoid reporting ambiguous syntax-only matches.

[romainbrenguier]

Addressed in commit 0e2ef4b: the check now compares resolved method symbols via sameMethodSymbol. When semantic information is available, overloaded methods with different signatures (e.g. overloaded(int) vs overloaded(String)) are correctly identified as different operations and not reported. When symbols are unknown (no semantics), the check falls back to treating them as the same method to avoid false negatives. Additionally, sameTree now uses SyntacticEquivalence.areEquivalent for proper structural comparison, and test cases have been added for these scenarios.

[nathsou]

This comparison ignores constructor identity and the rest of the creation context. condition ? new Overloaded(1) : new Overloaded("x") is reported even when those arguments select different constructors; enclosing instances and anonymous class bodies are ignored as well. Please compare resolved constructor symbols and relevant NewClassTree context before treating these branches as the same operation.

[romainbrenguier]

Addressed in commit 0e2ef4b: sameNewClass now compares resolved constructor symbols via sameMethodSymbol, checks for anonymous class bodies (returns false if either has a class body), and compares enclosing expressions via sameNullableTree. Test cases have been added for overloaded constructors, anonymous class bodies, and qualified instantiations (obj.new Inner(a) vs obj.new Inner(b)).

[romainbrenguier]

Addressed in commit 0e2ef4b: sameNewClass now compares resolved constructor symbols via sameMethodSymbol, checks for anonymous class bodies (returns false if either has a class body), and compares enclosing expressions via sameNullableTree. Test cases have been added for overloaded constructors, anonymous class bodies, and qualified instantiations (obj.new Inner(a) vs obj.new Inner(b)).

[github-actions]

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

Please review and merge it into your branch.

[gitar-bot]

Code Review ✅ Approved 6 resolved / 6 findings

Implements rule S9358 to detect duplicated operations in conditional expression branches with comprehensive test coverage. Addressed missing resource files, argument difference counting, and edge cases with parentheses and nested ternaries.

✅ 6 resolved

✅ Bug: Missing S9358 rule metadata (JSON/HTML) resource files

📄 java-checks/src/main/java/org/sonar/java/checks/TernaryOperatorSameOperationCheck.java:33
The rule declares @Rule(key = "S9358") but no S9358.json/S9358.html metadata files exist under org/sonar/l10n/java/rules/java/. Without these resources the rule cannot be registered/activated and the rule-definition tests will fail. Add the metadata files (and register the check in the check registrar if manual registration is required) mirroring an existing rule such as S9350.

✅ Bug: Parentheses not unwrapped; nested-ternary test case won't be detected

📄 java-checks/src/main/java/org/sonar/java/checks/TernaryOperatorSameOperationCheck.java:52-66 📄 java-checks-test-sources/default/src/main/java/checks/TernaryOperatorSameOperationCheckSample.java:38
The sample marks condition ? (other ? foo(x) : foo(y)) : foo(z) (line 38) as Noncompliant, but trueExpression() here is a PARENTHESIZED_EXPRESSION, so hasSameOperationStructure falls through all is(...) checks and returns false — no issue is raised and the test fails. Unwrap both branches with ExpressionUtils.skipParentheses(...) before comparing (as AllBranchesAreIdenticalCheck does).

✅ Edge Case: Only flags calls where every argument differs

📄 java-checks/src/main/java/org/sonar/java/checks/TernaryOperatorSameOperationCheck.java:80-85 📄 java-checks/src/main/java/org/sonar/java/checks/TernaryOperatorSameOperationCheck.java:128-132
sameMethodInvocation/sameNewClass return false as soon as any single argument pair is equal, so a case like foo(a, x) : foo(b, x) (one differing arg, one shared) is not reported even though the condition could be pushed into the differing argument. If the intent is 'at least one argument differs', invert the logic to report when any argument differs (and none... adjust to your semantics). Confirm the intended contract and adjust the loop accordingly.

✅ Bug: hasExactlyOneArgumentDifference flags any number of differing args

📄 java-checks/src/main/java/org/sonar/java/checks/TernaryOperatorSameOperationCheck.java:96-107
The method name promises "exactly one" differing argument, but the implementation only tracks a boolean anyDifferent and returns true whenever at least one argument differs. For calls where two or more arguments differ (e.g. condition ? foo(a, b) : foo(c, d)), the check fires, yet the suggested refactoring "Move the conditional expression inside this operation" cannot be applied with a single conditional — you'd need one ternary per differing argument. This yields false positives and misleading guidance. Track a difference counter and return count == 1 to match the intended semantics.

✅ Quality: New sameNullableTree/enclosingExpression branches are untested

📄 java-checks/src/main/java/org/sonar/java/checks/TernaryOperatorSameOperationCheck.java:115-126 📄 java-checks/src/main/java/org/sonar/java/checks/TernaryOperatorSameOperationCheck.java:82-96 📄 java-checks-test-sources/default/src/main/java/checks/TernaryOperatorSameOperationCheckSample.java:144-156
This commit adds sameNullableTree and the enclosingExpression() comparison in sameNewClass, but neither sample file contains a qualified instantiation (outer.new Inner(...)), so only the left == null && right == null branch ever executes — the one-null branch (L122-124) and the sameTree branch (L125) are dead in the test suite. The same applies to the sameTree fallback in sameMethodSelect (L95), which needs a non-identifier/non-member-select receiver such as getObj().foo(a) : getObj().foo(b). Add cases to TernaryOperatorSameOperationCheckSample.java so the new exclusion logic is actually verified (and to satisfy coverage on new code).

...and 1 more resolved from earlier reviews

Implementation Status ◻️ 0 of 1 objectives covered

◻️ SONARJAVA-6824 - 0 of 1 objectives covered

This PR does not cover the implementation of rule S9358 as the diff is empty or unrelated.

Other objectives on this issue, possibly covered elsewhere:

• ◻️ Implement rule S9358: Conditional expressions should not duplicate operations in both branches

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

See analysis details on SonarQube