From a1271368a58b66c785776609597b4542aefed97f Mon Sep 17 00:00:00 2001 From: Stephan Schroevers Date: Sat, 20 May 2023 17:58:13 +0200 Subject: [PATCH] Suggestions --- .../refasterrules/OptionalRules.java | 28 ++-- .../refasterrules/OptionalRulesTestInput.java | 4 +- .../OptionalRulesTestOutput.java | 4 +- .../matchers/IsLikelyTrivialComputation.java | 92 +++++++++++ .../IsMethodInvocationWithTwoOrMoreArgs.java | 20 --- .../IsLikelyTrivialComputationTest.java | 147 ++++++++++++++++++ ...MethodInvocationWithTwoOrMoreArgsTest.java | 69 -------- 7 files changed, 259 insertions(+), 105 deletions(-) create mode 100644 refaster-support/src/main/java/tech/picnic/errorprone/refaster/matchers/IsLikelyTrivialComputation.java delete mode 100644 refaster-support/src/main/java/tech/picnic/errorprone/refaster/matchers/IsMethodInvocationWithTwoOrMoreArgs.java create mode 100644 refaster-support/src/test/java/tech/picnic/errorprone/refaster/matchers/IsLikelyTrivialComputationTest.java delete mode 100644 refaster-support/src/test/java/tech/picnic/errorprone/refaster/matchers/IsMethodInvocationWithTwoOrMoreArgsTest.java diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/OptionalRules.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/OptionalRules.java index ce70017f3e5..1d9e5c1c28c 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/OptionalRules.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/OptionalRules.java @@ -6,8 +6,8 @@ import com.google.errorprone.refaster.Refaster; import com.google.errorprone.refaster.annotation.AfterTemplate; import com.google.errorprone.refaster.annotation.BeforeTemplate; -import com.google.errorprone.refaster.annotation.Matches; import com.google.errorprone.refaster.annotation.MayOptionallyUse; +import com.google.errorprone.refaster.annotation.NotMatches; import com.google.errorprone.refaster.annotation.Placeholder; import com.google.errorprone.refaster.annotation.UseImportPolicy; import java.util.Comparator; @@ -19,7 +19,7 @@ import java.util.stream.Stream; import org.jspecify.annotations.Nullable; import tech.picnic.errorprone.refaster.annotation.OnlineDocumentation; -import tech.picnic.errorprone.refaster.matchers.IsMethodInvocationWithTwoOrMoreArgs; +import tech.picnic.errorprone.refaster.matchers.IsLikelyTrivialComputation; /** Refaster rules related to expressions dealing with {@link Optional}s. */ @OnlineDocumentation @@ -119,7 +119,7 @@ Optional after(Iterator it) { /** Prefer {@link Optional#filter(Predicate)} over usage of the ternary operator. */ // XXX: This rule may introduce a compilation error: the `test` expression may reference a // non-effectively final variable, which is not allowed in the replacement lambda expression. - // Maybe our `Refaster` checker should test `compilesWithFix`? + // Review whether a `@Matcher` can be used to avoid this. abstract static class TernaryOperatorOptionalPositiveFiltering { @Placeholder abstract boolean test(T value); @@ -139,7 +139,7 @@ Optional after(T input) { /** Prefer {@link Optional#filter(Predicate)} over usage of the ternary operator. */ // XXX: This rule may introduce a compilation error: the `test` expression may reference a // non-effectively final variable, which is not allowed in the replacement lambda expression. - // Maybe our `Refaster` checker should test `compilesWithFix`? + // Review whether a `@Matcher` can be used to avoid this. abstract static class TernaryOperatorOptionalNegativeFiltering { @Placeholder abstract boolean test(T value); @@ -162,9 +162,9 @@ Optional after(T input) { */ static final class MapOptionalToBoolean { @BeforeTemplate + @SuppressWarnings("OptionalOrElseGet" /* Rule is confused by `Refaster#anyOf` usage. */) boolean before(Optional optional, Function predicate) { - return Refaster.anyOf( - optional.map(predicate).orElse(false), optional.map(predicate).orElse(Boolean.FALSE)); + return optional.map(predicate).orElse(Refaster.anyOf(false, Boolean.FALSE)); } @AfterTemplate @@ -227,13 +227,18 @@ T after(Optional o1, Optional o2) { } /** - * Prefer {@link Optional#orElseGet(Supplier)} over {@link Optional#orElse(Object)} if the given - * value is a method invocation with two or more arguments. + * Prefer {@link Optional#orElseGet(Supplier)} over {@link Optional#orElse(Object)} if the + * fallback value is not the result of a trivial computation. */ - // XXX: Extend rule to all method invocations (with less than 2 arguments). + // XXX: This rule may introduce a compilation error: the `value` expression may reference a + // non-effectively final variable, which is not allowed in the replacement lambda expression. + // Review whether a `@Matcher` can be used to avoid this. + // XXX: Once `MethodReferenceUsage` is "production ready", replace + // `@NotMatches(IsLikelyTrivialComputation.class)` with `@Matches(RequiresComputation.class)` (and + // reimplement the matcher accordingly). static final class OptionalOrElseGet { @BeforeTemplate - T before(Optional optional, @Matches(IsMethodInvocationWithTwoOrMoreArgs.class) T value) { + T before(Optional optional, @NotMatches(IsLikelyTrivialComputation.class) T value) { return optional.orElse(value); } @@ -344,6 +349,9 @@ static final class OptionalOrOtherOptional { Optional before(Optional optional1, Optional optional2) { // XXX: Note that rewriting the first and third variant will change the code's behavior if // `optional2` has side-effects. + // XXX: Note that rewriting the first and third variant will introduce a compilation error if + // `optional2` is not effectively final. Review whether a `@Matcher` can be used to avoid + // this. return Refaster.anyOf( optional1.map(Optional::of).orElse(optional2), optional1.map(Optional::of).orElseGet(() -> optional2), diff --git a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/OptionalRulesTestInput.java b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/OptionalRulesTestInput.java index 7ed4c579244..979717fcfd6 100644 --- a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/OptionalRulesTestInput.java +++ b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/OptionalRulesTestInput.java @@ -124,8 +124,6 @@ ImmutableSet testOptionalOrElseGet() { return ImmutableSet.of( Optional.of("foo").orElse("bar"), Optional.of("baz").orElse(toString()), - Optional.of("qux").orElse(String.valueOf(true)), - Optional.of("quux").orElse(String.format("%s", "quuz")), - Optional.of("corge").orElse(String.format("%s", "grault", "garply"))); + Optional.of("qux").orElse(String.valueOf(true))); } } diff --git a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/OptionalRulesTestOutput.java b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/OptionalRulesTestOutput.java index bda11bf1c40..cafc70cfa30 100644 --- a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/OptionalRulesTestOutput.java +++ b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/OptionalRulesTestOutput.java @@ -117,8 +117,6 @@ ImmutableSet testOptionalOrElseGet() { return ImmutableSet.of( Optional.of("foo").orElse("bar"), Optional.of("baz").orElse(toString()), - Optional.of("qux").orElse(String.valueOf(true)), - Optional.of("quux").orElseGet(() -> String.format("%s", "quuz")), - Optional.of("corge").orElseGet(() -> String.format("%s", "grault", "garply"))); + Optional.of("qux").orElseGet(() -> String.valueOf(true))); } } diff --git a/refaster-support/src/main/java/tech/picnic/errorprone/refaster/matchers/IsLikelyTrivialComputation.java b/refaster-support/src/main/java/tech/picnic/errorprone/refaster/matchers/IsLikelyTrivialComputation.java new file mode 100644 index 00000000000..9cfa30b5110 --- /dev/null +++ b/refaster-support/src/main/java/tech/picnic/errorprone/refaster/matchers/IsLikelyTrivialComputation.java @@ -0,0 +1,92 @@ +package tech.picnic.errorprone.refaster.matchers; + +import com.google.errorprone.VisitorState; +import com.google.errorprone.matchers.Matcher; +import com.sun.source.tree.ArrayAccessTree; +import com.sun.source.tree.ExpressionTree; +import com.sun.source.tree.IdentifierTree; +import com.sun.source.tree.LambdaExpressionTree; +import com.sun.source.tree.LiteralTree; +import com.sun.source.tree.MemberReferenceTree; +import com.sun.source.tree.MemberSelectTree; +import com.sun.source.tree.MethodInvocationTree; +import com.sun.source.tree.ParenthesizedTree; +import com.sun.source.tree.TypeCastTree; +import com.sun.source.tree.UnaryTree; + +/** A matcher of expressions that likely require little to no computation. */ +public final class IsLikelyTrivialComputation implements Matcher { + private static final long serialVersionUID = 1L; + + /** Instantiates a new {@link IsLikelyTrivialComputation} instance. */ + public IsLikelyTrivialComputation() {} + + @Override + public boolean matches(ExpressionTree expressionTree, VisitorState state) { + if (expressionTree instanceof MethodInvocationTree) { + // XXX: Method invocations are generally *not* trivial computations, but we make an exception + // for nullary method invocations on the result of a trivial computation. This exception + // allows this `Matcher` to by the `OptionalOrElseGet` Refaster rule, such that it does not + // suggest the introduction of lambda expressions that are better expressed as method + // references. Once the `MethodReferenceUsage` bug checker is production-ready, this exception + // should be removed. (But at that point, instead defining a `RequiresComputation` matcher may + // be more appropriate.) + MethodInvocationTree methodInvocation = (MethodInvocationTree) expressionTree; + if (methodInvocation.getArguments().isEmpty() + && matches(methodInvocation.getMethodSelect())) { + return true; + } + } + + return matches(expressionTree); + } + + // XXX: Some `BinaryTree`s may represent what could be considered "trivial computations". + // Depending on feedback such trees may be matched in the future. + private static boolean matches(ExpressionTree expressionTree) { + if (expressionTree instanceof ArrayAccessTree) { + return matches(((ArrayAccessTree) expressionTree).getExpression()) + && matches(((ArrayAccessTree) expressionTree).getIndex()); + } + + if (expressionTree instanceof LiteralTree) { + return true; + } + + if (expressionTree instanceof LambdaExpressionTree) { + /* + * Lambda expressions encapsulate computations, but their definition does not involve + * significant computation. + */ + return true; + } + + if (expressionTree instanceof IdentifierTree) { + return true; + } + + if (expressionTree instanceof MemberReferenceTree) { + return matches(((MemberReferenceTree) expressionTree).getQualifierExpression()); + } + + if (expressionTree instanceof MemberSelectTree) { + return matches(((MemberSelectTree) expressionTree).getExpression()); + } + + if (expressionTree instanceof ParenthesizedTree) { + return matches(((ParenthesizedTree) expressionTree).getExpression()); + } + + if (expressionTree instanceof TypeCastTree) { + return matches(((TypeCastTree) expressionTree).getExpression()); + } + + if (expressionTree instanceof UnaryTree) { + // XXX: Arguably side-effectful options such as pre- and post-increment and -decrement are not + // trivial. + return matches(((UnaryTree) expressionTree).getExpression()); + } + + return false; + } +} diff --git a/refaster-support/src/main/java/tech/picnic/errorprone/refaster/matchers/IsMethodInvocationWithTwoOrMoreArgs.java b/refaster-support/src/main/java/tech/picnic/errorprone/refaster/matchers/IsMethodInvocationWithTwoOrMoreArgs.java deleted file mode 100644 index 023517858f1..00000000000 --- a/refaster-support/src/main/java/tech/picnic/errorprone/refaster/matchers/IsMethodInvocationWithTwoOrMoreArgs.java +++ /dev/null @@ -1,20 +0,0 @@ -package tech.picnic.errorprone.refaster.matchers; - -import com.google.errorprone.VisitorState; -import com.google.errorprone.matchers.Matcher; -import com.sun.source.tree.ExpressionTree; -import com.sun.source.tree.MethodInvocationTree; - -/** A matcher for method invocations with two or more arguments. */ -public final class IsMethodInvocationWithTwoOrMoreArgs implements Matcher { - private static final long serialVersionUID = 1L; - - /** Instantiates a new {@link IsMethodInvocationWithTwoOrMoreArgs} instance. */ - public IsMethodInvocationWithTwoOrMoreArgs() {} - - @Override - public boolean matches(ExpressionTree expressionTree, VisitorState state) { - return expressionTree instanceof MethodInvocationTree - && ((MethodInvocationTree) expressionTree).getArguments().size() > 1; - } -} diff --git a/refaster-support/src/test/java/tech/picnic/errorprone/refaster/matchers/IsLikelyTrivialComputationTest.java b/refaster-support/src/test/java/tech/picnic/errorprone/refaster/matchers/IsLikelyTrivialComputationTest.java new file mode 100644 index 00000000000..a140e7248fc --- /dev/null +++ b/refaster-support/src/test/java/tech/picnic/errorprone/refaster/matchers/IsLikelyTrivialComputationTest.java @@ -0,0 +1,147 @@ +package tech.picnic.errorprone.refaster.matchers; + +import static com.google.errorprone.BugPattern.SeverityLevel.ERROR; + +import com.google.errorprone.BugPattern; +import com.google.errorprone.CompilationTestHelper; +import com.google.errorprone.bugpatterns.BugChecker; +import com.sun.source.tree.ReturnTree; +import org.junit.jupiter.api.Test; + +final class IsLikelyTrivialComputationTest { + @Test + void matches() { + CompilationTestHelper.newInstance(MatcherTestChecker.class, getClass()) + .addSourceLines( + "A.java", + "import java.util.function.Predicate;", + "", + "class A {", + " String negative1() {", + " return String.valueOf(1);", + " }", + "", + " String negative2() {", + " return toString().toString();", + " }", + "", + " String negative3() {", + " return \"foo\" + toString();", + " }", + "", + " byte negative4() {", + " return \"foo\".getBytes()[0];", + " }", + "", + " int negative5() {", + " int[] arr = new int[0];", + " return arr[hashCode()];", + " }", + "", + " int negative6() {", + " return 1 * 2;", + " }", + "", + " Predicate negative7() {", + " return toString()::equals;", + " }", + "", + " String negative8() {", + " return (toString());", + " }", + "", + " Object negative9() {", + " return (Object) toString();", + " }", + "", + " int negative10() {", + " return -hashCode();", + " }", + "", + " String positive1() {", + " // BUG: Diagnostic contains:", + " return toString();", + " }", + "", + " String positive2() {", + " // BUG: Diagnostic contains:", + " return this.toString();", + " }", + "", + " int positive3() {", + " int[] arr = new int[0];", + " // BUG: Diagnostic contains:", + " return arr[0];", + " }", + "", + " String positive4() {", + " // BUG: Diagnostic contains:", + " return null;", + " }", + "", + " boolean positive5() {", + " // BUG: Diagnostic contains:", + " return false;", + " }", + "", + " int positive6() {", + " // BUG: Diagnostic contains:", + " return 0;", + " }", + "", + " String positive7() {", + " // BUG: Diagnostic contains:", + " return \"foo\" + \"bar\";", + " }", + "", + " Predicate positive8() {", + " // BUG: Diagnostic contains:", + " return v -> \"foo\".equals(v);", + " }", + "", + " A positive9() {", + " // BUG: Diagnostic contains:", + " return this;", + " }", + "", + " Predicate positive10() {", + " // BUG: Diagnostic contains:", + " return \"foo\"::equals;", + " }", + "", + " A positive11() {", + " // BUG: Diagnostic contains:", + " return (this);", + " }", + "", + " Object positive12() {", + " // BUG: Diagnostic contains:", + " return (Object) this;", + " }", + "", + " boolean positive13() {", + " // BUG: Diagnostic contains:", + " return !false;", + " }", + "}") + .doTest(); + } + + /** A {@link BugChecker} that simply delegates to {@link IsLikelyTrivialComputation}. */ + @BugPattern( + summary = "Flags return statement expressions matched by `IsLikelyTrivialComputation`", + severity = ERROR) + public static final class MatcherTestChecker extends AbstractMatcherTestChecker { + private static final long serialVersionUID = 1L; + + // XXX: This is a false positive reported by Checkstyle. See + // https://github.com/checkstyle/checkstyle/issues/10161#issuecomment-1242732120. + @SuppressWarnings("RedundantModifier") + public MatcherTestChecker() { + super( + (expressionTree, state) -> + state.getPath().getParentPath().getLeaf() instanceof ReturnTree + && new IsLikelyTrivialComputation().matches(expressionTree, state)); + } + } +} diff --git a/refaster-support/src/test/java/tech/picnic/errorprone/refaster/matchers/IsMethodInvocationWithTwoOrMoreArgsTest.java b/refaster-support/src/test/java/tech/picnic/errorprone/refaster/matchers/IsMethodInvocationWithTwoOrMoreArgsTest.java deleted file mode 100644 index 33de7ecca06..00000000000 --- a/refaster-support/src/test/java/tech/picnic/errorprone/refaster/matchers/IsMethodInvocationWithTwoOrMoreArgsTest.java +++ /dev/null @@ -1,69 +0,0 @@ -package tech.picnic.errorprone.refaster.matchers; - -import static com.google.errorprone.BugPattern.SeverityLevel.ERROR; - -import com.google.errorprone.BugPattern; -import com.google.errorprone.CompilationTestHelper; -import com.google.errorprone.bugpatterns.BugChecker; -import org.junit.jupiter.api.Test; - -final class IsMethodInvocationWithTwoOrMoreArgsTest { - @Test - void matches() { - CompilationTestHelper.newInstance(MatcherTestChecker.class, getClass()) - .addSourceLines( - "A.java", - "class A {", - " Boolean negative1() {", - " return Boolean.TRUE;", - " }", - "", - " String negative2() {", - " return \"foo\";", - " }", - "", - " String negative3() {", - " return toString();", - " }", - "", - " String negative4() {", - " return String.valueOf(1);", - " }", - "", - " String positive1() {", - " // BUG: Diagnostic contains:", - " return m1(\"foo\", \"bar\");", - " }", - "", - " String positive2() {", - " // BUG: Diagnostic contains:", - " return String.format(\"%s\", \"foo\");", - " }", - "", - " String positive3() {", - " // BUG: Diagnostic contains:", - " return String.format(\"%s-%s\", \"foo\", \"bar\");", - " }", - "", - " private static String m1(String foo, String bar) {", - " return foo + bar;", - " }", - "}") - .doTest(); - } - - /** A {@link BugChecker} that simply delegates to {@link IsMethodInvocationWithTwoOrMoreArgs}. */ - @BugPattern( - summary = "Flags expressions matched by `IsMethodInvocationWithTwoOrMoreArgs`", - severity = ERROR) - public static final class MatcherTestChecker extends AbstractMatcherTestChecker { - private static final long serialVersionUID = 1L; - - // XXX: This is a false positive reported by Checkstyle. See - // https://github.com/checkstyle/checkstyle/issues/10161#issuecomment-1242732120. - @SuppressWarnings("RedundantModifier") - public MatcherTestChecker() { - super(new IsMethodInvocationWithTwoOrMoreArgs()); - } - } -}