Introduce OptionalOrElse check

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/OptionalOrElse.java

@@ -0,0 +1,135 @@
+package tech.picnic.errorprone.bugpatterns;
+
+import static com.google.errorprone.BugPattern.LinkType.NONE;
+import static com.google.errorprone.BugPattern.SeverityLevel.WARNING;
+import static com.google.errorprone.BugPattern.StandardTags.PERFORMANCE;
+import static com.google.errorprone.matchers.Matchers.instanceMethod;
+import static com.google.errorprone.matchers.Matchers.staticMethod;
+import static java.util.stream.Collectors.joining;
+
+import com.google.auto.service.AutoService;
+import com.google.common.collect.Iterables;
+import com.google.errorprone.BugPattern;
+import com.google.errorprone.VisitorState;
+import com.google.errorprone.bugpatterns.BugChecker;
+import com.google.errorprone.bugpatterns.BugChecker.MethodInvocationTreeMatcher;
+import com.google.errorprone.fixes.SuggestedFix;
+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.utils.SourceCode;
+
+/**
+ * A {@link BugChecker} that flags arguments to {@link Optional#orElse(Object)} that should be
+ * deferred using {@link Optional#orElseGet(Supplier)}.
+ *
+ * <p>The suggested fix assumes that the argument to {@code orElse} does not have side effects. If
+ * 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.
+@AutoService(BugChecker.class)
+@BugPattern(
+    summary =
+        """
+        Prefer `Optional#orElseGet` over `Optional#orElse` if the fallback requires additional \
+        computation""",
+    linkType = NONE,
+    severity = WARNING,
+    tags = PERFORMANCE)
+public final class OptionalOrElse extends BugChecker implements MethodInvocationTreeMatcher {
+  private static final long serialVersionUID = 1L;
+  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() {}
+
+  @Override
+  public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) {
+    if (!OPTIONAL_OR_ELSE_METHOD.matches(tree, state)) {
+      return Description.NO_MATCH;
+    }
+
+    ExpressionTree argument = Iterables.getOnlyElement(tree.getArguments());
+    if (!requiresComputation(argument) || REFASTER_METHOD.matches(argument, state)) {
+      return Description.NO_MATCH;
+    }
+
+    /*
+     * We have a match. Construct the method reference or lambda expression to be passed to the
+     * replacement `#orElseGet` invocation.
+     */
+    String newArgument =
+        tryMethodReferenceConversion(argument, state)
+            .orElseGet(() -> "() -> " + SourceCode.treeToString(argument, state));
+
+    /* Construct the suggested fix, replacing the method invocation and its argument. */
+    SuggestedFix fix =
+        SuggestedFix.builder()
+            .merge(SuggestedFixes.renameMethodInvocation(tree, "orElseGet", state))
+            .replace(argument, newArgument)
+            .build();
+
+    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) {
+    if (!(tree instanceof MethodInvocationTree methodInvocation)) {
+      return Optional.empty();
+    }
+
+    if (!methodInvocation.getArguments().isEmpty()) {
+      return Optional.empty();
+    }
+
+    if (!(methodInvocation.getMethodSelect() instanceof MemberSelectTree memberSelect)) {
+      return Optional.empty();
+    }
+
+    if (requiresComputation(memberSelect.getExpression())) {
+      return Optional.empty();
+    }
+
+    return Optional.of(
+        SourceCode.treeToString(memberSelect.getExpression(), state)
+            + "::"
+            + (methodInvocation.getTypeArguments().isEmpty()
+                ? ""
+                : methodInvocation.getTypeArguments().stream()
+                    .map(arg -> SourceCode.treeToString(arg, state))
+                    .collect(joining(",", "<", ">")))
+            + memberSelect.getIdentifier());
+  }
+}

error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/OptionalRules.java

@@ -360,7 +360,7 @@ 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" /* Auto-fix for the `NestedOptionals` check. */)
+    @SuppressWarnings("NestedOptionals")
     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.
