parser: fix suppression of syntax errors in range RHS

[birkenfeld]

Invalid expressions on the RHS were just swallowed without generating an error. The new version more closely mirrors the code for parsing ..x in the parse_prefix_range_expr method below, where no cancel is done either.

Fixes #33262.

[rust-highfive]

r? @nikomatsakis

(rust_highfive has picked a reviewer for you, use r? to override)

[jseyfried]

LGTM, but r? @nrc -- there may be a reason this was not done in #30723 that I'm not seeing.

[nrc]

I don't think it is that simple, this seems to be an experimental parse, where if we can't parse as the rhs we parse the range as open on the right (foo..) and try to parse whatever was on the right as something else. Thus, why we cancel the error.

I'm not actually sure that will work, I think we would need to rewind any tokens eaten while we did that parse, and there is no way to do that afaik. So the logic may be broken or it might be that to fix #33262 the error should be found and reported elsewhere, I only had a cursory look at the code so I'm not sure.

cc @durka

[birkenfeld]

@nrc I was thinking about that, and found two questions:

• if canceling the error is correct, shouldn't https://github.com/birkenfeld/rust/blob/issue33262/src/libsyntax/parse/parser.rs#L3152 (which covers the case of .. instead of foo..) use the same strategy?
• the intention appears to be that if is_at_start_of_range_notation_rhs returns true, the parse as expression must succeed - why have such a guard predicate otherwise?

[nrc]

Hmm, looking closer at the history, we should probably emit instead of cancelling here, it might be worth doing that explicitly and trying to recover, but probably not. I think this PR is correct.

shouldn't ... use the same strategy?

yeah, looks like the cancel was an ad hoc fix for an ICE, rather than a deliberate strategy.

the intention appears to be that if is_at_start_of_range_notation_rhs returns true, the parse as expression must succeed - why have such a guard predicate otherwise?

Agreed.

@bors: r+

[bors]

📌 Commit a36fb46 has been approved by nrc

[birkenfeld]

Thanks for the review!

[bors]

⌛ Testing commit a36fb46 with merge 2f9a29c...

[bors]

💔 Test failed - auto-win-msvc-64-opt-rustbuild

[alexcrichton]

@bors: retry

I just restarted the buildmaster which should hopefully fix this issue.

On Tue, May 3, 2016 at 4:23 AM, bors [email protected] wrote:

[image: 💔] Test failed - auto-win-msvc-64-opt-rustbuild
http://buildbot.rust-lang.org/builders/auto-win-msvc-64-opt-rustbuild/builds/974

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub
#33311 (comment)

[alexcrichton]

@bors: retry

[bors]

⌛ Testing commit a36fb46 with merge 6478583...

[bors]

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-32cross-opt, auto-linux-64-cargotest, auto-linux-64-cross-armhf, auto-linux-64-cross-armsf, auto-linux-64-cross-freebsd, auto-linux-64-cross-netbsd, 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-cross-opt, auto-linux-musl-64-opt, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-mac-64-opt-rustbuild, auto-mac-cross-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-cross-opt, auto-win-msvc-32-opt, auto-win-msvc-64-cargotest, auto-win-msvc-64-opt, auto-win-msvc-64-opt-mir, auto-win-msvc-64-opt-rustbuild