Fix handling of `@Nullable Optional<T> foo()` properties being set by `setFoo(@Nullable T)` setters.

Previously calling `setFoo(null)` resulted in the property being set to `null`, even though for `Optional<T> foo()` (without `@Nullable`) it would set the property to `Optional.empty()`. With this change, it is set to `Optional.empty()` whether or not it is `@Nullable`.

RELNOTES=Fixed handling of `@Nullable Optional<T> foo()` properties being set by `setFoo(@Nullable T)` setters. Now `setFoo(null)` results in `Optional.empty()`, not `null`.

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

3 files changed, 91 insertions, 51 deletions

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
index 27356c52..10812f8d 100644
--- 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
@@ -19,7 +19,7 @@ import static com.google.common.truth.Truth.assertThat;
 import static com.google.common.truth.Truth.assertWithMessage;
 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.Assert.assertThrows;
 import static org.junit.Assume.assumeTrue;
 
 import com.google.common.collect.ImmutableList;
@@ -375,12 +375,9 @@ public class AutoValueJava8Test {
 
   @Test
   public void testNotNullablePrimitiveArrays() {
-    try {
-      PrimitiveArrays.create(null, new int[0]);
-      fail("Construction with null value for non-@Nullable array should have failed");
-    } catch (NullPointerException e) {
-      assertThat(e.getMessage()).contains("booleans");
-    }
+    NullPointerException e =
+        assertThrows(NullPointerException.class, () -> PrimitiveArrays.create(null, new int[0]));
+    assertThat(e).hasMessageThat().contains("booleans");
   }
 
   @AutoValue
@@ -421,12 +418,10 @@ public class AutoValueJava8Test {
     assertThat(instance3.notNullable()).isEqualTo("hello");
     assertThat(instance3.nullable()).isEqualTo("world");
 
-    try {
-      NullablePropertyWithBuilder.builder().build();
-      fail("Expected IllegalStateException for unset non-@Nullable property");
-    } catch (IllegalStateException e) {
-      assertThat(e.getMessage()).contains("notNullable");
-    }
+    IllegalStateException e =
+        assertThrows(
+            IllegalStateException.class, () -> NullablePropertyWithBuilder.builder().build());
+    assertThat(e).hasMessageThat().contains("notNullable");
   }
 
   @AutoValue
@@ -470,13 +465,41 @@ public class AutoValueJava8Test {
     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) {
+    assertThrows(
+        IllegalStateException.class, () -> OptionalPropertyWithNullableBuilder.builder().build());
+  }
+
+  @AutoValue
+  public abstract static class NullableOptionalPropertyWithNullableBuilder {
+    public abstract @Nullable Optional<String> optional();
+
+    public static Builder builder() {
+      return new AutoValue_AutoValueJava8Test_NullableOptionalPropertyWithNullableBuilder.Builder();
+    }
+
+    @AutoValue.Builder
+    public interface Builder {
+      Builder optional(@Nullable String s);
+
+      NullableOptionalPropertyWithNullableBuilder build();
     }
   }
 
+  @Test
+  public void testNullableOptional() {
+    NullableOptionalPropertyWithNullableBuilder instance1 =
+        NullableOptionalPropertyWithNullableBuilder.builder().build();
+    assertThat(instance1.optional()).isNull();
+
+    NullableOptionalPropertyWithNullableBuilder instance2 =
+        NullableOptionalPropertyWithNullableBuilder.builder().optional(null).build();
+    assertThat(instance2.optional()).isEmpty();
+
+    NullableOptionalPropertyWithNullableBuilder instance3 =
+        NullableOptionalPropertyWithNullableBuilder.builder().optional("haruspex").build();
+    assertThat(instance3.optional()).hasValue("haruspex");
+  }
+
   @AutoValue
   @SuppressWarnings("AutoValueImmutableFields")
   public abstract static class BuilderWithUnprefixedGetters<T extends Comparable<T>> {
@@ -522,18 +545,10 @@ public class AutoValueJava8Test {
 
     BuilderWithUnprefixedGetters.Builder<String> builder = BuilderWithUnprefixedGetters.builder();
     assertThat(builder.t()).isNull();
-    try {
-      builder.list();
-      fail("Attempt to retrieve unset list property should have failed");
-    } catch (IllegalStateException e) {
-      assertThat(e).hasMessageThat().isEqualTo("Property \"list\" has not been set");
-    }
-    try {
-      builder.ints();
-      fail("Attempt to retrieve unset ints property should have failed");
-    } catch (IllegalStateException e) {
-      assertThat(e).hasMessageThat().isEqualTo("Property \"ints\" has not been set");
-    }
+    IllegalStateException e1 = assertThrows(IllegalStateException.class, () -> builder.list());
+    assertThat(e1).hasMessageThat().isEqualTo("Property \"list\" has not been set");
+    IllegalStateException e2 = assertThrows(IllegalStateException.class, () -> builder.ints());
+    assertThat(e2).hasMessageThat().isEqualTo("Property \"ints\" has not been set");
 
     builder.setList(names);
     assertThat(builder.list()).isSameInstanceAs(names);
@@ -596,12 +611,8 @@ public class AutoValueJava8Test {
 
     BuilderWithPrefixedGetters.Builder<String> builder = BuilderWithPrefixedGetters.builder();
     assertThat(builder.getInts()).isNull();
-    try {
-      builder.getList();
-      fail("Attempt to retrieve unset list property should have failed");
-    } catch (IllegalStateException e) {
-      assertThat(e).hasMessageThat().isEqualTo("Property \"list\" has not been set");
-    }
+    IllegalStateException e = assertThrows(IllegalStateException.class, () -> builder.getList());
+    assertThat(e).hasMessageThat().isEqualTo("Property \"list\" has not been set");
 
     builder.setList(names);
     assertThat(builder.getList()).isSameInstanceAs(names);
diff --git a/value/src/main/java/com/google/auto/value/processor/BuilderMethodClassifier.java b/value/src/main/java/com/google/auto/value/processor/BuilderMethodClassifier.java
index 724a3225..3e510a4a 100644
--- a/value/src/main/java/com/google/auto/value/processor/BuilderMethodClassifier.java
+++ b/value/src/main/java/com/google/auto/value/processor/BuilderMethodClassifier.java
@@ -20,6 +20,7 @@ import static com.google.common.collect.Sets.difference;
 
 import com.google.auto.common.MoreElements;
 import com.google.auto.common.MoreTypes;
+import com.google.auto.value.processor.BuilderSpec.Copier;
 import com.google.auto.value.processor.BuilderSpec.PropertySetter;
 import com.google.auto.value.processor.PropertyBuilderClassifier.PropertyBuilder;
 import com.google.common.base.Equivalence;
@@ -376,7 +377,7 @@ class BuilderMethodClassifier {
       checkForFailedJavaBean(method);
       return;
     }
-    Optional<Function<String, String>> function = getSetterFunction(valueGetter, method);
+    Optional<Copier> function = getSetterFunction(valueGetter, method);
     if (function.isPresent()) {
       DeclaredType builderTypeMirror = MoreTypes.asDeclared(builderType.asType());
       ExecutableType methodMirror =
@@ -419,7 +420,7 @@ class BuilderMethodClassifier {
    * using a method like {@code ImmutableList.copyOf} or {@code Optional.of}, when the returned
    * function will be something like {@code s -> "Optional.of(" + s + ")"}.
    */
-  private Optional<Function<String, String>> getSetterFunction(
+  private Optional<Copier> getSetterFunction(
       ExecutableElement valueGetter, ExecutableElement setter) {
     VariableElement parameterElement = Iterables.getOnlyElement(setter.getParameters());
     boolean nullableParameter =
@@ -445,7 +446,7 @@ class BuilderMethodClassifier {
           return Optional.empty();
         }
       }
-      return Optional.of(s -> s);
+      return Optional.of(Copier.IDENTITY);
     }
 
     // Parameter type is not equal to property type, but might be convertible with copyOf.
@@ -465,14 +466,14 @@ class BuilderMethodClassifier {
    * to the getter's return type using one of the given methods, or {@code Optional.empty()} if the
    * conversion isn't possible. An error will have been reported in the latter case.
    */
-  private Optional<Function<String, String>> getConvertingSetterFunction(
+  private Optional<Copier> getConvertingSetterFunction(
       ImmutableList<ExecutableElement> copyOfMethods,
       ExecutableElement valueGetter,
       ExecutableElement setter,
       TypeMirror parameterType) {
     DeclaredType targetType = MoreTypes.asDeclared(getterToPropertyType.get(valueGetter));
     for (ExecutableElement copyOfMethod : copyOfMethods) {
-      Optional<Function<String, String>> function =
+      Optional<Copier> function =
           getConvertingSetterFunction(copyOfMethod, targetType, parameterType);
       if (function.isPresent()) {
         return function;
@@ -516,7 +517,7 @@ class BuilderMethodClassifier {
    * @return a function that maps a string parameter to a method call using that parameter. For
    *     example it might map {@code foo} to {@code ImmutableList.copyOf(foo)}.
    */
-  private Optional<Function<String, String>> getConvertingSetterFunction(
+  private Optional<Copier> getConvertingSetterFunction(
       ExecutableElement copyOfMethod, DeclaredType targetType, TypeMirror parameterType) {
     // We have a parameter type, for example Set<? extends T>, and we want to know if it can be
     // passed to the given copyOf method, which might for example be one of these methods from
@@ -532,7 +533,15 @@ class BuilderMethodClassifier {
     if (TypeVariables.canAssignStaticMethodResult(
         copyOfMethod, parameterType, targetType, typeUtils)) {
       String method = TypeEncoder.encodeRaw(targetType) + "." + copyOfMethod.getSimpleName();
-      return Optional.of(s -> method + "(" + s + ")");
+      Function<String, String> callMethod = s -> method + "(" + s + ")";
+      // This is a big old hack. We guess that the method can accept a null parameter if it has
+      // "Nullable" in the name, which java.util.Optional.ofNullable and
+      // com.google.common.base.Optional.fromNullable do.
+      Copier copier =
+          method.contains("Nullable")
+              ? Copier.acceptingNull(callMethod)
+              : Copier.notAcceptingNull(callMethod);
+      return Optional.of(copier);
     }
     return Optional.empty();
   }
diff --git a/value/src/main/java/com/google/auto/value/processor/BuilderSpec.java b/value/src/main/java/com/google/auto/value/processor/BuilderSpec.java
index 613d5457..2c8c17b1 100644
--- a/value/src/main/java/com/google/auto/value/processor/BuilderSpec.java
+++ b/value/src/main/java/com/google/auto/value/processor/BuilderSpec.java
@@ -354,6 +354,30 @@ class BuilderSpec {
   }
 
   /**
+   * Specifies how to copy a parameter value into the target type. This might be the identity, or
+   * it might be something like {@code ImmutableList.of(...)} or {@code Optional.ofNullable(...)}.
+   */
+  static class Copier {
+    static final Copier IDENTITY = acceptingNull(x -> x);
+
+    private final Function<String, String> copy;
+    private final boolean acceptsNull;
+
+    private Copier(Function<String, String> copy, boolean acceptsNull) {
+      this.copy = copy;
+      this.acceptsNull = acceptsNull;
+    }
+
+    static Copier acceptingNull(Function<String, String> copy) {
+      return new Copier(copy, true);
+    }
+
+    static Copier notAcceptingNull(Function<String, String> copy) {
+      return new Copier(copy, false);
+    }
+  }
+
+  /**
    * Information about a property setter, referenced from the autovalue.vm template. A property
    * called foo (defined by a method {@code T foo()} or {@code T getFoo()}) can have a setter method
    * {@code foo(T)} or {@code setFoo(T)} that returns the builder type. Additionally, it can have a
@@ -369,12 +393,11 @@ class BuilderSpec {
     private final String parameterTypeString;
     private final boolean primitiveParameter;
     private final String nullableAnnotation;
-    private final Function<String, String> copyFunction;
+    private final Copier copier;
 
-    PropertySetter(
-        ExecutableElement setter, TypeMirror parameterType, Function<String, String> copyFunction) {
+    PropertySetter(ExecutableElement setter, TypeMirror parameterType, Copier copier) {
       this.setter = setter;
-      this.copyFunction = copyFunction;
+      this.copier = copier;
       this.access = SimpleMethod.access(setter);
       this.name = setter.getSimpleName().toString();
       primitiveParameter = parameterType.getKind().isPrimitive();
@@ -423,13 +446,10 @@ class BuilderSpec {
     }
 
     public String copy(AutoValueProcessor.Property property) {
-      String copy = copyFunction.apply(property.toString());
-
-      // Add a null guard only in cases where we are using copyOf and the property is @Nullable.
-      if (property.isNullable() && !copy.equals(property.toString())) {
+      String copy = copier.copy.apply(property.toString());
+      if (property.isNullable() && !copier.acceptsNull) {
         copy = String.format("(%s == null ? null : %s)", property, copy);
       }
-
       return copy;
     }
   }