Skip to content

Commit

Permalink
Suggestions
Browse files Browse the repository at this point in the history
  • Loading branch information
@Stephan202 @rickie
Stephan202 authored and rickie committed Apr 11, 2022
1 parent 451792c commit c873787
Show file tree
Hide file tree
Showing 2 changed files with 91 additions and 40 deletions.
33 changes: 21 additions & 12 deletions ...ne-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/PrimitiveComparisonCheck.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -78,12 +78,12 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState
}

return getPotentiallyBoxedReturnType(tree.getArguments().get(0))
.flatMap(cmpType -> tryMakeMethodCallMorePrecise(tree, cmpType, isStatic, state))
.flatMap(cmpType -> attemptMethodInvocationReplacement(tree, cmpType, isStatic, state))
.map(fix -> describeMatch(tree, fix))
.orElse(Description.NO_MATCH);
}

private static Optional<Fix> tryMakeMethodCallMorePrecise(
private static Optional<Fix> attemptMethodInvocationReplacement(
MethodInvocationTree tree, Type cmpType, boolean isStatic, VisitorState state) {
return Optional.ofNullable(ASTHelpers.getSymbol(tree))
.map(methodSymbol -> methodSymbol.getSimpleName().toString())
Expand All @@ -93,26 +93,35 @@ private static Optional<Fix> tryMakeMethodCallMorePrecise(
.filter(not(actualMethodName::equals)))
.map(
preferredMethodName ->
mayPrefixWithTypeArguments(preferredMethodName, tree, cmpType, state))
prefixTypeArgumentsIfRelevant(preferredMethodName, tree, cmpType, state))
.map(preferredMethodName -> suggestFix(tree, preferredMethodName, state));
}

