Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce bool shorthand (#[builder(flag)]) #142

Open
SteelAlloy opened this issue Sep 17, 2024 · 13 comments
Open

Introduce bool shorthand (#[builder(flag)]) #142

SteelAlloy opened this issue Sep 17, 2024 · 13 comments
Labels
feature request A new feature is requested

Comments

@SteelAlloy
Copy link

SteelAlloy commented Sep 17, 2024

Hey there,

Bon is great but it still feels weird to write .property(true) for booleans and I feel that an option to shorten this usage would be a nice addition.

The syntax is just an example of how I would see it implemented :

use bon::Builder;

#[derive(Builder)]
struct User {
    id: u32,
    #[builder(bool)] // add some attribute to a boolean property
    color: bool,
}

And then use it without specifying true :

let user = User::builder()
    .id(1)
    .color() // true
    .build();

For falsy values, one could think of adding the same method with a prefix (no_ by default) :

use bon::Builder;

#[derive(Builder)]
struct User {
    id: u32,
    #[builder(bool, false = "no")] // optional, defaults to "no"
    color: bool,
}

And then :

let user = User::builder()
    .id(1)
    .no_color() // false
    .build();

let user = User::builder()
    .id(1)
    .color() // still valid : true
    .build();

Feedback is welcome, it would greatly improve my builders!

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.

@Veetaha
Copy link
Contributor

Veetaha commented Sep 17, 2024

Maybe something like

#[derive(Builder)]
struct Example {
    #[builder(flag)]
    color: bool,
}

It would generate three methods:

  • color() -> true
  • no_color() -> false
  • set_color(value: bool)

I think a method that accepts a value is still useful in many cases to make conditional building painless. For example, if the value of color should be computed from an env var, the user will still be able to use set_color(std::env::var("COLOR").is_ok()) instead of doing an if-else dance of calling color/no_color methods.

In fact the if-else dance is the thing I tried to avoid and enforce the users of bon not to do by not providing such a "flag" API initially. I think it can be harmful for the API users who don't want to just hardcode the value to true or false but rather get the value for the boolean from some dynamic source (like env vars, CLI args, etc.).

Anyway this feature by itself can just provide 3 setters to simplify the syntax for the cases where the API is used with hardcoded values a lot.

Also, this is how I imagine the syntax to override the names of each of the 3 setters:

struct Example {
    #[builder(
        // the values specified here are the ones that will be used by default (#[builder(flag)])
        flag(
            true = color, // sets the bool to `true`
            false = no_color, // sets the bool to `false`
            set = set_color // accepts a `bool` argument 
        ),
    )]
    color: bool,
}

I'm not sure about flag(set = ...) naming yet though. I'd be happy to hear any other suggestions. Maybe instead it could be arg and then the default name would be arg_color (but this seems too unconventional).

For example, AWS SDK for Rust uses a similar convention of set_ prefix for methods that accept Option<T>, while methods that accept T have no prefix.

Also, what about Option<bool> or #[builder(default)] fields? I suppose it would then generate 4 setters.

#[derive(Builder)]
struct Example {
    #[builder(flag)]
    color: Option<bool>,
}

There will be methods:

  • color() -> Some(true)
  • no_color() -> Some(false)
  • set_color(value: bool)
  • maybe_color(value: Option<bool>)

But.. no_color doesn't look very intuitive for optional members as the callers may think as if it sets the value to None or the default value.

This has to be supported because booleans are often annotated with #[builder(default)] and sometimes the value of that default is default = true. So there has to be a method that accepts an Option<bool> where None uses the default to make evolving such API possible without breaking changes (i.e. changing the required boolean flag field to optional).

I like the no_color naming convention because it aligns with naming patterns in CLI applications, but what I currently don't like about this design is the potential confusion with None value for optional boolean fields. But maybe not to the extent of blocking this feature...

@Veetaha Veetaha added the feature request A new feature is requested label Sep 17, 2024
@SteelAlloy
Copy link
Author

