Fix JarInfer parameter indexes for instance methods (#1071)

uber · Nov 12, 2024 · 3d1eb3c · 3d1eb3c
1 parent 0a5fa0f
commit 3d1eb3c
Show file tree

Hide file tree

Showing 5 changed files with 19 additions and 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
@@ -172,11 +172,9 @@ private static void annotateBytecode(
       }
       Set<Integer> 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-lib/src/main/java/com/uber/nullaway/jarinfer/DefinitelyDerefedParamsDriver.java b/...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<Integer> 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
@@ -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<T> {",
         "  public String foo(T t) {",
         "    return t.toString();",

diff --git a/...ay-integration-test/src/test/java/com/uber/nullaway/jarinfer/JarInferIntegrationTest.java b/...ay-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
@@ -131,8 +131,7 @@ public Nullness[] onOverrideMethodInvocationParametersNullability(
     for (Map.Entry<Integer, Set<String>> 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;
       }