Skip to content

Commit c187bf5

Browse files
msridharmsridhar
authored
Abstract iterating over invocation arguments (#1284)
Creates a new class `InvocationArguments` that encapsulates the logic for iterating over arguments at a method / constructor call, including handling of varargs. This lets use remove some hairy vararg-handling logic from multiple places to make that code more readable, and this will also be useful in the future. As a bonus, we get the following improvements / bug fixes (with new tests): * Properly warn on potential unboxing errors when arguments are passed in the varargs position. * When multiple individual `@Nullable` varargs are passed when `@NonNull` is expected, we now report errors for all of them in a single run. * We properly catch mismatched generic type nullability when varargs are passed as an array. * We properly generate generic method inference constraints when varargs are passed as an array. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - New Features - Introduced a utility to uniformly process method/constructor arguments, including varargs. - Bug Fixes - Improved accuracy of nullability checks for varargs, whether passed individually or as an array. - More consistent generic type inference and diagnostics when arrays are supplied to varargs parameters. - Refactor - Consolidated invocation argument handling and simplified generics nullability checks for cleaner, more maintainable logic. - Tests - Added coverage for non-null varargs with nullable elements. - Added tests for varargs passed as arrays and generic inference outcomes. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
1 parent dd0da6d commit c187bf5

7 files changed

Lines changed: 272 additions & 149 deletions

File tree

‎nullaway/src/main/java/com/uber/nullaway/InvocationArguments.java‎

Lines changed: 127 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,127 @@
1+
package com.uber.nullaway;
2+
3+
import static com.google.common.base.Preconditions.checkNotNull;
4+
import static com.uber.nullaway.NullabilityUtil.castToNonNull;
5+
6+
import com.google.errorprone.util.ASTHelpers;
7+
import com.sun.source.tree.ExpressionTree;
8+
import com.sun.source.tree.MethodInvocationTree;
9+
import com.sun.source.tree.NewClassTree;
10+
import com.sun.source.tree.Tree;
11+
import com.sun.tools.javac.code.Symbol;
12+
import com.sun.tools.javac.code.Type;
13+
import java.util.List;
14+
import org.jspecify.annotations.Nullable;
15+
16+
/**
17+
* Utility class to encapsulate details of operating on all method/constructor invocation arguments,
18+
* including handling of varargs.
19+
*/
20+
public class InvocationArguments {
21+
22+
// Cached args as array to avoid O(n) indexed access on javac lists
23+
private final ExpressionTree[] argsArr;
24+
private final int numArgsPassed;
25+
26+
// Cached parameter types as array to avoid O(n) indexed access on javac lists
27+
private final Type[] paramTypesArr;
28+
29+
// Varargs metadata (fully precomputed)
30+
private final boolean isVarArgs;
31+
private final boolean varArgsPassedIndividually;
32+
private final int varArgsIndex;
33+
private final Type.@Nullable ArrayType varArgsArrayType; // null when not varargs
34+
private final @Nullable Type varArgsComponentType; // null when not varargs
35+
36+
/**
37+
* Construct an InvocationArguments instance for the given invocation tree and method type.
38+
*
39+
* @param invocationTree the invocation tree (method call or constructor call)
40+
* @param invokedMethodType the method type of the invoked method/constructor
41+
*/
42+
public InvocationArguments(Tree invocationTree, Type.MethodType invokedMethodType) {
43+
Symbol.MethodSymbol methodSymbol = (Symbol.MethodSymbol) ASTHelpers.getSymbol(invocationTree);
44+
checkNotNull(methodSymbol, "Expected method symbol for invocation tree");
45+
46+
// Cache args as array (fast indexed access)
47+
List<? extends ExpressionTree> argsList;
48+
if (invocationTree instanceof MethodInvocationTree) {
49+
argsList = ((MethodInvocationTree) invocationTree).getArguments();
50+
} else if (invocationTree instanceof NewClassTree) {
51+
argsList = ((NewClassTree) invocationTree).getArguments();
52+
} else {
53+
throw new IllegalStateException(
54+
"Unexpected invocation tree type " + invocationTree.getClass());
55+
}
56+
this.argsArr = argsList.toArray(new ExpressionTree[argsList.size()]);
57+
this.numArgsPassed = argsArr.length;
58+
59+
// Cache parameter types as array (fast indexed access)
60+
com.sun.tools.javac.util.List<Type> parameterTypes = invokedMethodType.getParameterTypes();
61+
this.paramTypesArr = parameterTypes.toArray(new Type[parameterTypes.size()]);
62+
63+
// Precompute varargs state and related types
64+
this.isVarArgs = methodSymbol.isVarArgs();
65+
if (this.isVarArgs) {
66+
this.varArgsIndex = paramTypesArr.length - 1;
67+
this.varArgsArrayType = (Type.ArrayType) paramTypesArr[varArgsIndex];
68+
this.varArgsComponentType = varArgsArrayType.getComponentType();
69+
this.varArgsPassedIndividually = NullabilityUtil.isVarArgsCall(invocationTree);
70+
} else {
71+
this.varArgsIndex = -1;
72+
this.varArgsArrayType = null;
73+
this.varArgsComponentType = null;
74+
this.varArgsPassedIndividually = false;
75+
}
76+
}
77+
78+
/**
79+
* Apply the given {@link ArgConsumer} to information about each argument
80+
*
81+
* @param consumer the consumer to apply
82+
*/
83+
public void forEach(ArgConsumer consumer) {
84+
if (!isVarArgs) {
85+
for (int i = 0; i < numArgsPassed; i++) {
86+
consumer.accept(argsArr[i], i, paramTypesArr[i], false);
87+
}
88+
return;
89+
}
90+
if (varArgsPassedIndividually) {
91+
Type varArgsComponentType = castToNonNull(this.varArgsComponentType);
92+
for (int i = 0; i < numArgsPassed; i++) {
93+
if (i < varArgsIndex) {
94+
consumer.accept(argsArr[i], i, paramTypesArr[i], false);
95+
} else {
96+
consumer.accept(argsArr[i], i, varArgsComponentType, false);
97+
}
98+
}
99+
} else {
100+
Type.ArrayType varArgsArrayType = castToNonNull(this.varArgsArrayType);
101+
for (int i = 0; i < numArgsPassed; i++) {
102+
if (i < varArgsIndex) {
103+
consumer.accept(argsArr[i], i, paramTypesArr[i], false);
104+
} else {
105+
consumer.accept(argsArr[i], i, varArgsArrayType, true);
106+
}
107+
}
108+
}
109+
}
110+
111+
/** Consumer type for information about each passed argument */
112+
@FunctionalInterface
113+
public interface ArgConsumer {
114+
115+
/**
116+
* Process information about a passed argument
117+
*
118+
* @param argTree the argument expression tree
119+
* @param argPos the argument position (0-based)
120+
* @param formalParamType the formal parameter type for this argument
121+
* @param varArgsPassedAsArray true if this argument corresponds to an array passed in the
122+
* varargs position (i.e., the array holds the individual varargs values)
123+
*/
124+
void accept(
125+
ExpressionTree argTree, int argPos, Type formalParamType, boolean varArgsPassedAsArray);
126+
}
127+
}

‎nullaway/src/main/java/com/uber/nullaway/NullAway.java‎

Lines changed: 55 additions & 72 deletions
Original file line numberDiff line numberDiff line change
@@ -1944,14 +1944,15 @@ private Description handleInvocation(
19441944
Symbol.MethodSymbol methodSymbol,
19451945
List<? extends ExpressionTree> actualParams) {
19461946
List<VarSymbol> formalParams = methodSymbol.getParameters();
1947-
boolean varArgsMethod = methodSymbol.isVarArgs();
19481947

1948+
InvocationArguments invArgs = new InvocationArguments(tree, methodSymbol.type.asMethodType());
19491949
// always do unboxing checks, whether or not the invoked method is annotated
1950-
for (int i = 0; i < formalParams.size() && i < actualParams.size(); i++) {
1951-
if (formalParams.get(i).type.isPrimitive()) {
1952-
doUnboxingCheck(state, actualParams.get(i));
1953-
}
1954-
}
1950+
invArgs.forEach(
1951+
(actual, argPos, formalParamType, varArgsPassedAsArray) -> {
1952+
if (formalParamType.isPrimitive()) {
1953+
doUnboxingCheck(state, actual);
1954+
}
1955+
});
19551956
boolean isMethodAnnotated =
19561957
!codeAnnotationInfo.isSymbolUnannotated(methodSymbol, config, handler);
19571958
// If argumentPositionNullness[i] == null, parameter i is unannotated
@@ -1986,89 +1987,71 @@ private Description handleInvocation(
19861987
}
19871988
}
19881989
if (config.isJSpecifyMode()) {
1989-
genericsChecks.compareGenericTypeParameterNullabilityForCall(
1990-
methodSymbol, tree, actualParams, varArgsMethod, state);
1990+
genericsChecks.compareGenericTypeParameterNullabilityForCall(methodSymbol, tree, state);
19911991
if (!methodSymbol.getTypeParameters().isEmpty()) {
19921992
genericsChecks.checkGenericMethodCallTypeArguments(tree, state);
19931993
}
19941994
}
19951995
}
19961996

