Merge pull request #173 from google/block.merge.from.builder

Simplify CodeGenerator.addMergeFromBuilderMethod
inferred · Jun 23, 2016 · 8ec314b · 8ec314b
2 parents e8862b0 + c94416f
commit 8ec314b
Show file tree

Hide file tree

Showing 20 changed files with 106 additions and 90 deletions.
diff --git a/src/main/java/org/inferred/freebuilder/processor/BuildablePropertyFactory.java b/src/main/java/org/inferred/freebuilder/processor/BuildablePropertyFactory.java
@@ -273,7 +273,7 @@ public void addMergeFromValue(SourceBuilder code, String value) {
     }
 
     @Override
-    public void addMergeFromBuilder(SourceBuilder code, String builder) {
+    public void addMergeFromBuilder(Block code, String builder) {
       String propertyName = property.getName();
       if (propertyName.equals(builder)) {
         propertyName = "this." + propertyName;  // see issue #78

diff --git a/src/main/java/org/inferred/freebuilder/processor/CodeGenerator.java b/src/main/java/org/inferred/freebuilder/processor/CodeGenerator.java
@@ -187,36 +187,20 @@ private static void addMergeFromValueMethod(SourceBuilder code, Metadata metadat
   }
 
   private static void addMergeFromBuilderMethod(SourceBuilder code, Metadata metadata) {
-    boolean hasRequiredProperties = any(metadata.getProperties(), IS_REQUIRED);
     code.addLine("")
         .addLine("/**")
         .addLine(" * Copies values from the given {@code %s}.",
-            metadata.getBuilder().getSimpleName());
-    if (hasRequiredProperties) {
-      code.addLine(" * Does not affect any properties not set on the input.");
-    }
-    code.addLine(" */")
+            metadata.getBuilder().getSimpleName())
+        .addLine(" * Does not affect any properties not set on the input.")
+        .addLine(" */")
         .addLine("public %1$s mergeFrom(%1$s template) {", metadata.getBuilder());
-    if (hasRequiredProperties) {
-      code.addLine("  // Upcast to access the private _unsetProperties field.")
-          .addLine("  // Otherwise, oddly, we get an access violation.")
-          .addLine("  %s<%s> _templateUnset = ((%s) template)._unsetProperties;",
-              EnumSet.class,
-              metadata.getPropertyEnum(),
-              metadata.getGeneratedBuilder());
-    }
+    Block body = new Block(code);
     for (Property property : metadata.getProperties()) {
-      if (property.getCodeGenerator().getType() == Type.REQUIRED) {
-        code.addLine("  if (!_templateUnset.contains(%s.%s)) {",
-            metadata.getPropertyEnum(), property.getAllCapsName());
-        property.getCodeGenerator().addMergeFromBuilder(code, "template");
-        code.addLine("  }");
-      } else {
-        property.getCodeGenerator().addMergeFromBuilder(code, "template");
-      }
+      property.getCodeGenerator().addMergeFromBuilder(body, "template");
     }
-    code.addLine("  return (%s) this;", metadata.getBuilder());
-    code.addLine("}");
+    code.add(body)
+        .addLine("  return (%s) this;", metadata.getBuilder())
+        .addLine("}");
   }
 
   private static void addClearMethod(SourceBuilder code, Metadata metadata) {

diff --git a/src/main/java/org/inferred/freebuilder/processor/Declarations.java b/src/main/java/org/inferred/freebuilder/processor/Declarations.java
@@ -8,6 +8,23 @@
 
 class Declarations {
 
+  /**
+   * Upcasts a Builder instance to the generated superclass, to allow access to private fields.
+   *
+   * @param block the {@link Block} to add the declaration to
+   * @param metadata metadata about the builder being generated
+   * @param builder the Builder instance to upcast
+   * @returns an Excerpt referencing the upcasted instance
+   */
+  public static Excerpt upcastToGeneratedBuilder(Block block, Metadata metadata, String builder) {
+    return block.declare(
+        "base",
+        "// Upcast to access private fields; otherwise, oddly, we get an access violation.%n"
+            + "%1$s base = (%1$s) %2$s;",
+        metadata.getGeneratedBuilder(),
+        builder);
+  }
+
   /**
    * Declares a fresh Builder to copy default property values from.
    *

diff --git a/src/main/java/org/inferred/freebuilder/processor/DefaultPropertyFactory.java b/src/main/java/org/inferred/freebuilder/processor/DefaultPropertyFactory.java
@@ -173,8 +173,16 @@ public void addMergeFromValue(SourceBuilder code, String value) {
     }
 
     @Override
-    public void addMergeFromBuilder(SourceBuilder code, String builder) {
-      code.addLine("%s(%s.%s());", setter(property), builder, getter(property));
+    public void addMergeFromBuilder(Block code, String builder) {
+      if (!hasDefault) {
+        Excerpt base = Declarations.upcastToGeneratedBuilder(code, metadata, builder);
+        code.addLine("if (!%s._unsetProperties.contains(%s.%s)) {",
+            base, metadata.getPropertyEnum(), property.getAllCapsName());
+      }
+      code.addLine("  %s(%s.%s());", setter(property), builder, getter(property));
+      if (!hasDefault) {
+        code.addLine("}");
+      }
     }
 
     @Override

diff --git a/src/main/java/org/inferred/freebuilder/processor/ListMultimapPropertyFactory.java b/src/main/java/org/inferred/freebuilder/processor/ListMultimapPropertyFactory.java
@@ -374,7 +374,7 @@ public void addMergeFromValue(SourceBuilder code, String value) {
     }
 
     @Override
-    public void addMergeFromBuilder(SourceBuilder code, String builder) {
+    public void addMergeFromBuilder(Block code, String builder) {
       code.addLine("%s(((%s) %s).%s);",
           putAllMethod(property),
           metadata.getGeneratedBuilder(),

diff --git a/src/main/java/org/inferred/freebuilder/processor/ListPropertyFactory.java b/src/main/java/org/inferred/freebuilder/processor/ListPropertyFactory.java
@@ -285,7 +285,7 @@ public void addMergeFromValue(SourceBuilder code, String value) {
     }
 
     @Override
-    public void addMergeFromBuilder(SourceBuilder code, String builder) {
+    public void addMergeFromBuilder(Block code, String builder) {
       code.addLine("%s(((%s) %s).%s);",
           addAllMethod(property),
           metadata.getGeneratedBuilder(),

diff --git a/src/main/java/org/inferred/freebuilder/processor/MapPropertyFactory.java b/src/main/java/org/inferred/freebuilder/processor/MapPropertyFactory.java
@@ -308,7 +308,7 @@ public void addMergeFromValue(SourceBuilder code, String value) {
     }
 
     @Override
-    public void addMergeFromBuilder(SourceBuilder code, String builder) {
+    public void addMergeFromBuilder(Block code, String builder) {
       code.addLine("%s(((%s) %s).%s);",
           putAllMethod(property),
           metadata.getGeneratedBuilder(),

diff --git a/src/main/java/org/inferred/freebuilder/processor/MultisetPropertyFactory.java b/src/main/java/org/inferred/freebuilder/processor/MultisetPropertyFactory.java
@@ -316,7 +316,7 @@ public void addMergeFromValue(SourceBuilder code, String value) {
     }
 
     @Override
-    public void addMergeFromBuilder(SourceBuilder code, String builder) {
+    public void addMergeFromBuilder(Block code, String builder) {
       code.addLine("%s(((%s) %s).%s);",
           addAllMethod(property),
           metadata.getGeneratedBuilder(),

diff --git a/src/main/java/org/inferred/freebuilder/processor/NullablePropertyFactory.java b/src/main/java/org/inferred/freebuilder/processor/NullablePropertyFactory.java
@@ -168,7 +168,7 @@ public void addMergeFromValue(SourceBuilder code, String value) {
     }
 
     @Override
-    public void addMergeFromBuilder(SourceBuilder code, String builder) {
+    public void addMergeFromBuilder(Block code, String builder) {
       code.addLine("%s(%s.%s());", setter(property), builder, getter(property));
     }
 

diff --git a/src/main/java/org/inferred/freebuilder/processor/OptionalPropertyFactory.java b/src/main/java/org/inferred/freebuilder/processor/OptionalPropertyFactory.java
@@ -329,7 +329,7 @@ public void addMergeFromValue(SourceBuilder code, String value) {
     }
 
     @Override
-    public void addMergeFromBuilder(SourceBuilder code, String builder) {
+    public void addMergeFromBuilder(Block code, String builder) {
       String propertyValue = builder + "." + getter(property) + "()";
       optional.invokeIfPresent(code, propertyValue, setter(property));
     }

diff --git a/src/main/java/org/inferred/freebuilder/processor/PropertyCodeGenerator.java b/src/main/java/org/inferred/freebuilder/processor/PropertyCodeGenerator.java
@@ -120,7 +120,7 @@ public void addPartialFieldAssignment(SourceBuilder code, String finalField, Str
   public abstract void addMergeFromValue(SourceBuilder code, String value);
 
   /** Add a merge from builder for the property to the builder's source code. */
-  public abstract void addMergeFromBuilder(SourceBuilder code, String builder);
+  public abstract void addMergeFromBuilder(Block code, String builder);
 
   /** Adds method annotations for the value type getter method. */
   public void addGetterAnnotations(@SuppressWarnings("unused") SourceBuilder code) {}

diff --git a/src/main/java/org/inferred/freebuilder/processor/SetMultimapPropertyFactory.java b/src/main/java/org/inferred/freebuilder/processor/SetMultimapPropertyFactory.java
@@ -370,7 +370,7 @@ public void addMergeFromValue(SourceBuilder code, String value) {
     }
 
     @Override
-    public void addMergeFromBuilder(SourceBuilder code, String builder) {
+    public void addMergeFromBuilder(Block code, String builder) {
       code.addLine("%s(((%s) %s).%s);",
           putAllMethod(property),
           metadata.getGeneratedBuilder(),

diff --git a/src/main/java/org/inferred/freebuilder/processor/SetPropertyFactory.java b/src/main/java/org/inferred/freebuilder/processor/SetPropertyFactory.java
@@ -304,7 +304,7 @@ public void addMergeFromValue(SourceBuilder code, String value) {
     }
 
     @Override
-    public void addMergeFromBuilder(SourceBuilder code, String builder) {
+    public void addMergeFromBuilder(Block code, String builder) {
       code.addLine("%s(((%s) %s).%s);",
           addAllMethod(property),
           metadata.getGeneratedBuilder(),

diff --git a/src/test/java/org/inferred/freebuilder/processor/DefaultSourceTest.java b/src/test/java/org/inferred/freebuilder/processor/DefaultSourceTest.java
@@ -145,14 +145,12 @@ public void testRequiredProperties_j6() {
         "   * Does not affect any properties not set on the input.",
         "   */",
         "  public Person.Builder mergeFrom(Person.Builder template) {",
-        "    // Upcast to access the private _unsetProperties field.",
-        "    // Otherwise, oddly, we get an access violation.",
-        "    EnumSet<Person_Builder.Property> _templateUnset = "
-            + "((Person_Builder) template)._unsetProperties;",
-        "    if (!_templateUnset.contains(Person_Builder.Property.NAME)) {",
+        "    // Upcast to access private fields; otherwise, oddly, we get an access violation.",
+        "    Person_Builder base = (Person_Builder) template;",
+        "    if (!base._unsetProperties.contains(Person_Builder.Property.NAME)) {",
         "      setName(template.getName());",
         "    }",
-        "    if (!_templateUnset.contains(Person_Builder.Property.AGE)) {",
+        "    if (!base._unsetProperties.contains(Person_Builder.Property.AGE)) {",
         "      setAge(template.getAge());",
         "    }",
         "    return (Person.Builder) this;",
@@ -371,6 +369,7 @@ public void testNoRequiredProperties_j6() {
         "",
         "  /**",
         "   * Copies values from the given {@code Builder}.",
+        "   * Does not affect any properties not set on the input.",
         "   */",
         "  public Person.Builder mergeFrom(Person.Builder template) {",
         "    setName(template.getName());",
@@ -603,14 +602,12 @@ public void testGenericType_j6() {
         "   * Does not affect any properties not set on the input.",
         "   */",
         "  public Person.Builder<A, B> mergeFrom(Person.Builder<A, B> template) {",
-        "    // Upcast to access the private _unsetProperties field.",
-        "    // Otherwise, oddly, we get an access violation.",
-        "    EnumSet<Person_Builder.Property> _templateUnset =",
-        "        ((Person_Builder<A, B>) template)._unsetProperties;",
-        "    if (!_templateUnset.contains(Person_Builder.Property.NAME)) {",
+        "    // Upcast to access private fields; otherwise, oddly, we get an access violation.",
+        "    Person_Builder<A, B> base = (Person_Builder<A, B>) template;",
+        "    if (!base._unsetProperties.contains(Person_Builder.Property.NAME)) {",
         "      setName(template.getName());",
         "    }",
-        "    if (!_templateUnset.contains(Person_Builder.Property.AGE)) {",
+        "    if (!base._unsetProperties.contains(Person_Builder.Property.AGE)) {",
         "      setAge(template.getAge());",
         "    }",
         "    return (Person.Builder<A, B>) this;",
@@ -862,14 +859,12 @@ public void testRequiredProperties_j7() {
         "   * Does not affect any properties not set on the input.",
         "   */",
         "  public Person.Builder mergeFrom(Person.Builder template) {",
-        "    // Upcast to access the private _unsetProperties field.",
-        "    // Otherwise, oddly, we get an access violation.",
-        "    EnumSet<Person_Builder.Property> _templateUnset = ((Person_Builder) template)"
-            + "._unsetProperties;",
-        "    if (!_templateUnset.contains(Person_Builder.Property.NAME)) {",
+        "    // Upcast to access private fields; otherwise, oddly, we get an access violation.",
+        "    Person_Builder base = (Person_Builder) template;",
+        "    if (!base._unsetProperties.contains(Person_Builder.Property.NAME)) {",
         "      setName(template.getName());",
         "    }",
-        "    if (!_templateUnset.contains(Person_Builder.Property.AGE)) {",
+        "    if (!base._unsetProperties.contains(Person_Builder.Property.AGE)) {",
         "      setAge(template.getAge());",
         "    }",
         "    return (Person.Builder) this;",
@@ -1079,6 +1074,7 @@ public void testNoRequiredProperties_j7() {
         "",
         "  /**",
         "   * Copies values from the given {@code Builder}.",
+        "   * Does not affect any properties not set on the input.",
         "   */",
         "  public Person.Builder mergeFrom(Person.Builder template) {",
         "    setName(template.getName());",
@@ -1300,14 +1296,12 @@ public void testGenericType_j7() {
         "   * Does not affect any properties not set on the input.",
         "   */",
         "  public Person.Builder<A, B> mergeFrom(Person.Builder<A, B> template) {",
-        "    // Upcast to access the private _unsetProperties field.",
-        "    // Otherwise, oddly, we get an access violation.",
-        "    EnumSet<Person_Builder.Property> _templateUnset =",
-        "        ((Person_Builder<A, B>) template)._unsetProperties;",
-        "    if (!_templateUnset.contains(Person_Builder.Property.NAME)) {",
+        "    // Upcast to access private fields; otherwise, oddly, we get an access violation.",
+        "    Person_Builder<A, B> base = (Person_Builder<A, B>) template;",
+        "    if (!base._unsetProperties.contains(Person_Builder.Property.NAME)) {",
         "      setName(template.getName());",
         "    }",
-        "    if (!_templateUnset.contains(Person_Builder.Property.AGE)) {",
+        "    if (!base._unsetProperties.contains(Person_Builder.Property.AGE)) {",
         "      setAge(template.getAge());",
         "    }",
         "    return (Person.Builder<A, B>) this;",
@@ -1551,14 +1545,12 @@ public void testRequiredProperties_noGuava_j6() {
         "   * Does not affect any properties not set on the input.",
         "   */",
         "  public Person.Builder mergeFrom(Person.Builder template) {",
-        "    // Upcast to access the private _unsetProperties field.",
-        "    // Otherwise, oddly, we get an access violation.",
-        "    EnumSet<Person_Builder.Property> _templateUnset = ((Person_Builder) template)"
-            + "._unsetProperties;",
-        "    if (!_templateUnset.contains(Person_Builder.Property.NAME)) {",
+        "    // Upcast to access private fields; otherwise, oddly, we get an access violation.",
+        "    Person_Builder base = (Person_Builder) template;",
+        "    if (!base._unsetProperties.contains(Person_Builder.Property.NAME)) {",
         "      setName(template.getName());",
         "    }",
-        "    if (!_templateUnset.contains(Person_Builder.Property.AGE)) {",
+        "    if (!base._unsetProperties.contains(Person_Builder.Property.AGE)) {",
         "      setAge(template.getAge());",
         "    }",
         "    return (Person.Builder) this;",
@@ -1814,14 +1806,12 @@ public void testRequiredProperties_noGuava_j7() {
         "   * Does not affect any properties not set on the input.",
         "   */",
         "  public Person.Builder mergeFrom(Person.Builder template) {",
-        "    // Upcast to access the private _unsetProperties field.",
-        "    // Otherwise, oddly, we get an access violation.",
-        "    EnumSet<Person_Builder.Property> _templateUnset = ((Person_Builder) template)"
-            + "._unsetProperties;",
-        "    if (!_templateUnset.contains(Person_Builder.Property.NAME)) {",
+        "    // Upcast to access private fields; otherwise, oddly, we get an access violation.",
+        "    Person_Builder base = (Person_Builder) template;",
+        "    if (!base._unsetProperties.contains(Person_Builder.Property.NAME)) {",
         "      setName(template.getName());",
         "    }",
-        "    if (!_templateUnset.contains(Person_Builder.Property.AGE)) {",
+        "    if (!base._unsetProperties.contains(Person_Builder.Property.AGE)) {",
         "      setAge(template.getAge());",
         "    }",
         "    return (Person.Builder) this;",
@@ -2036,6 +2026,7 @@ public void testNoRequiredProperties_noGuava_j6() {
         "",
         "  /**",
         "   * Copies values from the given {@code Builder}.",
+        "   * Does not affect any properties not set on the input.",
         "   */",
         "  public Person.Builder mergeFrom(Person.Builder template) {",
         "    setName(template.getName());",
@@ -2349,15 +2340,13 @@ public void testOneDefaultProperty_noGuava_j6() {
         "   * Does not affect any properties not set on the input.",
         "   */",
         "  public Person.Builder mergeFrom(Person.Builder template) {",
-        "    // Upcast to access the private _unsetProperties field.",
-        "    // Otherwise, oddly, we get an access violation.",
-        "    EnumSet<Person_Builder.Property> _templateUnset = ((Person_Builder) template)"
-            + "._unsetProperties;",
-        "    if (!_templateUnset.contains(Person_Builder.Property.NAME)) {",
+        "    // Upcast to access private fields; otherwise, oddly, we get an access violation.",
+        "    Person_Builder base = (Person_Builder) template;",
+        "    if (!base._unsetProperties.contains(Person_Builder.Property.NAME)) {",
         "      setName(template.getName());",
         "    }",
         "    setAge(template.getAge());",
-        "    if (!_templateUnset.contains(Person_Builder.Property.SHOE_SIZE)) {",
+        "    if (!base._unsetProperties.contains(Person_Builder.Property.SHOE_SIZE)) {",
         "      setShoeSize(template.getShoeSize());",
         "    }",
         "    return (Person.Builder) this;",
@@ -2666,14 +2655,12 @@ public void testRequiredProperties_j8() {
         "   * Does not affect any properties not set on the input.",
         "   */",
         "  public Person.Builder mergeFrom(Person.Builder template) {",
-        "    // Upcast to access the private _unsetProperties field.",
-        "    // Otherwise, oddly, we get an access violation.",
-        "    EnumSet<Person_Builder.Property> _templateUnset = ((Person_Builder) template)"
-            + "._unsetProperties;",
-        "    if (!_templateUnset.contains(Person_Builder.Property.NAME)) {",
+        "    // Upcast to access private fields; otherwise, oddly, we get an access violation.",
+        "    Person_Builder base = (Person_Builder) template;",
+        "    if (!base._unsetProperties.contains(Person_Builder.Property.NAME)) {",
         "      setName(template.getName());",
         "    }",
-        "    if (!_templateUnset.contains(Person_Builder.Property.AGE)) {",
+        "    if (!base._unsetProperties.contains(Person_Builder.Property.AGE)) {",
         "      setAge(template.getAge());",
         "    }",
         "    return (Person.Builder) this;",
@@ -2909,6 +2896,7 @@ public void testNoRequiredProperties_j8() {
         "",
         "  /**",
         "   * Copies values from the given {@code Builder}.",
+        "   * Does not affect any properties not set on the input.",
         "   */",
         "  public Person.Builder mergeFrom(Person.Builder template) {",
         "    setName(template.getName());",
@@ -3160,14 +3148,12 @@ public void testGenericType_j8() {
         "   * Does not affect any properties not set on the input.",
         "   */",
         "  public Person.Builder<A, B> mergeFrom(Person.Builder<A, B> template) {",
-        "    // Upcast to access the private _unsetProperties field.",
-        "    // Otherwise, oddly, we get an access violation.",
-        "    EnumSet<Person_Builder.Property> _templateUnset =",
-        "        ((Person_Builder<A, B>) template)._unsetProperties;",
-        "    if (!_templateUnset.contains(Person_Builder.Property.NAME)) {",
+        "    // Upcast to access private fields; otherwise, oddly, we get an access violation.",
+        "    Person_Builder<A, B> base = (Person_Builder<A, B>) template;",
+        "    if (!base._unsetProperties.contains(Person_Builder.Property.NAME)) {",
         "      setName(template.getName());",
         "    }",
-        "    if (!_templateUnset.contains(Person_Builder.Property.AGE)) {",
+        "    if (!base._unsetProperties.contains(Person_Builder.Property.AGE)) {",
         "      setAge(template.getAge());",
         "    }",
         "    return (Person.Builder<A, B>) this;",