private static String mayPrefixWithTypeArguments(
/**
* Prefixes the given method name with generic type parameters if it replaces a {@code
* Comparator#comparing{,Double,Long,Int}} method which also has generic type parameters.
*
* <p>Such type parameters are retained as they are likely required.
*
* <p>Note that any type parameter to {@code Comparator#thenComparing} is likely redundant, and in
* any case becomes obsolete once that method is replaced with {@code
* Comparator#thenComparing{Double,Long,Int}}. Conversion in the opposite direction does not
* require the introduction of a generic type parameter.
*/
private static String prefixTypeArgumentsIfRelevant(
String preferredMethodName, MethodInvocationTree tree, Type cmpType, VisitorState state) {
int typeArgumentsCount = tree.getTypeArguments().size();
boolean methodNameIsComparing = "comparing".equals(preferredMethodName);

if (typeArgumentsCount == 0 || (typeArgumentsCount == 1 && !methodNameIsComparing)) {
if (tree.getTypeArguments().isEmpty() || preferredMethodName.startsWith("then")) {
return preferredMethodName;
}

String typeArgument =
String typeArguments =
Stream.concat(
Stream.of(Util.treeToString(tree.getTypeArguments().get(0), state)),
Stream.of(cmpType.tsym.getSimpleName()).filter(u -> methodNameIsComparing))
.collect(joining(","));
Stream.of(cmpType.tsym.getSimpleName())
.filter(u -> "comparing".equals(preferredMethodName)))
.collect(joining(", ", "<", ">"));

return String.format("<%s>%s", typeArgument, preferredMethodName);
return typeArguments + preferredMethodName;
}

private static String getPreferredMethod(Type cmpType, boolean isStatic, VisitorState state) {
Expand Down
98 changes: 70 additions & 28 deletions ...ontrib/src/test/java/tech/picnic/errorprone/bugpatterns/PrimitiveComparisonCheckTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -434,21 +434,35 @@ void replacementWithPrimitiveVariants() {
"import java.util.Comparator;",
"",
"interface A extends Comparable<A> {",
" Comparator<A> bCmp = Comparator.<A, Byte>comparing(o -> (byte) 0);",
" Comparator<A> cCmp = Comparator.<A, Character>comparing(o -> (char) 0);",
" Comparator<A> sCmp = Comparator.<A, Short>comparing(o -> (short) 0);",
" Comparator<A> iCmp = Comparator.<A, Integer>comparing(o -> 0);",
" Comparator<A> lCmp = Comparator.<A, Long>comparing(o -> 0L);",
" Comparator<A> fCmp = Comparator.<A, Float>comparing(o -> 0.0f);",
" Comparator<A> dCmp = Comparator.<A, Double>comparing(o -> 0.0);",
" Comparator<A> bCmp = Comparator.comparing(o -> (byte) 0);",
" Comparator<A> bCmp2 = Comparator.<A, Byte>comparing(o -> (byte) 0);",
" Comparator<A> cCmp = Comparator.comparing(o -> (char) 0);",
" Comparator<A> cCmp2 = Comparator.<A, Character>comparing(o -> (char) 0);",
" Comparator<A> sCmp = Comparator.comparing(o -> (short) 0);",
" Comparator<A> sCmp2 = Comparator.<A, Short>comparing(o -> (short) 0);",
" Comparator<A> iCmp = Comparator.comparing(o -> 0);",
" Comparator<A> iCmp2 = Comparator.<A, Integer>comparing(o -> 0);",
" Comparator<A> lCmp = Comparator.comparing(o -> 0L);",
" Comparator<A> lCmp2 = Comparator.<A, Long>comparing(o -> 0L);",
" Comparator<A> fCmp = Comparator.comparing(o -> 0.0f);",
" Comparator<A> fCmp2 = Comparator.<A, Float>comparing(o -> 0.0f);",
" Comparator<A> dCmp = Comparator.comparing(o -> 0.0);",
" Comparator<A> dCmp2 = Comparator.<A, Double>comparing(o -> 0.0);",
"",
" default void m() {",
" bCmp.<Byte>thenComparing(o -> (byte) 0);",
" bCmp.thenComparing(o -> (byte) 0);",
" cCmp.<Character>thenComparing(o -> (char) 0);",
" cCmp.thenComparing(o -> (char) 0);",
" sCmp.<Short>thenComparing(o -> (short) 0);",
" sCmp.thenComparing(o -> (short) 0);",
" iCmp.<Integer>thenComparing(o -> 0);",
" iCmp.thenComparing(o -> 0);",
" lCmp.<Long>thenComparing(o -> 0L);",
" lCmp.thenComparing(o -> 0L);",
" fCmp.<Float>thenComparing(o -> 0.0f);",
" fCmp.thenComparing(o -> 0.0f);",
" dCmp.<Double>thenComparing(o -> 0.0);",
" dCmp.thenComparing(o -> 0.0);",
" }",
"}")
Expand All @@ -457,22 +471,36 @@ void replacementWithPrimitiveVariants() {
"import java.util.Comparator;",
"",
"interface A extends Comparable<A> {",
" 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);",
" Comparator<A> bCmp = Comparator.comparingInt(o -> (byte) 0);",
" Comparator<A> bCmp2 = Comparator.<A>comparingInt(o -> (byte) 0);",
" Comparator<A> cCmp = Comparator.comparingInt(o -> (char) 0);",
" Comparator<A> cCmp2 = Comparator.<A>comparingInt(o -> (char) 0);",
" Comparator<A> sCmp = Comparator.comparingInt(o -> (short) 0);",
" Comparator<A> sCmp2 = Comparator.<A>comparingInt(o -> (short) 0);",
" Comparator<A> iCmp = Comparator.comparingInt(o -> 0);",
" Comparator<A> iCmp2 = Comparator.<A>comparingInt(o -> 0);",
" Comparator<A> lCmp = Comparator.comparingLong(o -> 0L);",
" Comparator<A> lCmp2 = Comparator.<A>comparingLong(o -> 0L);",
" Comparator<A> fCmp = Comparator.comparingDouble(o -> 0.0f);",
" Comparator<A> fCmp2 = Comparator.<A>comparingDouble(o -> 0.0f);",
" Comparator<A> dCmp = Comparator.comparingDouble(o -> 0.0);",
" Comparator<A> dCmp2 = Comparator.<A>comparingDouble(o -> 0.0);",
"",
" default void m() {",
" bCmp.thenComparingInt(o -> (byte) 0);",
" bCmp.thenComparingInt(o -> (byte) 0);",
" cCmp.thenComparingInt(o -> (char) 0);",
" cCmp.thenComparingInt(o -> (char) 0);",
" sCmp.thenComparingInt(o -> (short) 0);",
" sCmp.thenComparingInt(o -> (short) 0);",
" iCmp.thenComparingInt(o -> 0);",
" iCmp.thenComparingInt(o -> 0);",
" lCmp.thenComparingLong(o -> 0L);",
" lCmp.thenComparingLong(o -> 0L);",
" fCmp.thenComparingDouble(o -> 0.0f);",
" fCmp.thenComparingDouble(o -> 0.0f);",
" dCmp.thenComparingDouble(o -> 0.0);",
" dCmp.thenComparingDouble(o -> 0.0);",
" }",
"}")
.doTest(TestMode.TEXT_MATCH);
Expand All @@ -486,13 +514,20 @@ void replacementWithBoxedVariants() {
"import java.util.Comparator;",
"",
"interface A extends Comparable<A> {",
" Comparator<A> bCmp = Comparator.<A>comparingInt(o -> Byte.valueOf((byte) 0));",
" Comparator<A> cCmp = Comparator.<A>comparingInt(o -> Character.valueOf((char) 0));",
" Comparator<A> sCmp = Comparator.<A>comparingInt(o -> Short.valueOf((short) 0));",
" Comparator<A> iCmp = Comparator.<A>comparingInt(o -> Integer.valueOf(0));",
" Comparator<A> lCmp = Comparator.<A>comparingLong(o -> Long.valueOf(0));",
" Comparator<A> fCmp = Comparator.<A>comparingDouble(o -> Float.valueOf(0));",
" Comparator<A> dCmp = Comparator.<A>comparingDouble(o -> Double.valueOf(0));",
" Comparator<A> bCmp = Comparator.comparingInt(o -> Byte.valueOf((byte) 0));",
" Comparator<A> bCmp2 = Comparator.<A>comparingInt(o -> Byte.valueOf((byte) 0));",
" Comparator<A> cCmp = Comparator.comparingInt(o -> Character.valueOf((char) 0));",
" Comparator<A> cCmp2 = Comparator.<A>comparingInt(o -> Character.valueOf((char) 0));",
" Comparator<A> sCmp = Comparator.comparingInt(o -> Short.valueOf((short) 0));",
" Comparator<A> sCmp2 = Comparator.<A>comparingInt(o -> Short.valueOf((short) 0));",
" Comparator<A> iCmp = Comparator.comparingInt(o -> Integer.valueOf(0));",
" Comparator<A> iCmp2 = Comparator.<A>comparingInt(o -> Integer.valueOf(0));",
" Comparator<A> lCmp = Comparator.comparingLong(o -> Long.valueOf(0));",
" Comparator<A> lCmp2 = Comparator.<A>comparingLong(o -> Long.valueOf(0));",
" Comparator<A> fCmp = Comparator.comparingDouble(o -> Float.valueOf(0));",
" Comparator<A> fCmp2 = Comparator.<A>comparingDouble(o -> Float.valueOf(0));",
" Comparator<A> dCmp = Comparator.comparingDouble(o -> Double.valueOf(0));",
" Comparator<A> dCmp2 = Comparator.<A>comparingDouble(o -> Double.valueOf(0));",
"",
" default void m() {",
" bCmp.thenComparingInt(o -> Byte.valueOf((byte) 0));",
Expand All @@ -509,13 +544,20 @@ void replacementWithBoxedVariants() {
"import java.util.Comparator;",
"",
"interface A extends Comparable<A> {",
" 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));",
" Comparator<A> bCmp = Comparator.comparing(o -> Byte.valueOf((byte) 0));",
" Comparator<A> bCmp2 = Comparator.<A, Byte>comparing(o -> Byte.valueOf((byte) 0));",
" Comparator<A> cCmp = Comparator.comparing(o -> Character.valueOf((char) 0));",
" Comparator<A> cCmp2 = Comparator.<A, Character>comparing(o -> Character.valueOf((char) 0));",
" Comparator<A> sCmp = Comparator.comparing(o -> Short.valueOf((short) 0));",
" Comparator<A> sCmp2 = Comparator.<A, Short>comparing(o -> Short.valueOf((short) 0));",
" Comparator<A> iCmp = Comparator.comparing(o -> Integer.valueOf(0));",
" Comparator<A> iCmp2 = Comparator.<A, Integer>comparing(o -> Integer.valueOf(0));",
" Comparator<A> lCmp = Comparator.comparing(o -> Long.valueOf(0));",
" Comparator<A> lCmp2 = Comparator.<A, Long>comparing(o -> Long.valueOf(0));",
" Comparator<A> fCmp = Comparator.comparing(o -> Float.valueOf(0));",
" Comparator<A> fCmp2 = Comparator.<A, Float>comparing(o -> Float.valueOf(0));",
" Comparator<A> dCmp = Comparator.comparing(o -> Double.valueOf(0));",
" Comparator<A> dCmp2 = Comparator.<A, Double>comparing(o -> Double.valueOf(0));",
"",
" default void m() {",
" bCmp.thenComparing(o -> Byte.valueOf((byte) 0));",
Expand Down

0 comments on commit c873787

Please sign in to comment.