diff --git a/java-checks-test-sources/default/src/main/java/checks/DuplicateImmutableCollectionArgumentsCheckSample.java b/java-checks-test-sources/default/src/main/java/checks/DuplicateImmutableCollectionArgumentsCheckSample.java new file mode 100644 index 00000000000..2d1731ceff1 --- /dev/null +++ b/java-checks-test-sources/default/src/main/java/checks/DuplicateImmutableCollectionArgumentsCheckSample.java @@ -0,0 +1,213 @@ +package checks; + +import java.util.List; +import java.util.Map; +import java.util.Set; +import java.util.UUID; +import static java.util.Map.entry; + +class DuplicateImmutableCollectionArgumentsCheckSample { + + private static final String CONST_A = "keyA"; + private static final String CONST_B = "keyB"; + private static final String CONST_A_ALIAS = "keyA"; + + private static final int INT_A = 1; + private static final int INT_B = 2; + private static final int INT_A_ALIAS = 1; + + enum Color { + RED, GREEN, BLUE + } + + private static final Color RED_ALIAS = Color.RED; + + void testMapOf() { + Map empty = Map.of(); // Compliant + Map single = Map.of("a", 1); // Compliant + Map distinct = Map.of("a", 1, "b", 2, "c", 3); // Compliant + + Map duplicateLiteral = Map.of( + "timeout", 30, + "retries", 3, + "timeout", 60 // Noncompliant {{Remove or rename this duplicate key; "Map.of" throws an "IllegalArgumentException" at runtime when keys are duplicated.}} [[secondary=-2]] + ); + + Map multipleDuplicates = Map.of( + "k1", 1, + "k2", 2, + "k1", 3, // Noncompliant {{Remove or rename this duplicate key; "Map.of" throws an "IllegalArgumentException" at runtime when keys are duplicated.}} [[secondary=-2]] + "k2", 4, // Noncompliant {{Remove or rename this duplicate key; "Map.of" throws an "IllegalArgumentException" at runtime when keys are duplicated.}} [[secondary=-2]] + "k1", 5 // Noncompliant {{Remove or rename this duplicate key; "Map.of" throws an "IllegalArgumentException" at runtime when keys are duplicated.}} [[secondary=-4]] + ); + + Map duplicateConstants = Map.of( + CONST_A, 1, + CONST_B, 2, + CONST_A_ALIAS, 3 // Noncompliant {{Remove or rename this duplicate key; "Map.of" throws an "IllegalArgumentException" at runtime when keys are duplicated.}} [[secondary=-2]] + ); + + Map duplicateConstAndLiteral = Map.of( + CONST_A, 1, + CONST_B, 2, + "keyA", 3 // Noncompliant {{Remove or rename this duplicate key; "Map.of" throws an "IllegalArgumentException" at runtime when keys are duplicated.}} [[secondary=-2]] + ); + + Map duplicateIntKeys = Map.of( + 1, "one", + 2, "two", + 1, "uno" // Noncompliant {{Remove or rename this duplicate key; "Map.of" throws an "IllegalArgumentException" at runtime when keys are duplicated.}} [[secondary=-2]] + ); + + Map duplicateAliasedInts = Map.of( + INT_A, "one", + INT_B, "two", + INT_A_ALIAS, "uno" // Noncompliant {{Remove or rename this duplicate key; "Map.of" throws an "IllegalArgumentException" at runtime when keys are duplicated.}} [[secondary=-2]] + ); + + Map duplicateIntAndConst = Map.of( + INT_A, "one", + INT_B, "two", + 1, "uno" // Noncompliant {{Remove or rename this duplicate key; "Map.of" throws an "IllegalArgumentException" at runtime when keys are duplicated.}} [[secondary=-2]] + ); + + Map duplicateBoolKeys = Map.of( + true, "yes", + false, "no", + true, "oui" // Noncompliant {{Remove or rename this duplicate key; "Map.of" throws an "IllegalArgumentException" at runtime when keys are duplicated.}} [[secondary=-2]] + ); + + Map parenthesizedKeys = Map.of( + ("key"), 1, + "other", 2, + "key", 3 // Noncompliant {{Remove or rename this duplicate key; "Map.of" throws an "IllegalArgumentException" at runtime when keys are duplicated.}} [[secondary=-2]] + ); + } + + void testMapOfVariables(String varKey1, String varKey2) { + Map distinctVars = Map.of(varKey1, 1, varKey2, 2); // Compliant + Map sameVar = Map.of( + varKey1, 1, + varKey2, 2, + varKey1, 3 // Noncompliant {{Remove or rename this duplicate key; "Map.of" throws an "IllegalArgumentException" at runtime when keys are duplicated.}} [[secondary=-2]] + ); + } + + void testMapOfEntries() { + Map empty = Map.ofEntries(); // Compliant + Map single = Map.ofEntries(Map.entry("a", "b")); // Compliant + Map distinct = Map.ofEntries( + Map.entry("k1", "v1"), + Map.entry("k2", "v2"), + entry("k3", "v3") + ); // Compliant + + Map duplicateEntries = Map.ofEntries( + Map.entry("host", "localhost"), + Map.entry("port", "8080"), + Map.entry("host", "remotehost") // Noncompliant {{Remove or rename this duplicate key; "Map.ofEntries" throws an "IllegalArgumentException" at runtime when keys are duplicated.}} [[secondary=-2]] + ); + + Map duplicateStaticImport = Map.ofEntries( + entry("user", "admin"), + entry("user", "guest") // Noncompliant {{Remove or rename this duplicate key; "Map.ofEntries" throws an "IllegalArgumentException" at runtime when keys are duplicated.}} [[secondary=-1]] + ); + + Map.Entry entryVariable = Map.entry("dynamic", "val"); + Map entryVarMap = Map.ofEntries(entryVariable); // Compliant + } + + void testSetOf() { + Set empty = Set.of(); // Compliant + Set single = Set.of("a"); // Compliant + Set distinct = Set.of("a", "b", "c"); // Compliant + + Set duplicateStrings = Set.of( + "read", + "write", + "read" // Noncompliant {{Remove or replace this duplicate element; "Set.of" throws an "IllegalArgumentException" at runtime when elements are duplicated.}} [[secondary=-2]] + ); + + Set duplicateConcat = Set.of( + "ab", + "cd", + "a" + "b" // Noncompliant {{Remove or replace this duplicate element; "Set.of" throws an "IllegalArgumentException" at runtime when elements are duplicated.}} [[secondary=-2]] + ); + + Set duplicateNumbers = Set.of( + 10, + 20, + 10 // Noncompliant {{Remove or replace this duplicate element; "Set.of" throws an "IllegalArgumentException" at runtime when elements are duplicated.}} [[secondary=-2]] + ); + + Set duplicateConsts = Set.of( + CONST_A, + CONST_B, + CONST_A // Noncompliant {{Remove or replace this duplicate element; "Set.of" throws an "IllegalArgumentException" at runtime when elements are duplicated.}} [[secondary=-2]] + ); + + Set duplicateEnums = Set.of( + Color.RED, + Color.GREEN, + Color.RED // Noncompliant {{Remove or replace this duplicate element; "Set.of" throws an "IllegalArgumentException" at runtime when elements are duplicated.}} [[secondary=-2]] + ); + + Set duplicateAliasedEnums = Set.of( + Color.RED, + Color.BLUE, + RED_ALIAS // Noncompliant {{Remove or replace this duplicate element; "Set.of" throws an "IllegalArgumentException" at runtime when elements are duplicated.}} [[secondary=-2]] + ); + + Set multipleSetDuplicates = Set.of( + "x", + "x", // Noncompliant {{Remove or replace this duplicate element; "Set.of" throws an "IllegalArgumentException" at runtime when elements are duplicated.}} [[secondary=-1]] + "x" // Noncompliant {{Remove or replace this duplicate element; "Set.of" throws an "IllegalArgumentException" at runtime when elements are duplicated.}} [[secondary=-2]] + ); + } + + void testSetOfVariables(String v1, String v2) { + Set distinctVars = Set.of(v1, v2); // Compliant + Set duplicateVars = Set.of( + v1, + v2, + v1 // Noncompliant {{Remove or replace this duplicate element; "Set.of" throws an "IllegalArgumentException" at runtime when elements are duplicated.}} [[secondary=-2]] + ); + } + + void testNonDeterministicExpressionsAreCompliant() { + Set newObjects = Set.of(new Object(), new Object()); // Compliant: each instance is distinct + Set methodCalls = Set.of(generateId(), generateId()); // Compliant: methods can return distinct values + Set randomUuids = Set.of(UUID.randomUUID(), UUID.randomUUID()); // Compliant + } + + void testListOfPermitsDuplicates() { + List list = List.of("dup", "dup", "dup"); // Compliant: List.of allows duplicates + } + + static class CyclicDeclarations { + int a; + int b; + int self; + + void setA() { + a = b; + } + + void setB() { + b = a; + } + + void setSelf() { + self = self; + } + + void testCyclic() { + Map map = Map.of(a, "a", b, "b"); // Compliant + Set set = Set.of(self, 1); // Compliant + } + } + + private String generateId() { + return UUID.randomUUID().toString(); + } +} diff --git a/java-checks/src/main/java/org/sonar/java/checks/DuplicateImmutableCollectionArgumentsCheck.java b/java-checks/src/main/java/org/sonar/java/checks/DuplicateImmutableCollectionArgumentsCheck.java new file mode 100644 index 00000000000..5e8de1125de --- /dev/null +++ b/java-checks/src/main/java/org/sonar/java/checks/DuplicateImmutableCollectionArgumentsCheck.java @@ -0,0 +1,194 @@ +/* + * SonarQube Java + * Copyright (C) SonarSource Sàrl + * mailto:info AT sonarsource DOT com + * + * You can redistribute and/or modify this program under the terms of + * the Sonar Source-Available License Version 1, as published by SonarSource Sàrl. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. + * See the Sonar Source-Available License for more details. + * + * You should have received a copy of the Sonar Source-Available License + * along with this program; if not, see https://sonarsource.com/license/ssal/ + */ +package org.sonar.java.checks; + +import java.util.ArrayList; +import java.util.Collections; +import java.util.HashSet; +import java.util.List; +import java.util.Optional; +import java.util.Set; +import org.sonar.check.Rule; +import org.sonar.java.checks.helpers.ExpressionsHelper; +import org.sonar.java.model.ExpressionUtils; +import org.sonar.java.model.SyntacticEquivalence; +import org.sonar.plugins.java.api.IssuableSubscriptionVisitor; +import org.sonar.plugins.java.api.JavaFileScannerContext; +import org.sonar.plugins.java.api.semantic.MethodMatchers; +import org.sonar.plugins.java.api.semantic.Symbol; +import org.sonar.plugins.java.api.tree.Arguments; +import org.sonar.plugins.java.api.tree.ExpressionTree; +import org.sonar.plugins.java.api.tree.IdentifierTree; +import org.sonar.plugins.java.api.tree.MethodInvocationTree; +import org.sonar.plugins.java.api.tree.Tree; + +@Rule(key = "S9361") +public class DuplicateImmutableCollectionArgumentsCheck extends IssuableSubscriptionVisitor { + + private static final String MESSAGE_TEMPLATE = "Remove or %s this duplicate %s; \"%s\" throws an \"IllegalArgumentException\" at runtime when %s are duplicated."; + private static final String MAP_OF_MESSAGE = String.format(MESSAGE_TEMPLATE, "rename", "key", "Map.of", "keys"); + private static final String MAP_OF_ENTRIES_MESSAGE = String.format(MESSAGE_TEMPLATE, "rename", "key", "Map.ofEntries", "keys"); + private static final String SET_OF_MESSAGE = String.format(MESSAGE_TEMPLATE, "replace", "element", "Set.of", "elements"); + + private static final String FIRST_KEY_SECONDARY_MESSAGE = "First occurrence of this key."; + private static final String FIRST_ELEMENT_SECONDARY_MESSAGE = "First occurrence of this element."; + + private static final String JAVA_UTIL_MAP = "java.util.Map"; + + private static final MethodMatchers MAP_OF = MethodMatchers.create() + .ofTypes(JAVA_UTIL_MAP) + .names("of") + .withAnyParameters() + .build(); + + private static final MethodMatchers MAP_OF_ENTRIES = MethodMatchers.create() + .ofTypes(JAVA_UTIL_MAP) + .names("ofEntries") + .withAnyParameters() + .build(); + + private static final MethodMatchers MAP_ENTRY = MethodMatchers.create() + .ofTypes(JAVA_UTIL_MAP) + .names("entry") + .addParametersMatcher(MethodMatchers.ANY, MethodMatchers.ANY) + .build(); + + private static final MethodMatchers SET_OF = MethodMatchers.create() + .ofTypes("java.util.Set") + .names("of") + .withAnyParameters() + .build(); + + @Override + public List nodesToVisit() { + return Collections.singletonList(Tree.Kind.METHOD_INVOCATION); + } + + @Override + public void visitNode(Tree tree) { + MethodInvocationTree mit = (MethodInvocationTree) tree; + if (MAP_OF.matches(mit)) { + checkMapOf(mit); + } else if (MAP_OF_ENTRIES.matches(mit)) { + checkMapOfEntries(mit); + } else if (SET_OF.matches(mit)) { + checkSetOf(mit); + } + } + + private void checkMapOf(MethodInvocationTree mit) { + Arguments arguments = mit.arguments(); + List keys = new ArrayList<>(); + for (int i = 0; i < arguments.size(); i += 2) { + keys.add(ExpressionUtils.skipParentheses(arguments.get(i))); + } + checkDuplicates(keys, MAP_OF_MESSAGE, FIRST_KEY_SECONDARY_MESSAGE); + } + + private void checkMapOfEntries(MethodInvocationTree mit) { + List keys = new ArrayList<>(); + for (ExpressionTree arg : mit.arguments()) { + ExpressionTree unwrapped = ExpressionUtils.skipParentheses(arg); + if (unwrapped.is(Tree.Kind.METHOD_INVOCATION)) { + MethodInvocationTree entryMit = (MethodInvocationTree) unwrapped; + if (MAP_ENTRY.matches(entryMit) && entryMit.arguments().size() == 2) { + keys.add(ExpressionUtils.skipParentheses(entryMit.arguments().get(0))); + } + } + } + checkDuplicates(keys, MAP_OF_ENTRIES_MESSAGE, FIRST_KEY_SECONDARY_MESSAGE); + } + + private void checkSetOf(MethodInvocationTree mit) { + List elements = new ArrayList<>(); + for (ExpressionTree arg : mit.arguments()) { + elements.add(ExpressionUtils.skipParentheses(arg)); + } + checkDuplicates(elements, SET_OF_MESSAGE, FIRST_ELEMENT_SECONDARY_MESSAGE); + } + + private void checkDuplicates(List expressions, String message, String secondaryMessage) { + List seen = new ArrayList<>(); + for (ExpressionTree expr : expressions) { + ExpressionTree firstOccurrence = findFirstEquivalent(seen, expr); + if (firstOccurrence != null) { + reportIssue( + expr, + message, + Collections.singletonList(new JavaFileScannerContext.Location(secondaryMessage, firstOccurrence)), + null + ); + } else { + seen.add(expr); + } + } + } + + private static ExpressionTree findFirstEquivalent(List seen, ExpressionTree target) { + for (ExpressionTree prior : seen) { + if (areEquivalent(prior, target)) { + return prior; + } + } + return null; + } + + private static boolean areEquivalent(ExpressionTree expr1, ExpressionTree expr2) { + ExpressionTree e1 = resolveExpression(expr1); + ExpressionTree e2 = resolveExpression(expr2); + + Optional const1 = e1.asConstant(); + Optional const2 = e2.asConstant(); + if (const1.isPresent() && const2.isPresent()) { + return const1.get().equals(const2.get()); + } + + String str1 = ExpressionsHelper.getConstantValueAsString(e1).value(); + String str2 = ExpressionsHelper.getConstantValueAsString(e2).value(); + if (str1 != null && str2 != null) { + return str1.equals(str2); + } + + Boolean bool1 = ExpressionsHelper.getConstantValueAsBoolean(e1).value(); + Boolean bool2 = ExpressionsHelper.getConstantValueAsBoolean(e2).value(); + if (bool1 != null && bool2 != null) { + return bool1.equals(bool2); + } + + return ExpressionsHelper.alwaysReturnSameValue(e1) + && ExpressionsHelper.alwaysReturnSameValue(e2) + && SyntacticEquivalence.areEquivalentIncludingSameVariables(e1, e2); + } + + private static ExpressionTree resolveExpression(ExpressionTree expression) { + return resolveExpression(expression, new HashSet<>()); + } + + private static ExpressionTree resolveExpression(ExpressionTree expression, Set visited) { + ExpressionTree current = ExpressionUtils.skipParentheses(expression); + if (current.is(Tree.Kind.IDENTIFIER)) { + Symbol symbol = ((IdentifierTree) current).symbol(); + if (!symbol.isUnknown() && visited.add(symbol)) { + ExpressionTree singleWrite = ExpressionsHelper.getSingleWriteUsage(symbol); + if (singleWrite != null) { + return resolveExpression(singleWrite, visited); + } + } + } + return current; + } +} diff --git a/java-checks/src/test/java/org/sonar/java/checks/DuplicateImmutableCollectionArgumentsCheckTest.java b/java-checks/src/test/java/org/sonar/java/checks/DuplicateImmutableCollectionArgumentsCheckTest.java new file mode 100644 index 00000000000..2d1bcd2fd73 --- /dev/null +++ b/java-checks/src/test/java/org/sonar/java/checks/DuplicateImmutableCollectionArgumentsCheckTest.java @@ -0,0 +1,42 @@ +/* + * SonarQube Java + * Copyright (C) SonarSource Sàrl + * mailto:info AT sonarsource DOT com + * + * You can redistribute and/or modify this program under the terms of + * the Sonar Source-Available License Version 1, as published by SonarSource Sàrl. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. + * See the Sonar Source-Available License for more details. + * + * You should have received a copy of the Sonar Source-Available License + * along with this program; if not, see https://sonarsource.com/license/ssal/ + */ +package org.sonar.java.checks; + +import org.junit.jupiter.api.Test; +import org.sonar.java.checks.verifier.CheckVerifier; + +import static org.sonar.java.checks.verifier.TestUtils.mainCodeSourcesPath; + +class DuplicateImmutableCollectionArgumentsCheckTest { + + @Test + void test() { + CheckVerifier.newVerifier() + .onFile(mainCodeSourcesPath("checks/DuplicateImmutableCollectionArgumentsCheckSample.java")) + .withCheck(new DuplicateImmutableCollectionArgumentsCheck()) + .verifyIssues(); + } + + @Test + void test_without_semantic() { + CheckVerifier.newVerifier() + .onFile(mainCodeSourcesPath("checks/DuplicateImmutableCollectionArgumentsCheckSample.java")) + .withCheck(new DuplicateImmutableCollectionArgumentsCheck()) + .withoutSemantic() + .verifyIssues(); + } +} diff --git a/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S9361.html b/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S9361.html new file mode 100644 index 00000000000..f8a391032c7 --- /dev/null +++ b/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S9361.html @@ -0,0 +1,92 @@ +

Java 9 introduced immutable collection factory methods: Map.of(), Map.ofEntries(), and Set.of(). Unlike +mutable collections such as HashMap (where duplicate keys overwrite previous entries) or HashSet (where duplicate elements +are ignored), these static factory methods disallow duplicate keys and elements.

+

When duplicate keys are passed to Map.of() or Map.ofEntries(), or duplicate elements are passed to Set.of(), +an IllegalArgumentException is thrown at runtime during collection creation.

+

Why is this an issue?

+

Passing duplicate keys or duplicate elements to immutable collection factories is almost always a copy-paste error or a misunderstanding of the API +contract. Because the factory methods validate their arguments at initialization time, duplicate entries cause an immediate runtime crash when the +collection is constructed.

+

How to fix it

+

Review the arguments passed to the factory method:

+
    +
  • For Map.of() and Map.ofEntries(), remove the redundant key-value pair or replace the duplicate key with the intended + distinct key.
  • +
  • For Set.of(), remove the duplicated element or replace it with the intended distinct element.
  • +
+

Code examples

+

Noncompliant code example

+
+import java.util.Map;
+import java.util.Set;
+
+class CollectionFactoryExamples {
+
+  void createMap() {
+    Map<String, Integer> config = Map.of(
+      "timeout", 30,
+      "retries", 3,
+      "timeout", 60 // Noncompliant: "timeout" is duplicated
+    );
+  }
+
+  void createEntriesMap() {
+    Map<String, String> endpoints = Map.ofEntries(
+      Map.entry("auth", "https://auth.example.com"),
+      Map.entry("api", "https://api.example.com"),
+      Map.entry("auth", "https://auth2.example.com") // Noncompliant: "auth" is duplicated
+    );
+  }
+
+  void createSet() {
+    Set<String> permissions = Set.of(
+      "read",
+      "write",
+      "read" // Noncompliant: "read" is duplicated
+    );
+  }
+}
+
+

Compliant solution

+
+import java.util.Map;
+import java.util.Set;
+
+class CollectionFactoryExamples {
+
+  void createMap() {
+    Map<String, Integer> config = Map.of(
+      "timeout", 30,
+      "retries", 3,
+      "port", 60
+    );
+  }
+
+  void createEntriesMap() {
+    Map<String, String> endpoints = Map.ofEntries(
+      Map.entry("auth", "https://auth.example.com"),
+      Map.entry("api", "https://api.example.com"),
+      Map.entry("admin", "https://auth2.example.com")
+    );
+  }
+
+  void createSet() {
+    Set<String> permissions = Set.of(
+      "read",
+      "write",
+      "execute"
+    );
+  }
+}
+
+

Resources

+

Documentation

+ + diff --git a/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S9361.json b/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S9361.json new file mode 100644 index 00000000000..3df98fd0501 --- /dev/null +++ b/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S9361.json @@ -0,0 +1,23 @@ +{ + "title": "Duplicate keys or elements should not be passed to immutable collection factory methods", + "type": "BUG", + "status": "ready", + "remediation": { + "func": "Constant\/Issue", + "constantCost": "5min" + }, + "tags": [ + "bad-practice" + ], + "defaultSeverity": "Major", + "ruleSpecification": "RSPEC-9361", + "sqKey": "S9361", + "scope": "Main", + "quickfix": "unknown", + "code": { + "impacts": { + "RELIABILITY": "HIGH" + }, + "attribute": "LOGICAL" + } +} diff --git a/sonar-java-plugin/src/main/resources/profiles/Sonar_way/S9361 b/sonar-java-plugin/src/main/resources/profiles/Sonar_way/S9361 new file mode 100644 index 00000000000..e69de29bb2d