Don't treat @ParametricNullness as @Nullable in JSpecify mode (#1174)

msridhar · web-flow · commit b84feb73782d · 2025-03-24T09:39:19.000-07:00
Fixes #1173 In JSpecify mode, we should rely on our checking of generic types and not treat `@ParametricNullness` as a `@Nullable` annotation.
diff --git a/guava-recent-unit-tests/build.gradle b/guava-recent-unit-tests/build.gradle
@@ -33,7 +33,7 @@ dependencies {
         exclude group: "junit", module: "junit"
     }
     testImplementation deps.test.jsr305Annotations
-    testImplementation "com.google.guava:guava:31.1-jre"
+    testImplementation "com.google.guava:guava:33.4.5-jre"
 
     errorProneOldest deps.build.errorProneCheckApiOld
     errorProneOldest(deps.build.errorProneTestHelpersOld) {
diff --git a/guava-recent-unit-tests/src/test/java/com/uber/nullaway/guava/NullAwayGuavaParametricNullnessTests.java b/guava-recent-unit-tests/src/test/java/com/uber/nullaway/guava/NullAwayGuavaParametricNullnessTests.java
@@ -19,10 +19,12 @@
  * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
  * THE SOFTWARE.
  */
+package com.uber.nullaway.guava;
 
 import com.google.errorprone.CompilationTestHelper;
 import com.uber.nullaway.NullAway;
 import java.util.Arrays;
+import org.junit.Assume;
 import org.junit.Before;
 import org.junit.Rule;
 import org.junit.Test;
@@ -33,6 +35,8 @@ public class NullAwayGuavaParametricNullnessTests {
 
   private CompilationTestHelper defaultCompilationHelper;
 
+  private CompilationTestHelper jspecifyCompilationHelper;
+
   @Before
   public void setup() {
     defaultCompilationHelper =
@@ -43,6 +47,14 @@ public void setup() {
                     temporaryFolder.getRoot().getAbsolutePath(),
                     "-XepOpt:NullAway:AnnotatedPackages=com.uber,com.google.common",
                     "-XepOpt:NullAway:UnannotatedSubPackages=com.uber.nullaway.[a-zA-Z0-9.]+.unannotated"));
+    jspecifyCompilationHelper =
+        CompilationTestHelper.newInstance(NullAway.class, getClass())
+            .setArgs(
+                Arrays.asList(
+                    "-d",
+                    temporaryFolder.getRoot().getAbsolutePath(),
+                    "-XepOpt:NullAway:OnlyNullMarked=true",
+                    "-XepOpt:NullAway:JSpecifyMode=true"));
   }
 
   @Test
@@ -70,6 +82,33 @@ public void testFutureCallbackParametricNullness() {
         .doTest();
   }
 
+  @Test
+  public void jspecifyFutureCallback() {
+    // to ensure javac reads proper generic types from the Guava jar
+    Assume.assumeTrue(Runtime.version().feature() >= 23);
+    jspecifyCompilationHelper
+        .addSourceLines(
+            "Test.java",
+            "import com.google.common.util.concurrent.FutureCallback;",
+            "import org.jspecify.annotations.*;",
+            "@NullMarked",
+            "class Test {",
+            "    public static <T> FutureCallback<@Nullable T> wrapFutureCallback(FutureCallback<@Nullable T> futureCallback) {",
+            "        return new FutureCallback<@Nullable T>() {",
+            "            @Override",
+            "            public void onSuccess(@Nullable T result) {",
+            "                futureCallback.onSuccess(result);",
+            "            }",
+            "            @Override",
+            "            public void onFailure(Throwable throwable) {",
+            "                futureCallback.onFailure(throwable);",
+            "            }",
+            "        };",
+            "    }",
+            "}")
+        .doTest();
+  }
+
   @Test
   public void testIterableParametricNullness() {
     defaultCompilationHelper
@@ -91,6 +130,59 @@ public void testIterableParametricNullness() {
         .doTest();
   }
 
+  @Test
+  public void jspecifyIterables() {
+    // to ensure javac reads proper generic types from the Guava jar
+    Assume.assumeTrue(Runtime.version().feature() >= 23);
+    jspecifyCompilationHelper
+        .addSourceLines(
+            "Test.java",
+            "import com.google.common.collect.ImmutableList;",
+            "import com.google.common.collect.Iterables;",
+            "import org.jspecify.annotations.*;",
+            "@NullMarked",
+            "class Test {",
+            "    public static String test1() {",
+            "        // BUG: Diagnostic contains: returning @Nullable expression",
+            "        return Iterables.<@Nullable String>getFirst(ImmutableList.<String>of(), null);",
+            "    }",
+            "    public static @Nullable String test2() {",
+            "        return Iterables.<@Nullable String>getFirst(ImmutableList.<String>of(), null);",
+            "    }",
+            "    public static String test3() {",
+            "        return Iterables.getOnlyElement(ImmutableList.of(\"hi\"));",
+            "    }",
+            "    public static String test4() {",
+            "        // BUG: Diagnostic contains: returning @Nullable expression",
+            "        return Iterables.<@Nullable String>getOnlyElement(ImmutableList.of(\"hi\"));",
+            "    }",
+            "}")
+        .doTest();
+  }
+
+  @Test
+  public void jspecifyComparators() {
+    // to ensure javac reads proper generic types from the Guava jar
+    Assume.assumeTrue(Runtime.version().feature() >= 23);
+    jspecifyCompilationHelper
+        .addSourceLines(
+            "Test.java",
+            "import com.google.common.collect.Comparators;",
+            "import java.util.Comparator;",
+            "import org.jspecify.annotations.*;",
+            "@NullMarked",
+            "class Test {",
+            "    public static String test1(String t1, String t2, Comparator<? super String> cmp) {",
+            "        return Comparators.min(t1, t2, cmp);",
+            "    }",
+            "    public static String test2(@Nullable String t1, @Nullable String t2, Comparator<? super @Nullable String> cmp) {",
+            "        // BUG: Diagnostic contains: returning @Nullable expression",
+            "        return Comparators.<@Nullable String>min(t1, t2, cmp);",
+            "    }",
+            "}")
+        .doTest();
+  }
+
   @Test
   public void testCloserParametricNullness() {
     defaultCompilationHelper
diff --git a/nullaway/src/main/java/com/uber/nullaway/Nullness.java b/nullaway/src/main/java/com/uber/nullaway/Nullness.java
@@ -183,12 +183,14 @@ public static boolean isNullableAnnotation(String annotName, Config config) {
         || annotName.endsWith(".checkerframework.checker.nullness.compatqual.NullableDecl")
         // matches javax.annotation.CheckForNull and edu.umd.cs.findbugs.annotations.CheckForNull
         || annotName.endsWith(".CheckForNull")
-        // matches any of the multiple @ParametricNullness annotations used within Guava
-        // (see https://github.com/google/guava/issues/6126)
-        // We check the simple name first and the package prefix second for boolean short
-        // circuiting, as Guava includes
-        // many annotations
-        || (annotName.endsWith(".ParametricNullness") && annotName.startsWith("com.google.common."))
+        // matches any of the multiple @ParametricNullness annotations used within Guava (see
+        // https://github.com/google/guava/issues/6126). We check the simple name first and the
+        // package prefix second for boolean short circuiting, as Guava includes many annotations.
+        // We do _not_ match @ParametricNullness annotations in JSpecify mode and rely on generics
+        // checking instead.
+        || (!config.isJSpecifyMode()
+            && annotName.endsWith(".ParametricNullness")
+            && annotName.startsWith("com.google.common."))
         || (config.acknowledgeAndroidRecent()
             && annotName.equals("androidx.annotation.RecentlyNullable"))
         || config.isCustomNullableAnnotation(annotName);
diff --git a/nullaway/src/main/java/com/uber/nullaway/handlers/StreamNullabilityPropagator.java b/nullaway/src/main/java/com/uber/nullaway/handlers/StreamNullabilityPropagator.java
@@ -22,6 +22,8 @@
  * THE SOFTWARE.
  */
 
+import static com.uber.nullaway.NullabilityUtil.castToNonNull;
+
 import com.google.auto.value.AutoValue;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
@@ -377,7 +379,7 @@ private void handleChainFromFilter(
       } else if (collectCallToRecordsAndInnerMethodsOrLambdas.containsKey(outerCallInChain)) {
         // Update mapOrCollectRecordToFilterMap for all relevant methods / lambdas
         for (CollectRecordAndInnerMethod collectRecordAndInnerMethod :
-            collectCallToRecordsAndInnerMethodsOrLambdas.get(outerCallInChain)) {
+            collectCallToRecordsAndInnerMethodsOrLambdas.get(castToNonNull(outerCallInChain))) {
           MapOrCollectMethodToFilterInstanceRecord record =
               new MapOrCollectMethodToFilterInstanceRecord(
                   collectRecordAndInnerMethod.getCollectLikeMethodRecord(), filterMethodOrLambda);

Original file line number	Diff line number	Diff line change
`@@ -33,7 +33,7 @@ dependencies {`
`33`	`33`	`exclude group: "junit", module: "junit"`
`34`	`34`	`}`
`35`	`35`	`testImplementation deps.test.jsr305Annotations`
`36`		`- testImplementation "com.google.guava:guava:31.1-jre"`
	`36`	`+ testImplementation "com.google.guava:guava:33.4.5-jre"`
`37`	`37`
`38`	`38`	`errorProneOldest deps.build.errorProneCheckApiOld`
`39`	`39`	`errorProneOldest(deps.build.errorProneTestHelpersOld) {`