PrimitiveComparison: Add type arguments if needed

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

@@ -37,7 +37,6 @@ import java.util.function.Function;
  */
 // XXX: Add more documentation. Explain how this is useful in the face of refactoring to more
 // specific types.
-// XXX: Change this checker's name?
 @AutoService(BugChecker.class)
 @BugPattern(
     name = "PrimitiveComparison",
@@ -77,23 +76,44 @@ public final class PrimitiveComparisonCheck extends BugChecker
     }
 
     return getPotentiallyBoxedReturnType(tree.getArguments().get(0))
-        .flatMap(cmpType -> tryFix(tree, state, cmpType, isStatic))
+        .flatMap(cmpType -> tryMakeMethodCallMorePrecise(tree, cmpType, isStatic, state))
         .map(fix -> describeMatch(tree, fix))
         .orElse(Description.NO_MATCH);
   }
 
-  private static Optional<Fix> tryFix(
-      MethodInvocationTree tree, VisitorState state, Type cmpType, boolean isStatic) {
+  private static Optional<Fix> tryMakeMethodCallMorePrecise(
+      MethodInvocationTree tree, Type cmpType, boolean isStatic, VisitorState state) {
     return Optional.ofNullable(ASTHelpers.getSymbol(tree))
         .map(methodSymbol -> methodSymbol.getSimpleName().toString())
         .flatMap(
             actualMethodName ->
-                Optional.of(getPreferredMethod(state, cmpType, isStatic))
+                Optional.of(getPreferredMethod(cmpType, isStatic, state))
                     .filter(not(actualMethodName::equals)))
+        .map(
+            preferredMethodName ->
+                prefixWithTypeArgumentsIfNeeded(preferredMethodName, tree, cmpType, state))
         .map(preferredMethodName -> suggestFix(tree, preferredMethodName, state));
   }
 
-  private static String getPreferredMethod(VisitorState state, Type cmpType, boolean isStatic) {
+  private static String prefixWithTypeArgumentsIfNeeded(
+      String preferredMethodName, MethodInvocationTree tree, Type cmpType, VisitorState state) {
+    int typeArguments = tree.getTypeArguments().size();
+    boolean methodNameIsComparing = preferredMethodName.equals("comparing");
+
+    if (typeArguments == 0 || (typeArguments == 1 && !methodNameIsComparing)) {
+      return preferredMethodName;
+    }
+
+    String optionalSecondTypeArgument =
+        methodNameIsComparing ? ", " + cmpType.tsym.getSimpleName() : "";
+    return String.format(
+        "<%s%s>%s",
+        Util.treeToString(tree.getTypeArguments().get(0), state),
+        optionalSecondTypeArgument,
+        preferredMethodName);
+  }
+
+  private static String getPreferredMethod(Type cmpType, boolean isStatic, VisitorState state) {
     Types types = state.getTypes();
     Symtab symtab = state.getSymtab();
 
@@ -128,9 +148,6 @@ public final class PrimitiveComparisonCheck extends BugChecker
     }
   }
 
-  // XXX: We drop explicitly specified generic type information. In case the number of type
-  // arguments before and after doesn't match, that's for the better. But if we e.g. replace
-  // `comparingLong` with `comparingInt`, then we should retain it.
   private static Fix suggestFix(
       MethodInvocationTree tree, String preferredMethodName, VisitorState state) {
     ExpressionTree expr = tree.getMethodSelect();

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

@@ -426,8 +426,6 @@ public final class PrimitiveComparisonCheckTest {
         .doTest();
   }
 
-  // XXX: If the explicit `<A, BoxedPrimitive>` generic type information was necessary, then this
-  // replacement drops too much information.
   @Test
   void replacementWithPrimitiveVariants() {
     refactoringTestHelper
@@ -459,13 +457,13 @@ public final class PrimitiveComparisonCheckTest {
             "import java.util.Comparator;",
             "",
             "interface A extends Comparable<A> {",
-            "  Comparator<A> bCmp = Comparator.comparingInt(o -> (byte) 0);",
-            "  Comparator<A> cCmp = Comparator.comparingInt(o -> (char) 0);",
-            "  Comparator<A> sCmp = Comparator.comparingInt(o -> (short) 0);",
-            "  Comparator<A> iCmp = Comparator.comparingInt(o -> 0);",
-            "  Comparator<A> lCmp = Comparator.comparingLong(o -> 0L);",
-            "  Comparator<A> fCmp = Comparator.comparingDouble(o -> 0.0f);",
-            "  Comparator<A> dCmp = Comparator.comparingDouble(o -> 0.0);",
+            "  Comparator<A> bCmp = Comparator.<A>comparingInt(o -> (byte) 0);",
+            "  Comparator<A> cCmp = Comparator.<A>comparingInt(o -> (char) 0);",
+            "  Comparator<A> sCmp = Comparator.<A>comparingInt(o -> (short) 0);",
+            "  Comparator<A> iCmp = Comparator.<A>comparingInt(o -> 0);",
+            "  Comparator<A> lCmp = Comparator.<A>comparingLong(o -> 0L);",
+            "  Comparator<A> fCmp = Comparator.<A>comparingDouble(o -> 0.0f);",
+            "  Comparator<A> dCmp = Comparator.<A>comparingDouble(o -> 0.0);",
             "",
             "  default void m() {",
             "    bCmp.thenComparingInt(o -> (byte) 0);",
@@ -480,8 +478,6 @@ public final class PrimitiveComparisonCheckTest {
         .doTest(TestMode.TEXT_MATCH);
   }
 
-  // XXX: If the explicit `<A>` generic type information was necessary, then this replacement drops
-  // too much information.
   @Test
   void replacementWithBoxedVariants() {
     refactoringTestHelper
@@ -513,13 +509,13 @@ public final class PrimitiveComparisonCheckTest {
             "import java.util.Comparator;",
             "",
             "interface A extends Comparable<A> {",
-            "  Comparator<A> bCmp = Comparator.comparing(o -> Byte.valueOf((byte) 0));",
-            "  Comparator<A> cCmp = Comparator.comparing(o -> Character.valueOf((char) 0));",
-            "  Comparator<A> sCmp = Comparator.comparing(o -> Short.valueOf((short) 0));",
-            "  Comparator<A> iCmp = Comparator.comparing(o -> Integer.valueOf(0));",
-            "  Comparator<A> lCmp = Comparator.comparing(o -> Long.valueOf(0));",
-            "  Comparator<A> fCmp = Comparator.comparing(o -> Float.valueOf(0));",
-            "  Comparator<A> dCmp = Comparator.comparing(o -> Double.valueOf(0));",
+            "  Comparator<A> bCmp = Comparator.<A, Byte>comparing(o -> Byte.valueOf((byte) 0));",
+            "  Comparator<A> cCmp = Comparator.<A, Character>comparing(o -> Character.valueOf((char) 0));",
+            "  Comparator<A> sCmp = Comparator.<A, Short>comparing(o -> Short.valueOf((short) 0));",
+            "  Comparator<A> iCmp = Comparator.<A, Integer>comparing(o -> Integer.valueOf(0));",
+            "  Comparator<A> lCmp = Comparator.<A, Long>comparing(o -> Long.valueOf(0));",
+            "  Comparator<A> fCmp = Comparator.<A, Float>comparing(o -> Float.valueOf(0));",
+            "  Comparator<A> dCmp = Comparator.<A, Double>comparing(o -> Double.valueOf(0));",
             "",
             "  default void m() {",
             "    bCmp.thenComparing(o -> Byte.valueOf((byte) 0));",