From d700d1a071cb54905d4e06f7ba68fe2bfd67abb0 Mon Sep 17 00:00:00 2001 From: Manu Sridharan Date: Tue, 8 Oct 2024 18:00:35 -0700 Subject: [PATCH 01/19] test case --- .../java/com/uber/nullaway/VarargsTests.java | 16 ++++++++++++++++ test-java-lib/build.gradle | 1 + .../src/main/java/com/uber/lib/Varargs.java | 8 ++++++++ 3 files changed, 25 insertions(+) create mode 100644 test-java-lib/src/main/java/com/uber/lib/Varargs.java diff --git a/nullaway/src/test/java/com/uber/nullaway/VarargsTests.java b/nullaway/src/test/java/com/uber/nullaway/VarargsTests.java index da142b6168..45076dc846 100644 --- a/nullaway/src/test/java/com/uber/nullaway/VarargsTests.java +++ b/nullaway/src/test/java/com/uber/nullaway/VarargsTests.java @@ -71,6 +71,22 @@ public void testNullableVarargs() { .doTest(); } + @Test + public void nullableVarArgsFromBytecode() { + defaultCompilationHelper + .addSourceLines( + "Test.java", + "package com.uber;", + "import com.uber.lib.Varargs;", + "public class Test {", + " public void testStrings(RuntimeException e) {", + " String x = null;", + " Varargs s = new Varargs(x);", + " }", + "}") + .doTest(); + } + @Test public void nullableTypeUseVarargs() { defaultCompilationHelper diff --git a/test-java-lib/build.gradle b/test-java-lib/build.gradle index 6be5a65469..369889f43e 100644 --- a/test-java-lib/build.gradle +++ b/test-java-lib/build.gradle @@ -22,6 +22,7 @@ plugins { dependencies { annotationProcessor project(":nullaway") + implementation deps.test.jsr305Annotations implementation deps.build.jspecify compileOnly deps.build.javaxValidation diff --git a/test-java-lib/src/main/java/com/uber/lib/Varargs.java b/test-java-lib/src/main/java/com/uber/lib/Varargs.java new file mode 100644 index 0000000000..2d80dcfbda --- /dev/null +++ b/test-java-lib/src/main/java/com/uber/lib/Varargs.java @@ -0,0 +1,8 @@ +package com.uber.lib; + +import javax.annotation.Nullable; + +public class Varargs { + + public Varargs(@Nullable String... args) {} +} From 2ba7a8261b31314b9909e80edba8b4405fb2352d Mon Sep 17 00:00:00 2001 From: Manu Sridharan Date: Tue, 8 Oct 2024 18:29:17 -0700 Subject: [PATCH 02/19] initial fix --- .../java/com/uber/nullaway/NullabilityUtil.java | 14 ++++++++++++++ .../src/main/java/com/uber/nullaway/Nullness.java | 6 ++++-- .../dataflow/CoreNullnessStoreInitializer.java | 1 + 3 files changed, 19 insertions(+), 2 deletions(-) diff --git a/nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java b/nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java index 6e306851c5..8f47616e73 100644 --- a/nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java +++ b/nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java @@ -456,12 +456,19 @@ public static boolean isArrayElementNullable(Symbol arraySymbol, Config config) } // For varargs symbols we also consider the elements to be @Nullable if there is a @Nullable // declaration annotation on the parameter + // TODO this flag check does not work for bytecodes!!! if ((arraySymbol.flags() & Flags.VARARGS) != 0) { return Nullness.hasNullableDeclarationAnnotation(arraySymbol, config); } return false; } + public static boolean nullableVarargsElementsForSourceOrBytecode( + Symbol varargsSymbol, Config config) { + return isArrayElementNullable(varargsSymbol, config) + || Nullness.hasNullableDeclarationAnnotation(varargsSymbol, config); + } + /** * Checks if the given array symbol has a {@code @NonNull} annotation for its elements. * @@ -482,12 +489,19 @@ public static boolean isArrayElementNonNull(Symbol arraySymbol, Config config) { } // For varargs symbols we also consider the elements to be @NonNull if there is a @NonNull // declaration annotation on the parameter + // TODO this flag check does not work for bytecodes!!! if ((arraySymbol.flags() & Flags.VARARGS) != 0) { return Nullness.hasNonNullDeclarationAnnotation(arraySymbol, config); } return false; } + public static boolean nonnullVarargsElementsForSourceOrBytecode( + Symbol varargsSymbol, Config config) { + return isArrayElementNonNull(varargsSymbol, config) + || Nullness.hasNonNullDeclarationAnnotation(varargsSymbol, config); + } + /** * Does the given symbol have a JetBrains @NotNull declaration annotation? Useful for workarounds * in light of https://github.com/uber/NullAway/issues/720 diff --git a/nullaway/src/main/java/com/uber/nullaway/Nullness.java b/nullaway/src/main/java/com/uber/nullaway/Nullness.java index 576e22d852..a359b0d051 100644 --- a/nullaway/src/main/java/com/uber/nullaway/Nullness.java +++ b/nullaway/src/main/java/com/uber/nullaway/Nullness.java @@ -229,7 +229,8 @@ public static boolean paramHasNullableAnnotation( && !config.isLegacyAnnotationLocation()) { // individual arguments passed in the varargs positions can be @Nullable if the array element // type of the parameter is @Nullable - return NullabilityUtil.isArrayElementNullable(symbol.getParameters().get(paramInd), config); + return NullabilityUtil.nullableVarargsElementsForSourceOrBytecode( + symbol.getParameters().get(paramInd), config); } else { return hasNullableAnnotation( NullabilityUtil.getAllAnnotationsForParameter(symbol, paramInd, config), config); @@ -271,7 +272,8 @@ public static boolean paramHasNonNullAnnotation( && !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); + return NullabilityUtil.nonnullVarargsElementsForSourceOrBytecode( + symbol.getParameters().get(paramInd), config); } else { return hasNonNullAnnotation( NullabilityUtil.getAllAnnotationsForParameter(symbol, paramInd, config), config); diff --git a/nullaway/src/main/java/com/uber/nullaway/dataflow/CoreNullnessStoreInitializer.java b/nullaway/src/main/java/com/uber/nullaway/dataflow/CoreNullnessStoreInitializer.java index a10b0829e4..6555031cdb 100644 --- a/nullaway/src/main/java/com/uber/nullaway/dataflow/CoreNullnessStoreInitializer.java +++ b/nullaway/src/main/java/com/uber/nullaway/dataflow/CoreNullnessStoreInitializer.java @@ -67,6 +67,7 @@ private static NullnessStore methodInitialStore( for (LocalVariableNode param : parameters) { Symbol paramSymbol = (Symbol) param.getElement(); Nullness assumed; + // TODO this flag check does not work for bytecodes!!! but that's ok here? if ((paramSymbol.flags() & Flags.VARARGS) != 0) { assumed = Nullness.varargsArrayIsNullable(paramSymbol, config) ? NULLABLE : NONNULL; } else { From e7ea1bf2563dce3b970772520adfa83f110a7947 Mon Sep 17 00:00:00 2001 From: Manu Sridharan Date: Tue, 8 Oct 2024 19:25:14 -0700 Subject: [PATCH 03/19] another test --- .../java/com/uber/nullaway/VarargsTests.java | 25 +++++++++++++++++++ .../RestrictivelyAnnotatedVarargs.java | 12 +++++++++ 2 files changed, 37 insertions(+) create mode 100644 test-java-lib/src/main/java/com/uber/lib/unannotated/RestrictivelyAnnotatedVarargs.java diff --git a/nullaway/src/test/java/com/uber/nullaway/VarargsTests.java b/nullaway/src/test/java/com/uber/nullaway/VarargsTests.java index 45076dc846..9bdea6c01f 100644 --- a/nullaway/src/test/java/com/uber/nullaway/VarargsTests.java +++ b/nullaway/src/test/java/com/uber/nullaway/VarargsTests.java @@ -531,4 +531,29 @@ public void testVarargsRestrictive() { "}") .doTest(); } + + @Test + public void testVarargsRestrictiveBytecodes() { + makeTestHelperWithArgs( + Arrays.asList( + "-d", + temporaryFolder.getRoot().getAbsolutePath(), + "-XepOpt:NullAway:AnnotatedPackages=com.uber", + "-XepOpt:NullAway:UnannotatedSubPackages=com.uber.lib.unannotated", + "-XepOpt:NullAway:AcknowledgeRestrictiveAnnotations=true")) + .addSourceLines( + "Test.java", + "package com.uber;", + "import com.uber.lib.unannotated.RestrictivelyAnnotatedVarargs;", + "public class Test {", + " public void testDeclaration() {", + " String x = null;", + " String[] y = null;", + " // BUG: Diagnostic contains: passing @Nullable parameter 'x'", + " RestrictivelyAnnotatedVarargs.test(x);", + " RestrictivelyAnnotatedVarargs.test(y);", + " }", + "}") + .doTest(); + } } diff --git a/test-java-lib/src/main/java/com/uber/lib/unannotated/RestrictivelyAnnotatedVarargs.java b/test-java-lib/src/main/java/com/uber/lib/unannotated/RestrictivelyAnnotatedVarargs.java new file mode 100644 index 0000000000..970fadaafb --- /dev/null +++ b/test-java-lib/src/main/java/com/uber/lib/unannotated/RestrictivelyAnnotatedVarargs.java @@ -0,0 +1,12 @@ +package com.uber.lib.unannotated; + +import javax.annotation.Nonnull; + +public class RestrictivelyAnnotatedVarargs { + + public static void test(@Nonnull String... args) { + for (String arg : args) { + System.out.println(arg); + } + } +} From 49652e41206f09faf4a5b471366f8f871db590a1 Mon Sep 17 00:00:00 2001 From: Manu Sridharan Date: Tue, 8 Oct 2024 19:28:12 -0700 Subject: [PATCH 04/19] docs --- .../com/uber/nullaway/NullabilityUtil.java | 22 +++++++++++++++++-- 1 file changed, 20 insertions(+), 2 deletions(-) diff --git a/nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java b/nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java index 8f47616e73..5c7638b272 100644 --- a/nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java +++ b/nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java @@ -456,13 +456,22 @@ public static boolean isArrayElementNullable(Symbol arraySymbol, Config config) } // For varargs symbols we also consider the elements to be @Nullable if there is a @Nullable // declaration annotation on the parameter - // TODO this flag check does not work for bytecodes!!! + // NOTE this flag check does not work for the varargs parameter of a method defined in bytecodes if ((arraySymbol.flags() & Flags.VARARGS) != 0) { return Nullness.hasNullableDeclarationAnnotation(arraySymbol, config); } return false; } + /** + * Checks if the given varargs symbol has a {@code @Nullable} annotation for its elements. Works + * for both source and bytecode. + * + * @param varargsSymbol the symbol of the varargs parameter + * @param config NullAway configuration + * @return true if the varargs symbol has a {@code @Nullable} annotation for its elements, false + * otherwise + */ public static boolean nullableVarargsElementsForSourceOrBytecode( Symbol varargsSymbol, Config config) { return isArrayElementNullable(varargsSymbol, config) @@ -489,13 +498,22 @@ public static boolean isArrayElementNonNull(Symbol arraySymbol, Config config) { } // For varargs symbols we also consider the elements to be @NonNull if there is a @NonNull // declaration annotation on the parameter - // TODO this flag check does not work for bytecodes!!! + // NOTE this flag check does not work for the varargs parameter of a method defined in bytecodes if ((arraySymbol.flags() & Flags.VARARGS) != 0) { return Nullness.hasNonNullDeclarationAnnotation(arraySymbol, config); } return false; } + /** + * Checks if the given varargs symbol has a {@code @NonNull} annotation for its elements. Works + * for both source and bytecode. + * + * @param varargsSymbol the symbol of the varargs parameter + * @param config NullAway configuration + * @return true if the varargs symbol has a {@code @NonNull} annotation for its elements, false + * otherwise + */ public static boolean nonnullVarargsElementsForSourceOrBytecode( Symbol varargsSymbol, Config config) { return isArrayElementNonNull(varargsSymbol, config) From 74740fe12722a9215b8ff7f9bb8ae8d327f0635b Mon Sep 17 00:00:00 2001 From: Manu Sridharan Date: Tue, 8 Oct 2024 19:31:26 -0700 Subject: [PATCH 05/19] more --- nullaway/src/main/java/com/uber/nullaway/Nullness.java | 4 ---- nullaway/src/test/java/com/uber/nullaway/VarargsTests.java | 5 +++++ .../uber/lib/unannotated/RestrictivelyAnnotatedVarargs.java | 6 +----- 3 files changed, 6 insertions(+), 9 deletions(-) diff --git a/nullaway/src/main/java/com/uber/nullaway/Nullness.java b/nullaway/src/main/java/com/uber/nullaway/Nullness.java index a359b0d051..c86520c5a4 100644 --- a/nullaway/src/main/java/com/uber/nullaway/Nullness.java +++ b/nullaway/src/main/java/com/uber/nullaway/Nullness.java @@ -227,8 +227,6 @@ public static boolean paramHasNullableAnnotation( if (symbol.isVarArgs() && paramInd == symbol.getParameters().size() - 1 && !config.isLegacyAnnotationLocation()) { - // individual arguments passed in the varargs positions can be @Nullable if the array element - // type of the parameter is @Nullable return NullabilityUtil.nullableVarargsElementsForSourceOrBytecode( symbol.getParameters().get(paramInd), config); } else { @@ -270,8 +268,6 @@ public static boolean paramHasNonNullAnnotation( 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.nonnullVarargsElementsForSourceOrBytecode( symbol.getParameters().get(paramInd), config); } else { diff --git a/nullaway/src/test/java/com/uber/nullaway/VarargsTests.java b/nullaway/src/test/java/com/uber/nullaway/VarargsTests.java index 9bdea6c01f..ca66c10fd4 100644 --- a/nullaway/src/test/java/com/uber/nullaway/VarargsTests.java +++ b/nullaway/src/test/java/com/uber/nullaway/VarargsTests.java @@ -71,6 +71,7 @@ public void testNullableVarargs() { .doTest(); } + /** Test for a @Nullable declaration annotation on a varargs parameter defined in bytecode */ @Test public void nullableVarArgsFromBytecode() { defaultCompilationHelper @@ -532,6 +533,10 @@ public void testVarargsRestrictive() { .doTest(); } + /** + * Test for a restrictive @NonNull declaration annotation on a varargs parameter defined in + * bytecode + */ @Test public void testVarargsRestrictiveBytecodes() { makeTestHelperWithArgs( diff --git a/test-java-lib/src/main/java/com/uber/lib/unannotated/RestrictivelyAnnotatedVarargs.java b/test-java-lib/src/main/java/com/uber/lib/unannotated/RestrictivelyAnnotatedVarargs.java index 970fadaafb..4a80a0da6c 100644 --- a/test-java-lib/src/main/java/com/uber/lib/unannotated/RestrictivelyAnnotatedVarargs.java +++ b/test-java-lib/src/main/java/com/uber/lib/unannotated/RestrictivelyAnnotatedVarargs.java @@ -4,9 +4,5 @@ public class RestrictivelyAnnotatedVarargs { - public static void test(@Nonnull String... args) { - for (String arg : args) { - System.out.println(arg); - } - } + public static void test(@Nonnull String... args) {} } From 6783bcec86eda2ef963c6366912c563d2a1fcbb2 Mon Sep 17 00:00:00 2001 From: Manu Sridharan Date: Tue, 8 Oct 2024 19:35:21 -0700 Subject: [PATCH 06/19] tweak comment --- .../uber/nullaway/dataflow/CoreNullnessStoreInitializer.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/nullaway/src/main/java/com/uber/nullaway/dataflow/CoreNullnessStoreInitializer.java b/nullaway/src/main/java/com/uber/nullaway/dataflow/CoreNullnessStoreInitializer.java index 6555031cdb..9c949c682f 100644 --- a/nullaway/src/main/java/com/uber/nullaway/dataflow/CoreNullnessStoreInitializer.java +++ b/nullaway/src/main/java/com/uber/nullaway/dataflow/CoreNullnessStoreInitializer.java @@ -67,7 +67,7 @@ private static NullnessStore methodInitialStore( for (LocalVariableNode param : parameters) { Symbol paramSymbol = (Symbol) param.getElement(); Nullness assumed; - // TODO this flag check does not work for bytecodes!!! but that's ok here? + // This flag check is safe we know paramSymbol represents a parameter defined in source code if ((paramSymbol.flags() & Flags.VARARGS) != 0) { assumed = Nullness.varargsArrayIsNullable(paramSymbol, config) ? NULLABLE : NONNULL; } else { From d21712b5fc7eb66b9a615ff5b3a8eed81674711d Mon Sep 17 00:00:00 2001 From: Manu Sridharan Date: Wed, 9 Oct 2024 17:20:48 -0700 Subject: [PATCH 07/19] a failing test --- .../java/com/uber/nullaway/VarargsTests.java | 18 +++++++++++++++++- .../src/main/java/com/uber/lib/Varargs.java | 2 ++ 2 files changed, 19 insertions(+), 1 deletion(-) diff --git a/nullaway/src/test/java/com/uber/nullaway/VarargsTests.java b/nullaway/src/test/java/com/uber/nullaway/VarargsTests.java index ca66c10fd4..b882989cd4 100644 --- a/nullaway/src/test/java/com/uber/nullaway/VarargsTests.java +++ b/nullaway/src/test/java/com/uber/nullaway/VarargsTests.java @@ -80,7 +80,7 @@ public void nullableVarArgsFromBytecode() { "package com.uber;", "import com.uber.lib.Varargs;", "public class Test {", - " public void testStrings(RuntimeException e) {", + " public void testDeclaration() {", " String x = null;", " Varargs s = new Varargs(x);", " }", @@ -88,6 +88,22 @@ public void nullableVarArgsFromBytecode() { .doTest(); } + @Test + public void nullableFullyQualifiedTypeUseVarArgsFromBytecode() { + defaultCompilationHelper + .addSourceLines( + "Test.java", + "package com.uber;", + "import com.uber.lib.Varargs;", + "public class Test {", + " public void testFullyQualifiedTypeUse() {", + " String[] x = null;", + " Varargs.fullyQualified(x);", + " }", + "}") + .doTest(); + } + @Test public void nullableTypeUseVarargs() { defaultCompilationHelper diff --git a/test-java-lib/src/main/java/com/uber/lib/Varargs.java b/test-java-lib/src/main/java/com/uber/lib/Varargs.java index 2d80dcfbda..39ab61f058 100644 --- a/test-java-lib/src/main/java/com/uber/lib/Varargs.java +++ b/test-java-lib/src/main/java/com/uber/lib/Varargs.java @@ -5,4 +5,6 @@ public class Varargs { public Varargs(@Nullable String... args) {} + + public static void fullyQualified(String @org.jspecify.annotations.Nullable ... args) {} } From b9d33b3f02c2891dad620410cacd7aa6c1e746ff Mon Sep 17 00:00:00 2001 From: Manu Sridharan Date: Wed, 9 Oct 2024 20:22:08 -0700 Subject: [PATCH 08/19] further bug fix --- .../com/uber/nullaway/NullabilityUtil.java | 45 ++++++++++++++++++- 1 file changed, 43 insertions(+), 2 deletions(-) diff --git a/nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java b/nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java index 1aadb13b8d..e44216745f 100644 --- a/nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java +++ b/nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java @@ -303,7 +303,18 @@ public static Stream getAllAnnotationsForParameter( */ public static Stream getTypeUseAnnotations( Symbol symbol, Config config) { - Stream rawTypeAttributes = symbol.getRawTypeAttributes().stream(); + // Adapted from Error Prone's MoreAnnotations class: + // https://github.com/google/error-prone/blob/5f71110374e63f3c35b661f538295fa15b5c1db2/check_api/src/main/java/com/google/errorprone/util/MoreAnnotations.java#L84-L91 + Symbol typeAnnotationOwner; + switch (symbol.getKind()) { + case PARAMETER: + typeAnnotationOwner = symbol.owner; + break; + default: + typeAnnotationOwner = symbol; + } + Stream rawTypeAttributes = + typeAnnotationOwner.getRawTypeAttributes().stream(); if (symbol instanceof Symbol.MethodSymbol) { // for methods, we want annotations on the return type return rawTypeAttributes.filter( @@ -313,7 +324,37 @@ public static Stream getTypeUseAnnotations( } else { // filter for annotations directly on the type return rawTypeAttributes.filter( - t -> NullabilityUtil.isDirectTypeUseAnnotation(t, symbol, config)); + t -> + targetTypeMatches(symbol, t.position) + && NullabilityUtil.isDirectTypeUseAnnotation(t, symbol, config)); + } + } + + // Adapted from Error Prone MoreAnnotations: + // https://github.com/google/error-prone/blob/5f71110374e63f3c35b661f538295fa15b5c1db2/check_api/src/main/java/com/google/errorprone/util/MoreAnnotations.java#L128 + private static boolean targetTypeMatches(Symbol sym, TypeAnnotationPosition position) { + switch (sym.getKind()) { + case LOCAL_VARIABLE: + return position.type == TargetType.LOCAL_VARIABLE; + case FIELD: + return position.type == TargetType.FIELD; + case CONSTRUCTOR: + case METHOD: + return position.type == TargetType.METHOD_RETURN; + case PARAMETER: + switch (position.type) { + case METHOD_FORMAL_PARAMETER: + return ((Symbol.MethodSymbol) sym.owner).getParameters().indexOf(sym) + == position.parameter_index; + default: + return false; + } + case CLASS: + // There are no type annotations on the top-level type of the class being declared, only + // on other types in the signature (e.g. `class Foo extends Bar<@A Baz> {}`). + return false; + default: + throw new AssertionError("unsupported element kind: " + sym.getKind()); } } From 989539a8a55e20aeda46b38dd4f32f6dc75d35ee Mon Sep 17 00:00:00 2001 From: Manu Sridharan Date: Wed, 9 Oct 2024 20:24:03 -0700 Subject: [PATCH 09/19] tweak comment --- .../uber/nullaway/dataflow/CoreNullnessStoreInitializer.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/nullaway/src/main/java/com/uber/nullaway/dataflow/CoreNullnessStoreInitializer.java b/nullaway/src/main/java/com/uber/nullaway/dataflow/CoreNullnessStoreInitializer.java index 9c949c682f..a969c08453 100644 --- a/nullaway/src/main/java/com/uber/nullaway/dataflow/CoreNullnessStoreInitializer.java +++ b/nullaway/src/main/java/com/uber/nullaway/dataflow/CoreNullnessStoreInitializer.java @@ -67,7 +67,8 @@ private static NullnessStore methodInitialStore( for (LocalVariableNode param : parameters) { Symbol paramSymbol = (Symbol) param.getElement(); Nullness assumed; - // This flag check is safe we know paramSymbol represents a parameter defined in source code + // Using this flag to check for a varargs parameter works since we know paramSymbol represents + // a parameter defined in source code if ((paramSymbol.flags() & Flags.VARARGS) != 0) { assumed = Nullness.varargsArrayIsNullable(paramSymbol, config) ? NULLABLE : NONNULL; } else { From 9dcac6badb94740bc1d2ada5824a256a9aa1fdb9 Mon Sep 17 00:00:00 2001 From: Manu Sridharan Date: Wed, 9 Oct 2024 20:25:36 -0700 Subject: [PATCH 10/19] rename --- nullaway/src/test/java/com/uber/nullaway/VarargsTests.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/nullaway/src/test/java/com/uber/nullaway/VarargsTests.java b/nullaway/src/test/java/com/uber/nullaway/VarargsTests.java index b882989cd4..1aa7f90f61 100644 --- a/nullaway/src/test/java/com/uber/nullaway/VarargsTests.java +++ b/nullaway/src/test/java/com/uber/nullaway/VarargsTests.java @@ -73,7 +73,7 @@ public void testNullableVarargs() { /** Test for a @Nullable declaration annotation on a varargs parameter defined in bytecode */ @Test - public void nullableVarArgsFromBytecode() { + public void nullableDeclarationVarArgsFromBytecode() { defaultCompilationHelper .addSourceLines( "Test.java", @@ -89,7 +89,7 @@ public void nullableVarArgsFromBytecode() { } @Test - public void nullableFullyQualifiedTypeUseVarArgsFromBytecode() { + public void nullableTypeUseVarArgsFromBytecode() { defaultCompilationHelper .addSourceLines( "Test.java", From 25ad62a23403e1adb28fd8fc2ee592ddadb54403 Mon Sep 17 00:00:00 2001 From: Manu Sridharan Date: Thu, 10 Oct 2024 08:03:41 -0700 Subject: [PATCH 11/19] fix explicitly-typed lambdas --- .../com/uber/nullaway/NullabilityUtil.java | 13 ++++++++++-- .../java/com/uber/nullaway/Java8Tests.java | 20 +++++++++++++++++++ 2 files changed, 31 insertions(+), 2 deletions(-) diff --git a/nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java b/nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java index e44216745f..c0a0e02d50 100644 --- a/nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java +++ b/nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java @@ -308,6 +308,7 @@ public static Stream getTypeUseAnnotations( Symbol typeAnnotationOwner; switch (symbol.getKind()) { case PARAMETER: + // use the symbol's owner for parameters, unless it's the parameter of a lambda typeAnnotationOwner = symbol.owner; break; default: @@ -344,8 +345,16 @@ private static boolean targetTypeMatches(Symbol sym, TypeAnnotationPosition posi case PARAMETER: switch (position.type) { case METHOD_FORMAL_PARAMETER: - return ((Symbol.MethodSymbol) sym.owner).getParameters().indexOf(sym) - == position.parameter_index; + int parameterIndex = position.parameter_index; + if (position.onLambda != null) { + com.sun.tools.javac.util.List lambdaParams = + position.onLambda.params; + return parameterIndex < lambdaParams.size() + && lambdaParams.get(parameterIndex).sym.equals(sym); + } else { + return ((Symbol.MethodSymbol) sym.owner).getParameters().indexOf(sym) + == parameterIndex; + } default: return false; } diff --git a/nullaway/src/test/java/com/uber/nullaway/Java8Tests.java b/nullaway/src/test/java/com/uber/nullaway/Java8Tests.java index 478d3e1275..83b535f7ea 100644 --- a/nullaway/src/test/java/com/uber/nullaway/Java8Tests.java +++ b/nullaway/src/test/java/com/uber/nullaway/Java8Tests.java @@ -47,4 +47,24 @@ public void methodReferenceOnNullableVariable() { "}") .doTest(); } + + @Test + public void testNullableLambdaParamTypeUse() { + defaultCompilationHelper + .addSourceLines( + "Test.java", + "package com.uber;", + "import org.jspecify.annotations.Nullable;", + "class Test {", + " @FunctionalInterface", + " interface NullableParamFunctionTypeUse {", + " U takeVal(@org.jspecify.annotations.Nullable T x);", + " }", + " static void testParamTypeUse() {", + " NullableParamFunctionTypeUse n3 = (@org.jspecify.annotations.Nullable Object x) -> (x == null) ? \"null\" : x.toString();", + " NullableParamFunctionTypeUse n4 = (x) -> (x == null) ? \"null\" : x.toString();", + " }", + "}") + .doTest(); + } } From b38e3cb527928e1e86685eb27b724310ed0514b3 Mon Sep 17 00:00:00 2001 From: Manu Sridharan Date: Thu, 10 Oct 2024 08:06:13 -0700 Subject: [PATCH 12/19] tweaks --- nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java | 1 - nullaway/src/test/java/com/uber/nullaway/Java8Tests.java | 4 ++-- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java b/nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java index c0a0e02d50..b1997385e6 100644 --- a/nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java +++ b/nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java @@ -308,7 +308,6 @@ public static Stream getTypeUseAnnotations( Symbol typeAnnotationOwner; switch (symbol.getKind()) { case PARAMETER: - // use the symbol's owner for parameters, unless it's the parameter of a lambda typeAnnotationOwner = symbol.owner; break; default: diff --git a/nullaway/src/test/java/com/uber/nullaway/Java8Tests.java b/nullaway/src/test/java/com/uber/nullaway/Java8Tests.java index 83b535f7ea..85a1265512 100644 --- a/nullaway/src/test/java/com/uber/nullaway/Java8Tests.java +++ b/nullaway/src/test/java/com/uber/nullaway/Java8Tests.java @@ -58,10 +58,10 @@ public void testNullableLambdaParamTypeUse() { "class Test {", " @FunctionalInterface", " interface NullableParamFunctionTypeUse {", - " U takeVal(@org.jspecify.annotations.Nullable T x);", + " U takeVal(@Nullable T x);", " }", " static void testParamTypeUse() {", - " NullableParamFunctionTypeUse n3 = (@org.jspecify.annotations.Nullable Object x) -> (x == null) ? \"null\" : x.toString();", + " NullableParamFunctionTypeUse n3 = (@Nullable Object x) -> (x == null) ? \"null\" : x.toString();", " NullableParamFunctionTypeUse n4 = (x) -> (x == null) ? \"null\" : x.toString();", " }", "}") From 270963bb8616f6c714be3b5cf61f05db2f31a1a9 Mon Sep 17 00:00:00 2001 From: Manu Sridharan Date: Thu, 10 Oct 2024 08:07:48 -0700 Subject: [PATCH 13/19] tweak name --- nullaway/src/test/java/com/uber/nullaway/VarargsTests.java | 4 ++-- test-java-lib/src/main/java/com/uber/lib/Varargs.java | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/nullaway/src/test/java/com/uber/nullaway/VarargsTests.java b/nullaway/src/test/java/com/uber/nullaway/VarargsTests.java index 1aa7f90f61..52a3eb4a23 100644 --- a/nullaway/src/test/java/com/uber/nullaway/VarargsTests.java +++ b/nullaway/src/test/java/com/uber/nullaway/VarargsTests.java @@ -96,9 +96,9 @@ public void nullableTypeUseVarArgsFromBytecode() { "package com.uber;", "import com.uber.lib.Varargs;", "public class Test {", - " public void testFullyQualifiedTypeUse() {", + " public void testTypeUse() {", " String[] x = null;", - " Varargs.fullyQualified(x);", + " Varargs.typeUse(x);", " }", "}") .doTest(); diff --git a/test-java-lib/src/main/java/com/uber/lib/Varargs.java b/test-java-lib/src/main/java/com/uber/lib/Varargs.java index 39ab61f058..a232d1c14a 100644 --- a/test-java-lib/src/main/java/com/uber/lib/Varargs.java +++ b/test-java-lib/src/main/java/com/uber/lib/Varargs.java @@ -6,5 +6,5 @@ public class Varargs { public Varargs(@Nullable String... args) {} - public static void fullyQualified(String @org.jspecify.annotations.Nullable ... args) {} + public static void typeUse(String @org.jspecify.annotations.Nullable ... args) {} } From 2f7a162e36fa399951e106b70cdcf6bcbb6d9d97 Mon Sep 17 00:00:00 2001 From: Manu Sridharan Date: Thu, 10 Oct 2024 19:07:46 -0700 Subject: [PATCH 14/19] more assert info --- nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java b/nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java index b1997385e6..f8d7386621 100644 --- a/nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java +++ b/nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java @@ -362,7 +362,7 @@ private static boolean targetTypeMatches(Symbol sym, TypeAnnotationPosition posi // on other types in the signature (e.g. `class Foo extends Bar<@A Baz> {}`). return false; default: - throw new AssertionError("unsupported element kind: " + sym.getKind()); + throw new AssertionError("unsupported element kind " + sym.getKind() + " symbol " + sym); } } From ddb1ab045bfbf790eada64c290b6152c4bed48f6 Mon Sep 17 00:00:00 2001 From: Manu Sridharan Date: Fri, 11 Oct 2024 09:55:49 -0700 Subject: [PATCH 15/19] fix --- nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java | 2 ++ 1 file changed, 2 insertions(+) diff --git a/nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java b/nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java index f8d7386621..08db99285e 100644 --- a/nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java +++ b/nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java @@ -337,6 +337,8 @@ private static boolean targetTypeMatches(Symbol sym, TypeAnnotationPosition posi case LOCAL_VARIABLE: return position.type == TargetType.LOCAL_VARIABLE; case FIELD: + // treated like a field + case ENUM_CONSTANT: return position.type == TargetType.FIELD; case CONSTRUCTOR: case METHOD: From dac889de3985f5b0614f71126560454134de99de Mon Sep 17 00:00:00 2001 From: Manu Sridharan Date: Mon, 14 Oct 2024 13:48:39 -0700 Subject: [PATCH 16/19] simplify switches with one case --- .../com/uber/nullaway/NullabilityUtil.java | 37 ++++++++----------- 1 file changed, 15 insertions(+), 22 deletions(-) diff --git a/nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java b/nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java index 08db99285e..13116eb266 100644 --- a/nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java +++ b/nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java @@ -55,6 +55,7 @@ import java.util.stream.Stream; import javax.lang.model.element.AnnotationMirror; import javax.lang.model.element.AnnotationValue; +import javax.lang.model.element.ElementKind; import javax.lang.model.element.ExecutableElement; import org.checkerframework.nullaway.javacutil.AnnotationUtils; import org.jspecify.annotations.Nullable; @@ -305,14 +306,8 @@ public static Stream getTypeUseAnnotations( Symbol symbol, Config config) { // Adapted from Error Prone's MoreAnnotations class: // https://github.com/google/error-prone/blob/5f71110374e63f3c35b661f538295fa15b5c1db2/check_api/src/main/java/com/google/errorprone/util/MoreAnnotations.java#L84-L91 - Symbol typeAnnotationOwner; - switch (symbol.getKind()) { - case PARAMETER: - typeAnnotationOwner = symbol.owner; - break; - default: - typeAnnotationOwner = symbol; - } + Symbol typeAnnotationOwner = + symbol.getKind().equals(ElementKind.PARAMETER) ? symbol.owner : symbol; Stream rawTypeAttributes = typeAnnotationOwner.getRawTypeAttributes().stream(); if (symbol instanceof Symbol.MethodSymbol) { @@ -344,20 +339,18 @@ private static boolean targetTypeMatches(Symbol sym, TypeAnnotationPosition posi case METHOD: return position.type == TargetType.METHOD_RETURN; case PARAMETER: - switch (position.type) { - case METHOD_FORMAL_PARAMETER: - int parameterIndex = position.parameter_index; - if (position.onLambda != null) { - com.sun.tools.javac.util.List lambdaParams = - position.onLambda.params; - return parameterIndex < lambdaParams.size() - && lambdaParams.get(parameterIndex).sym.equals(sym); - } else { - return ((Symbol.MethodSymbol) sym.owner).getParameters().indexOf(sym) - == parameterIndex; - } - default: - return false; + if (position.type.equals(TargetType.METHOD_FORMAL_PARAMETER)) { + int parameterIndex = position.parameter_index; + if (position.onLambda != null) { + com.sun.tools.javac.util.List lambdaParams = + position.onLambda.params; + return parameterIndex < lambdaParams.size() + && lambdaParams.get(parameterIndex).sym.equals(sym); + } else { + return ((Symbol.MethodSymbol) sym.owner).getParameters().indexOf(sym) == parameterIndex; + } + } else { + return false; } case CLASS: // There are no type annotations on the top-level type of the class being declared, only From a59a7a8402e489583870e1c9a5cb4e6200ab86b0 Mon Sep 17 00:00:00 2001 From: Manu Sridharan Date: Mon, 14 Oct 2024 13:53:09 -0700 Subject: [PATCH 17/19] test comment --- nullaway/src/test/java/com/uber/nullaway/Java8Tests.java | 1 + 1 file changed, 1 insertion(+) diff --git a/nullaway/src/test/java/com/uber/nullaway/Java8Tests.java b/nullaway/src/test/java/com/uber/nullaway/Java8Tests.java index 85a1265512..8541c445af 100644 --- a/nullaway/src/test/java/com/uber/nullaway/Java8Tests.java +++ b/nullaway/src/test/java/com/uber/nullaway/Java8Tests.java @@ -48,6 +48,7 @@ public void methodReferenceOnNullableVariable() { .doTest(); } + /** test that we can properly read an explicit type-use annotation on a lambda parameter */ @Test public void testNullableLambdaParamTypeUse() { defaultCompilationHelper From 0abca22a59be3e96f88cf7bb785f040a44956641 Mon Sep 17 00:00:00 2001 From: Manu Sridharan Date: Mon, 14 Oct 2024 16:03:26 -0700 Subject: [PATCH 18/19] tweak test --- nullaway/src/test/java/com/uber/nullaway/VarargsTests.java | 2 ++ 1 file changed, 2 insertions(+) diff --git a/nullaway/src/test/java/com/uber/nullaway/VarargsTests.java b/nullaway/src/test/java/com/uber/nullaway/VarargsTests.java index 52a3eb4a23..96d7433a63 100644 --- a/nullaway/src/test/java/com/uber/nullaway/VarargsTests.java +++ b/nullaway/src/test/java/com/uber/nullaway/VarargsTests.java @@ -83,6 +83,8 @@ public void nullableDeclarationVarArgsFromBytecode() { " public void testDeclaration() {", " String x = null;", " Varargs s = new Varargs(x);", + " String y = \"hello\", z = null;", + " Varargs s2 = new Varargs(y, z);", " }", "}") .doTest(); From f30299a7facc5a04b454c132226b49800436cae4 Mon Sep 17 00:00:00 2001 From: Manu Sridharan Date: Mon, 14 Oct 2024 16:33:29 -0700 Subject: [PATCH 19/19] fix handling of restrictive @NonNull type use annotation on varargs array contents --- .../com/uber/nullaway/NullabilityUtil.java | 40 ++++++++++++++----- .../java/com/uber/nullaway/VarargsTests.java | 4 ++ .../RestrictivelyAnnotatedVarargs.java | 3 ++ 3 files changed, 36 insertions(+), 11 deletions(-) diff --git a/nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java b/nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java index 13116eb266..eee5eb88b5 100644 --- a/nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java +++ b/nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java @@ -302,8 +302,21 @@ public static Stream getAllAnnotationsForParameter( * Gets the type use annotations on a symbol, ignoring annotations on components of the type (type * arguments, wildcards, etc.) */ - public static Stream getTypeUseAnnotations( - Symbol symbol, Config config) { + public static Stream getTypeUseAnnotations(Symbol symbol, Config config) { + return getTypeUseAnnotations(symbol, config, /* onlyDirect= */ true); + } + + /** + * Gets the type use annotations on a symbol + * + * @param symbol the symbol + * @param config NullAway configuration + * @param onlyDirect if true, only return annotations that are directly on the type, not on + * components of the type (type arguments, wildcards, array contents, etc.) + * @return the type use annotations on the symbol + */ + private static Stream getTypeUseAnnotations( + Symbol symbol, Config config, boolean onlyDirect) { // Adapted from Error Prone's MoreAnnotations class: // https://github.com/google/error-prone/blob/5f71110374e63f3c35b661f538295fa15b5c1db2/check_api/src/main/java/com/google/errorprone/util/MoreAnnotations.java#L84-L91 Symbol typeAnnotationOwner = @@ -321,7 +334,7 @@ public static Stream getTypeUseAnnotations( return rawTypeAttributes.filter( t -> targetTypeMatches(symbol, t.position) - && NullabilityUtil.isDirectTypeUseAnnotation(t, symbol, config)); + && (!onlyDirect || NullabilityUtil.isDirectTypeUseAnnotation(t, symbol, config))); } } @@ -563,14 +576,19 @@ public static boolean nullableVarargsElementsForSourceOrBytecode( * 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; - } - } - } + if (getTypeUseAnnotations(arraySymbol, config, /* onlyDirect= */ false) + .anyMatch( + t -> { + for (TypeAnnotationPosition.TypePathEntry entry : t.position.location) { + if (entry.tag == TypeAnnotationPosition.TypePathEntryKind.ARRAY) { + if (Nullness.isNonNullAnnotation(t.type.toString(), config)) { + return true; + } + } + } + return false; + })) { + return true; } // For varargs symbols we also consider the elements to be @NonNull if there is a @NonNull // declaration annotation on the parameter diff --git a/nullaway/src/test/java/com/uber/nullaway/VarargsTests.java b/nullaway/src/test/java/com/uber/nullaway/VarargsTests.java index 96d7433a63..3f29e1e082 100644 --- a/nullaway/src/test/java/com/uber/nullaway/VarargsTests.java +++ b/nullaway/src/test/java/com/uber/nullaway/VarargsTests.java @@ -575,6 +575,10 @@ public void testVarargsRestrictiveBytecodes() { " // BUG: Diagnostic contains: passing @Nullable parameter 'x'", " RestrictivelyAnnotatedVarargs.test(x);", " RestrictivelyAnnotatedVarargs.test(y);", + " // BUG: Diagnostic contains: passing @Nullable parameter 'x'", + " RestrictivelyAnnotatedVarargs.testTypeUse(x);", + " // TODO should report an error; requires a fuller fix for #1027", + " RestrictivelyAnnotatedVarargs.testTypeUse(y);", " }", "}") .doTest(); diff --git a/test-java-lib/src/main/java/com/uber/lib/unannotated/RestrictivelyAnnotatedVarargs.java b/test-java-lib/src/main/java/com/uber/lib/unannotated/RestrictivelyAnnotatedVarargs.java index 4a80a0da6c..5dcfddb03a 100644 --- a/test-java-lib/src/main/java/com/uber/lib/unannotated/RestrictivelyAnnotatedVarargs.java +++ b/test-java-lib/src/main/java/com/uber/lib/unannotated/RestrictivelyAnnotatedVarargs.java @@ -5,4 +5,7 @@ public class RestrictivelyAnnotatedVarargs { public static void test(@Nonnull String... args) {} + + public static void testTypeUse( + @org.jspecify.annotations.NonNull String @org.jspecify.annotations.NonNull ... args) {} }