Introduce assorted Optional Refaster rules

While there, move the `OptionalOrElseGet` Refaster rule test code to its properly location.
PicnicSupermarket · Aug 31, 2023 · 67c7cb6 · 67c7cb6
1 parent 8b5d180
commit 67c7cb6
Show file tree

Hide file tree

Showing 3 changed files with 69 additions and 20 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
@@ -100,6 +100,20 @@ Function<Optional<T>, T> after() {
     }
   }
 
+  /** Prefer {@link Optional#equals(Object)} over more contrived alternatives. */
+  static final class OptionalHasValue<T, S> {
+    @BeforeTemplate
+    boolean before(Optional<T> optional, S value) {
+      return Refaster.anyOf(
+          optional.filter(value::equals).isPresent(), optional.stream().anyMatch(value::equals));
+    }
+
+    @AfterTemplate
+    boolean after(Optional<T> 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<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.
+      // 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<? extends T> after(Optional<S> optional, Function<? super S, ? extends
     }
   }
 
+  /** Prefer {@link Optional#stream()} over more contrived alternatives. */
+  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));
+    }
+
+    @AfterTemplate
+    Stream<T> after(Optional<T> optional) {
+      return optional.stream();
+    }
+  }
+
   // XXX: Add a rule for:
   // `optional.flatMap(x -> pred(x) ? Optional.empty() : Optional.of(x))` and variants.
   // (Maybe canonicalize the inner expression. Maybe we rewrite already.)
-
-  // XXX: Add a rule for:
-  // `optional.map(Stream::of).orElse(Stream.empty())`
-  // `optional.map(Stream::of).orElseGet(Stream::empty)`
-  // -> `optional.stream()`
 }
diff --git a/...ntrib/src/test/resources/tech/picnic/errorprone/refasterrules/OptionalRulesTestInput.java b/...ntrib/src/test/resources/tech/picnic/errorprone/refasterrules/OptionalRulesTestInput.java
@@ -35,6 +35,12 @@ Function<Optional<Integer>, Integer> testOptionalOrElseThrowMethodReference() {
     return Optional::get;
   }
 
+  ImmutableSet<Boolean> testOptionalHasValue() {
+    return ImmutableSet.of(
+        Optional.of("foo").filter("bar"::equals).isPresent(),
+        Optional.of("baz").stream().anyMatch("qux"::equals));
+  }
+
   ImmutableSet<Optional<String>> 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<String> testOptionalOrElseGet() {
+    return ImmutableSet.of(
+        Optional.of("foo").orElse("bar"),
+        Optional.of("baz").orElse(toString()),
+        Optional.of("qux").orElse(String.valueOf(true)));
+  }
+
   ImmutableSet<Object> testStreamFlatMapOptional() {
     return ImmutableSet.of(
         Stream.of(Optional.empty()).filter(Optional::isPresent).map(Optional::orElseThrow),
@@ -99,7 +112,8 @@ ImmutableSet<Optional<String>> 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<Optional<String>> testOptionalIdentity() {
@@ -120,10 +134,9 @@ Optional<String> testOptionalMap() {
     return Optional.of(1).stream().map(String::valueOf).findAny();
   }
 
-  ImmutableSet<String> testOptionalOrElseGet() {
+  ImmutableSet<Stream<String>> 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/...trib/src/test/resources/tech/picnic/errorprone/refasterrules/OptionalRulesTestOutput.java b/...trib/src/test/resources/tech/picnic/errorprone/refasterrules/OptionalRulesTestOutput.java
@@ -35,6 +35,12 @@ Function<Optional<Integer>, Integer> testOptionalOrElseThrowMethodReference() {
     return Optional::orElseThrow;
   }
 
+  ImmutableSet<Boolean> testOptionalHasValue() {
+    return ImmutableSet.of(
+        Optional.of("foo").equals(Optional.of("bar")),
+        Optional.of("baz").equals(Optional.of("qux")));
+  }
+
   ImmutableSet<Optional<String>> 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<String> testOptionalOrElseGet() {
+    return ImmutableSet.of(
+        Optional.of("foo").orElse("bar"),
+        Optional.of("baz").orElse(toString()),
+        Optional.of("qux").orElseGet(() -> String.valueOf(true)));
+  }
+
   ImmutableSet<Object> testStreamFlatMapOptional() {
     return ImmutableSet.of(
         Stream.of(Optional.empty()).flatMap(Optional::stream),
@@ -96,7 +109,8 @@ ImmutableSet<Optional<String>> 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<Optional<String>> testOptionalIdentity() {
@@ -113,10 +127,7 @@ 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)));
+  ImmutableSet<Stream<String>> testOptionalStream() {
+    return ImmutableSet.of(Optional.of("foo").stream(), Optional.of("bar").stream());
   }
 }