excludedAnnotations(ErrorProneFlags flags) {
+ return Sets.union(BLACKLISTED_ANNOTATIONS, Flags.getSet(flags, EXCLUDED_ANNOTATIONS_FLAG))
+ .immutableCopy();
}
}
diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/NonStaticImport.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/NonStaticImport.java
index adbd88f7..eba7253b 100644
--- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/NonStaticImport.java
+++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/NonStaticImport.java
@@ -210,7 +210,7 @@ public final class NonStaticImport extends BugChecker implements CompilationUnit
}
}
- return super.visitIdentifier(node, unused);
+ return super.visitIdentifier(node, null);
}
}.scan(tree, null);
}
diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/OptionalOrElseGet.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/OptionalOrElseGet.java
new file mode 100644
index 00000000..692ee60e
--- /dev/null
+++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/OptionalOrElseGet.java
@@ -0,0 +1,123 @@
+package tech.picnic.errorprone.bugpatterns;
+
+import static com.google.errorprone.BugPattern.LinkType.NONE;
+import static com.google.errorprone.BugPattern.SeverityLevel.WARNING;
+import static com.google.errorprone.BugPattern.StandardTags.PERFORMANCE;
+import static com.google.errorprone.matchers.Matchers.instanceMethod;
+import static com.google.errorprone.matchers.Matchers.staticMethod;
+import static java.util.stream.Collectors.joining;
+
+import com.google.auto.service.AutoService;
+import com.google.common.collect.Iterables;
+import com.google.errorprone.BugPattern;
+import com.google.errorprone.VisitorState;
+import com.google.errorprone.bugpatterns.BugChecker;
+import com.google.errorprone.bugpatterns.BugChecker.MethodInvocationTreeMatcher;
+import com.google.errorprone.fixes.SuggestedFix;
+import com.google.errorprone.fixes.SuggestedFixes;
+import com.google.errorprone.matchers.Description;
+import com.google.errorprone.matchers.Matcher;
+import com.google.errorprone.refaster.Refaster;
+import com.sun.source.tree.ExpressionTree;
+import com.sun.source.tree.MemberSelectTree;
+import com.sun.source.tree.MethodInvocationTree;
+import java.util.Optional;
+import java.util.function.Supplier;
+import tech.picnic.errorprone.refaster.matchers.RequiresComputation;
+import tech.picnic.errorprone.utils.SourceCode;
+
+/**
+ * A {@link BugChecker} that flags arguments to {@link Optional#orElse(Object)} that should be
+ * deferred using {@link Optional#orElseGet(Supplier)}.
+ *
+ * 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: This rule may introduce a compilation error: the `value` expression may reference a
+// non-effectively final variable, which is not allowed in the replacement lambda expression.
+// Review whether a `@Matcher` can be used to avoid this.
+// XXX: Once the `MethodReferenceUsageCheck` bug checker 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 `@NotMatches(RequiresComputation.class)` constraint.
+@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 OptionalOrElseGet extends BugChecker implements MethodInvocationTreeMatcher {
+ private static final long serialVersionUID = 1L;
+ private static final Matcher REQUIRES_COMPUTATION = new RequiresComputation();
+ 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 OptionalOrElseGet} instance. */
+ public OptionalOrElseGet() {}
+
+ @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 (!REQUIRES_COMPUTATION.matches(argument, state)
+ || 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);
+ }
+
+ /** 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 (REQUIRES_COMPUTATION.matches(memberSelect.getExpression(), state)) {
+ 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/bugpatterns/RedundantStringConversion.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/RedundantStringConversion.java
index bc6dadbd..689996a0 100644
--- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/RedundantStringConversion.java
+++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/RedundantStringConversion.java
@@ -378,11 +378,11 @@ public final class RedundantStringConversion extends BugChecker
private static Matcher createConversionMethodMatcher(
ErrorProneFlags flags) {
- // XXX: ErrorProneFlags#getList splits by comma, but method signatures may also contain commas.
- // For this class methods accepting more than one argument are not valid, but still: not nice.
+ // XXX: `Flags#getSet` splits by comma, but method signatures may also contain commas. For this
+ // class methods accepting more than one argument are not valid, but still: not nice.
return anyOf(
WELL_KNOWN_STRING_CONVERSION_METHODS,
new MethodMatcherFactory()
- .create(Flags.getList(flags, EXTRA_STRING_CONVERSION_METHODS_FLAG)));
+ .create(Flags.getSet(flags, EXTRA_STRING_CONVERSION_METHODS_FLAG)));
}
}
diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/RequestParamType.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/RequestParamType.java
index ee736da1..3d2aef0c 100644
--- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/RequestParamType.java
+++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/RequestParamType.java
@@ -15,8 +15,8 @@ import static tech.picnic.errorprone.utils.Documentation.BUG_PATTERNS_BASE_URL;
import com.google.auto.service.AutoService;
import com.google.common.collect.ImmutableCollection;
-import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableSet;
import com.google.errorprone.BugPattern;
import com.google.errorprone.ErrorProneFlags;
import com.google.errorprone.VisitorState;
@@ -74,10 +74,10 @@ public final class RequestParamType extends BugChecker implements VariableTreeMa
return allOf(
annotations(AT_LEAST_ONE, isType("org.springframework.web.bind.annotation.RequestParam")),
anyOf(isSubtypeOf(ImmutableCollection.class), isSubtypeOf(ImmutableMap.class)),
- not(isSubtypeOfAny(Flags.getList(flags, SUPPORTED_CUSTOM_TYPES_FLAG))));
+ not(isSubtypeOfAny(Flags.getSet(flags, SUPPORTED_CUSTOM_TYPES_FLAG))));
}
- private static Matcher isSubtypeOfAny(ImmutableList inclusions) {
+ private static Matcher isSubtypeOfAny(ImmutableSet inclusions) {
return anyOf(
inclusions.stream()
.map(inclusion -> isSubtypeOf(Suppliers.typeFromString(inclusion)))
diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/Slf4jLoggerDeclaration.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/Slf4jLoggerDeclaration.java
new file mode 100644
index 00000000..84358607
--- /dev/null
+++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/Slf4jLoggerDeclaration.java
@@ -0,0 +1,185 @@
+package tech.picnic.errorprone.bugpatterns;
+
+import static com.google.common.base.Verify.verify;
+import static com.google.errorprone.BugPattern.LinkType.CUSTOM;
+import static com.google.errorprone.BugPattern.SeverityLevel.WARNING;
+import static com.google.errorprone.BugPattern.StandardTags.STYLE;
+import static com.google.errorprone.matchers.Matchers.allOf;
+import static com.google.errorprone.matchers.Matchers.classLiteral;
+import static com.google.errorprone.matchers.Matchers.instanceMethod;
+import static com.google.errorprone.matchers.Matchers.staticMethod;
+import static com.google.errorprone.matchers.Matchers.toType;
+import static java.util.Objects.requireNonNull;
+import static tech.picnic.errorprone.utils.Documentation.BUG_PATTERNS_BASE_URL;
+
+import com.google.auto.service.AutoService;
+import com.google.common.base.CaseFormat;
+import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.Iterables;
+import com.google.common.collect.Sets;
+import com.google.errorprone.BugPattern;
+import com.google.errorprone.ErrorProneFlags;
+import com.google.errorprone.VisitorState;
+import com.google.errorprone.bugpatterns.BugChecker;
+import com.google.errorprone.bugpatterns.BugChecker.VariableTreeMatcher;
+import com.google.errorprone.fixes.SuggestedFix;
+import com.google.errorprone.fixes.SuggestedFixes;
+import com.google.errorprone.matchers.Description;
+import com.google.errorprone.matchers.Matcher;
+import com.google.errorprone.util.ASTHelpers;
+import com.sun.source.tree.ClassTree;
+import com.sun.source.tree.ExpressionTree;
+import com.sun.source.tree.MethodInvocationTree;
+import com.sun.source.tree.ModifiersTree;
+import com.sun.source.tree.Tree.Kind;
+import com.sun.source.tree.VariableTree;
+import com.sun.tools.javac.code.Symbol;
+import java.util.EnumSet;
+import javax.inject.Inject;
+import javax.lang.model.element.Modifier;
+import tech.picnic.errorprone.utils.MoreASTHelpers;
+
+/** A {@link BugChecker} that flags non-canonical SLF4J logger declarations. */
+@AutoService(BugChecker.class)
+@BugPattern(
+ summary = "SLF4J logger declarations should follow established best-practices",
+ link = BUG_PATTERNS_BASE_URL + "Slf4jLoggerDeclaration",
+ linkType = CUSTOM,
+ severity = WARNING,
+ tags = STYLE)
+@SuppressWarnings("java:S2160" /* Super class equality definition suffices. */)
+public final class Slf4jLoggerDeclaration extends BugChecker implements VariableTreeMatcher {
+ private static final long serialVersionUID = 1L;
+ private static final Matcher IS_GET_LOGGER =
+ staticMethod().onDescendantOf("org.slf4j.LoggerFactory").named("getLogger");
+ private static final String CANONICAL_STATIC_LOGGER_NAME_FLAG =
+ "Slf4jLogDeclaration:CanonicalStaticLoggerName";
+ private static final String DEFAULT_CANONICAL_LOGGER_NAME = "LOG";
+ private static final Matcher IS_STATIC_ENCLOSING_CLASS_REFERENCE =
+ classLiteral(Slf4jLoggerDeclaration::isEnclosingClassReference);
+ private static final Matcher IS_DYNAMIC_ENCLOSING_CLASS_REFERENCE =
+ toType(
+ MethodInvocationTree.class,
+ allOf(
+ instanceMethod().anyClass().named("getClass").withNoParameters(),
+ Slf4jLoggerDeclaration::getClassReceiverIsEnclosingClassInstance));
+ private static final ImmutableSet INSTANCE_DECLARATION_MODIFIERS =
+ Sets.immutableEnumSet(Modifier.PRIVATE, Modifier.FINAL);
+ private static final ImmutableSet STATIC_DECLARATION_MODIFIERS =
+ Sets.immutableEnumSet(Modifier.PRIVATE, Modifier.STATIC, Modifier.FINAL);
+
+ private final String canonicalStaticFieldName;
+ private final String canonicalInstanceFieldName;
+
+ /** Instantiates a default {@link Slf4jLoggerDeclaration} instance. */
+ public Slf4jLoggerDeclaration() {
+ this(ErrorProneFlags.empty());
+ }
+
+ /**
+ * Instantiates a customized {@link Slf4jLoggerDeclaration}.
+ *
+ * @param flags Any provided command line flags.
+ */
+ @Inject
+ Slf4jLoggerDeclaration(ErrorProneFlags flags) {
+ canonicalStaticFieldName =
+ flags.get(CANONICAL_STATIC_LOGGER_NAME_FLAG).orElse(DEFAULT_CANONICAL_LOGGER_NAME);
+ canonicalInstanceFieldName =
+ CaseFormat.UPPER_UNDERSCORE.to(CaseFormat.LOWER_CAMEL, canonicalStaticFieldName);
+ }
+
+ @Override
+ public Description matchVariable(VariableTree tree, VisitorState state) {
+ ExpressionTree initializer = tree.getInitializer();
+ if (!IS_GET_LOGGER.matches(initializer, state)) {
+ return Description.NO_MATCH;
+ }
+
+ ClassTree clazz = getEnclosingClass(state);
+ ExpressionTree factoryArg =
+ Iterables.getOnlyElement(((MethodInvocationTree) initializer).getArguments());
+
+ SuggestedFix.Builder fix = SuggestedFix.builder();
+
+ if (clazz.getModifiers().getFlags().contains(Modifier.ABSTRACT)
+ && IS_DYNAMIC_ENCLOSING_CLASS_REFERENCE.matches(factoryArg, state)) {
+ /*
+ * While generally we prefer `Logger` declarations to be static and named after their
+ * enclosing class, we allow one exception: loggers in abstract classes with a name derived
+ * from `getClass()`.
+ */
+ suggestModifiers(tree, INSTANCE_DECLARATION_MODIFIERS, fix, state);
+ suggestRename(tree, canonicalInstanceFieldName, fix, state);
+ } else {
+ suggestModifiers(
+ tree,
+ clazz.getKind() == Kind.INTERFACE ? ImmutableSet.of() : STATIC_DECLARATION_MODIFIERS,
+ fix,
+ state);
+ suggestRename(tree, canonicalStaticFieldName, fix, state);
+
+ if (!MoreASTHelpers.isStringTyped(factoryArg, state)
+ && !IS_STATIC_ENCLOSING_CLASS_REFERENCE.matches(factoryArg, state)) {
+ /*
+ * Loggers with a custom string name are generally "special", but those with a name derived
+ * from a class other than the one that encloses it are likely in error.
+ */
+ fix.merge(SuggestedFix.replace(factoryArg, clazz.getSimpleName() + ".class"));
+ }
+ }
+
+ return fix.isEmpty() ? Description.NO_MATCH : describeMatch(tree, fix.build());
+ }
+
+ private static void suggestModifiers(
+ VariableTree tree,
+ ImmutableSet modifiers,
+ SuggestedFix.Builder fixBuilder,
+ VisitorState state) {
+ ModifiersTree modifiersTree =
+ requireNonNull(ASTHelpers.getModifiers(tree), "`VariableTree` must have modifiers");
+ SuggestedFixes.addModifiers(tree, modifiersTree, state, modifiers).ifPresent(fixBuilder::merge);
+ SuggestedFixes.removeModifiers(
+ modifiersTree, state, Sets.difference(EnumSet.allOf(Modifier.class), modifiers))
+ .ifPresent(fixBuilder::merge);
+ }
+
+ private static void suggestRename(
+ VariableTree variableTree, String name, SuggestedFix.Builder fixBuilder, VisitorState state) {
+ if (!variableTree.getName().contentEquals(name)) {
+ fixBuilder.merge(SuggestedFixes.renameVariable(variableTree, name, state));
+ }
+ }
+
+ private static boolean isEnclosingClassReference(ExpressionTree tree, VisitorState state) {
+ return ASTHelpers.getSymbol(getEnclosingClass(state)).equals(ASTHelpers.getSymbol(tree));
+ }
+
+ private static boolean getClassReceiverIsEnclosingClassInstance(
+ MethodInvocationTree getClassInvocationTree, VisitorState state) {
+ ExpressionTree receiver = ASTHelpers.getReceiver(getClassInvocationTree);
+ if (receiver == null) {
+ /*
+ * Method invocations without an explicit receiver either involve static methods (possibly
+ * statically imported), or instance methods invoked on the enclosing class. As the given
+ * `getClassInvocationTree` is guaranteed to be a nullary `#getClass()` invocation, the latter
+ * must be the case.
+ */
+ return true;
+ }
+
+ Symbol symbol = ASTHelpers.getSymbol(receiver);
+ return symbol != null
+ && symbol.asType().tsym.equals(ASTHelpers.getSymbol(getEnclosingClass(state)));
+ }
+
+ private static ClassTree getEnclosingClass(VisitorState state) {
+ ClassTree clazz = state.findEnclosing(ClassTree.class);
+ // XXX: Review whether we should relax this constraint in the face of so-called anonymous
+ // classes. See
+ // https://docs.oracle.com/en/java/javase/23/language/implicitly-declared-classes-and-instance-main-methods.html
+ verify(clazz != null, "Variable not defined inside class");
+ return clazz;
+ }
+}
diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/AssortedRules.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/AssortedRules.java
index 4841481e..a38a823e 100644
--- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/AssortedRules.java
+++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/AssortedRules.java
@@ -1,8 +1,6 @@
package tech.picnic.errorprone.refasterrules;
import static com.google.common.base.Preconditions.checkElementIndex;
-import static com.google.common.collect.ImmutableSet.toImmutableSet;
-import static com.google.common.collect.Sets.toImmutableEnumSet;
import static com.google.errorprone.refaster.ImportPolicy.STATIC_IMPORT_ALWAYS;
import static java.util.Collections.disjoint;
import static java.util.Objects.checkIndex;
@@ -70,28 +68,6 @@ final class AssortedRules {
}
}
- /**
- * Use {@link Sets#toImmutableEnumSet()} when possible, as it is more efficient than {@link
- * ImmutableSet#toImmutableSet()} and produces a more compact object.
- *
- * Warning: this rewrite rule is not completely behavior preserving: while the
- * original code produces a set that iterates over the elements in encounter order, the
- * replacement code iterates over the elements in enum definition order.
- */
- // XXX: ^ Consider emitting a comment warning about this fact?
- static final class StreamToImmutableEnumSet> {
- @BeforeTemplate
- ImmutableSet before(Stream stream) {
- return stream.collect(toImmutableSet());
- }
-
- @AfterTemplate
- @UseImportPolicy(STATIC_IMPORT_ALWAYS)
- ImmutableSet after(Stream stream) {
- return stream.collect(toImmutableEnumSet());
- }
- }
-
/** Prefer {@link Iterators#getNext(Iterator, Object)} over more contrived alternatives. */
static final class IteratorGetNextOrDefault {
@BeforeTemplate
diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/BugCheckerRules.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/BugCheckerRules.java
index db21ddb7..9ce8236f 100644
--- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/BugCheckerRules.java
+++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/BugCheckerRules.java
@@ -9,6 +9,7 @@ import com.google.errorprone.refaster.annotation.AfterTemplate;
import com.google.errorprone.refaster.annotation.BeforeTemplate;
import com.sun.tools.javac.util.Constants;
import com.sun.tools.javac.util.Convert;
+import javax.lang.model.element.Name;
import tech.picnic.errorprone.refaster.annotation.OnlineDocumentation;
/** Refaster rules related to {@link com.google.errorprone.bugpatterns.BugChecker} classes. */
@@ -67,4 +68,22 @@ final class BugCheckerRules {
return Constants.format(value);
}
}
+
+ /** Prefer {@link Name#contentEquals(CharSequence)} over more verbose alternatives. */
+ static final class NameContentEquals {
+ @BeforeTemplate
+ boolean before(Name name, CharSequence string) {
+ return name.toString().equals(string.toString());
+ }
+
+ @BeforeTemplate
+ boolean before(Name name, String string) {
+ return name.toString().equals(string);
+ }
+
+ @AfterTemplate
+ boolean after(Name name, CharSequence string) {
+ return name.contentEquals(string);
+ }
+ }
}
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..5b0d4780 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,21 @@ final class ClassRules {
return clazz::isInstance;
}
}
+
+ /**
+ * 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..3a02f6fe 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
@@ -327,18 +365,20 @@ final class CollectionRules {
}
}
- /**
- * Don't call {@link ImmutableCollection#asList()} if {@link ImmutableCollection#iterator()} is
- * called on the result; call it directly.
- */
- static final class ImmutableCollectionIterator {
+ /** Prefer {@link Collection#iterator()} over more contrived or less efficient alternatives. */
+ static final class CollectionIterator {
+ @BeforeTemplate
+ Iterator before(Collection collection) {
+ return collection.stream().iterator();
+ }
+
@BeforeTemplate
Iterator before(ImmutableCollection collection) {
return collection.asList().iterator();
}
@AfterTemplate
- Iterator after(ImmutableCollection collection) {
+ Iterator after(Collection collection) {
return collection.iterator();
}
}
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..f846de8a 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,13 +16,18 @@ 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;
+import java.util.Collection;
import java.util.Collections;
import java.util.Comparator;
+import java.util.List;
import java.util.Optional;
import java.util.function.BinaryOperator;
import java.util.function.Function;
@@ -92,6 +97,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
@@ -222,18 +245,77 @@ final class ComparatorRules {
}
}
+ /** Prefer {@link Collections#sort(List)} over more verbose alternatives. */
+ static final class CollectionsSort> {
+ @BeforeTemplate
+ void before(List collection) {
+ Collections.sort(collection, naturalOrder());
+ }
+
+ @AfterTemplate
+ void after(List collection) {
+ Collections.sort(collection);
+ }
+ }
+
+ /** Prefer {@link Collections#min(Collection)} over more verbose alternatives. */
+ static final class CollectionsMin> {
+ @BeforeTemplate
+ T before(Collection collection) {
+ return Refaster.anyOf(
+ Collections.min(collection, naturalOrder()), Collections.max(collection, reverseOrder()));
+ }
+
+ @AfterTemplate
+ T after(Collection collection) {
+ return Collections.min(collection);
+ }
+ }
+
/**
* Avoid unnecessary creation of a {@link Stream} to determine the minimum of a known collection
* of values.
*/
- static final class MinOfVarargs {
+ static final class MinOfArray {
@BeforeTemplate
- T before(@Repeated T value, Comparator cmp) {
+ T before(T[] array, Comparator cmp) {
+ return Arrays.stream(array).min(cmp).orElseThrow();
+ }
+
+ @AfterTemplate
+ T after(T[] array, Comparator cmp) {
+ return Collections.min(Arrays.asList(array), cmp);
+ }
+ }
+
+ /**
+ * Avoid unnecessary creation of a {@link Stream} to determine the minimum of a known collection
+ * of values.
+ */
+ static final class CollectionsMinWithComparator {
+ @BeforeTemplate
+ T before(Collection collection, Comparator cmp) {
+ return collection.stream().min(cmp).orElseThrow();
+ }
+
+ @AfterTemplate
+ T after(Collection collection, Comparator cmp) {
+ return Collections.min(collection, cmp);
+ }
+ }
+
+ /**
+ * Avoid unnecessary creation of a {@link Stream} to determine the minimum of a known collection
+ * of values.
+ */
+ static final class MinOfVarargs {
+ @BeforeTemplate
+ T before(@Repeated T value, Comparator cmp) {
return Stream.of(Refaster.asVarargs(value)).min(cmp).orElseThrow();
}
@AfterTemplate
- T after(@Repeated T value, Comparator cmp) {
+ T after(@Repeated T value, Comparator cmp) {
return Collections.min(Arrays.asList(value), cmp);
}
}
@@ -269,7 +351,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,23 +366,69 @@ 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);
}
}
+ /** Prefer {@link Collections#max(Collection)} over more verbose alternatives. */
+ static final class CollectionsMax> {
+ @BeforeTemplate
+ T before(Collection collection) {
+ return Refaster.anyOf(
+ Collections.max(collection, naturalOrder()), Collections.min(collection, reverseOrder()));
+ }
+
+ @AfterTemplate
+ T after(Collection collection) {
+ return Collections.max(collection);
+ }
+ }
+
/**
* Avoid unnecessary creation of a {@link Stream} to determine the maximum of a known collection
* of values.
*/
- static final class MaxOfVarargs {
+ static final class MaxOfArray {
@BeforeTemplate
- T before(@Repeated T value, Comparator cmp) {
+ T before(T[] array, Comparator cmp) {
+ return Arrays.stream(array).max(cmp).orElseThrow();
+ }
+
+ @AfterTemplate
+ T after(T[] array, Comparator cmp) {
+ return Collections.max(Arrays.asList(array), cmp);
+ }
+ }
+
+ /**
+ * Avoid unnecessary creation of a {@link Stream} to determine the maximum of a known collection
+ * of values.
+ */
+ static final class CollectionsMaxWithComparator {
+ @BeforeTemplate
+ T before(Collection collection, Comparator cmp) {
+ return collection.stream().max(cmp).orElseThrow();
+ }
+
+ @AfterTemplate
+ T after(Collection collection, Comparator cmp) {
+ return Collections.max(collection, cmp);
+ }
+ }
+
+ /**
+ * Avoid unnecessary creation of a {@link Stream} to determine the maximum of a known collection
+ * of values.
+ */
+ static final class MaxOfVarargs {
+ @BeforeTemplate
+ T before(@Repeated T value, Comparator cmp) {
return Stream.of(Refaster.asVarargs(value)).max(cmp).orElseThrow();
}
@AfterTemplate
- T after(@Repeated T value, Comparator cmp) {
+ T after(@Repeated T value, Comparator cmp) {
return Collections.max(Arrays.asList(value), cmp);
}
}
@@ -336,7 +464,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 +479,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 +547,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..4454e34f 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,18 @@ 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 com.google.errorprone.refaster.annotation.Repeated;
+import java.io.File;
import java.io.IOException;
+import java.net.URI;
import java.nio.charset.Charset;
import java.nio.file.Files;
import java.nio.file.Path;
+import java.nio.file.Paths;
+import java.nio.file.attribute.FileAttribute;
import tech.picnic.errorprone.refaster.annotation.OnlineDocumentation;
/** Refaster rules related to expressions dealing with files. */
@@ -15,6 +21,49 @@ import tech.picnic.errorprone.refaster.annotation.OnlineDocumentation;
final class FileRules {
private FileRules() {}
+ /** Prefer the more idiomatic {@link Path#of(URI)} over {@link Paths#get(URI)}. */
+ static final class PathOfUri {
+ @BeforeTemplate
+ Path before(URI uri) {
+ return Paths.get(uri);
+ }
+
+ @AfterTemplate
+ Path after(URI uri) {
+ return Path.of(uri);
+ }
+ }
+
+ /**
+ * Prefer the more idiomatic {@link Path#of(String, String...)} over {@link Paths#get(String,
+ * String...)}.
+ */
+ static final class PathOfString {
+ @BeforeTemplate
+ Path before(String first, @Repeated String more) {
+ return Paths.get(first, more);
+ }
+
+ @AfterTemplate
+ Path after(String first, @Repeated String more) {
+ return Path.of(first, more);
+ }
+ }
+
+ /** Avoid redundant conversions from {@link Path} to {@link File}. */
+ // XXX: Review whether a rule such as this one is better handled by the `IdentityConversion` rule.
+ static final class PathInstance {
+ @BeforeTemplate
+ Path before(Path path) {
+ return path.toFile().toPath();
+ }
+
+ @AfterTemplate
+ Path after(Path path) {
+ return path;
+ }
+ }
+
/** Prefer {@link Files#readString(Path, Charset)} over more contrived alternatives. */
static final class FilesReadStringWithCharset {
@BeforeTemplate
@@ -40,4 +89,44 @@ 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({
+ "FilesCreateTempFileInCustomDirectoryToFile" /* This is a more specific template. */,
+ "java:S5443" /* This violation will be rewritten. */,
+ "key-to-resolve-AnnotationUseStyle-and-TrailingComment-check-conflict"
+ })
+ 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();
+ }
+ }
+
+ /**
+ * Prefer {@link Files#createTempFile(Path, String, String, FileAttribute[])} over alternatives
+ * that create files with more liberal permissions.
+ */
+ static final class FilesCreateTempFileInCustomDirectoryToFile {
+ @BeforeTemplate
+ File before(File directory, String prefix, String suffix) throws IOException {
+ return File.createTempFile(prefix, suffix, directory);
+ }
+
+ @AfterTemplate
+ File after(File directory, String prefix, String suffix) throws IOException {
+ return Files.createTempFile(directory.toPath(), prefix, suffix).toFile();
+ }
+ }
}
diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableEnumSetRules.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableEnumSetRules.java
new file mode 100644
index 00000000..b7b40af6
--- /dev/null
+++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableEnumSetRules.java
@@ -0,0 +1,246 @@
+package tech.picnic.errorprone.refasterrules;
+
+import static com.google.common.collect.ImmutableSet.toImmutableSet;
+import static com.google.common.collect.Sets.toImmutableEnumSet;
+import static com.google.errorprone.refaster.ImportPolicy.STATIC_IMPORT_ALWAYS;
+
+import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.Sets;
+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.Repeated;
+import com.google.errorprone.refaster.annotation.UseImportPolicy;
+import java.util.Arrays;
+import java.util.Collection;
+import java.util.EnumSet;
+import java.util.stream.Stream;
+import tech.picnic.errorprone.refaster.annotation.OnlineDocumentation;
+
+/**
+ * Refaster rules related to expressions dealing with {@code
+ * com.google.common.collect.ImmutableEnumSet}s.
+ */
+// XXX: Some of the rules defined here impact iteration order. That's a rather subtle change. Should
+// we emit a comment warning about this fact? (This may produce a lot of noise. A bug checker could
+// in some cases determine whether iteration order is important.)
+// XXX: Consider replacing the `SetsImmutableEnumSet[N]` Refaster rules with a bug checker, such
+// that call to `ImmutableSet#of(Object, Object, Object, Object, Object, Object, Object[])` with
+// enum-typed values can also be rewritten.
+@OnlineDocumentation
+final class ImmutableEnumSetRules {
+ private ImmutableEnumSetRules() {}
+
+ /**
+ * Prefer {@link Sets#immutableEnumSet(Iterable)} for enum collections to take advantage of the
+ * internally used {@link EnumSet}.
+ *
+ * Warning: this rule is not completely behavior preserving: while the
+ * original code produces a set that iterates over its elements in the same order as the input
+ * {@link Iterable}, the replacement code iterates over the elements in enum definition order.
+ */
+ static final class SetsImmutableEnumSetIterable> {
+ @BeforeTemplate
+ ImmutableSet before(Iterable elements) {
+ return ImmutableSet.copyOf(elements);
+ }
+
+ @BeforeTemplate
+ ImmutableSet before(Collection elements) {
+ return ImmutableSet.copyOf(elements);
+ }
+
+ @AfterTemplate
+ ImmutableSet after(Iterable elements) {
+ return Sets.immutableEnumSet(elements);
+ }
+ }
+
+ /**
+ * Prefer {@link Sets#immutableEnumSet(Iterable)} for enum collections to take advantage of the
+ * internally used {@link EnumSet}.
+ *
+ * Warning: this rule is not completely behavior preserving: while the
+ * original code produces a set that iterates over its elements in the same order as defined in
+ * the array, the replacement code iterates over the elements in enum definition order.
+ */
+ static final class SetsImmutableEnumSetArraysAsList> {
+ @BeforeTemplate
+ ImmutableSet before(T[] elements) {
+ return ImmutableSet.copyOf(elements);
+ }
+
+ @AfterTemplate
+ ImmutableSet after(T[] elements) {
+ return Sets.immutableEnumSet(Arrays.asList(elements));
+ }
+ }
+
+ /**
+ * Prefer {@link Sets#immutableEnumSet(Enum, Enum[])} for enum collections to take advantage of
+ * the internally used {@link EnumSet}.
+ */
+ static final class SetsImmutableEnumSet1> {
+ @BeforeTemplate
+ @SuppressWarnings("SetsImmutableEnumSetIterable" /* This is a more specific template. */)
+ ImmutableSet before(T e1) {
+ return Refaster.anyOf(ImmutableSet.of(e1), ImmutableSet.copyOf(EnumSet.of(e1)));
+ }
+
+ @AfterTemplate
+ @SuppressWarnings("unchecked")
+ ImmutableSet after(T e1) {
+ return Sets.immutableEnumSet(e1);
+ }
+ }
+
+ /**
+ * Prefer {@link Sets#immutableEnumSet(Enum, Enum[])} for enum collections to take advantage of
+ * the internally used {@link EnumSet}.
+ *
+ * Warning: this rule is not completely behavior preserving: while the {@link
+ * ImmutableSet#of} expression produces a set that iterates over its elements in the listed order,
+ * the replacement code iterates over the elements in enum definition order.
+ */
+ static final class SetsImmutableEnumSet2> {
+ @BeforeTemplate
+ @SuppressWarnings("SetsImmutableEnumSetIterable" /* This is a more specific template. */)
+ ImmutableSet before(T e1, T e2) {
+ return Refaster.anyOf(ImmutableSet.of(e1, e2), ImmutableSet.copyOf(EnumSet.of(e1, e2)));
+ }
+
+ @AfterTemplate
+ @SuppressWarnings("unchecked")
+ ImmutableSet after(T e1, T e2) {
+ return Sets.immutableEnumSet(e1, e2);
+ }
+ }
+
+ /**
+ * Prefer {@link Sets#immutableEnumSet(Enum, Enum[])} for enum collections to take advantage of
+ * the internally used {@link EnumSet}.
+ *
+ * Warning: this rule is not completely behavior preserving: while the {@link
+ * ImmutableSet#of} expression produces a set that iterates over its elements in the listed order,
+ * the replacement code iterates over the elements in enum definition order.
+ */
+ static final class SetsImmutableEnumSet3> {
+ @BeforeTemplate
+ @SuppressWarnings("SetsImmutableEnumSetIterable" /* This is a more specific template. */)
+ ImmutableSet before(T e1, T e2, T e3) {
+ return Refaster.anyOf(
+ ImmutableSet.of(e1, e2, e3), ImmutableSet.copyOf(EnumSet.of(e1, e2, e3)));
+ }
+
+ @AfterTemplate
+ @SuppressWarnings("unchecked")
+ ImmutableSet after(T e1, T e2, T e3) {
+ return Sets.immutableEnumSet(e1, e2, e3);
+ }
+ }
+
+ /**
+ * Prefer {@link Sets#immutableEnumSet(Enum, Enum[])} for enum collections to take advantage of
+ * the internally used {@link EnumSet}.
+ *
+ * Warning: this rule is not completely behavior preserving: while the {@link
+ * ImmutableSet#of} expression produces a set that iterates over its elements in the listed order,
+ * the replacement code iterates over the elements in enum definition order.
+ */
+ static final class SetsImmutableEnumSet4> {
+ @BeforeTemplate
+ @SuppressWarnings("SetsImmutableEnumSetIterable" /* This is a more specific template. */)
+ ImmutableSet before(T e1, T e2, T e3, T e4) {
+ return Refaster.anyOf(
+ ImmutableSet.of(e1, e2, e3, e4), ImmutableSet.copyOf(EnumSet.of(e1, e2, e3, e4)));
+ }
+
+ @AfterTemplate
+ @SuppressWarnings("unchecked")
+ ImmutableSet after(T e1, T e2, T e3, T e4) {
+ return Sets.immutableEnumSet(e1, e2, e3, e4);
+ }
+ }
+
+ /**
+ * Prefer {@link Sets#immutableEnumSet(Enum, Enum[])} for enum collections to take advantage of
+ * the internally used {@link EnumSet}.
+ *
+ * Warning: this rule is not completely behavior preserving: while the {@link
+ * ImmutableSet#of} expression produces a set that iterates over its elements in the listed order,
+ * the replacement code iterates over the elements in enum definition order.
+ */
+ static final class SetsImmutableEnumSet5> {
+ @BeforeTemplate
+ @SuppressWarnings("SetsImmutableEnumSetIterable" /* This is a more specific template. */)
+ ImmutableSet before(T e1, T e2, T e3, T e4, T e5) {
+ return Refaster.anyOf(
+ ImmutableSet.of(e1, e2, e3, e4, e5), ImmutableSet.copyOf(EnumSet.of(e1, e2, e3, e4, e5)));
+ }
+
+ @AfterTemplate
+ @SuppressWarnings("unchecked")
+ ImmutableSet after(T e1, T e2, T e3, T e4, T e5) {
+ return Sets.immutableEnumSet(e1, e2, e3, e4, e5);
+ }
+ }
+
+ /**
+ * Prefer {@link Sets#immutableEnumSet(Enum, Enum[])} for enum collections to take advantage of
+ * the internally used {@link EnumSet}.
+ *
+ * Warning: this rule is not completely behavior preserving: while the
+ * original code produces a set that iterates over its elements in the listed order, the
+ * replacement code iterates over the elements in enum definition order.
+ */
+ static final class SetsImmutableEnumSet6> {
+ @BeforeTemplate
+ ImmutableSet before(T e1, T e2, T e3, T e4, T e5, T e6) {
+ return ImmutableSet.of(e1, e2, e3, e4, e5, e6);
+ }
+
+ @AfterTemplate
+ @SuppressWarnings("unchecked")
+ ImmutableSet after(T e1, T e2, T e3, T e4, T e5, T e6) {
+ return Sets.immutableEnumSet(e1, e2, e3, e4, e5, e6);
+ }
+ }
+
+ /**
+ * Prefer {@link Sets#immutableEnumSet(Enum, Enum[])} for enum collections to take advantage of
+ * the internally used {@link EnumSet}.
+ */
+ static final class SetsImmutableEnumSetVarArgs> {
+ @BeforeTemplate
+ @SuppressWarnings("SetsImmutableEnumSetIterable" /* This is a more specific template. */)
+ ImmutableSet before(T e1, @Repeated T elements) {
+ return ImmutableSet.copyOf(EnumSet.of(e1, Refaster.asVarargs(elements)));
+ }
+
+ @AfterTemplate
+ ImmutableSet after(T e1, @Repeated T elements) {
+ return Sets.immutableEnumSet(e1, Refaster.asVarargs(elements));
+ }
+ }
+
+ /**
+ * Use {@link Sets#toImmutableEnumSet()} when possible, as it is more efficient than {@link
+ * ImmutableSet#toImmutableSet()} and produces a more compact object.
+ *
+ * Warning: this rule is not completely behavior preserving: while the
+ * original code produces a set that iterates over its elements in encounter order, the
+ * replacement code iterates over the elements in enum definition order.
+ */
+ static final class StreamToImmutableEnumSet> {
+ @BeforeTemplate
+ ImmutableSet before(Stream stream) {
+ return stream.collect(toImmutableSet());
+ }
+
+ @AfterTemplate
+ @UseImportPolicy(STATIC_IMPORT_ALWAYS)
+ ImmutableSet after(Stream stream) {
+ return stream.collect(toImmutableEnumSet());
+ }
+ }
+}
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/MicrometerRules.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/MicrometerRules.java
new file mode 100644
index 00000000..40800c6d
--- /dev/null
+++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/MicrometerRules.java
@@ -0,0 +1,88 @@
+package tech.picnic.errorprone.refasterrules;
+
+import com.google.common.collect.ImmutableCollection;
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableSet;
+import com.google.errorprone.refaster.Refaster;
+import com.google.errorprone.refaster.annotation.AfterTemplate;
+import com.google.errorprone.refaster.annotation.BeforeTemplate;
+import io.micrometer.core.instrument.Tag;
+import io.micrometer.core.instrument.Tags;
+import tech.picnic.errorprone.refaster.annotation.OnlineDocumentation;
+
+/** Refaster rules related to expressions dealing with Micrometer. */
+// XXX: Consider replacing the `TagsOf[N]` rules with a bug checker, so that various other
+// expressions (e.g. those creating other collection types, those passing in tags some other way, or
+// those passing in more tags) can be replaced as wel.
+@OnlineDocumentation
+final class MicrometerRules {
+ private MicrometerRules() {}
+
+ /** Prefer using {@link Tags} over other immutable collections. */
+ static final class TagsOf1 {
+ @BeforeTemplate
+ ImmutableCollection before(Tag tag) {
+ return Refaster.anyOf(ImmutableSet.of(tag), ImmutableList.of(tag));
+ }
+
+ @AfterTemplate
+ Iterable after(Tag tag) {
+ return Tags.of(tag);
+ }
+ }
+
+ /** Prefer using {@link Tags} over other immutable collections. */
+ static final class TagsOf2 {
+ @BeforeTemplate
+ ImmutableCollection before(Tag tag1, Tag tag2) {
+ return Refaster.anyOf(ImmutableSet.of(tag1, tag2), ImmutableList.of(tag1, tag2));
+ }
+
+ @AfterTemplate
+ Iterable after(Tag tag1, Tag tag2) {
+ return Tags.of(tag1, tag2);
+ }
+ }
+
+ /** Prefer using {@link Tags} over other immutable collections. */
+ static final class TagsOf3 {
+ @BeforeTemplate
+ ImmutableCollection before(Tag tag1, Tag tag2, Tag tag3) {
+ return Refaster.anyOf(ImmutableSet.of(tag1, tag2, tag3), ImmutableList.of(tag1, tag2, tag3));
+ }
+
+ @AfterTemplate
+ Iterable after(Tag tag1, Tag tag2, Tag tag3) {
+ return Tags.of(tag1, tag2, tag3);
+ }
+ }
+
+ /** Prefer using {@link Tags} over other immutable collections. */
+ static final class TagsOf4 {
+ @BeforeTemplate
+ ImmutableCollection before(Tag tag1, Tag tag2, Tag tag3, Tag tag4) {
+ return Refaster.anyOf(
+ ImmutableSet.of(tag1, tag2, tag3, tag4), ImmutableList.of(tag1, tag2, tag3, tag4));
+ }
+
+ @AfterTemplate
+ Iterable after(Tag tag1, Tag tag2, Tag tag3, Tag tag4) {
+ return Tags.of(tag1, tag2, tag3, tag4);
+ }
+ }
+
+ /** Prefer using {@link Tags} over other immutable collections. */
+ static final class TagsOf5 {
+ @BeforeTemplate
+ ImmutableCollection before(Tag tag1, Tag tag2, Tag tag3, Tag tag4, Tag tag5) {
+ return Refaster.anyOf(
+ ImmutableSet.of(tag1, tag2, tag3, tag4, tag5),
+ ImmutableList.of(tag1, tag2, tag3, tag4, tag5));
+ }
+
+ @AfterTemplate
+ Iterable after(Tag tag1, Tag tag2, Tag tag3, Tag tag4, Tag tag5) {
+ return Tags.of(tag1, tag2, tag3, tag4, tag5);
+ }
+ }
+}
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..cc8928aa 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
@@ -20,13 +20,26 @@ import java.util.function.Supplier;
import java.util.stream.Stream;
import org.jspecify.annotations.Nullable;
import tech.picnic.errorprone.refaster.annotation.OnlineDocumentation;
-import tech.picnic.errorprone.refaster.matchers.IsLikelyTrivialComputation;
+import tech.picnic.errorprone.refaster.matchers.RequiresComputation;
/** Refaster rules related to expressions dealing with {@link Optional}s. */
@OnlineDocumentation
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.
@@ -242,24 +255,21 @@ final class OptionalRules {
}
/**
- * Prefer {@link Optional#orElseGet(Supplier)} over {@link Optional#orElse(Object)} if the
- * fallback value is not the result of a trivial computation.
+ * Prefer {@link Optional#orElse(Object)} over {@link Optional#orElseGet(Supplier)} if the
+ * fallback value does not require non-trivial computation.
*/
- // XXX: This rule may introduce a compilation error: the `value` expression may reference a
- // non-effectively final variable, which is not allowed in the replacement lambda expression.
- // Review whether a `@Matcher` can be used to avoid this.
- // XXX: Once `MethodReferenceUsage` is "production ready", replace
- // `@NotMatches(IsLikelyTrivialComputation.class)` with `@Matches(RequiresComputation.class)` (and
- // reimplement the matcher accordingly).
- static final class OptionalOrElseGet {
+ // XXX: This rule is the counterpart to the `OptionalOrElseGet` bug checker. Once the
+ // `MethodReferenceUsage` bug checker is "production ready", that bug checker may similarly be
+ // replaced with a Refaster rule.
+ static final class OptionalOrElse {
@BeforeTemplate
- T before(Optional optional, @NotMatches(IsLikelyTrivialComputation.class) T value) {
- return optional.orElse(value);
+ T before(Optional optional, @NotMatches(RequiresComputation.class) T value) {
+ return optional.orElseGet(() -> value);
}
@AfterTemplate
T after(Optional optional, T value) {
- return optional.orElseGet(() -> value);
+ return optional.orElse(value);
}
}
@@ -269,6 +279,9 @@ final class OptionalRules {
*/
// XXX: Do we need the `.filter(Optional::isPresent)`? If it's absent the caller probably assumed
// that the values are present. (If we drop it, we should rewrite vacuous filter steps.)
+ // XXX: The rewritten `filter`/`map` expression may be more performant than its replacement. See
+ // https://github.com/palantir/gradle-baseline/pull/2946. (There are plans to pair Refaster rules
+ // with JMH benchmarks; this would be a great use case.)
static final class StreamFlatMapOptional {
@BeforeTemplate
Stream before(Stream> stream) {
@@ -360,7 +373,12 @@ 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({
+ "LexicographicalAnnotationAttributeListing" /* `key-*` entry must remain last. */,
+ "NestedOptionals" /* This violation will be rewritten. */,
+ "OptionalOrElse" /* Parameters represent expressions that may require computation. */,
+ "key-to-resolve-AnnotationUseStyle-and-TrailingComment-check-conflict"
+ })
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.
@@ -380,15 +398,16 @@ final class OptionalRules {
}
}
- /**
- * Avoid unnecessary operations on an {@link Optional} that ultimately result in that very same
- * {@link Optional}.
- */
+ /** Don't unnecessarily transform an {@link Optional} to an equivalent instance. */
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 +461,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/PrimitiveRules.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/PrimitiveRules.java
index f39d4e99..b034ec73 100644
--- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/PrimitiveRules.java
+++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/PrimitiveRules.java
@@ -8,6 +8,8 @@ import com.google.common.primitives.Floats;
import com.google.common.primitives.Ints;
import com.google.common.primitives.Longs;
import com.google.common.primitives.Shorts;
+import com.google.common.primitives.UnsignedInts;
+import com.google.common.primitives.UnsignedLongs;
import com.google.errorprone.refaster.Refaster;
import com.google.errorprone.refaster.annotation.AfterTemplate;
import com.google.errorprone.refaster.annotation.AlsoNegation;
@@ -76,6 +78,8 @@ final class PrimitiveRules {
}
/** Prefer {@link Math#toIntExact(long)} over the Guava alternative. */
+ // XXX: This rule changes the exception possibly thrown from `IllegalArgumentException` to
+ // `ArithmeticException`.
static final class LongToIntExact {
@BeforeTemplate
int before(long l) {
@@ -192,97 +196,6 @@ final class PrimitiveRules {
}
}
- /** Prefer {@link Boolean#compare(boolean, boolean)} over the Guava alternative. */
- static final class BooleanCompare {
- @BeforeTemplate
- int before(boolean a, boolean b) {
- return Booleans.compare(a, b);
- }
-
- @AfterTemplate
- int after(boolean a, boolean b) {
- return Boolean.compare(a, b);
- }
- }
-
- /** Prefer {@link Character#compare(char, char)} over the Guava alternative. */
- static final class CharacterCompare {
- @BeforeTemplate
- int before(char a, char b) {
- return Chars.compare(a, b);
- }
-
- @AfterTemplate
- int after(char a, char b) {
- return Character.compare(a, b);
- }
- }
-
- /** Prefer {@link Short#compare(short, short)} over the Guava alternative. */
- static final class ShortCompare {
- @BeforeTemplate
- int before(short a, short b) {
- return Shorts.compare(a, b);
- }
-
- @AfterTemplate
- int after(short a, short b) {
- return Short.compare(a, b);
- }
- }
-
- /** Prefer {@link Integer#compare(int, int)} over the Guava alternative. */
- static final class IntegerCompare {
- @BeforeTemplate
- int before(int a, int b) {
- return Ints.compare(a, b);
- }
-
- @AfterTemplate
- int after(int a, int b) {
- return Integer.compare(a, b);
- }
- }
-
- /** Prefer {@link Long#compare(long, long)} over the Guava alternative. */
- static final class LongCompare {
- @BeforeTemplate
- int before(long a, long b) {
- return Longs.compare(a, b);
- }
-
- @AfterTemplate
- int after(long a, long b) {
- return Long.compare(a, b);
- }
- }
-
- /** Prefer {@link Float#compare(float, float)} over the Guava alternative. */
- static final class FloatCompare {
- @BeforeTemplate
- int before(float a, float b) {
- return Floats.compare(a, b);
- }
-
- @AfterTemplate
- int after(float a, float b) {
- return Float.compare(a, b);
- }
- }
-
- /** Prefer {@link Double#compare(double, double)} over the Guava alternative. */
- static final class DoubleCompare {
- @BeforeTemplate
- int before(double a, double b) {
- return Doubles.compare(a, b);
- }
-
- @AfterTemplate
- int after(double a, double b) {
- return Double.compare(a, b);
- }
- }
-
/** Prefer {@link Character#BYTES} over the Guava alternative. */
static final class CharacterBytes {
@BeforeTemplate
@@ -442,4 +355,205 @@ final class PrimitiveRules {
return Long.signum(l) == -1;
}
}
+
+ /** Prefer JDK's {@link Integer#compareUnsigned(int, int)} over third-party alternatives. */
+ static final class IntegerCompareUnsigned {
+ @BeforeTemplate
+ int before(int x, int y) {
+ return UnsignedInts.compare(x, y);
+ }
+
+ @AfterTemplate
+ int after(int x, int y) {
+ return Integer.compareUnsigned(x, y);
+ }
+ }
+
+ /** Prefer JDK's {@link Long#compareUnsigned(long, long)} over third-party alternatives. */
+ static final class LongCompareUnsigned {
+ @BeforeTemplate
+ long before(long x, long y) {
+ return UnsignedLongs.compare(x, y);
+ }
+
+ @AfterTemplate
+ long after(long x, long y) {
+ return Long.compareUnsigned(x, y);
+ }
+ }
+
+ /** Prefer JDK's {@link Integer#divideUnsigned(int, int)} over third-party alternatives. */
+ static final class IntegerDivideUnsigned {
+ @BeforeTemplate
+ int before(int x, int y) {
+ return UnsignedInts.divide(x, y);
+ }
+
+ @AfterTemplate
+ int after(int x, int y) {
+ return Integer.divideUnsigned(x, y);
+ }
+ }
+
+ /** Prefer JDK's {@link Long#divideUnsigned(long, long)} over third-party alternatives. */
+ static final class LongDivideUnsigned {
+ @BeforeTemplate
+ long before(long x, long y) {
+ return UnsignedLongs.divide(x, y);
+ }
+
+ @AfterTemplate
+ long after(long x, long y) {
+ return Long.divideUnsigned(x, y);
+ }
+ }
+
+ /** Prefer JDK's {@link Integer#remainderUnsigned(int, int)} over third-party alternatives. */
+ static final class IntegerRemainderUnsigned {
+ @BeforeTemplate
+ int before(int x, int y) {
+ return UnsignedInts.remainder(x, y);
+ }
+
+ @AfterTemplate
+ int after(int x, int y) {
+ return Integer.remainderUnsigned(x, y);
+ }
+ }
+
+ /** Prefer JDK's {@link Long#remainderUnsigned(long, long)} over third-party alternatives. */
+ static final class LongRemainderUnsigned {
+ @BeforeTemplate
+ long before(long x, long y) {
+ return UnsignedLongs.remainder(x, y);
+ }
+
+ @AfterTemplate
+ long after(long x, long y) {
+ return Long.remainderUnsigned(x, y);
+ }
+ }
+
+ /**
+ * Prefer JDK's {@link Integer#parseUnsignedInt(String)} over third-party or more verbose
+ * alternatives.
+ */
+ static final class IntegerParseUnsignedInt {
+ @BeforeTemplate
+ int before(String string) {
+ return Refaster.anyOf(
+ UnsignedInts.parseUnsignedInt(string), Integer.parseUnsignedInt(string, 10));
+ }
+
+ @AfterTemplate
+ int after(String string) {
+ return Integer.parseUnsignedInt(string);
+ }
+ }
+
+ /**
+ * Prefer JDK's {@link Long#parseUnsignedLong(String)} over third-party or more verbose
+ * alternatives.
+ */
+ static final class LongParseUnsignedLong {
+ @BeforeTemplate
+ long before(String string) {
+ return Refaster.anyOf(
+ UnsignedLongs.parseUnsignedLong(string), Long.parseUnsignedLong(string, 10));
+ }
+
+ @AfterTemplate
+ long after(String string) {
+ return Long.parseUnsignedLong(string);
+ }
+ }
+
+ /** Prefer JDK's {@link Integer#parseUnsignedInt(String, int)} over third-party alternatives. */
+ static final class IntegerParseUnsignedIntWithRadix {
+ @BeforeTemplate
+ int before(String string, int radix) {
+ return UnsignedInts.parseUnsignedInt(string, radix);
+ }
+
+ @AfterTemplate
+ int after(String string, int radix) {
+ return Integer.parseUnsignedInt(string, radix);
+ }
+ }
+
+ /** Prefer JDK's {@link Long#parseUnsignedLong(String, int)} over third-party alternatives. */
+ static final class LongParseUnsignedLongWithRadix {
+ @BeforeTemplate
+ long before(String string, int radix) {
+ return UnsignedLongs.parseUnsignedLong(string, radix);
+ }
+
+ @AfterTemplate
+ long after(String string, int radix) {
+ return Long.parseUnsignedLong(string, radix);
+ }
+ }
+
+ /**
+ * Prefer JDK's {@link Integer#toUnsignedString(int)} over third-party or more verbose
+ * alternatives.
+ */
+ static final class IntegerToUnsignedString {
+ @BeforeTemplate
+ String before(int i) {
+ return Refaster.anyOf(UnsignedInts.toString(i), Integer.toUnsignedString(i, 10));
+ }
+
+ @AfterTemplate
+ String after(int i) {
+ return Integer.toUnsignedString(i);
+ }
+ }
+
+ /**
+ * Prefer JDK's {@link Long#toUnsignedString(long)} over third-party or more verbose alternatives.
+ */
+ static final class LongToUnsignedString {
+ @BeforeTemplate
+ String before(long i) {
+ return Refaster.anyOf(UnsignedLongs.toString(i), Long.toUnsignedString(i, 10));
+ }
+
+ @AfterTemplate
+ String after(long i) {
+ return Long.toUnsignedString(i);
+ }
+ }
+
+ /**
+ * Prefer JDK's {@link Integer#toUnsignedString(int,int)} over third-party or more verbose
+ * alternatives.
+ */
+ static final class IntegerToUnsignedStringWithRadix {
+ @BeforeTemplate
+ String before(int i, int radix) {
+ return UnsignedInts.toString(i, radix);
+ }
+
+ @AfterTemplate
+ String after(int i, int radix) {
+ return Integer.toUnsignedString(i, radix);
+ }
+ }
+
+ /**
+ * Prefer JDK's {@link Long#toUnsignedString(long,int)} over third-party or more verbose
+ * alternatives.
+ */
+ static final class LongToUnsignedStringWithRadix {
+ @BeforeTemplate
+ String before(long i, int radix) {
+ return UnsignedLongs.toString(i, radix);
+ }
+
+ @AfterTemplate
+ String after(long i, int radix) {
+ return Long.toUnsignedString(i, radix);
+ }
+ }
}
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..404f7830 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;
@@ -14,6 +13,7 @@ import static java.util.stream.Collectors.toCollection;
import static org.assertj.core.api.Assertions.assertThat;
import static reactor.function.TupleUtils.function;
+import com.github.benmanes.caffeine.cache.AsyncLoadingCache;
import com.google.common.collect.ImmutableCollection;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableSet;
@@ -34,6 +34,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 +42,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 +54,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 +381,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 +483,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 +565,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 +1206,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
@@ -1761,6 +1768,60 @@ final class ReactorRules {
}
}
+ /**
+ * Prefer {@link StepVerifier#verify()} over a dangling {@link
+ * StepVerifier#verifyThenAssertThat()}.
+ */
+ // XXX: Application of this rule (and several others in this class) will cause invalid code if the
+ // result of the rewritten expression is dereferenced. Consider introducing a bug checker that
+ // identifies rules that change the return type of an expression and annotates them accordingly.
+ // The associated annotation can then be used to instruct an annotation processor to generate
+ // corresponding `void` rules that match only statements. This would allow the `Refaster` check to
+ // conditionally skip "not fully safe" rules. This allows conditionally flagging more dubious
+ // code, at the risk of compilation failures. With this rule, for example, we want to explicitly
+ // nudge users towards `StepVerifier.Step#assertNext(Consumer)` or
+ // `StepVerifier.Step#expectNext(Object)`, together with `Step#verifyComplete()`.
+ static final class StepVerifierVerify {
+ @BeforeTemplate
+ StepVerifier.Assertions before(StepVerifier stepVerifier) {
+ return stepVerifier.verifyThenAssertThat();
+ }
+
+ @AfterTemplate
+ Duration after(StepVerifier stepVerifier) {
+ return stepVerifier.verify();
+ }
+ }
+
+ /**
+ * Prefer {@link StepVerifier#verify(Duration)} over a dangling {@link
+ * StepVerifier#verifyThenAssertThat(Duration)}.
+ */
+ static final class StepVerifierVerifyDuration {
+ @BeforeTemplate
+ StepVerifier.Assertions before(StepVerifier stepVerifier, Duration duration) {
+ return stepVerifier.verifyThenAssertThat(duration);
+ }
+
+ @AfterTemplate
+ Duration after(StepVerifier stepVerifier, Duration duration) {
+ return stepVerifier.verify(duration);
+ }
+ }
+
+ /** Don't unnecessarily invoke {@link StepVerifier#verifyLater()} multiple times. */
+ static final class StepVerifierVerifyLater {
+ @BeforeTemplate
+ StepVerifier before(StepVerifier stepVerifier) {
+ return stepVerifier.verifyLater().verifyLater();
+ }
+
+ @AfterTemplate
+ StepVerifier after(StepVerifier stepVerifier) {
+ return stepVerifier.verifyLater();
+ }
+ }
+
/** Don't unnecessarily have {@link StepVerifier.Step} expect no elements. */
static final class StepVerifierStepIdentity {
@BeforeTemplate
@@ -1861,6 +1922,12 @@ final class ReactorRules {
return step.expectErrorMatches(predicate).verify();
}
+ @BeforeTemplate
+ @SuppressWarnings("StepVerifierVerify" /* This is a more specific template. */)
+ StepVerifier.Assertions before2(StepVerifier.LastStep step, Predicate predicate) {
+ return step.expectError().verifyThenAssertThat().hasOperatorErrorMatching(predicate);
+ }
+
@AfterTemplate
Duration after(StepVerifier.LastStep step, Predicate predicate) {
return step.verifyErrorMatches(predicate);
@@ -1883,6 +1950,30 @@ final class ReactorRules {
}
}
+ /**
+ * Prefer {@link StepVerifier.LastStep#verifyErrorSatisfies(Consumer)} with AssertJ over more
+ * contrived alternatives.
+ */
+ static final class StepVerifierLastStepVerifyErrorSatisfiesAssertJ {
+ @BeforeTemplate
+ @SuppressWarnings("StepVerifierVerify" /* This is a more specific template. */)
+ StepVerifier.Assertions before(StepVerifier.LastStep step, Class clazz, String message) {
+ return Refaster.anyOf(
+ step.expectError()
+ .verifyThenAssertThat()
+ .hasOperatorErrorOfType(clazz)
+ .hasOperatorErrorWithMessage(message),
+ step.expectError(clazz).verifyThenAssertThat().hasOperatorErrorWithMessage(message),
+ step.expectErrorMessage(message).verifyThenAssertThat().hasOperatorErrorOfType(clazz));
+ }
+
+ @AfterTemplate
+ @UseImportPolicy(STATIC_IMPORT_ALWAYS)
+ Duration after(StepVerifier.LastStep step, Class clazz, String message) {
+ return step.verifyErrorSatisfies(t -> assertThat(t).isInstanceOf(clazz).hasMessage(message));
+ }
+ }
+
/**
* Prefer {@link StepVerifier.LastStep#verifyErrorMessage(String)} over more verbose alternatives.
*/
@@ -1912,4 +2003,76 @@ 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