Skip to content

Commit 6d331c7

Browse files
msridharmsridhar
authored
Proper checking of vararg overrides with JSpecify annotations (#1116)
Fixes #1113 We weren't calling the right API to check the nullability of a varargs argument when checking valid overriding. Most of this PR is tests to check handling of most of the cases.
1 parent 7cb6b98 commit 6d331c7

File tree

5 files changed

+248
-4
lines changed

5 files changed

+248
-4
lines changed

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

+9-2
Original file line numberDiff line numberDiff line change
@@ -780,9 +780,16 @@ private Description checkParamOverriding(
780780
// (otherwise, whether we acknowledge @Nullable in unannotated code or not depends on the
781781
// -XepOpt:NullAway:AcknowledgeRestrictiveAnnotations flag and its handler).
782782
if (isOverriddenMethodAnnotated) {
783+
boolean overriddenMethodIsVarArgs = overriddenMethod.isVarArgs();
783784
for (int i = 0; i < superParamSymbols.size(); i++) {
784785
Nullness paramNullness;
785-
if (Nullness.paramHasNullableAnnotation(overriddenMethod, i, config)) {
786+
if (overriddenMethodIsVarArgs && i == superParamSymbols.size() - 1) {
787+
// For a varargs position, we need to check if the array itself is @Nullable
788+
paramNullness =
789+
Nullness.varargsArrayIsNullable(superParamSymbols.get(i), config)
790+
? Nullness.NULLABLE
791+
: Nullness.NONNULL;
792+
} else if (Nullness.paramHasNullableAnnotation(overriddenMethod, i, config)) {
786793
paramNullness = Nullness.NULLABLE;
787794
} else if (config.isJSpecifyMode()) {
788795
// Check if the parameter type is a type variable and the corresponding generic type
@@ -840,7 +847,7 @@ private Description checkParamOverriding(
840847
for (int i = 0; i < superParamSymbols.size(); i++) {
841848
if (!Objects.equals(overriddenMethodArgNullnessMap[i], Nullness.NULLABLE)) {
842849
// No need to check, unless the argument of the overridden method is effectively @Nullable,
843-
// in which case it can't be overridding a @NonNull arg.
850+
// in which case it can't be overridden by a @NonNull arg.
844851
continue;
845852
}
846853
int methodParamInd = i - startParam;

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

+1-1
Original file line numberDiff line numberDiff line change
@@ -277,7 +277,7 @@ public static boolean paramHasNonNullAnnotation(
277277
}
278278

279279
/**
280-
* Is the varargs parameter {@code paramSymbol} have a {@code @Nullable} annotation indicating
280+
* Does the varargs parameter {@code paramSymbol} have a {@code @Nullable} annotation indicating
281281
* that the argument array passed at a call site can be {@code null}? Syntactically, this would be
282282
* written as {@code foo(Object @Nullable... args}}
283283
*/

‎nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java

-1
Original file line numberDiff line numberDiff line change
@@ -1030,7 +1030,6 @@ private static void checkTypeParameterNullnessForOverridingMethodParameterType(
10301030
MethodTree tree, Type overriddenMethodType, NullAway analysis, VisitorState state) {
10311031
List<? extends VariableTree> methodParameters = tree.getParameters();
10321032
List<Type> overriddenMethodParameterTypes = overriddenMethodType.getParameterTypes();
1033-
// TODO handle varargs; they are not handled for now
10341033
for (int i = 0; i < methodParameters.size(); i++) {
10351034
Type overridingMethodParameterType = getTreeType(methodParameters.get(i), state);
10361035
Type overriddenMethodParameterType = overriddenMethodParameterTypes.get(i);

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

+117
Original file line numberDiff line numberDiff line change
@@ -599,4 +599,121 @@ public void testVarargsRestrictiveBytecodes() {
599599
"}")
600600
.doTest();
601601
}
602+
603+
@Test
604+
public void varargsOverride() {
605+
defaultCompilationHelper
606+
.addSourceLines(
607+
"VarargsOverride.java",
608+
"package com.uber;",
609+
"import org.jspecify.annotations.Nullable;",
610+
"public class VarargsOverride {",
611+
" interface NullableVarargsContents {",
612+
" void varargs(@Nullable Object... params);",
613+
" }",
614+
" static class NullableVarargsContentsImpl1 implements NullableVarargsContents {",
615+
" @Override",
616+
" public void varargs(@Nullable Object... params) {",
617+
" }",
618+
" }",
619+
" static class NullableVarargsContentsImpl2 implements NullableVarargsContents {",
620+
" @Override",
621+
" public void varargs(Object... params) {",
622+
" }",
623+
" }",
624+
" static class NullableVarargsContentsImpl3 implements NullableVarargsContents {",
625+
" @Override",
626+
" public void varargs(Object @Nullable... params) {",
627+
" }",
628+
" }",
629+
" static class NullableVarargsContentsImpl4 implements NullableVarargsContents {",
630+
" @Override",
631+
" public void varargs(@Nullable Object @Nullable... params) {",
632+
" }",
633+
" }",
634+
" interface NullableVarargsArray {",
635+
" void varargs(Object @Nullable... params);",
636+
" }",
637+
" static class NullableVarargsArrayImpl1 implements NullableVarargsArray {",
638+
" @Override",
639+
" // BUG: Diagnostic contains: parameter params is @NonNull",
640+
" public void varargs(@Nullable Object... params) {",
641+
" }",
642+
" }",
643+
" static class NullableVarargsArrayImpl2 implements NullableVarargsArray {",
644+
" @Override",
645+
" // BUG: Diagnostic contains: parameter params is @NonNull",
646+
" public void varargs(Object... params) {",
647+
" }",
648+
" }",
649+
" static class NullableVarargsArrayImpl3 implements NullableVarargsArray {",
650+
" @Override",
651+
" public void varargs(Object @Nullable... params) {",
652+
" }",
653+
" }",
654+
" static class NullableVarargsArrayImpl4 implements NullableVarargsArray {",
655+
" @Override",
656+
" public void varargs(@Nullable Object @Nullable... params) {",
657+
" }",
658+
" }",
659+
" interface NullableVarargsBoth {",
660+
" void varargs(@Nullable Object @Nullable... params);",
661+
" }",
662+
" static class NullableVarargsBothImpl1 implements NullableVarargsBoth {",
663+
" @Override",
664+
" // BUG: Diagnostic contains: parameter params is @NonNull",
665+
" public void varargs(@Nullable Object... params) {",
666+
" }",
667+
" }",
668+
" static class NullableVarargsBothImpl2 implements NullableVarargsBoth {",
669+
" @Override",
670+
" // BUG: Diagnostic contains: parameter params is @NonNull",
671+
" public void varargs(Object... params) {",
672+
" }",
673+
" }",
674+
" static class NullableVarargsBothImpl3 implements NullableVarargsBoth {",
675+
" @Override",
676+
" public void varargs(Object @Nullable... params) {",
677+
" }",
678+
" }",
679+
" static class NullableVarargsBothImpl4 implements NullableVarargsBoth {",
680+
" @Override",
681+
" public void varargs(@Nullable Object @Nullable... params) {",
682+
" }",
683+
" }",
684+
" interface NonNullVarargs {",
685+
" void varargs(Object... params);",
686+
" }",
687+
" static class NonNullVarargsImpl1 implements NonNullVarargs {",
688+
" @Override",
689+
" public void varargs(@Nullable Object... params) {",
690+
" }",
691+
" }",
692+
" static class NonNullVarargsImpl2 implements NonNullVarargs {",
693+
" @Override",
694+
" public void varargs(Object... params) {",
695+
" }",
696+
" }",
697+
" static class NonNullVarargsImpl3 implements NonNullVarargs {",
698+
" @Override",
699+
" public void varargs(Object @Nullable... params) {",
700+
" }",
701+
" }",
702+
" static class NonNullVarargsImpl4 implements NonNullVarargs {",
703+
" @Override",
704+
" public void varargs(@Nullable Object @Nullable... params) {",
705+
" }",
706+
" }",
707+
" interface MultiArgNullableVarargsArray {",
708+
" void varargs(String s, Object @Nullable... params);",
709+
" }",
710+
" static class MultiArgNullableVarargsArrayImpl implements MultiArgNullableVarargsArray {",
711+
" @Override",
712+
" // BUG: Diagnostic contains: parameter params is @NonNull",
713+
" public void varargs(String s, Object... params) {",
714+
" }",
715+
" }",
716+
"}")
717+
.doTest();
718+
}
602719
}

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

+121
Original file line numberDiff line numberDiff line change
@@ -516,6 +516,127 @@ public void testVarargsRestrictive() {
516516
.doTest();
517517
}
518518

519+
@Test
520+
public void varargsOverride() {
521+
makeHelper()
522+
.addSourceLines(
523+
"VarargsOverride.java",
524+
"package com.uber;",
525+
"import org.jspecify.annotations.Nullable;",
526+
"public class VarargsOverride {",
527+
" interface NullableVarargsContents {",
528+
" void varargs(@Nullable Object... params);",
529+
" }",
530+
" static class NullableVarargsContentsImpl1 implements NullableVarargsContents {",
531+
" @Override",
532+
" // legal override",
533+
" public void varargs(@Nullable Object... params) {",
534+
" }",
535+
" }",
536+
" static class NullableVarargsContentsImpl2 implements NullableVarargsContents {",
537+
" @Override",
538+
" // BUG: Diagnostic contains: Parameter has type Object[], but overridden method",
539+
" public void varargs(Object... params) {",
540+
" }",
541+
" }",
542+
" static class NullableVarargsContentsImpl3 implements NullableVarargsContents {",
543+
" @Override",
544+
// TODO open an issue to improve the error message in a follow up
545+
" // BUG: Diagnostic contains: Parameter has type Object[]",
546+
" public void varargs(Object @Nullable... params) {",
547+
" }",
548+
" }",
549+
" static class NullableVarargsContentsImpl4 implements NullableVarargsContents {",
550+
" @Override",
551+
" // legal override",
552+
" public void varargs(@Nullable Object @Nullable... params) {",
553+
" }",
554+
" }",
555+
" interface NullableVarargsArray {",
556+
" void varargs(Object @Nullable... params);",
557+
" }",
558+
" static class NullableVarargsArrayImpl1 implements NullableVarargsArray {",
559+
" @Override",
560+
" // BUG: Diagnostic contains: parameter params is @NonNull",
561+
" public void varargs(@Nullable Object... params) {",
562+
" }",
563+
" }",
564+
" static class NullableVarargsArrayImpl2 implements NullableVarargsArray {",
565+
" @Override",
566+
" // BUG: Diagnostic contains: parameter params is @NonNull",
567+
" public void varargs(Object... params) {",
568+
" }",
569+
" }",
570+
" static class NullableVarargsArrayImpl3 implements NullableVarargsArray {",
571+
" @Override",
572+
" // legal override",
573+
" public void varargs(Object @Nullable... params) {",
574+
" }",
575+
" }",
576+
" static class NullableVarargsArrayImpl4 implements NullableVarargsArray {",
577+
" @Override",
578+
" // ok: contravariance",
579+
" public void varargs(@Nullable Object @Nullable... params) {",
580+
" }",
581+
" }",
582+
" interface NullableVarargsBoth {",
583+
" void varargs(@Nullable Object @Nullable... params);",
584+
" }",
585+
" static class NullableVarargsBothImpl1 implements NullableVarargsBoth {",
586+
" @Override",
587+
" // BUG: Diagnostic contains: parameter params is @NonNull",
588+
" public void varargs(@Nullable Object... params) {",
589+
" }",
590+
" }",
591+
" static class NullableVarargsBothImpl2 implements NullableVarargsBoth {",
592+
" @Override",
593+
" // BUG: Diagnostic contains: parameter params is @NonNull",
594+
" public void varargs(Object... params) {",
595+
" }",
596+
" }",
597+
" static class NullableVarargsBothImpl3 implements NullableVarargsBoth {",
598+
" @Override",
599+
" // BUG: Diagnostic contains: Parameter has type Object[]",
600+
" public void varargs(Object @Nullable... params) {",
601+
" }",
602+
" }",
603+
" static class NullableVarargsBothImpl4 implements NullableVarargsBoth {",
604+
" @Override",
605+
" // legal override",
606+
" public void varargs(@Nullable Object @Nullable... params) {",
607+
" }",
608+
" }",
609+
" interface NonNullVarargs {",
610+
" void varargs(Object... params);",
611+
" }",
612+
" static class NonNullVarargsImpl1 implements NonNullVarargs {",
613+
" @Override",
614+
" // ok: contravariance",
615+
" public void varargs(@Nullable Object... params) {",
616+
" }",
617+
" }",
618+
" static class NonNullVarargsImpl2 implements NonNullVarargs {",
619+
" @Override",
620+
" // legal override",
621+
" public void varargs(Object... params) {",
622+
" }",
623+
" }",
624+
" static class NonNullVarargsImpl3 implements NonNullVarargs {",
625+
" @Override",
626+
" // legal override",
627+
" public void varargs(Object @Nullable... params) {",
628+
" }",
629+
" }",
630+
" static class NonNullVarargsImpl4 implements NonNullVarargs {",
631+
" @Override",
632+
" // ok: contravariance",
633+
" public void varargs(@Nullable Object @Nullable... params) {",
634+
" }",
635+
" }",
636+
"}")
637+
.doTest();
638+
}
639+
519640
private CompilationTestHelper makeHelper() {
520641
return makeTestHelperWithArgs(
521642
Arrays.asList(

0 commit comments

Comments
 (0)