From 5756d1af8499afdf7529305a7b0b9cd8c821d2c8 Mon Sep 17 00:00:00 2001 From: Md Armughanuddin Date: Sun, 28 Jul 2024 22:24:37 -0500 Subject: [PATCH 01/14] Added legacy annotation flag --- .../main/java/com/uber/nullaway/Config.java | 7 ++++ .../com/uber/nullaway/DummyOptionsConfig.java | 5 +++ .../nullaway/ErrorProneCLIFlagsConfig.java | 10 +++++ .../com/uber/nullaway/NullabilityUtil.java | 2 +- .../nullaway/TypeUseAnnotationsTests.java | 37 +++++++++++++++++-- 5 files changed, 57 insertions(+), 4 deletions(-) diff --git a/nullaway/src/main/java/com/uber/nullaway/Config.java b/nullaway/src/main/java/com/uber/nullaway/Config.java index 10f87ea2d6..5c683b49a9 100644 --- a/nullaway/src/main/java/com/uber/nullaway/Config.java +++ b/nullaway/src/main/java/com/uber/nullaway/Config.java @@ -318,4 +318,11 @@ public interface Config { /** Should new checks based on JSpecify (like checks for generic types) be enabled? */ boolean isJSpecifyMode(); + + /** + * Checks if legacy annotation locations are enabled. + * + * @return true if both type use and declaration annotation locations should be honored + */ + boolean isLegacyAnnotationLocation(); } diff --git a/nullaway/src/main/java/com/uber/nullaway/DummyOptionsConfig.java b/nullaway/src/main/java/com/uber/nullaway/DummyOptionsConfig.java index 117670d5c4..43852e96b9 100644 --- a/nullaway/src/main/java/com/uber/nullaway/DummyOptionsConfig.java +++ b/nullaway/src/main/java/com/uber/nullaway/DummyOptionsConfig.java @@ -229,4 +229,9 @@ public boolean acknowledgeAndroidRecent() { public boolean isJSpecifyMode() { throw new IllegalStateException(ERROR_MESSAGE); } + + @Override + public boolean isLegacyAnnotationLocation() { + throw new IllegalStateException(ERROR_MESSAGE); + } } diff --git a/nullaway/src/main/java/com/uber/nullaway/ErrorProneCLIFlagsConfig.java b/nullaway/src/main/java/com/uber/nullaway/ErrorProneCLIFlagsConfig.java index dd7436af16..785e1c7099 100644 --- a/nullaway/src/main/java/com/uber/nullaway/ErrorProneCLIFlagsConfig.java +++ b/nullaway/src/main/java/com/uber/nullaway/ErrorProneCLIFlagsConfig.java @@ -103,6 +103,9 @@ final class ErrorProneCLIFlagsConfig implements Config { static final String FL_FIX_SERIALIZATION_CONFIG_PATH = EP_FL_NAMESPACE + ":FixSerializationConfigPath"; + static final String FL_LEGACY_ANNOTATION_LOCATION = + EP_FL_NAMESPACE + ":LegacyAnnotationLocations"; + private static final String DELIMITER = ","; static final ImmutableSet DEFAULT_CLASS_ANNOTATIONS_TO_EXCLUDE = @@ -208,6 +211,7 @@ final class ErrorProneCLIFlagsConfig implements Config { private final boolean treatGeneratedAsUnannotated; private final boolean acknowledgeAndroidRecent; private final boolean jspecifyMode; + private final boolean legacyAnnotationLocation; private final ImmutableSet knownInitializers; private final ImmutableSet excludedClassAnnotations; private final ImmutableSet generatedCodeAnnotations; @@ -288,6 +292,7 @@ final class ErrorProneCLIFlagsConfig implements Config { getPackagePattern( getFlagStringSet(flags, FL_EXCLUDED_FIELD_ANNOT, DEFAULT_EXCLUDED_FIELD_ANNOT)); castToNonNullMethod = flags.get(FL_CTNN_METHOD).orElse(null); + legacyAnnotationLocation = flags.getBoolean(FL_LEGACY_ANNOTATION_LOCATION).orElse(false); autofixSuppressionComment = flags.get(FL_SUPPRESS_COMMENT).orElse(""); optionalClassPaths = new ImmutableSet.Builder() @@ -584,6 +589,11 @@ public boolean isJSpecifyMode() { return jspecifyMode; } + @Override + public boolean isLegacyAnnotationLocation() { + return legacyAnnotationLocation; + } + @AutoValue abstract static class MethodClassAndName { diff --git a/nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java b/nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java index 6092d46a55..a63fd09358 100644 --- a/nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java +++ b/nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java @@ -331,7 +331,7 @@ private static boolean isDirectTypeUseAnnotation(Attribute.TypeCompound t, Confi locationHasInnerTypes = true; break; case ARRAY: - if (config.isJSpecifyMode()) { + if (config.isJSpecifyMode() || !config.isLegacyAnnotationLocation()) { // In JSpecify mode, annotations on array element types do not apply to the top-level // type return false; diff --git a/nullaway/src/test/java/com/uber/nullaway/TypeUseAnnotationsTests.java b/nullaway/src/test/java/com/uber/nullaway/TypeUseAnnotationsTests.java index 79d4ea5c0e..68dbaac576 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; @@ -184,8 +186,8 @@ public void annotationAppliedToInnerTypeOfTypeArgument() { } @Test - public void typeUseAnnotationOnArray() { - defaultCompilationHelper + public void typeUseLegacyAnnotationOnArray() { + makeHelper() .addSourceLines( "Test.java", "package com.uber;", @@ -200,7 +202,32 @@ public void typeUseAnnotationOnArray() { " // 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 typeUseAnnotationOnArray() { + defaultCompilationHelper + .addSourceLines( + "Test.java", + "package com.uber;", + "import org.checkerframework.checker.nullness.qual.Nullable;", + "class Test {", + " // @Nullable is not applied on top-level of array", + " // BUG: Diagnostic contains: assigning @Nullable expression to @NonNull field", + " @Nullable Object[] foo1 = null;", + " // ok according to spec", + " Object @Nullable[] foo2 = null;", + " // ok only for backwards compat", + " // @Nullable is not applied on top-level of array", + " // BUG: Diagnostic contains: assigning @Nullable expression to @NonNull field", + " @Nullable Object [][] foo3 = null;", + " // ok according to spec", + " Object @Nullable [][] foo4 = null;", + " // @Nullable is not applied on top-level of array", + " // BUG: Diagnostic contains: assigning @Nullable expression to @NonNull field", " Object [] @Nullable [] foo5 = null;", "}") .doTest(); @@ -263,4 +290,8 @@ public void typeParameterAnnotationIsDistinctFromMethodReturnAnnotation() { "}") .doTest(); } + + private CompilationTestHelper makeHelper() { + return makeTestHelperWithArgs(Arrays.asList("-XepOpt:NullAway:LegacyAnnotationLocations=true")); + } } From 6bb33b586037f51363aaf0eaf9df332166541ae1 Mon Sep 17 00:00:00 2001 From: Md Armughanuddin Date: Sun, 28 Jul 2024 22:34:56 -0500 Subject: [PATCH 02/14] fixed comment --- .../main/java/com/uber/nullaway/NullabilityUtil.java | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java b/nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java index a63fd09358..ae0249b979 100644 --- a/nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java +++ b/nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java @@ -312,10 +312,12 @@ private static boolean isDirectTypeUseAnnotation(Attribute.TypeCompound t, Confi // 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, outside JSpecify mode, 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. - // In JSpecify mode, annotations on array dimensions are *not* treated as applying to the - // top-level type, consistent with the JSpecify spec. + // For arrays, when the LegacyAnnotationLocations flag is passed, 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. + // 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. // 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 f9f863e2db5115abc3012cd6732aecfced990e34 Mon Sep 17 00:00:00 2001 From: Md Armughanuddin Date: Sun, 28 Jul 2024 22:51:21 -0500 Subject: [PATCH 03/14] Fixed 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 ae0249b979..c8c41bdfce 100644 --- a/nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java +++ b/nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java @@ -334,8 +334,8 @@ private static boolean isDirectTypeUseAnnotation(Attribute.TypeCompound t, Confi break; case ARRAY: if (config.isJSpecifyMode() || !config.isLegacyAnnotationLocation()) { - // In JSpecify mode, annotations on array element types do not apply to the top-level - // type + // Annotations on array element types do not apply to the top-level + // type outside of legacy mode return false; } locationHasArray = true; From 6b4ab88c69ec1c60fa8940d439968654ae819bc0 Mon Sep 17 00:00:00 2001 From: Md Armughanuddin Date: Mon, 29 Jul 2024 00:44:50 -0500 Subject: [PATCH 04/14] Fix args --- .../test/java/com/uber/nullaway/TypeUseAnnotationsTests.java | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/nullaway/src/test/java/com/uber/nullaway/TypeUseAnnotationsTests.java b/nullaway/src/test/java/com/uber/nullaway/TypeUseAnnotationsTests.java index 68dbaac576..a695413a31 100644 --- a/nullaway/src/test/java/com/uber/nullaway/TypeUseAnnotationsTests.java +++ b/nullaway/src/test/java/com/uber/nullaway/TypeUseAnnotationsTests.java @@ -292,6 +292,9 @@ public void typeParameterAnnotationIsDistinctFromMethodReturnAnnotation() { } private CompilationTestHelper makeHelper() { - return makeTestHelperWithArgs(Arrays.asList("-XepOpt:NullAway:LegacyAnnotationLocations=true")); + return makeTestHelperWithArgs( + Arrays.asList( + "-XepOpt:NullAway:AnnotatedPackages=com.uber", + "-XepOpt:NullAway:LegacyAnnotationLocations=true")); } } From ccc7b1724bc8a91cd8e962f76752d79e9956b89e Mon Sep 17 00:00:00 2001 From: Md Armughanuddin Date: Mon, 29 Jul 2024 10:26:44 -0500 Subject: [PATCH 05/14] Added legacy flag for caffeine compiler --- .../main/java/com/uber/nullaway/jmh/CaffeineCompiler.java | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/jmh/src/main/java/com/uber/nullaway/jmh/CaffeineCompiler.java b/jmh/src/main/java/com/uber/nullaway/jmh/CaffeineCompiler.java index 59525f25eb..64ffd73859 100644 --- a/jmh/src/main/java/com/uber/nullaway/jmh/CaffeineCompiler.java +++ b/jmh/src/main/java/com/uber/nullaway/jmh/CaffeineCompiler.java @@ -109,4 +109,9 @@ protected String getAnnotatedPackages() { protected String getClasspath() { return System.getProperty("nullaway.caffeine.classpath"); } + + @Override + protected List getExtraErrorProneArgs() { + return List.of("-XepOpt:NullAway:LegacyAnnotationLocations=true"); + } } From 819fdfce45158c6bccb9b345a3c1fcb68e779bc8 Mon Sep 17 00:00:00 2001 From: Md Armughanuddin Date: Mon, 29 Jul 2024 10:32:36 -0500 Subject: [PATCH 06/14] Added declaration annotation test --- .../nullaway/TypeUseAnnotationsTests.java | 20 +++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/nullaway/src/test/java/com/uber/nullaway/TypeUseAnnotationsTests.java b/nullaway/src/test/java/com/uber/nullaway/TypeUseAnnotationsTests.java index a695413a31..b31cb337b5 100644 --- a/nullaway/src/test/java/com/uber/nullaway/TypeUseAnnotationsTests.java +++ b/nullaway/src/test/java/com/uber/nullaway/TypeUseAnnotationsTests.java @@ -207,6 +207,26 @@ public void typeUseLegacyAnnotationOnArray() { .doTest(); } + @Test + public void arrayDeclarationAnnotation() { + defaultCompilationHelper + .addSourceLines( + "Test.java", + "package com.uber;", + "import javax.annotation.Nullable;", + "class Test {", + " static @Nullable String [] fizz = {\"1\"};", + " static Object o1 = new Object();", + " static void foo() {", + " // BUG: Diagnostic contains: assigning @Nullable expression to @NonNull field", + " o1 = fizz;", + " // BUG: Diagnostic contains: dereferenced expression fizz is @Nullable", + " o1 = fizz.length;", + " }", + "}") + .doTest(); + } + @Test public void typeUseAnnotationOnArray() { defaultCompilationHelper From e497e1b1547eef472d1cc6a420d9ef8f406e9746 Mon Sep 17 00:00:00 2001 From: Md Armughanuddin Date: Mon, 12 Aug 2024 01:57:03 -0500 Subject: [PATCH 07/14] Added tests --- .../java/com/uber/nullaway/ArrayTests.java | 168 ++++++++++++++++++ .../nullaway/TypeUseAnnotationsTests.java | 77 -------- .../uber/nullaway/jspecify/ArrayTests.java | 19 ++ 3 files changed, 187 insertions(+), 77 deletions(-) create mode 100644 nullaway/src/test/java/com/uber/nullaway/ArrayTests.java diff --git a/nullaway/src/test/java/com/uber/nullaway/ArrayTests.java b/nullaway/src/test/java/com/uber/nullaway/ArrayTests.java new file mode 100644 index 0000000000..4e653e37dc --- /dev/null +++ b/nullaway/src/test/java/com/uber/nullaway/ArrayTests.java @@ -0,0 +1,168 @@ +/* + * 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 com.google.errorprone.CompilationTestHelper; +import java.util.Arrays; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +@RunWith(JUnit4.class) +public class ArrayTests extends NullAwayTestsBase { + @Test + public void arrayDeclarationAnnotation() { + defaultCompilationHelper + .addSourceLines( + "Test.java", + "package com.uber;", + "import javax.annotation.Nullable;", + "class Test {", + " static @Nullable String [] fizz = {\"1\"};", + " static Object o1 = new Object();", + " static void foo() {", + " // BUG: Diagnostic contains: assigning @Nullable expression to @NonNull field", + " o1 = fizz;", + " // BUG: Diagnostic contains: dereferenced expression fizz is @Nullable", + " o1 = fizz.length;", + " }", + "}") + .doTest(); + } + + @Test + public void arrayLegacyDeclarationAnnotation() { + makeHelper() + .addSourceLines( + "Test.java", + "package com.uber;", + "import javax.annotation.Nullable;", + "class Test {", + " static @Nullable String [] fizz = {\"1\"};", + " static Object o1 = new Object();", + " static void foo() {", + " // BUG: Diagnostic contains: assigning @Nullable expression to @NonNull field", + " o1 = fizz;", + " // BUG: Diagnostic contains: dereferenced expression fizz is @Nullable", + " o1 = fizz.length;", + " }", + "}") + .doTest(); + } + + @Test + public void typeUseLegacyAnnotationOnArray() { + makeHelper() + .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, but @Nullable is not applied on top-level of array ", + " @Nullable Object @Nullable[] foo3 = null;", + " // ok only for backwards compat", + " @Nullable Object [][] foo4 = null;", + " // ok according to spec", + " Object @Nullable [][] foo5 = null;", + " // NOT ok; @Nullable applies to first array dimension not the elements or the array ref", + " Object [] @Nullable [] foo6 = null;", + "}") + .doTest(); + } + + @Test + public void typeUseAnnotationOnArray() { + defaultCompilationHelper + .addSourceLines( + "Test.java", + "package com.uber;", + "import org.checkerframework.checker.nullness.qual.Nullable;", + "class Test {", + " // @Nullable is not applied on top-level of array", + " // BUG: Diagnostic contains: assigning @Nullable expression to @NonNull field", + " @Nullable Object[] foo1 = null;", + " // ok according to spec", + " Object @Nullable[] foo2 = null;", + " // ok, but @Nullable is not applied on top-level of array ", + " @Nullable Object @Nullable [] foo3 = null;", + " // ok only for backwards compat", + " // @Nullable is not applied on top-level of array", + " // BUG: Diagnostic contains: assigning @Nullable expression to @NonNull field", + " @Nullable Object [][] foo4 = null;", + " // ok according to spec", + " Object @Nullable [][] foo5 = null;", + " // @Nullable is not applied on top-level of array", + " // BUG: Diagnostic contains: assigning @Nullable expression to @NonNull field", + " Object [] @Nullable [] foo6 = null;", + "}") + .doTest(); + } + + @Test + public void typeUseAndDeclarationAnnotationOnArray() { + defaultCompilationHelper + .addSourceLines( + "Test.java", + "package com.uber;", + "import org.jetbrains.annotations.Nullable;", + "class Test {", + " @Nullable Object[] foo1 = null;", + " Object @Nullable[] foo2 = null;", + " @Nullable Object @Nullable [] foo3 = null;", + " @Nullable Object [][] foo4 = null;", + " Object @Nullable [][] foo5 = null;", + " // BUG: Diagnostic contains: assigning @Nullable expression to @NonNull field", + " Object [] @Nullable [] foo6 = null;", + "}") + .doTest(); + } + + @Test + public void typeUseAndDeclarationLegacyAnnotationOnArray() { + makeHelper() + .addSourceLines( + "Test.java", + "package com.uber;", + "import org.jetbrains.annotations.Nullable;", + "class Test {", + " @Nullable Object[] foo1 = null;", + " Object @Nullable[] foo2 = null;", + " @Nullable Object @Nullable [] foo3 = null;", + " @Nullable Object [][] foo4 = null;", + " Object @Nullable [][] foo5 = null;", + " Object [] @Nullable [] foo6 = null;", + "}") + .doTest(); + } + + private CompilationTestHelper makeHelper() { + return makeTestHelperWithArgs( + Arrays.asList( + "-XepOpt:NullAway:AnnotatedPackages=com.uber", + "-XepOpt:NullAway:LegacyAnnotationLocations=true")); + } +} diff --git a/nullaway/src/test/java/com/uber/nullaway/TypeUseAnnotationsTests.java b/nullaway/src/test/java/com/uber/nullaway/TypeUseAnnotationsTests.java index b31cb337b5..3a997403f5 100644 --- a/nullaway/src/test/java/com/uber/nullaway/TypeUseAnnotationsTests.java +++ b/nullaway/src/test/java/com/uber/nullaway/TypeUseAnnotationsTests.java @@ -22,8 +22,6 @@ 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; @@ -185,74 +183,6 @@ public void annotationAppliedToInnerTypeOfTypeArgument() { .doTest(); } - @Test - public void typeUseLegacyAnnotationOnArray() { - makeHelper() - .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", - " Object [] @Nullable [] foo5 = null;", - "}") - .doTest(); - } - - @Test - public void arrayDeclarationAnnotation() { - defaultCompilationHelper - .addSourceLines( - "Test.java", - "package com.uber;", - "import javax.annotation.Nullable;", - "class Test {", - " static @Nullable String [] fizz = {\"1\"};", - " static Object o1 = new Object();", - " static void foo() {", - " // BUG: Diagnostic contains: assigning @Nullable expression to @NonNull field", - " o1 = fizz;", - " // BUG: Diagnostic contains: dereferenced expression fizz is @Nullable", - " o1 = fizz.length;", - " }", - "}") - .doTest(); - } - - @Test - public void typeUseAnnotationOnArray() { - defaultCompilationHelper - .addSourceLines( - "Test.java", - "package com.uber;", - "import org.checkerframework.checker.nullness.qual.Nullable;", - "class Test {", - " // @Nullable is not applied on top-level of array", - " // BUG: Diagnostic contains: assigning @Nullable expression to @NonNull field", - " @Nullable Object[] foo1 = null;", - " // ok according to spec", - " Object @Nullable[] foo2 = null;", - " // ok only for backwards compat", - " // @Nullable is not applied on top-level of array", - " // BUG: Diagnostic contains: assigning @Nullable expression to @NonNull field", - " @Nullable Object [][] foo3 = null;", - " // ok according to spec", - " Object @Nullable [][] foo4 = null;", - " // @Nullable is not applied on top-level of array", - " // BUG: Diagnostic contains: assigning @Nullable expression to @NonNull field", - " Object [] @Nullable [] foo5 = null;", - "}") - .doTest(); - } - @Test public void typeUseAnnotationOnInnerMultiLevel() { defaultCompilationHelper @@ -310,11 +240,4 @@ public void typeParameterAnnotationIsDistinctFromMethodReturnAnnotation() { "}") .doTest(); } - - private CompilationTestHelper makeHelper() { - return makeTestHelperWithArgs( - Arrays.asList( - "-XepOpt:NullAway:AnnotatedPackages=com.uber", - "-XepOpt:NullAway:LegacyAnnotationLocations=true")); - } } diff --git a/nullaway/src/test/java/com/uber/nullaway/jspecify/ArrayTests.java b/nullaway/src/test/java/com/uber/nullaway/jspecify/ArrayTests.java index 796e8a4437..70a52fa72c 100644 --- a/nullaway/src/test/java/com/uber/nullaway/jspecify/ArrayTests.java +++ b/nullaway/src/test/java/com/uber/nullaway/jspecify/ArrayTests.java @@ -608,6 +608,25 @@ public void mismatchedIndexUse() { .doTest(); } + @Test + public void typeUseAndDeclarationLegacyAnnotationOnArray() { + makeHelper() + .addSourceLines( + "Test.java", + "package com.uber;", + "import org.jetbrains.annotations.Nullable;", + "class Test {", + " @Nullable Object[] foo1 = null;", + " Object @Nullable[] foo2 = null;", + " @Nullable Object @Nullable [] foo3 = null;", + " @Nullable Object [][] foo4 = null;", + " Object @Nullable [][] foo5 = null;", + " // BUG: Diagnostic contains: assigning @Nullable expression to @NonNull field", + " Object [] @Nullable [] foo6 = null;", + "}") + .doTest(); + } + private CompilationTestHelper makeHelper() { return makeTestHelperWithArgs( Arrays.asList( From 4b6187f8453f91f3d4397de0abec01c0f74f4a0e Mon Sep 17 00:00:00 2001 From: Md Armughanuddin Date: Tue, 20 Aug 2024 00:35:30 -0500 Subject: [PATCH 08/14] Address review comments --- .../java/com/uber/nullaway/ArrayTests.java | 31 +++++++++++-------- .../uber/nullaway/jspecify/ArrayTests.java | 19 +++++++++--- 2 files changed, 33 insertions(+), 17 deletions(-) diff --git a/nullaway/src/test/java/com/uber/nullaway/ArrayTests.java b/nullaway/src/test/java/com/uber/nullaway/ArrayTests.java index 4e653e37dc..cb4c3fa4bd 100644 --- a/nullaway/src/test/java/com/uber/nullaway/ArrayTests.java +++ b/nullaway/src/test/java/com/uber/nullaway/ArrayTests.java @@ -52,7 +52,7 @@ public void arrayDeclarationAnnotation() { @Test public void arrayLegacyDeclarationAnnotation() { - makeHelper() + makeLegacyModeHelper() .addSourceLines( "Test.java", "package com.uber;", @@ -72,11 +72,11 @@ public void arrayLegacyDeclarationAnnotation() { @Test public void typeUseLegacyAnnotationOnArray() { - makeHelper() + makeLegacyModeHelper() .addSourceLines( "Test.java", "package com.uber;", - "import org.checkerframework.checker.nullness.qual.Nullable;", + "import org.jspecify.annotations.Nullable;", "class Test {", " // ok only for backwards compat", " @Nullable Object[] foo1 = null;", @@ -88,7 +88,7 @@ public void typeUseLegacyAnnotationOnArray() { " @Nullable Object [][] foo4 = null;", " // ok according to spec", " Object @Nullable [][] foo5 = null;", - " // NOT ok; @Nullable applies to first array dimension not the elements or the array ref", + " // ok, but @Nullable applies to first array dimension not the elements or the array ref", " Object [] @Nullable [] foo6 = null;", "}") .doTest(); @@ -100,16 +100,15 @@ public void typeUseAnnotationOnArray() { .addSourceLines( "Test.java", "package com.uber;", - "import org.checkerframework.checker.nullness.qual.Nullable;", + "import org.jspecify.annotations.Nullable;", "class Test {", " // @Nullable is not applied on top-level of array", " // BUG: Diagnostic contains: assigning @Nullable expression to @NonNull field", " @Nullable Object[] foo1 = null;", " // ok according to spec", " Object @Nullable[] foo2 = null;", - " // ok, but @Nullable is not applied on top-level of array ", + " // ok according to spec", " @Nullable Object @Nullable [] foo3 = null;", - " // ok only for backwards compat", " // @Nullable is not applied on top-level of array", " // BUG: Diagnostic contains: assigning @Nullable expression to @NonNull field", " @Nullable Object [][] foo4 = null;", @@ -126,9 +125,12 @@ public void typeUseAnnotationOnArray() { public void typeUseAndDeclarationAnnotationOnArray() { defaultCompilationHelper .addSourceLines( - "Test.java", + "Nullable.java", "package com.uber;", - "import org.jetbrains.annotations.Nullable;", + "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 {}", "class Test {", " @Nullable Object[] foo1 = null;", " Object @Nullable[] foo2 = null;", @@ -143,11 +145,14 @@ public void typeUseAndDeclarationAnnotationOnArray() { @Test public void typeUseAndDeclarationLegacyAnnotationOnArray() { - makeHelper() + makeLegacyModeHelper() .addSourceLines( - "Test.java", + "Nullable.java", "package com.uber;", - "import org.jetbrains.annotations.Nullable;", + "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 {}", "class Test {", " @Nullable Object[] foo1 = null;", " Object @Nullable[] foo2 = null;", @@ -159,7 +164,7 @@ public void typeUseAndDeclarationLegacyAnnotationOnArray() { .doTest(); } - private CompilationTestHelper makeHelper() { + private CompilationTestHelper makeLegacyModeHelper() { return makeTestHelperWithArgs( Arrays.asList( "-XepOpt:NullAway:AnnotatedPackages=com.uber", diff --git a/nullaway/src/test/java/com/uber/nullaway/jspecify/ArrayTests.java b/nullaway/src/test/java/com/uber/nullaway/jspecify/ArrayTests.java index 70a52fa72c..d42bbb3273 100644 --- a/nullaway/src/test/java/com/uber/nullaway/jspecify/ArrayTests.java +++ b/nullaway/src/test/java/com/uber/nullaway/jspecify/ArrayTests.java @@ -609,15 +609,26 @@ public void mismatchedIndexUse() { } @Test - public void typeUseAndDeclarationLegacyAnnotationOnArray() { + public void typeUseAndDeclarationAnnotationOnArray() { makeHelper() .addSourceLines( - "Test.java", + "Nullable.java", "package com.uber;", - "import org.jetbrains.annotations.Nullable;", + "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 {}", "class Test {", " @Nullable Object[] foo1 = null;", - " Object @Nullable[] foo2 = null;", + " static String @Nullable[] foo2 = null;", + " static void bar() {", + " if (foo2 !=null){", + " // annotation is treated as declaration", + " String bar = foo2[0].toString(); ", + " }", + " // BUG: Diagnostic contains: dereferenced expression foo2 is @Nullable", + " String bar = foo2[0].toString(); ", + " }", " @Nullable Object @Nullable [] foo3 = null;", " @Nullable Object [][] foo4 = null;", " Object @Nullable [][] foo5 = null;", From 69b81ce75ce8502ee2efcfd63e32947c8f7221ba Mon Sep 17 00:00:00 2001 From: Md Armughanuddin Date: Tue, 20 Aug 2024 00:36:40 -0500 Subject: [PATCH 09/14] Update nullaway/src/test/java/com/uber/nullaway/ArrayTests.java Co-authored-by: Manu Sridharan --- nullaway/src/test/java/com/uber/nullaway/ArrayTests.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/nullaway/src/test/java/com/uber/nullaway/ArrayTests.java b/nullaway/src/test/java/com/uber/nullaway/ArrayTests.java index cb4c3fa4bd..9a41ada1a5 100644 --- a/nullaway/src/test/java/com/uber/nullaway/ArrayTests.java +++ b/nullaway/src/test/java/com/uber/nullaway/ArrayTests.java @@ -82,7 +82,7 @@ public void typeUseLegacyAnnotationOnArray() { " @Nullable Object[] foo1 = null;", " // ok according to spec", " Object @Nullable[] foo2 = null;", - " // ok, but @Nullable is not applied on top-level of array ", + " // ok, but elements are not treated as @Nullable outside of JSpecify mode", " @Nullable Object @Nullable[] foo3 = null;", " // ok only for backwards compat", " @Nullable Object [][] foo4 = null;", From 6dce3c67a4316dfb95416db5c384851ea48ed1e8 Mon Sep 17 00:00:00 2001 From: Md Armughanuddin Date: Wed, 21 Aug 2024 23:39:22 -0500 Subject: [PATCH 10/14] Address review comments --- .../com/uber/nullaway/ErrorProneCLIFlagsConfig.java | 8 ++++++++ .../src/test/java/com/uber/nullaway/ArrayTests.java | 10 ++++++++-- .../java/com/uber/nullaway/jspecify/ArrayTests.java | 5 ++++- 3 files changed, 20 insertions(+), 3 deletions(-) diff --git a/nullaway/src/main/java/com/uber/nullaway/ErrorProneCLIFlagsConfig.java b/nullaway/src/main/java/com/uber/nullaway/ErrorProneCLIFlagsConfig.java index 785e1c7099..95a498cf54 100644 --- a/nullaway/src/main/java/com/uber/nullaway/ErrorProneCLIFlagsConfig.java +++ b/nullaway/src/main/java/com/uber/nullaway/ErrorProneCLIFlagsConfig.java @@ -293,6 +293,14 @@ final class ErrorProneCLIFlagsConfig implements Config { getFlagStringSet(flags, FL_EXCLUDED_FIELD_ANNOT, DEFAULT_EXCLUDED_FIELD_ANNOT)); castToNonNullMethod = flags.get(FL_CTNN_METHOD).orElse(null); legacyAnnotationLocation = flags.getBoolean(FL_LEGACY_ANNOTATION_LOCATION).orElse(false); + if (legacyAnnotationLocation && jspecifyMode) { + throw new IllegalStateException( + "-XepOpt:" + + FL_LEGACY_ANNOTATION_LOCATION + + " cannot be used when " + + FL_JSPECIFY_MODE + + " is set "); + } autofixSuppressionComment = flags.get(FL_SUPPRESS_COMMENT).orElse(""); optionalClassPaths = new ImmutableSet.Builder() diff --git a/nullaway/src/test/java/com/uber/nullaway/ArrayTests.java b/nullaway/src/test/java/com/uber/nullaway/ArrayTests.java index 9a41ada1a5..1d6b95c17d 100644 --- a/nullaway/src/test/java/com/uber/nullaway/ArrayTests.java +++ b/nullaway/src/test/java/com/uber/nullaway/ArrayTests.java @@ -130,7 +130,10 @@ public void typeUseAndDeclarationAnnotationOnArray() { "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 {}", + "public @interface Nullable {}") + .addSourceLines( + "Test.java", + "package com.uber;", "class Test {", " @Nullable Object[] foo1 = null;", " Object @Nullable[] foo2 = null;", @@ -152,7 +155,10 @@ public void typeUseAndDeclarationLegacyAnnotationOnArray() { "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 {}", + "public @interface Nullable {}") + .addSourceLines( + "Test.java", + "package com.uber;", "class Test {", " @Nullable Object[] foo1 = null;", " Object @Nullable[] foo2 = null;", diff --git a/nullaway/src/test/java/com/uber/nullaway/jspecify/ArrayTests.java b/nullaway/src/test/java/com/uber/nullaway/jspecify/ArrayTests.java index d42bbb3273..1e14fd1ba9 100644 --- a/nullaway/src/test/java/com/uber/nullaway/jspecify/ArrayTests.java +++ b/nullaway/src/test/java/com/uber/nullaway/jspecify/ArrayTests.java @@ -617,7 +617,10 @@ public void typeUseAndDeclarationAnnotationOnArray() { "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 {}", + "public @interface Nullable {}") + .addSourceLines( + "Test.java", + "package com.uber;", "class Test {", " @Nullable Object[] foo1 = null;", " static String @Nullable[] foo2 = null;", From 3545a59454aff4d6d08c4bab17814bdfa5b65272 Mon Sep 17 00:00:00 2001 From: Md Armughanuddin Date: Wed, 21 Aug 2024 23:54:28 -0500 Subject: [PATCH 11/14] Address review comments --- .../java/com/uber/nullaway/jspecify/ArrayTests.java | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/nullaway/src/test/java/com/uber/nullaway/jspecify/ArrayTests.java b/nullaway/src/test/java/com/uber/nullaway/jspecify/ArrayTests.java index 1e14fd1ba9..533402aac5 100644 --- a/nullaway/src/test/java/com/uber/nullaway/jspecify/ArrayTests.java +++ b/nullaway/src/test/java/com/uber/nullaway/jspecify/ArrayTests.java @@ -632,7 +632,16 @@ public void typeUseAndDeclarationAnnotationOnArray() { " // BUG: Diagnostic contains: dereferenced expression foo2 is @Nullable", " String bar = foo2[0].toString(); ", " }", - " @Nullable Object @Nullable [] foo3 = null;", + " static @Nullable String @Nullable [] foo3 = null;", + " static void fizz() {", + " if (foo3 !=null){", + " // annotation is also applied to the elements", + " // BUG: Diagnostic contains: dereferenced expression foo3[0] is @Nullable", + " String bar = foo3[0].toString(); ", + " }", + " // BUG: Diagnostic contains: dereferenced expression foo3 is @Nullable", + " String bar = foo3[0].toString(); ", + " }", " @Nullable Object [][] foo4 = null;", " Object @Nullable [][] foo5 = null;", " // BUG: Diagnostic contains: assigning @Nullable expression to @NonNull field", From 6105a3de67f3eaeb214b4874779c6fe29fcce08a Mon Sep 17 00:00:00 2001 From: Md Armughanuddin Date: Fri, 23 Aug 2024 02:44:11 -0500 Subject: [PATCH 12/14] Added legacy and jspecify flag test --- .../nullaway/LegacyAndJspecifyModeTest.java | 47 +++++++++++++++++++ 1 file changed, 47 insertions(+) create mode 100644 nullaway/src/test/java/com/uber/nullaway/LegacyAndJspecifyModeTest.java diff --git a/nullaway/src/test/java/com/uber/nullaway/LegacyAndJspecifyModeTest.java b/nullaway/src/test/java/com/uber/nullaway/LegacyAndJspecifyModeTest.java new file mode 100644 index 0000000000..ebc90f202a --- /dev/null +++ b/nullaway/src/test/java/com/uber/nullaway/LegacyAndJspecifyModeTest.java @@ -0,0 +1,47 @@ +package com.uber.nullaway; + +import static com.uber.nullaway.ErrorProneCLIFlagsConfig.FL_JSPECIFY_MODE; +import static com.uber.nullaway.ErrorProneCLIFlagsConfig.FL_LEGACY_ANNOTATION_LOCATION; +import static org.hamcrest.CoreMatchers.instanceOf; +import static org.hamcrest.MatcherAssert.assertThat; +import static org.junit.Assert.assertEquals; +import static org.junit.jupiter.api.Assertions.assertThrows; + +import com.google.errorprone.ErrorProneFlags; +import java.lang.reflect.Constructor; +import java.lang.reflect.InvocationTargetException; +import org.junit.Test; + +public class LegacyAndJspecifyModeTest { + + private static final String ERROR = + "-XepOpt:" + + FL_LEGACY_ANNOTATION_LOCATION + + " cannot be used when " + + FL_JSPECIFY_MODE + + " is set "; + + @Test + public void testIllegalStateExceptionUsingReflection() throws Exception { + ErrorProneFlags flags = + ErrorProneFlags.builder() + .putFlag("NullAway:AnnotatedPackages", "com.uber") + .putFlag("NullAway:JSpecifyMode", "true") + .putFlag("NullAway:LegacyAnnotationLocations", "true") + .build(); + + Constructor constructor = + ErrorProneCLIFlagsConfig.class.getDeclaredConstructor(ErrorProneFlags.class); + constructor.setAccessible(true); + + InvocationTargetException exception = + assertThrows( + InvocationTargetException.class, + () -> constructor.newInstance(flags), + "Expected IllegalStateException when both jspecifyMode and legacyAnnotationLocation are true."); + + Throwable cause = exception.getCause(); + assertThat(cause, instanceOf(IllegalStateException.class)); + assertEquals(cause.getMessage(), ERROR); + } +} From a6a79e06079afa5162abf7e1270b69c51fb0bc20 Mon Sep 17 00:00:00 2001 From: Manu Sridharan Date: Fri, 23 Aug 2024 10:26:50 -0700 Subject: [PATCH 13/14] add / update copyright notices --- .../nullaway.java-test-conventions.gradle | 2 +- .../java/com/uber/nullaway/ArrayTests.java | 2 +- .../uber/nullaway/jspecify/ArrayTests.java | 22 +++++++++++++++++++ 3 files changed, 24 insertions(+), 2 deletions(-) diff --git a/buildSrc/src/main/groovy/nullaway.java-test-conventions.gradle b/buildSrc/src/main/groovy/nullaway.java-test-conventions.gradle index 37021611d0..02bbac208d 100644 --- a/buildSrc/src/main/groovy/nullaway.java-test-conventions.gradle +++ b/buildSrc/src/main/groovy/nullaway.java-test-conventions.gradle @@ -65,7 +65,7 @@ test { } // Tasks for testing on other JDK versions; see https://jakewharton.com/build-on-latest-java-test-through-lowest-java/ -[21, 22].each { majorVersion -> +[21, 22, 24].each { majorVersion -> def jdkTest = tasks.register("testJdk$majorVersion", Test) { onlyIf { // Only run when using the latest Error Prone version diff --git a/nullaway/src/test/java/com/uber/nullaway/ArrayTests.java b/nullaway/src/test/java/com/uber/nullaway/ArrayTests.java index 1d6b95c17d..5327408268 100644 --- a/nullaway/src/test/java/com/uber/nullaway/ArrayTests.java +++ b/nullaway/src/test/java/com/uber/nullaway/ArrayTests.java @@ -1,5 +1,5 @@ /* - * Copyright (c) 2023 Uber Technologies, Inc. + * Copyright (c) 2024 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 diff --git a/nullaway/src/test/java/com/uber/nullaway/jspecify/ArrayTests.java b/nullaway/src/test/java/com/uber/nullaway/jspecify/ArrayTests.java index 533402aac5..eee8b80969 100644 --- a/nullaway/src/test/java/com/uber/nullaway/jspecify/ArrayTests.java +++ b/nullaway/src/test/java/com/uber/nullaway/jspecify/ArrayTests.java @@ -1,3 +1,25 @@ +/* + * Copyright (c) 2024 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.jspecify; import com.google.errorprone.CompilationTestHelper; From 1d6c0dd28ce2a96608528d56c513caf893f07bac Mon Sep 17 00:00:00 2001 From: Manu Sridharan Date: Fri, 23 Aug 2024 10:27:41 -0700 Subject: [PATCH 14/14] revert change --- buildSrc/src/main/groovy/nullaway.java-test-conventions.gradle | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/buildSrc/src/main/groovy/nullaway.java-test-conventions.gradle b/buildSrc/src/main/groovy/nullaway.java-test-conventions.gradle index 02bbac208d..37021611d0 100644 --- a/buildSrc/src/main/groovy/nullaway.java-test-conventions.gradle +++ b/buildSrc/src/main/groovy/nullaway.java-test-conventions.gradle @@ -65,7 +65,7 @@ test { } // Tasks for testing on other JDK versions; see https://jakewharton.com/build-on-latest-java-test-through-lowest-java/ -[21, 22, 24].each { majorVersion -> +[21, 22].each { majorVersion -> def jdkTest = tasks.register("testJdk$majorVersion", Test) { onlyIf { // Only run when using the latest Error Prone version