Prevent bumping the parser past the EOF.

[eddyb]

Makes Parser::bump after EOF into an ICE, forcing callers to avoid repeated EOF bumps.
This ICE is intended to break infinite loops where EOF wasn't stopping the loop.

For example, the handling of EOF in parse_trait_items' recovery loop fixes #32446.
But even without this specific fix, the ICE is triggered, which helps diagnosis and UX.

This is a [breaking-change] for plugins authors who eagerly eat multiple EOFs.
See docopt/docopt.rs#171 for such an example and the necessary fix.

[nikomatsakis]

@bors r+

[bors]

📌 Commit 4b00d0c has been approved by nikomatsakis

[nikomatsakis]

@bors r-

[nikomatsakis]

starting a crater run

[nagisa]

To me it feels like at this point converting this chain to a match would be better here.

[eddyb]

I wish loop-match was a bit more first-class, but I agree.

[nagisa]

Perhaps a utility function like self.bump_even_through_eof() would be suitable?

[eddyb]

That's just asking to be called in a loop instead of manually analyzing each use site.

[nikomatsakis]

Crater report: https://gist.github.com/nikomatsakis/416a719bf29fb0fa8028

4431 crates tested: 2819 working / 1508 broken / 7 regressed / 1 fixed / 96 unknown.

haven't investigated yet.

[eddyb]

Looks like this breaks syntax extensions (regex, peg and docopt, at least).

[nikomatsakis]

cc @BurntSushi @kevinmehall -- curious to get your take here. In response to various bugs where the parser was looping infinitely, @eddyb implemented a change that caused the bump method to panic if you attempt to bump past EOF. This breaks some of your syntax extension crates. We're debating whether to land it and under what conditions. (One option might be to modify your crates.)

@eddyb, I know you were attempting to modify the PR to only abort if we bumped past EOF multiple times, what was the outcome of those experiments?

[eddyb]

@nikomatsakis Actually, that's the current state, but parse_unspanned_seq is giving me trouble because it's unconditionally bumping a token, even if it's not the ket and the travis failure was because that changes how errors are reported (presumably due to the "expected tokens" list which check, used by eat, pushes onto).

I realize right now that I can just handle the error case by not bumping, and I just pushed that change.

EDIT: Hah, it's not that easy, you can have both errors and a } to bump.

[BurntSushi]

@nikomatsakis I'm totally fine with breakage of plugins if there's a path to fixing them.

With that said, what part of the parsing code would break? Regex's interaction with the parser is quite small: https://github.com/rust-lang-nursery/regex/blob/master/regex_macros/src/lib.rs#L567

I guess it's clearer where it breaks in docopt: https://github.com/docopt/docopt.rs/blob/master/docopt_macros/src/macro.rs#L234

[eddyb]

@BurntSushi The original changes would make parser.eat(&token::Eof) always ICE - but that seemed impractical so I'm trying something which will only trigger on the second EOF bump, not the first one.

[kevinmehall]

peg uses parser.eat to obtain the string literal from a peg!("grammar goes here") invocation and check that no additional arguments are passed. If there's a better way to do that, I'm happy to change it. Unstable interfaces are unstable, after all.

[eddyb]

@kevinmehall Changing that shouldn't be necessary with my latest attempt - just waiting for travis to confirm everything's good with the tests before starting another crater run.

[eddyb]

@nrc See my changes to compile-fail tests. I believe that if we switch away from parsing everything as TTs first we can handle ; closing (, for example:

{ option.map(|some| 42; }

It currently parses as the following TTs:

{ option.map(|some| 42;) }

It accidentally gave relatively sane errors because parse_unspanned_seq was bumping the ; as if it was ) for the method call, resulting in the same effect as this:

{ option.map(|some| 42)) }

I fixed that so it behaves as it should, given what TTs we end up with, but if we didn't parse it to TTs, we could recover a statement based on ; and parse as:

{ option.map(|some| 42); }

Which is arguably what the intention was, and save for a single parse error, the compilation could continue unhindered.

[nikomatsakis]

Sounds good

On Sat, Mar 26, 2016 at 05:31:54AM -0700, Eduard-Mihai Burtescu wrote:

@nikomatsakis Actually, that's the current state, but parse_unspanned_seq is giving me trouble because it's unconditionally bumping a token, even if it's not the ket and the travis failure was because that changes how errors are reported (presumably due to the "expected tokens" list which check, used by eat, pushes onto).

I realize right now that I can just handle the error case by not bumping, and I just pushed that change.

---

You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub:
#32479 (comment)

[nrc]

lgtm

[eddyb]

@bors r=nikomatsakis

[bors]

📌 Commit 221d0fb has been approved by nikomatsakis

[bors]

⌛️ Testing commit 221d0fb with merge a111297...

[bors]

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-debug-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-opt-mir, auto-linux-64-opt-rustbuild, auto-linux-64-x-android-t, auto-linux-64-x-armhf, auto-linux-64-x-armsf, auto-linux-64-x-freebsd, auto-linux-64-x-netbsd, auto-linux-cross-opt, auto-linux-musl-64-opt, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-mac-ios-opt, auto-win-gnu-32-nopt-t, auto-win-gnu-32-opt, auto-win-gnu-32-opt-rustbuild, auto-win-gnu-64-nopt-t, auto-win-gnu-64-opt, auto-win-msvc-32-opt, auto-win-msvc-64-opt, auto-win-msvc-64-opt-mir, auto-win-msvc-64-opt-rustbuild