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

#[builder(default = ...)] for generic field got error "expected type parameter, found concrete type" #44

Open
cshuaimin opened this issue Jan 15, 2021 · 9 comments · May be fixed by #73

Comments

@cshuaimin
Copy link
Contributor

#[derive(TypedBuilder)]
struct Foo<T> {
    #[builder(default = 12)]
    x: T,
}
error[E0308]: mismatched types
  --> tests/tests.rs:60:29
   |
59 |     struct Foo<T> {
   |                - this type parameter
60 |         #[builder(default = 12)]
   |                             ^^ expected type parameter `T`, found integer
   |
   = note: expected type parameter `T`
                        found type `{integer}`
@idanarye
Copy link
Owner

This is the correct behavior. The default value 12 only works for integers, but T can be anything (excluding unsized types, of course). If that would work, Foo::<String>::builder()::build() for example would have tried to put 12 into a string value - which is not allowed.

@cshuaimin
Copy link
Contributor Author

Thanks for your reply!

I agree that Foo::<String>::builder()::build() should be rejected, because Foo::<String> { x: 12 } is rejected for the same type error.

Let's think about Foo { x: 12 }. In this example we let the compiler infer the type parameter T from the field value we give, then finally we get a Foo<i32> struct. Similarly by Foo::builder().x(12).build() we get a Foo<i32> too.

By using #[builder(default = ...)] what I exactly mean is "the user of Foo don't have an opinion about the field x, so let me give it a default value". I think "a default value" also implies a default type. What I expect is that the compiler infers the type of x from the default value and fill the type paramater of final struct with that inferred type.

In other words, by giving a generic field a default value, then I hope the macro can transform Foo::builder().build() to Foo { x: 12 }.

Currently what is going wrong is the direction of type inference: we get T from Foo::builder(), and then in turn constrain the default value is also a T. I believe this is not ideal.

@cshuaimin
Copy link
Contributor Author

The TL;DR is, code

// Compiling error
#[derive(TypedBuilder)]
struct Foo<T> {
    #[builder(default = 12)]
    x: T,
}
Foo::builder().build()

should equal to

// OK
#[derive(TypedBuilder)]
struct Foo<T> {
    x: T,
}

Foo::builder().x(12).build()

@idanarye
Copy link
Owner

So Foo::builder().build() should infer the generic parameter from the fact no value was given for x and its default value is an integer?

I'm not sure this is even possible in Rust. The TypedBuilder custom derive macro would have to generate a type constraint that 12 can be of type T - a facility that Rust does not currently have - in order to constrain the build() method with it.

And even if Rust had such a facility - best I could do is support a default that only works is the generic parameter is already inferred to be an integer by something else. Maybe Rust's type inference is powerful enough to take you the rest of the way - but I seriously doubt it.

Finally - this feels like breaking the Rust rule that the information about the types should be in the signatures, not in the implementation body. And yes, you can argue that the attributes are part of the signature, but #[builder(default)] acts as an implementing code generator and it feels wrong to have it affect the signature...

@cshuaimin
Copy link
Contributor Author

I think you are right. I tried to implement Default for Foo<T>, which failed when I return a Foo<i32> in fn default() -> Foo<T>
.

But I can implement Default for Foo<i32>, and that works well:

struct Foo<T> {
    x: T,
}

impl Default for Foo<i32> {
    fn default() -> Self {
        Foo { x: 12 }
    }
}

Foo::default()

support a default that only works is the generic parameter is already inferred to be an integer by something else

So I'm thinking If we can let the default attribute take an additional argument which is the type of the generic field. This is something like "implement Default for Foo<i32>. I edited the output of cargo expand and get this:

#[derive(Debug)]
struct Foo<X> {
    x: X,
}
impl<X> Foo<X> {
    #[allow(dead_code)]
    fn builder() -> FooBuilder<((),), X> {
        FooBuilder {
            fields: ((),),
            _phantom: core::default::Default::default(),
        }
    }
}
#[must_use]
#[doc(hidden)]
#[allow(dead_code, non_camel_case_types, non_snake_case)]
struct FooBuilder<TypedBuilderFields, X> {
    fields: TypedBuilderFields,
    _phantom: core::marker::PhantomData<X>,
}
#[doc(hidden)]
#[allow(dead_code, non_camel_case_types, non_snake_case)]
pub trait FooBuilder_Optional_x<T> {
    fn into_value(self) -> T;
}
impl FooBuilder_Optional_x<i32> for () {
    fn into_value(self) -> i32 {
        12 // I edited the trait to return default value directly from the impl
    }
}
impl<T> FooBuilder_Optional_x<T> for (T,) {
    fn into_value(self) -> T {
        self.0
    }
}
#[allow(dead_code, non_camel_case_types, missing_docs)]
impl<X> FooBuilder<((),), X> {
    pub fn x(self, x: X) -> FooBuilder<((X,),), X> {
        let x = (x,);
        let (_,) = self.fields;
        FooBuilder {
            fields: (x,),
            _phantom: self._phantom,
        }
    }
}
#[doc(hidden)]
#[allow(dead_code, non_camel_case_types, non_snake_case)]
pub enum FooBuilder_Error_Repeated_field_x {}
#[doc(hidden)]
#[allow(dead_code, non_camel_case_types, missing_docs)]
impl<X> FooBuilder<((X,),), X> {
    #[deprecated(note = "Repeated field x")]
    pub fn x(self, _: FooBuilder_Error_Repeated_field_x) -> FooBuilder<((X,),), X> {
        self
    }
}
#[allow(dead_code, non_camel_case_types, missing_docs)]
impl<X, __x: FooBuilder_Optional_x<X>> FooBuilder<(__x,), X> {
    pub fn build(self) -> Foo<X> {
        let (x,) = self.fields;
        let x = FooBuilder_Optional_x::into_value(x);
        Foo { x }
    }
}

fn main() {
    dbg!(Foo::builder().build()); // works, x = 12
    dbg!(Foo::builder().x("hello").build()); // works, x = "hello"
    dbg!(Foo::<String>::builder().build()); // error[E0599]: no method named `build` found for struct `FooBuilder<((),), String>` in the current scope
}

That works well except for the error message, and my initial try found that for better error message I will get "duplicate definition for build" error. So I'm stuck here, and maybe I should just use trait objects instead...

@cshuaimin
Copy link
Contributor Author

I found a workaround for my use case.

What I'm doing is storing an optional callback in a struct:

#[derive(TypedBuilder)]
struct Foo<F>
where
    F: FnMut(),
{
    #[builder(default = None)]
    f: Option<F>,
}

The code above does compile well. (It's weird that default = None works but default = 12 doesn't.)

To build the struct, I need to specify the type: Foo::<fn()>::builder().build().

@idanarye you can close this if you think it's impossible to make the initial code work.

@idanarye
Copy link
Owner

Oh... your type is a closure. Suddenly your problem makes much more sense...

Your solution will not work. Yes, it will compile, and yes, you can build Foo if you pass a closure - but not you won't be able to build using f's default value. The reason is that even if the value of f is None, Rust still need to infer a concrete type for F in order to create an instance of Foo.

Frankly - I'm not even sure if what you want can be done in Rust...

If you are willing to take a slight performance hit, you can box a dynamic closure:

#[derive(typed_builder::TypedBuilder)]
struct Foo
{
    #[builder(default = Box::new(|| println!("default f")))]
    f: Box<dyn FnMut()>,
}

fn main() {
    (Foo::builder().build().f)();
    (Foo::builder().f(Box::new(|| println!("nondefault f"))).build().f)();
}

I can even throw in a strip_box attribute for removing some of that boilerplate.

@cshuaimin
Copy link
Contributor Author

Yes I'm going to use boxed closure 😄

I just found that the default value of type parameter may help here, just like std::lazy::Lazy:

pub struct Lazy<T, F = fn() -> T> { ... }

And it's Default implementation at the end of that file.

Currently, in the generated FooBuilder, typed-builder seems to ignore the default value.


Just for more context: I'm writing a crate which brings Flutter's layout style to TUI world, and there's a proc-macro build! which transforms struct literal syntax to builder pattern. For example the code

build! {
    Padding {
        padding: EdgeInsets { left: 2, },
        child: Text {
            text: "Hello world!",
        },
    }
}

expands to

Padding::builder()
    .padding(EdgeInsets::builder().left(2).build())
    .child(Text::builder().text("Hello world!\n").build())
    .build()

In my crate I store the event handler callback in a struct.

@ZhennanWu
Copy link

Additional comment here. This builder pattern is actually in use in very popular Rust libraries. For example, in the rayon library:

https://github.com/rayon-rs/rayon/blob/3e3962cb8f7b50773bcc360b48a7a674a53a2c77/rayon-core/src/lib.rs#L443-L460

The default builder use a ThreadPoolBuilder<DefaultSpawn>. When the user supply it with a closure type, it became ThreadPoolBuilder<CustomSpawn> to store that closure. In order to achieve this, rayon's ThreadPoolBuilder uses moving self signature all over the place, but this pattern would be very compatible with a type-based builder macro.

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