Suggestions

PicnicSupermarket · Dec 30, 2022 · 67c1519 · 67c1519
1 parent ea50726
commit 67c1519
Show file tree

Hide file tree

Showing 3 changed files with 23 additions and 24 deletions.
diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/NullRules.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/NullRules.java
@@ -5,9 +5,9 @@
 import static java.util.Objects.requireNonNullElseGet;
 
 import com.google.common.base.MoreObjects;
+import com.google.errorprone.refaster.Refaster;
 import com.google.errorprone.refaster.annotation.AfterTemplate;
 import com.google.errorprone.refaster.annotation.BeforeTemplate;
-import com.google.errorprone.refaster.annotation.Matches;
 import com.google.errorprone.refaster.annotation.UseImportPolicy;
 import java.util.Objects;
 import java.util.Optional;
@@ -48,21 +48,17 @@ boolean after(@Nullable Object object) {
   }
 
   /**
-   * Prefer {@link Objects#requireNonNullElse(Object, Object)} over the Guava and Optional
+   * Prefer {@link Objects#requireNonNullElse(Object, Object)} over non-JDK or more contrived
    * alternatives.
    */
   // XXX: This rule is not valid in case `second` is `@Nullable`: in that case the Guava and
-  // Optional variants
-  // will return `null`, while the Objects.requireNonNullElse variant will throw an NPE.
+  // `Optional` variants will return `null`, where the `requireNonNullElse` alternative will throw
+  // an NPE.
   static final class RequireNonNullElse<T> {
     @BeforeTemplate
     T before(T first, T second) {
-      return MoreObjects.firstNonNull(first, second);
-    }
-
-    @BeforeTemplate
-    T beforeOptional(T first, T second) {
-      return Optional.ofNullable(first).orElse(second);
+      return Refaster.anyOf(
+          MoreObjects.firstNonNull(first, second), Optional.ofNullable(first).orElse(second));
     }
 
     @AfterTemplate
@@ -73,21 +69,22 @@ T after(T first, T second) {
   }
 
   /**
-   * Prefer {@link Objects#requireNonNullElseGet(Object, Supplier)} over {@link Optional}{@code
-   * .ofNullable(Object).orElseGet(Supplier)}
+   * Prefer {@link Objects#requireNonNullElseGet(Object, Supplier)} over more contrived
+   * alternatives.
    */
-  // XXX: This rule is not valid in case `supplier` may return `null`: Optional will return `null`,
-  // while the `requireNonNullElseGet` replacement will throw an NPE.
-  static final class RequireNonNullElseGet<T, S extends Supplier<T>> {
+  // XXX: This rule is not valid in case `supplier` yields `@Nullable` values: in that case the
+  // `Optional` variant will return `null`, where the `requireNonNullElseGet` alternative will throw
+  // an NPE.
+  static final class RequireNonNullElseGet<T, S extends T> {
     @BeforeTemplate
-    T before(T object, S defaultSupplier) {
-      return Optional.ofNullable(object).orElseGet(defaultSupplier);
+    T before(T object, Supplier<S> supplier) {
+      return Optional.ofNullable(object).orElseGet(supplier);
     }
 
     @AfterTemplate
     @UseImportPolicy(STATIC_IMPORT_ALWAYS)
-    T after(T object, S defaultSupplier) {
-      return requireNonNullElseGet(object, defaultSupplier);
+    T after(T object, Supplier<S> supplier) {
+      return requireNonNullElseGet(object, supplier);
     }
   }
 

diff --git a/...e-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/NullRulesTestInput.java b/...e-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/NullRulesTestInput.java
@@ -3,13 +3,14 @@
 import com.google.common.base.MoreObjects;
 import com.google.common.collect.ImmutableSet;
 import java.util.Objects;
+import java.util.Optional;
 import java.util.stream.Stream;
 import tech.picnic.errorprone.refaster.test.RefasterRuleCollectionTestCase;
 
 final class NullRulesTest implements RefasterRuleCollectionTestCase {
   @Override
   public ImmutableSet<?> elidedTypesAndStaticImports() {
-    return ImmutableSet.of(MoreObjects.class);
+    return ImmutableSet.of(MoreObjects.class, Optional.class);
   }
 
   boolean testIsNull() {
@@ -22,11 +23,11 @@ boolean testIsNotNull() {
 
   ImmutableSet<String> testRequireNonNullElse() {
     return ImmutableSet.of(
-        MoreObjects.firstNonNull("foo", "bar"), java.util.Optional.ofNullable("foo").orElse("bar"));
+        MoreObjects.firstNonNull("foo", "bar"), Optional.ofNullable("baz").orElse("qux"));
   }
 
   String testRequireNonNullElseGet() {
-    return java.util.Optional.ofNullable("foo").orElseGet(() -> "bar");
+    return Optional.ofNullable("foo").orElseGet(() -> "bar");
   }
 
   long testIsNullFunction() {

diff --git a/...-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/NullRulesTestOutput.java b/...-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/NullRulesTestOutput.java
@@ -6,13 +6,14 @@
 import com.google.common.base.MoreObjects;
 import com.google.common.collect.ImmutableSet;
 import java.util.Objects;
+import java.util.Optional;
 import java.util.stream.Stream;
 import tech.picnic.errorprone.refaster.test.RefasterRuleCollectionTestCase;
 
 final class NullRulesTest implements RefasterRuleCollectionTestCase {
   @Override
   public ImmutableSet<?> elidedTypesAndStaticImports() {
-    return ImmutableSet.of(MoreObjects.class);
+    return ImmutableSet.of(MoreObjects.class, Optional.class);
   }
 
   boolean testIsNull() {
@@ -24,7 +25,7 @@ boolean testIsNotNull() {
   }
 
   ImmutableSet<String> testRequireNonNullElse() {
-    return ImmutableSet.of(requireNonNullElse("foo", "bar"), requireNonNullElse("foo", "bar"));
+    return ImmutableSet.of(requireNonNullElse("foo", "bar"), requireNonNullElse("baz", "qux"));
   }
 
   String testRequireNonNullElseGet() {