Skip to content

#[builder(getter)] full batteries mode #225

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

Open
3 of 10 tasks
Veetaha opened this issue Dec 1, 2024 · 15 comments
Open
3 of 10 tasks

#[builder(getter)] full batteries mode #225

Veetaha opened this issue Dec 1, 2024 · 15 comments
Labels
feature request A new feature is requested

Comments

@Veetaha
Copy link
Collaborator

Veetaha commented Dec 1, 2024

This is a tracking issue for the #[builder(getter)] attribute feature. The stabilization of the design for this attribute requires some more feedback from the community.

If you think the current design is already solid and covers your current or potential future use cases, then just leave a 👍 reaction.

If you'd like to change something about the #[builder(getter)] attribute syntax and behavior or just extend it, then feel free to leave a comment. We'd be glad to hear your ideas!

Design Vision

Part of the following behaviors are already implemented, but not all.

#[builder(
    // For optional members automatically does `Option::as_ref` (reference
    // is moved under the Option).
    //
    // &T | Option<&T>
    getter,

    // For optional members does no conversion, because `&mut Option<_>` makes
    // sense if the caller wants to reset the `Option` to `None`.
    //
    // &mut T | &mut Option<T>
    getter(mut),

    // Deref is only supported for readonly getters. One probably doesn't
    // need deref for mutable getters, because returning e.g. `&mut String`
    // or `&mut Vec<T>` makes perfect sense.
    //
    // The argument to `deref` specifies the return type of the getter
    // (i.e. the target of the deref coercion). However, it can be omitted
    // (i.e. just `getter(deref)`) for some well-known types such as:
    // - String
    // - Vec<T>
    // - Box<T>
    // - Rc<T>
    // - Arc<T>
    // - Cow<T>
    // - PathBuf
    // - OsString
    // - CString
    // - Utf8PathBuf
    // - ...
    // Just look up the `Deref` impls in `std` here: https://doc.rust-lang.org/stable/std/ops/trait.Deref.html,
    // and in `camino` crate, we may extend this list in the future.
    //
    // &str | Option<&str>
    getter(deref(str)),

    // Useful for primitive types (e.g. `u32`)
    // Getter by `Copy` -> T | Option<T>
    getter(copy),

    // Getter by `Clone` -> T | Option<T>
    getter(clone),

    // Some other standard additional attributes
    getter(
        name = getter_name,
        vis = "",
        docs {
            /// Docs for getter_name
        }
    )

    // Multiple getters need to have name specified explicitly
    getter(name = getter_name_1),
    getter(name = getter_name_2, clone),

    // Custom readonly getter. Accepts a readonly reference and transforms it.
    // Only `&_` type is accepted (_ is replaced with member's type)
    getter = |value: &_| -> Ty { expr }

    // Custom mutable getter. Accepts a mutable reference and transforms it.
    // Only `&mut _` type is accepted (_ is replaced with member's type)
    getter = |value: &mut _| -> Ty { expr }

    // Give a name to the getter if there are several getters
    getter(name = foo, with = |value: &mut _| -> Ty { expr }),
)]

Initial discussion on this attribute: #221

Checklist

  • Support getters for &T
  • Support getters for #[builder(start_fn)] members
  • Support getters for #[builder(field)] members
  • Support getters for self receiver of methods
  • Support by-copy and by-clone getters
  • Support deref(...) config
  • Support mut getters
  • Support custom conversions (with closure and short getter = closure syntax)
  • Support multiple getters
  • Don't forget to extend Alternatives table in the docs with this new feature

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 Veetaha added the feature request A new feature is requested label Dec 1, 2024
@Veetaha Veetaha changed the title Introduce #[builder(getter)] Stabilize #[builder(getter)] Dec 1, 2024
@Veetaha
Copy link
Collaborator Author

Veetaha commented Dec 6, 2024

@angelorendina you mentioned in #149 (comment) that you got getter(mut) working. Although, I'd expect that to be difficult to do because mut is a keyword, and darling doesn't support parsing keywords due to syn's limitations (dtolnay/syn#1458).

Did you implement custom parsing of MetaList in your implementation?

@angelorendina
Copy link

