Re: [PATCH for Dlang support 2/2] d: add the custom error message feature

[Akim Demaille]

Hi Adela,

Same problems with the charset in your email, and use of UTF-8 quotes instead 
of @file in the Texinfo file.

> Le 25 oct. 2020 à 15:19, Adela Vais <adela.vais99@gmail.com> a écrit :
> 
> * data/skeletons/lalr1.d: Add the custom error message feature.
> * doc/bison.texi: Document it.
> * tests/calc.at, tests/local.at: Test it.

That's kind of short.

> @@ -77,7 +77,15 @@ public interface Lexer
>    * @@param loc The location of the element to which the
>    *                error message is related]])[
>    * @@param s The string for the error message.  */
> -   void yyerror (]b4_locations_if([b4_location_type[ loc, ]])[string s);
> +  void yyerror (]b4_locations_if([b4_location_type[ loc, ]])[string s);

Avoid changes of style in commits that really do something.  Ideally, this 
change should have been made into the previous commit.


> @@ -276,7 +284,7 @@ b4_user_union_members
>     return yylexer.yylex ();
>   }
> 
> -  protected final void yyerror (]b4_locations_if(ref [b4_location_type[ loc, 
> ]])[string s) {
> +  protected final void yyerror (]b4_locations_if([b4_location_type[ loc, 
> ]])[string s) {

Why is this needed?  Use the commit message to explain why you changed things.  
Think about the future: we might have to understand some changes in the 
skeleton, and the commit message is the natural place where the reader expects 
to find explanations.

>     yylexer.yyerror (]b4_locations_if([loc, ])[s);
>   }
> 
> @@ -555,7 +563,7 @@ m4_popdef([b4_at_dollar])])dnl
>           ++yynerrs_;
>           if (yychar == TokenKind.]b4_symbol(empty, id)[)
>             yytoken = ]b4_symbol(empty, kind)[;
> -          yyerror (]b4_locations_if([yylloc, ])[yysyntax_error(new 
> Context(yystack, yytoken]b4_locations_if([[, yylloc]])[)));
> +          yysyntax_error(new Context(yystack, yytoken]b4_locations_if([[, 
> yylloc]])[));

I'm confused here.  Are you changing the semantics of yysyntax_error to now 
output the error message?  In that case, please rename this function as 
yyreportSyntaxError, so that I can easily navigate between all the skeletons, 
and still understand what is going on.  That the skeletons use the local coding 
style is of course needed, yet they must closely look alike, for the Bison 
maintainers will have to maintain them all in the future.


>         }
> ]b4_locations_if([
>         yyerrloc = yylloc;])[
> @@ -659,8 +667,11 @@ m4_popdef([b4_at_dollar])])dnl
>   }
> 
>   // Generate an error message.
> -  private final string yysyntax_error(Context yyctx)
> -  {]b4_parse_error_case([verbose], [[
> +  private final void yysyntax_error(Context yyctx)
> +  {]b4_parse_error_bmatch(
> +[custom], [[
> +    yylexer.syntax_error(yyctx);]],
> +[detailed\|verbose], [[
>     /* There are many possibilities here to consider:
>        - Assume YYFAIL is not used.  It's too flawed to consider.
>          See
> @@ -691,16 +702,16 @@ m4_popdef([b4_at_dollar])])dnl
>          list is correct for canonical LR with one exception: it
>          will still contain any token that will not be accepted due
>          to an error action in a later state.
> -      */
> +     */
>     if (yyctx.getToken() != ]b4_symbol(empty, kind)[)
>     {
>       // FIXME: This method of building the message is not compatible
>       // with internationalization.
>       string res = "syntax error, unexpected ";
> -      res ~= format!"%s"(yyctx.getToken);
> +      res ~= format!"%s"(yyctx.getToken());
>       immutable int argmax = 5;
> -      SymbolKind[] yyarg = new SymbolKind[argmax];
> -      int yycount = yyctx.getExpectedTokens(yyarg, argmax);
> +      SymbolKind[] yyarg = new SymbolKind[yyntokens_];
> +      int yycount = yyctx.getExpectedTokens(yyarg, yyntokens_);
>       if (yycount < argmax)
>       {
>         for (int yyi = 0; yyi < yycount; yyi++)
> @@ -709,9 +720,10 @@ m4_popdef([b4_at_dollar])])dnl
>           res ~= format!"%s"(SymbolKind(yyarg[yyi]));
>         }
>       }
> -      return res;
> -    }]])[
> -    return "syntax error";
> +      yyerror(]b4_locations_if([yyctx.getLocation(), ])[res);
> +    }]],
> +[[simple]], [[
> +    yyerror(]b4_locations_if([yyctx.getLocation(), ])["syntax error");]])[
>   }
> 
>   /**
> @@ -770,7 +782,7 @@ m4_popdef([b4_at_dollar])])dnl
>           if (yycheck_[yyx + yyn] == yyx && yyx != ]b4_symbol(1, kind)[
>               && !yyTableValueIsError(yytable_[yyx + yyn]))
>             yycount++;
> -        if (yycount < yyargn)
> +        if (yycount < yyntokens_)

I don't understand this.  This seems wrong to me.

Actually, getExpectedTokens is very different from that of Java, and I don't 
understand why.


> +@noindent
> +This implementation is inappropriate for internationalization, see
> +the ‘c/bistromathic’ example for a better alternative.

Use @file, as in the other places.

> diff --git a/tests/calc.at b/tests/calc.at
> index a96fc98a..cd7c8622 100644
> --- a/tests/calc.at
> +++ b/tests/calc.at
> @@ -364,7 +364,7 @@ void location_print (FILE *o, Span s);
> }]])[
> 
> /* Bison Declarations */
> -%token CALC_EOF 0 ]AT_TOKEN_TRANSLATE_IF([_("end of input")], ["end of 
> input"])[
> +%token CALC_EOF 0 ]AT_TOKEN_TRANSLATE_IF([_("end of file")], ["end of 
> input"])[
> %token <]AT_VALUE_UNION_IF([int], [ival])[> NUM   "number"
> %type  <]AT_VALUE_UNION_IF([int], [ival])[> exp
> 
> @@ -666,20 +666,20 @@ m4_define([_AT_DATA_CALC_Y(d)],
> %printer { fprintf (yyo, "%d", $$); } <ival>;
> 
> /* Bison Declarations */
> -%token EOF 0 ]AT_TOKEN_TRANSLATE_IF([_("end of input")], ["end of input"])[
> +%token EOF 0 ]AT_TOKEN_TRANSLATE_IF([_("end of file")], ["end of input"])[
> %token <ival> NUM   "number"
> %type  <ival> exp
> 
> -%token PLUS   "+"
> +%token EQUAL  "="
>        MINUS  "-"
> +       PLUS   "+"
>        STAR   "*"
>        SLASH  "/"
> +       POW    "^"
> +       EOL    "\n"
>        LPAR   "("
>        RPAR   ")"
> -       EQUAL  "="
> -       POW    "^"
>        NOT    "!"
> -       EOL    "\n"

Please explain in the commit message why you did that.  The order of the tokens 
in the error messages matters, but let's be clear about that in the commit 
message.

> @@ -1448,6 +1447,14 @@ AT_CHECK_CALC_LALR1_D([%define parse.error verbose 
> %define api.prefix {calc} %ve
> 
> AT_CHECK_CALC_LALR1_D([%debug])
> 
> +AT_CHECK_CALC_LALR1_D([%define parse.error custom])
> +AT_CHECK_CALC_LALR1_D([%define parse.error detailed])
> +AT_CHECK_CALC_LALR1_D([%define parse.error simple])
> +AT_CHECK_CALC_LALR1_D([%define parse.error verbose])
> +AT_CHECK_CALC_LALR1_D([%locations %define parse.error custom])
> +AT_CHECK_CALC_LALR1_D([%locations %define parse.error detailed])
> +AT_CHECK_CALC_LALR1_D([%locations %define parse.error simple])
> +AT_CHECK_CALC_LALR1_D([%locations %define parse.error verbose])

This is good.  But I don't think we need all the flavors.  Just keep

> +AT_CHECK_CALC_LALR1_D([%define parse.error custom])
> +AT_CHECK_CALC_LALR1_D([%locations %define parse.error custom])
> +AT_CHECK_CALC_LALR1_D([%locations %define parse.error detailed])
> +AT_CHECK_CALC_LALR1_D([%locations %define parse.error simple])
> +AT_CHECK_CALC_LALR1_D([%locations %define parse.error verbose])

for instance.  We need to save cycles in the test suite.


> AT_CHECK_CALC_LALR1_D([%define parse.error verbose %debug %verbose])
> AT_CHECK_CALC_LALR1_D([%define parse.error verbose %debug %define 
> api.token.prefix {TOK_} %verbose])
> AT_CHECK_CALC_LALR1_D([%define parse.error verbose %debug %define 
> api.symbol.prefix {SYMB_} %verbose])
> diff --git a/tests/local.at b/tests/local.at
> index b18bdf3b..36019602 100644
> --- a/tests/local.at
> +++ b/tests/local.at
> @@ -871,7 +871,46 @@ m4_define([AT_YYERROR_DEFINE(d)],
> public void yyerror (]AT_LOCATION_IF([[YYLocation l, ]])[string m)
> {
>   stderr.writeln (]AT_LOCATION_IF([[l, ": ", ]])[m);
> -}]])
> +}
> +]AT_ERROR_CUSTOM_IF([[
> +// In the case of D, there are no single quotes around the symbols
> +// so they need to be added here
> +public string transformToken(]AT_API_PREFIX[Parser.SymbolKind token)
> +{
> +  string tokenStr;

Please name the returned value "res", always.

> +  foreach (i; format("%s", token))
> +  {
> +    if (i == '\"')
> +      tokenStr ~= '\'';
> +    else
> +      tokenStr ~= i;
> +  }
> +  if (tokenStr.length == 1)
> +    return '\'' ~ tokenStr ~ '\'';
> +  else
> +    return tokenStr;
> +}
> +
> +public void syntax_error(]AT_API_PREFIX[Parser.Context ctx)
> +{
> +    stderr.write(]AT_LOCATION_IF([[ctx.getLocation(), ": ",]])["syntax 
> error");
> +    {

Indentation is not consistent with the previous function.

> +      ]AT_API_PREFIX[Parser.SymbolKind token = ctx.getToken();
> +      stderr.write(" on token @<:@", transformToken(token), "@:>@");
> +    }
> +    {
> +      immutable int argmax = 7;
> +      ]AT_API_PREFIX[Parser.SymbolKind[] arg = new 
> ]AT_API_PREFIX[Parser.SymbolKind[argmax];
> +      int n = ctx.getExpectedTokens(arg, argmax);
> +      if (0 < n) {
> +        stderr.write(" (expected:");
> +        for (int i = 0; i < n; ++i)
> +          stderr.write(" @<:@", transformToken(arg[i]), "@:>@");
> +        stderr.writeln(")");
> +      }
> +    }
> +}
> +]])[]])

Adela, I have not installed this patch.  Please, make a revised version that 
addresses my concerns, and send it here as an answer to this message.

Cheers!