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.

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

Introduce IsInstanceLambdaUsage check #323

Merged
merged 5 commits into from
Nov 4, 2022
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
package tech.picnic.errorprone.bugpatterns;

import static com.google.errorprone.BugPattern.LinkType.CUSTOM;
import static com.google.errorprone.BugPattern.SeverityLevel.SUGGESTION;
import static com.google.errorprone.BugPattern.StandardTags.STYLE;
import static com.sun.source.tree.Tree.Kind.INSTANCE_OF;
import static com.sun.source.tree.Tree.Kind.LAMBDA_EXPRESSION;
import static tech.picnic.errorprone.bugpatterns.util.Documentation.BUG_PATTERNS_BASE_URL;

import com.google.auto.service.AutoService;
import com.google.errorprone.BugPattern;
import com.google.errorprone.VisitorState;
import com.google.errorprone.bugpatterns.BugChecker;
import com.google.errorprone.bugpatterns.BugChecker.LambdaExpressionTreeMatcher;
import com.google.errorprone.fixes.SuggestedFix;
import com.google.errorprone.matchers.Description;
import com.sun.source.tree.InstanceOfTree;
import com.sun.source.tree.LambdaExpressionTree;

/** A {@link BugChecker} that aligns usages of T.class::isInstance. */
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe {@code ...}?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤦

@AutoService(BugChecker.class)
@BugPattern(
summary = "Use Class::isInstance where possible",
link = BUG_PATTERNS_BASE_URL + "IsInstanceUsage",
linkType = CUSTOM,
severity = SUGGESTION,
tags = STYLE)
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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".)

public final class IsInstanceUsage extends BugChecker implements LambdaExpressionTreeMatcher {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

private static final long serialVersionUID = 1L;

/** Instantiates a new {@link IsInstanceUsage} instance. */
public IsInstanceUsage() {}

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

Choose a reason for hiding this comment

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

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

return constructDescription(
tree, constructFix(tree, ((InstanceOfTree) tree.getBody()).getType()));
}
return Description.NO_MATCH;
Copy link
Member

Choose a reason for hiding this comment

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

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 :).

}

private Description constructDescription(
LambdaExpressionTree tree, SuggestedFix.Builder fixBuilder) {
return describeMatch(tree, fixBuilder.build());
}

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

Choose a reason for hiding this comment

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

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. :)

return SuggestedFix.builder().replace(tree, target + ".class::isInstance");
Copy link
Member

Choose a reason for hiding this comment

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

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

}
Copy link
Member

Choose a reason for hiding this comment

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

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

}
Original file line number Diff line number Diff line change
Expand Up @@ -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)`.
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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? 👀

@SuppressWarnings("NestedOptionals")
private static Optional<SuggestedFix.Builder> constructMethodRef(
LambdaExpressionTree lambdaExpr, MethodInvocationTree subTree) {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
package tech.picnic.errorprone.bugpatterns;

import com.google.errorprone.BugCheckerRefactoringTestHelper;
import com.google.errorprone.CompilationTestHelper;
import org.junit.jupiter.api.Test;

final class IsInstanceUsageTest {
private final CompilationTestHelper compilationTestHelper =
CompilationTestHelper.newInstance(IsInstanceUsage.class, getClass());
private final BugCheckerRefactoringTestHelper refactoringTestHelper =
BugCheckerRefactoringTestHelper.newInstance(IsInstanceUsage.class, getClass());

@Test
void identification() {
compilationTestHelper
.addSourceLines(
"A.java",
"import reactor.core.publisher.Flux;",
"",
"class A {",
" void m() {",
Copy link
Member

Choose a reason for hiding this comment

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

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

Will push something.

" // BUG: Diagnostic contains:",
" Flux.just(1).filter(i -> i instanceof Integer);",
Copy link
Member

Choose a reason for hiding this comment

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

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

" // BUG: Diagnostic contains:",
" Flux.just(1).onErrorResume(t -> t instanceof Exception, t -> Flux.empty());",
"",
" Flux.just(1).filter(Integer.class::isInstance);",
" Flux.just(1).onErrorResume(Exception.class, t -> Flux.empty());",
" Flux.just(1).onErrorResume(Exception.class::isInstance, t -> Flux.empty());",
" }",
"}")
.doTest();
}

@Test
void replacement() {
refactoringTestHelper
.addInputLines(
"A.java",
"import reactor.core.publisher.Flux;",
"",
"class A {",
" void m() {",
" Flux.just(1).filter(i -> i instanceof Integer);",
" }",
"}")
.addOutputLines(
"A.java",
"import reactor.core.publisher.Flux;",
"",
"class A {",
" void m() {",
" Flux.just(1).filter(Integer.class::isInstance);",
" }",
"}")
.doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH);
Copy link
Member

Choose a reason for hiding this comment

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

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

We usually remove the qualifier here :).

}
}