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: #4334

Author: commonquail

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.sun.source.tree.AnnotationTree;
 import com.sun.source.tree.IdentifierTree;
 import com.sun.source.tree.MemberSelectTree;
@@ -37,6 +38,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;
@@ -59,6 +61,9 @@ public class NullnessAnnotations {
                   + "ProtoMethodMayReturnNull|ProtoMethodAcceptsNullParameter|"
                   + "ProtoPassThroughNullness")
           .asMatchPredicate();
+  private static final ImmutableSet<String> FALSE_NULLNESS_ANNOTATIONS =
+      ImmutableSet.of(
+          "jakarta.validation.constraints.NotNull", "javax.validation.constraints.NotNull");
 
   private NullnessAnnotations() {} // static methods only
 
@@ -83,12 +88,13 @@ public static boolean annotationsAreAmbiguous(
   public static ImmutableList<AnnotationMirror> annotationsRelevantToNullness(
       Collection<? extends AnnotationMirror> annotations) {
     return annotations.stream()
-        .filter(a -> ANNOTATION_RELEVANT_TO_NULLNESS.test(simpleName(a).toString()))
+        .filter(NullnessAnnotations::isAnnotationRelevant)
         .collect(toImmutableList());
   }
 
   public static ImmutableList<AnnotationTree> annotationsRelevantToNullness(
       List<? extends AnnotationTree> annotations) {
+    // Missing support for FALSE_NULLNESS_ANNOTATIONS
     return annotations.stream()
         .filter(a -> ANNOTATION_RELEVANT_TO_NULLNESS.test(simpleName(a)))
         .collect(toImmutableList());
@@ -209,7 +215,10 @@ public static Optional<Nullness> getUpperBound(TypeVariable typeVar) {
 
   private static Optional<Nullness> fromAnnotationStream(
       Stream<? extends AnnotationMirror> annotations) {
-    return fromAnnotationSimpleNames(annotations.map(a -> simpleName(a).toString()));
+    return fromAnnotationSimpleNames(
+        annotations
+            .filter(NullnessAnnotations::isAnnotationRelevant)
+            .map(a -> simpleName(a).toString()));
   }
 
   private static Optional<Nullness> fromAnnotationSimpleNames(Stream<String> annotations) {
@@ -218,4 +227,21 @@ private static Optional<Nullness> fromAnnotationSimpleNames(Stream<String> 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;
+  }
 }

core/pom.xml

@@ -360,6 +360,13 @@
       <artifactId>javax.inject</artifactId>
       <version>1</version>
     </dependency>
+    <dependency>
+      <!-- Apache 2.0 -->
+      <groupId>jakarta.validation</groupId>
+      <artifactId>jakarta.validation-api</artifactId>
+      <version>3.1.0</version>
+      <scope>test</scope>
+    </dependency>
   </dependencies>
 
   <build>

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();
+  }
 }

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();
+  }
 }