few cents on @Singular and @Default

[Matei]

I looked at the handling of Lists in Lombok and Chronicle Wire here.

I'm initializing an object with Wire, running a toBuilder session on it (or builder if there was no wire step), and checking the field types and contents.

I use reflection to parametrize the tests.

I couldn't get SuperBuilder to work, I don't know why.

I wanted to give my few cents on the subject of design.

- @Builder is an amazing thing to have, for both objects and function calls.

- @Singular is very nice to have.

- I see no reason why @Default & @Singular should not coexist, even though they can't right now.

- Moreover, it feels like @Default & @Singular should be implied for list/set/map fields; with @Default being universally implied. I'm sure there are strange initializers out there for which a @NoDefault would be warranted, but still, the moment one takes the time to write an explicit initializer, it's strange to require an explicit @Default to go with it.

- There is the question, how to combine (1) an existing initializer with a non-empty build() session, or (2) an existing value with a non-empty toBuild() session. Implementation effort aside, IMO what sounds right in terms of behavior would be for the build session to take over the existing container in (1), and attempt to clone it in (2). This would incidentally solve all mutability issues, as the initializer would end up being used as the destination type.

- A slight problem remains with an immutable existing value/initializer which cannot be used (throws?) in a build session. Since the list was immutable before, it makes sense to also return another immutable list.

- For reference, note how Wire does not change the type of an existing container initializer (tagged (2) in tests). It also does not add to it like a @Singular build session would, but probably clears it and repopulates it (guessing).

No real question, just food for thought.

Thanks.

[Reinier Zwitserloot]

