diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/LexicographicalAnnotationListing.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/LexicographicalAnnotationListing.java index 3924d8556b..d608cd79f0 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/LexicographicalAnnotationListing.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/LexicographicalAnnotationListing.java @@ -36,6 +36,10 @@ *

The idea behind this checker is that maintaining a sorted sequence simplifies conflict * resolution, and can even avoid it if two branches add the same annotation. */ +// XXX: Currently this checker only flags method-level annotations. It should likely also flag +// type-, field- and parameter-level annotations. +// XXX: Duplicate entries are often a mistake. Consider introducing a similar `BugChecker` that +// flags duplicates. @AutoService(BugChecker.class) @BugPattern( summary = "Sort annotations lexicographically where possible", 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 7eeda27c3c..23c3bc16d1 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 @@ -26,6 +26,7 @@ import org.junit.jupiter.api.function.Executable; import org.junit.jupiter.api.function.ThrowingSupplier; import tech.picnic.errorprone.refaster.annotation.OnlineDocumentation; +import tech.picnic.errorprone.refaster.annotation.TypeMigration; /** * Refaster rules to replace JUnit assertions with AssertJ equivalents. @@ -40,6 +41,264 @@ // `() -> toString()` match both `ThrowingSupplier` and `ThrowingCallable`, but `() -> "constant"` // is only compatible with the former. @OnlineDocumentation +@TypeMigration( + of = Assertions.class, + unmigratedMethods = { + "assertAll(Collection)", + "assertAll(Executable[])", + "assertAll(Stream)", + "assertAll(String, Collection)", + "assertAll(String, Executable[])", + "assertAll(String, Stream)", + "assertArrayEquals(boolean[], boolean[])", + "assertArrayEquals(boolean[], boolean[], String)", + "assertArrayEquals(boolean[], boolean[], Supplier)", + "assertArrayEquals(byte[], byte[])", + "assertArrayEquals(byte[], byte[], String)", + "assertArrayEquals(byte[], byte[], Supplier)", + "assertArrayEquals(char[], char[])", + "assertArrayEquals(char[], char[], String)", + "assertArrayEquals(char[], char[], Supplier)", + "assertArrayEquals(double[], double[])", + "assertArrayEquals(double[], double[], double)", + "assertArrayEquals(double[], double[], double, String)", + "assertArrayEquals(double[], double[], double, Supplier)", + "assertArrayEquals(double[], double[], String)", + "assertArrayEquals(double[], double[], Supplier)", + "assertArrayEquals(float[], float[])", + "assertArrayEquals(float[], float[], float)", + "assertArrayEquals(float[], float[], float, String)", + "assertArrayEquals(float[], float[], float, Supplier)", + "assertArrayEquals(float[], float[], String)", + "assertArrayEquals(float[], float[], Supplier)", + "assertArrayEquals(int[], int[])", + "assertArrayEquals(int[], int[], String)", + "assertArrayEquals(int[], int[], Supplier)", + "assertArrayEquals(long[], long[])", + "assertArrayEquals(long[], long[], String)", + "assertArrayEquals(long[], long[], Supplier)", + "assertArrayEquals(Object[], Object[])", + "assertArrayEquals(Object[], Object[], String)", + "assertArrayEquals(Object[], Object[], Supplier)", + "assertArrayEquals(short[], short[])", + "assertArrayEquals(short[], short[], String)", + "assertArrayEquals(short[], short[], Supplier)", + "assertEquals(Byte, Byte)", + "assertEquals(Byte, byte)", + "assertEquals(byte, Byte)", + "assertEquals(byte, byte)", + "assertEquals(Byte, Byte, String)", + "assertEquals(Byte, byte, String)", + "assertEquals(byte, Byte, String)", + "assertEquals(byte, byte, String)", + "assertEquals(Byte, Byte, Supplier)", + "assertEquals(Byte, byte, Supplier)", + "assertEquals(byte, Byte, Supplier)", + "assertEquals(byte, byte, Supplier)", + "assertEquals(char, char)", + "assertEquals(char, char, String)", + "assertEquals(char, char, Supplier)", + "assertEquals(char, Character)", + "assertEquals(char, Character, String)", + "assertEquals(char, Character, Supplier)", + "assertEquals(Character, char)", + "assertEquals(Character, char, String)", + "assertEquals(Character, char, Supplier)", + "assertEquals(Character, Character)", + "assertEquals(Character, Character, String)", + "assertEquals(Character, Character, Supplier)", + "assertEquals(Double, Double)", + "assertEquals(Double, double)", + "assertEquals(double, Double)", + "assertEquals(double, double)", + "assertEquals(double, double, double)", + "assertEquals(double, double, double, String)", + "assertEquals(double, double, double, Supplier)", + "assertEquals(Double, Double, String)", + "assertEquals(Double, double, String)", + "assertEquals(double, Double, String)", + "assertEquals(double, double, String)", + "assertEquals(Double, Double, Supplier)", + "assertEquals(Double, double, Supplier)", + "assertEquals(double, Double, Supplier)", + "assertEquals(double, double, Supplier)", + "assertEquals(Float, Float)", + "assertEquals(Float, float)", + "assertEquals(float, Float)", + "assertEquals(float, float)", + "assertEquals(float, float, float)", + "assertEquals(float, float, float, String)", + "assertEquals(float, float, float, Supplier)", + "assertEquals(Float, Float, String)", + "assertEquals(Float, float, String)", + "assertEquals(float, Float, String)", + "assertEquals(float, float, String)", + "assertEquals(Float, Float, Supplier)", + "assertEquals(Float, float, Supplier)", + "assertEquals(float, Float, Supplier)", + "assertEquals(float, float, Supplier)", + "assertEquals(int, int)", + "assertEquals(int, int, String)", + "assertEquals(int, int, Supplier)", + "assertEquals(int, Integer)", + "assertEquals(int, Integer, String)", + "assertEquals(int, Integer, Supplier)", + "assertEquals(Integer, int)", + "assertEquals(Integer, int, String)", + "assertEquals(Integer, int, Supplier)", + "assertEquals(Integer, Integer)", + "assertEquals(Integer, Integer, String)", + "assertEquals(Integer, Integer, Supplier)", + "assertEquals(Long, Long)", + "assertEquals(Long, long)", + "assertEquals(long, Long)", + "assertEquals(long, long)", + "assertEquals(Long, Long, String)", + "assertEquals(Long, long, String)", + "assertEquals(long, Long, String)", + "assertEquals(long, long, String)", + "assertEquals(Long, Long, Supplier)", + "assertEquals(Long, long, Supplier)", + "assertEquals(long, Long, Supplier)", + "assertEquals(long, long, Supplier)", + "assertEquals(Object, Object)", + "assertEquals(Object, Object, String)", + "assertEquals(Object, Object, Supplier)", + "assertEquals(Short, Short)", + "assertEquals(Short, short)", + "assertEquals(short, Short)", + "assertEquals(short, short)", + "assertEquals(Short, Short, String)", + "assertEquals(Short, short, String)", + "assertEquals(short, Short, String)", + "assertEquals(short, short, String)", + "assertEquals(Short, Short, Supplier)", + "assertEquals(Short, short, Supplier)", + "assertEquals(short, Short, Supplier)", + "assertEquals(short, short, Supplier)", + "assertFalse(BooleanSupplier)", + "assertFalse(BooleanSupplier, String)", + "assertFalse(BooleanSupplier, Supplier)", + "assertIterableEquals(Iterable, Iterable)", + "assertIterableEquals(Iterable, Iterable, String)", + "assertIterableEquals(Iterable, Iterable, Supplier)", + "assertLinesMatch(List, List)", + "assertLinesMatch(List, List, String)", + "assertLinesMatch(List, List, Supplier)", + "assertLinesMatch(Stream, Stream)", + "assertLinesMatch(Stream, Stream, String)", + "assertLinesMatch(Stream, Stream, Supplier)", + "assertNotEquals(Byte, Byte)", + "assertNotEquals(Byte, byte)", + "assertNotEquals(byte, Byte)", + "assertNotEquals(byte, byte)", + "assertNotEquals(Byte, Byte, String)", + "assertNotEquals(Byte, byte, String)", + "assertNotEquals(byte, Byte, String)", + "assertNotEquals(byte, byte, String)", + "assertNotEquals(Byte, Byte, Supplier)", + "assertNotEquals(Byte, byte, Supplier)", + "assertNotEquals(byte, Byte, Supplier)", + "assertNotEquals(byte, byte, Supplier)", + "assertNotEquals(char, char)", + "assertNotEquals(char, char, String)", + "assertNotEquals(char, char, Supplier)", + "assertNotEquals(char, Character)", + "assertNotEquals(char, Character, String)", + "assertNotEquals(char, Character, Supplier)", + "assertNotEquals(Character, char)", + "assertNotEquals(Character, char, String)", + "assertNotEquals(Character, char, Supplier)", + "assertNotEquals(Character, Character)", + "assertNotEquals(Character, Character, String)", + "assertNotEquals(Character, Character, Supplier)", + "assertNotEquals(Double, Double)", + "assertNotEquals(Double, double)", + "assertNotEquals(double, Double)", + "assertNotEquals(double, double)", + "assertNotEquals(double, double, double)", + "assertNotEquals(double, double, double, String)", + "assertNotEquals(double, double, double, Supplier)", + "assertNotEquals(Double, Double, String)", + "assertNotEquals(Double, double, String)", + "assertNotEquals(double, Double, String)", + "assertNotEquals(double, double, String)", + "assertNotEquals(Double, Double, Supplier)", + "assertNotEquals(Double, double, Supplier)", + "assertNotEquals(double, Double, Supplier)", + "assertNotEquals(double, double, Supplier)", + "assertNotEquals(Float, Float)", + "assertNotEquals(Float, float)", + "assertNotEquals(float, Float)", + "assertNotEquals(float, float)", + "assertNotEquals(float, float, float)", + "assertNotEquals(float, float, float, String)", + "assertNotEquals(float, float, float, Supplier)", + "assertNotEquals(Float, Float, String)", + "assertNotEquals(Float, float, String)", + "assertNotEquals(float, Float, String)", + "assertNotEquals(float, float, String)", + "assertNotEquals(Float, Float, Supplier)", + "assertNotEquals(Float, float, Supplier)", + "assertNotEquals(float, Float, Supplier)", + "assertNotEquals(float, float, Supplier)", + "assertNotEquals(int, int)", + "assertNotEquals(int, int, String)", + "assertNotEquals(int, int, Supplier)", + "assertNotEquals(int, Integer)", + "assertNotEquals(int, Integer, String)", + "assertNotEquals(int, Integer, Supplier)", + "assertNotEquals(Integer, int)", + "assertNotEquals(Integer, int, String)", + "assertNotEquals(Integer, int, Supplier)", + "assertNotEquals(Integer, Integer)", + "assertNotEquals(Integer, Integer, String)", + "assertNotEquals(Integer, Integer, Supplier)", + "assertNotEquals(Long, Long)", + "assertNotEquals(Long, long)", + "assertNotEquals(long, Long)", + "assertNotEquals(long, long)", + "assertNotEquals(Long, Long, String)", + "assertNotEquals(Long, long, String)", + "assertNotEquals(long, Long, String)", + "assertNotEquals(long, long, String)", + "assertNotEquals(Long, Long, Supplier)", + "assertNotEquals(Long, long, Supplier)", + "assertNotEquals(long, Long, Supplier)", + "assertNotEquals(long, long, Supplier)", + "assertNotEquals(Object, Object)", + "assertNotEquals(Object, Object, String)", + "assertNotEquals(Object, Object, Supplier)", + "assertNotEquals(Short, Short)", + "assertNotEquals(Short, short)", + "assertNotEquals(short, Short)", + "assertNotEquals(short, short)", + "assertNotEquals(Short, Short, String)", + "assertNotEquals(Short, short, String)", + "assertNotEquals(short, Short, String)", + "assertNotEquals(short, short, String)", + "assertNotEquals(Short, Short, Supplier)", + "assertNotEquals(Short, short, Supplier)", + "assertNotEquals(short, Short, Supplier)", + "assertNotEquals(short, short, Supplier)", + "assertTimeout(Duration, Executable)", + "assertTimeout(Duration, Executable, String)", + "assertTimeout(Duration, Executable, Supplier)", + "assertTimeout(Duration, ThrowingSupplier)", + "assertTimeout(Duration, ThrowingSupplier, String)", + "assertTimeout(Duration, ThrowingSupplier, Supplier)", + "assertTimeoutPreemptively(Duration, Executable)", + "assertTimeoutPreemptively(Duration, Executable, String)", + "assertTimeoutPreemptively(Duration, Executable, Supplier)", + "assertTimeoutPreemptively(Duration, ThrowingSupplier)", + "assertTimeoutPreemptively(Duration, ThrowingSupplier, String)", + "assertTimeoutPreemptively(Duration, ThrowingSupplier, Supplier)", + "assertTimeoutPreemptively(Duration, ThrowingSupplier, Supplier, TimeoutFailureFactory)", + "assertTrue(BooleanSupplier)", + "assertTrue(BooleanSupplier, String)", + "assertTrue(BooleanSupplier, Supplier)", + "fail(Supplier)" + }) final class JUnitToAssertJRules { private JUnitToAssertJRules() {} 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 89b0973563..e96628984e 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 @@ -29,6 +29,7 @@ import org.testng.Assert; import org.testng.Assert.ThrowingRunnable; import tech.picnic.errorprone.refaster.annotation.OnlineDocumentation; +import tech.picnic.errorprone.refaster.annotation.TypeMigration; /** * Refaster rules that replace TestNG assertions with equivalent AssertJ assertions. @@ -48,32 +49,107 @@ * List> myMaps = new ArrayList<>(); * assertEquals(myMaps, ImmutableList.of(ImmutableMap.of())); * } - * - *

