New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

As a User I want to be able to use @CopyConstructor annotation to get rid of the boilerplate code #908

Closed
Krasnyanskiy opened this Issue Aug 31, 2015 · 34 comments

Comments

Projects
None yet
@Krasnyanskiy

Krasnyanskiy commented Aug 31, 2015

Suppose I have a class with a copy constructor:

@Data
class MyEntity {
    private String column;
    private String type;

    public MyEntity(MyEntity entity){
        this.column = entity.getColumn();
        this.type = entity.getType();
    }
}

It's nice to get rid of boilerplate code and replace it with annotation @CopyConstructor to let Lombok autogenerate a copy constructor for me.

@Data
@CopyConstructor
class MyEntity {
    private String column;
    private String type;
}

Reinier, what do you think about that?

PS

It's even better to provide an annotation with a value which describes the copying type:

@Data
@CopyConstructor(type = SHALLOW) // or DEEP
class MyEntity {
    private String column;
    private String type;
}
@luanpotter

This comment has been minimized.

Show comment
Hide comment
@luanpotter

luanpotter Sep 1, 2015

Contributor

I think that is a very useful feature; would love seeing that :)
It would only take some concern as to whether it would shallow or deep, and how to control that.

Contributor

luanpotter commented Sep 1, 2015

I think that is a very useful feature; would love seeing that :)
It would only take some concern as to whether it would shallow or deep, and how to control that.

@Krasnyanskiy

This comment has been minimized.

Show comment
Hide comment
@Krasnyanskiy

Krasnyanskiy Sep 14, 2015

@luanpotter Good point! It's always possible to specify a copy constructor type. I think it would be easy to implement such feature in Lombok. I found a few libs for cloning objects which can be useful: kryo, cloning, dozer.

And here is a possible API for copy constructor feature with respect to copy type:

@Data
@CopyConstructor(type = SHALLOW)
class MyEntity {
    private String column;
    private String type;
}
@Documented
@Target(ElementType.TYPE)
@Retention(RetentionPolicy.SOURCE)
@interface CopyConstructor {
    CopyConstructorType type() default CopyConstructorType.DEEP;
}
enum CopyConstructorType {
    DEEP,
    SHALLOW
}

Krasnyanskiy commented Sep 14, 2015

@luanpotter Good point! It's always possible to specify a copy constructor type. I think it would be easy to implement such feature in Lombok. I found a few libs for cloning objects which can be useful: kryo, cloning, dozer.

And here is a possible API for copy constructor feature with respect to copy type:

@Data
@CopyConstructor(type = SHALLOW)
class MyEntity {
    private String column;
    private String type;
}
@Documented
@Target(ElementType.TYPE)
@Retention(RetentionPolicy.SOURCE)
@interface CopyConstructor {
    CopyConstructorType type() default CopyConstructorType.DEEP;
}
enum CopyConstructorType {
    DEEP,
    SHALLOW
}
@smoyer64

This comment has been minimized.

Show comment
Hide comment
@smoyer64

smoyer64 Sep 22, 2015

I'll also cast a vote for an annotation to create a copy constructor. In my typical use-cases, I'd say that I use a deep copy greater than 90% of the time, so that seems like a sensible default (to me).

smoyer64 commented Sep 22, 2015

I'll also cast a vote for an annotation to create a copy constructor. In my typical use-cases, I'd say that I use a deep copy greater than 90% of the time, so that seems like a sensible default (to me).

@awhitford

This comment has been minimized.

Show comment
Hide comment
@awhitford

awhitford Oct 9, 2015

Contributor

+1

This could help avoid some stupid hand-coding bugs.

Contributor

awhitford commented Oct 9, 2015

+1

This could help avoid some stupid hand-coding bugs.

@rspilker

This comment has been minimized.

Show comment
Hide comment
@rspilker

rspilker Nov 16, 2015

Collaborator

What about using @Builder(toBuilder = true)? In that case you can use

@Builder(toBuilder = true)
class Foo {
    // fields, etc
}

Foo foo = getReferenceToFooInstance();
Foo copy = foo.toBuilder().build();

Wouldn't that be enough?

Collaborator

rspilker commented Nov 16, 2015

What about using @Builder(toBuilder = true)? In that case you can use

@Builder(toBuilder = true)
class Foo {
    // fields, etc
}

Foo foo = getReferenceToFooInstance();
Foo copy = foo.toBuilder().build();

Wouldn't that be enough?

@nyakto

This comment has been minimized.

Show comment
Hide comment
@nyakto

nyakto Nov 16, 2015

@rspilker Any benefit in builder here? @Data classes are mutable and can be considered as builders themselves (if copy constructor is available).

nyakto commented Nov 16, 2015

@rspilker Any benefit in builder here? @Data classes are mutable and can be considered as builders themselves (if copy constructor is available).

@nyakto

This comment has been minimized.

Show comment
Hide comment
@nyakto

nyakto Nov 16, 2015

@rspilker Sorry, didn't know there is already existing @Builder functionality )
Cool! It's a way to go!

nyakto commented Nov 16, 2015

@rspilker Sorry, didn't know there is already existing @Builder functionality )
Cool! It's a way to go!

@stephanos

This comment has been minimized.

Show comment
Hide comment
@stephanos

stephanos Feb 1, 2016

I assume the @Builder(toBuilder = true) trick only creates shallow copies of collections?

stephanos commented Feb 1, 2016

I assume the @Builder(toBuilder = true) trick only creates shallow copies of collections?

@Krasnyanskiy Krasnyanskiy changed the title from As a User I want to be able to use @CopyConstructor annotation to replace boilerplate code to As a User I want to be able to use @CopyConstructor annotation to to get rid of the boilerplate code Jun 3, 2016

@Krasnyanskiy Krasnyanskiy changed the title from As a User I want to be able to use @CopyConstructor annotation to to get rid of the boilerplate code to As a User I want to be able to use @CopyConstructor annotation to get rid of the boilerplate code Jun 3, 2016

@rspilker

This comment has been minimized.

Show comment
Hide comment
@rspilker

rspilker Jul 22, 2016

Collaborator

Please stop posting +1 comments, and use the thumbs-up button under the original post.

Collaborator

rspilker commented Jul 22, 2016

Please stop posting +1 comments, and use the thumbs-up button under the original post.

@saboteurkid

This comment has been minimized.

Show comment
Hide comment
@saboteurkid

saboteurkid Oct 31, 2016

Hi guys,
Is there any plan for this feature?
@rspilker 👍 tks, I think toBuilder is an acceptable alternative.

saboteurkid commented Oct 31, 2016

Hi guys,
Is there any plan for this feature?
@rspilker 👍 tks, I think toBuilder is an acceptable alternative.

@timjacobi

This comment has been minimized.

Show comment
Hide comment
@timjacobi

timjacobi Dec 14, 2016

Do you accept PRs? Would love to look at this.

timjacobi commented Dec 14, 2016

Do you accept PRs? Would love to look at this.

@rspilker

This comment has been minimized.

Show comment
Hide comment
@rspilker

rspilker Dec 14, 2016

Collaborator

Before you start working on a pull request, we first need to figure out how to handle deep copies. Also, we need to find out why people would like to create a copy at all. In my experience, having Immutable objects is gaining more traction. For shallow copies we already have the toBuilder trick. What gap would adding CopyConstructor fill?

Collaborator

rspilker commented Dec 14, 2016

Before you start working on a pull request, we first need to figure out how to handle deep copies. Also, we need to find out why people would like to create a copy at all. In my experience, having Immutable objects is gaining more traction. For shallow copies we already have the toBuilder trick. What gap would adding CopyConstructor fill?

@Maaartinus

This comment has been minimized.

Show comment
Hide comment
@Maaartinus

Maaartinus Dec 14, 2016

Contributor

@rspilker The deep copies thing is surely important, but note that it applies also to other constructors, setters and getters, where it wasn't considered to be a blocker. Here it's more important, but this feature could be started without deep copying, e.g., as @CopyConstructor(value=SHALLOW) with SHALLOW being the default and (for now) the only enum member.

For shallow copies we already have the toBuilder trick.

We do, but it's ugly and inefficient. It's very ugly as you only need copying for mutable objects, which usually need no Builder. It's inefficient as it does the copying twice.

Contributor

Maaartinus commented Dec 14, 2016

@rspilker The deep copies thing is surely important, but note that it applies also to other constructors, setters and getters, where it wasn't considered to be a blocker. Here it's more important, but this feature could be started without deep copying, e.g., as @CopyConstructor(value=SHALLOW) with SHALLOW being the default and (for now) the only enum member.

For shallow copies we already have the toBuilder trick.

We do, but it's ugly and inefficient. It's very ugly as you only need copying for mutable objects, which usually need no Builder. It's inefficient as it does the copying twice.

@omega09

This comment has been minimized.

Show comment
Hide comment
@omega09

omega09 Jan 29, 2017

Deep copying is very complicated and personally I don't think it's Lombok's job to do this.

The common technique is to serialize and deserialize using object streams because most object graphs are too complicated. You lose transient data this way. No reason for Lombok to do this for you, there are tools like Apache commons that can do it.

The other way is to traverse the object graph recursively and copy each object in the graph. It's like recursively annotating a shallow copy constructor. Here you'll have to take care of collections since they require to deep copy their elements. Since not all classes in the JDK permit this functionality, I don't think it'll be possible.

