From cc5ef6580305f135b9768f1ec43ffaa8a9086572 Mon Sep 17 00:00:00 2001 From: Md Armughanuddin Date: Thu, 10 Oct 2024 04:15:24 +0530 Subject: [PATCH] Enforce correct type-use annotation locations for nested types (#1045) Enforce correct type-use annotation locations for nested types as per JSpecify norms. We enforce type-use annotations to be on the inner class and raise an error if they are not. For annotations that are both type use and declaration, we raise an error at an invalid location. **Current behavior** ``` // All three are ok @Nullable A.B.C foo1 = null; A.@Nullable B.C foo2 = null; A.B.@Nullable C foo3 = null; ``` **New behavior** ``` // 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; ``` For annotations which are both declaration and annotation and type-use, only `foo2` throws an error since the location isn't apt for either scenarios --------- Co-authored-by: Manu Sridharan --- .../java/com/uber/nullaway/ErrorMessage.java | 1 + .../main/java/com/uber/nullaway/NullAway.java | 101 ++++++++++ .../com/uber/nullaway/NullabilityUtil.java | 44 ++++- .../nullaway/TypeUseAnnotationsTests.java | 178 ++++++++++++++++-- 4 files changed, 298 insertions(+), 26 deletions(-) diff --git a/nullaway/src/main/java/com/uber/nullaway/ErrorMessage.java b/nullaway/src/main/java/com/uber/nullaway/ErrorMessage.java index aa3c364306..e5040a471f 100644 --- a/nullaway/src/main/java/com/uber/nullaway/ErrorMessage.java +++ b/nullaway/src/main/java/com/uber/nullaway/ErrorMessage.java @@ -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() { diff --git a/nullaway/src/main/java/com/uber/nullaway/NullAway.java b/nullaway/src/main/java/com/uber/nullaway/NullAway.java index 4d0f365d9a..e16d91c43e 100644 --- a/nullaway/src/main/java/com/uber/nullaway/NullAway.java +++ b/nullaway/src/main/java/com/uber/nullaway/NullAway.java @@ -33,6 +33,9 @@ import static com.uber.nullaway.ErrorBuilder.errMsgForInitializer; import static com.uber.nullaway.NullabilityUtil.castToNonNull; import static com.uber.nullaway.NullabilityUtil.isArrayElementNullable; +import static com.uber.nullaway.Nullness.isNullableAnnotation; +import static java.lang.annotation.ElementType.TYPE_PARAMETER; +import static java.lang.annotation.ElementType.TYPE_USE; import com.google.auto.service.AutoService; import com.google.auto.value.AutoValue; @@ -53,6 +56,8 @@ import com.google.errorprone.matchers.Matchers; import com.google.errorprone.suppliers.Suppliers; import com.google.errorprone.util.ASTHelpers; +import com.sun.source.tree.AnnotatedTypeTree; +import com.sun.source.tree.AnnotationTree; import com.sun.source.tree.ArrayAccessTree; import com.sun.source.tree.AssignmentTree; import com.sun.source.tree.BinaryTree; @@ -98,6 +103,8 @@ import com.uber.nullaway.handlers.Handler; import com.uber.nullaway.handlers.Handlers; import com.uber.nullaway.handlers.MethodAnalysisContext; +import java.lang.annotation.ElementType; +import java.lang.annotation.Target; import java.util.ArrayList; import java.util.LinkedHashMap; import java.util.LinkedHashSet; @@ -187,6 +194,8 @@ public class NullAway extends BugChecker static final String CORE_CHECK_NAME = "NullAway."; private static final Matcher THIS_MATCHER = NullAway::isThisIdentifierMatcher; + private static final ImmutableSet TYPE_USE_OR_TYPE_PARAMETER = + ImmutableSet.of(TYPE_USE, TYPE_PARAMETER); private final Predicate nonAnnotatedMethod; @@ -570,6 +579,11 @@ public Description matchMemberSelect(MemberSelectTree tree, VisitorState state) || symbol instanceof ModuleElement) { return Description.NO_MATCH; } + if ((tree.getExpression() instanceof AnnotatedTypeTree) + && !config.isLegacyAnnotationLocation()) { + checkNullableAnnotationPositionInType( + ((AnnotatedTypeTree) tree.getExpression()).getAnnotations(), tree, state); + } Description badDeref = matchDereference(tree.getExpression(), tree, state); if (!badDeref.equals(Description.NO_MATCH)) { @@ -638,6 +652,10 @@ public Description matchMethod(MethodTree tree, VisitorState state) { if (!withinAnnotatedCode(state)) { return Description.NO_MATCH; } + if (!config.isLegacyAnnotationLocation()) { + checkNullableAnnotationPositionInType( + tree.getModifiers().getAnnotations(), tree.getReturnType(), state); + } // if the method is overriding some other method, // check that nullability annotations are consistent with // overridden method (if overridden method is in an annotated @@ -1464,6 +1482,10 @@ public Description matchVariable(VariableTree tree, VisitorState state) { if (tree.getInitializer() != null && config.isJSpecifyMode()) { GenericsChecks.checkTypeParameterNullnessForAssignability(tree, this, state); } + if (!config.isLegacyAnnotationLocation()) { + checkNullableAnnotationPositionInType( + tree.getModifiers().getAnnotations(), tree.getType(), state); + } if (symbol.type.isPrimitive() && tree.getInitializer() != null) { doUnboxingCheck(state, tree.getInitializer()); @@ -1487,6 +1509,85 @@ public Description matchVariable(VariableTree tree, VisitorState state) { return Description.NO_MATCH; } + /** + * returns true if {@code anno} is a type use annotation; it may also be a declaration annotation + */ + private static boolean isTypeUseAnnotation(Symbol anno) { + Target target = anno.getAnnotation(Target.class); + ImmutableSet elementTypes = + target == null ? ImmutableSet.of() : ImmutableSet.copyOf(target.value()); + return elementTypes.contains(TYPE_USE); + } + + /** + * returns true if {@code anno} is a declaration annotation; it may also be a type use annotation + */ + private static boolean isDeclarationAnnotation(Symbol anno) { + Target target = anno.getAnnotation(Target.class); + if (target == null) { + return true; + } + ImmutableSet elementTypes = ImmutableSet.copyOf(target.value()); + // 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)); + } + + /** + * Checks whether any {@code @Nullable} annotation is at the right location for nested types. + * Raises an error iff the type is a field access expression (for an inner class type), the + * annotation is 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 state The visitor state + */ + private void checkNullableAnnotationPositionInType( + List annotations, Tree type, VisitorState state) { + + // Early return if the type is not a nested or inner class reference. + if (!(type instanceof MemberSelectTree)) { + return; + } + + // 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(((MemberSelectTree) type).getExpression()); + int startOfType = ((JCTree) type).getStartPosition(); + + for (AnnotationTree annotation : annotations) { + Symbol sym = ASTHelpers.getSymbol(annotation); + if (sym == null) { + continue; + } + + String qualifiedName = sym.getQualifiedName().toString(); + if (!isNullableAnnotation(qualifiedName, config)) { + continue; + } + + if (!isTypeUseAnnotation(sym)) { + continue; + } + // If an annotation is declaration ALSO, we check if it is at the correct location. If it is, + // we treat it as declaration and skip the checks. + if (isDeclarationAnnotation(sym) && state.getEndPosition(annotation) <= startOfType) { + continue; + } + + if (state.getEndPosition(annotation) < endOfOuterType) { + // annotation is not on the inner-most type + ErrorMessage errorMessage = + new ErrorMessage( + MessageTypes.NULLABLE_ON_WRONG_NESTED_CLASS_LEVEL, + "Type-use nullability annotations should be applied on inner class"); + + state.reportMatch( + errorBuilder.createErrorDescription(errorMessage, buildDescription(type), state, null)); + } + } + } + /** * Check if an inner class's annotation means this Compilation Unit is partially annotated. * diff --git a/nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java b/nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java index 6e306851c5..514e569d13 100644 --- a/nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java +++ b/nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java @@ -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; @@ -293,7 +294,7 @@ public static Stream getAllAnnotationsForParameter( t -> t.position.type.equals(TargetType.METHOD_FORMAL_PARAMETER) && t.position.parameter_index == paramInd - && NullabilityUtil.isDirectTypeUseAnnotation(t, config))); + && NullabilityUtil.isDirectTypeUseAnnotation(t, symbol, config))); } /** @@ -308,10 +309,11 @@ public static Stream 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)); } } @@ -323,11 +325,13 @@ public static Stream getTypeUseAnnotations( * but {@code List<@Nullable T> lst} is not. * * @param t the annotation and its position in the type + * @param symbol the symbol for the annotated element * @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) { + 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. @@ -340,6 +344,9 @@ 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. + // 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 @@ -349,10 +356,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()) { @@ -367,8 +377,30 @@ 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); + } + // For non-nested classes annotations apply to the innermost type. + if (!isTypeOfNestedClass(symbol.type)) { + return true; + } + // For nested classes the annotation is only valid if it is on the innermost type. + 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 isTypeOfNestedClass(Type type) { + return type.tsym != null && type.tsym.owner instanceof Symbol.ClassSymbol; } /** diff --git a/nullaway/src/test/java/com/uber/nullaway/TypeUseAnnotationsTests.java b/nullaway/src/test/java/com/uber/nullaway/TypeUseAnnotationsTests.java index 3a997403f5..745f8c8652 100644 --- a/nullaway/src/test/java/com/uber/nullaway/TypeUseAnnotationsTests.java +++ b/nullaway/src/test/java/com/uber/nullaway/TypeUseAnnotationsTests.java @@ -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; @@ -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();", " }", "}") @@ -151,13 +154,14 @@ public void annotationAppliedToInnerTypeExplicitly2() { "package com.uber;", "import org.checkerframework.checker.nullness.qual.Nullable;", "class Test {", - " Bar.@Nullable Foo f1;", - " @Nullable Bar.Foo f2;", - " public void test() {", - " // BUG: Diagnostic contains: dereferenced", - " f1.hashCode();", - " // BUG: Diagnostic contains: dereferenced", - " 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(); @@ -189,21 +193,148 @@ 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", - " @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!", - " // BUG: Diagnostic contains: assigning @Nullable expression to @NonNull field", - " A.B.@Nullable C [][] foo4 = null;", + " // 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;", + " A.B.C @Nullable [][] foo6 = 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;", + " @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(); + } + + @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(); + } + + @Test + public void typeUseAnnotationOnMethodReturnType() { + defaultCompilationHelper + .addSourceLines( + "Test.java", + "package com.uber;", + "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; }", + "}") + .doTest(); + } + + @Test + public void typeUseAnnotationOnMethodReturnTypeLegacy() { + makeLegacyModeHelper() + .addSourceLines( + "Test.java", + "package com.uber;", + "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; }", "}") .doTest(); } @@ -240,4 +371,11 @@ public void typeParameterAnnotationIsDistinctFromMethodReturnAnnotation() { "}") .doTest(); } + + private CompilationTestHelper makeLegacyModeHelper() { + return makeTestHelperWithArgs( + Arrays.asList( + "-XepOpt:NullAway:AnnotatedPackages=com.uber", + "-XepOpt:NullAway:LegacyAnnotationLocations=true")); + } }