Setter/Getter and constructor with support for copies

[Michael Piefel]

For some objects, it might not be a good idea to just assign or
initialize fields in a setter or constructor, or return the field in
the getter. You might expose the internal representation. Note that
Findbugs will complain about the following:

class Birthday {
private Date birthday;
public Birthday(Date birthday) { this.birthday = birthday; }
public void setBirthday(Date birthday) { this.birthday =
birthday; }
public Date getBirthday() { return birthday; }
}

The problem is that you can alter the objects state from the outside
by keeping a reference to the Date that you passed in or got out, and
set the time on that date. It is recommended to use clone() here, for
example.

It’s a small hassle to write getter and setter. Unfortunately, also
all generated constructors suffer from this, and since they set all
the other fields as well, that’s a bit of work.

I would wish for a way to specify that clone() shall be used. Or
perhaps a ‘copy constructor’ or some other specified function.

[Fabrizio Giudici]

On Fri, 23 Mar 2012 16:44:26 +0100, Michael Piefel
<michae...@googlemail.com> wrote:

> The problem is that you can alter the objects state from the outside
> by keeping a reference to the Date that you passed in or got out, and
> set the time on that date. It is recommended to use clone() here, for
> example.
>
> It’s a small hassle to write getter and setter. Unfortunately, also
> all generated constructors suffer from this, and since they set all
> the other fields as well, that’s a bit of work.
>
> I would wish for a way to specify that clone() shall be used. Or
> perhaps a ‘copy constructor’ or some other specified function.

I second this. The problem is that there are a lot of different details.
In some cases you have copy constructors (e.g. new Date(date)); in other
cases you don't, and wish clone(); in others you could wanting to do
something entirely different (such as returning a CopyOnWriteArrayList).

But I think that we can figure out a strategy for dealing with the most
frequent cases.

--
Fabrizio Giudici - Java Architect, Project Manager
Tidalwave s.a.s. - "We make Java work. Everywhere."
fabrizio...@tidalwave.it
http://tidalwave.it - http://fabriziogiudici.it

[Reinier Zwitserloot]

I can't think of a sound way to actually fix this; how do we know that Date needs defensive copying and Integer doesn't? Coding in specific logic for stuff in java.util.* does not sound like a winning strategy because the number of possible java classes is infinite.

