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

Support concrete default for generic/impl Trait argument. #168

Open
zakstucke opened this issue Oct 29, 2024 · 10 comments
Open

Support concrete default for generic/impl Trait argument. #168

zakstucke opened this issue Oct 29, 2024 · 10 comments
Labels
feature request A new feature is requested

Comments

@zakstucke
Copy link

zakstucke commented Oct 29, 2024

Great library!

It would be really powerful to provide concrete defaults for generic parameters, the below example fails to compile:

#[builder]
fn foo(#[builder(default = std::iter::empty())] arg: impl IntoIterator<Item = String>) {
    drop(arg);
}
213 |     #[builder]
    |     ---------- expected this type parameter
214 |     fn foo(#[builder(default = std::iter::empty())] arg: impl IntoIterator<Item = String>) {
    |                                ^^^^^^^^^^^^^^^^^^ expected type parameter `I1`, found `Empty<_>`

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 Oct 29, 2024
@Veetaha
Copy link
Contributor

Veetaha commented Oct 29, 2024

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 #[builder(default)] with generic parameters (including impl Trait) is an antipattern due to the current design of "eager generic parameters". I'm planing to document that antipattern for the next release.

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 impl Default parameter:

type annotations needed
cannot satisfy `_: std::default::Default`

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 #[builder(with = ...)] annotation allows you to move the generic parameters to the setter, removing it from the builder struct. So the setter still accepts impl IntoIterator<Item = String>, but in the function you already receive a concrete collection type.

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 bon from master (not released yet). Is there any specific feature that is currently unreleased that you are using it for?

@zakstucke
Copy link
Author

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 bon it might be possible 😄

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!

@Veetaha
Copy link
Contributor

Veetaha commented Oct 29, 2024

I see. Well, another way to work around this is to wrap the value in Option<> and defaulting internally:

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 arg.unwrap_or_else(std::iter::empty) because the types of the arg and the return value of std::iter::emtpy are different.

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)

@zakstucke
Copy link
Author

zakstucke commented Oct 29, 2024

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.
However I don't claim to know the internals or that this would work!

Feel free to close this issue if you're sure it won't be possible to action :)

@Veetaha
Copy link
Contributor

Veetaha commented Oct 29, 2024

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 syn accepts default values for generic types in function position like that, so this may be actually workable with this syntax where the user has to use the verbose named generic type parameter syntax, and has to specify the default value for the generic type for which the default expression is written.

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 arg. For example, when there are many generic parameters, the macro needs to only add generics that are used in the type of the arg and ignore all the rest. This requires traversing the type tree to find the generic type occurences in the type expression.

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 T::Assoc, where ... clauses and other type expressions.

@Veetaha
Copy link
Contributor

Veetaha commented Oct 29, 2024

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 default_expression to T. Maybe this can be achievable with some unsafe transmute or some other cool type system trick.. Need to think about this more

@zakstucke
Copy link
Author

zakstucke commented Oct 29, 2024

How should the finishing function be written?

By not having self.arg as Option<T> but instead T, or FnOnce(T) for performance. The value shouldn't be set to default at the end, but at the beginning and overidden if needed with this approach.

@Veetaha
Copy link
Contributor

Veetaha commented Oct 29, 2024

The value shouldn't be set to default at the end, but at the beginning

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:

You can also use the values of other members by referencing their names in the default expression. All members are initialized in the order of their declaration. It means only those members that are declared earlier (higher) in the code are available to the default expression.

Storing FnOnce(T) in the builder's fields is not possible because how would you then set an override for the value? Unless it's an enum { Set(T), Unset(F) }. Also FnOnce requires yet another generic parameter (F) on the builder's struct for the type of the function itself. Using fn() -> T pointer sacrifices performance as it adds dynamic dispatch.

@Veetaha
Copy link
Contributor

Veetaha commented Oct 29, 2024

I could make it work like this with a transmute (although it doesn't compile because the names of internal builder fields are generated with random words in them to ensure their privacy, but that wouldn't be a problem if this code was generated instead).

#[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 maybe_ version of the setter that accepts an Option<T>. I don't see how it could be written:

let _: Builder<??, _> = foo().maybe_arg(Some(["str".to_owned()]));

What would be the type ??? Is it [1; String] or is it std::iter::Empty<String>? The problem is that the type of that generic parameter strictly depends on whether a Some or None was passed. So such setter wouldn't be possible to write

@Veetaha
Copy link
Contributor

Veetaha commented Oct 29, 2024

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

@Veetaha Veetaha changed the title Support concrete default for impl argument. Support concrete default for generic/impl Trait argument. 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

2 participants