PrimitiveComparison: Retain type arguments if present #39

rickie · 2022-02-09T08:54:45Z

No description provided.

rickie · 2022-02-09T08:55:49Z

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

@@ -37,7 +37,6 @@
 */
 // XXX: Add more documentation. Explain how this is useful in the face of refactoring to more
 // specific types.
-// XXX: Change this checker's name?


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

Or keep the comment since it may still apply 🙃

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?

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.

rickie · 2022-02-09T08:56:30Z

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

@@ -77,23 +76,44 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState
    }

    return getPotentiallyBoxedReturnType(tree.getArguments().get(0))
-        .flatMap(cmpType -> tryFix(tree, state, cmpType, isStatic))
+        .flatMap(cmpType -> tryMakeMethodCallMorePrecise(tree, cmpType, isStatic, state))


Not entirely sure about the method name, but figured that we could be more clear than tryFix here.

rickie · 2022-02-09T08:57:36Z

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

@@ -128,9 +148,6 @@ private static String getPreferredMethod(VisitorState state, Type cmpType, boole
    }
  }

-  // 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.


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

werli

The tests convince me to approve, but some minor things are to improve here.

werli · 2022-02-15T11:04:21Z

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

@@ -37,7 +37,6 @@
 */
 // XXX: Add more documentation. Explain how this is useful in the face of refactoring to more
 // specific types.
-// XXX: Change this checker's name?


Or keep the comment since it may still apply 🙃

werli · 2022-02-15T11:08:59Z

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

-  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) {


(1) Why did we change the order? Isn't the VisitorState much more important? Here and in the other methods.
(2) What was this renamed? Don't we still try to fix it?

(1) It is sort of an "unwritten rule/convention". In both Error Prone and error-prone-support the VisitorState always comes last.

(2) IMO we can use the method name to better express what we are trying to do. The PrimitiveComparisonCheck tries to improve multiple things and by changing the method name, we can make it more clear which part we are trying to fix in this method.

(1) ✔️
(2) Not sure whether tryMakeMethodCallMorePrecise is expressing anything well 🙃 But I also don't have a better suggestion right now. 🙂

werli · 2022-02-15T11:13:10Z

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

        .map(preferredMethodName -> suggestFix(tree, preferredMethodName, state));
  }

