Clarifications and small fixes for checking JSpecify @nullable annota…

…tion (#859) Rename a variable and add docs to clarify that in certain places, our JSpecify support specifically checks for `@org.jspecify.annotations.Nullable` annotations and not others. Also, fix a couple of places where we were comparing types by their `String` representation.
uber · Nov 15, 2023 · 9092438 · 9092438
1 parent 60648a9
commit 9092438
Show file tree

Hide file tree

Showing 4 changed files with 34 additions and 7 deletions.
diff --git a/nullaway/src/main/java/com/uber/nullaway/generics/CompareNullabilityVisitor.java b/nullaway/src/main/java/com/uber/nullaway/generics/CompareNullabilityVisitor.java
@@ -1,6 +1,7 @@
 package com.uber.nullaway.generics;
 
 import com.google.errorprone.VisitorState;
+import com.google.errorprone.util.ASTHelpers;
 import com.sun.tools.javac.code.Attribute;
 import com.sun.tools.javac.code.Type;
 import com.sun.tools.javac.code.Types;
@@ -43,17 +44,19 @@ public Boolean visitClassType(Type.ClassType lhsType, Type rhsType) {
       boolean isLHSNullableAnnotated = false;
       List<Attribute.TypeCompound> lhsAnnotations = lhsTypeArgument.getAnnotationMirrors();
       // To ensure that we are checking only jspecify nullable annotations
+      Type jspecifyNullableType = GenericsChecks.JSPECIFY_NULLABLE_TYPE_SUPPLIER.get(state);
       for (Attribute.TypeCompound annotation : lhsAnnotations) {
-        if (annotation.getAnnotationType().toString().equals(GenericsChecks.NULLABLE_NAME)) {
+        if (ASTHelpers.isSameType(
+            (Type) annotation.getAnnotationType(), jspecifyNullableType, state)) {
           isLHSNullableAnnotated = true;
           break;
         }
       }
       boolean isRHSNullableAnnotated = false;
       List<Attribute.TypeCompound> rhsAnnotations = rhsTypeArgument.getAnnotationMirrors();
-      // To ensure that we are checking only jspecify nullable annotations
       for (Attribute.TypeCompound annotation : rhsAnnotations) {
-        if (annotation.getAnnotationType().toString().equals(GenericsChecks.NULLABLE_NAME)) {
+        if (ASTHelpers.isSameType(
+            (Type) annotation.getAnnotationType(), jspecifyNullableType, state)) {
           isRHSNullableAnnotated = true;
           break;
         }

diff --git a/nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java b/nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java
@@ -38,9 +38,13 @@
 /** Methods for performing checks related to generic types and nullability. */
 public final class GenericsChecks {
 
-  static final String NULLABLE_NAME = "org.jspecify.annotations.Nullable";
-
-  static final Supplier<Type> NULLABLE_TYPE_SUPPLIER = Suppliers.typeFromString(NULLABLE_NAME);
+  /**
+   * Supplier for the JSpecify {@code @Nullable} annotation. Required since for now, certain checks
+   * related to generics specifically look for {@code @org.jspecify.ananotations.Nullable}
+   * annotations and do not apply to other {@code @Nullable} annotations.
+   */
+  static final Supplier<Type> JSPECIFY_NULLABLE_TYPE_SUPPLIER =
+      Suppliers.typeFromString("org.jspecify.annotations.Nullable");
 
   /** Do not instantiate; all methods should be static */
   private GenericsChecks() {}

diff --git a/nullaway/src/main/java/com/uber/nullaway/generics/PreservedAnnotationTreeVisitor.java b/nullaway/src/main/java/com/uber/nullaway/generics/PreservedAnnotationTreeVisitor.java
@@ -42,7 +42,7 @@ public Type visitArrayType(ArrayTypeTree tree, Void p) {
   public Type visitParameterizedType(ParameterizedTypeTree tree, Void p) {
     Type.ClassType type = (Type.ClassType) ASTHelpers.getType(tree);
     Preconditions.checkNotNull(type);
-    Type nullableType = GenericsChecks.NULLABLE_TYPE_SUPPLIER.get(state);
+    Type nullableType = GenericsChecks.JSPECIFY_NULLABLE_TYPE_SUPPLIER.get(state);
     List<? extends Tree> typeArguments = tree.getTypeArguments();
     List<Type> newTypeArgs = new ArrayList<>();
     for (int i = 0; i < typeArguments.size(); i++) {

diff --git a/nullaway/src/test/java/com/uber/nullaway/NullAwayJSpecifyGenericsTests.java b/nullaway/src/test/java/com/uber/nullaway/NullAwayJSpecifyGenericsTests.java
@@ -291,6 +291,26 @@ public void genericsChecksForAssignments() {
         .doTest();
   }
 
+  @Test
+  public void genericsChecksForAssignmentsWithNonJSpecifyAnnotations() {
+    makeHelper()
+        .addSourceLines(
+            "Test.java",
+            "package com.uber;",
+            "import org.checkerframework.checker.nullness.qual.Nullable;",
+            "class Test {",
+            "  static class NullableTypeParam<E extends @Nullable Object> {}",
+            "  static void testNoWarningForMismatch(NullableTypeParam<@Nullable String> t1) {",
+            "    // no error here since we only do our checks for JSpecify @Nullable annotations",
+            "    NullableTypeParam<String> t2 = t1;",
+            "  }",
+            "  static void testNegative(NullableTypeParam<@Nullable String> t1) {",
+            "    NullableTypeParam<@Nullable String> t2 = t1;",
+            "  }",
+            "}")
+        .doTest();
+  }
+
   @Test
   public void nestedChecksForAssignmentsMultipleArguments() {
     makeHelper()