A few {@link Assert} methods are not rewritten: - * - *

*/ // XXX: As-is these rules do not result in a complete migration: // - Expressions containing comments are skipped due to a limitation of Refaster. // - Assertions inside lambda expressions are also skipped. Unclear why. +// XXX: The `assertEquals` tests for this class generally use the same expression for `expected` and +// `actual`, which makes the validation weaker than necessary; fix this. (And investigate whether we +// can introduce validation for this.) @OnlineDocumentation +@TypeMigration( + of = Assert.class, + unmigratedMethods = { + // XXX: Add migrations for the methods below. + "assertEquals(Boolean, Boolean)", + "assertEquals(Boolean, boolean)", + "assertEquals(boolean, Boolean)", + "assertEquals(Boolean, Boolean, String)", + "assertEquals(Boolean, boolean, String)", + "assertEquals(boolean, Boolean, String)", + "assertEquals(Byte, Byte)", + "assertEquals(Byte, byte)", + "assertEquals(byte, Byte)", + "assertEquals(Byte, Byte, String)", + "assertEquals(Byte, byte, String)", + "assertEquals(byte, Byte, String)", + "assertEquals(char, Character)", + "assertEquals(char, Character, String)", + "assertEquals(Character, char)", + "assertEquals(Character, char, String)", + "assertEquals(Character, Character)", + "assertEquals(Character, Character, String)", + "assertEquals(Double, Double)", + "assertEquals(Double, double)", + "assertEquals(double, Double)", + "assertEquals(Double, Double, String)", + "assertEquals(Double, double, String)", + "assertEquals(double, Double, String)", + "assertEquals(double[], double[], double)", + "assertEquals(double[], double[], double, String)", + "assertEquals(Float, Float)", + "assertEquals(Float, float)", + "assertEquals(float, Float)", + "assertEquals(Float, Float, String)", + "assertEquals(Float, float, String)", + "assertEquals(float, Float, String)", + "assertEquals(float[], float[], float)", + "assertEquals(float[], float[], float, String)", + "assertEquals(int, Integer)", + "assertEquals(int, Integer, String)", + "assertEquals(Integer, int)", + "assertEquals(Integer, int, String)", + "assertEquals(Integer, Integer)", + "assertEquals(Integer, Integer, String)", + "assertEquals(Long, Long)", + "assertEquals(Long, long)", + "assertEquals(long, Long)", + "assertEquals(Long, Long, String)", + "assertEquals(Long, long, String)", + "assertEquals(Short, Short)", + "assertEquals(Short, short)", + "assertEquals(short, Short)", + "assertEquals(Short, Short, String)", + "assertEquals(Short, short, String)", + "assertEquals(short, Short, String)", + /* + * These `assertEqualsDeep` methods cannot (easily) be expressed using AssertJ because they + * mix regular equality and array equality: + */ + "assertEqualsDeep(Map, Map)", + "assertEqualsDeep(Map, Map, String)", + "assertEqualsDeep(Set, Set, String)", + // XXX: Add migrations for the methods below. + "assertEqualsNoOrder(Collection, Collection)", + "assertEqualsNoOrder(Collection, Collection, String)", + "assertEqualsNoOrder(Iterator, Iterator)", + "assertEqualsNoOrder(Iterator, Iterator, String)", + "assertListContains(List, Predicate, String)", + "assertListContainsObject(List, T, String)", + "assertListNotContains(List, Predicate, String)", + "assertListNotContainsObject(List, T, String)", + "assertNotEquals(Collection, Collection)", + "assertNotEquals(Collection, Collection, String)", + "assertNotEquals(Iterator, Iterator)", + "assertNotEquals(Iterator, Iterator, String)", + "assertNotEquals(Object[], Object[], String)", + /* + * These `assertNotEqualsDeep` methods cannot (easily) be expressed using AssertJ because they + * mix regular equality and array equality: + */ + "assertNotEqualsDeep(Map, Map)", + "assertNotEqualsDeep(Map, Map, String)", + "assertNotEqualsDeep(Set, Set)", + "assertNotEqualsDeep(Set, Set, String)", + // XXX: Add a migration for this `assertThrows` method. + "assertThrows(String, Class, ThrowingRunnable)", + /* + * These `expectThrows` methods return the caught exception; there is no direct counterpart + * for this in AssertJ. + */ + "expectThrows(Class, ThrowingRunnable)", + "expectThrows(String, Class, ThrowingRunnable)" + }) final class TestNGToAssertJRules { private TestNGToAssertJRules() {} diff --git a/error-prone-guidelines/pom.xml b/error-prone-guidelines/pom.xml index da776aa9de..85cd8489fe 100644 --- a/error-prone-guidelines/pom.xml +++ b/error-prone-guidelines/pom.xml @@ -53,6 +53,16 @@ error-prone-utils provided + + ${project.groupId} + refaster-support + provided + + + com.google.auto + auto-common + provided + com.google.auto.service auto-service-annotations diff --git a/error-prone-guidelines/src/main/java/tech/picnic/errorprone/guidelines/bugpatterns/ExhaustiveRefasterTypeMigration.java b/error-prone-guidelines/src/main/java/tech/picnic/errorprone/guidelines/bugpatterns/ExhaustiveRefasterTypeMigration.java new file mode 100644 index 0000000000..b5c5c0d7a4 --- /dev/null +++ b/error-prone-guidelines/src/main/java/tech/picnic/errorprone/guidelines/bugpatterns/ExhaustiveRefasterTypeMigration.java @@ -0,0 +1,231 @@ +package tech.picnic.errorprone.guidelines.bugpatterns; + +import static com.google.common.base.Verify.verify; +import static com.google.common.collect.ImmutableList.toImmutableList; +import static com.google.errorprone.BugPattern.LinkType.CUSTOM; +import static com.google.errorprone.BugPattern.SeverityLevel.WARNING; +import static com.google.errorprone.BugPattern.StandardTags.LIKELY_ERROR; +import static com.google.errorprone.matchers.ChildMultiMatcher.MatchType.AT_LEAST_ONE; +import static com.google.errorprone.matchers.Matchers.annotations; +import static com.google.errorprone.matchers.Matchers.isType; +import static java.util.Comparator.comparing; +import static java.util.stream.Collectors.toCollection; +import static tech.picnic.errorprone.utils.Documentation.BUG_PATTERNS_BASE_URL; + +import com.google.auto.common.AnnotationMirrors; +import com.google.auto.service.AutoService; +import com.google.common.collect.ImmutableList; +import com.google.common.collect.Streams; +import com.google.errorprone.BugPattern; +import com.google.errorprone.VisitorState; +import com.google.errorprone.bugpatterns.BugChecker; +import com.google.errorprone.bugpatterns.BugChecker.ClassTreeMatcher; +import com.google.errorprone.fixes.SuggestedFixes; +import com.google.errorprone.matchers.Description; +import com.google.errorprone.matchers.MultiMatcher; +import com.google.errorprone.matchers.MultiMatcher.MultiMatchResult; +import com.google.errorprone.refaster.annotation.BeforeTemplate; +import com.google.errorprone.util.ASTHelpers; +import com.google.errorprone.util.Signatures; +import com.sun.source.tree.AnnotationTree; +import com.sun.source.tree.ClassTree; +import com.sun.source.tree.MethodInvocationTree; +import com.sun.source.tree.MethodTree; +import com.sun.source.tree.NewClassTree; +import com.sun.source.tree.Tree; +import com.sun.source.util.TreeScanner; +import com.sun.tools.javac.code.Attribute; +import com.sun.tools.javac.code.Symbol.ClassSymbol; +import com.sun.tools.javac.code.Symbol.MethodSymbol; +import com.sun.tools.javac.code.Symbol.TypeSymbol; +import com.sun.tools.javac.util.Constants; +import java.util.Comparator; +import java.util.HashMap; +import java.util.HashSet; +import java.util.Map; +import java.util.Set; +import java.util.function.Consumer; +import javax.lang.model.element.AnnotationMirror; +import javax.lang.model.element.AnnotationValue; +import javax.lang.model.element.Modifier; +import org.jspecify.annotations.Nullable; + +/** + * A {@link BugChecker} that validates the claim made by {@link + * tech.picnic.errorprone.refaster.annotation.TypeMigration} annotations. + */ +// XXX: As-is this checker assumes that a method is fully migrated if it is invoked inside at least +// one `@BeforeTemplate` method. A stronger check would be to additionally verify that: +// 1. Such invocations are not conditionally matched. That is, there should be no constraint on +// their context (i.e. any surrounding code), and their parameters must be `@BeforeTemplate` +// method parameters with types that are not more restrictive than those of the method itself. +// Additionally, the result of non-void methods should be "returned" by the `@BeforeTemplate` +// method, so that Refaster will match any expression, rather than just statements. (One caveat +// with this "context-independent migrations only" approach is that APIs often expose methods +// that are only useful in combination with other methods of the API; insisting that such methods +// are migrated in isolation is unreasonable.) +// 2. Where relevant, method references should also be migrated. (TBD what "relevant" means in this +// case, and whether in fact method reference matchers can be _derived_ from the associated +// method invocation matchers.) +// XXX: This checker currently does no concern itself with public fields. Consider adding support +// for those. +@AutoService(BugChecker.class) +@BugPattern( + summary = + "The set of unmigrated methods listed by the `@TypeMigration` annotation must be minimal " + + "yet exhaustive", + link = BUG_PATTERNS_BASE_URL + "ExhaustiveRefasterTypeMigration", + linkType = CUSTOM, + severity = WARNING, + tags = LIKELY_ERROR) +public final class ExhaustiveRefasterTypeMigration extends BugChecker implements ClassTreeMatcher { + private static final long serialVersionUID = 1L; + private static final MultiMatcher IS_TYPE_MIGRATION = + annotations(AT_LEAST_ONE, isType("tech.picnic.errorprone.refaster.annotation.TypeMigration")); + private static final MultiMatcher HAS_BEFORE_TEMPLATE = + annotations(AT_LEAST_ONE, isType(BeforeTemplate.class.getCanonicalName())); + private static final String TYPE_MIGRATION_TYPE_ELEMENT = "of"; + private static final String TYPE_MIGRATION_UNMIGRATED_METHODS_ELEMENT = "unmigratedMethods"; + + /** Instantiates a new {@link ExhaustiveRefasterTypeMigration} instance. */ + public ExhaustiveRefasterTypeMigration() {} + + @Override + public Description matchClass(ClassTree tree, VisitorState state) { + MultiMatchResult migrationAnnotations = + IS_TYPE_MIGRATION.multiMatchResult(tree, state); + if (!migrationAnnotations.matches()) { + return Description.NO_MATCH; + } + + AnnotationTree migrationAnnotation = migrationAnnotations.onlyMatchingNode(); + AnnotationMirror annotationMirror = ASTHelpers.getAnnotationMirror(migrationAnnotation); + TypeSymbol migratedType = getMigratedType(annotationMirror); + if (migratedType.asType().isPrimitive() || !(migratedType instanceof ClassSymbol)) { + return buildDescription(migrationAnnotation) + .setMessage(String.format("Migration of type '%s' is unsupported", migratedType)) + .build(); + } + + ImmutableList methodsClaimedUnmigrated = getMethodsClaimedUnmigrated(annotationMirror); + ImmutableList unmigratedMethods = + getMethodsDefinitelyUnmigrated( + tree, (ClassSymbol) migratedType, signatureOrder(methodsClaimedUnmigrated), state); + + if (unmigratedMethods.equals(methodsClaimedUnmigrated)) { + return Description.NO_MATCH; + } + + /* + * The `@TypeMigration` annotation lists a different set of unmigrated methods than the one + * produced by our analysis; suggest a replacement. + */ + // XXX: `updateAnnotationArgumentValues` will prepend the new attribute argument if it is not + // already present. It would be nicer if it _appended_ the new attribute. + return describeMatch( + migrationAnnotation, + SuggestedFixes.updateAnnotationArgumentValues( + migrationAnnotation, + state, + TYPE_MIGRATION_UNMIGRATED_METHODS_ELEMENT, + unmigratedMethods.stream().map(Constants::format).collect(toImmutableList())) + .build()); + } + + private static TypeSymbol getMigratedType(AnnotationMirror migrationAnnotation) { + AnnotationValue value = + AnnotationMirrors.getAnnotationValue(migrationAnnotation, TYPE_MIGRATION_TYPE_ELEMENT); + verify( + value instanceof Attribute.Class, + "Value of annotation element `%s` is '%s' rather than a class", + TYPE_MIGRATION_TYPE_ELEMENT, + value); + return ((Attribute.Class) value).classType.tsym; + } + + private static ImmutableList getMethodsClaimedUnmigrated( + AnnotationMirror migrationAnnotation) { + AnnotationValue value = + AnnotationMirrors.getAnnotationValue( + migrationAnnotation, TYPE_MIGRATION_UNMIGRATED_METHODS_ELEMENT); + verify( + value instanceof Attribute.Array, + "Value of annotation element `%s` is '%s' rather than an array", + TYPE_MIGRATION_UNMIGRATED_METHODS_ELEMENT, + value); + return ((Attribute.Array) value) + .getValue().stream().map(a -> a.getValue().toString()).collect(toImmutableList()); + } + + // XXX: Once only JDK 14 and above are supported, change the + // `m.getModifiers().contains(Modifier.PUBLIC)` check to just `m.isPublic()`. + private static ImmutableList getMethodsDefinitelyUnmigrated( + ClassTree tree, ClassSymbol migratedType, Comparator comparator, VisitorState state) { + Set publicMethods = + Streams.stream( + ASTHelpers.scope(migratedType.members()) + .getSymbols( + m -> + m.getModifiers().contains(Modifier.PUBLIC) + && m instanceof MethodSymbol)) + .map(MethodSymbol.class::cast) + .collect(toCollection(HashSet::new)); + + /* Remove methods that *appear* to be migrated. Note that this is an imperfect heuristic. */ + removeMethodsInvokedInBeforeTemplateMethods(tree, publicMethods, state); + + return publicMethods.stream() + .map(m -> Signatures.prettyMethodSignature(migratedType, m)) + .sorted(comparator) + .collect(toImmutableList()); + } + + /** + * Creates a {@link Comparator} that orders method signatures to match the given list of + * signatures, with any signatures not listed ordered first, lexicographically. + * + * @implNote This method does not use {@code comparing(list::indexOf)}, as that would make each + * comparison a linear, rather than constant-time operation. + */ + private static Comparator signatureOrder(ImmutableList existingOrder) { + Map knownEntries = new HashMap<>(); + for (int i = 0; i < existingOrder.size(); i++) { + knownEntries.putIfAbsent(existingOrder.get(i), i); + } + + // XXX: The lexicographical order applied to unknown entries aims to match the order applied by + // the `LexicographicalAnnotationAttributeListing` check; consider deduplicating this logic. + return comparing((String v) -> knownEntries.getOrDefault(v, -1)) + .thenComparing(String.CASE_INSENSITIVE_ORDER); + } + + /** + * Removes from the given set of {@link MethodSymbol}s the ones that refer to a method that is + * invoked inside a {@link com.google.errorprone.refaster.annotation.BeforeTemplate} method inside + * the specified {@link ClassTree}. + */ + private static void removeMethodsInvokedInBeforeTemplateMethods( + ClassTree tree, Set candidates, VisitorState state) { + new TreeScanner<@Nullable Void, Consumer>() { + @Override + public @Nullable Void visitMethod(MethodTree tree, Consumer sink) { + return HAS_BEFORE_TEMPLATE.matches(tree, state) + ? super.visitMethod(tree, candidates::remove) + : null; + } + + @Override + public @Nullable Void visitNewClass(NewClassTree tree, Consumer sink) { + sink.accept(ASTHelpers.getSymbol(tree)); + return super.visitNewClass(tree, sink); + } + + @Override + public @Nullable Void visitMethodInvocation( + MethodInvocationTree tree, Consumer sink) { + sink.accept(ASTHelpers.getSymbol(tree)); + return super.visitMethodInvocation(tree, sink); + } + }.scan(tree, s -> {}); + } +} diff --git a/error-prone-guidelines/src/test/java/tech/picnic/errorprone/guidelines/bugpatterns/ExhaustiveRefasterTypeMigrationTest.java b/error-prone-guidelines/src/test/java/tech/picnic/errorprone/guidelines/bugpatterns/ExhaustiveRefasterTypeMigrationTest.java new file mode 100644 index 0000000000..3e5fc44c0d --- /dev/null +++ b/error-prone-guidelines/src/test/java/tech/picnic/errorprone/guidelines/bugpatterns/ExhaustiveRefasterTypeMigrationTest.java @@ -0,0 +1,263 @@ +package tech.picnic.errorprone.guidelines.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 ExhaustiveRefasterTypeMigrationTest { + @Test + void identification() { + CompilationTestHelper.newInstance(ExhaustiveRefasterTypeMigration.class, getClass()) + .addSourceLines( + "Util.java", + "class Util {", + " public static int CONSTANT = 42;", + "", + " public static void publicStaticVoidMethod() {}", + "", + " static void packagePrivateStaticVoidMethod() {}", + "", + " protected static void protectedStaticVoidMethod() {}", + "", + " private static void privateStaticVoidMethod() {}", + "", + " public static int publicStaticIntMethod2() {", + " return 0;", + " }", + "", + " public String publicStringMethodWithArg(int arg) {", + " return String.valueOf(arg);", + " }", + "}") + .addSourceLines( + "A.java", + "import com.google.errorprone.refaster.annotation.AfterTemplate;", + "import com.google.errorprone.refaster.annotation.BeforeTemplate;", + "import tech.picnic.errorprone.refaster.annotation.TypeMigration;", + "", + "class A {", + " class UnannotatedEmptyClass {}", + "", + " // BUG: Diagnostic contains: Migration of type 'int' is unsupported", + " @TypeMigration(of = int.class)", + " class AnnotatedWithPrimitive {}", + "", + " @TypeMigration(", + " of = Util.class,", + " unmigratedMethods = {", + " \"publicStaticIntMethod2()\",", + " \"publicStringMethodWithArg(int)\",", + " \"publicStaticVoidMethod()\"", + " })", + " class AnnotatedEmptyClass {}", + "", + " @TypeMigration(", + " of = Util.class,", + " unmigratedMethods = {", + " \"publicStaticVoidMethod()\",", + " \"publicStringMethodWithArg(int)\",", + " \"publicStaticIntMethod2()\"", + " })", + " class AnnotatedEmptyClassWithUnsortedMethodListing {}", + "", + " class UnannotatedTemplate {", + " @BeforeTemplate", + " void before(int value) {", + " Util.publicStaticVoidMethod();", + " Util.publicStaticIntMethod2();", + " new Util().publicStringMethodWithArg(value);", + " }", + " }", + "", + " @TypeMigration(", + " of = Util.class,", + " unmigratedMethods = {", + " \"publicStaticIntMethod2()\",", + " \"publicStringMethodWithArg(int)\",", + " \"publicStaticVoidMethod()\"", + " })", + " class AnnotatedWithoutBeforeTemplate {", + " {", + " Util.publicStaticIntMethod2();", + " }", + "", + " @AfterTemplate", + " void after(int value) {", + " Util.publicStaticVoidMethod();", + " new Util().publicStringMethodWithArg(value);", + " }", + " }", + "", + " @TypeMigration(of = Util.class)", + " class AnnotatedFullyMigrated {", + " @BeforeTemplate", + " void before() {", + " new Util().publicStringMethodWithArg(Util.publicStaticIntMethod2());", + " }", + "", + " @BeforeTemplate", + " void before2() {", + " Util.publicStaticVoidMethod();", + " }", + " }", + "", + " @TypeMigration(of = Util.class, unmigratedMethods = \"publicStringMethodWithArg(int)\")", + " class AnnotatedPartiallyMigrated {", + " @BeforeTemplate", + " void before() {", + " Util.publicStaticVoidMethod();", + " Util.publicStaticIntMethod2();", + " }", + " }", + "", + " // BUG: Diagnostic contains: The set of unmigrated methods listed by the `@TypeMigration`", + " // annotation must be minimal yet exhaustive", + " @TypeMigration(of = Util.class, unmigratedMethods = \"publicStringMethodWithArg(int)\")", + " class AnnotatedWithIncompleteMethodListing {", + " @BeforeTemplate", + " void before() {", + " Util.publicStaticIntMethod2();", + " }", + " }", + "", + " // BUG: Diagnostic contains: The set of unmigrated methods listed by the `@TypeMigration`", + " // annotation must be minimal yet exhaustive", + " @TypeMigration(", + " of = Util.class,", + " unmigratedMethods = {\"publicStaticIntMethod2()\", \"publicStringMethodWithArg(int)\"})", + " class AnnotatedWithMigratedMethodReference {", + " @BeforeTemplate", + " void before() {", + " Util.publicStaticVoidMethod();", + " Util.publicStaticIntMethod2();", + " }", + " }", + "", + " // BUG: Diagnostic contains: The set of unmigrated methods listed by the `@TypeMigration`", + " // annotation must be minimal yet exhaustive", + " @TypeMigration(", + " of = Util.class,", + " unmigratedMethods = {\"extra\", \"publicStringMethodWithArg(int)\"})", + " class AnnotatedWithUnknownMethodReference {", + " @BeforeTemplate", + " void before() {", + " Util.publicStaticVoidMethod();", + " Util.publicStaticIntMethod2();", + " }", + " }", + "}") + .doTest(); + } + + @Test + void replacement() { + BugCheckerRefactoringTestHelper.newInstance(ExhaustiveRefasterTypeMigration.class, getClass()) + .addInputLines( + "Util.java", + "public final class Util {", + " public static void publicStaticVoidMethod() {}", + "", + " public static int publicStaticIntMethod2() {", + " return 0;", + " }", + "", + " public String publicStringMethodWithArg(int arg) {", + " return String.valueOf(arg);", + " }", + "", + " public String publicStringMethodWithArg(String arg) {", + " return arg;", + " }", + "}") + .expectUnchanged() + .addInputLines( + "A.java", + "import com.google.errorprone.refaster.annotation.BeforeTemplate;", + "import tech.picnic.errorprone.refaster.annotation.TypeMigration;", + "", + "class A {", + " @TypeMigration(of = Util.class)", + " class AnnotatedWithoutMethodListing {", + " {", + " new Util().publicStringMethodWithArg(1);", + " }", + "", + " @BeforeTemplate", + " void before() {", + " Util.publicStaticIntMethod2();", + " }", + " }", + "", + " @TypeMigration(", + " of = Util.class,", + " unmigratedMethods = {\"publicStaticIntMethod2()\", \"extra\", \"publicStringMethodWithArg(int)\"})", + " class AnnotatedWithIncorrectMethodReference {", + " @BeforeTemplate", + " void before() {", + " new Util().publicStringMethodWithArg(\"1\");", + " Util.publicStaticVoidMethod();", + " Util.publicStaticIntMethod2();", + " }", + " }", + "", + " @TypeMigration(", + " of = Util.class,", + " unmigratedMethods = {\"publicStaticVoidMethod()\", \"publicStaticVoidMethod()\"})", + " class AnnotatedWithDuplicateMethodReference {", + " @BeforeTemplate", + " void before() {", + " new Util().publicStringMethodWithArg(1);", + " new Util().publicStringMethodWithArg(\"1\");", + " Util.publicStaticIntMethod2();", + " }", + " }", + "}") + .addOutputLines( + "A.java", + "import com.google.errorprone.refaster.annotation.BeforeTemplate;", + "import tech.picnic.errorprone.refaster.annotation.TypeMigration;", + "", + "class A {", + " @TypeMigration(", + " unmigratedMethods = {", + " \"publicStaticVoidMethod()\",", + " \"publicStringMethodWithArg(int)\",", + " \"publicStringMethodWithArg(String)\",", + " \"Util()\"", + " },", + " of = Util.class)", + " class AnnotatedWithoutMethodListing {", + " {", + " new Util().publicStringMethodWithArg(1);", + " }", + "", + " @BeforeTemplate", + " void before() {", + " Util.publicStaticIntMethod2();", + " }", + " }", + "", + " @TypeMigration(of = Util.class, unmigratedMethods = \"publicStringMethodWithArg(int)\")", + " class AnnotatedWithIncorrectMethodReference {", + " @BeforeTemplate", + " void before() {", + " new Util().publicStringMethodWithArg(\"1\");", + " Util.publicStaticVoidMethod();", + " Util.publicStaticIntMethod2();", + " }", + " }", + "", + " @TypeMigration(of = Util.class, unmigratedMethods = \"publicStaticVoidMethod()\")", + " class AnnotatedWithDuplicateMethodReference {", + " @BeforeTemplate", + " void before() {", + " new Util().publicStringMethodWithArg(1);", + " new Util().publicStringMethodWithArg(\"1\");", + " Util.publicStaticIntMethod2();", + " }", + " }", + "}") + .doTest(TestMode.TEXT_MATCH); + } +} diff --git a/refaster-support/src/main/java/tech/picnic/errorprone/refaster/annotation/TypeMigration.java b/refaster-support/src/main/java/tech/picnic/errorprone/refaster/annotation/TypeMigration.java new file mode 100644 index 0000000000..d6d8ae4c04 --- /dev/null +++ b/refaster-support/src/main/java/tech/picnic/errorprone/refaster/annotation/TypeMigration.java @@ -0,0 +1,37 @@ +package tech.picnic.errorprone.refaster.annotation; + +import java.lang.annotation.ElementType; +import java.lang.annotation.Retention; +import java.lang.annotation.RetentionPolicy; +import java.lang.annotation.Target; + +/** + * Indicates that a Refaster rule or group of Refaster rules is intended to migrate away from the + * indicated type. + */ +// XXX: Add support for `#unmigratedFields()`. +// XXX: Consider making this annotation `@Repeatable`, for cases where a single Refaster rule +// collection migrates away from multiple types. +@Retention(RetentionPolicy.SOURCE) +@Target(ElementType.TYPE) +public @interface TypeMigration { + /** + * The type migrated away from. + * + * @return The type generally used in the {@link + * com.google.errorprone.refaster.annotation.BeforeTemplate} methods of annotated Refaster + * rule(s). + */ + Class of(); + + /** + * The signatures of public methods and constructors that are not (yet) migrated by the annotated + * Refaster rule(s). + * + * @return A possibly empty enumeration of method and constructor signatures, formatted according + * to {@link + * com.google.errorprone.util.Signatures#prettyMethodSignature(com.sun.tools.javac.code.Symbol.ClassSymbol, + * com.sun.tools.javac.code.Symbol.MethodSymbol)}. + */ + String[] unmigratedMethods() default {}; +}