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 7 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
35 changes: 27 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,40 @@ 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) {
/**
* Should a type-use annotation be treated as applying directly to the top-level type?
msridhar marked this conversation as resolved.
Show resolved Hide resolved
*
* @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 +326,12 @@ private static boolean isDirectTypeUseAnnotation(Attribute.TypeCompound t) {
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.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we say this should eventually return true? I added some docs to the enclosing method, and I think what it is meant to do is determine whether an annotation applies to a top-level type. Given that, shouldn't this method always return false in this spot in JSpecify mode?

Copy link
Collaborator Author

@armughan11 armughan11 Oct 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this and all subsequent comments are attributed to my fallacy that it works the other way around. All of these are actually working as we want them to. Will clean these up!

if (config.isJSpecifyMode()) {
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,91 @@
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() {
armughan11 marked this conversation as resolved.
Show resolved Hide resolved
makeHelper()
.addSourceLines(
"Test.java",
"package com.uber;",
"import org.jspecify.annotations.Nullable;",
"class Test {",
" static @Nullable Integer [] fizz = {1};",
" static void foo() {",
" // OK: @Nullable annotations on type are ignored currently.",
" // however, this should report an error eventually.",
" int bar = fizz.length;",
armughan11 marked this conversation as resolved.
Show resolved Hide resolved
" }",
"}")
.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.",
armughan11 marked this conversation as resolved.
Show resolved Hide resolved
" 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() {",
" // 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();",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This error is correct I think.

" }",
"}")
.doTest();
}

@Ignore("We do not support annotations on array elements currently")
@Test
public void ArrayElementAnnotationAssignment() {
makeHelper()
.addSourceLines(
"Test.java",
"package com.uber;",
"import org.jspecify.annotations.Nullable;",
"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;",
" }",
"}")
.doTest();
}

private CompilationTestHelper makeHelper() {
return makeTestHelperWithArgs(
Arrays.asList(
"-XepOpt:NullAway:AnnotatedPackages=com.uber", "-XepOpt:NullAway:JSpecifyMode=true"));
}
}