Re: [PATCH 7/7] yacc.c: push: don't clear the parser state when accepting/rejecting

[Akim Demaille]

[I just noticed this message was never sent.  I'm sending it now FTR,
but it was part of what 3.6.90 ships.]

> Le 28 juin 2020 à 16:52, Akim Demaille <akim.demaille@gmail.com> a écrit :
> 
> It seems much more natural to leave the parser state available for
> analysis when there is a failure.  And to provide an explicit means to
> reinitialize a parser state for future parses.
> 
> +** Backward incompatible changes
> +
> +*** Push parsers no longer clear their state when parsing is finished
> +
> +  TL;DR: call yypstate_clear before calling yypush_parse in loops.
> +
> +  Previously push-parsers cleared their state when parsing was finished (on
> +  success and on failure).  This made it impossible to check if there were
> +  parse errors, since yynerrs was also reset.  This can be especially
> +  troublesome when used in autocompletion, since a parser with error
> +  recovery would suggest (irrelevant) expected tokens even if there were
> +  failure.
> +
> +  Now the parser state can be examined when parsing is finished.  However,
> +  before reusing a parser instance, its state must be reset with
> +  `yypstate_clear (ps)`.

I'm replacing this commit by the following one, which preserves the previous
behavior, yet allows post-mortem analysis of a finished parse.


