Introduce UnqualifiedSuggestedFixImport check

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

@@ -58,7 +58,10 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState
 
     if (LIST_COLLECTOR.matches(tree, state)) {
       return suggestToCollectionAlternatives(
-          tree, "com.google.common.collect.ImmutableList.toImmutableList", "ArrayList", state);
+          tree,
+          "com.google.common.collect.ImmutableList.toImmutableList",
+          "java.util.ArrayList",
+          state);
     }
 
     if (MAP_COLLECTOR.matches(tree, state)) {
@@ -67,28 +70,31 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState
 
     if (SET_COLLECTOR.matches(tree, state)) {
       return suggestToCollectionAlternatives(
-          tree, "com.google.common.collect.ImmutableSet.toImmutableSet", "HashSet", state);
+          tree,
+          "com.google.common.collect.ImmutableSet.toImmutableSet",
+          "java.util.HashSet",
+          state);
     }
 
     return Description.NO_MATCH;
   }
 
   private Description suggestToCollectionAlternatives(
       MethodInvocationTree tree,
-      String fullyQualifiedImmutableReplacement,
+      String immutableReplacement,
       String mutableReplacement,
       VisitorState state) {
     SuggestedFix.Builder mutableFix = SuggestedFix.builder();
     String toCollectionSelect =
         SuggestedFixes.qualifyStaticImport(
             "java.util.stream.Collectors.toCollection", mutableFix, state);
+    String mutableCollection = SuggestedFixes.qualifyType(state, mutableFix, mutableReplacement);
 
     return buildDescription(tree)
-        .addFix(replaceMethodInvocation(tree, fullyQualifiedImmutableReplacement, state))
+        .addFix(replaceMethodInvocation(tree, immutableReplacement, state))
         .addFix(
             mutableFix
-                .addImport(String.format("java.util.%s", mutableReplacement))
-                .replace(tree, String.format("%s(%s::new)", toCollectionSelect, mutableReplacement))
+                .replace(tree, String.format("%s(%s::new)", toCollectionSelect, mutableCollection))
                 .build())
         .build();
   }
@@ -99,17 +105,19 @@ private Description suggestToMapAlternatives(MethodInvocationTree tree, VisitorS
       return Description.NO_MATCH;
     }
 
+    SuggestedFix.Builder mutableFix = SuggestedFix.builder();
+    String hashMap = SuggestedFixes.qualifyType(state, mutableFix, "java.util.HashMap");
+
     return buildDescription(tree)
         .addFix(
             replaceMethodInvocation(
                 tree, "com.google.common.collect.ImmutableMap.toImmutableMap", state))
         .addFix(
-            SuggestedFix.builder()
-                .addImport("java.util.HashMap")
+            mutableFix
                 .postfixWith(
                     tree.getArguments().get(argCount - 1),
                     (argCount == 2 ? ", (a, b) -> { throw new IllegalStateException(); }" : "")
-                        + ", HashMap::new")
+                        + String.format(", %s::new", hashMap))
                 .build())
         .build();
   }

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

@@ -30,6 +30,7 @@
 import com.google.errorprone.bugpatterns.BugChecker;
 import com.google.errorprone.bugpatterns.BugChecker.MethodTreeMatcher;
 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;
