Introduce `IsInstanceLambdaUsage` check #323

Ptijohn · 2022-10-29T12:27:26Z

Creating as draft as I'm not super comfortable in the usage of Expressions and all the nitty gritty details of that...

I'm not happy in the way I'm actually matching the second case of isInstance(Object) :/
But at the same time I'm not sure I see a way to do that in a recursive way 🤔

Ptijohn · 2022-10-29T12:28:28Z

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

+import com.sun.source.tree.MemberSelectTree;
+import com.sun.source.tree.MethodInvocationTree;
+
+/** A {@link BugChecker} that aligns usages of T.class::isInstance. */


Can't @link this one 🤔 (AFAICS)
And I don't want to do @link Class#isInstance(Object) as this is precisely one of the ones that we're getting rid of.

Maybe {@code ...}?

Ptijohn · 2022-10-29T12:32:07Z

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

+    if (MEMBER_SELECT != methodInvocationTree.getMethodSelect().getKind()
+        || !((MemberSelectTree) methodInvocationTree.getMethodSelect())
+            .getIdentifier()
+            .contentEquals("isInstance")) {
+      return Description.NO_MATCH;
+    }
+    MemberSelectTree methodSelect = (MemberSelectTree) methodInvocationTree.getMethodSelect();
+    if (MEMBER_SELECT != methodSelect.getExpression().getKind()
+        || IDENTIFIER
+            != ((MemberSelectTree) methodSelect.getExpression()).getExpression().getKind()) {
+      return Description.NO_MATCH;
+    }
+    IdentifierTree identifierTree =
+        (IdentifierTree) ((MemberSelectTree) methodSelect.getExpression()).getExpression();
+    return constructDescription(tree, state, constructFix(tree, identifierTree.getName()));


That's the part I'm quite unhappy about... Looks super specific :/

Stephan202

Tnx for filing the PR we discussed! I'm currently reviewing something else; will have a look at this one later :)

Stephan202 · 2022-10-29T12:31:34Z

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

+      } else if (METHOD_INVOCATION == tree.getBody().getKind()) {
+        return treatMethodInvocation(tree, (MethodInvocationTree) tree.getBody(), state);
+      }


IIUC the i -> Integer.class.isInstance(i) case will be flagged and fixed by MethodReferenceUsage, so it seems we can simplify this check by dropping support for that case :)

Oh, that would be awesome :D
Let me try to extend the test coverage of MethodReferenceUsage then ;)

If there is no suitable/equivalent test yet (didn't check): cool!

Yep exactly :)

Added this test to MethodReferenceUsage:

" // BUG: Diagnostic contains:", " Stream.of(1).filter(v -> Integer.class.isInstance(v));",

And it was actually not flagged 🤔
I'm wondering if it's because of the .class.isInstance 🤔
But then it indeed feels like it should be MethodReferenceUsage that should be extended there.

Hahaha, yeah I actually got inspired from this code, and got slightly afraid 😅
I'll leave that out for now.
That being said, I'll check in the branch 👌
And then adapt this code.
Thanks for the quick feedback!

Not covered in the new branch, will drop an XXX on MethodReferenceUsage and adapt the rest!

Done, PTAL :)

Tnx! On the review list it goes 😄

Stephan202 · 2022-10-31T05:25:57Z

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

+    return SuggestedFixes.compilesWithFix(
+            fix, state, ImmutableList.of(), /* onlyInSameCompilationUnit= */ true)


Aye, compilesWithFix is very convenient, but also very expensive. Unfortunately we'll have to find a way to avoid this. @Ptijohn which issue did you hit without this?

(In case you're wondering: indeed the MethodReferenceUsage check also relies on compilesWithFix, and this is why it hasn't been enabled yet for the internal code base. It's the primary reason for the work on this branch.)

To be honest? It's mainly that I'm discovering the tool 😅 (and thus I didn't know about the limitations of this one)
I will dig to see what's best in this case 👌

Oh so IIUC you just exact that we can just drop it right?
Will push it :)
I indeed thought for a sec that the compilexWithFix was a mandatory step in the validation of such a mechanism, but makes sense that it's not the case was "simpler" changes like this one.

rickie

Added a commit.

Left some questions and considerations :).

Cool check @Ptijohn 🚀 !

Suggested commit message (name subject to change):

Introduce `IsInstanceLambdaUsage` check (#323)

rickie · 2022-11-03T20:47:25Z

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

+import com.sun.source.tree.MemberSelectTree;
+import com.sun.source.tree.MethodInvocationTree;
+
+/** A {@link BugChecker} that aligns usages of T.class::isInstance. */


Maybe {@code ...}?

rickie · 2022-11-03T20:50:31Z

error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/IsInstanceUsageTest.java

+            "    Flux.just(1).filter(Integer.class::isInstance);",
+            "  }",
+            "}")
+        .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH);


Suggested change

.doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH);

.doTest(TestMode.TEXT_MATCH);

We usually remove the qualifier here :).

rickie · 2022-11-03T20:51:18Z

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

@@ -106,6 +106,7 @@ private static Optional<SuggestedFix.Builder> constructMethodRef(
  }

  // XXX: Replace nested `Optional` usage.
+  // XXX: Cover for deeper method invocation like `T.class.isInstance(i)`.


Suggested change

// XXX: Cover for deeper method invocation like `T.class.isInstance(i)`.

// XXX: Cover for deeper method invocations like `T.class.isInstance(i)`.

Right? 👀

rickie · 2022-11-03T20:52:33Z

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

+      return constructDescription(
+          tree, constructFix(tree, ((InstanceOfTree) tree.getBody()).getType()));
+    }
+    return Description.NO_MATCH;


Two things:
(1) I think we can simplify this a bit by inlining these methods.
(2) We usually first handle the return Description.NO_MATCH; case, so I'd suggest to invert this :).

rickie · 2022-11-03T20:53:51Z

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

+    link = BUG_PATTERNS_BASE_URL + "IsInstanceUsage",
+    linkType = CUSTOM,
+    severity = SUGGESTION,
+    tags = STYLE)


Although I agree with STYLE this could also be a SIMPLIFICATION, WDYT?

Edit: One thing to consider is that it could make the Javadoc a bit more strong if we use simplification. Using "aligns" in the Javadoc would mean this is a bit of an opinionated check, while this is IMO an actual improvement (which in this case would mean a simplification).

I see that MethodReferenceUsage does also use STYLE, I think in this case SIMPLIFICATION would work too. (In the general case method references may be longer, and are perhaps not always "simpler".)

rickie · 2022-11-03T21:10:18Z

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

