Merge branch 'jarinfer-generics-in-signature' into remove-inferred-ja…

…r-models-handler

.github/workflows/continuous-integration.yml

@@ -25,13 +25,13 @@ jobs:
             epVersion: 2.31.0
           - os: macos-latest
             java: 17
-            epVersion: 2.35.1
+            epVersion: 2.36.0
           - os: windows-latest
             java: 17
-            epVersion: 2.35.1
+            epVersion: 2.36.0
           - os: ubuntu-latest
             java: 17
-            epVersion: 2.35.1
+            epVersion: 2.36.0
       fail-fast: false
     runs-on: ${{ matrix.os }}
     steps:
@@ -60,7 +60,7 @@ jobs:
           ORG_GRADLE_PROJECT_epApiVersion: ${{ matrix.epVersion }}
         run: ./gradlew codeCoverageReport
         continue-on-error: true
-        if: runner.os == 'Linux' && matrix.java == '17' && matrix.epVersion == '2.35.1' && github.repository == 'uber/NullAway'
+        if: runner.os == 'Linux' && matrix.java == '17' && matrix.epVersion == '2.36.0' && github.repository == 'uber/NullAway'
       - name: Upload coverage reports to Codecov
         uses: codecov/codecov-action@v4
         with:

build.gradle

@@ -32,7 +32,7 @@ plugins {
     id "com.github.johnrengelman.shadow" version "8.1.1" apply false
     id "me.champeau.jmh" version "0.7.1" apply false
     id "com.github.ben-manes.versions" version "0.51.0"
-    id "com.felipefzdz.gradle.shellcheck" version "1.4.6"
+    id "com.felipefzdz.gradle.shellcheck" version "1.5.0"
 }
 
 repositories {
@@ -155,5 +155,7 @@ tasks.register('installGitHooks', Copy) {
         rename 'pre-commit-stub', 'pre-commit'
     }
     into file('.git/hooks')
-    fileMode 0777
+    filePermissions {
+        unix(0777)
+    }
 }

gradle/dependencies.gradle

@@ -19,7 +19,7 @@ import org.gradle.util.VersionNumber
 // The oldest version of Error Prone that we support running on
 def oldestErrorProneVersion = "2.14.0"
 // Latest released Error Prone version that we've tested with
-def latestErrorProneVersion = "2.35.1"
+def latestErrorProneVersion = "2.36.0"
 // Default to using latest tested Error Prone version
 def defaultErrorProneVersion =  latestErrorProneVersion
 def errorProneVersionToCompileAgainst = defaultErrorProneVersion
