Improve ExplicitArgumentEnumeration check (#1475)

Summary of changes:
- Also drop unnecessary `Immutable{List,Multiset,Set}#copyOf(E[])`
  invocations.
- Don't suggest simplifications that are likely to introduce unbounded
  recursion.

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/ExplicitArgumentEnumeration.java

@@ -5,9 +5,11 @@
 import static com.google.errorprone.BugPattern.SeverityLevel.SUGGESTION;
 import static com.google.errorprone.BugPattern.StandardTags.PERFORMANCE;
 import static com.google.errorprone.BugPattern.StandardTags.SIMPLIFICATION;
+import static com.google.errorprone.matchers.Matchers.allOf;
 import static com.google.errorprone.matchers.Matchers.anyOf;
 import static com.google.errorprone.matchers.Matchers.instanceMethod;
 import static com.google.errorprone.matchers.Matchers.staticMethod;
+import static com.google.errorprone.matchers.Matchers.symbolMatcher;
 import static tech.picnic.errorprone.utils.Documentation.BUG_PATTERNS_BASE_URL;
 
 import com.google.auto.service.AutoService;
@@ -28,11 +30,13 @@
 import com.google.errorprone.util.Visibility;
 import com.sun.source.tree.ExpressionTree;
 import com.sun.source.tree.MethodInvocationTree;
+import com.sun.source.tree.MethodTree;
 import com.sun.tools.javac.code.Symbol.MethodSymbol;
 import com.sun.tools.javac.code.Symbol.VarSymbol;
 import java.util.Arrays;
 import java.util.List;
 import java.util.Map;
+import java.util.Objects;
 import java.util.Optional;
 import java.util.Set;
 import javax.lang.model.element.Element;
@@ -74,6 +78,19 @@ public final class ExplicitArgumentEnumeration extends BugChecker
                   List.class.getCanonicalName(),
                   Set.class.getCanonicalName())
               .named("of"),
+          allOf(
+              staticMethod()
+                  .onClassAny(
+                      ImmutableList.class.getCanonicalName(),
+                      ImmutableMultiset.class.getCanonicalName(),
+                      ImmutableSet.class.getCanonicalName())
+                  .named("copyOf"),
+              symbolMatcher(
+                  (symbol, state) ->
+                      state
+                          .getSymtab()
+                          .arrayClass
+                          .equals(((MethodSymbol) symbol).params().get(0).type.tsym))),
           staticMethod().onClass(Arrays.class.getCanonicalName()).named("asList"));
   private static final Matcher<ExpressionTree> IMMUTABLE_COLLECTION_BUILDER =
       instanceMethod().onDescendantOf(ImmutableCollection.Builder.class.getCanonicalName());
@@ -109,7 +126,12 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState
     }
 
     MethodSymbol method = ASTHelpers.getSymbol(tree);
-    if (!isUnaryIterableAcceptingMethod(method, state)) {
+    if (!isUnaryIterableAcceptingMethod(method, state) || isLocalOverload(method, state)) {
+      /*
+       * This isn't a method invocation we can simplify, or it's an invocation of a local overload.
+       * The latter type of invocation we do not suggest replacing, as this is fairly likely to
+       * introduce an unbounded recursive call chain.
+       */
       return Description.NO_MATCH;
     }
 
@@ -131,6 +153,17 @@ private static boolean isUnaryIterableAcceptingMethod(MethodSymbol method, Visit
         && ASTHelpers.isSubtype(params.get(0).type, state.getSymtab().iterableType, state);
   }
 
+  private static boolean isLocalOverload(MethodSymbol calledMethod, VisitorState state) {
+    MethodTree enclosingMethod = state.findEnclosing(MethodTree.class);
+    if (enclosingMethod == null) {
+      return false;
+    }
+
+    MethodSymbol callingMethod = ASTHelpers.getSymbol(enclosingMethod);
+    return Objects.equals(callingMethod.getEnclosingElement(), calledMethod.getEnclosingElement())
+        && callingMethod.getSimpleName().equals(calledMethod.getSimpleName());
+  }
+
   private static Optional<SuggestedFix> trySuggestCallingVarargsOverload(
       MethodSymbol method, MethodInvocationTree argument, VisitorState state) {
     /*

error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/ExplicitArgumentEnumerationTest.java

@@ -21,6 +21,9 @@ void identification() {
             "import reactor.test.StepVerifier;",
             "",
             "class A {",
+            "  // BUG: Diagnostic contains:",
+            "  private final int value = unaryMethod(ImmutableList.of(1, 2));",
+            "",
             "  void m() {",
             "    ImmutableList<String> list = ImmutableList.of();",
             "    assertThat(ImmutableList.of()).containsAnyElementsOf(list);",
@@ -37,6 +40,9 @@ void identification() {
             "    ImmutableList.builder()",
             "        // BUG: Diagnostic contains:",
             "        .addAll(ImmutableList.of())",
+            "        // BUG: Diagnostic contains:",
+            "        .addAll(ImmutableList.copyOf(new String[0]))",
+            "        .addAll(ImmutableList.copyOf(ImmutableList.of()))",
             "        .build();",
             "",
             "    assertThat(ImmutableList.of(1))",
@@ -57,17 +63,25 @@ void identification() {
             "        .withClasspath();",
             "  }",
             "",
-            "  private void unaryMethod(ImmutableList<Integer> args) {}",
+            "  private int unaryMethod(ImmutableList<Integer> args) {",
+            "    return 0;",
+            "  }",
             "",
-            "  private void unaryMethod(Integer... args) {}",
+            "  private int unaryMethod(Integer... args) {",
+            "    return unaryMethod(ImmutableList.copyOf(args));",
+            "  }",
             "",
             "  void unaryMethodWithLessVisibleOverload(ImmutableList<Integer> args) {}",
             "",
-            "  private void unaryMethodWithLessVisibleOverload(Integer... args) {}",
+            "  private void unaryMethodWithLessVisibleOverload(Integer... args) {",
+            "    unaryMethodWithLessVisibleOverload(ImmutableList.copyOf(args));",
+            "  }",
             "",
             "  private void binaryMethod(ImmutableList<Integer> args, int extraArg) {}",
             "",
-            "  private void binaryMethod(Integer... args) {}",
+            "  private void binaryMethod(Integer... args) {",
+            "    binaryMethod(ImmutableList.copyOf(args), 0);",
+            "  }",
             "}")
         .doTest();
   }