Skip to content

Commit

Permalink
Partial handling for restrictive annotations on varargs in unannotate…
Browse files Browse the repository at this point in the history 
…d code (#1029)

Partially addresses #1027. Most importantly, handles the case with
unannotated methods and _no_ restrictive annotations; we were getting
warnings in some cases with the main branch, which impacts backward
compatibility.

I started implementing full support for restrictive annotations on
varargs parameters and then realized it would take some refactoring
(since there can be a restrictive annotation on the varargs array itself
or on its contents, and we have no way to express this in the current
code structure). This PR contains some incomplete code towards that
change, which I figured does no harm so I left it in for now. If desired
I can remove it and put it in a separate PR.
  • Loading branch information
@msridhar
msridhar authored Sep 7, 2024
1 parent fe9476a commit e2e5394
Show file tree
Hide file tree
Showing 7 changed files with 347 additions and 16 deletions.
20 changes: 15 additions & 5 deletions nullaway/src/main/java/com/uber/nullaway/NullAway.java
View file
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
View file
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ public class NullabilityUtil {
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 @@ public static boolean isArrayElementNullable(Symbol arraySymbol, Config config)
}
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;
}
}
}
}
// 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;
}

/**
* 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
View file
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);
}
}
11 changes: 3 additions & 8 deletions nullaway/src/main/java/com/uber/nullaway/handlers/RestrictiveAnnotationHandler.java
View file
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
View file
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
View file
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);",
" }",
" public void testTypeUseOnElements() {",
" Object x = null;",
" Object[] y = null;",
" // BUG: Diagnostic contains: passing @Nullable parameter 'x'",
" Unannotated.takesVarargsTypeUseOnElements(x);",
" Unannotated.takesVarargsTypeUseOnElements(y);",
" }",
"}")
.doTest();
}
}
Loading

0 comments on commit e2e5394

Please sign in to comment.