I agree with Maaartinus to implement a shallow copy constructor.

omega09 commented Jan 29, 2017

Deep copying is very complicated and personally I don't think it's Lombok's job to do this.

The common technique is to serialize and deserialize using object streams because most object graphs are too complicated. You lose transient data this way. No reason for Lombok to do this for you, there are tools like Apache commons that can do it.

The other way is to traverse the object graph recursively and copy each object in the graph. It's like recursively annotating a shallow copy constructor. Here you'll have to take care of collections since they require to deep copy their elements. Since not all classes in the JDK permit this functionality, I don't think it'll be possible.

I agree with Maaartinus to implement a shallow copy constructor.

@omega09

This comment has been minimized.

Show comment
Hide comment
@omega09

omega09 Mar 3, 2017

Please stop with the +1 comments. It notifies subscribers and creates noise. Use a thumbs up emoji on the top post instead.

omega09 commented Mar 3, 2017

Please stop with the +1 comments. It notifies subscribers and creates noise. Use a thumbs up emoji on the top post instead.

@dveyarangi

This comment has been minimized.

Show comment
Hide comment
@dveyarangi

dveyarangi Mar 7, 2017

As an alternative, maybe @builder or @CopyConstructure chaining is possible - make deep copy for member fields that are also annotated with @copyconstructor

dveyarangi commented Mar 7, 2017

As an alternative, maybe @builder or @CopyConstructure chaining is possible - make deep copy for member fields that are also annotated with @copyconstructor

@wdroste

This comment has been minimized.

Show comment
Hide comment
@wdroste

wdroste Mar 7, 2017

As a matter of preference I like what Immutables does..

ImmutableValue.builder() .from(otherValue) // merges attribute value into builder .addBuz("b") .build();

wdroste commented Mar 7, 2017

As a matter of preference I like what Immutables does..

ImmutableValue.builder() .from(otherValue) // merges attribute value into builder .addBuz("b") .build();

@omega09

This comment has been minimized.

Show comment
Hide comment
@omega09

omega09 Jun 21, 2017

@rspilker

What gap would adding CopyConstructor fill?

It could allow to instantiate a class from its superclass, providing a solution to problems like this.

omega09 commented Jun 21, 2017

@rspilker

What gap would adding CopyConstructor fill?

It could allow to instantiate a class from its superclass, providing a solution to problems like this.

@Maaartinus

This comment has been minimized.

Show comment
Hide comment
@Maaartinus

Maaartinus Aug 3, 2017

Contributor

Deep copying is very complicated

@omega09
... and rather underspecified. You obviously mean the creation of a completely new object graph, which (except for immutable members) has nothing in common with the original. I guess, what's usually needed is a sort of depth-one copy: My object contains some mutable parts like collections (or some obsolete crap like Date), which it "owns". The members of these collections may be immutables or other objects, which are not "owned" and needn't be copied. Usually, you don't want to copy all members.

A typical example would be a shopping cart having a User containing a Multiset<Item>. When you need a copy of the cart, you only want to shallowly copy the Multiset, as the user and the items themselves are separate entities, which should stay as they're when you play with the cart.

I agree, that the full-depth copy constructor is outside of the Lombok scope, but I'd like to get a depth-one copy constructor one day. It's still a bit complicated without Lombok being omniscient w.r.t. to copying. However, it's possible to make it configurable:

@Getter @Setter
@CopyConstructor(value=DEPTH_ONE) // better name needed
class ShoppingCart {
    @NoCopier
    private User owner;

    // known to Lombok, copied automatically
    private final Set<Discount> discounts = new HashSet<>();

    @Copier(MyCopiers.Multiset) // user-defined class containing the copying method
    // @Copier(HashMultiset::create) // <- this would be much nicer
    private final Multiset<Item> items = HashMultiset.create();
}

The only argument of @Copier would be Function<T, T>. Now that even Android supports Java 8...

Alternatively, introduce @CopyingMethod working just like @ExtensionMethod does: providing a collection of methods to pick from.

Contributor

Maaartinus commented Aug 3, 2017

Deep copying is very complicated

@omega09
... and rather underspecified. You obviously mean the creation of a completely new object graph, which (except for immutable members) has nothing in common with the original. I guess, what's usually needed is a sort of depth-one copy: My object contains some mutable parts like collections (or some obsolete crap like Date), which it "owns". The members of these collections may be immutables or other objects, which are not "owned" and needn't be copied. Usually, you don't want to copy all members.

A typical example would be a shopping cart having a User containing a Multiset<Item>. When you need a copy of the cart, you only want to shallowly copy the Multiset, as the user and the items themselves are separate entities, which should stay as they're when you play with the cart.

I agree, that the full-depth copy constructor is outside of the Lombok scope, but I'd like to get a depth-one copy constructor one day. It's still a bit complicated without Lombok being omniscient w.r.t. to copying. However, it's possible to make it configurable:

@Getter @Setter
@CopyConstructor(value=DEPTH_ONE) // better name needed
class ShoppingCart {
    @NoCopier
    private User owner;

    // known to Lombok, copied automatically
    private final Set<Discount> discounts = new HashSet<>();

    @Copier(MyCopiers.Multiset) // user-defined class containing the copying method
    // @Copier(HashMultiset::create) // <- this would be much nicer
    private final Multiset<Item> items = HashMultiset.create();
}

The only argument of @Copier would be Function<T, T>. Now that even Android supports Java 8...

Alternatively, introduce @CopyingMethod working just like @ExtensionMethod does: providing a collection of methods to pick from.

@omega09

This comment has been minimized.

Show comment
Hide comment
@omega09

omega09 Aug 9, 2017

For the second time, stop commenting with +1 and thumbs up the top comments instead. You're creating a lot of noise for nothing.

omega09 commented Aug 9, 2017

For the second time, stop commenting with +1 and thumbs up the top comments instead. You're creating a lot of noise for nothing.

@omega09

This comment has been minimized.

Show comment
Hide comment
@omega09

omega09 Aug 11, 2017

@Maaartinus

So delomboking the copy stuff, your class would be

@Getter @Setter
class ShoppingCart {

