Merge branch 'master' into issue-1088

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",

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;
@@ -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)
+          + ">";
+    }
   }
 }

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(

jar-infer/nullaway-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();

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");

nullaway/src/main/java/com/uber/nullaway/dataflow/AccessPath.java

@@ -28,6 +28,8 @@
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableSet;
 import com.google.errorprone.VisitorState;
+import com.google.errorprone.suppliers.Supplier;
+import com.google.errorprone.suppliers.Suppliers;
 import com.google.errorprone.util.ASTHelpers;
 import com.sun.source.tree.LiteralTree;
 import com.sun.source.tree.MethodInvocationTree;
@@ -75,6 +77,12 @@
  */
 public final class AccessPath implements MapKey {
 
+  private static final Supplier<Type> INTEGER_TYPE_SUPPLIER =
+      Suppliers.typeFromString("java.lang.Integer");
+
+  private static final Supplier<Type> LONG_TYPE_SUPPLIER =
+      Suppliers.typeFromString("java.lang.Long");
+
   /**
    * A prefix added for elements appearing in method invocation APs which represent fields that can
    * be proven to be class-initialization time constants (i.e. static final fields of a type known
@@ -278,14 +286,15 @@ private static Node stripCasts(Node node) {
         return new NumericMapKey(((LongLiteralNode) argument).getValue());
       case METHOD_INVOCATION:
         MethodAccessNode target = ((MethodInvocationNode) argument).getTarget();
-        Node receiver = stripCasts(target.getReceiver());
         List<Node> arguments = ((MethodInvocationNode) argument).getArguments();
         // Check for int/long boxing.
         if (target.getMethod().getSimpleName().toString().equals("valueOf")
-            && arguments.size() == 1
-            && castToNonNull(receiver.getTree()).getKind().equals(Tree.Kind.IDENTIFIER)
-            && (receiver.toString().equals("Integer") || receiver.toString().equals("Long"))) {
-          return argumentToMapKeySpecifier(arguments.get(0), state, apContext);
+            && arguments.size() == 1) {
+          Type ownerType = ((Symbol.MethodSymbol) target.getMethod()).owner.type;
+          if (ASTHelpers.isSameType(ownerType, INTEGER_TYPE_SUPPLIER.get(state), state)
+              || ASTHelpers.isSameType(ownerType, LONG_TYPE_SUPPLIER.get(state), state)) {
+            return argumentToMapKeySpecifier(arguments.get(0), state, apContext);
+          }
         }
       // Fine to fallthrough:
       default:

nullaway/src/main/java/com/uber/nullaway/generics/CheckIdenticalNullabilityVisitor.java

@@ -35,10 +35,10 @@ public Boolean visitClassType(Type.ClassType lhsType, Type rhsType) {
     // The base type of rhsType may be a subtype of lhsType's base type.  In such cases, we must
     // compare lhsType against the supertype of rhsType with a matching base type.
     Type rhsTypeAsSuper = types.asSuper(rhsType, lhsType.tsym);
-    // This is impossible, considering the fact that standard Java subtyping succeeds before
-    // running NullAway
     if (rhsTypeAsSuper == null) {
-      throw new RuntimeException("Did not find supertype of " + rhsType + " matching " + lhsType);
+      // Surprisingly, this can in fact occur, in cases involving raw types.  See, e.g.,
+      // GenericsTests#issue1082 and https://github.com/uber/NullAway/pull/1086. Bail out.
+      return true;
     }
     // bail out of checking raw types for now
     if (rhsTypeAsSuper.isRaw()) {

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();
-    }
-  }
 }

nullaway/src/test/java/com/uber/nullaway/AccessPathsTests.java

@@ -434,4 +434,58 @@ public void testAccessUsingExplicitThis() {
             "}")
         .doTest();
   }
+
+  @Test
+  public void mapKeysFromValueOf() {
+    defaultCompilationHelper
+        .addSourceLines(
+            "Foo.java",
+            "package com.uber;",
+            "import java.util.HashMap;",
+            "import java.util.Map;",
+            "public class Foo {",
+            "  private final Map<Integer, Object> map = new HashMap<>();",
+            "  private final Map<Long, Object> longMap = new HashMap<>();",
+            "  static Integer valueOf(int i) { return 0; }",
+            "  static Integer valueOf(int i, int j) { return i+j; }",
+            "  public void putThenGetIntegerValueOf() {",
+            "    map.put(Integer.valueOf(10), new Object());",
+            "    map.get(Integer.valueOf(10)).toString();",
+            "  }",
+            "  public void putThenGetLongValueOf() {",
+            "    longMap.put(Long.valueOf(10), new Object());",
+            "    longMap.get(Long.valueOf(10)).toString();",
+            "  }",
+            "  public void putThenGetFooValueOf() {",
+            "    map.put(valueOf(10), new Object());",
+            "    // Unknown valueOf method so we report a warning",
+            "    // BUG: Diagnostic contains: dereferenced expression map.get(valueOf(10)) is @Nullable",
+            "    map.get(valueOf(10)).toString();",
+            "    map.put(valueOf(10,20), new Object());",
+            "    // Unknown valueOf method so we report a warning",
+            "    // BUG: Diagnostic contains: dereferenced expression map.get(valueOf(10,20)) is @Nullable",
+            "    map.get(valueOf(10,20)).toString();",
+            "  }",
+            "}")
+        .doTest();
+  }
+
+  @Test
+  public void mapKeyFromIntegerValueOfStaticImport() {
+    defaultCompilationHelper
+        .addSourceLines(
+            "Foo.java",
+            "package com.uber;",
+            "import java.util.HashMap;",
+            "import java.util.Map;",
+            "import static java.lang.Integer.valueOf;",
+            "public class Foo {",
+            "  private final Map<Integer, Object> map = new HashMap<>();",
+            "  public void putThenGet() {",
+            "    map.put(valueOf(10), new Object());",
+            "    map.get(valueOf(10)).toString();",
+            "  }",
+            "}")
+        .doTest();
+  }
 }

nullaway/src/test/java/com/uber/nullaway/jspecify/GenericsTests.java

@@ -2002,6 +2002,31 @@ public void issue1019() {
         .doTest();
   }
 
+  @Test
+  public void issue1082() {
+    makeHelper()
+        .addSourceLines(
+            "Main.java",
+            "package com.uber;",
+            "import java.util.Optional;",
+            "public class Main {",
+            "  public interface Factory<T> {",
+            "    T create();",
+            "  }",
+            "  public interface Expiry<K, V> {}",
+            "  static class Config<K, V> {",
+            "    Config<K, V> setFactory(Optional<Factory<? extends Expiry<K, V>>> factory) {",
+            "      return this;",
+            "    }",
+            "  }",
+            "  static void caller(Config config) {",
+            "    // checking that we don't crash",
+            "    config.setFactory(Optional.<Object>empty());",
+            "  }",
+            "}")
+        .doTest();
+  }
+
   private CompilationTestHelper makeHelper() {
     return makeTestHelperWithArgs(
         Arrays.asList(