diff --git a/gradle/dependencies.gradle b/gradle/dependencies.gradle index 81af7d7b6f..417be1dde9 100755 --- 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/jar-infer-lib/src/main/java/com/uber/nullaway/jarinfer/DefinitelyDerefedParamsDriver.java b/jar-infer/jar-infer-lib/src/main/java/com/uber/nullaway/jarinfer/DefinitelyDerefedParamsDriver.java index ea06ebd4a1..1cd4c79ade 100644 --- a/jar-infer/jar-infer-lib/src/main/java/com/uber/nullaway/jarinfer/DefinitelyDerefedParamsDriver.java +++ b/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,18 +508,32 @@ 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 @@ -524,17 +541,61 @@ private static String getAstubxSignature(IMethod mtd) { + (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 index 10345ac7d5..a98ffa4100 100644 --- 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 {", " 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)", + Sets.newHashSet(0)), + "public class TestGeneric {", + " static class Generic {", + " public String foo(T t) {", + " return \"hi\";", + " }", + " }", + " public String getString(Generic g) {", + " return g.foo(\"test\");", + " }", + "}"); + } + @Test public void toyJARAnnotatingClasses() throws Exception { testAnnotationInJarTemplate( diff --git a/jar-infer/nullaway-integration-test/src/test/java/com/uber/nullaway/jarinfer/JarInferIntegrationTest.java b/jar-infer/nullaway-integration-test/src/test/java/com/uber/nullaway/jarinfer/JarInferIntegrationTest.java index e7f2c07e98..45d7487f6d 100644 --- a/jar-infer/nullaway-integration-test/src/test/java/com/uber/nullaway/jarinfer/JarInferIntegrationTest.java +++ b/jar-infer/nullaway-integration-test/src/test/java/com/uber/nullaway/jarinfer/JarInferIntegrationTest.java @@ -88,6 +88,8 @@ public void genericsTest() { " Toys.Generic 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/jar-infer/test-java-lib-jarinfer/src/main/java/com/uber/nullaway/jarinfer/toys/unannotated/Toys.java b/jar-infer/test-java-lib-jarinfer/src/main/java/com/uber/nullaway/jarinfer/toys/unannotated/Toys.java index 1c781faa52..f397a14942 100644 --- a/jar-infer/test-java-lib-jarinfer/src/main/java/com/uber/nullaway/jarinfer/toys/unannotated/Toys.java +++ b/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 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/dataflow/AccessPath.java b/nullaway/src/main/java/com/uber/nullaway/dataflow/AccessPath.java index a75fd858a9..a7b2d0a4e1 100644 --- a/nullaway/src/main/java/com/uber/nullaway/dataflow/AccessPath.java +++ b/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 INTEGER_TYPE_SUPPLIER = + Suppliers.typeFromString("java.lang.Integer"); + + private static final Supplier 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 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: diff --git a/nullaway/src/main/java/com/uber/nullaway/generics/CheckIdenticalNullabilityVisitor.java b/nullaway/src/main/java/com/uber/nullaway/generics/CheckIdenticalNullabilityVisitor.java index 5f1bac2742..d567c2e028 100644 --- a/nullaway/src/main/java/com/uber/nullaway/generics/CheckIdenticalNullabilityVisitor.java +++ b/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()) { diff --git a/nullaway/src/main/java/com/uber/nullaway/handlers/InferredJARModelsHandler.java b/nullaway/src/main/java/com/uber/nullaway/handlers/InferredJARModelsHandler.java index edaaf9d83b..aec47b9db9 100644 --- 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(); - } - } } diff --git a/nullaway/src/test/java/com/uber/nullaway/AccessPathsTests.java b/nullaway/src/test/java/com/uber/nullaway/AccessPathsTests.java index 1061e9f7c6..354b00cf35 100644 --- a/nullaway/src/test/java/com/uber/nullaway/AccessPathsTests.java +++ b/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 map = new HashMap<>();", + " private final Map 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 map = new HashMap<>();", + " public void putThenGet() {", + " map.put(valueOf(10), new Object());", + " map.get(valueOf(10)).toString();", + " }", + "}") + .doTest(); + } } diff --git a/nullaway/src/test/java/com/uber/nullaway/jspecify/GenericsTests.java b/nullaway/src/test/java/com/uber/nullaway/jspecify/GenericsTests.java index 74a7ef481f..946b6963c4 100644 --- a/nullaway/src/test/java/com/uber/nullaway/jspecify/GenericsTests.java +++ b/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 create();", + " }", + " public interface Expiry {}", + " static class Config {", + " Config setFactory(Optional>> factory) {", + " return this;", + " }", + " }", + " static void caller(Config config) {", + " // checking that we don't crash", + " config.setFactory(Optional.empty());", + " }", + "}") + .doTest(); + } + private CompilationTestHelper makeHelper() { return makeTestHelperWithArgs( Arrays.asList(