From 72656117b2b952d51a91521670eb254bd905ee60 Mon Sep 17 00:00:00 2001 From: Manu Sridharan Date: Tue, 18 Jul 2023 16:59:20 -0700 Subject: [PATCH] Revert "Resolve regression for type annotations directly on inner types. (#706)" This reverts commit c5e1a799e921a9620e5332af4c249fc1931ebeb3. --- .../com/uber/nullaway/NullabilityUtil.java | 34 +-- .../com/uber/nullaway/NullAwayCoreTests.java | 30 +++ .../NullAwayTypeUseAnnotationsTests.java | 221 ------------------ 3 files changed, 31 insertions(+), 254 deletions(-) delete mode 100644 nullaway/src/test/java/com/uber/nullaway/NullAwayTypeUseAnnotationsTests.java diff --git a/nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java b/nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java index c5e8716e17..ee1e6e8a82 100644 --- a/nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java +++ b/nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java @@ -22,9 +22,6 @@ 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; @@ -44,7 +41,6 @@ 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; @@ -296,35 +292,7 @@ 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. - // 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); + return t.position.location.isEmpty(); } /** diff --git a/nullaway/src/test/java/com/uber/nullaway/NullAwayCoreTests.java b/nullaway/src/test/java/com/uber/nullaway/NullAwayCoreTests.java index f8f3ed622c..0f4afe44a0 100644 --- a/nullaway/src/test/java/com/uber/nullaway/NullAwayCoreTests.java +++ b/nullaway/src/test/java/com/uber/nullaway/NullAwayCoreTests.java @@ -960,4 +960,34 @@ 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 deleted file mode 100644 index 7af8b253d4..0000000000 --- a/nullaway/src/test/java/com/uber/nullaway/NullAwayTypeUseAnnotationsTests.java +++ /dev/null @@ -1,221 +0,0 @@ -/* - * 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(); - } -}