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

Jump to bottom

Partial handling for restrictive annotations on varargs in unannotated code #1029

Merged
merged 13 commits into from
Sep 7, 2024
Merged

Partial handling for restrictive annotations on varargs in unannotated code #1029

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
e0be617
Properly handle varargs array parameter for unannotated methods
msridhar Sep 2, 2024
2309531
more tests
msridhar Sep 2, 2024
fcfeb94
more
msridhar Sep 2, 2024
14ae632
add TODO
msridhar Sep 2, 2024
18a7002
fix comment
msridhar Sep 2, 2024
c644768
comment tweaks
msridhar Sep 2, 2024
e5d7d1b
more comment fixes
msridhar Sep 2, 2024
4f07f82
more tests
msridhar Sep 2, 2024
e13dc58
fixes for legacy mode
msridhar Sep 3, 2024
e312e44
Merge branch 'master' into varargs-unannotated
msridhar Sep 6, 2024
32e9aa2
add test
msridhar Sep 7, 2024
7330a20
fully restore workarounds for JetBrains @NotNull from kotlinc
msridhar Sep 7, 2024
118dc17
more tests
msridhar Sep 7, 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
13 changes: 10 additions & 3 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 @@ -1788,9 +1788,16 @@ private Description handleInvocation(
&& actualParams.size() == argPos + 1) {
// This is the case where an array is explicitly passed in the position of the var args
// parameter
// If varargs array itself is not @Nullable, cannot pass @Nullable array
if (!Nullness.varargsArrayIsNullable(formalParams.get(argPos), config)) {
mayActualBeNull = mayBeNullExpr(state, actual);
// Only check for a nullable varargs array if the method is annotated or a @NonNull
// restrictive annotation is present in legacy mode (as previously the annotation was
// applied to both the array itself and the elements)
boolean checkForNullableVarargsArray =
isMethodAnnotated || (config.isLegacyAnnotationLocation() && argIsNonNull);
if (checkForNullableVarargsArray) {
// If varargs array itself is not @Nullable, cannot pass @Nullable array
if (!Nullness.varargsArrayIsNullable(formalParams.get(argPos), config)) {
mayActualBeNull = mayBeNullExpr(state, actual);
}
}
} else {
// This is the case were varargs are being passed individually, as 1 or more actual
Expand Down
26 changes: 26 additions & 0 deletions nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -440,4 +440,30 @@
}
return false;
}

