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".
PicnicSupermarket · Sep 3, 2024 · c679a3f · c679a3f
1 parent de54b4b
commit c679a3f
Show file tree

Hide file tree

Showing 7 changed files with 112 additions and 125 deletions.
diff --git a/...rrorprone/bugpatterns/OptionalOrElse.java → ...rprone/bugpatterns/OptionalOrElseGet.java b/...rrorprone/bugpatterns/OptionalOrElse.java → ...rprone/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<ExpressionTree> REQUIRES_COMPUTATION = new RequiresComputation();
   private static final Matcher<ExpressionTree> OPTIONAL_OR_ELSE_METHOD =
       instanceMethod().onExactClass(Optional.class.getCanonicalName()).namedAnyOf("orElse");
   // XXX: Also exclude invocations of `@Placeholder`-annotated methods.
   private static final Matcher<ExpressionTree> 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<String> tryMethodReferenceConversion(
       ExpressionTree tree, VisitorState state) {
@@ -118,7 +106,7 @@ private static Optional<String> 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
@@ -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<T> o1, Optional<T> 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<T> {
+  // 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<T> {
     @BeforeTemplate
-    T before(Optional<T> optional, @NotMatches(IsLikelyTrivialComputation.class) T value) {
-      return optional.orElse(value);
+    T before(Optional<T> optional, @NotMatches(RequiresComputation.class) T value) {
+      return optional.orElseGet(() -> value);
     }
 
     @AfterTemplate
     T after(Optional<T> optional, T value) {
-      return optional.orElseGet(() -> value);
+      return optional.orElse(value);
     }
   }
 
@@ -373,7 +370,12 @@ Optional<R> after(Optional<T> optional, Function<? super S, Optional<? extends R
   /** Prefer {@link Optional#or(Supplier)} over more verbose alternatives. */
   static final class OptionalOrOtherOptional<T> {
     @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<T> before(Optional<T> optional1, Optional<T> optional2) {
       // XXX: Note that rewriting the first and third variant will change the code's behavior if
       // `optional2` has side-effects.

diff --git a/...prone/bugpatterns/OptionalOrElseTest.java → ...ne/bugpatterns/OptionalOrElseGetTest.java b/...prone/bugpatterns/OptionalOrElseTest.java → ...ne/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<Object> optional = Optional.empty();",
@@ -27,6 +28,7 @@ void identification() {
             "    optional.orElse(string);",
             "    optional.orElse(this.string);",
             "    optional.orElse(Refaster.anyOf(\"constant\", \"another\"));",
+            "    Optional.<Supplier<String>>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/...ntrib/src/test/resources/tech/picnic/errorprone/refasterrules/OptionalRulesTestInput.java b/...ntrib/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<String> testOptionalOrElseGet() {
+  ImmutableSet<String> 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<Object> testStreamFlatMapOptional() {

diff --git a/...trib/src/test/resources/tech/picnic/errorprone/refasterrules/OptionalRulesTestOutput.java b/...trib/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<String> testOptionalOrElseGet() {
+  ImmutableSet<String> 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<Object> testStreamFlatMapOptional() {

diff --git a/.../matchers/IsLikelyTrivialComputation.java → ...efaster/matchers/RequiresComputation.java b/.../matchers/IsLikelyTrivialComputation.java → ...efaster/matchers/RequiresComputation.java
@@ -2,65 +2,54 @@
 
 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;
 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<ExpressionTree> {
+/** A matcher of expressions that may a non-trivial amount of computation. */
+public final class RequiresComputation implements Matcher<ExpressionTree> {
   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);
   }
 
   // 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 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) {
       /*
        * 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;
   }
 }