From 35279f0a41360c9fef200ffdcd5d8e17a2719d1e Mon Sep 17 00:00:00 2001 From: Manu Sridharan Date: Thu, 12 Dec 2024 13:02:45 -0800 Subject: [PATCH] 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. --- gradle/dependencies.gradle | 2 +- .../DefinitelyDerefedParamsDriver.java | 93 +++++++++++++++---- .../uber/nullaway/jarinfer/JarInferTest.java | 24 ++++- .../jarinfer/JarInferIntegrationTest.java | 2 + .../jarinfer/toys/unannotated/Toys.java | 4 + .../handlers/InferredJARModelsHandler.java | 22 +---- 6 files changed, 111 insertions(+), 36 deletions(-) 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/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(); - } - } }