/**
* Checks if the given array symbol has a {@code @NonNull} annotation for its elements.
*
* @param arraySymbol The symbol of the array to check.
* @param config NullAway configuration.
* @return true if the array symbol has a {@code @NonNull} annotation for its elements, false
* otherwise
*/
public static boolean isArrayElementNonNull(Symbol arraySymbol, Config config) {
for (Attribute.TypeCompound t : arraySymbol.getRawTypeAttributes()) {
for (TypeAnnotationPosition.TypePathEntry entry : t.position.location) {
if (entry.tag == TypeAnnotationPosition.TypePathEntryKind.ARRAY) {
if (Nullness.isNonNullAnnotation(t.type.toString(), config)) {
return true;
}
}
}

Check warning on line 460 in nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java

View check run for this annotation

Codecov / codecov/patch

nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java#L460 

Added line #L460 was not covered by tests
}
// For varargs symbols we also consider the elements to be @NonNull if there is a @NonNull
// declaration annotation on the parameter
if ((arraySymbol.flags() & Flags.VARARGS) != 0) {
return Nullness.hasNonNullDeclarationAnnotation(arraySymbol, config);
}
return false;

Check warning on line 467 in nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java

View check run for this annotation

Codecov / codecov/patch

nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java#L467 

Added line #L467 was not covered by tests
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is codecov correct here that we never test the case where this method returns false?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is correct. We should get coverage of this line when we add logic to process restrictive @NonNull annotations on array elements in positions other than the varargs position, which we will do in a follow up.

}
}
22 changes: 19 additions & 3 deletions 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 @@ -175,7 +175,7 @@ public static boolean isNullableAnnotation(String annotName, Config config) {
* @param annotName annotation name
* @return true if we treat annotName as a <code>@NonNull</code> annotation, false otherwise
*/
private static boolean isNonNullAnnotation(String annotName, Config config) {
public static boolean isNonNullAnnotation(String annotName, Config config) {
return annotName.endsWith(".NonNull")
|| annotName.endsWith(".NotNull")
|| annotName.endsWith(".Nonnull")
Expand Down Expand Up @@ -260,11 +260,22 @@ private static boolean isRecordEqualsParam(Symbol.MethodSymbol symbol, int param
/**
* Does the parameter of {@code symbol} at {@code paramInd} have a {@code @NonNull} declaration or
* type-use annotation? This method works for methods defined in either source or class files.
*
* <p>For a varargs parameter, this method returns true if <em>individual</em> arguments passed in
* the varargs positions must be {@code @NonNull}.
*/
public static boolean paramHasNonNullAnnotation(
Symbol.MethodSymbol symbol, int paramInd, Config config) {
return hasNonNullAnnotation(
NullabilityUtil.getAllAnnotationsForParameter(symbol, paramInd, config), config);
if (symbol.isVarArgs()
&& paramInd == symbol.getParameters().size() - 1
&& !config.isLegacyAnnotationLocation()) {
// individual arguments passed in the varargs positions must be @NonNull if the array element
// type of the parameter is @NonNull
return NullabilityUtil.isArrayElementNonNull(symbol.getParameters().get(paramInd), config);
} else {
return hasNonNullAnnotation(
NullabilityUtil.getAllAnnotationsForParameter(symbol, paramInd, config), config);
}
}

/**
Expand All @@ -282,4 +293,9 @@ public static boolean varargsArrayIsNullable(Symbol paramSymbol, Config config)
public static boolean hasNullableDeclarationAnnotation(Symbol symbol, Config config) {
return hasNullableAnnotation(symbol.getRawAttributes().stream(), config);
}

/** Checks if the symbol has a {@code @NonNull} declaration annotation */
public static boolean hasNonNullDeclarationAnnotation(Symbol symbol, Config config) {
return hasNonNullAnnotation(symbol.getRawAttributes().stream(), config);
}
}
82 changes: 81 additions & 1 deletion nullaway/src/test/java/com/uber/nullaway/LegacyVarargsTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,7 +200,8 @@ public void testSkipJetbrainsNotNullOnVarArgsFromThirdPartyJars() {
" ThirdParty.takesNullableVarargs(o1);", // Empty var args passed
" ThirdParty.takesNullableVarargs(o1, o4);",
" ThirdParty.takesNullableVarargs(o1, (Object) null);",
" // BUG: Diagnostic contains: passing @Nullable parameter '(Object[]) null' where @NonNull",
" // No error: we require a type-use annotation for nullability of the varargs array itself",
// TODO should we preserve the old behavior in legacy mode?
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lazaroclapp with this change, we no longer detect an error when passing a null array in the varargs position to a third-party method with a JetBrains @NotNull annotation on the varargs parameter. On the one hand, getting an error here is the "desired" behavior, in that the annotation is supposed to apply to the array itself (see #720). But, everywhere else, a declaration annotation on the varargs argument is treated as applying to the contents of the array, not the array itself. To me, adding the extra code complexity to detect these errors in legacy mode for this corner case is not worth it; hopefully Kotlinc will be convinced to put annotations in the "right" place soon. But I'd be interested in your opinion.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think without a coordinated change on kotlinc, I would prefer to keep the hack. But I don't feel super strongly about it. It's just an additional test of (a) is the annotation in bytecode, (b) is it the jetbrains annotation, (c) is it in var args... if all that is true, hacky override with a comment linking to the issue. No? Would this be hard to maintain?

Unfortunately, Java code interfacing with Kotlin code is a big portion of what we will see in Android for the foreseeable future, so kotlinc quirks might be the kind of thing we can't ignore for the sake of uniformity. Though if you can convince them to generate the "right" bytecode, I'd be happy to see this hack disappear (hopefully we don't need then to analyze magic numbers to see which kotlinc version produced the jar)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right; it looks like for now kotlinc will be emitting similar bytecodes. Fixed in 7330a20

" ThirdParty.takesNullableVarargs(o1, (Object[]) null);",
" // BUG: Diagnostic contains: passing @Nullable parameter 'o4' where @NonNull",
" ThirdParty.takesNullableVarargs(o4);", // First arg is not varargs.
Expand Down Expand Up @@ -416,6 +417,85 @@ public void varargsPassArrayAndOtherArgs() {
.doTest();
}

@Test
public void testVarargsNullArrayUnannotated() {
defaultCompilationHelper
.addSourceLines(
"Unannotated.java",
"package foo.unannotated;",
"public class Unannotated {",
" public static void takesVarargs(Object... args) {}",
"}")
.addSourceLines(
"Test.java",
"package com.uber;",
"import foo.unannotated.Unannotated;",
"public class Test {",
" public void test() {",
" Object[] x = null;",
" Unannotated.takesVarargs(x);",
" }",
"}")
.doTest();
}

/**
* This test is a WIP for restrictive annotations on varargs. More assertions still need to be
* written, and more support needs to be implemented.
*/
@Test
public void testVarargsRestrictive() {
makeTestHelperWithArgs(
Arrays.asList(
"-d",
temporaryFolder.getRoot().getAbsolutePath(),
"-XepOpt:NullAway:AnnotatedPackages=com.uber",
"-XepOpt:NullAway:LegacyAnnotationLocations=true",
"-XepOpt:NullAway:AcknowledgeRestrictiveAnnotations=true"))
.addSourceLines(
"NonNull.java",
"package com.uber.both;",
"import java.lang.annotation.ElementType;",
"import java.lang.annotation.Target;",
"@Target({ElementType.METHOD, ElementType.FIELD, ElementType.PARAMETER, ElementType.LOCAL_VARIABLE, ElementType.TYPE_USE})",
"public @interface NonNull {}")
.addSourceLines(
"Unannotated.java",
"package foo.unannotated;",
"public class Unannotated {",
" public static void takesVarargsDeclaration(@javax.annotation.Nonnull Object... args) {}",
" public static void takesVarargsTypeUseOnArray(Object @org.jspecify.annotations.NonNull... args) {}",
" public static void takesVarargsTypeUseOnElements(@org.jspecify.annotations.NonNull Object... args) {}",
" public static void takesVarargsTypeUseOnBoth(@org.jspecify.annotations.NonNull Object @org.jspecify.annotations.NonNull... args) {}",
" public static void takesVarargsBothOnArray(Object @com.uber.both.NonNull... args) {}",
" public static void takesVarargsBothOnElements(@com.uber.both.NonNull Object... args) {}",
" public static void takesVarargsBothOnBoth(@com.uber.both.NonNull Object @com.uber.both.NonNull... args) {}",
"}")
.addSourceLines(
"Test.java",
"package com.uber;",
"import foo.unannotated.Unannotated;",
"public class Test {",
" public void testDeclaration() {",
" Object x = null;",
" Object[] y = null;",
" // BUG: Diagnostic contains: passing @Nullable parameter 'x'",
" Unannotated.takesVarargsDeclaration(x);",
" // BUG: Diagnostic contains: passing @Nullable parameter 'y'",
" Unannotated.takesVarargsDeclaration(y);",
" }",
" public void testTypeUseOnArray() {",
" Object x = null;",
" Object[] y = null;",
" // BUG: Diagnostic contains: passing @Nullable parameter 'x'",
" Unannotated.takesVarargsTypeUseOnArray(x);",
" // BUG: Diagnostic contains: passing @Nullable parameter 'y'",
" Unannotated.takesVarargsTypeUseOnArray(y);",
" }",
"}")
.doTest();
}

private CompilationTestHelper makeHelper() {
return makeTestHelperWithArgs(
Arrays.asList(
Expand Down
81 changes: 80 additions & 1 deletion 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 @@ -196,7 +196,7 @@ public void testSkipJetbrainsNotNullOnVarArgsFromThirdPartyJars() {
" ThirdParty.takesNullableVarargs(o1);", // Empty var args passed
" ThirdParty.takesNullableVarargs(o1, o4);",
" ThirdParty.takesNullableVarargs(o1, (Object) null);",
" // BUG: Diagnostic contains: passing @Nullable parameter '(Object[]) null' where @NonNull",
" // No error: we require a type-use annotation for nullability of the varargs array itself",
" ThirdParty.takesNullableVarargs(o1, (Object[]) null);",
" // BUG: Diagnostic contains: passing @Nullable parameter 'o4' where @NonNull",
" ThirdParty.takesNullableVarargs(o4);", // First arg is not varargs.
Expand Down Expand Up @@ -417,4 +417,83 @@ public void varargsPassArrayAndOtherArgs() {
"}")
.doTest();
}

@Test
public void testVarargsNullArrayUnannotated() {
defaultCompilationHelper
.addSourceLines(
"Unannotated.java",
"package foo.unannotated;",
"public class Unannotated {",
" public static void takesVarargs(Object... args) {}",
"}")
.addSourceLines(
"Test.java",
"package com.uber;",
"import foo.unannotated.Unannotated;",
"public class Test {",
" public void test() {",
" Object x = null;",
" Object[] y = null;",
" Unannotated.takesVarargs(x);",
" Unannotated.takesVarargs(y);",
" }",
"}")
.doTest();
}

/**
* This test is a WIP for restrictive annotations on varargs. More assertions still need to be
* written, and more support needs to be implemented.
*/
@Test
public void testVarargsRestrictive() {
makeTestHelperWithArgs(
Arrays.asList(
"-d",
temporaryFolder.getRoot().getAbsolutePath(),
"-XepOpt:NullAway:AnnotatedPackages=com.uber",
"-XepOpt:NullAway:AcknowledgeRestrictiveAnnotations=true"))
.addSourceLines(
"NonNull.java",
"package com.uber.both;",
"import java.lang.annotation.ElementType;",
"import java.lang.annotation.Target;",
"@Target({ElementType.METHOD, ElementType.FIELD, ElementType.PARAMETER, ElementType.LOCAL_VARIABLE, ElementType.TYPE_USE})",
"public @interface NonNull {}")
.addSourceLines(
"Unannotated.java",
"package foo.unannotated;",
"public class Unannotated {",
" public static void takesVarargsDeclaration(@javax.annotation.Nonnull Object... args) {}",
" public static void takesVarargsTypeUseOnArray(Object @org.jspecify.annotations.NonNull... args) {}",
" public static void takesVarargsTypeUseOnElements(@org.jspecify.annotations.NonNull Object... args) {}",
" public static void takesVarargsTypeUseOnBoth(@org.jspecify.annotations.NonNull Object @org.jspecify.annotations.NonNull... args) {}",
" public static void takesVarargsBothOnArray(Object @com.uber.both.NonNull... args) {}",
" public static void takesVarargsBothOnElements(@com.uber.both.NonNull Object... args) {}",
" public static void takesVarargsBothOnBoth(@com.uber.both.NonNull Object @com.uber.both.NonNull... args) {}",
"}")
.addSourceLines(
"Test.java",
"package com.uber;",
"import foo.unannotated.Unannotated;",
"public class Test {",
" public void testDeclaration() {",
" Object x = null;",
" Object[] y = null;",
" // BUG: Diagnostic contains: passing @Nullable parameter 'x'",
" Unannotated.takesVarargsDeclaration(x);",
" Unannotated.takesVarargsDeclaration(y);",
" }",
" public void testTypeUseOnArray() {",
" Object x = null;",
" Object[] y = null;",
" Unannotated.takesVarargsTypeUseOnArray(x);",
// TODO report an error here; will require some refactoring of restrictive annotation
// handling
" Unannotated.takesVarargsTypeUseOnArray(y);",
" }",
msridhar marked this conversation as resolved.
Show resolved Hide resolved
"}")
.doTest();
}
}
80 changes: 79 additions & 1 deletion 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 @@ -206,7 +206,7 @@ public void testSkipJetbrainsNotNullOnVarArgsFromThirdPartyJars() {
" ThirdParty.takesNullableVarargs(o1);", // Empty var args passed
" ThirdParty.takesNullableVarargs(o1, o4);",
" ThirdParty.takesNullableVarargs(o1, (Object) null);",
" // BUG: Diagnostic contains: passing @Nullable parameter '(Object[]) null' where @NonNull",
" // No error: we require a type-use annotation for nullability of the varargs array itself",
" ThirdParty.takesNullableVarargs(o1, (Object[]) null);",
" // BUG: Diagnostic contains: passing @Nullable parameter 'o4' where @NonNull",
" ThirdParty.takesNullableVarargs(o4);", // First arg is not varargs.
Expand Down Expand Up @@ -431,6 +431,84 @@ public void varargsPassArrayAndOtherArgs() {
.doTest();
}

@Test
public void testVarargsNullArrayUnannotated() {
defaultCompilationHelper
.addSourceLines(
"Unannotated.java",
"package foo.unannotated;",
"public class Unannotated {",
" public static void takesVarargs(Object... args) {}",
"}")
.addSourceLines(
"Test.java",
"package com.uber;",
"import foo.unannotated.Unannotated;",
"public class Test {",
" public void test() {",
" Object[] x = null;",
" Unannotated.takesVarargs(x);",
" }",
"}")
.doTest();
}

/**
* This test is a WIP for restrictive annotations on varargs. More assertions still need to be
* written, and more support needs to be implemented.
*/
@Test
public void testVarargsRestrictive() {
makeTestHelperWithArgs(
Arrays.asList(
"-d",
temporaryFolder.getRoot().getAbsolutePath(),
"-XepOpt:NullAway:AnnotatedPackages=com.uber",
"-XepOpt:NullAway:JSpecifyMode=true",
"-XepOpt:NullAway:AcknowledgeRestrictiveAnnotations=true"))
.addSourceLines(
"NonNull.java",
"package com.uber.both;",
"import java.lang.annotation.ElementType;",
"import java.lang.annotation.Target;",
"@Target({ElementType.METHOD, ElementType.FIELD, ElementType.PARAMETER, ElementType.LOCAL_VARIABLE, ElementType.TYPE_USE})",
"public @interface NonNull {}")
.addSourceLines(
"Unannotated.java",
"package foo.unannotated;",
"public class Unannotated {",
" public static void takesVarargsDeclaration(@javax.annotation.Nonnull Object... args) {}",
" public static void takesVarargsTypeUseOnArray(Object @org.jspecify.annotations.NonNull... args) {}",
" public static void takesVarargsTypeUseOnElements(@org.jspecify.annotations.NonNull Object... args) {}",
" public static void takesVarargsTypeUseOnBoth(@org.jspecify.annotations.NonNull Object @org.jspecify.annotations.NonNull... args) {}",
" public static void takesVarargsBothOnArray(Object @com.uber.both.NonNull... args) {}",
" public static void takesVarargsBothOnElements(@com.uber.both.NonNull Object... args) {}",
" public static void takesVarargsBothOnBoth(@com.uber.both.NonNull Object @com.uber.both.NonNull... args) {}",
"}")
.addSourceLines(
"Test.java",
"package com.uber;",
"import foo.unannotated.Unannotated;",
"public class Test {",
" public void testDeclaration() {",
" Object x = null;",
" Object[] y = null;",
" // BUG: Diagnostic contains: passing @Nullable parameter 'x'",
" Unannotated.takesVarargsDeclaration(x);",
" Unannotated.takesVarargsDeclaration(y);",
" }",
" public void testTypeUseOnArray() {",
" Object x = null;",
" Object[] y = null;",
" Unannotated.takesVarargsTypeUseOnArray(x);",
// TODO report an error here; will require some refactoring of restrictive annotation
// handling
" Unannotated.takesVarargsTypeUseOnArray(y);",
" }",
"}")
.doTest();
}

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