@@ -199,16 +200,21 @@ private static Optional<SuggestedFix> tryConstructValueSourceFix(
     return getSingleReturnExpression(valueFactoryMethod)
         .flatMap(expression -> tryExtractValueSourceAttributeValue(expression, state))
         .map(
-            valueSourceAttributeValue ->
-                SuggestedFix.builder()
-                    .addImport("org.junit.jupiter.params.provider.ValueSource")
-                    .replace(
-                        methodSourceAnnotation,
-                        String.format(
-                            "@ValueSource(%s = %s)",
-                            toValueSourceAttributeName(parameterType), valueSourceAttributeValue))
-                    .delete(valueFactoryMethod)
-                    .build());
+            valueSourceAttributeValue -> {
+              SuggestedFix.Builder fix = SuggestedFix.builder();
+              String valueSource =
+                  SuggestedFixes.qualifyType(
+                      state, fix, "org.junit.jupiter.params.provider.ValueSource");
+              return fix.replace(
+                      methodSourceAnnotation,
+                      String.format(
+                          "@%s(%s = %s)",
+                          valueSource,
+                          toValueSourceAttributeName(parameterType),
+                          valueSourceAttributeValue))
+                  .delete(valueFactoryMethod)
+                  .build();
+            });
   }
 
   // XXX: This pattern also occurs a few times inside Error Prone; contribute upstream.

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

@@ -196,6 +196,8 @@ private static ImmutableList<Name> getVariables(LambdaExpressionTree tree) {
     return tree.getParameters().stream().map(VariableTree::getName).collect(toImmutableList());
   }
 
