From d11263a6c60008894213a1b465f2b16be3a03468 Mon Sep 17 00:00:00 2001 From: Jan Rieke Date: Fri, 1 Jun 2018 13:48:38 +0200 Subject: support @Builder.Default --- .../eclipse/handlers/HandleBuilderDefault.java | 6 +- .../eclipse/handlers/HandleSuperBuilder.java | 40 ++++--- .../javac/handlers/HandleBuilderDefault.java | 6 +- .../lombok/javac/handlers/HandleSuperBuilder.java | 42 ++++---- .../after-delombok/SuperBuilderWithDefaults.java | 119 +++++++++++++++++++++ .../after-ecj/SuperBuilderWithDefaults.java | 97 +++++++++++++++++ .../resource/before/SuperBuilderWithDefaults.java | 18 ++++ .../features/experimental/SuperBuilder.html | 4 +- 8 files changed, 290 insertions(+), 42 deletions(-) create mode 100644 test/transform/resource/after-delombok/SuperBuilderWithDefaults.java create mode 100644 test/transform/resource/after-ecj/SuperBuilderWithDefaults.java create mode 100644 test/transform/resource/before/SuperBuilderWithDefaults.java diff --git a/src/core/lombok/eclipse/handlers/HandleBuilderDefault.java b/src/core/lombok/eclipse/handlers/HandleBuilderDefault.java index be2b986d..d0c597fd 100644 --- a/src/core/lombok/eclipse/handlers/HandleBuilderDefault.java +++ b/src/core/lombok/eclipse/handlers/HandleBuilderDefault.java @@ -31,6 +31,7 @@ import lombok.core.AnnotationValues; import lombok.core.HandlerPriority; import lombok.eclipse.EclipseAnnotationHandler; import lombok.eclipse.EclipseNode; +import lombok.experimental.SuperBuilder; @ProviderFor(EclipseAnnotationHandler.class) @HandlerPriority(-1025) //HandleBuilder's level, minus one. @@ -39,8 +40,9 @@ public class HandleBuilderDefault extends EclipseAnnotationHandler { private static final char[] CLEAN_FIELD_NAME = "$lombokUnclean".toCharArray(); private static final char[] CLEAN_METHOD_NAME = "$lombokClean".toCharArray(); - private static final char[] DEFAULT_PREFIX = {'$', 'd', 'e', 'f', 'a', 'u', 'l', 't', '$'}; - private static final char[] SET_PREFIX = {'$', 's', 'e', 't'}; - private static final String SELF_METHOD = "self"; + private static final char[] SET_PREFIX = "$set".toCharArray(); + private static final char[] SELF_METHOD_NAME = "self".toCharArray(); private static final AbstractMethodDeclaration[] EMPTY_METHODS = {}; @@ -103,7 +103,6 @@ public class HandleSuperBuilder extends EclipseAnnotationHandler { TypeReference type; char[] rawName; char[] name; - char[] nameOfDefaultProvider; char[] nameOfSetFlag; SingularData singularData; ObtainVia obtainVia; @@ -185,13 +184,16 @@ public class HandleSuperBuilder extends EclipseAnnotationHandler { } if (isDefault != null) { - bfd.nameOfDefaultProvider = prefixWith(DEFAULT_PREFIX, bfd.name); bfd.nameOfSetFlag = prefixWith(bfd.name, SET_PREFIX); - - MethodDeclaration md = HandleBuilder.generateDefaultProvider(bfd.nameOfDefaultProvider, td.typeParameters, fieldNode, ast); - if (md != null) { - injectMethod(tdParent, md); - } + // The @Builder annotation removes the initializing expression on the field and moves + // it to a method called "$default$FIELDNAME". This method is then called upon building. + // We do NOT do this, because this is unexpected and may lead to bugs when using other + // constructors (see, e.g., issue #1347). + // Instead, we keep the init expression and only set a new value in the builder-based + // constructor if it was set in the builder. Drawback is that the init expression is + // always executed, even if it was unnecessary because its value is overwritten by the + // builder. + // TODO: Once the issue is resolved in @Builder, we can adapt the solution here. } addObtainVia(bfd, fieldNode); builderFields.add(bfd); @@ -481,9 +483,17 @@ public class HandleSuperBuilder extends EclipseAnnotationHandler { long[] positions = new long[] {p, p}; assignmentExpr = new QualifiedNameReference(variableInBuilder, positions, s, e); } - - Assignment assignment = new Assignment(thisX, assignmentExpr, (int) p); + Statement assignment = new Assignment(thisX, assignmentExpr, (int) p); + + // In case of @Builder.Default, only set the value if it really was set in the builder. + if (fieldNode.nameOfSetFlag != null) { + char[][] variableInBuilder = new char[][] {"b".toCharArray(), fieldNode.nameOfSetFlag}; + long[] positions = new long[] {p, p}; + QualifiedNameReference builderRef = new QualifiedNameReference(variableInBuilder, positions, s, e); + assignment = new IfStatement(builderRef, assignment, s, e); + } statements.add(assignment); + Annotation[] nonNulls = findAnnotations((FieldDeclaration)fieldNode.originalFieldNode.get(), NON_NULL_PATTERN); if (nonNulls.length != 0) { Statement nullCheck = generateNullCheck((FieldDeclaration)fieldNode.originalFieldNode.get(), sourceNode); @@ -527,7 +537,7 @@ public class HandleSuperBuilder extends EclipseAnnotationHandler { private MethodDeclaration generateAbstractSelfMethod(EclipseNode tdParent, boolean override, String builderGenericName) { MethodDeclaration out = new MethodDeclaration(((CompilationUnitDeclaration) tdParent.top().get()).compilationResult); - out.selector = SELF_METHOD.toCharArray(); + out.selector = SELF_METHOD_NAME; out.bits |= ECLIPSE_DO_NOT_TOUCH_FLAG; out.modifiers = ClassFileConstants.AccAbstract | ClassFileConstants.AccProtected | ExtraCompilerModifiers.AccSemicolonBody; if (override) { @@ -539,7 +549,7 @@ public class HandleSuperBuilder extends EclipseAnnotationHandler { private MethodDeclaration generateSelfMethod(EclipseNode builderImplType) { MethodDeclaration out = new MethodDeclaration(((CompilationUnitDeclaration) builderImplType.top().get()).compilationResult); - out.selector = SELF_METHOD.toCharArray(); + out.selector = SELF_METHOD_NAME; out.bits |= ECLIPSE_DO_NOT_TOUCH_FLAG; out.modifiers = ClassFileConstants.AccProtected; out.annotations = new Annotation[] {makeMarkerAnnotation(TypeConstants.JAVA_LANG_OVERRIDE, builderImplType.get())}; @@ -665,7 +675,7 @@ public class HandleSuperBuilder extends EclipseAnnotationHandler { @Override public ReturnStatement get() { MessageSend returnCall = new MessageSend(); returnCall.receiver = ThisReference.implicitThis(); - returnCall.selector = SELF_METHOD.toCharArray(); + returnCall.selector = SELF_METHOD_NAME; return new ReturnStatement(returnCall, 0, 0); } }; diff --git a/src/core/lombok/javac/handlers/HandleBuilderDefault.java b/src/core/lombok/javac/handlers/HandleBuilderDefault.java index 4c4ba0e8..af45a620 100644 --- a/src/core/lombok/javac/handlers/HandleBuilderDefault.java +++ b/src/core/lombok/javac/handlers/HandleBuilderDefault.java @@ -31,6 +31,7 @@ import lombok.Builder; import lombok.core.AST.Kind; import lombok.core.AnnotationValues; import lombok.core.HandlerPriority; +import lombok.experimental.SuperBuilder; import lombok.javac.JavacAnnotationHandler; import lombok.javac.JavacNode; @@ -41,8 +42,9 @@ public class HandleBuilderDefault extends JavacAnnotationHandler { JCExpression type; Name rawName; Name name; - Name nameOfDefaultProvider; Name nameOfSetFlag; SingularData singularData; ObtainVia obtainVia; @@ -164,13 +164,16 @@ public class HandleSuperBuilder extends JavacAnnotationHandler { } if (isDefault != null) { - bfd.nameOfDefaultProvider = tdParent.toName("$default$" + bfd.name); bfd.nameOfSetFlag = tdParent.toName(bfd.name + "$set"); - JCMethodDecl md = generateDefaultProvider(bfd.nameOfDefaultProvider, fieldNode, td.typarams); - recursiveSetGeneratedBy(md, ast, annotationNode.getContext()); - if (md != null) { - injectMethod(tdParent, md); - } + // The @Builder annotation removes the initializing expression on the field and moves + // it to a method called "$default$FIELDNAME". This method is then called upon building. + // We do NOT do this, because this is unexpected and may lead to bugs when using other + // constructors (see, e.g., issue #1347). + // Instead, we keep the init expression and only set a new value in the builder-based + // constructor if it was set in the builder. Drawback is that the init expression is + // always executed, even if it was unnecessary because its value is overwritten by the + // builder. + // TODO: Once the issue is resolved in @Builder, we can adapt the solution here. } addObtainVia(bfd, fieldNode); builderFields.add(bfd); @@ -435,9 +438,16 @@ public class HandleSuperBuilder extends JavacAnnotationHandler { } JCFieldAccess thisX = maker.Select(maker.Ident(typeNode.toName("this")), bfd.rawName); - JCExpression assign = maker.Assign(thisX, rhs); - - statements.append(maker.Exec(assign)); + JCStatement assign = maker.Exec(maker.Assign(thisX, rhs)); + + // In case of @Builder.Default, only set the value if it really was set in the builder. + if (bfd.nameOfSetFlag != null) { + JCFieldAccess setField = maker.Select(maker.Ident(builderVariableName), bfd.nameOfSetFlag); + JCIf ifSet = maker.If(setField, assign, null); + statements.append(ifSet); + } else { + statements.append(assign); + } } JCModifiers mods = maker.Modifiers(toJavacModifier(level), List.nil()); @@ -575,18 +585,6 @@ public class HandleSuperBuilder extends JavacAnnotationHandler { return maker.MethodDef(maker.Modifiers(Flags.PUBLIC), type.toName("$lombokClean"), maker.Type(Javac.createVoidType(type.getSymbolTable(), CTC_VOID)), List.nil(), List.nil(), List.nil(), body, null); } - private JCMethodDecl generateDefaultProvider(Name methodName, JavacNode fieldNode, List params) { - JavacTreeMaker maker = fieldNode.getTreeMaker(); - JCVariableDecl field = (JCVariableDecl) fieldNode.get(); - - JCStatement statement = maker.Return(field.init); - field.init = null; - - JCBlock body = maker.Block(0, List.of(statement)); - int modifiers = Flags.PRIVATE | Flags.STATIC; - return maker.MethodDef(maker.Modifiers(modifiers), methodName, cloneType(maker, field.vartype, field, fieldNode.getContext()), copyTypeParams(fieldNode, params), List.nil(), List.nil(), body, null); - } - private void generateBuilderFields(JavacNode builderType, java.util.List builderFields, JCTree source) { int len = builderFields.size(); java.util.List existing = new ArrayList(); diff --git a/test/transform/resource/after-delombok/SuperBuilderWithDefaults.java b/test/transform/resource/after-delombok/SuperBuilderWithDefaults.java new file mode 100644 index 00000000..7b6b4578 --- /dev/null +++ b/test/transform/resource/after-delombok/SuperBuilderWithDefaults.java @@ -0,0 +1,119 @@ +import java.util.List; +public class SuperBuilderWithDefaults { + public static class Parent { + private long millis = System.currentTimeMillis(); + private N numberField = null; + @java.lang.SuppressWarnings("all") + public static abstract class ParentBuilder, B extends ParentBuilder> { + @java.lang.SuppressWarnings("all") + private boolean millis$set; + @java.lang.SuppressWarnings("all") + private long millis; + @java.lang.SuppressWarnings("all") + private boolean numberField$set; + @java.lang.SuppressWarnings("all") + private N numberField; + @java.lang.SuppressWarnings("all") + protected abstract B self(); + @java.lang.SuppressWarnings("all") + public abstract C build(); + @java.lang.SuppressWarnings("all") + public B millis(final long millis) { + this.millis = millis; + millis$set = true; + return self(); + } + @java.lang.SuppressWarnings("all") + public B numberField(final N numberField) { + this.numberField = numberField; + numberField$set = true; + return self(); + } + @java.lang.Override + @java.lang.SuppressWarnings("all") + public java.lang.String toString() { + return "SuperBuilderWithDefaults.Parent.ParentBuilder(millis=" + this.millis + ", numberField=" + this.numberField + ")"; + } + } + @java.lang.SuppressWarnings("all") + private static final class ParentBuilderImpl extends ParentBuilder, ParentBuilderImpl> { + @java.lang.SuppressWarnings("all") + private ParentBuilderImpl() { + } + @java.lang.Override + @java.lang.SuppressWarnings("all") + protected ParentBuilderImpl self() { + return this; + } + @java.lang.Override + @java.lang.SuppressWarnings("all") + public Parent build() { + return new Parent(this); + } + } + @java.lang.SuppressWarnings("all") + protected Parent(final ParentBuilder b) { + if (b.millis$set) this.millis = b.millis; + if (b.numberField$set) this.numberField = b.numberField; + } + @java.lang.SuppressWarnings("all") + public static ParentBuilder builder() { + return new ParentBuilderImpl(); + } + } + public static class Child extends Parent { + private double doubleField = Math.PI; + @java.lang.SuppressWarnings("all") + public static abstract class ChildBuilder> extends Parent.ParentBuilder { + @java.lang.SuppressWarnings("all") + private boolean doubleField$set; + @java.lang.SuppressWarnings("all") + private double doubleField; + @java.lang.Override + @java.lang.SuppressWarnings("all") + protected abstract B self(); + @java.lang.Override + @java.lang.SuppressWarnings("all") + public abstract C build(); + @java.lang.SuppressWarnings("all") + public B doubleField(final double doubleField) { + this.doubleField = doubleField; + doubleField$set = true; + return self(); + } + @java.lang.Override + @java.lang.SuppressWarnings("all") + public java.lang.String toString() { + return "SuperBuilderWithDefaults.Child.ChildBuilder(super=" + super.toString() + ", doubleField=" + this.doubleField + ")"; + } + } + @java.lang.SuppressWarnings("all") + private static final class ChildBuilderImpl extends ChildBuilder { + @java.lang.SuppressWarnings("all") + private ChildBuilderImpl() { + } + @java.lang.Override + @java.lang.SuppressWarnings("all") + protected ChildBuilderImpl self() { + return this; + } + @java.lang.Override + @java.lang.SuppressWarnings("all") + public Child build() { + return new Child(this); + } + } + @java.lang.SuppressWarnings("all") + protected Child(final ChildBuilder b) { + super(b); + if (b.doubleField$set) this.doubleField = b.doubleField; + } + @java.lang.SuppressWarnings("all") + public static ChildBuilder builder() { + return new ChildBuilderImpl(); + } + } + public static void test() { + Child x = Child.builder().doubleField(0.1).numberField(5).millis(1234567890L).build(); + } +} diff --git a/test/transform/resource/after-ecj/SuperBuilderWithDefaults.java b/test/transform/resource/after-ecj/SuperBuilderWithDefaults.java new file mode 100644 index 00000000..f0e3d8be --- /dev/null +++ b/test/transform/resource/after-ecj/SuperBuilderWithDefaults.java @@ -0,0 +1,97 @@ +import java.util.List; +public class SuperBuilderWithDefaults { + public static @lombok.experimental.SuperBuilder class Parent { + public static abstract @java.lang.SuppressWarnings("all") class ParentBuilder, B extends ParentBuilder> { + private @java.lang.SuppressWarnings("all") long millis; + private @java.lang.SuppressWarnings("all") boolean millis$set; + private @java.lang.SuppressWarnings("all") N numberField; + private @java.lang.SuppressWarnings("all") boolean numberField$set; + public ParentBuilder() { + super(); + } + protected abstract @java.lang.SuppressWarnings("all") B self(); + public abstract @java.lang.SuppressWarnings("all") C build(); + public @java.lang.SuppressWarnings("all") B millis(final long millis) { + this.millis = millis; + millis$set = true; + return self(); + } + public @java.lang.SuppressWarnings("all") B numberField(final N numberField) { + this.numberField = numberField; + numberField$set = true; + return self(); + } + public @java.lang.Override @java.lang.SuppressWarnings("all") java.lang.String toString() { + return (((("SuperBuilderWithDefaults.Parent.ParentBuilder(millis=" + this.millis) + ", numberField=") + this.numberField) + ")"); + } + } + private static final @java.lang.SuppressWarnings("all") class ParentBuilderImpl extends ParentBuilder, ParentBuilderImpl> { + private ParentBuilderImpl() { + super(); + } + protected @java.lang.Override @java.lang.SuppressWarnings("all") ParentBuilderImpl self() { + return this; + } + public @java.lang.Override @java.lang.SuppressWarnings("all") Parent build() { + return new Parent(this); + } + } + private @lombok.Builder.Default long millis = System.currentTimeMillis(); + private @lombok.Builder.Default N numberField = null; + protected @java.lang.SuppressWarnings("all") Parent(final ParentBuilder b) { + super(); + if (b.millis$set) + this.millis = b.millis; + if (b.numberField$set) + this.numberField = b.numberField; + } + public static @java.lang.SuppressWarnings("all") ParentBuilder builder() { + return new ParentBuilderImpl(); + } + } + public static @lombok.experimental.SuperBuilder class Child extends Parent { + public static abstract @java.lang.SuppressWarnings("all") class ChildBuilder> extends Parent.ParentBuilder { + private @java.lang.SuppressWarnings("all") double doubleField; + private @java.lang.SuppressWarnings("all") boolean doubleField$set; + public ChildBuilder() { + super(); + } + protected abstract @java.lang.Override @java.lang.SuppressWarnings("all") B self(); + public abstract @java.lang.Override @java.lang.SuppressWarnings("all") C build(); + public @java.lang.SuppressWarnings("all") B doubleField(final double doubleField) { + this.doubleField = doubleField; + doubleField$set = true; + return self(); + } + public @java.lang.Override @java.lang.SuppressWarnings("all") java.lang.String toString() { + return (((("SuperBuilderWithDefaults.Child.ChildBuilder(super=" + super.toString()) + ", doubleField=") + this.doubleField) + ")"); + } + } + private static final @java.lang.SuppressWarnings("all") class ChildBuilderImpl extends ChildBuilder { + private ChildBuilderImpl() { + super(); + } + protected @java.lang.Override @java.lang.SuppressWarnings("all") ChildBuilderImpl self() { + return this; + } + public @java.lang.Override @java.lang.SuppressWarnings("all") Child build() { + return new Child(this); + } + } + private @lombok.Builder.Default double doubleField = Math.PI; + protected @java.lang.SuppressWarnings("all") Child(final ChildBuilder b) { + super(b); + if (b.doubleField$set) + this.doubleField = b.doubleField; + } + public static @java.lang.SuppressWarnings("all") ChildBuilder builder() { + return new ChildBuilderImpl(); + } + } + public SuperBuilderWithDefaults() { + super(); + } + public static void test() { + Child x = Child.builder().doubleField(0.1).numberField(5).millis(1234567890L).build(); + } +} diff --git a/test/transform/resource/before/SuperBuilderWithDefaults.java b/test/transform/resource/before/SuperBuilderWithDefaults.java new file mode 100644 index 00000000..d6859a78 --- /dev/null +++ b/test/transform/resource/before/SuperBuilderWithDefaults.java @@ -0,0 +1,18 @@ +import java.util.List; + +public class SuperBuilderWithDefaults { + @lombok.experimental.SuperBuilder + public static class Parent { + @lombok.Builder.Default private long millis = System.currentTimeMillis(); + @lombok.Builder.Default private N numberField = null; + } + + @lombok.experimental.SuperBuilder + public static class Child extends Parent { + @lombok.Builder.Default private double doubleField = Math.PI; + } + + public static void test() { + Child x = Child.builder().doubleField(0.1).numberField(5).millis(1234567890L).build(); + } +} diff --git a/website/templates/features/experimental/SuperBuilder.html b/website/templates/features/experimental/SuperBuilder.html index a1e8cb4d..a36ae499 100644 --- a/website/templates/features/experimental/SuperBuilder.html +++ b/website/templates/features/experimental/SuperBuilder.html @@ -23,6 +23,8 @@ Therefore, a @SuperBuilder must extend the @SuperBuilder of the superclass in order to include those methods. Furthermore, the generated builder code heavily relies on generics to avoid class casting when using the builder. @SuperBuilder generates a private constructor on the class that takes a builder instances as a parameter. This constructor sets the fields of the new instance to the values from the builder. +

+ @SuperBuilder is not compatible with @Builder.

To ensure type-safety, @SuperBuilder generates two inner builder classes for each annotated class, one abstract and one concrete class named FoobarBuilder and FoobarBuilderImpl (where Foobar is the name of the annotated class).

@@ -43,7 +45,7 @@ <@f.confKeys>

- lombok.builder.flagUsage = [warning | error] (default: not set) + lombok.superBuilder.flagUsage = [warning | error] (default: not set)
Lombok will flag any usage of @SuperBuilder as a warning or error if configured.
-- cgit