#283 Fix toBuilder() issues

Generate Partial.toBuilder whenever a package-visible no-args constructor exists, and issue a compiler error if one doesn't

Author: alicederyn

README.md

@@ -111,9 +111,10 @@ public interface Person {
 ```
 
 The `toBuilder()` method here is optional but highly recommended.
-If you are writing an abstract class, or using Java 8, you may wish to hide the
-builder's constructor and manually provide instead a static `builder()` method
-on the value type (though <em>Effective Java</em> does not do this).
+If you are writing an abstract class, or using Java 8, you may wish to make the
+builder's constructor package-protected and manually provide instead a static
+`builder()` method on the value type (though <em>Effective Java</em> does not do
+this).
 
 
 ### What you get

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

@@ -140,7 +140,7 @@ Metadata analyse(TypeElement type) throws CannotGenerateCodeException {
     QualifiedName generatedBuilder = QualifiedName.of(
         pkg.getQualifiedName().toString(), generatedBuilderSimpleName(type));
     Optional<TypeElement> builder = tryFindBuilder(generatedBuilder, type);
-    Optional<BuilderFactory> builderFactory = builderFactory(builder);
+    Metadata.Builder constructionAndExtension = constructionAndExtension(builder);
     QualifiedName valueType = generatedBuilder.nestedType("Value");
     QualifiedName partialType = generatedBuilder.nestedType("Partial");
     QualifiedName propertyType = generatedBuilder.nestedType("Property");
@@ -151,7 +151,7 @@ Metadata analyse(TypeElement type) throws CannotGenerateCodeException {
         .setType(QualifiedName.of(type).withParameters(typeParameters))
         .setInterfaceType(type.getKind().isInterface())
         .setBuilder(parameterized(builder, typeParameters))
-        .setBuilderFactory(builderFactory)
+        .mergeFrom(constructionAndExtension)
         .setGeneratedBuilder(generatedBuilder.withParameters(typeParameters))
         .setValueType(valueType.withParameters(typeParameters))
         .setPartialType(partialType.withParameters(typeParameters))
@@ -161,7 +161,8 @@ Metadata analyse(TypeElement type) throws CannotGenerateCodeException {
         .addVisibleNestedTypes(propertyType)
         .addAllVisibleNestedTypes(visibleTypesIn(type))  // Because we inherit from type
         .putAllStandardMethodUnderrides(findUnderriddenMethods(methods))
-        .setHasToBuilderMethod(hasToBuilderMethod(builder, builderFactory, methods))
+        .setHasToBuilderMethod(hasToBuilderMethod(
+            builder, constructionAndExtension.isExtensible(), methods))
         .setBuilderSerializable(shouldBuilderBeSerializable(builder))
         .addAllProperties(properties.values());
     Metadata baseMetadata = metadataBuilder.build();
@@ -310,18 +311,17 @@ private Map<StandardMethod, UnderrideLevel> findUnderriddenMethods(
     return result.build();
   }
 
-  /** Find a toBuilder method, if the user has provided one.
-   * @param builderFactory */
+  /** Find a toBuilder method, if the user has provided one. */
   private boolean hasToBuilderMethod(
       Optional<TypeElement> builder,
-      Optional<BuilderFactory> builderFactory,
+      boolean isExtensible,
       Iterable<ExecutableElement> methods) {
     if (!builder.isPresent()) {
       return false;
     }
     for (ExecutableElement method : methods) {
       if (isToBuilderMethod(builder.get(), method)) {
-        if (!builderFactory.isPresent()) {
+        if (!isExtensible) {
           messager.printMessage(ERROR,
               "No accessible no-args Builder constructor available to implement toBuilder",
               method);
@@ -408,15 +408,19 @@ private Optional<TypeElement> tryFindBuilder(
     return userClass;
   }
 
-  private Optional<BuilderFactory> builderFactory(Optional<TypeElement> builder) {
+  private Metadata.Builder constructionAndExtension(Optional<TypeElement> builder) {
     if (!builder.isPresent()) {
-      return Optional.of(NO_ARGS_CONSTRUCTOR);
+      return new Metadata.Builder()
+          .setExtensible(false)
+          .setBuilderFactory(NO_ARGS_CONSTRUCTOR);
     }
     if (!builder.get().getModifiers().contains(Modifier.STATIC)) {
       messager.printMessage(ERROR, "Builder must be static on @FreeBuilder types", builder.get());
-      return Optional.absent();
+      return new Metadata.Builder().setExtensible(false);
     }
-    return BuilderFactory.from(builder.get());
+    return new Metadata.Builder()
+        .setExtensible(BuilderFactory.hasNoArgsConstructor(builder.get()))
+        .setBuilderFactory(BuilderFactory.from(builder.get()));
   }
 
   private Map<ExecutableElement, Property> findProperties(

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

@@ -104,7 +104,7 @@ public static Optional<BuilderFactory> from(TypeElement builderType) {
       return Optional.of(BUILDER_METHOD);
     } else if (staticMethods.contains("newBuilder")) {
       return Optional.of(NEW_BUILDER_METHOD);
-    } else if (hasExplicitNoArgsConstructor(builderType)) {
+    } else if (hasNoArgsConstructor(builderType)) {
       return Optional.of(NO_ARGS_CONSTRUCTOR);
     } else {
       return Optional.absent();
@@ -118,7 +118,9 @@ private static boolean typeIsAbstract(TypeElement type) {
     return type.getModifiers().contains(Modifier.ABSTRACT);
   }
 
-  private static boolean hasExplicitNoArgsConstructor(TypeElement type) {
+  static boolean hasNoArgsConstructor(TypeElement type) {
+    // A type with no explictly-declared constructors will still have an implicit constructor in its
+    // enclosed elements.
     for (ExecutableElement constructor : constructorsIn(type.getEnclosedElements())) {
       if (constructor.getParameters().isEmpty()
           && !constructor.getModifiers().contains(Modifier.PRIVATE)) {

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

@@ -643,8 +643,7 @@ private static void addPartialToBuilderMethod(SourceBuilder code, Metadata metad
     if (!metadata.getHasToBuilderMethod()) {
       return;
     }
-    BuilderFactory builderFactory = metadata.getBuilderFactory().orNull();
-    if (builderFactory == BuilderFactory.NO_ARGS_CONSTRUCTOR) {
+    if (metadata.isExtensible()) {
       code.addLine("")
           .addLine("  private static class PartialBuilder%s extends %s {",
               metadata.getBuilder().declarationParameters(), metadata.getBuilder())
@@ -656,7 +655,7 @@ private static void addPartialToBuilderMethod(SourceBuilder code, Metadata metad
     code.addLine("")
         .addLine("  @%s", Override.class)
         .addLine("  public %s toBuilder() {", metadata.getBuilder());
-    if (builderFactory == BuilderFactory.NO_ARGS_CONSTRUCTOR) {
+    if (metadata.isExtensible()) {
       code.addLine("    %s builder = new PartialBuilder%s();",
               metadata.getBuilder(), metadata.getBuilder().typeParametersOrDiamondOperator());
       Block block = new Block(code);

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

@@ -112,6 +112,9 @@ public ParameterizedType getBuilder() {
     return getOptionalBuilder().get();
   }
 
+  /** Whether there is a package-visible, no-args constructor so we can subclass the Builder. */
+  public abstract boolean isExtensible();
+
   /** Returns the builder factory mechanism the user has exposed, if any. */
   public abstract Optional<BuilderFactory> getBuilderFactory();