#438 Change name of build and other methods

If generating public build, buildPartial, clear or mergeFrom methods would cause a compiler error, instead make them package-protected and, if necessary, find an alternative method name by prepending an underscore and appending Impl and optionally a number.

Author: alicederyn

README.md

@@ -533,6 +533,27 @@ For instance, `assertEquals` in JUnit relies on equality; it will not know to ch
 If you are only testing a subset of your fields for equality, consider separating your class in two, as you may have accidentally combined the key and the value of a map into a single object, and you may find your code becomes healthier after the separation.
 Alternatively, creating a custom [Comparator] will make it explicit that you are not using the natural definition of equality.
 
+### Custom conventional method names
+
+If for any reason your types cannot use the conventional method names (`build`, `buildPartial`, `clear` and `mergeFrom`), you can force FreeBuilder to generate package protected implementations, and even select alternative fallback names if necessary, by declaring an alternative visibility and/or incompatible signature. If the default name is not available, FreeBuilder will prepend an underscore and append "Impl" (and, if necessary, a number), e.g. `build` becomes `_buildImpl`.
+
+```java
+public interface MyType {
+  class Builder extends MyType_Builder {
+    public OtherDataType build() {
+      // This signature is not compatible with the default build method.
+      // FreeBuilder will instead declare a package-scoped _buildImpl.
+      ...
+    }
+    public DataType buildMyType() {
+      return _buildImpl();
+    }
+  }
+}
+```
+
+Note that this will, unfortunately, disable FreeBuilder's [enhanced support for nested builders](#nested-buildable-types) for this type, as it needs to be able to call these methods.
+
 ### Custom functional interfaces
 
 FreeBuilder's generated map and mutate methods take [UnaryOperator] or [Consumer] functional interfaces. If you need to use a different functional interface, you can override the generated methods in your Builder and change the parameter type. FreeBuilder will spot the incompatible override and change the code it generates to match:

generated/main/java/org/inferred/freebuilder/processor/Datatype_Builder.java

@@ -21,6 +21,7 @@
 import static com.google.common.collect.Iterables.transform;
 
 import static org.inferred.freebuilder.processor.GwtSupport.gwtMetadata;
+import static org.inferred.freebuilder.processor.NamePicker.pickName;
 import static org.inferred.freebuilder.processor.model.MethodFinder.methodsOn;
 import static org.inferred.freebuilder.processor.model.ModelUtils.asElement;
 import static org.inferred.freebuilder.processor.model.ModelUtils.getReturnType;
