Skip to content

Commit 4a0b555

Browse files
msridharmsridhar
authored
Properly handle method references to null-unmarked methods (#1205)
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.
1 parent 1f1b386 commit 4a0b555

6 files changed

Lines changed: 141 additions & 10 deletions

File tree

‎nullaway/src/main/java/com/uber/nullaway/ErrorProneCLIFlagsConfig.java‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -500,7 +500,8 @@ public boolean suggestSuppressions() {
500500

501501
@Override
502502
public boolean acknowledgeRestrictiveAnnotations() {
503-
return isAcknowledgeRestrictive;
503+
// restrictive annotations must always be acknowledged in JSpecify mode
504+
return isAcknowledgeRestrictive || jspecifyMode;
504505
}
505506

506507
@Override

‎nullaway/src/main/java/com/uber/nullaway/NullAway.java‎

Lines changed: 27 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -776,20 +776,27 @@ public Description matchParameterizedType(ParameterizedTypeTree tree, VisitorSta
776776
* LambdaExpressionTree}; otherwise {@code null}
777777
* @param memberReferenceTree if the overriding method is a member reference (which "overrides" a
778778
* functional interface method), the {@link MemberReferenceTree}; otherwise {@code null}
779+
* @param state visitor state
780+
* @param overridingMethod if available, the symbol for the overriding method
779781
* @return discovered error, or {@link Description#NO_MATCH} if no error
780782
*/
781783
private Description checkParamOverriding(
782784
List<VarSymbol> overridingParamSymbols,
783785
Symbol.MethodSymbol overriddenMethod,
784786
@Nullable LambdaExpressionTree lambdaExpressionTree,
785787
@Nullable MemberReferenceTree memberReferenceTree,
786-
VisitorState state) {
788+
VisitorState state,
789+
Symbol.@Nullable MethodSymbol overridingMethod) {
787790
com.sun.tools.javac.util.List<VarSymbol> superParamSymbols = overriddenMethod.getParameters();
788791
boolean unboundMemberRef =
789792
(memberReferenceTree != null)
790793
&& ((JCTree.JCMemberReference) memberReferenceTree).kind.isUnbound();
791794
boolean isOverriddenMethodAnnotated =
792795
!codeAnnotationInfo.isSymbolUnannotated(overriddenMethod, config, handler);
796+
boolean isOverridingMethodAnnotated =
797+
(overridingMethod != null
798+
&& !codeAnnotationInfo.isSymbolUnannotated(overridingMethod, config, handler))
799+
|| lambdaExpressionTree != null;
793800

794801
// Get argument nullability for the overridden method. If overriddenMethodArgNullnessMap[i] is
795802
// null, parameter i is treated as unannotated.
@@ -885,7 +892,7 @@ private Description checkParamOverriding(
885892
lambdaExpressionTree != null
886893
&& NullabilityUtil.lambdaParamIsImplicitlyTyped(
887894
lambdaExpressionTree.getParameters().get(methodParamInd));
888-
if (!Nullness.hasNullableAnnotation(paramSymbol, config) && !implicitlyTypedLambdaParam) {
895+
if (!implicitlyTypedLambdaParam && paramIsNonNull(paramSymbol, isOverridingMethodAnnotated)) {
889896
String message =
890897
"parameter "
891898
+ paramSymbol.name.toString()
@@ -915,6 +922,16 @@ private Description checkParamOverriding(
915922
return Description.NO_MATCH;
916923
}
917924

925+
private boolean paramIsNonNull(VarSymbol paramSymbol, boolean isMethodAnnotated) {
926+
if (isMethodAnnotated) {
927+
return !Nullness.hasNullableAnnotation(paramSymbol, config);
928+
} else if (config.acknowledgeRestrictiveAnnotations()) {
929+
// can still be @NonNull if there is a restrictive annotation
930+
return Nullness.hasNonNullAnnotation(paramSymbol, config);
931+
}
932+
return false;
933+
}
934+
918935
static Trees getTreesInstance(VisitorState state) {
919936
return Trees.instance(JavacProcessingEnvironment.instance(state.context));
920937
}
@@ -1023,7 +1040,8 @@ public Description matchLambdaExpression(LambdaExpressionTree tree, VisitorState
10231040
funcInterfaceMethod,
10241041
tree,
10251042
null,
1026-
state);
1043+
state,
1044+
null);
10271045
if (description != Description.NO_MATCH) {
10281046
return description;
10291047
}
@@ -1117,7 +1135,12 @@ && getMethodReturnNullness(overridingMethod, state, Nullness.NONNULL)
11171135
// if any parameter in the super method is annotated @Nullable,
11181136
// overriding method cannot assume @Nonnull
11191137
return checkParamOverriding(
1120-
overridingMethod.getParameters(), overriddenMethod, null, memberReferenceTree, state);
1138+
overridingMethod.getParameters(),
1139+
overriddenMethod,
1140+
null,
1141+
memberReferenceTree,
1142+
state,
1143+
overridingMethod);
11211144
}
11221145

11231146
private boolean overriddenMethodReturnsNonNull(

‎nullaway/src/main/java/com/uber/nullaway/handlers/Handlers.java‎

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -46,12 +46,8 @@ public static Handler buildDefault(Config config) {
4646
ImmutableList.Builder<Handler> handlerListBuilder = ImmutableList.builder();
4747
MethodNameUtil methodNameUtil = new MethodNameUtil();
4848

49-
// Acknowledge restrictive annotations if in JSpecify mode or if the user has explicitly
50-
// requested it
51-
boolean acknowledgeRestrictive =
52-
config.acknowledgeRestrictiveAnnotations() || config.isJSpecifyMode();
5349
RestrictiveAnnotationHandler restrictiveAnnotationHandler = null;
54-
if (acknowledgeRestrictive) {
50+
if (config.acknowledgeRestrictiveAnnotations()) {
5551
// This runs before LibraryModelsHandler, so that library models can override third-party
5652
// bytecode annotations
5753
restrictiveAnnotationHandler = new RestrictiveAnnotationHandler(config);

‎nullaway/src/test/java/com/uber/nullaway/AcknowledgeRestrictiveAnnotationsTests.java‎

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -428,4 +428,62 @@ public void annotatedVsUnannotatedMethodRefOverrideWithRestrictiveAnnotations()
428428
"}")
429429
.doTest();
430430
}
431+
432+
@Test
433+
public void methodRefToNullUnmarked() {
434+
makeTestHelperWithArgs(
435+
Arrays.asList(
436+
"-d",
437+
temporaryFolder.getRoot().getAbsolutePath(),
438+
"-XepOpt:NullAway:AnnotatedPackages=com.uber",
439+
"-XepOpt:NullAway:AcknowledgeRestrictiveAnnotations=true"))
440+
.addSourceLines(
441+
"Test.java",
442+
"import org.jspecify.annotations.*;",
443+
"@NullMarked",
444+
"class Test {",
445+
" interface I {",
446+
" void m(@Nullable Object o);",
447+
" }",
448+
" @NullUnmarked",
449+
" void unmarkedParam(Object o) {}",
450+
" @NullUnmarked",
451+
" void nonNullParam(@NonNull Object o) {}",
452+
" void test() {",
453+
" I i = this::unmarkedParam;",
454+
" // BUG: Diagnostic contains: parameter o of referenced method is @NonNull",
455+
" I i2 = this::nonNullParam;",
456+
" }",
457+
"}")
458+
.doTest();
459+
}
460+
461+
@Test
462+
public void methodRefToNullUnmarkedVarargs() {
463+
makeTestHelperWithArgs(
464+
Arrays.asList(
465+
"-d",
466+
temporaryFolder.getRoot().getAbsolutePath(),
467+
"-XepOpt:NullAway:AnnotatedPackages=com.uber",
468+
"-XepOpt:NullAway:AcknowledgeRestrictiveAnnotations=true"))
469+
.addSourceLines(
470+
"Test.java",
471+
"import org.jspecify.annotations.*;",
472+
"@NullMarked",
473+
"class Test {",
474+
" interface I {",
475+
" void m(Object @Nullable ... o);",
476+
" }",
477+
" @NullUnmarked",
478+
" void unmarkedParam(Object... o) {}",
479+
" @NullUnmarked",
480+
" void nonNullParam(Object @NonNull ... o) {}",
481+
" void test() {",
482+
" I i = this::unmarkedParam;",
483+
" // BUG: Diagnostic contains: parameter o of referenced method is @NonNull",
484+
" I i2 = this::nonNullParam;",
485+
" }",
486+
"}")
487+
.doTest();
488+
}
431489
}

‎nullaway/src/test/java/com/uber/nullaway/jspecify/GenericsTests.java‎

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2353,6 +2353,31 @@ public void issue1156() {
23532353
.doTest();
23542354
}
23552355

2356+
@Test
2357+
public void methodReferenceToNullUnmarked() {
2358+
makeHelper()
2359+
.addSourceLines(
2360+
"Test.java",
2361+
"import org.jspecify.annotations.*;",
2362+
"import java.util.function.Function;",
2363+
"@NullMarked",
2364+
"public class Test {",
2365+
" @NullUnmarked",
2366+
" public static Boolean isNull(Object o) { return o == null; }",
2367+
" @NullUnmarked",
2368+
" public static Boolean isNullRestrictParam(@NonNull Object o) { return o == null; }",
2369+
" @NullUnmarked",
2370+
" public static @Nullable Boolean isNullRestrictReturn(Object o) { return o == null; }",
2371+
" static Function<@Nullable Object, Boolean> filter1 = Test::isNull;",
2372+
" // BUG: Diagnostic contains: parameter o of referenced method is @NonNull, but parameter in functional",
2373+
" static Function<@Nullable Object, Boolean> filter2 = Test::isNullRestrictParam;",
2374+
" // BUG: Diagnostic contains: referenced method returns @Nullable",
2375+
" static Function<@Nullable Object, Boolean> filter3 = Test::isNullRestrictReturn;",
2376+
" static Function<@Nullable Object, @Nullable Boolean> filter4 = Test::isNullRestrictReturn;",
2377+
"}")
2378+
.doTest();
2379+
}
2380+
23562381
@Test
23572382
public void varargsOfGenericType() {
23582383
makeHelper()

‎nullaway/src/test/java/com/uber/nullaway/jspecify/NullMarkednessTests.java‎

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1256,4 +1256,32 @@ public void writeToNullUnmarkedField() {
12561256
"}")
12571257
.doTest();
12581258
}
1259+
1260+
@Test
1261+
public void methodRefToNullUnmarkedNoAcknowledgeRestrictive() {
1262+
makeTestHelperWithArgs(
1263+
Arrays.asList(
1264+
"-d",
1265+
temporaryFolder.getRoot().getAbsolutePath(),
1266+
"-XepOpt:NullAway:AnnotatedPackages=com.uber"))
1267+
.addSourceLines(
1268+
"Test.java",
1269+
"import org.jspecify.annotations.*;",
1270+
"@NullMarked",
1271+
"class Test {",
1272+
" interface I {",
1273+
" void m(@Nullable Object o);",
1274+
" }",
1275+
" @NullUnmarked",
1276+
" void unmarkedParam(Object o) {}",
1277+
" @NullUnmarked",
1278+
" void nonNullParam(@NonNull Object o) {}",
1279+
" void test() {",
1280+
" I i = this::unmarkedParam;",
1281+
" // no error since we don't acknowledge restrictive annotations",
1282+
" I i2 = this::nonNullParam;",
1283+
" }",
1284+
"}")
1285+
.doTest();
1286+
}
12591287
}

0 commit comments

Comments
 (0)