Improve AnnotationPosition diagnostics to only mention the annotation…

…s that are being moved PiperOrigin-RevId: 580185539
google · Nov 7, 2023 · 0149a77 · 0149a77
1 parent af227ac
commit 0149a77
Show file tree

Hide file tree

Showing 2 changed files with 44 additions and 25 deletions.
diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/AnnotationPosition.java b/core/src/main/java/com/google/errorprone/bugpatterns/AnnotationPosition.java
@@ -214,37 +214,53 @@ private Description checkAnnotations(
                 })
             .collect(toImmutableList());
 
-    boolean annotationsInCorrectPlace =
-        shouldBeBefore.stream().allMatch(a -> getStartPosition(a) < firstModifierPos)
-            && shouldBeAfter.stream().allMatch(a -> getStartPosition(a) > lastModifierPos);
+    int lastNonTypeAnnotationOrModifierPosition =
+        Streams.concat(
+                Stream.of(lastModifierPos),
+                shouldBeBefore.stream().map(ASTHelpers::getStartPosition))
+            .max(naturalOrder())
+            .get();
+    int firstTypeAnnotationOrModifierPosition =
+        Stream.concat(
+                Stream.of(firstModifierPos),
+                shouldBeAfter.stream().map(ASTHelpers::getStartPosition))
+            .min(naturalOrder())
+            .get();
+    ImmutableList<AnnotationTree> moveBefore =
+        shouldBeBefore.stream()
+            .filter(a -> getStartPosition(a) > firstTypeAnnotationOrModifierPosition)
+            .collect(toImmutableList());
+    ImmutableList<AnnotationTree> moveAfter =
+        shouldBeAfter.stream()
+            .filter(a -> getStartPosition(a) < lastNonTypeAnnotationOrModifierPosition)
+            .collect(toImmutableList());
 
-    if (annotationsInCorrectPlace && isOrderingIsCorrect(shouldBeBefore, shouldBeAfter)) {
+    if (moveBefore.isEmpty() && moveAfter.isEmpty()) {
       return NO_MATCH;
     }
     SuggestedFix.Builder fix = SuggestedFix.builder();
-    for (AnnotationTree annotation : concat(shouldBeBefore, shouldBeAfter)) {
+    for (AnnotationTree annotation : concat(moveBefore, moveAfter)) {
       fix.delete(annotation);
     }
     String javadoc = danglingJavadoc == null ? "" : removeJavadoc(state, danglingJavadoc, fix);
     if (lastModifierPos == 0) {
       fix.replace(
           getStartPosition(tree),
           getStartPosition(tree),
-          String.format(
-              "%s%s ", javadoc, joinSource(state, concat(shouldBeBefore, shouldBeAfter))));
+          String.format("%s%s ", javadoc, joinSource(state, concat(moveBefore, moveAfter))));
     } else {
       fix.replace(
               firstModifierPos,
               firstModifierPos,
-              String.format("%s%s ", javadoc, joinSource(state, shouldBeBefore)))
+              String.format("%s%s ", javadoc, joinSource(state, moveBefore)))
           .replace(
               lastModifierPos,
               lastModifierPos,
-              String.format(" %s ", joinSource(state, shouldBeAfter)));
+              String.format(" %s ", joinSource(state, moveAfter)));
     }
     Stream.Builder<String> messages = Stream.builder();
-    if (!shouldBeBefore.isEmpty()) {
-      ImmutableList<String> names = annotationNames(shouldBeBefore);
+    if (!moveBefore.isEmpty()) {
+      ImmutableList<String> names = annotationNames(moveBefore);
       String flattened = String.join(", ", names);
       String isAre =
           names.size() > 1 ? "are not TYPE_USE annotations" : "is not a TYPE_USE annotation";
@@ -253,8 +269,8 @@ private Description checkAnnotations(
               "%s %s, so should appear before any modifiers and after Javadocs.",
               flattened, isAre));
     }
-    if (!shouldBeAfter.isEmpty()) {
-      ImmutableList<String> names = annotationNames(shouldBeAfter);
+    if (!moveAfter.isEmpty()) {
+      ImmutableList<String> names = annotationNames(moveAfter);
       String flattened = String.join(", ", names);
       String isAre = names.size() > 1 ? "are TYPE_USE annotations" : "is a TYPE_USE annotation";
       messages.add(
@@ -268,18 +284,6 @@ private Description checkAnnotations(
         .build();
   }
 
-  private static boolean isOrderingIsCorrect(
-      List<AnnotationTree> shouldBeBefore, List<AnnotationTree> shouldBeAfter) {
-    if (shouldBeBefore.isEmpty() || shouldBeAfter.isEmpty()) {
-      return true;
-    }
-    int largestNonTypeAnnotationPosition =
-        shouldBeBefore.stream().map(ASTHelpers::getStartPosition).max(naturalOrder()).get();
-    int smallestTypeAnnotationPosition =
-        shouldBeAfter.stream().map(ASTHelpers::getStartPosition).min(naturalOrder()).get();
-    return largestNonTypeAnnotationPosition < smallestTypeAnnotationPosition;
-  }
-
   private static Position annotationPosition(Tree tree, AnnotationType annotationType) {
     if (tree instanceof ClassTree || annotationType == null) {
       return Position.BEFORE;

diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/AnnotationPositionTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/AnnotationPositionTest.java
@@ -605,4 +605,19 @@ public void variable_genericType_modifiers() {
             "}")
         .doTest(TEXT_MATCH);
   }
+
+  @Test
+  public void twoNonTypeAnnotation() {
+    helper
+        .addSourceLines(
+            "AnotherNonTypeUse.java", //
+            "@interface AnotherNonTypeUse {}")
+        .addSourceLines(
+            "Test.java", //
+            "interface Test {",
+            "  // BUG: Diagnostic contains: [AnnotationPosition] @AnotherNonTypeUse is not",
+            "  @NonTypeUse public @AnotherNonTypeUse void f();",
+            "}")
+        .doTest();
+  }
 }