Properly handle method references to null-unmarked methods (#1205)

msridhar · web-flow · commit 4a0b555401eb · 2025-04-29T19:19:36.000-07:00
Fixes #1195 We adjust our method overriding checks for a method reference to a `@NullUnmarked` method. Any overriding should be allowed, except in the presence of a restrictive annotation (`@Nullable` on the return or `@NonNull` on a parameter). Also add tests for `@NonNull` on the varargs parameter.
diff --git a/nullaway/src/main/java/com/uber/nullaway/ErrorProneCLIFlagsConfig.java b/nullaway/src/main/java/com/uber/nullaway/ErrorProneCLIFlagsConfig.java
@@ -500,7 +500,8 @@ public boolean suggestSuppressions() {
 
   @Override
   public boolean acknowledgeRestrictiveAnnotations() {
-    return isAcknowledgeRestrictive;
+    // restrictive annotations must always be acknowledged in JSpecify mode
+    return isAcknowledgeRestrictive || jspecifyMode;
   }
 
   @Override
diff --git a/nullaway/src/main/java/com/uber/nullaway/NullAway.java b/nullaway/src/main/java/com/uber/nullaway/NullAway.java
@@ -776,20 +776,27 @@ public Description matchParameterizedType(ParameterizedTypeTree tree, VisitorSta
    *     LambdaExpressionTree}; otherwise {@code null}
    * @param memberReferenceTree if the overriding method is a member reference (which "overrides" a
    *     functional interface method), the {@link MemberReferenceTree}; otherwise {@code null}
+   * @param state visitor state
+   * @param overridingMethod if available, the symbol for the overriding method
    * @return discovered error, or {@link Description#NO_MATCH} if no error
    */
   private Description checkParamOverriding(
       List<VarSymbol> overridingParamSymbols,
       Symbol.MethodSymbol overriddenMethod,
       @Nullable LambdaExpressionTree lambdaExpressionTree,
       @Nullable MemberReferenceTree memberReferenceTree,
-      VisitorState state) {
+      VisitorState state,
+      Symbol.@Nullable MethodSymbol overridingMethod) {
     com.sun.tools.javac.util.List<VarSymbol> superParamSymbols = overriddenMethod.getParameters();
     boolean unboundMemberRef =
         (memberReferenceTree != null)
             && ((JCTree.JCMemberReference) memberReferenceTree).kind.isUnbound();
     boolean isOverriddenMethodAnnotated =
         !codeAnnotationInfo.isSymbolUnannotated(overriddenMethod, config, handler);
+    boolean isOverridingMethodAnnotated =
+        (overridingMethod != null
+                && !codeAnnotationInfo.isSymbolUnannotated(overridingMethod, config, handler))
+            || lambdaExpressionTree != null;
 
     // Get argument nullability for the overridden method.  If overriddenMethodArgNullnessMap[i] is
     // null, parameter i is treated as unannotated.
@@ -885,7 +892,7 @@ private Description checkParamOverriding(
           lambdaExpressionTree != null
               && NullabilityUtil.lambdaParamIsImplicitlyTyped(
                   lambdaExpressionTree.getParameters().get(methodParamInd));
-      if (!Nullness.hasNullableAnnotation(paramSymbol, config) && !implicitlyTypedLambdaParam) {
+      if (!implicitlyTypedLambdaParam && paramIsNonNull(paramSymbol, isOverridingMethodAnnotated)) {
         String message =
             "parameter "
                 + paramSymbol.name.toString()
@@ -915,6 +922,16 @@ private Description checkParamOverriding(
     return Description.NO_MATCH;
   }
 
+  private boolean paramIsNonNull(VarSymbol paramSymbol, boolean isMethodAnnotated) {
+    if (isMethodAnnotated) {
+      return !Nullness.hasNullableAnnotation(paramSymbol, config);
+    } else if (config.acknowledgeRestrictiveAnnotations()) {
+      // can still be @NonNull if there is a restrictive annotation
+      return Nullness.hasNonNullAnnotation(paramSymbol, config);
+    }
+    return false;
+  }
+
   static Trees getTreesInstance(VisitorState state) {
     return Trees.instance(JavacProcessingEnvironment.instance(state.context));
   }
@@ -1023,7 +1040,8 @@ public Description matchLambdaExpression(LambdaExpressionTree tree, VisitorState
             funcInterfaceMethod,
             tree,
             null,
-            state);
+            state,
+            null);
     if (description != Description.NO_MATCH) {
       return description;
     }
@@ -1117,7 +1135,12 @@ && getMethodReturnNullness(overridingMethod, state, Nullness.NONNULL)
     // if any parameter in the super method is annotated @Nullable,
     // overriding method cannot assume @Nonnull
     return checkParamOverriding(
-        overridingMethod.getParameters(), overriddenMethod, null, memberReferenceTree, state);
+        overridingMethod.getParameters(),
+        overriddenMethod,
+        null,
+        memberReferenceTree,
+        state,
+        overridingMethod);
   }
 
   private boolean overriddenMethodReturnsNonNull(
diff --git a/nullaway/src/main/java/com/uber/nullaway/handlers/Handlers.java b/nullaway/src/main/java/com/uber/nullaway/handlers/Handlers.java
@@ -46,12 +46,8 @@ public static Handler buildDefault(Config config) {
     ImmutableList.Builder<Handler> handlerListBuilder = ImmutableList.builder();
     MethodNameUtil methodNameUtil = new MethodNameUtil();
 
-    // Acknowledge restrictive annotations if in JSpecify mode or if the user has explicitly
-    // requested it
-    boolean acknowledgeRestrictive =
-        config.acknowledgeRestrictiveAnnotations() || config.isJSpecifyMode();
     RestrictiveAnnotationHandler restrictiveAnnotationHandler = null;
-    if (acknowledgeRestrictive) {
+    if (config.acknowledgeRestrictiveAnnotations()) {
       // This runs before LibraryModelsHandler, so that library models can override third-party
       // bytecode annotations
       restrictiveAnnotationHandler = new RestrictiveAnnotationHandler(config);
diff --git a/nullaway/src/test/java/com/uber/nullaway/AcknowledgeRestrictiveAnnotationsTests.java b/nullaway/src/test/java/com/uber/nullaway/AcknowledgeRestrictiveAnnotationsTests.java
@@ -428,4 +428,62 @@ public void annotatedVsUnannotatedMethodRefOverrideWithRestrictiveAnnotations()
             "}")
         .doTest();
   }
+
+  @Test
+  public void methodRefToNullUnmarked() {
+    makeTestHelperWithArgs(
+            Arrays.asList(
+                "-d",
+                temporaryFolder.getRoot().getAbsolutePath(),
+                "-XepOpt:NullAway:AnnotatedPackages=com.uber",
+                "-XepOpt:NullAway:AcknowledgeRestrictiveAnnotations=true"))
+        .addSourceLines(
+            "Test.java",
+            "import org.jspecify.annotations.*;",
+            "@NullMarked",
+            "class Test {",
+            "  interface I {",
+            "    void m(@Nullable Object o);",
+            "  }",
+            "  @NullUnmarked",
+            "  void unmarkedParam(Object o) {}",
+            "  @NullUnmarked",
+            "  void nonNullParam(@NonNull Object o) {}",
+            "  void test() {",
+            "    I i = this::unmarkedParam;",
+            "    // BUG: Diagnostic contains: parameter o of referenced method is @NonNull",
+            "    I i2 = this::nonNullParam;",
+            "  }",
+            "}")
+        .doTest();
+  }
+
+  @Test
+  public void methodRefToNullUnmarkedVarargs() {
+    makeTestHelperWithArgs(
+            Arrays.asList(
+                "-d",
+                temporaryFolder.getRoot().getAbsolutePath(),
+                "-XepOpt:NullAway:AnnotatedPackages=com.uber",
+                "-XepOpt:NullAway:AcknowledgeRestrictiveAnnotations=true"))
+        .addSourceLines(
+            "Test.java",
+            "import org.jspecify.annotations.*;",
+            "@NullMarked",
+            "class Test {",
+            "  interface I {",
+            "    void m(Object @Nullable ... o);",
+            "  }",
+            "  @NullUnmarked",
+            "  void unmarkedParam(Object... o) {}",
+            "  @NullUnmarked",
+            "  void nonNullParam(Object @NonNull ... o) {}",
+            "  void test() {",
+            "    I i = this::unmarkedParam;",
+            "    // BUG: Diagnostic contains: parameter o of referenced method is @NonNull",
+            "    I i2 = this::nonNullParam;",
+            "  }",
+            "}")
+        .doTest();
+  }
 }
diff --git a/nullaway/src/test/java/com/uber/nullaway/jspecify/GenericsTests.java b/nullaway/src/test/java/com/uber/nullaway/jspecify/GenericsTests.java
@@ -2353,6 +2353,31 @@ public void issue1156() {
         .doTest();
   }
 
+  @Test
+  public void methodReferenceToNullUnmarked() {
+    makeHelper()
+        .addSourceLines(
+            "Test.java",
+            "import org.jspecify.annotations.*;",
+            "import java.util.function.Function;",
+            "@NullMarked",
+            "public class Test {",
+            "  @NullUnmarked",
+            "  public static Boolean isNull(Object o) { return o == null; }",
+            "  @NullUnmarked",
+            "  public static Boolean isNullRestrictParam(@NonNull Object o) { return o == null; }",
+            "  @NullUnmarked",
+            "  public static @Nullable Boolean isNullRestrictReturn(Object o) { return o == null; }",
+            "  static Function<@Nullable Object, Boolean> filter1 = Test::isNull;",
+            "  // BUG: Diagnostic contains: parameter o of referenced method is @NonNull, but parameter in functional",
+            "  static Function<@Nullable Object, Boolean> filter2 = Test::isNullRestrictParam;",
+            "  // BUG: Diagnostic contains: referenced method returns @Nullable",
+            "  static Function<@Nullable Object, Boolean> filter3 = Test::isNullRestrictReturn;",
+            "  static Function<@Nullable Object, @Nullable Boolean> filter4 = Test::isNullRestrictReturn;",
+            "}")
+        .doTest();
+  }
+
   @Test
   public void varargsOfGenericType() {
     makeHelper()
diff --git a/nullaway/src/test/java/com/uber/nullaway/jspecify/NullMarkednessTests.java b/nullaway/src/test/java/com/uber/nullaway/jspecify/NullMarkednessTests.java
@@ -1256,4 +1256,32 @@ public void writeToNullUnmarkedField() {
             "}")
         .doTest();
   }
+
+  @Test
+  public void methodRefToNullUnmarkedNoAcknowledgeRestrictive() {
+    makeTestHelperWithArgs(
+            Arrays.asList(
+                "-d",
+                temporaryFolder.getRoot().getAbsolutePath(),
+                "-XepOpt:NullAway:AnnotatedPackages=com.uber"))
+        .addSourceLines(
+            "Test.java",
+            "import org.jspecify.annotations.*;",
+            "@NullMarked",
+            "class Test {",
+            "  interface I {",
+            "    void m(@Nullable Object o);",
+            "  }",
+            "  @NullUnmarked",
+            "  void unmarkedParam(Object o) {}",
+            "  @NullUnmarked",
+            "  void nonNullParam(@NonNull Object o) {}",
+            "  void test() {",
+            "    I i = this::unmarkedParam;",
+            "    // no error since we don't acknowledge restrictive annotations",
+            "    I i2 = this::nonNullParam;",
+            "  }",
+            "}")
+        .doTest();
+  }
 }

Original file line number	Diff line number	Diff line change
`@@ -500,7 +500,8 @@ public boolean suggestSuppressions() {`
`500`	`500`
`501`	`501`	`@Override`
`502`	`502`	`public boolean acknowledgeRestrictiveAnnotations() {`
`503`		`- return isAcknowledgeRestrictive;`
	`503`	`+ // restrictive annotations must always be acknowledged in JSpecify mode`
	`504`	`+ return isAcknowledgeRestrictive \|\| jspecifyMode;`
`504`	`505`	`}`
`505`	`506`
`506`	`507`	`@Override`