What would it mean if you have an @Singular list (let's say a List<Color>), marked with a @Default of List.of(Color.RED)?

1. I call .color(Color.BLUE) on the builder, then build it. Is it a list of BLUE, or a list of RED,BLUE?

2. I call nothing color-ish on the builder. I get a List.of(Color.RED), no doubt, right?

3. I call only .emptyColors() on the builder. What do I get now? Color.RED or an empty list?

I can add some more questions. It's not about coming up with answers to these questions. It's about the fact that there is no single clearly obvious answer, and therefore, we don't allow it.

I'm not really understanding what you're driving at, I think. Perhaps show some examples of 'how I would want to write it with lombok' and 'what lombok would desugar that to'.

[Matei]

I didn't intend this post about what I'm trying to achieve. FTR, I wanted to use builder()&toBuilder() on objects and obtain mutable lists, as you might expect with most people questioning the immutability choice. For the time being I will not use @Singular at all, or perhaps I'll add a hand-made mutable() call which undoes the immutable choice made by Lombok (as in, decluttering the declutterer): sift through fields, find lists, set accesibility, replace them with mutable ones, ugly. I also noticed that, if I intend to use objects created/cloned from builder./toBuilder, I end up sprinkling @Default all over the place, because really, 95% of initializers out there are "true, -1, 1, new CollectionImpl<>()" for which you really want a @Default.

Re @Singular immutability, a quick search relveals many people complaining about this, and, in my opinion, rather defensive arguments on the maintainers' side against recognizing this as an issue: "if you need mutable lists, you shouldn't use @Singular or @Builder". In my opinion it's the wrong approach to maintaining such a useful library. This shouldn't be so much about the context in which a feature was added, and how to work around current limitations, as about how it can be improved.

With @Builder on a function:

(a) there is no good place a user might have expected to put in a default value for a list

(b) generally, it's ok for the list to be immutable, it's rare (but not unheard of) to modify list arguments

So in the context of functions, it might be acceptable to produce immutable lists.

With @Builder on objects, neither of (a) and (b) is true any more:

(a) an initializer is a perfect place to place a default

(b) usually, I would say, when you create a new object with a list field, it's much closer to 50-50 whether you want a mutable list or not.

So I think it's quite a valid thing for users to expect that a "new ArrayList<>()" initializer gets honored, all while accessing the niceties of @Singular.

There are 2 valid issues, in my opinion, from here to there:

- As you mention, the behavior has to be clear. I really don't see this being a problem, see below.

- Continuity, as in, how disruptive a change is it to exsting code. I tend to underestimate this problem, but it feels this should be a showstopper.

In terms of behavior, consider the following quasi-complete specification, so that it's crystal clear what we're talking about:

(1) if a field is detected to be exactly one of List/Set/Map, the following applies, else it doesn't. (I know "detected" needs resolution which is a touchy subject, but you must already have something in place to implement the current @Singular)

(2) in each builder()/toBuilder() call, a temporary store is: created, initialized, populated, and converted to the destination store.

(3) creation:

(3a) the store is created & initialized lazily, on population or conversion

(3b) the store is created as ArrayList/LinkedHashSet/LinkedHashMap

(4) initialization:

(4a) in builder(), the store is initialized with the contents of the field initializer, if any

(4b) in toBuilder(), the store is initialized with the contents of the existing field

(4c) the initialization consists of a single call to store.addAll/putAll(source)

(5) creation and initialization are bypassed by an explicit call to

with<Plural>(Collection) - use given collection as builder store; throw if used after a population statement

(6) population:

(6a) the first population statement triggers creation and initialization, unless (5) was used

(6b) during a builder session, the store is populated using:

clear<Plural>() - clears the store, keeps type (effectively calls clear())

add<Singular>(Element) - add one element

add<Plural>(Collection) - add all elements from the given collection

(7) conversion:

(7a) the final conversion between the builder store and the field value is controlled by a Function<Container,Container> provider.

(7c) the provider can be set explicitly during the build session with:

with<Plural><Container>Provider(Function<Container,Container>) (e.g. withMethodsListProvider(LinkedList::new) for a field List<String> methods)

(7d) if the provider is set explicitly, it is used irrespective of anything else

if the provider is not set explicitly...

(7e) if (5) was used, an identity provider is used. I.e., the store passed with with<Plural>() is assigned to the destination field value

if (5) was not used...

(7f) if the field has an initializer that is not marked with @NoDefault: call it for the sake of producing a list, then call clear() and addAll(). if this throws, fall through to branch below.

(7g) use provider Collections::unmodifiableList/Set/Map

To all explain this orthogonally, from a usage point of view:

- builder()/toBuilder() uses a temporary store of type ArrayList/LinkedHashSet/LinkedHashMap to build up the final collection contents

- the collection contents are initialized to the initializer in builder(), and to the existing value in toBuilder(); or empty for a builder() with explicit @NoDefault initializer

- the user may take over both the type and the contents of the temporary store by using with<Plural>(). this must be done prior to any clear/addOne/addMany calls; also, this disables any implicit final cast into the destination field

- the user may explicitly request a destination type using with<Plural><Container>Provider(); this is honored in all cases.

- when neither of the 2 with() calls above are used, and an initializer for that field exists, the builder makes an "honest attempt" to honor the type of that initializer, as follows: builder executes the initializer expression, then clear() and addAll() with the contents of the store. if an exception is caught in clear() or addAll(), the code falls through below

- lastly, if none of the options above applies, the builder casts the temporary store into an unmodifiable collection.

Under the hood quirks:

- When an accessible (non-@NoDefault) initializer exists, the scheme above mentions calling it twice, once at the start, to initialize the temporary store, and once at the end, to obtain the object to clear. In this case the second call can be foregone, and the value produced in the first call can be used.

- The "honest attempt" branch (7f) can forego exceptions under the hood by detecting known immutable types.

- An accessible field initializer might end up being executed even in a toBuilder() session, for the explicit purpose of obtaining the destination container type. This should be documented in @NoDefault. I don't see this being overly counterintuitive on the basis that a toBuilder() also creates an object, so in a sense it's not unexpected initializers are involved. I actually see this being more intutive than the current choice of slamming a hard-coded immutable provider on top.

Above, I'm suggesting the slightly explicit names addMethod()/addMethods()/withMethods() to prevent the name clash with the existing method()/methods(). The plural notably does different things today for fields with and without @Singular. To be more adventurous would be to use method()/methods()/withMethods().

For the examples in your email:

(1) you get RED (initial), BLUE (addOne)

(2) you get RED (initial)

(3) you get empty; the list had started with RED but it was cleared.

I'm not sure why any part of this would be confusing or unexpected, please do throw more questions at it.

The benefit I see is that it would:

- eliminate @Singular (meaning, all containers would be @Singular, with its benefits)

- allow users to choose destination types in what I would consider to be an intuitive way, or certainly more intuitive than the current implementation

[Reinier Zwitserloot]

I guess I didn't properly convey the problem here.

It's NOT about answers to my questions!

I need an answer to the question (such as 'well, then you get a list containing RED, then BLUE)... _AND_ I need a reasonable argument that this is the answer 90%+ of the community would confidently provide given nothing other than the annotation's full name ('Builder', 'Singular' and 'Default' in this case), and a short paragraph describing each feature.

I'd be extremely surprised if your answers here get anywhere near the '90%+ amongst the Joe Q. Javacoder crowd would confidently give the same answer'.

We CAN add those features anyway, knowing its a learning curve burden, but that is a very high cost. We have to make 'do you get enough value for the cost' analyses for all features, so, with this marked increase, the demands on the value add are that much larger, and I'm not seeing it here.

Your questioning of how we run the project is understood, but I regret that I do not share your feelings on this matter. I guess we have to agree to disagree on that one; I feel that the community is likely to want things without quite realizing that the thing they want has issues, side effects, lacks sufficient specification or consensus, etc... and then it is up to the maintainers to put their foot down and wait until (if ever) there is a true workable right answer. It doesn't matter how much the community whinges about the lack of such a feature: No obviously workable feature proposal -> it doesn't get implemented. period.

Separate from that, builder is _A LOT_ of work and the changes you propose completely change how almost all of the builder code works. Even if we skip on all the many issues where a workable feature proposal (which involves at least that 90%+ random users would confidently guess correctly how the feature works), what you're proposing is that we split builder's inner workings in two, with 2 completely different implementations AND details of how it works depending on whether its immutable or mutable. We like to believe we have our ear on the ground, so to speak: I think you are vastly overestimating the # of people who want this feature. We do need a lot to warrant all this complexity, both from within (maintenance burden on us) and without (learning curve burden on our users, makes the lombok features more confusing than they are now).

[Martin Grajcar]

I agree with you in general, but not concerning this question. The colors might confuse more than 10% people, but only until they realize that it's not only the default value for the created object, but also the initial value for the builder. Then it's perfectly clear, at least to me. It's straightforward and it doesn't take any possibility away, as you always can call emptyColors().

There are some details like that with (I'm assuming useGuava=true in the following)

@Builder.Default ImmutableList<Color> colors = ImmutableList.of(Color.RED)

it can't be used in the Builder because of immutability, but then the Builder uses a mutable copy of it. IMHO obvious, as what else should it do?

Another detail is whether the original ImmutableList.of(Color.RED) gets used or it gets converted too a mutable list and back. But this is just an optimization and hardly anybody cares.

IMHO Default and Singular could work nicely together and I guess, neither the implementation overhead not the learning curve would be too bad. The open question is whether it gets used often enough to warrant the effort. Concerning the learning curve, anyone combining two advanced features should be prepared to think about the outcome. Even when it wasn't obvious (IMHO it is), it's surely not counter-intuitive:

• Anyone using a default can rightfully expect to get it when they're not fiddling with it.
• Otherwise, they should expect their default to interact with their fiddle, which isn't bad
• as they can call emptyColors().

Obviously, toBuilder() should work the same.

I don't get what the OP's other points are about....

--
You received this message because you are subscribed to the Google Groups "Project Lombok" group.
To unsubscribe from this group and stop receiving emails from it, send an email to project-lombo...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/project-lombok/0d1b4e12-2d8b-4672-8ba3-a00c7362aa5e%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.