-
Notifications
You must be signed in to change notification settings - Fork 13.1k
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
Clean up parse_bottom_expr
to use list parsing utility
#64105
Conversation
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @eddyb (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. |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
@@ -787,51 +789,16 @@ impl<'a> Parser<'a> { | |||
parse_lit!() | |||
} | |||
token::OpenDelim(token::Paren) => { | |||
self.bump(); |
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.
The order matters here. You've changed the parser to accept ( inner_attr* expr_0, ..., expr_n )
to accepting inner_attr* ( expr_0, ..., expr_n )
.
To preserve the semantics you'll need to use parse_paren_comma_seq
instead and then conditionally do parse_inner_attributes()
on the first element. To avoid duplication, you'll also need to refactor the match p.parse_expr() { ... }
bit in parse_paren_expr_seq
into a different function.
To catch similar mistakes in the future, let's also add a test:
#![feature(stmt_expr_attributes)]
fn main() {
let x = #![allow(warnings)] (1, 2);
//~^ ERROR an inner attribute is not permitted in this context
}
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.
Ok, I have a test and a proper fix now.
My original refactoring of parse_paren_expr_seq
is now unnecessary, and currently just contributes extra noise. I'd like to undo it. But if I back it out locally and force push I believe the current review comments will be lost -- is that ok? Or should I do something else?
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.
The comments themselves and the code snippet remain, it should be fine.
As for the errors about r? @estebank |
☔ The latest upstream changes (presumably #64264) made this pull request unmergeable. Please resolve the merge conflicts. |
Ping from triage |
I've re-pushed the code that I have so far. The errors about |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
Ping from triage, any updates? @estebank |
I haven't had the free time to look at this,. I'll try to make some time this weekend. |
Pinging again from triage - @estebank |
Ping from triage. @estebank any updates on this? Thanks. |
Finally took a look at this in a local branch. The new errors in the output are caused by interactions between this change and existing error recovery. |
Ping from triage: |
Pinging again from triage: |
I'll take another look at the Parser, but I'm not super confident that I can make it work. |
Pinging again from triage: |
Have tried to resolve the error recovery issues, but haven't been able to so far. Closing this pull request, will re-open if I manage to fix this. |
refactor expr & stmt parsing + improve recovery Summary of important changes (best read commit-by-commit, ignoring whitespace changes): - `AttrVec` is introduces as an alias for `ThinVec<Attribute>` - `parse_expr_bottom` and `parse_stmt` are thoroughly refactored. - Extract diagnostics logic for `vec![...]` in a pattern context. - Recovery is added for `do catch { ... }` - Recovery is added for `'label: non_block_expr` - Recovery is added for `var $local`, `auto $local`, and `mut $local`. Fixes #65257. - Recovery is added for `e1 and e2` and `e1 or e2`. - ~~`macro_legacy_warnings` is turned into an error (has been a warning for 3 years!)~~ - Fixes #63396 by forward-porting #64105 which now works thanks to added recovery. - `ui-fulldeps/ast_stmt_expr_attr.rs` is turned into UI and pretty tests. - Recovery is fixed for `#[attr] if expr {}` r? @estebank
Fixes #63396