From e2e53948a2569a84c5f6bea06a61b82bdca3f289 Mon Sep 17 00:00:00 2001 From: Manu Sridharan Date: Sat, 7 Sep 2024 11:54:00 -0700 Subject: [PATCH] Partial handling for restrictive annotations on varargs in unannotated code (#1029) Partially addresses #1027. Most importantly, handles the case with unannotated methods and _no_ restrictive annotations; we were getting warnings in some cases with the main branch, which impacts backward compatibility. I started implementing full support for restrictive annotations on varargs parameters and then realized it would take some refactoring (since there can be a restrictive annotation on the varargs array itself or on its contents, and we have no way to express this in the current code structure). This PR contains some incomplete code towards that change, which I figured does no harm so I left it in for now. If desired I can remove it and put it in a separate PR. --- .../main/java/com/uber/nullaway/NullAway.java | 20 +++- .../com/uber/nullaway/NullabilityUtil.java | 40 ++++++++ .../main/java/com/uber/nullaway/Nullness.java | 22 ++++- .../RestrictiveAnnotationHandler.java | 11 +-- .../com/uber/nullaway/LegacyVarargsTests.java | 87 ++++++++++++++++ .../java/com/uber/nullaway/VarargsTests.java | 98 +++++++++++++++++++ .../jspecify/JSpecifyVarargsTests.java | 85 ++++++++++++++++ 7 files changed, 347 insertions(+), 16 deletions(-) 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(