diff --git a/nullaway/src/main/java/com/uber/nullaway/NullAway.java b/nullaway/src/main/java/com/uber/nullaway/NullAway.java index 5d8dc8304e..d4ce4e827d 100644 --- a/nullaway/src/main/java/com/uber/nullaway/NullAway.java +++ b/nullaway/src/main/java/com/uber/nullaway/NullAway.java @@ -1962,7 +1962,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); } @@ -2219,7 +2220,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..2a04d46cb1 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,27 +277,44 @@ 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) { + /** + * 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 + * @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 // annotation is directly on the type. // 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 @@ -313,6 +330,11 @@ private static boolean isDirectTypeUseAnnotation(Attribute.TypeCompound t) { locationHasInnerTypes = true; break; case ARRAY: + if (config.isJSpecifyMode()) { + // In JSpecify mode, annotations on array element types do not apply to the top-level + // type + return false; + } locationHasArray = true; break; default: 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..54267859fb --- /dev/null +++ b/nullaway/src/test/java/com/uber/nullaway/NullAwayJSpecifyArrayTests.java @@ -0,0 +1,118 @@ +package com.uber.nullaway; + +import com.google.errorprone.CompilationTestHelper; +import java.util.Arrays; +import org.junit.Test; + +public class NullAwayJSpecifyArrayTests extends NullAwayTestsBase { + + @Test + public void arrayTopLevelAnnotationDereference() { + makeHelper() + .addSourceLines( + "Test.java", + "package com.uber;", + "import org.jspecify.annotations.Nullable;", + "class Test {", + " static Integer @Nullable [] fizz = {1};", + " static void foo() {", + " // 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(); + } + + @Test + public void arrayTopLevelAnnotationAssignment() { + makeHelper() + .addSourceLines( + "Test.java", + "package com.uber;", + "import org.jspecify.annotations.Nullable;", + "class Test {", + " Object foo = new Object();", + " void m( Integer @Nullable [] bar) {", + " // BUG: Diagnostic contains: assigning @Nullable expression to @NonNull field", + " foo = bar;", + " }", + "}") + .doTest(); + } + + @Test + public void arrayContentsAnnotationDereference() { + makeHelper() + .addSourceLines( + "Test.java", + "package com.uber;", + "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 of the array can be null", + " foo = fizz.length;", + " }", + "}") + .doTest(); + } + + @Test + public void arrayContentsAnnotationAssignment() { + makeHelper() + .addSourceLines( + "Test.java", + "package com.uber;", + "import org.jspecify.annotations.Nullable;", + "class Test {", + " Object fizz = 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", + " 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. We will very likely revisit this design in the future. + */ + @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(); + } + + private CompilationTestHelper makeHelper() { + return makeTestHelperWithArgs( + Arrays.asList( + "-XepOpt:NullAway:AnnotatedPackages=com.uber", "-XepOpt:NullAway:JSpecifyMode=true")); + } +}