Skip to content

Try to recover from unmatched delimiters in the parser #54029

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

Closed
Closed
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
4 changes: 2 additions & 2 deletions src/librustc_errors/emitter.rs
Original file line number Diff line number Diff line change
@@ -678,8 +678,8 @@ impl EmitterWriter {
// | | something about `foo`
// | something about `fn foo()`
annotations_position.sort_by(|a, b| {
// Decreasing order
a.1.len().cmp(&b.1.len()).reverse()
// Decreasing order. When `a` and `b` are the same length, prefer `Primary`.
(a.1.len(), !a.1.is_primary).cmp(&(b.1.len(), !b.1.is_primary)).reverse()
});

// Write the underlines.
2 changes: 1 addition & 1 deletion src/librustc_metadata/cstore_impl.rs
Original file line number Diff line number Diff line change
@@ -465,7 +465,7 @@ impl cstore::CStore {

let source_file = sess.parse_sess.source_map().new_source_file(source_name, def.body);
let local_span = Span::new(source_file.start_pos, source_file.end_pos, NO_EXPANSION);
let body = source_file_to_stream(&sess.parse_sess, source_file, None);
let body = source_file_to_stream(&sess.parse_sess, source_file, None).0;

// Mark the attrs as used
let attrs = data.get_item_attrs(id.index, sess);
2 changes: 2 additions & 0 deletions src/libsyntax/parse/lexer/mod.rs
Original file line number Diff line number Diff line change
@@ -66,6 +66,7 @@ pub struct StringReader<'a> {
/// The raw source span which *does not* take `override_span` into account
span_src_raw: Span,
open_braces: Vec<(token::DelimToken, Span)>,
crate unmatched_braces: Vec<(token::DelimToken, Span)>,
/// The type and spans for all braces
///
/// Used only for error recovery when arriving to EOF with mismatched braces.
@@ -221,6 +222,7 @@ impl<'a> StringReader<'a> {
span_src_raw: syntax_pos::DUMMY_SP,
open_braces: Vec::new(),
matching_delim_spans: Vec::new(),
unmatched_braces: Vec::new(),
override_span,
last_unclosed_found_span: None,
}
5 changes: 3 additions & 2 deletions src/libsyntax/parse/lexer/tokentrees.rs
Original file line number Diff line number Diff line change
@@ -49,8 +49,9 @@ impl<'a> StringReader<'a> {
token::Eof => {
let msg = "this file contains an un-closed delimiter";
let mut err = self.sess.span_diagnostic.struct_span_err(self.span, msg);
for &(_, sp) in &self.open_braces {
for &(tok, sp) in &self.open_braces {
err.span_label(sp, "un-closed delimiter");
self.unmatched_braces.push((tok, sp));
}

if let Some((delim, _)) = self.open_braces.last() {
@@ -134,7 +135,7 @@ impl<'a> StringReader<'a> {
}
err.emit();
}
self.open_braces.pop().unwrap();
self.unmatched_braces.push(self.open_braces.pop().unwrap());

// If the incorrect delimiter matches an earlier opening
// delimiter, then don't consume it (it can be used to
42 changes: 25 additions & 17 deletions src/libsyntax/parse/mod.rs
Original file line number Diff line number Diff line change
@@ -168,10 +168,13 @@ crate fn parse_stmt_from_source_str(name: FileName, source: String, sess: &Parse
new_parser_from_source_str(sess, name, source).parse_stmt()
}

pub fn parse_stream_from_source_str(name: FileName, source: String, sess: &ParseSess,
override_span: Option<Span>)
-> TokenStream {
source_file_to_stream(sess, sess.source_map().new_source_file(name, source), override_span)
pub fn parse_stream_from_source_str(
name: FileName,
source: String,
sess: &ParseSess,
override_span: Option<Span>,
) -> TokenStream {
source_file_to_stream(sess, sess.source_map().new_source_file(name, source), override_span).0
Copy link
Contributor

Choose a reason for hiding this comment

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

does this mean that for rustc invocations that don't read the source from a file but from stdin we don't get these suggestions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct. On the one hand, those will not have any worse output. On the other, this was a bit explorative and want to nail it down before trying to get to to work in all cases.

}

// Create a new parser from a source string
@@ -191,26 +194,28 @@ pub fn new_parser_from_file<'a>(sess: &'a ParseSess, path: &Path) -> Parser<'a>
/// Given a session, a crate config, a path, and a span, add
/// the file at the given path to the source_map, and return a parser.
/// On an error, use the given span as the source of the problem.
crate fn new_sub_parser_from_file<'a>(sess: &'a ParseSess,
path: &Path,
directory_ownership: DirectoryOwnership,
module_name: Option<String>,
sp: Span) -> Parser<'a> {
crate fn new_sub_parser_from_file<'a>(
sess: &'a ParseSess,
path: &Path,
directory_ownership: DirectoryOwnership,
module_name: Option<String>,
sp: Span,
) -> Parser<'a> {
let mut p = source_file_to_parser(sess, file_to_source_file(sess, path, Some(sp)));
p.directory.ownership = directory_ownership;
p.root_module_name = module_name;
p
}

/// Given a source_file and config, return a parser
fn source_file_to_parser(sess: & ParseSess, source_file: Lrc<SourceFile>) -> Parser {
fn source_file_to_parser(sess: &ParseSess, source_file: Lrc<SourceFile>) -> Parser {
let end_pos = source_file.end_pos;
let mut parser = stream_to_parser(sess, source_file_to_stream(sess, source_file, None));

let (tts, open_braces) = source_file_to_stream(sess, source_file, None);
let mut parser = stream_to_parser(sess, tts);
parser.open_braces = open_braces;
if parser.token == token::Eof && parser.span.is_dummy() {
parser.span = Span::new(end_pos, end_pos, parser.span.ctxt());
}

parser
}

@@ -240,12 +245,15 @@ fn file_to_source_file(sess: &ParseSess, path: &Path, spanopt: Option<Span>)
}

/// Given a source_file, produce a sequence of token-trees
pub fn source_file_to_stream(sess: &ParseSess,
source_file: Lrc<SourceFile>,
override_span: Option<Span>) -> TokenStream {
pub fn source_file_to_stream(
sess: &ParseSess,
source_file: Lrc<SourceFile>,
override_span: Option<Span>,
) -> (TokenStream, Vec<(token::DelimToken, Span)>) {
let mut srdr = lexer::StringReader::new(sess, source_file, override_span);
srdr.real_token();
panictry!(srdr.parse_all_token_trees())
let tt = panictry!(srdr.parse_all_token_trees());
(tt, srdr.unmatched_braces)
}

/// Given stream and the `ParseSess`, produce a parser
220 changes: 157 additions & 63 deletions src/libsyntax/parse/parser.rs

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion src/libsyntax/util/parser_testing.rs
Original file line number Diff line number Diff line change
@@ -22,7 +22,7 @@ use std::path::PathBuf;
pub fn string_to_stream(source_str: String) -> TokenStream {
let ps = ParseSess::new(FilePathMapping::empty());
source_file_to_stream(&ps, ps.source_map()
.new_source_file(PathBuf::from("bogofile").into(), source_str), None)
.new_source_file(PathBuf::from("bogofile").into(), source_str), None).0
}

/// Map string to parser (via tts)
2 changes: 1 addition & 1 deletion src/test/ui/augmented-assignments.nll.stderr
Original file line number Diff line number Diff line change
@@ -9,7 +9,7 @@ LL | x //~ error: use of moved value: `x`
LL | | //~^ value used here after move
LL | | +=
LL | | x; //~ value moved here
| | -
| | ^
| | |
| |_____move out of `x` occurs here
| borrow later used here
24 changes: 24 additions & 0 deletions src/test/ui/parser-recovery-3.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
// 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.

struct A;

impl A {
fn banana(&mut self) {
fn peach(this: &Self, foo: usize {
//~^ ERROR expected one of
//~| ERROR can't use type parameters from outer function
}
}
//~^ ERROR incorrect close delimiter
//~| ERROR expected expression
}

fn main() {}
41 changes: 41 additions & 0 deletions src/test/ui/parser-recovery-3.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
error: incorrect close delimiter: `}`
--> $DIR/parser-recovery-3.rs:19:5
|
LL | fn banana(&mut self) {
| - close delimiter possibly meant for this
LL | fn peach(this: &Self, foo: usize {
| - un-closed delimiter
...
LL | }
| ^ incorrect close delimiter

error: expected one of `!`, `(`, `)`, `+`, `,`, `::`, or `<`, found `{`
Copy link
Contributor

Choose a reason for hiding this comment

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

it's kind of surprising to me that this advice comes here, in a second error after the "main" error above... why is that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The lexer first performs the basic matching braces checks and emits the appropriate error. Then the parser takes the tokenstream and tries to parse things. So the first error is about the lexer finding unbalanced delimiters, while the second one is a parse error not finding an expected close delimiter.

There're two options: keeping the un-emitted DiagnosticBuilders from the lexer and emitting them once we are parsing and find an appropriate place for the close delimiter suggestion, or completely stop performing balanced delimiter checks in the lexer and rely entirely on the parser for parse errors. Both felt like too much work for little win. I'd prefer to iterate this PR until we have the recovery nailed down before revisiting the above possibilities.

--> $DIR/parser-recovery-3.rs:15:42
|
LL | fn peach(this: &Self, foo: usize {
| - -^ expected one of 7 possible tokens here
| | |
| | help: ...the missing `)` may belong here
| if you meant to close this...

error: expected expression, found `)`
Copy link
Contributor

Choose a reason for hiding this comment

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

"found )" is confusing too...?

Copy link
Contributor Author

@estebank estebank Sep 12, 2018

Choose a reason for hiding this comment

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

Mmm... This is surprising. I'm not sure why the parser is finding a ) at all here...

--> $DIR/parser-recovery-3.rs:19:5
|
LL | }
| ^ expected expression

error[E0401]: can't use type parameters from outer function
Copy link
Contributor

Choose a reason for hiding this comment

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

still, nice that we recovered here

--> $DIR/parser-recovery-3.rs:15:25
|
LL | impl A {
| ---- `Self` type implicitly declared here, by this `impl`
LL | fn banana(&mut self) {
LL | fn peach(this: &Self, foo: usize {
| ^^^^
| |
| use of type variable from outer function
| use a type here instead

error: aborting due to 4 previous errors

For more information about this error, try `rustc --explain E0401`.
21 changes: 21 additions & 0 deletions src/test/ui/parser-recovery-4.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
// 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.

fn foo(_: usize) {}
fn bar() {}

fn main() {
let x = 1;
foo(x
bar()
//~^ ERROR expected one of
//~| ERROR expected one of
}
//~^ ERROR incorrect close delimiter
34 changes: 34 additions & 0 deletions src/test/ui/parser-recovery-4.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
error: incorrect close delimiter: `}`
--> $DIR/parser-recovery-4.rs:20:1
|
LL | fn main() {
| - close delimiter possibly meant for this
LL | let x = 1;
LL | foo(x
| - un-closed delimiter
...
LL | }
| ^ incorrect close delimiter

error: expected one of `!`, `)`, `,`, `.`, `::`, `?`, `{`, or an operator, found `bar`
Copy link
Contributor

Choose a reason for hiding this comment

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

something feels..inelegant to me here. Like, I would like to be suppressing these "fallout" errors, and just picking up parsing from some other point..? Hmm, I'm not sure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is an option, to just bail on parsing the entire block from that point on, as we do in other parts of the parser.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem with out right bailing until the next closing brace is that we then don't get the nice recovery shown above. Maybe that's a reasonable trade-off...

Copy link
Contributor

Choose a reason for hiding this comment

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

which is the nice recovery you are referring to?

I tend to think that giving "false errors" is kind of the most confusing thing we can do and we should bias in favor of too few errors and not too many, but opinions vary.

--> $DIR/parser-recovery-4.rs:17:5
|
LL | foo(x
| - -
| | |
| | expected one of 8 possible tokens here
| | help: ...the missing `)` may belong here
| if you meant to close this...
LL | bar()
| ^^^ unexpected token

error: expected one of `.`, `;`, `?`, `}`, or an operator, found `bar`
--> $DIR/parser-recovery-4.rs:17:5
|
LL | foo(x
| - expected one of `.`, `;`, `?`, `}`, or an operator here
LL | bar()
| ^^^ unexpected token
Copy link
Contributor Author

@estebank estebank Sep 12, 2018

Choose a reason for hiding this comment

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

In the ideal end state, this error would not get emitted, instead having the prior one be

error: expected one of `!`, `)`, `,`, `.`, `::`, `?`, `{`, or an operator, found `bar`
  --> $DIR/parser-recovery-4.rs:17:5
   |
LL |     foo(x
   |        - -- help: you might also be missing a semicolon here: `;`
   |        | |
   |        | expected one of 8 possible tokens here
   |        | help: ...the missing `)` may belong here
   |        if you meant to close this...
LL |     bar()
   |     ^^^ unexpected token

but I'm not sure how to verify that we don't start suggesting bogus code that wouldn't work...

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 I understand. But I do think that this example you just gave is kind of cluttered -- I feel like it'd be better to break it apart into multiple sections or something.


error: aborting due to 3 previous errors

20 changes: 10 additions & 10 deletions src/test/ui/resolve/token-error-correct-3.rs
Original file line number Diff line number Diff line change
@@ -20,21 +20,21 @@ pub mod raw {
pub fn ensure_dir_exists<P: AsRef<Path>, F: FnOnce(&Path)>(path: P,
callback: F)
-> io::Result<bool> {
if !is_directory(path.as_ref()) { //~ ERROR: cannot find function `is_directory`
callback(path.as_ref(); //~ ERROR expected one of
fs::create_dir_all(path.as_ref()).map(|()| true) //~ ERROR: mismatched types
//~^ expected (), found enum `std::result::Result`
//~| expected type `()`
//~| found type `std::result::Result<bool, std::io::Error>`
//~| expected one of
} else { //~ ERROR: incorrect close delimiter: `}`
//~^ ERROR: expected one of
//~| unexpected token
if !is_directory(path.as_ref()) {
callback(path.as_ref();
//~^ ERROR expected one of
fs::create_dir_all(path.as_ref()).map(|()| true)
} else {
//~^ ERROR incorrect close delimiter: `}`
//~| ERROR expected one of
//~^^^^ ERROR mismatched types
Ok(false);
}

panic!();
}

fn is_directory<P: AsRef<Path>>(_: P) -> bool { true }
}

fn main() {}
38 changes: 17 additions & 21 deletions src/test/ui/resolve/token-error-correct-3.stderr
Original file line number Diff line number Diff line change
@@ -1,47 +1,43 @@
error: incorrect close delimiter: `}`
--> $DIR/token-error-correct-3.rs:30:9
--> $DIR/token-error-correct-3.rs:27:9
|
LL | if !is_directory(path.as_ref()) { //~ ERROR: cannot find function `is_directory`
LL | if !is_directory(path.as_ref()) {
| - close delimiter possibly meant for this
LL | callback(path.as_ref(); //~ ERROR expected one of
LL | callback(path.as_ref();
| - un-closed delimiter
...
LL | } else { //~ ERROR: incorrect close delimiter: `}`
LL | } else {
| ^ incorrect close delimiter

error: expected one of `)`, `,`, `.`, `?`, or an operator, found `;`
--> $DIR/token-error-correct-3.rs:24:35
|
LL | callback(path.as_ref(); //~ ERROR expected one of
| ^ expected one of `)`, `,`, `.`, `?`, or an operator here
LL | callback(path.as_ref();
| - ^
| | |
| | expected one of `)`, `,`, `.`, `?`, or an operator here
| | help: ...the missing `)` may belong here
| if you meant to close this...

error: expected one of `.`, `;`, `?`, `}`, or an operator, found `)`
--> $DIR/token-error-correct-3.rs:30:9
--> $DIR/token-error-correct-3.rs:27:9
|
LL | fs::create_dir_all(path.as_ref()).map(|()| true) //~ ERROR: mismatched types
LL | fs::create_dir_all(path.as_ref()).map(|()| true)
| - expected one of `.`, `;`, `?`, `}`, or an operator here
...
LL | } else { //~ ERROR: incorrect close delimiter: `}`
LL | } else {
| ^ unexpected token

error[E0425]: cannot find function `is_directory` in this scope
--> $DIR/token-error-correct-3.rs:23:13
|
LL | if !is_directory(path.as_ref()) { //~ ERROR: cannot find function `is_directory`
| ^^^^^^^^^^^^ not found in this scope

error[E0308]: mismatched types
--> $DIR/token-error-correct-3.rs:25:13
--> $DIR/token-error-correct-3.rs:26:13
|
LL | fs::create_dir_all(path.as_ref()).map(|()| true) //~ ERROR: mismatched types
LL | fs::create_dir_all(path.as_ref()).map(|()| true)
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^- help: try adding a semicolon: `;`
| |
| expected (), found enum `std::result::Result`
|
= note: expected type `()`
found type `std::result::Result<bool, std::io::Error>`

error: aborting due to 5 previous errors
error: aborting due to 4 previous errors

Some errors occurred: E0308, E0425.
For more information about an error, try `rustc --explain E0308`.
For more information about this error, try `rustc --explain E0308`.
8 changes: 6 additions & 2 deletions src/test/ui/resolve/token-error-correct.rs
Original file line number Diff line number Diff line change
@@ -12,6 +12,10 @@

fn main() {
foo(bar(;
//~^ ERROR: expected expression, found `;`
//~^ ERROR expected expression, found `;`
//~| ERROR expected one of
//~| ERROR cannot find function `foo` in this scope
//~| ERROR cannot find function `bar` in this scope
}
//~^ ERROR: incorrect close delimiter: `}`
//~^ ERROR incorrect close delimiter: `}`
//~| ERROR expected expression
41 changes: 37 additions & 4 deletions src/test/ui/resolve/token-error-correct.stderr
Original file line number Diff line number Diff line change
@@ -1,19 +1,52 @@
error: incorrect close delimiter: `}`
--> $DIR/token-error-correct.rs:16:1
--> $DIR/token-error-correct.rs:19:1
|
LL | fn main() {
| - close delimiter possibly meant for this
LL | foo(bar(;
| - un-closed delimiter
LL | //~^ ERROR: expected expression, found `;`
...
LL | }
| ^ incorrect close delimiter

error: expected expression, found `;`
--> $DIR/token-error-correct.rs:14:13
|
LL | foo(bar(;
| ^ expected expression
| - ^
| | |
| | expected expression
| | help: ...the missing `)` may belong here
| if you meant to close this...

error: aborting due to 2 previous errors
error: expected one of `)`, `,`, `.`, `?`, or an operator, found `;`
--> $DIR/token-error-correct.rs:14:13
|
LL | foo(bar(;
| -^
| ||
| |expected one of `)`, `,`, `.`, `?`, or an operator here
| |help: ...the missing `)` may belong here
| if you meant to close this...

error: expected expression, found `)`
--> $DIR/token-error-correct.rs:19:1
|
LL | }
| ^ expected expression

error[E0425]: cannot find function `foo` in this scope
--> $DIR/token-error-correct.rs:14:5
|
LL | foo(bar(;
| ^^^ not found in this scope

error[E0425]: cannot find function `bar` in this scope
--> $DIR/token-error-correct.rs:14:9
|
LL | foo(bar(;
| ^^^ not found in this scope

error: aborting due to 6 previous errors

For more information about this error, try `rustc --explain E0425`.
7 changes: 4 additions & 3 deletions src/test/ui/token/issue-10636-2.rs
Original file line number Diff line number Diff line change
@@ -13,9 +13,10 @@

pub fn trace_option(option: Option<isize>) {
option.map(|some| 42;
//~^ ERROR: expected one of
//~^ ERROR expected one of

} //~ ERROR: incorrect close delimiter
//~^ ERROR: expected expression, found `)`
}
//~^ ERROR: incorrect close delimiter
//~| ERROR expected expression

fn main() {}
10 changes: 7 additions & 3 deletions src/test/ui/token/issue-10636-2.stderr
Original file line number Diff line number Diff line change
@@ -6,19 +6,23 @@ LL | pub fn trace_option(option: Option<isize>) {
LL | option.map(|some| 42;
| - un-closed delimiter
...
LL | } //~ ERROR: incorrect close delimiter
LL | }
| ^ incorrect close delimiter

error: expected one of `)`, `,`, `.`, `?`, or an operator, found `;`
--> $DIR/issue-10636-2.rs:15:25
|
LL | option.map(|some| 42;
| ^ expected one of `)`, `,`, `.`, `?`, or an operator here
| - ^
| | |
| | expected one of `)`, `,`, `.`, `?`, or an operator here
| | help: ...the missing `)` may belong here
| if you meant to close this...

error: expected expression, found `)`
--> $DIR/issue-10636-2.rs:18:1
|
LL | } //~ ERROR: incorrect close delimiter
LL | }
| ^ expected expression

error: aborting due to 3 previous errors