Skip to content

Commit

Permalink
Modify JSpecify Array Handling
Browse files Browse the repository at this point in the history
Added Unit Tests for @nullable annotations on array in JSpecify mode and modified behaviour.
  • Loading branch information
armughan11 committed Oct 18, 2023
1 parent 5355c7c commit f1ba82a
Show file tree
Hide file tree
Showing 4 changed files with 111 additions and 10 deletions.
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 @@ -1949,7 +1949,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 @@ -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);
}
Expand Down
24 changes: 18 additions & 6 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,19 +277,22 @@ 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) {
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.
Expand All @@ -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) {
Expand All @@ -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.
*
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 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"));
}
}

0 comments on commit f1ba82a

Please sign in to comment.