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"); + } } 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..95a498cf54 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,15 @@ 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); + 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() @@ -584,6 +597,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..c8c41bdfce 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 @@ -331,9 +333,9 @@ private static boolean isDirectTypeUseAnnotation(Attribute.TypeCompound t, Confi locationHasInnerTypes = true; break; case ARRAY: - if (config.isJSpecifyMode()) { - // In JSpecify mode, annotations on array element types do not apply to the top-level - // type + if (config.isJSpecifyMode() || !config.isLegacyAnnotationLocation()) { + // Annotations on array element types do not apply to the top-level + // type outside of legacy mode return false; } locationHasArray = true; 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..5327408268 --- /dev/null +++ b/nullaway/src/test/java/com/uber/nullaway/ArrayTests.java @@ -0,0 +1,179 @@ +/* + * 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; + +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() { + makeLegacyModeHelper() + .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() { + makeLegacyModeHelper() + .addSourceLines( + "Test.java", + "package com.uber;", + "import org.jspecify.annotations.Nullable;", + "class Test {", + " // ok only for backwards compat", + " @Nullable Object[] foo1 = null;", + " // ok according to spec", + " Object @Nullable[] foo2 = null;", + " // 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;", + " // ok according to spec", + " Object @Nullable [][] foo5 = null;", + " // ok, but @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.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 according to spec", + " @Nullable Object @Nullable [] foo3 = null;", + " // @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( + "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 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() { + 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 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 makeLegacyModeHelper() { + return makeTestHelperWithArgs( + Arrays.asList( + "-XepOpt:NullAway:AnnotatedPackages=com.uber", + "-XepOpt:NullAway:LegacyAnnotationLocations=true")); + } +} 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); + } +} diff --git a/nullaway/src/test/java/com/uber/nullaway/TypeUseAnnotationsTests.java b/nullaway/src/test/java/com/uber/nullaway/TypeUseAnnotationsTests.java index 79d4ea5c0e..3a997403f5 100644 --- a/nullaway/src/test/java/com/uber/nullaway/TypeUseAnnotationsTests.java +++ b/nullaway/src/test/java/com/uber/nullaway/TypeUseAnnotationsTests.java @@ -183,29 +183,6 @@ public void annotationAppliedToInnerTypeOfTypeArgument() { .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 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..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; @@ -608,6 +630,48 @@ public void mismatchedIndexUse() { .doTest(); } + @Test + public void typeUseAndDeclarationAnnotationOnArray() { + makeHelper() + .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 Test {", + " @Nullable Object[] foo1 = 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(); ", + " }", + " 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", + " Object [] @Nullable [] foo6 = null;", + "}") + .doTest(); + } + private CompilationTestHelper makeHelper() { return makeTestHelperWithArgs( Arrays.asList(