Closed ModProg closed 10 months ago
Maybe something like this?
#[derive(TypedBuilder)]
struct Component {
normal_field: String,
#[builder(setter(skip), default, mutators(
fn child(child: impl ToHtml) {
let child = child.to_html();
move |children| children.append(child)
}
))]
children: Html
}
The return value of the mutator function is omitted on purpose, but it's basically always going to be impl FnOnce(&mut <field-type>)
. It'd generate a child
method on the builder, with the same arguments as the child
function inside the mutators
attribute field. With many Rust-ceremony omitted, it'd look something like:
impl ComponentBuilder {
fn child(mut self, child: impl ToHtml) -> Self {
// This is the exact same function from `mutators`
fn child(child: impl ToHtml) {
let child = child.to_html();
move |children| children.append(child)
}
(child(child))(&mut self.fields.1);
self
}
}
yes, that is exactly what I need. I assume the default would be eagerly evaluated and filled in (::builder()
) for such a field?
(In my case it would be
#[derive(TypedBuilder)]
struct Component {
normal_field: String,
#[builder(setter(skip), default, mutators(
fn child(child: impl ToHtml) {
move |children| child.write_to_html(children)
}
))]
children: Html
}
to avoid unnecessary allocations)
The reason to not do fn child(children: &mut Html, child: impl Html)
is that you want to avoid ambiguity about which argument is the field?
Otherwise, one could also say, that the first argument is always the field, and allow:
fn child(children: &mut Html, child: impl ToHtml) {
child.write_to_html(children);
}
Maybe something like this?
#[derive(TypedBuilder)] struct Component { normal_field: String, #[builder(setter(skip), default, mutators( fn child(child: impl ToHtml) { let child = child.to_html(); move |children| children.append(child) } ))] children: Html }
if this is the api you want to go for, I'd be willing to implement this.
Another option:
#[derive(TypedBuilder)]
struct Component {
normal_field: String,
#[builder(setter(skip), default, mutators(
fn child(self, child: impl ToHtml) {
self.children.append(child.to_html());
}
))]
children: Html
}
Which will generate:
impl ComponentBuilder {
fn child(mut self, child: impl ToHtml) -> Self {
struct TypedBuilderFieldMutator<'a> {
children: &'a mut Html,
}
impl TypedBuilderFieldMutator<'_> {
fn child(&mut self, child: impl ToHtml) {
self.children.append(child.to_html());
}
}
TypedBuilderFieldMutator {
children: &mut self.fields.1
}.child(child);
self
}
}
This is simpler than my original proposition, because it does not have that weird closure returning. And it has the advantage over your proposition that the user won't have to repeat the type. Also, using self
to access the field should feel natural.
I do wonder though if having self
in the function is okay, or if it should be &mut self
- which is more correct.
This is simpler than my original proposition, because it does not have that weird closure returning. And it has the advantage over your proposition that the user won't have to repeat the type. Also, using
self
to access the field should feel natural.I do wonder though if having
self
in the function is okay, or if it should be&mut self
- which is more correct.
Having access to self
would make me assume, I could also modify other fields, with the current approach this wouldn't be allowed.
(Though one could, as a future extension, create global mutators, that get mutable access to all fields, marked as mutable
.)
I'm not really worried about that, because they'd just get a compiler error. Maybe even a rust-analyzer linter error, if we do it right.
I do wonder, though, if mutators
should be mutually exclusive to setter
and default
. Consider this (lets ditch the Html type for something simpler):
#[derive(TypedBuilder)]
struct Foo {
#[builder(
default = vec![1, 2],
mutators(
fn bar(self, value: u32) {
self.bars.push(value);
}
),
)]
bars: Vec<u32>,
}
What should this resolve to?
Foo::builder().bar(3).bar(4).build().bars
Should it be [3, 4]
because we added some values so the default should be ignored? Or should it be [1, 2, 3, 4]
, because it starts form the default
and each call to bar
mutated that?
Also, how about this?
Foo::builder().bar(3).bars(vec![4, 5]).bar(6).build().bars
Should it be [3, 4, 5, 6]
, because that's all the numbers we set? Or should it be [4, 5, 6]
, because the call to bars
had overriden whatever was there?
For both questions, the first answer is what would make sense to a native reader to the API, and the second is what would make sense from the implementation side. I don't like that they differ, so I'd rather just prevent the possibility that they'd be used together.
Actually, for the second question, it'd make more sense to just disallow it - because after the first bar
, the field already has a value so bars
cannot be set. bars
should only be called as the first bars
-setting method. And of course, if there is no default
, bars
must be called before any bar
- otherwise we don't have a way to construct the initial value (unless we dictate that fields with mutators must always implement Default
)
Of course, we want the ability to declare an initial value (in part, so that we can have non-Default
fields with mutators). Not sure how to tie it to the mutators though. Maybe this?
#[derive(TypedBuilder)]
struct Foo {
#[builder(
mutators(
init = vec![1, 2];
fn bar(self, value: u32) {
self.bars.push(value);
}
),
)]
bars: Vec<u32>,
}
Though this can get confusing. So maybe just this?
#[derive(TypedBuilder)]
struct Foo {
#[builder(
init = vec![1, 2],
mutators(
fn bar(self, value: u32) {
self.bars.push(value);
}
),
)]
bars: Vec<u32>,
}
And make it an error to set init
without mutators
?
(Though one could, as a future extension, create global mutators, that get mutable access to all fields, marked as mutable.)
What if we do it like that from the get-go?
#[derive(TypedBuilder)]
struct Foo {
#![builder(mutators(
fn bar(self, value: u32) {
self.bars.push(value);
}
))]
#[builder(
delegated(init = vec![1, 2]), // maybe need better names?
)]
bars: Vec<u32>,
}
This syntax is starting to get ugly though. Maybe we can just move the mutator fields to be named fields under the builder strunct, and rely on the fact that using mutators does not change the generic type of the builder?
#[derive(TypedBuilder)]
struct Foo {
#[builder(
delegated(init = vec![1, 2]), // maybe need better names?
)]
bars: Vec<u32>,
}
impl<B> FooBuilder<B> {
fn bar(mut self, value: u32) -> Self {
self.bars.push(value);
self
}
}
I'm not sure that would work though. If it turns out we need to constratin B
, it may make the API too complex to use...
I agree it makes sense to disallow a setter after a mutator, and I wouldn't mind making them exclusive.
If someone wants to have a setter still, it would be straight forward to define, independently of the syntax we choose, and they would need to figure out how it should behave themselves.
e.g. for a vec it could make sense to say
struct Struct {
#![builder(mutators(
fn bar(&mut self, value: u32) {
self.bars.push(value);
}
fn bars(&mut self, value: &[u32]) {
self.bars.extend_from_slice(value);
}
))]
// field is mutably exclusive with a predefined setter
#[builder(field)] // this would mean default
#[builder(field = vec![1, 2, 3])] // this would init with additional value
#[builder(field(pub))] // would make field public
#[builder(field(pub, init = vec![1, 2, 3]))] // would make field public
bars: Vec<u32>,
}
Another approach would be to allow mutators, to be type-state dependend, i.e.:
struct Struct {
#![builder(mutators(
fn f1_f2_set(self: (f1, f2), a: u32, b: bool) {
// this function would have mutable access to
self.initialized_field;
self.f1;
self.f2;
// but can only be called when f1 and f2 are set
}
fn only_init_fields_are_set(self, b: bool) {
// This function can always be called, but only has
// access to fields that start out initialized
}
)]
f1: Vec<u32>,
#[builder(mutator(
fn extend_f2(self, value: u32) {
// This function is only availible if `f2` is set prior.
self.f2.push(value);
}
)]
f2: Vec<u32>,
#[builder(init)] // This field starts initialized with default
#[builder(init = value)] // This field starts initialized with `value`
initialized_field: Vec<u32>
}
This would seperate the ability to use a mutator from pre initialization of a field.
One thing to note is, that init
should imply setter(skip)
.
We could (but that is independent of this IMO) create a setter(overwrite)
that allows calling a setter twice or calling a setter on an init
field.
One thing to note is, that
init
should implysetter(skip)
.
Which is why I'm worried about this name - which does not seem related to the mutators thing.
Ideally I'd like a name that could do this:
#[derive(TypedBuilder)]
struct Foo {
#![builder(mutators(...))]
// `bar` is a regular field with a generated setter:
bar: Bar,
// `baz` has no setter and uses mutators. It gets initialized to `Baz::default()`.
#[builder(something)]
baz: Baz,
// `qux` has no setter and uses mutators. It gets initialized to `Qux::new()`.
#[builder(something = Qux::new())]
qux: Qux,
}
But I'm not sure what would be a good name for "something". "via_mutators" won't work because #[builder(via_mutators = Qux::new())]
does not indicate that Qux::new()
is the initial value.
Maybe something like this?
#[builder(via_mutators(init = Qux::new()))]
qux: Qux,
#[builder(via_mutators(init = Qux::new()))] qux: Qux,
I can't think of anything better right now. The only other Idea I had was to make init
require setter(skip)
to ensure that this fact is understood (similarly to how setter(skip)
requires default
).
Actually, this got me thinking about the relationship with setter(skip)
.
setter(skip)
requires that a field has a default
and disables the setter… we could use setter(skip)
to mark fields as init
.
I'm not sure what implications this would have, though. One result would be, that the default
expression of setter(skip)
is called in builder()
instead of build()
. Which would be unnecessary if one doesn't plan on using mutators.
It might also be a bit unintuitive.
I still think it should not be called deafult
in case of mutators, because that would imply that if you use the mutators something other than the default
is used.
My TypedBuilderFieldMutator
has a potential problem - because the fields are &mut
references to the original fields, they have to be dereferenced for some operations.
This will probably not be an issue for most things, since you'd use methods on them anyway, but it may still be confusing.
Creating an impl
block for the builder will overcome this limitation. Of course, that does require some big changes to the builder struct (namely - adding all mutator-able fields as struct fields, which means we can no longer use regular names for the fields the macro uses...). I'm not sure what would be better...
Creating an
impl
block for the builder will overcome this limitation. Of course, that does require some big changes to the builder struct (namely - adding all mutator-able fields as struct fields, which means we can no longer use regular names for the fields the macro uses...). I'm not sure what would be better...
This would also remove the option to integrate the mutators in the typestate pattern
My
TypedBuilderFieldMutator
has a potential problem - because the fields are&mut
references to the original fields, they have to be dereferenced for some operations.This will probably not be an issue for most things, since you'd use methods on them anyway, but it may still be confusing.
Actually, I think this can work:
struct Builder<A, B, C> {
fields: (A, B, C),
}
impl<A, C> Builder<A, usize, C> {
fn add_to_b(self, b: usize) -> Self {
struct TypeBuilderFieldMutator_B {
b: usize,
}
impl TypeBuilderFieldMutator_B {
// original fn from #[builder(mutators...
fn add_to_b(&mut self, b: usize) {
self.b += b
}
}
let Self { fields: (a, b, c) };
let mut mutator = TypeBuilderFieldMutator_B { b };
mutator.add_to_b(b);
let TypeBuilderFieldMutator_B { b } = mutator;
Self { fields: (a, b, c) }
}
}
No mutable references needed
Another name could be set
or pre-set
, because it is as if the setter
was executed from the get go.
Both set
and pre-set
only convey the correct meaning if you know what they do. If you just see them, they can be quite confusing.
Currently, TypedBuilder
exposes the whole of fields
as one generic Parameter.
struct StructBuilder<TypedBuilderFields = ((), ())> {
fields: TypedBuilderFields,
phantom: ::core::marker::PhantomData<()>,
}
I see two options to extend this for via_mutators
fields:
Either, keep the generic, but always set it, to the type of the field:
struct StructBuilder<TypedBuilderFields = ((), String)> {
fields: TypedBuilderFields,
phantom: ::core::marker::PhantomData<()>,
}
Alternatively, we could switch from one generic to one per field, and avoid the via_muators
fields:
struct StructBuilder<__field1: ()> {
fields: (__field1, String),
phantom: ::core::marker::PhantomData<()>,
}
Or move the via_mutator
fields in a separate field
struct StructBuilder<TypedBuilderFields = ((),)> {
fields: TypedBuilderFields,
via_mutators_fields: (String,)
phantom: ::core::marker::PhantomData<()>,
}
I'd lean towards the first option, as it seems like the least disrupting to existing implementation.
Should we have error
functions for mutators
with required fields, that are called without those fields set?
Akin to the build() functions for unset required fields?
I don't think the question of required fields should even be relevant. I see no reason to allow mutators access to fields that don't have via_mutators
, and fields that have via_mutators
will not have the concept of "required" - they will always be initialized.
I'm open to the idea of having required via_mutators
fields, that must first be initialized and are only then open to mutation, but since this complicates both the interface and the implementation, I'll have to be convinced first that this is really needed.
I'm open to the idea of having required
via_mutators
fields, that must first be initialized and are only then open to mutation, but since this complicates both the interface and the implementation, I'll have to be convinced first that this is really needed.
Well, I was already implementing that before your response, but I don't think it actually complicates the code much:
There is some additional parsing for the syntax I choose (Open to change that): https://github.com/idanarye/rust-typed-builder/blob/5d8714a4f7e92d1eb8f84cc5dd04b73eb5e167b5/typed-builder-macro/src/util.rs#L256-L276
In the actual macro expansion, there isn't much of a difference: https://github.com/idanarye/rust-typed-builder/blob/5d8714a4f7e92d1eb8f84cc5dd04b73eb5e167b5/typed-builder-macro/src/struct_info.rs#L433-L439
After seeing the error messages, I think the required_fields_span
doesn't do anything, and I'll remove it, simplifying this a bit more.
My usecase is the following, I have a component with multiple fields, one of them is special, the children.
I would like to use the builder to extend the Html, I created a workaround manually implementing a
child()
function:I would prefer being able to have this built-in, maybe just as a parameter where I could pass an
Fn(&mut Html, impl IntoHtml)
orFn(Option<Html>, impl IntoHtml)
.Something else I'd like would be to disable the default generated
children
function without removing the generic forchildren
, i.e. something that was like#[builder(setter(skip))]
but only for the setter and not for the generic.