@Veetaha in my implementation attempt, I simply used getter_mut as attribute 😄
I got it working with a simple example (essentially a copy of what I've found in the playground and tests folder) but I haven't given much thought on how the feature would interact with other attributes or functionalities, so I cannot guarantee there are no inconsistencies or pitfalls.

@Veetaha
Copy link
Collaborator Author

Veetaha commented Dec 6, 2024

Yeah, getter_mut is what I was expecting to see, since that would not fall into the darling parsing limitation trap.

Just a headsup, I have a draft PR, that I'm currently working on for deref, copy, clone support (#229). I added a custom FromMeta implementation in that PR for GetterConfig. I think once that one is merged it should be easy to add custom getter(mut) parsing in that FromMeta impl.

@Veetaha
Copy link
Collaborator Author

Veetaha commented Dec 8, 2024

I'm revisiting the getters design for start_fn members and self receiver of methods. I think we can avoid providing getters for them, and make those available as regular private fields on the builder instead. This aligns their behavior with #[builder(field)]. I think, providing them as regular fields is more convenient than providing getters. E.g. the following should be possible:

#[derive(bon::Builder)]
struct Command {
    #[builder(start_fn)]
    args: Vec<String>,
}

impl<S: command_builder::State> CommandBuilder<S> {
    fn arg(mut self, arg: String) -> Self {
        self.args.push(arg);
        self
    }
}

The dilemma here is in the naming of the builder's field for the method's self receiver. Rust disallows r#self identifier, so how should it be called instead?

  • self_
  • receiver
  • this
  • Something else?

Maybe the name has to be explicitly configured via #[builder(name)]? I'm leaning more towards the latter

@musjj
Copy link

musjj commented Jan 12, 2025

I think one thing I would love to see is the ability to generate getters for all fields directly:

#[derive(Builder)]
#[builder(on(_, getter))]
struct User {
    name: String,
    is_admin: bool,
    level: Option<u32>,
}

This currently generates an error.

Also, I'm not sure if I'm misunderstanding the scope here, but will getters be accessible after the struct is built? The current implementation doesn't seem to do this, but I'm not sure if it's by design.

@Veetaha
Copy link
Collaborator Author

Veetaha commented Jan 12, 2025

I think having on(_, getter) makes sense, although I wonder what is your use case for getters on builders, @musjj?

derive(Builder) isn't going to generate getters on the built struct, and this is by design. Otherwise it would be too surprising to see some other methods generated on the struct by derive(Builder) other than builder(). There is an open issue about deriving accessors (#147) where that could fit. However l haven't even decided wether to put those into the same or separate crate and how to organize the docs for that to make it coexist with the builder feature

@musjj
Copy link

musjj commented Jan 12, 2025

Yeah, I guess it's out-of-scope-ish for this crate. I just found out there's already a crate for this: https://github.com/jbaublitz/getset and it works well together with bon.

@TheButlah
Copy link

TheButlah commented Feb 25, 2025

Posting here since you asked for feedback. I am brand new to bon. Here is my use case:

Builder roughly looks like:

struct Cfg {
  a: A,
  b: B,
}

I want to write a custom setter to allow the user to "derive" b from the value of a. I also want to keep the existing setter for b so that it can just be set directly by the user. So I was planning to do something like this:

impl<S: cfg_builder::State> CfgBuilder<S> {
    pub fn b_from_a(
        self,
    ) -> CfgBuilder<cfg_builder::SetB<S>>
    where
        S::A: cfg_builder::IsSet,
        S::B: cfg_builder::IsUnset,
    {
        let a = &self.a; // not possible! Unless maybe with the getters api?
        let b = {
            // some code that computes a `B` from `a`.
        };
        self.b(b)
    }
}

Importantly, the getter for a should not be publicly visible, its just for internal use by my custom setter.

Is this something that the getter feature would be intended for? Alternatively, perhaps just having a way to directly access the field would be sufficient? Again, I would want to have control over the field visibility, as ideally it is only directly accessible in this manner by b_from_a.

@Veetaha
Copy link
Collaborator Author

Veetaha commented Feb 25, 2025

Hi @TheButlah, I think for your use case it may be enough to set a default for field b that depends on the value of field a like this:

#[derive(bon::Builder)]
struct Cfg {
  a: u32,

  #[builder(default = a * 2)]
  b: u32,
}
fn main() {
    // Derive the value of `b` from `a` if `b` is not provided
    let cfg = Cfg::builder().a(3).build();
    assert_eq!(cfg.b, 6);

    // Use the provided value of `b` if it is provided
    let cfg = Cfg::builder().a(3).b(5).build();
    assert_eq!(cfg.b, 5);
}

For more advanced use cases I recommend defining a builder based on a function (function builder paradigm shift).

Does this solution fit your problem?

@TheButlah
Copy link

TheButlah commented Feb 25, 2025

I think this would have worked, but in reality, my b_from_a is an async function. AFAICT async is not supported in the expression passed to the builder(default) attribute. Also, having it be initialized by default is a bit less explicit than I'd like, I like the user knowing that b is actually going to be set from a.

I just read the page you linked about the function builder paradigm shift, but I didn't understand how this applies here. Can you elaborate?

@TheButlah
Copy link

TheButlah commented Feb 25, 2025

More generally though, I think it is reasonable that custom setters should get access to any prior initialized values, via the IsSet marker trait. Or at least access to the internal Options. Letting custom setters get access to previous initialized values would solve my problem, and make the getter api unecessary.

@Veetaha
Copy link
Collaborator Author

Veetaha commented Feb 25, 2025

@TheButlah I see, the fact that your b_from_a should be async changes everything. The function-builder approach can still work here like this, although your builder would then be uncoditionally async:

struct Cfg {
    a: u32,
    b: u32,
}

#[bon::bon]
impl Cfg {
    #[builder]
    async fn new(a: u32, b: Option<u32>) -> Self {
        let b = if let Some(b) = b {
            b
        } else {
            // Do smth async to compute `b` based on `a` if `b` is not provided
            async { a * 2 }.await
        };

        Self { a, b }
    }
}

#[tokio::main]
async fn main() {
    // Derive the value of `b` from `a` if `b` is not provided
    let cfg = Cfg::builder().a(3).build().await;
    assert_eq!(cfg.b, 6);

    // Use the provided value of `b` if it is provided
    // Unfortunately, the `build()` in this case is still async
    let cfg = Cfg::builder().a(3).b(5).build().await;
    assert_eq!(cfg.b, 5);
}

You could also use #[builder(required)] for b or use some other enum rather than Option for b to require the user to explicitly call .b(None) or .b(MyCustomEnum::DeriveBFromA).

However, if you want to be even more granular and explicit and have a sync signature you do need the getter feature to implement a custom async setter as you shown.

So now I get your point, the #[builder(getter)] feature does solve your use case. Note that we can't expose the underlying fields of the builder (they are considered private), because the builder does an unsafe unwrap_unchecked() in its finishing function for required fields because it relies on IsSet type state to guarantee that internal Option<_>s are Some.

@TheButlah
Copy link

OK, I will upvote the original post not because I actually want end-users to have access to the getters, but because I want access to the getters in my custom setters :P

@Veetaha
Copy link
Collaborator Author

Veetaha commented Feb 25, 2025

Understandable, using getters only privately in setters is one of the use cases I envisioned. Just make sure to mark the getters as private with #[builder(getter(vis = ""))] or #[builder(getter(vis = "pub(self)"))]

Veetaha added a commit that referenced this issue Mar 5, 2025
…lds (#250)

This is according to my design decision here:
#225 (comment)
@Veetaha Veetaha changed the title Stabilize #[builder(getter)] #[builder(getter)] full batteries mode Mar 5, 2025
@Veetaha
Copy link
Collaborator Author

Veetaha commented Mar 5, 2025

I'm stabilizing the current MVP of this feature that's been in experimental state for 3 months: #251.

The next minor version of bon 3.4, that I'll release today, will no longer require the cargo feature experimental-getters to use the existing #[builder(getter)] API. That cargo feature will be left as a no-op for backwards compatibility, and will be removed in 4.0 if/when bon actually comes to the point of a new major breaking release, for which I have no plans at the moment.

I've renamed this issue to denote that what's left is to implement the rest of the batteries design as described in the issue body:

  • mutable getters
  • multiple getters for the same field
  • getter with closure attribute syntax

I've also implemented a change that makes the self receiver of the method and #[builder(start_fn)] members accessible as private fields on the builder (#250).

Now bon generates stable names for some native private fields of the builder that are documented and accessible to bon macro users:

  • self_receiver - stores the self value of the method (for builders generated from a method)
  • {member_name} - stores the value of the member annotated with #[builder(start_fn)]. These members are initialized when the builder is created.

These native private builder fields can be freely accessed in custom setters/getters or generally within the same module scope.

Veetaha added a commit that referenced this issue Mar 5, 2025
It's been ~3 months since we've introduced an MVP of the
`#[builder(getter)]` attribute, and the feature hasn't changed since its
initial design. There are already some people using this API and asking
to get it stabilized.

I believe the current API design is already firm, and still amenable to
the future extensions described in
#225, so let's deploy this 🚀
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

4 participants