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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tharvik
Copy link

@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
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 });
}

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.

None yet

2 participants