Introduce `IdentityConversionCheck` #27

rickie · 2022-01-05T13:13:10Z

No description provided.

rickie · 2022-01-05T13:14:01Z

This PR replaces: PicnicSupermarket/error-prone#5

rickie · 2022-01-06T15:43:12Z

The IdentityConversionCheck replaces a number of Refaster templates 😄 . Really like the EPS self-check 😉 .

werli

Some suggestions and questions 🙂

werli · 2022-01-07T10:38:13Z

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

+                  Integer.class.getName(),
+                  String.class.getName())
+              .named("valueOf"),
+          staticMethod().onClass("reactor.adapter.rxjava.RxJava2Adapter"),


Probably I just had a really hard time finding it, for which method does this check apply for the RxJava2Adapter? 🤔

For the conversion methods like flowableToFlux and singleToMono. The idea for this check came while we were working on the migration of RxJava to Reactor. What we saw is that there were unnecessary conversions left after the migration was "complete". Since the Flux#flatMap accepts a Publisher, a Flowable is valid input for the flatMap. What we saw is RxJava2Adapter.fluxToFlowable(flux) in the flatMap. This check should identify these cases and remove the unnecessary conversion. In this class, you can see that we could remove some candidates because of this check.

Ah, that makes total sense, thanks a lot!

NB: Would still make sense to test this. 👀

werli · 2022-01-07T10:44:10Z

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

+          staticMethod()
+              .onClass("reactor.core.publisher.Flux")
+              .namedAnyOf("concat", "firstWithSignal", "from", "merge"),


IIUC for Flux#concat, this check only handles the

Flux<T> concat(Publisher<? extends T>... sources)

case. What about e.g.

Flux<T> concat(Iterable<? extends Publisher<? extends T>> sources)

couldn't we statically also check whether the Iterable only contains a single element?

Or would this be handled by a combination by refaster + this check already? 👀

Same thing should also apply for Flux#merge and its overloads.

Interesting, good one. I think we could best achieve this by creating a Matcher that is named something like: ...ContainsSingleElement and add some Refaster templates that use the Matcher. However, if we decide to go for this, I would suggest a follow-up PR.

werli · 2022-01-07T10:50:27Z

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

+                  "com.google.common.collect.ImmutableBiMap",
+                  "com.google.common.collect.ImmutableList",


What about ImmutableEnumMap and ImmutableEnumSet?

They don't have a copyOf method 😉

Hmm, both inherit a copyOf method from Immutable{Map,Set} respectively.
I frankly don't know: Would this then already be covered by the defined Immutable{Map,Set} here?

Ah good one, I totally forgot that 😄. Will check that.

ImmutableEnumMap and ImmutableEnumSet are package-private and final. So in the source code we can only ever match (public) supertypes of these. I.o.w.: ImmutableEnumMap and ImmutableEnumSet are implementation details we don't need to concern ourselves with.

werli · 2022-01-07T10:51:40Z

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

+                  "com.google.common.collect.ImmutableMap",
+                  "com.google.common.collect.ImmutableMultimap",
+                  "com.google.common.collect.ImmutableMultiset",
+                  "com.google.common.collect.ImmutableRangeSet",


TIL of a RangeSet, and of a RangeMap. Why not add the latter as ImmutableRangeMap?

Good one, that one has a copyOf method 😄 .

werli · 2022-01-07T10:54:17Z

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

+
+    ExpressionTree sourceTree = arguments.get(0);
+    Type sourceType = ASTHelpers.getType(sourceTree);
+    TargetType targetType = ASTHelpers.targetType(state);


Genuine question: What's the target type here exactly? Why not ASTHelpers.getType(state)? 🙇

Javadoc getType:

Returns the Type of the given tree, or null if the type could not be determined.

Javadoc getTargetType:

Returns the target type of the tree at the given VisitorState's path, or else null.
For example, the target type of an assignment expression is the variable's type, and the target type of a return statement is the enclosing method's type.

Nice question; the type would only give us information about the current expression itself (state), while we want to know what the "expected" type of the current expression is (i.o.w. what the parent expects/wants to receive) and thus what the target type is of this expression.

So in the Flux#flatMap case, let's say we matched a RxJava2Adapter.fluxToFlowable(flux) and we ask for the target type, we know that we want to have Publisher in the end. A Flux is already satiesfies that, so we can remove the conversion.

werli · 2022-01-07T10:56:00Z

...ne-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/IdentityConversionCheckTest.java

