Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

JSpecify: Modify Array Type Use Annotation Syntax #850

Merged
merged 26 commits into from
Oct 25, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
f1ba82a
Modify JSpecify Array Handling
armughan11 Oct 18, 2023
4bdd570
Fix incorrect wildcard/inner type handling in JSpecify mode
armughan11 Oct 19, 2023
06e816e
Fix Unit Tests
armughan11 Oct 19, 2023
6a19f36
Update comment
armughan11 Oct 19, 2023
d1e0cb2
Merge branch 'master' into jspecify-array-handling
armughan11 Oct 19, 2023
2b6d746
add some documentation
msridhar Oct 20, 2023
622ecc4
remove extra blank line
msridhar Oct 20, 2023
96bc62b
Merge branch 'uber:master' into jspecify-array-handling
armughan11 Oct 20, 2023
7f9947b
Fixed docs
armughan11 Oct 20, 2023
b02eedd
Fixed docs
armughan11 Oct 20, 2023
0389903
remove space
armughan11 Oct 20, 2023
4fd6df6
Update JSpecify escape comment
armughan11 Oct 22, 2023
ac5598b
update test cases with TODO
armughan11 Oct 22, 2023
b1a3f5c
Merge remote-tracking branch 'origin/jspecify-array-handling' into js…
armughan11 Oct 22, 2023
f037985
replace ignore with TODO
armughan11 Oct 22, 2023
c41de6e
FIx comment
armughan11 Oct 23, 2023
49cf926
update unit test names
armughan11 Oct 23, 2023
d7783de
Update test names
armughan11 Oct 23, 2023
5726080
Update test name
armughan11 Oct 23, 2023
23802f2
Update nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java
msridhar Oct 25, 2023
adcce63
Merge branch 'master' into jspecify-array-handling
msridhar Oct 25, 2023
c139e13
formatting
msridhar Oct 25, 2023
c328b82
add true negative case for element annotation
armughan11 Oct 25, 2023
48824bb
add declaration annotation tests
armughan11 Oct 25, 2023
5816c99
add test for fizz[0] when fizz is @Nullable
msridhar Oct 25, 2023
b6318af
use javadoc
msridhar Oct 25, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions nullaway/src/main/java/com/uber/nullaway/NullAway.java
Original file line number Diff line number Diff line change
Expand Up @@ -1962,7 +1962,8 @@ private SetMultimap<MethodTree, Symbol> 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);
}
Expand Down Expand Up @@ -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);
}
Expand Down
38 changes: 30 additions & 8 deletions nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java
Original file line number Diff line number Diff line change
Expand Up @@ -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<? extends AnnotationMirror> getAllAnnotations(Symbol symbol) {
public static Stream<? extends AnnotationMirror> getAllAnnotations(Symbol symbol, Config config) {
// for methods, we care about annotations on the return type, not on the method type itself
Stream<? extends AnnotationMirror> typeUseAnnotations = getTypeUseAnnotations(symbol);
Stream<? extends AnnotationMirror> typeUseAnnotations = getTypeUseAnnotations(symbol, config);
return Stream.concat(symbol.getAnnotationMirrors().stream(), typeUseAnnotations);
}

Expand Down Expand Up @@ -277,27 +277,44 @@ public static Stream<? extends AnnotationMirror> getAllAnnotationsForParameter(
* Gets the type use annotations on a symbol, ignoring annotations on components of the type (type
* arguments, wildcards, etc.)
*/
private static Stream<? extends AnnotationMirror> getTypeUseAnnotations(Symbol symbol) {
private static Stream<? extends AnnotationMirror> getTypeUseAnnotations(
Symbol symbol, Config config) {
Stream<Attribute.TypeCompound> 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
*
* <p>For example {@code @Nullable List<T> 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
Expand All @@ -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;
msridhar marked this conversation as resolved.
Show resolved Hide resolved
}
locationHasArray = true;
break;
default:
Expand Down
4 changes: 2 additions & 2 deletions nullaway/src/main/java/com/uber/nullaway/Nullness.java
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

/**
Expand All @@ -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);
}

/**
Expand Down
Original file line number Diff line number Diff line change
@@ -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]",
msridhar marked this conversation as resolved.
Show resolved Hide resolved
" 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"));
}
}