Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Proper checking of vararg overrides with JSpecify annotations #1116

Merged
merged 6 commits into from
Dec 26, 2024
+248 −4
Merged

Proper checking of vararg overrides with JSpecify annotations #1116

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
bcf56aa
test case
msridhar Dec 25, 2024
1895f51
more
msridhar Dec 25, 2024
1df07bf
more testing
msridhar Dec 25, 2024
d55f567
fix comment
msridhar Dec 25, 2024
ed1add0
improve test coverage
msridhar Dec 25, 2024
53aeb1b
Merge branch 'master' into issue-1113
msridhar Dec 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 9 additions & 2 deletions nullaway/src/main/java/com/uber/nullaway/NullAway.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -780,9 +780,16 @@ 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 overriddenMethodIsVarArgs = overriddenMethod.isVarArgs();
for (int i = 0; i < superParamSymbols.size(); i++) {
Nullness paramNullness;
if (Nullness.paramHasNullableAnnotation(overriddenMethod, i, config)) {
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)
? 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
Expand Down Expand Up @@ -840,7 +847,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;
Expand Down
2 changes: 1 addition & 1 deletion nullaway/src/main/java/com/uber/nullaway/Nullness.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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}}
*/
Expand Down
1 change: 0 additions & 1 deletion nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1030,7 +1030,6 @@ private static void checkTypeParameterNullnessForOverridingMethodParameterType(
MethodTree tree, Type overriddenMethodType, NullAway analysis, VisitorState state) {
List<? extends VariableTree> methodParameters = tree.getParameters();
List<Type> 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);
Expand Down
117 changes: 117 additions & 0 deletions nullaway/src/test/java/com/uber/nullaway/VarargsTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -599,4 +599,121 @@ 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) {",
" }",
" }",
" 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();
}
}
121 changes: 121 additions & 0 deletions nullaway/src/test/java/com/uber/nullaway/jspecify/JSpecifyVarargsTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -516,6 +516,127 @@ public void testVarargsRestrictive() {
.doTest();
}

@Test
public void varargsOverride() {
makeHelper()
.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",
" // 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",
// 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) {",
" }",
" }",
" 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: 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",
" // legal override",
" public void varargs(Object @Nullable... params) {",
" }",
" }",
" static class NullableVarargsArrayImpl4 implements NullableVarargsArray {",
" @Override",
" // ok: contravariance",
" 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",
" // BUG: Diagnostic contains: Parameter has type Object[]",
" 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",
" // ok: contravariance",
" 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",
" // ok: contravariance",
" public void varargs(@Nullable Object @Nullable... params) {",
" }",
" }",
"}")
.doTest();
}

private CompilationTestHelper makeHelper() {
return makeTestHelperWithArgs(
Arrays.asList(
Expand Down
Loading