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.

Sign up for GitHub

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

Jump to bottom

Enforce correct type-use annotation locations for nested types #1045

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
+178 −20
Open

Enforce correct type-use annotation locations for nested types #1045

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4b0c8b2
Added legacy and jspecify flag test
armughan11 Sep 25, 2024
c4393c0
fix tests
armughan11 Sep 25, 2024
61b0cdc
fix comment
armughan11 Sep 25, 2024
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
1 change: 1 addition & 0 deletions nullaway/src/main/java/com/uber/nullaway/ErrorMessage.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ public enum MessageTypes {
WRONG_OVERRIDE_RETURN_GENERIC,
WRONG_OVERRIDE_PARAM_GENERIC,
ASSIGN_NULLABLE_TO_NONNULL_ARRAY,
NULLABLE_ON_WRONG_NESTED_CLASS_LEVEL
}

public String getMessage() {
Expand Down
34 changes: 30 additions & 4 deletions nullaway/src/main/java/com/uber/nullaway/NullAway.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,7 @@
import com.sun.tools.javac.code.Symbol.ClassSymbol;
import com.sun.tools.javac.code.Symbol.VarSymbol;
import com.sun.tools.javac.code.Type;
import com.sun.tools.javac.code.Types;
import com.sun.tools.javac.processing.JavacProcessingEnvironment;
import com.sun.tools.javac.tree.JCTree;
import com.uber.nullaway.ErrorMessage.MessageTypes;
Expand Down Expand Up @@ -1475,10 +1476,19 @@
if (initializer != null) {
if (!symbol.type.isPrimitive() && !skipDueToFieldAnnotation(symbol)) {
if (mayBeNullExpr(state, initializer)) {
ErrorMessage errorMessage =
new ErrorMessage(
MessageTypes.ASSIGN_FIELD_NULLABLE,
"assigning @Nullable expression to @NonNull field");
ErrorMessage errorMessage;
if (hasNestedClass(state.getTypes(), symbol.type)) {
errorMessage =
new ErrorMessage(
MessageTypes.NULLABLE_ON_WRONG_NESTED_CLASS_LEVEL,
"Type-use nullability annotations should be applied on inner class");
Comment on lines +1480 to +1484
Copy link
Collaborator

Choose a reason for hiding this comment

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

I can't understand this check and why it occurs here? I think the check should occur independent of whether the initializer assignment is valid. If we need to possibly report multiple error messages from this method, we can use the state.reportMatch API to report the errors and then just always return Description.NO_MATCH.

} else {
errorMessage =
new ErrorMessage(
MessageTypes.ASSIGN_FIELD_NULLABLE,
"assigning @Nullable expression to @NonNull field");
}

return errorBuilder.createErrorDescriptionForNullAssignment(
errorMessage, initializer, buildDescription(tree), state, symbol);
}
Expand All @@ -1487,6 +1497,22 @@
return Description.NO_MATCH;
}

public static boolean hasNestedClass(Types types, Type type) {
if (types == null || type == null) {
return false;

Check warning on line 1502 in nullaway/src/main/java/com/uber/nullaway/NullAway.java

View check run for this annotation

Codecov / codecov/patch

nullaway/src/main/java/com/uber/nullaway/NullAway.java#L1502 

Added line #L1502 was not covered by tests
}
Comment on lines +1501 to +1503
Copy link
Collaborator

Choose a reason for hiding this comment

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

If these null checks are necessary the parameters should be @Nullable. But I don't think they are needed?

if (type.getKind() == TypeKind.DECLARED && type.isParameterized()) {
return false;
}

Type enclosingType = type.getEnclosingType();
if (enclosingType == null) {
return false;
}

return !types.isSameType(enclosingType, Type.noType);
}

/**
* Check if an inner class's annotation means this Compilation Unit is partially annotated.
*
Expand Down
46 changes: 40 additions & 6 deletions nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@
import com.sun.tools.javac.code.Type;
import com.sun.tools.javac.code.TypeAnnotationPosition;
import com.sun.tools.javac.code.TypeAnnotationPosition.TypePathEntry;
import com.sun.tools.javac.code.TypeTag;
import com.sun.tools.javac.code.Types;
import com.sun.tools.javac.tree.JCTree;
import com.sun.tools.javac.util.JCDiagnostic;
Expand Down Expand Up @@ -273,7 +274,7 @@ public static Stream<? extends AnnotationMirror> getAllAnnotationsForParameter(
t ->
t.position.type.equals(TargetType.METHOD_FORMAL_PARAMETER)
&& t.position.parameter_index == paramInd
&& NullabilityUtil.isDirectTypeUseAnnotation(t, config)));
&& NullabilityUtil.isDirectTypeUseAnnotation(t, symbol, config)));
}

/**
Expand All @@ -288,10 +289,11 @@ public static Stream<? extends AnnotationMirror> getTypeUseAnnotations(
return rawTypeAttributes.filter(
(t) ->
t.position.type.equals(TargetType.METHOD_RETURN)
&& isDirectTypeUseAnnotation(t, config));
&& isDirectTypeUseAnnotation(t, symbol, config));
} else {
// filter for annotations directly on the type
return rawTypeAttributes.filter(t -> NullabilityUtil.isDirectTypeUseAnnotation(t, config));
return rawTypeAttributes.filter(
t -> NullabilityUtil.isDirectTypeUseAnnotation(t, symbol, config));
}
}

Expand All @@ -307,7 +309,8 @@ public static Stream<? extends AnnotationMirror> getTypeUseAnnotations(
* @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) {
private static boolean isDirectTypeUseAnnotation(
Attribute.TypeCompound t, Symbol symbol, 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 @@ -320,6 +323,10 @@ private static boolean isDirectTypeUseAnnotation(Attribute.TypeCompound t, Confi
// 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 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 All @@ -329,10 +336,13 @@ private static boolean isDirectTypeUseAnnotation(Attribute.TypeCompound t, Confi
// See https://github.com/uber/NullAway/issues/708
boolean locationHasInnerTypes = false;
boolean locationHasArray = false;
int innerTypeCount = 0;
int nestingDepth = getNestingDepth(symbol.type);
for (TypePathEntry entry : t.position.location) {
switch (entry.tag) {
case INNER_TYPE:
locationHasInnerTypes = true;
innerTypeCount++;
break;
case ARRAY:
if (config.isJSpecifyMode() || !config.isLegacyAnnotationLocation()) {
Expand All @@ -347,8 +357,32 @@ private static boolean isDirectTypeUseAnnotation(Attribute.TypeCompound t, Confi
return false;
}
}
// Make sure it's not a mix of inner types and arrays for this annotation's location
return !(locationHasInnerTypes && locationHasArray);
if (config.isLegacyAnnotationLocation()) {
// Make sure it's not a mix of inner types and arrays for this annotation's location
return !(locationHasInnerTypes && locationHasArray);
}
if (!hasNestedClass(symbol.type)) {
if (innerTypeCount > 0) {
return true;
}
}
return innerTypeCount == nestingDepth - 1;
}

private static int getNestingDepth(Type type) {
int depth = 0;
for (Type curr = type;
curr != null && !curr.hasTag(TypeTag.NONE);
curr = curr.getEnclosingType()) {
depth++;
}
return depth;
}

private static boolean hasNestedClass(Type type) {
return type.tsym != null
&& type.tsym.owner instanceof Symbol.ClassSymbol
&& !(type.tsym.owner.owner instanceof Symbol.PackageSymbol);
}

/**
Expand Down
117 changes: 107 additions & 10 deletions nullaway/src/test/java/com/uber/nullaway/TypeUseAnnotationsTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@

package com.uber.nullaway;

import com.google.errorprone.CompilationTestHelper;
import java.util.Arrays;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.JUnit4;
Expand Down Expand Up @@ -124,12 +126,13 @@ public void annotationAppliedToInnerTypeExplicitly() {
"import org.checkerframework.checker.nullness.qual.Nullable;",
"class Test {",
" Test.@Nullable Foo f1;",
" // @Nullable does not apply to the inner type",
" // BUG: Diagnostic contains: @NonNull field f2 not initialized",
" @Nullable Test.Foo f2;",
" class Foo { }",
" public void test() {",
" // BUG: Diagnostic contains: dereferenced",
" f1.hashCode();",
" // BUG: Diagnostic contains: dereferenced",
" f2.hashCode();",
" }",
"}")
Expand All @@ -152,11 +155,12 @@ public void annotationAppliedToInnerTypeExplicitly2() {
"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();",
" // BUG: Diagnostic contains: dereferenced",
" f2.hashCode();",
" }",
"}")
Expand Down Expand Up @@ -189,21 +193,107 @@ public void typeUseAnnotationOnInnerMultiLevel() {
.addSourceLines(
"Test.java",
"package com.uber;",
"import java.util.Set;",
"import org.checkerframework.checker.nullness.qual.Nullable;",
"class A { class B { class C {} } }",
"class Test {",
" // At some point, we should not treat foo1 or foo2 as @Nullable.",
" // For now we do, for ease of compatibility.",
" // TODO: Fix this as part of https://github.com/uber/NullAway/issues/708",
" // 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;",
"}")
.doTest();
}

@Test
public void typeUseAnnotationOnInnerMultiLevelLegacy() {
makeLegacyModeHelper()
.addSourceLines(
"Test.java",
"package com.uber;",
"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;",
" // No good reason to support the case below, though!",
" // It neither matches were a correct type use annotation for marking foo4 as @Nullable would be,",
" // nor the natural position of a declaration annotation at the start of the type!",
" @Nullable A.B foo4 = null;",
" // BUG: Diagnostic contains: assigning @Nullable expression to @NonNull field",
" A.B.@Nullable C [][] foo4 = null;",
" A.B.@Nullable C [][] foo5 = null;",
"}")
.doTest();
}

@Test
public void declarationAnnotationOnInnerMultiLevel() {
defaultCompilationHelper
.addSourceLines(
"Test.java",
"package com.uber;",
"import javax.annotation.Nullable;",
"class A { class B { class C {} } }",
"class Test {",
" @Nullable A.B.C foo1 = null;",
" @Nullable A.B foo4 = null;",
"}")
.doTest();
}

@Test
public void typeUseAndDeclarationAnnotationOnInnerMultiLevel() {
defaultCompilationHelper
.addSourceLines(
"Nullable.java",
"package com.uber;",
"import java.lang.annotation.ElementType;",
"import java.lang.annotation.Target;",
"@Target({ElementType.METHOD, ElementType.FIELD, ElementType.PARAMETER, ElementType.LOCAL_VARIABLE, ElementType.TYPE_USE})",
"public @interface Nullable {}")
.addSourceLines(
"Test.java",
"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;",
"}")
.doTest();
}

@Test
public void typeUseAndDeclarationAnnotationOnInnerMultiLevelLegacy() {
makeLegacyModeHelper()
.addSourceLines(
"Nullable.java",
"package com.uber;",
"import java.lang.annotation.ElementType;",
"import java.lang.annotation.Target;",
"@Target({ElementType.METHOD, ElementType.FIELD, ElementType.PARAMETER, ElementType.LOCAL_VARIABLE, ElementType.TYPE_USE})",
"public @interface Nullable {}")
.addSourceLines(
"Test.java",
"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;",
"}")
.doTest();
}
Expand Down Expand Up @@ -240,4 +330,11 @@ public void typeParameterAnnotationIsDistinctFromMethodReturnAnnotation() {
"}")
.doTest();
}

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