Add an optional check that recommends using Guava's immutable collections

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<ExpressionTree> MATCHER =
+      staticMethod()
+          .onClassAny("java.util.List", "java.util.Set", "java.util.Map")
+          .namedAnyOf("of", "copyOf", "ofEntries");
+
+  private static final Matcher<ExpressionTree> OF_VARARGS =
+      staticMethod().anyClass().withSignature("<E>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<Fix> 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());
+  }
+}

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,

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();
+  }
+}

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