Allow an AutoValue property builder to have a parameter.

Previously, a property builder for `abstract ImmutableSet<String> foos()` had to look like `abstract ImmutableSet.Builder<String> foosBuilder()`. For `ImmutableSortedSet`, this meant that you could only construct a builder using `ImmutableSortedSet.naturalOrder()`. Now, if you have `abstract ImmutableSortedSet<String> foos()` you can optionally write `abstract ImmutableSortedSet.Builder<String> foosBuilder(Comparator<String> comparator)`. This works because `ImmutableSortedSet.Builder<T>` has a constructor `ImmutableSortedSet.Builder(Comparator<T>)`.

This change is closely based on #983 by Paweł Łabaj. I adjusted the formatting slightly, renamed a couple of methods, and reworded the new text in the user guide. The only substantive change is that I removed the special knowledge of the static `orderedBy` method (from `ImmutableSortedSet` and `ImmutableSortedMap`) since the builder constructor works just as well in those cases.

Closes #983.
Fixes #984.

RELNOTES=AutoValue property builders can now have a single parameter which is passed to the constructor of the new builder.
PiperOrigin-RevId: 358583454

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

@@ -34,6 +34,7 @@
 import com.google.common.collect.ImmutableSortedMap;
 import com.google.common.collect.ImmutableSortedSet;
 import com.google.common.collect.ImmutableTable;
+import com.google.common.collect.Ordering;
 import com.google.common.testing.EqualsTester;
 import com.google.common.testing.SerializableTester;
 import java.io.ObjectStreamClass;
@@ -52,6 +53,7 @@
 import java.util.Arrays;
 import java.util.Collection;
 import java.util.Collections;
+import java.util.Comparator;
 import java.util.HashMap;
 import java.util.LinkedHashMap;
 import java.util.List;
@@ -60,6 +62,7 @@
 import java.util.NavigableSet;
 import java.util.NoSuchElementException;
 import java.util.SortedMap;
+import java.util.SortedSet;
 import java.util.TreeMap;
 import java.util.TreeSet;
 import javax.annotation.Nullable;
@@ -3476,4 +3479,92 @@ public void builderAnnotationsCopiedIfRequested() {
     assertThat(builder.getClass().getAnnotations()).asList().containsExactly(myAnnotation("thing"));
     assertThat(builder.setFoo("foo").build().foo()).isEqualTo("foo");
   }
