diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/ClassCast.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/ClassCastLambdaUsage.java similarity index 68% rename from error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/ClassCast.java rename to error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/ClassCastLambdaUsage.java index 353340c1e53..11af5823db5 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/ClassCast.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/ClassCastLambdaUsage.java @@ -17,27 +17,29 @@ import com.sun.source.tree.LambdaExpressionTree; import com.sun.source.tree.TypeCastTree; import com.sun.source.tree.VariableTree; +import com.sun.tools.javac.code.Type; import tech.picnic.errorprone.utils.SourceCode; /** * A {@link BugChecker} that flags lambda expressions that can be replaced with a method reference - * of the form {@code T.class::cast}, if applicable. + * of the form {@code T.class::cast}. */ // XXX: Consider folding this logic into the `MethodReferenceUsage` check of the // `error-prone-experimental` module. +// XXX: This check and its tests are structurally nearly identical to `IsInstanceLambdaUsage`. +// Unless folded into `MethodReferenceUsage`, consider merging the two. @AutoService(BugChecker.class) @BugPattern( - summary = - "Prefer `Class::cast` method reference over equivalent lambda expression, if applicable", - link = BUG_PATTERNS_BASE_URL + "ClassCast", + summary = "Prefer `Class::cast` method reference over equivalent lambda expression", + link = BUG_PATTERNS_BASE_URL + "ClassCastLambdaUsage", linkType = CUSTOM, severity = SUGGESTION, tags = SIMPLIFICATION) -public final class ClassCast extends BugChecker implements LambdaExpressionTreeMatcher { +public final class ClassCastLambdaUsage extends BugChecker implements LambdaExpressionTreeMatcher { private static final long serialVersionUID = 1L; - /** Instantiates a new {@link ClassCast} instance. */ - public ClassCast() {} + /** Instantiates a new {@link ClassCastLambdaUsage} instance. */ + public ClassCastLambdaUsage() {} @Override public Description matchLambdaExpression(LambdaExpressionTree tree, VisitorState state) { @@ -45,8 +47,8 @@ public Description matchLambdaExpression(LambdaExpressionTree tree, VisitorState return Description.NO_MATCH; } - String classCast = SourceCode.treeToString(typeCast.getType(), state); - if (isGenericCastExpression(classCast)) { + Type type = ASTHelpers.getType(typeCast); + if (type == null || type.isParameterized()) { return Description.NO_MATCH; } @@ -55,10 +57,9 @@ public Description matchLambdaExpression(LambdaExpressionTree tree, VisitorState return Description.NO_MATCH; } - return describeMatch(tree, SuggestedFix.replace(tree, classCast + ".class::cast")); - } - - private static boolean isGenericCastExpression(String expression) { - return expression.contains("<") && expression.contains(">"); + return describeMatch( + tree, + SuggestedFix.replace( + tree, SourceCode.treeToString(typeCast.getType(), state) + ".class::cast")); } } 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 2fa02a75e81..62bd72da8ce 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 @@ -25,6 +25,8 @@ */ // XXX: Consider folding this logic into the `MethodReferenceUsage` check of the // `error-prone-experimental` module. +// XXX: This check and its tests are structurally nearly identical to `ClassCastLambdaUsage`. Unless +// folded into `MethodReferenceUsage`, consider merging the two. @AutoService(BugChecker.class) @BugPattern( summary = "Prefer `Class::isInstance` method reference over equivalent lambda expression", diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/ClassCastLambdaUsageTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/ClassCastLambdaUsageTest.java new file mode 100644 index 00000000000..5b445ecacfa --- /dev/null +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/ClassCastLambdaUsageTest.java @@ -0,0 +1,68 @@ +package tech.picnic.errorprone.bugpatterns; + +import com.google.errorprone.BugCheckerRefactoringTestHelper; +import com.google.errorprone.BugCheckerRefactoringTestHelper.TestMode; +import com.google.errorprone.CompilationTestHelper; +import org.junit.jupiter.api.Test; + +final class ClassCastLambdaUsageTest { + @Test + void identification() { + CompilationTestHelper.newInstance(ClassCastLambdaUsage.class, getClass()) + .addSourceLines( + "A.java", + "import com.google.common.collect.ImmutableSet;", + "import java.util.stream.Stream;", + "", + "class A {", + " void m() {", + " Number localVariable = 0;", + "", + " Stream.of(0).map(i -> i);", + " Stream.of(1).map(i -> i + 1);", + " Stream.of(2).map(Integer.class::cast);", + " Stream.of(3).map(i -> (Integer) 2);", + " Stream.of(4).map(i -> (Integer) localVariable);", + " // XXX: Ideally this case is also flagged. Pick this up in the context of merging the", + " // `ClassCastLambdaUsage` and `MethodReferenceUsage` checks, or introduce a separate check that", + " // simplifies unnecessary block lambda expressions.", + " Stream.of(5)", + " .map(", + " i -> {", + " return (Integer) i;", + " });", + " Stream.of(ImmutableSet.of(5)).map(l -> (ImmutableSet) l);", + " Stream.of(ImmutableSet.of(6)).map(l -> (ImmutableSet) l);", + " Stream.of(7).reduce((a, b) -> (Integer) a);", + "", + " // BUG: Diagnostic contains:", + " Stream.of(8).map(i -> (Integer) i);", + " }", + "}") + .doTest(); + } + + @Test + void replacement() { + BugCheckerRefactoringTestHelper.newInstance(ClassCastLambdaUsage.class, getClass()) + .addInputLines( + "A.java", + "import java.util.stream.Stream;", + "", + "class A {", + " void m() {", + " Stream.of(1).map(i -> (Integer) i);", + " }", + "}") + .addOutputLines( + "A.java", + "import java.util.stream.Stream;", + "", + "class A {", + " void m() {", + " Stream.of(1).map(Integer.class::cast);", + " }", + "}") + .doTest(TestMode.TEXT_MATCH); + } +} diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/ClassCastTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/ClassCastTest.java deleted file mode 100644 index 6106d2faa80..00000000000 --- a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/ClassCastTest.java +++ /dev/null @@ -1,62 +0,0 @@ -package tech.picnic.errorprone.bugpatterns; - -import com.google.errorprone.BugCheckerRefactoringTestHelper; -import com.google.errorprone.BugCheckerRefactoringTestHelper.TestMode; -import com.google.errorprone.CompilationTestHelper; -import org.junit.jupiter.api.Test; - -final class ClassCastTest { - @Test - void identification() { - CompilationTestHelper.newInstance(ClassCast.class, getClass()) - .addSourceLines( - "A.java", - "import com.google.common.collect.ImmutableList;", - "import com.google.common.collect.ImmutableSet;", - "import java.util.Optional;", - "import java.util.Set;", - "", - "class A {", - " void m() {", - " Number foo = 0;", - " Number bar = 1;", - "", - " // BUG: Diagnostic contains:", - " Optional.of(foo).map(i -> (Integer) i);", - "", - " Optional.of(foo).map(i -> 2);", - " Optional.of(foo).map(i -> (Integer) 2);", - " Optional.of(foo).map(i -> bar);", - " Optional.of(foo).map(i -> (Integer) bar);", - "", - " ImmutableList.of(Set.of(foo)).stream().map(l -> (ImmutableSet) l);", - " ImmutableList.of(Set.of(foo)).stream().map(l -> (ImmutableSet) l);", - " }", - "}") - .doTest(); - } - - @Test - void replacement() { - BugCheckerRefactoringTestHelper.newInstance(ClassCast.class, getClass()) - .addInputLines( - "A.java", - "import java.util.stream.Stream;", - "", - "class A {", - " void m() {", - " Stream.of(1).map(i -> (Integer) i);", - " }", - "}") - .addOutputLines( - "A.java", - "import java.util.stream.Stream;", - "", - "class A {", - " void m() {", - " Stream.of(1).map(Integer.class::cast);", - " }", - "}") - .doTest(TestMode.TEXT_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 a6204c5abf8..e57f0fbd41e 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 @@ -18,22 +18,23 @@ void identification() { " 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);", + " Stream.of(0).map(i -> i);", + " Stream.of(1).map(i -> i + 1);", + " Stream.of(2).filter(Integer.class::isInstance);", + " Stream.of(3).filter(i -> i.getClass() instanceof Class);", + " Stream.of(4).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)", + " Stream.of(5)", " .filter(", " i -> {", - " return localVariable instanceof Integer;", + " return i instanceof Integer;", " });", - " Flux.just(5, \"foo\").distinctUntilChanged(v -> v, (a, b) -> a instanceof Integer);", + " Flux.just(6, \"foo\").distinctUntilChanged(v -> v, (a, b) -> a instanceof Integer);", "", " // BUG: Diagnostic contains:", - " Stream.of(6).filter(i -> i instanceof Integer);", + " Stream.of(7).filter(i -> i instanceof Integer);", " }", "}") .doTest();