External Library Models: Adding support for @Nullable Method parameters

gradle/dependencies.gradle

@@ -52,6 +52,7 @@ def versions = [
     commonscli             : "1.4",
     autoValue              : "1.10.2",
     autoService            : "1.1.1",
+    javaparser             : "3.26.2",
 ]
 
 def apt = [
@@ -75,7 +76,8 @@ def build = [
     errorProneTestHelpersOld: "com.google.errorprone:error_prone_test_helpers:${oldestErrorProneVersion}",
     checkerDataflow         : "org.checkerframework:dataflow-nullaway:${versions.checkerFramework}",
     guava                   : "com.google.guava:guava:30.1-jre",
-    javaparser              : "com.github.javaparser:javaparser-core:3.26.0",
+    javaparser              : "com.github.javaparser:javaparser-core:${versions.javaparser}",
+    javaparserSymbolSolver  : "com.github.javaparser:javaparser-symbol-solver-core:${versions.javaparser}",
     javaxValidation         : "javax.validation:validation-api:2.0.1.Final",
     jspecify                : "org.jspecify:jspecify:1.0.0",
     commonsIO               : "commons-io:commons-io:2.11.0",

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

@@ -26,6 +26,7 @@
 import com.ibm.wala.classLoader.PhantomClass;
 import com.ibm.wala.classLoader.ShrikeCTMethod;
 import com.ibm.wala.core.util.config.AnalysisScopeReader;
+import com.ibm.wala.core.util.strings.StringStuff;
 import com.ibm.wala.core.util.warnings.Warnings;
 import com.ibm.wala.ipa.callgraph.AnalysisCache;
 import com.ibm.wala.ipa.callgraph.AnalysisCacheImpl;
@@ -456,6 +457,7 @@ private void writeModel(DataOutputStream out) throws IOException {
         packageAnnotations,
         typeAnnotations,
         methodRecords,
+        Collections.emptySet(),
         Collections.emptyMap());
   }
 
@@ -527,28 +529,6 @@ private static String getAstubxSignature(IMethod mtd) {
    * @return String Unqualified type name.
    */
   private static String getSimpleTypeName(TypeReference typ) {
-    ImmutableMap<String, String> mapFullTypeName =
-        ImmutableMap.<String, String>builder()
-            .put("B", "byte")
-            .put("C", "char")
-            .put("D", "double")
-            .put("F", "float")
-            .put("I", "int")
-            .put("J", "long")
-            .put("S", "short")
-            .put("Z", "boolean")
-            .build();
-    if (typ.isArrayType()) {
-      return "Array";
-    }
-    String typName = typ.getName().toString();
-    if (typName.startsWith("L")) {
-      typName = typName.split("<")[0].substring(1); // handle generics
-      typName = typName.substring(typName.lastIndexOf('/') + 1); // get unqualified name
-      typName = typName.substring(typName.lastIndexOf('$') + 1); // handle inner classes
-    } else {
-      typName = mapFullTypeName.get(typName);
-    }
-    return typName;
+    return StringStuff.jvmToBinaryName(typ.getName().toString());
   }
 }

jar-infer/jar-infer-lib/src/test/java/com/uber/nullaway/jarinfer/JarInferTest.java

@@ -53,7 +53,6 @@
 
 /** Unit tests for {@link com.uber.nullaway.jarinfer}. */
 @RunWith(JUnit4.class)
-@SuppressWarnings("CheckTestExtendsBaseClass")
 public class JarInferTest {
 
   @Rule public TemporaryFolder temporaryFolder = new TemporaryFolder();
@@ -206,8 +205,8 @@ public void toyStatic() throws Exception {
         "toys",
         "Test",
         ImmutableMap.of(
-            "toys.Test:void test(String, Foo, Bar)", Sets.newHashSet(0, 2),
-            "toys.Foo:boolean run(String)", Sets.newHashSet(1)),
+            "toys.Test:void test(java.lang.String, toys.Foo, toys.Bar)", Sets.newHashSet(0, 2),
+            "toys.Foo:boolean run(java.lang.String)", Sets.newHashSet(1)),
         "class Foo {",
         "  private String foo;",
         "  public Foo(String str) {",
@@ -267,7 +266,8 @@ public void toyNonStatic() throws Exception {
         "toyNonStatic",
         "toys",
         "Foo",
-        ImmutableMap.of("toys.Foo:void test(String, String)", Sets.newHashSet(1)),
+        ImmutableMap.of(
+            "toys.Foo:void test(java.lang.String, java.lang.String)", Sets.newHashSet(1)),
         "class Foo {",
         "  private String foo;",
         "  public Foo(String str) {",
@@ -303,7 +303,9 @@ public void toyNullTestAPI() throws Exception {
         "toyNullTestAPI",
         "toys",
         "Foo",
-        ImmutableMap.of("toys.Foo:void test(String, String, String)", Sets.newHashSet(1, 3)),
+        ImmutableMap.of(
+            "toys.Foo:void test(java.lang.String, java.lang.String, java.lang.String)",
+            Sets.newHashSet(1, 3)),
         "import com.google.common.base.Preconditions;",
         "import java.util.Objects;",
         "import org.junit.Assert;",
@@ -332,7 +334,9 @@ public void toyConditionalFlow() throws Exception {
         "toyNullTestAPI",
         "toys",
         "Foo",
-        ImmutableMap.of("toys.Foo:void test(String, String, String)", Sets.newHashSet(1, 2)),
+        ImmutableMap.of(
+            "toys.Foo:void test(java.lang.String, java.lang.String, java.lang.String)",
+            Sets.newHashSet(1, 2)),
         "import com.google.common.base.Preconditions;",
         "import java.util.Objects;",
         "import org.junit.Assert;",
@@ -362,7 +366,8 @@ public void toyConditionalFlow2() throws Exception {
         "toys",
         "Foo",
         ImmutableMap.of(
-            "toys.Foo:void test(Object, Object, Object, Object)", Sets.newHashSet(1, 4)),
+            "toys.Foo:void test(java.lang.Object, java.lang.Object, java.lang.Object, java.lang.Object)",
+            Sets.newHashSet(1, 4)),
         "import com.google.common.base.Preconditions;",
         "import java.util.Objects;",
         "import org.junit.Assert;",
@@ -401,7 +406,8 @@ public void toyReassigningTest() throws Exception {
         "toyNullTestAPI",
         "toys",
         "Foo",
-        ImmutableMap.of("toys.Foo:void test(String, String)", Sets.newHashSet(1)),
+        ImmutableMap.of(
+            "toys.Foo:void test(java.lang.String, java.lang.String)", Sets.newHashSet(1)),
         "import com.google.common.base.Preconditions;",
         "import java.util.Objects;",
         "import org.junit.Assert;",

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

@@ -179,4 +179,53 @@ public void libraryModelNullableUpperBoundsWithoutJarInferTest() {
             "}")
         .doTest();
   }
+
+  @Test
+  public void libraryModelDefaultParameterNullabilityTest() {
+    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 com.test.ParameterAnnotationExample;",
+            "class Test {",
+            "  static ParameterAnnotationExample annotationExample = new ParameterAnnotationExample();",
+            "  static void testPositive() {",
+            "    // BUG: Diagnostic contains: passing @Nullable parameter 'null' where @NonNull is required",
+            "    annotationExample.add(5,null);",
+            "  }",
+            "}")
+        .doTest();
+  }
+
+  @Test
+  public void libraryModelParameterNullabilityTest() {
+    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 com.test.ParameterAnnotationExample;",
+            "class Test {",
+            "  static ParameterAnnotationExample annotationExample = new ParameterAnnotationExample();",
+            "  static void testNegative() {",
+            "    annotationExample.getNewObjectIfNull(null);",
+            "  }",
+            "}")
+        .doTest();
+  }
 }

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

@@ -21,6 +21,7 @@ plugins {
 dependencies {
     implementation deps.build.guava
     implementation deps.build.javaparser
+    implementation deps.build.javaparserSymbolSolver
     compileOnly deps.apt.autoValueAnnot
     annotationProcessor deps.apt.autoValue
 

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

@@ -29,11 +29,19 @@
 import com.github.javaparser.ast.PackageDeclaration;
 import com.github.javaparser.ast.body.ClassOrInterfaceDeclaration;
 import com.github.javaparser.ast.body.MethodDeclaration;
+import com.github.javaparser.ast.body.Parameter;
 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.resolution.TypeSolver;
+import com.github.javaparser.resolution.declarations.ResolvedMethodDeclaration;
+import com.github.javaparser.resolution.types.ResolvedType;
+import com.github.javaparser.symbolsolver.JavaSymbolSolver;
+import com.github.javaparser.symbolsolver.resolution.typesolvers.CombinedTypeSolver;
+import com.github.javaparser.symbolsolver.resolution.typesolvers.JavaParserTypeSolver;
+import com.github.javaparser.symbolsolver.resolution.typesolvers.ReflectionTypeSolver;
 import com.github.javaparser.utils.CollectionStrategy;
 import com.github.javaparser.utils.ParserCollectionStrategy;
 import com.github.javaparser.utils.ProjectRoot;
@@ -47,6 +55,7 @@
 import java.nio.file.Path;
 import java.nio.file.Paths;
 import java.util.Collections;
+import java.util.HashSet;
 import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
@@ -69,12 +78,15 @@
   public static class LibraryModelData {
     public final Map<String, MethodAnnotationsRecord> methodRecords;
     public final Map<String, Set<Integer>> nullableUpperBounds;
+    public final Set<String> nullMarkedClasses;
 
     public LibraryModelData(
         Map<String, MethodAnnotationsRecord> methodRecords,
-        Map<String, Set<Integer>> nullableUpperBounds) {
+        Map<String, Set<Integer>> nullableUpperBounds,
+        Set<String> nullMarkedClasses) {
       this.methodRecords = methodRecords;
       this.nullableUpperBounds = nullableUpperBounds;
+      this.nullMarkedClasses = nullMarkedClasses;
     }
 
     @Override
@@ -84,6 +96,8 @@
           + methodRecords
           + ", nullableUpperBounds="
           + nullableUpperBounds
+          + ", nullMarkedClasses="
+          + nullMarkedClasses
           + '}';
     }
   }
@@ -97,14 +111,21 @@
   public static LibraryModelData generateAstubxForLibraryModels(
       String inputSourceDirectory, String outputFile) {
     Map<String, MethodAnnotationsRecord> methodRecords = new LinkedHashMap<>();
+    Set<String> nullMarkedClasses = new HashSet<>();
     Map<String, Set<Integer>> nullableUpperBounds = new LinkedHashMap<>();
     Path root = dirnameToPath(inputSourceDirectory);
-    LibraryModelData modelData = new LibraryModelData(methodRecords, nullableUpperBounds);
+    LibraryModelData modelData =
+        new LibraryModelData(methodRecords, nullableUpperBounds, nullMarkedClasses);
     AnnotationCollectorCallback ac = new AnnotationCollectorCallback(modelData);
     CollectionStrategy strategy = new ParserCollectionStrategy();
     // Required to include directories that contain a module-info.java, which don't parse by
     // default.
+    TypeSolver typeSolver =
+        new CombinedTypeSolver(
+            new ReflectionTypeSolver(), new JavaParserTypeSolver(Paths.get(inputSourceDirectory)));
+    JavaSymbolSolver symbolSolver = new JavaSymbolSolver(typeSolver);
     strategy.getParserConfiguration().setLanguageLevel(LanguageLevel.JAVA_17);
+    strategy.getParserConfiguration().setSymbolResolver(symbolSolver);
     ProjectRoot projectRoot = strategy.collect(root);
 
     projectRoot
@@ -130,7 +151,8 @@
   private static void writeToAstubx(String outputPath, LibraryModelData modelData) {
     Map<String, MethodAnnotationsRecord> methodRecords = modelData.methodRecords;
     Map<String, Set<Integer>> nullableUpperBounds = modelData.nullableUpperBounds;
-    if (methodRecords.isEmpty() && nullableUpperBounds.isEmpty()) {
+    Set<String> nullMarkedClasses = modelData.nullMarkedClasses;
+    if (methodRecords.isEmpty() && nullableUpperBounds.isEmpty() && nullMarkedClasses.isEmpty()) {
       return;
     }
     ImmutableMap<String, String> importedAnnotations =
@@ -147,6 +169,7 @@
             Collections.emptyMap(),
             Collections.emptyMap(),
             methodRecords,
+            nullMarkedClasses,
             nullableUpperBounds);
       }
     } catch (IOException e) {
@@ -189,7 +212,9 @@
     private boolean isJspecifyNullableImportPresent = false;
     private boolean isNullMarked = false;
     private final Map<String, MethodAnnotationsRecord> methodRecords;
+    private final Set<String> nullMarkedClasses;
     private final Map<String, Set<Integer>> nullableUpperBounds;
+    // TODO this is sketchy!
     private static final String ARRAY_RETURN_TYPE_STRING = "Array";
     private static final String NULL_MARKED = "NullMarked";
     private static final String NULLABLE = "Nullable";
@@ -198,6 +223,7 @@
     public AnnotationCollectionVisitor(LibraryModelData modelData) {
       this.methodRecords = modelData.methodRecords;
       this.nullableUpperBounds = modelData.nullableUpperBounds;
+      this.nullMarkedClasses = modelData.nullMarkedClasses;
     }
 
     @Override
@@ -231,6 +257,7 @@
               a -> {
                 if (a.getNameAsString().equalsIgnoreCase(NULL_MARKED)) {
                   this.isNullMarked = true;
+                  this.nullMarkedClasses.add(parentName);
                 }
               });
       if (this.isNullMarked) {
@@ -247,10 +274,24 @@
 
     @Override
     public void visit(MethodDeclaration md, Void arg) {
-      if (this.isNullMarked && hasNullableReturn(md)) {
+      ImmutableMap<Integer, ImmutableSet<String>> nullableParameterMap = getNullableParameters(md);
+      boolean isReturnNullable = hasNullableReturn(md);
+      ImmutableSet<String> nullableReturn =
+          isReturnNullable ? ImmutableSet.of(NULLABLE) : ImmutableSet.of();
+      // We write the method record into the astubx if it contains a Nullable return or any Nullable
+      // parameter.
+      ResolvedMethodDeclaration resolved = md.resolve();
+      String qualifiedSignature = resolved.getQualifiedSignature();
+      String methodSignatureWithQualifiedParameters =
+          qualifiedSignature.substring(qualifiedSignature.lastIndexOf(md.getNameAsString()));
+      if (this.isNullMarked && (isReturnNullable || !nullableParameterMap.isEmpty())) {
         methodRecords.put(
-            parentName + ":" + getMethodReturnTypeString(md) + " " + md.getSignature().toString(),
-            MethodAnnotationsRecord.create(ImmutableSet.of("Nullable"), ImmutableMap.of()));
+            parentName
+                + ":"
+                + getMethodReturnTypeString(resolved)
+                + " "
+                + methodSignatureWithQualifiedParameters,
+            MethodAnnotationsRecord.create(nullableReturn, nullableParameterMap));
       }
       super.visit(md, null);
     }
@@ -288,13 +329,15 @@
      * @param md The MethodDeclaration instance.
      * @return The return type string value to be written into the astubx file.
      */
-    private String getMethodReturnTypeString(MethodDeclaration md) {
-      if (md.getType() instanceof ClassOrInterfaceType) {
-        return md.getType().getChildNodes().get(0).toString();
-      } else if (md.getType() instanceof ArrayType) {
+    private String getMethodReturnTypeString(ResolvedMethodDeclaration md) {
+      ResolvedType returnType = md.getReturnType();
+      if (returnType.isReferenceType()) {
+        return returnType.asReferenceType().getQualifiedName().toString();
+      } else if (returnType.isArray()) {
         return ARRAY_RETURN_TYPE_STRING;
       } else {
-        return md.getType().toString();
+        throw new RuntimeException("Unexpected return type: " + returnType);
+        // return returnType.asPrimitive().toString();
       }
     }
 
@@ -327,5 +370,26 @@
       }
       return setBuilder.build();
     }
+
+    /**
+     * Takes a MethodDeclaration instance and provides a map containing the Integer indices for the
+     * parameters with their Nullable annotation to be written in the astubx format.
+     *
+     * @param md MethodDeclaration instance.
+     * @return Map of Nullable parameters for the method.
+     */
+    private ImmutableMap<Integer, ImmutableSet<String>> getNullableParameters(
+        MethodDeclaration md) {
+      ImmutableMap.Builder<Integer, ImmutableSet<String>> mapBuilder = ImmutableMap.builder();
+      List<Parameter> parameterList = md.getParameters();
+      for (int i = 0; i < parameterList.size(); i++) {
+        Parameter parameter = parameterList.get(i);
+        Optional<AnnotationExpr> nullableAnnotation = parameter.getAnnotationByName(NULLABLE);
+        if (nullableAnnotation.isPresent() && isAnnotationNullable(nullableAnnotation.get())) {
+          mapBuilder.put(i, ImmutableSet.of("Nullable"));
+        }
+      }
+      return mapBuilder.build();
+    }
   }
 }