Optionally transform Extend fields to add items one by one

[Tamschi]

This is only a proof of concept right now, and has many rough edges. I decided to make a draft PR early to discuss this more easily.

Thanks for your consideration.

---

Motivation

I'm working on a web frontend framework where I use typed-builder to enable type-safe named and optional parameters, like this:

<*Custom .a = {1} .b = {"B"}>
<*Custom .b = {"B"} .a = {1}>

Something I'd like to add to this (without special syntax) is a way to declare arguments as accepting zero-or-more or one-or-more parameters:

<*Custom .a = {1} .a = {2} .a = {3} .b = {"B"}>
<*Custom .b = {"B"} .b = {"C"} .b = {"D"}>

I could probably also do this on my end, but the feature is most likely a bit cleaner and easier to implement here in this crate.

The changes to examples/example.rs contain usage examples.

(Planned) Changes

For now, I added a setter setting strip_extend that optionally takes an expression to transform the first given argument into the collection type (which is necessary to make this work with, for example, Vec1).
I'll have to think about the naming here, maybe use two different names to make it clear that the parametrised variant needs to process the the argument too, instead of just providing an empty seed value.

The unparametrised version uses a specified default = or Default::default() to seed the field, but a field that's not set at all is still rejected unless default is specified explicitly.

The generated setter method itself is generic over <Argument>, which means for example Vec will accept both its item type and references to it (if the items are Copy). This doesn't seem to cause too many issues with type inference, but the type can also be specified explicitly when calling the method.
This could cause issues with custom initialisers though, since Argument can only be used to call .extend(iter::once(...)) right now. I'm not yet sure how to fix this.

Other issues with this draft that still need to be resolved:

• Change strip_collection to extend

• Change extend = … to extend(initializer = …) extend(from_first = |…| …)

• Extending by an iterator

  This probably shouldn't be missing, but would need another method name. ("extend_{field_name}"?)
  The plain field name now is used to accept iterators. Methods accepting items are suffixed _item by default.)

• Compatibility with into/auto_into

  This might need a bit of work so that Argument can still be specified explicitly on the builder method call.

• Compatibility with strip_option

  I think this can be done in a hacky way without many changes, but it would definitely be cleaner if the field value was only wrapped in Some once .build() is called. (I'm not sure whether this would affect performance, but more importantly it's a breaking change regarding what TypedBuilderFields is set to. The builder field is only unwrapped if both strip_option and extend are used.)

• Custom errors where from_iter and/or from_item don't exist?

  Similarly to the missing fields message, these should communicate the name of the other method (if it exists) and that this one can be called later. If both are missing, it should suggest adding from_first and/or from_iter to #[builder(setter(extend(...)))] on this field.

  The missing field error message should be similarly changed if no initialisers are available at all.

• Tests

• Documentation

• Example improvements (see below)

• Changelog entry

• Undo auto-formatting of unrelated code

• Make sure the minimum Rust version didn't change

[Tamschi]

The public API feels right to me now, as far as implemented. The first setter call for a given collection field isn't generic anymore, which simplifies and solves typing problems around custom collection initialisers and avoids any potential issues with type inference.

Writing a custom initialiser isn't entirely intuitive in that it should also process the first item (which is necessary for strictly non-empty collections, as mentioned above). For this reason, I added the following error on there:

I don't think the error location can be improved nicely with current proc macro tooling, but this could get sorted out with either upcoming changes to Spans or a Syn update building on them. The lint level definition is located on the = token.

[idanarye]

Can you please not run the code through a formatter? If the code needs to be formatted it deserves a PR of its own, where the settings of the formatter can be discussed and more importantly - where there is no need to go through all the code changes because it's auto-generated anyways and contains no real changes. When you do it as part of a regular PR, most of the changes are formatting changes which makes it very hard to review the real changes.

Regarding the PR itself - I've only skimmed it (I can't properly read it with all the auto-generated formatting noise), but a few remarks:

