From 7b86718dd1bdc405a3db75403b3101ba72969c8c Mon Sep 17 00:00:00 2001 From: Stephan Schroevers Date: Sun, 14 Nov 2021 21:00:19 +0100 Subject: [PATCH 1/4] Introduce `OptionalOrElse` check While there, extend the `OptionalIdentity` Refaster rule to automatically resolve once class of `NestedOptionals` violations. --- .../bugpatterns/OptionalOrElse.java | 135 ++++++++++++++++++ .../refasterrules/OptionalRules.java | 8 +- .../bugpatterns/OptionalOrElseTest.java | 135 ++++++++++++++++++ .../refasterrules/OptionalRulesTestInput.java | 16 +-- .../OptionalRulesTestOutput.java | 8 +- .../errorprone/refaster/runner/Refaster.java | 2 +- 6 files changed, 289 insertions(+), 15 deletions(-) create mode 100644 error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/OptionalOrElse.java create mode 100644 error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/OptionalOrElseTest.java 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..7b359c735c --- /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 + && !requiresComputation(((MemberSelectTree) tree).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)) { + return Optional.empty(); + } + + MethodInvocationTree invocation = (MethodInvocationTree) tree; + if (!invocation.getArguments().isEmpty()) { + return Optional.empty(); + } + + if (!(invocation.getMethodSelect() instanceof MemberSelectTree)) { + return Optional.empty(); + } + + MemberSelectTree method = (MemberSelectTree) invocation.getMethodSelect(); + if (requiresComputation(method.getExpression())) { + return Optional.empty(); + } + + return Optional.of( + SourceCode.treeToString(method.getExpression(), state) + + "::" + + (invocation.getTypeArguments().isEmpty() + ? "" + : invocation.getTypeArguments().stream() + .map(arg -> SourceCode.treeToString(arg, state)) + .collect(joining(",", "<", ">"))) + + method.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..b8da9ac8c3 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 @@ -386,9 +386,13 @@ Optional after(Optional optional1, Optional optional2) { */ static final class OptionalIdentity { @BeforeTemplate + @SuppressWarnings("NestedOptionals" /* Auto-fix for the `NestedOptionals` check. */) 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/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(); } From 519512047b0e5bc99c25929a55624d6d324faf15 Mon Sep 17 00:00:00 2001 From: Stephan Schroevers Date: Sun, 11 Feb 2024 18:10:50 +0100 Subject: [PATCH 2/4] Use JDK 17 features --- .../bugpatterns/OptionalOrElse.java | 26 +++++++++---------- 1 file changed, 13 insertions(+), 13 deletions(-) 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 index 7b359c735c..6e1a83f413 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/OptionalOrElse.java @@ -45,7 +45,9 @@ @AutoService(BugChecker.class) @BugPattern( summary = - "Prefer `Optional#orElseGet` over `Optional#orElse` if the fallback requires additional computation", + """ + Prefer `Optional#orElseGet` over `Optional#orElse` if the fallback requires additional \ + computation""", linkType = NONE, severity = WARNING, tags = PERFORMANCE) @@ -96,40 +98,38 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState private static boolean requiresComputation(ExpressionTree tree) { return !(tree instanceof IdentifierTree || tree instanceof LiteralTree - || (tree instanceof MemberSelectTree - && !requiresComputation(((MemberSelectTree) tree).getExpression())) + || (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)) { + if (!(tree instanceof MethodInvocationTree methodInvocation)) { return Optional.empty(); } - MethodInvocationTree invocation = (MethodInvocationTree) tree; - if (!invocation.getArguments().isEmpty()) { + if (!methodInvocation.getArguments().isEmpty()) { return Optional.empty(); } - if (!(invocation.getMethodSelect() instanceof MemberSelectTree)) { + if (!(methodInvocation.getMethodSelect() instanceof MemberSelectTree memberSelect)) { return Optional.empty(); } - MemberSelectTree method = (MemberSelectTree) invocation.getMethodSelect(); - if (requiresComputation(method.getExpression())) { + if (requiresComputation(memberSelect.getExpression())) { return Optional.empty(); } return Optional.of( - SourceCode.treeToString(method.getExpression(), state) + SourceCode.treeToString(memberSelect.getExpression(), state) + "::" - + (invocation.getTypeArguments().isEmpty() + + (methodInvocation.getTypeArguments().isEmpty() ? "" - : invocation.getTypeArguments().stream() + : methodInvocation.getTypeArguments().stream() .map(arg -> SourceCode.treeToString(arg, state)) .collect(joining(",", "<", ">"))) - + method.getIdentifier()); + + memberSelect.getIdentifier()); } } From 50dfd35a5d9a70809a5cf83d45765359e47a018d Mon Sep 17 00:00:00 2001 From: Stephan Schroevers Date: Sat, 23 Mar 2024 23:04:21 +0100 Subject: [PATCH 3/4] Sync integration test --- integration-tests/checkstyle-expected-changes.patch | 9 +++++++++ 1 file changed, 9 insertions(+) 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); From c05aa21daf01b9aaad8cbf772b6341bc9ffc0761 Mon Sep 17 00:00:00 2001 From: Stephan Schroevers Date: Sun, 24 Mar 2024 09:26:06 +0100 Subject: [PATCH 4/4] Tweak --- .../tech/picnic/errorprone/refasterrules/OptionalRules.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) 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 b8da9ac8c3..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,7 +386,7 @@ Optional after(Optional optional1, Optional optional2) { */ static final class OptionalIdentity { @BeforeTemplate - @SuppressWarnings("NestedOptionals" /* Auto-fix for the `NestedOptionals` check. */) + @SuppressWarnings("NestedOptionals") Optional before(Optional optional, Comparator comparator) { return Refaster.anyOf( optional.or(Refaster.anyOf(() -> Optional.empty(), Optional::empty)),