From bcf56aa5b0c42e8aaab60fc540f5463f623a96f4 Mon Sep 17 00:00:00 2001 From: Manu Sridharan Date: Tue, 24 Dec 2024 21:42:49 -0500 Subject: [PATCH 1/5] test case --- .../jspecify/JSpecifyVarargsTests.java | 20 +++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/nullaway/src/test/java/com/uber/nullaway/jspecify/JSpecifyVarargsTests.java b/nullaway/src/test/java/com/uber/nullaway/jspecify/JSpecifyVarargsTests.java index a72124ffa3..caecf6e557 100644 --- a/nullaway/src/test/java/com/uber/nullaway/jspecify/JSpecifyVarargsTests.java +++ b/nullaway/src/test/java/com/uber/nullaway/jspecify/JSpecifyVarargsTests.java @@ -516,6 +516,26 @@ public void testVarargsRestrictive() { .doTest(); } + @Test + public void varargsOverride() { + makeHelper() + .addSourceLines( + "VarargsOverride.java", + "package com.uber;", + "import org.jspecify.annotations.Nullable;", + "public class VarargsOverride {", + " interface I {", + " void varargs(@Nullable Object... params);", + " }", + " static class C implements I {", + " @Override", + " public void varargs(@Nullable Object... params) {", + " }", + " }", + "}") + .doTest(); + } + private CompilationTestHelper makeHelper() { return makeTestHelperWithArgs( Arrays.asList( From 1895f510a3113e5df5da175b4fc80ab8e37fc858 Mon Sep 17 00:00:00 2001 From: Manu Sridharan Date: Tue, 24 Dec 2024 22:23:31 -0500 Subject: [PATCH 2/5] more --- .../main/java/com/uber/nullaway/NullAway.java | 14 ++- .../main/java/com/uber/nullaway/Nullness.java | 2 +- .../jspecify/JSpecifyVarargsTests.java | 104 +++++++++++++++++- 3 files changed, 115 insertions(+), 5 deletions(-) diff --git a/nullaway/src/main/java/com/uber/nullaway/NullAway.java b/nullaway/src/main/java/com/uber/nullaway/NullAway.java index 8611cade8c..5f3e6a7f1a 100644 --- a/nullaway/src/main/java/com/uber/nullaway/NullAway.java +++ b/nullaway/src/main/java/com/uber/nullaway/NullAway.java @@ -780,9 +780,19 @@ private Description checkParamOverriding( // (otherwise, whether we acknowledge @Nullable in unannotated code or not depends on the // -XepOpt:NullAway:AcknowledgeRestrictiveAnnotations flag and its handler). if (isOverriddenMethodAnnotated) { + boolean overridenMethodIsVarArgs = overriddenMethod.isVarArgs(); for (int i = 0; i < superParamSymbols.size(); i++) { Nullness paramNullness; - if (Nullness.paramHasNullableAnnotation(overriddenMethod, i, config)) { + if (overridenMethodIsVarArgs && i == superParamSymbols.size() - 1) { + // TODO for override checking, for a varargs position, we actually want to check if the + // array _itself_ is @Nullable, which this API does not do. Be careful to try to + // preserve + // behavior in LegacyAnnotationsMode + paramNullness = + Nullness.varargsArrayIsNullable(superParamSymbols.get(i), config) + ? Nullness.NULLABLE + : Nullness.NONNULL; + } else if (Nullness.paramHasNullableAnnotation(overriddenMethod, i, config)) { paramNullness = Nullness.NULLABLE; } else if (config.isJSpecifyMode()) { // Check if the parameter type is a type variable and the corresponding generic type @@ -840,7 +850,7 @@ private Description checkParamOverriding( for (int i = 0; i < superParamSymbols.size(); i++) { if (!Objects.equals(overriddenMethodArgNullnessMap[i], Nullness.NULLABLE)) { // No need to check, unless the argument of the overridden method is effectively @Nullable, - // in which case it can't be overridding a @NonNull arg. + // in which case it can't be overridden by a @NonNull arg. continue; } int methodParamInd = i - startParam; diff --git a/nullaway/src/main/java/com/uber/nullaway/Nullness.java b/nullaway/src/main/java/com/uber/nullaway/Nullness.java index c86520c5a4..158c8e33a9 100644 --- a/nullaway/src/main/java/com/uber/nullaway/Nullness.java +++ b/nullaway/src/main/java/com/uber/nullaway/Nullness.java @@ -277,7 +277,7 @@ public static boolean paramHasNonNullAnnotation( } /** - * Is the varargs parameter {@code paramSymbol} have a {@code @Nullable} annotation indicating + * Does the varargs parameter {@code paramSymbol} have a {@code @Nullable} annotation indicating * that the argument array passed at a call site can be {@code null}? Syntactically, this would be * written as {@code foo(Object @Nullable... args}} */ diff --git a/nullaway/src/test/java/com/uber/nullaway/jspecify/JSpecifyVarargsTests.java b/nullaway/src/test/java/com/uber/nullaway/jspecify/JSpecifyVarargsTests.java index caecf6e557..b071d2688c 100644 --- a/nullaway/src/test/java/com/uber/nullaway/jspecify/JSpecifyVarargsTests.java +++ b/nullaway/src/test/java/com/uber/nullaway/jspecify/JSpecifyVarargsTests.java @@ -524,14 +524,114 @@ public void varargsOverride() { "package com.uber;", "import org.jspecify.annotations.Nullable;", "public class VarargsOverride {", - " interface I {", + " interface NullableVarargsContents {", " void varargs(@Nullable Object... params);", " }", - " static class C implements I {", + " static class NullableVarargsContentsImpl1 implements NullableVarargsContents {", " @Override", + " // legal override", " public void varargs(@Nullable Object... params) {", " }", " }", + " static class NullableVarargsContentsImpl2 implements NullableVarargsContents {", + " @Override", + " // BUG: Diagnostic contains: Parameter has type Object[], but overridden method", + " public void varargs(Object... params) {", + " }", + " }", + " static class NullableVarargsContentsImpl3 implements NullableVarargsContents {", + " @Override", + " // BUG: Diagnostic contains: Parameter has type Object @Nullable []", + " public void varargs(Object @Nullable... params) {", + " }", + " }", + " static class NullableVarargsContentsImpl4 implements NullableVarargsContents {", + " @Override", + " // legal override", + " public void varargs(@Nullable Object @Nullable... params) {", + " }", + " }", + " interface NullableVarargsArray {", + " void varargs(Object @Nullable... params);", + " }", + " static class NullableVarargsArrayImpl1 implements NullableVarargsArray {", + " @Override", + " // BUG: Diagnostic contains: ...", + " public void varargs(@Nullable Object... params) {", + " }", + " }", + " static class NullableVarargsArrayImpl2 implements NullableVarargsArray {", + " @Override", + " // BUG: Diagnostic contains: ...", + " public void varargs(Object... params) {", + " }", + " }", + " static class NullableVarargsArrayImpl3 implements NullableVarargsArray {", + " @Override", + " // legal override", + " public void varargs(Object @Nullable... params) {", + " }", + " }", + " static class NullableVarargsArrayImpl4 implements NullableVarargsArray {", + " @Override", + " // BUG: Diagnostic contains: ...", + " public void varargs(@Nullable Object @Nullable... params) {", + " }", + " }", + " interface NullableVarargsBoth {", + " void varargs(@Nullable Object @Nullable... params);", + " }", + " static class NullableVarargsBothImpl1 implements NullableVarargsBoth {", + " @Override", + " // BUG: Diagnostic contains: ...", + " public void varargs(@Nullable Object... params) {", + " }", + " }", + " static class NullableVarargsBothImpl2 implements NullableVarargsBoth {", + " @Override", + " // BUG: Diagnostic contains: ...", + " public void varargs(Object... params) {", + " }", + " }", + " static class NullableVarargsBothImpl3 implements NullableVarargsBoth {", + " @Override", + " // BUG: Diagnostic contains: ...", + " public void varargs(Object @Nullable... params) {", + " }", + " }", + " static class NullableVarargsBothImpl4 implements NullableVarargsBoth {", + " @Override", + " // legal override", + " public void varargs(@Nullable Object @Nullable... params) {", + " }", + " }", + " interface NonNullVarargs {", + " void varargs(Object... params);", + " }", + " static class NonNullVarargsImpl1 implements NonNullVarargs {", + " @Override", + " // BUG: Diagnostic contains: ...", + " public void varargs(@Nullable Object... params) {", + " }", + " }", + " static class NonNullVarargsImpl2 implements NonNullVarargs {", + " @Override", + " // legal override", + " public void varargs(Object... params) {", + " }", + " }", + " static class NonNullVarargsImpl3 implements NonNullVarargs {", + " @Override", + " // legal override", + " public void varargs(Object @Nullable... params) {", + " }", + " }", + " static class NonNullVarargsImpl4 implements NonNullVarargs {", + " @Override", + " // BUG: Diagnostic contains: ...", + " public void varargs(@Nullable Object @Nullable... params) {", + " }", + " }", "}") .doTest(); } From 1df07bfeae4632f5d310a726f81b63931aac3412 Mon Sep 17 00:00:00 2001 From: Manu Sridharan Date: Wed, 25 Dec 2024 09:19:48 -0500 Subject: [PATCH 3/5] more testing --- .../nullaway/generics/GenericsChecks.java | 1 - .../java/com/uber/nullaway/VarargsTests.java | 108 ++++++++++++++++++ .../jspecify/JSpecifyVarargsTests.java | 19 +-- 3 files changed, 118 insertions(+), 10 deletions(-) diff --git a/nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java b/nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java index 85ca6861bb..76c1afd364 100644 --- a/nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java +++ b/nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java @@ -1026,7 +1026,6 @@ private static void checkTypeParameterNullnessForOverridingMethodParameterType( MethodTree tree, Type overriddenMethodType, NullAway analysis, VisitorState state) { List methodParameters = tree.getParameters(); List overriddenMethodParameterTypes = overriddenMethodType.getParameterTypes(); - // TODO handle varargs; they are not handled for now for (int i = 0; i < methodParameters.size(); i++) { Type overridingMethodParameterType = getTreeType(methodParameters.get(i), state); Type overriddenMethodParameterType = overriddenMethodParameterTypes.get(i); diff --git a/nullaway/src/test/java/com/uber/nullaway/VarargsTests.java b/nullaway/src/test/java/com/uber/nullaway/VarargsTests.java index cfee867589..dfb2f9a343 100644 --- a/nullaway/src/test/java/com/uber/nullaway/VarargsTests.java +++ b/nullaway/src/test/java/com/uber/nullaway/VarargsTests.java @@ -599,4 +599,112 @@ public void testVarargsRestrictiveBytecodes() { "}") .doTest(); } + + @Test + public void varargsOverride() { + defaultCompilationHelper + .addSourceLines( + "VarargsOverride.java", + "package com.uber;", + "import org.jspecify.annotations.Nullable;", + "public class VarargsOverride {", + " interface NullableVarargsContents {", + " void varargs(@Nullable Object... params);", + " }", + " static class NullableVarargsContentsImpl1 implements NullableVarargsContents {", + " @Override", + " public void varargs(@Nullable Object... params) {", + " }", + " }", + " static class NullableVarargsContentsImpl2 implements NullableVarargsContents {", + " @Override", + " public void varargs(Object... params) {", + " }", + " }", + " static class NullableVarargsContentsImpl3 implements NullableVarargsContents {", + " @Override", + " public void varargs(Object @Nullable... params) {", + " }", + " }", + " static class NullableVarargsContentsImpl4 implements NullableVarargsContents {", + " @Override", + " public void varargs(@Nullable Object @Nullable... params) {", + " }", + " }", + " interface NullableVarargsArray {", + " void varargs(Object @Nullable... params);", + " }", + " static class NullableVarargsArrayImpl1 implements NullableVarargsArray {", + " @Override", + " // BUG: Diagnostic contains: parameter params is @NonNull", + " public void varargs(@Nullable Object... params) {", + " }", + " }", + " static class NullableVarargsArrayImpl2 implements NullableVarargsArray {", + " @Override", + " // BUG: Diagnostic contains: parameter params is @NonNull", + " public void varargs(Object... params) {", + " }", + " }", + " static class NullableVarargsArrayImpl3 implements NullableVarargsArray {", + " @Override", + " public void varargs(Object @Nullable... params) {", + " }", + " }", + " static class NullableVarargsArrayImpl4 implements NullableVarargsArray {", + " @Override", + " public void varargs(@Nullable Object @Nullable... params) {", + " }", + " }", + " interface NullableVarargsBoth {", + " void varargs(@Nullable Object @Nullable... params);", + " }", + " static class NullableVarargsBothImpl1 implements NullableVarargsBoth {", + " @Override", + " // BUG: Diagnostic contains: parameter params is @NonNull", + " public void varargs(@Nullable Object... params) {", + " }", + " }", + " static class NullableVarargsBothImpl2 implements NullableVarargsBoth {", + " @Override", + " // BUG: Diagnostic contains: parameter params is @NonNull", + " public void varargs(Object... params) {", + " }", + " }", + " static class NullableVarargsBothImpl3 implements NullableVarargsBoth {", + " @Override", + " public void varargs(Object @Nullable... params) {", + " }", + " }", + " static class NullableVarargsBothImpl4 implements NullableVarargsBoth {", + " @Override", + " public void varargs(@Nullable Object @Nullable... params) {", + " }", + " }", + " interface NonNullVarargs {", + " void varargs(Object... params);", + " }", + " static class NonNullVarargsImpl1 implements NonNullVarargs {", + " @Override", + " public void varargs(@Nullable Object... params) {", + " }", + " }", + " static class NonNullVarargsImpl2 implements NonNullVarargs {", + " @Override", + " public void varargs(Object... params) {", + " }", + " }", + " static class NonNullVarargsImpl3 implements NonNullVarargs {", + " @Override", + " public void varargs(Object @Nullable... params) {", + " }", + " }", + " static class NonNullVarargsImpl4 implements NonNullVarargs {", + " @Override", + " public void varargs(@Nullable Object @Nullable... params) {", + " }", + " }", + "}") + .doTest(); + } } diff --git a/nullaway/src/test/java/com/uber/nullaway/jspecify/JSpecifyVarargsTests.java b/nullaway/src/test/java/com/uber/nullaway/jspecify/JSpecifyVarargsTests.java index b071d2688c..b08feb77e7 100644 --- a/nullaway/src/test/java/com/uber/nullaway/jspecify/JSpecifyVarargsTests.java +++ b/nullaway/src/test/java/com/uber/nullaway/jspecify/JSpecifyVarargsTests.java @@ -541,7 +541,8 @@ public void varargsOverride() { " }", " static class NullableVarargsContentsImpl3 implements NullableVarargsContents {", " @Override", - " // BUG: Diagnostic contains: Parameter has type Object @Nullable []", + // TODO open an issue to improve the error message in a follow up + " // BUG: Diagnostic contains: Parameter has type Object[]", " public void varargs(Object @Nullable... params) {", " }", " }", @@ -556,13 +557,13 @@ public void varargsOverride() { " }", " static class NullableVarargsArrayImpl1 implements NullableVarargsArray {", " @Override", - " // BUG: Diagnostic contains: ...", + " // BUG: Diagnostic contains: parameter params is @NonNull", " public void varargs(@Nullable Object... params) {", " }", " }", " static class NullableVarargsArrayImpl2 implements NullableVarargsArray {", " @Override", - " // BUG: Diagnostic contains: ...", + " // BUG: Diagnostic contains: parameter params is @NonNull", " public void varargs(Object... params) {", " }", " }", @@ -574,7 +575,7 @@ public void varargsOverride() { " }", " static class NullableVarargsArrayImpl4 implements NullableVarargsArray {", " @Override", - " // BUG: Diagnostic contains: ...", + " // ok: contravariance", " public void varargs(@Nullable Object @Nullable... params) {", " }", " }", @@ -583,19 +584,19 @@ public void varargsOverride() { " }", " static class NullableVarargsBothImpl1 implements NullableVarargsBoth {", " @Override", - " // BUG: Diagnostic contains: ...", + " // BUG: Diagnostic contains: parameter params is @NonNull", " public void varargs(@Nullable Object... params) {", " }", " }", " static class NullableVarargsBothImpl2 implements NullableVarargsBoth {", " @Override", - " // BUG: Diagnostic contains: ...", + " // BUG: Diagnostic contains: parameter params is @NonNull", " public void varargs(Object... params) {", " }", " }", " static class NullableVarargsBothImpl3 implements NullableVarargsBoth {", " @Override", - " // BUG: Diagnostic contains: ...", + " // BUG: Diagnostic contains: Parameter has type Object[]", " public void varargs(Object @Nullable... params) {", " }", " }", @@ -610,7 +611,7 @@ public void varargsOverride() { " }", " static class NonNullVarargsImpl1 implements NonNullVarargs {", " @Override", - " // BUG: Diagnostic contains: ...", + " // ok: contravariance", " public void varargs(@Nullable Object... params) {", " }", " }", @@ -628,7 +629,7 @@ public void varargsOverride() { " }", " static class NonNullVarargsImpl4 implements NonNullVarargs {", " @Override", - " // BUG: Diagnostic contains: ...", + " // ok: contravariance", " public void varargs(@Nullable Object @Nullable... params) {", " }", " }", From d55f567ec2b6ea5728aa5f63094d8670b8426f08 Mon Sep 17 00:00:00 2001 From: Manu Sridharan Date: Wed, 25 Dec 2024 09:25:01 -0500 Subject: [PATCH 4/5] fix comment --- nullaway/src/main/java/com/uber/nullaway/NullAway.java | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/nullaway/src/main/java/com/uber/nullaway/NullAway.java b/nullaway/src/main/java/com/uber/nullaway/NullAway.java index 5f3e6a7f1a..4dd4a4ab55 100644 --- a/nullaway/src/main/java/com/uber/nullaway/NullAway.java +++ b/nullaway/src/main/java/com/uber/nullaway/NullAway.java @@ -784,10 +784,7 @@ private Description checkParamOverriding( for (int i = 0; i < superParamSymbols.size(); i++) { Nullness paramNullness; if (overridenMethodIsVarArgs && i == superParamSymbols.size() - 1) { - // TODO for override checking, for a varargs position, we actually want to check if the - // array _itself_ is @Nullable, which this API does not do. Be careful to try to - // preserve - // behavior in LegacyAnnotationsMode + // For a varargs position, we need to check if the array itself is @Nullable paramNullness = Nullness.varargsArrayIsNullable(superParamSymbols.get(i), config) ? Nullness.NULLABLE From ed1add065f119f2fdcc1240d490d0ed2e3ee8039 Mon Sep 17 00:00:00 2001 From: Manu Sridharan Date: Wed, 25 Dec 2024 10:57:55 -0700 Subject: [PATCH 5/5] improve test coverage --- nullaway/src/main/java/com/uber/nullaway/NullAway.java | 4 ++-- .../src/test/java/com/uber/nullaway/VarargsTests.java | 9 +++++++++ 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/nullaway/src/main/java/com/uber/nullaway/NullAway.java b/nullaway/src/main/java/com/uber/nullaway/NullAway.java index 4dd4a4ab55..b9d42df44c 100644 --- a/nullaway/src/main/java/com/uber/nullaway/NullAway.java +++ b/nullaway/src/main/java/com/uber/nullaway/NullAway.java @@ -780,10 +780,10 @@ private Description checkParamOverriding( // (otherwise, whether we acknowledge @Nullable in unannotated code or not depends on the // -XepOpt:NullAway:AcknowledgeRestrictiveAnnotations flag and its handler). if (isOverriddenMethodAnnotated) { - boolean overridenMethodIsVarArgs = overriddenMethod.isVarArgs(); + boolean overriddenMethodIsVarArgs = overriddenMethod.isVarArgs(); for (int i = 0; i < superParamSymbols.size(); i++) { Nullness paramNullness; - if (overridenMethodIsVarArgs && i == superParamSymbols.size() - 1) { + if (overriddenMethodIsVarArgs && i == superParamSymbols.size() - 1) { // For a varargs position, we need to check if the array itself is @Nullable paramNullness = Nullness.varargsArrayIsNullable(superParamSymbols.get(i), config) diff --git a/nullaway/src/test/java/com/uber/nullaway/VarargsTests.java b/nullaway/src/test/java/com/uber/nullaway/VarargsTests.java index dfb2f9a343..dba43ef1a5 100644 --- a/nullaway/src/test/java/com/uber/nullaway/VarargsTests.java +++ b/nullaway/src/test/java/com/uber/nullaway/VarargsTests.java @@ -704,6 +704,15 @@ public void varargsOverride() { " public void varargs(@Nullable Object @Nullable... params) {", " }", " }", + " interface MultiArgNullableVarargsArray {", + " void varargs(String s, Object @Nullable... params);", + " }", + " static class MultiArgNullableVarargsArrayImpl implements MultiArgNullableVarargsArray {", + " @Override", + " // BUG: Diagnostic contains: parameter params is @NonNull", + " public void varargs(String s, Object... params) {", + " }", + " }", "}") .doTest(); }