Parse unnamed struct and union fields #99754
Conversation
rustbot
added
the
T-compiler
|
(rust-highfive has picked a reviewer for you, use r? to override) |
|
Some changes occurred in src/tools/rustfmt cc @rust-lang/rustfmt |
rust-highfive
added
the
S-waiting-on-review
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Review only applicable to the rustfmt subset of the diff
Thanks for including an implementation, but it's actually not necessary to do so and generally our preference in these new cases to leave the formatting untouched initially. We'll probably end up with something fairly similar on the rustfmt side, but we'll take care of that a little later on.
Inline comment left below on the one change requested for rustfmt, then believe the rustfmt changes in the other files could be reverted
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Sorry for taking so long to get to this, not sure if I have enough context to review this. r? compiler |
compiler/rustc_feature/src/active.rs
Outdated
| @@ -521,6 +521,8 @@ declare_features! ( | |||
| /// Allows creation of instances of a struct by moving fields that have | |||
| /// not changed from prior instances of the same struct (RFC #2528) | |||
| (active, type_changing_struct_update, "1.58.0", Some(86555), None), | |||
| /// Allows unnamed fields of struct and union type | |||
| (incomplete, unnamed_fields, "1.53.0", Some(49804), None), | |||
There was a problem hiding this comment.
The version number does not seem to be right..
|
I am not very familiar with ast code. r? compiler |
|
If I understand the RFC correctly, we are trying to allow unnamed fields in struct & unions with either named or unnamed struct/union type. So an unnamed struct/union can only happen inside a enum FieldDefKind {
Named(Ident, P<Ty>),
Unnamed(P<Ty>),
Nested(StructOrUnion, Vec<FieldDef>)
}This would probably remove a lot of the headaches about parsing ambiguities and having |
|
That approach is #84415, and a comment was made suggesting the other approach. #84415 (comment) Is the reasoning expressed in the comment still valid? |
There was a problem hiding this comment.
Thanks for the explanation @carbotaniuman. I left a few questions.
| } | ||
| } else { | ||
| self.parse_path_start_ty(lo, allow_plus, ty_generics)? | ||
| } |
There was a problem hiding this comment.
Why in this order? Since we only allow anonymous types for recovery, should we start by parsing the path, and recover by parsing an anonymous type?
There was a problem hiding this comment.
Ok, so this is the part of the change that I'm least sure of. In this code, because union can be an identifier, you have to expressly opt-in to parsing an anonymous union type. struct on the other hand is a keyword and will attempt to parse an anonymous struct type for recovery. This is really more of hack than anything proper.
This has some implications, such as impl struct {} having an error mentioning anonymous struct types (instead of expected type found keyword), but impl struct{} {} having a "proper" error. This may require recovery changes in the impl block parsing beyond what I have already done, but I'm not too sure on this.
There was a problem hiding this comment.
@Amanieu and I were reviewing this, and wondering if it would be possible to confine this entirely to field definitions, and even then only when the field name is _.
If the field name is _, then look for struct or union in the type. If the type is struct then it must continue on with {. If the type is union, it could be union; or union::something..., either of which would be the other case of unnamed fields (where you have a named type), but if it isn't the start of a path then it has to continue with {.
Either way, we're wondering if this can only be parsed in field types, and nowhere else, avoiding the need to handle it (for recovery or otherwise) in other locations a type can appear.
| @@ -456,6 +456,9 @@ pub fn walk_ty<'a, V: Visitor<'a>>(visitor: &mut V, typ: &'a Ty) { | |||
| TyKind::Typeof(ref expression) => visitor.visit_anon_const(expression), | |||
| TyKind::Infer | TyKind::ImplicitSelf | TyKind::Err => {} | |||
| TyKind::MacCall(ref mac) => visitor.visit_mac_call(mac), | |||
| TyKind::AnonymousStruct(ref fields, ..) | TyKind::AnonymousUnion(ref fields, ..) => { | |||
There was a problem hiding this comment.
Please explicitly ignore fields in visitors, so we have a compiler error when we change them.
| TyKind::AnonymousStruct(ref fields, ..) | TyKind::AnonymousUnion(ref fields, ..) => { | |
| TyKind::AnonymousStruct(ref fields, _recovered) | TyKind::AnonymousUnion(ref fields, _recovered) => { |
| let (mut impl_items, err) = | ||
| self.parse_item_list(attrs, |p| p.parse_impl_item(ForceCollect::No))?; | ||
|
|
||
| if let Some(mut err) = err { |
There was a problem hiding this comment.
Could you add a comment explaining what happens here?
There was a problem hiding this comment.
Uhh this is with the other comment that I'm not too sure if this is the right way to do things, but basically I'm trying to parse an impl block, if that fails I try and parse an anonymous struct or union type and then parse an impl block. (Really I would need to do it three times to recover form impl union {}, but again I'm not even sure this is the right path.
There was a problem hiding this comment.
Is the error message really bad if you do not change the code for parse_item_list?
There was a problem hiding this comment.
It's less that error message is bad and it's more that I can't do recovery (at least how I'm doing it) because calling the method during recovery will emit the error without allowing me to suppress it.
|
☔ The latest upstream changes (presumably #100516) made this pull request unmergeable. Please resolve the merge conflicts. |
cjgillot
added
S-waiting-on-author
|
Ping from triage: |
|
I've been busy with other stuff, but I'm back to working on this.
…On Sun, Oct 2, 2022, 4:23 PM John Simon ***@***.***> wrote:
Ping from triage:
@carbotaniuman <https://github.com/carbotaniuman> - can you please post
your status on this PR? Thank you
—
Reply to this email directly, view it on GitHub
<#99754 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AJ4ICPZSR3USA7OYZGX3IK3WBH4MXANCNFSM54VD23AA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
carbotaniuman
force-pushed
the
rfc-2102
branch
from
3f53ecc to
88ab19e
Compare
|
@rustbot label -S-waiting-on-author +S-waiting-on-review |
|
Some changes occurred in src/tools/cargo cc @ehuss |
carbotaniuman
force-pushed
the
rfc-2102
branch
2 times, most recently
from
15c7dec to
8edba01
Compare
This comment has been minimized.
This comment has been minimized.
|
☔ The latest upstream changes (presumably #104330) made this pull request unmergeable. Please resolve the merge conflicts. |
|
Switching review status as at a glance I'm unsure if the latest commits replied in full to the previous review. Feel free to request a review with @rustbot author |
rustbot
added
S-waiting-on-author
|
@rustbot ready Mainly want a review on the general approach for error handling here, as I'm not too sure it's correct. |
rustbot
added
S-waiting-on-review
carbotaniuman
force-pushed
the
rfc-2102
branch
from
8edba01 to
2527416
Compare
carbotaniuman
force-pushed
the
rfc-2102
branch
from
2dc4cd2 to
d2a6a93
Compare
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
☔ The latest upstream changes (presumably #105670) made this pull request unmergeable. Please resolve the merge conflicts. |
|
Marking as waiting on author again (merge conflicts and failing CI) |
WaffleLapkin
added
S-waiting-on-author
|
@carbotaniuman are you still working on this? @cjgillot would you be willing to provide feedback on the error handling questions around #99754 (review)? |
|
@carbotaniuman It looks like we're going to want this for the next version of libc. @Amanieu and I are really interested in what it would take to successfully ship this (both the parsing, and the subsequent implementation). Is this something you're likely to continue working on in the near future? |
|
I've been on vacation, but I'm able to respond and push the work through
now.
All of the parser recovery here is to pass the tests that make recovery
happen, and not just error out if an unnamed type is present somewhere
else. I can of course not have this recovery and simplify the code, but I'm
not sure if that's something I can do?
…On Sat, May 20, 2023, 7:58 AM Josh Triplett ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In compiler/rustc_parse/src/parser/ty.rs
<#99754 (comment)>:
> + && self.can_start_anonymous_type()
+ {
+ // If we can parse an anonymous type, do so and let
+ // ast validation error out later
+ let snapshot = self.create_snapshot_for_diagnostic();
+ match self.parse_anonymous_ty(lo) {
+ Ok(ty) => ty,
+ Err(err) => {
+ err.cancel();
+ self.restore_snapshot(snapshot);
+ self.parse_path_start_ty(lo, allow_plus, ty_generics)?
+ }
+ }
+ } else {
+ self.parse_path_start_ty(lo, allow_plus, ty_generics)?
+ }
@carbotaniuman <https://github.com/carbotaniuman>:
@Amanieu <https://github.com/Amanieu> and I were reviewing this, and
wondering if it would be possible to confine this *entirely* to field
definitions, and even then only when the field name is _.
If the field name is _, then look for struct or union in the type. If the
type is struct then it *must* continue on with {. If the type is union,
it *could* be union; or union::something..., either of which would be the
*other* case of unnamed fields (where you have a named type), but if it
isn't the start of a path then it has to continue with {.
Either way, we're wondering if this can *only* be parsed in field types,
and nowhere else, avoiding the need to handle it (for recovery or
otherwise) in other locations a type can appear.
—
Reply to this email directly, view it on GitHub
<#99754 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AJ4ICPZYNHYOBGJ5ZDKHQ33XHC5YPANCNFSM54VD23AA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
@carbotaniuman Ah, I see. So, by parsing the entire type, the parser then knows how much to skip over after saying that unnamed types aren't allowed in the context (rather than just encountering That makes sense. |
|
Ping from triage: I'm closing this due to inactivity, Please reopen when you are ready to continue with this. @rustbot label: +S-inactive |
rustbot
added
the
S-inactive
Reincorporates code from #84571 and #85515, for #49804 which was reverted due to context-sensitive issues around the
unionkeyword.This also incorporates a modified version of the error recovery from #88815, with the modifications being in where the parser recovery occurs.
The main issue I'm facing right now is in recovering for these types, as the current implementation fails to properly reject
impl union {} for union {} {}without also incorrectly rejectingimpl union {}and I'm not really sure how to resolve that without making the code a mess.