From 13fbba93e116f9e369ea717dded593b5ddcfe65b Mon Sep 17 00:00:00 2001 From: Mikkel Kjeldsen Date: Fri, 19 Sep 2025 18:47:15 +0200 Subject: [PATCH] Nullness: ignore Jakarta Validation Teach checks like `MultipleNullnessAnnotations` and `RedundantNullCheck` to disregard annotations that look like nullness specification annotations but that actually are not; namely the Jakarta Validation `@NotNull` constraint. Resolves: https://github.com/google/error-prone/issues/4334 --- .../NullnessAnnotations.java | 30 +++++++++++++++++-- core/pom.xml | 7 +++++ .../MultipleNullnessAnnotationsTest.java | 22 ++++++++++++++ .../nullness/RedundantNullCheckTest.java | 28 +++++++++++++++++ 4 files changed, 85 insertions(+), 2 deletions(-) diff --git a/check_api/src/main/java/com/google/errorprone/dataflow/nullnesspropagation/NullnessAnnotations.java b/check_api/src/main/java/com/google/errorprone/dataflow/nullnesspropagation/NullnessAnnotations.java index c4d2e02aac4..1466ddec6df 100644 --- a/check_api/src/main/java/com/google/errorprone/dataflow/nullnesspropagation/NullnessAnnotations.java +++ b/check_api/src/main/java/com/google/errorprone/dataflow/nullnesspropagation/NullnessAnnotations.java @@ -20,6 +20,7 @@ import static javax.lang.model.element.ElementKind.TYPE_PARAMETER; import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableSet; import com.google.errorprone.util.MoreAnnotations; import com.sun.source.tree.AnnotationTree; import com.sun.source.tree.IdentifierTree; @@ -38,6 +39,7 @@ import javax.lang.model.element.ExecutableElement; import javax.lang.model.element.Name; import javax.lang.model.element.Parameterizable; +import javax.lang.model.element.QualifiedNameable; import javax.lang.model.element.TypeParameterElement; import javax.lang.model.type.IntersectionType; import javax.lang.model.type.TypeMirror; @@ -60,6 +62,9 @@ public class NullnessAnnotations { + "ProtoMethodMayReturnNull|ProtoMethodAcceptsNullParameter|" + "ProtoPassThroughNullness") .asMatchPredicate(); + private static final ImmutableSet FALSE_NULLNESS_ANNOTATIONS = + ImmutableSet.of( + "jakarta.validation.constraints.NotNull", "javax.validation.constraints.NotNull"); private NullnessAnnotations() {} // static methods only @@ -84,12 +89,13 @@ public static boolean annotationsAreAmbiguous( public static ImmutableList annotationsRelevantToNullness( Collection annotations) { return annotations.stream() - .filter(a -> ANNOTATION_RELEVANT_TO_NULLNESS.test(simpleName(a).toString())) + .filter(NullnessAnnotations::isAnnotationRelevant) .collect(toImmutableList()); } public static ImmutableList annotationsRelevantToNullness( List annotations) { + // Missing support for FALSE_NULLNESS_ANNOTATIONS return annotations.stream() .filter(a -> ANNOTATION_RELEVANT_TO_NULLNESS.test(simpleName(a))) .collect(toImmutableList()); @@ -216,7 +222,10 @@ public static Optional getUpperBound(TypeVariable typeVar) { private static Optional fromAnnotationStream( Stream annotations) { - return fromAnnotationSimpleNames(annotations.map(a -> simpleName(a).toString())); + return fromAnnotationSimpleNames( + annotations + .filter(NullnessAnnotations::isAnnotationRelevant) + .map(a -> simpleName(a).toString())); } private static Optional fromAnnotationSimpleNames(Stream annotations) { @@ -225,4 +234,21 @@ private static Optional fromAnnotationSimpleNames(Stream annot .map(annot -> NULLABLE_ANNOTATION.test(annot) ? Nullness.NULLABLE : Nullness.NONNULL) .reduce(Nullness::greatestLowerBound); } + + private static boolean isAnnotationRelevant(AnnotationMirror annotation) { + // https://github.com/google/error-prone/issues/4334 + // Some @NotNull annotations, e.g. com.google.firebase.database.annotations.NotNull, resemble + // JSpecify @NonNull semantics. However, the Jakarta Bean Validation @NotNull constraint instead + // represents a runtime check to reject a value that turns out to be null, rather than a static + // promise that null will not be observed. Therefore, "@j.v.c.NotNull @o.j.a.Nullable Object o" + // is a sensible and unambiguous declaration. + if (annotation.getAnnotationType().asElement() instanceof QualifiedNameable qn) { + var fqn = qn.getQualifiedName().toString(); + if (FALSE_NULLNESS_ANNOTATIONS.contains(fqn)) { + return false; + } + return ANNOTATION_RELEVANT_TO_NULLNESS.test(simpleName(annotation).toString()); + } + return true; + } } diff --git a/core/pom.xml b/core/pom.xml index eaaab041d4a..99ebf982629 100644 --- a/core/pom.xml +++ b/core/pom.xml @@ -360,6 +360,13 @@ javax.inject 1 + + + jakarta.validation + jakarta.validation-api + 3.1.0 + test + diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/nullness/MultipleNullnessAnnotationsTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/nullness/MultipleNullnessAnnotationsTest.java index 17c5b1cb849..715313dc011 100644 --- a/core/src/test/java/com/google/errorprone/bugpatterns/nullness/MultipleNullnessAnnotationsTest.java +++ b/core/src/test/java/com/google/errorprone/bugpatterns/nullness/MultipleNullnessAnnotationsTest.java @@ -166,4 +166,26 @@ class T { """) .doTest(); } + + @Test + public void falseNullnessAnnotations() { + testHelper + .addSourceLines( + "T.java", + """ + import static java.lang.annotation.ElementType.*; + import java.lang.annotation.Target; + import org.jspecify.annotations.Nullable; + + @Target(FIELD) + @interface NotNull {} + + abstract class T { + @jakarta.validation.constraints.NotNull @Nullable Object negative; + // BUG: Diagnostic contains: + @NotNull @Nullable Object positive; + } + """) + .doTest(); + } } diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/nullness/RedundantNullCheckTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/nullness/RedundantNullCheckTest.java index f429bf7b791..c160d414d5a 100644 --- a/core/src/test/java/com/google/errorprone/bugpatterns/nullness/RedundantNullCheckTest.java +++ b/core/src/test/java/com/google/errorprone/bugpatterns/nullness/RedundantNullCheckTest.java @@ -1034,4 +1034,32 @@ void foo(@NonNull String s) { """) .doTest(); } + + @Test + public void negative_falseNullnessAnnotation_inNullMarkedScope() { + compilationHelper + .addSourceLines( + "Test.java", + """ + import org.jspecify.annotations.NullMarked; + import org.jspecify.annotations.NonNull; + import org.jspecify.annotations.Nullable; + import jakarta.validation.constraints.NotNull; + + @NullMarked + class Test { + @NotNull @Nullable String negative; + @NonNull @Nullable String positive; + + void foo() { + if (negative == null) { + /* This is fine */ + } + // BUG: Diagnostic contains: RedundantNullCheck + if (positive == null) {} + } + } + """) + .doTest(); + } }