-  private static String getPreferredMethod(VisitorState state, Type cmpType, boolean isStatic) {
+  private static String prefixWithTypeArgumentsIfNeeded(


The "if needed" part looks off to me. WDYT?

Suggested change

private static String prefixWithTypeArgumentsIfNeeded(

private static String mayPrefixWithTypeArguments(

werli · 2022-02-15T11:19:45Z

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

+    String optionalSecondTypeArgument =
+        methodNameIsComparing ? ", " + cmpType.tsym.getSimpleName() : "";
+    return String.format(
+        "<%s%s>%s",
+        Util.treeToString(tree.getTypeArguments().get(0), state),
+        optionalSecondTypeArgument,
+        preferredMethodName);


This part feels pretty hacky. Why can't we join both potential type arguments?

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

Stephan202

Rebased and added one more commit. Suggested commit message:

Have `PrimitiveComparisonCheck` retain relevant generic type parameters (#39)

Stephan202 · 2022-04-09T11:45:13Z

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

-  private static Optional<Fix> tryFix(
-      MethodInvocationTree tree, VisitorState state, Type cmpType, boolean isStatic) {
+  private static Optional<Fix> tryMakeMethodCallMorePrecise(


I'm not sure what it means to make a method call "more precise". IMHO the old name was fine, but how about attemptMethodInvocationReplacement?

(That said, tryFix is also used in other checks.)

Yeah, I like attemptMethodInvocationReplacement. I can see why tryFix in the context of a specific check could make sense, but I feel that (e.g.) the name attemptMethodInvocationReplacement makes it clearer than the original name.

Stephan202 · 2022-04-09T11:47:11Z

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

        .map(preferredMethodName -> suggestFix(tree, preferredMethodName, state));
  }

-  private static String getPreferredMethod(VisitorState state, Type cmpType, boolean isStatic) {
+  private static String mayPrefixWithTypeArguments(


I'd expect a method named mayX to return a boolean. How about e.g. prefixTypeArgumentsIfRelevant?

Stephan202 · 2022-04-09T11:48:05Z

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

-  private static String getPreferredMethod(VisitorState state, Type cmpType, boolean isStatic) {
+  private static String mayPrefixWithTypeArguments(
+      String preferredMethodName, MethodInvocationTree tree, Type cmpType, VisitorState state) {
+    int typeArgumentsCount = tree.getTypeArguments().size();


In English "count" is preceded by the singular of whatever is counted.

Suggested change

int typeArgumentsCount = tree.getTypeArguments().size();

int typeArgumentCount = tree.getTypeArguments().size();

Stephan202 · 2022-04-09T11:49:23Z

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

+        Stream.concat(
+                Stream.of(Util.treeToString(tree.getTypeArguments().get(0), state)),
+                Stream.of(cmpType.tsym.getSimpleName()).filter(u -> methodNameIsComparing))
+            .collect(joining(","));


Suggested change

.collect(joining(","));

.collect(joining(", "));

That said, we can drop the String.format below if here we do

Suggested change

.collect(joining(","));

.collect(joining(", ", "<", ">"));

Stephan202 · 2022-04-09T12:13:50Z

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

+    int typeArgumentsCount = tree.getTypeArguments().size();
+    boolean methodNameIsComparing = "comparing".equals(preferredMethodName);
+
+    if (typeArgumentsCount == 0 || (typeArgumentsCount == 1 && !methodNameIsComparing)) {


The tests also pass if I simplify this to:

Suggested change

if (typeArgumentsCount == 0 || (typeArgumentsCount == 1 && !methodNameIsComparing)) {

if (typeArgumentsCount == 0) {

I think we should add additional tests to demonstrate in which case the added complexity is necessary. IIUC the check can then become:

Suggested change

if (typeArgumentsCount == 0 || (typeArgumentsCount == 1 && !methodNameIsComparing)) {

if (tree.getTypeArguments().isEmpty() || preferredMethodName.startsWith("then")) {

Stephan202 · 2022-04-09T12:55:52Z

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

+            "  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);",


We should also test the cases where (in a non-static import context) absent type information stays absent.

Stephan202 · 2022-04-09T13:03:51Z

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

@@ -37,7 +37,6 @@
 */
 // XXX: Add more documentation. Explain how this is useful in the face of refactoring to more
 // specific types.
-// XXX: Change this checker's name?


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.

rickie · 2022-04-11T13:57:16Z

Rebased. Changes look good!

rickie requested review from Stephan202 and werli February 9, 2022 08:54

rickie commented Feb 9, 2022

View reviewed changes

werli approved these changes Feb 15, 2022

View reviewed changes

rickie added this to the 0.1.0 milestone Apr 4, 2022

Stephan202 force-pushed the rossendrijver/primitive_comparison_type_arguments branch from 9f7ddfa to 2ef98c3 Compare April 9, 2022 13:05

Stephan202 approved these changes Apr 9, 2022

View reviewed changes

rickie and others added 4 commits April 11, 2022 15:46

PrimitiveComparison: Add type arguments if needed

dabca71

Apply suggestions

8b133f9

Some feedback from yesterday ;)

451792c

Suggestions

c873787

rickie force-pushed the rossendrijver/primitive_comparison_type_arguments branch from 2ef98c3 to c873787 Compare April 11, 2022 13:47

Stephan202 merged commit e8d9221 into master Apr 11, 2022

Stephan202 deleted the rossendrijver/primitive_comparison_type_arguments branch April 11, 2022 15:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PrimitiveComparison: Retain type arguments if present #39

PrimitiveComparison: Retain type arguments if present #39

rickie commented Feb 9, 2022

rickie Feb 9, 2022

werli Feb 15, 2022

rickie Feb 15, 2022

Stephan202 Apr 9, 2022

rickie Feb 9, 2022

rickie Feb 9, 2022

werli left a comment

werli Feb 15, 2022

werli Feb 15, 2022

rickie Feb 15, 2022

werli Feb 15, 2022

werli Feb 15, 2022

werli Feb 15, 2022

Stephan202 left a comment

Stephan202 Apr 9, 2022

rickie Apr 11, 2022

Stephan202 Apr 9, 2022

Stephan202 Apr 9, 2022

Stephan202 Apr 9, 2022

Stephan202 Apr 9, 2022

Stephan202 Apr 9, 2022

Stephan202 Apr 9, 2022

rickie commented Apr 11, 2022

	private static String prefixWithTypeArgumentsIfNeeded(
	private static String mayPrefixWithTypeArguments(

	int typeArgumentsCount = tree.getTypeArguments().size();
	int typeArgumentCount = tree.getTypeArguments().size();

	if (typeArgumentsCount == 0 \|\| (typeArgumentsCount == 1 && !methodNameIsComparing)) {
	if (typeArgumentsCount == 0) {

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

PrimitiveComparison: Retain type arguments if present #39

PrimitiveComparison: Retain type arguments if present #39

Conversation

rickie commented Feb 9, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

werli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Stephan202 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rickie commented Apr 11, 2022