+  // XXX: Resolve this suppression.
+  @SuppressWarnings("UnqualifiedSuggestedFixImport")
   private static Optional<SuggestedFix.Builder> constructFix(
       LambdaExpressionTree lambdaExpr, Symbol target, Object methodName) {
     Name sName = target.getSimpleName();

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

@@ -17,6 +17,7 @@
 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;
@@ -167,10 +168,11 @@ private static Fix suggestFix(
     ExpressionTree expr = tree.getMethodSelect();
     switch (expr.getKind()) {
       case IDENTIFIER:
-        return SuggestedFix.builder()
-            .addStaticImport(Comparator.class.getCanonicalName() + '.' + preferredMethodName)
-            .replace(expr, preferredMethodName)
-            .build();
+        SuggestedFix.Builder fix = SuggestedFix.builder();
+        String replacement =
+            SuggestedFixes.qualifyStaticImport(
+                Comparator.class.getCanonicalName() + '.' + preferredMethodName, fix, state);
+        return fix.replace(expr, replacement).build();
       case MEMBER_SELECT:
         MemberSelectTree ms = (MemberSelectTree) tree.getMethodSelect();
         return SuggestedFix.replace(

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

@@ -18,6 +18,7 @@
 import com.google.errorprone.bugpatterns.BugChecker.AnnotationTreeMatcher;
 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.sun.source.tree.AnnotationTree;
 import com.sun.source.tree.AssignmentTree;
@@ -114,9 +115,8 @@ private static Fix replaceAnnotation(
             .map(arg -> SourceCode.treeToString(arg, state))
             .collect(joining(", "));
 
-    return SuggestedFix.builder()
-        .addImport(ANN_PACKAGE_PREFIX + newAnnotation)
-        .replace(tree, String.format("@%s(%s)", newAnnotation, newArguments))
-        .build();
+    SuggestedFix.Builder fix = SuggestedFix.builder();
+    String annotation = SuggestedFixes.qualifyType(state, fix, ANN_PACKAGE_PREFIX + newAnnotation);
+    return fix.replace(tree, String.format("@%s(%s)", annotation, newArguments)).build();
   }
 }

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

@@ -0,0 +1,67 @@
+package tech.picnic.errorprone.bugpatterns;
+
+import static com.google.errorprone.BugPattern.LinkType.CUSTOM;
+import static com.google.errorprone.BugPattern.SeverityLevel.WARNING;
+import static com.google.errorprone.BugPattern.StandardTags.FRAGILE_CODE;
+import static com.google.errorprone.matchers.Matchers.instanceMethod;
+import static tech.picnic.errorprone.bugpatterns.util.Documentation.BUG_PATTERNS_BASE_URL;
+
+import com.google.auto.service.AutoService;
+import com.google.errorprone.BugPattern;
+import com.google.errorprone.VisitorState;
+import com.google.errorprone.bugpatterns.BugChecker;
+import com.google.errorprone.bugpatterns.BugChecker.MethodInvocationTreeMatcher;
+import com.google.errorprone.fixes.SuggestedFix;
+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;
+
+/**
+ * A {@link BugChecker} that flags suggested fixes that involve unconditional imports.
+ *
+ * <p>Such unconditional imports may clash with other imports of the same identifier.
+ */
+@AutoService(BugChecker.class)
+@BugPattern(
+    summary = "Avoid direct invocation of `SuggestedFix#add{,Static}Import`",
+    link = BUG_PATTERNS_BASE_URL + "UnqualifiedSuggestedFixImport",
+    linkType = CUSTOM,
+    severity = WARNING,
+    tags = FRAGILE_CODE)
+public final class UnqualifiedSuggestedFixImport extends BugChecker
+    implements MethodInvocationTreeMatcher {
+  private static final long serialVersionUID = 1L;
+  private static final Matcher<ExpressionTree> FIX_BUILDER_METHOD =
+      instanceMethod().onDescendantOf(SuggestedFix.Builder.class.getCanonicalName());
+
+  /** Instantiates a new {@link UnqualifiedSuggestedFixImport} instance. */
+  public UnqualifiedSuggestedFixImport() {}
+
+  @Override
+  public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) {
+    if (!FIX_BUILDER_METHOD.matches(tree, state)) {
+      return Description.NO_MATCH;
+    }
+
+    switch (ASTHelpers.getSymbol(tree).getSimpleName().toString()) {
+      case "addImport":
+        return createDescription(
+            tree, "SuggestedFix.Builder#addImport", "SuggestedFixes#qualifyType");
+      case "addStaticImport":
+        return createDescription(
+            tree, "SuggestedFix.Builder#addStaticImport", "SuggestedFixes#qualifyStaticImport");
+      default:
+        return Description.NO_MATCH;
+    }
+  }
+
+  private Description createDescription(
+      MethodInvocationTree tree, String method, String alternative) {
+    return buildDescription(tree)
+        .setMessage(
+            String.format("Prefer `%s` over direct invocation of `%s`", alternative, method))
+        .build();
+  }
+}

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

@@ -0,0 +1,44 @@
+package tech.picnic.errorprone.bugpatterns;
+
+import com.google.errorprone.CompilationTestHelper;
+import org.junit.jupiter.api.Test;
+
+final class UnqualifiedSuggestedFixImportTest {
+  @Test
+  void identification() {
+    CompilationTestHelper.newInstance(UnqualifiedSuggestedFixImport.class, getClass())
+        .expectErrorMessage(
+            "IMPORT",
+            m ->
+                m.contains(
+                    "Prefer `SuggestedFixes#qualifyType` over direct invocation of `SuggestedFix.Builder#addImport`"))
+        .expectErrorMessage(
+            "STATIC_IMPORT",
+            m ->
+                m.contains(
+                    "Prefer `SuggestedFixes#qualifyStaticImport` over direct invocation of `SuggestedFix.Builder#addStaticImport`"))
+        .addSourceLines(
+            "A.java",
+            "import com.google.errorprone.fixes.SuggestedFix;",
+            "",
+            "class A {",
+            "  void m() {",
+            "    System.out.println(\"foo\");",
+            "    addImport(\"bar\");",
+            "    addStaticImport(\"baz\");",
+            "",
+            "    SuggestedFix.Builder builder = SuggestedFix.builder();",
+            "    // BUG: Diagnostic matches: IMPORT",
+            "    builder.addImport(\"java.lang.String\");",
+            "    // BUG: Diagnostic matches: STATIC_IMPORT",
+            "    builder.addStaticImport(\"java.lang.String.toString\");",
+            "    builder.build();",
+            "  }",
+            "",
+            "  private void addImport(String s) {}",
+            "",
+            "  private void addStaticImport(String s) {}",
+            "}")
+        .doTest();
+  }
+}