Skip to content

Commit 4987a2e

Browse files
msridharmsridhar
authored
JSpecify: handle varargs whose element type is a type variable (#1200)
Partially addresses #1199. After substituting for a type variable, we were not retrieving the correct nullability for a varargs parameter, which should be the nullability of the component, not the array type itself. Also adds some simplification of our handling of varargs calls based on a tip from @jeffrey-easyesi
1 parent 647e77e commit 4987a2e

File tree

6 files changed

+197
-25
lines changed

6 files changed

+197
-25
lines changed

‎guava-recent-unit-tests/build.gradle

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,7 @@ dependencies {
3333
exclude group: "junit", module: "junit"
3434
}
3535
testImplementation deps.test.jsr305Annotations
36-
testImplementation "com.google.guava:guava:33.4.5-jre"
36+
testImplementation "com.google.guava:guava:33.4.8-jre"
3737

3838
errorProneOldest deps.build.errorProneCheckApiOld
3939
errorProneOldest(deps.build.errorProneTestHelpersOld) {

‎guava-recent-unit-tests/src/test/java/com/uber/nullaway/guava/NullAwayGuavaParametricNullnessTests.java

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,13 @@ public void setup() {
4545
Arrays.asList(
4646
"-d",
4747
temporaryFolder.getRoot().getAbsolutePath(),
48+
// Since Guava is now @NullMarked we shouldn't need com.google.common in the
49+
// annotated packages list.
50+
// We still need it here since we are still trying to support
51+
// Error Prone 2.14.0, on which reading @NullMarked from the package-info.java
52+
// file isn't working using ASTHelpers for some reason.
53+
// TODO Once we bump our minimum Error Prone version, remove com.google.common
54+
// from this list.
4855
"-XepOpt:NullAway:AnnotatedPackages=com.uber,com.google.common",
4956
"-XepOpt:NullAway:UnannotatedSubPackages=com.uber.nullaway.[a-zA-Z0-9.]+.unannotated"));
5057
jspecifyCompilationHelper =
@@ -241,4 +248,23 @@ public void testFunctionMethodOverride() {
241248
"}")
242249
.doTest();
243250
}
251+
252+
@Test
253+
public void newHashSetPassingNullable() {
254+
// to ensure javac reads proper generic types from the Guava jar
255+
Assume.assumeTrue(Runtime.version().feature() >= 23);
256+
jspecifyCompilationHelper
257+
.addSourceLines(
258+
"Test.java",
259+
"import com.google.common.collect.Sets;",
260+
"import java.util.Set;",
261+
"import org.jspecify.annotations.*;",
262+
"@NullMarked",
263+
"class Test {",
264+
" public static void test(@Nullable String s) {",
265+
" Set<@Nullable String> params = Sets.newHashSet(s);",
266+
" }",
267+
"}")
268+
.doTest();
269+
}
244270
}

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

