External Library Models: Adding support for Nullable upper bounds of Generic Type parameters

jar-infer/jar-infer-lib/src/main/java/com/uber/nullaway/jarinfer/DefinitelyDerefedParamsDriver.java

@@ -58,6 +58,7 @@
 import java.nio.file.Paths;
 import java.nio.file.attribute.FileTime;
 import java.util.Arrays;
+import java.util.Collections;
 import java.util.HashMap;
 import java.util.HashSet;
 import java.util.LinkedHashMap;
@@ -449,7 +450,13 @@ private void writeModel(DataOutputStream out) throws IOException {
           nullableReturnMethodSign,
           MethodAnnotationsRecord.create(ImmutableSet.of("Nullable"), ImmutableMap.of()));
     }
-    StubxWriter.write(out, importedAnnotations, packageAnnotations, typeAnnotations, methodRecords);
+    StubxWriter.write(
+        out,
+        importedAnnotations,
+        packageAnnotations,
+        typeAnnotations,
+        methodRecords,
+        Collections.emptyMap());
   }
 
   private void writeAnnotations(String inPath, String outFile) throws IOException {

library-model/library-model-generator-integration-test/build.gradle

@@ -22,6 +22,7 @@ dependencies {
     testImplementation project(":nullaway")
     testImplementation project(":library-model:test-library-model-generator")
     testImplementation deps.test.junit4
+    testImplementation deps.build.jspecify
     testImplementation(deps.build.errorProneTestHelpers) {
         exclude group: "junit", module: "junit"
     }

library-model/library-model-generator-integration-test/src/test/java/com/uber/nullaway/libmodel/LibraryModelIntegrationTest.java

@@ -8,6 +8,11 @@
 import org.junit.Test;
 import org.junit.rules.TemporaryFolder;
 
+/**
+ * Integration test for library model support. The library models are contained in the jar for the
+ * test-library-model-generator project, as a stubx file. These tests ensure that NullAway correctly
+ * loads the stubx file and reports the right errors based on those models.
+ */
 public class LibraryModelIntegrationTest {
 
   @Rule public final TemporaryFolder temporaryFolder = new TemporaryFolder();
@@ -42,6 +47,8 @@ public void libraryModelNullableReturnsTest() {
             "    test(annotationExample.makeUpperCase(\"nullaway\"));",
             "  }",
             "  static void testNegative() {",
+            "    // no error since nullReturn is annotated with javax.annotation.Nullable,",
+            "    // which is not considered when generating stubx files",
             "    test(annotationExample.nullReturn());",
             "  }",
             "}")
@@ -123,4 +130,53 @@ public void libraryModelInnerClassNullableReturnsTest() {
             "}")
         .doTest();
   }
+
+  @Test
+  public void libraryModelInnerClassNullableUpperBoundsTest() {
+    compilationHelper
+        .setArgs(
+            Arrays.asList(
+                "-d",
+                temporaryFolder.getRoot().getAbsolutePath(),
+                "-XepOpt:NullAway:AnnotatedPackages=com.uber",
+                "-XepOpt:NullAway:JSpecifyMode=true",
+                "-XepOpt:NullAway:JarInferEnabled=true",
+                "-XepOpt:NullAway:JarInferUseReturnAnnotations=true"))
+        .addSourceLines(
+            "Test.java",
+            "package com.uber;",
+            "import org.jspecify.annotations.Nullable;",
+            "import com.uber.nullaway.libmodel.AnnotationExample;",
+            "class Test {",
+            "  static AnnotationExample.UpperBoundExample<@Nullable Object> upperBoundExample = new AnnotationExample.UpperBoundExample<@Nullable Object>();",
+            "  static void test(Object value){",
+            "  }",
+            "  static void testPositive() {",
+            "    // BUG: Diagnostic contains: passing @Nullable parameter 'upperBoundExample.getNullable()'",
+            "    test(upperBoundExample.getNullable());",
+            "  }",
+            "}")
+        .doTest();
+  }
+
+  @Test
+  public void libraryModelNullableUpperBoundsWithoutJarInferTest() {
+    compilationHelper
+        .setArgs(
+            Arrays.asList(
+                "-d",
+                temporaryFolder.getRoot().getAbsolutePath(),
+                "-XepOpt:NullAway:AnnotatedPackages=com.uber",
+                "-XepOpt:NullAway:JSpecifyMode=true"))
+        .addSourceLines(
+            "Test.java",
+            "package com.uber;",
+            "import org.jspecify.annotations.Nullable;",
+            "import com.uber.nullaway.libmodel.AnnotationExample;",
+            "class Test {",
+            "  // BUG: Diagnostic contains: Generic type parameter cannot be @Nullable",
+            "  static AnnotationExample.UpperBoundExample<@Nullable Object> upperBoundExample = new AnnotationExample.UpperBoundExample<@Nullable Object>();",
+            "}")
+        .doTest();
+  }
 }

library-model/library-model-generator/src/main/java/com/uber/nullaway/libmodel/LibraryModelGenerator.java

@@ -32,6 +32,7 @@
 import com.github.javaparser.ast.expr.AnnotationExpr;
 import com.github.javaparser.ast.type.ArrayType;
 import com.github.javaparser.ast.type.ClassOrInterfaceType;
+import com.github.javaparser.ast.type.TypeParameter;
 import com.github.javaparser.ast.visitor.VoidVisitorAdapter;
 import com.github.javaparser.utils.CollectionStrategy;
 import com.github.javaparser.utils.ParserCollectionStrategy;
@@ -47,8 +48,10 @@
 import java.nio.file.Paths;
 import java.util.Collections;
 import java.util.LinkedHashMap;
+import java.util.List;
 import java.util.Map;
 import java.util.Optional;
+import java.util.Set;
 
 /**
  * Utilized for generating an astubx file from a directory containing annotated Java source code.
@@ -59,21 +62,18 @@
  */
 public class LibraryModelGenerator {
 
-  public void generateAstubxForLibraryModels(String inputSourceDirectory, String outputDirectory) {
-    Map<String, MethodAnnotationsRecord> methodRecords = processDirectory(inputSourceDirectory);
-    writeToAstubx(outputDirectory, methodRecords);
-  }
-
   /**
    * Parses all the source files within the directory using javaparser.
    *
-   * @param sourceDirectoryRoot Directory containing annotated java source files.
-   * @return a Map containing the Nullability annotation information from the source files.
+   * @param inputSourceDirectory Directory containing annotated java source files.
+   * @param outputDirectory Directory to write the astubx file into.
    */
-  private Map<String, MethodAnnotationsRecord> processDirectory(String sourceDirectoryRoot) {
+  public void generateAstubxForLibraryModels(String inputSourceDirectory, String outputDirectory) {
     Map<String, MethodAnnotationsRecord> methodRecords = new LinkedHashMap<>();
-    Path root = dirnameToPath(sourceDirectoryRoot);
-    AnnotationCollectorCallback ac = new AnnotationCollectorCallback(methodRecords);
+    Map<String, Set<Integer>> nullableUpperBounds = new LinkedHashMap<>();
+    Path root = dirnameToPath(inputSourceDirectory);
+    AnnotationCollectorCallback ac =
+        new AnnotationCollectorCallback(methodRecords, nullableUpperBounds);
     CollectionStrategy strategy = new ParserCollectionStrategy();
     // Required to include directories that contain a module-info.java, which don't parse by
     // default.
@@ -90,7 +90,7 @@ private Map<String, MethodAnnotationsRecord> processDirectory(String sourceDirec
                 throw new RuntimeException(e);
               }
             });
-    return methodRecords;
+    writeToAstubx(outputDirectory, methodRecords, nullableUpperBounds);
   }
 
   /**
@@ -100,8 +100,10 @@ private Map<String, MethodAnnotationsRecord> processDirectory(String sourceDirec
    * @param methodRecords Map containing the collected Nullability annotation information.
    */
   private void writeToAstubx(
-      String outputPath, Map<String, MethodAnnotationsRecord> methodRecords) {
-    if (methodRecords.isEmpty()) {
+      String outputPath,
+      Map<String, MethodAnnotationsRecord> methodRecords,
+      Map<String, Set<Integer>> nullableUpperBounds) {
+    if (methodRecords.isEmpty() && nullableUpperBounds.isEmpty()) {
       return;
     }
     Map<String, String> importedAnnotations =
@@ -117,7 +119,8 @@ private void writeToAstubx(
             importedAnnotations,
             Collections.emptyMap(),
             Collections.emptyMap(),
-            methodRecords);
+            methodRecords,
+            nullableUpperBounds);
       }
     } catch (IOException e) {
       throw new RuntimeException(e);
@@ -137,8 +140,11 @@ private static class AnnotationCollectorCallback implements SourceRoot.Callback
 
     private final AnnotationCollectionVisitor annotationCollectionVisitor;
 
-    public AnnotationCollectorCallback(Map<String, MethodAnnotationsRecord> methodRecords) {
-      this.annotationCollectionVisitor = new AnnotationCollectionVisitor(methodRecords);
+    public AnnotationCollectorCallback(
+        Map<String, MethodAnnotationsRecord> methodRecords,
+        Map<String, Set<Integer>> nullableUpperBounds) {
+      this.annotationCollectionVisitor =
+          new AnnotationCollectionVisitor(methodRecords, nullableUpperBounds);
     }
 
     @Override
@@ -158,14 +164,18 @@ private static class AnnotationCollectionVisitor extends VoidVisitorAdapter<Void
     private String parentName = "";
     private boolean isJspecifyNullableImportPresent = false;
     private boolean isNullMarked = false;
-    private Map<String, MethodAnnotationsRecord> methodRecords;
+    private final Map<String, MethodAnnotationsRecord> methodRecords;
+    private final Map<String, Set<Integer>> nullableUpperBounds;
     private static final String ARRAY_RETURN_TYPE_STRING = "Array";
     private static final String NULL_MARKED = "NullMarked";
     private static final String NULLABLE = "Nullable";
     private static final String JSPECIFY_NULLABLE_IMPORT = "org.jspecify.annotations.Nullable";
 
-    public AnnotationCollectionVisitor(Map<String, MethodAnnotationsRecord> methodRecords) {
+    public AnnotationCollectionVisitor(
+        Map<String, MethodAnnotationsRecord> methodRecords,
+        Map<String, Set<Integer>> nullableUpperBounds) {
       this.methodRecords = methodRecords;
+      this.nullableUpperBounds = nullableUpperBounds;
     }
 
     @Override
@@ -197,6 +207,12 @@ public void visit(ClassOrInterfaceDeclaration cid, Void arg) {
                   this.isNullMarked = true;
                 }
               });
+      if (this.isNullMarked) {
+        Set<Integer> nullableUpperBoundParams = getGenericTypeParameterNullableUpperBounds(cid);
+        if (!nullableUpperBoundParams.isEmpty()) {
+          nullableUpperBounds.put(parentName, nullableUpperBoundParams);
+        }
+      }
       super.visit(cid, null);
       // We reset the variable that constructs the parent name after visiting all the children.
       parentName = parentName.substring(0, parentName.lastIndexOf("." + cid.getNameAsString()));
@@ -261,5 +277,28 @@ private boolean isAnnotationNullable(AnnotationExpr annotation) {
               && this.isJspecifyNullableImportPresent)
           || annotation.getNameAsString().equalsIgnoreCase(JSPECIFY_NULLABLE_IMPORT);
     }