+  @Test
+  void identification() {


Frankly, this test doesn't cover nearly all of the cases. I don't feel comfortable approving without all cases being covered in tests, knowing this will be rolled out to all Picnic services and even being open sourced. 👀

I added extra identifications. I'm not sure whether adding more cases would be that helpful, but if you feel it would be, let me know 😄 .

I don't see a reason not to test all cases, especially since the reactive ones are not yet tested and these are the actual more complex ones here.

Stephan202

Forgot to flush this comment.

Stephan202 · 2022-01-11T07:31:09Z

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

+                  "com.google.common.collect.ImmutableBiMap",
+                  "com.google.common.collect.ImmutableList",


ImmutableEnumMap and ImmutableEnumSet are package-private and final. So in the source code we can only ever match (public) supertypes of these. I.o.w.: ImmutableEnumMap and ImmutableEnumSet are implementation details we don't need to concern ourselves with.

rickie · 2022-01-11T10:48:26Z

Working on some extra tests, but I don't know when I will finish that 😄, to be continued.

rickie · 2022-01-11T18:18:50Z

I wrote extra tests, tests for RxJava2Adapter, Flux and Mono. I think now the tests are sufficient. PTAL 😄.

werli

Okay, nothing more from my side.

Thanks a lot for adding the tests. 👍

...ne-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/IdentityConversionCheckTest.java

Also remove identity conversions if the source type is equal to the type of the current tree.

rickie · 2022-02-18T11:47:03Z

Here we identified that we should look into the TypesWithUndefinedEquality. During one of our conversations we discussed that java.lang.Object was not defined in that enum and suggested that there was a chance we have to add it ourselves.

Wrote some extra tests to validate that this approach works. It appeared that to make this work, we have to add java.lang.Object to the enum. Therefore, this PR is opened in the fork: PicnicSupermarket/error-prone#29.
This PR will now fail, because it expects the java.lang.Object to be there. Let's discuss this solution and whether it is the right approach.

While working on this, I identified another case we were missing. Namely the case where the type of the conversion method is the same as the type we are passing to the (e.g.) copyOf (i.o.w. the sourceType here in the code). In hindsight this is an obvious case but the focus (at least for me) was on comparing the targetType with the sourceType. An extra check (and tests) are added to account for this case.

rickie · 2022-03-02T16:07:39Z

Updated the code to check for java.lang.Object.

Suggested commit message:

Introduce `IdentityConversionCheck`

Remove templates that are now covered by this check.

rickie · 2022-03-18T21:13:30Z

Encountered a few instances of yield in payments so the checkState was not good enough 😄.

rickie · 2022-03-18T21:15:07Z

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

-        "sourceType `%s` or targetType `%s` is null.",
-        sourceType,
-        targetType);
+    if (sourceType == null || targetType == null) {


I changed the checkState to an if statement, while we could also add these checks to the one below. However, I feel that this is a nice separation and (arguably) improves readability. Also fine with merging the two 😅 .

Stephan202

Added a commit. Did not yet review the tests :)

Stephan202 · 2022-04-05T12:39:37Z

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

+
+  private static boolean isSubtypeWithDefinedEquality(
+      Type sourceType, Type targetType, VisitorState state) {
+    Type objectType = typeFromString("java.lang.Object").get(state);


We should not evaluate typeFromString("java.lang.Object") each time. In this case we can reuse com.google.errorprone.suppliers.Suppliers.OBJECT_TYPE.

Stephan202 · 2022-04-05T12:44:25Z

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

+    return types.isSubtype(sourceType, targetType)
+        && !types.isSameType(sourceType, objectType)
+        && !types.isSameType(targetType, objectType)


The first and third condition imply the second, so we can drop that one. I also suspect that the third check is faster, so would propose to test that one first.

(This also shows that we should run PITest against PRs. Maybe something for OSS support 🙈.)

Good one, added it to the list.

Stephan202 · 2022-04-05T13:41:08Z

...-prone-contrib/src/main/java/tech/picnic/errorprone/refastertemplates/AssortedTemplates.java

      return Refaster.anyOf(
-          Collections.disjoint(ImmutableSet.copyOf(collection1), collection2),
          Collections.disjoint(new HashSet<>(collection1), collection2),
-          Collections.disjoint(collection1, ImmutableSet.copyOf(collection2)),
          Collections.disjoint(collection1, new HashSet<>(collection2)));


IIUC these changes can now be reverted.

Stephan202

Added two more commits. I think that's it from my side. Very nice!

Tweaked suggested commit message:

Introduce `IdentityConversionCheck` (#27)

And remove any Refaster templates it subsumes.

Stephan202 · 2022-04-05T16:14:28Z

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

+      return Description.NO_MATCH;
+    }
+
+    if (state.getTypes().isSameType(sourceType, ASTHelpers.getType(tree))


I think that in practice ASTHelpers.getType(tree) won't ever return null here, but the method does allow it. Since isSameType would then throw an NPE, perhaps we can make this a bit more robust.

Stephan202 · 2022-04-05T16:17:28Z

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

+          .addFix(SuggestedFixes.addSuppressWarnings(state, canonicalName()))
+          .build();
+    }
+    return Description.NO_MATCH;


We can reduce the indentation by matching the case. Early return Description.NO_MATCH statements also matches other code in this repo.

Stephan202 · 2022-04-05T16:18:40Z

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

+      return buildDescription(tree)
+          .setMessage(
+              "This method invocation appears redundant; remove it or suppress this warning and "
+                  + "add an comment explaining its purpose")


Suggested change

+ "add an comment explaining its purpose")

