Skip to content

SONARJAVA-6788: Implement S9361: Duplicate keys or elements should not be passed to immutable collection factory methods - #5988

Open
nathsou wants to merge 4 commits into
masterfrom
new-rule/S9361
Open

SONARJAVA-6788: Implement S9361: Duplicate keys or elements should not be passed to immutable collection factory methods#5988
nathsou wants to merge 4 commits into
masterfrom
new-rule/S9361

Conversation

@nathsou

@nathsou nathsou commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Implement S9361 (DuplicateImmutableCollectionArgumentsCheck) in SonarJava.
  • Flag duplicate keys in Map.of(...) and Map.ofEntries(...).
  • Flag duplicate elements in Set.of(...).
  • Add comprehensive test samples and .withoutSemantic() check verifier tests.
  • Add generated rule metadata and Sonar way profile entry.

Links

AI disclosure

  • LLM model used for implementation: gemini-3.7-flash-high

@nathsou nathsou self-assigned this Aug 21, 2026
@hashicorp-vault-sonar-prod

hashicorp-vault-sonar-prod Bot commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

SONARJAVA-6788

Comment thread sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S9361.html Outdated
@datadog-sonarsource

This comment has been minimized.

@gitar-bot

gitar-bot Bot commented Aug 21, 2026

Copy link
Copy Markdown
Code Review ✅ Approved 3 resolved / 3 findings

Implements rule S9361 to flag duplicate keys or elements in immutable collection factory methods, resolving infinite recursion on cyclic field initializers and fixing HTML formatting in the metadata.

✅ 3 resolved
Quality: HTML 'How to fix it' uses markdown bullets that won't render

📄 sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S9361.html:10-13
The <p> in the "How to fix it" section embeds * For Map.of()... and * For Set.of()... as inline markdown-style bullets. In rendered HTML these asterisks appear as literal text on a single run-on line instead of a list. Use proper <ul>/<li> markup so the guidance renders as a list.

Edge Case: resolveExpression can recurse infinitely on cyclic field initializers

📄 java-checks/src/main/java/org/sonar/java/checks/DuplicateImmutableCollectionArgumentsCheck.java:172-184
resolveExpression recurses through getSingleWriteUsage with no visited/depth guard, unlike the analogous ExpressionsHelper.valueResolution which tracks evaluatedSymbols to break cycles. Mutually- or self-referential field constants (e.g. static int a = b; static int b = a; or int a = a;) each report a single write usage pointing back at the other, so resolving one key against another in Map.of(a, 1, b, 2) recurses forever and throws StackOverflowError, aborting analysis of the file. Track already-visited symbols (or cap recursion depth) and stop when a symbol repeats.

Bug: withoutSemantic test expects issues that require semantics

📄 java-checks/src/test/java/org/sonar/java/checks/DuplicateImmutableCollectionArgumentsCheckTest.java:34-41 📄 java-checks/src/main/java/org/sonar/java/checks/DuplicateImmutableCollectionArgumentsCheck.java:76-82
test_without_semantic calls .withoutSemantic().verifyIssues(), but the check reports issues only when MAP_OF/MAP_OF_ENTRIES/SET_OF (all MethodMatchers on fully-qualified types like java.util.Map) match. Without semantics, id.symbol() is unknown so MethodMatchers.matches returns false and no issues are raised, while verifyIssues() expects every // Noncompliant line in the sample to be flagged. The test will fail. Change the assertion to .verifyNoIssues() (the conventional pattern for semantics-dependent checks).

Implementation Status ✅ 1 / 1 issues implemented
SONARJAVA-6788 — 1 / 1 objectives

The PR successfully implements rule S9361, detecting duplicate keys or elements passed to Map.of(), Map.ofEntries(), and Set.of() with comprehensive tests and documentation.

✅ 1 complete
  • ✅ Implement rule S9361 to detect duplicate keys or elements passed to Java immutable collection factory methods like Map.of(), Map.ofEntries(), and Set.of()
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

Copy link
Copy Markdown
Contributor

@romainbrenguier romainbrenguier left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some changes to make on the rspec side, but the java implementation looks good.

Comment on lines +1 to +5
<p>Java 9 introduced immutable collection factory methods: <code>Map.of()</code>, <code>Map.ofEntries()</code>, and <code>Set.of()</code>. Unlike
mutable collections such as <code>HashMap</code> (where duplicate keys overwrite previous entries) or <code>HashSet</code> (where duplicate elements
are ignored), these static factory methods disallow duplicate keys and elements.</p>
<p>When duplicate keys are passed to <code>Map.of()</code> or <code>Map.ofEntries()</code>, or duplicate elements are passed to <code>Set.of()</code>,
an <code>IllegalArgumentException</code> is thrown at runtime during collection creation.</p>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The introduction of the rule should start by describing when/under which conditions the issue is raised.

href="https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/util/Map.html#ofEntries(java.util.Map.Entry…​)">Method
Map.ofEntries</a></li>
<li>Java Documentation - <a href="https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/util/Set.html#of(E…​)">Method Set.of</a></li>
<li>{rule:java:S4143} - Map values should not be replaced unconditionally</li>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should go to a === Related rules section

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants