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

forward default typeparam #319

Merged
merged 2 commits into from
Aug 29, 2024
Merged

forward default typeparam #319

merged 2 commits into from
Aug 29, 2024

Conversation

tharvik
Copy link
Contributor

@tharvik tharvik commented May 9, 2024

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
Copy link
Collaborator

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
Copy link
Contributor Author

tharvik commented May 21, 2024

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
Copy link
Contributor Author

tharvik commented Aug 21, 2024

@TedDriggs any way to help in moving this along?

@TedDriggs
Copy link
Collaborator

@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
Copy link
Contributor Author

tharvik commented Aug 27, 2024

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
Copy link
Contributor Author

tharvik commented Aug 28, 2024

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 TedDriggs merged commit 6daff1e into colin-kiegel:master Aug 29, 2024
15 checks passed
@tharvik tharvik deleted the add-typeparam-default branch August 29, 2024 07:38
@TedDriggs
Copy link
Collaborator

This is published in v0.20.1

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

Successfully merging this pull request may close these issues.

2 participants