From 401d048bb15f1dbbb2b51fd758218557c338701b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?L=C3=A1zaro=20Clapp?= Date: Fri, 29 Mar 2019 11:32:04 -0700 Subject: [PATCH] Improved varargs support. (#291) 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. --- .../java/com/uber/nullaway/ErrorMessage.java | 1 + .../main/java/com/uber/nullaway/NullAway.java | 54 ++++++++--- .../main/java/com/uber/nullaway/Nullness.java | 4 +- .../java/com/uber/nullaway/NullAwayTest.java | 95 +++++++++++++++++++ .../testdata/NullAwayPositiveCases.java | 4 + 5 files changed, 146 insertions(+), 12 deletions(-) diff --git a/nullaway/src/main/java/com/uber/nullaway/ErrorMessage.java b/nullaway/src/main/java/com/uber/nullaway/ErrorMessage.java index 15d1910cb6..6b12e8c680 100644 --- a/nullaway/src/main/java/com/uber/nullaway/ErrorMessage.java +++ b/nullaway/src/main/java/com/uber/nullaway/ErrorMessage.java @@ -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; diff --git a/nullaway/src/main/java/com/uber/nullaway/NullAway.java b/nullaway/src/main/java/com/uber/nullaway/NullAway.java index e0c357cc87..5c527030d7 100644 --- a/nullaway/src/main/java/com/uber/nullaway/NullAway.java +++ b/nullaway/src/main/java/com/uber/nullaway/NullAway.java @@ -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; @@ -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; } @@ -1255,16 +1273,11 @@ private Description handleInvocation( handler.onUnannotatedInvocationGetNonNullPositions( this, state, methodSymbol, actualParams, ImmutableSet.of()); } + List formalParams = methodSymbol.getParameters(); if (nonNullPositions == null) { ImmutableSet.Builder builder = ImmutableSet.builder(); // compute which arguments are @NonNull - List 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)); @@ -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( diff --git a/nullaway/src/main/java/com/uber/nullaway/Nullness.java b/nullaway/src/main/java/com/uber/nullaway/Nullness.java index e907f824e0..d3b2eb911a 100644 --- a/nullaway/src/main/java/com/uber/nullaway/Nullness.java +++ b/nullaway/src/main/java/com/uber/nullaway/Nullness.java @@ -154,7 +154,9 @@ public static boolean isNullableAnnotation(String annotName) { * @return true if we treat annotName as a @NonNull 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"); } /** diff --git a/nullaway/src/test/java/com/uber/nullaway/NullAwayTest.java b/nullaway/src/test/java/com/uber/nullaway/NullAwayTest.java index 641854f783..8e1bb802e4 100644 --- a/nullaway/src/test/java/com/uber/nullaway/NullAwayTest.java +++ b/nullaway/src/test/java/com/uber/nullaway/NullAwayTest.java @@ -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(); + } } diff --git a/nullaway/src/test/resources/com/uber/nullaway/testdata/NullAwayPositiveCases.java b/nullaway/src/test/resources/com/uber/nullaway/testdata/NullAwayPositiveCases.java index 0418a51cf1..8d5d2ae69b 100644 --- a/nullaway/src/test/resources/com/uber/nullaway/testdata/NullAwayPositiveCases.java +++ b/nullaway/src/test/resources/com/uber/nullaway/testdata/NullAwayPositiveCases.java @@ -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);