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

Default for field if generic type implements trait (without specialization) #51

Open
kitlith opened this issue Apr 12, 2021 · 8 comments

Comments

@kitlith
Copy link

kitlith commented Apr 12, 2021

the idea is you have something like

use typed_builder::TypedBuilder;

#[derive(TypedBuilder)]
struct HasGeneric<T> {
    x: i32,
    #[builder(default_generic(T: Default, Default::default()))]
    y: T
}

which effectively replaces some of the fake build implementations for error messages with ones that actually work,
but are bounded on the trait that was passed to default_generic. default_generic without any arguments can probably desugar to exactly what's in the example

i'll write out a sample of what i'd expect it to output at some point, but not now.

@kitlith
Copy link
Author

kitlith commented Apr 13, 2021

concrete codegen for the above example

#[doc(hidden)]
#[allow(dead_code, non_camel_case_types, missing_docs, clippy::panic)]
impl<T> HasGenericBuilder<((i32,), ()), T> {
    pub fn build(self) -> HasGeneric<T> where T: Default {
        let (x, y) = self.fields;
        let x = x.0;
        let y = Default::default();
        HasGeneric { x, y }
        // or maybe possibly
        // self.y(Default::default()).build()
    }
}

@idanarye
Copy link
Owner

The problem is that TypedBuilder is not implementing a build() for HasGenericBuilder<((i32,), ()), T>. That would require an implementation for every one of the 2^N possibilities (where N is the number of non-mandatory fields). Instead, TypedBuilder generates a build method for HasGenericBuilder<((i32,), __y), T> where __y: HasGenericBuilder_Optional (yea, the names are complex...) which can handle both the (T,) case (value provided) and the () case (use default value). But both cases require the build method to provide a closure - which needs to be valid even for the (T,), case, even though that case ignores it. But in this case the closure would only be valid if T: Default - which means for defaultless T's we won't be able to build HasGeneric even if we set a value for y.

@kitlith
Copy link
Author

kitlith commented Apr 13, 2021

perhaps i need to take a closer look again, i think i completely missed the closures? but the whole idea was to do this in a way that wouldn't interfere with the other optionals, and thus avoid requiring specialization.

it's worth noting that i originally came up with this design via a reimplementation that someone made, designed based on this library because they didn't want to generate usage of macros inside of their proc-macro (sigh, i've somewhat given up on convincing them otherwise) and it's a simple translation of said design, but if i missed a few details in the process, that's on me, my apologies.

To explain my design better, the default_generic was always meant to provide optionals in a different way than the current way to handle optionals, exactly so that you don't run into being able to work only if the trait is implemented regardless of whether a value is provided or not. my design has been requiring creating 2^N-1 extra implementations of build, though in this case N is "only" the number of usages of "generic_default", which is only to be used when you can't use a regular default, and the idea is to discourage more than one use in a struct. (you could make that a hard limit, but meh.) it also looks like you are (sometimes?) generating build() for these cases anyway, it's just that they are error cases with a deprecation notice and extra argument preventing it from compiling. My "concrete codegen" above was just taking one of these error cases and modifying it so it actually worked, in theory, but i apologize for not getting around to actually testing it.

i'm gonna take another look and play around more thoroughly in the context of this library. Even if it works, you may not like the design, and so you are free to close the issue.

@idanarye
Copy link
Owner

2^N is not something one should bring up so casually. Learn to respect the exponential. Respect - and fear. Succumbing to it would be so easy - being able to generate the code for each case instead of generating a complex composition of generic impls. But then you try making a struct with 10 fields and everything explodes...

The extra build methods for error reports are not exponential. Each one is covering all the cases where all the previous fields either have a default or their setter was called, so the result is linear. The code size is actually O(N^2) because each implementation must address all the fields, but this is still something the computer can manage.

I do need to see if I can change the architecture to be based around const generics and construct everything around traits. A trait can have a const generic index that's the index of the parameter, so we might be able to generate something like:

impl<T> CanBuild<1, (T,)> for HasGeneric<T> {
    fn make_value((value,): T) -> T {
        value
    }
}

impl<T> CanBuild<1, ()> for HasGeneric<T>
where
    T: Default,
{
    fn make_value((): T) -> T {
        T::default()
    }
}

And then:

impl<(i32,), F1, T> HasGenericBuilder<T>
where
    HasGeneric<T>: CanBuild<1, F1>,
{
    // ...
}

Generally speaking, using traits with const generics should allow me to create more complex constraints. It may require a big rewrite though...

@kitlith
Copy link
Author

kitlith commented Apr 13, 2021

2^N is not something one should bring up so casually. Learn to respect the exponential. Respect - and fear. Succumbing to it would be so easy - being able to generate the code for each case instead of generating a complex composition of generic impls. But then you try making a struct with 10 fields and everything explodes...

I agree, which is why i'm not touching the existing optionals at all. I only suggested it because i hadn't found any other way to do it yet. 2^N is acceptable when N is small (and only when N is small) and in my use case it would only be used sparingly. (N = 1, maybe 2 at most.) But I definitely get how it would be concerning from a general pov.

The extra build methods for error reports are not exponential.
D'oh, my bad. hmmm. i should check to see if i can make a more linear solution work without running into overlapping impls, but i believe that said linear solution basically requires overlapping impls.

if you have a struct with two fields that both require trait bounds in this manner in order to default, when trying to do it linearly you'd have something like ((), HasGenericBuilder_Optional) where A: Default which provides A and then calls build... but actually it doesn't know whether it can provide B, so this is a non-starter, and you'd have to fall back to individual cases, nevermind that trying to provide two of these cases would then result in conflicting impls for ((), ()), causing you to once again have to fall back.

I do need to see if I can change the architecture to be based around const generics and construct everything around traits. A trait can have a const generic index that's the index of the parameter, so we might be able to generate something like:

impl<T> CanBuild<1, (T,)> for HasGeneric<T> {
    fn make_value((value,): T) -> T {
        value
    }
}

impl<T> CanBuild<1, ()> for HasGeneric<T>
where
    T: Default,
{
    fn make_value((): T) -> T {
        T::default()
    }
}

And then:

impl<(i32,), F1, T> HasGenericBuilder<T>
where
    HasGeneric<T>: CanBuild<1, F1>,
{
    // ...
}

Generally speaking, using traits with const generics should allow me to create more complex constraints. It may require a big rewrite though...

fuck, I think that could work. you can probably also get away without needing const generics (think generating a unique type for each field, e.g. enum HasGenericBuilder_Field_x {}) if you wanted to avoid increasing MSRV, but just using const generics may be easier to prototype with.

i wish i could contribute more to that design, but i think that's pretty fleshed out. i need to try and remember stuff like that next time i run into potential issues with overlapping impls.

@idanarye
Copy link
Owner

Yea, making this crate thought me a lot about traits - and there is still much I don't know. Rust can be like that sometimes...

@rakshith-ravi
Copy link

Hi @idanarye - This is something that will ever be possible on typed builder? I'd be happy to put a PR to add this if you can guide me on how to implement it

@idanarye
Copy link
Owner

idanarye commented Sep 3, 2023

@rakshith-ravi This will require some big changes. I've managed to make something like what I envisioned sort of work:

use crate::some_module::Foo;

mod trait_to_put_in_typed_builder {
    pub trait CanBuild<const INDEX: usize> {
        type Input;
        type Output;

        fn generate(input: Self::Input) -> Self::Output;
    }
}

mod some_module {
    use crate::trait_to_put_in_typed_builder::*;
    use typed_builder::TypedBuilder;

    #[derive(TypedBuilder, Debug)]
    pub struct Foo<T> {
        pub bar: i32,
        pub baz: T,
    }

    impl<T, Bar, Baz> FooBuilder<T, (Bar, Baz)>
    where
        Self: CanBuild<0, Input = Bar, Output = i32> + CanBuild<1, Input = Baz, Output = T>,
    {
        pub fn build2(self) -> Foo<T> {
            Foo {
                bar: <Self as CanBuild<0>>::generate(self.fields.0),
                baz: <Self as CanBuild<1>>::generate(self.fields.1),
            }
        }
    }

    impl<T, Baz> CanBuild<0> for FooBuilder<T, ((i32,), Baz)> {
        type Input = (i32,);

        type Output = i32;

        fn generate(input: Self::Input) -> Self::Output {
            input.0
        }
    }

    impl<T, Bar> CanBuild<1> for FooBuilder<T, (Bar, (T,))> {
        type Input = (T,);

        type Output = T;

        fn generate(input: Self::Input) -> Self::Output {
            input.0
        }
    }

    // This is where the `Default` magic happens
    impl<T, Bar> CanBuild<1> for FooBuilder<T, (Bar, ())>
    where
        T: Default,
    {
        type Input = ();

        type Output = T;

        fn generate(_input: Self::Input) -> Self::Output {
            Default::default()
        }
    }
}

fn main() {
    println!("{:?}", Foo::builder().bar(1).baz(2.0).build2());
    println!("{:?}", Foo::<f32>::builder().bar(1).build2());
}

But there are two problems with this design:

  1. It prevents default = ... expressions from using the values of other fields.
  2. There is no place to put a fake build method for the compiler errors.

The first problem, I think it would be safe to ignore. This feature was never documented anyway. If we do want to add it, we can always make CanBuild::generate accept (references to) the other fields as a context argument (this should also satisfy #75)

The second problem can be solved by making the build declare its generic bounds the old way but use CanBuild instead of Optional.

The solutions to both problems brings us away from the clean, easily extensible, traits-based API I'm dreaming of...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants