Skip to content

Commit

Permalink
Improved varargs support. (#291)
Browse files Browse the repository at this point in the history 
Resolves #290 by supporting vararg nullability.

Previously, we ignored the default nullability info of vararg arguments,
but would error out if the vararg formal was deemed non-null by a handler.

Instead, we now correctly acknowledge the non-null status of vararg arguments
whether they are non-null due to default code assumptions or due to handlers
(restrictive annotations, `@Contract`, etc).

Note that we don't actually support checking methods taking `@Nullable` var args 
(e.g. we don't support `@Nullable` var args in methods defined in annotated code).
Supporting that would require supporting nullability in generics/array elements, which 
NullAway currently punts on. Instead, we print a corresponding error in this case.

This PR also adds tests and support for `javax.annotations.Nonnull` which
we were ignoring before and is needed for those tests.
  • Loading branch information
@lazaroclapp
lazaroclapp committed Mar 29, 2019
1 parent 18b386a commit 401d048
Show file tree
Hide file tree
Showing 5 changed files with 146 additions and 12 deletions.
1 change: 1 addition & 0 deletions nullaway/src/main/java/com/uber/nullaway/ErrorMessage.java
View file
Expand Up @@ -48,6 +48,7 @@ public enum MessageTypes {
FIELD_NO_INIT,
UNBOX_NULLABLE,
NONNULL_FIELD_READ_BEFORE_INIT,
NULLABLE_VARARGS_UNSUPPORTED,
ANNOTATION_VALUE_INVALID,
CAST_TO_NONNULL_ARG_NONNULL,
GET_ON_EMPTY_OPTIONAL;
Expand Down
54 changes: 43 additions & 11 deletions nullaway/src/main/java/com/uber/nullaway/NullAway.java
View file
Expand Up @@ -32,6 +32,7 @@

import com.google.auto.service.AutoService;
import com.google.auto.value.AutoValue;
import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMultimap;
import com.google.common.collect.ImmutableSet;
Expand Down Expand Up @@ -487,10 +488,27 @@ public Description matchMethod(MethodTree tree, VisitorState state) {
if (isOverriding || !exhaustiveOverride) {
Symbol.MethodSymbol closestOverriddenMethod =
getClosestOverriddenMethod(methodSymbol, state.getTypes());
if (closestOverriddenMethod == null) {
return Description.NO_MATCH;
if (closestOverriddenMethod != null) {
return checkOverriding(closestOverriddenMethod, methodSymbol, null, state);
}
}
// Check that var args (if any) is @Nullable, as NullAway doesn't currently support this case
if (methodSymbol.isVarArgs()) {
VarSymbol varArgsSymbol =
methodSymbol.getParameters().get(methodSymbol.getParameters().size() - 1);
if (Nullness.hasNullableAnnotation(varArgsSymbol)) {
String message =
"NullAway doesn't currently support @Nullable VarArgs. "
+ "Consider removing the @Nullable annotation from "
+ varArgsSymbol.toString()
+ " in "
+ methodSymbol.toString()
+ " (this issue can cause other errors below, wherever the var args array is dereferenced)";
return errorBuilder.createErrorDescription(
new ErrorMessage(MessageTypes.NULLABLE_VARARGS_UNSUPPORTED, message),
state.getPath(),
buildDescription(tree));
}
return checkOverriding(closestOverriddenMethod, methodSymbol, null, state);
}
return Description.NO_MATCH;
}
Expand Down Expand Up @@ -1255,16 +1273,11 @@ private Description handleInvocation(
handler.onUnannotatedInvocationGetNonNullPositions(
this, state, methodSymbol, actualParams, ImmutableSet.of());
}
List<VarSymbol> formalParams = methodSymbol.getParameters();
if (nonNullPositions == null) {
ImmutableSet.Builder<Integer> builder = ImmutableSet.builder();
// compute which arguments are @NonNull
List<VarSymbol> formalParams = methodSymbol.getParameters();
for (int i = 0; i < formalParams.size(); i++) {
if (i == formalParams.size() - 1 && methodSymbol.isVarArgs()) {
// eventually, handle this case properly. I *think* a null
// array could be passed in incorrectly. For now, punt
continue;
}
VarSymbol param = formalParams.get(i);
if (param.type.isPrimitive()) {
Description unboxingCheck = doUnboxingCheck(state, actualParams.get(i));
Expand All @@ -1286,9 +1299,28 @@ private Description handleInvocation(
// NOTE: the case of an invocation on a possibly-null reference
// is handled by matchMemberSelect()
for (int argPos : nonNullPositions) {
ExpressionTree actual = null;
boolean mayActualBeNull = false;
if (argPos == formalParams.size() - 1 && methodSymbol.isVarArgs()) {
// Check all vararg actual arguments for nullability
if (actualParams.size() <= argPos) {
continue;
}
for (ExpressionTree arg : actualParams.subList(argPos, actualParams.size())) {
actual = arg;
mayActualBeNull = mayBeNullExpr(state, actual);
if (mayActualBeNull) {
break;
}
}
} else {
actual = actualParams.get(argPos);
mayActualBeNull = mayBeNullExpr(state, actual);
}
// This statement should be unreachable without assigning actual beforehand:
Preconditions.checkNotNull(actual);
// make sure we are passing a non-null value
ExpressionTree actual = actualParams.get(argPos);
if (mayBeNullExpr(state, actual)) {
if (mayActualBeNull) {
String message =
"passing @Nullable parameter '" + actual.toString() + "' where @NonNull is required";
return errorBuilder.createErrorDescriptionForNullAssignment(
Expand Down
4 changes: 3 additions & 1 deletion nullaway/src/main/java/com/uber/nullaway/Nullness.java
View file
Expand Up @@ -154,7 +154,9 @@ public static boolean isNullableAnnotation(String annotName) {
* @return true if we treat annotName as a <code>@NonNull</code> annotation, false otherwise
*/
public static boolean isNonNullAnnotation(String annotName) {
return annotName.endsWith(".NonNull") || annotName.endsWith(".NotNull");
return annotName.endsWith(".NonNull")
|| annotName.endsWith(".NotNull")
|| annotName.endsWith(".Nonnull");
}

/**
Expand Down
95 changes: 95 additions & 0 deletions nullaway/src/test/java/com/uber/nullaway/NullAwayTest.java
View file
Expand Up @@ -1675,4 +1675,99 @@ public void testVariablesInAccessPathsPositive() {
"}")
.doTest();
}

@Test
public void testNonNullVarargs() {
compilationHelper
.addSourceLines(
"Utilities.java",
"package com.uber;",
"import javax.annotation.Nullable;",
"public class Utilities {",
" public static String takesNonNullVarargs(Object o, Object... others) {",
" String s = o.toString() + \" \";",
" for (Object other : others) {",
" s += other.toString() + \" \";",
" }",
" return s;",
" }",
"}")
.addSourceLines(
"Test.java",
"package com.uber;",
"import javax.annotation.Nullable;",
"public class Test {",
" public void testNonNullVarargs(Object o1, Object o2, Object o3, @Nullable Object o4) {",
" Utilities.takesNonNullVarargs(o1, o2, o3);",
" Utilities.takesNonNullVarargs(o1);", // Empty var args passed
" // BUG: Diagnostic contains: passing @Nullable parameter 'o4' where @NonNull",
" Utilities.takesNonNullVarargs(o1, o4);",
" }",
"}")
.doTest();
}

@Test
public void testNullableVarargs() {
compilationHelper
.addSourceLines(
"Utilities.java",
"package com.uber;",
"import javax.annotation.Nullable;",
"public class Utilities {",
" // BUG: Diagnostic contains: NullAway doesn't currently support @Nullable VarArgs",
" public static String takesNullableVarargs(Object o, @Nullable Object... others) {",
" String s = o.toString() + \" \";",
" // BUG: Diagnostic contains: enhanced-for expression others is @Nullable",
" for (Object other : others) {",
" s += (other == null) ? \"(null) \" : other.toString() + \" \";",
" }",
" return s;",
" }",
"}")
.doTest();
}

@Test
public void testNonNullVarargsFromHandler() {
String generatedAnnot =
(Double.parseDouble(System.getProperty("java.specification.version")) >= 11)
? "@javax.annotation.processing.Generated"
: "@javax.annotation.Generated";
compilationHelper
.setArgs(
Arrays.asList(
"-d",
temporaryFolder.getRoot().getAbsolutePath(),
"-XepOpt:NullAway:AnnotatedPackages=com.uber",
"-XepOpt:NullAway:TreatGeneratedAsUnannotated=true",
"-XepOpt:NullAway:AcknowledgeRestrictiveAnnotations=true"))
.addSourceLines(
"Generated.java",
"package com.uber;",
"import javax.annotation.Nonnull;",
generatedAnnot + "(\"foo\")",
"public class Generated {",
" public static String takesNonNullVarargs(@Nonnull Object o, @Nonnull Object... others) {",
" String s = o.toString() + \" \";",
" for (Object other : others) {",
" s += other.toString() + \" \";",
" }",
" return s;",
" }",
"}")
.addSourceLines(
"Test.java",
"package com.uber;",
"import javax.annotation.Nullable;",
"public class Test {",
" public void testNonNullVarargs(Object o1, Object o2, Object o3, @Nullable Object o4) {",
" Generated.takesNonNullVarargs(o1, o2, o3);",
" Generated.takesNonNullVarargs(o1);", // Empty var args passed
" // BUG: Diagnostic contains: passing @Nullable parameter 'o4' where @NonNull",
" Generated.takesNonNullVarargs(o1, o4);",
" }",
"}")
.doTest();
}
}
4 changes: 4 additions & 0 deletions nullaway/src/test/resources/com/uber/nullaway/testdata/NullAwayPositiveCases.java
View file
Expand Up @@ -382,8 +382,12 @@ static void f(boolean b) {
// BUG: Diagnostic contains: unboxing
g("", b ? null : 0);
// SHOULDBUSG: Diagnostic contains: unboxing
// BUG: Diagnostic contains: passing @Nullable parameter 'b ? null : 0' where @NonNull is
// required
h("", 1, b ? null : 0);
// SHOULDBUSG: Diagnostic contains: unboxing
// BUG: Diagnostic contains: passing @Nullable parameter 'b ? null : 0' where @NonNull is
// required
h("", 1, b ? null : 0, 3);
// BUG: Diagnostic contains: unboxing
int z = 0 + (b ? null : 1);
Expand Down

0 comments on commit 401d048

Please sign in to comment.