Lines changed: 35 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -1940,13 +1940,25 @@ private Description handleInvocation(
19401940
continue;
19411941
}
19421942
actual = actualParams.get(argPos);
1943-
// check if the varargs arguments are being passed as an array
19441943
VarSymbol formalParamSymbol = formalParams.get(formalParams.size() - 1);
1945-
Type.ArrayType varargsArrayType = (Type.ArrayType) formalParamSymbol.type;
1946-
Type actualParameterType = ASTHelpers.getType(actual);
1947-
if (actualParameterType != null
1948-
&& state.getTypes().isAssignable(actualParameterType, varargsArrayType)
1949-
&& actualParams.size() == argPos + 1) {
1944+
boolean isVarArgsCall = isVarArgsCall(tree);
1945+
if (isVarArgsCall) {
1946+
// This is the case were varargs are being passed individually, as 1 or more actual
1947+
// arguments starting at the position of the var args formal.
1948+
// If the formal var args accepts `@Nullable`, then there is nothing for us to check.
1949+
if (!argIsNonNull) {
1950+
continue;
1951+
}
1952+
// TODO report all varargs errors in a single build; this code only reports the first
1953+
// error
1954+
for (ExpressionTree arg : actualParams.subList(argPos, actualParams.size())) {
1955+
actual = arg;
1956+
mayActualBeNull = mayBeNullExpr(state, actual);
1957+
if (mayActualBeNull) {
1958+
break;
1959+
}
1960+
}
1961+
} else {
19501962
// This is the case where an array is explicitly passed in the position of the var args
19511963
// parameter
19521964
// Only check for a nullable varargs array if the method is annotated, or a @NonNull
@@ -1963,22 +1975,6 @@ private Description handleInvocation(
19631975
mayActualBeNull = mayBeNullExpr(state, actual);
19641976
}
19651977
}
1966-
} else {
1967-
// This is the case were varargs are being passed individually, as 1 or more actual
1968-
// arguments starting at the position of the var args formal.
1969-
// If the formal var args accepts `@Nullable`, then there is nothing for us to check.
1970-
if (!argIsNonNull) {
1971-
continue;
1972-
}
1973-
// TODO report all varargs errors in a single build; this code only reports the first
1974-
// error
1975-
for (ExpressionTree arg : actualParams.subList(argPos, actualParams.size())) {
1976-
actual = arg;
1977-
mayActualBeNull = mayBeNullExpr(state, actual);
1978-
if (mayActualBeNull) {
1979-
break;
1980-
}
1981-
}
19821978
}
19831979
} else { // not the vararg position
19841980
actual = actualParams.get(argPos);
@@ -1999,6 +1995,23 @@ private Description handleInvocation(
19991995
return checkCastToNonNullTakesNullable(tree, state, methodSymbol, actualParams);
20001996
}
20011997

1998+
/**
1999+
* Checks if the method invocation is a varargs call, i.e., if individual arguments are being
2000+
* passed in the varargs position. If false, it means that an array is being passed in the varargs
2001+
* position.
2002+
*
2003+
* @param tree the method invocation tree (MethodInvocationTree or NewClassTree)
2004+
* @return true if the method invocation is a varargs call, false otherwise
2005+
*/
2006+
private boolean isVarArgsCall(Tree tree) {
2007+
// javac sets the varargsElement field to a non-null value if the invocation is a varargs call
2008+
Type varargsElement =
2009+
tree instanceof JCTree.JCMethodInvocation
2010+
? ((JCTree.JCMethodInvocation) tree).varargsElement
2011+
: ((JCTree.JCNewClass) tree).varargsElement;
2012+
return varargsElement != null;
2013+
}
2014+
20022015
private Description checkCastToNonNullTakesNullable(
20032016
Tree tree,
20042017
VisitorState state,

‎nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java

Lines changed: 37 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1015,6 +1015,9 @@ public Nullness getGenericParameterNullnessAtInvocation(
10151015
MethodInvocationTree tree,
10161016
VisitorState state,
10171017
Config config) {
1018+
boolean isVarargsParam =
1019+
invokedMethodSymbol.isVarArgs()
1020+
&& paramIndex == invokedMethodSymbol.getParameters().size() - 1;
10181021
// If generic method invocation
10191022
if (!invokedMethodSymbol.getTypeParameters().isEmpty()) {
10201023
// Substitute the argument types within the MethodType
@@ -1025,7 +1028,9 @@ public Nullness getGenericParameterNullnessAtInvocation(
10251028
// type variables declared on the enclosing class
10261029
if (substitutedParamTypes != null
10271030
&& Objects.equals(
1028-
getTypeNullness(substitutedParamTypes.get(paramIndex), config), Nullness.NULLABLE)) {
1031+
getParameterTypeNullness(
1032+
substitutedParamTypes.get(paramIndex), config, isVarargsParam),
1033+
Nullness.NULLABLE)) {
10291034
return Nullness.NULLABLE;
10301035
}
10311036
}
@@ -1105,10 +1110,13 @@ public static Nullness getGenericMethodParameterNullness(
11051110
// @Nullable annotation is handled elsewhere)
11061111
return Nullness.NONNULL;
11071112
}
1113+
boolean isVarargsParam =
1114+
method.isVarArgs() && parameterIndex == method.getParameters().size() - 1;
1115+
11081116
Type methodType =
11091117
TypeSubstitutionUtils.memberType(state.getTypes(), enclosingType, method, config);
11101118
Type paramType = methodType.getParameterTypes().get(parameterIndex);
1111-
return getTypeNullness(paramType, config);
1119+
return getParameterTypeNullness(paramType, config, isVarargsParam);
11121120
}
11131121

11141122
/**
@@ -1171,6 +1179,33 @@ private static void checkTypeParameterNullnessForOverridingMethodReturnType(
11711179
}
11721180
}
11731181

1182+
/**
1183+
* Returns the nullness of a formal parameter type, based on the nullability annotations on the
1184+
* type.
1185+
*
1186+
* @param type The type of the parameter
1187+
* @param config The analysis config
1188+
* @param isVarargsParam true if the parameter is a varargs parameter
1189+
* @return The nullness of the parameter type
1190+
*/
1191+
private static Nullness getParameterTypeNullness(
1192+
Type type, Config config, boolean isVarargsParam) {
1193+
if (isVarargsParam) {
1194+
// type better be an array type
1195+
verify(
1196+
type.getKind().equals(TypeKind.ARRAY),
1197+
"expected array type for varargs parameter, got %s",
1198+
type);
1199+
// use the component type to determine nullness
1200+
Type.ArrayType arrayType = (Type.ArrayType) type;
1201+
Type componentType = arrayType.getComponentType();
1202+
return getTypeNullness(componentType, config);
1203+
} else {
1204+
// For non-varargs, we just check the type itself
1205+
return getTypeNullness(type, config);
1206+
}
1207+
}
1208+
11741209
/**
11751210
* @param type A type for which we need the Nullness.
11761211
* @param config The analysis config

‎nullaway/src/test/java/com/uber/nullaway/jspecify/GenericMethodTests.java

Lines changed: 72 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -526,6 +526,78 @@ public void issue1178() {
526526
.doTest();
527527
}
528528

529+
@Test
530+
public void varargsOfGenericType() {
531+
makeHelper()
532+
.addSourceLines(
533+
"Varargs.java",
534+
"import org.jspecify.annotations.NullMarked;",
535+
"import org.jspecify.annotations.Nullable;",
536+
"@NullMarked",
537+
"public class Varargs {",
538+
" static <T extends @Nullable Object> void foo(T... args) {",
539+
" }",
540+
" static void testNegative(@Nullable String s) {",
541+
" Varargs.<@Nullable String>foo(s);",
542+
" }",
543+
" static void testPositive(@Nullable String s) {",
544+
" // BUG: Diagnostic contains: passing @Nullable parameter",
545+
" Varargs.<String>foo(s);",
546+
" }",
547+
"}")
548+
.doTest();
549+
}
550+
551+
@Test
552+
public void nullableVarargsArray() {
553+
makeHelper()
554+
.addSourceLines(
555+
"Test.java",
556+
"import org.jspecify.annotations.Nullable;",
557+
"import org.jspecify.annotations.NullMarked;",
558+
"@NullMarked",
559+
"class Test {",
560+
" <T extends Object> void varargsTest(T @Nullable... args) {}",
561+
" void f() {",
562+
" String[] x = null;",
563+
" this.<String>varargsTest(x);",
564+
" this.<String>varargsTest((String[])null);",
565+
" }",
566+
"}")
567+
.doTest();
568+
}
569+
570+
@Test
571+
public void varargsConstructor() {
572+
makeHelper()
573+
.addSourceLines(
574+
"Test.java",
575+
"import org.jspecify.annotations.Nullable;",
576+
"import org.jspecify.annotations.NullMarked;",
577+
"@NullMarked",
578+
"class Test {",
579+
" static class Foo {",
580+
" <T> Foo(T @Nullable... args) {}",
581+
" }",
582+
" void testNegative() {",
583+
" String[] x = null;",
584+
" Foo f = new <String>Foo(x);",
585+
" f = new <String>Foo((String[])null);",
586+
" }",
587+
" static class Bar {",
588+
" <T> Bar(T... args) {}",
589+
" }",
590+
" void testPositive() {",
591+
" String[] x = null;",
592+
" // BUG: Diagnostic contains: passing @Nullable parameter",
593+
" Bar b = new <String>Bar(x);",
594+
" // BUG: Diagnostic contains: passing @Nullable parameter",
595+
" b = new <String>Bar((String[])null);",
596+
" }",
597+
"}")
598+
.doTest();
599+
}
600+
529601
private CompilationTestHelper makeHelper() {
530602
return makeTestHelperWithArgs(
531603
Arrays.asList(

‎nullaway/src/test/java/com/uber/nullaway/jspecify/GenericsTests.java

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2353,6 +2353,32 @@ public void issue1156() {
23532353
.doTest();
23542354
}
23552355

2356+
@Test
2357+
public void varargsOfGenericType() {
2358+
makeHelper()
2359+
.addSourceLines(
2360+
"Varargs.java",
2361+
"import org.jspecify.annotations.NullMarked;",
2362+
"import org.jspecify.annotations.Nullable;",
2363+
"@NullMarked",
2364+
"public class Varargs {",
2365+
" static class Foo<T extends @Nullable Object> {",
2366+
" void foo(T... args) {",
2367+
" }",
2368+
" }",
2369+
" static void testNegative(@Nullable String s) {",
2370+
" Foo<@Nullable String> f = new Foo<>();",
2371+
" f.foo(s);",
2372+
" }",
2373+
" static void testPositive(@Nullable String s) {",
2374+
" Foo<String> f = new Foo<>();",
2375+
" // BUG: Diagnostic contains: passing @Nullable parameter",
2376+
" f.foo(s);",
2377+
" }",
2378+
"}")
2379+
.doTest();
2380+
}
2381+
23562382
private CompilationTestHelper makeHelper() {
23572383
return makeTestHelperWithArgs(
23582384
Arrays.asList(

0 commit comments

Comments
 (0)