commit 6848d96ca375732e8947fe9b4c9e092a28d85612
Author: Akim Demaille <akim.demaille@gmail.com>
Date:   Sun Jun 28 16:09:13 2020 +0200

    yacc.c: push: don't clear the parser state when accepting/rejecting
    
    Currently when a push parser finishes its parsing (i.e., it did not
    return YYPUSH_MORE), it also clears its state.  It is therefore
    impossible to see if it had parse errors.
    
    In the context of autocompletion, because error recovery might have
    fired, the parser is actually already in a different state.  For
    instance on `(1 + + <TAB>` in the bistromathic, because there's a
    `exp: "(" error ")"` recovery rule, `1 + +` tokens have already been
    popped, replaced by `error`, and autocompletions think we are ready
    for the closing ")".  So here, we would like to see if there was a
    syntax error, yet `yynerrs` was cleared.
    
    In the case of a successful parse, we still have a problem: if error
    recovery succeeded, we won't know it, since, again, `yynerrs` is
    clearer.
    
    It seems much more natural to leave the parser state available for
    analysis when there is a failure.
    
    To reuse the parser, we should either:
    
    1. provide an explicit means to reinitialize a parser state for future
       parses.
    
    2. automatically reset the parser state when it is used in a new
       parse.
    
    Option 2 requires to check whether we need to reinitialize the parser
    each time we call `yypush_parse`, i.e., each time we give a new token.
    This seems expensive compared to Option 1, but I benchmarks revealed
    no difference.  Option 1 is incompatible with the documentation
    ("After `yypush_parse` returns a status other than `YYPUSH_MORE`, the
    parser instance `yyps` may be reused for a new parse.")
    
    So Option 2 wins, reusing the private `yynew` member to record that a
    parse was finished, and therefore that the state must reset in the
    next call to `yypull_parse`.
    
    While at it, this implementation now reuses the previously enlarged
    stacks from one parse to another.
    
    * data/skeletons/yacc.c (yypstate_new): Set up the stacks in their
    initial configurations (setting their bottom to the stack array), and
    use yypstate_clear to reset them (moving their top to their bottom).
    (yypstate_delete): Adjust.
    (yypush_parse): At the beginning, clear yypstate if needed, and at the
    end, record when yypstate needs to be clearer.
    
    * examples/c/bistromathic/parse.y (expected_tokens): Do not propose
    autocompletion when there are parse errors.
    * examples/c/bistromathic/bistromathic.test: Check that case.

diff --git a/NEWS b/NEWS
index a715694c..9e1c5076 100644
--- a/NEWS
+++ b/NEWS
@@ -42,6 +42,22 @@ GNU Bison NEWS
     filename_type       -> api.filename.type
     package             -> api.package
 
+*** Push parser state
+
+** Backward incompatible changes
+
+*** Push parsers no longer clear their state when parsing is finished
+
+  Previously push-parsers cleared their state when parsing was finished (on
+  success and on failure).  This made it impossible to check if there were
+  parse errors, since `yynerrs` was also reset.  This can be especially
+  troublesome when used in autocompletion, since a parser with error
+  recovery would suggest (irrelevant) expected tokens even if there were
+  failure.
+
+  Now the parser state can be examined when parsing is finished.  The parser
+  state is reset when starting a new parse.
+
 ** Bug fixes
 
 *** Include the generated header (yacc.c)
diff --git a/TODO b/TODO
index 31734891..25ad2b5c 100644
--- a/TODO
+++ b/TODO
@@ -37,8 +37,6 @@ Unless we play it dumb (little structure).
 examples about conflicts in the reports.
 
 ** Bistromathic
-- Hitting tab on a line with a syntax error is ugly
-
 - How about not evaluating incomplete lines when the text is not finished
   (as shells do).
 
diff --git a/data/skeletons/yacc.c b/data/skeletons/yacc.c
index 0cd204b0..08ec9883 100644
--- a/data/skeletons/yacc.c
+++ b/data/skeletons/yacc.c
@@ -808,7 +808,8 @@ int yydebug;
 /* Parser data structure.  */
 struct yypstate
   {]b4_declare_parser_state_variables[
-    /* Whether this instance has not started parsing yet.  */
+    /* Whether this instance has not started parsing yet.
+     * If 2, it corresponds to a finished parsing.  */
     int yynew;
   };]b4_pure_if([], [[
 
@@ -1463,18 +1464,14 @@ yypstate_clear (yypstate *yyps)
   yystate = 0;
   yyerrstatus = 0;
 
-  yystacksize = YYINITDEPTH;
-  yyssp = yyss = yyssa;
-  yyvsp = yyvs = yyvsa;]b4_locations_if([[
-  yylsp = yyls = yylsa;]])[]b4_lac_if([[
+  yyssp = yyss;
+  yyvsp = yyvs;]b4_locations_if([[
+  yylsp = yyls;]])[
 
-  yyes = yyesa;
-  yyes_capacity = ]b4_percent_define_get([[parse.lac.es-capacity-initial]])[;
-  if (YYMAXDEPTH < yyes_capacity)
-    yyes_capacity = YYMAXDEPTH;]])[
   /* Initialize the state stack, in case yypcontext_expected_tokens is
      called before the first call to yyparse. */
   *yyssp = 0;
+  yyps->yynew = 1;
 }
 
 /* Initialize the parser data structure.  */
@@ -1486,9 +1483,16 @@ yypstate_new (void)
     return YY_NULLPTR;]])[
   yyps = YY_CAST (yypstate *, malloc (sizeof *yyps));
   if (!yyps)
-    return YY_NULLPTR;
-  yyps->yynew = 1;]b4_pure_if([], [[
+    return YY_NULLPTR;]b4_pure_if([], [[
   yypstate_allocated = 1;]])[
+  yystacksize = YYINITDEPTH;
+  yyss = yyssa;
+  yyvs = yyvsa;]b4_locations_if([[
+  yyls = yylsa;]])[]b4_lac_if([[
+  yyes = yyesa;
+  yyes_capacity = ]b4_percent_define_get([[parse.lac.es-capacity-initial]])[;
+  if (YYMAXDEPTH < yyes_capacity)
+    yyes_capacity = YYMAXDEPTH;]])[
   yypstate_clear (yyps);
   return yyps;
 }
@@ -1501,10 +1505,10 @@ yypstate_delete (yypstate *yyps)
 #ifndef yyoverflow
       /* If the stack was reallocated but the parse did not complete, then the
          stack still needs to be freed.  */
-      if (!yyps->yynew && yyss != yyssa)
+      if (yyss != yyssa)
         YYSTACK_FREE (yyss);
 #endif]b4_lac_if([[
-      if (!yyps->yynew && yyes != yyesa)
+      if (yyes != yyesa)
         YYSTACK_FREE (yyes);]])[
       free (yyps);]b4_pure_if([], [[
       yypstate_allocated = 0;]])[
@@ -1562,8 +1566,14 @@ yyparse (]m4_ifset([b4_parse_param], 
[b4_formals(b4_parse_param)], [void])[)]])[
      Keep to zero when no symbol should be popped.  */
   int yylen = 0;]b4_push_if([[
 
-  if (!yyps->yynew)
+  switch (yyps->yynew)
     {
+    case 2:
+      yypstate_clear (yyps);
+      goto case_0;
+
+    case_0:
+    case 0:
       yyn = yypact[yystate];
       goto yyread_pushed_token;
     }]])[
@@ -2060,22 +2070,21 @@ yyreturn:
       yydestruct ("Cleanup: popping",
                   YY_ACCESSING_SYMBOL (+*yyssp), yyvsp]b4_locations_if([, 
yylsp])[]b4_user_args[);
       YYPOPSTACK (1);
-    }
-#ifndef yyoverflow
-  if (yyss != yyssa)
-    YYSTACK_FREE (yyss);
-#endif]b4_lac_if([[
-  if (yyes != yyesa)
-    YYSTACK_FREE (yyes);]])b4_push_if([[
-  yypstate_clear (yyps);
-  yyps->yynew = 1;
+    }]b4_push_if([[
+  yyps->yynew = 2;
   goto yypushreturn;
 
 
 /*-------------------------.
 | yypushreturn -- return.  |
 `-------------------------*/
-yypushreturn:]])[
+yypushreturn:]], [[
+#ifndef yyoverflow
+  if (yyss != yyssa)
+    YYSTACK_FREE (yyss);
+#endif]b4_lac_if([[
+  if (yyes != yyesa)
+    YYSTACK_FREE (yyes);]])])[
 ]b4_parse_error_bmatch([detailed\|verbose],
 [[  if (yymsg != yymsgbuf)
     YYSTACK_FREE (yymsg);]])[
diff --git a/doc/bison.texi b/doc/bison.texi
index 678b6efe..320a645b 100644
--- a/doc/bison.texi
+++ b/doc/bison.texi
@@ -7146,6 +7146,10 @@ The value returned by @code{yypush_parse} is the same as 
for @code{yyparse}
 with the following exception: it returns @code{YYPUSH_MORE} if more input is
 required to finish parsing the grammar.
 
+After @code{yypush_parse} returned, the instance may be consulted.  For
+instance check @code{yynerrs} to see whether there were (possibly recovered)
+syntax errors.
+
 After @code{yypush_parse} returns a status other than @code{YYPUSH_MORE},
 the parser instance @code{yyps} may be reused for a new parse.
 @end deftypefun
diff --git a/examples/c/bistromathic/bistromathic.test 
b/examples/c/bistromathic/bistromathic.test
index 7301fb8b..f10cb69c 100755
--- a/examples/c/bistromathic/bistromathic.test
+++ b/examples/c/bistromathic/bistromathic.test
@@ -307,16 +307,24 @@ end of file  exit         exp          ''
 > ''
 err: '
 
-# Check that completion when there is an error prints valid locations.
+# Check that completion prints valid locations even when there is an error.
+sed -e 's/\\t/ /g' >input <<EOF
+1++\t
+EOF
+run -n 0 '> 1++ ''
+> ''
+err: 1.1: syntax error: expected - or ( or number or function or variable 
before +
+err: 1.3: syntax error: expected - or ( or number or function or variable 
before +
+'
+
+# And even when the error was recovered from.
 sed -e 's/\\t/ /g' >input <<EOF
 (1++2) + 3 +\t\t
 EOF
-run -n 0 '> (1++2) + 3 +
-(       -       atan    cos     exp     ln      number  sin     sqrt
-> (1++2) + 3 +
+run -n 0 '> (1++2) + 3 +  ''
 > 
 err: 1.1: syntax error: expected - or ( or number or function or variable 
before +
-err: 1.1: syntax error: expected - or ( or number or function or variable 
before +
+err: 1.1: syntax error: expected - or ( or number or function or variable 
before +
 err: 1.4: syntax error: expected - or ( or number or function or variable 
before +
-err: 1.13: syntax error: expected - or ( or number or function or variable 
before end of file
+err: 1.15: syntax error: expected - or ( or number or function or variable 
before end of file
 '
diff --git a/examples/c/bistromathic/parse.y b/examples/c/bistromathic/parse.y
index 97b60bc9..6d86c77f 100644
--- a/examples/c/bistromathic/parse.y
+++ b/examples/c/bistromathic/parse.y
@@ -456,10 +456,15 @@ expected_tokens (const char *input,
     status = yypush_parse (ps, token, &lval, &lloc);
   } while (status == YYPUSH_MORE);
 
-  // Then query for the accepted tokens at this point.
-  int res = yypstate_expected_tokens (ps, tokens, ntokens);
-  if (res < 0)
-    abort ();
+  int res = 0;
+  // If there were parse errors, don't propose completions.
+  if (!ps->yynerrs)
+    {
+      // Then query for the accepted tokens at this point.
+      res = yypstate_expected_tokens (ps, tokens, ntokens);
+      if (res < 0)
+        abort ();
+    }
   yypstate_delete (ps);
   return res;
 }