colin-kiegel / rust-derive-builder

derive builder implementation for rust structs
https://colin-kiegel.github.io/rust-derive-builder/
Apache License 2.0
1.32k stars 88 forks source link

forward default typeparam #319

Closed tharvik closed 2 months ago

tharvik commented 6 months ago

the derived builder doesn't offer the same default types as the source struct. that forces the user to explicit all types when theses can't be infered.

in the case I have, I want to derive a builder on a struct with a bunch of callbacks:

pub struct Settings<T, S = fn(T), R = fn(T), C = fn(T)>
where S: FnOnce(T), R: FnMut(T), C: FnMut(T)

on which I only need to specific T when instanciating Settings "by hand". but when using the derived builder, I need to specifiy all the types (which is especially cumbersome as most of theses callbacks are optional).

there was a confusion in the implementation between generics meant for the builder struct (which can contain default types) and the ones for impl (which can't contain theses). this PR fixes that by directly using the generics specified on the struct (with bounds and defaults), and a test to guard against regression for this kinda obscure feature.

I encountered dtolnay/syn#782 while doing that but as it seems that upstream seems to have forgot this issue, a workaround seemed better.

TedDriggs commented 5 months ago

When I last tried this in #282, I was surprised to find that it didn't work the way I'd predicted. Does the test case from that conversation work with this PR?

tharvik commented 5 months ago

When I last tried this in #282, I was surprised to find that it didn't work the way I'd predicted. Does the test case from that conversation work with this PR?

No, it doesn't, but that's a way larger issue on how default type params are handled. It won't ~work as predicted until upstream addresses it: one still needs to specify some generic params. but that reduces the number of needed params from my use case from four to one which is already a step in the right direction.

Note that slightly modified version of your test case would work but is quite weird to use

#[test]
fn generic_builder_with_defaults() {
    let x: GenericWithDefaultBuilder = Default::default();
    let y = x.build().unwrap();

    assert_eq!(y, GenericWithDefault { lorem: None });
}
tharvik commented 2 months ago

@TedDriggs any way to help in moving this along?

TedDriggs commented 2 months ago

@tharvik I missed your previous reply. Thanks for doing the research into the upstream issue here. My one ask would be that you add a test to derive_builder/tests/run-pass which exercises your use-case, so we have an end-to-end demonstration of it working. Once that's added, we can merge this and I'll ship a new version of the crate.

tharvik commented 2 months ago

great then, thanks for taking the time of handling this! I added a small use-case at derive_builder/tests/run-pass/default_typeparams.rs which tests most features that I need (my actual use-case is way larger yet works flawlessly with this patch).

tharvik commented 2 months ago

soo, stable rust got updated to 1.80 since last CI build, which introduces a new lint and changed the name of one. latest commit takes care of that, a bit out of scope IMO, feel free to drop it.

TedDriggs commented 2 months ago

This is published in v0.20.1