Introduce StringIs{,Not}EmptyPredicate Refaster rules (#577)

PicnicSupermarket · Apr 18, 2023 · 3af81d8 · 3af81d8
1 parent ebd64c1
commit 3af81d8
Show file tree

Hide file tree

Showing 3 changed files with 62 additions and 8 deletions.
diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/StringRules.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/StringRules.java
@@ -1,6 +1,8 @@
 package tech.picnic.errorprone.refasterrules;
 
+import static com.google.errorprone.refaster.ImportPolicy.STATIC_IMPORT_ALWAYS;
 import static java.nio.charset.StandardCharsets.UTF_8;
+import static java.util.function.Predicate.not;
 import static java.util.stream.Collectors.joining;
 
 import com.google.common.base.Joiner;
@@ -11,21 +13,23 @@
 import com.google.errorprone.refaster.annotation.AfterTemplate;
 import com.google.errorprone.refaster.annotation.AlsoNegation;
 import com.google.errorprone.refaster.annotation.BeforeTemplate;
+import com.google.errorprone.refaster.annotation.UseImportPolicy;
 import java.util.Arrays;
 import java.util.Collection;
 import java.util.Objects;
 import java.util.Optional;
 import java.util.function.Function;
+import java.util.function.Predicate;
 import org.jspecify.annotations.Nullable;
 import tech.picnic.errorprone.refaster.annotation.OnlineDocumentation;
 
 /** Refaster rules related to expressions dealing with {@link String}s. */
-// XXX: Should we prefer `s -> !s.isEmpty()` or `not(String::isEmpty)`?
 @OnlineDocumentation
 final class StringRules {
   private StringRules() {}
 
   /** Prefer {@link String#isEmpty()} over alternatives that consult the string's length. */
+  // XXX: Once we target JDK 15+, generalize this rule to cover all `CharSequence` subtypes.
   static final class StringIsEmpty {
     @BeforeTemplate
     boolean before(String str) {
@@ -39,6 +43,37 @@ boolean after(String str) {
     }
   }
 
+  /** Prefer a method reference to {@link String#isEmpty()} over the equivalent lambda function. */
+  // XXX: Once we target JDK 15+, generalize this rule to cover all `CharSequence` subtypes.
+  // XXX: As it stands, this rule is a special case of what `MethodReferenceUsage` tries to achieve.
+  // If/when `MethodReferenceUsage` becomes production ready, we should simply drop this check.
+  static final class StringIsEmptyPredicate {
+    @BeforeTemplate
+    Predicate<String> before() {
+      return s -> s.isEmpty();
+    }
+
+    @AfterTemplate
+    Predicate<String> after() {
+      return String::isEmpty;
+    }
+  }
+
+  /** Prefer a method reference to {@link String#isEmpty()} over the equivalent lambda function. */
+  // XXX: Once we target JDK 15+, generalize this rule to cover all `CharSequence` subtypes.
+  static final class StringIsNotEmptyPredicate {
+    @BeforeTemplate
+    Predicate<String> before() {
+      return s -> !s.isEmpty();
+    }
+
+    @AfterTemplate
+    @UseImportPolicy(STATIC_IMPORT_ALWAYS)
+    Predicate<String> after() {
+      return not(String::isEmpty);
+    }
+  }
+
   /** Prefer {@link Strings#isNullOrEmpty(String)} over the more verbose alternative. */
   static final class StringIsNullOrEmpty {
     @BeforeTemplate
@@ -65,7 +100,7 @@ Optional<String> before(String str) {
 
     @AfterTemplate
     Optional<String> after(String str) {
-      return Optional.ofNullable(str).filter(s -> !s.isEmpty());
+      return Optional.ofNullable(str).filter(not(String::isEmpty));
     }
   }
 
@@ -76,8 +111,9 @@ Optional<String> before(Optional<String> optional) {
     }
 
     @AfterTemplate
+    @UseImportPolicy(STATIC_IMPORT_ALWAYS)
     Optional<String> after(Optional<String> optional) {
-      return optional.filter(s -> !s.isEmpty());
+      return optional.filter(not(String::isEmpty));
     }
   }
 

diff --git a/...contrib/src/test/resources/tech/picnic/errorprone/refasterrules/StringRulesTestInput.java b/...contrib/src/test/resources/tech/picnic/errorprone/refasterrules/StringRulesTestInput.java
@@ -33,6 +33,14 @@ ImmutableSet<Boolean> testStringIsEmpty() {
         "baz".length() >= 1);
   }
 
+  boolean testStringIsEmptyPredicate() {
+    return Stream.of("foo").anyMatch(s -> s.isEmpty());
+  }
+
+  boolean testStringIsNotEmptyPredicate() {
+    return Stream.of("foo").anyMatch(s -> !s.isEmpty());
+  }
+
   ImmutableSet<Boolean> testStringIsNullOrEmpty() {
     return ImmutableSet.of(
         getClass().getName() == null || getClass().getName().isEmpty(),

diff --git a/...ontrib/src/test/resources/tech/picnic/errorprone/refasterrules/StringRulesTestOutput.java b/...ontrib/src/test/resources/tech/picnic/errorprone/refasterrules/StringRulesTestOutput.java
@@ -1,6 +1,7 @@
 package tech.picnic.errorprone.refasterrules;
 
 import static java.nio.charset.StandardCharsets.UTF_8;
+import static java.util.function.Predicate.not;
 import static java.util.stream.Collectors.joining;
 
 import com.google.common.base.Joiner;
@@ -14,6 +15,7 @@
 import java.util.Objects;
 import java.util.Optional;
 import java.util.function.Function;
+import java.util.function.Predicate;
 import java.util.stream.Stream;
 import tech.picnic.errorprone.refaster.test.RefasterRuleCollectionTestCase;
 
@@ -34,21 +36,29 @@ ImmutableSet<Boolean> testStringIsEmpty() {
         !"baz".isEmpty());
   }
 
+  boolean testStringIsEmptyPredicate() {
+    return Stream.of("foo").anyMatch(String::isEmpty);
+  }
+
+  boolean testStringIsNotEmptyPredicate() {
+    return Stream.of("foo").anyMatch(not(String::isEmpty));
+  }
+
   ImmutableSet<Boolean> testStringIsNullOrEmpty() {
     return ImmutableSet.of(
         Strings.isNullOrEmpty(getClass().getName()), !Strings.isNullOrEmpty(getClass().getName()));
   }
 
   ImmutableSet<Optional<String>> testOptionalNonEmptyString() {
     return ImmutableSet.of(
-        Optional.ofNullable(toString()).filter(s -> !s.isEmpty()),
-        Optional.ofNullable(toString()).filter(s -> !s.isEmpty()),
-        Optional.ofNullable(toString()).filter(s -> !s.isEmpty()),
-        Optional.ofNullable(toString()).filter(s -> !s.isEmpty()));
+        Optional.ofNullable(toString()).filter(Predicate.not(String::isEmpty)),
+        Optional.ofNullable(toString()).filter(Predicate.not(String::isEmpty)),
+        Optional.ofNullable(toString()).filter(Predicate.not(String::isEmpty)),
+        Optional.ofNullable(toString()).filter(Predicate.not(String::isEmpty)));
   }
 
   Optional<String> testFilterEmptyString() {
-    return Optional.of("foo").filter(s -> !s.isEmpty());
+    return Optional.of("foo").filter(not(String::isEmpty));
   }
 
   ImmutableSet<String> testJoinStrings() {