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

Allow specifying mandatory Option argument #35

Closed
pablosichert opened this issue Aug 5, 2024 · 20 comments · Fixed by #157
Closed

Allow specifying mandatory Option argument #35

pablosichert opened this issue Aug 5, 2024 · 20 comments · Fixed by #157
Labels
feature request A new feature is requested

Comments

@pablosichert
Copy link

pablosichert commented Aug 5, 2024

Awesome library! Had a lot of fun using it so far.

I have an edge case where one of my input argument is an Option<bool> since I want to encode the states yes/no/maybe via Some(true)/Some(false)/None. Currently, bon::builder would interpret that argument as optional, even though I want it to be mandatory.

#22 could be related in a sense that it would allow me to work around this issue.


For other people looking for a workaround: create a typealias type ... = Option<T>, such that bon is not aware of the top-level Option type.

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 Aug 5, 2024
@Veetaha
Copy link
Contributor

Veetaha commented Aug 5, 2024

Hi! Thank you for creating the issue! I've been thinking of adding smth like #[builder(must_use/explicit)] (name is not settled yet) for an Option and #[builder(default)] parameters to explicitly configure that the caller must not omit calling the setter for the optional field. Do you think this will solve it for you?

Example:

struct Foo {
     #[builder(must_use)]
     optional: Option<u32>,

     #[builder(default = 42, must_use)]
     has_default: u32,
}

let foo = Foo::builder()
    .maybe_optional(None)    // not calling this or `.optional(value)` will be a compile error
    .maybe_has_default(None) // not calling this or `.has_default(value)` will be a compile error
    .build();
    
assert_eq!(foo.optional, None);
assert_eq!(foo.has_default, 42);

Note that the workaround you suggested (hiding Option from bon behind a type alias) has a drawback in that bon won't generate a pair of methods member(value: T) and maybe_member(value: Option<T>) in this case.

@Veetaha
Copy link
Contributor

Veetaha commented Aug 5, 2024

I re-read the issue again. I suppose in this use case Option is used, but no None default value is not implied, right? You'd like to make it genuinely required and have the same behavior as if was just a regular field of Option<T> type without any special treatment like having a pair of member/maybe_member methods?

This would be a bit different from what I suggested higher. In this case another attribute like #[builder(transparent)] would be needed in bon (we don't have it now yet). The difference is that #[builder(transparent)] will prevent generating a pair of member/maybe_member methods, and make the argument required

@pablosichert
Copy link
Author

You'd like to make it genuinely required and have the same behavior as if was just a regular field of Option<T> type without any special treatment like having a pair of member/maybe_member methods?

Yes, exactly. Thank you for looking into this!

The suggested #[builder(transparent)] sounds good (without having looked into which other edge cases this might open up from that).

@Veetaha
Copy link
Contributor

Veetaha commented Aug 8, 2024

Here are some more thoughts about this.

The proposed #[bon(transparent)] may be confusing. Users may mistakenly think that this attribute can be used to disable automatic Into conversions or some other automatic behaviours. Also, I'd want to follow the rule "add attribute only if unavoidable". Having an attribute for a niche use case (such as this) increases the complexity of the API of bon and it makes it harder for people to study it (from docs perspective the is just more attributes to learn).

Maybe it's fine to use a type alias in this case to explicitly state that bon shouldn't treat the field as Option. I guess it doesn't hurt if you need to do this very rarely.

Also another interesting workaround for this that I found is that you can prefix Option with r# to make bon stop detecting it as an Option. The r# prefix is used in Rust to prevent the compiler from treating an identifier as a keyword (special behavior).

Turns out I unintentionally coded bon such that it has a similar behavior for Option detection in this case as well. If Option is prefixed with r#, then bon stops treating the type specially. Sounds rather fun that this unintended behavior actually kinda fits the use case 🐱.

For example, this works as you'd like:

#[bon::builder]
fn foo(val: r#Option<i32>) {}

// `val` is a mandatory field that has no default and must be set
// No special `Option` handling was applied by `bon`
foo().val(Some(32)).call();

Maybe this behavior of r#Option could even be documented and guaranteed in by bon API. What do you think @pablosichert?

@pablosichert
Copy link
Author

That's a neat little workaround – could roll with it.

However, I must say that if I was reading code that used r#Option I would immediately think that the r# is superfluous and the author forgot it there after changing away from a previously used type that clashes with a keyword.

Maybe a #[required] attribute could do?

That said, both the typealias and r# would be good enough for me to handle this case.

@Veetaha
Copy link
Contributor

Veetaha commented Aug 8, 2024

The term required is a bit overloaded. There are two parts of "being required".

  1. The setter method must be called during building (exhaustiveness).
  2. There is no default value for the member (no maybe_ method existing).

There should be some way to configure both of these independently, and I just can't find good names and syntax for that.

Maybe #[builder(required)] forces the setter not to be omitted (exhaustiveness) and removes the maybe_ method. And then #[builder(required, default)] generates the maybe_ method but still makes the setter required to call (exhaustiveness). The required, default combo looks a bit confusing, so I'm still looking for better design. I'll keep this issue open to see if I or anyone else has other bright ideas

@Veetaha
Copy link
Contributor

Veetaha commented Aug 9, 2024

On the second thought #[builder(required)] looks bearable when paired with #[builder(default)]. So the proposed behavior is the following:

struct Example {
    #[builder(required)]
    arg1: Option<String>,

    // A bit confusing, but it means the setter is required to call, although a default
    // value may be requested (also explicitly). That default value may be changed
    // by the implementor and the caller won't be broken if it changes
    #[builder(required, default = "foo")]
    arg2: String,

    #[builder(required, default = Some("bar".to_owned()))]
    arg3: Option<String>
}

let example = Example::builder()
    // accepts an `Option<String>`. There is no `maybe_` method.
    .arg1(Some("value".to_owned()))
    // For this member a `maybe_` method was generated (because of `#[builder(default)]`).
    // However calling the setter is still required (because of `#[builder(required)]`).
    // If you want to set the default value, pass `None` explicitly.
    // Not calling any setter for `arg2` won't compile
    .maybe_arg2(None::<String>)
    // This works the same as with `arg3`, although there is `Option<Option<String>>`
    // accepted here. If `None` is passed the default value of `Some("bar".to_owned())`
    // is used. The method `.arg3()` accept a single-level `Option<String>` though
    .maybe_arg3(None)
    .build();

assert_eq!(example.arg1, "value");
assert_eq!(example.arg2, "foo");
assert_eq!(example.arg2.as_deref(), Some("bar"));

Additionally, the macro may generate default_{member}() methods such as .default_arg2() method to explicitly request the default value for the member. This avoids the need to pass a type annotation for None literal when it's passed into a function that accepts an Option<impl Into<String>>.

@musjj
Copy link

musjj commented Oct 10, 2024

I'm liking how the #[builder(required)] API looks.

My main use-case for bon is for enforcing immutability in certain structs (where all fields are private with builder+getter), rather for partially filling struct fields.

So something that would make it easy to make all fields mandatory during construction would be great!

@Veetaha
Copy link
Contributor

Veetaha commented Oct 10, 2024

Hi @musjj, I've already revisited this issue and design a bit on the background while working on #145.

To me, the feature of having exhaustiveness in the builder (where you need to fill all fields, even ones that have default values) looks quite complicated to even express and explain for the users. However, I'm not entirely giving up on this idea, and I'd be glad to have it if user feedback proves me wrong on this.

The main problem for me has been figuring out the proper naming for the different behaviors that I'd like to be configurable.

  • Opt-out from the special handling for members of type Option. This means two things:
    • The setter for this field becomes required to call.
    • There is just one setter generated for the field as if it was just a regular field: {member}(Option<T>)
  • Opt-out from only part of the special handling for members of type Option:
    • The setter for this field becomes required to call.
    • There still are two setters generated {member}(T) and maybe_{member}(Option<T>)

For the first behavior I'm thinking to return to the #[builder(transparent)] terminology. This tells the reader, that the field shouldn't be treated in any special way, and be a regular required field just like all fields are by default.

Then I'd like to reserve something like #[builder(explicit)] to mark fields for which the caller must invoke setters, even if these fields have default values or they are of Option<T> type.

So it would look like this:

#[derive(bon::Builder)]
struct Example {
    field1: Option<u32>,
    
    #[builder(transparent)]
    field2: Option<u32>,
    
    #[builder(explicit)]
    field3: Option<u32>
}

Example::builder()
    // `field1` has two setters generated, and it includes a `maybe_{}` setter. We could also omit
    // calling this setter.
    .maybe_field1(None)
    // `field2` has just one setter generated that accepts `Option<T>` and it's required to call
    .field2(None)
    // It works the same as `field1`, but with a difference that if you don't call any setter for
    // the `field3` the code won't compile
    .maybe_field3(None)
    .build()

I've already added support for #[builder(transparent)] in my yet-in-progress PR #145.

WDYT about this design @musjj?

So something that would make it easy to make all fields mandatory during construction would be great!

Is your use case for a #[builder(transparent)] or rather #[builder(explicit)]? I.e. would you still like to have two setters generated for the optional field?

Note that #[builder(explicit)] can also be combined with #[builder(default)]:

struct Example {
    #[builder(explicit, default = 42)]
    field1: u32
} 

Example::builder()
    // We can't omit calling a setter for `field1`. This requires the caller to 
    // explicitly state "yes, I acknowledge there is a `field1`" and I want to
    // use a default value for it
    .maybe_field1(None)
    .build()

Right now I'm not adding #[builder(explicit)] attribute yet (as I said, it's quite hard to explain and understand for the readers I think). However, if it better fits your use case, I can still add it. The problem is that this attribute would introduce a dimension into the previously two-dimensional optional/required behavior, where it would allow for optional-implicit and optional-explicit members.

