From 1931898c5dbab1ba83606b1d12418a77f761c055 Mon Sep 17 00:00:00 2001 From: mohamedsamehsalah Date: Mon, 28 Oct 2024 15:35:09 +0100 Subject: [PATCH 1/2] Introduce `ClassLiteralCast` bug checker --- .../errorprone/bugpatterns/ClassCast.java | 64 +++++++++++++++++++ .../errorprone/refasterrules/ClassRules.java | 18 ------ .../errorprone/bugpatterns/ClassCastTest.java | 62 ++++++++++++++++++ .../refasterrules/ClassRulesTestInput.java | 4 -- .../refasterrules/ClassRulesTestOutput.java | 4 -- 5 files changed, 126 insertions(+), 26 deletions(-) create mode 100644 error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/ClassCast.java create mode 100644 error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/ClassCastTest.java 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/ClassCast.java new file mode 100644 index 0000000000..353340c1e5 --- /dev/null +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/ClassCast.java @@ -0,0 +1,64 @@ +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.SIMPLIFICATION; +import static tech.picnic.errorprone.utils.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.LambdaExpressionTree; +import com.sun.source.tree.TypeCastTree; +import com.sun.source.tree.VariableTree; +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. + */ +// XXX: Consider folding this logic into the `MethodReferenceUsage` check of the +// `error-prone-experimental` module. +@AutoService(BugChecker.class) +@BugPattern( + summary = + "Prefer `Class::cast` method reference over equivalent lambda expression, if applicable", + link = BUG_PATTERNS_BASE_URL + "ClassCast", + linkType = CUSTOM, + severity = SUGGESTION, + tags = SIMPLIFICATION) +public final class ClassCast extends BugChecker implements LambdaExpressionTreeMatcher { + private static final long serialVersionUID = 1L; + + /** Instantiates a new {@link ClassCast} instance. */ + public ClassCast() {} + + @Override + public Description matchLambdaExpression(LambdaExpressionTree tree, VisitorState state) { + if (tree.getParameters().size() != 1 || !(tree.getBody() instanceof TypeCastTree typeCast)) { + return Description.NO_MATCH; + } + + String classCast = SourceCode.treeToString(typeCast.getType(), state); + if (isGenericCastExpression(classCast)) { + return Description.NO_MATCH; + } + + VariableTree param = Iterables.getOnlyElement(tree.getParameters()); + if (!ASTHelpers.getSymbol(param).equals(ASTHelpers.getSymbol(typeCast.getExpression()))) { + return Description.NO_MATCH; + } + + return describeMatch(tree, SuggestedFix.replace(tree, classCast + ".class::cast")); + } + + private static boolean isGenericCastExpression(String expression) { + return expression.contains("<") && expression.contains(">"); + } +} diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ClassRules.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ClassRules.java index 496b1f1fcb..5b0d47803a 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ClassRules.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ClassRules.java @@ -73,24 +73,6 @@ Predicate after(Class clazz) { } } - /** - * Prefer {@link Class#cast(Object)} method references over lambda expressions that require naming - * a variable. - */ - // XXX: Once the `ClassReferenceCast` rule is dropped, rename this rule to just `ClassCast`. - static final class ClassLiteralCast { - @BeforeTemplate - @SuppressWarnings("unchecked") - Function before() { - return t -> (S) t; - } - - @AfterTemplate - Function after() { - return Refaster.clazz()::cast; - } - } - /** * Prefer {@link Class#cast(Object)} method references over lambda expressions that require naming * a variable. 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 new file mode 100644 index 0000000000..6106d2faa8 --- /dev/null +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/ClassCastTest.java @@ -0,0 +1,62 @@ +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/resources/tech/picnic/errorprone/refasterrules/ClassRulesTestInput.java b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ClassRulesTestInput.java index 94f6db36f9..59897bc36b 100644 --- a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ClassRulesTestInput.java +++ b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ClassRulesTestInput.java @@ -24,10 +24,6 @@ Predicate testClassReferenceIsInstancePredicate() { return s -> clazz.isInstance(s); } - Function testClassLiteralCast() { - return i -> (Integer) i; - } - Function testClassReferenceCast() { return i -> Integer.class.cast(i); } diff --git a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ClassRulesTestOutput.java b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ClassRulesTestOutput.java index 2c4314157a..fd205051bc 100644 --- a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ClassRulesTestOutput.java +++ b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ClassRulesTestOutput.java @@ -24,10 +24,6 @@ Predicate testClassReferenceIsInstancePredicate() { return clazz::isInstance; } - Function testClassLiteralCast() { - return Integer.class::cast; - } - Function testClassReferenceCast() { return Integer.class::cast; } From 0d45c3a9f80ebd60d445dc2f5f0f0724060d52e6 Mon Sep 17 00:00:00 2001 From: Stephan Schroevers Date: Mon, 28 Oct 2024 18:14:16 +0100 Subject: [PATCH 2/2] Suggestions --- ...assCast.java => ClassCastLambdaUsage.java} | 29 ++++---- .../bugpatterns/IsInstanceLambdaUsage.java | 2 + .../bugpatterns/ClassCastLambdaUsageTest.java | 68 +++++++++++++++++++ .../errorprone/bugpatterns/ClassCastTest.java | 62 ----------------- .../IsInstanceLambdaUsageTest.java | 17 ++--- 5 files changed, 94 insertions(+), 84 deletions(-) rename error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/{ClassCast.java => ClassCastLambdaUsage.java} (68%) create mode 100644 error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/ClassCastLambdaUsageTest.java delete mode 100644 error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/ClassCastTest.java 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 353340c1e5..11af5823db 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 2fa02a75e8..62bd72da8c 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 0000000000..5b445ecacf --- /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 6106d2faa8..0000000000 --- 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 a6204c5abf..e57f0fbd41 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();