The suggested fix assumes that the argument to {@code orElse} does not have side effects. If
+ * it does, the suggested fix changes the program's semantics. Such fragile code must instead be
+ * refactored such that the side-effectful code does not appear accidental.
+ */
+// XXX: Consider also implementing the inverse, in which `.orElseGet(() -> someConstant)` is
+// flagged.
+// XXX: Once the `MethodReferenceUsageCheck` becomes generally usable, consider leaving the method
+// reference cleanup to that check, and express the remainder of the logic in this class using a
+// Refaster template, i.c.w. a `@Matches` constraint that implements the `requiresComputation`
+// logic.
+@AutoService(BugChecker.class)
+@BugPattern(
+ summary =
+ """
+ Prefer `Optional#orElseGet` over `Optional#orElse` if the fallback requires additional \
+ computation""",
+ linkType = NONE,
+ severity = WARNING,
+ tags = PERFORMANCE)
+public final class OptionalOrElse extends BugChecker implements MethodInvocationTreeMatcher {
+ private static final long serialVersionUID = 1L;
+ private static final Matcher OPTIONAL_OR_ELSE_METHOD =
+ instanceMethod().onExactClass(Optional.class.getCanonicalName()).namedAnyOf("orElse");
+ // XXX: Also exclude invocations of `@Placeholder`-annotated methods.
+ private static final Matcher REFASTER_METHOD =
+ staticMethod().onClass(Refaster.class.getCanonicalName());
+
+ /** Instantiates a new {@link OptionalOrElse} instance. */
+ public OptionalOrElse() {}
+
+ @Override
+ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) {
+ if (!OPTIONAL_OR_ELSE_METHOD.matches(tree, state)) {
+ return Description.NO_MATCH;
+ }
+
+ ExpressionTree argument = Iterables.getOnlyElement(tree.getArguments());
+ if (!requiresComputation(argument) || REFASTER_METHOD.matches(argument, state)) {
+ return Description.NO_MATCH;
+ }
+
+ /*
+ * We have a match. Construct the method reference or lambda expression to be passed to the
+ * replacement `#orElseGet` invocation.
+ */
+ String newArgument =
+ tryMethodReferenceConversion(argument, state)
+ .orElseGet(() -> "() -> " + SourceCode.treeToString(argument, state));
+
+ /* Construct the suggested fix, replacing the method invocation and its argument. */
+ SuggestedFix fix =
+ SuggestedFix.builder()
+ .merge(SuggestedFixes.renameMethodInvocation(tree, "orElseGet", state))
+ .replace(argument, newArgument)
+ .build();
+
+ return describeMatch(tree, fix);
+ }
+
+ /**
+ * Tells whether the given expression contains anything other than a literal or a (possibly
+ * dereferenced) variable or constant.
+ */
+ private static boolean requiresComputation(ExpressionTree tree) {
+ return !(tree instanceof IdentifierTree
+ || tree instanceof LiteralTree
+ || (tree instanceof MemberSelectTree memberSelect
+ && !requiresComputation(memberSelect.getExpression()))
+ || ASTHelpers.constValue(tree) != null);
+ }
+
+ /** Returns the nullary method reference matching the given expression, if any. */
+ private static Optional tryMethodReferenceConversion(
+ ExpressionTree tree, VisitorState state) {
+ if (!(tree instanceof MethodInvocationTree methodInvocation)) {
+ return Optional.empty();
+ }
+
+ if (!methodInvocation.getArguments().isEmpty()) {
+ return Optional.empty();
+ }
+
+ if (!(methodInvocation.getMethodSelect() instanceof MemberSelectTree memberSelect)) {
+ return Optional.empty();
+ }
+
+ if (requiresComputation(memberSelect.getExpression())) {
+ return Optional.empty();
+ }
+
+ return Optional.of(
+ SourceCode.treeToString(memberSelect.getExpression(), state)
+ + "::"
+ + (methodInvocation.getTypeArguments().isEmpty()
+ ? ""
+ : methodInvocation.getTypeArguments().stream()
+ .map(arg -> SourceCode.treeToString(arg, state))
+ .collect(joining(",", "<", ">")))
+ + memberSelect.getIdentifier());
+ }
+}
diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ClassRules.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ClassRules.java
index 2890dd1b..496b1f1f 100644
--- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ClassRules.java
+++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ClassRules.java
@@ -3,6 +3,7 @@ package tech.picnic.errorprone.refasterrules;
import com.google.errorprone.refaster.Refaster;
import com.google.errorprone.refaster.annotation.AfterTemplate;
import com.google.errorprone.refaster.annotation.BeforeTemplate;
+import java.util.function.Function;
import java.util.function.Predicate;
import tech.picnic.errorprone.refaster.annotation.OnlineDocumentation;
@@ -37,7 +38,12 @@ final class ClassRules {
}
}
- /** Prefer {@link Class#isInstance(Object)} method references over more verbose alternatives. */
+ /**
+ * Prefer {@link Class#isInstance(Object)} method references over lambda expressions that require
+ * naming a variable.
+ */
+ // XXX: Once the `ClassReferenceIsInstancePredicate` rule is dropped, rename this rule to just
+ // `ClassIsInstancePredicate`.
static final class ClassLiteralIsInstancePredicate {
@BeforeTemplate
Predicate before() {
@@ -50,7 +56,11 @@ final class ClassRules {
}
}
- /** Prefer {@link Class#isInstance(Object)} method references over more verbose alternatives. */
+ /**
+ * Prefer {@link Class#isInstance(Object)} method references over lambda expressions that require
+ * naming a variable.
+ */
+ // XXX: Drop this rule once the `MethodReferenceUsage` rule is enabled by default.
static final class ClassReferenceIsInstancePredicate {
@BeforeTemplate
Predicate before(Class clazz) {
@@ -62,4 +72,39 @@ final class ClassRules {
return clazz::isInstance;
}
}
+
+ /**
+ * Prefer {@link Class#cast(Object)} method references over lambda expressions that require naming
+ * a variable.
+ */
+ // XXX: Once the `ClassReferenceCast` rule is dropped, rename this rule to just `ClassCast`.
+ static final class ClassLiteralCast {
+ @BeforeTemplate
+ @SuppressWarnings("unchecked")
+ Function before() {
+ return t -> (S) t;
+ }
+
+ @AfterTemplate
+ Function after() {
+ return Refaster.clazz()::cast;
+ }
+ }
+
+ /**
+ * Prefer {@link Class#cast(Object)} method references over lambda expressions that require naming
+ * a variable.
+ */
+ // XXX: Drop this rule once the `MethodReferenceUsage` rule is enabled by default.
+ static final class ClassReferenceCast {
+ @BeforeTemplate
+ Function before(Class clazz) {
+ return o -> clazz.cast(o);
+ }
+
+ @AfterTemplate
+ Function after(Class clazz) {
+ return clazz::cast;
+ }
+ }
}
diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/CollectionRules.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/CollectionRules.java
index cd341e79..4ee37189 100644
--- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/CollectionRules.java
+++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/CollectionRules.java
@@ -8,7 +8,9 @@ import com.google.errorprone.refaster.Refaster;
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.NotMatches;
import java.util.ArrayList;
+import java.util.Arrays;
import java.util.Collection;
import java.util.Iterator;
import java.util.List;
@@ -21,6 +23,7 @@ import java.util.function.Consumer;
import java.util.function.IntFunction;
import java.util.stream.Stream;
import tech.picnic.errorprone.refaster.annotation.OnlineDocumentation;
+import tech.picnic.errorprone.refaster.matchers.IsRefasterAsVarargs;
/** Refaster rules related to expressions dealing with (arbitrary) collections. */
// XXX: There are other Guava `Iterables` methods that should not be called if the input is known to
@@ -39,7 +42,7 @@ final class CollectionRules {
"java:S1155" /* This violation will be rewritten. */,
"LexicographicalAnnotationAttributeListing" /* `key-*` entry must remain last. */,
"OptionalFirstCollectionElement" /* This is a more specific template. */,
- "StreamIsEmpty" /* This is a more specific template. */,
+ "StreamFindAnyIsEmpty" /* This is a more specific template. */,
"key-to-resolve-AnnotationUseStyle-and-TrailingComment-check-conflict"
})
boolean before(Collection collection) {
@@ -184,6 +187,24 @@ final class CollectionRules {
}
}
+ /** Don't unnecessarily call {@link Stream#distinct()} on an already-unique stream of elements. */
+ // XXX: This rule assumes that the `Set` relies on `Object#equals`, rather than a custom
+ // equivalence relation.
+ // XXX: Expressions that drop or reorder elements from the stream, such as `.filter`, `.skip` and
+ // `sorted`, can similarly be simplified. Covering all cases is better done using an Error Prone
+ // check.
+ static final class SetStream {
+ @BeforeTemplate
+ Stream before(Set set) {
+ return set.stream().distinct();
+ }
+
+ @AfterTemplate
+ Stream after(Set set) {
+ return set.stream();
+ }
+ }
+
/** Prefer {@link ArrayList#ArrayList(Collection)} over the Guava alternative. */
@SuppressWarnings(
"NonApiType" /* Matching against `List` would unnecessarily constrain the rule. */)
@@ -276,6 +297,23 @@ final class CollectionRules {
}
}
+ /** Prefer {@link Arrays#asList(Object[])} over more contrived alternatives. */
+ // XXX: Consider moving this rule to `ImmutableListRules` and having it suggest
+ // `ImmutableList#copyOf`. That would retain immutability, at the cost of no longer handling
+ // `null`s.
+ static final class ArraysAsList {
+ // XXX: This expression produces an unmodifiable list, while the alternative doesn't.
+ @BeforeTemplate
+ List before(@NotMatches(IsRefasterAsVarargs.class) T[] array) {
+ return Arrays.stream(array).toList();
+ }
+
+ @AfterTemplate
+ List after(T[] array) {
+ return Arrays.asList(array);
+ }
+ }
+
/** Prefer calling {@link Collection#toArray()} over more contrived alternatives. */
static final class CollectionToArray {
@BeforeTemplate
diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ComparatorRules.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ComparatorRules.java
index c6d537c0..932e5e90 100644
--- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ComparatorRules.java
+++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ComparatorRules.java
@@ -16,8 +16,11 @@ import com.google.common.collect.ImmutableSet;
import com.google.errorprone.annotations.CanIgnoreReturnValue;
import com.google.errorprone.refaster.Refaster;
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.Matches;
+import com.google.errorprone.refaster.annotation.MayOptionallyUse;
+import com.google.errorprone.refaster.annotation.Placeholder;
import com.google.errorprone.refaster.annotation.Repeated;
import com.google.errorprone.refaster.annotation.UseImportPolicy;
import java.util.Arrays;
@@ -92,6 +95,24 @@ final class ComparatorRules {
}
}
+ /** Don't explicitly compare enums by their ordinal. */
+ abstract static class ComparingEnum, T> {
+ @Placeholder(allowsIdentity = true)
+ abstract E toEnumFunction(@MayOptionallyUse T value);
+
+ @BeforeTemplate
+ @SuppressWarnings("EnumOrdinal" /* This violation will be rewritten. */)
+ Comparator before() {
+ return comparingInt(v -> toEnumFunction(v).ordinal());
+ }
+
+ @AfterTemplate
+ @UseImportPolicy(STATIC_IMPORT_ALWAYS)
+ Comparator after() {
+ return comparing(v -> toEnumFunction(v));
+ }
+ }
+
/** Don't explicitly create {@link Comparator}s unnecessarily. */
static final class ThenComparing> {
@BeforeTemplate
@@ -269,7 +290,7 @@ final class ComparatorRules {
static final class MinOfPairCustomOrder {
@BeforeTemplate
@SuppressWarnings("java:S1067" /* The conditional operators are independent. */)
- T before(T value1, T value2, Comparator cmp) {
+ T before(T value1, T value2, Comparator cmp) {
return Refaster.anyOf(
cmp.compare(value1, value2) <= 0 ? value1 : value2,
cmp.compare(value1, value2) > 0 ? value2 : value1,
@@ -284,7 +305,7 @@ final class ComparatorRules {
}
@AfterTemplate
- T after(T value1, T value2, Comparator cmp) {
+ T after(T value1, T value2, Comparator cmp) {
return Comparators.min(value1, value2, cmp);
}
}
@@ -336,7 +357,7 @@ final class ComparatorRules {
static final class MaxOfPairCustomOrder {
@BeforeTemplate
@SuppressWarnings("java:S1067" /* The conditional operators are independent. */)
- T before(T value1, T value2, Comparator cmp) {
+ T before(T value1, T value2, Comparator cmp) {
return Refaster.anyOf(
cmp.compare(value1, value2) >= 0 ? value1 : value2,
cmp.compare(value1, value2) < 0 ? value2 : value1,
@@ -351,7 +372,7 @@ final class ComparatorRules {
}
@AfterTemplate
- T after(T value1, T value2, Comparator cmp) {
+ T after(T value1, T value2, Comparator cmp) {
return Comparators.max(value1, value2, cmp);
}
}
@@ -419,4 +440,34 @@ final class ComparatorRules {
return maxBy(naturalOrder());
}
}
+
+ /** Don't explicitly compare enums by their ordinal. */
+ static final class IsLessThan> {
+ @BeforeTemplate
+ @SuppressWarnings("EnumOrdinal" /* This violation will be rewritten. */)
+ boolean before(E value1, E value2) {
+ return value1.ordinal() < value2.ordinal();
+ }
+
+ @AfterTemplate
+ @AlsoNegation
+ boolean after(E value1, E value2) {
+ return value1.compareTo(value2) < 0;
+ }
+ }
+
+ /** Don't explicitly compare enums by their ordinal. */
+ static final class IsLessThanOrEqualTo> {
+ @BeforeTemplate
+ @SuppressWarnings("EnumOrdinal" /* This violation will be rewritten. */)
+ boolean before(E value1, E value2) {
+ return value1.ordinal() <= value2.ordinal();
+ }
+
+ @AfterTemplate
+ @AlsoNegation
+ boolean after(E value1, E value2) {
+ return value1.compareTo(value2) <= 0;
+ }
+ }
}
diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/EqualityRules.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/EqualityRules.java
index e47918d4..dcce6aa0 100644
--- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/EqualityRules.java
+++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/EqualityRules.java
@@ -1,5 +1,6 @@
package tech.picnic.errorprone.refasterrules;
+import static java.util.function.Predicate.isEqual;
import static java.util.function.Predicate.not;
import com.google.errorprone.annotations.CanIgnoreReturnValue;
@@ -19,9 +20,9 @@ import tech.picnic.errorprone.refaster.annotation.OnlineDocumentation;
final class EqualityRules {
private EqualityRules() {}
- /** Prefer reference-based quality for enums. */
- // Primitive value comparisons are not listed, because Error Prone flags those out of the box.
- static final class PrimitiveOrReferenceEquality> {
+ /** Prefer reference-based equality for enums. */
+ // Primitive value comparisons are not matched, because Error Prone flags those out of the box.
+ static final class EnumReferenceEquality> {
/**
* Enums can be compared by reference. It is safe to do so even in the face of refactorings,
* because if the type is ever converted to a non-enum, then Error-Prone will complain about any
@@ -30,8 +31,9 @@ final class EqualityRules {
// XXX: This Refaster rule is the topic of https://github.com/google/error-prone/issues/559. We
// work around the issue by selecting the "largest replacements". See the `Refaster` check.
@BeforeTemplate
+ @SuppressWarnings("EnumOrdinal" /* This violation will be rewritten. */)
boolean before(T a, T b) {
- return Refaster.anyOf(a.equals(b), Objects.equals(a, b));
+ return Refaster.anyOf(a.equals(b), Objects.equals(a, b), a.ordinal() == b.ordinal());
}
@AfterTemplate
@@ -42,6 +44,20 @@ final class EqualityRules {
}
}
+ /** Prefer reference-based equality for enums. */
+ static final class EnumReferenceEqualityLambda> {
+ @BeforeTemplate
+ Predicate before(T e) {
+ return Refaster.anyOf(isEqual(e), e::equals);
+ }
+
+ @AfterTemplate
+ @SuppressWarnings("java:S1698" /* Reference comparison is valid for enums. */)
+ Predicate after(T e) {
+ return v -> v == e;
+ }
+ }
+
/** Prefer {@link Object#equals(Object)} over the equivalent lambda function. */
// 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.
diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/FileRules.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/FileRules.java
index bc230e0a..3a45fe1e 100644
--- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/FileRules.java
+++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/FileRules.java
@@ -2,12 +2,15 @@ package tech.picnic.errorprone.refasterrules;
import static java.nio.charset.StandardCharsets.UTF_8;
+import com.google.errorprone.refaster.Refaster;
import com.google.errorprone.refaster.annotation.AfterTemplate;
import com.google.errorprone.refaster.annotation.BeforeTemplate;
+import java.io.File;
import java.io.IOException;
import java.nio.charset.Charset;
import java.nio.file.Files;
import java.nio.file.Path;
+import java.nio.file.attribute.FileAttribute;
import tech.picnic.errorprone.refaster.annotation.OnlineDocumentation;
/** Refaster rules related to expressions dealing with files. */
@@ -40,4 +43,24 @@ final class FileRules {
return Files.readString(path);
}
}
+
+ /**
+ * Prefer {@link Files#createTempFile(String, String, FileAttribute[])} over alternatives that
+ * create files with more liberal permissions.
+ */
+ static final class FilesCreateTempFileToFile {
+ @BeforeTemplate
+ @SuppressWarnings("java:S5443" /* This violation will be rewritten. */)
+ File before(String prefix, String suffix) throws IOException {
+ return Refaster.anyOf(
+ File.createTempFile(prefix, suffix), File.createTempFile(prefix, suffix, null));
+ }
+
+ @AfterTemplate
+ @SuppressWarnings(
+ "java:S5443" /* On POSIX systems the file will only have user read-write permissions. */)
+ File after(String prefix, String suffix) throws IOException {
+ return Files.createTempFile(prefix, suffix).toFile();
+ }
+ }
}
diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/JUnitToAssertJRules.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/JUnitToAssertJRules.java
index 23c3bc16..e7915237 100644
--- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/JUnitToAssertJRules.java
+++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/JUnitToAssertJRules.java
@@ -302,16 +302,22 @@ import tech.picnic.errorprone.refaster.annotation.TypeMigration;
final class JUnitToAssertJRules {
private JUnitToAssertJRules() {}
- static final class ThrowNewAssertionError {
+ static final class Fail {
@BeforeTemplate
- void before() {
- Assertions.fail();
+ T before() {
+ return Assertions.fail();
}
+ // XXX: Add `@UseImportPolicy(STATIC_IMPORT_ALWAYS)` once
+ // https://github.com/google/error-prone/pull/3584 is resolved. Until that time, statically
+ // importing AssertJ's `fail` is likely to clash with an existing static import of JUnit's
+ // `fail`. Note that combining Error Prone's `RemoveUnusedImports` and
+ // `UnnecessarilyFullyQualified` checks and our `StaticImport` check will anyway cause the
+ // method to be imported statically if possible; just in a less efficient manner.
@AfterTemplate
@DoNotCall
- void after() {
- throw new AssertionError();
+ T after() {
+ return fail();
}
}
@@ -321,12 +327,7 @@ final class JUnitToAssertJRules {
return Assertions.fail(message);
}
- // XXX: Add `@UseImportPolicy(STATIC_IMPORT_ALWAYS)` once
- // https://github.com/google/error-prone/pull/3584 is resolved. Until that time, statically
- // importing AssertJ's `fail` is likely to clash with an existing static import of JUnit's
- // `fail`. Note that combining Error Prone's `RemoveUnusedImports` and
- // `UnnecessarilyFullyQualified` checks and our `StaticImport` check will anyway cause the
- // method to be imported statically if possible; just in a less efficient manner.
+ // XXX: Add `@UseImportPolicy(STATIC_IMPORT_ALWAYS)`. See `Fail` comment.
@AfterTemplate
T after(String message) {
return fail(message);
@@ -339,28 +340,24 @@ final class JUnitToAssertJRules {
return Assertions.fail(message, throwable);
}
- // XXX: Add `@UseImportPolicy(STATIC_IMPORT_ALWAYS)` once
- // https://github.com/google/error-prone/pull/3584 is resolved. Until that time, statically
- // importing AssertJ's `fail` is likely to clash with an existing static import of JUnit's
- // `fail`. Note that combining Error Prone's `RemoveUnusedImports` and
- // `UnnecessarilyFullyQualified` checks and our `StaticImport` check will anyway cause the
- // method to be imported statically if possible; just in a less efficient manner.
+ // XXX: Add `@UseImportPolicy(STATIC_IMPORT_ALWAYS)`. See `Fail` comment.
@AfterTemplate
T after(String message, Throwable throwable) {
return fail(message, throwable);
}
}
- static final class FailWithThrowable {
+ static final class FailWithThrowable {
@BeforeTemplate
- void before(Throwable throwable) {
- Assertions.fail(throwable);
+ T before(Throwable throwable) {
+ return Assertions.fail(throwable);
}
+ // XXX: Add `@UseImportPolicy(STATIC_IMPORT_ALWAYS)`. See `Fail` comment.
@AfterTemplate
@DoNotCall
- void after(Throwable throwable) {
- throw new AssertionError(throwable);
+ T after(Throwable throwable) {
+ return fail(throwable);
}
}
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 1491b18e..9f73ae38 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
@@ -27,6 +27,19 @@ import tech.picnic.errorprone.refaster.matchers.IsLikelyTrivialComputation;
final class OptionalRules {
private OptionalRules() {}
+ /** Prefer {@link Optional#empty()} over the more contrived alternative. */
+ static final class OptionalEmpty {
+ @BeforeTemplate
+ Optional before() {
+ return Optional.ofNullable(null);
+ }
+
+ @AfterTemplate
+ Optional after() {
+ return Optional.empty();
+ }
+ }
+
static final class OptionalOfNullable {
// XXX: Refaster should be smart enough to also rewrite occurrences in which there are
// parentheses around the null check, but that's currently not the case. Try to fix that.
@@ -360,7 +373,7 @@ final class OptionalRules {
/** Prefer {@link Optional#or(Supplier)} over more verbose alternatives. */
static final class OptionalOrOtherOptional {
@BeforeTemplate
- @SuppressWarnings("NestedOptionals" /* Auto-fix for the `NestedOptionals` check. */)
+ @SuppressWarnings("NestedOptionals")
Optional before(Optional optional1, Optional optional2) {
// XXX: Note that rewriting the first and third variant will change the code's behavior if
// `optional2` has side-effects.
@@ -386,9 +399,13 @@ final class OptionalRules {
*/
static final class OptionalIdentity {
@BeforeTemplate
+ @SuppressWarnings("NestedOptionals")
Optional before(Optional optional, Comparator comparator) {
return Refaster.anyOf(
optional.or(Refaster.anyOf(() -> Optional.empty(), Optional::empty)),
+ optional
+ .map(Optional::of)
+ .orElseGet(Refaster.anyOf(() -> Optional.empty(), Optional::empty)),
optional.stream().findFirst(),
optional.stream().findAny(),
optional.stream().min(comparator),
@@ -442,9 +459,7 @@ final class OptionalRules {
static final class OptionalStream {
@BeforeTemplate
Stream before(Optional optional) {
- return Refaster.anyOf(
- optional.map(Stream::of).orElse(Stream.empty()),
- optional.map(Stream::of).orElseGet(Stream::empty));
+ return optional.map(Stream::of).orElseGet(Stream::empty);
}
@AfterTemplate
diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ReactorRules.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ReactorRules.java
index 57cb55a6..f29c0029 100644
--- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ReactorRules.java
+++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ReactorRules.java
@@ -3,7 +3,6 @@ package tech.picnic.errorprone.refasterrules;
import static com.google.common.collect.ImmutableList.toImmutableList;
import static com.google.common.collect.ImmutableSet.toImmutableSet;
import static com.google.common.collect.MoreCollectors.toOptional;
-import static com.google.errorprone.BugPattern.SeverityLevel.WARNING;
import static com.google.errorprone.refaster.ImportPolicy.STATIC_IMPORT_ALWAYS;
import static java.util.Comparator.naturalOrder;
import static java.util.Comparator.reverseOrder;
@@ -34,6 +33,7 @@ import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.concurrent.Callable;
+import java.util.concurrent.CompletableFuture;
import java.util.function.BiConsumer;
import java.util.function.BiFunction;
import java.util.function.Consumer;
@@ -41,6 +41,7 @@ import java.util.function.Function;
import java.util.function.Predicate;
import java.util.function.Supplier;
import java.util.stream.Collector;
+import java.util.stream.Stream;
import org.jspecify.annotations.Nullable;
import org.reactivestreams.Publisher;
import reactor.core.publisher.Flux;
@@ -52,7 +53,6 @@ import reactor.util.context.Context;
import reactor.util.function.Tuple2;
import tech.picnic.errorprone.refaster.annotation.Description;
import tech.picnic.errorprone.refaster.annotation.OnlineDocumentation;
-import tech.picnic.errorprone.refaster.annotation.Severity;
import tech.picnic.errorprone.refaster.matchers.IsEmpty;
import tech.picnic.errorprone.refaster.matchers.IsIdentityOperation;
import tech.picnic.errorprone.refaster.matchers.ThrowsCheckedException;
@@ -380,30 +380,23 @@ final class ReactorRules {
}
/**
- * Prefer {@link Flux#take(long, boolean)} over {@link Flux#take(long)}.
+ * Prefer {@link Flux#take(long)} over {@link Flux#take(long, boolean)} where relevant.
*
*
In Reactor versions prior to 3.5.0, {@code Flux#take(long)} makes an unbounded request
- * upstream, and is equivalent to {@code Flux#take(long, false)}. In 3.5.0, the behavior of {@code
- * Flux#take(long)} will change to that of {@code Flux#take(long, true)}.
- *
- *
The intent with this Refaster rule is to get the new behavior before upgrading to Reactor
- * 3.5.0.
+ * upstream, and is equivalent to {@code Flux#take(long, false)}. From version 3.5.0 onwards, the
+ * behavior of {@code Flux#take(long)} instead matches {@code Flux#take(long, true)}.
*/
- // XXX: Drop this rule some time after upgrading to Reactor 3.6.0, or introduce a way to apply
- // this rule only when an older version of Reactor is on the classpath.
- // XXX: Once Reactor 3.6.0 is out, introduce a rule that rewrites code in the opposite direction.
@Description(
- "Prior to Reactor 3.5.0, `take(n)` requests and unbounded number of elements upstream.")
- @Severity(WARNING)
+ "From Reactor 3.5.0 onwards, `take(n)` no longer requests an unbounded number of elements upstream.")
static final class FluxTake {
@BeforeTemplate
Flux before(Flux flux, long n) {
- return flux.take(n);
+ return flux.take(n, /* limitRequest= */ true);
}
@AfterTemplate
Flux after(Flux flux, long n) {
- return flux.take(n, /* limitRequest= */ true);
+ return flux.take(n);
}
}
@@ -489,15 +482,20 @@ final class ReactorRules {
}
/** Prefer {@link Flux#just(Object)} over more contrived alternatives. */
- static final class FluxJust {
+ static final class FluxJust {
@BeforeTemplate
- Flux before(int start) {
- return Flux.range(start, 1);
+ Flux before(int value) {
+ return Flux.range(value, 1);
+ }
+
+ @BeforeTemplate
+ Flux before(T value) {
+ return Mono.just(value).repeat().take(1);
}
@AfterTemplate
- Flux after(int start) {
- return Flux.just(start);
+ Flux after(T value) {
+ return Flux.just(value);
}
}
@@ -566,6 +564,7 @@ final class ReactorRules {
@Matches(IsIdentityOperation.class)
Function> identityOperation) {
return Refaster.anyOf(
+ flux.concatMap(function, 0),
flux.flatMap(function, 1),
flux.flatMapSequential(function, 1),
flux.map(function).concatMap(identityOperation));
@@ -1206,10 +1205,17 @@ final class ReactorRules {
}
/** Prefer {@link Flux#fromIterable(Iterable)} over less efficient alternatives. */
+ // XXX: Once the `FluxFromStreamSupplier` rule is constrained using
+ // `@NotMatches(IsIdentityOperation.class)`, this rule should also cover
+ // `Flux.fromStream(collection.stream())`.
static final class FluxFromIterable {
+ // XXX: Once the `MethodReferenceUsage` check is generally enabled, drop the second
+ // `Refaster.anyOf` variant.
@BeforeTemplate
Flux before(Collection collection) {
- return Flux.fromStream(collection.stream());
+ return Flux.fromStream(
+ Refaster.>>anyOf(
+ collection::stream, () -> collection.stream()));
}
@AfterTemplate
@@ -1912,4 +1918,60 @@ final class ReactorRules {
return step.verifyTimeout(duration);
}
}
+
+ /**
+ * Prefer {@link Mono#fromFuture(Supplier)} over {@link Mono#fromFuture(CompletableFuture)}, as
+ * the former may defer initiation of the asynchronous computation until subscription.
+ */
+ static final class MonoFromFutureSupplier {
+ // XXX: Constrain the `future` parameter using `@NotMatches(IsIdentityOperation.class)` once
+ // `IsIdentityOperation` no longer matches nullary method invocations.
+ @BeforeTemplate
+ Mono before(CompletableFuture future) {
+ return Mono.fromFuture(future);
+ }
+
+ @AfterTemplate
+ Mono after(CompletableFuture future) {
+ return Mono.fromFuture(() -> future);
+ }
+ }
+
+ /**
+ * Prefer {@link Mono#fromFuture(Supplier, boolean)} over {@link
+ * Mono#fromFuture(CompletableFuture, boolean)}, as the former may defer initiation of the
+ * asynchronous computation until subscription.
+ */
+ static final class MonoFromFutureSupplierBoolean {
+ // XXX: Constrain the `future` parameter using `@NotMatches(IsIdentityOperation.class)` once
+ // `IsIdentityOperation` no longer matches nullary method invocations.
+ @BeforeTemplate
+ Mono before(CompletableFuture future, boolean suppressCancel) {
+ return Mono.fromFuture(future, suppressCancel);
+ }
+
+ @AfterTemplate
+ Mono after(CompletableFuture future, boolean suppressCancel) {
+ return Mono.fromFuture(() -> future, suppressCancel);
+ }
+ }
+
+ /**
+ * Prefer {@link Flux#fromStream(Supplier)} over {@link Flux#fromStream(Stream)}, as the former
+ * yields a {@link Flux} that is more likely to behave as expected when subscribed to more than
+ * once.
+ */
+ static final class FluxFromStreamSupplier {
+ // XXX: Constrain the `stream` parameter using `@NotMatches(IsIdentityOperation.class)` once
+ // `IsIdentityOperation` no longer matches nullary method invocations.
+ @BeforeTemplate
+ Flux before(Stream stream) {
+ return Flux.fromStream(stream);
+ }
+
+ @AfterTemplate
+ Flux after(Stream stream) {
+ return Flux.fromStream(() -> stream);
+ }
+ }
}
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 5ab7f9de..e7bea741 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
@@ -25,6 +25,7 @@ import com.google.common.collect.Streams;
import com.google.errorprone.annotations.CanIgnoreReturnValue;
import com.google.errorprone.refaster.Refaster;
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.Matches;
import com.google.errorprone.refaster.annotation.MayOptionallyUse;
@@ -256,7 +257,7 @@ final class StreamRules {
// XXX: This rule assumes that any matched `Collector` does not perform any filtering.
// (Perhaps we could add a `@Matches` guard that validates that the collector expression does not
// contain a `Collectors#filtering` call. That'd still not be 100% accurate, though.)
- static final class StreamIsEmpty, M extends Map> {
+ static final class StreamFindAnyIsEmpty, M extends Map> {
@BeforeTemplate
boolean before(Stream stream, Collector collector) {
return Refaster.anyOf(
@@ -274,20 +275,20 @@ final class StreamRules {
}
@AfterTemplate
+ @AlsoNegation
boolean after(Stream stream) {
return stream.findAny().isEmpty();
}
}
- /** In order to test whether a stream has any element, simply try to find one. */
- static final class StreamIsNotEmpty {
+ /**
+ * Prefer {@link Stream#findAny()} over {@link Stream#findFirst()} if one only cares whether the
+ * stream is nonempty.
+ */
+ static final class StreamFindAnyIsPresent {
@BeforeTemplate
boolean before(Stream stream) {
- return Refaster.anyOf(
- stream.count() != 0,
- stream.count() > 0,
- stream.count() >= 1,
- stream.findFirst().isPresent());
+ return stream.findFirst().isPresent();
}
@AfterTemplate
diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/TestNGToAssertJRules.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/TestNGToAssertJRules.java
index e9662898..987812ac 100644
--- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/TestNGToAssertJRules.java
+++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/TestNGToAssertJRules.java
@@ -161,8 +161,9 @@ final class TestNGToAssertJRules {
@AfterTemplate
@DoNotCall
+ @UseImportPolicy(STATIC_IMPORT_ALWAYS)
void after() {
- throw new AssertionError();
+ fail();
}
}
diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/OptionalOrElseTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/OptionalOrElseTest.java
new file mode 100644
index 00000000..620a532a
--- /dev/null
+++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/OptionalOrElseTest.java
@@ -0,0 +1,135 @@
+package tech.picnic.errorprone.bugpatterns;
+
+import com.google.errorprone.BugCheckerRefactoringTestHelper;
+import com.google.errorprone.BugCheckerRefactoringTestHelper.TestMode;
+import com.google.errorprone.CompilationTestHelper;
+import org.junit.jupiter.api.Test;
+
+final class OptionalOrElseTest {
+ @Test
+ void identification() {
+ CompilationTestHelper.newInstance(OptionalOrElse.class, getClass())
+ .addSourceLines(
+ "A.java",
+ "import com.google.errorprone.refaster.Refaster;",
+ "import java.util.Optional;",
+ "",
+ "class A {",
+ " private final Optional