diff --git a/nullaway/src/main/java/com/uber/nullaway/NullAway.java b/nullaway/src/main/java/com/uber/nullaway/NullAway.java index ee9236d959..546dae75c2 100644 --- a/nullaway/src/main/java/com/uber/nullaway/NullAway.java +++ b/nullaway/src/main/java/com/uber/nullaway/NullAway.java @@ -1780,17 +1780,27 @@ private Description handleInvocation( } actual = actualParams.get(argPos); // check if the varargs arguments are being passed as an array - Type.ArrayType varargsArrayType = - (Type.ArrayType) formalParams.get(formalParams.size() - 1).type; + VarSymbol formalParamSymbol = formalParams.get(formalParams.size() - 1); + Type.ArrayType varargsArrayType = (Type.ArrayType) formalParamSymbol.type; Type actualParameterType = ASTHelpers.getType(actual); if (actualParameterType != null && state.getTypes().isAssignable(actualParameterType, varargsArrayType) && actualParams.size() == argPos + 1) { // This is the case where an array is explicitly passed in the position of the var args // parameter - // If varargs array itself is not @Nullable, cannot pass @Nullable array - if (!Nullness.varargsArrayIsNullable(formalParams.get(argPos), config)) { - mayActualBeNull = mayBeNullExpr(state, actual); + // Only check for a nullable varargs array if the method is annotated, or a @NonNull + // restrictive annotation is present in legacy mode (as previously the annotation was + // applied to both the array itself and the elements), or a JetBrains @NotNull declaration + // annotation is present (due to https://github.com/uber/NullAway/issues/720) + boolean checkForNullableVarargsArray = + isMethodAnnotated + || (config.isLegacyAnnotationLocation() && argIsNonNull) + || NullabilityUtil.hasJetBrainsNotNullDeclarationAnnotation(formalParamSymbol); + if (checkForNullableVarargsArray) { + // If varargs array itself is not @Nullable, cannot pass @Nullable array + if (!Nullness.varargsArrayIsNullable(formalParams.get(argPos), config)) { + mayActualBeNull = mayBeNullExpr(state, actual); + } } } else { // This is the case were varargs are being passed individually, as 1 or more actual diff --git a/nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java b/nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java index 56d54512cf..6bae2114a0 100644 --- a/nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java +++ b/nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java @@ -64,6 +64,7 @@ public class NullabilityUtil { public static final String NULLUNMARKED_SIMPLE_NAME = "NullUnmarked"; private static final Supplier MAP_TYPE_SUPPLIER = Suppliers.typeFromString("java.util.Map"); + private static final String JETBRAINS_NOT_NULL = "org.jetbrains.annotations.NotNull"; private NullabilityUtil() {} @@ -440,4 +441,43 @@ public static boolean isArrayElementNullable(Symbol arraySymbol, Config config) } return false; } + + /** + * Checks if the given array symbol has a {@code @NonNull} annotation for its elements. + * + * @param arraySymbol The symbol of the array to check. + * @param config NullAway configuration. + * @return true if the array symbol has a {@code @NonNull} annotation for its elements, false + * otherwise + */ + public static boolean isArrayElementNonNull(Symbol arraySymbol, Config config) { + for (Attribute.TypeCompound t : arraySymbol.getRawTypeAttributes()) { + for (TypeAnnotationPosition.TypePathEntry entry : t.position.location) { + if (entry.tag == TypeAnnotationPosition.TypePathEntryKind.ARRAY) { + if (Nullness.isNonNullAnnotation(t.type.toString(), config)) { + return true; + } + } + } + } + // For varargs symbols we also consider the elements to be @NonNull if there is a @NonNull + // declaration annotation on the parameter + if ((arraySymbol.flags() & Flags.VARARGS) != 0) { + return Nullness.hasNonNullDeclarationAnnotation(arraySymbol, config); + } + return false; + } + + /** + * Does the given symbol have a JetBrains @NotNull declaration annotation? Useful for workarounds + * in light of https://github.com/uber/NullAway/issues/720 + */ + public static boolean hasJetBrainsNotNullDeclarationAnnotation(Symbol.VarSymbol varSymbol) { + // We explicitly ignore type-use annotations here, looking for @NotNull used as a + // declaration annotation, which is why this logic is simpler than e.g. + // NullabilityUtil.getAllAnnotationsForParameter. + return varSymbol.getAnnotationMirrors().stream() + .map(a -> a.getAnnotationType().toString()) + .anyMatch(annotName -> annotName.equals(JETBRAINS_NOT_NULL)); + } } diff --git a/nullaway/src/main/java/com/uber/nullaway/Nullness.java b/nullaway/src/main/java/com/uber/nullaway/Nullness.java index 56cf28dcba..576e22d852 100644 --- a/nullaway/src/main/java/com/uber/nullaway/Nullness.java +++ b/nullaway/src/main/java/com/uber/nullaway/Nullness.java @@ -175,7 +175,7 @@ public static boolean isNullableAnnotation(String annotName, Config config) { * @param annotName annotation name * @return true if we treat annotName as a @NonNull annotation, false otherwise */ - private static boolean isNonNullAnnotation(String annotName, Config config) { + public static boolean isNonNullAnnotation(String annotName, Config config) { return annotName.endsWith(".NonNull") || annotName.endsWith(".NotNull") || annotName.endsWith(".Nonnull") @@ -260,11 +260,22 @@ private static boolean isRecordEqualsParam(Symbol.MethodSymbol symbol, int param /** * Does the parameter of {@code symbol} at {@code paramInd} have a {@code @NonNull} declaration or * type-use annotation? This method works for methods defined in either source or class files. + * + *

