Skip to content

struct pattern parsing and diagnostic tweaks #47242

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Jan 13, 2018
Merged

struct pattern parsing and diagnostic tweaks #47242

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
53 changes: 42 additions & 11 deletions src/libsyntax/parse/parser.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -609,14 +609,21 @@ impl<'a> Parser<'a> {
Parser::token_to_string(&self.token)
}

pub fn token_descr(&self) -> Option<&'static str> {
Some(match &self.token {
t if t.is_special_ident() => "reserved identifier",
t if t.is_used_keyword() => "keyword",
t if t.is_unused_keyword() => "reserved keyword",
_ => return None,
})
}

pub fn this_token_descr(&self) -> String {
let prefix = match &self.token {
t if t.is_special_ident() => "reserved identifier ",
t if t.is_used_keyword() => "keyword ",
t if t.is_unused_keyword() => "reserved keyword ",
_ => "",
};
format!("{}`{}`", prefix, self.this_token_to_string())
if let Some(prefix) = self.token_descr() {
format!("{} `{}`", prefix, self.this_token_to_string())
} else {
format!("`{}`", self.this_token_to_string())
}
}

pub fn unexpected_last<T>(&self, t: &token::Token) -> PResult<'a, T> {
Expand Down Expand Up @@ -752,11 +759,27 @@ impl<'a> Parser<'a> {
}

pub fn parse_ident(&mut self) -> PResult<'a, ast::Ident> {
self.parse_ident_common(true)
}

