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".
2026-03-10 08:11:25 +00:00 · 2024-09-03 16:00:33 +02:00
parent de54b4bf64
commit c679a3fc0c
7 changed files with 112 additions and 125 deletions
--- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/OptionalOrElseGet.java
+++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/OptionalOrElseGet.java
@@ -18,14 +18,12 @@ import com.google.errorprone.fixes.SuggestedFixes;
 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 @@ import tech.picnic.errorprone.utils.SourceCode;
 * 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 @@ import tech.picnic.errorprone.utils.SourceCode;
    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 final class OptionalOrElse extends BugChecker implements MethodInvocation
    }

    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 final class OptionalOrElse extends BugChecker implements MethodInvocation
    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 @@ public final class OptionalOrElse extends BugChecker implements MethodInvocation
      return Optional.empty();
    }

-    if (requiresComputation(memberSelect.getExpression())) {
+    if (REQUIRES_COMPUTATION.matches(memberSelect.getExpression(), state)) {
      return Optional.empty();
    }

--- 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.function.Supplier;
 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 @@ final class OptionalRules {
  }

  /**
-   * 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 @@ final class OptionalRules {
  /** 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.
--- a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/OptionalOrElseGetTest.java
+++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/OptionalOrElseGetTest.java
@@ -5,14 +5,15 @@ import com.google.errorprone.BugCheckerRefactoringTestHelper.TestMode;
 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 @@ final class OptionalOrElseTest {
            "    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 @@ final class OptionalOrElseTest {

  @Test
  void replacement() {
-    BugCheckerRefactoringTestHelper.newInstance(OptionalOrElse.class, getClass())
+    BugCheckerRefactoringTestHelper.newInstance(OptionalOrElseGet.class, getClass())
        .addInputLines(
            "A.java",
            "import java.util.Optional;",
--- 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 @@ final class OptionalRulesTest implements RefasterRuleCollectionTestCase {
    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() {
--- 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 @@ final class OptionalRulesTest implements RefasterRuleCollectionTestCase {
    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() {
--- 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
@@ -2,6 +2,7 @@ package tech.picnic.errorprone.refaster.matchers;

 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.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);
  }

@@ -44,11 +30,11 @@ public final class IsLikelyTrivialComputation implements Matcher<ExpressionTree>
  // 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 @@ public final class IsLikelyTrivialComputation implements Matcher<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 @@ public final class IsLikelyTrivialComputation implements Matcher<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;
  }
 }
--- 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
@@ -8,54 +8,69 @@ import com.google.errorprone.bugpatterns.BugChecker;
 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\";",
            "  }",
            "",
-            "  int negative6() {",
-            "    return 1 * 2;",
+            "  Predicate<String> negative6() {",
+            "    return v -> \"foo\".equals(v);",
            "  }",
            "",
-            "  Predicate<String> negative7() {",
-            "    return toString()::equals;",
+            "  A negative7() {",
+            "    return this;",
            "  }",
            "",
-            "  String negative8() {",
-            "    return (toString());",
+            "  Predicate<String> negative8() {",
+            "    return \"foo\"::equals;",
            "  }",
            "",
-            "  Object negative9() {",
-            "    return (Object) toString();",
+            "  OutputStream negative9() {",
+            "    return System.out;",
            "  }",
            "",
-            "  int negative10() {",
-            "    return -hashCode();",
+            "  A negative10() {",
+            "    return (this);",
+            "  }",
+            "",
+            "  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 @@ final class IsLikelyTrivialComputationTest {
            "    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<String> positive8() {",
            "    // BUG: Diagnostic contains:",
-            "    return v -> \"foo\".equals(v);",
+            "    return toString()::equals;",
            "  }",
            "",
-            "  A positive9() {",
+            "  Comparator<String> positive9() {",
            "    // BUG: Diagnostic contains:",
-            "    return this;",
+            "    return toString().CASE_INSENSITIVE_ORDER;",
            "  }",
            "",
-            "  Predicate<String> positive10() {",
+            "  String positive10() {",
            "    // BUG: Diagnostic contains:",
-            "    return \"foo\"::equals;",
+            "    return (toString());",
            "  }",
            "",
-            "  A positive11() {",
+            "  Object positive11() {",
            "    // BUG: Diagnostic contains:",
-            "    return (this);",
+            "    return (Object) toString();",
            "  }",
            "",
-            "  Object positive12() {",
+            "  int positive12() {",
            "    // BUG: Diagnostic contains:",
-            "    return (Object) this;",
-            "  }",
-            "",
-            "  boolean positive13() {",
-            "    // 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 @@ final class IsLikelyTrivialComputationTest {
      super(
          (expressionTree, state) ->
              state.getPath().getParentPath().getLeaf() instanceof ReturnTree
-                  && new IsLikelyTrivialComputation().matches(expressionTree, state));
+                  && new RequiresComputation().matches(expressionTree, state));
    }
  }
 }