Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rewrite Optional#orElse to Optional#orElseGet for non-constant expressions #527

Merged
merged 9 commits into from
May 26, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
import com.google.errorprone.refaster.annotation.AfterTemplate;
import com.google.errorprone.refaster.annotation.BeforeTemplate;
import com.google.errorprone.refaster.annotation.MayOptionallyUse;
import com.google.errorprone.refaster.annotation.NotMatches;
import com.google.errorprone.refaster.annotation.Placeholder;
import com.google.errorprone.refaster.annotation.UseImportPolicy;
import java.util.Comparator;
Expand All @@ -19,6 +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;

/** Refaster rules related to expressions dealing with {@link Optional}s. */
@OnlineDocumentation
Expand Down Expand Up @@ -118,7 +120,7 @@ Optional<T> after(Iterator<T> it) {
/** Prefer {@link Optional#filter(Predicate)} over usage of the ternary operator. */
// XXX: This rule may introduce a compilation error: the `test` expression may reference a
// non-effectively final variable, which is not allowed in the replacement lambda expression.
// Maybe our `Refaster` checker should test `compilesWithFix`?
// Review whether a `@Matcher` can be used to avoid this.
abstract static class TernaryOperatorOptionalPositiveFiltering<T> {
@Placeholder
abstract boolean test(T value);
Expand All @@ -138,7 +140,7 @@ Optional<T> after(T input) {
/** Prefer {@link Optional#filter(Predicate)} over usage of the ternary operator. */
// XXX: This rule may introduce a compilation error: the `test` expression may reference a
// non-effectively final variable, which is not allowed in the replacement lambda expression.
// Maybe our `Refaster` checker should test `compilesWithFix`?
// Review whether a `@Matcher` can be used to avoid this.
abstract static class TernaryOperatorOptionalNegativeFiltering<T> {
@Placeholder
abstract boolean test(T value);
Expand All @@ -161,6 +163,7 @@ Optional<T> after(T input) {
*/
static final class MapOptionalToBoolean<T> {
@BeforeTemplate
@SuppressWarnings("OptionalOrElseGet" /* Rule is confused by `Refaster#anyOf` usage. */)
boolean before(Optional<T> optional, Function<? super T, Boolean> predicate) {
return optional.map(predicate).orElse(Refaster.anyOf(false, Boolean.FALSE));
}
Expand Down Expand Up @@ -224,6 +227,28 @@ 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.
*/
// 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> {
@BeforeTemplate
T before(Optional<T> optional, @NotMatches(IsLikelyTrivialComputation.class) T value) {
return optional.orElse(value);
}

@AfterTemplate
T after(Optional<T> optional, T value) {
return optional.orElseGet(() -> value);
}
}

/**
* Flatten a stream of {@link Optional}s using {@link Optional#stream()}, rather than using one of
* the more verbose alternatives.
Expand Down Expand Up @@ -325,6 +350,9 @@ static final class OptionalOrOtherOptional<T> {
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.
// XXX: Note that rewriting the first and third variant will introduce a compilation error if
// `optional2` is not effectively final. Review whether a `@Matcher` can be used to avoid
// this.
return Refaster.anyOf(
optional1.map(Optional::of).orElse(optional2),
optional1.map(Optional::of).orElseGet(() -> optional2),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -119,4 +119,11 @@ ImmutableSet<Optional<String>> testOptionalFilter() {
Optional<String> testOptionalMap() {
return Optional.of(1).stream().map(String::valueOf).findAny();
}

ImmutableSet<String> testOptionalOrElseGet() {
return ImmutableSet.of(
Optional.of("foo").orElse("bar"),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We make sure to use distinct values for every statement :).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it need to be distinct for mutation testing?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope, it's about making sure that Refaster properly moves things around (which is easier to see if all values are distinct). Unfortunately for technical reasons our Refaster tests aren't exercised by Pitest.

Optional.of("baz").orElse(toString()),
Optional.of("qux").orElse(String.valueOf(true)));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -112,4 +112,11 @@ ImmutableSet<Optional<String>> testOptionalFilter() {
Optional<String> testOptionalMap() {
return Optional.of(1).map(String::valueOf);
}

ImmutableSet<String> testOptionalOrElseGet() {
return ImmutableSet.of(
Optional.of("foo").orElse("bar"),
Optional.of("baz").orElse(toString()),
Optional.of("qux").orElseGet(() -> String.valueOf(true)));
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,92 @@
package tech.picnic.errorprone.refaster.matchers;

import com.google.errorprone.VisitorState;
import com.google.errorprone.matchers.Matcher;
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> {
private static final long serialVersionUID = 1L;

/** Instantiates a new {@link IsLikelyTrivialComputation} instance. */
public IsLikelyTrivialComputation() {}

@Override
public boolean matches(ExpressionTree expressionTree, VisitorState state) {
if (expressionTree instanceof MethodInvocationTree) {
// 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.)
MethodInvocationTree methodInvocation = (MethodInvocationTree) expressionTree;
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) {
return matches(((ArrayAccessTree) expressionTree).getExpression())
&& matches(((ArrayAccessTree) expressionTree).getIndex());
}

if (expressionTree instanceof LiteralTree) {
return true;
}

if (expressionTree instanceof LambdaExpressionTree) {
/*
* Lambda expressions encapsulate computations, but their definition does not involve
* significant computation.
*/
return true;
}

if (expressionTree instanceof IdentifierTree) {
return true;
}

if (expressionTree instanceof MemberReferenceTree) {
return matches(((MemberReferenceTree) expressionTree).getQualifierExpression());
}

if (expressionTree instanceof MemberSelectTree) {
return matches(((MemberSelectTree) expressionTree).getExpression());
}

if (expressionTree instanceof ParenthesizedTree) {
return matches(((ParenthesizedTree) expressionTree).getExpression());
}

if (expressionTree instanceof TypeCastTree) {
return matches(((TypeCastTree) expressionTree).getExpression());
}

if (expressionTree instanceof UnaryTree) {
// XXX: Arguably side-effectful options such as pre- and post-increment and -decrement are not
// trivial.
return matches(((UnaryTree) expressionTree).getExpression());
}

return false;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,147 @@
package tech.picnic.errorprone.refaster.matchers;

import static com.google.errorprone.BugPattern.SeverityLevel.ERROR;

import com.google.errorprone.BugPattern;
import com.google.errorprone.CompilationTestHelper;
import com.google.errorprone.bugpatterns.BugChecker;
import com.sun.source.tree.ReturnTree;
import org.junit.jupiter.api.Test;

final class IsLikelyTrivialComputationTest {
@Test
void matches() {
CompilationTestHelper.newInstance(MatcherTestChecker.class, getClass())
.addSourceLines(
"A.java",
"import java.util.function.Predicate;",
"",
"class A {",
" String negative1() {",
" return String.valueOf(1);",
" }",
"",
" String negative2() {",
" return toString().toString();",
" }",
"",
" String negative3() {",
" return \"foo\" + toString();",
" }",
"",
" byte negative4() {",
" return \"foo\".getBytes()[0];",
" }",
"",
" int negative5() {",
" int[] arr = new int[0];",
" return arr[hashCode()];",
" }",
"",
" int negative6() {",
" return 1 * 2;",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the BinaryTree we can check if both left and right are an LiteralTree or IdentifierTree as start? Also fine with leaving this as is.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, one could argue that this is a trivial computation. But then maybe so is 4 * 60 * 60 ("four hours"), which still wouldn't be flagged. And since the matcher is already quite complex, I'm not sure how far to go for diminishing returns. Even this case I don't expect to be triggered too frequently (at least in our internal code base).

" }",
"",
" Predicate<String> negative7() {",
" return toString()::equals;",
" }",
"",
" String negative8() {",
" return (toString());",
" }",
"",
" Object negative9() {",
" return (Object) toString();",
" }",
"",
" int negative10() {",
" return -hashCode();",
" }",
"",
" String positive1() {",
" // BUG: Diagnostic contains:",
" return toString();",
" }",
"",
" String positive2() {",
" // BUG: Diagnostic contains:",
" return this.toString();",
" }",
"",
" int positive3() {",
" int[] arr = new int[0];",
" // BUG: Diagnostic contains:",
" return arr[0];",
" }",
"",
" String positive4() {",
" // BUG: Diagnostic contains:",
" return null;",
" }",
"",
" boolean positive5() {",
" // BUG: Diagnostic contains:",
" return false;",
" }",
"",
" int positive6() {",
" // BUG: Diagnostic contains:",
" return 0;",
" }",
"",
" String positive7() {",
" // BUG: Diagnostic contains:",
" return \"foo\" + \"bar\";",
" }",
"",
" Predicate<String> positive8() {",
" // BUG: Diagnostic contains:",
" return v -> \"foo\".equals(v);",
" }",
"",
" A positive9() {",
" // BUG: Diagnostic contains:",
" return this;",
" }",
"",
" Predicate<String> positive10() {",
" // BUG: Diagnostic contains:",
" return \"foo\"::equals;",
" }",
"",
" A positive11() {",
" // BUG: Diagnostic contains:",
" return (this);",
" }",
"",
" Object positive12() {",
" // BUG: Diagnostic contains:",
" return (Object) this;",
" }",
"",
" boolean positive13() {",
" // BUG: Diagnostic contains:",
" return !false;",
" }",
"}")
.doTest();
}

/** A {@link BugChecker} that simply delegates to {@link IsLikelyTrivialComputation}. */
@BugPattern(
summary = "Flags return statement expressions matched by `IsLikelyTrivialComputation`",
severity = ERROR)
public static final class MatcherTestChecker extends AbstractMatcherTestChecker {
private static final long serialVersionUID = 1L;

// XXX: This is a false positive reported by Checkstyle. See
// https://github.com/checkstyle/checkstyle/issues/10161#issuecomment-1242732120.
@SuppressWarnings("RedundantModifier")
public MatcherTestChecker() {
super(
(expressionTree, state) ->
state.getPath().getParentPath().getLeaf() instanceof ReturnTree
&& new IsLikelyTrivialComputation().matches(expressionTree, state));
}
}
}