Refine the nullability logic for type variables.

value/src/it/functional/src/test/java/com/google/auto/value/AutoValueJava8Test.java

@@ -1139,19 +1139,50 @@ public abstract static class NullableVariableBound<T extends @Nullable Object, U
 
     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);
+    public static <T extends @Nullable Object, U extends T>
+        NullableVariableBound.Builder<T, U> builder() {
+      return new AutoValue_AutoValueJava8Test_NullableVariableBound.Builder<>();
+    }
+
+    public abstract NullableVariableBound.Builder<T, U> toBuilder();
+
+    @AutoValue.Builder
+    public abstract static class Builder<T extends @Nullable Object, U extends T> {
+      public abstract Builder<T, U> setNullOne(T nullOne);
+
+      public abstract Builder<T, U> setNullTwo(U nullTwo);
+
+      public abstract NullableVariableBound<T, U> build();
     }
   }
 
   @Test
   public void nullableVariableBound() {
     assumeTrue(javacHandlesTypeAnnotationsCorrectly);
     NullableVariableBound<@Nullable CharSequence, @Nullable String> x =
-        NullableVariableBound.create(null, null);
+        NullableVariableBound.<@Nullable CharSequence, @Nullable String>builder()
+            .setNullOne(null)
+            .setNullTwo(null)
+            .build();
     assertThat(x.nullOne()).isNull();
     assertThat(x.nullTwo()).isNull();
+    NullableVariableBound<@Nullable CharSequence, @Nullable String> x2 = x.toBuilder().build();
+    assertThat(x2).isNotSameInstanceAs(x);
+    assertThat(x2).isEqualTo(x);
+    assertThat(x2.hashCode()).isEqualTo(x.hashCode());
+  }
+
+  @Test
+  public void nullableVariableBoundBuilderFieldsAreNullable() throws ReflectiveOperationException {
+    assumeTrue(javacHandlesTypeAnnotationsCorrectly);
+    NullableVariableBound.Builder<@Nullable CharSequence, @Nullable String> builder =
+        NullableVariableBound.builder();
+    assertThat(builder.getClass().getDeclaredField("nullOne").getAnnotatedType().getAnnotations())
+        .asList()
+        .containsAnyIn(nullables(AutoValue_AutoValueJava8Test_NullableVariableBound.class));
+    assertThat(builder.getClass().getDeclaredField("nullTwo").getAnnotatedType().getAnnotations())
+        .asList()
+        .containsAnyIn(nullables(AutoValue_AutoValueJava8Test_NullableVariableBound.class));
   }
 
   @AutoValue

value/src/main/java/com/google/auto/value/processor/AutoValueishProcessor.java

@@ -310,6 +310,10 @@ public boolean isNullable() {
       return nullableAnnotation.isPresent();
     }
 
