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 86fbe12501..2a84eaf419 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 @@ -8,6 +8,7 @@ import com.google.errorprone.refaster.annotation.AfterTemplate; import com.google.errorprone.refaster.annotation.BeforeTemplate; 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,6 +20,7 @@ import java.util.stream.Stream; import org.jspecify.annotations.Nullable; import tech.picnic.errorprone.refaster.annotation.OnlineDocumentation; +import tech.picnic.errorprone.refaster.matchers.IsLikelyTrivialComputation; /** Refaster rules related to expressions dealing with {@link Optional}s. */ @OnlineDocumentation @@ -118,7 +120,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); @@ -138,7 +140,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); @@ -161,6 +163,7 @@ 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 optional.map(predicate).orElse(Refaster.anyOf(false, Boolean.FALSE)); } @@ -224,6 +227,28 @@ T after(Optional o1, Optional o2) { } } + /** + * Prefer {@link Optional#orElseGet(Supplier)} over {@link Optional#orElse(Object)} if the + * fallback value is not the result of a trivial computation. + */ + // 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, @NotMatches(IsLikelyTrivialComputation.class) T value) { + return optional.orElse(value); + } + + @AfterTemplate + T after(Optional optional, T value) { + return optional.orElseGet(() -> value); + } + } + /** * Flatten a stream of {@link Optional}s using {@link Optional#stream()}, rather than using one of * the more verbose alternatives. @@ -325,6 +350,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 16c4d856c7..979717fcfd 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 @@ -119,4 +119,11 @@ ImmutableSet> testOptionalFilter() { Optional testOptionalMap() { return Optional.of(1).stream().map(String::valueOf).findAny(); } + + ImmutableSet testOptionalOrElseGet() { + return ImmutableSet.of( + Optional.of("foo").orElse("bar"), + Optional.of("baz").orElse(toString()), + 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 a24fb5e8f9..cafc70cfa3 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 @@ -112,4 +112,11 @@ ImmutableSet> testOptionalFilter() { Optional testOptionalMap() { return Optional.of(1).map(String::valueOf); } + + ImmutableSet testOptionalOrElseGet() { + return ImmutableSet.of( + Optional.of("foo").orElse("bar"), + Optional.of("baz").orElse(toString()), + 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 0000000000..9cfa30b511 --- /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/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 0000000000..a140e7248f --- /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)); + } + } +}