Fix bug with computing direct type use annotations on parameters (#864)

NullAway was still treating annotations on generic type arguments as being on the top-level type of a parameter itself, which could lead to false positives and potentially also missed bugs.
uber · Nov 19, 2023 · fbd076a · fbd076a
1 parent 01aa34e
commit fbd076a
Show file tree

Hide file tree

Showing 5 changed files with 46 additions and 6 deletions.
diff --git a/gradle/dependencies.gradle b/gradle/dependencies.gradle
@@ -102,7 +102,7 @@ def test = [
         "org.junit.jupiter:junit-jupiter-api:5.0.2",
         "org.apiguardian:apiguardian-api:1.0.0"
     ],
-    jetbrainsAnnotations    : "org.jetbrains:annotations:13.0",
+    jetbrainsAnnotations    : "org.jetbrains:annotations:24.1.0",
     cfQual                  : "org.checkerframework:checker-qual:${versions.checkerFramework}",
     // 2.5.5 is the last release to contain this artifact
     cfCompatQual            : "org.checkerframework:checker-compat-qual:2.5.5",

diff --git a/nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java b/nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java
@@ -175,7 +175,8 @@ public static TreePath findEnclosingMethodOrLambdaOrInitializer(TreePath path) {
 
   /**
    * NOTE: this method does not work for getting all annotations of parameters of methods from class
-   * files. For that case, use {@link #getAllAnnotationsForParameter(Symbol.MethodSymbol, int)}
+   * files. For that case, use {@link #getAllAnnotationsForParameter(Symbol.MethodSymbol, int,
+   * Config)}
    *
    * @param symbol the symbol
    * @return all annotations on the symbol and on the type of the symbol
@@ -259,18 +260,20 @@ public static Stream<? extends AnnotationMirror> getAllAnnotations(Symbol symbol
    *
    * @param symbol the method symbol
    * @param paramInd index of the parameter
+   * @param config NullAway configuration
    * @return all declaration and type-use annotations for the parameter
    */
   public static Stream<? extends AnnotationMirror> getAllAnnotationsForParameter(
-      Symbol.MethodSymbol symbol, int paramInd) {
+      Symbol.MethodSymbol symbol, int paramInd, Config config) {
     Symbol.VarSymbol varSymbol = symbol.getParameters().get(paramInd);
     return Stream.concat(
         varSymbol.getAnnotationMirrors().stream(),
         symbol.getRawTypeAttributes().stream()
             .filter(
                 t ->
                     t.position.type.equals(TargetType.METHOD_FORMAL_PARAMETER)
-                        && t.position.parameter_index == paramInd));
+                        && t.position.parameter_index == paramInd
+                        && NullabilityUtil.isDirectTypeUseAnnotation(t, config)));
   }
 
   /**

diff --git a/nullaway/src/main/java/com/uber/nullaway/Nullness.java b/nullaway/src/main/java/com/uber/nullaway/Nullness.java
@@ -218,7 +218,7 @@ public static boolean paramHasNullableAnnotation(
       return true;
     }
     return hasNullableAnnotation(
-        NullabilityUtil.getAllAnnotationsForParameter(symbol, paramInd), config);
+        NullabilityUtil.getAllAnnotationsForParameter(symbol, paramInd, config), config);
   }
 
   private static boolean isRecordEqualsParam(Symbol.MethodSymbol symbol, int paramInd) {
@@ -249,6 +249,6 @@ private static boolean isRecordEqualsParam(Symbol.MethodSymbol symbol, int param
   public static boolean paramHasNonNullAnnotation(
       Symbol.MethodSymbol symbol, int paramInd, Config config) {
     return hasNonNullAnnotation(
-        NullabilityUtil.getAllAnnotationsForParameter(symbol, paramInd), config);
+        NullabilityUtil.getAllAnnotationsForParameter(symbol, paramInd, config), config);
   }
 }
diff --git a/nullaway/src/test/java/com/uber/nullaway/NullAwayJSpecifyTests.java b/nullaway/src/test/java/com/uber/nullaway/NullAwayJSpecifyTests.java
@@ -987,6 +987,31 @@ public void nullUnmarkedAndAcknowledgeRestrictiveAnnotations() {
         .doTest();
   }
 
+  @Test
+  public void nullUnmarkedRestrictiveAnnotationsAndGenerics() {
+    makeTestHelperWithArgs(
+            Arrays.asList(
+                "-d",
+                temporaryFolder.getRoot().getAbsolutePath(),
+                "-XepOpt:NullAway:AnnotatedPackages=com.uber",
+                "-XepOpt:NullAway:AcknowledgeRestrictiveAnnotations=true"))
+        .addSourceLines(
+            "Test.java",
+            "package com.uber;",
+            "import org.jspecify.annotations.NullUnmarked;",
+            "import org.jetbrains.annotations.Nullable;",
+            "import org.jetbrains.annotations.NotNull;",
+            "import java.util.List;",
+            "public class Test {",
+            "  @NullUnmarked",
+            "  public static void takesNullable(@Nullable List<@NotNull String> l) {}",
+            "  public static void test() {",
+            "    takesNullable(null);",
+            "  }",
+            "}")
+        .doTest();
+  }
+
   @Test
   public void nullMarkedStaticImports() {
     makeTestHelperWithArgs(

diff --git a/nullaway/src/test/java/com/uber/nullaway/NullAwayTypeUseAnnotationsTests.java b/nullaway/src/test/java/com/uber/nullaway/NullAwayTypeUseAnnotationsTests.java
@@ -38,16 +38,28 @@ public void annotationAppliedToTypeParameter() {
             "import java.util.List;",
             "import java.util.ArrayList;",
             "import org.checkerframework.checker.nullness.qual.Nullable;",
+            "import org.checkerframework.checker.nullness.qual.NonNull;",
             "class TypeArgumentAnnotation {",
             "  List<@Nullable String> fSafe = new ArrayList<>();",
             "  @Nullable List<String> fUnsafe = new ArrayList<>();",
             "  void useParamSafe(List<@Nullable String> list) {",
             "    list.hashCode();",
             "  }",
+            "  void unsafeCall() {",
+            "    // BUG: Diagnostic contains: passing @Nullable parameter",
+            "    useParamSafe(null);",
+            "  }",
             "  void useParamUnsafe(@Nullable List<String> list) {",
             "    // BUG: Diagnostic contains: dereferenced",
             "    list.hashCode();",
             "  }",
+            "  void useParamUnsafeNonNullElements(@Nullable List<@NonNull String> list) {",
+            "    // BUG: Diagnostic contains: dereferenced",
+            "    list.hashCode();",
+            "  }",
+            "  void safeCall() {",
+            "    useParamUnsafeNonNullElements(null);",
+            "  }",
             "  void useFieldSafe() {",
             "    fSafe.hashCode();",
             "  }",