From 3d1eb3c7cfdb00043b661f26dd8165d6f553d76c Mon Sep 17 00:00:00 2001 From: Manu Sridharan Date: Tue, 12 Nov 2024 08:11:58 -0800 Subject: [PATCH] Fix JarInfer parameter indexes for instance methods (#1071) --- .../uber/nullaway/jarinfer/BytecodeAnnotator.java | 4 +--- .../jarinfer/DefinitelyDerefedParamsDriver.java | 8 +++++++- .../com/uber/nullaway/jarinfer/JarInferTest.java | 14 +++++++------- .../nullaway/jarinfer/JarInferIntegrationTest.java | 3 +++ .../handlers/InferredJARModelsHandler.java | 3 +-- 5 files changed, 19 insertions(+), 13 deletions(-) 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..ea06ebd4a1 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 @@ -273,7 +273,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 +284,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) { 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..10345ac7d5 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;", @@ -449,7 +449,7 @@ public void testGenericMethod() throws Exception { "generic", "TestGeneric", ImmutableMap.of( - "generic.TestGeneric:java.lang.String foo(java.lang.Object)", Sets.newHashSet(1)), + "generic.TestGeneric:java.lang.String foo(java.lang.Object)", Sets.newHashSet(0)), "public class TestGeneric {", " public String foo(T t) {", " return t.toString();", 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..e7f2c07e98 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; @@ -121,6 +122,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/nullaway/src/main/java/com/uber/nullaway/handlers/InferredJARModelsHandler.java b/nullaway/src/main/java/com/uber/nullaway/handlers/InferredJARModelsHandler.java index 6915406d7c..edaaf9d83b 100644 --- a/nullaway/src/main/java/com/uber/nullaway/handlers/InferredJARModelsHandler.java +++ b/nullaway/src/main/java/com/uber/nullaway/handlers/InferredJARModelsHandler.java @@ -131,8 +131,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; }