+    linkType = CUSTOM,
+    severity = SUGGESTION,
+    tags = STYLE)
+public final class IsInstanceUsage extends BugChecker implements LambdaExpressionTreeMatcher {


As this is quite specific to lambda's, we could update the name to reflect that. Maybe something like LambdaIsInstanceUage or IsInstanceLambdaUsage?

rickie · 2022-11-03T21:10:59Z

error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/IsInstanceUsageTest.java

+            "import reactor.core.publisher.Flux;",
+            "",
+            "class A {",
+            "  void m() {",


It'd be nice to also add a non-Reactor related example.

Will push something.

Stephan202

@rickie since you didn't push yet, let me push what I had (not flushed earlier because I had to leave the train).

Stephan202 · 2022-11-03T19:15:28Z

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

+
+  @Override
+  public Description matchLambdaExpression(LambdaExpressionTree tree, VisitorState state) {
+    if (LAMBDA_EXPRESSION == tree.getKind() && INSTANCE_OF == tree.getBody().getKind()) {


We generally do the NO_MATCH case first, have the enum values on the RHS, and import Kind. (Though I see we're not fully consistent on the latter 👀.

Stephan202 · 2022-11-03T19:16:01Z

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

+  private Description constructDescription(
+      LambdaExpressionTree tree, SuggestedFix.Builder fixBuilder) {
+    return describeMatch(tree, fixBuilder.build());
+  }
+
+  private static SuggestedFix.Builder constructFix(LambdaExpressionTree tree, Object target) {
+    return SuggestedFix.builder().replace(tree, target + ".class::isInstance");
+  }


These can be inlined without impacting readability, IMHO. Will push a proposal.

Stephan202 · 2022-11-03T19:18:44Z

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

@@ -106,6 +106,7 @@ private static Optional<SuggestedFix.Builder> constructMethodRef(
  }

  // XXX: Replace nested `Optional` usage.
+  // XXX: Cover for deeper method invocation like `T.class.isInstance(i)`.


Without diving into the root cause I'm not sure what the best place for this comment is; let's move it to the top of the class.

Stephan202 · 2022-11-03T19:22:53Z

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

+    linkType = CUSTOM,
+    severity = SUGGESTION,
+    tags = STYLE)
+public final class IsInstanceUsage extends BugChecker implements LambdaExpressionTreeMatcher {


The MethodReferenceUsage javadoc states:

A {@link BugChecker} that flags lambda expressions that can be replaced with method references.

By that logic we could collapse these two checks. But since the other one isn't active yet, let's keep his one separate and revisit this option later. Will add a comment :)

Stephan202 · 2022-11-03T19:26:15Z

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

+  }
+
+  private static SuggestedFix.Builder constructFix(LambdaExpressionTree tree, Object target) {
+    return SuggestedFix.builder().replace(tree, target + ".class::isInstance");


We can avoid the builder if we use SuggestedFix#replace.

Stephan202 · 2022-11-03T19:35:33Z

error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/IsInstanceUsageTest.java

+            "class A {",
+            "  void m() {",
+            "    // BUG: Diagnostic contains:",
+            "    Flux.just(1).filter(i -> i instanceof Integer);",


Given the simplicity of the check I guess a single case suffices. Propose that we use a JDK API such as Stream for this.

Stephan202 · 2022-11-03T19:54:40Z

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

+    return describeMatch(tree, fixBuilder.build());
+  }
+
+  private static SuggestedFix.Builder constructFix(LambdaExpressionTree tree, Object target) {


Suggested change

private static SuggestedFix.Builder constructFix(LambdaExpressionTree tree, Object target) {

private static SuggestedFix.Builder constructFix(LambdaExpressionTree tree, Tree target) {

... and if we do that, we get an error that Tree#toString shouldn't be used. For this we have SourceCode#treeToString. :)

rickie · 2022-11-03T21:24:39Z

Will start resolving the conflicts 😄.
Okay we almost had the exact same changes so basically nothing is left haha.

For me the only things left are the naming discussion and the tag discussion.

Stephan202 · 2022-11-03T21:34:15Z

Okay we almost had the exact same changes so basically nothing is left haha.

GMTA!

(Will have a look at the open points in a bit.)

Stephan202

Added a commit and tweaked the suggested commit message :)

Stephan202 · 2022-11-03T21:43:29Z

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

+    link = BUG_PATTERNS_BASE_URL + "IsInstanceUsage",
+    linkType = CUSTOM,
+    severity = SUGGESTION,
+    tags = STYLE)


I see that MethodReferenceUsage does also use STYLE, I think in this case SIMPLIFICATION would work too. (In the general case method references may be longer, and are perhaps not always "simpler".)

Ptijohn · 2022-11-04T08:22:29Z

Wow thank you both for the reviews and extra commits @rickie and @Stephan202 💪
I still need to go fully through them, but I think there's a lot of learning for me there :D
If you want to merge before that, don't hesitate, I'm quite full today so I don't know when I'll to it 🤔

Ptijohn

Alright, can't approve myself, but got through the feedback, I learned a lot for next time :D
Thanks! 💪

rickie · 2022-11-04T10:47:45Z

That's the spirit 🚀 😄 @Ptijohn !

Ptijohn requested a review from Stephan202 October 29, 2022 12:27

Ptijohn commented Oct 29, 2022

View reviewed changes

Stephan202 reviewed Oct 29, 2022

View reviewed changes

Stephan202 mentioned this pull request Oct 29, 2022

Introduce assorted Reactor error handling Refaster rules #318

Merged

Ptijohn force-pushed the bdiederichs/rewrite-isinstance branch from 005de9d to 23fcdd6 Compare October 29, 2022 13:40

Ptijohn marked this pull request as ready for review October 29, 2022 13:40

Stephan202 reviewed Oct 31, 2022

View reviewed changes

rickie self-requested a review October 31, 2022 07:16

Ptijohn force-pushed the bdiederichs/rewrite-isinstance branch from 4b276c9 to b223227 Compare October 31, 2022 07:38

Ptijohn requested a review from Stephan202 November 3, 2022 15:18

Ptijohn added 3 commits November 3, 2022 21:42

Align usages of Class::isInstance

c3b5ec8

Simplify

135f75c

Simplify more

6c223b3

rickie force-pushed the bdiederichs/rewrite-isinstance branch from b223227 to 6c223b3 Compare November 3, 2022 20:42

rickie added the new feature label Nov 3, 2022

rickie reviewed Nov 3, 2022

View reviewed changes

rickie added this to the 0.6.0 milestone Nov 3, 2022

Suggestions

cb4d9a9

Stephan202 reviewed Nov 3, 2022

View reviewed changes

Suggestions (2)

9f8b928

Stephan202 approved these changes Nov 3, 2022

View reviewed changes

rickie approved these changes Nov 4, 2022

View reviewed changes

Ptijohn commented Nov 4, 2022

View reviewed changes

rickie changed the title ~~Align usages of Class::isInstance~~ Introduce IsInstanceLambdaUsage check Nov 4, 2022

rickie merged commit 42e632e into master Nov 4, 2022

rickie deleted the bdiederichs/rewrite-isinstance branch November 4, 2022 10:47

		return SuggestedFixes.compilesWithFix(
		fix, state, ImmutableList.of(), /* onlyInSameCompilationUnit= */ true)

	.doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH);
	.doTest(TestMode.TEXT_MATCH);

	// XXX: Cover for deeper method invocation like `T.class.isInstance(i)`.
	// XXX: Cover for deeper method invocations like `T.class.isInstance(i)`.

	private static SuggestedFix.Builder constructFix(LambdaExpressionTree tree, Object target) {
	private static SuggestedFix.Builder constructFix(LambdaExpressionTree tree, Tree target) {

Introduce IsInstanceLambdaUsage check #323

Introduce IsInstanceLambdaUsage check #323

Conversation

Ptijohn commented Oct 29, 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

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

Ptijohn Oct 29, 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

rickie left a comment • edited by Stephan202

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

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 Nov 3, 2022

Stephan202 commented Nov 3, 2022

Stephan202 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ptijohn commented Nov 4, 2022

Ptijohn left a comment

Choose a reason for hiding this comment

rickie commented Nov 4, 2022

Introduce `IsInstanceLambdaUsage` check #323

Introduce `IsInstanceLambdaUsage` check #323

Ptijohn Oct 29, 2022 •

edited

rickie left a comment •

edited by Stephan202