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

Fix reading of JSpecify @Nullable annotations from varargs parameter in bytecode #1089

Merged
merged 5 commits into from
Dec 12, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
86 changes: 49 additions & 37 deletions nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@
import java.util.List;
import java.util.Map;
import java.util.Set;
import java.util.function.BiPredicate;
import java.util.stream.Collectors;
import java.util.stream.Stream;
import javax.lang.model.element.AnnotationMirror;
Expand Down Expand Up @@ -328,13 +329,13 @@ private static Stream<Attribute.TypeCompound> getTypeUseAnnotations(
return rawTypeAttributes.filter(
(t) ->
t.position.type.equals(TargetType.METHOD_RETURN)
&& isDirectTypeUseAnnotation(t, symbol, config));
&& (!onlyDirect || isDirectTypeUseAnnotation(t, symbol, config)));
} else {
// filter for annotations directly on the type
return rawTypeAttributes.filter(
t ->
targetTypeMatches(symbol, t.position)
&& (!onlyDirect || NullabilityUtil.isDirectTypeUseAnnotation(t, symbol, config)));
&& (!onlyDirect || isDirectTypeUseAnnotation(t, symbol, config)));
}
}

Expand Down Expand Up @@ -540,22 +541,11 @@ public static <T> T castToNonNull(@Nullable T obj) {
* otherwise
*/
public static boolean isArrayElementNullable(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.isNullableAnnotation(t.type.toString(), config)) {
return true;
}
}
}
}
// For varargs symbols we also consider the elements to be @Nullable if there is a @Nullable
// declaration annotation on the parameter
// NOTE this flag check does not work for the varargs parameter of a method defined in bytecodes
if ((arraySymbol.flags() & Flags.VARARGS) != 0) {
return Nullness.hasNullableDeclarationAnnotation(arraySymbol, config);
}
return false;
return checkArrayElementAnnotations(
arraySymbol,
config,
Nullness::isNullableAnnotation,
Nullness::hasNullableDeclarationAnnotation);
}

/**
Expand All @@ -582,12 +572,50 @@ public static boolean nullableVarargsElementsForSourceOrBytecode(
* otherwise
*/
public static boolean isArrayElementNonNull(Symbol arraySymbol, Config config) {
return checkArrayElementAnnotations(
arraySymbol,
config,
Nullness::isNonNullAnnotation,
Nullness::hasNonNullDeclarationAnnotation);
}

/**
* Checks if the given varargs symbol has a {@code @NonNull} annotation for its elements. Works
* for both source and bytecode.
*
* @param varargsSymbol the symbol of the varargs parameter
* @param config NullAway configuration
* @return true if the varargs symbol has a {@code @NonNull} annotation for its elements, false
* otherwise
*/
public static boolean nonnullVarargsElementsForSourceOrBytecode(
Symbol varargsSymbol, Config config) {
return isArrayElementNonNull(varargsSymbol, config)
|| Nullness.hasNonNullDeclarationAnnotation(varargsSymbol, config);
}

/**
* Checks if the annotations on the elements of some array symbol satisfy some predicate.
*
* @param arraySymbol the array symbol
* @param config NullAway configuration
* @param typeUseCheck the predicate to check the type-use annotations
* @param declarationCheck the predicate to check the declaration annotations (applied only to
* varargs symbols)
* @return true if the annotations on the elements of the array symbol satisfy the given
* predicates, false otherwise
*/
private static boolean checkArrayElementAnnotations(
Symbol arraySymbol,
Config config,
BiPredicate<String, Config> typeUseCheck,
BiPredicate<Symbol, Config> declarationCheck) {
if (getTypeUseAnnotations(arraySymbol, config, /* onlyDirect= */ false)
.anyMatch(
t -> {
for (TypeAnnotationPosition.TypePathEntry entry : t.position.location) {
if (entry.tag == TypeAnnotationPosition.TypePathEntryKind.ARRAY) {
if (Nullness.isNonNullAnnotation(t.type.toString(), config)) {
if (typeUseCheck.test(t.type.toString(), config)) {
return true;
}
}
Expand All @@ -596,30 +624,14 @@ public static boolean isArrayElementNonNull(Symbol arraySymbol, Config config) {
})) {
return true;
}
// For varargs symbols we also consider the elements to be @NonNull if there is a @NonNull
// declaration annotation on the parameter
// For varargs symbols we also check for declaration annotations on the parameter
// NOTE this flag check does not work for the varargs parameter of a method defined in bytecodes
if ((arraySymbol.flags() & Flags.VARARGS) != 0) {
return Nullness.hasNonNullDeclarationAnnotation(arraySymbol, config);
return declarationCheck.test(arraySymbol, config);
}
return false;
}

/**
* Checks if the given varargs symbol has a {@code @NonNull} annotation for its elements. Works
* for both source and bytecode.
*
* @param varargsSymbol the symbol of the varargs parameter
* @param config NullAway configuration
* @return true if the varargs symbol has a {@code @NonNull} annotation for its elements, false
* otherwise
*/
public static boolean nonnullVarargsElementsForSourceOrBytecode(
Symbol varargsSymbol, Config config) {
return isArrayElementNonNull(varargsSymbol, config)
|| Nullness.hasNonNullDeclarationAnnotation(varargsSymbol, config);
}

/**
* Does the given symbol have a JetBrains @NotNull declaration annotation? Useful for workarounds
* in light of https://github.com/uber/NullAway/issues/720
Expand Down
16 changes: 16 additions & 0 deletions nullaway/src/test/java/com/uber/nullaway/VarargsTests.java
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,22 @@ public void nullableTypeUseVarArgsFromBytecode() {
.doTest();
}

@Test
public void nullableVarArgsFromBytecodeJSpecify() {
defaultCompilationHelper
.addSourceLines(
"Test.java",
"package com.uber;",
"import com.uber.lib.Varargs;",
"public class Test {",
" public void testTypeUse() {",
" String x = null;",
" Varargs.typeUseNullableElementsJSpecify(x);",
" }",
"}")
.doTest();
}

@Test
public void nullableTypeUseVarargs() {
defaultCompilationHelper
Expand Down
3 changes: 3 additions & 0 deletions test-java-lib/src/main/java/com/uber/lib/Varargs.java
Original file line number Diff line number Diff line change
Expand Up @@ -7,4 +7,7 @@ public class Varargs {
public Varargs(@Nullable String... args) {}

public static void typeUse(String @org.jspecify.annotations.Nullable ... args) {}

public static void typeUseNullableElementsJSpecify(
@org.jspecify.annotations.Nullable String... args) {}
}
Loading