+
+    /**
+     * Takes a ClassOrInterfaceDeclaration instance and returns a Map of the indexes and a set of
+     * annotations for generic type parameters with Nullable upper bounds.
+     *
+     * @param cid ClassOrInterfaceDeclaration instance.
+     * @return Set of indices for generic type parameters with Nullable upper bounds.
+     */
+    private ImmutableSet<Integer> getGenericTypeParameterNullableUpperBounds(
+        ClassOrInterfaceDeclaration cid) {
+      ImmutableSet.Builder<Integer> setBuilder = ImmutableSet.builder();
+      List<TypeParameter> typeParamList = cid.getTypeParameters();
+      for (int i = 0; i < typeParamList.size(); i++) {
+        TypeParameter param = typeParamList.get(i);
+        for (ClassOrInterfaceType type : param.getTypeBound()) {
+          Optional<AnnotationExpr> nullableAnnotation = type.getAnnotationByName(NULLABLE);
+          if (nullableAnnotation.isPresent() && isAnnotationNullable(nullableAnnotation.get())) {
+            setBuilder.add(i);
+          }
+        }
+      }
+      return setBuilder.build();
+    }
   }
 }

library-model/library-model-generator/src/main/java/com/uber/nullaway/libmodel/StubxWriter.java

@@ -36,7 +36,8 @@ public static void write(
       Map<String, String> importedAnnotations,
       Map<String, Set<String>> packageAnnotations,
       Map<String, Set<String>> typeAnnotations,
-      Map<String, MethodAnnotationsRecord> methodRecords)
+      Map<String, MethodAnnotationsRecord> methodRecords,
+      Map<String, Set<Integer>> nullableUpperBounds)
       throws IOException {
     // File format version/magic number
     out.writeInt(VERSION_0_FILE_MAGIC_NUMBER);
@@ -49,7 +50,8 @@ public static void write(
             importedAnnotations.values(),
             packageAnnotations.keySet(),
             typeAnnotations.keySet(),
-            methodRecords.keySet());
+            methodRecords.keySet(),
+            nullableUpperBounds.keySet());
     for (Collection<String> keyset : keysets) {
       for (String key : keyset) {
         assert !encodingDictionary.containsKey(key);
@@ -118,5 +120,17 @@ public static void write(
         }
       }
     }