@@ -144,6 +145,13 @@ GeneratedType analyse(TypeElement type) throws CannotGenerateCodeException {
         .setPartialType(partialType.withParameters(typeParameters))
         .setPropertyEnum(propertyType.withParameters())
         .putAllStandardMethodUnderrides(findUnderriddenMethods(methods))
+        .setBuildMethod(pickName(builder, elements, types, type.asType(), "build"))
+        .setBuildPartialMethod(pickName(builder, elements, types, type.asType(), "buildPartial"))
+        .setClearMethod(pickName(builder, elements, types, builder, "clear"))
+        .setMergeFromBuilderMethod(pickName(
+            builder, elements, types, builder, "mergeFrom", builder))
+        .setMergeFromValueMethod(pickName(
+            builder, elements, types, builder, "mergeFrom", type.asType()))
         .setHasToBuilderMethod(hasToBuilderMethod(
             builder, constructionAndExtension.isExtensible(), methods))
         .setBuilderSerializable(shouldBuilderBeSerializable(builder))

src/main/java/org/inferred/freebuilder/processor/Analyser.java

@@ -15,6 +15,7 @@
  */
 package org.inferred.freebuilder.processor;
 
+import static org.inferred.freebuilder.processor.NamePicker.pickName;
 import static org.inferred.freebuilder.processor.model.ModelUtils.asElement;
 import static org.inferred.freebuilder.processor.model.ModelUtils.findAnnotationMirror;
 import static org.inferred.freebuilder.processor.model.ModelUtils.needsSafeVarargs;
@@ -24,6 +25,7 @@
 import static javax.lang.model.element.Modifier.PUBLIC;
 
 import org.inferred.freebuilder.FreeBuilder;
+import org.inferred.freebuilder.processor.Datatype.Visibility;
 import org.inferred.freebuilder.processor.source.Excerpt;
 import org.inferred.freebuilder.processor.source.Excerpts;
 import org.inferred.freebuilder.processor.source.Type;
@@ -134,7 +136,17 @@ public static Optional<DeclaredType> maybeBuilder(
      * probably skip this for @FreeBuilder-types anyway, to avoid extra types whenever possible,
      * which leaves a lot of complicated code supporting a currently non-existent edge case.
      */
-    if (!findAnnotationMirror(element, FreeBuilder.class).isPresent()) {
+    if (findAnnotationMirror(element, FreeBuilder.class).isPresent()) {
+      // Make sure the user isn't preventing us generating required methods.
+      if (methodIsObscured(builderMirror, elements, types, type, "build")
+         || methodIsObscured(builderMirror, elements, types, type, "buildPartial")
+         || methodIsObscured(builderMirror, elements, types, builderMirror, "clear")
+         || methodIsObscured(
+             builderMirror, elements, types, builderMirror, "mergeFrom", builderMirror)
+         || methodIsObscured(builderMirror, elements, types, builderMirror, "mergeFrom", type)) {
+        return Optional.empty();
+      }
+    } else {
       List<ExecutableElement> methods = elements.getAllMembers(builder)
           .stream()
           .flatMap(METHODS)
@@ -165,6 +177,18 @@ public static Optional<DeclaredType> maybeBuilder(
     return Optional.of(builderMirror);
   }
 
+  private static boolean methodIsObscured(
+      DeclaredType targetType,
+      Elements elements,
+      Types types,
+      DeclaredType returnType,
+      String methodName,
+      DeclaredType... parameterTypes) {
+    NameAndVisibility buildMethod =
+        pickName(targetType, elements, types, returnType, methodName, parameterTypes);
+    return buildMethod.name() != methodName || buildMethod.visibility() != Visibility.PUBLIC;
+  }
+
   public static BuildableType create(
       DeclaredType datatype,
       DeclaredType builder,

src/main/java/org/inferred/freebuilder/processor/BuildableType.java

@@ -128,6 +128,21 @@ public UnderrideLevel standardMethodUnderride(StandardMethod standardMethod) {
   /** Returns whether the value type has a toBuilder method that needs to be generated. */
   public abstract boolean getHasToBuilderMethod();
 
+  /** Returns the build method to be generated. */
+  public abstract NameAndVisibility getBuildMethod();
+
+  /** Returns the partial build method to be generated. */
+  public abstract NameAndVisibility getBuildPartialMethod();
+
+  /** Returns the clear method to be generated. */
+  public abstract NameAndVisibility getClearMethod();
+
+  /** Returns the mergeFrom(Builder) method to be generated. */
+  public abstract NameAndVisibility getMergeFromBuilderMethod();
+
+  /** Returns the mergeFrom(Value) method to be generated. */
+  public abstract NameAndVisibility getMergeFromValueMethod();
+
   /** Returns a list of annotations that should be applied to the generated builder class. */
   public abstract ImmutableList<Excerpt> getGeneratedBuilderAnnotations();
 
@@ -148,6 +163,11 @@ public Builder toBuilder() {
   public static class Builder extends Datatype_Builder {
 
     public Builder() {
+      super.setBuildMethod(NameAndVisibility.of("build", Visibility.PUBLIC));
+      super.setBuildPartialMethod(NameAndVisibility.of("buildPartial", Visibility.PUBLIC));
+      super.setClearMethod(NameAndVisibility.of("clear", Visibility.PUBLIC));
+      super.setMergeFromBuilderMethod(NameAndVisibility.of("mergeFrom", Visibility.PUBLIC));
+      super.setMergeFromValueMethod(NameAndVisibility.of("mergeFrom", Visibility.PUBLIC));
       super.setValueTypeVisibility(Visibility.PRIVATE);
       super.setHasToBuilderMethod(false);
     }

src/main/java/org/inferred/freebuilder/processor/Datatype.java

@@ -149,8 +149,9 @@ private void addStaticFromMethod(SourceBuilder code) {
       code.addLine("  if (value instanceof %s) {", rebuildable.getQualifiedName())
           .addLine("    return ((%s) value).toBuilder();", rebuildable)
           .addLine("  } else {")
-          .addLine("    return %s.mergeFrom(value);",
-              builderFactory.newBuilder(datatype.getBuilder(), EXPLICIT_TYPES))
+          .addLine("    return %s.%s(value);",
+              builderFactory.newBuilder(datatype.getBuilder(), EXPLICIT_TYPES),
+              datatype.getMergeFromValueMethod().name())
           .addLine("  }");
     }
     code.addLine("}");
@@ -182,7 +183,10 @@ private void addBuildMethod(SourceBuilder code) {
           .addLine(" * @throws IllegalStateException if any field has not been set");
     }
     code.addLine(" */")
-        .addLine("public %s build() {", datatype.getType());
+        .addLine("%s%s %s() {",
+            datatype.getBuildMethod().visibility(),
+            datatype.getType(),
+            datatype.getBuildMethod().name());
     if (hasRequiredProperties) {
       code.add(PreconditionExcerpts.checkState(
           "%1$s.isEmpty()", "Not set: %1$s", UNSET_PROPERTIES));
@@ -199,7 +203,11 @@ private void addMergeFromValueMethod(SourceBuilder code) {
         .addLine(" *")
         .addLine(" * @return this {@code %s} object", datatype.getBuilder().getSimpleName())
         .addLine(" */")
-        .addLine("public %s mergeFrom(%s value) {", datatype.getBuilder(), datatype.getType());
+        .addLine("%s%s %s(%s value) {",
+            datatype.getMergeFromValueMethod().visibility(),
+            datatype.getBuilder(),
+            datatype.getMergeFromValueMethod().name(),
+            datatype.getType());
     generatorsByProperty.values().forEach(generator -> generator.addMergeFromValue(code, "value"));
     code.addLine("  return (%s) this;", datatype.getBuilder())
         .addLine("}");
@@ -213,7 +221,10 @@ private void addMergeFromBuilderMethod(SourceBuilder code) {
         .addLine(" *")
         .addLine(" * @return this {@code %s} object", datatype.getBuilder().getSimpleName())
         .addLine(" */")
-        .addLine("public %1$s mergeFrom(%1$s template) {", datatype.getBuilder());
+        .addLine("%1$s%2$s %3$s(%2$s template) {",
+            datatype.getMergeFromBuilderMethod().visibility(),
+            datatype.getBuilder(),
+            datatype.getMergeFromBuilderMethod().name());
     generatorsByProperty.values().forEach(generator -> {
       generator.addMergeFromBuilder(code, "template");
     });
@@ -235,7 +246,10 @@ private void addClearMethod(SourceBuilder code) {
         .addLine(" *")
         .addLine(" * @return this {@code %s} object", datatype.getBuilder().getSimpleName())
         .addLine(" */")
-        .addLine("public %s clear() {", datatype.getBuilder());
+        .addLine("%s%s %s() {",
+            datatype.getClearMethod().visibility(),
+            datatype.getBuilder(),
+            datatype.getClearMethod().name());
     generatorsByProperty.values().forEach(codeGenerator -> {
       codeGenerator.addClearField(code);
     });
@@ -268,7 +282,9 @@ private void addBuildPartialMethod(SourceBuilder code) {
     }
     code.addLine("will propagate the partial status of its input, overriding")
         .addLine(" * %s to return another partial.",
-            datatype.getBuilder().javadocNoArgMethodLink("build").withText("build()"))
+            datatype.getBuilder()
+                .javadocNoArgMethodLink(datatype.getBuildMethod().name())
+                .withText(datatype.getBuildMethod().name() + "()"))
         .addLine(" * This allows for robust tests of modify-rebuild code.")
         .addLine(" *")
         .addLine(" * <p>Partials should only ever be used in tests. They permit writing robust")
@@ -279,7 +295,10 @@ private void addBuildPartialMethod(SourceBuilder code) {
     if (code.feature(GUAVA).isAvailable()) {
       code.addLine("@%s()", VisibleForTesting.class);
     }
-    code.addLine("public %s buildPartial() {", datatype.getType())
+    code.addLine("%s%s %s() {",
+            datatype.getBuildPartialMethod().visibility(),
+            datatype.getType(),
+            datatype.getBuildPartialMethod().name())
         .addLine("  return %s(this);", datatype.getPartialType().constructor())
         .addLine("}");
   }
@@ -535,8 +554,9 @@ private void addPartialToBuilderMethod(SourceBuilder code) {
       code.addLine("")
           .addLine("  private static class PartialBuilder%s extends %s {",
               datatype.getType().declarationParameters(), datatype.getBuilder())
-          .addLine("    @Override public %s build() {", datatype.getType())
-          .addLine("      return buildPartial();")
+          .addLine("    @Override public %s %s() {",
+              datatype.getType(), datatype.getBuildMethod().name())
+          .addLine("      return %s();", datatype.getBuildPartialMethod().name())
           .addLine("    }")
           .addLine("  }");
     }

src/main/java/org/inferred/freebuilder/processor/GeneratedBuilder.java

@@ -141,7 +141,8 @@ private void addInstantiateInstance(SourceBuilder code) {
               .addLine("    }");
         }
       }
-      code.addLine("    return (%s) %s.build();", datatype.getValueType(), builder)
+      code.addLine("    return (%s) %s.%s();",
+              datatype.getValueType(), builder, datatype.getBuildMethod().name())
           .addLine("  }");
     }
 

src/main/java/org/inferred/freebuilder/processor/GwtSupport.java

@@ -0,0 +1,50 @@
+package org.inferred.freebuilder.processor;
+
+import org.inferred.freebuilder.processor.Datatype.Visibility;
+
+import java.util.Objects;
+
+public class NameAndVisibility {
+
+  public static NameAndVisibility of(String name, Visibility visibility) {
+    return new NameAndVisibility(name, visibility);
+  }
+
+  private final String name;
+  private final Visibility visibility;
+
+  private NameAndVisibility(String name, Visibility visibility) {
+    this.name = name;
+    this.visibility = visibility;
+  }
+
+  public String name() {
+    return name;
+  }
+
+  public Visibility visibility() {
+    return visibility;
+  }
+
+  @Override
+  public int hashCode() {
+    return Objects.hash(name, visibility);
+  }
+
+  @Override
+  public boolean equals(Object obj) {
+    if (this == obj) {
+      return true;
+    }
+    if (!(obj instanceof NameAndVisibility)) {
+      return false;
+    }
+    NameAndVisibility other = (NameAndVisibility) obj;
+    return Objects.equals(name, other.name) && visibility == other.visibility;
+  }
+
+  @Override
+  public String toString() {
+    return "NameAndVisibility{name=" + name + ", visibility=" + visibility + "}";
+  }
+}

src/main/java/org/inferred/freebuilder/processor/NameAndVisibility.java

@@ -0,0 +1,104 @@
+package org.inferred.freebuilder.processor;
+
+import static org.inferred.freebuilder.processor.model.MethodFinder.methodsOn;
+import static org.inferred.freebuilder.processor.model.ModelUtils.asElement;
+import static org.inferred.freebuilder.processor.model.ModelUtils.getReturnType;
+
+import static java.util.stream.Collectors.toMap;
+
+import org.inferred.freebuilder.processor.Datatype.Visibility;
+
+import java.util.Map;
+import java.util.function.Predicate;
+import java.util.stream.Collector;
+
+import javax.lang.model.element.ExecutableElement;
+import javax.lang.model.element.Modifier;
+import javax.lang.model.element.VariableElement;
+import javax.lang.model.type.DeclaredType;
+import javax.lang.model.type.TypeMirror;
+import javax.lang.model.util.Elements;
+import javax.lang.model.util.Types;
+
+public class NamePicker {
+
+  /** Find an available name and visibility for a method. */
+  public static NameAndVisibility pickName(
+      DeclaredType targetType,
+      Elements elements,
+      Types types,
+      TypeMirror returnType,
+      String preferredName,
+      TypeMirror... parameterTypes) {
+    Map<String, ExecutableElement> methodsByName =
+        methodsOn(asElement(targetType), elements, errorType -> { })
+            .stream()
+            .filter(matchingErasedParameters(types, parameterTypes))
+            .collect(byName());
+
+    String name = preferredName;
+    Visibility visibility = Visibility.PUBLIC;
+
+    for (int attempt = 1; true; attempt++) {
+      ExecutableElement method = methodsByName.get(name);
+      if (method == null) {
+        // No (parameterless) method exists with this name, so we can create one
+        return NameAndVisibility.of(name, visibility);
+      }
+      boolean sufficientVisibility = !method.getModifiers().contains(Modifier.PRIVATE);
+      boolean correctSignature = parametersMatchExactly(types, method, parameterTypes);
+      TypeMirror actualReturnType = getReturnType(targetType, method, types);
+      boolean correctReturnType = types.isSameType(actualReturnType, returnType);
+      if (sufficientVisibility && correctSignature && correctReturnType) {
+        // A method exists with this name, but it is not incompatible
+        if (!method.getModifiers().contains(Modifier.PUBLIC)) {
+          visibility = Visibility.PACKAGE;
+        }
+        return NameAndVisibility.of(name, visibility);
+      }
+
+      // This method name is already taken by an incompatible method; try a different name.
+      name = "_" + preferredName + "Impl";
+      visibility = Visibility.PACKAGE;
+      if (attempt > 1) {
+        name += attempt;
+      }
+    }
+  }
+
+  private static Predicate<? super ExecutableElement> matchingErasedParameters(
+      Types types, TypeMirror... parameterTypes) {
+    return method -> {
+      if (method.getParameters().size() != parameterTypes.length) {
+        return false;
+      }
+      for (int i = 0; i < parameterTypes.length; i++) {
+        VariableElement parameter = method.getParameters().get(i);
+        TypeMirror erasedType = types.erasure(parameterTypes[i]);
+        TypeMirror expectedErasedType = types.erasure(parameter.asType());
+        if (!types.isSameType(erasedType, expectedErasedType)) {
+          return false;
+        }
+      }
+      return true;
+    };
+  }
+
+  private static boolean parametersMatchExactly(
+      Types types, ExecutableElement method, TypeMirror... parameterTypes) {
+    if (method.getParameters().size() != parameterTypes.length) {
+      return false;
+    }
+    for (int i = 0; i < parameterTypes.length; i++) {
+      VariableElement parameter = method.getParameters().get(i);
+      if (!types.isSameType(parameterTypes[i], parameter.asType())) {
+        return false;
+      }
+    }
+    return true;
+  }
+
+  private static Collector<ExecutableElement, ?, Map<String, ExecutableElement>> byName() {
+    return toMap(method -> method.getSimpleName().toString(), method -> method);
+  }
+}