fn parse_ident_common(&mut self, recover: bool) -> PResult<'a, ast::Ident> {
match self.token {
token::Ident(i) => {
if self.token.is_reserved_ident() {
self.span_err(self.span, &format!("expected identifier, found {}",
self.this_token_descr()));
let mut err = self.struct_span_err(self.span,
&format!("expected identifier, found {}",
self.this_token_descr()));
if let Some(token_descr) = self.token_descr() {
err.span_label(self.span, format!("expected identifier, found {}",
token_descr));
} else {
err.span_label(self.span, "expected identifier");
}
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 see much point in a label that duplicates the primary message exactly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There have been (off hand) talks about RLS and other tools being able to reliably show only the error message or the label and although the message works better in isolation, the primary span (should) works better when pointing at the code, and should be shorter as they don't have to duplicate the code's context.

I also want to accommodate people that we might have been "training" to ignore the error message in lieu of the primary label (I know I have that behavior at times).

Because of those reasons, I've been trying to move away from errors that have highlighted spans devoid of labels.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, sounds reasonable.

if recover {
err.emit();
} else {
return Err(err);
}
}
self.bump();
Ok(i)
Expand All @@ -767,6 +790,12 @@ impl<'a> Parser<'a> {
} else {
let mut err = self.fatal(&format!("expected identifier, found `{}`",
self.this_token_to_string()));
if let Some(token_descr) = self.token_descr() {
err.span_label(self.span, format!("expected identifier, found {}",
token_descr));
} else {
err.span_label(self.span, "expected identifier");
}
if self.token == token::Underscore {
err.note("`_` is a wildcard pattern, not an identifier");
}
Expand Down Expand Up @@ -2056,7 +2085,7 @@ impl<'a> Parser<'a> {
self.bump();
Ok(Ident::with_empty_ctxt(name))
} else {
self.parse_ident()
self.parse_ident_common(false)
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 think we should fix this in parse_ident.
The condition is "doesn't start like a struct literal", not "is_reserved_ident".
#15980 just happens to use return as something that isn't a struct literal, but the block can start with any other statement and the fix won't work.
I'd rather move the fix into parsing code for struct literals and used "not a non-reserved ident followed by colon" as a condition.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could you provide incorrect code that you'd expect to fail to be addressed by this PR? I'm confused, as currently (and after this PR) other types of statements will have the expected behavior (continue and point out the lack of =>).

Copy link
Contributor Author

@estebank estebank Jan 10, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Also, for the record, I'm slowly going through the parser looking for places where the span of an error is context dependent on the current state parser, in this case for example, the reason a non reserved ident was found is because we're parsing a struct, in the case where

fn main() {
if 3 == {
    4
}
}

is expecting { is because it wants to parse the block following the 3 == { 4 } if condition, and closer to this case, the reason "- expected one of ., =>, ?, or an operator here" (the actual correct error to be displayed with the original issue's code) is displayed here is because it is expecting a block for the Err(ref e) if e.kind == io::EndOfFile { return } match arm.

Do you feel this is too heavy handed? The main advantage I see of this is 1) these are easy enough mistakes to make and 2) the diagnostic doesn't show you enough context as it is now to identify why the problem is happening in the first place.

Copy link
Contributor

Choose a reason for hiding this comment

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

I thought we get a fatal error from Struct { non_ident } from which we cannot recover, but after rereading it turns out not to be the case.
And ability to not recover from a keyword in parse_ident seems usable in other contexts too.

}
}

Expand All @@ -2073,7 +2102,7 @@ impl<'a> Parser<'a> {
hi = self.prev_span;
(fieldname, self.parse_expr()?, false)
} else {
let fieldname = self.parse_ident()?;
let fieldname = self.parse_ident_common(false)?;
hi = self.prev_span;

// Mimic `x: x` for the `x` field shorthand.
Expand Down Expand Up @@ -2424,6 +2453,7 @@ impl<'a> Parser<'a> {

fn parse_struct_expr(&mut self, lo: Span, pth: ast::Path, mut attrs: ThinVec<Attribute>)
-> PResult<'a, P<Expr>> {
let struct_sp = lo.to(self.prev_span);
self.bump();
let mut fields = Vec::new();
let mut base = None;
Expand Down Expand Up @@ -2458,6 +2488,7 @@ impl<'a> Parser<'a> {
match self.parse_field() {
Ok(f) => fields.push(f),
Err(mut e) => {
e.span_label(struct_sp, "while parsing this struct");
e.emit();
self.recover_stmt();
break;
Expand Down
2 changes: 1 addition & 1 deletion src/test/compile-fail/issue-28433.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
enum bird {
pub duck,
//~^ ERROR: expected identifier, found keyword `pub`
//~^^ ERROR: expected
//~| ERROR: expected
goose
}

Expand Down
1 change: 1 addition & 0 deletions src/test/parse-fail/issue-32501.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,4 +18,5 @@ fn main() {
let mut _b = 0;
let mut _ = 0; //~ ERROR expected identifier, found `_`
//~^ NOTE `_` is a wildcard pattern, not an identifier
//~| NOTE expected identifier
}
2 changes: 1 addition & 1 deletion src/test/ui/issue-44406.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ error: expected identifier, found keyword `true`
--> $DIR/issue-44406.rs:18:10
|
18 | foo!(true); //~ ERROR expected type, found keyword
| ^^^^
| ^^^^ expected identifier, found keyword
Copy link
Contributor

Choose a reason for hiding this comment

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

This case looks interesting.
We don't actually expect an identifier here, why does the note appears?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A previous commit in a separate PR actually removed this incorrect issue (mostly by accident), but it broke too many things to be mergeable.

The macro is expecting a token tree as an argument, so I'm sure it's coming from that. Both cases should probably be pointing at $rest instead, like macro errors that happen after parsing.

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 actually expect an identifier here

My eyes deceive me, we expect an identifier (but this is not related to tt).
First we expect a type, but then we try to do recovery in parse_assoc_op_cast and get one more message (not entirely desirable) about expecting an identifier.
So, it's ok, not related to this PR.


error: expected type, found keyword `true`
--> $DIR/issue-44406.rs:18:10
Expand Down
2 changes: 1 addition & 1 deletion src/test/ui/pub/pub-restricted-error.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ error: expected identifier, found `(`
--> $DIR/pub-restricted-error.rs:16:16
|
16 | pub(crate) () foo: usize, //~ ERROR expected identifier
| ^
| ^ expected identifier

error: aborting due to previous error

2 changes: 1 addition & 1 deletion src/test/ui/pub/pub-restricted-non-path.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ error: expected identifier, found `.`
--> $DIR/pub-restricted-non-path.rs:13:6
|
13 | pub (.) fn afn() {} //~ ERROR expected identifier
| ^
| ^ expected identifier

error: aborting due to previous error

29 changes: 29 additions & 0 deletions src/test/ui/token/issue-15980.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
// Copyright 2018 The Rust Project Developers. See the COPYRIGHT
// file at the top-level directory of this distribution and at
// http://rust-lang.org/COPYRIGHT.
//
// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
// option. This file may not be copied, modified, or distributed
// except according to those terms.

use std::io;

fn main(){
let x: io::IoResult<()> = Ok(());
//~^ ERROR cannot find type `IoResult` in module `io`
//~| NOTE did you mean `Result`?
match x {
Err(ref e) if e.kind == io::EndOfFile {
//~^ NOTE while parsing this struct
return
//~^ ERROR expected identifier, found keyword `return`
//~| NOTE expected identifier, found keyword
}
//~^ NOTE expected one of `.`, `=>`, `?`, or an operator here
_ => {}
//~^ ERROR expected one of `.`, `=>`, `?`, or an operator, found `_`
//~| NOTE unexpected token
}
}
26 changes: 26 additions & 0 deletions src/test/ui/token/issue-15980.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
error: expected identifier, found keyword `return`
--> $DIR/issue-15980.rs:20:13
|
18 | Err(ref e) if e.kind == io::EndOfFile {
| ------------- while parsing this struct
19 | //~^ NOTE while parsing this struct
20 | return
| ^^^^^^ expected identifier, found keyword
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was the only line originally being emitted for this input. Now it correctly complains about missing => below, as well as continue to type checking.


error: expected one of `.`, `=>`, `?`, or an operator, found `_`
--> $DIR/issue-15980.rs:25:9
|
23 | }
| - expected one of `.`, `=>`, `?`, or an operator here
24 | //~^ NOTE expected one of `.`, `=>`, `?`, or an operator here
25 | _ => {}
| ^ unexpected token

error[E0412]: cannot find type `IoResult` in module `io`
--> $DIR/issue-15980.rs:14:16
|
14 | let x: io::IoResult<()> = Ok(());
| ^^^^^^^^ did you mean `Result`?

error: aborting due to 3 previous errors