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

Recover on mut $p = $e; and var/auto $p = $e suggesting let instead. #65811

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
88 changes: 82 additions & 6 deletions src/libsyntax/parse/parser/stmt.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,9 @@ use crate::ast::{self, DUMMY_NODE_ID, Stmt, StmtKind, Local, Block, BlockCheckMo
use crate::ast::{Attribute, AttrStyle, VisibilityKind, MacStmtStyle, Mac, MacDelimiter};
use crate::parse::{classify, DirectoryOwnership};
use crate::parse::token;
use crate::source_map::{respan, Span};
use crate::source_map::respan;
use crate::symbol::{kw, sym};
use syntax_pos::Span;

use std::mem;
use errors::Applicability;
Expand Down Expand Up @@ -41,11 +42,29 @@ impl<'a> Parser<'a> {
let lo = self.token.span;

Ok(Some(if self.eat_keyword(kw::Let) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Stmt {
id: DUMMY_NODE_ID,
kind: StmtKind::Local(self.parse_local(attrs.into())?),
span: lo.to(self.prev_span),
}
let local = self.parse_local(attrs.into())?;
self.mk_dummy_stmt(lo.to(self.prev_span), local)

} else if self.is_ident_mut() {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

return self.recover_stmt_local(
lo,
attrs,
"missing `let`",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"missing `let`",
"missing `let` before `mut`",

);
} else if self.is_var_sym() {
self.bump();
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
self.bump();
self.bump(); // `var`

return self.recover_stmt_local(
lo,
attrs,
"to introduce a variable, write `let` instead of `var`",
);
} else if self.is_auto_for_let() {
self.bump();
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
self.bump();
self.bump(); // `auto`

return self.recover_stmt_local(
lo,
attrs,
"to introduce a variable, write `let` instead of `auto`",
);
} else if let Some(macro_def) = self.eat_macro_def(
&attrs,
&respan(lo, VisibilityKind::Inherited),
Expand Down Expand Up @@ -473,4 +492,61 @@ impl<'a> Parser<'a> {
"this was erroneously allowed and will become a hard error in a future release"
}).emit();
}

fn recover_stmt_local(
&mut self,
span: Span,
attrs: Vec<Attribute>,
msg: &str,
) -> PResult<'a, Option<Stmt>> {
Comment on lines +495 to +501
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel all of these methods should live in parse/parser/diagnostics.rs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only the ones added in this PR?
There are other methods that do similar things, that were added in the past.

Copy link
Contributor

Choose a reason for hiding this comment

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

I had a talk with @Centril and it's fine either way.

let local = self.parse_local(attrs.into())?;
self.struct_span_err(span, "invalid variable declaration")
.span_suggestion_short(
span,
msg,
"let".to_string(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"let".to_string(),
"let mut".to_string(),

The current code should transform mut x = 0; to let x = 0;.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Applicability::MachineApplicable
).emit();
return Ok(Some(self.mk_dummy_stmt(span, local)));
}

fn mk_dummy_stmt(&mut self, span: Span, kind: P<Local>) -> Stmt {
Copy link
Contributor

Choose a reason for hiding this comment

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

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 {

Stmt {
id: DUMMY_NODE_ID,
kind: StmtKind::Local(kind),
span,
}
}

/// Returns `true` if the next token is Brace
fn is_next_brace(&self) -> bool {
self.look_ahead(1, |t| t.eq(&token::OpenDelim(token::Brace)))
}

/// Returns `true` if the token is kw::Mut and next one is an Ident
fn is_ident_mut(&self) -> bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

self.token.is_keyword(kw::Mut) &&
self.look_ahead(1, |t| t.is_ident())
}

/// Returns `true` if the token is sym::var.
fn is_var_sym(&self) -> bool {
self.token.is_non_raw_ident_where(|ident| ident.name == sym::var) &&
self.is_malformed_declr()
}

fn has_equals(&self) -> bool {
self.look_ahead(2, |t| t.eq(&token::Eq))
}

fn is_auto_for_let(&self) -> bool {
self.token.is_keyword(kw::Auto) && self.is_malformed_declr()
Copy link
Contributor

@Centril Centril Oct 26, 2019

Choose a reason for hiding this comment

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

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

}

fn is_malformed_declr(&self) -> bool {
(self.has_equals() && !self.is_next_brace()) ||
(self.look_ahead(1, |t| t.is_ident()) &&
self.look_ahead(2, |t| t.eq(&token::Semi))
)
}
}
2 changes: 1 addition & 1 deletion src/libsyntax/parse/token.rs
Original file line number Diff line number Diff line change
Expand Up @@ -538,7 +538,7 @@ impl Token {
}

/// Returns `true` if the token is a non-raw identifier for which `pred` holds.
fn is_non_raw_ident_where(&self, pred: impl FnOnce(ast::Ident) -> bool) -> bool {
pub fn is_non_raw_ident_where(&self, pred: impl FnOnce(ast::Ident) -> bool) -> bool {
match self.ident() {
Some((id, false)) => pred(id),
_ => false,
Expand Down
1 change: 1 addition & 0 deletions src/libsyntax_pos/symbol.rs
Original file line number Diff line number Diff line change
Expand Up @@ -725,6 +725,7 @@ symbols! {
usize,
v1,
val,
var,
vec,
Vec,
vis,
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
#[allow(dead_code)]
fn main() {
auto n = 0;//~ ERROR invalid variable declaration
//~^ HELP to introduce a variable, write `let` instead of `auto`
auto m;//~ ERROR invalid variable declaration
//~^ HELP to introduce a variable, write `let` instead of `auto`
m = 0;
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
error: invalid variable declaration
--> $DIR/issue-65257--auto-instead-of-let-recovery.rs:4:5
|
LL | auto n = 0;
| ^^^^ help: to introduce a variable, write `let` instead of `auto`

error: invalid variable declaration
--> $DIR/issue-65257--auto-instead-of-let-recovery.rs:6:5
|
LL | auto m;
| ^^^^ help: to introduce a variable, write `let` instead of `auto`

error: aborting due to 2 previous errors

Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
#[allow(dead_code)]
fn main() {
var n = 0;//~ ERROR invalid variable declaration
//~^ HELP to introduce a variable, write `let` instead of `var`
var m;//~ ERROR invalid variable declaration
//~^ HELP to introduce a variable, write `let` instead of `var`
m = 0;
}
14 changes: 14 additions & 0 deletions src/test/ui/parser/issue-65257--var-instead-of-let-recovery.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
error: invalid variable declaration
--> $DIR/issue-65257--var-instead-of-let-recovery.rs:4:5
|
LL | var n = 0;
| ^^^ help: to introduce a variable, write `let` instead of `var`

error: invalid variable declaration
--> $DIR/issue-65257--var-instead-of-let-recovery.rs:6:5
|
LL | var m;
| ^^^ help: to introduce a variable, write `let` instead of `var`

error: aborting due to 2 previous errors

8 changes: 8 additions & 0 deletions src/test/ui/parser/issue-65257-mut-instead-of-let-recovery.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
#[allow(dead_code)]
fn main() {
mut n = 0;//~ ERROR invalid variable declaration
//~^ HELP missing `let`
mut var;//~ ERROR invalid variable declaration
//~^ HELP missing `let`
var = 0;
}
14 changes: 14 additions & 0 deletions src/test/ui/parser/issue-65257-mut-instead-of-let-recovery.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
error: invalid variable declaration
--> $DIR/issue-65257-mut-instead-of-let-recovery.rs:4:5
|
LL | mut n = 0;
| ^^^ help: missing `let`

error: invalid variable declaration
--> $DIR/issue-65257-mut-instead-of-let-recovery.rs:6:5
|
LL | mut var;
| ^^^ help: missing `let`

error: aborting due to 2 previous errors