• I'm not sure strip_collection is the right term. I think extend better conveys what t his feature does (each call to the setter extends the collection)
• I'm not sure how I feel about the custom initizlizer:
  • At the very least, it should be #[builder(setter(extend(initializer = ...)))] and not #[builder(setter(extend = ...))] because it's not intuitive that the primary argument of extend is the custom initializer.
  • Is it even required? Does it have real use-cases? I understand it can be used to start a collection with already existing entries, but every other field type requires the user to either use the default or set the entire content of the field. We don't have a setting for string fields to prefix the set strings with something...
• I don't think we need the HashMap in the example.rs. It makes it too cumbersome.
• Missing:
  • Documentation (goes in lib.rs)
  • Changelog entry.
  • Tests.

[Tamschi]

Thanks for the feedback (and thank you for considering this feature submission at all)!

Can you please not run the code through a formatter? If the code needs to be formatted it deserves a PR of its own, where the settings of the formatter can be discussed and more importantly - where there is no need to go through all the code changes because it's auto-generated anyways and contains no real changes. When you do it as part of a regular PR, most of the changes are formatting changes which makes it very hard to review the real changes.

Sure, and sorry about that. Not using the auto-formatter at all makes it pretty difficult for me personally to work on code, but I'll add a commit that reverts those changes once I take care of the other problems with my changes.

Feel free to unsubscribe from the pull-request until then if you'd like to avoid progress notifications - I'll ping you via @ mention after I clean it up.

Regarding the PR itself - I've only skimmed it (I can't properly read it with all the auto-generated formatting noise), but a few remarks:

• I'm not sure strip_collection is the right term. I think extend better conveys what t his feature does (each call to the setter extends the collection)

It's a bit more complicated, since (as far as I can tell) collections aren't thoroughly defined by a single trait.
Right now I'm using Extend for repeat calls, but IntoIterator::Item to strictly constrain the argument on the first call and FromIterator for the default collection initialiser if no other default is present. This mirrors how I'd hand-write such a builder.

It's definitely possible to reduce this to just Extend, but I'm concerned that would hurt ergonomics and could have a slight performance impact.

I'm open to the rename regardless of the above, though.
I originally chose strip_extend/strip_collection for consistency with strip_option, since the argument type is "unwrapped" in a similar way.

• I'm not sure how I feel about the custom initizlizer:

  • At the very least, it should be #[builder(setter(extend(initializer = ...)))] and not #[builder(setter(extend = ...))] because it's not intuitive that the primary argument of extend is the custom initializer.

That sounds a lot better than what I came up with, thanks! I'll change it 👍

• Is it even required? Does it have real use-cases? I understand it can be used to start a collection with already existing entries, but every other field type requires the user to either use the default or set the entire content of the field. We don't have a setting for string fields to prefix the set strings with something...

It's an edge case, I think. For the default collections in std it's not necessary at all.

Collections that by type require at least one entry are more ergonomic in some use-cases, in that you don't have to .unwrap() any return values from e.g. .first() and .last(). This also means they mostly can't implement Default and usually won't implement FromIterator, though.
There doesn't seem to be a standard way to move a single item into a new collection, which makes configuration necessary.

I'd like to have this feature in my framework because it's, in my experience, quite likely to get GUI designs where the empty case is under-specified. If that's actually not expected to be possible, I'd rather mark it in the type system than add it as comment and try to invent a style that looks sort of okay when the component is misused.

That said, in my particular framework I'll likely be able to do this transparently even without specific support here.
I just think it would be cleaner to add support for it inside the parameter and builder types, instead.

The (existing) String case is different to me in that the value is set all at once. A custom extend(initializer = …) shouldn't be used to add a prefix to the collection, but I don't see a way to actually prevent this.

