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 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
55 changes: 40 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,55 @@ 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
// This is the case where no actual parameter is passed for the var args parameter
// (i.e. it defaults to an empty array)
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) {
// 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
msridhar marked this conversation as resolved.
Show resolved Hide resolved
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
// arguments starting at the position of the var args formal.
// If the formal var args accepts `@Nullable`, then there is nothing for us to check.
if (!argIsNonNull) {
continue;
}
// TODO report all varargs errors in a single build; this code only reports the first
// error
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 @@ -38,6 +38,7 @@
import com.sun.source.tree.VariableTree;
import com.sun.source.util.TreePath;
import com.sun.tools.javac.code.Attribute;
import com.sun.tools.javac.code.Flags;
import com.sun.tools.javac.code.Symbol;
import com.sun.tools.javac.code.TargetType;
import com.sun.tools.javac.code.Type;
Expand Down Expand Up @@ -278,7 +279,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 Expand Up @@ -432,6 +433,11 @@ public static boolean isArrayElementNullable(Symbol arraySymbol, Config config)
}
}
}
// For varargs symbols we also consider the elements to be @Nullable if there is a @Nullable
// declaration annotation on the parameter
if ((arraySymbol.flags() & Flags.VARARGS) != 0) {
return Nullness.hasNullableDeclarationAnnotation(arraySymbol, config);
}
return false;
}
}
35 changes: 33 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,16 @@ 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()) {
// individual arguments passed in the varargs positions can be @Nullable if the array element
// type of the parameter is @Nullable
return NullabilityUtil.isArrayElementNullable(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 +266,20 @@ 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));
}

/** Checks if the symbol has a {@code @Nullable} declaration annotation */
public 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