struct Example {
    #[builder(
        // the values specified here are the ones that will be used by default (#[builder(flag)])
        flag(
            true = color, // sets the bool to `true`
            false = no_color, // sets the bool to `false`
            set = set_color // accepts a `bool` argument 
        ),
    )]
    color: bool,
}

The naming sounds good to me 👍

Need to think about the potential confusion with options, that's an aspect I hadn't considered.

@Veetaha
Copy link
Contributor

Veetaha commented Sep 19, 2024

Alternatively, the false setter may just be omitted by default. Since having that many setters also complicates the API. I wonder if that'd really be a frequent use case for the false setter.

Also, I'm currently working on a wider feature that allows adding arbitrary setters and methods to the builder that will cover this use case, although it'll require you to write your own impl block with methods which is a bit more code, but will at least make it possible to create such API by yourself. After that is done we may have the #[builder(flag)] syntax sugar

@SteelAlloy
Copy link
Author

Such a feature would be great, I'm currently having trouble with custom methods in my builders
I don't mind writing my own impl block

@Veetaha
Copy link
Contributor

Veetaha commented Sep 21, 2024

I've started work on the feature for custom method extensions of builder type and stable type state signature in #145.

See the usage shown in the PR description there for an example of how you may implement this feature using this new upcoming flexible low-level API.

I still need to add some syntax for hiding setters (overriding their visibility and renaming them) to make this use case possible, which I'll do as part of that PR. Unfortunatelly this update will require a 3.0 version bump of bon because it generates new items in the user's scope (namely a module named after the builder, but in snake case). It should be a minor breaking change such that 99% of users can upgrade without any migration, but still potentially breaking.

@Veetaha Veetaha changed the title Feature request: bool shorthand Feature request: bool shorthand (#[builder(flag)]) Sep 30, 2024
@Veetaha
Copy link
Contributor

Veetaha commented Sep 30, 2024

I'm revisiting the design for this yet again as part of the bigger 3.0 design effort.

I propose to make this syntax sugar a bit more limited, but probably more intuitive.

// Generates two setters for booleans:
// - `my_lovely_flag() -> true`
// - `with_my_lovely_flag(bool)`
//
// It also automatically implies that the setters are optional to call.
// The default value is `false` automatically. The `#[builder(default)]`
// attribute is not allowed with the `flag` attribute.
#[builder(flag)]
my_lovely_flag: bool,

I decided to change the by-value setter prefix to with_, which is more convetional for setters that accept self by value. I'd like to reserve the set_ prefix convention for by-&mut self setters (future #[builder(mutable)] attribute).

In the future there will be a member-level #[builder(required)] attribute (supported with #[builder(on(_, required))] at the top-level) that can be combined with flag fields to make sure the user doesn't omit calling their setters (exhaustiveness).

@b4D8
Copy link

b4D8 commented Oct 14, 2024

Hi there, thanks for your great work on this crate!

I can relate that this flag feature would be a great enhancement to make the generated API feel more natural.

I think we could recycle the logic applied to Options (with the maybe_ prefix) to bools and even any other type with a default value.

The flag attribute would then allow any type with a default value to opt-in the current behavior of optional values, just preventing the exponential growth of the API. Example:

pub struct MyType<T> {
    my_option: Option<T>,
    my_flag: bool,
}

#[bon]
impl<T> MyType<T> {
    #[builder]
    fn new(my_option: Option<T>, my_flag: #[builder(flag)] bool) -> Self {
       Self { my_option, my_flag }
    }
}

MyType::builder().maybe_my_option(Some(t)).build();
MyType::builder().maybe_my_option(None).build();
MyType::builder().my_option(t).build();

MyType::builder().maybe_my_flag(true).build();
MyType::builder().maybe_my_flag(false).build();
MyType::builder().my_flag().build();

Also, this flag feature could be extended to support enums.

Here's an example use case with redis's SET request.

With the following AST:

pub enum Condition {
    // if not exists
    Nx,
    // if already exists
    Xx,
}

pub struct Duration<P>(i64, PhantomData<P>);

pub struct UnixTimestamp<P>(Duration<P>);

pub struct Seconds;

pub struct Milliseconds;

pub enum Expiration {
    Ex(Duration<Seconds>),
    ExAt(UnixTimestamp<Seconds>),
    Px(Duration<Milliseconds>),
    PxAt(UnixTimestamp<Milliseconds>),
    KeepTtl,
}

pub struct SetRequest<'a> {
    key: &'a [u8],
    value: &'a [u8],
    condition: Option<Condition>,
    expiration: Option<Expiration>,
    get: bool
}

