Allow an Optional property to be set in a builder through a method wi…

…th a @nullable parameter.

This is based on google/auto#353 by Kenzie Togami.

Closes google/auto#353.

RELNOTES:
  Allow an Optional property to be set in a builder through a method with a @nullable parameter.
  NOTE: As a side-effect of this change, a setter for a @nullable property must have its parameter also marked @nullable. Previously, if the @nullable was not a TYPE_USE @nullable, it was copied from the getter to the parameter of the generated setter implementation. For TYPE_USE @nullable it was already necessary to mark the setter parameter @nullable.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=195471227

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

@@ -16,6 +16,7 @@
 package com.google.auto.value;
 
 import static com.google.common.truth.Truth.assertThat;
+import static com.google.common.truth.Truth8.assertThat;
 import static com.google.testing.compile.CompilationSubject.assertThat;
 import static org.junit.Assert.fail;
 import static org.junit.Assume.assumeTrue;
@@ -37,6 +38,7 @@
 import java.lang.reflect.TypeVariable;
 import java.util.Arrays;
 import java.util.List;
+import java.util.Optional;
 import java.util.Set;
 import javax.annotation.processing.AbstractProcessor;
 import javax.annotation.processing.RoundEnvironment;
@@ -368,6 +370,54 @@ public void testOmitNullableWithBuilder() {
     }
   }
 