With this complex behaviors having #[builder(required)] becomes confusing (the reader doesn't know if it means whether there still are a pair of {member}/maybe_{member} setters generated. It also becomes easy to confuse it with #[builder(explicit)], so I'm thinking of using #[builder(transparent)] naming instead to future-proof for a potential #[builder(explicit)] attribute in the future.

@musjj
Copy link

musjj commented Oct 10, 2024

For my personal use-case, I'm heavily preferring the #[builder(transparent)] method. Maybe you can wait for feedback from other users to see if there are demands for a #[builder(explicit)].

Btw, any chance of allowing the attribute to be used on the top level? So something like:

#[derive(bon::Builder)]
#[builder(transparent)] // All fields have to be explicitly constructed
struct Example {
    field1: Option<u32>,
    field2: Option<u32>,
    field3: Option<u32>
}

It would make it harder to forget to mark a field.

@Veetaha
Copy link
Contributor

Veetaha commented Oct 10, 2024

I see, thank you for the feedback! I'll just postpone the #[builder(explicit)] for now, and wait for someone with a use case for it.

Regarding the top-level config, it does make sense! I can make an extension of the #[builder(on(...))] syntax for this. So it would look like this if you want to blanket-apply transparent:

#[builder(on(_, transparent))]

With this you can also select members of specific types. For example, say you want to apply transparency only for members of type Option<u32>, but not touch members of Option<String> or Option<bool>:

#[builder(on(u32, transparent))]

Note that I plan to extend the syntax of the selector part of the on(selector, attrs...) attribute, and allow selecting members not only by their types, but for example on(prefix(foo_), ...) to select all members with a specific prefix, or other properties in the future.

With the bare #[builder(transparent)] syntax at the top-level the users may confuse this attribute to apply to the builder type itself (instead of individual fields), so on(...) wrapper makes it more clear that this config is inherited at field-level.

@adrian-budau
Copy link

Hello, I found myself in need of this feature with no workarounds, though I only really require the #[builder(explicit)] part.

I use protobuf for some between-service communication, and, at the moment I use prost with proto3 files, which for the sake of my explanation here is harder to change than adding a feature to bon.
All of my protobuf messages live in a separate crate so they can be used both between the server and the client(s).
To not break semver constantly, these protobuff rust structs are marked #[non_exhaustive] and use the builder pattern though bon.

Now proto3 has the unfortunate side-effect of having all fields optional, even if you mark them required, so there is no way to force every depender of a crate which exports proto3 messages to fill a field, and even though None is a valid value for a field, I'd rather the consumer of this type think before supplying None

proto3 file:

message Foo {
 // multiple fields
 // ...
 required Bar bar = 10;
}

in rust would be

[derive(Clone, Debug, PartialEq, Message, bon::Builder)]
#[non_exhaustive]
pub struct Foo {
  // the fields added here
  // ..
  bar: Option<Bar>, // notice this is not mandatory in bon, but it would be if I could use #[builder(explicit)]
}

I hope this counts as a valid use-case since this would greatly help me, thanks!

@Veetaha
Copy link
Contributor

Veetaha commented Oct 21, 2024

@adrian-budau

I see the problem, thank you for describing it. I have a followup question.

Are these types annotated with prost::Message written manually by you or are they generated by prost-build? If they are written manually by you, then you could force the callers to pass a non-Option value for such fields using the following combination of annotations:

