Fix struct_expr grammar: make base update part optional. #38957
Conversation
The current grammar rule 'struct_expr' does not set the base update part as optional. Fixed by a single ?.
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @steveklabnik (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
| @@ -512,7 +512,7 @@ unit_expr : "()" ; | |||
| ```antlr | |||
| struct_expr : expr_path '{' ident ':' expr | |||
| [ ',' ident ':' expr ] * | |||
| [ ".." expr ] '}' | | |||
| [ ".." expr ] ? '}' | | |||
There was a problem hiding this comment.
IIRC [ <rules> ] already means <rules> are optional.
Side note: this rule says A { a: 0 .. A { ..Default::default() } (note the missing comma after the 0) would do base update twice, but the comma between fields and base update cannot be omitted here. It also disallows A { ..Default::default() } which is valid, because it requires at least one field. Want to fix this rule?
There was a problem hiding this comment.
Often in BNF () is used for grouping and [] for optional but in grammar.md it's used just for grouping. ( quote: Square brackets are used to group rules. )
There was a problem hiding this comment.
But yeah I'll fix the rule to be valid, but this will be affected by my other PR.
I think I'll just close this down and do some more thought out work on the grammar.
The current grammar rule 'struct_expr' does not set the base update part
as optional. ( S { x: 55, ..base } ).
Fixed by a single ?.
I noticed this in #38956, but I'm not sure about the policy for PR. This seems like a disjoint change to me, therefore the separate PR. If things like this should be just mashed together to keep the queue cleaner, let me know.
The grammar also needs a lot of work (fix me everywhere). I might have time to make it more complete in the next weeks.
r? @steveklabnik