Stabilize #[builder(overwritable)]
#149
Comments
|
The name itself doesn't include something in relation to performance currently so this might be misleading… Thought about having this as a feature or something to enable it only on debug or something like that. But it doesn't seem that good either… Not sure what I think about mixing feature and performance thingy… 🤔 |
|
Well, it can't be a cargo feature because it changes the type signature of the builder in a non-additive way (it removes some generic associated types from the builder's state trait). I'm planning to make that type signature and that state trait stable (i.e. to be able to name the actual type of the builder). So if anyone enables it as a cargo feature, their code may cease to compile if downstream code depends on some type states being available. Therefore, this should be configured via attributes. The name of the attribute indeed doesn't make it clear that it improves the compilation performance, but I don't think that's important, because the attribute name describes what it actually does (makes it possible to call the setter multiple times for the same member). The fact, that it simplifies the generics machinery under the hood is just a considerable bonus, which will be part of the docs and maybe have a separate page on "improving compilation perf" in the guide. I expect people will just enable this option in a single place (like with an attribute alias), and put a descriptive comment that says "it improves the compile perf. by N seconds". |
|
So, you'd still like to keep the overwrites disabled? My expectation was that it wouldn't be as important since the overwrites problem doesn't happen that often, and people are used to this when working with manually-written builders. For some, it may even be a feature they would use (not only to improve compile times). |
|
im not entirely sure about it. Setting something twice seems like a mistake so having that is a code smell. Normally code smells are something for the linters… Could it create another kind of warning? Also… is this a requirement for the basic library? Maybe it should allow overrides all the time and have a flag to prevent setting it more than once? Then all the people get better compile time performance for free and only the ones that need the override prevention can enable it? Maybe that's a better default? |
|
Well, the difference between In the general case, for structs of reasonable sizes and counts, having the overwrites protection makes sense by default, so I think this option would only be useful for projects of bigger scale like Unfortunately, there isn't a good way to write a custom lint in Rust today. There are some tools that do that, but they are unpopular and 99% of people don't use anything fancier than
Yes, but another scenario could be where the developer uses this feature to implement a base fixture for tests. For example, the tests need to fill only the fields they are interested in and keep all other fields with dummy values. #[derive(bon::Builder)]
#[cfg_attr(test, builder(on(_, overwritable)))]
struct User {
login: String,
name: String,
level: u32,
}
#[cfg(test)]
mod tests {
use super::*;
use user_builder::{SetLevel, SetLogin, SetName};
// Base fixture that has dummy values for all fields
fn user() -> UserBuilder<SetLevel<SetName<SetLogin>>> {
User::builder()
.level(24)
.name("Bon Bon".to_owned())
.login("@bonbon".to_owned())
}
#[test]
fn test_with_empty_name() {
// Build user with an empty name, and all other fields filled with dummy data
let user = user()
.name("".to_owned())
.build();
// ...
}
#[test]
fn test_with_zero_level_and_admin_login() {
let admin = user()
.level(0)
.login("@admin".to_owned())
.build();
// ...
}
}I think this pattern actually really handy and might also deserve a separate guide page. Note that it should be completely fine to have such a |
|
Hm… I like thinking about this but for some reason it's still something I don't particularly like. Your example with the tests still requires knowing / specifying the types generated by the builder (
I think this is another argument against this as a performance thingy. The default should protect against overwrites so it should also do that for bigger structs. The performance with frankenstein is not the best, but it's not a major change and it loses a benefit. Also, the compilation of the frankenstein crate should be cached locally and not affect another dev build. I'm not sure how much it impacts the linking speed? So personally, I would not use it for performance only. Only when it's an interesting feature for something, and I'm not sure how often that's the case. Guess the question then is… is this feature worth it as a feature or does it add more complexity to bon which is annoying to maintain? |
|
Yeah, I see you points. Anyway, I already have this feature implemented in my branch. I'll probably just keep it disabled by default and make it available only if the user enables some cargo feature flag like |
|
Is there also a plan / time to remove the feature when it doesn't seem to be used and no one stated their use case? Keeping the code clean seems like a good idea 😇 |
|
I don't think this particular feature bears high maintenance burden. I'll just keep it experimental for 6 months, for example. And if I receive no feedback on this issue, I'll remove it |
|
Hello, glad to see you have implemented this feature already! Thank you! For my use case, I am building UI structs that looks like this: #[derive(bon::Builder)]
#[builder(on(String, into))]
pub struct TextInput {
name: String,
value: Option<String>,
error: Option<String>,
}And let's say the // let's say the value is an email (e.g. example@domain.com)
let value_from_db = db_get_current_value().ok();
let value_from_user_input = user_input_value();
let default_value = String::from("default value");
let error_from_user_input = value_from_user_input.as_ref().and_then(|email| email.validate().err());
let system_error = value_from_user_input.as_ref().and_then(|email| verify_email_is_available(email).err());
// Building the UI
let email_input = TextInput::builder()
.name("email")
.value(default_value)
.maybe_value(value_from_db)
.maybe_value(value_from_user_input)
.maybe_error(error_from_user_input)
.maybe_error(system_error)
.build()Note that I didn't test the snippet, it's just for illustration. I already use TypedBuilder for this, other types that doesn't require this and mutators, I use bon for it. Hope this help. |
Could you elaborate how you configured Anyway, I think the code you shown contains a mistake. Every call to a setter overwrites the previous value. For example, if you do smth like Just think of this like the following way (with the let mut value = Some(default_value); // equivalent to `value(default_value)`
value = value_from_db; // equivalent to `maybe_value(value_from_db)`
value = value_from_user_input; // equivalent to `maybe_value(value_from_user_input)`So it's more like assigning to a variable. I think from the code above it's clear that the last assignment fully overwrites previous assignments. Instead, if you want to have a chain of defaults, then you should use #[derive(bon::Builder)]
#[builder(on(String, into))]
pub struct TextInput {
name: String,
value: String,
error: Option<String>,
}
let value_from_db: Option<String> = todo!();
let value_from_user_input: Option<String> = todo!();
let default_value = String::from("default value");
let error_from_user_input: Option<String> = todo!();
let system_error: Option<String> = todo!();
// Building the UI
let email_input = TextInput::builder()
.name("email")
.value(
// This chain will stop at the first non-None value,
// or on the final `unwrap_or`
value_from_user_input
.or(value_from_db)
.unwrap_or(default_value),
)
.maybe_error(error_from_user_input.or(system_error))
.build();If you do it like this, you'll also notice that |
|
Oh! sorry I forget that I have used mutators like this: #[derive(TypedBuilder)]
#[builder(field_defaults(setter(into)), mutators(
pub fn error(&mut self, error: impl ToString) {
self.error = Some(value.to_string())
}
pub fn try_error(&mut self, error: Option<impl ToString>) {
if let Some(error) = error {
self.error = Some(error.to_string())
}
}
pub fn value(&mut self, value: impl ToString) {
self.value = Some(value.to_string())
}
pub fn try_value(&mut self, value: Option<impl ToString>) {
if let Some(value) = value {
self.value = Some(value.to_string())
}
}
))]
pub struct TextInput {
name: String,
value: Option<String>,
error: Option<String>,
}And yes, this is is not the same behaviour as overwrite would be, it's like |
|
Aha, with that If you'd like to adapt that code to The difference is that your custom setters need to be defined in a separate impl block for the builder: #[derive(bon::Builder)]
pub struct TextInput {
#[builder(field)]
value: Option<String>,
#[builder(field)]
error: Option<String>,
name: String,
}
impl<S: text_input_builder::State> TextInputBuilder<S> {
pub fn error(&mut self, error: impl ToString) {
self.error = Some(error.to_string())
}
pub fn try_error(&mut self, error: Option<impl ToString>) {
if let Some(error) = error {
self.error = Some(error.to_string())
}
}
pub fn value(&mut self, value: impl ToString) {
self.value = Some(value.to_string())
}
pub fn try_value(&mut self, value: Option<impl ToString>) {
if let Some(value) = value {
self.value = Some(value.to_string())
}
}
} |
Veetaha
changed the title
#[builder(overwritable)]#[builder(overwritable)]
|
An issue I have with the A setter currently maps Also, as mentioned above by EdJoPaTo, perhaps a setter should only be allowed (at most) once, which would make I have fiddled a bit trying to implement |
|
@angelorendina, thank you for looking into this and considering a contribution!
Also, could you share your use case for Feel free to message me at https://bon-rs.com/discord for easier communication |
|
@Veetaha thanks for pointing out the I can actually repurpose the example in the documentation to explain our issue: imagine we needed the I have a workaround that looks like this: #[derive(bon::Builder)]
struct Command {
#[builder(field)]
inner_args: Vec<String>,
// marker field that at least one argument was passed, and the list is not empty
args: (),
name: String,
}
impl Command {
fn to_string(self) -> String {
// safe to assume inner_args is non-empty
assert!(!self.inner_args.is_empty());
todo!("should construct the string `name arg1 args2 ...`")
}
}
impl<S> CommandBuilder<S> where S: command_builder::State, S::Args: command_builder::IsUnset<> {
pub fn arg(self, arg: String) -> CommandBuilder<command_builder::SetArgs<S>> {
self.args(()).append_arg(arg)
}
}
impl<S> CommandBuilder<S> where S: command_builder::State, S::Args: command_builder::IsSet<> {
// (*) Does not compile, because rustc cannot infer that this is mutually exclusive with `arg` above...
// pub fn arg(mut self, arg: String) -> CommandBuilder<S> {
// self.inner_args.push(arg);
// self
// }
}
impl<S> CommandBuilder<S> where S: command_builder::State, S::Args: command_builder::IsSet<> {
// (*) ...so we have to give it a different name...
pub fn append_arg(mut self, arg: String) -> CommandBuilder<S> {
self.inner_args.push(arg);
self
}
}
#[test]
fn requires_arguments() {
let command = Command::builder()
.name("name".to_string())
.arg("first".to_string())
// (*) ...hence this does not compile...
// .arg("would be nice".to_string())
// (*) ...but this works fine.
.append_arg("second".to_string())
.build();
// Does not compile since `args` is never set :)
// let _no_compile = Command::builder().build();
}but it would be nice if this kind of functionality was supported by a simple #[derive(bon::Builder)]
struct Command {
#[builder(field)]
args: Vec1<String>,
name: String,
} |
Thank you for sharing this. Although I don't see how the example use case you shown could be nicely solved with Also, here is a bit simpler workaround without pub struct Command {
name: String,
args: Vec<String>,
}
// Decouple struct's representation from the builder (use method builder instead)
#[bon::bon]
impl Command {
#[builder]
fn new(
#[builder(field)]
inner_args: Vec<String>,
// Hide the setter for this member (make it private), `vis = "pub(self)"` would work exactly the same
#[builder(overwritable, setters(vis = ""))]
_args: (),
name: String,
) -> Self {
Self {
name,
args: inner_args,
}
}
}
use command_builder::{SetArgs, State};
impl<S: State> CommandBuilder<S> {
pub fn arg(mut self, arg: String) -> CommandBuilder<SetArgs<S>> {
self.inner_args.push(arg);
self.args(())
}
}
fn main() {
Command::builder()
.name("bon".to_owned())
.arg("one".to_owned())
.arg("two".to_owned())
.build();
// This doesn't compile:
// .build();
// ^^^^^ the member `Unset<args>` was not set, but this method requires it to be set
Command::builder()
.name("bon".to_owned())
.build();
}Yeah, it's still a bit ugly, but do you have a specific example with |
|
@angelorendina, I've just discovered a problem in the current By itself, this nesting of However, I tried to work around this problem and suggest an alternative solution to the one proposed above (#149 (comment)). With this approach the building process is split into two stages:
As you can see, the limitation here is that the order of the building matters. Although, this may be a plus, because I don't think you'd want to see the pattern like use bon::{bon, builder};
pub struct Command {
args: Vec<String>,
name: String,
}
#[bon]
impl Command {
// The first stage. Here we specify all regular named parameters.
// We specify `arg` as the `finish_fn` parameter, and make the `finish_fn` named
// after it. This way the `finish_fn` appears just like a regular setter to the caller,
// however, it returns a new type of the builder, once it is called, thus transitioning
// to a new state at the type level.
#[builder(finish_fn(name = arg))]
pub fn new(
#[builder(finish_fn)] //
arg: String,
name: String,
) -> CommandWithArg1Builder {
Self::with_arg1(arg, name)
}
// Here we accept all values from the stage 1, and add the ability to push new args
// via `#[builder(field)]`. A small problem here is that `arg1` needs to be passed as
// a `start_fn` parameter separate from `builder(field)`. Ideally, we'd have the ability
// to access start_fn parameters just like regular `#[builder(field)]`s
#[builder(builder_type(vis = "pub"), finish_fn = build)]
fn with_arg1(
#[builder(start_fn)] //
arg1: String,
#[builder(start_fn)] //
name: String,
#[builder(field)] //
mut args: Vec<String>,
) -> Self {
// That's suboptimal, but we can make `start_fn` parameters accessible in `self`
// of the builder just like `builder(field)` params. We don't even need a getter(mut)
// for that.
args.insert(0, arg1);
Self { args, name }
}
}
impl<S: command_with_arg1_builder::State> CommandWithArg1Builder<S> {
pub fn arg(mut self, arg: String) -> Self {
self.args.push(arg);
self
}
}
fn main() {
Command::builder()
.name("echo".to_owned())
.arg("Hello, World!".to_owned())
.arg("asd".to_owned())
.arg("asdsa".to_owned())
.build();
// Doesn't compile (method `build` was not found)
// Command::builder()
// .name("echo".to_owned())
// .build();
}Also, I think it's possible to swap the stages 1 and 2, if you'd like the caller to fill So I think what would be cool here, is the ability to access UPD After posting this, I came up with an even simpler solution. We can use the use bon::{bon, builder};
pub struct Command {
args: Vec<String>,
name: String,
}
#[bon]
impl Command {
// The first stage. Here we specify all
#[builder(finish_fn(name = arg))]
pub fn new(
#[builder(finish_fn)] //
arg: String,
name: String,
) -> Self {
Self {
args: vec![arg],
name,
}
}
}
impl Command {
pub fn arg(mut self, arg: String) -> Self {
self.args.push(arg);
self
}
}
fn main() {
// We finished the building process on the first call to `.arg(...)`
let command: Command = Command::builder()
.name("echo".to_owned())
.arg("Hello, World!".to_owned())
.arg("asd".to_owned())
.arg("asdsa".to_owned());
// Here building process isn't finished yet, so we have `CommandBuilder` type.
let command: CommandBuilder<_> = Command::builder()
.name("echo".to_owned());
} |
|
@Veetaha indeed, the issue we are facing is that we do need to explicitly spell out the type of the builder: we have a state machine that wraps various stages of the builder, so we need each stage to explicitly state which fields are set or unset. The issue you mentioned about overwritable is exactly what I meant when I originally said that I would expect setter: Builder<S> -> Builder<SetField<S>> where S: IsUnset<Field>
setter: Builder<S> -> Builder<S> where S: IsSet<Field>so that the setter avoid appending further I was fiddling with another possible implementation of the internals of the derive, and I got the following to work in a way that does not append multiple /// The actual structure we will build.
struct Thing {
a: u8,
b: u16,
}
/// Marker for a field that was populated with a value
struct Set<T>(T);
/// Marker for a field that was not populated
struct Unset;
/// Only implemented for [Set] and [Unset]
trait Field {}
impl<T> Field for Set<T> {}
impl Field for Unset {}
/// Represents the state of each stage of the builder,
/// where its associated types correspond to the final fields
/// and are marked as either [Set] or [Unset].
trait State {
type A: Field;
type B: Field;
}
/// Marker for a builder whose fields have not been set.
struct Empty;
impl State for Empty {
type A = Unset;
type B = Unset;
}
/// Marker for a builder whose A field has been set.
struct WithA<S: State>(PhantomData<S>);
impl<S: State> State for WithA<S> {
type A = Set<u8>;
type B = S::B;
}
/// Marker for a builder whose B field has been set.
struct WithB<S: State>(PhantomData<S>);
impl<S: State> State for WithB<S> {
type A = S::A;
type B = Set<u16>;
}
/// The actual builder, together with its generic representing its completion state.
struct Builder<S: State> {
a: S::A,
b: S::B,
}
// (*) Wrapper workaround to allow for mutually exclusive implementations, because...
trait SetterA<S: State>: Sized {
/// Sets the value of `a` for the builder.
fn a(self, a: u8) -> Builder<S>;
}
impl<S: State<A = Unset>> SetterA<WithA<S>> for Builder<S> {
// (*) ...this...
fn a(self, a: u8) -> Builder<WithA<S>> {
Builder {
a: Set(a),
b: self.b,
}
}
}
impl<S: State<A = Set<u8>>> SetterA<S> for Builder<S> {
// (*) ...and this should be mutually exclusive,
// but cause a compiler error due to a known bug.
fn a(self, a: u8) -> Builder<S> {
Builder {
a: Set(a),
b: self.b,
}
}
}
trait SetterB<S: State>: Sized {
/// Sets the value of `b` for the builder.
fn b(self, b: u16) -> Builder<S>;
}
impl<S: State<B = Unset>> SetterB<WithB<S>> for Builder<S> {
fn b(self, b: u16) -> Builder<WithB<S>> {
Builder {
a: self.a,
b: Set(b),
}
}
}
impl<S: State<B = Set<u16>>> SetterB<S> for Builder<S> {
fn b(self, b: u16) -> Builder<S> {
Builder {
a: self.a,
b: Set(b),
}
}
}
impl<S: State<A = Set<u8>, B = Set<u16>>> Builder<S> {
/// Finishes the build, returning the fully populated [Thing].
// Only available when all fields have been set, guaranteed at compile time by the trait bounds.
fn build(self) -> Thing {
Thing {
a: self.a.0,
b: self.b.0,
}
}
}
impl Thing {
/// Gets a Builder for [Thing].
fn builder() -> Builder<Empty> {
Builder { a: Unset, b: Unset }
}
}
#[test]
fn builder_typechecks_and_works() {
let x = Thing::builder().a(8).b(16).build();
assert_eq!(x.a, 8);
assert_eq!(x.b, 16);
let y = Thing::builder().b(16).a(8).build();
assert_eq!(y.a, 8);
assert_eq!(y.b, 16);
let z = Thing::builder()
// appends WithA
.a(8)
// appends WithB
.b(16)
// does NOT append WithA
.a(7)
// does NOT append WithB
.b(15)
.build();
assert_eq!(z.a, 7);
assert_eq!(z.b, 15);
// let _no_compile = Thing::builder()
// .a(8)
// .build();
} |
Yeah, now I see, I somehow missed that point initially 😳, sorry for that.
Thank you for sharing this, that's very helpful! I tried to fiddle with this approach as well, and adapt it to bon's codegen. However, one of the main problems that I see here is that this requires generating additional traits (one per each member), several method implementations (two per each member), which incurs compile time overhead. My initial idea for However, even if there weren't any compile time overhead, the bigger problem for me is that the generated setters become trait methods instead of being inherent methods. This would require the user of the builder to import the necessary traits into their scope, which is inconvenient. It also increases the API surface of the builder, making it prone to more API breakages. I couldn't find a way to keep the setters as inherent methods (yet?). I'll try to fiddle with this more. Hopefully there is a better way to do that. Otherwise I think this may be blocked by rust-lang/rust#20400 |
|
actually, i'd find it quite usable, as it adds some flexibility, which is simply missing otherwise, and bon cannot be used at all |
|
@dj8yfo could you share an example where |
|
@Veetaha it'll pop up some (small) time later in a link to this issue. Please don't mind if it appears silly to you |
This new attribute can be placed on individual members or at the top level via
#[builder(on(_, overwritable)]. It disables the compile-time check for overwriting already set members. For example with this attribute this code compiles:Why is this useful? It removes a whole bunch of generic state variables and
wherebounds from the generated code and considerably improves the compile times. For example in thefrankensteinrepo the difference for me is around 2-3 seconds of compile time.@EdJoPaTo would you be interested in enabling this in
frankensteinto improve the compile times (even at the cost of allowing overwrites)? To me it looks like the problem of overwrites is not that big and the compilation time perf. gains are probably more important.I have this feature already implemented on my branch #145, and I'm planning to include it in the next release.
A note for the community from the maintainers
Please vote on this issue by adding a 👍 reaction to help the maintainers with prioritizing it. You may add a comment describing your real use case related to this issue for us to better understand the problem domain.
The text was updated successfully, but these errors were encountered: