Ignore incompatibly annotated var args from Kotlin code. (#721)

See #720 for a detailed explanation. Long story short: 1. `kotlinc` will add `@org.jetbrains.annotations.NotNull` as a declaration annotation for any argument that is non-null, which we pick up from jars when running with `AcknowledgeRestrictiveAnnotations=true`. 2. Unfortunately, it will mark code such as `open fun w(vararg args: Any?)` as having `@NotNull args` (meaning the array itself). 3. This is indistinguishable at the bytecode level from Java code such as `w(@NotNull Object... args)`, which we believe should be interpreted as marking the elements of `args` as being `@NotNull`. 4. This PR hacks `RestrictiveAnnotationHandler` to skip `@org.jetbrains.annotations.NotNull` on var args only. Additionally, our basic handling of var args is pretty broken. I went over our test cases and commented where I think our current behavior is different from the desired behavior as documented in #720 and #674. Foregoing fixing it for now, as I am worried about the breaking change and I think we want to avoid changing that before 0.10.9.
uber · Jan 26, 2023 · 0a78a82 · 0a78a82
1 parent 57a89e8
commit 0a78a82
Show file tree

Hide file tree

Showing 2 changed files with 97 additions and 1 deletion.
diff --git a/nullaway/src/main/java/com/uber/nullaway/handlers/RestrictiveAnnotationHandler.java b/nullaway/src/main/java/com/uber/nullaway/handlers/RestrictiveAnnotationHandler.java
@@ -41,6 +41,8 @@
 
 public class RestrictiveAnnotationHandler extends BaseNoOpHandler {
 
+  private static final String JETBRAINS_NOT_NULL = "org.jetbrains.annotations.NotNull";
+
   private final Config config;
 
   RestrictiveAnnotationHandler(Config config) {
@@ -101,6 +103,20 @@ public Nullness[] onOverrideMethodInvocationParametersNullability(
     }
     for (int i = 0; i < methodSymbol.getParameters().size(); ++i) {
       if (Nullness.paramHasNonNullAnnotation(methodSymbol, i, config)) {
+        if (methodSymbol.isVarArgs() && i == methodSymbol.getParameters().size() - 1) {
+          // Special handling: ignore org.jetbrains.annotations.NotNull on varargs parameters
+          // to handle kotlinc generated jars (see #720)
+          // We explicitly ignore type-use annotations here, looking for @NotNull used as a
+          // declaration annotation, which is why this logic is simpler than e.g.
+          // NullabilityUtil.getAllAnnotationsForParameter.
+          boolean jetBrainsNotNullAnnotated =
+              methodSymbol.getParameters().get(i).getAnnotationMirrors().stream()
+                  .map(a -> a.getAnnotationType().toString())
+                  .anyMatch(annotName -> annotName.equals(JETBRAINS_NOT_NULL));
+          if (jetBrainsNotNullAnnotated) {
+            continue;
+          }
+        }
         argumentPositionNullness[i] = Nullness.NONNULL;
       } else if (Nullness.paramHasNullableAnnotation(methodSymbol, i, config)) {
         argumentPositionNullness[i] = Nullness.NULLABLE;

diff --git a/nullaway/src/test/java/com/uber/nullaway/NullAwayVarargsTests.java b/nullaway/src/test/java/com/uber/nullaway/NullAwayVarargsTests.java
@@ -46,13 +46,31 @@ public void testNullableVarargs() {
             "public class Utilities {",
             " public static String takesNullableVarargs(Object o, @Nullable Object... others) {",
             "  String s = o.toString() + \" \";",
-            "  // BUG: Diagnostic contains: enhanced-for expression others is @Nullable",
+            "  // BUG: Diagnostic contains: enhanced-for expression others is @Nullable", // Shouldn't be an error!
             "  for (Object other : others) {",
             "    s += (other == null) ? \"(null) \" : other.toString() + \" \";",
             "  }",
+            "  // BUG: Diagnostic contains: enhanced-for expression others is @Nullable", // Shouldn't be an error!
+            "  for (Object other : others) {",
+            "    s += other.toString();", // SHOULD be an error!
+            "  }",
             "  return s;",
             " }",
             "}")
+        .addSourceLines(
+            "Test.java",
+            "package com.uber;",
+            "import javax.annotation.Nullable;",
+            "public class Test {",
+            "  public void testNonNullVarargs(Object o1, Object o2, Object o3, @Nullable Object o4) {",
+            "    Utilities.takesNullableVarargs(o1, o2, o3, o4);",
+            "    Utilities.takesNullableVarargs(o1);", // Empty var args passed
+            "    Utilities.takesNullableVarargs(o1, o4);",
+            "    Utilities.takesNullableVarargs(o1, (java.lang.Object) null);",
+            // SHOULD be an error!
+            "    Utilities.takesNullableVarargs(o1, (java.lang.Object[]) null);",
+            "  }",
+            "}")
         .doTest();
   }
 
@@ -97,4 +115,66 @@ public void testNonNullVarargsFromHandler() {
             "}")
         .doTest();
   }
+
+  // This is required for compatibility with kotlinc generated jars
+  // See https://github.com/uber/NullAway/issues/720
+  @Test
+  public void testSkipJetbrainsNotNullOnVarArgsFromThirdPartyJars() {
+    makeTestHelperWithArgs(
+            Arrays.asList(
+                "-d",
+                temporaryFolder.getRoot().getAbsolutePath(),
+                "-XepOpt:NullAway:AnnotatedPackages=com.uber",
+                "-XepOpt:NullAway:UnannotatedSubPackages=com.uber.nullaway.[a-zA-Z0-9.]+.unannotated",
+                "-XepOpt:NullAway:AcknowledgeRestrictiveAnnotations=true"))
+        .addSourceLines(
+            "ThirdParty.java",
+            "package com.uber.nullaway.lib.unannotated;",
+            "import org.jetbrains.annotations.NotNull;",
+            "public class ThirdParty {",
+            " public static String takesNullableVarargs(@NotNull Object o, @NotNull Object... others) {",
+            "  String s = o.toString() + \" \";",
+            "  for (Object other : others) {",
+            "    s += (other == null) ? \"(null) \" : other.toString() + \" \";",
+            "  }",
+            "  return s;",
+            " }",
+            "}")
+        .addSourceLines(
+            "FirstParty.java",
+            "package com.uber;",
+            "import org.jetbrains.annotations.NotNull;",
+            "public class FirstParty {",
+            " public static String takesNonNullVarargs(@NotNull Object o, @NotNull Object... others) {",
+            "  String s = o.toString() + \" \";",
+            "  for (Object other : others) {",
+            "    s += other.toString() + \" \";",
+            "  }",
+            "  return s;",
+            " }",
+            "}")
+        .addSourceLines(
+            "Test.java",
+            "package com.uber;",
+            "import javax.annotation.Nullable;",
+            "import com.uber.nullaway.lib.unannotated.ThirdParty;",
+            "public class Test {",
+            "  public void testNullableVarargs(Object o1, Object o2, Object o3, @Nullable Object o4) {",
+            "    ThirdParty.takesNullableVarargs(o1, o2, o3, o4);",
+            "    ThirdParty.takesNullableVarargs(o1);", // Empty var args passed
+            "    ThirdParty.takesNullableVarargs(o1, o4);",
+            "    ThirdParty.takesNullableVarargs(o1, (Object) null);",
+            "    ThirdParty.takesNullableVarargs(o1, (Object[]) null);", // SHOULD be an error!
+            "    // BUG: Diagnostic contains: passing @Nullable parameter 'o4' where @NonNull",
+            "    ThirdParty.takesNullableVarargs(o4);", // First arg is not varargs.
+            "  }",
+            "  public void testNonNullVarargs(Object o1, Object o2, Object o3, @Nullable Object o4) {",
+            "    FirstParty.takesNonNullVarargs(o1, o2, o3);",
+            "    FirstParty.takesNonNullVarargs(o1);", // Empty var args passed
+            "    // BUG: Diagnostic contains: passing @Nullable parameter 'o4' where @NonNull",
+            "    FirstParty.takesNonNullVarargs(o1, o4);",
+            "  }",
+            "}")
+        .doTest();
+  }
 }