-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Do not suggest to derive Default
on generics with implicit arguments
#10399
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Conversation
r? @Manishearth (rustbot has picked a reviewer for you, use r? to override) |
Default
on generics with implicit arguments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good, needs a rebase
} | ||
if let Some(PathSegment { args, .. }) = p.segments.last() { | ||
let args = args.map(|a| a.args).unwrap_or(&[]); | ||
if substs.len() != args.len() || args.iter().any(|arg| !matches!(arg, GenericArg::Lifetime(_))) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: comment explaining what this is checking (and why checking against substs
solves the problem)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, will do while rebasing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This hasn't addressed my parenthetical:
Please make it clear why substs.len() != args.len()
is sufficient to check that if there are defaults involved, so someone reading this code without a clear idea of substs
can still understand
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hopefully this is better now.
☔ The latest upstream changes (presumably #10401) made this pull request unmergeable. Please resolve the merge conflicts. |
825bd23
to
50b5f33
Compare
Done. |
8a08279
to
f7042b5
Compare
f7042b5
to
c82ff00
Compare
(noticed a typo in the explanation and fixed it) |
@bors r+ thanks for working on this! |
☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test |
1 similar comment
☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test |
Fixes #10396
changelog: FP: [
derivable_impls
]: do not suggest to deriveDefault
on generics with implicit arguments