From 82f15192397cf4b26eb48d7a976bb5bf50c6d3fd Mon Sep 17 00:00:00 2001 From: Stephan Schroevers Date: Sun, 1 Jan 2023 17:55:32 +0100 Subject: [PATCH 1/9] Introduce `ExhaustiveRefasterTypeMigration` check The new `@TypeMigration` annotation can be placed on Refaster rule collections to indicate that they migrate most or all public methods of an indicated type. The new check validates the claim made by the annotation. --- .../ExhaustiveRefasterTypeMigration.java | 224 +++++++++++++++ .../LexicographicalAnnotationListing.java | 2 + .../refasterrules/JUnitToAssertJRules.java | 259 ++++++++++++++++++ .../refasterrules/TestNGToAssertJRules.java | 118 ++++++-- .../ExhaustiveRefasterTypeMigrationTest.java | 252 +++++++++++++++++ .../refaster/annotation/TypeMigration.java | 37 +++ 6 files changed, 871 insertions(+), 21 deletions(-) create mode 100644 error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/ExhaustiveRefasterTypeMigration.java create mode 100644 error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/ExhaustiveRefasterTypeMigrationTest.java create mode 100644 refaster-support/src/main/java/tech/picnic/errorprone/refaster/annotation/TypeMigration.java diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/ExhaustiveRefasterTypeMigration.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/ExhaustiveRefasterTypeMigration.java new file mode 100644 index 0000000000..dac78ccf29 --- /dev/null +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/ExhaustiveRefasterTypeMigration.java @@ -0,0 +1,224 @@ +package tech.picnic.errorprone.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.Comparator.naturalOrder; +import static java.util.stream.Collectors.toCollection; +import static tech.picnic.errorprone.bugpatterns.util.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.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 javax.lang.model.element.AnnotationMirror; +import javax.lang.model.element.AnnotationValue; +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("com.google.errorprone.refaster.annotation.BeforeTemplate")); + 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()); + } + + private static ImmutableList getMethodsDefinitelyUnmigrated( + ClassTree tree, ClassSymbol migratedType, Comparator comparator, VisitorState state) { + Set publicMethods = + Streams.stream( + ASTHelpers.scope(migratedType.members()) + .getSymbols(m -> m.isPublic() && 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); + } + + return comparing((String v) -> knownEntries.getOrDefault(v, -1)).thenComparing(naturalOrder()); + } + + /** + * 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, Boolean>() { + @Override + public @Nullable Void visitMethod(MethodTree tree, Boolean inBeforeTemplate) { + return HAS_BEFORE_TEMPLATE.matches(tree, state) ? super.visitMethod(tree, true) : null; + } + + @Override + public @Nullable Void visitNewClass(NewClassTree tree, Boolean inBeforeTemplate) { + if (inBeforeTemplate) { + candidates.remove(ASTHelpers.getSymbol(tree)); + } + + return super.visitNewClass(tree, inBeforeTemplate); + } + + @Override + public @Nullable Void visitMethodInvocation( + MethodInvocationTree tree, Boolean inBeforeTemplate) { + if (inBeforeTemplate) { + candidates.remove(ASTHelpers.getSymbol(tree)); + } + + return super.visitMethodInvocation(tree, inBeforeTemplate); + } + }.scan(tree, false); + } +} 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..9ef9e8e34e 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,8 @@ *

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: 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: - * - *

    - *
  • These methods cannot (easily) be expressed using AssertJ because they mix regular equality - * and array equality: - *
      - *
    • {@link Assert#assertEqualsDeep(Map, Map)} - *
    • {@link Assert#assertEqualsDeep(Map, Map, String)} - *
    • {@link Assert#assertEqualsDeep(Set, Set, String)} - *
    • {@link Assert#assertNotEqualsDeep(Map, Map)} - *
    • {@link Assert#assertNotEqualsDeep(Map, Map, String)} - *
    • {@link Assert#assertNotEqualsDeep(Set, Set)} - *
    • {@link Assert#assertNotEqualsDeep(Set, Set, String)} - *
    - *
  • This method returns the caught exception; there is no direct counterpart for this in - * AssertJ: - *
      - *
    • {@link Assert#expectThrows(Class, ThrowingRunnable)} - *
    - *
*/ // 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-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/ExhaustiveRefasterTypeMigrationTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/ExhaustiveRefasterTypeMigrationTest.java new file mode 100644 index 0000000000..0030a63c70 --- /dev/null +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/ExhaustiveRefasterTypeMigrationTest.java @@ -0,0 +1,252 @@ +package tech.picnic.errorprone.bugpatterns; + +import com.google.errorprone.BugCheckerRefactoringTestHelper; +import com.google.errorprone.BugCheckerRefactoringTestHelper.TestMode; +import com.google.errorprone.CompilationTestHelper; +import org.junit.jupiter.api.Test; + +final class 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);", + " }", + "}") + .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() {", + " Util.publicStaticVoidMethod();", + " Util.publicStaticIntMethod2();", + " }", + " }", + "", + " @TypeMigration(", + " of = Util.class,", + " unmigratedMethods = {\"publicStaticVoidMethod()\", \"publicStaticVoidMethod()\"})", + " class AnnotatedWithDuplicateMethodReference {", + " @BeforeTemplate", + " void before() {", + " 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 = {\"Util()\", \"publicStaticVoidMethod()\", \"publicStringMethodWithArg(int)\"},", + " of = Util.class)", + " class AnnotatedWithoutMethodListing {", + " {", + " new Util().publicStringMethodWithArg(1);", + " }", + "", + " @BeforeTemplate", + " void before() {", + " Util.publicStaticIntMethod2();", + " }", + " }", + "", + " @TypeMigration(", + " of = Util.class,", + " unmigratedMethods = {\"Util()\", \"publicStringMethodWithArg(int)\"})", + " class AnnotatedWithIncorrectMethodReference {", + " @BeforeTemplate", + " void before() {", + " Util.publicStaticVoidMethod();", + " Util.publicStaticIntMethod2();", + " }", + " }", + "", + " @TypeMigration(of = Util.class, unmigratedMethods = \"publicStaticVoidMethod()\")", + " class AnnotatedWithDuplicateMethodReference {", + " @BeforeTemplate", + " void before() {", + " 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..1cb97276b9 --- /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 rules +// collection migrates away from multiple types. +@Target(ElementType.TYPE) +@Retention(RetentionPolicy.SOURCE) +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 {}; +} From 3bf11cb5f4239d8b5865d834b6b3c9fa6d087bec Mon Sep 17 00:00:00 2001 From: Stephan Schroevers Date: Tue, 29 Aug 2023 09:58:07 +0300 Subject: [PATCH 2/9] Fix JDK 11 compatibility --- .../bugpatterns/ExhaustiveRefasterTypeMigration.java | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/ExhaustiveRefasterTypeMigration.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/ExhaustiveRefasterTypeMigration.java index dac78ccf29..a7a7c90d85 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/ExhaustiveRefasterTypeMigration.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/ExhaustiveRefasterTypeMigration.java @@ -46,6 +46,7 @@ import java.util.Set; import javax.lang.model.element.AnnotationMirror; import javax.lang.model.element.AnnotationValue; +import javax.lang.model.element.Modifier; import org.jspecify.annotations.Nullable; /** @@ -154,12 +155,17 @@ private static ImmutableList getMethodsClaimedUnmigrated( .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.isPublic() && m instanceof MethodSymbol)) + .getSymbols( + m -> + m.getModifiers().contains(Modifier.PUBLIC) + && m instanceof MethodSymbol)) .map(MethodSymbol.class::cast) .collect(toCollection(HashSet::new)); From fc70c3d34d74aec470403ae74d864f4ef4bae9b3 Mon Sep 17 00:00:00 2001 From: Stephan Schroevers Date: Tue, 29 Aug 2023 10:25:00 +0300 Subject: [PATCH 3/9] Resolve some Sonarcloud warnings --- .../ExhaustiveRefasterTypeMigration.java | 32 +++++++++---------- 1 file changed, 15 insertions(+), 17 deletions(-) diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/ExhaustiveRefasterTypeMigration.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/ExhaustiveRefasterTypeMigration.java index a7a7c90d85..18c512ed0a 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/ExhaustiveRefasterTypeMigration.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/ExhaustiveRefasterTypeMigration.java @@ -44,6 +44,7 @@ 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; @@ -71,7 +72,8 @@ @AutoService(BugChecker.class) @BugPattern( summary = - "The set of unmigrated methods listed by the `@TypeMigration` annotation must be minimal yet exhaustive", + "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, @@ -201,30 +203,26 @@ private static Comparator signatureOrder(ImmutableList existingO */ private static void removeMethodsInvokedInBeforeTemplateMethods( ClassTree tree, Set candidates, VisitorState state) { - new TreeScanner<@Nullable Void, Boolean>() { + new TreeScanner<@Nullable Void, Consumer>() { @Override - public @Nullable Void visitMethod(MethodTree tree, Boolean inBeforeTemplate) { - return HAS_BEFORE_TEMPLATE.matches(tree, state) ? super.visitMethod(tree, true) : null; + 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, Boolean inBeforeTemplate) { - if (inBeforeTemplate) { - candidates.remove(ASTHelpers.getSymbol(tree)); - } - - return super.visitNewClass(tree, inBeforeTemplate); + 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, Boolean inBeforeTemplate) { - if (inBeforeTemplate) { - candidates.remove(ASTHelpers.getSymbol(tree)); - } - - return super.visitMethodInvocation(tree, inBeforeTemplate); + MethodInvocationTree tree, Consumer sink) { + sink.accept(ASTHelpers.getSymbol(tree)); + return super.visitMethodInvocation(tree, sink); } - }.scan(tree, false); + }.scan(tree, s -> {}); } } From 0ec05b73404de1ed59ba79765c9fcb5471c47172 Mon Sep 17 00:00:00 2001 From: Stephan Schroevers Date: Sun, 28 Jan 2024 13:18:38 +0100 Subject: [PATCH 4/9] Post-rebase fixes --- error-prone-contrib/pom.xml | 5 +++++ .../bugpatterns/ExhaustiveRefasterTypeMigration.java | 3 ++- 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/error-prone-contrib/pom.xml b/error-prone-contrib/pom.xml index d7cf45b813..590468a153 100644 --- a/error-prone-contrib/pom.xml +++ b/error-prone-contrib/pom.xml @@ -67,6 +67,11 @@ jackson-annotations provided + + com.google.auto + auto-common + provided + com.google.auto.service auto-service-annotations diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/ExhaustiveRefasterTypeMigration.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/ExhaustiveRefasterTypeMigration.java index 18c512ed0a..3ea288eb66 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/ExhaustiveRefasterTypeMigration.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/ExhaustiveRefasterTypeMigration.java @@ -25,6 +25,7 @@ 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; @@ -83,7 +84,7 @@ public final class ExhaustiveRefasterTypeMigration extends BugChecker implements 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("com.google.errorprone.refaster.annotation.BeforeTemplate")); + 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"; From a5049bdc84a65938fb5e76e8a233d3979bcc3c9a Mon Sep 17 00:00:00 2001 From: Stephan Schroevers Date: Sun, 28 Jan 2024 18:21:04 +0100 Subject: [PATCH 5/9] Test overload support --- .../ExhaustiveRefasterTypeMigration.java | 6 ++++-- .../ExhaustiveRefasterTypeMigrationTest.java | 19 +++++++++++++++---- 2 files changed, 19 insertions(+), 6 deletions(-) diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/ExhaustiveRefasterTypeMigration.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/ExhaustiveRefasterTypeMigration.java index 3ea288eb66..78f539d26a 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/ExhaustiveRefasterTypeMigration.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/ExhaustiveRefasterTypeMigration.java @@ -9,7 +9,6 @@ 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.Comparator.naturalOrder; import static java.util.stream.Collectors.toCollection; import static tech.picnic.errorprone.bugpatterns.util.Documentation.BUG_PATTERNS_BASE_URL; @@ -194,7 +193,10 @@ private static Comparator signatureOrder(ImmutableList existingO knownEntries.putIfAbsent(existingOrder.get(i), i); } - return comparing((String v) -> knownEntries.getOrDefault(v, -1)).thenComparing(naturalOrder()); + // 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); } /** diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/ExhaustiveRefasterTypeMigrationTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/ExhaustiveRefasterTypeMigrationTest.java index 0030a63c70..f684d28593 100644 --- a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/ExhaustiveRefasterTypeMigrationTest.java +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/ExhaustiveRefasterTypeMigrationTest.java @@ -165,6 +165,10 @@ void replacement() { " public String publicStringMethodWithArg(int arg) {", " return String.valueOf(arg);", " }", + "", + " public String publicStringMethodWithArg(String arg) {", + " return arg;", + " }", "}") .expectUnchanged() .addInputLines( @@ -191,6 +195,7 @@ void replacement() { " class AnnotatedWithIncorrectMethodReference {", " @BeforeTemplate", " void before() {", + " new Util().publicStringMethodWithArg(\"1\");", " Util.publicStaticVoidMethod();", " Util.publicStaticIntMethod2();", " }", @@ -203,6 +208,7 @@ void replacement() { " @BeforeTemplate", " void before() {", " new Util().publicStringMethodWithArg(1);", + " new Util().publicStringMethodWithArg(\"1\");", " Util.publicStaticIntMethod2();", " }", " }", @@ -214,7 +220,12 @@ void replacement() { "", "class A {", " @TypeMigration(", - " unmigratedMethods = {\"Util()\", \"publicStaticVoidMethod()\", \"publicStringMethodWithArg(int)\"},", + " unmigratedMethods = {", + " \"publicStaticVoidMethod()\",", + " \"publicStringMethodWithArg(int)\",", + " \"publicStringMethodWithArg(String)\",", + " \"Util()\"", + " },", " of = Util.class)", " class AnnotatedWithoutMethodListing {", " {", @@ -227,12 +238,11 @@ void replacement() { " }", " }", "", - " @TypeMigration(", - " of = Util.class,", - " unmigratedMethods = {\"Util()\", \"publicStringMethodWithArg(int)\"})", + " @TypeMigration(of = Util.class, unmigratedMethods = \"publicStringMethodWithArg(int)\")", " class AnnotatedWithIncorrectMethodReference {", " @BeforeTemplate", " void before() {", + " new Util().publicStringMethodWithArg(\"1\");", " Util.publicStaticVoidMethod();", " Util.publicStaticIntMethod2();", " }", @@ -243,6 +253,7 @@ void replacement() { " @BeforeTemplate", " void before() {", " new Util().publicStringMethodWithArg(1);", + " new Util().publicStringMethodWithArg(\"1\");", " Util.publicStaticIntMethod2();", " }", " }", From 42dc15b050354eee0f55e9772cb4f0220f80ee58 Mon Sep 17 00:00:00 2001 From: Stephan Schroevers Date: Sat, 10 Feb 2024 14:15:48 +0100 Subject: [PATCH 6/9] Fix build --- .../errorprone/bugpatterns/ExhaustiveRefasterTypeMigration.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/ExhaustiveRefasterTypeMigration.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/ExhaustiveRefasterTypeMigration.java index 78f539d26a..23d2860290 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/ExhaustiveRefasterTypeMigration.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/ExhaustiveRefasterTypeMigration.java @@ -10,7 +10,7 @@ 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.bugpatterns.util.Documentation.BUG_PATTERNS_BASE_URL; +import static tech.picnic.errorprone.utils.Documentation.BUG_PATTERNS_BASE_URL; import com.google.auto.common.AnnotationMirrors; import com.google.auto.service.AutoService; From 81fb81d675505149261006803e29b8a45b8d6308 Mon Sep 17 00:00:00 2001 From: Stephan Schroevers Date: Sat, 10 Feb 2024 14:26:02 +0100 Subject: [PATCH 7/9] Move the check --- error-prone-contrib/pom.xml | 5 ----- error-prone-guidelines/pom.xml | 10 ++++++++++ .../bugpatterns/ExhaustiveRefasterTypeMigration.java | 2 +- .../ExhaustiveRefasterTypeMigrationTest.java | 2 +- 4 files changed, 12 insertions(+), 7 deletions(-) rename {error-prone-contrib/src/main/java/tech/picnic/errorprone => error-prone-guidelines/src/main/java/tech/picnic/errorprone/guidelines}/bugpatterns/ExhaustiveRefasterTypeMigration.java (99%) rename {error-prone-contrib/src/test/java/tech/picnic/errorprone => error-prone-guidelines/src/test/java/tech/picnic/errorprone/guidelines}/bugpatterns/ExhaustiveRefasterTypeMigrationTest.java (99%) diff --git a/error-prone-contrib/pom.xml b/error-prone-contrib/pom.xml index 590468a153..d7cf45b813 100644 --- a/error-prone-contrib/pom.xml +++ b/error-prone-contrib/pom.xml @@ -67,11 +67,6 @@ jackson-annotations provided - - com.google.auto - auto-common - provided - com.google.auto.service auto-service-annotations 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-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/ExhaustiveRefasterTypeMigration.java b/error-prone-guidelines/src/main/java/tech/picnic/errorprone/guidelines/bugpatterns/ExhaustiveRefasterTypeMigration.java similarity index 99% rename from error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/ExhaustiveRefasterTypeMigration.java rename to error-prone-guidelines/src/main/java/tech/picnic/errorprone/guidelines/bugpatterns/ExhaustiveRefasterTypeMigration.java index 23d2860290..5672afb449 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/ExhaustiveRefasterTypeMigration.java +++ b/error-prone-guidelines/src/main/java/tech/picnic/errorprone/guidelines/bugpatterns/ExhaustiveRefasterTypeMigration.java @@ -1,4 +1,4 @@ -package tech.picnic.errorprone.bugpatterns; +package tech.picnic.errorprone.guidelines.bugpatterns; import static com.google.common.base.Verify.verify; import static com.google.common.collect.ImmutableList.toImmutableList; diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/ExhaustiveRefasterTypeMigrationTest.java b/error-prone-guidelines/src/test/java/tech/picnic/errorprone/guidelines/bugpatterns/ExhaustiveRefasterTypeMigrationTest.java similarity index 99% rename from error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/ExhaustiveRefasterTypeMigrationTest.java rename to error-prone-guidelines/src/test/java/tech/picnic/errorprone/guidelines/bugpatterns/ExhaustiveRefasterTypeMigrationTest.java index f684d28593..3e5fc44c0d 100644 --- a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/ExhaustiveRefasterTypeMigrationTest.java +++ b/error-prone-guidelines/src/test/java/tech/picnic/errorprone/guidelines/bugpatterns/ExhaustiveRefasterTypeMigrationTest.java @@ -1,4 +1,4 @@ -package tech.picnic.errorprone.bugpatterns; +package tech.picnic.errorprone.guidelines.bugpatterns; import com.google.errorprone.BugCheckerRefactoringTestHelper; import com.google.errorprone.BugCheckerRefactoringTestHelper.TestMode; From 76db3dd6bbdcd89cedf095eca5a10f858956f4c3 Mon Sep 17 00:00:00 2001 From: Rick Ossendrijver Date: Sun, 11 Feb 2024 10:13:00 +0100 Subject: [PATCH 8/9] Small typos and a annotation order swap --- .../bugpatterns/ExhaustiveRefasterTypeMigration.java | 2 +- .../picnic/errorprone/refaster/annotation/TypeMigration.java | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) 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 index 5672afb449..b5c5c0d7a4 100644 --- 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 @@ -57,7 +57,7 @@ // 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` +// 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 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 index 1cb97276b9..d6d8ae4c04 100644 --- 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 @@ -10,10 +10,10 @@ * indicated type. */ // XXX: Add support for `#unmigratedFields()`. -// XXX: Consider making this annotation `@Repeatable`, for cases where a single Refaster rules +// XXX: Consider making this annotation `@Repeatable`, for cases where a single Refaster rule // collection migrates away from multiple types. -@Target(ElementType.TYPE) @Retention(RetentionPolicy.SOURCE) +@Target(ElementType.TYPE) public @interface TypeMigration { /** * The type migrated away from. From 3e54b8a5ba3eca2b9c5315323cca25db6501d9ac Mon Sep 17 00:00:00 2001 From: Stephan Schroevers Date: Sun, 11 Feb 2024 12:28:13 +0100 Subject: [PATCH 9/9] Tweak --- .../bugpatterns/LexicographicalAnnotationListing.java | 2 ++ 1 file changed, 2 insertions(+) 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 9ef9e8e34e..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,8 @@ *

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)