refactor expr & stmt parsing + improve recovery#66994
refactor expr & stmt parsing + improve recovery#66994bors merged 34 commits intorust-lang:masterfrom
Conversation
rust-highfive
added
the
S-waiting-on-review
|
Looking at some more changes to |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
We don't have a style guide covering this yet, but this mix&match of lowercase led sentence followed by properly capitalized sentence is jarring to me. In other errors I've sidestepped the issue by issuing two notes or reworking the text to work as a single sentence.
There was a problem hiding this comment.
Well it's pre-existing and I would prefer not to do anything about it here, but maybe you want to file an issue for this or something. :)
There was a problem hiding this comment.
These other cases should also be emitting warnings, right?
There was a problem hiding this comment.
Well warnings are being allowed so that doesn't seem right. The point of this test is only parsing behavior tho, not the lint system.
src/test/ui/did_you_mean/issue-54109-and_instead_of_ampersands.stderr
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
"logical conjuction/disjunction" makes sense and is accurate, but it's not very friendly to most people, who will most likely need to do a double take to understand what it means (if we reword in a more terse manner as suggested above).
There was a problem hiding this comment.
I'm not sure what a less jargon-y wording would be. At least "logical conjunction" is easy to google for.
src/test/ui/parser/issue-65257-invalid-var-decl-recovery.stderr
Outdated
Show resolved
Hide resolved
src/test/ui/parser/issue-65257-invalid-var-decl-recovery.stderr
Outdated
Show resolved
Hide resolved
src/librustc_parse/parser/stmt.rs
Outdated
There was a problem hiding this comment.
@petrochenkov you'll want to take a look at the changes in this file.
src/librustc_parse/parser/expr.rs
Outdated
There was a problem hiding this comment.
Should this use the snapshotting hack to to try and keep the error count down? There are cases where error recovery is too eager and we end up with wall of texts on a single typo :-/
There was a problem hiding this comment.
To be clear, this is fine as is, just something to consider when working on this code doing these kind of things. If an expression can't be parsed, at that point it is nicer to just give up and raise FatalError.
There was a problem hiding this comment.
Imo not worth it until more people complain :)
There was a problem hiding this comment.
What if I'm the one complaining about the output I'm seeing? :^)
src/librustc_parse/parser/stmt.rs
Outdated
There was a problem hiding this comment.
Where did this go? We can't suddenly start rejecting code, even if we were unconditionally warning on it.
There was a problem hiding this comment.
It was always intended to become an error eventually and has been a warning for 3 years, so I figured we can turn it into a hard error by now.
There was a problem hiding this comment.
SGTM, make sure to run crater :)
There was a problem hiding this comment.
SGTM, make sure to run crater :)
There was a problem hiding this comment.
I'll remove it from the PR for now so we don't have to wait for crater and so I don't have to rebase. I've filed #67098 as a reminder.
estebank
left a comment
estebank
left a comment
There was a problem hiding this comment.
LGTM, besides my prior comments
This comment has been minimized.
This comment has been minimized.
|
Rebased & dropped the legacy warning stuff for a future PR. |
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.
|
Rebased. :) |
| LL | let _ = a and b; | ||
| | ^^^ help: use `&&` to perform logical conjunction | ||
| | | ||
| = note: unlike in e.g., python and PHP, `&&` and `||` are used for logical operators |
There was a problem hiding this comment.
I don't know if the note calling out python and PHP here is a good idea.
There was a problem hiding this comment.
I debated this with myself a few times but in the end I thought that habits from python (PHP less so) would be the most likely source of using and so it felt like relevant information for such users.
|
@bors r+ |
|
📌 Commit 621661f has been approved by |
|
🌲 The tree is currently closed for pull requests below priority 100, this pull request will be tested once the tree is reopened |
bors
added
S-waiting-on-bors
|
@bors p=199 |
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
|
☀️ Test successful - checks-azure |
Refactor expression parsing thoroughly Based on rust-lang#66994 together with which this has refactored basically the entirety of `expr.rs`. r? @estebank
7 participants
Summary of important changes (best read commit-by-commit, ignoring whitespace changes):
AttrVecis introduces as an alias forThinVec<Attribute>parse_expr_bottomandparse_stmtare thoroughly refactored.vec![...]in a pattern context.do catch { ... }'label: non_block_exprvar $local,auto $local, andmut $local. Fixes Use 'mut' as alias for 'let mut' #65257.e1 and e2ande1 or e2.macro_legacy_warningsis turned into an error (has been a warning for 3 years!)parse_bottom_exprto use list parsing utility #63396 by forward-porting Clean upparse_bottom_exprto use list parsing utility #64105 which now works thanks to added recovery.ui-fulldeps/ast_stmt_expr_attr.rsis turned into UI and pretty tests.#[attr] if expr {}r? @estebank