For a varargs parameter, this method returns true if individual arguments passed in + * the varargs positions must be {@code @NonNull}. */ public static boolean paramHasNonNullAnnotation( Symbol.MethodSymbol symbol, int paramInd, Config config) { - return hasNonNullAnnotation( - NullabilityUtil.getAllAnnotationsForParameter(symbol, paramInd, config), config); + if (symbol.isVarArgs() + && paramInd == symbol.getParameters().size() - 1 + && !config.isLegacyAnnotationLocation()) { + // individual arguments passed in the varargs positions must be @NonNull if the array element + // type of the parameter is @NonNull + return NullabilityUtil.isArrayElementNonNull(symbol.getParameters().get(paramInd), config); + } else { + return hasNonNullAnnotation( + NullabilityUtil.getAllAnnotationsForParameter(symbol, paramInd, config), config); + } } /** @@ -282,4 +293,9 @@ public static boolean varargsArrayIsNullable(Symbol paramSymbol, Config config) public static boolean hasNullableDeclarationAnnotation(Symbol symbol, Config config) { return hasNullableAnnotation(symbol.getRawAttributes().stream(), config); } + + /** Checks if the symbol has a {@code @NonNull} declaration annotation */ + public static boolean hasNonNullDeclarationAnnotation(Symbol symbol, Config config) { + return hasNonNullAnnotation(symbol.getRawAttributes().stream(), config); + } } diff --git a/nullaway/src/main/java/com/uber/nullaway/handlers/RestrictiveAnnotationHandler.java b/nullaway/src/main/java/com/uber/nullaway/handlers/RestrictiveAnnotationHandler.java index ef04e8bf91..f668394b56 100644 --- a/nullaway/src/main/java/com/uber/nullaway/handlers/RestrictiveAnnotationHandler.java +++ b/nullaway/src/main/java/com/uber/nullaway/handlers/RestrictiveAnnotationHandler.java @@ -31,6 +31,7 @@ import com.uber.nullaway.CodeAnnotationInfo; import com.uber.nullaway.Config; import com.uber.nullaway.NullAway; +import com.uber.nullaway.NullabilityUtil; import com.uber.nullaway.Nullness; import com.uber.nullaway.dataflow.AccessPath; import com.uber.nullaway.dataflow.AccessPathNullnessPropagation; @@ -40,8 +41,6 @@ public class RestrictiveAnnotationHandler extends BaseNoOpHandler { - private static final String JETBRAINS_NOT_NULL = "org.jetbrains.annotations.NotNull"; - private final Config config; RestrictiveAnnotationHandler(Config config) { @@ -113,13 +112,9 @@ public Nullness[] onOverrideMethodInvocationParametersNullability( if (methodSymbol.isVarArgs() && i == methodSymbol.getParameters().size() - 1) { // Special handling: ignore org.jetbrains.annotations.NotNull on varargs parameters // to handle kotlinc generated jars (see #720) - // We explicitly ignore type-use annotations here, looking for @NotNull used as a - // declaration annotation, which is why this logic is simpler than e.g. - // NullabilityUtil.getAllAnnotationsForParameter. + Symbol.VarSymbol varargsParamSymbol = methodSymbol.getParameters().get(i); boolean jetBrainsNotNullAnnotated = - methodSymbol.getParameters().get(i).getAnnotationMirrors().stream() - .map(a -> a.getAnnotationType().toString()) - .anyMatch(annotName -> annotName.equals(JETBRAINS_NOT_NULL)); + NullabilityUtil.hasJetBrainsNotNullDeclarationAnnotation(varargsParamSymbol); if (jetBrainsNotNullAnnotated) { continue; } diff --git a/nullaway/src/test/java/com/uber/nullaway/LegacyVarargsTests.java b/nullaway/src/test/java/com/uber/nullaway/LegacyVarargsTests.java index 038c37baa8..a42a0a5404 100644 --- a/nullaway/src/test/java/com/uber/nullaway/LegacyVarargsTests.java +++ b/nullaway/src/test/java/com/uber/nullaway/LegacyVarargsTests.java @@ -416,6 +416,93 @@ public void varargsPassArrayAndOtherArgs() { .doTest(); } + @Test + public void testVarargsNullArrayUnannotated() { + defaultCompilationHelper + .addSourceLines( + "Unannotated.java", + "package foo.unannotated;", + "public class Unannotated {", + " public static void takesVarargs(Object... args) {}", + "}") + .addSourceLines( + "Test.java", + "package com.uber;", + "import foo.unannotated.Unannotated;", + "public class Test {", + " public void test() {", + " Object[] x = null;", + " Unannotated.takesVarargs(x);", + " }", + "}") + .doTest(); + } + + /** + * This test is a WIP for restrictive annotations on varargs. More assertions still need to be + * written, and more support needs to be implemented. + */ + @Test + public void testVarargsRestrictive() { + makeTestHelperWithArgs( + Arrays.asList( + "-d", + temporaryFolder.getRoot().getAbsolutePath(), + "-XepOpt:NullAway:AnnotatedPackages=com.uber", + "-XepOpt:NullAway:LegacyAnnotationLocations=true", + "-XepOpt:NullAway:AcknowledgeRestrictiveAnnotations=true")) + .addSourceLines( + "NonNull.java", + "package com.uber.both;", + "import java.lang.annotation.ElementType;", + "import java.lang.annotation.Target;", + "@Target({ElementType.METHOD, ElementType.FIELD, ElementType.PARAMETER, ElementType.LOCAL_VARIABLE, ElementType.TYPE_USE})", + "public @interface NonNull {}") + .addSourceLines( + "Unannotated.java", + "package foo.unannotated;", + "public class Unannotated {", + " public static void takesVarargsDeclaration(@javax.annotation.Nonnull Object... args) {}", + " public static void takesVarargsTypeUseOnArray(Object @org.jspecify.annotations.NonNull... args) {}", + " public static void takesVarargsTypeUseOnElements(@org.jspecify.annotations.NonNull Object... args) {}", + " public static void takesVarargsTypeUseOnBoth(@org.jspecify.annotations.NonNull Object @org.jspecify.annotations.NonNull... args) {}", + " public static void takesVarargsBothOnArray(Object @com.uber.both.NonNull... args) {}", + " public static void takesVarargsBothOnElements(@com.uber.both.NonNull Object... args) {}", + " public static void takesVarargsBothOnBoth(@com.uber.both.NonNull Object @com.uber.both.NonNull... args) {}", + "}") + .addSourceLines( + "Test.java", + "package com.uber;", + "import foo.unannotated.Unannotated;", + "public class Test {", + " public void testDeclaration() {", + " Object x = null;", + " Object[] y = null;", + " // BUG: Diagnostic contains: passing @Nullable parameter 'x'", + " Unannotated.takesVarargsDeclaration(x);", + " // BUG: Diagnostic contains: passing @Nullable parameter 'y'", + " Unannotated.takesVarargsDeclaration(y);", + " }", + " public void testTypeUseOnArray() {", + " Object x = null;", + " Object[] y = null;", + " // BUG: Diagnostic contains: passing @Nullable parameter 'x'", + " Unannotated.takesVarargsTypeUseOnArray(x);", + " // BUG: Diagnostic contains: passing @Nullable parameter 'y'", + " Unannotated.takesVarargsTypeUseOnArray(y);", + " }", + " public void testTypeUseOnElements() {", + " Object x = null;", + " Object[] y = null;", + " // BUG: Diagnostic contains: passing @Nullable parameter 'x'", + " Unannotated.takesVarargsTypeUseOnElements(x);", + " // BUG: Diagnostic contains: passing @Nullable parameter 'y'", + " Unannotated.takesVarargsTypeUseOnElements(y);", + " }", + "}") + .doTest(); + } + private CompilationTestHelper makeHelper() { return makeTestHelperWithArgs( Arrays.asList( diff --git a/nullaway/src/test/java/com/uber/nullaway/VarargsTests.java b/nullaway/src/test/java/com/uber/nullaway/VarargsTests.java index c6638a8f3f..da142b6168 100644 --- a/nullaway/src/test/java/com/uber/nullaway/VarargsTests.java +++ b/nullaway/src/test/java/com/uber/nullaway/VarargsTests.java @@ -417,4 +417,102 @@ public void varargsPassArrayAndOtherArgs() { "}") .doTest(); } + + @Test + public void testVarargsNullArrayUnannotated() { + String[] unannotatedSource = { + "package foo.unannotated;", + "public class Unannotated {", + " public static void takesVarargs(Object... args) {}", + "}" + }; + String[] testSource = { + "package com.uber;", + "import foo.unannotated.Unannotated;", + "public class Test {", + " public void test() {", + " Object x = null;", + " Object[] y = null;", + " Unannotated.takesVarargs(x);", + " Unannotated.takesVarargs(y);", + " }", + "}" + }; + // test with both restrictive annotations enabled and disabled + defaultCompilationHelper + .addSourceLines("Unannotated.java", unannotatedSource) + .addSourceLines("Test.java", testSource) + .doTest(); + makeTestHelperWithArgs( + Arrays.asList( + "-d", + temporaryFolder.getRoot().getAbsolutePath(), + "-XepOpt:NullAway:AnnotatedPackages=com.uber", + "-XepOpt:NullAway:AcknowledgeRestrictiveAnnotations=true")) + .addSourceLines("Unannotated.java", unannotatedSource) + .addSourceLines("Test.java", testSource) + .doTest(); + } + + /** + * This test is a WIP for restrictive annotations on varargs. More assertions still need to be + * written, and more support needs to be implemented. + */ + @Test + public void testVarargsRestrictive() { + makeTestHelperWithArgs( + Arrays.asList( + "-d", + temporaryFolder.getRoot().getAbsolutePath(), + "-XepOpt:NullAway:AnnotatedPackages=com.uber", + "-XepOpt:NullAway:AcknowledgeRestrictiveAnnotations=true")) + .addSourceLines( + "NonNull.java", + "package com.uber.both;", + "import java.lang.annotation.ElementType;", + "import java.lang.annotation.Target;", + "@Target({ElementType.METHOD, ElementType.FIELD, ElementType.PARAMETER, ElementType.LOCAL_VARIABLE, ElementType.TYPE_USE})", + "public @interface NonNull {}") + .addSourceLines( + "Unannotated.java", + "package foo.unannotated;", + "public class Unannotated {", + " public static void takesVarargsDeclaration(@javax.annotation.Nonnull Object... args) {}", + " public static void takesVarargsTypeUseOnArray(Object @org.jspecify.annotations.NonNull... args) {}", + " public static void takesVarargsTypeUseOnElements(@org.jspecify.annotations.NonNull Object... args) {}", + " public static void takesVarargsTypeUseOnBoth(@org.jspecify.annotations.NonNull Object @org.jspecify.annotations.NonNull... args) {}", + " public static void takesVarargsBothOnArray(Object @com.uber.both.NonNull... args) {}", + " public static void takesVarargsBothOnElements(@com.uber.both.NonNull Object... args) {}", + " public static void takesVarargsBothOnBoth(@com.uber.both.NonNull Object @com.uber.both.NonNull... args) {}", + "}") + .addSourceLines( + "Test.java", + "package com.uber;", + "import foo.unannotated.Unannotated;", + "public class Test {", + " public void testDeclaration() {", + " Object x = null;", + " Object[] y = null;", + " // BUG: Diagnostic contains: passing @Nullable parameter 'x'", + " Unannotated.takesVarargsDeclaration(x);", + " Unannotated.takesVarargsDeclaration(y);", + " }", + " public void testTypeUseOnArray() {", + " Object x = null;", + " Object[] y = null;", + " Unannotated.takesVarargsTypeUseOnArray(x);", + // TODO report an error here; will require some refactoring of restrictive annotation + // handling + " Unannotated.takesVarargsTypeUseOnArray(y);", + " }", + " public void testTypeUseOnElements() {", + " Object x = null;", + " Object[] y = null;", + " // BUG: Diagnostic contains: passing @Nullable parameter 'x'", + " Unannotated.takesVarargsTypeUseOnElements(x);", + " Unannotated.takesVarargsTypeUseOnElements(y);", + " }", + "}") + .doTest(); + } } diff --git a/nullaway/src/test/java/com/uber/nullaway/jspecify/JSpecifyVarargsTests.java b/nullaway/src/test/java/com/uber/nullaway/jspecify/JSpecifyVarargsTests.java index 6ff2f2d754..a72124ffa3 100644 --- a/nullaway/src/test/java/com/uber/nullaway/jspecify/JSpecifyVarargsTests.java +++ b/nullaway/src/test/java/com/uber/nullaway/jspecify/JSpecifyVarargsTests.java @@ -431,6 +431,91 @@ public void varargsPassArrayAndOtherArgs() { .doTest(); } + @Test + public void testVarargsNullArrayUnannotated() { + defaultCompilationHelper + .addSourceLines( + "Unannotated.java", + "package foo.unannotated;", + "public class Unannotated {", + " public static void takesVarargs(Object... args) {}", + "}") + .addSourceLines( + "Test.java", + "package com.uber;", + "import foo.unannotated.Unannotated;", + "public class Test {", + " public void test() {", + " Object[] x = null;", + " Unannotated.takesVarargs(x);", + " }", + "}") + .doTest(); + } + + /** + * This test is a WIP for restrictive annotations on varargs. More assertions still need to be + * written, and more support needs to be implemented. + */ + @Test + public void testVarargsRestrictive() { + makeTestHelperWithArgs( + Arrays.asList( + "-d", + temporaryFolder.getRoot().getAbsolutePath(), + "-XepOpt:NullAway:AnnotatedPackages=com.uber", + "-XepOpt:NullAway:JSpecifyMode=true", + "-XepOpt:NullAway:AcknowledgeRestrictiveAnnotations=true")) + .addSourceLines( + "NonNull.java", + "package com.uber.both;", + "import java.lang.annotation.ElementType;", + "import java.lang.annotation.Target;", + "@Target({ElementType.METHOD, ElementType.FIELD, ElementType.PARAMETER, ElementType.LOCAL_VARIABLE, ElementType.TYPE_USE})", + "public @interface NonNull {}") + .addSourceLines( + "Unannotated.java", + "package foo.unannotated;", + "public class Unannotated {", + " public static void takesVarargsDeclaration(@javax.annotation.Nonnull Object... args) {}", + " public static void takesVarargsTypeUseOnArray(Object @org.jspecify.annotations.NonNull... args) {}", + " public static void takesVarargsTypeUseOnElements(@org.jspecify.annotations.NonNull Object... args) {}", + " public static void takesVarargsTypeUseOnBoth(@org.jspecify.annotations.NonNull Object @org.jspecify.annotations.NonNull... args) {}", + " public static void takesVarargsBothOnArray(Object @com.uber.both.NonNull... args) {}", + " public static void takesVarargsBothOnElements(@com.uber.both.NonNull Object... args) {}", + " public static void takesVarargsBothOnBoth(@com.uber.both.NonNull Object @com.uber.both.NonNull... args) {}", + "}") + .addSourceLines( + "Test.java", + "package com.uber;", + "import foo.unannotated.Unannotated;", + "public class Test {", + " public void testDeclaration() {", + " Object x = null;", + " Object[] y = null;", + " // BUG: Diagnostic contains: passing @Nullable parameter 'x'", + " Unannotated.takesVarargsDeclaration(x);", + " Unannotated.takesVarargsDeclaration(y);", + " }", + " public void testTypeUseOnArray() {", + " Object x = null;", + " Object[] y = null;", + " Unannotated.takesVarargsTypeUseOnArray(x);", + // TODO report an error here; will require some refactoring of restrictive annotation + // handling + " Unannotated.takesVarargsTypeUseOnArray(y);", + " }", + " public void testTypeUseOnElements() {", + " Object x = null;", + " Object[] y = null;", + " // BUG: Diagnostic contains: passing @Nullable parameter 'x'", + " Unannotated.takesVarargsTypeUseOnElements(x);", + " Unannotated.takesVarargsTypeUseOnElements(y);", + " }", + "}") + .doTest(); + } + private CompilationTestHelper makeHelper() { return makeTestHelperWithArgs( Arrays.asList(