External Library Models: Adding support for Nullable upper bounds of …

…Generic Type parameters (#949) With these changes we are able to read and create library models for Nullable upper bounds of generic type parameters from externally annotated source code as shown in the below example. Externally annotated source code example: ```java public static class GenericExample<T extends @nullable Object> { public T getNull() { return null; } } ``` ```java public static class GenericExample<T> { public T getNull() { return null; } } static GenericExample<@nullable Object> genericExample = new GenericExample<@nullable Object>(); static void test(Object value){} // BUG: Diagnostic contains: passing @nullable parameter 'genericExample.getNull()' test(genericExample.getNull()); ``` --------- Co-authored-by: Manu Sridharan <[email protected]>
uber · Jun 7, 2024 · 866b140 · 866b140
1 parent 6cec433
commit 866b140
Show file tree

Hide file tree

Showing 10 changed files with 430 additions and 125 deletions.
diff --git a/...jar-infer-lib/src/main/java/com/uber/nullaway/jarinfer/DefinitelyDerefedParamsDriver.java b/...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 {

diff --git a/library-model/library-model-generator-integration-test/build.gradle b/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"
     }

diff --git a/...ntegration-test/src/test/java/com/uber/nullaway/libmodel/LibraryModelIntegrationTest.java b/...ntegration-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();
+  }
 }
diff --git a/...brary-model-generator/src/main/java/com/uber/nullaway/libmodel/LibraryModelGenerator.java b/...brary-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();
+    }
   }
 }
diff --git a/...y-model/library-model-generator/src/main/java/com/uber/nullaway/libmodel/StubxWriter.java b/...y-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);
+      }
+    }
   }
 }
diff --git a/...t-library-model-generator/src/main/java/com/uber/nullaway/libmodel/AnnotationExample.java b/...t-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;
+    }
+  }
 }
diff --git a/...src/main/resources/sample_annotated/src/com/uber/nullaway/libmodel/AnnotationExample.java b/...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;
+        }
+    }
 }