From dbc173ba5c575eea404d3a76b3bc24d35656767c Mon Sep 17 00:00:00 2001 From: Jan Rieke Date: Thu, 16 Aug 2018 14:38:44 +0200 Subject: @SuperBuilder adapts @Builder.Default behavior from @Builder as #1347 is fixed now (javac) --- src/core/lombok/javac/handlers/HandleBuilder.java | 2 +- .../lombok/javac/handlers/HandleSuperBuilder.java | 31 ++++++++++------------ 2 files changed, 15 insertions(+), 18 deletions(-) (limited to 'src/core/lombok/javac') diff --git a/src/core/lombok/javac/handlers/HandleBuilder.java b/src/core/lombok/javac/handlers/HandleBuilder.java index f04ea8b1..a6ba5d0a 100644 --- a/src/core/lombok/javac/handlers/HandleBuilder.java +++ b/src/core/lombok/javac/handlers/HandleBuilder.java @@ -595,7 +595,7 @@ public class HandleBuilder extends JavacAnnotationHandler { return maker.MethodDef(maker.Modifiers(Flags.PUBLIC), type.toName(buildName), returnType, List.nil(), List.nil(), thrownExceptions, body, null); } - public JCMethodDecl generateDefaultProvider(Name methodName, JavacNode fieldNode, List params) { + public static JCMethodDecl generateDefaultProvider(Name methodName, JavacNode fieldNode, List params) { JavacTreeMaker maker = fieldNode.getTreeMaker(); JCVariableDecl field = (JCVariableDecl) fieldNode.get(); diff --git a/src/core/lombok/javac/handlers/HandleSuperBuilder.java b/src/core/lombok/javac/handlers/HandleSuperBuilder.java index 38fec2f6..c00bd66d 100644 --- a/src/core/lombok/javac/handlers/HandleSuperBuilder.java +++ b/src/core/lombok/javac/handlers/HandleSuperBuilder.java @@ -33,12 +33,12 @@ import com.sun.tools.javac.code.BoundKind; import com.sun.tools.javac.code.Flags; import com.sun.tools.javac.tree.JCTree; import com.sun.tools.javac.tree.JCTree.JCAnnotation; +import com.sun.tools.javac.tree.JCTree.JCAssign; import com.sun.tools.javac.tree.JCTree.JCBlock; import com.sun.tools.javac.tree.JCTree.JCClassDecl; import com.sun.tools.javac.tree.JCTree.JCExpression; import com.sun.tools.javac.tree.JCTree.JCFieldAccess; import com.sun.tools.javac.tree.JCTree.JCIdent; -import com.sun.tools.javac.tree.JCTree.JCIf; import com.sun.tools.javac.tree.JCTree.JCMethodDecl; import com.sun.tools.javac.tree.JCTree.JCMethodInvocation; import com.sun.tools.javac.tree.JCTree.JCModifiers; @@ -83,6 +83,7 @@ public class HandleSuperBuilder extends JavacAnnotationHandler { JCExpression type; Name rawName; Name name; + Name nameOfDefaultProvider; Name nameOfSetFlag; SingularData singularData; ObtainVia obtainVia; @@ -154,16 +155,12 @@ public class HandleSuperBuilder extends JavacAnnotationHandler { } if (isDefault != null) { + bfd.nameOfDefaultProvider = tdParent.toName("$default$" + bfd.name); bfd.nameOfSetFlag = tdParent.toName(bfd.name + "$set"); - // 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. + bfd.nameOfSetFlag = tdParent.toName(bfd.name + "$set"); + JCMethodDecl md = HandleBuilder.generateDefaultProvider(bfd.nameOfDefaultProvider, fieldNode, td.typarams); + recursiveSetGeneratedBy(md, ast, annotationNode.getContext()); + if (md != null) injectMethod(tdParent, md); } addObtainVia(bfd, fieldNode); builderFields.add(bfd); @@ -427,17 +424,17 @@ public class HandleSuperBuilder extends JavacAnnotationHandler { } else { rhs = maker.Select(maker.Ident(builderVariableName), bfd.rawName); } - JCFieldAccess thisX = maker.Select(maker.Ident(typeNode.toName("this")), bfd.rawName); + JCFieldAccess fieldInThis = maker.Select(maker.Ident(typeNode.toName("this")), bfd.rawName); - JCStatement assign = maker.Exec(maker.Assign(thisX, rhs)); + JCStatement assign = maker.Exec(maker.Assign(fieldInThis, rhs)); + statements.append(assign); - // In case of @Builder.Default, only set the value if it really was set in the builder. + // In case of @Builder.Default, set the value to the default if it was NOT 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); + fieldInThis = maker.Select(maker.Ident(typeNode.toName("this")), bfd.rawName); + JCAssign assignDefault = maker.Assign(fieldInThis, maker.Apply(typeParameterNames(maker, ((JCClassDecl) typeNode.get()).typarams), maker.Select(maker.Ident(((JCClassDecl) typeNode.get()).name), bfd.nameOfDefaultProvider), List.nil())); + statements.append(maker.If(maker.Unary(CTC_NOT, setField), maker.Exec(assignDefault), null)); } } -- cgit From 46837d6d8be26cea9d0b8cf2959866ac308cbf2d Mon Sep 17 00:00:00 2001 From: Jan Rieke Date: Thu, 16 Aug 2018 17:48:50 +0200 Subject: do null checks after assignment so that default==null is covered --- .../lombok/javac/handlers/HandleSuperBuilder.java | 15 ++- .../after-delombok/SuperBuilderWithNonNull.java | 117 +++++++++++++++++++++ .../resource/before/SuperBuilderWithNonNull.java | 20 ++++ 3 files changed, 144 insertions(+), 8 deletions(-) create mode 100644 test/transform/resource/after-delombok/SuperBuilderWithNonNull.java create mode 100644 test/transform/resource/before/SuperBuilderWithNonNull.java (limited to 'src/core/lombok/javac') diff --git a/src/core/lombok/javac/handlers/HandleSuperBuilder.java b/src/core/lombok/javac/handlers/HandleSuperBuilder.java index c00bd66d..cc295beb 100644 --- a/src/core/lombok/javac/handlers/HandleSuperBuilder.java +++ b/src/core/lombok/javac/handlers/HandleSuperBuilder.java @@ -406,17 +406,10 @@ public class HandleSuperBuilder extends JavacAnnotationHandler { AccessLevel level = AccessLevel.PROTECTED; - ListBuffer nullChecks = new ListBuffer(); ListBuffer statements = new ListBuffer(); Name builderVariableName = typeNode.toName("b"); for (BuilderFieldData bfd : builderFields) { - List nonNulls = findAnnotations(bfd.originalFieldNode, NON_NULL_PATTERN); - if (!nonNulls.isEmpty()) { - JCStatement nullCheck = generateNullCheck(maker, bfd.originalFieldNode, source); - if (nullCheck != null) nullChecks.append(nullCheck); - } - JCExpression rhs; if (bfd.singularData != null && bfd.singularData.getSingularizer() != null) { bfd.singularData.getSingularizer().appendBuildCode(bfd.singularData, bfd.originalFieldNode, bfd.type, statements, bfd.name, "b"); @@ -436,6 +429,12 @@ public class HandleSuperBuilder extends JavacAnnotationHandler { JCAssign assignDefault = maker.Assign(fieldInThis, maker.Apply(typeParameterNames(maker, ((JCClassDecl) typeNode.get()).typarams), maker.Select(maker.Ident(((JCClassDecl) typeNode.get()).name), bfd.nameOfDefaultProvider), List.nil())); statements.append(maker.If(maker.Unary(CTC_NOT, setField), maker.Exec(assignDefault), null)); } + + List nonNulls = findAnnotations(bfd.originalFieldNode, NON_NULL_PATTERN); + if (!nonNulls.isEmpty()) { + JCStatement nullCheck = generateNullCheck(maker, bfd.originalFieldNode, source); + if (nullCheck != null) statements.append(nullCheck); + } } JCModifiers mods = maker.Modifiers(toJavacModifier(level), List.nil()); @@ -465,7 +464,7 @@ public class HandleSuperBuilder extends JavacAnnotationHandler { JCMethodDecl constr = recursiveSetGeneratedBy(maker.MethodDef(mods, typeNode.toName(""), null, List.nil(), params.toList(), List.nil(), - maker.Block(0L, nullChecks.appendList(statements).toList()), null), source.get(), typeNode.getContext()); + maker.Block(0L, statements.toList()), null), source.get(), typeNode.getContext()); injectMethod(typeNode, constr, null, Javac.createVoidType(typeNode.getSymbolTable(), CTC_VOID)); } diff --git a/test/transform/resource/after-delombok/SuperBuilderWithNonNull.java b/test/transform/resource/after-delombok/SuperBuilderWithNonNull.java new file mode 100644 index 00000000..6fea9d17 --- /dev/null +++ b/test/transform/resource/after-delombok/SuperBuilderWithNonNull.java @@ -0,0 +1,117 @@ +import java.util.List; +public class SuperBuilderWithNonNull { + public static class Parent { + @lombok.NonNull + String nonNullParentField; + @java.lang.SuppressWarnings("all") + private static String $default$nonNullParentField() { + return "default"; + } + @java.lang.SuppressWarnings("all") + public static abstract class ParentBuilder> { + @java.lang.SuppressWarnings("all") + private boolean nonNullParentField$set; + @java.lang.SuppressWarnings("all") + private String nonNullParentField; + @java.lang.SuppressWarnings("all") + protected abstract B self(); + @java.lang.SuppressWarnings("all") + public abstract C build(); + @java.lang.SuppressWarnings("all") + public B nonNullParentField(final String nonNullParentField) { + this.nonNullParentField = nonNullParentField; + nonNullParentField$set = true; + return self(); + } + @java.lang.Override + @java.lang.SuppressWarnings("all") + public java.lang.String toString() { + return "SuperBuilderWithNonNull.Parent.ParentBuilder(nonNullParentField=" + this.nonNullParentField + ")"; + } + } + @java.lang.SuppressWarnings("all") + private static final class ParentBuilderImpl extends ParentBuilder { + @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) { + this.nonNullParentField = b.nonNullParentField; + if (!b.nonNullParentField$set) this.nonNullParentField = Parent.$default$nonNullParentField(); + if (nonNullParentField == null) { + throw new java.lang.NullPointerException("nonNullParentField is marked @NonNull but is null"); + } + } + @java.lang.SuppressWarnings("all") + public static ParentBuilder builder() { + return new ParentBuilderImpl(); + } + } + public static class Child extends Parent { + @lombok.NonNull + String nonNullChildField; + @java.lang.SuppressWarnings("all") + public static abstract class ChildBuilder> extends Parent.ParentBuilder { + @java.lang.SuppressWarnings("all") + private String nonNullChildField; + @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 nonNullChildField(final String nonNullChildField) { + this.nonNullChildField = nonNullChildField; + return self(); + } + @java.lang.Override + @java.lang.SuppressWarnings("all") + public java.lang.String toString() { + return "SuperBuilderWithNonNull.Child.ChildBuilder(super=" + super.toString() + ", nonNullChildField=" + this.nonNullChildField + ")"; + } + } + @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); + this.nonNullChildField = b.nonNullChildField; + if (nonNullChildField == null) { + throw new java.lang.NullPointerException("nonNullChildField is marked @NonNull but is null"); + } + } + @java.lang.SuppressWarnings("all") + public static ChildBuilder builder() { + return new ChildBuilderImpl(); + } + } + public static void test() { + Child x = Child.builder().nonNullChildField("child").nonNullParentField("parent").build(); + } +} diff --git a/test/transform/resource/before/SuperBuilderWithNonNull.java b/test/transform/resource/before/SuperBuilderWithNonNull.java new file mode 100644 index 00000000..21f67a47 --- /dev/null +++ b/test/transform/resource/before/SuperBuilderWithNonNull.java @@ -0,0 +1,20 @@ +import java.util.List; + +public class SuperBuilderWithNonNull { + @lombok.experimental.SuperBuilder + public static class Parent { + @lombok.NonNull + @lombok.Builder.Default + String nonNullParentField = "default"; + } + + @lombok.experimental.SuperBuilder + public static class Child extends Parent { + @lombok.NonNull + String nonNullChildField; + } + + public static void test() { + Child x = Child.builder().nonNullChildField("child").nonNullParentField("parent").build(); + } +} -- cgit From 31aef5219d6e6a39afe5dfa5d888bddd6e942e25 Mon Sep 17 00:00:00 2001 From: Jan Rieke Date: Thu, 16 Aug 2018 17:58:05 +0200 Subject: only assign value once so that final fields work (javac) --- src/core/lombok/javac/handlers/HandleSuperBuilder.java | 7 ++++--- .../transform/resource/after-delombok/SuperBuilderWithNonNull.java | 6 +++--- test/transform/resource/before/SuperBuilderWithNonNull.java | 2 +- 3 files changed, 8 insertions(+), 7 deletions(-) (limited to 'src/core/lombok/javac') diff --git a/src/core/lombok/javac/handlers/HandleSuperBuilder.java b/src/core/lombok/javac/handlers/HandleSuperBuilder.java index cc295beb..aaf94dd8 100644 --- a/src/core/lombok/javac/handlers/HandleSuperBuilder.java +++ b/src/core/lombok/javac/handlers/HandleSuperBuilder.java @@ -420,14 +420,15 @@ public class HandleSuperBuilder extends JavacAnnotationHandler { JCFieldAccess fieldInThis = maker.Select(maker.Ident(typeNode.toName("this")), bfd.rawName); JCStatement assign = maker.Exec(maker.Assign(fieldInThis, rhs)); - statements.append(assign); - // In case of @Builder.Default, set the value to the default if it was NOT set in the builder. + // In case of @Builder.Default, set the value to the default if it was not set in the builder. if (bfd.nameOfSetFlag != null) { JCFieldAccess setField = maker.Select(maker.Ident(builderVariableName), bfd.nameOfSetFlag); fieldInThis = maker.Select(maker.Ident(typeNode.toName("this")), bfd.rawName); JCAssign assignDefault = maker.Assign(fieldInThis, maker.Apply(typeParameterNames(maker, ((JCClassDecl) typeNode.get()).typarams), maker.Select(maker.Ident(((JCClassDecl) typeNode.get()).name), bfd.nameOfDefaultProvider), List.nil())); - statements.append(maker.If(maker.Unary(CTC_NOT, setField), maker.Exec(assignDefault), null)); + statements.append(maker.If(setField, assign, maker.Exec(assignDefault))); + } else { + statements.append(assign); } List nonNulls = findAnnotations(bfd.originalFieldNode, NON_NULL_PATTERN); diff --git a/test/transform/resource/after-delombok/SuperBuilderWithNonNull.java b/test/transform/resource/after-delombok/SuperBuilderWithNonNull.java index 6fea9d17..5eba938f 100644 --- a/test/transform/resource/after-delombok/SuperBuilderWithNonNull.java +++ b/test/transform/resource/after-delombok/SuperBuilderWithNonNull.java @@ -2,7 +2,7 @@ import java.util.List; public class SuperBuilderWithNonNull { public static class Parent { @lombok.NonNull - String nonNullParentField; + final String nonNullParentField; @java.lang.SuppressWarnings("all") private static String $default$nonNullParentField() { return "default"; @@ -47,8 +47,8 @@ public class SuperBuilderWithNonNull { } @java.lang.SuppressWarnings("all") protected Parent(final ParentBuilder b) { - this.nonNullParentField = b.nonNullParentField; - if (!b.nonNullParentField$set) this.nonNullParentField = Parent.$default$nonNullParentField(); + if (b.nonNullParentField$set) this.nonNullParentField = b.nonNullParentField; + else this.nonNullParentField = Parent.$default$nonNullParentField(); if (nonNullParentField == null) { throw new java.lang.NullPointerException("nonNullParentField is marked @NonNull but is null"); } diff --git a/test/transform/resource/before/SuperBuilderWithNonNull.java b/test/transform/resource/before/SuperBuilderWithNonNull.java index 21f67a47..34668bbc 100644 --- a/test/transform/resource/before/SuperBuilderWithNonNull.java +++ b/test/transform/resource/before/SuperBuilderWithNonNull.java @@ -5,7 +5,7 @@ public class SuperBuilderWithNonNull { public static class Parent { @lombok.NonNull @lombok.Builder.Default - String nonNullParentField = "default"; + final String nonNullParentField = "default"; } @lombok.experimental.SuperBuilder -- cgit