Gate parser recovery via debugflag #32494
Conversation
|
By the way, I freely admit that I added the calls to (One alternative implementation strategy I considered but did not attempt to implement would be to replace the parser's emitter object with one that aborts after the first emit method is invoked with an error message rather than a warning. That may be a more robust way to go but it also just seemed like it might be a pain in the short term.) |
pnkfelix
added
the
beta-nominated
| @@ -8,6 +8,8 @@ | |||
| // option. This file may not be copied, modified, or distributed | |||
| // except according to those terms. | |||
|
|
|||
| // compile-flags: -Z continue-parse-after-error | |||
|
|
|||
There was a problem hiding this comment.
why do these tests need the flag when they didn't before?
There was a problem hiding this comment.
I think the commit message answers this Q?
Update: that is, without the flag, the parsing bails out on the first error, so all the subsequent error messages are never emitted, and the test fails.
|
cc @nrc @eddyb -- in @rust-lang/compiler meeting, we were considering whether we felt like it was likely that there were still more "bad parse recovery" cases out there. It seemed like the prudent thing to do was to introduce a flag for the short term (at least for beta..) that avoided error recovery in the parser, fix the known bugs, and then try to re-enable recovery for nightly. Given that many of the known bugs have pending fixes -- I still think we ought to take this PR (and hence disable recovery), but maybe we should re-enable it on master by default, at least once #32435 lands. (But leave it disabled on beta.) Then, prior to the next beta, we can decide whether to leave it enabled, or switch it back off (based on whether we've seen further bug reports). |
|
@bors r+ |
|
📌 Commit eb167e3 has been approved by |
|
Disable on beta and enable on master seems like a good plan to me. I think an implementation where the emitter treats the first error as fatal should work, would just need a flag in the emitter. |
I had thought the core team had argued against this previously; see in particular this comment: #31994 (comment)
I tried doing something like this during my first draft, but ... do you want every emit call to include an extra argument indicating the context where its calling from? Because my main problem, when I tried to do it this way, was that emitting errors from non-parser contexts was also causing the whole compile attempt to abort... Or do you want the parser to replace the emitter at the outset of the parse, so that the parser behavior can be modified independently of the other compiler code? |
|
I did not mean to disable on beta and enable on nightly as a permanent state of affairs. I meant: Backport a change to THIS beta that disables recovery. But leave recovery enabled on nightly. Shortly before the next beta, decide whether to change the default back. My mental model here is that at some point we have to switch recovery on, and that would take place in nightly (and then proceed to beta as usual). The question is whether we think enough of the problems are fixed to make it worth giving it a go and starting this process (clearly that was not the case before the last beta shipped). Part of my mental model is that I am somewhat nervous about backporting lots of small changes to beta. But I'm not honestly sure what is lowest risk here. |
|
My opinion is that the situation is maybe less bad and easier to fix than the core team believe. Thus we should do our best to get the error recovery stuff as much testing as possible, fix the issues and get it turned on everywhere. That means having it on for nightly. I do however agree that it might be best to play it safe and turn off on beta for now. Hopefully be the next beta it will be back on again. I agree with Niko that porting lots of stuff to beta is risky. I think we should be doing less of that. On the implementation side, I imagined the emitter would have a bool field for abort_on_error which would be set to true initially and set to false after parsing. It's kind of a gross hack but it should be temporary and should impact the minimum amount of code. Would it work? |
I think the hack is grosser than you realize. As I tried to explain in my comment, I tried using a stateful emitter in my first draft, but also left the API unchanged (apart from the constructors, I think) The problem I faced was that, at least in my attempt, the compilation would abort on the first error emitted from any module, not just the parser. I think the man goal here was to roll back error recovery in just the parser, not elsewhere. That's why I asked for more detail about what you were envisioning (e.g. installing a parser-specific emitter, or feeding a context argument into the single emitter that is shared across compiler phases). Anyway I will look at it again. |
|
Ah I missed this phrase in @nrc's comment:
let me see if I can figure out the right place to add that "set to false" in a stateful emitter... |
|
(i'll merge things together when I rebase in a bit which should make this commit history look a lot nicer.) |
This works by adding a boolean flag, `continue_after_error`, to `syntax::errors::Handler` that can be imperatively set to `true` or `false` via a new `fn set_continue_after_error`. The flag starts off true (since we generally try to recover from compiler errors, and `Handler` is shared across all phases). Then, during the `phase_1_parse_input`, we consult the setting of the `-Z continue-parse-after-error` debug flag to determine whether we should leave the flag set to `true` or should change it to `false`. ---- (We might consider adding a debugflag to do such aborts in other places where we are currently attempting recovery, such as resolve, but I think the parser is the really important case to handle in the face of rust-lang#31994 and the parser bugs of varying degrees that were injected by parse error recovery.)
parser recovery (so that expected errors match up) I'm opting into parser recovery in all these cases out of expediency, not because the error messages you get with recovery enabled are actually all that usable in all cases listed.
pnkfelix
force-pushed
the
gate-parser-recovery-via-debugflag
branch
from
217d540
to
e1d8ad3
Compare
|
Okay this should fit what @nrc was asking for (and also build ... and pass |
|
@bors: r+ |
|
📌 Commit e1d8ad3 has been approved by |
…ebugflag, r=nrc Gate parser recovery via debugflag Gate parser recovery via debugflag Put in `-Z continue_parse_after_error` This works by adding a method, `fn abort_if_no_parse_recovery`, to the diagnostic handler in `syntax::errors`, and calling it after each error is emitted in the parser. (We might consider adding a debugflag to do such aborts in other places where we are currently attempting recovery, such as resolve, but I think the parser is the really important case to handle in the face of rust-lang#31994 and the parser bugs of varying degrees that were injected by parse error recovery.) r? @nikomatsakis
nrc
added
T-compiler
alexcrichton
removed
the
beta-nominated
Gate parser recovery via debugflag
Put in
-Z continue_parse_after_errorThis works by adding a method,
fn abort_if_no_parse_recovery, to thediagnostic handler in
syntax::errors, and calling it after eacherror is emitted in the parser.
(We might consider adding a debugflag to do such aborts in other
places where we are currently attempting recovery, such as resolve,
but I think the parser is the really important case to handle in the
face of #31994 and the parser bugs of varying degrees that were
injected by parse error recovery.)
r? @nikomatsakis