Ideally, I'd like to be able to define the builder as such:

use bon::bon;

#[bon]
impl<'a> SetRequest<'a> {
    #[builder]
    fn new<K, V>(
        key: &'a (impl ?Sized + AsRef<[u8]>),
        value: &'a (impl ?Sized + AsRef<[u8]>),
        #[builder(flag)] condition: Option<Condition>,
        #[builder(flag)] expiration: Option<Expiration>,
        #[builder(flag)] get: bool,
    ) -> Self {
        Self {
            key: key.as_ref(),
            value: value.as_ref(),
            condition,
            expiration,
            get
        }
    }
}

And it would provide the following usages:

SetRequest::builder()
    .key("hello")
    .value("world")
    .maybe_condition(Some(Condition::Nx))
    .maybe_expiration(Some(Expiration::Ex(Duration::<Seconds>::try_from(42)?)))
    .maybe_get(true)
    .build()?;

SetRequest::builder()
    .key("hello")
    .value("world")
    .condition(Condition::Nx)
    .expiration(Expiration::Ex(Duration::<Seconds>::try_from(42)?))
    .get()
    .build()?;

SetRequest::builder()
    .key("hello")
    .value("world")
    .nx()
    .ex(Duration::<Seconds>::try_from(42)?)
    .get()
    .build()?;

If you feel like this is interesting, it'd be happy to contribute :)

PS: Sorry for the long post, I figured it would be easier to explain with actual code and real world example ^^"

@Veetaha
Copy link
Contributor

Veetaha commented Oct 14, 2024

Hi, thank you for sharing your ideas!

#[builder(flag)] condition: Option<Condition>,

I don't understand what the attribute #[builder(flag)] changes for members of type Option<T>. Members of type Option<T> already generate two setters {member}(T) and maybe_{member}(Option<T>) by default, so there is no need for additional attribute to enable that behavior.

I think we could recycle the logic applied to Options (with the maybe_ prefix) to bools and even any other type with a default value.

The maybe_ setter is generated for members with #[builder(default)] by default as well.

The prefix maybe_ was chosen to denote a setter that takes an Option<T> value. The naming of such setter implies "the setter accepts a value that can be None, and if it's None - then use the default value". In case of boolean flags, the default value is always false, and that additional setter accepts a bool (not an Option<bool>). Therefore, it would be confusing to have maybe_flag(bool), because the word "maybe" implies there could be a "lack of value", when in fact there is no "lack of value" to represent for a bool (it always has a value: true or false).

Therefore, I think maybe_{member} terminology doesn't really fit to boolean flags. I propose to use the with_{member}(bool) naming instead, which doesn't imply a potential "lack of value".

@b4D8
Copy link

b4D8 commented Oct 14, 2024

Wow, thanks for the fast and thorough reply.

I didn't know that the builder(default) attribute was already generating the maybe_{member} setters, that's great!

I totally understand now why we should have a different prefix for bools.

My idea with the builder(flag) attribute on an optional value was that it could somehow "traverse" the Option allowing to use the variants of an enum as flags, like this:

SetRequest::builder()
    .key("hello")
    .value("world")
    .nx() // sugar for `.condition(Condition::Nx)` or `.maybe_condition(Some(Condition::Nx))`
    .ex(42.try_into()?) // sugar for `.expiration(Expiration::Ex(42.try_into()?))` or `.maybe_expiration(Some(Expiration::Ex(42.try_into()?)))`
    .get() // sugar for `get(true)` which will become `.with_get(true)` as far as I understand
    .build()?;

@Veetaha
Copy link
Contributor

Veetaha commented Oct 14, 2024

My idea with the builder(flag) attribute on an optional value was that it could somehow "traverse" the Option allowing to use the variants of an enum as flags

Aha, now I see the idea. There are several problems with this design, such that I think, it shouldn't be part of #[builder(flag)] or some other specialized attribute. Here is why.

Limited lexical scope

The fundamental limitation of macros (both declarative and procedural) is that they only see "the code" they were applied to. For example:

#[bon]
impl<'a> SetRequest<'a> {
    #[builder]
    fn new<K, V>(
        key: &'a (impl ?Sized + AsRef<[u8]>),
        value: &'a (impl ?Sized + AsRef<[u8]>),
        #[builder(flag)] condition: Option<Condition>,
        #[builder(flag)] expiration: Option<Expiration>,
        #[builder(flag)] get: bool,
    ) -> Self {
        Self {
            key: key.as_ref(),
            value: value.as_ref(),
            condition,
            expiration,
            get
        }
    }
}

Here, the code generation is performed by the active #[bon] attribute. This is "the code" that it sees. Now, imagine you are the #[bon] attribute. Do you know if Condition and Expiration are enums or structs from only this context? Even if you assume that these two types are enums, how do you know what variants they have? The problem is that you don't know. The macro only sees the words Condition and Expiration, but nothing more than that... So it can't generate setters for every enum variant having this context provided to it by the compiler.

Although, there are some hacks to provide more context about the Condition and Expiration types structure that would involve annotating these types with additional macros. But.. They involve generating declarative macro callbacks, and this approach is quite fragile, and doesn't work that well when the types come from external modules.

Demand for this feature

This feature of generating a setter for every enum variant looks rather exotic to my taste. My gut feeling is that there isn't a considerable demand for this feature out there. So.. it doesn't really make sense to provide syntax sugar for this case. The attribute #[builder(flag)] is fine because I do see how many people would like to use it frequently. But.. for the enums case I don't, and thus even if we had smth like #[builder(enum)] for that, it would only be used so rarely, that the cognitive overhead of this feature isn't worth the syntax saving, as for me.

However, I did say the words "syntax sugar". The term "syntax sugar" means just a shorter notation for what's already possible to express. So it doesn't mean that this API isn't physically possible to express with bon, it will be possible to define such API, albeit it will take a bit more characters to type to do that.

Alternative

