From f1ba82a6b8caf55b58c128a2c405e6e498213ac2 Mon Sep 17 00:00:00 2001 From: Md Date: Wed, 18 Oct 2023 15:03:10 -0700 Subject: [PATCH] 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")); + } +}