There'd have to be some sort of repository of 'copy strategies', which means unless we explicitly break backwards compatibility and choose to make 'no strategy = error' the policy, that you could 'silently' end up doing the wrong thing if your local copy strategy database does not include the relevant type. It also means the code that is generated, i.e. the 'meaning' of @Setter, depends on your local copy strategies database. Also, it can't just be a list of classes which are safe to just identity-copy and which classes have a copy constructor; the exact way that one makes a defensive copy depends on the class (there's copy constructors, .clone(), doing it by hand, and others).

We did just set a step in this direction (that is, that context relatively far away from a lombok annotation has a grave effect on what the lombok annotation ends up doing) via @Accessors, but that's still always in the same file at least, and it's experimental (as indicated by being in the .experimental package), we may backtrack on that decision.

[Fabrizio Giudici]

On Tue, 27 Mar 2012 01:39:50 +0200, Reinier Zwitserloot
<rein...@gmail.com> wrote:

> I can't think of a sound way to actually fix this; how do we know that
> Date
> needs defensive copying and Integer doesn't? Coding in specific logic for
> stuff in java.util.* does not sound like a winning strategy because the
> number of possible java classes is infinite.

I'm really not thinking of auto-detecting by class. I'm thinking of adding
attributes to the annotations to explicitly let the programmer choose.

E.g.

@Getter(variant=CLONE)

@Getter(variant=COPY_CONST)

@Getter(variant=COPY_CONST, clazz=CopyOnWriteArrayList.class)

etc...

'variant', 'clazz' are not good attribute names, just the idea.

[Michael Piefel]

On 27 Mrz., 01:58, "Fabrizio Giudici" <Fabrizio.Giud...@tidalwave.it>
wrote:

> I'm really not thinking of auto-detecting by class. I'm thinking of adding
> attributes to the annotations to explicitly let the programmer choose.

That’s exactly what I was thinking. It should be configurable, and it
should not change existing behaviour.

Note that the getter is not a good place for the annotation in my
opinion. The strategy applies to the setter and the constructor as
well. Therefore I would prefer a new annotation such as @CopyStrategy:

@Data MyClass {
private Long id;
@CopyStrategy(CLONE)
private Date birthday;
@CopyStrategy(copierFunction="MyClass.copier")
private Foo foo;

public static Foo copier(Foo other) { ... }
}

[Michael Piefel]

Browsing Lombok’s issue database today I saw that the concrete problem I was facing actually resulted in reported issues: See #77 and #347, both could easily be handled by using clone().

[Reinier Zwitserloot]

If its part of getter and setter, you can specify separate strategies for read and write. For example, Date needs defensive copying on both edges, but if you copy a list into CopyOnWriteList on create/set, no defensive cloning is needed on read.

But, that does mean that the constructor either needs to call the setter, or, read the copy strategy from @Setter.

The alternative is that you put a @CopyStrategy on the field, but that would need a way to specify both the read and write strategy which is annoying.

I'm leaning towards:

@Getter(copying=@CopyStrategy(COPY_CONSTRUCTOR, using=CopyOnWriteList.class))

Thoughts?

On Jun 5, 2012 8:16 AM, "Michael Piefel" <michae...@googlemail.com> wrote:

Browsing Lombok’s issue database today I saw that the concrete problem I was facing actually resulted in reported issues: See #77 and #347, both could easily be handled by using clone().

--
You received this message because you are subscribed to the Google
Groups group for http://projectlombok.org/
 
To post to this group, send email to project...@googlegroups.com
To unsubscribe from this group, send email to
project-lombo...@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/project-lombok?hl=en

[Maaartin G]

On Tuesday, June 5, 2012 9:48:53 AM UTC+2, Reinier Zwitserloot wrote:

If its part of getter and setter, you can specify separate strategies for read and write. For example, Date needs defensive copying on both edges, but if you copy a list into CopyOnWriteList on create/set, no defensive cloning is needed on read.

I don't see why it's not needed, unless you really want to allow the reader to modify the owned list.

 

But, that does mean that the constructor either needs to call the setter, or, read the copy strategy from @Setter.

Quite often you want to do the copying in the setter (since it's public) but avoid it in a (package) private constructor, since you own the incoming object.

 

The alternative is that you put a @CopyStrategy on the field, but that would need a way to specify both the read and write strategy which is annoying.

I'd say that in most cases you need the same CopyStrategy for both reads and writes. In the few cases you do not, you just want to omit the copying.

 

I'm leaning towards:

@Getter(copying=@CopyStrategy(COPY_CONSTRUCTOR, using=CopyOnWriteList.class))

I'm not sure if I understand this example. 

For the case you want a copy in the getter but not in the setter, I'd suggest

@CopyStrategy(COPY_CONSTRUCTOR, using=CopyOnWriteList.class)

// @Getter - may be omitted if given on the whole class

@Setter(noCopy=true)

or maybe simply

@CopyStrategy(COPY_CONSTRUCTOR, using=CopyOnWriteList.class, inSetter=false)

[Reinier Zwitserloot]

On Tuesday, June 5, 2012 4:39:05 PM UTC+2, Maaartin G wrote:

On Tuesday, June 5, 2012 9:48:53 AM UTC+2, Reinier Zwitserloot wrote:

If its part of getter and setter, you can specify separate strategies for read and write. For example, Date needs defensive copying on both edges, but if you copy a list into CopyOnWriteList on create/set, no defensive cloning is needed on read.

I don't see why it's not needed, unless you really want to allow the reader to modify the owned list.

Right, nevermind, brainfart. Consider using some sort of ImmutableList arrangement. You want to copy the incoming list into that on write, but you don't need a copy at all on read.

 

 

But, that does mean that the constructor either needs to call the setter, or, read the copy strategy from @Setter.

Quite often you want to do the copying in the setter (since it's public) but avoid it in a (package) private constructor, since you own the incoming object.

This is rapidly heading into this-isnt-boilerplate territory. I don't think it's appropriate for lombok to become a complex DSL for creating objects like this; at the point where people who aren't familiar with lombok are reading this code and can't grasp the intent, we've screwed up.

We need copy strategies for generated constructors, the setter, and the getter. This is quite complicated. We might get away with defining 1 copy strategy and then simply saying, for each of these 3, whether the copy strategy must be used or the reference can be straight up passed/stored, no copying needed, OR, we need to have the ability to let the programmer choose a different copy strategy for each of these 3 things.

 

 

The alternative is that you put a @CopyStrategy on the field, but that would need a way to specify both the read and write strategy which is annoying.

I'd say that in most cases you need the same CopyStrategy for both reads and writes. In the few cases you do not, you just want to omit the copying.

Right. This might result in a less messy API.

 

 

I'm leaning towards:

@Getter(copying=@CopyStrategy(COPY_CONSTRUCTOR, using=CopyOnWriteList.class))

I'm not sure if I understand this example. 

For the case you want a copy in the getter but not in the setter, I'd suggest

@CopyStrategy(COPY_CONSTRUCTOR, using=CopyOnWriteList.class)

// @Getter - may be omitted if given on the whole class

@Setter(noCopy=true)

That would imply we need noCopy on each of the 3 generative things (@XArgsConstructor, @Setter, @Getter), which all default to useCopyStrategy=true. @CopyStrategy would be its own annotation which you stick on a field, similar to your example.

What strategies do we need? I have identified at least these 3, which are IMO must-haves:

(A) copy constructor, using either its own class or an optional alternate class: @ConstructorCopyStrategy(type=HashMap.class)

(B) static method, which requires the name of a class and the name of a static method. This is quite ugly, but guava's ImmutableList needs it, and that's a very important use case: @StaticMethodCopyStrategy(type=ImmutableList.class, method="copyOf").

(C) clone(): @CloneCopyStrategy()

Then some optional ones which are much more complicated and may, hopefully, not be necessary

(D) @ConstructorByFieldCopyStrategy(type=?) - this strategy will not pass 'this', but instead it passes each field in the order listed in the class.

(E) A similar thing but for static methods.

(F) Expand @CloneCopyStrategy to allow an optional method name; there are a few classes out there which opted out of the wonky API particulars of Object.clone(); these usually have a method named 'copy()'.

[Maaartin G]

On Friday, June 8, 2012 5:12:00 PM UTC+2, Reinier Zwitserloot wrote:

On Tuesday, June 5, 2012 4:39:05 PM UTC+2, Maaartin G wrote:

On Tuesday, June 5, 2012 9:48:53 AM UTC+2, Reinier Zwitserloot wrote:

If its part of getter and setter, you can specify separate strategies for read and write. For example, Date needs defensive copying on both edges, but if you copy a list into CopyOnWriteList on create/set, no defensive cloning is needed on read.

I don't see why it's not needed, unless you really want to allow the reader to modify the owned list.

Right, nevermind, brainfart. Consider using some sort of ImmutableList arrangement. You want to copy the incoming list into that on write, but you don't need a copy at all on read.

Yes, but there's one more thing. With a field declared as

@StaticMethodCopyStrategy(type=ImmutableList.class, method="copyOf")

@Getter(noCopy=true) @Setter

List<String> x;

how should the accessors look like? Probably something like

List<String> getX() {

    return x;

}

void setX(List<String> x) {

    this.x = ImmutableList.copyOf(x);

}

But what if you need the return type of the getter to be an ImmutableList? This might be more common then willing a List there. The most obvious way is to declare the field as ImmutableList, but then the setter's argument is an ImmutableList, too, and we need no copying.

This opens the question if the types of both accessors may differ (you must have been assuming this for your example to work), and how to specify them if so. As this makes sense only with a copy strategy, this annotation might be the right place:

@StaticMethodCopyStrategy(type=ImmutableList.class, )

I let the method name default to "copyOf", as it's clearly the best name and systematically used in  Guava.

 

That would imply we need noCopy on each of the 3 generative things (@XArgsConstructor, @Setter, @Getter), which all default to useCopyStrategy=true. @CopyStrategy would be its own annotation which you stick on a field, similar to your example.

Yes. 

 

What strategies do we need? I have identified at least these 3, which are IMO must-haves:

(A) copy constructor, using either its own class or an optional alternate class: @ConstructorCopyStrategy(type=HashMap.class)

  

(B) static method, which requires the name of a class and the name of a static method. This is quite ugly, but guava's ImmutableList needs it, and that's a very important use case: @StaticMethodCopyStrategy(type=ImmutableList.class, method="copyOf").

This as actually more important and more general then the copy constructor (e.g., there's Lists.newHashMap(Map) in Guava).

(C) clone(): @CloneCopyStrategy()

Then some optional ones which are much more complicated and may, hopefully, not be necessary

(D) @ConstructorByFieldCopyStrategy(type=?) - this strategy will not pass 'this', but instead it passes each field in the order listed in the class.

What class does this? Anyway, I'd prefer

@ConstructorCopyStrategy(decompose=true)

which applies to the next bullet as well. 

(E) A similar thing but for static methods.

(F) Expand @CloneCopyStrategy to allow an optional method name; there are a few classes out there which opted out of the wonky API particulars of Object.clone(); these usually have a method named 'copy()'.

It's not complicated at all:

- It can be one of the three basic strategies CONSTRUCTOR, CLONE, STATIC_METHOD.

- The argument "type" makes sense for all of them. It is always optional.

- The argument "method" makes sense except for CONSTRUCTOR. It is always optional and defaults to "clone" and "copyOf", respectively.

- The argument "decompose" makes sense except for CLONE and defaults to false.

Only implementing decompose=true requires resolution, right?

Above I've suggested an argument like "accept=Collection.class", which makes sense except for CLONE.

[Maaartin G]

It slowly becomes clearer to me what I actually need: Nearly always my fields contains something immutable, so I need no copying in getters. With fields mostly final, I rarely need setters, but when I do, their arguments should be transformed the same way as constructor arguments. The same holds for withers.

Summarized, I quite often need some transform on input and seldom on output. The transform should make an immutable copy, and therefore it's mostly a static method. But I'd suggest to replace all the `StaticMethodCopyStrategy`, `ConstructorCopyStrategy`, and `CloneCopyStrategy` by something more general and simpler.

No matter how the method looks like, use a non-static method together with `@ExtensionMethods`. Something like

@ExtensionMethods(MyCopieres.class) // we define all the needed methods therein

@CopyMethodName("toImmutable") // if copying gets used, it calls the method "toImmutable()"

@AllArgsConstructor

@Wither

class C {

    // this is probably a too generous example

    @Incoming(Collection.class) // accept any Collection and convert it via `.toImmutable()`

    private final ImmutableList<String> list;

    // converting an object to an immutable counterpart is surely fairly common

    @Incoming(Set.class) // accept any Set and convert it via `.toImmutable()`

    private final ImmutableSet<String> set;

    @Incoming(value=CharSequence.class, name="toString")

    @Nonnull

    private final String name;

    @Incoming(name="clone")

    @Outgoing(name="clone")

    private final BitSet bitSet;

}

Together with

class MyCopieres {

    public static ImmutableList<T> toImmutable(Collection<T> original) {

        return ImmutableList.copyOf(original);

    }

    public static ImmutableSet<T> toImmutable(Set<T> original) {

        return ImmutableSet.copyOf(original);

    }

}

it would generate

class C {

    public C(Collection<String> list, Set<String> set, CharSequence name, BitSet bitSet) {

        this.list = ImmutableList.copyOf(list);

        // it's actually

        // this.list = list.toImmutable()

        // transformed by @ExtensionMethods

        this.set = ImmutableSet.copyOf(set);

        this.name = name.toString();

        this.bitSet = bitSet.clone();

    }

    public C withList(Collection<String> list) {

        return new C(list, set, name, bitSet);

        // here nothing special needs to be done

        // as everything gets handled by the constructor

    }

    ... 3 more trivial withers

    ... 3 trivial getters

    public BitSet getBitSet() {

        return bitSet.clone();

    }

}

which is quite often exactly how such an immutable class should look like. Just an idea....