diff --git a/nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java b/nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java index ee1e6e8a82..c5e8716e17 100644 --- a/nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java +++ b/nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java @@ -22,6 +22,9 @@ package com.uber.nullaway; +import static com.sun.tools.javac.code.TypeAnnotationPosition.TypePathEntryKind.ARRAY; +import static com.sun.tools.javac.code.TypeAnnotationPosition.TypePathEntryKind.INNER_TYPE; + import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableSet; import com.google.errorprone.VisitorState; @@ -41,6 +44,7 @@ import com.sun.tools.javac.code.Symbol; import com.sun.tools.javac.code.TargetType; import com.sun.tools.javac.code.Type; +import com.sun.tools.javac.code.TypeAnnotationPosition.TypePathEntry; import com.sun.tools.javac.code.Types; import com.sun.tools.javac.tree.JCTree; import com.sun.tools.javac.util.JCDiagnostic; @@ -292,7 +296,35 @@ private static boolean isDirectTypeUseAnnotation(Attribute.TypeCompound t) { // 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. - return t.position.location.isEmpty(); + // We care about both annotations directly on the outer type and also those directly + // on an inner type or array dimension, but wish to discard annotations on wildcards, + // or type arguments. + // For arrays, we treat annotations on the outer type and on any dimension of the array + // as applying to the nullability of the array itself, not the elements. + // 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 + // compromises to keep some semblance of backwards compatibility until we can do a + // proper deprecation of the incorrect behaviors for type use annotations when their + // semantics don't match those of a declaration annotation in the same position. + // See https://github.com/uber/NullAway/issues/708 + boolean locationHasInnerTypes = false; + boolean locationHasArray = false; + for (TypePathEntry entry : t.position.location) { + switch (entry.tag) { + case INNER_TYPE: + locationHasInnerTypes = true; + break; + case ARRAY: + locationHasArray = true; + break; + default: + // Wildcard or type argument! + return false; + } + } + // Make sure it's not a mix of inner types and arrays for this annotation's location + return !(locationHasInnerTypes && locationHasArray); } /** diff --git a/nullaway/src/test/java/com/uber/nullaway/NullAwayCoreTests.java b/nullaway/src/test/java/com/uber/nullaway/NullAwayCoreTests.java index 0f4afe44a0..f8f3ed622c 100644 --- a/nullaway/src/test/java/com/uber/nullaway/NullAwayCoreTests.java +++ b/nullaway/src/test/java/com/uber/nullaway/NullAwayCoreTests.java @@ -960,34 +960,4 @@ public void primitiveCastsRememberNullChecks() { "}") .doTest(); } - - @Test - public void annotationAppliedToTypeParameter() { - defaultCompilationHelper - .addSourceLines( - "Test.java", - "package com.uber;", - "import java.util.List;", - "import java.util.ArrayList;", - "import org.checkerframework.checker.nullness.qual.Nullable;", - "class TypeArgumentAnnotation {", - " List<@Nullable String> fSafe = new ArrayList<>();", - " @Nullable List fUnsafe = new ArrayList<>();", - " void useParamSafe(List<@Nullable String> list) {", - " list.hashCode();", - " }", - " void useParamUnsafe(@Nullable List list) {", - " // BUG: Diagnostic contains: dereferenced", - " list.hashCode();", - " }", - " void useFieldSafe() {", - " fSafe.hashCode();", - " }", - " void useFieldUnsafe() {", - " // BUG: Diagnostic contains: dereferenced", - " fUnsafe.hashCode();", - " }", - "}") - .doTest(); - } } diff --git a/nullaway/src/test/java/com/uber/nullaway/NullAwayTypeUseAnnotationsTests.java b/nullaway/src/test/java/com/uber/nullaway/NullAwayTypeUseAnnotationsTests.java new file mode 100644 index 0000000000..7af8b253d4 --- /dev/null +++ b/nullaway/src/test/java/com/uber/nullaway/NullAwayTypeUseAnnotationsTests.java @@ -0,0 +1,221 @@ +/* + * Copyright (c) 2023 Uber Technologies, Inc. + * + * Permission is hereby granted, free of charge, to any person obtaining a copy + * of this software and associated documentation files (the "Software"), to deal + * in the Software without restriction, including without limitation the rights + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell + * copies of the Software, and to permit persons to whom the Software is + * furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in + * all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN + * THE SOFTWARE. + */ + +package com.uber.nullaway; + +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +@RunWith(JUnit4.class) +public class NullAwayTypeUseAnnotationsTests extends NullAwayTestsBase { + + @Test + public void annotationAppliedToTypeParameter() { + defaultCompilationHelper + .addSourceLines( + "Test.java", + "package com.uber;", + "import java.util.List;", + "import java.util.ArrayList;", + "import org.checkerframework.checker.nullness.qual.Nullable;", + "class TypeArgumentAnnotation {", + " List<@Nullable String> fSafe = new ArrayList<>();", + " @Nullable List fUnsafe = new ArrayList<>();", + " void useParamSafe(List<@Nullable String> list) {", + " list.hashCode();", + " }", + " void useParamUnsafe(@Nullable List list) {", + " // BUG: Diagnostic contains: dereferenced", + " list.hashCode();", + " }", + " void useFieldSafe() {", + " fSafe.hashCode();", + " }", + " void useFieldUnsafe() {", + " // BUG: Diagnostic contains: dereferenced", + " fUnsafe.hashCode();", + " }", + "}") + .doTest(); + } + + @Test + public void annotationAppliedToInnerTypeImplicitly() { + defaultCompilationHelper + .addSourceLines( + "Test.java", + "package com.uber;", + "import org.checkerframework.checker.nullness.qual.Nullable;", + "class Test {", + " @Nullable Foo f;", // i.e. Test.@Nullable Foo + " class Foo { }", + " public void test() {", + " // BUG: Diagnostic contains: dereferenced", + " f.hashCode();", + " }", + "}") + .doTest(); + } + + @Test + public void annotationAppliedToInnerTypeImplicitlyWithTypeArgs() { + defaultCompilationHelper + .addSourceLines( + "Test.java", + "package com.uber;", + "import org.checkerframework.checker.nullness.qual.Nullable;", + "class Test {", + " @Nullable Foo f1 = null;", // i.e. Test.@Nullable Foo (location [INNER]) + " // BUG: Diagnostic contains: assigning @Nullable expression to @NonNull field", + " Foo<@Nullable String> f2 = null;", // (location [INNER, TYPE_ARG(0)]) + " @Nullable Foo<@Nullable String> f3 = null;", // two annotations, each with the + // locations above + " class Foo { }", + " public void test() {", + " // BUG: Diagnostic contains: dereferenced", + " f1.hashCode();", + " // safe, because nonnull", + " f2.hashCode();", + " // BUG: Diagnostic contains: dereferenced", + " f3.hashCode();", + " }", + "}") + .doTest(); + } + + @Test + public void annotationAppliedToInnerTypeExplicitly() { + defaultCompilationHelper + .addSourceLines( + "Test.java", + "package com.uber;", + "import org.checkerframework.checker.nullness.qual.Nullable;", + "class Test {", + " Test.@Nullable Foo f1;", + " @Nullable Test.Foo f2;", + " class Foo { }", + " public void test() {", + " // BUG: Diagnostic contains: dereferenced", + " f1.hashCode();", + " // BUG: Diagnostic contains: dereferenced", + " f2.hashCode();", + " }", + "}") + .doTest(); + } + + @Test + public void annotationAppliedToInnerTypeExplicitly2() { + defaultCompilationHelper + .addSourceLines( + "Bar.java", + "package com.uber;", + "import org.checkerframework.checker.nullness.qual.Nullable;", + "class Bar {", + " public class Foo { }", + "}") + .addSourceLines( + "Test.java", + "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();", + " }", + "}") + .doTest(); + } + + @Test + public void annotationAppliedToInnerTypeOfTypeArgument() { + defaultCompilationHelper + .addSourceLines( + "Test.java", + "package com.uber;", + "import java.util.Set;", + "import org.checkerframework.checker.nullness.qual.Nullable;", + "class Test {", + " // BUG: Diagnostic contains: @NonNull field s not initialized", + " Set<@Nullable Foo> s;", // i.e. Set + " class Foo { }", + " public void test() {", + " // safe because field is @NonNull", + " s.hashCode();", + " }", + "}") + .doTest(); + } + + @Test + public void typeUseAnnotationOnArray() { + defaultCompilationHelper + .addSourceLines( + "Test.java", + "package com.uber;", + "import org.checkerframework.checker.nullness.qual.Nullable;", + "class Test {", + " // ok only for backwards compat", + " @Nullable Object[] foo1 = null;", + " // ok according to spec", + " Object @Nullable[] foo2 = null;", + " // ok only for backwards compat", + " @Nullable Object [][] foo3 = null;", + " // ok according to spec", + " Object @Nullable [][] foo4 = null;", + " // NOT ok; @Nullable applies to first array dimension not the elements or the array ref", + " // TODO: Fix this as part of https://github.com/uber/NullAway/issues/708", + " Object [] @Nullable [] foo5 = null;", + "}") + .doTest(); + } + + @Test + public void typeUseAnnotationOnInnerMultiLevel() { + defaultCompilationHelper + .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;", + "}") + .doTest(); + } +}