+
+  @AutoValue
+  @SuppressWarnings({"rawtypes", "unchecked"}) // deliberately checking handling of raw types
+  abstract static class DataWithSortedCollectionBuilders<K, V> {
+    abstract ImmutableSortedMap<K, V> anImmutableSortedMap();
+    abstract ImmutableSortedSet<V> anImmutableSortedSet();
+    abstract ImmutableSortedMap<Integer, V> nonGenericImmutableSortedMap();
+    abstract ImmutableSortedSet rawImmutableSortedSet();
+    abstract DataWithSortedCollectionBuilders.Builder<K, V> toBuilder();
+
+    static <K, V> DataWithSortedCollectionBuilders.Builder<K, V> builder() {
+      return new AutoValue_AutoValueTest_DataWithSortedCollectionBuilders.Builder<K, V>();
+    }
+
+    @AutoValue.Builder
+    abstract static class Builder<K, V> {
+      abstract DataWithSortedCollectionBuilders.Builder<K, V> anImmutableSortedMap(
+          SortedMap<K, V> anImmutableSortedMap);
+      abstract ImmutableSortedMap.Builder<K, V> anImmutableSortedMapBuilder(
+          Comparator<K> keyComparator);
+      abstract DataWithSortedCollectionBuilders.Builder<K, V> anImmutableSortedSet(
+          SortedSet<V> anImmutableSortedSet);
+      abstract ImmutableSortedSet.Builder<V> anImmutableSortedSetBuilder(Comparator<V> comparator);
+      abstract ImmutableSortedMap.Builder<Integer, V> nonGenericImmutableSortedMapBuilder(
+          Comparator<Integer> keyComparator);
+      abstract ImmutableSortedSet.Builder rawImmutableSortedSetBuilder(Comparator comparator);
+      abstract DataWithSortedCollectionBuilders<K, V> build();
+    }
+  }
+
+  @Test
+  @SuppressWarnings({"rawtypes", "unchecked"}) // deliberately checking handling of raw types
+  public void shouldGenerateBuildersWithComparators() {
+    Comparator<String> stringComparator = new Comparator<String>() {
+      @Override
+      public int compare(String left, String right) {
+        return left.compareTo(right);
+      }
+    };
+
+    Comparator<Integer> intComparator = new Comparator<Integer>() {
+      @Override
+      public int compare(Integer o1, Integer o2) {
+        return o1 - o2;
+      }
+    };
+
+    Comparator comparator = new Comparator() {
+      @Override
+      public int compare(Object left, Object right) {
+        return String.valueOf(left).compareTo(String.valueOf(right));
+      }
+    };
+
+    AutoValueTest.DataWithSortedCollectionBuilders.Builder<String, Integer> builder =
+        AutoValueTest.DataWithSortedCollectionBuilders.builder();
+
+    builder.anImmutableSortedMapBuilder(stringComparator)
+        .put("Charlie", 1).put("Alfa", 2).put("Bravo", 3);
+    builder.anImmutableSortedSetBuilder(intComparator)
+        .add(1,5,9,3);
+    builder.nonGenericImmutableSortedMapBuilder(intComparator)
+        .put(9, 99).put(1, 11).put(3, 33);
+    builder.rawImmutableSortedSetBuilder(comparator)
+        .add("Bravo", "Charlie", "Alfa");
+
+    AutoValueTest.DataWithSortedCollectionBuilders<String, Integer> data = builder.build();
+
+    AutoValueTest.DataWithSortedCollectionBuilders.Builder<String, Integer> copiedBuilder =
+        data.toBuilder();
+    AutoValueTest.DataWithSortedCollectionBuilders<String, Integer> copiedData =
+        copiedBuilder.build();
+
+    assertThat(data.anImmutableSortedMap().keySet())
+        .containsExactly("Alfa", "Bravo", "Charlie")
+        .inOrder();
+    assertThat(data.anImmutableSortedSet()).containsExactly(1, 3, 5, 9).inOrder();
+    assertThat(data.nonGenericImmutableSortedMap().keySet()).containsExactly(1, 3, 9).inOrder();
+    assertThat(data.rawImmutableSortedSet()).containsExactly("Alfa", "Bravo", "Charlie").inOrder();
+
+    assertThat(copiedData).isEqualTo(data);
+
+    try {
+      builder.anImmutableSortedMapBuilder(Ordering.from(stringComparator).reverse());
+      fail("Calling property builder method a second time should have failed");
+    } catch (IllegalStateException expected) {
+    }
+  }
 }

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

@@ -350,11 +350,21 @@ private void classifyGetter(ExecutableElement builderGetter, ExecutableElement o
   }
 
   /**
-   * Classifies a method given that it has one argument. Currently, a method with one argument can
-   * only be a setter, meaning that it must look like {@code foo(T)} or {@code setFoo(T)}, where the
-   * {@code AutoValue} class has a property called {@code foo} of type {@code T}.
+   * Classifies a method given that it has one argument. A method with one argument can be:
+   *
+   * <ul>
+   *   <li>a setter, meaning that it looks like {@code foo(T)} or {@code setFoo(T)}, where the
+   *       {@code AutoValue} class has a property called {@code foo} of type {@code T};
+   *   <li>a property builder with one argument, meaning it looks like {@code
+   *       ImmutableSortedSet.Builder<V> foosBuilder(Comparator<V>)}, where the {@code AutoValue}
+   *       class has a property called {@code foos} with a type whose builder can be made with an
+   *       argument of the given type.
+   * </ul>
    */
   private void classifyMethodOneArg(ExecutableElement method) {
+    if (classifyPropertyBuilderOneArg(method)) {
+      return;
+    }
     String methodName = method.getSimpleName().toString();
     Map<String, ExecutableElement> propertyNameToGetter = getterToPropertyName.inverse();
     String propertyName = null;
@@ -417,6 +427,38 @@ private void classifyMethodOneArg(ExecutableElement method) {
     }
   }
 
