Allow a property to be null if its type is a type variable with a `@N…

…ullable` bound. For example `<T extends @nullable Object>`. Fixes #1320. RELNOTES=An AutoValue or AutoBuilder property is now allowed to be null if its type is a type variable with a `@Nullable` bound, like `<T extends @nullable Object>`. PiperOrigin-RevId: 516301365
google · Mar 13, 2023 · 1b58cff · 1b58cff
1 parent 3c7541f
commit 1b58cff
Show file tree

Hide file tree

Showing 4 changed files with 154 additions and 9 deletions.
diff --git a/value/src/it/functional/src/test/java/com/google/auto/value/AutoBuilderTest.java b/value/src/it/functional/src/test/java/com/google/auto/value/AutoBuilderTest.java
@@ -18,7 +18,10 @@
 import static com.google.common.base.Preconditions.checkNotNull;
 import static com.google.common.truth.Truth.assertThat;
 import static com.google.common.truth.Truth8.assertThat;
+import static java.lang.annotation.ElementType.TYPE_USE;
+import static org.junit.Assert.assertThrows;
 import static org.junit.Assert.fail;
+import static org.junit.Assume.assumeTrue;
 
 import com.google.common.base.MoreObjects;
 import com.google.common.collect.ImmutableList;
@@ -27,6 +30,7 @@
 import java.io.IOException;
 import java.lang.annotation.Retention;
 import java.lang.annotation.RetentionPolicy;
+import java.lang.annotation.Target;
 import java.math.BigInteger;
 import java.time.LocalTime;
 import java.util.AbstractSet;
@@ -37,6 +41,7 @@
 import java.util.Objects;
 import java.util.Optional;
 import java.util.Set;
+import javax.lang.model.SourceVersion;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.junit.runners.JUnit4;
@@ -751,4 +756,35 @@ public void builderAnnotationsCopiedIfRequested() {
         .asList()
         .contains(myAnnotationBuilder().value("thing").build());
   }
+
+  @Target(TYPE_USE)
+  public @interface Nullable {}
+
+  public static <T extends @Nullable Object, U> T frob(T arg, U notNull) {
+    return arg;
+  }
+
+  @AutoBuilder(callMethod = "frob")
+  interface FrobCaller<T extends @Nullable Object, U> {
+    FrobCaller<T, U> arg(T arg);
+
+    FrobCaller<T, U> notNull(U notNull);
+
+    T call();
+
+    static <T extends @Nullable Object, U> FrobCaller<T, U> caller() {
+      return new AutoBuilder_AutoBuilderTest_FrobCaller<>();
+    }
+  }
+
+  @Test
+  public void builderTypeVariableWithNullableBound() {
+    // The Annotation Processing API doesn't see the @Nullable Object bound on Java 8.
+    assumeTrue(SourceVersion.latest().ordinal() > SourceVersion.RELEASE_8.ordinal());
+    assertThat(FrobCaller.<@Nullable String, String>caller().arg(null).notNull("foo").call())
+        .isNull();
+    assertThrows(
+        NullPointerException.class,
+        () -> FrobCaller.<@Nullable String, String>caller().arg(null).notNull(null).call());
+  }
 }
diff --git a/value/src/it/functional/src/test/java/com/google/auto/value/AutoValueJava8Test.java b/value/src/it/functional/src/test/java/com/google/auto/value/AutoValueJava8Test.java
@@ -30,6 +30,7 @@
 import com.google.testing.compile.Compilation;
 import com.google.testing.compile.Compiler;
 import com.google.testing.compile.JavaFileObjects;
+import java.io.Serializable;
 import java.lang.annotation.Annotation;
 import java.lang.annotation.ElementType;
 import java.lang.annotation.Retention;
@@ -39,8 +40,8 @@
 import java.lang.reflect.Constructor;
 import java.lang.reflect.Method;
 import java.lang.reflect.TypeVariable;
-import java.util.Arrays;
 import java.util.ArrayList;
+import java.util.Arrays;
 import java.util.List;
 import java.util.Optional;
 import java.util.OptionalDouble;
