From c679a3fc0cb29b93b34ee8f93eb01b711dab5ddd Mon Sep 17 00:00:00 2001 From: Stephan Schroevers Date: Tue, 3 Sep 2024 16:00:33 +0200 Subject: [PATCH] Improve `Optional#orElse{,Get}` support (#1283) Summary of changes: - Consolidate the `OptionalOrElseGet` Refaster rule and the `OptionalOrElse` bug checker into the latter, keeping the best of both. - Rename the `OptionalOrElse` bug checker to `OptionalOrElseGet` to avoid confusion. - Replace the `IsLikelyTrivialComputation` matcher with the inverse `RequiresComputation` variant. - Introduce an `OptionalOrElse` Refaster rule that simplifies expressions in the "opposite direction". --- ...onalOrElse.java => OptionalOrElseGet.java} | 40 +++---- .../refasterrules/OptionalRules.java | 30 ++--- ...seTest.java => OptionalOrElseGetTest.java} | 8 +- .../refasterrules/OptionalRulesTestInput.java | 6 +- .../OptionalRulesTestOutput.java | 6 +- ...putation.java => RequiresComputation.java} | 41 +++---- ...Test.java => RequiresComputationTest.java} | 106 ++++++++++-------- 7 files changed, 112 insertions(+), 125 deletions(-) rename error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/{OptionalOrElse.java => OptionalOrElseGet.java} (76%) rename error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/{OptionalOrElseTest.java => OptionalOrElseGetTest.java} (94%) rename refaster-support/src/main/java/tech/picnic/errorprone/refaster/matchers/{IsLikelyTrivialComputation.java => RequiresComputation.java} (61%) rename refaster-support/src/test/java/tech/picnic/errorprone/refaster/matchers/{IsLikelyTrivialComputationTest.java => RequiresComputationTest.java} (72%) diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/OptionalOrElse.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/OptionalOrElseGet.java similarity index 76% rename from error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/OptionalOrElse.java rename to error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/OptionalOrElseGet.java index 6e1a83f413..692ee60e6e 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/OptionalOrElse.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/OptionalOrElseGet.java @@ -18,14 +18,12 @@ import com.google.errorprone.matchers.Description; import com.google.errorprone.matchers.Matcher; import com.google.errorprone.refaster.Refaster; -import com.google.errorprone.util.ASTHelpers; import com.sun.source.tree.ExpressionTree; -import com.sun.source.tree.IdentifierTree; -import com.sun.source.tree.LiteralTree; import com.sun.source.tree.MemberSelectTree; import com.sun.source.tree.MethodInvocationTree; import java.util.Optional; import java.util.function.Supplier; +import tech.picnic.errorprone.refaster.matchers.RequiresComputation; import tech.picnic.errorprone.utils.SourceCode; /** @@ -36,12 +34,12 @@ * it does, the suggested fix changes the program's semantics. Such fragile code must instead be * refactored such that the side-effectful code does not appear accidental. */ -// XXX: Consider also implementing the inverse, in which `.orElseGet(() -> someConstant)` is -// flagged. -// XXX: Once the `MethodReferenceUsageCheck` becomes generally usable, consider leaving the method -// reference cleanup to that check, and express the remainder of the logic in this class using a -// Refaster template, i.c.w. a `@Matches` constraint that implements the `requiresComputation` -// logic. +// 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 the `MethodReferenceUsageCheck` bug checker becomes generally usable, consider leaving +// the method reference cleanup to that check, and express the remainder of the logic in this class +// using a Refaster template, i.c.w. a `@NotMatches(RequiresComputation.class)` constraint. @AutoService(BugChecker.class) @BugPattern( summary = @@ -51,16 +49,17 @@ linkType = NONE, severity = WARNING, tags = PERFORMANCE) -public final class OptionalOrElse extends BugChecker implements MethodInvocationTreeMatcher { +public final class OptionalOrElseGet extends BugChecker implements MethodInvocationTreeMatcher { private static final long serialVersionUID = 1L; + private static final Matcher REQUIRES_COMPUTATION = new RequiresComputation(); private static final Matcher OPTIONAL_OR_ELSE_METHOD = instanceMethod().onExactClass(Optional.class.getCanonicalName()).namedAnyOf("orElse"); // XXX: Also exclude invocations of `@Placeholder`-annotated methods. private static final Matcher REFASTER_METHOD = staticMethod().onClass(Refaster.class.getCanonicalName()); - /** Instantiates a new {@link OptionalOrElse} instance. */ - public OptionalOrElse() {} + /** Instantiates a new {@link OptionalOrElseGet} instance. */ + public OptionalOrElseGet() {} @Override public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) { @@ -69,7 +68,8 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState } ExpressionTree argument = Iterables.getOnlyElement(tree.getArguments()); - if (!requiresComputation(argument) || REFASTER_METHOD.matches(argument, state)) { + if (!REQUIRES_COMPUTATION.matches(argument, state) + || REFASTER_METHOD.matches(argument, state)) { return Description.NO_MATCH; } @@ -91,18 +91,6 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState return describeMatch(tree, fix); } - /** - * Tells whether the given expression contains anything other than a literal or a (possibly - * dereferenced) variable or constant. - */ - private static boolean requiresComputation(ExpressionTree tree) { - return !(tree instanceof IdentifierTree - || tree instanceof LiteralTree - || (tree instanceof MemberSelectTree memberSelect - && !requiresComputation(memberSelect.getExpression())) - || ASTHelpers.constValue(tree) != null); - } - /** Returns the nullary method reference matching the given expression, if any. */ private static Optional tryMethodReferenceConversion( ExpressionTree tree, VisitorState state) { @@ -118,7 +106,7 @@ private static Optional tryMethodReferenceConversion( return Optional.empty(); } - if (requiresComputation(memberSelect.getExpression())) { + if (REQUIRES_COMPUTATION.matches(memberSelect.getExpression(), state)) { return Optional.empty(); } 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 9f73ae38e6..e053959b5a 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 @@ -20,7 +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; +import tech.picnic.errorprone.refaster.matchers.RequiresComputation; /** Refaster rules related to expressions dealing with {@link Optional}s. */ @OnlineDocumentation @@ -255,24 +255,21 @@ 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. + * Prefer {@link Optional#orElse(Object)} over {@link Optional#orElseGet(Supplier)} if the + * fallback value does not require non-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 { + // XXX: This rule is the counterpart to the `OptionalOrElseGet` bug checker. Once the + // `MethodReferenceUsage` bug checker is "production ready", that bug checker may similarly be + // replaced with a Refaster rule. + static final class OptionalOrElse { @BeforeTemplate - T before(Optional optional, @NotMatches(IsLikelyTrivialComputation.class) T value) { - return optional.orElse(value); + T before(Optional optional, @NotMatches(RequiresComputation.class) T value) { + return optional.orElseGet(() -> value); } @AfterTemplate T after(Optional optional, T value) { - return optional.orElseGet(() -> value); + return optional.orElse(value); } } @@ -373,7 +370,12 @@ Optional after(Optional optional, Function { @BeforeTemplate - @SuppressWarnings("NestedOptionals") + @SuppressWarnings({ + "LexicographicalAnnotationAttributeListing" /* `key-*` entry must remain last. */, + "NestedOptionals" /* This violation will be rewritten. */, + "OptionalOrElse" /* Parameters represent expressions that may require computation. */, + "key-to-resolve-AnnotationUseStyle-and-TrailingComment-check-conflict" + }) 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. diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/OptionalOrElseTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/OptionalOrElseGetTest.java similarity index 94% rename from error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/OptionalOrElseTest.java rename to error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/OptionalOrElseGetTest.java index 620a532ae6..86372b1552 100644 --- a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/OptionalOrElseTest.java +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/OptionalOrElseGetTest.java @@ -5,14 +5,15 @@ import com.google.errorprone.CompilationTestHelper; import org.junit.jupiter.api.Test; -final class OptionalOrElseTest { +final class OptionalOrElseGetTest { @Test void identification() { - CompilationTestHelper.newInstance(OptionalOrElse.class, getClass()) + CompilationTestHelper.newInstance(OptionalOrElseGet.class, getClass()) .addSourceLines( "A.java", "import com.google.errorprone.refaster.Refaster;", "import java.util.Optional;", + "import java.util.function.Supplier;", "", "class A {", " private final Optional optional = Optional.empty();", @@ -27,6 +28,7 @@ void identification() { " optional.orElse(string);", " optional.orElse(this.string);", " optional.orElse(Refaster.anyOf(\"constant\", \"another\"));", + " Optional.>empty().orElse(() -> \"constant\");", "", " // BUG: Diagnostic contains:", " Optional.empty().orElse(string + \"constant\");", @@ -67,7 +69,7 @@ void identification() { @Test void replacement() { - BugCheckerRefactoringTestHelper.newInstance(OptionalOrElse.class, getClass()) + BugCheckerRefactoringTestHelper.newInstance(OptionalOrElseGet.class, getClass()) .addInputLines( "A.java", "import java.util.Optional;", 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 73f0a9be9c..dc6ded46b7 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 @@ -83,11 +83,9 @@ String testOrOrElseThrow() { return Optional.of("foo").orElseGet(() -> Optional.of("bar").orElseThrow()); } - ImmutableSet testOptionalOrElseGet() { + ImmutableSet testOptionalOrElse() { return ImmutableSet.of( - Optional.of("foo").orElse("bar"), - Optional.of("baz").orElse(toString()), - Optional.of("qux").orElse(String.valueOf(true))); + Optional.of("foo").orElseGet(() -> "bar"), Optional.of("baz").orElseGet(() -> toString())); } ImmutableSet testStreamFlatMapOptional() { 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 bc8f38c0cd..eb3af24377 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 @@ -80,11 +80,9 @@ String testOrOrElseThrow() { return Optional.of("foo").or(() -> Optional.of("bar")).orElseThrow(); } - ImmutableSet testOptionalOrElseGet() { + ImmutableSet testOptionalOrElse() { return ImmutableSet.of( - Optional.of("foo").orElse("bar"), - Optional.of("baz").orElse(toString()), - Optional.of("qux").orElseGet(() -> String.valueOf(true))); + Optional.of("foo").orElse("bar"), Optional.of("baz").orElseGet(() -> toString())); } ImmutableSet testStreamFlatMapOptional() { 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/RequiresComputation.java similarity index 61% rename from refaster-support/src/main/java/tech/picnic/errorprone/refaster/matchers/IsLikelyTrivialComputation.java rename to refaster-support/src/main/java/tech/picnic/errorprone/refaster/matchers/RequiresComputation.java index dbf56e80e6..f4a7e0777c 100644 --- a/refaster-support/src/main/java/tech/picnic/errorprone/refaster/matchers/IsLikelyTrivialComputation.java +++ b/refaster-support/src/main/java/tech/picnic/errorprone/refaster/matchers/RequiresComputation.java @@ -2,6 +2,7 @@ import com.google.errorprone.VisitorState; import com.google.errorprone.matchers.Matcher; +import com.google.errorprone.util.ASTHelpers; import com.sun.source.tree.ArrayAccessTree; import com.sun.source.tree.ExpressionTree; import com.sun.source.tree.IdentifierTree; @@ -9,34 +10,19 @@ 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 { +/** A matcher of expressions that may a non-trivial amount of computation. */ +public final class RequiresComputation implements Matcher { private static final long serialVersionUID = 1L; - /** Instantiates a new {@link IsLikelyTrivialComputation} instance. */ - public IsLikelyTrivialComputation() {} + /** Instantiates a new {@link RequiresComputation} instance. */ + public RequiresComputation() {} @Override public boolean matches(ExpressionTree expressionTree, VisitorState state) { - if (expressionTree instanceof MethodInvocationTree methodInvocation) { - // 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.) - if (methodInvocation.getArguments().isEmpty() - && matches(methodInvocation.getMethodSelect())) { - return true; - } - } - return matches(expressionTree); } @@ -44,11 +30,11 @@ && matches(methodInvocation.getMethodSelect())) { // Depending on feedback such trees may be matched in the future. private static boolean matches(ExpressionTree expressionTree) { if (expressionTree instanceof ArrayAccessTree arrayAccess) { - return matches(arrayAccess.getExpression()) && matches(arrayAccess.getIndex()); + return matches(arrayAccess.getExpression()) || matches(arrayAccess.getIndex()); } if (expressionTree instanceof LiteralTree) { - return true; + return false; } if (expressionTree instanceof LambdaExpressionTree) { @@ -56,11 +42,14 @@ private static boolean matches(ExpressionTree expressionTree) { * Lambda expressions encapsulate computations, but their definition does not involve * significant computation. */ - return true; + return false; } if (expressionTree instanceof IdentifierTree) { - return true; + // XXX: Generally identifiers don't by themselves represent a computation, though they may be + // a stand-in for one if they are a Refaster template method argument. Can we identify such + // cases, also when the `Matcher` is invoked by Refaster? + return false; } if (expressionTree instanceof MemberReferenceTree memberReference) { @@ -80,11 +69,11 @@ private static boolean matches(ExpressionTree expressionTree) { } if (expressionTree instanceof UnaryTree unary) { - // XXX: Arguably side-effectful options such as pre- and post-increment and -decrement are not - // trivial. + // XXX: Arguably side-effectful options such as pre- and post-increment and -decrement + // represent non-trivial computations. return matches(unary.getExpression()); } - return false; + return ASTHelpers.constValue(expressionTree) == null; } } 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/RequiresComputationTest.java similarity index 72% rename from refaster-support/src/test/java/tech/picnic/errorprone/refaster/matchers/IsLikelyTrivialComputationTest.java rename to refaster-support/src/test/java/tech/picnic/errorprone/refaster/matchers/RequiresComputationTest.java index a140e7248f..ef212bdc9c 100644 --- a/refaster-support/src/test/java/tech/picnic/errorprone/refaster/matchers/IsLikelyTrivialComputationTest.java +++ b/refaster-support/src/test/java/tech/picnic/errorprone/refaster/matchers/RequiresComputationTest.java @@ -8,54 +8,69 @@ import com.sun.source.tree.ReturnTree; import org.junit.jupiter.api.Test; -final class IsLikelyTrivialComputationTest { +final class RequiresComputationTest { @Test void matches() { CompilationTestHelper.newInstance(MatcherTestChecker.class, getClass()) .addSourceLines( "A.java", + "import java.io.OutputStream;", + "import java.util.Comparator;", "import java.util.function.Predicate;", "", "class A {", - " String negative1() {", - " return String.valueOf(1);", + " int negative1() {", + " int[] arr = new int[0];", + " return arr[0];", " }", "", " String negative2() {", - " return toString().toString();", + " return null;", " }", "", - " String negative3() {", - " return \"foo\" + toString();", + " boolean negative3() {", + " return false;", " }", "", - " byte negative4() {", - " return \"foo\".getBytes()[0];", + " int negative4() {", + " return 0;", " }", "", - " int negative5() {", - " int[] arr = new int[0];", - " return arr[hashCode()];", + " String negative5() {", + " return \"foo\" + \"bar\";", + " }", + "", + " Predicate negative6() {", + " return v -> \"foo\".equals(v);", " }", "", - " int negative6() {", - " return 1 * 2;", + " A negative7() {", + " return this;", " }", "", - " Predicate negative7() {", - " return toString()::equals;", + " Predicate negative8() {", + " return \"foo\"::equals;", " }", "", - " String negative8() {", - " return (toString());", + " OutputStream negative9() {", + " return System.out;", " }", "", - " Object negative9() {", - " return (Object) toString();", + " A negative10() {", + " return (this);", " }", "", - " int negative10() {", - " return -hashCode();", + " Object negative11() {", + " return (Object) this;", + " }", + "", + " boolean negative12() {", + " boolean[] arr = new boolean[0];", + " return !arr[0];", + " }", + "", + " String negative13() {", + " return \"foo\" + 0;", " }", "", " String positive1() {", @@ -68,68 +83,63 @@ void matches() { " return this.toString();", " }", "", - " int positive3() {", - " int[] arr = new int[0];", + " String positive3() {", " // BUG: Diagnostic contains:", - " return arr[0];", + " return String.valueOf(1);", " }", "", " String positive4() {", " // BUG: Diagnostic contains:", - " return null;", + " return toString().toString();", " }", "", - " boolean positive5() {", + " String positive5() {", " // BUG: Diagnostic contains:", - " return false;", + " return \"foo\" + toString();", " }", "", - " int positive6() {", + " byte positive6() {", " // BUG: Diagnostic contains:", - " return 0;", + " return \"foo\".getBytes()[0];", " }", "", - " String positive7() {", + " int positive7() {", + " int[] arr = new int[0];", " // BUG: Diagnostic contains:", - " return \"foo\" + \"bar\";", + " return arr[hashCode()];", " }", "", " Predicate positive8() {", " // BUG: Diagnostic contains:", - " return v -> \"foo\".equals(v);", - " }", - "", - " A positive9() {", - " // BUG: Diagnostic contains:", - " return this;", + " return toString()::equals;", " }", "", - " Predicate positive10() {", + " Comparator positive9() {", " // BUG: Diagnostic contains:", - " return \"foo\"::equals;", + " return toString().CASE_INSENSITIVE_ORDER;", " }", "", - " A positive11() {", + " String positive10() {", " // BUG: Diagnostic contains:", - " return (this);", + " return (toString());", " }", "", - " Object positive12() {", + " Object positive11() {", " // BUG: Diagnostic contains:", - " return (Object) this;", + " return (Object) toString();", " }", "", - " boolean positive13() {", + " int positive12() {", " // BUG: Diagnostic contains:", - " return !false;", + " return -hashCode();", " }", "}") .doTest(); } - /** A {@link BugChecker} that simply delegates to {@link IsLikelyTrivialComputation}. */ + /** A {@link BugChecker} that simply delegates to {@link RequiresComputation}. */ @BugPattern( - summary = "Flags return statement expressions matched by `IsLikelyTrivialComputation`", + summary = "Flags return statement expressions matched by `RequiresComputation`", severity = ERROR) public static final class MatcherTestChecker extends AbstractMatcherTestChecker { private static final long serialVersionUID = 1L; @@ -141,7 +151,7 @@ public MatcherTestChecker() { super( (expressionTree, state) -> state.getPath().getParentPath().getLeaf() instanceof ReturnTree - && new IsLikelyTrivialComputation().matches(expressionTree, state)); + && new RequiresComputation().matches(expressionTree, state)); } } }