+    public boolean isTypeVarWithNullableBound() {
+      return isTypeVariableWithNullableBound(annotatedType.getType());
+    }
+
     /**
      * Returns the name of the getter method for this property as defined by the {@code @AutoValue}
      * or {@code @AutoBuilder} class. For property {@code foo}, this will be {@code foo} or {@code
@@ -740,40 +744,41 @@ static Optional<String> nullableAnnotationFor(Element element, TypeMirror elemen
 
   private static OptionalInt nullableAnnotationIndex(List<? extends AnnotationMirror> annotations) {
     return IntStream.range(0, annotations.size())
-        .filter(i -> isNullable(annotations.get(i)))
+        .filter(i -> isNullableAnnotation(annotations.get(i)))
         .findFirst();
   }
 
+  private static boolean isNullableAnnotation(AnnotationMirror annotation) {
+    return annotation.getAnnotationType().asElement().getSimpleName().contentEquals("Nullable");
+  }
+
   private static boolean isNullable(TypeMirror type) {
-    return isNullable(type, 0);
+    return nullableAnnotationIndex(type.getAnnotationMirrors()).isPresent();
+  }
+
+  private static boolean isTypeVariableWithNullableBound(TypeMirror type) {
+    return isTypeVariableWithNullableBound(type, 0);
   }
 
-  private static boolean isNullable(TypeMirror type, int depth) {
+  private static boolean isTypeVariableWithNullableBound(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)) {
+      return false;
     }
-    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);
+    TypeVariable typeVariable = MoreTypes.asTypeVariable(type);
+    TypeMirror bound = typeVariable.getUpperBound();
+    if (bound.getKind().equals(TypeKind.INTERSECTION)) {
+      // An intersection bound cannot itself be a type variable. <T extends U> is allowed, but
+      // <T extends U & V> is not (JLS §4.4).
+      return MoreTypes.asIntersection(bound).getBounds().stream().allMatch(t -> isNullable(t));
     }
-    return false;
-  }
-
-  private static boolean isNullable(AnnotationMirror annotation) {
-    return annotation.getAnnotationType().asElement().getSimpleName().contentEquals("Nullable");
+    return isNullable(bound) || isTypeVariableWithNullableBound(bound, depth + 1);
   }
 
   /**
@@ -1246,7 +1251,7 @@ private ImmutableList<AnnotationMirror> propertyFieldAnnotations(
           .filter(
               a -> {
                 TypeElement annotationType = asType(a.getAnnotationType().asElement());
-                return isNullable(a)
+                return isNullableAnnotation(a)
                     && !returnTypeAnnotations.contains(annotationType.getQualifiedName().toString())
                     && annotationAppliesToFields(annotationType);
               })

value/src/main/java/com/google/auto/value/processor/BuilderSpec.java

@@ -357,7 +357,7 @@ void defineVars(AutoValueOrBuilderTemplateVars vars, BuilderMethodClassifier<?>
 
       ImmutableSet<Property> requiredProperties =
           vars.props.stream()
-              .filter(p -> !p.isNullable())
+              .filter(p -> !p.isNullable() && !p.isTypeVarWithNullableBound())
               .filter(p -> p.getBuilderInitializer().isEmpty())
               .filter(p -> !p.hasDefault())
               .filter(p -> !vars.builderPropertyBuilders.containsKey(p.getName()))

value/src/main/java/com/google/auto/value/processor/autovalue.vm

@@ -69,7 +69,8 @@ ${modifiers}class $subclass$formalTypes extends $origClass$actualTypes {
       ${p.nullableAnnotation}$p.type $p #if ($foreach.hasNext) , #end
 #end ) {
 #foreach ($p in $props)
-  #if (!$p.kind.primitive && !$p.nullable && ($builderTypeName == "" || !$isFinal))
+  #if (!$p.kind.primitive && !$p.nullable && !$p.typeVarWithNullableBound
+      && ($builderTypeName == "" || !$isFinal))
     ## We don't need a null check if the type is primitive or @Nullable. We also don't need it
     ## if there is a builder, since the build() method will check for us. However, if there is a
     ## builder but there are also extensions (!$isFinal) then we can't omit the null check because

value/src/main/java/com/google/auto/value/processor/builder.vm

@@ -89,7 +89,8 @@ ${builderClassModifiers}class ${builderName}${builderFormalTypes} ##
 
     ## Omit null check for primitive, or @Nullable, or if we are going to be calling a copy method
     ## such as Optional.of, which will have its own null check if appropriate.
-    #if (!$setter.primitiveParameter && !$p.nullable && ${setter.copy($p)} == $p)
+    #if (!$setter.primitiveParameter && !$p.nullable
+        && !$p.typeVarWithNullableBound && ${setter.copy($p)} == $p)
 
       #if ($identifiers)
     if ($p == null) {

value/src/main/java/com/google/auto/value/processor/equalshashcode.vm

@@ -50,7 +50,7 @@
   #elseif ($p.kind == "ARRAY")
     `java.util.Arrays`.equals(this.$p, ##
         (that instanceof $subclass) ? (($subclass$wildcardTypes) that).$p : that.${p.getter}()) ##
-  #elseif ($p.nullable)
+  #elseif ($p.nullable || $p.typeVarWithNullableBound)
     (this.$p == null ? that.${p.getter}() == null : this.${p}.equals(that.${p.getter}())) ##
   #else
     this.${p}.equals(that.${p.getter}()) ##
@@ -74,7 +74,7 @@
     $p ##
   #elseif ($p.kind == "ARRAY")
     `java.util.Arrays`.hashCode($p) ##
-  #elseif ($p.nullable)
+  #elseif ($p.nullable || $p.typeVarWithNullableBound)
     ($p == null) ? 0 : ${p}.hashCode() ##
   #else
     ${p}.hashCode() ##