Skip to content

Parser: fix ICE on failure to parse token tree #33726

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
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
17 changes: 10 additions & 7 deletions src/libsyntax/parse/parser.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2807,14 +2807,14 @@ impl<'a> Parser<'a> {
let span = Span { hi: close_span.hi, ..pre_span };

match self.token {
// Correct delmiter.
// Correct delimiter.
token::CloseDelim(d) if d == delim => {
self.open_braces.pop().unwrap();

// Parse the close delimiter.
self.bump();
}
// Incorect delimiter.
// Incorrect delimiter.
token::CloseDelim(other) => {
let token_str = self.this_token_to_string();
let mut err = self.diagnostic().struct_span_err(self.span,
Expand All @@ -2829,9 +2829,9 @@ impl<'a> Parser<'a> {

self.open_braces.pop().unwrap();

// If the incorrect delimter matches an earlier opening
// If the incorrect delimiter matches an earlier opening
// delimiter, then don't consume it (it can be used to
// close the earlier one)Otherwise, consume it.
// close the earlier one). Otherwise, consume it.
// E.g., we try to recover from:
// fn foo() {
// bar(baz(
Expand All @@ -2844,8 +2844,11 @@ impl<'a> Parser<'a> {
// Silently recover, the EOF token will be seen again
// and an error emitted then. Thus we don't pop from
// self.open_braces here.
},
_ => unreachable!(),
}
_ => {
// Bad token: an error will already have been emitted; just skip it.
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.

Is there a reason we need to bump() here?

}
}

Ok(TokenTree::Delimited(span, Rc::new(Delimited {
Expand All @@ -2859,7 +2862,7 @@ impl<'a> Parser<'a> {
// invariants: the current token is not a left-delimiter,
// not an EOF, and not the desired right-delimiter (if
// it were, parse_seq_to_before_end would have prevented
// reaching this point.
// reaching this point).
maybe_whole!(deref self, NtTT);
match self.token {
token::CloseDelim(_) => {
Expand Down
15 changes: 15 additions & 0 deletions src/test/parse-fail/issue-33569.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
// Copyright 2016 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.

// compile-flags: -Z no-analysis

macro_rules! foo {
{ $+ } => () //~ ERROR: expected identifier, found `+`
} //~^ ERROR: no rules expected the token `}`
Copy link
Contributor

@jseyfried jseyfried May 21, 2016

Choose a reason for hiding this comment

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

If we removed the call to self.bump() above, I believe the second error would be

no rules expected for the token `+`

I would prefer this since there's nothing wrong with the } token.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right about the error message.

I did it this way mostly because it matches the behaviour of stable rust (1.8.0).

But I'm happy to change it. I have no strong feelings about which is better. Ideally I think the second error message should not be emitted at all, but that would probably be a bigger change (which I haven't looked into).

Copy link
Contributor

Choose a reason for hiding this comment

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

the second error message should not be emitted at all

Agreed. I believe we can avoid the second error message entirely by replacing this line with

self.parse_ident().unwrap_or_else(|mut e| {
    e.emit();
    keywords::Invalid.ident()
})

instead of modifying the unreachable! match arm in parse_token_tree.