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

Update handling of annotations on varargs argument #1025

Merged
merged 29 commits into from
Sep 1, 2024
Merged
Show file tree
Hide file tree
Changes from 18 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
47 changes: 32 additions & 15 deletions nullaway/src/main/java/com/uber/nullaway/NullAway.java
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,6 @@

import com.google.auto.service.AutoService;
import com.google.auto.value.AutoValue;
import com.google.common.base.Preconditions;
import com.google.common.base.Verify;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMultimap;
Expand Down Expand Up @@ -1705,8 +1704,9 @@ private Description handleInvocation(
List<? extends ExpressionTree> actualParams) {
List<VarSymbol> formalParams = methodSymbol.getParameters();

boolean varArgsMethod = methodSymbol.isVarArgs();
if (formalParams.size() != actualParams.size()
&& !methodSymbol.isVarArgs()
&& !varArgsMethod
&& !methodSymbol.isStatic()
&& methodSymbol.isConstructor()
&& methodSymbol.enclClass().isInner()) {
Expand Down Expand Up @@ -1751,7 +1751,7 @@ private Description handleInvocation(
}
if (config.isJSpecifyMode()) {
GenericsChecks.compareGenericTypeParameterNullabilityForCall(
formalParams, actualParams, methodSymbol.isVarArgs(), this, state);
formalParams, actualParams, varArgsMethod, this, state);
}
}