+  @AutoValue
+  public abstract static class OptionalPropertyWithNullableBuilder {
+    public abstract String notOptional();
+
+    public abstract Optional<String> optional();
+
+    public static Builder builder() {
+      return new AutoValue_AutoValueJava8Test_OptionalPropertyWithNullableBuilder.Builder();
+    }
+
+    @AutoValue.Builder
+    public interface Builder {
+      Builder notOptional(String s);
+
+      Builder optional(@Nullable String s);
+
+      OptionalPropertyWithNullableBuilder build();
+    }
+  }
+
+  @Test
+  public void testOmitOptionalWithNullableBuilder() {
+    OptionalPropertyWithNullableBuilder instance1 =
+        OptionalPropertyWithNullableBuilder.builder().notOptional("hello").build();
+    assertThat(instance1.notOptional()).isEqualTo("hello");
+    assertThat(instance1.optional()).isEmpty();
+
+    OptionalPropertyWithNullableBuilder instance2 =
+        OptionalPropertyWithNullableBuilder.builder().notOptional("hello").optional(null).build();
+    assertThat(instance2.notOptional()).isEqualTo("hello");
+    assertThat(instance2.optional()).isEmpty();
+    assertThat(instance1).isEqualTo(instance2);
+
+    OptionalPropertyWithNullableBuilder instance3 =
+        OptionalPropertyWithNullableBuilder.builder()
+            .notOptional("hello")
+            .optional("world")
+            .build();
+    assertThat(instance3.notOptional()).isEqualTo("hello");
+    assertThat(instance3.optional()).hasValue("world");
+
+    try {
+      OptionalPropertyWithNullableBuilder.builder().build();
+      fail("Expected IllegalStateException for unset non-Optional property");
+    } catch (IllegalStateException expected) {
+    }
+  }
+
   @AutoValue
   public abstract static class BuilderWithUnprefixedGetters<T extends Comparable<T>> {
     public abstract ImmutableList<T> list();
@@ -497,20 +547,23 @@ abstract static class FunkyNullable {
 
     abstract @Nullable String foo();
 
+    abstract Optional<String> bar();
+
     static Builder builder() {
       return new AutoValue_AutoValueJava8Test_FunkyNullable.Builder();
     }
 
     @AutoValue.Builder
     interface Builder {
       Builder setFoo(@Nullable String foo);
+      Builder setBar(@Nullable String bar);
       FunkyNullable build();
     }
   }
 
   @Test
   public void testFunkyNullable() {
-    FunkyNullable explicitNull = FunkyNullable.builder().setFoo(null).build();
+    FunkyNullable explicitNull = FunkyNullable.builder().setFoo(null).setBar(null).build();
     FunkyNullable implicitNull = FunkyNullable.builder().build();
     assertThat(explicitNull).isEqualTo(implicitNull);
   }

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

@@ -1511,6 +1511,54 @@ public void testOmitOptionalWithBuilder() {
     assertThat(suppliedDirectly.optionalInteger()).hasValue(23);
   }
 
+  @AutoValue
+  public abstract static class OptionalPropertyWithNullableBuilder {
+    public abstract String notOptional();
+
+    public abstract com.google.common.base.Optional<String> optional();
+
+    public static Builder builder() {
+      return new AutoValue_AutoValueTest_OptionalPropertyWithNullableBuilder.Builder();
+    }
+
+    @AutoValue.Builder
+    public interface Builder {
+      Builder notOptional(String s);
+
+      Builder optional(@Nullable String s);
+
+      OptionalPropertyWithNullableBuilder build();
+    }
+  }
+
+  @Test
+  public void testOmitOptionalWithNullableBuilder() {
+    OptionalPropertyWithNullableBuilder instance1 =
+        OptionalPropertyWithNullableBuilder.builder().notOptional("hello").build();
+    assertThat(instance1.notOptional()).isEqualTo("hello");
+    assertThat(instance1.optional()).isAbsent();
+
+    OptionalPropertyWithNullableBuilder instance2 =
+        OptionalPropertyWithNullableBuilder.builder().notOptional("hello").optional(null).build();
+    assertThat(instance2.notOptional()).isEqualTo("hello");
+    assertThat(instance2.optional()).isAbsent();
+    assertThat(instance1).isEqualTo(instance2);
+
+    OptionalPropertyWithNullableBuilder instance3 =
+        OptionalPropertyWithNullableBuilder.builder()
+            .notOptional("hello")
+            .optional("world")
+            .build();
+    assertThat(instance3.notOptional()).isEqualTo("hello");
+    assertThat(instance3.optional()).hasValue("world");
+
+    try {
+      OptionalPropertyWithNullableBuilder.builder().build();
+      fail("Expected IllegalStateException for unset non-Optional property");
+    } catch (IllegalStateException expected) {
+    }
+  }
+
   @AutoValue
   public abstract static class NullableOptionalPropertiesWithBuilder {
     @Nullable

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

@@ -25,7 +25,6 @@
 import com.google.auto.common.MoreTypes;
 import com.google.auto.service.AutoService;
 import com.google.common.collect.ImmutableBiMap;
-import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableListMultimap;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
@@ -246,8 +245,7 @@ private void defineVarsForType(
   }
 
   @Override
-  Optional<String> nullableAnnotationForMethod(
-      ExecutableElement propertyMethod, ImmutableList<AnnotationMirror> methodAnnotations) {
+  Optional<String> nullableAnnotationForMethod(ExecutableElement propertyMethod) {
     return Optional.empty();
   }
 

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

@@ -222,7 +222,7 @@ public Optionalish getOptional() {
      * but empty.
      */
     public final String getNullableAnnotation() {
-      return nullableAnnotation.map(s -> s + " ").orElse("");
+      return nullableAnnotation.orElse("");
     }
 
     public boolean isNullable() {
@@ -318,12 +318,8 @@ public final boolean process(Set<? extends TypeElement> annotations, RoundEnviro
    * {@code @Nullable}, this method returns {@code Optional.of("")}, to indicate that the property
    * is nullable but the <i>method</i> isn't. The {@code @Nullable} annotation will instead appear
    * when the return type of the method is spelled out in the implementation.
-   *
-   * @param methodAnnotations the annotations to include on {@code propertyMethod}. This is
-   *     governed by the {@code AutoValue.CopyAnnotations} logic.
    */
-  abstract Optional<String> nullableAnnotationForMethod(
-      ExecutableElement propertyMethod, ImmutableList<AnnotationMirror> methodAnnotations);
+  abstract Optional<String> nullableAnnotationForMethod(ExecutableElement propertyMethod);
 
   /**
    * Returns the ordered set of {@link Property} definitions for the given {@code @AutoValue} or
@@ -357,8 +353,7 @@ final ImmutableSet<Property> propertySet(
       ImmutableList<AnnotationMirror> annotationMirrors =
           annotatedPropertyMethods.get(propertyMethod);
       ImmutableList<String> annotations = annotationStrings(annotationMirrors);
-      Optional<String> nullableAnnotation =
-          nullableAnnotationForMethod(propertyMethod, annotationMirrors);
+      Optional<String> nullableAnnotation = nullableAnnotationForMethod(propertyMethod);
       Property p = new Property(
           propertyName, identifier, propertyMethod, propertyType, annotations, nullableAnnotation);
       props.add(p);
@@ -394,8 +389,7 @@ final void defineSharedVarsForType(
   }
 
   /** Returns the spelling to be used in the generated code for the given list of annotations. */
-  static ImmutableList<String> annotationStrings(
-      ImmutableList<AnnotationMirror> annotations) {
+  static ImmutableList<String> annotationStrings(List<? extends AnnotationMirror> annotations) {
     // TODO(b/68008628): use ImmutableList.toImmutableList() when that works.
     return ImmutableList.copyOf(annotations
         .stream()

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

@@ -45,6 +45,7 @@
 import javax.annotation.processing.SupportedAnnotationTypes;
 import javax.annotation.processing.SupportedOptions;
 import javax.lang.model.element.AnnotationMirror;
+import javax.lang.model.element.Element;
 import javax.lang.model.element.ElementKind;
 import javax.lang.model.element.ExecutableElement;
 import javax.lang.model.element.TypeElement;
@@ -369,19 +370,35 @@ private void defineVarsForType(
   }
 
   @Override
-  Optional<String> nullableAnnotationForMethod(
-      ExecutableElement propertyMethod, ImmutableList<AnnotationMirror> methodAnnotations) {
-    OptionalInt nullableAnnotationIndex = nullableAnnotationIndex(methodAnnotations);
+  Optional<String> nullableAnnotationForMethod(ExecutableElement propertyMethod) {
+    return nullableAnnotationFor(propertyMethod, propertyMethod.getReturnType());
+  }
+
+  /**
+   * Returns an appropriate annotation spelling to indicate the nullability of an element. If the
+   * return value is a non-empty Optional, that indicates that the element is nullable, and the
+   * string should be used to annotate it. If the return value is an empty Optional, the element
+   * is not nullable. The return value can be {@code Optional.of("")}, which indicates that the
+   * element is nullable but that the nullability comes from a type annotation. In this case, the
+   * annotation will appear when the type is written, and must not be specified again. If the
+   * Optional contains a present non-empty string then that string will end with a space.
+   *
+   * @param element the element that might be {@code @Nullable}, either a method or a parameter.
+   * @param elementType the relevant type of the element: the return type for a method, or the
+   *     parameter type for a parameter.
+   */
+  static Optional<String> nullableAnnotationFor(Element element, TypeMirror elementType) {
+    List<? extends AnnotationMirror> typeAnnotations = elementType.getAnnotationMirrors();
+    if (nullableAnnotationIndex(typeAnnotations).isPresent()) {
+      return Optional.of("");
+    }
+    List<? extends AnnotationMirror> elementAnnotations = element.getAnnotationMirrors();
+    OptionalInt nullableAnnotationIndex = nullableAnnotationIndex(elementAnnotations);
     if (nullableAnnotationIndex.isPresent()) {
-      ImmutableList<String> annotations = annotationStrings(methodAnnotations);
-      return Optional.of(annotations.get(nullableAnnotationIndex.getAsInt()));
+      ImmutableList<String> annotations = annotationStrings(elementAnnotations);
+      return Optional.of(annotations.get(nullableAnnotationIndex.getAsInt()) + " ");
     } else {
-      List<? extends AnnotationMirror> typeAnnotations =
-          propertyMethod.getReturnType().getAnnotationMirrors();
-      return
-          nullableAnnotationIndex(typeAnnotations).isPresent()
-              ? Optional.of("")
-              : Optional.empty();
+      return Optional.empty();
     }
   }
 

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

@@ -40,6 +40,7 @@
 import javax.lang.model.element.Modifier;
 import javax.lang.model.element.TypeElement;
 import javax.lang.model.element.TypeParameterElement;
+import javax.lang.model.element.VariableElement;
 import javax.lang.model.type.DeclaredType;
 import javax.lang.model.type.TypeKind;
 import javax.lang.model.type.TypeMirror;
@@ -207,7 +208,8 @@ void defineVars(
         String property = entry.getKey();
         ExecutableElement setter = entry.getValue();
         TypeMirror propertyType = getterToPropertyName.inverse().get(property).getReturnType();
-        setterBuilder.put(property, new PropertySetter(setter, propertyType));
+        setterBuilder.put(
+            property, new PropertySetter(setter, propertyType, processingEnv.getTypeUtils()));
       }
       vars.builderSetters = setterBuilder.build();
 
@@ -282,32 +284,29 @@ public Optionalish getOptional() {
    * method {@code setFoo(Collection<String> foos)}. And, if {@code T} is {@code Optional},
    * it can have a setter with a type that can be copied to {@code T} through {@code Optional.of}.
    */
-  public class PropertySetter {
+  public static class PropertySetter {
     private final String access;
     private final String name;
     private final String parameterTypeString;
     private final boolean primitiveParameter;
+    private final String nullableAnnotation;
     private final String copyOf;
 
-    public PropertySetter(ExecutableElement setter, TypeMirror propertyType) {
+    PropertySetter(ExecutableElement setter, TypeMirror propertyType, Types typeUtils) {
       this.access = SimpleMethod.access(setter);
       this.name = setter.getSimpleName().toString();
-      TypeMirror parameterType = Iterables.getOnlyElement(setter.getParameters()).asType();
+      VariableElement parameterElement = Iterables.getOnlyElement(setter.getParameters());
+      TypeMirror parameterType = parameterElement.asType();
       primitiveParameter = parameterType.getKind().isPrimitive();
       this.parameterTypeString = parameterTypeString(setter, parameterType);
-      Types typeUtils = processingEnv.getTypeUtils();
-      TypeMirror erasedPropertyType = typeUtils.erasure(propertyType);
-      boolean sameType = typeUtils.isSameType(typeUtils.erasure(parameterType), erasedPropertyType);
-      if (sameType) {
-        this.copyOf = null;
-      } else {
-        String rawTarget = TypeEncoder.encodeRaw(erasedPropertyType);
-        String of = Optionalish.isOptional(propertyType) ? "of" : "copyOf";
-        this.copyOf = rawTarget + "." + of + "(%s)";
-      }
+      Optional<String> maybeNullable =
+          AutoValueProcessor.nullableAnnotationFor(parameterElement, parameterType);
+      this.nullableAnnotation = maybeNullable.orElse("");
+      boolean nullable = maybeNullable.isPresent();
+      this.copyOf = copyOfString(propertyType, parameterType, typeUtils, nullable);
     }
 
-    private String parameterTypeString(ExecutableElement setter, TypeMirror parameterType) {
+    private static String parameterTypeString(ExecutableElement setter, TypeMirror parameterType) {
       if (setter.isVarArgs()) {
         TypeMirror componentType = MoreTypes.asArray(parameterType).getComponentType();
         // This is a bit ugly. It's OK to annotate just the component type, because if it is
@@ -321,6 +320,26 @@ private String parameterTypeString(ExecutableElement setter, TypeMirror paramete
       }
     }
 
+    private static String copyOfString(
+        TypeMirror propertyType, TypeMirror parameterType, Types typeUtils, boolean nullable) {
+      TypeMirror erasedPropertyType = typeUtils.erasure(propertyType);
+      boolean sameType = typeUtils.isSameType(typeUtils.erasure(parameterType), erasedPropertyType);
+      if (sameType) {
+        return null;
+      }
+      String rawTarget = TypeEncoder.encodeRaw(erasedPropertyType);
+      String of;
+      Optionalish optionalish = Optionalish.createIfOptional(propertyType);
+      if (optionalish == null) {
+        of = "copyOf";
+      } else if (nullable) {
+        of = optionalish.ofNullable();
+      } else {
+        of = "of";
+      }
+      return rawTarget + "." + of + "(%s)";
+    }
+
     public String getAccess() {
       return access;
     }
@@ -337,6 +356,10 @@ public boolean getPrimitiveParameter() {
       return primitiveParameter;
     }
 
+    public String getNullableAnnotation() {
+      return nullableAnnotation;
+    }
+
     public String copy(AutoValueProcessor.Property property) {
       if (copyOf == null) {
         return property.toString();