diff --git a/its/ruling/src/test/resources/expected/eclipse-jetty/java-S9412.json b/its/ruling/src/test/resources/expected/eclipse-jetty/java-S9412.json new file mode 100644 index 00000000000..c479592845b --- /dev/null +++ b/its/ruling/src/test/resources/expected/eclipse-jetty/java-S9412.json @@ -0,0 +1,14 @@ +{ +"org.eclipse.jetty:jetty-project:jetty-server/src/test/java/org/eclipse/jetty/server/ProxyCustomizerTest.java": [ +97 +], +"org.eclipse.jetty:jetty-project:jetty-server/src/test/java/org/eclipse/jetty/server/handler/ContextHandlerTest.java": [ +792 +], +"org.eclipse.jetty:jetty-project:jetty-util/src/main/java/org/eclipse/jetty/util/PathWatcher.java": [ +840 +], +"org.eclipse.jetty:jetty-project:jetty-xml/src/test/java/org/eclipse/jetty/xml/XmlConfigurationTest.java": [ +693 +] +} diff --git a/its/ruling/src/test/resources/expected/guava/java-S9412.json b/its/ruling/src/test/resources/expected/guava/java-S9412.json new file mode 100644 index 00000000000..cae1771dcca --- /dev/null +++ b/its/ruling/src/test/resources/expected/guava/java-S9412.json @@ -0,0 +1,14 @@ +{ +"com.google.guava:guava:src/com/google/common/collect/ImmutableMultimap.java": [ +280 +], +"com.google.guava:guava:src/com/google/common/collect/Ordering.java": [ +698 +], +"com.google.guava:guava:src/com/google/common/collect/RegularImmutableTable.java": [ +125 +], +"com.google.guava:guava:src/com/google/common/util/concurrent/ServiceManager.java": [ +597 +] +} diff --git a/its/ruling/src/test/resources/expected/sonar-server/java-S9412.json b/its/ruling/src/test/resources/expected/sonar-server/java-S9412.json new file mode 100644 index 00000000000..522555a68f5 --- /dev/null +++ b/its/ruling/src/test/resources/expected/sonar-server/java-S9412.json @@ -0,0 +1,19 @@ +{ +"org.sonarsource.sonarqube:sonar-server:src/main/java/org/sonar/server/computation/task/projectanalysis/source/DuplicationLineReader.java": [ +69 +], +"org.sonarsource.sonarqube:sonar-server:src/main/java/org/sonar/server/computation/task/projectanalysis/source/SymbolsLineReader.java": [ +58, +83 +], +"org.sonarsource.sonarqube:sonar-server:src/main/java/org/sonar/server/duplication/ws/DuplicationsParser.java": [ +75, +78 +], +"org.sonarsource.sonarqube:sonar-server:src/main/java/org/sonar/server/es/IndexDefinitionHash.java": [ +79 +], +"org.sonarsource.sonarqube:sonar-server:src/test/java/org/sonar/server/computation/task/projectanalysis/duplication/TextBlockTest.java": [ +92 +] +} diff --git a/java-checks-test-sources/default/src/main/java/checks/CollectionsSortCheckSample.java b/java-checks-test-sources/default/src/main/java/checks/CollectionsSortCheckSample.java new file mode 100644 index 00000000000..1eda818db7c --- /dev/null +++ b/java-checks-test-sources/default/src/main/java/checks/CollectionsSortCheckSample.java @@ -0,0 +1,75 @@ +package checks; + +import java.util.ArrayList; +import java.util.Arrays; +import java.util.Collections; +import java.util.Comparator; +import java.util.LinkedList; +import java.util.List; + +class CollectionsSortCheckSample { + + void noncompliant(List myList, Comparator myComparator) { + Collections.sort(myList); // Noncompliant {{Replace this "Collections.sort()" with "List.sort()".}} [[quickfixes=qf1]] +// ^^^^ + // fix@qf1 {{Replace with "List.sort()"}} + // edit@qf1 [[sc=5;ec=29]] {{myList.sort(null)}} + Collections.sort(myList, Comparator.naturalOrder()); // Noncompliant {{Replace this "Collections.sort()" with "List.sort()".}} [[quickfixes=qf2]] +// ^^^^ + // fix@qf2 {{Replace with "List.sort()"}} + // edit@qf2 [[sc=5;ec=56]] {{myList.sort(Comparator.naturalOrder())}} + Collections.sort(myList, myComparator); // Noncompliant {{Replace this "Collections.sort()" with "List.sort()".}} [[quickfixes=qf3]] +// ^^^^ + // fix@qf3 {{Replace with "List.sort()"}} + // edit@qf3 [[sc=5;ec=43]] {{myList.sort(myComparator)}} + } + + void noncompliantSubtypes() { + ArrayList arrayList = new ArrayList<>(); + Collections.sort(arrayList); // Noncompliant +// ^^^^ + + LinkedList linkedList = new LinkedList<>(); + Collections.sort(linkedList); // Noncompliant +// ^^^^ + } + + void noncompliantMethodReturn() { + Collections.sort(getList()); // Noncompliant +// ^^^^ + } + + void noncompliantComplexExpression(boolean flag, List a, List b) { + Collections.sort(flag ? a : b); // Noncompliant [[quickfixes=qf4]] +// ^^^^ + // fix@qf4 {{Replace with "List.sort()"}} + // edit@qf4 [[sc=5;ec=35]] {{(flag ? a : b).sort(null)}} + } + + void noncompliantFullyQualified(List myList) { + java.util.Collections.sort(myList); // Noncompliant + } + + void compliant(List myList, Comparator comparator) { + myList.sort(null); + myList.sort(comparator); + Collections.shuffle(myList); + Collections.reverse(myList); + Collections.unmodifiableList(myList); + int[] array = {3, 1, 2}; + Arrays.sort(array); + } + + void compliantCustomClass(List list) { + MyCollections.sort(list); + } + + private List getList() { + return new ArrayList<>(); + } + + static class MyCollections { + static void sort(List list) { + } + } +} diff --git a/java-checks/src/main/java/org/sonar/java/checks/CollectionsSortCheck.java b/java-checks/src/main/java/org/sonar/java/checks/CollectionsSortCheck.java new file mode 100644 index 00000000000..45f078d87fc --- /dev/null +++ b/java-checks/src/main/java/org/sonar/java/checks/CollectionsSortCheck.java @@ -0,0 +1,91 @@ +/* + * 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.EnumSet; +import java.util.Set; +import org.sonar.check.Rule; +import org.sonar.java.checks.helpers.QuickFixHelper; +import org.sonar.java.checks.methods.AbstractMethodDetection; +import org.sonar.java.model.ExpressionUtils; +import org.sonar.java.reporting.JavaQuickFix; +import org.sonar.java.reporting.JavaTextEdit; +import org.sonar.plugins.java.api.JavaVersion; +import org.sonar.plugins.java.api.JavaVersionAwareVisitor; +import org.sonar.plugins.java.api.semantic.MethodMatchers; +import org.sonar.plugins.java.api.tree.ExpressionTree; +import org.sonar.plugins.java.api.tree.MethodInvocationTree; +import org.sonar.plugins.java.api.tree.Tree; + +@Rule(key = "S9412") +public class CollectionsSortCheck extends AbstractMethodDetection implements JavaVersionAwareVisitor { + + private static final Set SAFE_KINDS = EnumSet.of( + Tree.Kind.IDENTIFIER, + Tree.Kind.MEMBER_SELECT, + Tree.Kind.METHOD_INVOCATION, + Tree.Kind.ARRAY_ACCESS_EXPRESSION, + Tree.Kind.PARENTHESIZED_EXPRESSION, + Tree.Kind.NEW_CLASS); + + @Override + public boolean isCompatibleWithJavaVersion(JavaVersion version) { + return version.isJava8Compatible(); + } + + private static final String MESSAGE = "Replace this \"Collections.sort()\" with \"List.sort()\"."; + + @Override + protected MethodMatchers getMethodInvocationMatchers() { + return MethodMatchers.create() + .ofTypes("java.util.Collections") + .names("sort") + .addParametersMatcher("java.util.List") + .addParametersMatcher("java.util.List", "java.util.Comparator") + .build(); + } + + @Override + protected void onMethodInvocationFound(MethodInvocationTree mit) { + ExpressionTree listArgument = mit.arguments().get(0); + boolean hasTwoArgs = mit.arguments().size() == 2; + + QuickFixHelper.newIssue(context) + .forRule(this) + .onTree(ExpressionUtils.methodName(mit)) + .withMessage(MESSAGE) + .withQuickFix(() -> buildQuickFix(mit, listArgument, hasTwoArgs)) + .report(); + } + + private JavaQuickFix buildQuickFix(MethodInvocationTree mit, ExpressionTree listArgument, boolean hasTwoArgs) { + String listText = QuickFixHelper.contentForTree(listArgument, context); + if (!listArgument.is(SAFE_KINDS.toArray(new Tree.Kind[0]))) { + listText = "(" + listText + ")"; + } + String replacement; + if (hasTwoArgs) { + String comparatorText = QuickFixHelper.contentForTree(mit.arguments().get(1), context); + replacement = listText + ".sort(" + comparatorText + ")"; + } else { + replacement = listText + ".sort(null)"; + } + return JavaQuickFix.newQuickFix("Replace with \"List.sort()\"") + .addTextEdit(JavaTextEdit.replaceTree(mit, replacement)) + .build(); + } +} diff --git a/java-checks/src/test/java/org/sonar/java/checks/CollectionsSortCheckTest.java b/java-checks/src/test/java/org/sonar/java/checks/CollectionsSortCheckTest.java new file mode 100644 index 00000000000..9df2ba194af --- /dev/null +++ b/java-checks/src/test/java/org/sonar/java/checks/CollectionsSortCheckTest.java @@ -0,0 +1,55 @@ +/* + * 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 CollectionsSortCheckTest { + + @Test + void test() { + createCheckVerifier() + .withJavaVersion(8) + .verifyIssues(); + } + + @Test + void test_no_issues_for_older_versions() { + createCheckVerifier() + .withJavaVersion(7) + .verifyNoIssues(); + } + + @Test + void test_without_semantic() { + CheckVerifier.newVerifier() + .onFile(mainCodeSourcesPath("checks/CollectionsSortCheckSample.java")) + .withCheck(new CollectionsSortCheck()) + .withJavaVersion(8) + .withoutSemantic() + .verifyIssues(); + } + + private static CheckVerifier createCheckVerifier() { + return CheckVerifier.newVerifier() + .onFile(mainCodeSourcesPath("checks/CollectionsSortCheckSample.java")) + .withCheck(new CollectionsSortCheck()); + } +} diff --git a/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S9412.html b/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S9412.html new file mode 100644 index 00000000000..eb9b9385702 --- /dev/null +++ b/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S9412.html @@ -0,0 +1,48 @@ +

This rule raises an issue when a standalone utility function for sorting is called on a collection object in language versions where the collection +type provides its own instance method for sorting.

+

In Java, this specifically refers to calling Collections.sort() on a List object in Java 8 or later, where +List.sort() should be used instead.

+

Why is this an issue?

+

In earlier versions of the language, sorting a collection required using a static utility method from a helper class. This approach followed the +pattern of having utility functions in separate classes for operations on data structures.

+

In later versions, the collection interface itself gained its own sorting method. This change embraced a more object-oriented design where the +collection can be sorted directly on the instance.

+

Using the static utility sorting function in modern code is outdated for several reasons:

+
    +
  • Readability: myCollection.sort(comparator) reads more naturally than UtilityClass.sort(myCollection, + comparator). The object performing the action comes first, making the code flow clearer.
    • +
  • API design: The newer approach aligns with modern design patterns that favor adding methods directly to interfaces, reducing + the need for separate utility classes.
    • +
  • Consistency: Using the instance method makes the code consistent with other modern APIs in the language.
    • +
+

While the static utility sorting function still works correctly and has the same behavior, using it signals that the code hasn’t been updated to +take advantage of newer language features.

+

In Java specifically, this refers to using Collections.sort() versus the List.sort() method that was introduced in Java +8.

+

What is the potential impact?

+

Using a utility class method to sort a collection instead of the collection’s own sorting method has minimal practical impact on the application’s +behavior, as both methods produce the same result. However, it affects code maintainability:

+
    +
  • The code appears outdated, suggesting it may not be taking advantage of other modern language features.
    • +
  • New team members familiar with modern language practices may find the code less intuitive.
    • +
  • The codebase becomes inconsistent if some parts use the collection’s instance method while others use the utility class method.
    • +
+

How to fix it

+

Replace Collections.sort(myList) with myList.sort(null). When sorting with the natural order, pass null as +the comparator to the sort() method.

+

Code examples

+

Noncompliant code example

+
+Collections.sort(myList); // Noncompliant
+
+

Compliant solution

+
+myList.sort(null);
+
+

Resources

+

Documentation

+ + diff --git a/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S9412.json b/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S9412.json new file mode 100644 index 00000000000..0c438afafc9 --- /dev/null +++ b/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S9412.json @@ -0,0 +1,25 @@ +{ + "title": "\"Collections.sort()\" should not be used", + "type": "CODE_SMELL", + "status": "ready", + "remediation": { + "func": "Constant\/Issue", + "constantCost": "5min" + }, + "tags": [ + "java8", + "obsolete", + "convention" + ], + "defaultSeverity": "Minor", + "ruleSpecification": "RSPEC-9412", + "sqKey": "S9412", + "scope": "All", + "quickfix": "targeted", + "code": { + "impacts": { + "MAINTAINABILITY": "LOW" + }, + "attribute": "CONVENTIONAL" + } +} diff --git a/sonar-java-plugin/src/main/resources/profiles/Sonar_way/S9412 b/sonar-java-plugin/src/main/resources/profiles/Sonar_way/S9412 new file mode 100644 index 00000000000..e69de29bb2d