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 11 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
33 changes: 25 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 @@
* @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 @@
* 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,10 @@
locationHasInnerTypes = true;
break;
case ARRAY:
if (config.isJSpecifyMode()) {
// Ignoring @Nullable annotations on array type in JSpecify mode
armughan11 marked this conversation as resolved.
Show resolved Hide resolved
return false;

Check warning on line 331 in nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java

View check run for this annotation

Codecov / codecov/patch

nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java#L331

Added line #L331 was not covered by tests
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,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 arrayDimensionAnnotationDereference() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Test needs to be renamed; annotation is now on top-level type

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed!

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;",
msridhar marked this conversation as resolved.
Show resolved Hide resolved
" }",
"}")
.doTest();
}

@Test
public void arrayDimensionAnnotationAssignment() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

another test rename

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed!

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();
}

@Ignore("Array type annotations aren't supported currently.")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Rather than doing @Ignore let's put a TODO comment within the test. The test serves a useful purpose of checking that writing @Nullable String [] does not apply the annotation to the top-level type in JSpecify mode.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed

@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 String [] fizz = {\"1\"};",
" static void foo() {",
" // BUG: Diagnostic contains: dereferenced expression fizz[0] is @Nullable.",
" int bar = fizz[0].length();",
" }",
"}")
.doTest();
}

@Ignore("Array type annotations aren't supported currently.")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same comment as above about not ignoring

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed

@Test
public void arrayTypeAnnotationAssignment() {
armughan11 marked this conversation as resolved.
Show resolved Hide resolved
makeHelper()
.addSourceLines(
"Test.java",
"package com.uber;",
"import org.jspecify.annotations.Nullable;",
"class Test {",
" Object fizz = new Object();",
" Object bar = new Object();",
" 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();
}

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