@@ -48,7 +48,7 @@ def versions = [
     // The version of Error Prone that NullAway is compiled and tested against
     errorProneApi          : errorProneVersionToCompileAgainst,
     support                : "27.1.1",
-    wala                   : "1.6.7",
+    wala                   : "1.6.8",
     commonscli             : "1.4",
     autoValue              : "1.10.2",
     autoService            : "1.1.1",

gradle/wrapper/gradle-wrapper.properties

@@ -1,7 +1,7 @@
 distributionBase=GRADLE_USER_HOME
 distributionPath=wrapper/dists
-distributionSha256Sum=31c55713e40233a8303827ceb42ca48a47267a0ad4bab9177123121e71524c26
-distributionUrl=https\://services.gradle.org/distributions/gradle-8.10.2-bin.zip
+distributionSha256Sum=57dafb5c2622c6cc08b993c85b7c06956a2f53536432a30ead46166dbca0f1e9
+distributionUrl=https\://services.gradle.org/distributions/gradle-8.11-bin.zip
 networkTimeout=10000
 validateDistributionUrl=true
 zipStoreBase=GRADLE_USER_HOME

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

@@ -172,11 +172,9 @@ private static void annotateBytecode(
       }
       Set<Integer> params = nonnullParams.get(methodSignature);
       if (params != null) {
-        boolean isStatic = (method.access & Opcodes.ACC_STATIC) != 0;
         for (Integer param : params) {
-          int paramNum = isStatic ? param : param - 1;
           // Add a @Nonnull annotation on this parameter.
-          method.visitParameterAnnotation(paramNum, nonnullDesc, visible);
+          method.visitParameterAnnotation(param, nonnullDesc, visible);
           LOG(
               debug,
               "DEBUG",

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

@@ -42,6 +42,9 @@
 import com.ibm.wala.ssa.SSAInstruction;
 import com.ibm.wala.types.ClassLoaderReference;
 import com.ibm.wala.types.TypeReference;
+import com.ibm.wala.types.generics.MethodTypeSignature;
+import com.ibm.wala.types.generics.TypeSignature;
+import com.ibm.wala.types.generics.TypeVariableSignature;
 import com.ibm.wala.util.collections.Iterator2Iterable;
 import com.ibm.wala.util.config.FileOfClasses;
 import com.uber.nullaway.libmodel.MethodAnnotationsRecord;
@@ -273,7 +276,8 @@ private void analyzeFile(String pkgName, String inPath, boolean includeNonPublic
               String sign = "";
               try {
                 // Parameter analysis
-                if (mtd.getNumberOfParameters() > (mtd.isStatic() ? 0 : 1)) {
+                boolean isStatic = mtd.isStatic();
+                if (mtd.getNumberOfParameters() > (isStatic ? 0 : 1)) {
                   // For inferring parameter nullability, our criteria is based on finding
                   // unchecked dereferences of that parameter. We perform a quick bytecode
                   // check and skip methods containing no dereferences (i.e. method calls
@@ -283,6 +287,11 @@ private void analyzeFile(String pkgName, String inPath, boolean includeNonPublic
                   if (bytecodeHasAnyDereferences(mtd)) {
                     analysisDriver = getAnalysisDriver(mtd, options, cache);
                     Set<Integer> result = analysisDriver.analyze();
+                    if (!isStatic) {
+                      // subtract 1 from each parameter index to account for 'this' parameter
+                      result =
+                          result.stream().map(i -> i - 1).collect(ImmutableSet.toImmutableSet());
+                    }
                     sign = getSignature(mtd);
                     LOG(DEBUG, "DEBUG", "analyzed method: " + sign);
                     if (!result.isEmpty() || DEBUG) {
@@ -499,36 +508,94 @@ private String getSignature(IMethod mtd) {
    * @param mtd Method reference.
    * @return String Method signature.
    */
-  // TODO: handle generics and inner classes
   private static String getAstubxSignature(IMethod mtd) {
-    String classType =
-        mtd.getDeclaringClass().getName().toString().replaceAll("/", "\\.").substring(1);
-    classType = classType.replaceAll("\\$", "\\."); // handle inner class
-    String returnType = mtd.isInit() ? null : getSimpleTypeName(mtd.getReturnType());
-    String strArgTypes = "";
-    int argi = mtd.isStatic() ? 0 : 1; // Skip 'this' parameter
-    for (; argi < mtd.getNumberOfParameters(); argi++) {
-      strArgTypes += getSimpleTypeName(mtd.getParameterType(argi));
-      if (argi < mtd.getNumberOfParameters() - 1) {
-        strArgTypes += ", ";
+    Preconditions.checkArgument(
+        mtd instanceof ShrikeCTMethod, "Method is not a ShrikeCTMethod from bytecodes");
+    String classType = getSourceLevelQualifiedTypeName(mtd.getDeclaringClass().getReference());
+    MethodTypeSignature genericSignature = null;
+    try {
+      genericSignature = ((ShrikeCTMethod) mtd).getMethodTypeSignature();
+    } catch (InvalidClassFileException e) {
+      // don't fail, just proceed without the generic signature
+      LOG(DEBUG, "DEBUG", "Invalid class file exception: " + e.getMessage());
+    }
+    String returnType;
+    int numParams = mtd.isStatic() ? mtd.getNumberOfParameters() : mtd.getNumberOfParameters() - 1;
+    String[] argTypes = new String[numParams];
+    if (genericSignature != null) {
+      // get types that include generic type arguments
+      returnType = getSourceLevelQualifiedTypeName(genericSignature.getReturnType().toString());
+      TypeSignature[] argTypeSigs = genericSignature.getArguments();
+      for (int i = 0; i < argTypeSigs.length; i++) {
+        argTypes[i] = getSourceLevelQualifiedTypeName(argTypeSigs[i].toString());
+      }
+    } else { // no generics
+      returnType = mtd.isInit() ? null : getSourceLevelQualifiedTypeName(mtd.getReturnType());
+      int argi = mtd.isStatic() ? 0 : 1; // Skip 'this' parameter
+      for (int i = 0; i < numParams; i++) {
+        argTypes[i] = getSourceLevelQualifiedTypeName(mtd.getParameterType(argi++));
       }
     }
     return classType
         + ":"
         + (returnType == null ? "void " : returnType + " ")
         + mtd.getName().toString()
         + "("
-        + strArgTypes
+        + String.join(", ", argTypes)
         + ")";
   }
 
   /**
-   * Get simple unqualified type name.
+   * Get the source-level qualified type name for a TypeReference.
    *
    * @param typ Type Reference.
-   * @return String Unqualified type name.
+   * @return source-level qualified type name.
+   * @see #getSourceLevelQualifiedTypeName(String)
    */
-  private static String getSimpleTypeName(TypeReference typ) {
-    return StringStuff.jvmToBinaryName(typ.getName().toString());
+  private static String getSourceLevelQualifiedTypeName(TypeReference typ) {
+    String typeName = typ.getName().toString();
+    return getSourceLevelQualifiedTypeName(typeName);
+  }
+
+  /**
+   * Converts a JVM-level qualified type (e.g., {@code Lcom/example/Foo$Baz;}) to a source-level
+   * qualified type (e.g., {@code com.example.Foo.Baz}). Nested types like generic type arguments
+   * are converted recursively.
+   *
+   * @param typeName JVM-level qualified type name.
+   * @return source-level qualified type name.
+   */
+  private static String getSourceLevelQualifiedTypeName(String typeName) {
+    if (!typeName.endsWith(";")) {
+      // we need the semicolon since some of WALA's TypeSignature APIs expect it
+      typeName = typeName + ";";
+    }
+    boolean isGeneric = typeName.contains("<");
+    if (!isGeneric) { // base case
+      TypeSignature ts = TypeSignature.make(typeName);
+      if (ts.isTypeVariable()) {
+        // TypeVariableSignature's toString() returns more than just the identifier
+        return ((TypeVariableSignature) ts).getIdentifier();
+      } else {
+        String tsStr = ts.toString();
+        if (tsStr.endsWith(";")) {
+          // remove trailing semicolon
+          tsStr = tsStr.substring(0, tsStr.length() - 1);
+        }
+        return StringStuff.jvmToReadableType(tsStr);
+      }
+    } else { // generic type
+      int idx = typeName.indexOf("<");
+      String baseType = typeName.substring(0, idx);
+      // generic type args are separated by semicolons in signature stored in bytecodes
+      String[] genericTypeArgs = typeName.substring(idx + 1, typeName.length() - 2).split(";");
+      for (int i = 0; i < genericTypeArgs.length; i++) {
+        genericTypeArgs[i] = getSourceLevelQualifiedTypeName(genericTypeArgs[i]);
+      }
+      return getSourceLevelQualifiedTypeName(baseType)
+          + "<"
+          + String.join(",", genericTypeArgs)
+          + ">";
+    }
   }
 }

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

@@ -206,7 +206,7 @@ public void toyStatic() throws Exception {
         "Test",
         ImmutableMap.of(
             "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)),
+            "toys.Foo:boolean run(java.lang.String)", Sets.newHashSet(0)),
         "class Foo {",
         "  private String foo;",
         "  public Foo(String str) {",
@@ -267,7 +267,7 @@ public void toyNonStatic() throws Exception {
         "toys",
         "Foo",
         ImmutableMap.of(
-            "toys.Foo:void test(java.lang.String, java.lang.String)", Sets.newHashSet(1)),
+            "toys.Foo:void test(java.lang.String, java.lang.String)", Sets.newHashSet(0)),
         "class Foo {",
         "  private String foo;",
         "  public Foo(String str) {",
@@ -305,7 +305,7 @@ public void toyNullTestAPI() throws Exception {
         "Foo",
         ImmutableMap.of(
             "toys.Foo:void test(java.lang.String, java.lang.String, java.lang.String)",
-            Sets.newHashSet(1, 3)),
+            Sets.newHashSet(0, 2)),
         "import com.google.common.base.Preconditions;",
         "import java.util.Objects;",
         "import org.junit.Assert;",
@@ -336,7 +336,7 @@ public void toyConditionalFlow() throws Exception {
         "Foo",
         ImmutableMap.of(
             "toys.Foo:void test(java.lang.String, java.lang.String, java.lang.String)",
-            Sets.newHashSet(1, 2)),
+            Sets.newHashSet(0, 1)),
         "import com.google.common.base.Preconditions;",
         "import java.util.Objects;",
         "import org.junit.Assert;",
@@ -367,7 +367,7 @@ public void toyConditionalFlow2() throws Exception {
         "Foo",
         ImmutableMap.of(
             "toys.Foo:void test(java.lang.Object, java.lang.Object, java.lang.Object, java.lang.Object)",
-            Sets.newHashSet(1, 4)),
+            Sets.newHashSet(0, 3)),
         "import com.google.common.base.Preconditions;",
         "import java.util.Objects;",
         "import org.junit.Assert;",
@@ -407,7 +407,7 @@ public void toyReassigningTest() throws Exception {
         "toys",
         "Foo",
         ImmutableMap.of(
-            "toys.Foo:void test(java.lang.String, java.lang.String)", Sets.newHashSet(1)),
+            "toys.Foo:void test(java.lang.String, java.lang.String)", Sets.newHashSet(0)),
         "import com.google.common.base.Preconditions;",
         "import java.util.Objects;",
         "import org.junit.Assert;",
@@ -448,8 +448,7 @@ public void testGenericMethod() throws Exception {
         "testGenericMethod",
         "generic",
         "TestGeneric",
-        ImmutableMap.of(
-            "generic.TestGeneric:java.lang.String foo(java.lang.Object)", Sets.newHashSet(1)),
+        ImmutableMap.of("generic.TestGeneric:java.lang.String foo(T)", Sets.newHashSet(0)),
         "public class TestGeneric<T> {",
         "  public String foo(T t) {",
         "    return t.toString();",
@@ -461,6 +460,27 @@ public void testGenericMethod() throws Exception {
         "}");
   }
 
+  @Test
+  public void testMethodWithGenericParameter() throws Exception {
+    testTemplate(
+        "testMethodWithGenericParameter",
+        "generic",
+        "TestGeneric",
+        ImmutableMap.of(
+            "generic.TestGeneric:java.lang.String getString(generic.TestGeneric.Generic<java.lang.String,java.lang.String>)",
+            Sets.newHashSet(0)),
+        "public class TestGeneric {",
+        "  static class Generic<T,U> {",
+        "    public String foo(T t) {",
+        "      return \"hi\";",
+        "    }",
+        "  }",
+        "  public String getString(Generic<String,String> g) {",
+        "    return g.foo(\"test\");",
+        "  }",
+        "}");
+  }
+
   @Test
   public void toyJARAnnotatingClasses() throws Exception {
     testAnnotationInJarTemplate(

jar-infer/nullaway-integration-test/src/test/java/com/uber/nullaway/jarinfer/JarInferIntegrationTest.java

@@ -4,6 +4,7 @@
 import com.uber.nullaway.NullAway;
 import java.util.Arrays;
 import org.junit.Before;
+import org.junit.Ignore;
 import org.junit.Rule;
 import org.junit.Test;
 import org.junit.rules.TemporaryFolder;
@@ -87,6 +88,8 @@ public void genericsTest() {
             "    Toys.Generic<String> g = new Toys.Generic<>();",
             "    // BUG: Diagnostic contains: passing @Nullable parameter 'null'",
             "    g.getString(null);",
+            "    // BUG: Diagnostic contains: passing @Nullable parameter 'null'",
+            "    Toys.genericParam(null);",
             "  }",
             "}")
         .doTest();
@@ -121,6 +124,8 @@ public void jarinferNullableReturnsTest() {
    * project which determines which SDK version's models are being tested.
    */
   @Test
+  @Ignore(
+      "temporarily ignore while making some astubx format changes; see https://github.com/uber/NullAway/issues/1072")
   public void jarInferAndroidSDKModels() {
     compilationHelper
         .setArgs(

jar-infer/test-java-lib-jarinfer/src/main/java/com/uber/nullaway/jarinfer/toys/unannotated/Toys.java

@@ -43,6 +43,10 @@ public String getString(T t) {
     }
   }
 
+  public static void genericParam(Generic<String> g) {
+    g.getString("hello");
+  }
+
   public static void main(String arg[]) throws java.io.IOException {
     String s = "test string...";
     Foo f = new Foo("let's");

jmh/src/main/java/com/uber/nullaway/jmh/NullawayJavac.java

@@ -186,6 +186,7 @@ private NullawayJavac(
             "-d",
             outputDir.toAbsolutePath().toString(),
             "-XDcompilePolicy=simple",
+            "--should-stop=ifError=FLOW",
             "-Xplugin:ErrorProne -XepDisableAllChecks -Xep:NullAway:ERROR -XepOpt:NullAway:AnnotatedPackages="
                 + annotatedPackages
                 + String.join(" ", extraErrorProneArgs)));

nullaway/src/main/java/com/uber/nullaway/NullAway.java

@@ -1852,7 +1852,7 @@ private Description handleInvocation(
       }
       if (config.isJSpecifyMode()) {
         GenericsChecks.compareGenericTypeParameterNullabilityForCall(
-            formalParams, actualParams, varArgsMethod, this, state);
+            methodSymbol, tree, actualParams, varArgsMethod, this, state);
         if (!methodSymbol.getTypeParameters().isEmpty()) {
           GenericsChecks.checkGenericMethodCallTypeArguments(tree, state, this, config, handler);
         }