    public ShoppingCart(ShoppingCart cart) {
        ShoppingCart newCart = new ShoppingCart();
        newCart.setDiscounts(new HashSet<>(cart.getDiscounts());
        newCart.setItems(HashMultiset.create(cart.getItem());
    }

    private User owner;
    private final Set<Discount> discounts = new HashSet<>();
    private final Multiset<Item> items = HashMultiset.create();
}

?

omega09 commented Aug 11, 2017

@Maaartinus

So delomboking the copy stuff, your class would be

@Getter @Setter
class ShoppingCart {

    public ShoppingCart(ShoppingCart cart) {
        ShoppingCart newCart = new ShoppingCart();
        newCart.setDiscounts(new HashSet<>(cart.getDiscounts());
        newCart.setItems(HashMultiset.create(cart.getItem());
    }

    private User owner;
    private final Set<Discount> discounts = new HashSet<>();
    private final Multiset<Item> items = HashMultiset.create();
}

?

@Maaartinus

This comment has been minimized.

Show comment
Hide comment
@Maaartinus

Maaartinus Aug 11, 2017

Contributor

@omega09 Sort of... what you wrote won't compile, as discounts and items are final. (*) Additionally, you are creating a new instance of ShoppingCart in the constructor of ShoppingCart. And you forgot user. Nonetheless, I believe, you've got the idea right.

public ShoppingCart(ShoppingCart cart) {
    user = cart.user;

    // was initially empty, otherwise we'd need also discounts.clear()
    discounts.addAll(cart.discounts);

    // with a final field, the @Copier as specified won't work, but lets ignore it

    // VARIANT 1 (for a non-final field)
    // if lombok knew `HashMultiset` or if we could feed it with HashMultiset::create, then it'd use your line
    newCart.setItems(HashMultiset.create(cart.getItem());

    // VARIANT 2 (for a non-final field)
    // with @Copier(MyCopiers.Multiset) it would do
    newCart.setItems(MyCopiers.Multiset.apply(cart.multiset))
    // the user would most probably implement `apply` using `HashMultiset::create`
}

It turned more complicated than I thought, mostly because of my example not matching what I was thinking about.

Btw., some strange languages have assign, which copies the content of one instance into another. This allows to simulate the copy constructor easily and to reuse existing objects.


(*) Using final with mutable collections which are exposed to the outside makes sometimes sense as it allows to modify them without providing any methods for it. The disadvantage is that any user of the class may keep a copy and modify it at any time later, which leads to hard to find bugs. No idea why I did it.

Contributor

Maaartinus commented Aug 11, 2017

@omega09 Sort of... what you wrote won't compile, as discounts and items are final. (*) Additionally, you are creating a new instance of ShoppingCart in the constructor of ShoppingCart. And you forgot user. Nonetheless, I believe, you've got the idea right.

public ShoppingCart(ShoppingCart cart) {
    user = cart.user;

    // was initially empty, otherwise we'd need also discounts.clear()
    discounts.addAll(cart.discounts);

    // with a final field, the @Copier as specified won't work, but lets ignore it

    // VARIANT 1 (for a non-final field)
    // if lombok knew `HashMultiset` or if we could feed it with HashMultiset::create, then it'd use your line
    newCart.setItems(HashMultiset.create(cart.getItem());

    // VARIANT 2 (for a non-final field)
    // with @Copier(MyCopiers.Multiset) it would do
    newCart.setItems(MyCopiers.Multiset.apply(cart.multiset))
    // the user would most probably implement `apply` using `HashMultiset::create`
}

It turned more complicated than I thought, mostly because of my example not matching what I was thinking about.

Btw., some strange languages have assign, which copies the content of one instance into another. This allows to simulate the copy constructor easily and to reuse existing objects.


(*) Using final with mutable collections which are exposed to the outside makes sometimes sense as it allows to modify them without providing any methods for it. The disadvantage is that any user of the class may keep a copy and modify it at any time later, which leads to hard to find bugs. No idea why I did it.

@omega09

This comment has been minimized.

Show comment
Hide comment
@omega09

omega09 Aug 11, 2017

@Maaartinus

what you wrote won't compile, as discounts and items are final

Yes, I completely ignored that :)

you are creating a new instance of ShoppingCart in the constructor of ShoppingCart

Oops, I changed the example a few times and it remained from a previous iteration where I was using a factory approach:

public static ShoppingCart copyDepthOne(ShoppingCart cart) {
    ShoppingCart newCart = new ShoppingCart(); // now makes sense
    // add/set everything
    return newCart;
}

which is more @Builder territory (and there are a few issues discussing this already).

And you forgot user.

But it's marked with @NoCopier so I though just don't copy. In your example I see that what you meant was a depth 0 (=shallow) copy. So to me it looks like the resolution is: if @NoCopier create a shallow copy, otherwise create a copy of depth as specified in @CopyConstructor(value) .

I would naively see in my mind something like this:

@Getter @Setter
@CopyConstructor
class ShoppingCart {

    // no annotation - no copy
    private Session session;

    @Copy(value = DEPTH_ZERO)
    private User owner;

    @Copy(value = DEPTH_ONE)
    private Collection<Discount> discounts = new CollectionImpl<>();

    @Copy(value = DEPTH_ONE, method = ForeignCollectionImpl::create) // let's assume this works somehow
    private ForeignCollection<Item> items = ForeignCollectionImpl.create();
}

which would create

ShoppingCart(ShoppingCart cart) {
    owner = cart.owner;

    // if we assume a copy constructor exists for the collection:
    discounts = new CollectionImpl<>(cart.getCollectionImpl())
    // or if we assume a setter/addAll exists for the collection's contents
    discounts = new CollectionImpl<>();
    discounts.addAll(cart.getDiscounts());

    // and now for the magic trick - is the given method a copy constructor or an empty constructor?
   items = ForeignCollectionImpl.create(cart.getItems());
}

BUT

  • What if I annotate User owner with DEPTH_ONE? Generate owner = new User(cart.owner);? Doesn't
    this make sense mostly for collections? Is this worth it just for collections and fringe cases?
  • Are the DEPTH_ONE classes supporting their own copy constructors? Are they supporting setters? I'm probably protected by compiler checks on the generated code but can Lombok check this without type resolution? I don't think so.

If this can be done then a full deep copy can be done by recursively applying depth one copy, which would solve the world's problems (Java core serialization anyone?). For DEPTH_ZERO I give my blessings and I think I can implement it myself even, except for maybe edge cases. For deeper copies which aren't just assignment expressions and involve invocations the details need to be really ironed out.

omega09 commented Aug 11, 2017

@Maaartinus

what you wrote won't compile, as discounts and items are final

Yes, I completely ignored that :)

you are creating a new instance of ShoppingCart in the constructor of ShoppingCart

Oops, I changed the example a few times and it remained from a previous iteration where I was using a factory approach:

public static ShoppingCart copyDepthOne(ShoppingCart cart) {
    ShoppingCart newCart = new ShoppingCart(); // now makes sense
    // add/set everything
    return newCart;
}

which is more @Builder territory (and there are a few issues discussing this already).

And you forgot user.

But it's marked with @NoCopier so I though just don't copy. In your example I see that what you meant was a depth 0 (=shallow) copy. So to me it looks like the resolution is: if @NoCopier create a shallow copy, otherwise create a copy of depth as specified in @CopyConstructor(value) .

I would naively see in my mind something like this:

@Getter @Setter
@CopyConstructor
class ShoppingCart {

    // no annotation - no copy
    private Session session;

    @Copy(value = DEPTH_ZERO)
    private User owner;

    @Copy(value = DEPTH_ONE)
    private Collection<Discount> discounts = new CollectionImpl<>();

    @Copy(value = DEPTH_ONE, method = ForeignCollectionImpl::create) // let's assume this works somehow
    private ForeignCollection<Item> items = ForeignCollectionImpl.create();
}

which would create

ShoppingCart(ShoppingCart cart) {
    owner = cart.owner;

    // if we assume a copy constructor exists for the collection:
    discounts = new CollectionImpl<>(cart.getCollectionImpl())
    // or if we assume a setter/addAll exists for the collection's contents
    discounts = new CollectionImpl<>();
    discounts.addAll(cart.getDiscounts());

    // and now for the magic trick - is the given method a copy constructor or an empty constructor?
   items = ForeignCollectionImpl.create(cart.getItems());
}

BUT

  • What if I annotate User owner with DEPTH_ONE? Generate owner = new User(cart.owner);? Doesn't
    this make sense mostly for collections? Is this worth it just for collections and fringe cases?
  • Are the DEPTH_ONE classes supporting their own copy constructors? Are they supporting setters? I'm probably protected by compiler checks on the generated code but can Lombok check this without type resolution? I don't think so.

If this can be done then a full deep copy can be done by recursively applying depth one copy, which would solve the world's problems (Java core serialization anyone?). For DEPTH_ZERO I give my blessings and I think I can implement it myself even, except for maybe edge cases. For deeper copies which aren't just assignment expressions and involve invocations the details need to be really ironed out.

@Maaartinus

This comment has been minimized.

Show comment
Hide comment
@Maaartinus

Maaartinus Aug 12, 2017

Contributor

@omega09

But it's marked with @NoCopier so I though just don't copy.

I see, it was a stupid name. Indeed, the object won't get copied at all, just a reference to it, so @ReferenceCopier might be better. For what you understood, maybe @CopyConstructor.Exclude could be a good name.

// no annotation - no copy

That's very error-prone. Moreover, it makes the @CopyConstructor copy nothing by default!

What if I annotate User owner with DEPTH_ONE? Generate owner = new User(cart.owner);?

I guess so. If there's such an annotation.

Doesn't this make sense mostly for collections?

Yes.

Are the DEPTH_ONE classes supporting their own copy constructors?

Lombok should handle classes it knows (JDK + Guava) perfectly. For others, it should have a simple default strategy like call their copy constructor and let the user handle it, if it doesn't compile. When the compile-time error is obvious, then it's fine. If it should lead to cryptic error, then giving up immediately with a sane message asking the user to provide their copier is better.

If this can be done then a full deep copy can be done by recursively applying depth one copy

I don't think so: You'd lose the object identity and any circle would lead to an infinite graph. IMHO the real full deep copy is grossly overrated, so far I had no single use for it.

the details need to be really ironed out.

Indeed. That's why I wrote that the shallow constructor should be done first and independently. Anything beyond that is orders of magnitude more complicated; even understanding what it should do is hard. IMHO it's impossible to do anything useful (beyond the shallow copy) without letting the user specify their own helper methods (and I see things like @Copy(value=DEPTH_ONE) just like shortcuts for the common cases).

Maybe we should write a copier using an ordinary annotation processor (which is much simpler) or look if someone already did. Or do it using reflection, which is even easier. Then we'd have a reference implementation to play with.

Contributor

Maaartinus commented Aug 12, 2017

@omega09

But it's marked with @NoCopier so I though just don't copy.

I see, it was a stupid name. Indeed, the object won't get copied at all, just a reference to it, so @ReferenceCopier might be better. For what you understood, maybe @CopyConstructor.Exclude could be a good name.

// no annotation - no copy

That's very error-prone. Moreover, it makes the @CopyConstructor copy nothing by default!

What if I annotate User owner with DEPTH_ONE? Generate owner = new User(cart.owner);?

I guess so. If there's such an annotation.

Doesn't this make sense mostly for collections?

Yes.

Are the DEPTH_ONE classes supporting their own copy constructors?

Lombok should handle classes it knows (JDK + Guava) perfectly. For others, it should have a simple default strategy like call their copy constructor and let the user handle it, if it doesn't compile. When the compile-time error is obvious, then it's fine. If it should lead to cryptic error, then giving up immediately with a sane message asking the user to provide their copier is better.

If this can be done then a full deep copy can be done by recursively applying depth one copy

I don't think so: You'd lose the object identity and any circle would lead to an infinite graph. IMHO the real full deep copy is grossly overrated, so far I had no single use for it.

the details need to be really ironed out.

Indeed. That's why I wrote that the shallow constructor should be done first and independently. Anything beyond that is orders of magnitude more complicated; even understanding what it should do is hard. IMHO it's impossible to do anything useful (beyond the shallow copy) without letting the user specify their own helper methods (and I see things like @Copy(value=DEPTH_ONE) just like shortcuts for the common cases).

Maybe we should write a copier using an ordinary annotation processor (which is much simpler) or look if someone already did. Or do it using reflection, which is even easier. Then we'd have a reference implementation to play with.

@omega09

This comment has been minimized.

Show comment
Hide comment
@omega09

omega09 Aug 12, 2017

@Maaartinus

So let's decide on some annotation scheme. It looks to me like we're working with 3 options: exclude, shallow, and depth 1. Should the latter be the default for collections* while for all other types the default is shallow? Is the level specified in the class annotation and then overriden in the field annotations?

*Don't think we can check is a field is a collection in Lombok, while we can with reflection.

omega09 commented Aug 12, 2017

@Maaartinus

So let's decide on some annotation scheme. It looks to me like we're working with 3 options: exclude, shallow, and depth 1. Should the latter be the default for collections* while for all other types the default is shallow? Is the level specified in the class annotation and then overriden in the field annotations?

*Don't think we can check is a field is a collection in Lombok, while we can with reflection.

@Maaartinus

This comment has been minimized.

Show comment
Hide comment
@Maaartinus

Maaartinus Aug 12, 2017

Contributor

@omega09
IIUYC shallow on a field means copy the reference but not the object. That's confusing as the "shallowness" for the whole object means "reference" for the fields. Similarly for depth 1. I'd call it differently:

  • @CopyConstructor.Exclude: leave it as is, probably null
  • @CopyConstructor.Reference: copy the reference to the old object
  • @CopyConstructor.Copy:
    • for known immutables (String, Guava immutables like ImmutableSet, ...), copy the reference
    • for other known collections and maps, create a new one as a shallow copy (i.e., referring to the same objects as the old one)
    • for other known classes, do what's needed (Date.clone or alike)
    • for anything else, invoke their copy constructor, which may or may not compile
  • @CopyConstructor.Copy(MyCopyingHelper.class): invoke MyCopyingHelper.copy(thisField), which may or may not compile, just like @ExtensionMethod

On the whole class, I see three possibilities:

  • @CopyConstructor: a shallow copy, using @CopyConstructor.Reference on fields by default
  • @CopyConstructor(DEPTH_1): a depth 1 copy, using @CopyConstructor.Copy on fields by default
  • @CopyConstructor(MyCopyingHelper.class): a user-defined constructor, using @CopyConstructor.Copy(MyCopyingHelper.class) on fields by default

Don't think we can check is a field is a collection in Lombok

Agreed. I'd only check for known classes according to their declared type.

Contributor

Maaartinus commented Aug 12, 2017

@omega09
IIUYC shallow on a field means copy the reference but not the object. That's confusing as the "shallowness" for the whole object means "reference" for the fields. Similarly for depth 1. I'd call it differently:

  • @CopyConstructor.Exclude: leave it as is, probably null
  • @CopyConstructor.Reference: copy the reference to the old object
  • @CopyConstructor.Copy:
    • for known immutables (String, Guava immutables like ImmutableSet, ...), copy the reference
    • for other known collections and maps, create a new one as a shallow copy (i.e., referring to the same objects as the old one)
    • for other known classes, do what's needed (Date.clone or alike)
    • for anything else, invoke their copy constructor, which may or may not compile
  • @CopyConstructor.Copy(MyCopyingHelper.class): invoke MyCopyingHelper.copy(thisField), which may or may not compile, just like @ExtensionMethod

On the whole class, I see three possibilities:

  • @CopyConstructor: a shallow copy, using @CopyConstructor.Reference on fields by default
  • @CopyConstructor(DEPTH_1): a depth 1 copy, using @CopyConstructor.Copy on fields by default
  • @CopyConstructor(MyCopyingHelper.class): a user-defined constructor, using @CopyConstructor.Copy(MyCopyingHelper.class) on fields by default

Don't think we can check is a field is a collection in Lombok

Agreed. I'd only check for known classes according to their declared type.

@omega09

This comment has been minimized.

Show comment
Hide comment
@omega09

omega09 Aug 14, 2017

@Maaartinus

Alright, I'll fork something this weekend.

omega09 commented Aug 14, 2017

@Maaartinus

Alright, I'll fork something this weekend.

Repository owner deleted a comment from ractive Nov 27, 2017

Repository owner deleted a comment from ojacquemart Nov 27, 2017

Repository owner deleted a comment from thisisthekap Nov 27, 2017

Repository owner deleted a comment from nyakto Nov 27, 2017

Repository owner deleted a comment from alexpetroaica Nov 27, 2017

Repository owner deleted a comment from iprateek Nov 27, 2017

Repository owner deleted a comment from Waize Nov 27, 2017

Repository owner deleted a comment from greg-witczak Nov 27, 2017

Repository owner deleted a comment from Geniy00 Nov 27, 2017

Repository owner deleted a comment from peeeto Nov 27, 2017

Repository owner deleted a comment from huangkang-chn Nov 27, 2017

Repository owner deleted a comment from naoto243 Nov 27, 2017

Repository owner deleted a comment from burcakulug Nov 27, 2017

Repository owner deleted a comment from seales Nov 27, 2017

Repository owner deleted a comment from cheruvian Nov 27, 2017

Repository owner deleted a comment from wicksome Nov 27, 2017

Repository owner deleted a comment from asiftasleem Nov 27, 2017

Repository owner deleted a comment from achimbitzer Nov 27, 2017

Repository owner deleted a comment from lifedemons Nov 27, 2017

Repository owner deleted a comment from jibumjung Nov 27, 2017

Repository owner deleted a comment from monster28g Nov 27, 2017

Repository owner deleted a comment from uladkaminski Nov 27, 2017

Repository owner deleted a comment from beihaifeiwu Nov 27, 2017

@balajeetm

This comment has been minimized.

Show comment
Hide comment
@balajeetm

balajeetm Dec 21, 2017

Why is this still open? It's very useful feature indeed.

balajeetm commented Dec 21, 2017

Why is this still open? It's very useful feature indeed.

@omega09

This comment has been minimized.

Show comment
Hide comment
@omega09

omega09 Dec 21, 2017

@balajeetm You mean why it's not implemented? It needs to be finished and then a PR need to get reviewed. I more or less forgot about it since I posted the initial implementation. The previous post's "Copy behavior" section contains some questions that need to be answered.

omega09 commented Dec 21, 2017

@balajeetm You mean why it's not implemented? It needs to be finished and then a PR need to get reviewed. I more or less forgot about it since I posted the initial implementation. The previous post's "Copy behavior" section contains some questions that need to be answered.

@archenroot

This comment has been minimized.

Show comment
Hide comment
@archenroot

archenroot Jun 17, 2018

@omega09 - Can you make summary of questions open regarding this cool feature?

archenroot commented Jun 17, 2018

@omega09 - Can you make summary of questions open regarding this cool feature?

@janrieke

This comment has been minimized.

Show comment
Hide comment
@janrieke

janrieke Jun 18, 2018

Contributor

What about a parameter to control whether to call the @CopyConstructor of the superclass (instead of calling super())? This would allow for copying fields from superclasses.

Contributor

janrieke commented Jun 18, 2018

What about a parameter to control whether to call the @CopyConstructor of the superclass (instead of calling super())? This would allow for copying fields from superclasses.

@FeanorsCurse

This comment has been minimized.

Show comment
Hide comment
@FeanorsCurse

FeanorsCurse Aug 7, 2018

I just tried the Builder and it's not working properly, see also: peichhorn/lombok-pg#78 - it is not considering inherited attributes, which means it's basically unusable for me (want a replacement for clone()). So I would also prefer a dedicated @copyconstructor, along with option to set visibility to private.

FeanorsCurse commented Aug 7, 2018

I just tried the Builder and it's not working properly, see also: peichhorn/lombok-pg#78 - it is not considering inherited attributes, which means it's basically unusable for me (want a replacement for clone()). So I would also prefer a dedicated @copyconstructor, along with option to set visibility to private.

@rzwitserloot

This comment has been minimized.

Show comment
Hide comment
@rzwitserloot

rzwitserloot Oct 15, 2018

Owner

Making a deep clone is not possible: There is no general way to clone objects; you need to know what you're dealing with. Integer, String, etc are immutable and a deep clone should just copy the reference, though sometimes for exotic reasons you really do want to deep clone an immutable in which case you need custom code.

arrays and such need special treatment to clone. You can't just go around and call clone() on things; clone() generally makes shallow clones, and once you commit to making a deep clone, that should be turtles all the way down.

Thus, the only feasible thing we can add is a shallow clone, and toBuilder already suffices for that use case.

We will not add this; please don't waste your time with PRs or further discussion.

Owner

rzwitserloot commented Oct 15, 2018

Making a deep clone is not possible: There is no general way to clone objects; you need to know what you're dealing with. Integer, String, etc are immutable and a deep clone should just copy the reference, though sometimes for exotic reasons you really do want to deep clone an immutable in which case you need custom code.

arrays and such need special treatment to clone. You can't just go around and call clone() on things; clone() generally makes shallow clones, and once you commit to making a deep clone, that should be turtles all the way down.

Thus, the only feasible thing we can add is a shallow clone, and toBuilder already suffices for that use case.

We will not add this; please don't waste your time with PRs or further discussion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment