Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PrimitiveComparison: Retain type arguments if present #39

Merged
merged 4 commits into from
Apr 11, 2022
Merged

PrimitiveComparison: Retain type arguments if present #39

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
dabca71
PrimitiveComparison: Add type arguments if needed
rickie Feb 2, 2022
8b133f9
Apply suggestions
rickie Feb 15, 2022
451792c
Some feedback from yesterday ;)
rickie Apr 9, 2022
c873787
Suggestions
Stephan202 Apr 9, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 37 additions & 9 deletions ...ne-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/PrimitiveComparisonCheck.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
import static com.google.errorprone.matchers.method.MethodMatchers.instanceMethod;
import static com.google.errorprone.matchers.method.MethodMatchers.staticMethod;
import static java.util.function.Predicate.not;
import static java.util.stream.Collectors.joining;

import com.google.auto.service.AutoService;
import com.google.common.base.VerifyException;
Expand All @@ -30,14 +31,14 @@
import java.util.Comparator;
import java.util.Optional;
import java.util.function.Function;
import java.util.stream.Stream;

/**
* A {@link BugChecker} which flags {@code Comparator#comparing*} invocations that can be replaced
* with an equivalent alternative so as to avoid unnecessary (un)boxing.
*/
// XXX: Add more documentation. Explain how this is useful in the face of refactoring to more
// specific types.
// XXX: Change this checker's name?
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did not improve the naming, but suggest that we either improve the naming or delete this XXX 😄.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or keep the comment since it may still apply 🙃

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To shortly clarify, I did think about the XXX but think that the name is fine as it is.
So I removed the line to "start the discussion" because I feel that we can easily do the XXX by either removing it or coming up with an improvement 😄.

Some ideas: ComparatorUsage, ComparatorPrimitive, CanonicalComparatorUsage.

WDYT?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ComparatorUsage and CanonicalComparatorUsage are nice, but hint at something more generic. Let's keep the current name, at least in the context of the current PR.

@AutoService(BugChecker.class)
@BugPattern(
name = "PrimitiveComparison",
Expand Down Expand Up @@ -77,23 +78,53 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState
}

return getPotentiallyBoxedReturnType(tree.getArguments().get(0))
.flatMap(cmpType -> tryFix(tree, state, cmpType, isStatic))
.flatMap(cmpType -> attemptMethodInvocationReplacement(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> attemptMethodInvocationReplacement(
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 ->
prefixTypeArgumentsIfRelevant(preferredMethodName, tree, cmpType, state))
.map(preferredMethodName -> suggestFix(tree, preferredMethodName, state));
}

private static String getPreferredMethod(VisitorState state, Type cmpType, boolean isStatic) {
/**
* 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) {
if (tree.getTypeArguments().isEmpty() || preferredMethodName.startsWith("then")) {
return preferredMethodName;
}

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

return typeArguments + preferredMethodName;
}

private static String getPreferredMethod(Type cmpType, boolean isStatic, VisitorState state) {
Types types = state.getTypes();
Symtab symtab = state.getSymtab();

Expand Down Expand Up @@ -128,9 +159,6 @@ private static Optional<Type> getPotentiallyBoxedReturnType(ExpressionTree tree)
}
}

// 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.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tests don't show such a replacement. Is this a realistic scenario that we (will) handle with this check?

private static Fix suggestFix(
MethodInvocationTree tree, String preferredMethodName, VisitorState state) {
ExpressionTree expr = tree.getMethodSelect();
Expand Down
74 changes: 56 additions & 18 deletions ...ontrib/src/test/java/tech/picnic/errorprone/bugpatterns/PrimitiveComparisonCheckTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -426,8 +426,6 @@ void stringComparison() {
.doTest();
}

// XXX: If the explicit `<A, BoxedPrimitive>` generic type information was necessary, then this
// replacement drops too much information.
@Test
void replacementWithPrimitiveVariants() {
refactoringTestHelper
Expand All @@ -436,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 @@ -460,28 +472,40 @@ void replacementWithPrimitiveVariants() {
"",
"interface A extends Comparable<A> {",
" 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);
}

// XXX: If the explicit `<A>` generic type information was necessary, then this replacement drops
// too much information.
@Test
void replacementWithBoxedVariants() {
refactoringTestHelper
Expand All @@ -490,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 @@ -514,12 +545,19 @@ void replacementWithBoxedVariants() {
"",
"interface A extends Comparable<A> {",
" 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