Open reisners opened 6 years ago
victorwss
commented
6 years ago The thingBuilder.otherThing(thingBuilder); is no more than a very contrieved way to make thingBuilder get a reference for what it would call as this, i.e., not a real example. Can you provide a complete example more similar to a real use case of the code that you would like? I didn't get it. I suspect that what you want to do would be easier to be solved with traditionally-coded stuff instead of lombokized-stuff.
randakar
commented
6 years ago Let's talk a bit about a real world example and see how useful this actually would be, then.
Here's some code lifted from a real project, but anonimized to match the first example:
-- @Builder @Data private class Inner {
@NonNull
private Long id;
@NonNull
private String name;
@NonNull
private Outer outer;
}
@Builder
@Data
private class Outer {
@NonNull
private Set<Inner> inners;
}
private final List<Long> outerIndices = LongStream.rangeClosed(0,3).boxed().collect(Collectors.toList();
private final List
private final List<Outer> example = outerIndices.stream()
.map(outerIndex ->
Outer.builder()
.inners(innerIndices.stream()
.map(innerThingIndex -> Inner.builder()
.id(innerThingIndex)
.name(format("inner_%s",In this case we have a classic circular reference: All instances of Inner refer to a singular instance of Outer, and each Outer refers to a Set of Inner objects.
Now, with this solution using the classic builder, we run into a problem: We don't have a reference to the outer when we build the inner objects, so we have to call setOuter() on them afterwards. This is done in the .peek() call. ( This is also why neither of these objects uses @Value, by the way - that, and hibernate. Which I won't talk about further. )
So, I guess that what the OP wants to be able to do, is this:
@Builder @Value private class Inner {
@NonNull
private Long id;
@NonNull
private String name;
@NonNull
private Outer outer;
}
@Builder
@Value
private class Outer {
@NonNull
private Set<Inner> inners;
}
private final List<Long> outerIndices = LongStream.rangeClosed(0,3).boxed().collect(Collectors.toList();
private final List
private final List<Outer> example = outerIndices.stream()
.map(outerIndex -> {
Outer.OuterBuilder outerBuilder = Outer.builder();
return outerBuilder
.innersBuilder(innerIndices.stream()
.map(innerThingIndex -> Inner.builder()
.id(innerThingIndex)
.name(format("inner_%s",innerThingIndex)) .outerBuilder(outerBuilder)) .collect(Collectors.toSet())) .build(); }) .collect(Collectors.toList());
We have to get a reference to that outer builder here, so we're breaking the lamda, but now we can call .outerBuilder() on that inner builder and supply it with a builder instance. We do have to call .innersBuilder() on the outer builder as well, so both builders need to be aware of the mutual dependency, and when build() is called on the one it will need to call .build() on the inner builder, too.
So I suppose this is possible. If complicated. And I'd probably not turn it on by default - you'd have to put a flag on that Builder annotation to make this be generated.
On Thu, Mar 22, 2018 at 3:15 PM, Victor Williams Stafusa da Silva < notifications@github.com> wrote:
The thingBuilder.otherThing(thingBuilder); is no more than a very contrieved way to make thingBuilder get a reference for what it would call as this, i.e., not a real example. Can you provide a complete example more similar to a real use case of the code that you would like? I didn't get it. I suspect that what you want to do would be easier to be solved with traditionally-coded stuff instead of lombokized-stuff.
— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/rzwitserloot/lombok/issues/1623#issuecomment-375321493, or mute the thread https://github.com/notifications/unsubscribe-auth/AAKCRa33vbqagBO8ekCq5D_Wt8I5RUkjks5tg7ISgaJpZM4S27zl .
-- "Don't only practice your art, but force your way into it's secrets, for it and knowledge can raise men to the divine." -- Ludwig von Beethoven
Maaartinus
commented
6 years ago @randakar
```, so it get formatted properly.Why? Forget Lombok, forget builders, consider two classes Aand B referring to each other.
How would you create the circle without re-setting the fields via reflection?
AFAIK it is possible with a constructor like
A(BBuilder bb) {
this.b = bb.a(this).build();
}You need a special constructor taking the builder and passing to it the not-yet-finished this.
How does it generalize to three classes?
randakar
commented
6 years ago I never said it was good idea. Just trying to figure out what the original poster was trying to do here.
As for the circle .. yeah, that's a hard one. It's not possible with just constructor calls, that's for sure. Something needs to gets instantiated first and then accept it doesn't have any instances of the other object yet.
I'm guessing that as a static inner class builders have access to the fields of the object it's instantiating directly, though, so it could have a special private constructor without the affected fields, use that to create a partial instance, then call any remaining builders remaining with references of that partial object to put in place the rest of the data. Also, any calls to build() on the original builder object would have to be basically ignored and return the partial instance that is already being created, to prevent loops.
Not saying it's pretty, either. But it's definitely possible.
On Fri, Mar 23, 2018 at 2:23 PM, Maaartinus notifications@github.com wrote:
@randakar https://github.com/randakar
- Please enclose your code in ```, so it get formatted properly.
- The example looks pretty complicated, and...
- I don't think, it's really possible for Lombok.
Why? Forget Lombok, forget builders, consider two classes Aand B referring to each other. How would you create the circle without re-setting the fields via reflection? AFAIK it is possible with a constructor like
A(BBuilder bb) { this.b = bb.a(this).build(); }
You need a special constructor taking the builder and passing to it the not-yet-finished this. How does it generalize to three classes?
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/rzwitserloot/lombok/issues/1623#issuecomment-375663418, or mute the thread https://github.com/notifications/unsubscribe-auth/AAKCRWHICp1s40FSY87IksbHPj1drBHGks5thPdvgaJpZM4S27zl .
-- "Don't only practice your art, but force your way into it's secrets, for it and knowledge can raise men to the divine." -- Ludwig von Beethoven
Maaartinus
commented
6 years ago @randakar
Something needs to gets instantiated first
Not exactly. Something has to start first and something has to finish last. When the calls overlap, magic arises.
use that to create a partial instance
That's ugly. What's worse: It loses the final field visibility guarantee.
This creates two mutually linked objects:
class A {
private final B b;
A(Function<A, B> linkingFunction) {
this.b = linkingFunction.apply(this);
}
}
@RequiredArgsConstructor
@Builder
class B {
private final A a;
}
B.Builder bb = B.builder();
A a = new A(a2 -> bb.a(a2).build());The magic is just passing the not-yet-fully-created A to the B constructor. There're still problems:
B uses a builder and can be meaningfully configured, I can't see how to configure A.Anyway, I don't think, Lombok should do such funny things itself. But maybe it can support it in some simple and generally useful way.
randakar
commented
6 years ago Neat trick. Didn't think of using lambda's that way. Does it actually work? I kinda expect something to blow up spectacularly, I highly doubt anyone tests this kinda corner case ;)
Anyway - there really is no way around it. Any way you cut it, this would be a massive unmaintainable hack. Fun to think about implementing, bad idea to actually do..
Op 24 mrt. 2018 04:02 schreef "Maaartinus" notifications@github.com:
@randakar https://github.com/randakar
Something needs to gets instantiated first
Not exactly. Something has to start first and something has to finish last. When the calls overlap, magic arises.
use that to create a partial instance
This creates two mutually linked objects:
class A { private final B b;
A(Function<A, B> linkingFunction) {
this.b = linkingFunction.apply(this);
}}
@RequiredArgsConstructor @Builder class B { private final A a; }
B.Builder bb = B.builder(); A a = new A(a2 -> bb.a(a2).build());
The magic is just passing the not-yet-fully-created A to the B constructor. There're still problems:
Anyway, I don't think, Lombok should do such funny things itself. But maybe it can support it in some simple and generally useful way.
— You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub https://github.com/rzwitserloot/lombok/issues/1623#issuecomment-375841956, or mute the thread https://github.com/notifications/unsubscribe-auth/AAKCRdqFU5pTfUBx_GlUcw0iLPuVzzFVks5thbcygaJpZM4S27zl .
reisners
commented
6 years ago In my request, I tried to come up with a minimal example. It seems it turned out to be degenerate. To motivate my request, I think @randakar's example of an object model containing back references does a very good job. There might be more complex cycles (depending on the domain), but if it works for back references it will work for anything else as well. I would like to work with an object model that is semantically immutable. I don't insist on declaring the member final, since I'm well aware that it cannot work if each element of the reference cycle is declared final (as in my bad example, sorry if that added to the confusion). Immutability imho is sufficiently expressed by not having @Setter. Passing the referenced object's builder instead of the object itself, one could defer the instantiation of that object. The framework could discover that a cycle is to be created. It could then in a first pass instantiate all objects with as many references in place as possible, and in a second pass set all the missing references by calling private setters. Just my thoughts.
Maaartinus
commented
6 years ago @randakar
Neat trick. Didn't think of using lambda's that way. Does it actually work?
Lambdas do only the trick of allowing linking the objects flexibly. The cycle trick is the constructor calling another one as in my first comment here. I haven't tried it, but it must work.
@reisners
Immutability imho is sufficiently expressed by not having
@Setter.
But without final, there's no safe publishing and someone might spend days on hunting down the problem in some multithreaded code.
However, with final it gets too complicated. Without it, it's much simpler, but very different from how @Builder currently works.
Passing the referenced object's builder instead of the object itself, one could defer the instantiation of that object.
A the very same object can be reference multiple times, passing the builder doesn't seem to be optimal as calling .build() returns a new instance each time. This could be solved via a IdentityHashMap, extending each builder by a reference to the most recently build object, or maybe passing a Supplier<?> instead.
What would be the usage for three objects building a cycle?
A.Builder aBuilder = A.builder();
B.Builder bBuilder = B.builder();
C.Builder cBuilder = C.builder();
aBuilder.data("A-data").b(bBuilder);
bBuilder.data("B-data").c(cBuilder);
cBuilder.data("C-data").a(aBuilder);
A a = aBuilder.build(); // <- this creates all three objects
B b = bBuilder.get(); // retrieving the already build instanceThis is pretty ugly. isn't it?
victorwss
commented
6 years ago I think that it is doable even if there are many interrelated circular dependencies, tough it is somewhat complicated.
Let's suppose that this is our lombok-spiced code, using an hypothetical @lombok.Builder.Supplied annotation and also an hypothetical incompletePublishing method in the @lombok.Builder annotation:
import lombok.*;
@Value
@Builder(incompletePublishing = true)
@AllArgsConstructor
public class A {
@NonNull
@Builder.Supplied
private B b;
@NonNull
@Builder.Supplied("suppliedC")
private C c;
}import lombok.*;
@Value
@Builder(incompletePublishing = true)
@AllArgsConstructor
public class B {
@NonNull
@Builder.Supplied
private A a;
@NonNull
@Builder.Supplied
private C c;
// Something else not related to circular dependencies.
@NonNull
private String d;
}import java.util.List;
import lombok.*;
@Value
@Builder(incompletePublishing = true)
@AllArgsConstructor
public class C {
// Circular dependencies with items of a List.
@NonNull
@Singular("a")
@Builder.Supplied
private List<A> as;
@NonNull
@Builder.Supplied
private B b1;
@NonNull
@Builder.Supplied
private B b2;
}Those classes A, B and C would be delomboked into something like this:
public final class A {
private final B b;
private final C c;
private A(
java.util.function.Consumer<A> $lombokIncompletePublisher,
java.util.function.Supplier<? extends B> b,
java.util.function.Supplier<? extends C> c)
{
if (b == null) throw new NullPointerException("b");
if (c == null) throw new NullPointerException("c");
$lombokIncompletePublisher.accept(this);
this.b = b.get();
this.c = c.get();
}
public B getB() {
return b;
}
public C getC() {
return c;
}
public static ABuilder builder() {
return new ABuilder();
}
// equals(Object), hashCode() and toString() methods ommited.
public static final class ABuilder {
private java.util.function.Supplier<? extends B> b;
private java.util.function.Supplier<? extends C> c;
private ABuilder() {
}
public ABuilder b(B b) {
this.b = () -> b;
return this;
}
public ABuilder b(java.util.function.Supplier<? extends B> b) {
this.b = b;
return this;
}
public ABuilder c(C c) {
this.c = () -> c;
return this;
}
public ABuilder suppliedC(java.util.function.Supplier<? extends C> c) {
this.c = c;
return this;
}
public A build() {
return build($lombokDummy -> {});
}
public A build(java.util.function.Consumer<A> $lombokIncompletePublisher) {
return new A($lombokIncompletePublisher, this.b, this.c);
}
// toString() method ommited.
}
}public final class B {
private final A a;
private final C c;
private final String d;
private B(
java.util.function.Consumer<B> $lombokIncompletePublisher,
java.util.function.Supplier<? extends A> a,
java.util.function.Supplier<? extends C> c,
String d)
{
if (a == null) throw new NullPointerException("a");
if (c == null) throw new NullPointerException("c");
if (d == null) throw new NullPointerException("d");
$lombokIncompletePublisher.accept(this);
this.a = a.get();
this.c = c.get();
this.d = d;
}
public A getA() {
return a;
}
public C getC() {
return c;
}
public String getD() {
return d;
}
public static BBuilder builder() {
return new BBuilder();
}
// equals(Object), hashCode() and toString() methods ommited.
public static final class BBuilder {
private java.util.function.Supplier<? extends A> a;
private java.util.function.Supplier<? extends C> c;
private String d;
private BBuilder() {
}
public BBuilder a(A a) {
this.a = () -> a;
return this;
}
public BBuilder a(java.util.function.Supplier<? extends A> a) {
this.a = a;
return this;
}
public BBuilder c(C c) {
this.c = () -> c;
return this;
}
public BBuilder c(java.util.function.Supplier<? extends C> c) {
this.c = c;
return this;
}
public BBuilder d(String d) {
this.d = d;
return this;
}
public B build() {
return build($lombokDummy -> {});
}
public B build(java.util.function.Consumer<B> $lombokIncompletePublisher) {
return new B($lombokIncompletePublisher, this.a, this.c, this.d);
}
// toString() method ommited.
}
}import java.util.List;
public final class C {
private final List<A> as;
private final B b1;
private final B b2;
private C(
java.util.function.Consumer<C> $lombokIncompletePublisher,
java.util.List<java.util.function.Supplier<? extends A>> as,
java.util.function.Supplier<? extends B> b1,
java.util.function.Supplier<? extends B> b2)
{
if (as == null) throw new NullPointerException("as");
if (b1 == null) throw new NullPointerException("b1");
if (b2 == null) throw new NullPointerException("b2");
$lombokIncompletePublisher.accept(this);
this.as = as.stream()
.map(java.util.function.Supplier::get)
.collect(java.util.stream.Collectors.toUnmodifiableList());
this.b1 = b1.get();
this.b2 = b2.get();
}
public List<A> getAs() {
return as;
}
public B getB1() {
return b1;
}
public B getB2() {
return b2;
}
public static CBuilder builder() {
return new CBuilder();
}
// equals(Object), hashCode() and toString() methods ommited.
public static class CBuilder {
private final java.util.List<java.util.function.Supplier<? extends A>> as;
private java.util.function.Supplier<? extends B> b1;
private java.util.function.Supplier<? extends B> b2;
private CBuilder() {
this.as = new java.util.ArrayList<>();
}
public CBuilder a(A a) {
this.as.add(() -> a);
return this;
}
public CBuilder a(java.util.function.Supplier<? extends A> a) {
this.as.add(a);
return this;
}
public CBuilder as(java.util.Collection<? extends A> as) {
as.forEach(a -> this.as.add(() -> a));
return this;
}
public CBuilder clearAs() {
this.as.clear();
return this;
}
public CBuilder b1(B b1) {
this.b1 = () -> b1;
return this;
}
public CBuilder b1(java.util.function.Supplier<? extends B> b1) {
this.b1 = b1;
return this;
}
public CBuilder b2(B b2) {
this.b2 = () -> b2;
return this;
}
public CBuilder b2(java.util.function.Supplier<? extends B> b2) {
this.b2 = b2;
return this;
}
public C build() {
return build($lombokDummy -> {});
}
public C build(java.util.function.Consumer<C> $lombokIncompletePublisher) {
return new C($lombokIncompletePublisher, this.as, this.b1, this.b2);
}
// toString() method ommited.
}
}Now, this should do the magic:
import java.util.function.Supplier;
public class Test {
public static void main(String[] args) {
// 1. Declare all the builders.
A.ABuilder builderA1 = A.builder();
A.ABuilder builderA2 = A.builder();
B.BBuilder builderB1 = B.builder();
B.BBuilder builderB2 = B.builder();
C.CBuilder builderC = C.builder();
// 2. Get the cached suppliers.
Supplier<A> sa1 = Cached.cache(builderA1::build);
Supplier<A> sa2 = Cached.cache(builderA2::build);
Supplier<B> sb1 = Cached.cache(builderB1::build);
Supplier<B> sb2 = Cached.cache(builderB2::build);
Supplier<C> sc = Cached.cache(builderC::build);
// 3. Populate all the builders
builderA1.b(sb1).suppliedC(sc);
builderA2.b(sb2).suppliedC(sc);
builderB1.a(sa2).c(sc).d("Cupcake");
builderB2.a(sa1).c(sc).d("Brownie");
builderC.a(sa1).a(sa2).b1(sb1).b2(sb2);
// 4. Produce the objects.
A a1 = sa1.get();
A a2 = sa2.get();
B b1 = sb1.get();
B b2 = sb2.get();
C c = sc.get();
// 5. Assert that the objects are what we wanted.
if (a1.getB() != b1) throw new AssertionError();
if (a2.getB() != b2) throw new AssertionError();
if (b1.getA() != a2) throw new AssertionError();
if (b2.getA() != a1) throw new AssertionError();
if (a1.getC() != c) throw new AssertionError();
if (a2.getC() != c) throw new AssertionError();
if (b1.getC() != c) throw new AssertionError();
if (b2.getC() != c) throw new AssertionError();
if (!"Cupcake".equals(b1.getD())) throw new AssertionError();
if (!"Brownie".equals(b2.getD())) throw new AssertionError();
if (c.getB1() != b1) throw new AssertionError();
if (c.getB2() != b2) throw new AssertionError();
if (c.getAs().size() != 2) throw new AssertionError();
if (c.getAs().get(0) != a1) throw new AssertionError();
if (c.getAs().get(1) != a2) throw new AssertionError();
try {
c.getAs().add(a1);
throw new AssertionError();
} catch (UnsupportedOperationException expected) {
// Swallow it.
}
try {
c.getAs().remove(a1);
throw new AssertionError();
} catch (UnsupportedOperationException expected) {
// Swallow it.
}
}
}But that magic needs a Cached class in order to work:
import java.util.function.Consumer;
import java.util.function.Function;
import java.util.function.Supplier;
public final class Cached<X> implements Supplier<X> {
private Function<Consumer<X>, X> wrapped;
private Cached(Function<Consumer<X>, X> wrapped) {
this.wrapped = wrapped;
}
@Override
public X get() {
return wrapped.apply(x -> wrapped = z -> x);
}
public static <X> Supplier<X> cache(Function<Consumer<X>, X> wrapped) {
return new Cached<>(wrapped);
}
}I tested the program composed of the five classes above and it compiles and run correctly in Java 10.
To be able to generate that, those changes would be needed in lombok's implementation:
A new @Builder.Supplied annotation would be needed. It have a String value() default ""; method.
A new boolean incompletePublishing() default false; method in the @Builder annotation would also be needed.
For every field z typed Y in a class X, whenever z is annotated with @Builder.Supplied, two corresponding methods to populate z into the XBuilder would be generated. One is the method generated by the current implementation that receives a Y as a parameter and has the same name of the z field. The other one receives a java.util.function.Supplier<Y> and, if a different name is not specified in the annotation, also has the same name of the z field. Specifying a different name is useful in the situations where simply overloading the method would cause ambiguities or would be undesirable to the user for some other reason.
When the field incompletePublishing = true is present in the @Builder annotation, a Consumer will be provided as the first constructor parameter to allow the this reference leak, so we will be able to crosslink the objects. This also overloads the build method adding one that receives the Consumer in addition to the traditional one that does not. The traditional build method is then implemented as a call to the new one passing a dummy Consumer as a parameter.
For handling the case of @Singular, I'm relying in Java 10 to provide the Collectors.toUnmodifiableList() method. Also, I'm strongly relying in Suppliers, lambdas, method's references and streams everywhere, which are unavailable for Java <= 7. Of course, those could be replaced by other things down to Java 6, at the cost of having a much more complicated implementation (possibly including the horrible switch that is generated nowadays).
For lists annotated with @Singular, the current implementation do not initializes it, leaving it as null and only instantiates the List when it would be needed. Here, I changed this in favor of having it initialized in the constructor and with the final modifier, so it is never null.
I changed the constructors' accessibility to private. Currently, lombok generates them with package-private acessibility. Any reason why not use private instead of package-private?
There is also a few remarks in this code:
If @Singular is used, no bulk-adding method for a List of Suppliers is provided, much less for a List that mixes instances of both the target type and its Suppliers.
Note the builders are not thread-safe, so is the Cached class. BTW, on a side note, the @Builder documentation should explicitly state that builders are mutable and not thread-safe.
The usage form (exemplified in the steps 1-4 in the Test) is still significantly complex, requiring the user to handle the cached Suppliers directly. Nonetheless, this proves that it is possible without major changes in lombok. However, I think that it is very desirable if somebody finds a simpler way that do not breaks things nor requires too much changes.
randakar
commented
6 years ago Nice work, though far from finished. Point 3, especially. The second you introduce Cached as a thing the user would need to instantiate directly you lose the whole "we want as little boilerplate as possible" game. Especially static methods like that.
I guess this counts as a proof of concept, though. It's nice that for existing code with existing annotations almost nothing would change. Still feels complex though, for something that just makes instantiating immutable objects a bit more convenient.
On Sun, Mar 25, 2018 at 10:20 PM, Victor Williams Stafusa da Silva < notifications@github.com> wrote:
I think that it is doable even if there are many interrelated circular dependencies, tough it is somewhat complicated.
Let's suppose that this is our lombok-spiced code, using an hypothetical @lombok.Builder.Supplied annotation:
import lombok.*; @Value@Builder(incompletePublishing = true)@AllArgsConstructorpublic class A { @NonNull @Builder.Supplied private B b;
@NonNull @Builder.Supplied("suppliedC") private C c;}
import lombok.*; @Value@Builder(incompletePublishing = true)@AllArgsConstructorpublic class B { @NonNull @Builder.Supplied private A a;
@NonNull @Builder.Supplied private C c; // Something else not related to circular dependencies. @NonNull private String d;}
import java.util.List;import lombok.*; @Value@Builder(incompletePublishing = true)@AllArgsConstructorpublic class C { // Circular dependencies with items of a List. @NonNull @Singular("a") @Builder.Supplied private List as;
@NonNull @Builder.Supplied private B b1; @NonNull @Builder.Supplied private B b2;}
Those classes A, B and C would be delomboked into something like this:
public final class A { private final B b;
private final C c; private A( java.util.function.Consumer<A> $lombokIncompletePublisher, java.util.function.Supplier<? extends B> b, java.util.function.Supplier<? extends C> c) { if (b == null) throw new NullPointerException("b"); if (c == null) throw new NullPointerException("c"); $lombokIncompletePublisher.accept(this); this.b = b.get(); this.c = c.get(); } public B getB() { return b; } public C getC() { return c; } public static ABuilder builder() { return new ABuilder(); } // equals(Object), hashCode() and toString() methods ommited. public static final class ABuilder { private java.util.function.Supplier<? extends B> b; private java.util.function.Supplier<? extends C> c; private ABuilder() { } public ABuilder b(B b) { this.b = () -> b; return this; } public ABuilder b(java.util.function.Supplier<? extends B> b) { this.b = b; return this; } public ABuilder c(C c) { this.c = () -> c; return this; } public ABuilder suppliedC(java.util.function.Supplier<? extends C> c) { this.c = c; return this; } public A build() { return build($lombokDummy -> {}); } public A build(java.util.function.Consumer<A> $lombokIncompletePublisher) { return new A($lombokIncompletePublisher, this.b, this.c); } // toString() method ommited. }}
public final class B { private final A a;
private final C c; private final String d; private B( java.util.function.Consumer<B> $lombokIncompletePublisher, java.util.function.Supplier<? extends A> a, java.util.function.Supplier<? extends C> c, String d) { if (a == null) throw new NullPointerException("a"); if (c == null) throw new NullPointerException("c"); if (d == null) throw new NullPointerException("d"); $lombokIncompletePublisher.accept(this); this.a = a.get(); this.c = c.get(); this.d = d; } public A getA() { return a; } public C getC() { return c; } public String getD() { return d; } public static BBuilder builder() { return new BBuilder(); } // equals(Object), hashCode() and toString() methods ommited. public static final class BBuilder { private java.util.function.Supplier<? extends A> a; private java.util.function.Supplier<? extends C> c; private String d; private BBuilder() { } public BBuilder a(A a) { this.a = () -> a; return this; } public BBuilder a(java.util.function.Supplier<? extends A> a) { this.a = a; return this; } public BBuilder c(C c) { this.c = () -> c; return this; } public BBuilder c(java.util.function.Supplier<? extends C> c) { this.c = c; return this; } public BBuilder d(String d) { this.d = d; return this; } public B build() { return build($lombokDummy -> {}); } public B build(java.util.function.Consumer<B> $lombokIncompletePublisher) { return new B($lombokIncompletePublisher, this.a, this.c, this.d); } // toString() method ommited. }}
import java.util.List; public final class C { private final List as;
private final B b1; private final B b2; private C( java.util.function.Consumer<C> $lombokIncompletePublisher, java.util.List<java.util.function.Supplier<? extends A>> as, java.util.function.Supplier<? extends B> b1, java.util.function.Supplier<? extends B> b2) { if (as == null) throw new NullPointerException("as"); if (b1 == null) throw new NullPointerException("b1"); if (b2 == null) throw new NullPointerException("b2"); $lombokIncompletePublisher.accept(this); this.as = as.stream() .map(java.util.function.Supplier::get) .collect(java.util.stream.Collectors.toUnmodifiableList()); this.b1 = b1.get(); this.b2 = b2.get(); } public List<A> getAs() { return as; } public B getB1() { return b1; } public B getB2() { return b2; } public static CBuilder builder() { return new CBuilder(); } // equals(Object), hashCode() and toString() methods ommited. public static class CBuilder { private final java.util.List<java.util.function.Supplier<? extends A>> as; private java.util.function.Supplier<? extends B> b1; private java.util.function.Supplier<? extends B> b2; private CBuilder() { this.as = new java.util.ArrayList<>(); } public CBuilder a(A a) { this.as.add(() -> a); return this; } public CBuilder a(java.util.function.Supplier<? extends A> a) { this.as.add(a); return this; } public CBuilder as(java.util.Collection<? extends A> as) { as.forEach(a -> this.as.add(() -> a)); return this; } public CBuilder clearAs() { this.as.clear(); return this; } public CBuilder b1(B b1) { this.b1 = () -> b1; return this; } public CBuilder b1(java.util.function.Supplier<? extends B> b1) { this.b1 = b1; return this; } public CBuilder b2(B b2) { this.b2 = () -> b2; return this; } public CBuilder b2(java.util.function.Supplier<? extends B> b2) { this.b2 = b2; return this; } public C build() { return build($lombokDummy -> {}); } public C build(java.util.function.Consumer<C> $lombokIncompletePublisher) { return new C($lombokIncompletePublisher, this.as, this.b1, this.b2); } // toString() method ommited. }}
Now, this should do the magic:
import java.util.function.Supplier; public class Test { public static void main(String[] args) { // 1. Declare all the builders. A.ABuilder builderA1 = A.builder(); A.ABuilder builderA2 = A.builder(); B.BBuilder builderB1 = B.builder(); B.BBuilder builderB2 = B.builder(); C.CBuilder builderC = C.builder();
// 2. Get the cached suppliers. Supplier<A> sa1 = Cached.cache(builderA1::build); Supplier<A> sa2 = Cached.cache(builderA2::build); Supplier<B> sb1 = Cached.cache(builderB1::build); Supplier<B> sb2 = Cached.cache(builderB2::build); Supplier<C> sc = Cached.cache(builderC::build); // 3. Populate all the builders builderA1.b(sb1).suppliedC(sc); builderA2.b(sb2).suppliedC(sc); builderB1.a(sa2).c(sc).d("Cupcake"); builderB2.a(sa1).c(sc).d("Brownie"); builderC.a(sa1).a(sa2).b1(sb1).b2(sb2); // 4. Produce the objects. A a1 = sa1.get(); A a2 = sa2.get(); B b1 = sb1.get(); B b2 = sb2.get(); C c = sc.get(); // 5. Assert that the objects are what we wanted. if (a1.getB() != b1) throw new AssertionError(); if (a2.getB() != b2) throw new AssertionError(); if (b1.getA() != a2) throw new AssertionError(); if (b2.getA() != a1) throw new AssertionError(); if (a1.getC() != c) throw new AssertionError(); if (a2.getC() != c) throw new AssertionError(); if (b1.getC() != c) throw new AssertionError(); if (b2.getC() != c) throw new AssertionError(); if (!"Cupcake".equals(b1.getD())) throw new AssertionError(); if (!"Brownie".equals(b2.getD())) throw new AssertionError(); if (c.getB1() != b1) throw new AssertionError(); if (c.getB2() != b2) throw new AssertionError(); if (c.getAs().size() != 2) throw new AssertionError(); if (c.getAs().get(0) != a1) throw new AssertionError(); if (c.getAs().get(1) != a2) throw new AssertionError(); }}
But that magic needs a Cached class in order to work:
import java.util.function.Consumer;import java.util.function.Function;import java.util.function.Supplier; public final class Cached
implements Supplier , Consumer { private Function<Consumer , X> wrapped; private Cached(Function<Consumer<X>, X> wrapped) { this.wrapped = wrapped; } @Override public void accept(X x) { wrapped = z -> x; } @Override public X get() { return wrapped.apply(this); } public static <X> Supplier<X> cache(Function<Consumer<X>, X> wrapped) { return new Cached<>(wrapped); }}
I tested the program composed of the five classes above and it compiles and run correctly in Java 10.
To be able to generate that, those changes would be needed in lombok's implementation:
1.
A new @Builder.Supplied annotation would be needed. It have a String value() default ""; method. 2.
A new boolean incompletePublishing() default false; method in the @Builder annotation would also be needed. 3.
For every field z typed Y in a class X, whenever z is annotated with @Builder.Supplied, two corresponding methods to populate z into the XBuilder would be generated. One is the method generated by the current implementation that receives a Y as a parameter and has the same name of the z field. The other one receives a java.util.function.Supplier
and, if a different name is not specified in the annotation, also has the same name of the z field. Specifying a different name is useful in the situations where simply overloading the method would cause ambiguities or would be undesirable to the user for some other reason. 4. When the field incompletePublishing = true is present in the @Builder annotation, a Consumer will be provided as the first constructor parameter to allow the this reference leak, so we will be able to crosslink the objects. This also overloads the build method adding one that receives the Consumer in addition to the traditional one that does not. The traditional build method is then implemented as a call to the new one passing a dummy Consumer as a parameter. 5.
For handling the case of @Singular, I'm relying in Java 10 to provide the Collectors.toUnmodifiableList() method. Also, I'm strongly relying in Suppliers, lambdas, method's references and streams everywhere, which are unavailable for Java <= 7. Of course, those could be replaced by other things down to Java 6, at the cost of having a much more complicated implementation (possibly including the horrible switch that is generated nowadays). 6.
For lists annotated with @Singular, the current implementation do not initializes it, leaving it as null and only instantiates the List when it would be needed. Here, I changed this in favor of having it initialized in the constructor and with the final modifier, so it is never null. 7.
I changed the constructors' accessibility to private. Currently, lombok generates them with package-private acessibility. Any reason why not use private instead of package-private?
There is also a few remarks in this code:
1.
If @Singular is used, no bulk-adding method for a List of Suppliers is provided, much less for a List that mixes instances of both the target type and its Suppliers. 2.
Note the builders are not thread-safe, so is the Cached class. BTW, on a side note, the @Builder documentation should explicitly state that builders are mutable and not thread-safe. 3.
The usage form (exemplified in the steps 1-4 in the Test) is still significantly complex, requiring the user to handle the cached Suppliers directly. Nonetheless, this proves that it is possible without major changes in lombok. However, I think that it is very desirable if somebody finds a simpler way that do not breaks things nor requires too much changes.
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/rzwitserloot/lombok/issues/1623#issuecomment-376000268, or mute the thread https://github.com/notifications/unsubscribe-auth/AAKCRZaq5KhrfOM0go-y_ftBGZFdYoGiks5th_wqgaJpZM4S27zl .
-- "Don't only practice your art, but force your way into it's secrets, for it and knowledge can raise men to the divine." -- Ludwig von Beethoven
Maaartinus
commented
6 years ago @victorwss This nice thing is that all the fields can be final - my Eclipse added the modifiers and it compiles. It has to compile as the fields don't ever get reassigned. The less nice thing is that I don't really understand how it works. ;)
I see a few big problems there:
P1 The helper class Cached adds a runtime dependency. That's something the Lombok authors systematically avoid.P2 - Java 8+ dependency. AFAIK, Currently, Lombok works with Java 5+. The implementation could be changed somehow, but there's no Consumer and no Supplier before Java 8.P3 - Java 10+ dependency. This is trivially solvable.P4 - Implementation. Someone has to do it and the changes aren't exactly small. The projects owners should be asked if they'd accept the PR.P5 - Usability. A possibility to create an immutable cyclic object graph is fantastic, but the supplier stuff is a pain. I guess, most users would ignore this new feature (and it's good that nothing changes for them), but those who'd want to use it would complain.I guess, it's possible to avoid some problems by using the Builder as both the consumer and producer,
ideally without letting them implement the interface. I guess, something like Builder::getOrBuild() (returning the last created instance, if any; otherwise creating and caching a new one) would do. Maybe changing the semantics of Builder::build would be fine, too (e.g., the caching would only get activated by incompletePublishing = true and the cache would get cleared on any change, so that even such builders remain reusable). Am I right?
Actually, incompletePublishing is misnomer, given that the fields can be final. Maybe caching or supportCycles would do.
P4 is the big problem.
victorwss
commented
6 years ago @randakar and @Maaartinus. Let me try again:
public final class A {
private final B b;
private final C c;
private A(ABuilder $lombokTemp0) {
if ($lombokTemp0.b == null) throw new NullPointerException("b");
if ($lombokTemp0.c == null) throw new NullPointerException("c");
$lombokTemp0.$lombokLastInstance = this;
this.b = $lombokTemp0.b.getOrBuild();
this.c = $lombokTemp0.c.getOrBuild();
}
public B getB() {
return b;
}
public C getC() {
return c;
}
public static ABuilder builder() {
return new ABuilder();
}
public ABuilderFactoryInterface $lombokHold() {
return new ABuilderFactoryInterface() {
@Override
public A getOrBuild() {
return A.this;
}
};
}
// equals(Object), hashCode() and toString() methods ommited.
public static final class ABuilder implements ABuilderFactoryInterface {
private B.BBuilderFactoryInterface b;
private C.CBuilderFactoryInterface c;
private A $lombokLastInstance;
private ABuilder() {
}
public ABuilder b(B b) {
$lombokLastInstance = null;
this.b = b.$lombokHold();
return this;
}
public ABuilder b(B.BBuilder b) {
$lombokLastInstance = null;
this.b = b;
return this;
}
public ABuilder c(C c) {
$lombokLastInstance = null;
this.c = c.$lombokHold();
return this;
}
public ABuilder suppliedC(C.CBuilder c) {
$lombokLastInstance = null;
this.c = c;
return this;
}
public A build() {
$lombokLastInstance = new A(this);
return $lombokLastInstance;
}
@Override
public A getOrBuild() {
if ($lombokLastInstance == null) build();
return $lombokLastInstance;
}
// toString() method ommited.
}
public static interface ABuilderFactoryInterface {
public A getOrBuild();
}
}public final class B {
private final A a;
private final C c;
private final String d;
private B(BBuilder $lombokTemp0) {
if ($lombokTemp0.a == null) throw new NullPointerException("a");
if ($lombokTemp0.c == null) throw new NullPointerException("c");
if ($lombokTemp0.d == null) throw new NullPointerException("d");
$lombokTemp0.$lombokLastInstance = this;
this.a = $lombokTemp0.a.getOrBuild();
this.c = $lombokTemp0.c.getOrBuild();
this.d = $lombokTemp0.d;
}
public A getA() {
return a;
}
public C getC() {
return c;
}
public String getD() {
return d;
}
public static BBuilder builder() {
return new BBuilder();
}
public BBuilderFactoryInterface $lombokHold() {
return new BBuilderFactoryInterface() {
@Override
public B getOrBuild() {
return B.this;
}
};
}
// equals(Object), hashCode() and toString() methods ommited.
public static final class BBuilder implements BBuilderFactoryInterface {
private A.ABuilderFactoryInterface a;
private C.CBuilderFactoryInterface c;
private String d;
private B $lombokLastInstance;
private BBuilder() {
}
public BBuilder a(A a) {
$lombokLastInstance = null;
this.a = a.$lombokHold();
return this;
}
public BBuilder a(A.ABuilder a) {
$lombokLastInstance = null;
this.a = a;
return this;
}
public BBuilder c(C c) {
$lombokLastInstance = null;
this.c = c.$lombokHold();
return this;
}
public BBuilder c(C.CBuilder c) {
$lombokLastInstance = null;
this.c = c;
return this;
}
public BBuilder d(String d) {
$lombokLastInstance = null;
this.d = d;
return this;
}
public B build() {
$lombokLastInstance = new B(this);
return $lombokLastInstance;
}
@Override
public B getOrBuild() {
if ($lombokLastInstance == null) build();
return $lombokLastInstance;
}
// toString() method ommited.
}
public static interface BBuilderFactoryInterface {
public B getOrBuild();
}
}import java.util.List;
public final class C {
private final List<A> as;
private final B b1;
private final B b2;
private C(CBuilder $lombokTemp0) {
if ($lombokTemp0.as == null) throw new NullPointerException("as");
if ($lombokTemp0.b1 == null) throw new NullPointerException("b1");
if ($lombokTemp0.b2 == null) throw new NullPointerException("b2");
$lombokTemp0.$lombokLastInstance = this;
java.util.List<A> $lombokTemp1 = new java.util.ArrayList<A>($lombokTemp0.as.size());
for (A.ABuilderFactoryInterface $lombokTemp2 : $lombokTemp0.as) {
$lombokTemp1.add($lombokTemp2.getOrBuild());
}
this.as = $lombokCopyList($lombokTemp1);
this.b1 = $lombokTemp0.b1.getOrBuild();
this.b2 = $lombokTemp0.b2.getOrBuild();
}
public List<A> getAs() {
return as;
}
public B getB1() {
return b1;
}
public B getB2() {
return b2;
}
public static CBuilder builder() {
return new CBuilder();
}
// equals(Object), hashCode() and toString() methods ommited.
public CBuilderFactoryInterface $lombokHold() {
return new CBuilderFactoryInterface() {
@Override
public C getOrBuild() {
return C.this;
}
};
}
private static <X> java.util.List<X> $lombokCopyList(java.util.List<X> list) {
switch (list.size()) {
case 0:
return java.util.Collections.<X>emptyList();
case 1:
return java.util.Collections.singletonList(list.get(0));
default:
return java.util.Collections.unmodifiableList(list);
}
}
public static class CBuilder implements CBuilderFactoryInterface {
private final java.util.List<A.ABuilderFactoryInterface> as;
private B.BBuilderFactoryInterface b1;
private B.BBuilderFactoryInterface b2;
private C $lombokLastInstance;
private CBuilder() {
this.as = new java.util.ArrayList<A.ABuilderFactoryInterface>();
}
public CBuilder a(A a) {
$lombokLastInstance = null;
this.as.add(a.$lombokHold());
return this;
}
public CBuilder a(A.ABuilder a) {
$lombokLastInstance = null;
this.as.add(a);
return this;
}
public CBuilder as(java.util.Collection<? extends A> as) {
$lombokLastInstance = null;
for (A a : as) {
this.as.add(a.$lombokHold());
}
return this;
}
public CBuilder clearAs() {
$lombokLastInstance = null;
this.as.clear();
return this;
}
public CBuilder b1(B b1) {
$lombokLastInstance = null;
this.b1 = b1.$lombokHold();
return this;
}
public CBuilder b1(B.BBuilder b1) {
$lombokLastInstance = null;
this.b1 = b1;
return this;
}
public CBuilder b2(B b2) {
$lombokLastInstance = null;
this.b2 = b2.$lombokHold();
return this;
}
public CBuilder b2(B.BBuilder b2) {
$lombokLastInstance = null;
this.b2 = b2;
return this;
}
public C build() {
$lombokLastInstance = new C(this);
return $lombokLastInstance;
}
@Override
public C getOrBuild() {
if ($lombokLastInstance == null) build();
return $lombokLastInstance;
}
// toString() method ommited.
}
public static interface CBuilderFactoryInterface {
public C getOrBuild();
}
}public class Test {
public static void main(String[] args) {
// 1. Declare all the builders.
A.ABuilder builderA1 = A.builder();
A.ABuilder builderA2 = A.builder();
B.BBuilder builderB1 = B.builder();
B.BBuilder builderB2 = B.builder();
C.CBuilder builderC = C.builder();
// 2. Populate all the builders.
builderA1.b(builderB1).suppliedC(builderC);
builderA2.b(builderB2).suppliedC(builderC);
builderB1.a(builderA2).c(builderC).d("Cupcake");
builderB2.a(builderA1).c(builderC).d("Brownie");
builderC.a(builderA1).a(builderA2).b1(builderB1).b2(builderB2);
// 3. Produce the objects.
A a1 = builderA1.getOrBuild();
A a2 = builderA2.getOrBuild();
B b1 = builderB1.getOrBuild();
B b2 = builderB2.getOrBuild();
C c = builderC.getOrBuild();
// 4. Assert that the objects are what we wanted.
if (a1.getB() != b1) throw new AssertionError();
if (a2.getB() != b2) throw new AssertionError();
if (b1.getA() != a2) throw new AssertionError();
if (b2.getA() != a1) throw new AssertionError();
if (a1.getC() != c) throw new AssertionError();
if (a2.getC() != c) throw new AssertionError();
if (b1.getC() != c) throw new AssertionError();
if (b2.getC() != c) throw new AssertionError();
if (!"Cupcake".equals(b1.getD())) throw new AssertionError();
if (!"Brownie".equals(b2.getD())) throw new AssertionError();
if (c.getB1() != b1) throw new AssertionError();
if (c.getB2() != b2) throw new AssertionError();
if (c.getAs().size() != 2) throw new AssertionError();
if (c.getAs().get(0) != a1) throw new AssertionError();
if (c.getAs().get(1) != a2) throw new AssertionError();
try {
c.getAs().add(a1);
throw new AssertionError();
} catch (UnsupportedOperationException expected) {
// Swallow it.
}
try {
c.getAs().remove(a1);
throw new AssertionError();
} catch (UnsupportedOperationException expected) {
// Swallow it.
}
// 4. Assert that the builders have expected side-effects if reused.
B b3 = builderB1.getOrBuild();
if (b3 != b1) throw new AssertionError();
builderB1.d("Strawberry");
B b4 = builderB1.getOrBuild();
if (b1 == b4) throw new AssertionError();
B b5 = builderB1.getOrBuild();
if (b5 != b4) throw new AssertionError();
B b6 = builderB1.build();
if (b6 == b4) throw new AssertionError();
B b7 = builderB1.build();
if (b7 == b6) throw new AssertionError();
}
}If I didn't commit any mistake, this should compile and run from Java 5 without any change. Further, no need to have the Cached class nor to directly handle Suppliers.
However it has a cost (most because we're lacking lambdas):
The constructor signature changed and now only receives its builder. This should be ok as long as it only happens when incompletePublishing = true (or whatever we call that).
We need that horrible $lombokHold methods. They are there to emulate Suppliers that gives the instances on which they're called.
We need to publicly define some unpleasant interfaces that are no more than fancy specifications of Suppliers.
I don't know if we will need those interfaces and thos @lombokHold methods for every builder (I'm afraid of that) or only for those specified with incompletePublishing = true.
@Builder.Supplied became a misnomer, since now it is for injecting builder intances instead of Suppliers.
This should fix P1, P2 and P3. P5 improved significantly, and I don't think it is really possibly to improve it anymore, so I consider it solved. this "only" leaves P4 open.
About incompletePublishing = true being a misnomer, I somewhat agree. Its purpose is to tell lombok that the this reference should leak out of the constructor back to the builder through a callback (now an assignment) before the constructor finishes, when all its fields are still blank. Hence, the name. However, from the user point of view, this name makes no sense (and ditto for caching).
Normally, leaking the this out the of the constructor before it finishes is considered a very bad programming practice, but I'm confident that our case is one of the very rare ones where it is being used safely (as long as the user don't try to use the generated constructor directly or share builders between different threads).
Although there might be other cool tricks that could be possible with that other than making cyclic data, I couldn't think about any. So, supportCycles seems to be the best name so far. Also, using this in a class that has no field annotated with @Builder.Supplied is pointless (or if none of the ones annotated with that are capable of producing a cycle), so a warning should be generated in such cases.
randakar
commented
6 years ago Well, P4 is basically "how does this translate to lombok code that generates this stuff???" ;-) Minor nit: At a glance it seems that both the constructors and the call to .build() are setting $lombokLastInstance. Do we really need both of them to do that?
Bigger issue in my mind is "How do the generated constructors collide with the ones we have to generate based on existing annotations?". How do we deal with things like builder annotations on constructors, for instance?
Anyway, trying to wrap my head around this, so a bit of thinking out loud: The trick here seems to be: 1) Generating constructors on objects that take builders for themselves as an argument. 2) Those builders cache previously constructed instances of the object in question so that other objects can get the actual object reference at construction time and things are only constructed once. 3) Doing all of the constructor calls at construction time, using references to each other's builders to fill in the missing data while you also have access to a "this" reference.
This is not thread safe in the sense that multiple calls to build() from multiple threads on the same builder object will quite probably go boom. I think that that is acceptable though, multiple thread sharing instances of builder objects sounds like a very weird thing to do.
Overall this subject is pretty hard to wrap my head around. Sounds like debugging this is going to be 'fun' if it explodes. ;-)
On Tue, Mar 27, 2018 at 8:58 AM, Victor Williams Stafusa da Silva < notifications@github.com> wrote:
@randakar https://github.com/randakar and @Maaartinus https://github.com/Maaartinus. Let me try again:
public final class A { private final B b;
private final C c; private A(ABuilder $lombokTemp0) { if ($lombokTemp0.b == null) throw new NullPointerException("b"); if ($lombokTemp0.c == null) throw new NullPointerException("c"); $lombokTemp0.$lombokLastInstance = this; this.b = $lombokTemp0.b.getOrBuild(); this.c = $lombokTemp0.c.getOrBuild(); } public B getB() { return b; } public C getC() { return c; } public static ABuilder builder() { return new ABuilder(); } public ABuilderFactoryInterface $lombokHold() { return new ABuilderFactoryInterface() { @Override public A getOrBuild() { return A.this; } }; } // equals(Object), hashCode() and toString() methods ommited. public static final class ABuilder implements ABuilderFactoryInterface { private B.BBuilderFactoryInterface b; private C.CBuilderFactoryInterface c; private A $lombokLastInstance; private ABuilder() { } public ABuilder b(B b) { $lombokLastInstance = null; this.b = b.$lombokHold(); return this; } public ABuilder b(B.BBuilder b) { $lombokLastInstance = null; this.b = b; return this; } public ABuilder c(C c) { $lombokLastInstance = null; this.c = c.$lombokHold(); return this; } public ABuilder suppliedC(C.CBuilder c) { $lombokLastInstance = null; this.c = c; return this; } public A build() { $lombokLastInstance = new A(this); return $lombokLastInstance; } @Override public A getOrBuild() { if ($lombokLastInstance == null) build(); return $lombokLastInstance; } // toString() method ommited. } public static interface ABuilderFactoryInterface { public A getOrBuild(); }}
public final class B { private final A a;
private final C c; private final String d; private B(BBuilder $lombokTemp0) { if ($lombokTemp0.a == null) throw new NullPointerException("a"); if ($lombokTemp0.c == null) throw new NullPointerException("c"); if ($lombokTemp0.d == null) throw new NullPointerException("d"); $lombokTemp0.$lombokLastInstance = this; this.a = $lombokTemp0.a.getOrBuild(); this.c = $lombokTemp0.c.getOrBuild(); this.d = $lombokTemp0.d; } public A getA() { return a; } public C getC() { return c; } public String getD() { return d; } public static BBuilder builder() { return new BBuilder(); } public BBuilderFactoryInterface $lombokHold() { return new BBuilderFactoryInterface() { @Override public B getOrBuild() { return B.this; } }; } // equals(Object), hashCode() and toString() methods ommited. public static final class BBuilder implements BBuilderFactoryInterface { private A.ABuilderFactoryInterface a; private C.CBuilderFactoryInterface c; private String d; private B $lombokLastInstance; private BBuilder() { } public BBuilder a(A a) { $lombokLastInstance = null; this.a = a.$lombokHold(); return this; } public BBuilder a(A.ABuilder a) { $lombokLastInstance = null; this.a = a; return this; } public BBuilder c(C c) { $lombokLastInstance = null; this.c = c.$lombokHold(); return this; } public BBuilder c(C.CBuilder c) { $lombokLastInstance = null; this.c = c; return this; } public BBuilder d(String d) { $lombokLastInstance = null; this.d = d; return this; } public B build() { $lombokLastInstance = new B(this); return $lombokLastInstance; } @Override public B getOrBuild() { if ($lombokLastInstance == null) build(); return $lombokLastInstance; } // toString() method ommited. } public static interface BBuilderFactoryInterface { public B getOrBuild(); }}
import java.util.List; public final class C { private final List as;
private final B b1; private final B b2; private C(CBuilder $lombokTemp0) { if ($lombokTemp0.as == null) throw new NullPointerException("as"); if ($lombokTemp0.b1 == null) throw new NullPointerException("b1"); if ($lombokTemp0.b2 == null) throw new NullPointerException("b2"); $lombokTemp0.$lombokLastInstance = this; java.util.List<A> $lombokTemp1 = new java.util.ArrayList<A>($lombokTemp0.as.size()); for (A.ABuilderFactoryInterface $lombokTemp2 : $lombokTemp0.as) { $lombokTemp1.add($lombokTemp2.getOrBuild()); } this.as = $lombokCopyList($lombokTemp1); this.b1 = $lombokTemp0.b1.getOrBuild(); this.b2 = $lombokTemp0.b2.getOrBuild(); } public List<A> getAs() { return as; } public B getB1() { return b1; } public B getB2() { return b2; } public static CBuilder builder() { return new CBuilder(); } // equals(Object), hashCode() and toString() methods ommited. public CBuilderFactoryInterface $lombokHold() { return new CBuilderFactoryInterface() { @Override public C getOrBuild() { return C.this; } }; } private static <X> java.util.List<X> $lombokCopyList(java.util.List<X> list) { switch (list.size()) { case 0: return java.util.Collections.<X>emptyList(); case 1: return java.util.Collections.singletonList(list.get(0)); default: return java.util.Collections.unmodifiableList(list); } } public static class CBuilder implements CBuilderFactoryInterface { private final java.util.List<A.ABuilderFactoryInterface> as; private B.BBuilderFactoryInterface b1; private B.BBuilderFactoryInterface b2; private C $lombokLastInstance; private CBuilder() { this.as = new java.util.ArrayList<A.ABuilderFactoryInterface>(); } public CBuilder a(A a) { $lombokLastInstance = null; this.as.add(a.$lombokHold()); return this; } public CBuilder a(A.ABuilder a) { $lombokLastInstance = null; this.as.add(a); return this; } public CBuilder as(java.util.Collection<? extends A> as) { $lombokLastInstance = null; for (A a : as) { this.as.add(a.$lombokHold()); } return this; } public CBuilder clearAs() { $lombokLastInstance = null; this.as.clear(); return this; } public CBuilder b1(B b1) { $lombokLastInstance = null; this.b1 = b1.$lombokHold(); return this; } public CBuilder b1(B.BBuilder b1) { $lombokLastInstance = null; this.b1 = b1; return this; } public CBuilder b2(B b2) { $lombokLastInstance = null; this.b2 = b2.$lombokHold(); return this; } public CBuilder b2(B.BBuilder b2) { $lombokLastInstance = null; this.b2 = b2; return this; } public C build() { $lombokLastInstance = new C(this); return $lombokLastInstance; } @Override public C getOrBuild() { if ($lombokLastInstance == null) build(); return $lombokLastInstance; } // toString() method ommited. } public static interface CBuilderFactoryInterface { public C getOrBuild(); }}
public class Test { public static void main(String[] args) { // 1. Declare all the builders. A.ABuilder builderA1 = A.builder(); A.ABuilder builderA2 = A.builder(); B.BBuilder builderB1 = B.builder(); B.BBuilder builderB2 = B.builder(); C.CBuilder builderC = C.builder();
// 2. Populate all the builders. builderA1.b(builderB1).suppliedC(builderC); builderA2.b(builderB2).suppliedC(builderC); builderB1.a(builderA2).c(builderC).d("Cupcake"); builderB2.a(builderA1).c(builderC).d("Brownie"); builderC.a(builderA1).a(builderA2).b1(builderB1).b2(builderB2); // 3. Produce the objects. A a1 = builderA1.getOrBuild(); A a2 = builderA2.getOrBuild(); B b1 = builderB1.getOrBuild(); B b2 = builderB2.getOrBuild(); C c = builderC.getOrBuild(); // 4. Assert that the objects are what we wanted. if (a1.getB() != b1) throw new AssertionError(); if (a2.getB() != b2) throw new AssertionError(); if (b1.getA() != a2) throw new AssertionError(); if (b2.getA() != a1) throw new AssertionError(); if (a1.getC() != c) throw new AssertionError(); if (a2.getC() != c) throw new AssertionError(); if (b1.getC() != c) throw new AssertionError(); if (b2.getC() != c) throw new AssertionError(); if (!"Cupcake".equals(b1.getD())) throw new AssertionError(); if (!"Brownie".equals(b2.getD())) throw new AssertionError(); if (c.getB1() != b1) throw new AssertionError(); if (c.getB2() != b2) throw new AssertionError(); if (c.getAs().size() != 2) throw new AssertionError(); if (c.getAs().get(0) != a1) throw new AssertionError(); if (c.getAs().get(1) != a2) throw new AssertionError(); try { c.getAs().add(a1); throw new AssertionError(); } catch (UnsupportedOperationException expected) { // Swallow it. } try { c.getAs().remove(a1); throw new AssertionError(); } catch (UnsupportedOperationException expected) { // Swallow it. } // 4. Assert that the builders have expected side-effects if reused. B b3 = builderB1.getOrBuild(); if (b3 != b1) throw new AssertionError(); builderB1.d("Strawberry"); B b4 = builderB1.getOrBuild(); if (b1 == b4) throw new AssertionError(); B b5 = builderB1.getOrBuild(); if (b5 != b4) throw new AssertionError(); B b6 = builderB1.build(); if (b6 == b4) throw new AssertionError(); B b7 = builderB1.build(); if (b7 == b6) throw new AssertionError(); }}
If I didn't commit any mistake, this should compile and run from Java 5 without any change. Further, no need to have the Cached class nor to directly handle Suppliers.
However it has a cost (most because we're lacking lambdas):
1.
The constructor signature changed and now only receives its builder. This should be ok as long as it only happens when incompletePublishing = true (or whatever we call that). 2.
We need that horrible $lombokHold methods. They are there to emulate Suppliers that gives the instances on which they're called. 3.
We need to publicly define some unpleasant interfaces that are no more than fancy specifications of Suppliers. 4.
I don't know if we will need those interfaces and thos @lombokHold methods for every builder (I'm afraid of that) or only for those specified with incompletePublishing = true. 5.
@Builder.Supplied became a misnomer, since now it is for injecting builder intances instead of Suppliers.
This should fix P1, P2 and P3. P5 improved significantly, and I don't think it is really possibly to improve it anymore, so I consider it solved. this "only" leaves P4 open.
About incompletePublishing = true being a misnomer, I somewhat agree. Its purpose is to tell lombok that the this reference should leak out of the constructor back to the builder through a callback (now an assignment) before the constructor finishes, when all its fields are still blank. Hence, the name. However, from the user point of view, this name makes no sense (and ditto for caching).
Normally, leaking the this out the of the constructor before it finishes is considered a very bad programming practice, but I'm confident that our case is one of the very rare ones where it is being used safely (as long as the user don't try to use the generated constructor directly or share builders between different threads).
Although there might be other cool tricks that could be possible with that other than making cyclic data, I couldn't think about any. So, supportCycles seems to be the best name so far. Also, using this in a class that has no field annotated with @Builder.Supplied is pointless (or if none of the ones annotated with that are capable of producing a cycle), so a warning should be generated in such cases.
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/rzwitserloot/lombok/issues/1623#issuecomment-376417982, or mute the thread https://github.com/notifications/unsubscribe-auth/AAKCRQVdSNHtOdXMLOz-sdkg0M1M_uCCks5tieL6gaJpZM4S27zl .
-- "Don't only practice your art, but force your way into it's secrets, for it and knowledge can raise men to the divine." -- Ludwig von Beethoven
victorwss
commented
6 years ago @randakar Oh, that minor nit slipped off by accident. Although it is harmless, only the assignment in the constructor is needed.
About the rest, it is exactly that.
rspilker
commented
6 years ago And then the equals, hashcode and toString have an infinite loop or stackoverflow?
randakar
commented
6 years ago You have to exclude fields from those methods to prevent that. There's nothing new there.
Op ma 9 apr. 2018 23:48 schreef Roel Spilker notifications@github.com:
And then the equals, hashcode and toString have an infinite loop or stackoverflow?
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/rzwitserloot/lombok/issues/1623#issuecomment-379904732, or mute the thread https://github.com/notifications/unsubscribe-auth/AAKCRSdjJJTttf3SAp4jqeiRVhBdX8OCks5tm9dCgaJpZM4S27zl .
Maaartinus
commented
6 years ago @randakar But depending on what fields you exclude, you'll get different results.
Moreover, even for a trivial cycle a <-> b, no matter how you break the cycle, you'll get one hashCode fully ignoring the other object. That's not nice. The same for equals.
Anyway, when Lombok creates such a beast, it has to be able to handle it, too. Automatically. And that in an easy to follow way. I guess, a depth first search avoiding repetitions using an IdentityHashSet is the obvious choice. Moreover, it doesn't ignore any member of the object graph.
Compared to the crazy @Builder, it's rather trivial. It should be switched on by the presence of the crazy builder.
The other thing it that such a graph will still drive crazy about every tool (JSON, XML, ...).
randakar
commented
6 years ago The thing is, objects with circular references have this problem regardless of how you look at it. And in some cases the circles can't really be avoided.
There is a case to be made that in such a case the two objects really cannot have their identity defined by their link to the other object type. They shouldn't have. If you have a basket of apples, the identity of the apples is not defined by the basket it's in, nor is the identity of the basket defined to the precise set of apples contained within it.
So it should be up to the developer to manually break them. Lombok can't do that for you. Nor should it - lombok is a boilerplate reduction tool, not a solve-the-world fix everything artificial intelligence that understands what you mean before you even start writing code ;-)
On Tue, Apr 10, 2018 at 8:22 AM, Maaartinus notifications@github.com wrote:
@randakar https://github.com/randakar But depending on what fields you exclude, you'll get different results.
Moreover, even for a trivial cycle a <-> b, no matter how you break the cycle, you'll get one hashCode fully ignoring the other object. That's not nice. The same for equals.
Anyway, when Lombok creates such a beast, it has to be able to handle it, too. Automatically. And that in an easy to follow way. I guess, a depth first search avoiding repetitions using an IdentityHashSet is the obvious choice. Moreover, it doesn't ignore any member of the object graph.
Compared to the crazy @Builder, it's rather trivial. It should be switched on by the presence of the crazy builder.
The other thing it that such a graph will still drive crazy about every tool (JSON, XML, ...).
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/rzwitserloot/lombok/issues/1623#issuecomment-379987442, or mute the thread https://github.com/notifications/unsubscribe-auth/AAKCRa6Wpb3gG9b6FTQl0arZh12xmbZ3ks5tnE-ggaJpZM4S27zl .
-- "Don't only practice your art, but force your way into it's secrets, for it and knowledge can raise men to the divine." -- Ludwig von Beethoven
wheredevel
commented
6 years ago +1000
This is really an important issue. BUG really.
Would cause StackOverflow and crash!!!
Jackson has Forward/Backward reference annotation, or something. Please, implement it the same way to indicate Equals/ToString should work unidirectionally.
With an immutable class like
@Getter @Builder public class ImmutableThing { private final ImmutableThing otherThing; }we would love to be able to doImmutableThing.ImmutableThingBuilder thingBuilder = ImmutableThing.builder(); thingBuilder.otherThing(thingBuilder);in order to instantiate the class with circular references. Currently, the builder that lombok generates does not seem to allow passing builders instead of actual instances.