From 25c84df1192833e84b7d6d590049312837e98ef3 Mon Sep 17 00:00:00 2001
From: Phil Werli <phil.werli@teampicnic.com>
Date: Tue, 1 Nov 2022 11:31:05 +0100
Subject: [PATCH 1/2] Introduce `OptionalStream{Filter,Map}` Refaster rules

---
 .../refasterrules/OptionalRules.java          | 35 +++++++++++++++++++
 .../refasterrules/OptionalRulesTestInput.java | 12 +++++++
 .../OptionalRulesTestOutput.java              | 10 ++++++
 3 files changed, 57 insertions(+)

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 1c58ee3d7a..a5e75f3f77 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
@@ -355,6 +355,41 @@ Optional<T> after(Optional<T> optional) {
     }
   }
 
+  /**
+   * Avoid unnecessary {@link Optional} to {@link Stream} conversion when filtering that ultimately
+   * returns the same result.
+   */
+  static final class OptionalStreamFilter<T> {
+    @BeforeTemplate
+    Optional<T> before(Optional<T> optional, Predicate<? super T> predicate) {
+      return Refaster.anyOf(
+          optional.stream().filter(predicate).findAny(),
+          optional.stream().filter(predicate).findFirst());
+    }
+
+    @AfterTemplate
+    Optional<T> after(Optional<T> optional, Predicate<? super T> predicate) {
+      return optional.filter(predicate);
+    }
+  }
+
+  /**
+   * Avoid unnecessary {@link Optional} to {@link Stream} conversion when mapping that ultimately
+   * returns the same result.
+   */
+  static final class OptionalStreamMap<S, T> {
+    @BeforeTemplate
+    Optional<? extends T> before(Optional<S> optional, Function<? super S, ? extends T> function) {
+      return Refaster.anyOf(
+          optional.stream().map(function).findAny(), optional.stream().map(function).findFirst());
+    }
+
+    @AfterTemplate
+    Optional<? extends T> after(Optional<S> optional, Function<? super S, ? extends T> function) {
+      return optional.map(function);
+    }
+  }
+
   // 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.)
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 d06d9da7d9..8c6d59b77f 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
@@ -109,4 +109,16 @@ ImmutableSet<Optional<String>> testOptionalIdentity() {
         Optional.of("baz").stream().min(String::compareTo),
         Optional.of("qux").stream().max(String::compareTo));
   }
+
+  ImmutableSet<Optional<String>> testOptionalStreamFilter() {
+    return ImmutableSet.of(
+        Optional.of("foo").stream().filter(String::isEmpty).findFirst(),
+        Optional.of("bar").stream().filter(String::isEmpty).findAny());
+  }
+
+  ImmutableSet<Optional<String>> testOptionalStreamMap() {
+    return ImmutableSet.of(
+        Optional.of(1).stream().map(String::valueOf).findFirst(),
+        Optional.of(2).stream().map(String::valueOf).findAny());
+  }
 }
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 e20f6b4a6f..89f32dc10c 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
@@ -103,4 +103,14 @@ ImmutableSet<Optional<String>> testOptionalIdentity() {
     return ImmutableSet.of(
         Optional.of("foo"), Optional.of("bar"), Optional.of("baz"), Optional.of("qux"));
   }
+
+  ImmutableSet<Optional<String>> testOptionalStreamFilter() {
+    return ImmutableSet.of(
+        Optional.of("foo").filter(String::isEmpty), Optional.of("bar").filter(String::isEmpty));
+  }
+
+  ImmutableSet<Optional<String>> testOptionalStreamMap() {
+    return ImmutableSet.of(
+        Optional.of(1).map(String::valueOf), Optional.of(2).map(String::valueOf));
+  }
 }

From d88bfec4180cd9de7cfa1f1b6a0d4787e9931655 Mon Sep 17 00:00:00 2001
From: Stephan Schroevers <stephan.schroevers@teampicnic.com>
Date: Fri, 4 Nov 2022 06:02:47 +0100
Subject: [PATCH 2/2] Suggestions

---
 .../refasterrules/OptionalRules.java          | 21 ++++++++++---------
 .../errorprone/refasterrules/StreamRules.java |  5 +++--
 .../refasterrules/OptionalRulesTestInput.java |  8 +++----
 .../OptionalRulesTestOutput.java              |  7 +++----
 4 files changed, 20 insertions(+), 21 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 a5e75f3f77..a42a8198f2 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
