From f1ba82a6b8caf55b58c128a2c405e6e498213ac2 Mon Sep 17 00:00:00 2001 From: Md Date: Wed, 18 Oct 2023 15:03:10 -0700 Subject: [PATCH 01/22] Modify JSpecify Array Handling Added Unit Tests for @Nullable annotations on array in JSpecify mode and modified behaviour. --- .../main/java/com/uber/nullaway/NullAway.java | 5 +- .../com/uber/nullaway/NullabilityUtil.java | 24 +++-- .../main/java/com/uber/nullaway/Nullness.java | 4 +- .../nullaway/NullAwayJSpecifyArrayTests.java | 88 +++++++++++++++++++ 4 files changed, 111 insertions(+), 10 deletions(-) create mode 100644 nullaway/src/test/java/com/uber/nullaway/NullAwayJSpecifyArrayTests.java diff --git a/nullaway/src/main/java/com/uber/nullaway/NullAway.java b/nullaway/src/main/java/com/uber/nullaway/NullAway.java index 3a1f328cd5..823914a440 100644 --- a/nullaway/src/main/java/com/uber/nullaway/NullAway.java +++ b/nullaway/src/main/java/com/uber/nullaway/NullAway.java @@ -1949,7 +1949,8 @@ private SetMultimap checkConstructorInitialization( } private boolean symbolHasExternalInitAnnotation(Symbol symbol) { - return StreamSupport.stream(NullabilityUtil.getAllAnnotations(symbol).spliterator(), false) + return StreamSupport.stream( + NullabilityUtil.getAllAnnotations(symbol, config).spliterator(), false) .map((anno) -> anno.getAnnotationType().toString()) .anyMatch(config::isExternalInitClassAnnotation); } @@ -2206,7 +2207,7 @@ private boolean isInitializerMethod(VisitorState state, Symbol.MethodSymbol symb } private boolean skipDueToFieldAnnotation(Symbol fieldSymbol) { - return NullabilityUtil.getAllAnnotations(fieldSymbol) + return NullabilityUtil.getAllAnnotations(fieldSymbol, config) .map(anno -> anno.getAnnotationType().toString()) .anyMatch(config::isExcludedFieldAnnotation); } diff --git a/nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java b/nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java index ba4be9f0f3..b78c1f25ad 100644 --- a/nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java +++ b/nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java @@ -180,9 +180,9 @@ public static TreePath findEnclosingMethodOrLambdaOrInitializer(TreePath path) { * @param symbol the symbol * @return all annotations on the symbol and on the type of the symbol */ - public static Stream getAllAnnotations(Symbol symbol) { + public static Stream getAllAnnotations(Symbol symbol, Config config) { // for methods, we care about annotations on the return type, not on the method type itself - Stream typeUseAnnotations = getTypeUseAnnotations(symbol); + Stream typeUseAnnotations = getTypeUseAnnotations(symbol, config); return Stream.concat(symbol.getAnnotationMirrors().stream(), typeUseAnnotations); } @@ -277,19 +277,22 @@ public static Stream getAllAnnotationsForParameter( * Gets the type use annotations on a symbol, ignoring annotations on components of the type (type * arguments, wildcards, etc.) */ - private static Stream getTypeUseAnnotations(Symbol symbol) { + private static Stream getTypeUseAnnotations( + Symbol symbol, Config config) { Stream rawTypeAttributes = symbol.getRawTypeAttributes().stream(); if (symbol instanceof Symbol.MethodSymbol) { // for methods, we want annotations on the return type return rawTypeAttributes.filter( - (t) -> t.position.type.equals(TargetType.METHOD_RETURN) && isDirectTypeUseAnnotation(t)); + (t) -> + t.position.type.equals(TargetType.METHOD_RETURN) + && isDirectTypeUseAnnotation(t, config)); } else { // filter for annotations directly on the type - return rawTypeAttributes.filter(NullabilityUtil::isDirectTypeUseAnnotation); + return rawTypeAttributes.filter(t -> NullabilityUtil.isDirectTypeUseAnnotation(t, config)); } } - private static boolean isDirectTypeUseAnnotation(Attribute.TypeCompound t) { + private static boolean isDirectTypeUseAnnotation(Attribute.TypeCompound t, Config config) { // location is a list of TypePathEntry objects, indicating whether the annotation is // on an array, inner type, wildcard, or type argument. If it's empty, then the // annotation is directly on the type. @@ -305,6 +308,9 @@ private static boolean isDirectTypeUseAnnotation(Attribute.TypeCompound t) { // proper deprecation of the incorrect behaviors for type use annotations when their // semantics don't match those of a declaration annotation in the same position. // See https://github.com/uber/NullAway/issues/708 + if (config.isJSpecifyMode()) { + return isDirectTypeUseAnnotationJSpecify(t); + } boolean locationHasInnerTypes = false; boolean locationHasArray = false; for (TypePathEntry entry : t.position.location) { @@ -324,6 +330,12 @@ private static boolean isDirectTypeUseAnnotation(Attribute.TypeCompound t) { return !(locationHasInnerTypes && locationHasArray); } + private static boolean isDirectTypeUseAnnotationJSpecify(Attribute.TypeCompound t) { + // Currently we are ignoring @Nullable annotations on type in JSpecify mode. + // Eventually, this should return true if annotation is on type. + return t.position.location.isEmpty(); + } + /** * Check if a field might be null, based on the type. * diff --git a/nullaway/src/main/java/com/uber/nullaway/Nullness.java b/nullaway/src/main/java/com/uber/nullaway/Nullness.java index c1b8198de0..845d547a57 100644 --- a/nullaway/src/main/java/com/uber/nullaway/Nullness.java +++ b/nullaway/src/main/java/com/uber/nullaway/Nullness.java @@ -192,7 +192,7 @@ private static boolean isNonNullAnnotation(String annotName, Config config) { * Config)} */ public static boolean hasNonNullAnnotation(Symbol symbol, Config config) { - return hasNonNullAnnotation(NullabilityUtil.getAllAnnotations(symbol), config); + return hasNonNullAnnotation(NullabilityUtil.getAllAnnotations(symbol, config), config); } /** @@ -203,7 +203,7 @@ public static boolean hasNonNullAnnotation(Symbol symbol, Config config) { * Config)} */ public static boolean hasNullableAnnotation(Symbol symbol, Config config) { - return hasNullableAnnotation(NullabilityUtil.getAllAnnotations(symbol), config); + return hasNullableAnnotation(NullabilityUtil.getAllAnnotations(symbol, config), config); } /** diff --git a/nullaway/src/test/java/com/uber/nullaway/NullAwayJSpecifyArrayTests.java b/nullaway/src/test/java/com/uber/nullaway/NullAwayJSpecifyArrayTests.java new file mode 100644 index 0000000000..4077ea80da --- /dev/null +++ b/nullaway/src/test/java/com/uber/nullaway/NullAwayJSpecifyArrayTests.java @@ -0,0 +1,88 @@ +package com.uber.nullaway; + +import com.google.errorprone.CompilationTestHelper; +import java.util.Arrays; +import org.junit.Ignore; +import org.junit.Test; + +public class NullAwayJSpecifyArrayTests extends NullAwayTestsBase { + + @Test + public void ArrayTypeAnnotationDereference() { + makeHelper() + .addSourceLines( + "Test.java", + "package com.uber;", + "import org.jspecify.annotations.Nullable;", + "class Test {", + " static @Nullable Integer [] fizz = {1};", + " static void foo() {", + " // Ok since @Nullable annotations on type are ignored currently.", + " // however, this should report an error eventually.", + " int bar = fizz.length;", + " }", + "}") + .doTest(); + } + + @Test + public void ArrayTypeAnnotationAssignment() { + makeHelper() + .addSourceLines( + "Test.java", + "package com.uber;", + "import org.jspecify.annotations.Nullable;", + "class Test {", + " Object foo = new Object();", + " void m( @Nullable Integer [] bar) {", + " // OK: @Nullable annotations on type are ignored currently.", + " // however, this should report an error eventually.", + " foo = bar;", + " }", + "}") + .doTest(); + } + + @Test + public void ArrayElementAnnotationDereference() { + makeHelper() + .addSourceLines( + "Test.java", + "package com.uber;", + "import org.jspecify.annotations.Nullable;", + "class Test {", + " static String @Nullable [] fizz = {\"1\"};", + " static void foo() {", + " // BUG: Diagnostic contains: dereferenced expression fizz is @Nullable", + " int bar = fizz[0].length();", + " }", + "}") + .doTest(); + } + + @Ignore("We do not support annotations on array currently") + @Test + public void ArrayElementAnnotationAssignment() { + makeHelper() + .addSourceLines( + "Test.java", + "package com.uber;", + "import org.jspecify.annotations.Nullable;", + "class Test {", + " Object fizz = new Object();", + " void m( @Nullable Integer [] foo) {", + " // BUG: Diagnostic contains: dereferenced expression arr[0] is @Nullable", + " int bar = foo[0];", + " //valid assignment", + " fizz = foo;", + " }", + "}") + .doTest(); + } + + private CompilationTestHelper makeHelper() { + return makeTestHelperWithArgs( + Arrays.asList( + "-XepOpt:NullAway:AnnotatedPackages=com.uber", "-XepOpt:NullAway:JSpecifyMode=true")); + } +} From 4bdd570ba0738ded79aef3294a69d9a569486861 Mon Sep 17 00:00:00 2001 From: Md Date: Wed, 18 Oct 2023 17:50:42 -0700 Subject: [PATCH 02/22] Fix incorrect wildcard/inner type handling in JSpecify mode --- .../java/com/uber/nullaway/NullabilityUtil.java | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java b/nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java index b78c1f25ad..4d5aa84e5b 100644 --- a/nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java +++ b/nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java @@ -308,9 +308,7 @@ private static boolean isDirectTypeUseAnnotation(Attribute.TypeCompound t, Confi // proper deprecation of the incorrect behaviors for type use annotations when their // semantics don't match those of a declaration annotation in the same position. // See https://github.com/uber/NullAway/issues/708 - if (config.isJSpecifyMode()) { - return isDirectTypeUseAnnotationJSpecify(t); - } + boolean locationHasInnerTypes = false; boolean locationHasArray = false; for (TypePathEntry entry : t.position.location) { @@ -319,6 +317,11 @@ private static boolean isDirectTypeUseAnnotation(Attribute.TypeCompound t, Confi locationHasInnerTypes = true; break; case ARRAY: + // Currently we are ignoring @Nullable annotations on type in JSpecify mode. + // Eventually, this should return true if annotation is on type. + if (config.isJSpecifyMode()) { + return false; + } locationHasArray = true; break; default: @@ -330,12 +333,6 @@ private static boolean isDirectTypeUseAnnotation(Attribute.TypeCompound t, Confi return !(locationHasInnerTypes && locationHasArray); } - private static boolean isDirectTypeUseAnnotationJSpecify(Attribute.TypeCompound t) { - // Currently we are ignoring @Nullable annotations on type in JSpecify mode. - // Eventually, this should return true if annotation is on type. - return t.position.location.isEmpty(); - } - /** * Check if a field might be null, based on the type. * From 06e816e1cab658493b12d98f2e23d8dca97bec31 Mon Sep 17 00:00:00 2001 From: Md Date: Wed, 18 Oct 2023 18:01:23 -0700 Subject: [PATCH 03/22] Fix Unit Tests Minor changes and better documentation --- .../nullaway/NullAwayJSpecifyArrayTests.java | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/nullaway/src/test/java/com/uber/nullaway/NullAwayJSpecifyArrayTests.java b/nullaway/src/test/java/com/uber/nullaway/NullAwayJSpecifyArrayTests.java index 4077ea80da..dafc4865f0 100644 --- a/nullaway/src/test/java/com/uber/nullaway/NullAwayJSpecifyArrayTests.java +++ b/nullaway/src/test/java/com/uber/nullaway/NullAwayJSpecifyArrayTests.java @@ -17,7 +17,7 @@ public void ArrayTypeAnnotationDereference() { "class Test {", " static @Nullable Integer [] fizz = {1};", " static void foo() {", - " // Ok since @Nullable annotations on type are ignored currently.", + " // OK: @Nullable annotations on type are ignored currently.", " // however, this should report an error eventually.", " int bar = fizz.length;", " }", @@ -53,6 +53,8 @@ public void ArrayElementAnnotationDereference() { "class Test {", " static String @Nullable [] fizz = {\"1\"};", " static void foo() {", + " // This currently reports an error since fizz is @Nullable,", + " // but it should eventually report due to fizz[0] being @Nullable", " // BUG: Diagnostic contains: dereferenced expression fizz is @Nullable", " int bar = fizz[0].length();", " }", @@ -60,7 +62,7 @@ public void ArrayElementAnnotationDereference() { .doTest(); } - @Ignore("We do not support annotations on array currently") + @Ignore("We do not support annotations on array elements currently") @Test public void ArrayElementAnnotationAssignment() { makeHelper() @@ -70,11 +72,12 @@ public void ArrayElementAnnotationAssignment() { "import org.jspecify.annotations.Nullable;", "class Test {", " Object fizz = new Object();", - " void m( @Nullable Integer [] foo) {", - " // BUG: Diagnostic contains: dereferenced expression arr[0] is @Nullable", - " int bar = foo[0];", - " //valid assignment", - " fizz = foo;", + " Object bar = new Object();", + " void m( Integer @Nullable [] foo) {", + " // BUG: assigning @Nullable expression to @NonNull field", + " fizz = foo[0];", + " // OK: valid assignment since only elements can be null", + " bar = foo;", " }", "}") .doTest(); From 6a19f36e596a575f15b99f1b4fb1c0b482915dfe Mon Sep 17 00:00:00 2001 From: Md Date: Thu, 19 Oct 2023 15:23:38 -0700 Subject: [PATCH 04/22] Update comment --- nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java | 3 ++- 1 file changed, 2 insertions(+), 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 4d5aa84e5b..768bb8f0c5 100644 --- a/nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java +++ b/nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java @@ -318,7 +318,8 @@ private static boolean isDirectTypeUseAnnotation(Attribute.TypeCompound t, Confi break; case ARRAY: // Currently we are ignoring @Nullable annotations on type in JSpecify mode. - // Eventually, this should return true if annotation is on type. + // Eventually, this should return true, and array access should be handled + // elsewhere. if (config.isJSpecifyMode()) { return false; } From 2b6d74692c90aa05eddcc33ec11c4ef6fa690c82 Mon Sep 17 00:00:00 2001 From: Manu Sridharan Date: Fri, 20 Oct 2023 10:03:08 -0700 Subject: [PATCH 05/22] add some documentation --- .../java/com/uber/nullaway/NullabilityUtil.java | 14 ++++++++++++-- 1 file changed, 12 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 768bb8f0c5..20b06721fc 100644 --- a/nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java +++ b/nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java @@ -292,6 +292,14 @@ private static Stream getTypeUseAnnotations( } } + /** + * Should a type-use annotation be treated as applying directly to the top-level type? + * + * @param t the annotation and its position in the type + * @param config NullAway configuration + * @return {@code true} if the annotation should be treated as applying directly to the top-level + * type, false otherwise + */ private static boolean isDirectTypeUseAnnotation(Attribute.TypeCompound t, Config config) { // location is a list of TypePathEntry objects, indicating whether the annotation is // on an array, inner type, wildcard, or type argument. If it's empty, then the @@ -299,8 +307,10 @@ private static boolean isDirectTypeUseAnnotation(Attribute.TypeCompound t, Confi // We care about both annotations directly on the outer type and also those directly // on an inner type or array dimension, but wish to discard annotations on wildcards, // or type arguments. - // For arrays, we treat annotations on the outer type and on any dimension of the array - // as applying to the nullability of the array itself, not the elements. + // For arrays, outside JSpecify mode, we treat annotations on the outer type and on any + // dimension of the array as applying to the nullability of the array itself, not the elements. + // In JSpecify mode, annotations on array dimensions are *not* treated as applying to the + // top-level type, consistent with the JSpecify spec. // We don't allow mixing of inner types and array dimensions in the same location // (i.e. `Foo.@Nullable Bar []` is meaningless). // These aren't correct semantics for type use annotations, but a series of hacky From 622ecc4602bbf330e7de83c720a90e82065bca55 Mon Sep 17 00:00:00 2001 From: Manu Sridharan Date: Fri, 20 Oct 2023 10:05:51 -0700 Subject: [PATCH 06/22] remove extra blank line --- nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java | 1 - 1 file changed, 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 20b06721fc..ba41534119 100644 --- a/nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java +++ b/nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java @@ -318,7 +318,6 @@ private static boolean isDirectTypeUseAnnotation(Attribute.TypeCompound t, Confi // proper deprecation of the incorrect behaviors for type use annotations when their // semantics don't match those of a declaration annotation in the same position. // See https://github.com/uber/NullAway/issues/708 - boolean locationHasInnerTypes = false; boolean locationHasArray = false; for (TypePathEntry entry : t.position.location) { From 7f9947bd03b1f7c759a0f1a4681af8db63a78915 Mon Sep 17 00:00:00 2001 From: Md Armughanuddin Date: Fri, 20 Oct 2023 15:26:36 -0700 Subject: [PATCH 07/22] Fixed docs --- .../com/uber/nullaway/NullabilityUtil.java | 4 +- .../nullaway/NullAwayJSpecifyArrayTests.java | 43 +++++++++---------- 2 files changed, 21 insertions(+), 26 deletions(-) diff --git a/nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java b/nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java index ba41534119..cc1d116a3f 100644 --- a/nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java +++ b/nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java @@ -326,10 +326,8 @@ private static boolean isDirectTypeUseAnnotation(Attribute.TypeCompound t, Confi locationHasInnerTypes = true; break; case ARRAY: - // Currently we are ignoring @Nullable annotations on type in JSpecify mode. - // Eventually, this should return true, and array access should be handled - // elsewhere. if (config.isJSpecifyMode()) { + // Ignoring @Nullable annotations on array dimensions in JSpecify mode return false; } locationHasArray = true; diff --git a/nullaway/src/test/java/com/uber/nullaway/NullAwayJSpecifyArrayTests.java b/nullaway/src/test/java/com/uber/nullaway/NullAwayJSpecifyArrayTests.java index dafc4865f0..febf7e4506 100644 --- a/nullaway/src/test/java/com/uber/nullaway/NullAwayJSpecifyArrayTests.java +++ b/nullaway/src/test/java/com/uber/nullaway/NullAwayJSpecifyArrayTests.java @@ -8,25 +8,24 @@ public class NullAwayJSpecifyArrayTests extends NullAwayTestsBase { @Test - public void ArrayTypeAnnotationDereference() { + public void arrayDimensionAnnotationDereference() { makeHelper() .addSourceLines( "Test.java", "package com.uber;", "import org.jspecify.annotations.Nullable;", "class Test {", - " static @Nullable Integer [] fizz = {1};", + " static Integer @Nullable [] fizz = {1};", " static void foo() {", - " // OK: @Nullable annotations on type are ignored currently.", - " // however, this should report an error eventually.", - " int bar = fizz.length;", + " // BUG: Diagnostic contains: dereferenced expression fizz is @Nullable", + " int bar = fizz.length;", " }", "}") .doTest(); } @Test - public void ArrayTypeAnnotationAssignment() { + public void arrayDimensionAnnotationAssignment() { makeHelper() .addSourceLines( "Test.java", @@ -34,37 +33,35 @@ public void ArrayTypeAnnotationAssignment() { "import org.jspecify.annotations.Nullable;", "class Test {", " Object foo = new Object();", - " void m( @Nullable Integer [] bar) {", - " // OK: @Nullable annotations on type are ignored currently.", - " // however, this should report an error eventually.", - " foo = bar;", + " void m( Integer @Nullable [] bar) {", + " // BUG: Diagnostic contains: assigning @Nullable expression to @NonNull field", + " foo = bar;", " }", "}") .doTest(); } + @Ignore("Array type annotations aren't supported currently.") @Test - public void ArrayElementAnnotationDereference() { + public void arrayTypeAnnotationDereference() { makeHelper() .addSourceLines( "Test.java", "package com.uber;", "import org.jspecify.annotations.Nullable;", "class Test {", - " static String @Nullable [] fizz = {\"1\"};", + " static @Nullable String [] fizz = {\"1\"};", " static void foo() {", - " // This currently reports an error since fizz is @Nullable,", - " // but it should eventually report due to fizz[0] being @Nullable", - " // BUG: Diagnostic contains: dereferenced expression fizz is @Nullable", - " int bar = fizz[0].length();", + " // BUG: Diagnostic contains: dereferenced expression fizz[0] is @Nullable. ", + " int bar = fizz[0].length();", " }", "}") .doTest(); } - @Ignore("We do not support annotations on array elements currently") + @Ignore("Array type annotations aren't supported currently.") @Test - public void ArrayElementAnnotationAssignment() { + public void arrayTypeAnnotationAssignment() { makeHelper() .addSourceLines( "Test.java", @@ -73,11 +70,11 @@ public void ArrayElementAnnotationAssignment() { "class Test {", " Object fizz = new Object();", " Object bar = new Object();", - " void m( Integer @Nullable [] foo) {", - " // BUG: assigning @Nullable expression to @NonNull field", - " fizz = foo[0];", - " // OK: valid assignment since only elements can be null", - " bar = foo;", + " void m( @Nullable Integer [] foo) {", + " // BUG: Diagnostic contains: assigning @Nullable expression to @NonNull field", + " fizz = foo[0];", + " // OK: valid assignment since only elements can be null", + " bar = foo;", " }", "}") .doTest(); From b02eeddbfacaf4d96281494aafea4bde0880c24c Mon Sep 17 00:00:00 2001 From: Md Armughanuddin Date: Fri, 20 Oct 2023 15:28:12 -0700 Subject: [PATCH 08/22] Fixed docs --- 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 cc1d116a3f..d1d4ec3422 100644 --- a/nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java +++ b/nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java @@ -327,7 +327,7 @@ private static boolean isDirectTypeUseAnnotation(Attribute.TypeCompound t, Confi break; case ARRAY: if (config.isJSpecifyMode()) { - // Ignoring @Nullable annotations on array dimensions in JSpecify mode + // Ignoring @Nullable annotations on array type in JSpecify mode return false; } locationHasArray = true; From 0389903b50cee41578bb1dae192a0df660e64c4e Mon Sep 17 00:00:00 2001 From: Md Armughanuddin Date: Fri, 20 Oct 2023 15:33:28 -0700 Subject: [PATCH 09/22] remove space --- .../test/java/com/uber/nullaway/NullAwayJSpecifyArrayTests.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/nullaway/src/test/java/com/uber/nullaway/NullAwayJSpecifyArrayTests.java b/nullaway/src/test/java/com/uber/nullaway/NullAwayJSpecifyArrayTests.java index febf7e4506..5a4c1fd759 100644 --- a/nullaway/src/test/java/com/uber/nullaway/NullAwayJSpecifyArrayTests.java +++ b/nullaway/src/test/java/com/uber/nullaway/NullAwayJSpecifyArrayTests.java @@ -52,7 +52,7 @@ public void arrayTypeAnnotationDereference() { "class Test {", " static @Nullable String [] fizz = {\"1\"};", " static void foo() {", - " // BUG: Diagnostic contains: dereferenced expression fizz[0] is @Nullable. ", + " // BUG: Diagnostic contains: dereferenced expression fizz[0] is @Nullable.", " int bar = fizz[0].length();", " }", "}") From 4fd6df69c6a65fc0e26dbabba860ccfbe0f66eb6 Mon Sep 17 00:00:00 2001 From: Md Armughanuddin Date: Sat, 21 Oct 2023 19:52:47 -0700 Subject: [PATCH 10/22] Update JSpecify escape comment Co-authored-by: Manu Sridharan --- 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 d1d4ec3422..dd5f890698 100644 --- a/nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java +++ b/nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java @@ -327,7 +327,7 @@ private static boolean isDirectTypeUseAnnotation(Attribute.TypeCompound t, Confi break; case ARRAY: if (config.isJSpecifyMode()) { - // Ignoring @Nullable annotations on array type in JSpecify mode + // In JSpecify mode, annotations on array element types do not apply to the top-level type return false; } locationHasArray = true; From ac5598b201eb6c7e79de8c5810eb5e9c018948ad Mon Sep 17 00:00:00 2001 From: Md Armughanuddin Date: Sat, 21 Oct 2023 19:53:16 -0700 Subject: [PATCH 11/22] update test cases with TODO --- .../java/com/uber/nullaway/NullAwayJSpecifyArrayTests.java | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/nullaway/src/test/java/com/uber/nullaway/NullAwayJSpecifyArrayTests.java b/nullaway/src/test/java/com/uber/nullaway/NullAwayJSpecifyArrayTests.java index 5a4c1fd759..eefd964b95 100644 --- a/nullaway/src/test/java/com/uber/nullaway/NullAwayJSpecifyArrayTests.java +++ b/nullaway/src/test/java/com/uber/nullaway/NullAwayJSpecifyArrayTests.java @@ -2,7 +2,6 @@ import com.google.errorprone.CompilationTestHelper; import java.util.Arrays; -import org.junit.Ignore; import org.junit.Test; public class NullAwayJSpecifyArrayTests extends NullAwayTestsBase { @@ -41,7 +40,6 @@ public void arrayDimensionAnnotationAssignment() { .doTest(); } - @Ignore("Array type annotations aren't supported currently.") @Test public void arrayTypeAnnotationDereference() { makeHelper() @@ -52,14 +50,13 @@ public void arrayTypeAnnotationDereference() { "class Test {", " static @Nullable String [] fizz = {\"1\"};", " static void foo() {", - " // BUG: Diagnostic contains: dereferenced expression fizz[0] is @Nullable.", + " // TODO: This should report an error due to dereference of @Nullable fizz[0]", " int bar = fizz[0].length();", " }", "}") .doTest(); } - @Ignore("Array type annotations aren't supported currently.") @Test public void arrayTypeAnnotationAssignment() { makeHelper() @@ -71,7 +68,7 @@ public void arrayTypeAnnotationAssignment() { " Object fizz = new Object();", " Object bar = new Object();", " void m( @Nullable Integer [] foo) {", - " // BUG: Diagnostic contains: assigning @Nullable expression to @NonNull field", + " // TODO: This should report an error due to assignment of @Nullable fizz[0] to @NonNull field", " fizz = foo[0];", " // OK: valid assignment since only elements can be null", " bar = foo;", From f0379851ac65119fa673dc4c7be82809eaf3f85c Mon Sep 17 00:00:00 2001 From: Md Armughanuddin Date: Sat, 21 Oct 2023 19:56:54 -0700 Subject: [PATCH 12/22] replace ignore with TODO --- nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java | 3 ++- 1 file changed, 2 insertions(+), 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 dd5f890698..1464db0db8 100644 --- a/nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java +++ b/nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java @@ -327,7 +327,8 @@ private static boolean isDirectTypeUseAnnotation(Attribute.TypeCompound t, Confi break; case ARRAY: if (config.isJSpecifyMode()) { - // In JSpecify mode, annotations on array element types do not apply to the top-level type + // In JSpecify mode, annotations on array element types do not apply to the top-level + // type return false; } locationHasArray = true; From c41de6e3f3465f4aeedeea1dced6c265de30e4b0 Mon Sep 17 00:00:00 2001 From: Md Armughanuddin Date: Sun, 22 Oct 2023 21:48:13 -0700 Subject: [PATCH 13/22] FIx comment Co-authored-by: Manu Sridharan --- .../test/java/com/uber/nullaway/NullAwayJSpecifyArrayTests.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/nullaway/src/test/java/com/uber/nullaway/NullAwayJSpecifyArrayTests.java b/nullaway/src/test/java/com/uber/nullaway/NullAwayJSpecifyArrayTests.java index eefd964b95..2ce10d6e9a 100644 --- a/nullaway/src/test/java/com/uber/nullaway/NullAwayJSpecifyArrayTests.java +++ b/nullaway/src/test/java/com/uber/nullaway/NullAwayJSpecifyArrayTests.java @@ -68,7 +68,7 @@ public void arrayTypeAnnotationAssignment() { " Object fizz = new Object();", " Object bar = new Object();", " void m( @Nullable Integer [] foo) {", - " // TODO: This should report an error due to assignment of @Nullable fizz[0] to @NonNull field", + " // TODO: This should report an error due to assignment of @Nullable foo[0] to @NonNull field", " fizz = foo[0];", " // OK: valid assignment since only elements can be null", " bar = foo;", From 49cf926dcb146338acfa1c3cdf18ff753bbd683c Mon Sep 17 00:00:00 2001 From: Md Armughanuddin Date: Sun, 22 Oct 2023 21:52:15 -0700 Subject: [PATCH 14/22] update unit test names --- .../java/com/uber/nullaway/NullAwayJSpecifyArrayTests.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/nullaway/src/test/java/com/uber/nullaway/NullAwayJSpecifyArrayTests.java b/nullaway/src/test/java/com/uber/nullaway/NullAwayJSpecifyArrayTests.java index 2ce10d6e9a..6db19fea18 100644 --- a/nullaway/src/test/java/com/uber/nullaway/NullAwayJSpecifyArrayTests.java +++ b/nullaway/src/test/java/com/uber/nullaway/NullAwayJSpecifyArrayTests.java @@ -7,7 +7,7 @@ public class NullAwayJSpecifyArrayTests extends NullAwayTestsBase { @Test - public void arrayDimensionAnnotationDereference() { + public void arrayTopLevelAnnotationDereference() { makeHelper() .addSourceLines( "Test.java", @@ -24,7 +24,7 @@ public void arrayDimensionAnnotationDereference() { } @Test - public void arrayDimensionAnnotationAssignment() { + public void arrayTopLevelAnnotationAssignment() { makeHelper() .addSourceLines( "Test.java", From d7783defb4d7814fe4015b1bb2dbfccc47b773cf Mon Sep 17 00:00:00 2001 From: Md Armughanuddin Date: Mon, 23 Oct 2023 16:22:49 -0700 Subject: [PATCH 15/22] Update test names Co-authored-by: Manu Sridharan --- .../test/java/com/uber/nullaway/NullAwayJSpecifyArrayTests.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/nullaway/src/test/java/com/uber/nullaway/NullAwayJSpecifyArrayTests.java b/nullaway/src/test/java/com/uber/nullaway/NullAwayJSpecifyArrayTests.java index 6db19fea18..b7d889d3ec 100644 --- a/nullaway/src/test/java/com/uber/nullaway/NullAwayJSpecifyArrayTests.java +++ b/nullaway/src/test/java/com/uber/nullaway/NullAwayJSpecifyArrayTests.java @@ -41,7 +41,7 @@ public void arrayTopLevelAnnotationAssignment() { } @Test - public void arrayTypeAnnotationDereference() { + public void arrayContentsAnnotationDereference() { makeHelper() .addSourceLines( "Test.java", From 5726080dda133769c31edd2a7d872fe67e258d39 Mon Sep 17 00:00:00 2001 From: Md Armughanuddin Date: Mon, 23 Oct 2023 16:22:57 -0700 Subject: [PATCH 16/22] Update test name Co-authored-by: Manu Sridharan --- .../test/java/com/uber/nullaway/NullAwayJSpecifyArrayTests.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/nullaway/src/test/java/com/uber/nullaway/NullAwayJSpecifyArrayTests.java b/nullaway/src/test/java/com/uber/nullaway/NullAwayJSpecifyArrayTests.java index b7d889d3ec..ff1f92bf81 100644 --- a/nullaway/src/test/java/com/uber/nullaway/NullAwayJSpecifyArrayTests.java +++ b/nullaway/src/test/java/com/uber/nullaway/NullAwayJSpecifyArrayTests.java @@ -58,7 +58,7 @@ public void arrayContentsAnnotationDereference() { } @Test - public void arrayTypeAnnotationAssignment() { + public void arrayContentsAnnotationAssignment() { makeHelper() .addSourceLines( "Test.java", From 23802f25579c44c19b945ba17ccf07b8e0fed292 Mon Sep 17 00:00:00 2001 From: Manu Sridharan Date: Wed, 25 Oct 2023 16:20:28 +0100 Subject: [PATCH 17/22] Update nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Lázaro Clapp --- .../src/main/java/com/uber/nullaway/NullabilityUtil.java | 5 ++++- 1 file changed, 4 insertions(+), 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 1464db0db8..4a94745d97 100644 --- a/nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java +++ b/nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java @@ -293,7 +293,10 @@ private static Stream getTypeUseAnnotations( } /** - * Should a type-use annotation be treated as applying directly to the top-level type? + * Check whether a type-use annotation should be treated as applying directly to the top-level type + * + * For example {@code @Nullable List lst} is a direct type use annotation of {@code lst}, + * but {@code List<@Nullable T> lst} is not. * * @param t the annotation and its position in the type * @param config NullAway configuration From c139e13285c3fe1ae493e20bae844beeed64c502 Mon Sep 17 00:00:00 2001 From: Manu Sridharan Date: Wed, 25 Oct 2023 16:31:39 +0100 Subject: [PATCH 18/22] formatting --- .../src/main/java/com/uber/nullaway/NullabilityUtil.java | 7 ++++--- 1 file changed, 4 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 4a94745d97..2a04d46cb1 100644 --- a/nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java +++ b/nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java @@ -293,10 +293,11 @@ private static Stream getTypeUseAnnotations( } /** - * Check whether a type-use annotation should be treated as applying directly to the top-level type + * Check whether a type-use annotation should be treated as applying directly to the top-level + * type * - * For example {@code @Nullable List lst} is a direct type use annotation of {@code lst}, - * but {@code List<@Nullable T> lst} is not. + *

For example {@code @Nullable List lst} is a direct type use annotation of {@code lst}, + * but {@code List<@Nullable T> lst} is not. * * @param t the annotation and its position in the type * @param config NullAway configuration From c328b82275d95b209eda838cc928db92221b4cfa Mon Sep 17 00:00:00 2001 From: Md Armughanuddin Date: Wed, 25 Oct 2023 08:53:37 -0700 Subject: [PATCH 19/22] add true negative case for element annotation --- .../java/com/uber/nullaway/NullAwayJSpecifyArrayTests.java | 3 +++ 1 file changed, 3 insertions(+) diff --git a/nullaway/src/test/java/com/uber/nullaway/NullAwayJSpecifyArrayTests.java b/nullaway/src/test/java/com/uber/nullaway/NullAwayJSpecifyArrayTests.java index ff1f92bf81..c9762607fd 100644 --- a/nullaway/src/test/java/com/uber/nullaway/NullAwayJSpecifyArrayTests.java +++ b/nullaway/src/test/java/com/uber/nullaway/NullAwayJSpecifyArrayTests.java @@ -49,9 +49,12 @@ public void arrayContentsAnnotationDereference() { "import org.jspecify.annotations.Nullable;", "class Test {", " static @Nullable String [] fizz = {\"1\"};", + " static Object foo = new Object();", " static void foo() {", " // TODO: This should report an error due to dereference of @Nullable fizz[0]", " int bar = fizz[0].length();", + " // OK: valid dereference since only elements can be null", + " foo = fizz.length;", " }", "}") .doTest(); From 48824bb12aa8afddafdcf3653489ecf8ac303a19 Mon Sep 17 00:00:00 2001 From: Md Armughanuddin Date: Wed, 25 Oct 2023 14:24:22 -0700 Subject: [PATCH 20/22] add declaration annotation tests --- .../nullaway/NullAwayJSpecifyArrayTests.java | 29 +++++++++++++++++-- 1 file changed, 27 insertions(+), 2 deletions(-) diff --git a/nullaway/src/test/java/com/uber/nullaway/NullAwayJSpecifyArrayTests.java b/nullaway/src/test/java/com/uber/nullaway/NullAwayJSpecifyArrayTests.java index c9762607fd..8c93baa88a 100644 --- a/nullaway/src/test/java/com/uber/nullaway/NullAwayJSpecifyArrayTests.java +++ b/nullaway/src/test/java/com/uber/nullaway/NullAwayJSpecifyArrayTests.java @@ -69,12 +69,37 @@ public void arrayContentsAnnotationAssignment() { "import org.jspecify.annotations.Nullable;", "class Test {", " Object fizz = new Object();", - " Object bar = new Object();", " void m( @Nullable Integer [] foo) {", " // TODO: This should report an error due to assignment of @Nullable foo[0] to @NonNull field", " fizz = foo[0];", " // OK: valid assignment since only elements can be null", - " bar = foo;", + " fizz = foo;", + " }", + "}") + .doTest(); + } + + /* Currently in JSpecify mode, JSpecify syntax only applies to + type-use annotations. Declaration annotations preserve their existing behavior, + with annotations being treated on the top-level type. + */ + @Test + public void arrayDeclarationAnnotation() { + makeHelper() + .addSourceLines( + "Test.java", + "package com.uber;", + "import javax.annotation.Nullable;", + "class Test {", + " static @Nullable String [] fizz = {\"1\"};", + " static Object o1 = new Object();", + " static void foo() {", + " // This should not report an error while using JSpecify type-use annotation", + " // BUG: Diagnostic contains: assigning @Nullable expression to @NonNull field", + " o1 = fizz;", + " // This should not report an error while using JSpecify type-use annotation", + " // BUG: Diagnostic contains: dereferenced expression fizz is @Nullable", + " o1 = fizz.length;", " }", "}") .doTest(); From 5816c99f58dfb9ea8e8452b61977c3268836382e Mon Sep 17 00:00:00 2001 From: Manu Sridharan Date: Wed, 25 Oct 2023 23:19:18 +0100 Subject: [PATCH 21/22] add test for fizz[0] when fizz is @Nullable --- .../com/uber/nullaway/NullAwayJSpecifyArrayTests.java | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/nullaway/src/test/java/com/uber/nullaway/NullAwayJSpecifyArrayTests.java b/nullaway/src/test/java/com/uber/nullaway/NullAwayJSpecifyArrayTests.java index 8c93baa88a..f727e00256 100644 --- a/nullaway/src/test/java/com/uber/nullaway/NullAwayJSpecifyArrayTests.java +++ b/nullaway/src/test/java/com/uber/nullaway/NullAwayJSpecifyArrayTests.java @@ -16,8 +16,12 @@ public void arrayTopLevelAnnotationDereference() { "class Test {", " static Integer @Nullable [] fizz = {1};", " static void foo() {", - " // BUG: Diagnostic contains: dereferenced expression fizz is @Nullable", - " int bar = fizz.length;", + " // BUG: Diagnostic contains: dereferenced expression fizz is @Nullable", + " int bar = fizz.length;", + " }", + " static void bar() {", + " // BUG: Diagnostic contains: dereferenced expression fizz is @Nullable", + " int bar = fizz[0];", " }", "}") .doTest(); @@ -53,7 +57,7 @@ public void arrayContentsAnnotationDereference() { " static void foo() {", " // TODO: This should report an error due to dereference of @Nullable fizz[0]", " int bar = fizz[0].length();", - " // OK: valid dereference since only elements can be null", + " // OK: valid dereference since only elements of the array can be null", " foo = fizz.length;", " }", "}") From b6318afe517f7c7c2882f5989b840d3c191d9510 Mon Sep 17 00:00:00 2001 From: Manu Sridharan Date: Wed, 25 Oct 2023 23:21:35 +0100 Subject: [PATCH 22/22] use javadoc --- .../java/com/uber/nullaway/NullAwayJSpecifyArrayTests.java | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/nullaway/src/test/java/com/uber/nullaway/NullAwayJSpecifyArrayTests.java b/nullaway/src/test/java/com/uber/nullaway/NullAwayJSpecifyArrayTests.java index f727e00256..54267859fb 100644 --- a/nullaway/src/test/java/com/uber/nullaway/NullAwayJSpecifyArrayTests.java +++ b/nullaway/src/test/java/com/uber/nullaway/NullAwayJSpecifyArrayTests.java @@ -83,9 +83,10 @@ public void arrayContentsAnnotationAssignment() { .doTest(); } - /* Currently in JSpecify mode, JSpecify syntax only applies to - type-use annotations. Declaration annotations preserve their existing behavior, - with annotations being treated on the top-level type. + /** + * Currently in JSpecify mode, JSpecify syntax only applies to type-use annotations. Declaration + * annotations preserve their existing behavior, with annotations being treated on the top-level + * type. We will very likely revisit this design in the future. */ @Test public void arrayDeclarationAnnotation() {