diff --git a/.github/workflows/continuous-integration.yml b/.github/workflows/continuous-integration.yml index 486da4d1fc..fcb2fb1602 100644 --- a/.github/workflows/continuous-integration.yml +++ b/.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: diff --git a/build.gradle b/build.gradle index a8533c903b..c3cb4c0c0d 100644 --- a/build.gradle +++ b/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) + } } diff --git a/gradle/dependencies.gradle b/gradle/dependencies.gradle index b74e030fda..08bb8bf672 100755 --- a/gradle/dependencies.gradle +++ b/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", diff --git a/gradle/wrapper/gradle-wrapper.properties b/gradle/wrapper/gradle-wrapper.properties index fb602ee2af..82dd18b204 100644 --- a/gradle/wrapper/gradle-wrapper.properties +++ b/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 diff --git a/jar-infer/jar-infer-lib/src/main/java/com/uber/nullaway/jarinfer/BytecodeAnnotator.java b/jar-infer/jar-infer-lib/src/main/java/com/uber/nullaway/jarinfer/BytecodeAnnotator.java index c23f5214b1..43e0692fb1 100644 --- a/jar-infer/jar-infer-lib/src/main/java/com/uber/nullaway/jarinfer/BytecodeAnnotator.java +++ b/jar-infer/jar-infer-lib/src/main/java/com/uber/nullaway/jarinfer/BytecodeAnnotator.java @@ -172,11 +172,9 @@ private static void annotateBytecode( } Set 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", 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 17f1f72a9f..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; @@ -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 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,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 @@ -518,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 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) + + ">"; + } } } 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 e1fe8495d0..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 @@ -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 {", " 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 2b04b5fd4c..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 @@ -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 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( 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/jmh/src/main/java/com/uber/nullaway/jmh/NullawayJavac.java b/jmh/src/main/java/com/uber/nullaway/jmh/NullawayJavac.java index 0691af9c62..4c8a752bdf 100644 --- a/jmh/src/main/java/com/uber/nullaway/jmh/NullawayJavac.java +++ b/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))); diff --git a/nullaway/src/main/java/com/uber/nullaway/NullAway.java b/nullaway/src/main/java/com/uber/nullaway/NullAway.java index e7c6af8f47..38b1b1d6dd 100644 --- a/nullaway/src/main/java/com/uber/nullaway/NullAway.java +++ b/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); } diff --git a/nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java b/nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java index 7f39ca2375..85ca6861bb 100644 --- a/nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java +++ b/nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java @@ -595,14 +595,16 @@ public static void checkTypeParameterNullnessForConditionalExpression( * Checks that for each parameter p at a call, the type parameter nullability for p's type matches * that of the corresponding formal parameter. If a mismatch is found, report an error. * - * @param formalParams the formal parameters - * @param actualParams the actual parameters + * @param methodSymbol the symbol for the method being called + * @param tree the tree representing the method call + * @param actualParams the actual parameters at the call * @param isVarArgs true if the call is to a varargs method * @param analysis the analysis object * @param state the visitor state */ public static void compareGenericTypeParameterNullabilityForCall( - List formalParams, + Symbol.MethodSymbol methodSymbol, + Tree tree, List actualParams, boolean isVarArgs, NullAway analysis, @@ -610,14 +612,35 @@ public static void compareGenericTypeParameterNullabilityForCall( if (!analysis.getConfig().isJSpecifyMode()) { return; } - int n = formalParams.size(); + Type invokedMethodType = methodSymbol.type; + // substitute class-level type arguments for instance methods + if (!methodSymbol.isStatic() && tree instanceof MethodInvocationTree) { + ExpressionTree methodSelect = ((MethodInvocationTree) tree).getMethodSelect(); + Type enclosingType; + if (methodSelect instanceof MemberSelectTree) { + enclosingType = getTreeType(((MemberSelectTree) methodSelect).getExpression(), state); + } else { + // implicit this parameter + enclosingType = methodSymbol.owner.type; + } + if (enclosingType != null) { + invokedMethodType = state.getTypes().memberType(enclosingType, methodSymbol); + } + } + // substitute type arguments for generic methods + if (tree instanceof MethodInvocationTree && methodSymbol.type instanceof Type.ForAll) { + invokedMethodType = + substituteTypeArgsInGenericMethodType((MethodInvocationTree) tree, methodSymbol, state); + } + List formalParamTypes = invokedMethodType.getParameterTypes(); + int n = formalParamTypes.size(); if (isVarArgs) { // If the last argument is var args, don't check it now, it will be checked against // all remaining actual arguments in the next loop. n = n - 1; } for (int i = 0; i < n; i++) { - Type formalParameter = formalParams.get(i).type; + Type formalParameter = formalParamTypes.get(i); if (formalParameter.isRaw()) { // bail out of any checking involving raw types for now return; @@ -630,11 +653,11 @@ public static void compareGenericTypeParameterNullabilityForCall( } } } - if (isVarArgs && !formalParams.isEmpty()) { + if (isVarArgs && !formalParamTypes.isEmpty()) { Type.ArrayType varargsArrayType = - (Type.ArrayType) formalParams.get(formalParams.size() - 1).type; + (Type.ArrayType) formalParamTypes.get(formalParamTypes.size() - 1); Type varargsElementType = varargsArrayType.elemtype; - for (int i = formalParams.size() - 1; i < actualParams.size(); i++) { + for (int i = formalParamTypes.size() - 1; i < actualParams.size(); i++) { Type actualParameterType = getTreeType(actualParams.get(i), state); // If the actual parameter type is assignable to the varargs array type, then the call site // is passing the varargs directly in an array, and we should skip our check. @@ -796,19 +819,9 @@ public static Nullness getGenericReturnNullnessAtInvocation( Config config) { // If generic method invocation if (!invokedMethodSymbol.getTypeParameters().isEmpty()) { - List typeArgumentTrees = tree.getTypeArguments(); - com.sun.tools.javac.util.List explicitTypeArgs = - convertTreesToTypes(typeArgumentTrees); // Convert to Type objects - Type.ForAll forAllType = (Type.ForAll) invokedMethodSymbol.type; - // Extract the underlying MethodType (the actual signature) - Type.MethodType methodTypeInsideForAll = (Type.MethodType) forAllType.asMethodType(); // Substitute type arguments inside the return type - // NOTE: if the return type it not a type variable of the method itself, or if - // explicitTypeArgs is empty, this is a noop. Type substitutedReturnType = - state - .getTypes() - .subst(methodTypeInsideForAll.restype, forAllType.tvars, explicitTypeArgs); + substituteTypeArgsInGenericMethodType(tree, invokedMethodSymbol, state).getReturnType(); // If this condition evaluates to false, we fall through to the subsequent logic, to handle // type variables declared on the enclosing class if (substitutedReturnType != null @@ -842,6 +855,27 @@ private static com.sun.tools.javac.util.List convertTreesToTypes( return com.sun.tools.javac.util.List.from(types); } + /** + * Substitutes the type arguments from a generic method invocation into the method's type. + * + * @param methodInvocationTree the method invocation tree + * @param methodSymbol symbol for the invoked generic method + * @param state the visitor state + * @return the substituted method type for the generic method + */ + private static Type substituteTypeArgsInGenericMethodType( + MethodInvocationTree methodInvocationTree, + Symbol.MethodSymbol methodSymbol, + VisitorState state) { + + List typeArgumentTrees = methodInvocationTree.getTypeArguments(); + com.sun.tools.javac.util.List explicitTypeArgs = convertTreesToTypes(typeArgumentTrees); + + Type.ForAll forAllType = (Type.ForAll) methodSymbol.type; + Type.MethodType underlyingMethodType = (Type.MethodType) forAllType.qtype; + return state.getTypes().subst(underlyingMethodType, forAllType.tvars, explicitTypeArgs); + } + /** * Computes the nullness of a formal parameter of a generic method at an invocation, in the * context of the declared type of its receiver argument. If the formal parameter's type is a type @@ -884,23 +918,11 @@ public static Nullness getGenericParameterNullnessAtInvocation( Config config) { // If generic method invocation if (!invokedMethodSymbol.getTypeParameters().isEmpty()) { - List typeArgumentTrees = tree.getTypeArguments(); - com.sun.tools.javac.util.List explicitTypeArgs = - convertTreesToTypes(typeArgumentTrees); // Convert to Type objects - - Type.ForAll forAllType = (Type.ForAll) invokedMethodSymbol.type; - // Extract the underlying MethodType (the actual signature) - Type.MethodType methodTypeInsideForAll = (Type.MethodType) forAllType.qtype; // Substitute the argument types within the MethodType // NOTE: if explicitTypeArgs is empty, this is a noop List substitutedParamTypes = - state - .getTypes() - .subst( - methodTypeInsideForAll.argtypes, - forAllType.tvars, // The type variables from the ForAll - explicitTypeArgs // The actual type arguments from the method invocation - ); + substituteTypeArgsInGenericMethodType(tree, invokedMethodSymbol, state) + .getParameterTypes(); // If this condition evaluates to false, we fall through to the subsequent logic, to handle // type variables declared on the enclosing class if (substitutedParamTypes != null 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 6915406d7c..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; @@ -131,8 +129,7 @@ public Nullness[] onOverrideMethodInvocationParametersNullability( for (Map.Entry> annotationEntry : methodArgAnnotations.entrySet()) { if (annotationEntry.getKey() != RETURN && annotationEntry.getValue().contains("javax.annotation.Nonnull")) { - // Skip 'this' param for non-static methods - int nonNullPosition = annotationEntry.getKey() - (methodSymbol.isStatic() ? 0 : 1); + int nonNullPosition = annotationEntry.getKey(); jiNonNullParams.add(nonNullPosition); argumentPositionNullness[nonNullPosition] = Nullness.NONNULL; } @@ -231,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/main/java/com/uber/nullaway/handlers/contract/ContractHandler.java b/nullaway/src/main/java/com/uber/nullaway/handlers/contract/ContractHandler.java index a1d15c7fc5..27b3b8dd9f 100644 --- a/nullaway/src/main/java/com/uber/nullaway/handlers/contract/ContractHandler.java +++ b/nullaway/src/main/java/com/uber/nullaway/handlers/contract/ContractHandler.java @@ -208,7 +208,7 @@ public NullnessHint onDataflowVisitMethodInvocation( for (int i = 0; i < antecedent.length; ++i) { String valueConstraint = antecedent[i].trim(); if (valueConstraint.equals("_")) { - continue; + // do nothing } else if (valueConstraint.equals("false") || valueConstraint.equals("true")) { // We handle boolean constraints in the case that the boolean argument is the result // of a null or not-null check. For example, @@ -262,8 +262,7 @@ public NullnessHint onDataflowVisitMethodInvocation( } else if (valueConstraint.equals("!null") && inputs.valueOfSubNode(node.getArgument(i)).equals(Nullness.NONNULL)) { // We already know this argument can't be null, so we can treat it as not part of the - // clause for the purpose of deciding the non-nullness of the other arguments. - continue; + // clause for the purpose of deciding the non-nullness of the other arguments; do nothing } else if (valueConstraint.equals("null") || valueConstraint.equals("!null")) { if (arg != null) { // More than one argument involved in the antecedent, ignore this rule diff --git a/nullaway/src/test/java/com/uber/nullaway/jspecify/GenericMethodTests.java b/nullaway/src/test/java/com/uber/nullaway/jspecify/GenericMethodTests.java index 6b9d409ba2..7bbe2c4ddf 100644 --- a/nullaway/src/test/java/com/uber/nullaway/jspecify/GenericMethodTests.java +++ b/nullaway/src/test/java/com/uber/nullaway/jspecify/GenericMethodTests.java @@ -105,8 +105,36 @@ public void genericInstanceMethods() { } @Test - @Ignore("requires generic method support") public void genericMethodAndVoidType() { + makeHelper() + .addSourceLines( + "Test.java", + "package com.uber;", + "import org.jspecify.annotations.Nullable;", + "class Test {", + " static class Foo {", + " void foo(C c, Visitor visitor) {", + " visitor.visit(this, c);", + " }", + " }", + " static abstract class Visitor {", + " abstract void visit(Foo foo, C c);", + " }", + " static class MyVisitor extends Visitor<@Nullable Void> {", + " @Override", + " void visit(Foo foo, @Nullable Void c) {}", + " }", + " static void test(Foo f) {", + " // this is safe", + " f.<@Nullable Void>foo(null, new MyVisitor());", + " }", + "}") + .doTest(); + } + + @Test + @Ignore("requires inference of generic method type arguments") + public void genericMethodAndVoidTypeWithInference() { makeHelper() .addSourceLines( "Test.java", @@ -133,6 +161,35 @@ public void genericMethodAndVoidType() { .doTest(); } + @Test + public void issue1035() { + makeHelper() + .addSourceLines( + "Todo.java", + "import org.jspecify.annotations.*;", + "@NullMarked", + "public class Todo {", + " public static T foo(NullableSupplier code) {", + " return code.get();", + " }", + " public static void main(String[] args) {", + " // BUG: Diagnostic contains: returning @Nullable expression from method with @NonNull return type", + " Todo.foo(() -> null);", + " Todo.<@Nullable Object>foo(() -> null);", + " }", + " // this method should have no errors once we support inference for generic methods", + " public static void requiresInferenceSupport() {", + " // BUG: Diagnostic contains: returning @Nullable expression from method with @NonNull return type", + " Todo.foo(() -> null);", + " }", + " @FunctionalInterface", + " public interface NullableSupplier {", + " T get();", + " }", + "}") + .doTest(); + } + private CompilationTestHelper makeHelper() { return makeTestHelperWithArgs( Arrays.asList( 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 c686b25f0c..74a7ef481f 100644 --- a/nullaway/src/test/java/com/uber/nullaway/jspecify/GenericsTests.java +++ b/nullaway/src/test/java/com/uber/nullaway/jspecify/GenericsTests.java @@ -938,6 +938,28 @@ public void parameterPassing() { .doTest(); } + @Test + public void parameterPassingInstanceMethods() { + makeHelper() + .addSourceLines( + "Test.java", + "package com.uber;", + "import org.jspecify.annotations.Nullable;", + "class Test {", + " static class A {", + " void foo(A a) {}", + " void bar(A a) { foo(a); this.foo(a); }", + " }", + " static void test(A<@Nullable String> p, A q) {", + " // BUG: Diagnostic contains: Cannot pass parameter of type", + " p.foo(q);", + " // this one is fine", + " p.foo(p);", + " }", + "}") + .doTest(); + } + @Test public void varargsParameter() { makeHelper() diff --git a/sample-app/build.gradle b/sample-app/build.gradle index 0ab1e6f7f0..40c7b1bde5 100644 --- a/sample-app/build.gradle +++ b/sample-app/build.gradle @@ -47,6 +47,7 @@ android { variant.getJavaCompileProvider().configure { options.compilerArgs += [ "-XDcompilePolicy=simple", + "--should-stop=ifError=FLOW", "-Xplugin:ErrorProne -XepOpt:NullAway:AnnotatedPackages=com.uber", ] options.fork = true