diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/JdkImmutableCollections.java b/core/src/main/java/com/google/errorprone/bugpatterns/JdkImmutableCollections.java new file mode 100644 index 00000000000..153bd15df51 --- /dev/null +++ b/core/src/main/java/com/google/errorprone/bugpatterns/JdkImmutableCollections.java @@ -0,0 +1,85 @@ +/* + * Copyright 2024 The Error Prone Authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.google.errorprone.bugpatterns; + +import static com.google.errorprone.BugPattern.SeverityLevel.ERROR; +import static com.google.errorprone.matchers.Description.NO_MATCH; +import static com.google.errorprone.matchers.method.MethodMatchers.staticMethod; +import static com.google.errorprone.util.ASTHelpers.getReceiverType; + +import com.google.errorprone.BugPattern; +import com.google.errorprone.VisitorState; +import com.google.errorprone.bugpatterns.BugChecker.MethodInvocationTreeMatcher; +import com.google.errorprone.fixes.Fix; +import com.google.errorprone.fixes.SuggestedFix; +import com.google.errorprone.fixes.SuggestedFixes; +import com.google.errorprone.matchers.Description; +import com.google.errorprone.matchers.Matcher; +import com.google.errorprone.util.ASTHelpers; +import com.sun.source.tree.ExpressionTree; +import com.sun.source.tree.MethodInvocationTree; +import java.util.Optional; + +/** A {@link BugChecker}; see the associated {@link BugPattern} annotation for details. */ +@BugPattern( + summary = + "Prefer Google's immutable collections to the convenience methods in collection interfaces", + severity = ERROR) +public final class JdkImmutableCollections extends BugChecker + implements MethodInvocationTreeMatcher { + + private static final Matcher MATCHER = + staticMethod() + .onClassAny("java.util.List", "java.util.Set", "java.util.Map") + .namedAnyOf("of", "copyOf", "ofEntries"); + + private static final Matcher OF_VARARGS = + staticMethod().anyClass().withSignature("of(E...)"); + + @Override + public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) { + if (!MATCHER.matches(tree, state)) { + return NO_MATCH; + } + String typeName = getReceiverType(tree).asElement().getSimpleName().toString(); + Description.Builder description = buildDescription(tree); + buildFix(tree, typeName, state).ifPresent(description::addFix); + return description + .setMessage( + String.format( + "Prefer Immutable%s to the convenience methods in %s", typeName, typeName)) + .build(); + } + + private static Optional buildFix( + MethodInvocationTree tree, String typeName, VisitorState state) { + SuggestedFix.Builder fix = SuggestedFix.builder(); + if (tree.getArguments().size() == 1 && OF_VARARGS.matches(tree, state)) { + // ImmutableList.of(x) is always a single-element list containing `x`; + // List.of(x) is a single-element list containing `x` *unless x is an Object[]*, + // in which case the varargs overload of List.of is selected and it returns a + // list containing the elements of `x`. We rewrite to ImmutableList.copyOf + // in that case to be semantics preserving. + // (see corresponding Guava changes in cl/14626886, cl//24840447) + fix.merge(SuggestedFixes.renameMethodInvocation(tree, "copyOf", state)); + } + fix.replace( + ASTHelpers.getReceiver(tree), + SuggestedFixes.qualifyType(state, fix, "com.google.common.collect.Immutable" + typeName)); + return Optional.of(fix.build()); + } +} diff --git a/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java b/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java index 0d5a42d2dad..2efa2d70374 100644 --- a/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java +++ b/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java @@ -216,6 +216,7 @@ import com.google.errorprone.bugpatterns.JUnitParameterMethodNotFound; import com.google.errorprone.bugpatterns.JavaLangClash; import com.google.errorprone.bugpatterns.JavaUtilDateChecker; +import com.google.errorprone.bugpatterns.JdkImmutableCollections; import com.google.errorprone.bugpatterns.JdkObsolete; import com.google.errorprone.bugpatterns.LabelledBreakTarget; import com.google.errorprone.bugpatterns.LambdaFunctionalInterface; @@ -1209,6 +1210,7 @@ public static ScannerSupplier warningChecks() { IterablePathParameter.class, Java7ApiChecker.class, Java8ApiChecker.class, + JdkImmutableCollections.class, LambdaFunctionalInterface.class, LongLiteralLowerCaseSuffix.class, MethodCanBeStatic.class, diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/JdkImmutableCollectionsTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/JdkImmutableCollectionsTest.java new file mode 100644 index 00000000000..12f1b823341 --- /dev/null +++ b/core/src/test/java/com/google/errorprone/bugpatterns/JdkImmutableCollectionsTest.java @@ -0,0 +1,174 @@ +/* + * Copyright 2024 The Error Prone Authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.google.errorprone.bugpatterns; + +import com.google.errorprone.BugCheckerRefactoringTestHelper; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +/** {@link JdkImmutableCollections}Test */ +@RunWith(JUnit4.class) +public class JdkImmutableCollectionsTest { + private final BugCheckerRefactoringTestHelper testHelper = + BugCheckerRefactoringTestHelper.newInstance(JdkImmutableCollections.class, getClass()); + + @Test + public void factories() { + testHelper + .addInputLines( + "Test.java", + """ + import java.util.List; + import java.util.Map; + import java.util.Set; + + class Test { + void f() { + List.of(42); + Map.of(42, true); + Set.of(42); + } + } + """) + .addOutputLines( + "Test.java", + """ + import com.google.common.collect.ImmutableList; + import com.google.common.collect.ImmutableMap; + import com.google.common.collect.ImmutableSet; + import java.util.List; + import java.util.Map; + import java.util.Set; + + class Test { + void f() { + ImmutableList.of(42); + ImmutableMap.of(42, true); + ImmutableSet.of(42); + } + } + """) + .doTest(); + } + + @Test + public void copyOf() { + testHelper + .addInputLines( + "Test.java", + """ + import java.util.List; + import java.util.Map; + import java.util.Set; + + class Test { + void f(List l, Map m, Set s) { + List.copyOf(l); + Map.copyOf(m); + Set.copyOf(s); + } + } + """) + .addOutputLines( + "Test.java", + """ + import com.google.common.collect.ImmutableList; + import com.google.common.collect.ImmutableMap; + import com.google.common.collect.ImmutableSet; + import java.util.List; + import java.util.Map; + import java.util.Set; + + class Test { + void f(List l, Map m, Set s) { + ImmutableList.copyOf(l); + ImmutableMap.copyOf(m); + ImmutableSet.copyOf(s); + } + } + """) + .doTest(); + } + + @Test + public void enhancedForLoop() { + testHelper + .addInputLines( + "Test.java", + """ + import java.util.List; + + class Test { + void f() { + for (int x : List.of(42)) {} + } + } + """) + .addOutputLines( + "Test.java", + """ + import com.google.common.collect.ImmutableList; + import java.util.List; + + class Test { + void f() { + for (int x : ImmutableList.of(42)) {} + } + } + """) + .doTest(); + } + + @Test + public void arrays() { + testHelper + .addInputLines( + "Test.java", + """ + import java.util.List; + import java.util.Set; + + class Test { + void f(Object[] o, int[] x) { + List.of(o); + List.of(x); + Set.of(o); + Set.of(x); + } + } + """) + .addOutputLines( + "Test.java", + """ + import com.google.common.collect.ImmutableList; + import com.google.common.collect.ImmutableSet; + import java.util.List; + import java.util.Set; + + class Test { + void f(Object[] o, int[] x) { + ImmutableList.copyOf(o); + ImmutableList.of(x); + ImmutableSet.copyOf(o); + ImmutableSet.of(x); + } + } + """) + .doTest(); + } +} diff --git a/docs/bugpattern/JdkImmutableCollections.md b/docs/bugpattern/JdkImmutableCollections.md new file mode 100644 index 00000000000..2ad3efc66c6 --- /dev/null +++ b/docs/bugpattern/JdkImmutableCollections.md @@ -0,0 +1,43 @@ +Consider using [Google's immutable collections][javadoc] (e.g., `ImmutableList.of(...)`) instead of the JDK's immutable collection factories (e.g., `List.of(...)`) that were introduced in Java 9. + +## Comparison + +Like the JDK immutable collection factories, Google's immutable collections: + +* Reject null elements +* Prevent all modifications at runtime +* Offer `copyOf` methods that avoid provably-unnecessary copies +* Are open-sourced as part of [Guava](http://guava.dev) + +But they also: + +* Have deterministic (insertion-ordered) iteration, instead of randomized + order, which can lead to flaky tests. +* Are public *types* that guarantee immutability, which make for far nicer + field types and return types than the general-purpose types. + * This enables them to prevent most modification at + [*compile-time*][`DoNotCall`]. In spirit, it's nearly as though the + modification methods are not even there. + * It's never *your* responsibility to remember when to make a "defensive + copy"; the compiler just tells you when you have to. +* Have a richer set of construction paths (including builders), and a few + other features like `ImmutableSet.asList()`. +* Include immutable forms of additional collection types like multimaps. +* Don't fail on calls to `contains(null)` (same as virtually every other + collection you've ever used). +* Are available for (and optimized for) Android. + +The advantages of the JDK immutable collection factories are fewer: + +* No `com.google.common.collect` dependency (smaller code footprint). +* Shorter class names (by 9 characters). +* Will become (or, at some point, became) more popular than Google's immutable + collections in the world at large. + +Both libraries offer `copyOf` methods that skip making an actual copy in certain +cases. However, neither type (JDK or Google) can recognize the *other* as being +safely immutable. Therefore, codebases should strive for homogeneous immutable +collection usage. + +[`DoNotCall`]: https://errorprone.info/bugpattern/DoNotCall +[javadoc]: https://guava.dev/ImmutableCollection