Stabilize `#[builder(overwritable)]`

[Veetaha]

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:

#[derive(bon::Builder)]
struct Example {
    #[builder(overwritable)]
    x: u32,
}

Example::builder()
    .x(1)
    .x(2)
    .build();

Why is this useful? It removes a whole bunch of generic state variables and where bounds from the generated code and considerably improves the compile times. For example in the frankenstein repo the difference for me is around 2-3 seconds of compile time.

@EdJoPaTo would you be interested in enabling this in frankenstein to 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.

[EdJoPaTo]

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… 🤔

[Veetaha]

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".

[Veetaha]

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).

[EdJoPaTo]

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?

[Veetaha]

Well, the difference between overwritable and non-overwritable becomes visible only at a greater scale (especially in structs with 50+ fields, because the compile time complexity grows quadratically (unfortunatelly) with the number of fields in the struct).

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 frankenstein. This is where you already use the attribute alias approach, whereby requiring users to have an attribute alias by default to protect from overwrites doesn't seem reasonable.

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 clippy.

Setting something twice seems like a mistake so having that is a code smell

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 cfg_attr(..., on(_, overwritable)) if and only if you know your code doesn't depend on the type signature of the builder (i.e. it doesn't spell the UserBuilder<...> type exactly anywhere in the business logic of your code). Also you may be sure that code outside of your crate can't spell the type of the builder because its type signature will now reference symbols "unrechable" (or "undenotable") from outside of the module that derives the builder.

[EdJoPaTo]

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 (user() return type) which is something a user of the lib shouldn't care about. Which seems like a red flag to me: requiring internals that should be transparent and ignored in the first place. (yeah, we can just let Rust tell us which type to use as a return type, but needing to specify generated internals seems like a smell.)

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 frankenstein. This is where you already use the attribute alias approach, whereby requiring users to have an attribute alias by default to protect from overwrites doesn't seem reasonable.

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?

[Veetaha]

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 "experimental-overwritable" waiting for some more user feedback on the use cases.

[EdJoPaTo]

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 😇

[Veetaha]

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

[MuhannadAlrusayni]

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 value and error values comes from multiple sources and they have precedences too. With these requirements I would write something like this:

// 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.

[Veetaha]

@MuhannadAlrusayni

I already use TypedBuilder for this

Could you elaborate how you configured TypedBuilder to allow for overwrites? As I know, TypedBuilder doesn't allow calling setters repeatedly, and there is no attribute to disable the overwrites protection in typed-builder.

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 .a(Some(1)).a(None), then the resulting value in a will be None. There is no special behavior for Option<T> in this case.

Just think of this like the following way (with the .value()/maybe_value() from your example):

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 Option::or/Option::or_else and Option::unwrap_or/Option::unwrap_or_else methods to implement that.

#[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 value should not be wrapped in Option<...>, because it always has a default value, so it can never be None.