From e952fc94c70f2859d4c7ff8831bf9c2bd43a534f Mon Sep 17 00:00:00 2001 From: Rick Ossendrijver Date: Wed, 2 Feb 2022 14:18:44 +0100 Subject: [PATCH] PrimitiveComparison: Add type arguments if needed --- .../bugpatterns/PrimitiveComparisonCheck.java | 35 ++++++++++++++----- .../PrimitiveComparisonCheckTest.java | 32 ++++++++--------- 2 files changed, 40 insertions(+), 27 deletions(-) diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/PrimitiveComparisonCheck.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/PrimitiveComparisonCheck.java index 3d1d183c..ff7a9687 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/PrimitiveComparisonCheck.java +++ b/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 tryFix( - MethodInvocationTree tree, VisitorState state, Type cmpType, boolean isStatic) { + private static Optional 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(); diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/PrimitiveComparisonCheckTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/PrimitiveComparisonCheckTest.java index f8111e0e..dfbd5ded 100644 --- a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/PrimitiveComparisonCheckTest.java +++ b/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 `` 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 {", - " Comparator bCmp = Comparator.comparingInt(o -> (byte) 0);", - " Comparator cCmp = Comparator.comparingInt(o -> (char) 0);", - " Comparator sCmp = Comparator.comparingInt(o -> (short) 0);", - " Comparator iCmp = Comparator.comparingInt(o -> 0);", - " Comparator lCmp = Comparator.comparingLong(o -> 0L);", - " Comparator fCmp = Comparator.comparingDouble(o -> 0.0f);", - " Comparator dCmp = Comparator.comparingDouble(o -> 0.0);", + " Comparator bCmp = Comparator.comparingInt(o -> (byte) 0);", + " Comparator cCmp = Comparator.comparingInt(o -> (char) 0);", + " Comparator sCmp = Comparator.comparingInt(o -> (short) 0);", + " Comparator iCmp = Comparator.comparingInt(o -> 0);", + " Comparator lCmp = Comparator.comparingLong(o -> 0L);", + " Comparator fCmp = Comparator.comparingDouble(o -> 0.0f);", + " Comparator dCmp = Comparator.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 `` 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 {", - " Comparator bCmp = Comparator.comparing(o -> Byte.valueOf((byte) 0));", - " Comparator cCmp = Comparator.comparing(o -> Character.valueOf((char) 0));", - " Comparator sCmp = Comparator.comparing(o -> Short.valueOf((short) 0));", - " Comparator iCmp = Comparator.comparing(o -> Integer.valueOf(0));", - " Comparator lCmp = Comparator.comparing(o -> Long.valueOf(0));", - " Comparator fCmp = Comparator.comparing(o -> Float.valueOf(0));", - " Comparator dCmp = Comparator.comparing(o -> Double.valueOf(0));", + " Comparator bCmp = Comparator.comparing(o -> Byte.valueOf((byte) 0));", + " Comparator cCmp = Comparator.comparing(o -> Character.valueOf((char) 0));", + " Comparator sCmp = Comparator.comparing(o -> Short.valueOf((short) 0));", + " Comparator iCmp = Comparator.comparing(o -> Integer.valueOf(0));", + " Comparator lCmp = Comparator.comparing(o -> Long.valueOf(0));", + " Comparator fCmp = Comparator.comparing(o -> Float.valueOf(0));", + " Comparator dCmp = Comparator.comparing(o -> Double.valueOf(0));", "", " default void m() {", " bCmp.thenComparing(o -> Byte.valueOf((byte) 0));",