Fix NullablePrimitiveArray to report errors even when the method/fi…

…eld contains declaration annotations, including nullness declaration annotations. PiperOrigin-RevId: 642087958
google · Jun 11, 2024 · 1bf08c2 · 1bf08c2
1 parent 4c06673
commit 1bf08c2
Show file tree

Hide file tree

Showing 2 changed files with 75 additions and 13 deletions.
diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/nullness/NullablePrimitiveArray.java b/core/src/main/java/com/google/errorprone/bugpatterns/nullness/NullablePrimitiveArray.java
@@ -16,6 +16,7 @@
 
 package com.google.errorprone.bugpatterns.nullness;
 
+import static com.google.common.collect.ImmutableList.toImmutableList;
 import static com.google.errorprone.BugPattern.SeverityLevel.WARNING;
 import static com.google.errorprone.matchers.Description.NO_MATCH;
 import static com.google.errorprone.util.ASTHelpers.getSymbol;
@@ -25,6 +26,7 @@
 import com.google.common.collect.ImmutableList;
 import com.google.errorprone.BugPattern;
 import com.google.errorprone.BugPattern.StandardTags;
+import com.google.errorprone.ErrorProneFlags;
 import com.google.errorprone.VisitorState;
 import com.google.errorprone.bugpatterns.BugChecker;
 import com.google.errorprone.bugpatterns.BugChecker.MethodTreeMatcher;
@@ -46,6 +48,7 @@
 import java.util.List;
 import java.util.Optional;
 import java.util.Set;
+import javax.inject.Inject;
 import javax.lang.model.element.AnnotationValue;
 import javax.lang.model.element.VariableElement;
 import javax.lang.model.type.TypeKind;
@@ -60,6 +63,13 @@
     tags = StandardTags.STYLE)
 public class NullablePrimitiveArray extends BugChecker
     implements VariableTreeMatcher, MethodTreeMatcher {
+  private final boolean fixEvenWhenMixedWithDeclaration;
+
+  @Inject
+  NullablePrimitiveArray(ErrorProneFlags flags) {
+    this.fixEvenWhenMixedWithDeclaration =
+        flags.getBoolean("NullablePrimitiveArray:FixEvenWhenMixedWithDeclaration").orElse(true);
+  }
 
   @Override
   public Description matchMethod(MethodTree tree, VisitorState state) {
@@ -73,7 +83,7 @@ public Description matchVariable(VariableTree tree, VisitorState state) {
 
   // other cases of `@Nullable int[]` are covered by the existing NullablePrimitive
   private Description check(
-      Tree typeTree, List<? extends AnnotationTree> annotations, VisitorState state) {
+      Tree typeTree, List<? extends AnnotationTree> allTreeAnnos, VisitorState state) {
     Type type = getType(typeTree);
     if (type == null) {
       return NO_MATCH;
@@ -87,30 +97,42 @@ private Description check(
     if (!type.isPrimitive()) {
       return NO_MATCH;
     }
-    ImmutableList<AnnotationTree> annotationsRelevantToNullness =
-        NullnessAnnotations.annotationsRelevantToNullness(annotations);
-    if (annotationsRelevantToNullness.isEmpty()) {
+    ImmutableList<AnnotationTree> treeNullnessAnnos =
+        NullnessAnnotations.annotationsRelevantToNullness(allTreeAnnos);
+    if (treeNullnessAnnos.isEmpty()) {
       return NO_MATCH;
     }
     Symbol target = state.getSymtab().annotationTargetType.tsym;
-    if (annotations.stream()
-        .anyMatch(annotation -> !isTypeAnnotation(getSymbol(annotation).attribute(target)))) {
+    ImmutableList<AnnotationTree> typeNullnessAnnos =
+        treeNullnessAnnos.stream()
+            .filter(annotation -> isTypeAnnotation(getSymbol(annotation).attribute(target)))
+            .collect(toImmutableList());
+    if (typeNullnessAnnos.isEmpty()) {
+      return NO_MATCH;
+    }
+    if (!fixEvenWhenMixedWithDeclaration
+        && allTreeAnnos.stream()
+            .anyMatch(annotation -> !isTypeAnnotation(getSymbol(annotation).attribute(target)))) {
       return NO_MATCH;
     }
     Tree dims = typeTree;
     while (dims instanceof ArrayTypeTree) {
       dims = ((ArrayTypeTree) dims).getType();
     }
     SuggestedFix.Builder fix = SuggestedFix.builder();
-    annotations.forEach(fix::delete);
-    if (!(dims instanceof AnnotatedTypeTree)
-        || NullnessAnnotations.annotationsRelevantToNullness(
-                ((AnnotatedTypeTree) dims).getAnnotations())
-            .isEmpty()) {
+    typeNullnessAnnos.forEach(fix::delete);
+    boolean hasDeclarationNullnessAnno = typeNullnessAnnos.size() < treeNullnessAnnos.size();
+    boolean hasTypeNullnessAnnoOnArray =
+        dims instanceof AnnotatedTypeTree
+            && !NullnessAnnotations.annotationsRelevantToNullness(
+                    ((AnnotatedTypeTree) dims).getAnnotations())
+                .isEmpty();
+    if (!hasDeclarationNullnessAnno && !hasTypeNullnessAnnoOnArray) {
       fix.postfixWith(
-          dims, annotations.stream().map(state::getSourceForNode).collect(joining(" ", " ", " ")));
+          dims,
+          typeNullnessAnnos.stream().map(state::getSourceForNode).collect(joining(" ", " ", " ")));
     }
-    return describeMatch(annotations.get(0), fix.build());
+    return describeMatch(typeNullnessAnnos.get(0), fix.build());
   }
 
   private static boolean isTypeAnnotation(Attribute.Compound attribute) {

diff --git a/.../src/test/java/com/google/errorprone/bugpatterns/nullness/NullablePrimitiveArrayTest.java b/.../src/test/java/com/google/errorprone/bugpatterns/nullness/NullablePrimitiveArrayTest.java
@@ -60,6 +60,46 @@ public void typeAnnotation() {
         .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH);
   }
 
+  @Test
+  public void typeAnnotationWithOtherAnnotation() {
+    testHelper
+        .addInputLines(
+            "Test.java",
+            "import org.checkerframework.checker.nullness.qual.Nullable;",
+            "abstract class Test {",
+            "  @SuppressWarnings(\"SomeOtherChecker\") // unrelated annotation",
+            "  @Nullable abstract byte[] f();",
+            "}")
+        .addOutputLines(
+            "Test.java",
+            "import org.checkerframework.checker.nullness.qual.Nullable;",
+            "abstract class Test {",
+            "  @SuppressWarnings(\"SomeOtherChecker\") // unrelated annotation",
+            "  abstract byte @Nullable [] f();",
+            "}")
+        .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH);
+  }
+
+  @Test
+  public void typeAnnotationWithOtherNullnessAnnotationDoesNotSuggestDoubleAnnotation() {
+    testHelper
+        .addInputLines(
+            "Test.java",
+            "import javax.annotation.CheckForNull;",
+            "import org.checkerframework.checker.nullness.qual.Nullable;",
+            "abstract class Test {",
+            "  @CheckForNull @Nullable abstract byte[] f();",
+            "}")
+        .addOutputLines(
+            "Test.java",
+            "import javax.annotation.CheckForNull;",
+            "import org.checkerframework.checker.nullness.qual.Nullable;",
+            "abstract class Test {",
+            "  @CheckForNull abstract byte[] f();",
+            "}")
+        .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH);
+  }
+
   @Test
   public void negative() {
     testHelper