+  /**
+   * Classifies a method given that it has one argument and is a property builder with a parameter,
+   * like {@code ImmutableSortedSet.Builder<String> foosBuilder(Comparator<String>)}.
+   *
+   * @param method A method to classify
+   * @return true if method has been classified successfully
+   */
+  private boolean classifyPropertyBuilderOneArg(ExecutableElement method) {
+    String methodName = method.getSimpleName().toString();
+    if (!methodName.endsWith("Builder")) {
+      return false;
+    }
+    String property = methodName.substring(0, methodName.length() - "Builder".length());
+    if (!getterToPropertyName.containsValue(property)) {
+      return false;
+    }
+    PropertyBuilderClassifier propertyBuilderClassifier =
+        new PropertyBuilderClassifier(
+            errorReporter,
+            typeUtils,
+            elementUtils,
+            this,
+            getterToPropertyName,
+            getterToPropertyType,
+            eclipseHack);
+    Optional<PropertyBuilder> maybePropertyBuilder =
+        propertyBuilderClassifier.makePropertyBuilder(method, property);
+    maybePropertyBuilder.ifPresent(
+        propertyBuilder -> propertyNameToPropertyBuilder.put(property, propertyBuilder));
+    return maybePropertyBuilder.isPresent();
+  }
+
   // A frequent source of problems is where the JavaBeans conventions have been followed for
   // most but not all getters. Then AutoValue considers that they haven't been followed at all,
   // so you might have a property called getFoo where you thought it was called just foo, and

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

@@ -15,16 +15,21 @@
  */
 package com.google.auto.value.processor;
 
+import static java.util.stream.Collectors.collectingAndThen;
+import static java.util.stream.Collectors.joining;
+import static java.util.stream.Collectors.toMap;
+
 import com.google.auto.common.MoreElements;
 import com.google.auto.common.MoreTypes;
 import com.google.common.collect.ImmutableBiMap;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
-import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
+import java.util.Objects;
 import java.util.Optional;
+import java.util.function.Function;
 import javax.lang.model.element.AnnotationMirror;
 import javax.lang.model.element.ElementKind;
 import javax.lang.model.element.ExecutableElement;
@@ -115,6 +120,14 @@ public ExecutableElement getPropertyBuilderMethod() {
       return propertyBuilderMethod;
     }
 
+    /** The property builder method parameters, for example {@code Comparator<T> comparator} */
+    public String getPropertyBuilderMethodParameters() {
+      return propertyBuilderMethod.getParameters().stream()
+          .map(
+              parameter -> TypeEncoder.encode(parameter.asType()) + " " + parameter.getSimpleName())
+          .collect(joining(", "));
+    }
+
     public String getAccess() {
       return SimpleMethod.access(propertyBuilderMethod);
     }
@@ -251,8 +264,13 @@ Optional<PropertyBuilder> makePropertyBuilder(ExecutableElement method, String p
       return Optional.empty();
     }
 