I'm currently working on a big PR stabilizing the builder's type state API (#145). It would allow you to add custom setters to the builder struct directly, by defining your own impl block for the builder. So this will be possible to achieve using the following lower-level code. It already compiles from my PR's branch (#145):

use bon::bon;
use std::marker::PhantomData;

#[bon]
impl<'a> SetRequest<'a> {
    #[builder]
    fn new(
        key: &'a (impl ?Sized + AsRef<[u8]>),
        value: &'a (impl ?Sized + AsRef<[u8]>),
        condition: Option<Condition>,
        expiration: Option<Expiration>,
        #[builder(setters(name = set_get))] get: bool,
    ) -> Self {
        Self {
            key: key.as_ref(),
            value: value.as_ref(),
            condition,
            expiration,
            get,
        }
    }
}

// Import type-state API tooling from the generated module
use set_request_builder::{IsUnset, SetCondition, SetExpiration, SetGet, State};

// Define a custom impl block for the builder with custom setters
impl<'a, S: State, K, V> SetRequestBuilder<'a, K, V, S>
where
    K: ?Sized + AsRef<[u8]>,
    V: ?Sized + AsRef<[u8]>,
{
    // Custom setter for the enum
    fn nx(self) -> SetRequestBuilder<'a, K, V, SetCondition<S>>
    // Setters need to have a `where` clause requiring that they can only be called if the
    // member wasn't set yet (i.e. the state for the member implements `IsUnset` trait)
    where
        S::Condition: IsUnset,
    {
        self.condition(Condition::Nx)
    }

    // Custom setter for the enum that accepts a value
    fn ex(self, duration: Duration<Seconds>) -> SetRequestBuilder<'a, K, V, SetExpiration<S>>
    where
        S::Expiration: IsUnset,
    {
        self.expiration(Expiration::Ex(duration))
    }

    // Custom boolean `flag` setter. Note that `#[builder(flag)]` isn't implemented yet, so in the
    // mean time it will be possible to just use this API to implement flag-like setters, but the
    // `#[builder(flag)]` feature should be available in the `3.1` release shortly after `3.0` is released
    fn get(self) -> SetRequestBuilder<'a, K, V, SetGet<S>>
    where
        S::Get: IsUnset,
    {
        self.set_get(true)
    }
}


pub enum Condition {
    // if not exists
    Nx,
    // if already exists
    Xx,
}

pub struct Duration<P>(i64, PhantomData<P>);

impl<P> Duration<P> {
    fn new(value: i64) -> Self {
        Self(value, PhantomData)
    }
}

pub struct UnixTimestamp<P>(Duration<P>);

pub struct Seconds;

pub struct Milliseconds;

pub enum Expiration {
    Ex(Duration<Seconds>),
    ExAt(UnixTimestamp<Seconds>),
    Px(Duration<Milliseconds>),
    PxAt(UnixTimestamp<Milliseconds>),
    KeepTtl,
}

pub struct SetRequest<'a> {
    key: &'a [u8],
    value: &'a [u8],
    condition: Option<Condition>,
    expiration: Option<Expiration>,
    get: bool,
}

fn main() {
    SetRequest::builder()
        .key("hello")
        .value("world")
        .maybe_condition(Some(Condition::Nx))
        .maybe_expiration(Some(Expiration::Ex(Duration::new(42))))
        .set_get(true)
        .build();

    SetRequest::builder()
        .key("hello")
        .value("world")
        .condition(Condition::Nx)
        .expiration(Expiration::Ex(Duration::new(42)))
        .get()
        .build();

    SetRequest::builder()
        .key("hello")
        .value("world")
        .nx()
        .ex(Duration::new(42))
        .get()
        .build();
}

@Veetaha
Copy link
Contributor

Veetaha commented Oct 14, 2024

Btw, I just noticed, that the impl block with the #[bon] attribute can be avoided. In the next version of bon it will be possible to express such a builder using the derive(Builder) on the struct directly thanks to the new #[builder(with)] attribute that overrides the signature of the generated setters:

#[derive(bon::Builder)]
pub struct SetRequest<'a> {
    #[builder(with = |value: &'a (impl ?Sized + AsRef<[u8]>)| value.as_ref())]
    key: &'a [u8],

    #[builder(with = |value: &'a (impl ?Sized + AsRef<[u8]>)| value.as_ref())]
    value: &'a [u8],

    condition: Option<Condition>,
    expiration: Option<Expiration>,
    
    // In `3.1` (after 3.0) this will just be `#[builder(flag)]`
    #[builder(setters(name = set_get))]
    get: bool,
}

But you'll still need to have a custom impl block for the specialized enum setters

@b4D8
Copy link

b4D8 commented Oct 14, 2024

I see you've given deep thoughts in the overall design already and it all makes sense.
I actually prefer your alternative :)

@Veetaha
Copy link
Contributor

Veetaha commented Nov 9, 2024

3.0.0-rc version was published. I'll prepare a blog post for the release and switch it to 3.0.0 on November 13-th

@Veetaha Veetaha changed the title Feature request: bool shorthand (#[builder(flag)]) Introduce bool shorthand (#[builder(flag)]) Nov 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request A new feature is requested
Projects
None yet
Development

No branches or pull requests

3 participants