-
-
Notifications
You must be signed in to change notification settings - Fork 22
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
Support concrete default for generic/impl Trait
argument.
#168
Comments
Hi, thank you for creating the issue! Unfortunately, this isn't possible with the current design of the builder struct's type signature. The builder struct captures the generic parameters eagerly. In this case the generated builder struct looks like this: struct FooBuilder<I1: IntoIterator<Item = String>> {
// ...
} There is a cool article that explores the design where generic parameters are resolved lazily, but that approach is a huge rabbit hole of complexity and probably will lead to unacceptable compile times. Additionally, I think using In short the problem is the following: #[bon::builder]
fn example(#[builder(default)] value: impl Default) {}
let _ = example().call(); This code doesn't compile with the error because the compiler can't infer the type of the omitted
Instead, once the next version of bon (#156) is released, you could write this code like this: #[bon::builder]
fn foo(
#[builder(default = vec![], with = FromIterator::from_iter)]
arg: Vec<String>
) {
drop(arg);
}
let _ = foo().call(); The new Let me know if this makes sense to you and you understand the footguns with the generic types inference here. I'll need to document this and want to make sure the explanation is clear. Btw. by the error message you posted I can tell that you are using the version of |
Thanks for the detailed explanation! I can't say i'm surprised, there's a reason I've never seen a solution to this before! It just felt with No specific feature, I just went on master to look around the code and see if adding this was something easy to PR, until realising it was a rabbit hole! Your workaround doesn't work for these 2 examples I was just migrating to bon I don't think: trait MyTrait;
impl MyTrait for ();
fn foo(arg: impl MyTrait) {}
foo(()) fn bar(cb: impl Fn()) {}
bar(|| {}) Both are sometimes real values, mostly noops. This is how they were written pre-bon, and with bon still need manually specifying, but I was hoping I could avoid! |
I see. Well, another way to work around this is to wrap the value in fn foo(arg: Option<impl MyTrait>) {
if let Some(arg) = arg {
// ...
} else {
// ...
}
} In this case you can actually "feel" the problem because you can't do something like Although... there is still a problem with infering the generic type if you don't pass a value to a setter, so this doesn't compile: foo().call(); instead you'd need to write this: foo::<TraitImplType>().call() Unfortunatelly, this is the limitation of the language where you can't assign default values for generic type parameters in functions (although that's possible to do with generic struct/enum/union) |
Yep it's not possible to solve without bon/builders. I was imagining with bon it could work like: trait MyTrait;
impl MyTrait for ();
impl MyTrait for usize;
#[builder]
fn foo(#[builder(default = ())] arg: impl MyTrait) {}
foo() // Builder<()>
.call() // foo::<()>
foo() // Builder<()>
.arg(10) // Builder<usize>
.call() // foo::<usize> so not actually "lazy", but pre-determined until overridden. Feel free to close this issue if you're sure it won't be possible to action :) |
Interesting idea... Although in general case, and after desugaring this should work with named generic types and the user needs to clarify the exact type of the default value: #[builder]
fn foo<T: MyTrait = ()>(#[builder(default = ())] arg: T) {
} I think The tough part is that the setter needs to be generated like this: impl<T, State> FooBuilder<T, State> {
fn arg<U>(value: U) -> FooBuilder<U, SetArg<State>> { /**/ }
} This is where the macro needs to figure out the proper generics for the This approach is definitely workable in simple cases, but needs some more prior design to prove that it can work in a general case with complex |
A problem with this approach.. How should the finishing function be written? impl<T, State> FooBuilder<T, State> {
fn call(self) {
let arg: T = self.arg.unwrap_or_else(|| default_expression);
__orig_foo(arg)
}
} This suffers from the same problem where we need to cast |
By not having |
That's unacceptable. Imagine the default value performs some complex computation or allocation. Calculating it eagerly would require paying the cost of that computation even if the default value is overridden. Also, the default expression today guarantees access to already set members in scope. From the docs:
Storing |
I could make it work like this with a #[bon::builder(builder_type = FooBuilder, finish_fn(vis = ""))]
fn foo_internal(
#[builder(setters(name = _unused, vis = ""))] arg: impl IntoIterator<Item = String>
) {}
fn foo() -> FooBuilder<std::iter::Empty<String>> {
foo_internal()
}
impl<T: IntoIterator<Item = String>, State: foo_builder::State> FooBuilder<T, State> {
fn arg<U: IntoIterator<Item = String>>(self, value: U) -> FooBuilder<U, State>
where
State::Arg: foo_builder::IsUnset,
{
FooBuilder {
__private_twilight_phantom: std::marker::PhantomData,
__private_twilight_named_members: (Some(value),),
}
}
fn call(self) {
let arg: T = self.__private_twilight_named_members.0.unwrap_or_else(|| {
let default: std::iter::Empty<String> = std::iter::empty();
unsafe { std::mem::transmute(default) }
});
__orig_foo_internal(arg)
}
}
fn main() {
foo().call();
foo().arg(["foo".to_owned()]).call();
} But then I completely forgot about the let _: Builder<??, _> = foo().maybe_arg(Some(["str".to_owned()])); What would be the type |
There is a lot to research and experiment with here. I don't see an obvious design that would fit into the existing model without compromising some existing features, but maybe there is such a design that satisfies all requirements. I'd leave this issue open until a comprehensive design appears that takes into account all the edge cases. The first thing to do is to have a code snippet of the generated code like I did above that makes it obvious how generation of such code could be automated in a macro and how it would interact with other existing configs of the macro and handle all kinds of possible syntax. I'll try to think about this more after I close the current higher priority items |
impl
argument.impl Trait
argument.
Great library!
It would be really powerful to provide concrete defaults for generic parameters, the below example fails to compile:
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.
The text was updated successfully, but these errors were encountered: