From 213bc4449750f82fdbefb9bec93cf7e9ffa3eaf6 Mon Sep 17 00:00:00 2001 From: Md Armughanuddin Date: Mon, 7 Oct 2024 16:14:51 +0530 Subject: [PATCH 01/14] fix comment --- .../java/com/uber/nullaway/ErrorMessage.java | 1 + .../main/java/com/uber/nullaway/NullAway.java | 87 ++++++++++ .../com/uber/nullaway/NullabilityUtil.java | 46 ++++- .../nullaway/TypeUseAnnotationsTests.java | 158 ++++++++++++++++-- 4 files changed, 276 insertions(+), 16 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 546dae75c2..bc83d5a377 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()) { + handleNullabilityOnNestedClass( + ((AnnotatedTypeTree) tree.getExpression()).getAnnotations(), tree, 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()) { + handleNullabilityOnNestedClass( + tree.getModifiers().getAnnotations(), tree.getReturnType(), tree, 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()) { + handleNullabilityOnNestedClass( + tree.getModifiers().getAnnotations(), tree.getType(), tree, state); + } if (symbol.type.isPrimitive() && tree.getInitializer() != null) { doUnboxingCheck(state, tree.getInitializer()); @@ -1487,6 +1509,71 @@ public Description matchVariable(VariableTree tree, VisitorState state) { return Description.NO_MATCH; } + private static boolean isOnlyTypeAnnotation(Symbol anno) { + Target target = anno.getAnnotation(Target.class); + ImmutableSet elementTypes = + target == null ? ImmutableSet.of() : ImmutableSet.copyOf(target.value()); + return elementTypes.contains(TYPE_USE); + } + + 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 only if annotation is not type-use only + return !(elementTypes.equals(ImmutableSet.of(ElementType.TYPE_USE)) + || TYPE_USE_OR_TYPE_PARAMETER.containsAll(elementTypes)); + } + + private void handleNullabilityOnNestedClass( + List annotations, Tree type, Tree tree, VisitorState state) { + if (!(type instanceof JCTree.JCFieldAccess)) { + return; + } + JCTree.JCFieldAccess fieldAccess = (JCTree.JCFieldAccess) type; + + int endOfOuterType = state.getEndPosition(fieldAccess.getExpression()); + int startOfType = ((JCTree) type).getStartPosition(); + + for (AnnotationTree annotation : annotations) { + Symbol sym = ASTHelpers.getSymbol(annotation); + if (sym == null) { + continue; + } + if (!isOnlyTypeAnnotation(sym)) { + continue; + } + + if (isDeclarationAnnotation(sym) && state.getEndPosition(annotation) <= startOfType) { + continue; + } + + Symbol annotationSymbol = ASTHelpers.getSymbol(annotation); + if (annotationSymbol == null) { + continue; + } + + String qualifiedName = annotationSymbol.getQualifiedName().toString(); + if (!isNullableAnnotation(qualifiedName, config)) { + continue; + } + + if (state.getEndPosition(annotation) < endOfOuterType) { + + 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(tree), 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 6bae2114a0..81e0d846bc 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; @@ -273,7 +274,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))); } /** @@ -288,10 +289,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)); } } @@ -307,7 +309,8 @@ public static Stream 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. @@ -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 @@ -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()) { @@ -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); } /** diff --git a/nullaway/src/test/java/com/uber/nullaway/TypeUseAnnotationsTests.java b/nullaway/src/test/java/com/uber/nullaway/TypeUseAnnotationsTests.java index 3a997403f5..22a9eea7c7 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();", " }", "}") @@ -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();", " }", "}") @@ -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", + " // 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(); + } + + @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; }", + " public @Nullable int method6() { return 0; }", + "}") + .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; }", + " public @Nullable int method6() { return 0; }", "}") .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")); + } } From 41a037127fa32433870302844d03a8a33b01d1db Mon Sep 17 00:00:00 2001 From: Md Armughanuddin Date: Mon, 7 Oct 2024 16:25:56 +0530 Subject: [PATCH 02/14] add declaration and type use check comment --- nullaway/src/main/java/com/uber/nullaway/NullAway.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/nullaway/src/main/java/com/uber/nullaway/NullAway.java b/nullaway/src/main/java/com/uber/nullaway/NullAway.java index b6eb58651b..37a11036a9 100644 --- a/nullaway/src/main/java/com/uber/nullaway/NullAway.java +++ b/nullaway/src/main/java/com/uber/nullaway/NullAway.java @@ -1546,7 +1546,8 @@ private void handleNullabilityOnNestedClass( if (!isOnlyTypeAnnotation(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; } From a08ae750c6c4be1e365d0fb3b2d84933dc9360de Mon Sep 17 00:00:00 2001 From: Md Armughanuddin Date: Mon, 7 Oct 2024 17:49:37 +0530 Subject: [PATCH 03/14] fix nullability of new methods --- nullaway/src/main/java/com/uber/nullaway/NullAway.java | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/nullaway/src/main/java/com/uber/nullaway/NullAway.java b/nullaway/src/main/java/com/uber/nullaway/NullAway.java index 37a11036a9..edc56a1fd2 100644 --- a/nullaway/src/main/java/com/uber/nullaway/NullAway.java +++ b/nullaway/src/main/java/com/uber/nullaway/NullAway.java @@ -1521,7 +1521,6 @@ private static boolean isDeclarationAnnotation(Symbol anno) { if (target == null) { return true; } - ; ImmutableSet elementTypes = ImmutableSet.copyOf(target.value()); // Return true only if annotation is not type-use only return !(elementTypes.equals(ImmutableSet.of(ElementType.TYPE_USE)) @@ -1552,12 +1551,7 @@ private void handleNullabilityOnNestedClass( continue; } - Symbol annotationSymbol = ASTHelpers.getSymbol(annotation); - if (annotationSymbol == null) { - continue; - } - - String qualifiedName = annotationSymbol.getQualifiedName().toString(); + String qualifiedName = sym.getQualifiedName().toString(); if (!isNullableAnnotation(qualifiedName, config)) { continue; } From d1d534d9ef2d35eb194a0caa9ce54ecf18e3404a Mon Sep 17 00:00:00 2001 From: Manu Sridharan Date: Mon, 7 Oct 2024 13:34:08 -0700 Subject: [PATCH 04/14] rename variables --- .../src/main/java/com/uber/nullaway/NullAway.java | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/nullaway/src/main/java/com/uber/nullaway/NullAway.java b/nullaway/src/main/java/com/uber/nullaway/NullAway.java index edc56a1fd2..cca6afbfc7 100644 --- a/nullaway/src/main/java/com/uber/nullaway/NullAway.java +++ b/nullaway/src/main/java/com/uber/nullaway/NullAway.java @@ -1528,14 +1528,17 @@ private static boolean isDeclarationAnnotation(Symbol anno) { } private void handleNullabilityOnNestedClass( - List annotations, Tree type, Tree tree, VisitorState state) { - if (!(type instanceof JCTree.JCFieldAccess)) { + List annotations, + Tree typeTree, + Tree errorReportingTree, + VisitorState state) { + if (!(typeTree instanceof JCTree.JCFieldAccess)) { return; } - JCTree.JCFieldAccess fieldAccess = (JCTree.JCFieldAccess) type; + JCTree.JCFieldAccess fieldAccess = (JCTree.JCFieldAccess) typeTree; int endOfOuterType = state.getEndPosition(fieldAccess.getExpression()); - int startOfType = ((JCTree) type).getStartPosition(); + int startOfType = ((JCTree) typeTree).getStartPosition(); for (AnnotationTree annotation : annotations) { Symbol sym = ASTHelpers.getSymbol(annotation); @@ -1564,7 +1567,8 @@ private void handleNullabilityOnNestedClass( "Type-use nullability annotations should be applied on inner class"); state.reportMatch( - errorBuilder.createErrorDescription(errorMessage, buildDescription(tree), state, null)); + errorBuilder.createErrorDescription( + errorMessage, buildDescription(errorReportingTree), state, null)); } } } From d1203afcd92fcf87180dea9c47af158bc355b99f Mon Sep 17 00:00:00 2001 From: Manu Sridharan Date: Mon, 7 Oct 2024 13:47:12 -0700 Subject: [PATCH 05/14] reflow comment --- .../src/main/java/com/uber/nullaway/NullabilityUtil.java | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java b/nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java index fb605a3254..9199e2011f 100644 --- a/nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java +++ b/nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java @@ -344,9 +344,8 @@ private static boolean isDirectTypeUseAnnotation( // 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. + // 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 From 49bb92a8a74ec5d8d97bbc1ef3dbf0ee66e43977 Mon Sep 17 00:00:00 2001 From: Manu Sridharan Date: Mon, 7 Oct 2024 13:48:28 -0700 Subject: [PATCH 06/14] tweak comment --- nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java b/nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java index 9199e2011f..4eb3c99eae 100644 --- a/nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java +++ b/nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java @@ -344,8 +344,8 @@ private static boolean isDirectTypeUseAnnotation( // 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. + // 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. // 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 From b1e3bc10db65f19d842e8f6bafd2c405d7e2464d Mon Sep 17 00:00:00 2001 From: Md Armughanuddin Date: Wed, 9 Oct 2024 14:55:21 +0530 Subject: [PATCH 07/14] fix review comments --- .../main/java/com/uber/nullaway/NullAway.java | 43 ++++--- .../com/uber/nullaway/NullabilityUtil.java | 10 +- .../nullaway/TypeUseAnnotationsTests.java | 111 +++++++++--------- 3 files changed, 90 insertions(+), 74 deletions(-) diff --git a/nullaway/src/main/java/com/uber/nullaway/NullAway.java b/nullaway/src/main/java/com/uber/nullaway/NullAway.java index cca6afbfc7..67008913c8 100644 --- a/nullaway/src/main/java/com/uber/nullaway/NullAway.java +++ b/nullaway/src/main/java/com/uber/nullaway/NullAway.java @@ -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); } @@ -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, @@ -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); } @@ -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 elementTypes = target == null ? ImmutableSet.of() : ImmutableSet.copyOf(target.value()); @@ -1522,30 +1522,42 @@ private static boolean isDeclarationAnnotation(Symbol anno) { return true; } ImmutableSet 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 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 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, @@ -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)); } } } diff --git a/nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java b/nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java index 4eb3c99eae..0d97a885cd 100644 --- a/nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java +++ b/nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java @@ -325,6 +325,7 @@ 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 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 @@ -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 @@ -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; } diff --git a/nullaway/src/test/java/com/uber/nullaway/TypeUseAnnotationsTests.java b/nullaway/src/test/java/com/uber/nullaway/TypeUseAnnotationsTests.java index 22a9eea7c7..2bee712cf2 100644 --- a/nullaway/src/test/java/com/uber/nullaway/TypeUseAnnotationsTests.java +++ b/nullaway/src/test/java/com/uber/nullaway/TypeUseAnnotationsTests.java @@ -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(); @@ -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(); } @@ -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(); } @@ -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(); @@ -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(); } @@ -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(); } @@ -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(); } @@ -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(); } From 3ed138f5adf6f0c013bd45184b5f2165e78525d1 Mon Sep 17 00:00:00 2001 From: Manu Sridharan Date: Wed, 9 Oct 2024 12:48:17 -0700 Subject: [PATCH 08/14] docs --- nullaway/src/main/java/com/uber/nullaway/NullAway.java | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/nullaway/src/main/java/com/uber/nullaway/NullAway.java b/nullaway/src/main/java/com/uber/nullaway/NullAway.java index 67008913c8..472c23e1e2 100644 --- a/nullaway/src/main/java/com/uber/nullaway/NullAway.java +++ b/nullaway/src/main/java/com/uber/nullaway/NullAway.java @@ -1509,6 +1509,9 @@ 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 = @@ -1516,6 +1519,9 @@ private static boolean isTypeUseAnnotation(Symbol anno) { 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) { From d70531751215e591d607cbf0f2ff1675704b2e84 Mon Sep 17 00:00:00 2001 From: Manu Sridharan Date: Wed, 9 Oct 2024 12:53:47 -0700 Subject: [PATCH 09/14] tweaks --- .../main/java/com/uber/nullaway/NullAway.java | 20 +++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/nullaway/src/main/java/com/uber/nullaway/NullAway.java b/nullaway/src/main/java/com/uber/nullaway/NullAway.java index 472c23e1e2..f5a07a4def 100644 --- a/nullaway/src/main/java/com/uber/nullaway/NullAway.java +++ b/nullaway/src/main/java/com/uber/nullaway/NullAway.java @@ -1534,9 +1534,9 @@ private static boolean isDeclarationAnnotation(Symbol anno) { } /** - * 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. + * 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 @@ -1547,7 +1547,6 @@ private void checkNullableAnnotationPositionInType( List 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; } @@ -1563,6 +1562,12 @@ private void checkNullableAnnotationPositionInType( if (sym == null) { continue; } + + String qualifiedName = sym.getQualifiedName().toString(); + if (!isNullableAnnotation(qualifiedName, config)) { + continue; + } + if (!isTypeUseAnnotation(sym)) { continue; } @@ -1572,13 +1577,8 @@ private void checkNullableAnnotationPositionInType( continue; } - String qualifiedName = sym.getQualifiedName().toString(); - if (!isNullableAnnotation(qualifiedName, config)) { - 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, From 18dd6b9d14540d29595e1da0c93fded52eeede73 Mon Sep 17 00:00:00 2001 From: Manu Sridharan Date: Wed, 9 Oct 2024 13:00:22 -0700 Subject: [PATCH 10/14] tweak comment --- nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java b/nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java index 0d97a885cd..3331c7d201 100644 --- a/nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java +++ b/nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java @@ -325,7 +325,7 @@ 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 method symbol + * @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 From 5ded42772fdc4f39ae851d556ec45c015a3033be Mon Sep 17 00:00:00 2001 From: Manu Sridharan Date: Wed, 9 Oct 2024 13:52:03 -0700 Subject: [PATCH 11/14] remove unnecessary local --- nullaway/src/main/java/com/uber/nullaway/NullAway.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/nullaway/src/main/java/com/uber/nullaway/NullAway.java b/nullaway/src/main/java/com/uber/nullaway/NullAway.java index f5a07a4def..5b40b837c5 100644 --- a/nullaway/src/main/java/com/uber/nullaway/NullAway.java +++ b/nullaway/src/main/java/com/uber/nullaway/NullAway.java @@ -1550,11 +1550,10 @@ private void checkNullableAnnotationPositionInType( if (!(type instanceof MemberSelectTree)) { return; } - 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 endOfOuterType = state.getEndPosition(((MemberSelectTree) type).getExpression()); int startOfType = ((JCTree) type).getStartPosition(); for (AnnotationTree annotation : annotations) { From 9aadf2bc47207310666acd61e9551ec27b4c6c2c Mon Sep 17 00:00:00 2001 From: Md Armughanuddin Date: Thu, 10 Oct 2024 02:46:28 +0530 Subject: [PATCH 12/14] fix review comments --- .../src/main/java/com/uber/nullaway/NullAway.java | 11 +++++------ .../main/java/com/uber/nullaway/NullabilityUtil.java | 11 +++-------- 2 files changed, 8 insertions(+), 14 deletions(-) diff --git a/nullaway/src/main/java/com/uber/nullaway/NullAway.java b/nullaway/src/main/java/com/uber/nullaway/NullAway.java index f5a07a4def..43ba40b9f0 100644 --- a/nullaway/src/main/java/com/uber/nullaway/NullAway.java +++ b/nullaway/src/main/java/com/uber/nullaway/NullAway.java @@ -582,7 +582,7 @@ public Description matchMemberSelect(MemberSelectTree tree, VisitorState state) if ((tree.getExpression() instanceof AnnotatedTypeTree) && !config.isLegacyAnnotationLocation()) { checkNullableAnnotationPositionInType( - ((AnnotatedTypeTree) tree.getExpression()).getAnnotations(), tree, tree, state); + ((AnnotatedTypeTree) tree.getExpression()).getAnnotations(), tree, state); } Description badDeref = matchDereference(tree.getExpression(), tree, state); @@ -654,7 +654,7 @@ public Description matchMethod(MethodTree tree, VisitorState state) { } if (!config.isLegacyAnnotationLocation()) { checkNullableAnnotationPositionInType( - tree.getModifiers().getAnnotations(), tree.getReturnType(), tree, state); + tree.getModifiers().getAnnotations(), tree.getReturnType(), state); } // if the method is overriding some other method, // check that nullability annotations are consistent with @@ -1484,7 +1484,7 @@ public Description matchVariable(VariableTree tree, VisitorState state) { } if (!config.isLegacyAnnotationLocation()) { checkNullableAnnotationPositionInType( - tree.getModifiers().getAnnotations(), tree.getType(), tree, state); + tree.getModifiers().getAnnotations(), tree.getType(), state); } if (symbol.type.isPrimitive() && tree.getInitializer() != null) { @@ -1540,11 +1540,10 @@ private static boolean isDeclarationAnnotation(Symbol anno) { * * @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 annotations, Tree type, Tree tree, VisitorState state) { + List annotations, Tree type, VisitorState state) { // Early return if the type is not a nested or inner class reference. if (!(type instanceof MemberSelectTree)) { @@ -1585,7 +1584,7 @@ private void checkNullableAnnotationPositionInType( "Type-use nullability annotations should be applied on inner class"); state.reportMatch( - errorBuilder.createErrorDescription(errorMessage, buildDescription(tree), state, null)); + errorBuilder.createErrorDescription(errorMessage, buildDescription(type), state, null)); } } } diff --git a/nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java b/nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java index 3331c7d201..0d362787ef 100644 --- a/nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java +++ b/nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java @@ -381,12 +381,9 @@ 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. + // For non-nested classes annotations apply to the innermost type. if (!hasNestedClass(symbol.type)) { - if (innerTypeCount > 0) { - return true; - } + return true; } // For nested classes the annotation is only valid if it is on the innermost type. return innerTypeCount == nestingDepth - 1; @@ -403,9 +400,7 @@ private static int getNestingDepth(Type type) { } private static boolean hasNestedClass(Type type) { - return type.tsym != null - && type.tsym.owner instanceof Symbol.ClassSymbol - && !(type.tsym.owner.owner instanceof Symbol.PackageSymbol); + return type.tsym != null && type.tsym.owner instanceof Symbol.ClassSymbol; } /** From 82f381389c5cc07b7fdf9664012f403f433a2bf4 Mon Sep 17 00:00:00 2001 From: Manu Sridharan Date: Wed, 9 Oct 2024 15:32:05 -0700 Subject: [PATCH 13/14] rename method --- nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java b/nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java index 0d362787ef..514e569d13 100644 --- a/nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java +++ b/nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java @@ -382,7 +382,7 @@ private static boolean isDirectTypeUseAnnotation( return !(locationHasInnerTypes && locationHasArray); } // For non-nested classes annotations apply to the innermost type. - if (!hasNestedClass(symbol.type)) { + if (!isTypeOfNestedClass(symbol.type)) { return true; } // For nested classes the annotation is only valid if it is on the innermost type. @@ -399,7 +399,7 @@ private static int getNestingDepth(Type type) { return depth; } - private static boolean hasNestedClass(Type type) { + private static boolean isTypeOfNestedClass(Type type) { return type.tsym != null && type.tsym.owner instanceof Symbol.ClassSymbol; } From cddd77b446f4613a5590f9d15eef8a0e8fec098c Mon Sep 17 00:00:00 2001 From: Manu Sridharan Date: Wed, 9 Oct 2024 15:35:15 -0700 Subject: [PATCH 14/14] minor tweaks to tests --- .../java/com/uber/nullaway/TypeUseAnnotationsTests.java | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/nullaway/src/test/java/com/uber/nullaway/TypeUseAnnotationsTests.java b/nullaway/src/test/java/com/uber/nullaway/TypeUseAnnotationsTests.java index 2bee712cf2..745f8c8652 100644 --- a/nullaway/src/test/java/com/uber/nullaway/TypeUseAnnotationsTests.java +++ b/nullaway/src/test/java/com/uber/nullaway/TypeUseAnnotationsTests.java @@ -126,8 +126,8 @@ 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 does not apply to the inner type", + " // BUG: Diagnostic contains: @NonNull field f2 not initialized", " @Nullable Test.Foo f2;", " class Foo { }", " public void test() {", @@ -202,7 +202,7 @@ public void typeUseAnnotationOnInnerMultiLevel() { " 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;", + " @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;", @@ -317,7 +317,6 @@ public void typeUseAnnotationOnMethodReturnType() { " // 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(); }