@@ -356,15 +356,15 @@ Optional<T> after(Optional<T> optional) {
   }
 
   /**
-   * Avoid unnecessary {@link Optional} to {@link Stream} conversion when filtering that ultimately
-   * returns the same result.
+   * Avoid unnecessary {@link Optional} to {@link Stream} conversion when filtering a value of the
+   * former type.
    */
-  static final class OptionalStreamFilter<T> {
+  static final class OptionalFilter<T> {
     @BeforeTemplate
     Optional<T> before(Optional<T> optional, Predicate<? super T> predicate) {
       return Refaster.anyOf(
-          optional.stream().filter(predicate).findAny(),
-          optional.stream().filter(predicate).findFirst());
+          optional.stream().filter(predicate).findFirst(),
+          optional.stream().filter(predicate).findAny());
     }
 
     @AfterTemplate
@@ -374,14 +374,15 @@ Optional<T> after(Optional<T> optional, Predicate<? super T> predicate) {
   }
 
   /**
-   * Avoid unnecessary {@link Optional} to {@link Stream} conversion when mapping that ultimately
-   * returns the same result.
+   * Avoid unnecessary {@link Optional} to {@link Stream} conversion when mapping a value of the
+   * former type.
    */
-  static final class OptionalStreamMap<S, T> {
+  // XXX: If `StreamMapFirst` also simplifies `.findAny()` expressions, then this rule can be
+  // dropped in favour of `StreamMapFirst` and `OptionalIdentity`.
+  static final class OptionalMap<S, T> {
     @BeforeTemplate
     Optional<? extends T> before(Optional<S> optional, Function<? super S, ? extends T> function) {
-      return Refaster.anyOf(
-          optional.stream().map(function).findAny(), optional.stream().map(function).findFirst());
+      return optional.stream().map(function).findAny();
     }
 
     @AfterTemplate
diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/StreamRules.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/StreamRules.java
index 17abcf97c3..9238ec0f80 100644
--- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/StreamRules.java
+++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/StreamRules.java
@@ -168,8 +168,9 @@ Stream<R> after(Stream<T> stream, Function<? super S, ? extends Stream<? extends
    * Where possible, clarify that a mapping operation will be applied only to a single stream
    * element.
    */
-  // XXX: Consider whether to have a similar rule for `.findAny()`. For parallel streams it
-  // wouldn't be quite the same....
+  // XXX: Implement a similar rule for `.findAny()`. For parallel streams this wouldn't be quite the
+  // same, so such a rule requires a `Matcher` that heuristically identifies `Stream` expressions
+  // with deterministic order.
   // XXX: This change is not equivalent for `null`-returning functions, as the original code throws
   // an NPE if the first element is `null`, while the latter yields an empty `Optional`.
   static final class StreamMapFirst<T, S> {
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 8c6d59b77f..5dbe4d699e 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
@@ -110,15 +110,13 @@ ImmutableSet<Optional<String>> testOptionalIdentity() {
         Optional.of("qux").stream().max(String::compareTo));
   }
 
-  ImmutableSet<Optional<String>> testOptionalStreamFilter() {
+  ImmutableSet<Optional<String>> testOptionalFilter() {
     return ImmutableSet.of(
         Optional.of("foo").stream().filter(String::isEmpty).findFirst(),
         Optional.of("bar").stream().filter(String::isEmpty).findAny());
   }
 
-  ImmutableSet<Optional<String>> testOptionalStreamMap() {
-    return ImmutableSet.of(
-        Optional.of(1).stream().map(String::valueOf).findFirst(),
-        Optional.of(2).stream().map(String::valueOf).findAny());
+  Optional<String> testOptionalMap() {
+    return Optional.of(1).stream().map(String::valueOf).findAny();
   }
 }
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 89f32dc10c..e22d20066f 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
@@ -104,13 +104,12 @@ ImmutableSet<Optional<String>> testOptionalIdentity() {
         Optional.of("foo"), Optional.of("bar"), Optional.of("baz"), Optional.of("qux"));
   }
 
-  ImmutableSet<Optional<String>> testOptionalStreamFilter() {
+  ImmutableSet<Optional<String>> testOptionalFilter() {
     return ImmutableSet.of(
         Optional.of("foo").filter(String::isEmpty), Optional.of("bar").filter(String::isEmpty));
   }
 
-  ImmutableSet<Optional<String>> testOptionalStreamMap() {
-    return ImmutableSet.of(
-        Optional.of(1).map(String::valueOf), Optional.of(2).map(String::valueOf));
+  Optional<String> testOptionalMap() {
+    return Optional.of(1).map(String::valueOf);
   }
 }