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 660458f2..046d95e7 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 @@ -4,10 +4,13 @@ import static com.google.common.collect.ImmutableList.toImmutableList; import static com.google.errorprone.BugPattern.LinkType.CUSTOM; import static com.google.errorprone.BugPattern.SeverityLevel.SUGGESTION; import static com.google.errorprone.BugPattern.StandardTags.STYLE; +import static com.sun.tools.javac.code.TypeAnnotations.AnnotationType.DECLARATION; +import static com.sun.tools.javac.code.TypeAnnotations.AnnotationType.TYPE; import static java.util.Comparator.comparing; import static tech.picnic.errorprone.bugpatterns.util.Documentation.BUG_PATTERNS_BASE_URL; import com.google.auto.service.AutoService; +import com.google.common.base.VerifyException; import com.google.common.collect.ImmutableList; import com.google.common.collect.Streams; import com.google.errorprone.BugPattern; @@ -17,10 +20,14 @@ import com.google.errorprone.bugpatterns.BugChecker.MethodTreeMatcher; import com.google.errorprone.fixes.Fix; import com.google.errorprone.fixes.SuggestedFix; import com.google.errorprone.matchers.Description; +import com.google.errorprone.util.ASTHelpers; import com.sun.source.tree.AnnotationTree; import com.sun.source.tree.MethodTree; +import com.sun.tools.javac.code.Symbol; +import com.sun.tools.javac.code.TypeAnnotations.AnnotationType; +import java.util.Comparator; import java.util.List; -import java.util.Optional; +import org.jspecify.nullness.Nullable; import tech.picnic.errorprone.bugpatterns.util.SourceCode; /** @@ -39,6 +46,9 @@ import tech.picnic.errorprone.bugpatterns.util.SourceCode; public final class LexicographicalAnnotationListing extends BugChecker implements MethodTreeMatcher { private static final long serialVersionUID = 1L; + private static final Comparator<@Nullable AnnotationType> BY_ANNOTATION_TYPE = + (a, b) -> + (a == null || a == DECLARATION) && b == TYPE ? -1 : a == TYPE && b == DECLARATION ? 1 : 0; /** Instantiates a new {@link LexicographicalAnnotationListing} instance. */ public LexicographicalAnnotationListing() {} @@ -50,26 +60,29 @@ public final class LexicographicalAnnotationListing extends BugChecker return Description.NO_MATCH; } - ImmutableList sortedAnnotations = sort(originalOrdering, state); + ImmutableList sortedAnnotations = + sort(originalOrdering, ASTHelpers.getSymbol(tree), state); if (originalOrdering.equals(sortedAnnotations)) { return Description.NO_MATCH; } - Optional fix = tryFixOrdering(originalOrdering, sortedAnnotations, state); - - Description.Builder description = buildDescription(originalOrdering.get(0)); - fix.ifPresent(description::addFix); - return description.build(); + return describeMatch( + originalOrdering.get(0), fixOrdering(originalOrdering, sortedAnnotations, state)); } private static ImmutableList sort( - List annotations, VisitorState state) { + List annotations, Symbol symbol, VisitorState state) { return annotations.stream() - .sorted(comparing(annotation -> SourceCode.treeToString(annotation, state))) + .sorted( + comparing( + (AnnotationTree annotation) -> + ASTHelpers.getAnnotationType(annotation, symbol, state), + BY_ANNOTATION_TYPE) + .thenComparing(annotation -> SourceCode.treeToString(annotation, state))) .collect(toImmutableList()); } - private static Optional tryFixOrdering( + private static Fix fixOrdering( List originalAnnotations, ImmutableList sortedAnnotations, VisitorState state) { @@ -80,6 +93,7 @@ public final class LexicographicalAnnotationListing extends BugChecker SuggestedFix.builder() .replace(original, SourceCode.treeToString(replacement, state))) .reduce(SuggestedFix.Builder::merge) - .map(SuggestedFix.Builder::build); + .map(SuggestedFix.Builder::build) + .orElseThrow(() -> new VerifyException("No annotations were provided")); } } diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/LexicographicalAnnotationListingTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/LexicographicalAnnotationListingTest.java index 8f45b394..287a0e95 100644 --- a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/LexicographicalAnnotationListingTest.java +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/LexicographicalAnnotationListingTest.java @@ -1,7 +1,5 @@ package tech.picnic.errorprone.bugpatterns; -import static com.google.common.base.Predicates.containsPattern; - import com.google.errorprone.BugCheckerRefactoringTestHelper; import com.google.errorprone.BugCheckerRefactoringTestHelper.TestMode; import com.google.errorprone.CompilationTestHelper; @@ -9,9 +7,7 @@ import org.junit.jupiter.api.Test; final class LexicographicalAnnotationListingTest { private final CompilationTestHelper compilationTestHelper = - CompilationTestHelper.newInstance(LexicographicalAnnotationListing.class, getClass()) - .expectErrorMessage( - "X", containsPattern("Sort annotations lexicographically where possible")); + CompilationTestHelper.newInstance(LexicographicalAnnotationListing.class, getClass()); private final BugCheckerRefactoringTestHelper refactoringTestHelper = BugCheckerRefactoringTestHelper.newInstance( LexicographicalAnnotationListing.class, getClass()); @@ -21,7 +17,9 @@ final class LexicographicalAnnotationListingTest { compilationTestHelper .addSourceLines( "A.java", + "import java.lang.annotation.ElementType;", "import java.lang.annotation.Repeatable;", + "import java.lang.annotation.Target;", "", "interface A {", " @Repeatable(Foos.class)", @@ -33,6 +31,7 @@ final class LexicographicalAnnotationListingTest { " Bar[] anns() default {};", " }", "", + " @Target(ElementType.METHOD)", " @interface Bar {", " String[] value() default {};", " }", @@ -45,11 +44,21 @@ final class LexicographicalAnnotationListingTest { " Foo[] value();", " }", "", - " // BUG: Diagnostic matches: X", + " @Target(ElementType.TYPE_USE)", + " @interface FooTypeUse {", + " String[] value() default {};", + " }", + "", + " @Target(ElementType.TYPE_USE)", + " @interface BarTypeUse {", + " String[] value() default {};", + " }", + "", + " // BUG: Diagnostic contains:", " @Foo", " @Bar", " A unsortedSimpleCase();", - " // BUG: Diagnostic matches: X", + " // BUG: Diagnostic contains:", " @Foo()", " @Bar()", " A unsortedWithParens();", @@ -61,12 +70,12 @@ final class LexicographicalAnnotationListingTest { " @Foo()", " A sortedAnnotationsOneWithParens();", "", - " // BUG: Diagnostic matches: X", + " // BUG: Diagnostic contains:", " @Foo", " @Baz", " @Bar", " A threeUnsortedAnnotationsSameInitialLetter();", - " // BUG: Diagnostic matches: X", + " // BUG: Diagnostic contains:", " @Bar", " @Foo()", " @Baz", @@ -77,16 +86,16 @@ final class LexicographicalAnnotationListingTest { " @Foo()", " A threeSortedAnnotations();", "", - " // BUG: Diagnostic matches: X", + " // BUG: Diagnostic contains:", " @Foo({\"b\"})", " @Bar({\"a\"})", " A unsortedWithStringAttributes();", - " // BUG: Diagnostic matches: X", + " // BUG: Diagnostic contains:", " @Baz(str = {\"a\", \"b\"})", " @Foo(ints = {1, 0})", " @Bar", " A unsortedWithAttributes();", - " // BUG: Diagnostic matches: X", + " // BUG: Diagnostic contains:", " @Bar", " @Foo(anns = {@Bar(\"b\"), @Bar(\"a\")})", " @Baz", @@ -101,11 +110,23 @@ final class LexicographicalAnnotationListingTest { " @Foo(ints = {1, 2})", " @Foo({\"b\"})", " A sortedRepeatableAnnotation();", - " // BUG: Diagnostic matches: X", + " // BUG: Diagnostic contains:", " @Foo(anns = {@Bar(\"b\"), @Bar(\"a\")})", " @Bar", " @Foo(ints = {1, 2})", " A unsortedRepeatableAnnotation();", + "", + " // BUG: Diagnostic contains:", + " default @FooTypeUse @BarTypeUse A unsortedTypeAnnotations() {", + " return null;", + " }", + "", + " // BUG: Diagnostic contains:", + " @Baz", + " @Bar", + " default @FooTypeUse @BarTypeUse A unsortedTypeUseAndOtherAnnotations() {", + " return null;", + " }", "}") .doTest(); } @@ -115,7 +136,9 @@ final class LexicographicalAnnotationListingTest { refactoringTestHelper .addInputLines( "A.java", + "import java.lang.annotation.ElementType;", "import java.lang.annotation.Repeatable;", + "import java.lang.annotation.Target;", "", "interface A {", " @Repeatable(Foos.class)", @@ -127,6 +150,7 @@ final class LexicographicalAnnotationListingTest { " Bar[] anns() default {};", " }", "", + " @Target(ElementType.METHOD)", " @interface Bar {", " String[] value() default {};", " }", @@ -139,6 +163,16 @@ final class LexicographicalAnnotationListingTest { " Foo[] value();", " }", "", + " @Target(ElementType.TYPE_USE)", + " @interface FooTypeUse {", + " String[] value() default {};", + " }", + "", + " @Target(ElementType.TYPE_USE)", + " @interface BarTypeUse {", + " String[] value() default {};", + " }", + "", " @Bar", " A singleAnnotation();", "", @@ -174,10 +208,18 @@ final class LexicographicalAnnotationListingTest { " @Bar", " @Foo(ints = {1, 2})", " A unsortedRepeatableAnnotation();", + "", + " @Baz", + " @Bar", + " default @FooTypeUse @BarTypeUse A unsortedWithTypeUseAnnotations() {", + " return null;", + " }", "}") .addOutputLines( "A.java", + "import java.lang.annotation.ElementType;", "import java.lang.annotation.Repeatable;", + "import java.lang.annotation.Target;", "", "interface A {", " @Repeatable(Foos.class)", @@ -189,6 +231,7 @@ final class LexicographicalAnnotationListingTest { " Bar[] anns() default {};", " }", "", + " @Target(ElementType.METHOD)", " @interface Bar {", " String[] value() default {};", " }", @@ -201,6 +244,16 @@ final class LexicographicalAnnotationListingTest { " Foo[] value();", " }", "", + " @Target(ElementType.TYPE_USE)", + " @interface FooTypeUse {", + " String[] value() default {};", + " }", + "", + " @Target(ElementType.TYPE_USE)", + " @interface BarTypeUse {", + " String[] value() default {};", + " }", + "", " @Bar", " A singleAnnotation();", "", @@ -236,6 +289,12 @@ final class LexicographicalAnnotationListingTest { " @Foo(anns = {@Bar(\"b\"), @Bar(\"a\")})", " @Foo(ints = {1, 2})", " A unsortedRepeatableAnnotation();", + "", + " @Bar", + " @Baz", + " default @BarTypeUse @FooTypeUse A unsortedWithTypeUseAnnotations() {", + " return null;", + " }", "}") .doTest(TestMode.TEXT_MATCH); }