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

Improved varargs support. #291

Merged
merged 3 commits into from Mar 29, 2019
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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions nullaway/src/main/java/com/uber/nullaway/ErrorMessage.java
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
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
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
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();
}
}
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