#[derive(bon::Builder)]
pub struct Example {
    #[builder(transparent, with = |value: Bar| Some(value))]
    required: Option<Bar>,
}

Example::builder()
    .required(Bar {}) // The setter now accepts `Bar` directly and wraps with `Some` internally
    .build()

The attributes builder(transparent) and builder(with) aren't released yet, but they are available and working in the current master branch of bon, so you could try them out with:

[dependencies]
bon = { git = "https://github.com/elastio/bon", branch = "master" }

Alternatively, this could be solved with a (not implemented yet) #[builder(explicit)], and an additional parameter (also not implemented yet) to disable the maybe_* setter that accepts an Option. Smth like #[builder(explicit, setters(option_fn(disabled)))].


Instead, if you just want to make sure that developers fill all the fields of the protobuf message explicitly, I guess using a builder doesn't really buy you anything. Adding #[builder(explicit)] to all fields (including optional ones) practically defeats the purpose of #[non_exhaustive] , because it requires exhaustively specifying values for all fields. Using a struct literal in such case would be simpler

@adrian-budau
Copy link

adrian-budau commented Oct 21, 2024

I would only need this for some fields, not all, otherwise it would defeat the purpose of #[non_exhaustive] yes.

i guess the transparent feature would be great when I would like to force consumers to provide a value, but there are cases (and this is more common) where None is fine, I just want to force the user of the builder to provide something.
In a way, both features are useful to me.

I also find it weird that the maybe setters remain active in a universe with #[builder(explicit)] and require explicit setters(option_fn(disabled))

Later Edit: it seems I forgot to answer the first question, sorry. They are generated using a customprost-build so I have control over struct/field annotations.

@Veetaha
Copy link
Contributor

Veetaha commented Oct 21, 2024

I also find it weird that the maybe setters remain active in a universe with #[builder(explicit)] and require explicit setters(option_fn(disabled)).

This is why I think #[builder(explicit)] is not worth the cognitive overhead. Why I think maybe_ setter should still be available even when #[builder(explicit)] is set, is that explicit is not exactly "required", it's more of "exhaustive". The maybe_ setter is still there, the user just needs to call it explicitly if they want to set None or use the default value.


So I'd prefer if #[builder(transparent, with = ...)] worked for this use case instead, which I think is easier to understand.

They are generated using a custom prost-build so I have control over struct/field annotations.

Does it mean you would use field_attribute config of prost-build to specify which fields are required? That means you'd need to maintain that config separately from the .proto source files and enumerate all the required fields for all messages in your prost-build build.rs script.

A small ergonomic problem of #[builder(with = closure)] syntax in this use case is that it requires you to annotate the type of closure parameter.

I could make a special case of #[builder(with = Some)] work which would wrap the value with Some in the setter internally and infer the proper type of the setter parameter automatically.

Do you think this would work then for you and not require a #[builder(explicit)] feature? It would look like this in the generated code:

#[derive(bon::Builder)]
pub struct Example {
    #[builder(transparent, with = Some)]
    required: Option<Bar>,
}

@adrian-budau
Copy link

Thanks for your time, I misunderstood what transparent was doing.

#[builder(transparent)] solves 95% of my issues and #[builder(transparent, with = Some)] solves the last 5% (while it's absence will not really affect me that much). Thanks for your time, I'll just use it directly as soon as it's in a stable release of bon, so thank you!

@Veetaha
Copy link
Contributor

Veetaha commented Oct 25, 2024

Added an initial implementation of #[builder(on(_, transparent))]: #155. Merged into master. Pending documentation update in a separate PR and 3.0 release. 3.0 progress is tracked at #156

@Veetaha
Copy link
Contributor

Veetaha commented Oct 26, 2024

The PR #157 added support for #[builder(with = Some)]. So all the features discussed here are in master.

You may subscribe to the 3.0 checklist issue #156. I'll close it once I do a new release, the transparent and with = Some features will be in it.

@Veetaha
Copy link
Contributor

Veetaha commented Nov 8, 2024

Small update, I decided to do a last-minute change before the release of this feature as part of 3.0.

I'll use the term #[builder(required)] instead of #[builder(transparent)], because it's so obviously easier to read.

I believe my idea of #[builder(explicit)] doesn't actually have a real use case. Even if there is a real use case for it, I think I have ideas for other syntax people could use to achieve that, that wouldn't be so confusing.

@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

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

Successfully merging a pull request may close this issue.

4 participants