-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
Lower struct patterns and struct expressions with unnamed fields #121553
Conversation
@rustbot author |
@rustbot label F-unnamed_fields |
cc #121757, which I believe isn't currently fixed by these changes |
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.
Some first pass review comments
&'tcx ty::FieldDef, | ||
// The inner-most field type | ||
Ty<'tcx>, | ||
)> { |
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 should be a type.
match mentioned { | ||
Some((idx, _need_recheck @ true)) => { | ||
// Check the only mentioned field of the union again, | ||
// really collecting the unamed fields. |
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.
// really collecting the unamed fields. | |
// really collecting the unnamed fields. |
|
||
error: union patterns should have exactly one field | ||
--> $DIR/union-fields-2.rs:17:9 | ||
| | ||
LL | let U { a, b } = u; | ||
| ^^^^^^^^^^ | ||
| | ||
note: there are multiple fields from this union |
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.
note: there are multiple fields from this union | |
note: multiple fields from this union are matched but the union can only be one at a time |
Something a bit more verbose like this might be better.
@@ -50,6 +50,11 @@ pub struct TypeckResults<'tcx> { | |||
/// `_(2).field`. | |||
nested_fields: ItemLocalMap<Vec<(Ty<'tcx>, FieldIdx)>>, |
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.
Can field_maps
replace this?
return self.collect_unmentioned( | ||
variant.fields.iter().skip(idx.index()).take(usize::MAX), | ||
collect_unmentioned, | ||
) || has_mentioned; |
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.
I think the implementation of this check is a bit more complex than it needs to be I'm not sure why you need to go into collect_unmentioned
here rather than just add to unmentioned_fields
from this loop.
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.
Or rather, if there are unmentioned fields in one call to this function, then we could just queue up a UnionPatUnmentioned
like you do for the other errors.
// In case of the multiple fields from the union are used, | ||
// we do not collect those fields to report unmentioned errors. | ||
self.check_struct_variant(nested_adt_def.non_enum_variant(), false) | ||
.then_some((idx, /* need_recheck */ true)) |
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.
Similarly, I'm not sure that it's that big of a deal if we report an error for unmentioned fields from two unnamed struct types of an unnamed union (instead of just one of them). It may not be worth the additional complexity this adds.
☔ The latest upstream changes (presumably #122041) made this pull request unmergeable. Please resolve the merge conflicts. |
@frank-king any updates on this? thanks |
I'm sorry that I haven't got some time to work on this. I'm going to close it. |
This implements #49804.
Goals:
Non-Goals (will be in the next PRs)
r? @davidtwco