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

[datadog-sonarsource]

_{This comment will be updated automatically if new data arrives.
🔗 Commit SHA: 4c652ad | Docs | View more details | Give us feedback!}

[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, 5 issues added - new ruling file

Added src/com/google/common/collect/ImmutableList.java (line 209)

       204 |    *
       205 |    * @throws NullPointerException if any of {@code elements} is null
       206 |    */
       207 |   public static <E> ImmutableList<E> copyOf(Iterable<? extends E> elements) {
       208 |     checkNotNull(elements); // TODO(kevinb): is this here only for GWT?
>>>    209 |     return (elements instanceof Collection)
       210 |         ? copyOf((Collection<? extends E>) elements)
       211 |         : copyOf(elements.iterator());
       212 |   }
       213 | 
       214 |   /**

Added src/com/google/common/collect/ImmutableSet.java (line 263)

       258 |    * happen are undefined and subject to change.
       259 |    *
       260 |    * @throws NullPointerException if any of {@code elements} is null
       261 |    */
       262 |   public static <E> ImmutableSet<E> copyOf(Iterable<? extends E> elements) {
>>>    263 |     return (elements instanceof Collection)
       264 |         ? copyOf((Collection<? extends E>) elements)
       265 |         : copyOf(elements.iterator());
       266 |   }
       267 | 
       268 |   /**

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/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, 3 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/ws/ShowAction.java (line 66)

(source file not found at this revision: src/main/java/org/sonar/server/qualitygate/ws/ShowAction.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.

[gitar-bot]

Code Review ✅ Approved 4 resolved / 4 findings

Implements rule S9358 to detect duplicate operations in conditional expression branches with comprehensive test cases, addressing missing metadata resource files, argument difference counting, and nested ternary handling.

✅ 4 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.

Implementation Status ✅ 1 / 1 issues implemented

✅ SONARJAVA-6824 — 1 / 1 objectives

The PR successfully implements rule S9358 with the corresponding check, tests, metadata, documentation, and ruling expectations.

✅ 1 complete

• ✅ 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.4% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube

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

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