Enforce Strict Interpretation Of Type Use Annotation Locations Outside of JSpecify mode

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<String> getExtraErrorProneArgs() {
+    return List.of("-XepOpt:NullAway:LegacyAnnotationLocations=true");
+  }
 }

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();
 }

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);
+  }
 }

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<String> 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<MethodClassAndName> knownInitializers;
   private final ImmutableSet<String> excludedClassAnnotations;
   private final ImmutableSet<String> 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<String>()
@@ -584,6 +597,11 @@ public boolean isJSpecifyMode() {
     return jspecifyMode;
   }
 
+  @Override
+  public boolean isLegacyAnnotationLocation() {
+    return legacyAnnotationLocation;
+  }
+
   @AutoValue
   abstract static class MethodClassAndName {
 

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;

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"));
+  }
+}

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);
+  }
+}

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