@@ -386,9 +386,13 @@ Optional<T> after(Optional<T> optional1, Optional<T> optional2) {
    */
   static final class OptionalIdentity<T> {
     @BeforeTemplate
+    @SuppressWarnings("NestedOptionals")
     Optional<T> before(Optional<T> optional, Comparator<? super T> comparator) {
       return Refaster.anyOf(
           optional.or(Refaster.anyOf(() -> Optional.empty(), Optional::empty)),
+          optional
+              .map(Optional::of)
+              .orElseGet(Refaster.anyOf(() -> Optional.empty(), Optional::empty)),
           optional.stream().findFirst(),
           optional.stream().findAny(),
           optional.stream().min(comparator),
@@ -442,9 +446,7 @@ Optional<? extends T> after(Optional<S> optional, Function<? super S, ? extends
   static final class OptionalStream<T> {
     @BeforeTemplate
     Stream<T> before(Optional<T> optional) {
-      return Refaster.anyOf(
-          optional.map(Stream::of).orElse(Stream.empty()),
-          optional.map(Stream::of).orElseGet(Stream::empty));
+      return optional.map(Stream::of).orElseGet(Stream::empty);
     }
 
     @AfterTemplate

error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/OptionalOrElseTest.java

@@ -0,0 +1,135 @@
+package tech.picnic.errorprone.bugpatterns;
+
+import com.google.errorprone.BugCheckerRefactoringTestHelper;
+import com.google.errorprone.BugCheckerRefactoringTestHelper.TestMode;
+import com.google.errorprone.CompilationTestHelper;
+import org.junit.jupiter.api.Test;
+
+final class OptionalOrElseTest {
+  @Test
+  void identification() {
+    CompilationTestHelper.newInstance(OptionalOrElse.class, getClass())
+        .addSourceLines(
+            "A.java",
+            "import com.google.errorprone.refaster.Refaster;",
+            "import java.util.Optional;",
+            "",
+            "class A {",
+            "  private final Optional<Object> optional = Optional.empty();",
+            "  private final String string = optional.toString();",
+            "",
+            "  void m() {",
+            "    Optional.empty().orElse(null);",
+            "    optional.orElse(null);",
+            "    optional.orElse(\"constant\");",
+            "    optional.orElse(\"constant\" + 0);",
+            "    optional.orElse(Boolean.TRUE);",
+            "    optional.orElse(string);",
+            "    optional.orElse(this.string);",
+            "    optional.orElse(Refaster.anyOf(\"constant\", \"another\"));",
+            "",
+            "    // BUG: Diagnostic contains:",
+            "    Optional.empty().orElse(string + \"constant\");",
+            "    // BUG: Diagnostic contains:",
+            "    optional.orElse(string + \"constant\");",
+            "    // BUG: Diagnostic contains:",
+            "    optional.orElse(\"constant\".toString());",
+            "    // BUG: Diagnostic contains:",
+            "    optional.orElse(string.toString());",
+            "    // BUG: Diagnostic contains:",
+            "    optional.orElse(this.string.toString());",
+            "    // BUG: Diagnostic contains:",
+            "    optional.orElse(String.valueOf(42));",
+            "    // BUG: Diagnostic contains:",
+            "    optional.orElse(string.toString().length());",
+            "    // BUG: Diagnostic contains:",
+            "    optional.orElse(\"constant\".equals(string));",
+            "    // BUG: Diagnostic contains:",
+            "    optional.orElse(string.equals(string));",
+            "    // BUG: Diagnostic contains:",
+            "    optional.orElse(this.string.equals(string));",
+            "    // BUG: Diagnostic contains:",
+            "    optional.orElse(foo());",
+            "    // BUG: Diagnostic contains:",
+            "    optional.orElse(this.foo());",
+            "    // BUG: Diagnostic contains:",
+            "    optional.orElse(new Object() {});",
+            "    // BUG: Diagnostic contains:",
+            "    optional.orElse(new int[0].length);",
+            "  }",
+            "",
+            "  private <T> T foo() {",
+            "    return null;",
+            "  }",
+            "}")
+        .doTest();
+  }
+
+  @Test
+  void replacement() {
+    BugCheckerRefactoringTestHelper.newInstance(OptionalOrElse.class, getClass())
+        .addInputLines(
+            "A.java",
+            "import java.util.Optional;",
+            "",
+            "class A {",
+            "  private final Optional<Object> optional = Optional.empty();",
+            "  private final String string = optional.toString();",
+            "",
+            "  void m() {",
+            "    optional.orElse(string + \"constant\");",
+            "    optional.orElse(\"constant\".toString());",
+            "    optional.orElse(string.toString());",
+            "    optional.orElse(this.string.toString());",
+            "    optional.orElse(String.valueOf(42));",
+            "    optional.orElse(string.toString().length());",
+            "    optional.orElse(string.equals(string));",
+            "    optional.orElse(foo());",
+            "    optional.orElse(this.<Number>foo());",
+            "    optional.orElse(this.<String, Integer>bar());",
+            "    optional.orElse(new Object() {});",
+            "    optional.orElse(new int[0].length);",
+            "  }",
+            "",
+            "  private <T> T foo() {",
+            "    return null;",
+            "  }",
+            "",
+            "  private <S, T> T bar() {",
+            "    return null;",
+            "  }",
+            "}")
+        .addOutputLines(
+            "A.java",
+            "import java.util.Optional;",
+            "",
+            "class A {",
+            "  private final Optional<Object> optional = Optional.empty();",
+            "  private final String string = optional.toString();",
+            "",
+            "  void m() {",
+            "    optional.orElseGet(() -> string + \"constant\");",
+            "    optional.orElseGet(\"constant\"::toString);",
+            "    optional.orElseGet(string::toString);",
+            "    optional.orElseGet(this.string::toString);",
+            "    optional.orElseGet(() -> String.valueOf(42));",
+            "    optional.orElseGet(() -> string.toString().length());",
+            "    optional.orElseGet(() -> string.equals(string));",
+            "    optional.orElseGet(() -> foo());",
+            "    optional.orElseGet(this::<Number>foo);",
+            "    optional.orElseGet(this::<String, Integer>bar);",
+            "    optional.orElseGet(() -> new Object() {});",
+            "    optional.orElseGet(() -> new int[0].length);",
+            "  }",
+            "",
+            "  private <T> T foo() {",
+            "    return null;",
+            "  }",
+            "",
+            "  private <S, T> T bar() {",
+            "    return null;",
+            "  }",
+            "}")
+        .doTest(TestMode.TEXT_MATCH);
+  }
+}

error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/OptionalRulesTestInput.java

@@ -120,10 +120,12 @@ ImmutableSet<Optional<String>> testOptionalIdentity() {
     return ImmutableSet.of(
         Optional.of("foo").or(() -> Optional.empty()),
         Optional.of("bar").or(Optional::empty),
-        Optional.of("baz").stream().findFirst(),
-        Optional.of("qux").stream().findAny(),
-        Optional.of("quux").stream().min(String::compareTo),
-        Optional.of("quuz").stream().max(String::compareTo));
+        Optional.of("baz").map(Optional::of).orElseGet(() -> Optional.empty()),
+        Optional.of("qux").map(Optional::of).orElseGet(Optional::empty),
+        Optional.of("quux").stream().findFirst(),
+        Optional.of("quuz").stream().findAny(),
+        Optional.of("corge").stream().min(String::compareTo),
+        Optional.of("grault").stream().max(String::compareTo));
   }
 
   ImmutableSet<Optional<String>> testOptionalFilter() {
@@ -136,9 +138,7 @@ Optional<String> testOptionalMap() {
     return Optional.of(1).stream().map(String::valueOf).findAny();
   }
 
-  ImmutableSet<Stream<String>> testOptionalStream() {
-    return ImmutableSet.of(
-        Optional.of("foo").map(Stream::of).orElse(Stream.empty()),
-        Optional.of("bar").map(Stream::of).orElseGet(Stream::empty));
+  Stream<String> testOptionalStream() {
+    return Optional.of("foo").map(Stream::of).orElseGet(Stream::empty);
   }
 }

error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/OptionalRulesTestOutput.java

@@ -120,7 +120,9 @@ ImmutableSet<Optional<String>> testOptionalIdentity() {
         Optional.of("baz"),
         Optional.of("qux"),
         Optional.of("quux"),
-        Optional.of("quuz"));
+        Optional.of("quuz"),
+        Optional.of("corge"),
+        Optional.of("grault"));
   }
 
   ImmutableSet<Optional<String>> testOptionalFilter() {
@@ -132,7 +134,7 @@ Optional<String> testOptionalMap() {
     return Optional.of(1).map(String::valueOf);
   }
 
-  ImmutableSet<Stream<String>> testOptionalStream() {
-    return ImmutableSet.of(Optional.of("foo").stream(), Optional.of("bar").stream());
+  Stream<String> testOptionalStream() {
+    return Optional.of("foo").stream();
   }
 }

integration-tests/checkstyle-expected-changes.patch

@@ -53686,6 +53686,15 @@
      final ModuleFactory moduleFactory = TestUtil.getPackageObjectFactory();
 
      final Path path = Paths.get(XdocUtil.DIRECTORY_PATH + "/config.xml");
+@@ -1081,7 +1083,7 @@ public class XdocsPagesTest {
+         Optional.ofNullable(field)
+             .map(nonNullField -> nonNullField.getAnnotation(XdocsPropertyType.class))
+             .map(propertyType -> propertyType.value().getDescription())
+-            .orElse(fieldClass.getSimpleName());
++            .orElseGet(fieldClass::getSimpleName);
+     final String expectedValue =
+         getModulePropertyExpectedValue(sectionName, propertyName, field, fieldClass, instance);
+
 @@ -1364,7 +1366,7 @@ public class XdocsPagesTest {
          final Object[] array = (Object[]) value;
          valuesStream = Arrays.stream(array);

refaster-runner/src/main/java/tech/picnic/errorprone/refaster/runner/Refaster.java

@@ -166,7 +166,7 @@ private static Description augmentDescription(
             "Refaster Rule",
             description.getLink(),
             String.join(": ", description.checkName, description.getRawMessage()))
-        .overrideSeverity(severityOverride.orElse(description.severity()))
+        .overrideSeverity(severityOverride.orElseGet(description::severity))
         .addAllFixes(description.fixes)
         .build();
   }