+ "add a comment explaining its purpose")

Stephan202 · 2022-04-05T16:19:48Z

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

+          .setMessage(
+              "This method invocation appears redundant; remove it or suppress this warning and "
+                  + "add an comment explaining its purpose")
+          .addFix(SuggestedFix.replace(tree, state.getSourceForNode(sourceTree)))


Suggested change

.addFix(SuggestedFix.replace(tree, state.getSourceForNode(sourceTree)))

.addFix(SuggestedFix.replace(tree, Util.treeToString(sourceTree, state)))

Stephan202 · 2022-04-05T16:27:37Z

...ne-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/IdentityConversionCheckTest.java

+            "  public void foo() {",
+            "    // BUG: Diagnostic contains:",
+            "    Byte b1 = Byte.valueOf((Byte) Byte.MIN_VALUE);",
+            "    Byte b2 = Byte.valueOf(Byte.MIN_VALUE);",


Arguably we should flag this one as well (IDEA does, too). Sounds like we should more generically handle comparison between boxed and unboxed types. Looks like all we need to change is isSubtype -> isConvertible.

When doing this some other not-so-relevant Refaster templates are flagged; will drop those.

Stephan202 · 2022-04-05T16:33:41Z

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

+    return Description.NO_MATCH;
+  }
+
+  private static boolean isSubtypeWithDefinedEquality(


"Defined" might be a bit vague. How about "well-defined"?

Suggested change

private static boolean isSubtypeWithDefinedEquality(

private static boolean isSubtypeWithWellDefinedEquality(

Thought it wasn't "worth" it to make the method longer, but I can see how this is clearer :).

Stephan202 · 2022-04-05T16:35:53Z

...ne-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/IdentityConversionCheckTest.java

+            "    String s2 = String.valueOf(\"1\");",
+            "",
+            "    // BUG: Diagnostic contains:",
+            "    ImmutableBiMap<Object, Object> i2 = ImmutableBiMap.copyOf(ImmutableBiMap.of());",


What happened to i1? 😄

Stephan202 · 2022-04-05T18:22:47Z

...ne-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/IdentityConversionCheckTest.java

+            "",
+            "    // BUG: Diagnostic contains:",
+            "    Integer int1 = Integer.valueOf((Integer) 1);",
+            "    Integer int2 = Integer.valueOf(1);",
+            "    // BUG: Diagnostic contains:",
+            "    int int3 = Integer.valueOf((Integer) 1);",
+            "    // BUG: Diagnostic contains:",
+            "    int int4 = Integer.valueOf(1);",


If we're trying to be complete, then we need a few more sections for other (boxed) primitives :)

Additionally, in some context int -> long conversions also happen implicitly, so let's add a test for that. (Likewise for other primitives, but we need to stop somewhere.)

Stephan202 · 2022-04-05T18:24:14Z

...ne-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/IdentityConversionCheckTest.java

+            "    Character c1 = Character.valueOf((Character) 'a');",
+            "    Character c2 = Character.valueOf('a');",
+            "    // BUG: Diagnostic contains:",
+            "    char c3 = Character.valueOf((Character)'a');",


Suggested change

" char c3 = Character.valueOf((Character)'a');",

" char c3 = Character.valueOf((Character) 'a');",

Stephan202 · 2022-04-05T18:28:06Z

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

+                  Byte.class.getName(),
+                  Character.class.getName(),
+                  Double.class.getName(),
+                  Float.class.getName(),
+                  Integer.class.getName(),
+                  String.class.getName())


Boolean, Short and Long? :) We could use Primitives.allWrapperTypes() here. (Void.valueOf doesn't exist, but that's okay.)

That's a nice one! Didn't know that one.

Stephan202 · 2022-04-05T20:52:06Z

Hmm, in case the converted value is passed to a method, then the suggested fix may cause another overload to be selected. It would be odd if that overload had different semantics, but this could happen. Maybe we should detect this case? (Or just add an XXX comment and see whether this is ever an issue in practice?)

rickie · 2022-04-06T12:58:18Z

I like the suggestions 🚀!

Pushed a minor tweak in the @BugPattern annotation.
Besides that, did a suggestions for the XXX. WDYT?

Stephan202

Added one more commit.

Repeating the suggested commit message:

Introduce `IdentityConversionCheck` (#27)

And remove any Refaster templates it subsumes.

rickie requested review from Stephan202 and werli January 5, 2022 13:13

werli reviewed Jan 7, 2022

View reviewed changes

Stephan202 reviewed Jan 11, 2022

View reviewed changes

werli self-requested a review January 12, 2022 11:02

werli approved these changes Jan 24, 2022

View reviewed changes

...ne-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/IdentityConversionCheckTest.java Outdated Show resolved Hide resolved

...ne-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/IdentityConversionCheckTest.java Show resolved Hide resolved

rickie and others added 6 commits February 18, 2022 12:29

Introduce IdentityConversionCheck

b592066

Remove obsolete Refaster templates

57180ca

Add extra testcases and apply suggestion

07ebfd3

Add more test cases

e060553

IdentityConversion: drop *Sorted* candidates

8bf0f8f

IdentityConversion: validate types have a defined equality check

9c9825e

Also remove identity conversions if the source type is equal to the type of the current tree.

rickie force-pushed the rossendrijver/identity_conversion branch from b45636f to 9c9825e Compare February 18, 2022 11:34

Add check for java.lang.Object in IdentityConversion

bd40a22

Remove checkState as targetType can be null in the case of a yield

6886c85

rickie commented Mar 18, 2022

View reviewed changes

rickie added this to the 0.1.0 milestone Apr 4, 2022

Suggestions

47205e6

Stephan202 reviewed Apr 5, 2022

View reviewed changes

Stephan202 added 2 commits April 5, 2022 21:49

More suggestions

943eebc

Trim some more templates

d9bc85a

Stephan202 approved these changes Apr 5, 2022

View reviewed changes

Suggestions

1f0ccf9

Suggestion

0506118

Stephan202 approved these changes Apr 8, 2022

View reviewed changes

Stephan202 merged commit d2bbee3 into master Apr 8, 2022

Stephan202 deleted the rossendrijver/identity_conversion branch April 8, 2022 11:28

		"com.google.common.collect.ImmutableBiMap",
		"com.google.common.collect.ImmutableList",

	+ "add an comment explaining its purpose")
	+ "add a comment explaining its purpose")

	.addFix(SuggestedFix.replace(tree, state.getSourceForNode(sourceTree)))
	.addFix(SuggestedFix.replace(tree, Util.treeToString(sourceTree, state)))

	private static boolean isSubtypeWithDefinedEquality(
	private static boolean isSubtypeWithWellDefinedEquality(

	" char c3 = Character.valueOf((Character)'a');",
	" char c3 = Character.valueOf((Character) 'a');",

Introduce IdentityConversionCheck #27

Introduce IdentityConversionCheck #27

Conversation

rickie commented Jan 5, 2022

rickie commented Jan 5, 2022 • edited

rickie commented Jan 6, 2022

werli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rickie Jan 8, 2022 • edited

Choose a reason for hiding this comment

werli Jan 10, 2022 • edited

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

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

rickie commented Jan 11, 2022

rickie commented Jan 11, 2022

werli left a comment

Choose a reason for hiding this comment

rickie commented Feb 18, 2022

rickie commented Mar 2, 2022 • edited

rickie commented Mar 18, 2022

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

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

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 commented Apr 5, 2022

rickie commented Apr 6, 2022

Stephan202 left a comment

Choose a reason for hiding this comment

Introduce `IdentityConversionCheck` #27

Introduce `IdentityConversionCheck` #27

rickie commented Jan 5, 2022 •

edited

rickie Jan 8, 2022 •

edited

werli Jan 10, 2022 •

edited

rickie commented Mar 2, 2022 •

edited