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 2a84eaf419..a8eb97b452 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 @@ -100,6 +100,20 @@ Function, T> after() { } } + /** Prefer {@link Optional#equals(Object)} over more contrived alternatives. */ + static final class OptionalEqualsOptional { + @BeforeTemplate + boolean before(Optional optional, S value) { + return Refaster.anyOf( + optional.filter(value::equals).isPresent(), optional.stream().anyMatch(value::equals)); + } + + @AfterTemplate + boolean after(Optional optional, S value) { + return optional.equals(Optional.of(value)); + } + } + /** * Don't use the ternary operator to extract the first element of a possibly-empty {@link * Iterator} as an {@link Optional}. @@ -350,13 +364,14 @@ static final class OptionalOrOtherOptional { 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. - // 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. + // XXX: Note that rewriting the first, third and fourth 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), - Stream.of(optional1, optional2).flatMap(Optional::stream).findFirst()); + Stream.of(optional1, optional2).flatMap(Optional::stream).findFirst(), + optional1.isPresent() ? optional1 : optional2); } @AfterTemplate @@ -422,12 +437,22 @@ Optional after(Optional optional, Function pred(x) ? Optional.empty() : Optional.of(x))` and variants. - // (Maybe canonicalize the inner expression. Maybe we rewrite already.) + /** Prefer {@link Optional#stream()} over more contrived alternatives. */ + static final class OptionalStream { + @BeforeTemplate + Stream before(Optional optional) { + return Refaster.anyOf( + optional.map(Stream::of).orElse(Stream.empty()), + optional.map(Stream::of).orElseGet(Stream::empty)); + } + + @AfterTemplate + Stream after(Optional optional) { + return optional.stream(); + } + } // XXX: Add a rule for: - // `optional.map(Stream::of).orElse(Stream.empty())` - // `optional.map(Stream::of).orElseGet(Stream::empty)` - // -> `optional.stream()` + // `optional.flatMap(x -> pred(x) ? Optional.empty() : Optional.of(x))` and variants. + // (Maybe canonicalize the inner expression. Maybe we rewrite it already.) } 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 979717fcfd..c09719bea5 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 @@ -35,6 +35,12 @@ Function, Integer> testOptionalOrElseThrowMethodReference() { return Optional::get; } + ImmutableSet testOptionalEqualsOptional() { + return ImmutableSet.of( + Optional.of("foo").filter("bar"::equals).isPresent(), + Optional.of("baz").stream().anyMatch("qux"::equals)); + } + ImmutableSet> testOptionalFirstIteratorElement() { return ImmutableSet.of( ImmutableSet.of("foo").iterator().hasNext() @@ -73,6 +79,13 @@ String testOrOrElseThrow() { return Optional.of("foo").orElseGet(() -> Optional.of("bar").orElseThrow()); } + ImmutableSet testOptionalOrElseGet() { + return ImmutableSet.of( + Optional.of("foo").orElse("bar"), + Optional.of("baz").orElse(toString()), + Optional.of("qux").orElse(String.valueOf(true))); + } + ImmutableSet testStreamFlatMapOptional() { return ImmutableSet.of( Stream.of(Optional.empty()).filter(Optional::isPresent).map(Optional::orElseThrow), @@ -99,7 +112,8 @@ ImmutableSet> testOptionalOrOtherOptional() { return ImmutableSet.of( Optional.of("foo").map(Optional::of).orElse(Optional.of("bar")), Optional.of("baz").map(Optional::of).orElseGet(() -> Optional.of("qux")), - Stream.of(Optional.of("quux"), Optional.of("quuz")).flatMap(Optional::stream).findFirst()); + Stream.of(Optional.of("quux"), Optional.of("quuz")).flatMap(Optional::stream).findFirst(), + Optional.of("corge").isPresent() ? Optional.of("corge") : Optional.of("grault")); } ImmutableSet> testOptionalIdentity() { @@ -120,10 +134,9 @@ Optional testOptionalMap() { return Optional.of(1).stream().map(String::valueOf).findAny(); } - ImmutableSet testOptionalOrElseGet() { + ImmutableSet> testOptionalStream() { return ImmutableSet.of( - Optional.of("foo").orElse("bar"), - Optional.of("baz").orElse(toString()), - Optional.of("qux").orElse(String.valueOf(true))); + Optional.of("foo").map(Stream::of).orElse(Stream.empty()), + Optional.of("bar").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 cafc70cfa3..8881529247 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 @@ -35,6 +35,12 @@ Function, Integer> testOptionalOrElseThrowMethodReference() { return Optional::orElseThrow; } + ImmutableSet testOptionalEqualsOptional() { + return ImmutableSet.of( + Optional.of("foo").equals(Optional.of("bar")), + Optional.of("baz").equals(Optional.of("qux"))); + } + ImmutableSet> testOptionalFirstIteratorElement() { return ImmutableSet.of( stream(ImmutableSet.of("foo").iterator()).findFirst(), @@ -70,6 +76,13 @@ String testOrOrElseThrow() { return Optional.of("foo").or(() -> Optional.of("bar")).orElseThrow(); } + ImmutableSet testOptionalOrElseGet() { + return ImmutableSet.of( + Optional.of("foo").orElse("bar"), + Optional.of("baz").orElse(toString()), + Optional.of("qux").orElseGet(() -> String.valueOf(true))); + } + ImmutableSet testStreamFlatMapOptional() { return ImmutableSet.of( Stream.of(Optional.empty()).flatMap(Optional::stream), @@ -96,7 +109,8 @@ ImmutableSet> testOptionalOrOtherOptional() { return ImmutableSet.of( Optional.of("foo").or(() -> Optional.of("bar")), Optional.of("baz").or(() -> Optional.of("qux")), - Optional.of("quux").or(() -> Optional.of("quuz"))); + Optional.of("quux").or(() -> Optional.of("quuz")), + Optional.of("corge").or(() -> Optional.of("grault"))); } ImmutableSet> testOptionalIdentity() { @@ -113,10 +127,7 @@ Optional testOptionalMap() { return Optional.of(1).map(String::valueOf); } - ImmutableSet testOptionalOrElseGet() { - return ImmutableSet.of( - Optional.of("foo").orElse("bar"), - Optional.of("baz").orElse(toString()), - Optional.of("qux").orElseGet(() -> String.valueOf(true))); + ImmutableSet> testOptionalStream() { + return ImmutableSet.of(Optional.of("foo").stream(), Optional.of("bar").stream()); } }