Skip to content

Commit

Permalink
fix review comments
Browse files Browse the repository at this point in the history
  • Loading branch information
@armughan11
armughan11 committed Oct 9, 2024
1 parent 49bb92a commit b1e3bc1
Show file tree
Hide file tree
Showing 3 changed files with 90 additions and 74 deletions.
43 changes: 27 additions & 16 deletions nullaway/src/main/java/com/uber/nullaway/NullAway.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -581,7 +581,7 @@ public Description matchMemberSelect(MemberSelectTree tree, VisitorState state)
}
if ((tree.getExpression() instanceof AnnotatedTypeTree)
&& !config.isLegacyAnnotationLocation()) {
handleNullabilityOnNestedClass(
checkNullableAnnotationPositionInType(
((AnnotatedTypeTree) tree.getExpression()).getAnnotations(), tree, tree, state);
}

Expand Down Expand Up @@ -653,7 +653,7 @@ public Description matchMethod(MethodTree tree, VisitorState state) {
return Description.NO_MATCH;
}
if (!config.isLegacyAnnotationLocation()) {
handleNullabilityOnNestedClass(
checkNullableAnnotationPositionInType(
tree.getModifiers().getAnnotations(), tree.getReturnType(), tree, state);
}
// if the method is overriding some other method,
Expand Down Expand Up @@ -1483,7 +1483,7 @@ public Description matchVariable(VariableTree tree, VisitorState state) {
GenericsChecks.checkTypeParameterNullnessForAssignability(tree, this, state);
}
if (!config.isLegacyAnnotationLocation()) {
handleNullabilityOnNestedClass(
checkNullableAnnotationPositionInType(
tree.getModifiers().getAnnotations(), tree.getType(), tree, state);
}

Expand All @@ -1509,7 +1509,7 @@ public Description matchVariable(VariableTree tree, VisitorState state) {
return Description.NO_MATCH;
}

private static boolean isOnlyTypeAnnotation(Symbol anno) {
private static boolean isTypeUseAnnotation(Symbol anno) {
Target target = anno.getAnnotation(Target.class);
ImmutableSet<ElementType> elementTypes =
target == null ? ImmutableSet.of() : ImmutableSet.copyOf(target.value());
Expand All @@ -1522,30 +1522,42 @@ private static boolean isDeclarationAnnotation(Symbol anno) {
return true;
}
ImmutableSet<ElementType> elementTypes = ImmutableSet.copyOf(target.value());
// Return true only if annotation is not type-use only
// Return true for any annotation that is not exclusively a type-use annotation
return !(elementTypes.equals(ImmutableSet.of(ElementType.TYPE_USE))
|| TYPE_USE_OR_TYPE_PARAMETER.containsAll(elementTypes));
}

private void handleNullabilityOnNestedClass(
List<? extends AnnotationTree> annotations,
Tree typeTree,
Tree errorReportingTree,
VisitorState state) {
if (!(typeTree instanceof JCTree.JCFieldAccess)) {
/**
* Checks whether the annotation is at the right location for nested types. Raises an error iff
* the type is a field access expression, the annotation is (or also) type-use and the annotation
* is not applied on the innermost type.
*
* @param annotations The annotations to check
* @param type The tree representing the type structure
* @param tree The tree context (variable, member select or method)
* @param state The visitor state
*/
private void checkNullableAnnotationPositionInType(
List<? extends AnnotationTree> annotations, Tree type, Tree tree, VisitorState state) {

// Early return if the type is not a nested or inner class reference.

if (!(type instanceof MemberSelectTree)) {
return;
}
JCTree.JCFieldAccess fieldAccess = (JCTree.JCFieldAccess) typeTree;
MemberSelectTree fieldAccess = (MemberSelectTree) type;

// Get the end position of the outer type expression. Any nullable annotation before this
// position is considered to be on the outer type, which is incorrect.
int endOfOuterType = state.getEndPosition(fieldAccess.getExpression());
int startOfType = ((JCTree) typeTree).getStartPosition();
int startOfType = ((JCTree) type).getStartPosition();

for (AnnotationTree annotation : annotations) {
Symbol sym = ASTHelpers.getSymbol(annotation);
if (sym == null) {
continue;
}
if (!isOnlyTypeAnnotation(sym)) {
if (!isTypeUseAnnotation(sym)) {
continue;
}
// If an annotation is declaration ALSO, we check if it is at the correct location. If it is,
Expand All @@ -1567,8 +1579,7 @@ private void handleNullabilityOnNestedClass(
"Type-use nullability annotations should be applied on inner class");

state.reportMatch(
errorBuilder.createErrorDescription(
errorMessage, buildDescription(errorReportingTree), state, null));
errorBuilder.createErrorDescription(errorMessage, buildDescription(tree), state, null));
}
}
}
Expand Down
10 changes: 7 additions & 3 deletions nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -325,6 +325,7 @@ public static Stream<? extends AnnotationMirror> getTypeUseAnnotations(
* but {@code List<@Nullable T> lst} is not.
*
* @param t the annotation and its position in the type
* @param symbol the method symbol
* @param config NullAway configuration
* @return {@code true} if the annotation should be treated as applying directly to the top-level
* type, false otherwise
Expand All @@ -343,9 +344,9 @@ private static boolean isDirectTypeUseAnnotation(
// In JSpecify mode and without the LegacyAnnotationLocations flag, annotations on array
// dimensions are *not* treated as applying to the top-level type, consistent with the JSpecify
// spec.
// Outside of JSpecify mode, annotations which are *not* on the inner type are not treated as
// being applied to the inner type. This is bypassed when the LegacyAnnotationLocations flag is
// passed, in which case annotations on all locations are treated as applying to the inner type.
// Annotations which are *not* on the inner type are not treated as being applied to the inner
// type. This can be bypassed the LegacyAnnotationLocations flag, in which
// annotations on all locations are treated as applying to the inner type.
// 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 Down Expand Up @@ -380,11 +381,14 @@ private static boolean isDirectTypeUseAnnotation(
// Make sure it's not a mix of inner types and arrays for this annotation's location
return !(locationHasInnerTypes && locationHasArray);
}
// For non-nested classes if there are any inner types in the annotation location the annotation
// is considered valid to allow annotations on type arguments.
if (!hasNestedClass(symbol.type)) {
if (innerTypeCount > 0) {
return true;
}
}
// For nested classes the annotation is only valid if it is on the innermost type.
return innerTypeCount == nestingDepth - 1;
}

Expand Down
111 changes: 56 additions & 55 deletions nullaway/src/test/java/com/uber/nullaway/TypeUseAnnotationsTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -154,14 +154,14 @@ public void annotationAppliedToInnerTypeExplicitly2() {
"package com.uber;",
"import org.checkerframework.checker.nullness.qual.Nullable;",
"class Test {",
" Bar.@Nullable Foo f1;",
" // @Nullable does not apply to the inner type",
" // BUG: Diagnostic contains: @NonNull field f2 not initialized",
" @Nullable Bar.Foo f2;",
" public void test() {",
" // BUG: Diagnostic contains: dereferenced",
" f1.hashCode();",
" f2.hashCode();",
" Bar.@Nullable Foo f1;",
" // @Nullable does not apply to the inner type",
" // BUG: Diagnostic contains: @NonNull field f2 not initialized",
" @Nullable Bar.Foo f2;",
" public void test() {",
" // BUG: Diagnostic contains: dereferenced",
" f1.hashCode();",
" f2.hashCode();",
" }",
"}")
.doTest();
Expand Down Expand Up @@ -196,15 +196,16 @@ public void typeUseAnnotationOnInnerMultiLevel() {
"import org.checkerframework.checker.nullness.qual.Nullable;",
"class A { class B { class C {} } }",
"class Test {",
" // BUG: Diagnostic contains: Type-use nullability annotations should be applied on inner class",
" @Nullable A.B.C foo1 = null;",
" // BUG: Diagnostic contains: Type-use nullability annotations should be applied on inner class",
" A.@Nullable B.C foo2 = null;",
" A.B.@Nullable C foo3 = null;",
" // BUG: Diagnostic contains: Type-use nullability annotations should be applied on inner class",
" // BUG: Diagnostic contains: Type-use nullability annotations should be applied on inner class",
" @Nullable A.B.C foo1 = null;",
" // BUG: Diagnostic contains: Type-use nullability annotations should be applied on inner class",
" A.@Nullable B.C foo2 = null;",
" A.B.@Nullable C foo3 = null;",
" // BUG: Diagnostic contains: Type-use nullability annotations should be applied on inner class",
" @Nullable A.B foo4 = null;",
" // BUG: Diagnostic contains: assigning @Nullable expression to @NonNull field",
" A.B.@Nullable C [][] foo5 = null;",
" // BUG: Diagnostic contains: assigning @Nullable expression to @NonNull field",
" A.B.@Nullable C [][] foo5 = null;",
" A.B.C @Nullable [][] foo6 = null;",
"}")
.doTest();
}
Expand All @@ -218,12 +219,13 @@ public void typeUseAnnotationOnInnerMultiLevelLegacy() {
"import org.checkerframework.checker.nullness.qual.Nullable;",
"class A { class B { class C {} } }",
"class Test {",
" @Nullable A.B.C foo1 = null;",
" A.@Nullable B.C foo2 = null;",
" A.B.@Nullable C foo3 = null;",
" @Nullable A.B foo4 = null;",
" // BUG: Diagnostic contains: assigning @Nullable expression to @NonNull field",
" A.B.@Nullable C [][] foo5 = null;",
" @Nullable A.B.C foo1 = null;",
" A.@Nullable B.C foo2 = null;",
" A.B.@Nullable C foo3 = null;",
" @Nullable A.B foo4 = null;",
" // BUG: Diagnostic contains: assigning @Nullable expression to @NonNull field",
" A.B.@Nullable C [][] foo5 = null;",
" A.B.C @Nullable [][] foo6 = null;",
"}")
.doTest();
}
Expand All @@ -237,7 +239,7 @@ public void declarationAnnotationOnInnerMultiLevel() {
"import javax.annotation.Nullable;",
"class A { class B { class C {} } }",
"class Test {",
" @Nullable A.B.C foo1 = null;",
" @Nullable A.B.C foo1 = null;",
" @Nullable A.B foo4 = null;",
"}")
.doTest();
Expand All @@ -258,15 +260,15 @@ public void typeUseAndDeclarationAnnotationOnInnerMultiLevel() {
"package com.uber;",
"class A { class B { class C {} } }",
"class Test {",
" // ok, treated as declaration",
" @Nullable A.B.C foo1 = null;",
" // not ok, invalid location for both type-use and declaration",
" // BUG: Diagnostic contains: Type-use nullability annotations should be applied on inner class",
" A.@Nullable B.C foo2 = null;",
" // ok, treated as type-use",
" A.B.@Nullable C foo3 = null;",
" // ok, treated as declaration",
" @Nullable A.B foo4 = null;",
" // ok, treated as declaration",
" @Nullable A.B.C foo1 = null;",
" // not ok, invalid location for both type-use and declaration",
" // BUG: Diagnostic contains: Type-use nullability annotations should be applied on inner class",
" A.@Nullable B.C foo2 = null;",
" // ok, treated as type-use",
" A.B.@Nullable C foo3 = null;",
" // ok, treated as declaration",
" @Nullable A.B foo4 = null;",
"}")
.doTest();
}
Expand All @@ -286,14 +288,14 @@ public void typeUseAndDeclarationAnnotationOnInnerMultiLevelLegacy() {
"package com.uber;",
"class A { class B { class C {} } }",
"class Test {",
" // ok, treated as declaration",
" @Nullable A.B.C foo1 = null;",
" // ok, treated as type-use",
" A.@Nullable B.C foo2 = null;",
" // ok, treated as type-use",
" A.B.@Nullable C foo3 = null;",
" // ok, treated as declaration",
" @Nullable A.B foo4 = null;",
" // ok, treated as declaration",
" @Nullable A.B.C foo1 = null;",
" // ok, treated as type-use",
" A.@Nullable B.C foo2 = null;",
" // ok, treated as type-use",
" A.B.@Nullable C foo3 = null;",
" // ok, treated as declaration",
" @Nullable A.B foo4 = null;",
"}")
.doTest();
}
Expand All @@ -307,15 +309,15 @@ public void typeUseAnnotationOnMethodReturnType() {
"import org.checkerframework.checker.nullness.qual.Nullable;",
"class A { class B { class C {} } }",
"class Test {",
" // BUG: Diagnostic contains: Type-use nullability annotations should be applied on inner class",
" public @Nullable A.B.C method1() { return null; }",
" // BUG: Diagnostic contains: Type-use nullability annotations should be applied on inner class",
" public A.@Nullable B.C method2() { return null; }",
" public A.B.@Nullable C method3() { return null; }",
" // BUG: Diagnostic contains: Type-use nullability annotations should be applied on inner class",
" public @Nullable A.B method4() { return null; }",
" public @Nullable A method5() { return null; }",
" public @Nullable int method6() { return 0; }",
" // BUG: Diagnostic contains: Type-use nullability annotations should be applied on inner class",
" public @Nullable A.B.C method1() { return null; }",
" // BUG: Diagnostic contains: Type-use nullability annotations should be applied on inner class",
" public A.@Nullable B.C method2() { return null; }",
" public A.B.@Nullable C method3() { return null; }",
" // BUG: Diagnostic contains: Type-use nullability annotations should be applied on inner class",
" public @Nullable A.B method4() { return null; }",
" public @Nullable A method5() { return null; }",
" public @Nullable int method6() { return 0; }",
"}")
.doTest();
}
Expand All @@ -329,12 +331,11 @@ public void typeUseAnnotationOnMethodReturnTypeLegacy() {
"import org.checkerframework.checker.nullness.qual.Nullable;",
"class A { class B { class C {} } }",
"class Test {",
" public @Nullable A.B.C method1() { return null; }",
" public A.@Nullable B.C method2() { return null; }",
" public A.B.@Nullable C method3() { return null; }",
" public @Nullable A.B method4() { return null; }",
" public @Nullable A method5() { return null; }",
" public @Nullable int method6() { return 0; }",
" public @Nullable A.B.C method1() { return null; }",
" public A.@Nullable B.C method2() { return null; }",
" public A.B.@Nullable C method3() { return null; }",
" public @Nullable A.B method4() { return null; }",
" public @Nullable A method5() { return null; }",
"}")
.doTest();
}
Expand Down

0 comments on commit b1e3bc1

Please sign in to comment.