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.

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

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

Merged
merged 13 commits into from
Sep 7, 2024
20 changes: 15 additions & 5 deletions nullaway/src/main/java/com/uber/nullaway/NullAway.java
Original file line number Diff line number Diff line change
Expand Up @@ -1780,17 +1780,27 @@ private Description handleInvocation(
}
actual = actualParams.get(argPos);
// check if the varargs arguments are being passed as an array
Type.ArrayType varargsArrayType =
(Type.ArrayType) formalParams.get(formalParams.size() - 1).type;
VarSymbol formalParamSymbol = formalParams.get(formalParams.size() - 1);
Type.ArrayType varargsArrayType = (Type.ArrayType) formalParamSymbol.type;
Type actualParameterType = ASTHelpers.getType(actual);
if (actualParameterType != null
&& state.getTypes().isAssignable(actualParameterType, varargsArrayType)
&& 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), or a JetBrains @NotNull declaration
// annotation is present (due to https://github.com/uber/NullAway/issues/720)
boolean checkForNullableVarargsArray =
isMethodAnnotated
|| (config.isLegacyAnnotationLocation() && argIsNonNull)
|| NullabilityUtil.hasJetBrainsNotNullDeclarationAnnotation(formalParamSymbol);
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
40 changes: 40 additions & 0 deletions nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@
public static final String NULLUNMARKED_SIMPLE_NAME = "NullUnmarked";

private static final Supplier<Type> MAP_TYPE_SUPPLIER = Suppliers.typeFromString("java.util.Map");
private static final String JETBRAINS_NOT_NULL = "org.jetbrains.annotations.NotNull";

private NullabilityUtil() {}

Expand Down Expand Up @@ -440,4 +441,43 @@
}
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 461 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#L461

Added line #L461 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 468 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#L468

Added line #L468 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.

}

/**
* Does the given symbol have a JetBrains @NotNull declaration annotation? Useful for workarounds
* in light of https://github.com/uber/NullAway/issues/720
*/
public static boolean hasJetBrainsNotNullDeclarationAnnotation(Symbol.VarSymbol varSymbol) {
// We explicitly ignore type-use annotations here, looking for @NotNull used as a
// declaration annotation, which is why this logic is simpler than e.g.
// NullabilityUtil.getAllAnnotationsForParameter.
return varSymbol.getAnnotationMirrors().stream()
.map(a -> a.getAnnotationType().toString())
.anyMatch(annotName -> annotName.equals(JETBRAINS_NOT_NULL));
}
}
22 changes: 19 additions & 3 deletions nullaway/src/main/java/com/uber/nullaway/Nullness.java
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);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
import com.uber.nullaway.CodeAnnotationInfo;
import com.uber.nullaway.Config;
import com.uber.nullaway.NullAway;
import com.uber.nullaway.NullabilityUtil;
import com.uber.nullaway.Nullness;
import com.uber.nullaway.dataflow.AccessPath;
import com.uber.nullaway.dataflow.AccessPathNullnessPropagation;
Expand All @@ -40,8 +41,6 @@

public class RestrictiveAnnotationHandler extends BaseNoOpHandler {

private static final String JETBRAINS_NOT_NULL = "org.jetbrains.annotations.NotNull";

private final Config config;

RestrictiveAnnotationHandler(Config config) {
Expand Down Expand Up @@ -113,13 +112,9 @@ public Nullness[] onOverrideMethodInvocationParametersNullability(
if (methodSymbol.isVarArgs() && i == methodSymbol.getParameters().size() - 1) {
// Special handling: ignore org.jetbrains.annotations.NotNull on varargs parameters
// to handle kotlinc generated jars (see #720)
// We explicitly ignore type-use annotations here, looking for @NotNull used as a
// declaration annotation, which is why this logic is simpler than e.g.
// NullabilityUtil.getAllAnnotationsForParameter.
Symbol.VarSymbol varargsParamSymbol = methodSymbol.getParameters().get(i);
boolean jetBrainsNotNullAnnotated =
methodSymbol.getParameters().get(i).getAnnotationMirrors().stream()
.map(a -> a.getAnnotationType().toString())
.anyMatch(annotName -> annotName.equals(JETBRAINS_NOT_NULL));
NullabilityUtil.hasJetBrainsNotNullDeclarationAnnotation(varargsParamSymbol);
if (jetBrainsNotNullAnnotated) {
continue;
}
Expand Down
87 changes: 87 additions & 0 deletions nullaway/src/test/java/com/uber/nullaway/LegacyVarargsTests.java
Original file line number Diff line number Diff line change
Expand Up @@ -416,6 +416,93 @@ 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);",
" }",
" public void testTypeUseOnElements() {",
" Object x = null;",
" Object[] y = null;",
" // BUG: Diagnostic contains: passing @Nullable parameter 'x'",
" Unannotated.takesVarargsTypeUseOnElements(x);",
" // BUG: Diagnostic contains: passing @Nullable parameter 'y'",
" Unannotated.takesVarargsTypeUseOnElements(y);",
" }",
"}")
.doTest();
}

private CompilationTestHelper makeHelper() {
return makeTestHelperWithArgs(
Arrays.asList(
Expand Down
98 changes: 98 additions & 0 deletions nullaway/src/test/java/com/uber/nullaway/VarargsTests.java
Original file line number Diff line number Diff line change
Expand Up @@ -417,4 +417,102 @@ public void varargsPassArrayAndOtherArgs() {
"}")
.doTest();
}

@Test
public void testVarargsNullArrayUnannotated() {
String[] unannotatedSource = {
"package foo.unannotated;",
"public class Unannotated {",
" public static void takesVarargs(Object... args) {}",
"}"
};
String[] testSource = {
"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);",
" }",
"}"
};
// test with both restrictive annotations enabled and disabled
defaultCompilationHelper
.addSourceLines("Unannotated.java", unannotatedSource)
.addSourceLines("Test.java", testSource)
.doTest();
makeTestHelperWithArgs(
Arrays.asList(
"-d",
temporaryFolder.getRoot().getAbsolutePath(),
"-XepOpt:NullAway:AnnotatedPackages=com.uber",
"-XepOpt:NullAway:AcknowledgeRestrictiveAnnotations=true"))
.addSourceLines("Unannotated.java", unannotatedSource)
.addSourceLines("Test.java", testSource)
.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
" public void testTypeUseOnElements() {",
" Object x = null;",
" Object[] y = null;",
" // BUG: Diagnostic contains: passing @Nullable parameter 'x'",
" Unannotated.takesVarargsTypeUseOnElements(x);",
" Unannotated.takesVarargsTypeUseOnElements(y);",
" }",
"}")
.doTest();
}
}
Loading