Expand All @@ -1764,30 +1764,47 @@ private Description handleInvocation(
// NOTE: the case of an invocation on a possibly-null reference
// is handled by matchMemberSelect()
for (int argPos = 0; argPos < argumentPositionNullness.length; argPos++) {
if (!Objects.equals(Nullness.NONNULL, argumentPositionNullness[argPos])) {
boolean varargPosition = varArgsMethod && argPos == formalParams.size() - 1;
boolean argIsNonNull = Objects.equals(Nullness.NONNULL, argumentPositionNullness[argPos]);
if (!varargPosition && !argIsNonNull) {
continue;
}
ExpressionTree actual = null;
ExpressionTree actual;
boolean mayActualBeNull = false;
if (argPos == formalParams.size() - 1 && methodSymbol.isVarArgs()) {
if (varargPosition) {
// Check all vararg actual arguments for nullability
if (actualParams.size() <= argPos) {
msridhar marked this conversation as resolved.
Show resolved Hide resolved
continue;
}
for (ExpressionTree arg : actualParams.subList(argPos, actualParams.size())) {
actual = arg;
mayActualBeNull = mayBeNullExpr(state, actual);
if (mayActualBeNull) {
break;
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;
Type actualParameterType = ASTHelpers.getType(actual);
if (actualParameterType != null
&& state.getTypes().isAssignable(actualParameterType, varargsArrayType)
&& actualParams.size() == argPos + 1) {
// If varargs array itself is not @Nullable, cannot pass @Nullable array
msridhar marked this conversation as resolved.
Show resolved Hide resolved
if (!Nullness.varargsArrayIsNullable(formalParams.get(argPos), config)) {
mayActualBeNull = mayBeNullExpr(state, actual);
}
} else { // varargs are being passed individually
if (!argIsNonNull) {
msridhar marked this conversation as resolved.
Show resolved Hide resolved
continue;
}
// TODO report an error for each violating vararg
for (ExpressionTree arg : actualParams.subList(argPos, actualParams.size())) {
actual = arg;
mayActualBeNull = mayBeNullExpr(state, actual);
if (mayActualBeNull) {
break;
}
}
}
} else {
} else { // not the vararg position
actual = actualParams.get(argPos);
mayActualBeNull = mayBeNullExpr(state, actual);
}
// This statement should be unreachable without assigning actual beforehand:
Preconditions.checkNotNull(actual);
Comment on lines -1788 to -1789
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is still the case, right? Is just that now NullAway can prove this statically?

Copy link
Collaborator Author

@msridhar msridhar Sep 1, 2024

Choose a reason for hiding this comment

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

Correct, javac's initialized variables check now proves it (along with the fact that we never assign null to the variable)

// make sure we are passing a non-null value
if (mayActualBeNull) {
String message =
"passing @Nullable parameter '"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -278,7 +278,7 @@ public static Stream<? extends AnnotationMirror> getAllAnnotationsForParameter(
* Gets the type use annotations on a symbol, ignoring annotations on components of the type (type
* arguments, wildcards, etc.)
*/
private static Stream<? extends AnnotationMirror> getTypeUseAnnotations(
public static Stream<? extends AnnotationMirror> getTypeUseAnnotations(
Symbol symbol, Config config) {
Stream<Attribute.TypeCompound> rawTypeAttributes = symbol.getRawTypeAttributes().stream();
if (symbol instanceof Symbol.MethodSymbol) {
Expand Down
38 changes: 36 additions & 2 deletions nullaway/src/main/java/com/uber/nullaway/Nullness.java
Original file line number Diff line number Diff line change
Expand Up @@ -206,9 +206,16 @@ public static boolean hasNullableAnnotation(Symbol symbol, Config config) {
return hasNullableAnnotation(NullabilityUtil.getAllAnnotations(symbol, config), config);
}

private static boolean hasNullableTypeUseAnnotation(Symbol symbol, Config config) {
return hasNullableAnnotation(NullabilityUtil.getTypeUseAnnotations(symbol, config), config);
}

/**
* Does the parameter of {@code symbol} at {@code paramInd} have a {@code @Nullable} 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 can be null.
*/
public static boolean paramHasNullableAnnotation(
Symbol.MethodSymbol symbol, int paramInd, Config config) {
Expand All @@ -217,8 +224,14 @@ public static boolean paramHasNullableAnnotation(
if (isRecordEqualsParam(symbol, paramInd)) {
return true;
}
return hasNullableAnnotation(
NullabilityUtil.getAllAnnotationsForParameter(symbol, paramInd, config), config);
if (symbol.isVarArgs()
&& paramInd == symbol.getParameters().size() - 1
&& !config.isLegacyAnnotationLocation()) {
return individualVarargsParamsAreNullable(symbol.getParameters().get(paramInd), config);
} else {
return hasNullableAnnotation(
NullabilityUtil.getAllAnnotationsForParameter(symbol, paramInd, config), config);
}
}

private static boolean isRecordEqualsParam(Symbol.MethodSymbol symbol, int paramInd) {
Expand Down Expand Up @@ -251,4 +264,25 @@ public static boolean paramHasNonNullAnnotation(
return hasNonNullAnnotation(
NullabilityUtil.getAllAnnotationsForParameter(symbol, paramInd, config), config);
}

/**
* Is 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}}
*/
public static boolean varargsArrayIsNullable(Symbol paramSymbol, Config config) {
return hasNullableTypeUseAnnotation(paramSymbol, config)
|| (config.isLegacyAnnotationLocation()
&& hasNullableDeclarationAnnotation(paramSymbol, config));
}

private static boolean individualVarargsParamsAreNullable(Symbol paramSymbol, Config config) {
// declaration annotation, or type-use annotation on the elements
return hasNullableDeclarationAnnotation(paramSymbol, config)
|| NullabilityUtil.isArrayElementNullable(paramSymbol, config);
}

private static boolean hasNullableDeclarationAnnotation(Symbol symbol, Config config) {
return hasNullableAnnotation(symbol.getRawAttributes().stream(), config);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
import com.sun.source.tree.ClassTree;
import com.sun.source.tree.LambdaExpressionTree;
import com.sun.source.tree.VariableTree;
import com.sun.tools.javac.code.Flags;
import com.sun.tools.javac.code.Symbol;
import com.sun.tools.javac.code.Type;
import com.sun.tools.javac.code.Types;
Expand Down Expand Up @@ -64,9 +65,13 @@ private static NullnessStore methodInitialStore(
NullnessStore envStore = getEnvNullnessStoreForClass(classTree, context);
NullnessStore.Builder result = envStore.toBuilder();
for (LocalVariableNode param : parameters) {
Element element = param.getElement();
Nullness assumed =
Nullness.hasNullableAnnotation((Symbol) element, config) ? NULLABLE : NONNULL;
Symbol paramSymbol = (Symbol) param.getElement();
Nullness assumed;
if ((paramSymbol.flags() & Flags.VARARGS) != 0) {
assumed = Nullness.varargsArrayIsNullable(paramSymbol, config) ? NULLABLE : NONNULL;
} else {
assumed = Nullness.hasNullableAnnotation(paramSymbol, config) ? NULLABLE : NONNULL;
}
result.setInformation(AccessPath.fromLocal(param), assumed);
}
result = handler.onDataflowInitialStore(underlyingAST, parameters, result);
Expand Down
Loading