19971997
// Allow handlers to override the list of non-null argument positions
1998-
argumentPositionNullness =
1998+
@Nullable Nullness[] finalArgumentPositionNullness =
19991999
handler.onOverrideMethodInvocationParametersNullability(
20002000
state.context, methodSymbol, isMethodAnnotated, argumentPositionNullness);
20012001

20022002
// now actually check the arguments
20032003
// NOTE: the case of an invocation on a possibly-null reference
20042004
// is handled by matchMemberSelect()
2005-
for (int argPos = 0; argPos < argumentPositionNullness.length; argPos++) {
2006-
boolean varargPosition = varArgsMethod && argPos == formalParams.size() - 1;
2007-
boolean argIsNonNull = Objects.equals(Nullness.NONNULL, argumentPositionNullness[argPos]);
2008-
if (!varargPosition && !argIsNonNull) {
2009-
continue;
2010-
}
2011-
ExpressionTree actual;
2012-
boolean mayActualBeNull = false;
2013-
if (varargPosition) {
2014-
// Check all vararg actual arguments for nullability
2015-
// This is the case where no actual parameter is passed for the var args parameter
2016-
// (i.e. it defaults to an empty array)
2017-
if (actualParams.size() <= argPos) {
2018-
continue;
2019-
}
2020-
actual = actualParams.get(argPos);
2021-
VarSymbol formalParamSymbol = formalParams.get(formalParams.size() - 1);
2022-
boolean isVarArgsCall = NullabilityUtil.isVarArgsCall(tree);
2023-
if (isVarArgsCall) {
2024-
// This is the case were varargs are being passed individually, as 1 or more actual
2025-
// arguments starting at the position of the var args formal.
2026-
// If the formal var args accepts `@Nullable`, then there is nothing for us to check.
2027-
if (!argIsNonNull) {
2028-
continue;
2005+
invArgs.forEach(
2006+
(actual, argPos, formalParamType, varArgsPassedAsArray) -> {
2007+
if (argPos >= formalParams.size()) {
2008+
// extra varargs argument; nullness info stored in last position
2009+
argPos = finalArgumentPositionNullness.length - 1;
20292010
}
2030-
// TODO report all varargs errors in a single build; this code only reports the first
2031-
// error
2032-
for (ExpressionTree arg : actualParams.subList(argPos, actualParams.size())) {
2033-
actual = arg;
2034-
mayActualBeNull = mayBeNullExpr(state, actual);
2035-
if (mayActualBeNull) {
2036-
break;
2011+
boolean argIsNonNull =
2012+
Objects.equals(Nullness.NONNULL, finalArgumentPositionNullness[argPos]);
2013+
boolean mayActualBeNull = false;
2014+
if (varArgsPassedAsArray) {
2015+
// This is the case where an array is explicitly passed in the position of the
2016+
// varargs parameter
2017+
// Only check for a nullable varargs array if the method is annotated, or a @NonNull
2018+
// restrictive annotation is present in legacy mode (as previously the annotation
2019+
// was applied to both the array itself and the elements), or a JetBrains @NotNull
2020+
// declaration annotation is present (due to
2021+
// https://github.com/uber/NullAway/issues/720)
2022+
VarSymbol formalParamSymbol = formalParams.get(formalParams.size() - 1);
2023+
boolean checkForNullableVarargsArray =
2024+
isMethodAnnotated
2025+
|| (config.isLegacyAnnotationLocation() && argIsNonNull)
2026+
|| NullabilityUtil.hasJetBrainsNotNullDeclarationAnnotation(formalParamSymbol);
2027+
if (checkForNullableVarargsArray) {
2028+
// If varargs array itself is not @Nullable, cannot pass @Nullable array
2029+
if (!Nullness.varargsArrayIsNullable(formalParams.get(argPos), config)) {
2030+
mayActualBeNull = mayBeNullExpr(state, actual);
2031+
}
20372032
}
2038-
}
2039-
} else {
2040-
// This is the case where an array is explicitly passed in the position of the var args
2041-
// parameter
2042-
// Only check for a nullable varargs array if the method is annotated, or a @NonNull
2043-
// restrictive annotation is present in legacy mode (as previously the annotation was
2044-
// applied to both the array itself and the elements), or a JetBrains @NotNull declaration
2045-
// annotation is present (due to https://github.com/uber/NullAway/issues/720)
2046-
boolean checkForNullableVarargsArray =
2047-
isMethodAnnotated
2048-
|| (config.isLegacyAnnotationLocation() && argIsNonNull)
2049-
|| NullabilityUtil.hasJetBrainsNotNullDeclarationAnnotation(formalParamSymbol);
2050-
if (checkForNullableVarargsArray) {
2051-
// If varargs array itself is not @Nullable, cannot pass @Nullable array
2052-
if (!Nullness.varargsArrayIsNullable(formalParams.get(argPos), config)) {
2053-
mayActualBeNull = mayBeNullExpr(state, actual);
2033+
} else {
2034+
if (!argIsNonNull) {
2035+
// argument can be @Nullable, so nothing to check
2036+
return;
20542037
}
2038+
mayActualBeNull = mayBeNullExpr(state, actual);
20552039
}
2056-
}
2057-
} else { // not the vararg position
2058-
actual = actualParams.get(argPos);
2059-
mayActualBeNull = mayBeNullExpr(state, actual);
2060-
}
2061-
if (mayActualBeNull) {
2062-
String message =
2063-
"passing @Nullable parameter '"
2064-
+ state.getSourceForNode(actual)
2065-
+ "' where @NonNull is required";
2066-
ErrorMessage errorMessage = new ErrorMessage(MessageTypes.PASS_NULLABLE, message);
2067-
state.reportMatch(
2068-
errorBuilder.createErrorDescriptionForNullAssignment(
2069-
errorMessage, actual, buildDescription(actual), state, formalParams.get(argPos)));
2070-
}
2071-
}
2040+
if (mayActualBeNull) {
2041+
String message =
2042+
"passing @Nullable parameter '"
2043+
+ state.getSourceForNode(actual)
2044+
+ "' where @NonNull is required";
2045+
ErrorMessage errorMessage = new ErrorMessage(MessageTypes.PASS_NULLABLE, message);
2046+
state.reportMatch(
2047+
errorBuilder.createErrorDescriptionForNullAssignment(
2048+
errorMessage,
2049+
actual,
2050+
buildDescription(actual),
2051+
state,
2052+
formalParams.get(argPos)));
2053+
}
2054+
});
20722055
// Check for @NonNull being passed to castToNonNull (if configured)
20732056
return checkCastToNonNullTakesNullable(tree, state, methodSymbol, actualParams);
20742057
}

0 commit comments

Comments
 (0)