It could also be used to start a collection without existing elements if the default has some (since I'd like to have the extend setters strictly default to extending the value). That to me seems a bit more likely as a way to have a placeholder, but I personally would much rather use a construct like

#[builder(default, setter(strip_option, extend(initializer = vec1![entry_first])))]
entry: Option<Vec1<Entry>>,

in that case (which would most likely be written as entry+?: Entry, in my macro, once I implement it).

• I don't think we need the HashMap in the example.rs. It makes it too cumbersome.

I'll remove it. I think it would be a good idea to demo the extend feature in a separate example, so I'll revert the changes to this file if you don't have any objections.

• Missing:

  • Documentation (goes in lib.rs)
  • Changelog entry.
  • Tests.

I'll add it to my checklist in PR summary 👍

[idanarye]

Thanks for the feedback (and thank you for considering this feature submission at all)!

Can you please not run the code through a formatter? If the code needs to be formatted it deserves a PR of its own, where the settings of the formatter can be discussed and more importantly - where there is no need to go through all the code changes because it's auto-generated anyways and contains no real changes. When you do it as part of a regular PR, most of the changes are formatting changes which makes it very hard to review the real changes.

Sure, and sorry about that. Not using the auto-formatter at all makes it pretty difficult for me personally to work on code, but I'll add a commit that reverts those changes once I take care of the other problems with my changes.

I don't mind setting up a formatter configuration and formatting all the code - as long as it's a separate PR. Or I can do it myself, if you don't mind handling the merge conflicts in this PR. Personally I prefer to set up a linter and do what it says because I like my line breaks at logical places and formatters tend to blindly re-format macros and function arguments according to width, but that can probably turned off in the configuration.

• I'm not sure strip_collection is the right term. I think extend better conveys what t his feature does (each call to the setter extends the collection)

It's a bit more complicated, since (as far as I can tell) collections aren't thoroughly defined by a single trait.
Right now I'm using Extend for repeat calls, but IntoIterator::Item to strictly constrain the argument on the first call and FromIterator for the default collection initialiser if no other default is present. This mirrors how I'd hand-write such a builder.

It's definitely possible to reduce this to just Extend, but I'm concerned that would hurt ergonomics and could have a slight performance impact.

Maybe we are looking at it wrong. What if instead of building the end collection, the builder will always build a Vec and convert it on build() to whatever

• Is it even required? Does it have real use-cases? I understand it can be used to start a collection with already existing entries, but every other field type requires the user to either use the default or set the entire content of the field. We don't have a setting for string fields to prefix the set strings with something...

It's an edge case, I think. For the default collections in std it's not necessary at all.

Collections that by type require at least one entry are more ergonomic in some use-cases, in that you don't have to .unwrap() any return values from e.g. .first() and .last(). This also means they mostly can't implement Default and usually won't implement FromIterator, though.
There doesn't seem to be a standard way to move a single item into a new collection, which makes configuration necessary.

I think I kind of missed that v1_first is the item passed to the first call to v1(). I thought it was some global const or something, and that your v1 will always have two members - v1_first and the argument of the first call. Which seems weird, unless you want to demonstrate an example and then it's just weird to have that feature in the first place.

That's what I get for skimming a PR, I guess, because if the initializer creates it the collection from the first item then it does make sense.

May I suggest to use a closure instead? So instead of

#[builder(setter(extend(initializer = vec![v1_first])))]
v1: Vec<i32>, 

You'd have

#[builder(setter(extend(initializer = |first| vec![first])))]
v1: Vec<i32>, 

This makes it more clear that an argument is involved.

#[builder(default, setter(strip_option, extend(initializer = vec1![entry_first])))]
entry: Option<Vec1<Entry>>,

[Tamschi]

(I'm going to collapse a few of the nested quotes for brevity's sake.)

[…]

I don't mind setting up a formatter configuration and formatting all the code - as long as it's a separate PR. Or I can do it myself, if you don't mind handling the merge conflicts in this PR. Personally I prefer to set up a linter and do what it says because I like my line breaks at logical places and formatters tend to blindly re-format macros and function arguments according to width, but that can probably turned off in the configuration.

I'd be glad if you did, thank you. I was actually considering submitting a rustfmt.toml that changes the existing code as little as possible, but it's better if you choose the settings.

Unfortunately, I don't think it's possible to make rustfmt preserve argument formatting. (You can set a maximum width, but that's it.)
I did the cargo fmt in a separate commit at the very start to make it easy to revert, so even if there won't be a formatter configuration, it won't cause extra work for me here.

• I'm not sure strip_collection is the right term. I think extend better conveys what t his feature does (each call to the setter extends the collection)

It's a bit more complicated, since (as far as I can tell) collections aren't thoroughly defined by a single trait.
Right now I'm using Extend for repeat calls, but IntoIterator::Item to strictly constrain the argument on the first call and FromIterator for the default collection initialiser if no other default is present. This mirrors how I'd hand-write such a builder.
It's definitely possible to reduce this to just Extend, but I'm concerned that would hurt ergonomics and could have a slight performance impact.

Maybe we are looking at it wrong. What if instead of building the end collection, the builder will always build a Vec and convert it on build() to whatever

It's definitely possible, yes. This would then use only FromIterator and IntoIterator::Item by default, or maybe no specific trait at all with a custom initializer.

I see three drawbacks with this:

• Slightly lower performance, since the values would (likely) be copied through the heap first in all cases.

  I consider this only a minor issue since the library doesn't advertise runtime speed. It's hard to tell whether this would be noticeable with my framework, too, since this probably wouldn't be super frequent even if it does appear in the render path.

• A limitation on one item type, since Extends could be overloaded quite a bit. Syn's Punctuated is an example of this, but maybe that (generally speaking) is out of scope?

• Delayed panics until .build() is called. Some collection types may accept only a certain range of values or panic on duplicate keys. Only panicking on .build() in that case seems like it would be harder to debug.

  I consider this the most serious problem, but it's still something that wouldn't come up very often, I think.

• Is it even required? Does it have real use-cases? I understand it can be used to start a collection with already existing entries, but every other field type requires the user to either use the default or set the entire content of the field. We don't have a setting for string fields to prefix the set strings with something...

It's an edge case, I think. For the default collections in std it's not necessary at all.
Collections that by type require at least one entry are more ergonomic in some use-cases, in that you don't have to .unwrap() any return values from e.g. .first() and .last(). This also means they mostly can't implement Default and usually won't implement FromIterator, though.
There doesn't seem to be a standard way to move a single item into a new collection, which makes configuration necessary.

I think I kind of missed that v1_first is the item passed to the first call to v1(). I thought it was some global const or something, and that your v1 will always have two members - v1_first and the argument of the first call. Which seems weird, unless you want to demonstrate an example and then it's just weird to have that feature in the first place.

That's what I get for skimming a PR, I guess, because if the initializer creates it the collection from the first item then it does make sense.

May I suggest to use a closure instead? So instead of

#[builder(setter(extend(initializer = vec![v1_first])))]
v1: Vec<i32>, 

You'd have

#[builder(setter(extend(initializer = |first| vec![first])))]
v1: Vec<i32>, 

This makes it more clear that an argument is involved.

#[builder(default, setter(strip_option, extend(initializer = vec1![entry_first])))]
entry: Option<Vec1<Entry>>,

Noted!

In hindsight that was really confusing syntax, yeah.

I'll make sure to move away from this in my other code too.

Event handlers in my web framework currently get a self implicitly, but I may move away from that when I rewrite the parser to ignore syntax errors there. The event argument itself will definitely be explicit once I make it available in the rework, and I'll have to look into hiding the DI node and bump allocator handle from user code if they aren't already, or at least namespace them somehow…

Thank you! This is very helpful.
I'm trying to take a programming break for a few days, but you should see updates again around the weekend.

[idanarye]

I've added a rustfmt.toml and formatted the code. Could you please rebase to remove all the formatting changes noise?

[Tamschi]

Thank you and done 👍
I'll most likely take a break for a few more days, but for now I've added a few comments where I think the changes could use some explanation.

There might be a bit much copy-paste right now. I tend to clean that up fairly late into my features, but I'll have to see how it turns out in this case.

[Tamschi]

I wasn't sure about the placeholder syntax here, but I think I got "assignment or call" right.
Since ident and !ident are accepted too, this might need an update, though. I didn't change this for now since those cases both were already present.

[Tamschi]

Maybe using Lookahead1 would make sense here, since that can generate a similar error message automatically. The syntax isn't as clean as the match statement here, though.

[Tamschi]

I'll split this and put it into a separate file. You're right that it's just too much at once.

[Tamschi]

I haven't changed the argument name here yet, but I've been thinking that item for the single case and items when extending with an iterator might be more clear. That would also avoid the field_hygienic workaround.

The generic type parameter Argument is a bad choice on my part - I'll change it to Item once I get back to this.

[Tamschi]

I think from_first is more clear than initializer here, and will nicely pair with from_iter once added.

I'm concerned about what to default to if just one of the parameters is present, or with empty parentheses.
My current hunch is that initialisers missing from explicit parentheses shouldn't be generated at all, but also to accept from_first and from_iter without assignment to generate the default each. This would mirror how default works as builder(…) parameter.

I could also add support for !from_first and !from_iter to delete them again once set, but that feels a bit odd to me.
Is there a specific reason this is supported on the fields? The application I can think of is for overrides when combined with a macro, but I'm just guessing here 🤔

[idanarye]

Is from_first without arguments even meaningful? What would be the default?

As for from_iter - I think maybe it should be the default - as in, without an argument you won't even need to specify it. Instead of keeping the actual target collection we can keep I: Iterator, and each time we extend with the builder, we can make an std::iter::Chain. We can still specify from_iter = |it| ... if FromIterator is not available for that collection or if for whatever reason we need custom behavior.

[Tamschi]

I tend to use quote_spanned quite often even when it's not strictly necessary, since it tends to make for better error locations.

My earlier hack with #[forbid(unused_variables)] is gone, of course, but this should still flag some type/trait errors on the field itself or the matching attribute or attribute parameter.

[Emilgardis]

Just a fyi, rustfmt will skip items marked #[rustfmt::skip]

[Tamschi]

I haven't been able to work on this much lately, but here's a little progress.

Accepting IntoIterators with the same name as the field and single items with a separate name should be much more intuitive.
I went with -_item as default suffix, but this is configurable through the item_name parameter.

Just extend will implement everything with default settings, while extend() (with empty parentheses) will generate only the methods that extend an existing collection, which makes it impossible to set the field. Adding from_first or from_iter as custom keywords into the parentheses will add the default implementation (which extends a specified default if present), assigning lambdas to them will use a custom implementation as before.

I'll have to think a bit about how to integrate this with strip_option. The cleanest version might be to strip the option also from the builder field type (only in combination with extend? Changing it in general could break things downstream.), wrapping it in Some(…) only in .build().

[Tamschi]

Writing this out in the tests made me realise this is probably unexpected behaviour (and would also combine awkwardly with strip_option). I'll change it so that the auto-implementations always start with only the given items.

[Tamschi]

I'll likely revisit this, making a closure argument type override the outer method argument type verbatim (including any impl).
That isn't quite standard Rust (though Syn should still parse it), but is (likely) needed to constrain the argument type for e.g. non-empty target collections.

[Tamschi]

I ended up using this method in a few more places, so I unified the error handling here.
This also means I changed a few other functions slightly to propagate these errors, but that should be fairly minimal.

[Tamschi]

There probably should be a special message in case neither of these are available as initialisers.
I'll stop for today, but I've added this point to my to-do list.

(As an aside, I've just been able to confirm this feature will work for my use-case 🥳
I've queued that feature up just so it's easier to keep track of, but that won't change how I work on this one.)

[Tamschi]

A small status update since it's been a month: A few things with higher priority have been happening, so I haven't found time to continue here. I currently don't have an ETA on when I'll be able to resume work on this draft.

[Tamschi]

I should probably put a(nother) small status update here so that it doesn't look like I ran off as soon as this started working for my specific use case…

The reality of the situation is that I've been swamped with higher-priority work pretty much constantly since April, so I haven't found time to continue work on this feature here or the grammar feature in my framework that I'll need this for.
I'll likely be able to maintain this feature branch to make sure it doesn't fall behind over time, but I don't have an estimate on when I'll get around to completing it. Sorry about that.

[idanarye]

No need to apologize. This is open source - we all work for free here, and no one should feel pressured to prioritize contribution over other things they have in their life.

Also - it's not like I've been working on this crate that much this year...