-    Optional<ExecutableElement> maybeBuilderMaker =
-        builderMaker(barNoArgMethods, barBuilderTypeElement);
+    Optional<ExecutableElement> maybeBuilderMaker;
+    if (method.getParameters().isEmpty()) {
+      maybeBuilderMaker = noArgBuilderMaker(barNoArgMethods, barBuilderTypeElement);
+    } else {
+      Map<String, ExecutableElement> barOneArgMethods = oneArgumentMethodsOf(barTypeElement);
+      maybeBuilderMaker = oneArgBuilderMaker(barOneArgMethods, barBuilderTypeElement);
+    }
     if (!maybeBuilderMaker.isPresent()) {
       errorReporter.reportError(
           method,
@@ -268,10 +286,14 @@ Optional<PropertyBuilder> makePropertyBuilder(ExecutableElement method, String p
 
     String barBuilderType = TypeEncoder.encodeWithAnnotations(barBuilderTypeMirror);
     String rawBarType = TypeEncoder.encodeRaw(barTypeMirror);
+    String arguments =
+        method.getParameters().isEmpty()
+            ? "()"
+            : "(" + method.getParameters().get(0).getSimpleName() + ")";
     String initializer =
         (builderMaker.getKind() == ElementKind.CONSTRUCTOR)
-            ? "new " + barBuilderType + "()"
-            : rawBarType + "." + builderMaker.getSimpleName() + "()";
+            ? "new " + barBuilderType + arguments
+            : rawBarType + "." + builderMaker.getSimpleName() + arguments;
     String builtToBuilder = null;
     String copyAll = null;
     ExecutableElement toBuilder = barNoArgMethods.get("toBuilder");
@@ -324,41 +346,75 @@ Optional<PropertyBuilder> makePropertyBuilder(ExecutableElement method, String p
   private static final ImmutableSet<String> BUILDER_METHOD_NAMES =
       ImmutableSet.of("naturalOrder", "builder", "newBuilder");
 
-  // (2) `BarBuilder must have a public no-arg constructor, or `Bar` must have a visible static
-  //      method `naturalOrder(), `builder()`, or `newBuilder()` that returns `BarBuilder`.
-  private Optional<ExecutableElement> builderMaker(
+  // (2) `BarBuilder` must have a public no-arg constructor, or `Bar` must have a visible static
+  //      method `naturalOrder(), `builder()`, or `newBuilder()` that returns `BarBuilder`; or,
+  //      if we have a foosBuilder(T) method, then `BarBuilder` must have a public constructor with
+  //      a single parameter assignable from T, or a visible static `builder(T)` method.
+  private Optional<ExecutableElement> noArgBuilderMaker(
       Map<String, ExecutableElement> barNoArgMethods, TypeElement barBuilderTypeElement) {
-    for (String builderMethodName : BUILDER_METHOD_NAMES) {
-      ExecutableElement method = barNoArgMethods.get(builderMethodName);
-      if (method != null
-          && method.getModifiers().contains(Modifier.STATIC)
-          && typeUtils.isSameType(
-              typeUtils.erasure(method.getReturnType()),
-              typeUtils.erasure(barBuilderTypeElement.asType()))) {
-        // TODO(emcmanus): check visibility. We don't want to require public for @AutoValue
-        // builders. By not checking visibility we risk accepting something as a builder maker
-        // and then failing when the generated code tries to call Bar.builder(). But the risk
-        // seems small.
-        return Optional.of(method);
-      }
+    return builderMaker(BUILDER_METHOD_NAMES, barNoArgMethods, barBuilderTypeElement, 0);
+  }
+
+  private static final ImmutableSet<String> ONE_ARGUMENT_BUILDER_METHOD_NAMES =
+      ImmutableSet.of("builder");
+
+  private Optional<ExecutableElement> oneArgBuilderMaker(
+      Map<String, ExecutableElement> barOneArgMethods, TypeElement barBuilderTypeElement) {
+
+    return builderMaker(
+        ONE_ARGUMENT_BUILDER_METHOD_NAMES, barOneArgMethods, barBuilderTypeElement, 1);
+  }
+
+  private Optional<ExecutableElement> builderMaker(
+      ImmutableSet<String> methodNamesToCheck,
+      Map<String, ExecutableElement> methods,
+      TypeElement barBuilderTypeElement,
+      int argumentCount) {
+    Optional<ExecutableElement> maybeMethod =
+        methodNamesToCheck.stream()
+            .map(methods::get)
+            .filter(Objects::nonNull)
+            .filter(method -> method.getModifiers().contains(Modifier.STATIC))
+            .filter(
+                method ->
+                    typeUtils.isSameType(
+                        typeUtils.erasure(method.getReturnType()),
+                        typeUtils.erasure(barBuilderTypeElement.asType())))
+            .findFirst();
+
+    if (maybeMethod.isPresent()) {
+      // TODO(emcmanus): check visibility. We don't want to require public for @AutoValue
+      // builders. By not checking visibility we risk accepting something as a builder maker
+      // and then failing when the generated code tries to call Bar.builder(). But the risk
+      // seems small.
+      return maybeMethod;
     }
-    return ElementFilter.constructorsIn(barBuilderTypeElement.getEnclosedElements())
-        .stream()
-        .filter(c -> c.getParameters().isEmpty())
+
+    return ElementFilter.constructorsIn(barBuilderTypeElement.getEnclosedElements()).stream()
+        .filter(c -> c.getParameters().size() == argumentCount)
         .filter(c -> c.getModifiers().contains(Modifier.PUBLIC))
         .findFirst();
   }
 
   private Map<String, ExecutableElement> noArgMethodsOf(TypeElement type) {
-    // Can't easily use ImmutableMap here because getAllMembers could return more than one method
-    // with the same name.
-    Map<String, ExecutableElement> methods = new LinkedHashMap<>();
-    for (ExecutableElement method : ElementFilter.methodsIn(elementUtils.getAllMembers(type))) {
-      if (method.getParameters().isEmpty() && !isStaticInterfaceMethodNotIn(method, type)) {
-        methods.put(method.getSimpleName().toString(), method);
-      }
-    }
-    return methods;
+    return methodsOf(type, 0);
+  }
+
+  private ImmutableMap<String, ExecutableElement> oneArgumentMethodsOf(TypeElement type) {
+    return methodsOf(type, 1);
+  }
+
+  private ImmutableMap<String, ExecutableElement> methodsOf(TypeElement type, int argumentCount) {
+    return ElementFilter.methodsIn(elementUtils.getAllMembers(type)).stream()
+        .filter(method -> method.getParameters().size() == argumentCount)
+        .filter(method -> !isStaticInterfaceMethodNotIn(method, type))
+        .collect(
+            collectingAndThen(
+                toMap(
+                    method -> method.getSimpleName().toString(),
+                    Function.identity(),
+                    (method1, method2) -> method1),
+                ImmutableMap::copyOf));
   }
 
   // Work around an Eclipse compiler bug: https://bugs.eclipse.org/bugs/show_bug.cgi?id=547185

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

@@ -291,7 +291,7 @@ ${modifiers}class $subclass$formalTypes extends $origClass$actualTypes {
     #if ($propertyBuilder)
 
     @`java.lang.Override`
-    ${propertyBuilder.access}$propertyBuilder.builderType ${p.name}Builder() {
+    ${propertyBuilder.access}$propertyBuilder.builderType ${p.name}Builder($propertyBuilder.propertyBuilderMethodParameters) {
       if (${propertyBuilder.name} == null) {
 
         ## This is the first time someone has asked for the builder. If the property it sets already
@@ -327,7 +327,16 @@ ${modifiers}class $subclass$formalTypes extends $origClass$actualTypes {
 
         #end
 
+      } #if (!$propertyBuilder.propertyBuilderMethodParameters.empty) else {
+          ## This check only happens if the property-builder method has a parameter.
+          ## We don't know if the existing builder was created with the same parameter,
+          ## so we throw to avoid possibly giving you a builder that is different from
+          ## the one you asked for.
+
+        throw new IllegalStateException("Property builder for $p.name is already defined");
       }
+        #end
+
       return $propertyBuilder.name;
     }