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 index 4b403a02fb..c45c0550f5 100644 --- 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 before() { + return s -> s.isEmpty(); + } + + @AfterTemplate + Predicate 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 before() { + return s -> !s.isEmpty(); + } + + @AfterTemplate + @UseImportPolicy(STATIC_IMPORT_ALWAYS) + Predicate 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 before(String str) { @AfterTemplate Optional after(String str) { - return Optional.ofNullable(str).filter(s -> !s.isEmpty()); + return Optional.ofNullable(str).filter(not(String::isEmpty)); } } @@ -76,8 +111,9 @@ Optional before(Optional optional) { } @AfterTemplate + @UseImportPolicy(STATIC_IMPORT_ALWAYS) Optional after(Optional optional) { - return optional.filter(s -> !s.isEmpty()); + return optional.filter(not(String::isEmpty)); } } diff --git a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/StringRulesTestInput.java b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/StringRulesTestInput.java index f70407c02b..420411b245 100644 --- a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/StringRulesTestInput.java +++ b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/StringRulesTestInput.java @@ -33,6 +33,14 @@ ImmutableSet 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 testStringIsNullOrEmpty() { return ImmutableSet.of( getClass().getName() == null || getClass().getName().isEmpty(), diff --git a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/StringRulesTestOutput.java b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/StringRulesTestOutput.java index 723cfe3ce3..f637936691 100644 --- a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/StringRulesTestOutput.java +++ b/error-prone-contrib/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,6 +36,14 @@ ImmutableSet testStringIsEmpty() { !"baz".isEmpty()); } + boolean testStringIsEmptyPredicate() { + return Stream.of("foo").anyMatch(String::isEmpty); + } + + boolean testStringIsNotEmptyPredicate() { + return Stream.of("foo").anyMatch(not(String::isEmpty)); + } + ImmutableSet testStringIsNullOrEmpty() { return ImmutableSet.of( Strings.isNullOrEmpty(getClass().getName()), !Strings.isNullOrEmpty(getClass().getName())); @@ -41,14 +51,14 @@ ImmutableSet testStringIsNullOrEmpty() { ImmutableSet> 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 testFilterEmptyString() { - return Optional.of("foo").filter(s -> !s.isEmpty()); + return Optional.of("foo").filter(not(String::isEmpty)); } ImmutableSet testJoinStrings() {