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/OptionalOrElse.java new file mode 100644 index 0000000000..6e1a83f413 --- /dev/null +++ b/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)}. + * + *

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 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() {} + + @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 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()); + } +} 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 1491b18eac..cf1c2cb3ce 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 @@ -360,7 +360,7 @@ Optional after(Optional optional, Function { @BeforeTemplate - @SuppressWarnings("NestedOptionals" /* Auto-fix for the `NestedOptionals` check. */) + @SuppressWarnings("NestedOptionals") 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. @@ -386,9 +386,13 @@ Optional after(Optional optional1, Optional optional2) { */ static final class OptionalIdentity { @BeforeTemplate + @SuppressWarnings("NestedOptionals") Optional before(Optional optional, Comparator 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 after(Optional optional, Function { @BeforeTemplate Stream before(Optional 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 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/OptionalOrElseTest.java new file mode 100644 index 0000000000..620a532ae6 --- /dev/null +++ b/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 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 foo() {", + " return null;", + " }", + "}") + .doTest(); + } + + @Test + void replacement() { + BugCheckerRefactoringTestHelper.newInstance(OptionalOrElse.class, getClass()) + .addInputLines( + "A.java", + "import java.util.Optional;", + "", + "class A {", + " private final Optional 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.foo());", + " optional.orElse(this.bar());", + " optional.orElse(new Object() {});", + " optional.orElse(new int[0].length);", + " }", + "", + " private T foo() {", + " return null;", + " }", + "", + " private T bar() {", + " return null;", + " }", + "}") + .addOutputLines( + "A.java", + "import java.util.Optional;", + "", + "class A {", + " private final Optional 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::foo);", + " optional.orElseGet(this::bar);", + " optional.orElseGet(() -> new Object() {});", + " optional.orElseGet(() -> new int[0].length);", + " }", + "", + " private T foo() {", + " return null;", + " }", + "", + " private T bar() {", + " return null;", + " }", + "}") + .doTest(TestMode.TEXT_MATCH); + } +} 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 20eb7fe221..ccd2d3f5b2 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 @@ -120,10 +120,12 @@ ImmutableSet> 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> testOptionalFilter() { @@ -136,9 +138,7 @@ Optional testOptionalMap() { return Optional.of(1).stream().map(String::valueOf).findAny(); } - ImmutableSet> testOptionalStream() { - return ImmutableSet.of( - Optional.of("foo").map(Stream::of).orElse(Stream.empty()), - Optional.of("bar").map(Stream::of).orElseGet(Stream::empty)); + Stream testOptionalStream() { + return Optional.of("foo").map(Stream::of).orElseGet(Stream::empty); } } 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 cf589e9d94..2a2985fe9b 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 @@ -120,7 +120,9 @@ ImmutableSet> testOptionalIdentity() { Optional.of("baz"), Optional.of("qux"), Optional.of("quux"), - Optional.of("quuz")); + Optional.of("quuz"), + Optional.of("corge"), + Optional.of("grault")); } ImmutableSet> testOptionalFilter() { @@ -132,7 +134,7 @@ Optional testOptionalMap() { return Optional.of(1).map(String::valueOf); } - ImmutableSet> testOptionalStream() { - return ImmutableSet.of(Optional.of("foo").stream(), Optional.of("bar").stream()); + Stream testOptionalStream() { + return Optional.of("foo").stream(); } } diff --git a/integration-tests/checkstyle-expected-changes.patch b/integration-tests/checkstyle-expected-changes.patch index 412e42f4fa..efb017e9e5 100644 --- a/integration-tests/checkstyle-expected-changes.patch +++ b/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); diff --git a/refaster-runner/src/main/java/tech/picnic/errorprone/refaster/runner/Refaster.java b/refaster-runner/src/main/java/tech/picnic/errorprone/refaster/runner/Refaster.java index 274b9b5710..94abfc23a8 100644 --- a/refaster-runner/src/main/java/tech/picnic/errorprone/refaster/runner/Refaster.java +++ b/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(); }