Skip to content
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

refactor expr & stmt parsing + improve recovery #66994

Merged
merged 34 commits into from
Dec 21, 2019

Conversation

Centril
Copy link
Contributor

@Centril Centril commented Dec 3, 2019

Summary of important changes (best read commit-by-commit, ignoring whitespace changes):

r? @estebank

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 3, 2019
@petrochenkov petrochenkov self-assigned this Dec 3, 2019
@Centril
Copy link
Contributor Author

Centril commented Dec 3, 2019

Looking at some more changes to expr.rs right now because it's still a mess. :(
Will make those changes as a different PR based on this one tho.

@rust-highfive

This comment has been minimized.

LL | let a = #![allow(warnings)] (1, 2);
| ^^^^^^^^^^^^^^^^^^^
|
= note: inner attributes, like `#![no_std]`, annotate the item enclosing them, and are usually found at the beginning of source files. Outer attributes, like `#[test]`, annotate the item following them.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. :)

let a = #![allow(warnings)] (1, 2);
//~^ ERROR an inner attribute is not permitted in this context

let b = (#![allow(warnings)] 1, 2);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These other cases should also be emitting warnings, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

--> $DIR/issue-54109-and_instead_of_ampersands.rs:20:15
|
LL | let _ = a or b;
| ^^ help: instead of `or`, use `||` to perform logical disjunction: `||`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"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).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what a less jargon-y wording would be. At least "logical conjunction" is easy to google for.

@@ -426,11 +410,11 @@ impl<'a> Parser<'a> {
}

/// Parses a statement, including the trailing semicolon.
pub fn parse_full_stmt(&mut self, macro_legacy_warnings: bool) -> PResult<'a, Option<Stmt>> {
pub fn parse_full_stmt(&mut self) -> PResult<'a, Option<Stmt>> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@petrochenkov you'll want to take a look at the changes in this file.

.span_label(self.token.span, msg)
.emit();
// Continue as an expression in an effort to recover on `'label: non_block_expr`.
self.parse_expr()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :-/

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Imo not worth it until more people complain :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if I'm the one complaining about the output I'm seeing? :^)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you tho? ;)

&format!("expected `;`, found {}", self.this_token_descr())
}).note({
"this was erroneously allowed and will become a hard error in a future release"
}).emit();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where did this go? We can't suddenly start rejecting code, even if we were unconditionally warning on it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SGTM, make sure to run crater :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SGTM, make sure to run crater :)

Copy link
Contributor Author

@Centril Centril Dec 6, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Copy link
Contributor

@estebank estebank left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, besides my prior comments

@bors

This comment has been minimized.

@Centril
Copy link
Contributor Author

Centril commented Dec 6, 2019

Rebased & dropped the legacy warning stuff for a future PR.

@bors

This comment has been minimized.

@JohnCSimon

This comment has been minimized.

@bors

This comment has been minimized.

@Centril
Copy link
Contributor Author

Centril commented Dec 20, 2019

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know if the note calling out python and PHP here is a good idea.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@estebank
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Dec 21, 2019

📌 Commit 621661f has been approved by estebank

@bors
Copy link
Contributor

bors commented Dec 21, 2019

🌲 The tree is currently closed for pull requests below priority 100, this pull request will be tested once the tree is reopened

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 21, 2019
@Centril
Copy link
Contributor Author

Centril commented Dec 21, 2019

@bors p=199

@bors
Copy link
Contributor

bors commented Dec 21, 2019

⌛ Testing commit 621661f with merge c64eecf...

bors added a commit that referenced this pull request Dec 21, 2019
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
@bors
Copy link
Contributor

bors commented Dec 21, 2019

☀️ Test successful - checks-azure
Approved by: estebank
Pushing c64eecf to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 21, 2019
@bors bors merged commit 621661f into rust-lang:master Dec 21, 2019
@Centril Centril deleted the stmt-polish branch December 21, 2019 14:48
Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this pull request Dec 26, 2019
Refactor expression parsing thoroughly

Based on rust-lang#66994 together with which this has refactored basically the entirety of `expr.rs`.

r? @estebank
bors added a commit that referenced this pull request Dec 27, 2019
Refactor expression parsing thoroughly

Based on #66994 together with which this has refactored basically the entirety of `expr.rs`.

r? @estebank
bors added a commit that referenced this pull request Dec 29, 2019
Refactor expression parsing thoroughly

Based on #66994 together with which this has refactored basically the entirety of `expr.rs`.

r? @estebank
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use 'mut' as alias for 'let mut' Clean up parse_bottom_expr to use list parsing utility
7 participants