+    // Followed by the number of nullable upper bounds records
+    out.writeInt(nullableUpperBounds.size());
+    for (Map.Entry<String, Set<Integer>> entry : nullableUpperBounds.entrySet()) {
+      // Followed by the number of parameters with nullable upper bound
+      Set<Integer> parameters = entry.getValue();
+      out.writeInt(parameters.size());
+      for (Integer parameter : parameters) {
+        // Followed by the nullable upper bound record as a par of integers
+        out.writeInt(encodingDictionary.get(entry.getKey()));
+        out.writeInt(parameter);
+      }
+    }
   }
 }

library-model/test-library-model-generator/src/main/java/com/uber/nullaway/libmodel/AnnotationExample.java

@@ -39,4 +39,14 @@ public String returnNull() {
       return null;
     }
   }
+
+  /** In the library model we add a {@code @Nullable} upper bound for T */
+  public static class UpperBoundExample<T> {
+
+    T nullableObject;
+
+    public T getNullable() {
+      return nullableObject;
+    }
+  }
 }

library-model/test-library-model-generator/src/main/resources/sample_annotated/src/com/uber/nullaway/libmodel/AnnotationExample.java

@@ -45,4 +45,13 @@ public String returnNull() {
             return null;
         }
     }
+
+    public static class UpperBoundExample<T extends @Nullable Object> {
+
+        T nullableObject;
+
+        public T getNullable() {
+            return nullableObject;
+        }
+    }
 }