Recover on mut $p = $e; and var/auto $p = $e suggesting let instead.#65811
Closed
petar-dambovaliev wants to merge 2 commits intorust-lang:masterfrom
petar-dambovaliev:issues-65257
Closed
Recover on mut $p = $e; and var/auto $p = $e suggesting let instead.#65811petar-dambovaliev wants to merge 2 commits intorust-lang:masterfrom petar-dambovaliev:issues-65257
mut $p = $e; and var/auto $p = $e suggesting let instead.#65811petar-dambovaliev wants to merge 2 commits intorust-lang:masterfrom
petar-dambovaliev:issues-65257
Conversation
Contributor
|
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. |
rust-highfive
added
the
S-waiting-on-review
Contributor
|
r? @Centril |
Contributor
|
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 |
Centril
changed the title
mut $p = $e; and var/auto $p = $e suggesting let instead.
Centril
reviewed
Oct 26, 2019
Contributor
Centril
left a comment
Centril
left a comment
There was a problem hiding this comment.
Thanks! This is a nice start. Here are some thoughts I have.
| return Ok(Some(self.mk_dummy_stmt(span, local))); | ||
| } | ||
|
|
||
| fn mk_dummy_stmt(&mut self, span: Span, kind: P<Local>) -> Stmt { |
Contributor
There was a problem hiding this comment.
Suggested change
| fn mk_dummy_stmt(&mut self, span: Span, kind: P<Local>) -> Stmt { | |
| /// Construct a `StmtKind::Local` provided the given `Local`. | |
| fn mk_stmt_local(&mut self, span: Span, kind: P<Local>) -> Stmt { |
| } | ||
|
|
||
| fn is_auto_for_let(&self) -> bool { | ||
| self.token.is_keyword(kw::Auto) && self.is_malformed_declr() |
Contributor
There was a problem hiding this comment.
I think we can be more permissive here.
So reasoning using https://doc.rust-lang.org/nightly/nightly-rustc/syntax/ast/enum.ExprKind.html auto is a contextual keyword. The ways in which it can be a valid statement begin with:
auto( // function call
auto[ // indexing
auto { // struct literal
//--^ These are all the delimiters so we can just check for `OpenDelim(_)`.
auto. // method call
auto; // drop `auto` on the floor
auto!( // macro call
auto trait // e.g. `auto trait Foo {`
auto binop // e.g. `auto + 1`
auto as // a cast, e.g. `auto as u8`
auto : // a type ascription, e.g. `auto : u8`meanwhile, reasoning via https://doc.rust-lang.org/nightly/nightly-rustc/syntax/ast/enum.PatKind.html and intersecting with the list above, I believe we should return true if the token after auto is any identifier (including keyword, because ref, ref mut, and mut) that isn't trait because two consecutive identifiers can only be the start of a statement if it is an auto trait and it cannot become a valid expression. We could also cover literals, but given that this is an irrefutable matching context, it doesn't seem necessary.
We can apply a similar logic to var as well but here we can be unrestricted wrt. identifiers (var trait is not a thing).
| } | ||
|
|
||
| /// Returns `true` if the token is kw::Mut and next one is an Ident | ||
| fn is_ident_mut(&self) -> bool { |
Contributor
There was a problem hiding this comment.
Tho I appreciate the spirit of writing small functions, this function is small enough that it can be inlined where it is used. :) (General point that applies to other functions introduced.)
| "missing `let`", | ||
| ); | ||
| } else if self.is_var_sym() { | ||
| self.bump(); |
Contributor
There was a problem hiding this comment.
Suggested change
| self.bump(); | |
| self.bump(); // `var` |
| "to introduce a variable, write `let` instead of `var`", | ||
| ); | ||
| } else if self.is_auto_for_let() { | ||
| self.bump(); |
Contributor
There was a problem hiding this comment.
Suggested change
| self.bump(); | |
| self.bump(); // `auto` |
| return self.recover_stmt_local( | ||
| lo, | ||
| attrs, | ||
| "missing `let`", |
Contributor
There was a problem hiding this comment.
Suggested change
| "missing `let`", | |
| "missing `let` before `mut`", |
| .span_suggestion_short( | ||
| span, | ||
| msg, | ||
| "let".to_string(), |
Contributor
There was a problem hiding this comment.
Suggested change
| "let".to_string(), | |
| "let mut".to_string(), |
The current code should transform mut x = 0; to let x = 0;.
Contributor
There was a problem hiding this comment.
I think the appropriate thing to do here is to make the span for span_suggestion_short be span.shrink_to_lo() and the suggested code "let ".to_string() (notice the trailing space).
| @@ -41,11 +42,29 @@ impl<'a> Parser<'a> { | |||
| let lo = self.token.span; | |||
|
|
|||
| Ok(Some(if self.eat_keyword(kw::Let) { | |||
Contributor
There was a problem hiding this comment.
It may be a good idea to put all of the "parse let binding"-related code into one function since they share some code, e.g. mk_dummy_stmt, which could then be inlined.
Contributor
|
To get some fresh, less biased eyes on this I'll also reassign to r? @estebank :) |
estebank
reviewed
Oct 26, 2019
Comment on lines
+495
to
+501
|
|
||
| fn recover_stmt_local( | ||
| &mut self, | ||
| span: Span, | ||
| attrs: Vec<Attribute>, | ||
| msg: &str, | ||
| ) -> PResult<'a, Option<Stmt>> { |
Contributor
There was a problem hiding this comment.
I feel all of these methods should live in parse/parser/diagnostics.rs.
Contributor
Author
There was a problem hiding this comment.
Only the ones added in this PR?
There are other methods that do similar things, that were added in the past.
Contributor
There was a problem hiding this comment.
I had a talk with @Centril and it's fine either way.
Member
|
ping from triage Thank you! |
JohnCSimon
added
S-waiting-on-author
glorv
reviewed
Nov 7, 2019
| let local = self.parse_local(attrs.into())?; | ||
| self.mk_dummy_stmt(lo.to(self.prev_span), local) | ||
|
|
||
| } else if self.is_ident_mut() { |
There was a problem hiding this comment.
I'm not sure if this is possible to do this. I think its better to do this kind of error check after we parse statement failed instead in the main parse tree. I had take a dip about this try to do this check after parse var or mut as a path according to the previous parse logic but got lost to find where did the error recovering located. @Centril
Contributor
There was a problem hiding this comment.
The reason you need to look-ahead is to prevent e.g. parsing var(); and similar things the wrong way since that would be a function call.
Collaborator
|
☔ The latest upstream changes (presumably #66189) made this pull request unmergeable. Please resolve the merge conflicts. |
Member
|
Ping from triage |
Member
|
Pinging again from triage Thank you! |
JohnCSimon
added
the
S-inactive
Member
|
Ping from triage: Please do not push to the PR while it is closed as that prevents it from being reopened. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
8 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #65257.
As discussed with @Centril
#65257 (comment)