From 1bf08c25af71ba7908e25299d4304cd11976b08d Mon Sep 17 00:00:00 2001 From: cpovirk Date: Mon, 10 Jun 2024 18:21:27 -0700 Subject: [PATCH] Fix `NullablePrimitiveArray` to report errors even when the method/field contains declaration annotations, including nullness declaration annotations. PiperOrigin-RevId: 642087958 --- .../nullness/NullablePrimitiveArray.java | 48 ++++++++++++++----- .../nullness/NullablePrimitiveArrayTest.java | 40 ++++++++++++++++ 2 files changed, 75 insertions(+), 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 index 26d9b6bcf19..7e03579a61f 100644 --- 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 annotations, VisitorState state) { + Tree typeTree, List allTreeAnnos, VisitorState state) { Type type = getType(typeTree); if (type == null) { return NO_MATCH; @@ -87,14 +97,22 @@ private Description check( if (!type.isPrimitive()) { return NO_MATCH; } - ImmutableList annotationsRelevantToNullness = - NullnessAnnotations.annotationsRelevantToNullness(annotations); - if (annotationsRelevantToNullness.isEmpty()) { + ImmutableList 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 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; @@ -102,15 +120,19 @@ private Description check( 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/core/src/test/java/com/google/errorprone/bugpatterns/nullness/NullablePrimitiveArrayTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/nullness/NullablePrimitiveArrayTest.java index faea7d975f7..874966b940d 100644 --- a/core/src/test/java/com/google/errorprone/bugpatterns/nullness/NullablePrimitiveArrayTest.java +++ b/core/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