Pullreq 5 - parser built-in grammar error recovery was completely b0rked (it would never act): fixed. Now searches for the nearest error rule and resolves to that one. #147

Merged
merged 2 commits into from Apr 17, 2013

Conversation

Projects
None yet
3 participants
Contributor

GerHobbelt commented Feb 16, 2013

quick manual extract of these:

SHA-1: 24e36a5

  • adjusted the code to remove the error recovery from the run-time when no error recovery rules are included in the user-provided grammar. (The correction was necessary as the run-time has been altered in the previous commits.)

SHA-1: d48466f

  • fixes error recovery logic in the parser run-time (tested & verified using the errorlab.js test file): previously 5 tests would fail, but after this fix, all pass.

The loop which looks for a matching error rule has been abstracted out into the function locateNearestErrorRecoveryRule() because the first cause for failed the tests was the parseError() handler firing before the erorr recovery could kick in: this (and user-defined) parseError handlers need a way to detect whether an error recovery rule is available (via the hash.recoverable boolean).

The tests also uncovered an infinitely loop in error recovery in the new code when the lexer hits EOF. This bug has been fixed.

WARNING: a heads up: this code has been tested but not extensively; knowing myself there's probably lurking a python in the grass here. Anyway, it's WAY better than what was because what was did nada.

GerHobbelt added some commits Feb 16, 2013

@GerHobbelt GerHobbelt gitignore: editor bak files 17f8d26
@GerHobbelt GerHobbelt quick manual extract of these:
SHA-1: 24e36a5

* adjusted the code to remove the error recovery from the run-time when no error recovery rules are included in the user-provided grammar. (The correction was necessary as the run-time has been altered in the previous commits.)

SHA-1: d48466f

* fixes error recovery logic in the parser run-time (tested & verified using the errorlab.js test file): previously 5 tests would fail, but after this fix, all pass.

The loop which looks for a matching error rule has been abstracted out into the function locateNearestErrorRecoveryRule() because the first cause for failed the tests was the parseError() handler firing before the erorr recovery could kick in: this (and user-defined) parseError handlers need a way to detect whether an error recovery rule is available (via the hash.recoverable boolean).

The tests also uncovered an infinitely loop in error recovery in the new code when the lexer hits EOF. This bug has been fixed.
f61dfaf
Owner

zaach commented Feb 16, 2013

When you say that tests were failing but have now been fixed, are you referring to a version of jison where you've been making changes? I don't see any error recovery tests failing on master, for instance.

Contributor

GerHobbelt commented Feb 18, 2013

pHew. 'Probably'. Dang. Too long ago (just two months, but then...) to
recall exactly.

What I noticed was that the generated parser kernels were code which would
always enter the error section when an error occurred (logical & expected!)
and then had an if() condition which in my grammar at least would always
trigger (unexpected), thus producing an error statement instead of going
through with error recovery.
Code inspection + debugging (of the jison-generated parser) also showed
that the error recovery, once that if() was 'fixed'[_] would never resolve
to the proper error rules, where 'proper' means I expected the explicit
error rules I had written to receive the parse errors at hand. I also seem
to recall some surprise at finding a heuristic about
consuming/skipping/whatever? _3 tokens* which was resulting in all the
wrong things for my grammar error recovery.
Again, sorry for the vagueness; if it does not answer your wondering about
all this, then throw it in the garbage can. I'm writing this off the top
off my head and only a mail machine here.

[*] 'fixed' in quotes because I was in the process of fixing it to make the
generated code behave the way I wanted/expected it to; I was right smack in
the middle of debugging the critter there, so think 'disabled the if(..)'
here and you're close enough.

Trouble is the grammar is proprietary but it has quite a few 'error'
rules, which were done by me once I got the grammar itself working. (It's
my old yacc/bison coding practice: first make it work; then take on the
error handling and see how far you can dial up the quality of
grammar-assisted error diagnostics.)

To give you an inkling of the nastinesses of mine, here's a snippet:

