From cbef916c1c6d9feba370631220189e309aee0537 Mon Sep 17 00:00:00 2001 From: Bastien Diederichs Date: Thu, 8 Dec 2022 12:44:23 +0100 Subject: [PATCH 1/3] Improve `IsInstanceLambdaUsage` check By checking `isinstanceof` usages only against identifiers. --- .../picnic/errorprone/bugpatterns/IsInstanceLambdaUsage.java | 4 +++- .../errorprone/bugpatterns/IsInstanceLambdaUsageTest.java | 2 ++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/IsInstanceLambdaUsage.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/IsInstanceLambdaUsage.java index 874260702c..75b1c44a45 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/IsInstanceLambdaUsage.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/IsInstanceLambdaUsage.java @@ -39,7 +39,9 @@ public IsInstanceLambdaUsage() {} @Override public Description matchLambdaExpression(LambdaExpressionTree tree, VisitorState state) { - if (tree.getKind() != Kind.LAMBDA_EXPRESSION || tree.getBody().getKind() != Kind.INSTANCE_OF) { + if (tree.getKind() != Kind.LAMBDA_EXPRESSION + || tree.getBody().getKind() != Kind.INSTANCE_OF + || ((InstanceOfTree) tree.getBody()).getExpression().getKind() != Kind.IDENTIFIER) { return Description.NO_MATCH; } diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/IsInstanceLambdaUsageTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/IsInstanceLambdaUsageTest.java index 205cebd270..3fc1315004 100644 --- a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/IsInstanceLambdaUsageTest.java +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/IsInstanceLambdaUsageTest.java @@ -16,6 +16,7 @@ void identification() { compilationTestHelper .addSourceLines( "A.java", + "import com.google.common.collect.ImmutableSet;", "import java.util.stream.Stream;", "", "class A {", @@ -23,6 +24,7 @@ void identification() { " // BUG: Diagnostic contains:", " Stream.of(1).filter(i -> i instanceof Integer);", " Stream.of(2).filter(Integer.class::isInstance);", + " Stream.of(1).filter(i -> i.getClass() instanceof Class);", " }", "}") .doTest(); From f5d437ae6778e19f6481d9e91e87dba2371addf3 Mon Sep 17 00:00:00 2001 From: Bastien Diederichs Date: Thu, 8 Dec 2022 12:52:21 +0100 Subject: [PATCH 2/3] Doh --- .../picnic/errorprone/bugpatterns/IsInstanceLambdaUsageTest.java | 1 - 1 file changed, 1 deletion(-) diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/IsInstanceLambdaUsageTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/IsInstanceLambdaUsageTest.java index 3fc1315004..91d8a195eb 100644 --- a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/IsInstanceLambdaUsageTest.java +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/IsInstanceLambdaUsageTest.java @@ -16,7 +16,6 @@ void identification() { compilationTestHelper .addSourceLines( "A.java", - "import com.google.common.collect.ImmutableSet;", "import java.util.stream.Stream;", "", "class A {", From 3c2c6579eefd1c0f192cf211c4b3e263543dc1d1 Mon Sep 17 00:00:00 2001 From: Stephan Schroevers Date: Fri, 9 Dec 2022 07:36:23 +0100 Subject: [PATCH 3/3] Suggestions --- .../bugpatterns/IsInstanceLambdaUsage.java | 17 +++++++++------ .../IsInstanceLambdaUsageTest.java | 21 ++++++++++++++++--- 2 files changed, 29 insertions(+), 9 deletions(-) diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/IsInstanceLambdaUsage.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/IsInstanceLambdaUsage.java index 75b1c44a45..75c4e1b630 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/IsInstanceLambdaUsage.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/IsInstanceLambdaUsage.java @@ -6,15 +6,18 @@ import static tech.picnic.errorprone.bugpatterns.util.Documentation.BUG_PATTERNS_BASE_URL; import com.google.auto.service.AutoService; +import com.google.common.collect.Iterables; 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.google.errorprone.util.ASTHelpers; import com.sun.source.tree.InstanceOfTree; import com.sun.source.tree.LambdaExpressionTree; import com.sun.source.tree.Tree.Kind; +import com.sun.source.tree.VariableTree; import tech.picnic.errorprone.bugpatterns.util.SourceCode; /** @@ -39,17 +42,19 @@ public IsInstanceLambdaUsage() {} @Override public Description matchLambdaExpression(LambdaExpressionTree tree, VisitorState state) { - if (tree.getKind() != Kind.LAMBDA_EXPRESSION - || tree.getBody().getKind() != Kind.INSTANCE_OF - || ((InstanceOfTree) tree.getBody()).getExpression().getKind() != Kind.IDENTIFIER) { + if (tree.getParameters().size() != 1 || tree.getBody().getKind() != Kind.INSTANCE_OF) { + return Description.NO_MATCH; + } + + VariableTree param = Iterables.getOnlyElement(tree.getParameters()); + InstanceOfTree instanceOf = (InstanceOfTree) tree.getBody(); + if (!ASTHelpers.getSymbol(param).equals(ASTHelpers.getSymbol(instanceOf.getExpression()))) { return Description.NO_MATCH; } return describeMatch( tree, SuggestedFix.replace( - tree, - SourceCode.treeToString(((InstanceOfTree) tree.getBody()).getType(), state) - + ".class::isInstance")); + tree, SourceCode.treeToString(instanceOf.getType(), state) + ".class::isInstance")); } } diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/IsInstanceLambdaUsageTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/IsInstanceLambdaUsageTest.java index 91d8a195eb..3249f5a464 100644 --- a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/IsInstanceLambdaUsageTest.java +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/IsInstanceLambdaUsageTest.java @@ -17,13 +17,28 @@ void identification() { .addSourceLines( "A.java", "import java.util.stream.Stream;", + "import reactor.core.publisher.Flux;", "", "class A {", " void m() {", + " Integer localVariable = 0;", + "", + " Stream.of(0).map(i -> i + 1);", + " Stream.of(1).filter(Integer.class::isInstance);", + " Stream.of(2).filter(i -> i.getClass() instanceof Class);", + " Stream.of(3).filter(i -> localVariable instanceof Integer);", + " // XXX: Ideally this case is also flagged. Pick this up in the context of merging the", + " // `IsInstanceLambdaUsage` and `MethodReferenceUsage` checks, or introduce a separate check that", + " // simplifies unnecessary block lambda expressions.", + " Stream.of(4)", + " .filter(", + " i -> {", + " return localVariable instanceof Integer;", + " });", + " Flux.just(5, \"foo\").distinctUntilChanged(v -> v, (a, b) -> a instanceof Integer);", + "", " // BUG: Diagnostic contains:", - " Stream.of(1).filter(i -> i instanceof Integer);", - " Stream.of(2).filter(Integer.class::isInstance);", - " Stream.of(1).filter(i -> i.getClass() instanceof Class);", + " Stream.of(6).filter(i -> i instanceof Integer);", " }", "}") .doTest();