@@ -934,7 +935,7 @@ public void nestedOptionalGetter() {
   public abstract static class PropertyBuilderWildcard<T> {
     public abstract List<? extends T> list();
 
-    public static <T>PropertyBuilderWildcard.Builder<T> builder() {
+    public static <T> PropertyBuilderWildcard.Builder<T> builder() {
       return new AutoValue_AutoValueJava8Test_PropertyBuilderWildcard.Builder<>();
     }
 
@@ -964,4 +965,82 @@ public void propertyBuilderWildcard() {
     builder.listBuilder().add("foo");
     assertThat(builder.build().list()).containsExactly("foo");
   }
+
+  @AutoValue
+  public abstract static class NullableBound<T extends @Nullable Object> {
+    public abstract T maybeNullable();
+
+    public static <T extends @Nullable Object> NullableBound<T> create(T maybeNullable) {
+      return new AutoValue_AutoValueJava8Test_NullableBound<>(maybeNullable);
+    }
+  }
+
+  @Test
+  public void propertyCanBeNullIfNullableBound() {
+    assumeTrue(javacHandlesTypeAnnotationsCorrectly);
+    // The generated class doesn't know what the actual type argument is, so it can't know whether
+    // it is @Nullable. Because of the @Nullable bound, it omits an explicit null check, under the
+    // assumption that some static-checking framework is validating type uses.
+    NullableBound<@Nullable String> x = NullableBound.create(null);
+    assertThat(x.maybeNullable()).isNull();
+  }
+
+  @AutoValue
+  public abstract static class NullableIntersectionBound<
+      T extends @Nullable Object & @Nullable Serializable> {
+    public abstract T maybeNullable();
+
+    public static <T extends @Nullable Object & @Nullable Serializable>
+        NullableIntersectionBound<T> create(T maybeNullable) {
+      return new AutoValue_AutoValueJava8Test_NullableIntersectionBound<>(maybeNullable);
+    }
+  }
+
+  @Test
+  public void propertyCanBeNullIfNullableIntersectionBound() {
+    assumeTrue(javacHandlesTypeAnnotationsCorrectly);
+    // The generated class doesn't know what the actual type argument is, so it can't know whether
+    // it is @Nullable. Because of the @Nullable bound, it omits an explicit null check, under the
+    // assumption that some static-checking framework is validating type uses.
+    NullableIntersectionBound<@Nullable String> x = NullableIntersectionBound.create(null);
+    assertThat(x.maybeNullable()).isNull();
+  }
+
+  @AutoValue
+  public abstract static class PartlyNullableIntersectionBound<
+      T extends @Nullable Object & Serializable> {
+    public abstract T notNullable();
+
+    public static <T extends @Nullable Object & Serializable>
+        PartlyNullableIntersectionBound<T> create(T notNullable) {
+      return new AutoValue_AutoValueJava8Test_PartlyNullableIntersectionBound<>(notNullable);
+    }
+  }
+
+  @Test
+  public void propertyCannotBeNullWithPartlyNullableIntersectionBound() {
+    assumeTrue(javacHandlesTypeAnnotationsCorrectly);
+    assertThrows(NullPointerException.class, () -> PartlyNullableIntersectionBound.create(null));
+  }
+
+  @AutoValue
+  public abstract static class NullableVariableBound<T extends @Nullable Object, U extends T> {
+    public abstract T nullOne();
+
+    public abstract U nullTwo();
+
+    public static <T extends @Nullable Object, U extends T> NullableVariableBound<T, U> create(
+        T nullOne, U nullTwo) {
+      return new AutoValue_AutoValueJava8Test_NullableVariableBound<>(nullOne, nullTwo);
+    }
+  }
+
+  @Test
+  public void nullableVariableBound() {
+    assumeTrue(javacHandlesTypeAnnotationsCorrectly);
+    NullableVariableBound<@Nullable CharSequence, @Nullable String> x =
+        NullableVariableBound.create(null, null);
+    assertThat(x.nullOne()).isNull();
+    assertThat(x.nullTwo()).isNull();
+  }
 }
diff --git a/value/src/it/functional/src/test/java/com/google/auto/value/CompileWithEclipseTest.java b/value/src/it/functional/src/test/java/com/google/auto/value/CompileWithEclipseTest.java
@@ -80,11 +80,13 @@ public static void setSourceRoot() {
   private static final Predicate<File> JAVA_FILE =
       f -> f.getName().endsWith(".java") && !IGNORED_TEST_FILES.contains(f.getName());
 
-  private static final Predicate<File> JAVA8_TEST =
-      f ->
-          f.getName().equals("AutoValueJava8Test.java")
-              || f.getName().equals("AutoOneOfJava8Test.java")
-              || f.getName().equals("EmptyExtension.java");
+  private static final ImmutableSet<String> JAVA8_TEST_FILES =
+      ImmutableSet.of(
+          "AutoBuilderTest.java",
+          "AutoOneOfJava8Test.java",
+          "AutoValueJava8Test.java",
+          "EmptyExtension.java");
+  private static final Predicate<File> JAVA8_TEST = f -> JAVA8_TEST_FILES.contains(f.getName());
 
   @Test
   public void compileWithEclipseJava7() throws Exception {

diff --git a/value/src/main/java/com/google/auto/value/processor/AutoValueishProcessor.java b/value/src/main/java/com/google/auto/value/processor/AutoValueishProcessor.java
@@ -78,6 +78,7 @@
 import javax.lang.model.type.DeclaredType;
 import javax.lang.model.type.TypeKind;
 import javax.lang.model.type.TypeMirror;
+import javax.lang.model.type.TypeVariable;
 import javax.lang.model.util.ElementFilter;
 import javax.lang.model.util.Elements;
 import javax.lang.model.util.SimpleAnnotationValueVisitor8;
@@ -715,8 +716,7 @@ private static boolean gettersAllPrefixed(Set<ExecutableElement> methods) {
    *     parameter type for a parameter.
    */
   static Optional<String> nullableAnnotationFor(Element element, TypeMirror elementType) {
-    List<? extends AnnotationMirror> typeAnnotations = elementType.getAnnotationMirrors();
-    if (nullableAnnotationIndex(typeAnnotations).isPresent()) {
+    if (isNullable(elementType)) {
       return Optional.of("");
     }
     List<? extends AnnotationMirror> elementAnnotations = element.getAnnotationMirrors();
@@ -736,6 +736,34 @@ private static OptionalInt nullableAnnotationIndex(List<? extends AnnotationMirr
         .findFirst();
   }
 
+  private static boolean isNullable(TypeMirror type) {
+    return isNullable(type, 0);
+  }
+
+  private static boolean isNullable(TypeMirror type, int depth) {
+    // Some versions of the Eclipse compiler can report that the upper bound of a type variable T
+    // is another T, and if you ask for the upper bound of that other T you'll get a third T, and so
+    // ad infinitum. To avoid StackOverflowError, we bottom out after 10 iterations.
+    if (depth > 10) {
+      return false;
+    }
+    List<? extends AnnotationMirror> typeAnnotations = type.getAnnotationMirrors();
+    // TODO(emcmanus): also check if there is a @NonNull bound and return false if so.
+    if (nullableAnnotationIndex(typeAnnotations).isPresent()) {
+      return true;
+    }
+    if (type.getKind().equals(TypeKind.TYPEVAR)) {
+      TypeVariable typeVariable = MoreTypes.asTypeVariable(type);
+      TypeMirror bound = typeVariable.getUpperBound();
+      if (bound.getKind().equals(TypeKind.INTERSECTION)) {
+        return MoreTypes.asIntersection(bound).getBounds().stream()
+            .allMatch(t -> isNullable(t, depth + 1));
+      }
+      return isNullable(bound, depth + 1);
+    }
+    return false;
+  }
+
   private static boolean isNullable(AnnotationMirror annotation) {
     return annotation.getAnnotationType().asElement().getSimpleName().contentEquals("Nullable");
   }