data_filter
/*
: DATA_FILTER '(' data_source ',' json_filter_expression ')'
| DATA_FILTER '(' cell_reference ',' json_filter_expression ')'
| cell_reference '.' json_filter_expression
*/
: DATA_FILTER data_filter_before_json_filter_expression
data_filter_json_filter_expression_and_close
{
while (this.lexer.topState() !== 'INITIAL') {
this.lexer.popState();
}

        @1.outer = @$;
        $$ = $1
            .setLocationInfo(@1)
            .attachChildren($2, $3);

        // ASTopcode(FKW_DATA_FILTER | FT_ANY | FU_ANY)
    }

| DATA_FILTER '(' error ')'
    {
        // switch lexer mode back to normal RIGHT NOW: *POP* the

condition stack until we hit 'INITIAL', if we haven't arrived there yet!
while (this.lexer.topState() !== 'INITIAL') {
this.lexer.popState();
}

        @3.outer = @$;
        rv = (new

Visyond.FormulaParser.ASTerror(FERR_EXPECTED_DATAFILTER_EXPRESSION,
"Expected a data filter expression."))
.setLocationInfo(@3)
.setCommentsIndex($4.getCommentsIndex());
if ($3 instanceof Visyond.FormulaParser.ASTabstractBase) {
rv.attachChildren($3);
}

        @1.outer = @$;
        $$ = $1
            .setLocationInfo(@1)
            .attachChildren(rv);
    }

| DATA_FILTER error
    {
        // switch lexer mode back to normal RIGHT NOW: *POP* the

condition stack until we hit 'INITIAL', if we haven't arrived there yet!
while (this.lexer.topState() !== 'INITIAL') {
this.lexer.popState();
}

        @2.outer = @$;
        rv = (new

Visyond.FormulaParser.ASTerror(FERR_EXPECTED_DATAFILTER_ARGUMENTS,
"Expected data filter arguments such as a filter expression."))
.setLocationInfo(@2)
.setCommentsIndex($2.getCommentsIndex());
if ($2 instanceof Visyond.FormulaParser.ASTabstractBase) {
rv.attachChildren($2);
}

        @1.outer = @$;
        $$ = $1
            .setLocationInfo(@1)
            .attachChildren(rv);
    }
;

This snippet contains two different error recovery rules which aim to help
diagnose the issue why things went pearshaped. I picked this bit, because
this one contains a hint to the knowledgeable that 'lexer hacks' are also
involved in the the process: that json_filter_expression rule requires a
slightly different lexer behaviour, so I'm pushing and popping lexer states
here at precisely selected spots in the grammar where I 'know' the position
the lexer currently has taken in the input stream, so that the 'lexer mode
switch' via push/pop is guaranteed to be 'just in time' to get the 'right
kind of token lexing' here.

The rules look so odd and have so much 'documentation' because they are a
'hack'/'tweak'/whatchammacallit to have the equivalent of in-rule action
blocks, which in some older yacc-y LR generators are notorious look-ahead
killers, e.g.
data_filter: DATA_FILTER '(' data_source ',' json_filter_expression ')'
==>
data_filter: DATA_FILTER '(' data_source ',' %{ flip-lexer-mode-to-B %}
json_filter_expression ')' %{ revert-lexer-mode-to-A %}
(I hope you get what I'm trying to explain here; I haven't always got the
right [jargon] words ready.)

Met vriendelijke groeten / Best regards,

Ger Hobbelt


web: http://www.hobbelt.com/
http://www.hebbut.net/
mail: ger@hobbelt.com

mobile: +31-6-11 120 978

On Sat, Feb 16, 2013 at 6:51 PM, Zach Carter notifications@github.comwrote:

When you say that tests were failing but have now been fixed, are you
referring to a version of jison where you've been making changes? I don't
see any error recovery failing tests on master, for instance.


Reply to this email directly or view it on GitHubhttps://github.com/zaach/jison/pull/147#issuecomment-13662123.

I can confirm that the current version of jison on npm has this problem. Error modes do not work, but the error tests do pass.

@zaach zaach merged commit f61dfaf into zaach:master Apr 17, 2013

1 check passed

default The Travis build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment