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

Stabilize #[builder(getter)] #225

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

Stabilize #[builder(getter)] #225

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

Comments

@Veetaha
Copy link
Contributor

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
Contributor 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
Contributor 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
Contributor 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

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

2 participants