Fix JarInfer handling of generic types (#1078)

In particular, fix cases where the type of a method parameter either contains generic type arguments or is itself a type variable. These fixes are needed for #1072. We take advantage of the fact that even though `javac` erases generics, it stores full generic type signatures in class file attributes, which WALA is able to read. So we can recover generic type information from these attributes. We needed a bug fix from WALA for this to work, hence the WALA version bump. Not every case is supported here (e.g., we haven't tested generic methods). But these changes are enough to enable #1072 to move forward, after which we will have a single place to add features and fix bugs.
uber · Dec 12, 2024 · 35279f0 · 35279f0
1 parent 933b1af
commit 35279f0
Show file tree

Hide file tree

Showing 6 changed files with 111 additions and 36 deletions.
diff --git a/gradle/dependencies.gradle b/gradle/dependencies.gradle
@@ -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.9",
     commonscli             : "1.4",
     autoValue              : "1.10.2",
     autoService            : "1.1.1",

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
@@ -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;
@@ -505,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 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 getSimpleTypeName(TypeReference typ) {
-    return StringStuff.jvmToBinaryName(typ.getName().toString());
+  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)
+          + ">";
+    }
   }
 }
diff --git a/jar-infer/jar-infer-lib/src/test/java/com/uber/nullaway/jarinfer/JarInferTest.java b/jar-infer/jar-infer-lib/src/test/java/com/uber/nullaway/jarinfer/JarInferTest.java
@@ -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(0)),
+        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(

diff --git a/...ay-integration-test/src/test/java/com/uber/nullaway/jarinfer/JarInferIntegrationTest.java b/...ay-integration-test/src/test/java/com/uber/nullaway/jarinfer/JarInferIntegrationTest.java
@@ -88,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();

diff --git a/...est-java-lib-jarinfer/src/main/java/com/uber/nullaway/jarinfer/toys/unannotated/Toys.java b/...est-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");

diff --git a/nullaway/src/main/java/com/uber/nullaway/handlers/InferredJARModelsHandler.java b/nullaway/src/main/java/com/uber/nullaway/handlers/InferredJARModelsHandler.java
@@ -27,7 +27,6 @@
 import com.sun.source.tree.ExpressionTree;
 import com.sun.source.tree.Tree;
 import com.sun.tools.javac.code.Symbol;
-import com.sun.tools.javac.code.Type;
 import com.sun.tools.javac.util.Context;
 import com.uber.nullaway.NullAway;
 import com.uber.nullaway.Nullness;
@@ -38,7 +37,6 @@
 import java.util.Map;
 import java.util.Set;
 import javax.lang.model.element.Modifier;
-import javax.lang.model.type.TypeKind;
 import org.checkerframework.nullaway.dataflow.cfg.node.MethodInvocationNode;
 import org.jspecify.annotations.Nullable;
 
@@ -230,27 +228,17 @@ private String getMethodSignature(Symbol.MethodSymbol method) {
     String methodSign =
         method.enclClass().getQualifiedName().toString()
             + ":"
-            + (method.isStaticOrInstanceInit()
-                ? ""
-                : getSimpleTypeName(method.getReturnType()) + " ")
+            + (method.isStaticOrInstanceInit() ? "" : method.getReturnType() + " ")
             + method.getSimpleName()
             + "(";
     if (!method.getParameters().isEmpty()) {
-      for (Symbol.VarSymbol var : method.getParameters()) {
-        methodSign += getSimpleTypeName(var.type) + ", ";
-      }
-      methodSign = methodSign.substring(0, methodSign.lastIndexOf(','));
+      methodSign +=
+          String.join(
+              ", ",
+              method.getParameters().stream().map(p -> p.type.toString()).toArray(String[]::new));
     }
     methodSign += ")";
     LOG(DEBUG, "DEBUG", "@ method sign: " + methodSign);
     return methodSign;
   }
-
-  private String getSimpleTypeName(Type typ) {
-    if (typ.getKind() == TypeKind.TYPEVAR) {
-      return typ.getUpperBound().tsym.getQualifiedName().toString();
-    } else {
-      return typ.toString();
-    }
-  }
 }