When checking builder setter parameters, use the final type. The final type is the type after type-variable substitution. Report this type in error messages, since it may not be obvious.

eamonnmcmanus · nick-someone · commit e97d1f085274 · 2020-01-02T11:08:42.000-05:00
(See AutoValueNotEclipseTest for an example of the problem this is fixing.) Fix CompileWithEclipseTest so that it actually does exclude AutoValueNotEclipseTest.java from the compilation, as intended. Move the existing test within that file into AutoValueTest.java, since apparently the Eclipse bug it was provoking has been fixed. Add the test for the bug being fixed here into AutoValueNotEclipseTest.java, because it hits another Eclipse bug. Fixes #802. RELNOTES=Fixed an issue with type checking of setter parameters in the presence of inheritance and generics. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=287580372
diff --git a/value/src/it/functional/src/test/java/com/google/auto/value/AutoValueNotEclipseTest.java b/value/src/it/functional/src/test/java/com/google/auto/value/AutoValueNotEclipseTest.java
@@ -15,9 +15,10 @@
  */
 package com.google.auto.value;
 
-import static com.google.common.truth.Truth.assertThat;
+import static com.google.common.truth.Truth8.assertThat;
 
-import com.google.common.collect.ImmutableList;
+import java.util.Optional;
+import javax.annotation.Nullable;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.junit.runners.JUnit4;
@@ -29,37 +30,36 @@
  */
 @RunWith(JUnit4.class)
 public class AutoValueNotEclipseTest {
-  interface ImmutableListOf<T> {
-    ImmutableList<T> list();
+  // This produced the following error with JDT 4.6:
+  // Internal compiler error: java.lang.Exception: java.lang.IllegalArgumentException: element
+  // public abstract B setOptional(T)  is not a member of the containing type
+  // com.google.auto.value.AutoValueTest.ConcreteOptional.Builder nor any of its superclasses at
+  // org.eclipse.jdt.internal.compiler.apt.dispatch.RoundDispatcher.handleProcessor(RoundDispatcher.java:169)
+  interface AbstractOptional<T> {
+    Optional<T> optional();
+
+    interface Builder<T, B extends Builder<T, B>> {
+      B setOptional(@Nullable T t);
+    }
   }
 
-  // This provoked the following with the Eclipse compiler:
-  // java.lang.NullPointerException
-  //   at org.eclipse.jdt.internal.compiler.lookup.ParameterizedTypeBinding.readableName(ParameterizedTypeBinding.java:1021)
-  //   at org.eclipse.jdt.internal.compiler.apt.model.DeclaredTypeImpl.toString(DeclaredTypeImpl.java:118)
-  //   at java.lang.String.valueOf(String.java:2996)
-  //   at java.lang.StringBuilder.append(StringBuilder.java:131)
-  //   at org.eclipse.jdt.internal.compiler.apt.model.TypesImpl.asMemberOf(TypesImpl.java:130)
-  //   at com.google.auto.value.processor.EclipseHack.methodReturnType(EclipseHack.java:124)
-  //   at com.google.auto.value.processor.TypeVariables.lambda$rewriteReturnTypes$1(TypeVariables.java:106)
   @AutoValue
-  abstract static class PropertyBuilderInheritsType implements ImmutableListOf<String> {
+  abstract static class ConcreteOptional implements AbstractOptional<String> {
     static Builder builder() {
-      return new AutoValue_AutoValueNotEclipseTest_PropertyBuilderInheritsType.Builder();
+      return new AutoValue_AutoValueNotEclipseTest_ConcreteOptional.Builder();
     }
 
     @AutoValue.Builder
-    abstract static class Builder {
-      abstract ImmutableList.Builder<String> listBuilder();
-      abstract PropertyBuilderInheritsType build();
+    interface Builder extends AbstractOptional.Builder<String, Builder> {
+      ConcreteOptional build();
     }
   }
 
   @Test
-  public void propertyBuilderInheritsType() {
-    PropertyBuilderInheritsType.Builder builder = PropertyBuilderInheritsType.builder();
-    builder.listBuilder().add("foo", "bar");
-    PropertyBuilderInheritsType x = builder.build();
-    assertThat(x.list()).containsExactly("foo", "bar").inOrder();
+  public void genericOptionalOfNullable() {
+    ConcreteOptional empty = ConcreteOptional.builder().build();
+    assertThat(empty.optional()).isEmpty();
+    ConcreteOptional notEmpty = ConcreteOptional.builder().setOptional("foo").build();
+    assertThat(notEmpty.optional()).hasValue("foo");
   }
 }
diff --git a/value/src/it/functional/src/test/java/com/google/auto/value/AutoValueTest.java b/value/src/it/functional/src/test/java/com/google/auto/value/AutoValueTest.java
@@ -2160,6 +2160,31 @@ public void testBuilderWithPropertyBuilders() {
     }
   }
 
+  interface ImmutableListOf<T> {
+    ImmutableList<T> list();
+  }
+
+  @AutoValue
+  abstract static class PropertyBuilderInheritsType implements ImmutableListOf<String> {
+    static Builder builder() {
+      return new AutoValue_AutoValueTest_PropertyBuilderInheritsType.Builder();
+    }
+
+    @AutoValue.Builder
+    abstract static class Builder {
+      abstract ImmutableList.Builder<String> listBuilder();
+      abstract PropertyBuilderInheritsType build();
+    }
+  }
+
+  @Test
+  public void propertyBuilderInheritsType() {
+    PropertyBuilderInheritsType.Builder builder = PropertyBuilderInheritsType.builder();
+    builder.listBuilder().add("foo", "bar");
+    PropertyBuilderInheritsType x = builder.build();
+    assertThat(x.list()).containsExactly("foo", "bar").inOrder();
+  }
+
   @AutoValue
   public abstract static class BuilderWithExoticPropertyBuilders<
       K extends Number, V extends Comparable<K>> {
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
@@ -434,7 +434,7 @@ private Optional<Function<String, String>> getSetterFunction(
     // Parameter type is not equal to property type, but might be convertible with copyOf.
     ImmutableList<ExecutableElement> copyOfMethods = copyOfMethods(targetType, setter);
     if (!copyOfMethods.isEmpty()) {
-      return getConvertingSetterFunction(copyOfMethods, valueGetter, setter);
+      return getConvertingSetterFunction(copyOfMethods, valueGetter, setter, parameterType);
     }
     String error =
         String.format(
@@ -452,9 +452,9 @@ private Optional<Function<String, String>> getSetterFunction(
   private Optional<Function<String, String>> getConvertingSetterFunction(
       ImmutableList<ExecutableElement> copyOfMethods,
       ExecutableElement valueGetter,
-      ExecutableElement setter) {
+      ExecutableElement setter,
+      TypeMirror parameterType) {
     DeclaredType targetType = MoreTypes.asDeclared(getterToPropertyType.get(valueGetter));
-    TypeMirror parameterType = setter.getParameters().get(0).asType();
     for (ExecutableElement copyOfMethod : copyOfMethods) {
       Optional<Function<String, String>> function =
           getConvertingSetterFunction(copyOfMethod, targetType, parameterType);
@@ -465,8 +465,9 @@ private Optional<Function<String, String>> getConvertingSetterFunction(
     String targetTypeSimpleName = targetType.asElement().getSimpleName().toString();
     String error =
         String.format(
-            "Parameter type of setter method should be %s to match getter %s.%s, or it should be a "
-                + "type that can be passed to %s.%s to produce %s",
+            "Parameter type %s of setter method should be %s to match getter %s.%s,"
+                + " or it should be a type that can be passed to %s.%s to produce %s",
+            parameterType,
             targetType,
             autoValueClass,
             valueGetter.getSimpleName(),
diff --git a/value/src/test/java/com/google/auto/value/processor/AutoValueCompilationTest.java b/value/src/test/java/com/google/auto/value/processor/AutoValueCompilationTest.java
@@ -1599,7 +1599,7 @@ public void autoValueBuilderWrongTypeSetterWithCopyOf() {
             .compile(javaFileObject);
     assertThat(compilation)
         .hadErrorContaining(
-            "Parameter type of setter method should be "
+            "Parameter type java.lang.String of setter method should be "
                 + "com.google.common.collect.ImmutableList<java.lang.String> to match getter "
                 + "foo.bar.Baz.blam, or it should be a type that can be passed to "
                 + "ImmutableList.copyOf")
@@ -1636,7 +1636,7 @@ public void autoValueBuilderWrongTypeSetterWithCopyOfGenericallyWrong() {
             .compile(javaFileObject);
     assertThat(compilation)
         .hadErrorContaining(
-            "Parameter type of setter method should be "
+            "Parameter type java.util.Collection<java.lang.Integer> of setter method should be "
                 + "com.google.common.collect.ImmutableList<java.lang.String> to match getter "
                 + "foo.bar.Baz.blam, or it should be a type that can be passed to "
                 + "ImmutableList.copyOf to produce "
