Patch for column number in doas error messages

classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|

Patch for column number in doas error messages

Phil Eaton
Hey,

Doas currently tells you the line but not the column for syntax errors. In
the case of a missing newline at the end of a line I was confused. So I
added the column number to the message as well.

Also, is there any interest in relaxing the grammar so a trailing rule
without a newline is ok?

Let me know what you think.

diff --git parse.y parse.y
index fde406bcf5a..f98deb81706 100644
--- parse.y
+++ parse.y
@@ -195,7 +195,7 @@ yyerror(const char *fmt, ...)
  va_start(va, fmt);
  vfprintf(stderr, fmt, va);
  va_end(va);
- fprintf(stderr, " at line %d\n", yylval.lineno + 1);
+ fprintf(stderr, " at line %d, column %d\n", yylval.lineno + 1,
yylval.colno);
  parse_errors++;
 }



--
Phil Eaton
Reply | Threaded
Open this post in threaded view
|

Re: Patch for column number in doas error messages

Martijn van Duren-5
Hello Phil,

On 07/09/18 02:35, Phil Eaton wrote:
> Hey,
>
> Doas currently tells you the line but not the column for syntax errors. In
> the case of a missing newline at the end of a line I was confused. So I
> added the column number to the message as well.

I don't care much about the change one way or the other, but your patch
doesn't apply. Also, does this help in other cases that are not POSIX
violations? And if we choose to accept the patch, should this be applied
any or all of our other yyerror implementations?
>
> Also, is there any interest in relaxing the grammar so a trailing rule
> without a newline is ok?

No, POSIX requires lines to end with a newline.[0]

martijn@

>
> Let me know what you think.
>
> diff --git parse.y parse.y
> index fde406bcf5a..f98deb81706 100644
> --- parse.y
> +++ parse.y
> @@ -195,7 +195,7 @@ yyerror(const char *fmt, ...)
>   va_start(va, fmt);
>   vfprintf(stderr, fmt, va);
>   va_end(va);
> - fprintf(stderr, " at line %d\n", yylval.lineno + 1);
> + fprintf(stderr, " at line %d, column %d\n", yylval.lineno + 1,
> yylval.colno);
>   parse_errors++;
>  }
>
>
>
[0] http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap03.html#tag_03_206

Reply | Threaded
Open this post in threaded view
|

Re: Patch for column number in doas error messages

Theo de Raadt-2
In reply to this post by Phil Eaton
Phil Eaton <[hidden email]> wrote:

> Hey,
>
> Doas currently tells you the line but not the column for syntax errors. In
> the case of a missing newline at the end of a line I was confused. So I
> added the column number to the message as well.
>
> Also, is there any interest in relaxing the grammar so a trailing rule
> without a newline is ok?
>
> Let me know what you think.
>
> diff --git parse.y parse.y
> index fde406bcf5a..f98deb81706 100644
> --- parse.y
> +++ parse.y
> @@ -195,7 +195,7 @@ yyerror(const char *fmt, ...)
>   va_start(va, fmt);
>   vfprintf(stderr, fmt, va);
>   va_end(va);
> - fprintf(stderr, " at line %d\n", yylval.lineno + 1);
> + fprintf(stderr, " at line %d, column %d\n", yylval.lineno + 1,
> yylval.colno);
>   parse_errors++;
>  }

I don't see the point of this verboseness, and doubt our yacc and lexer
cooperate well enough to provide a correct colno.

Reply | Threaded
Open this post in threaded view
|

Re: Patch for column number in doas error messages

Otto Moerbeek
On Mon, Jul 09, 2018 at 01:21:25AM -0600, Theo de Raadt wrote:

> Phil Eaton <[hidden email]> wrote:
>
> > Hey,
> >
> > Doas currently tells you the line but not the column for syntax errors. In
> > the case of a missing newline at the end of a line I was confused. So I
> > added the column number to the message as well.
> >
> > Also, is there any interest in relaxing the grammar so a trailing rule
> > without a newline is ok?
> >
> > Let me know what you think.
> >
> > diff --git parse.y parse.y
> > index fde406bcf5a..f98deb81706 100644
> > --- parse.y
> > +++ parse.y
> > @@ -195,7 +195,7 @@ yyerror(const char *fmt, ...)
> >   va_start(va, fmt);
> >   vfprintf(stderr, fmt, va);
> >   va_end(va);
> > - fprintf(stderr, " at line %d\n", yylval.lineno + 1);
> > + fprintf(stderr, " at line %d, column %d\n", yylval.lineno + 1,
> > yylval.colno);
> >   parse_errors++;
> >  }
>
> I don't see the point of this verboseness, and doubt our yacc and lexer
> cooperate well enough to provide a correct colno.

Agreed. I prefer to have the parser print the unexpected token. This
is on my to-do list, but give it a shot if you want.  You can use
bc/bc.y as an exmaple of a possible approach.

        -Otto

Reply | Threaded
Open this post in threaded view
|

Re: Patch for column number in doas error messages

Theo de Raadt-2
Otto Moerbeek <[hidden email]> wrote:

> On Mon, Jul 09, 2018 at 01:21:25AM -0600, Theo de Raadt wrote:
>
> > Phil Eaton <[hidden email]> wrote:
> >
> > > Hey,
> > >
> > > Doas currently tells you the line but not the column for syntax errors. In
> > > the case of a missing newline at the end of a line I was confused. So I
> > > added the column number to the message as well.
> > >
> > > Also, is there any interest in relaxing the grammar so a trailing rule
> > > without a newline is ok?
> > >
> > > Let me know what you think.
> > >
> > > diff --git parse.y parse.y
> > > index fde406bcf5a..f98deb81706 100644
> > > --- parse.y
> > > +++ parse.y
> > > @@ -195,7 +195,7 @@ yyerror(const char *fmt, ...)
> > >   va_start(va, fmt);
> > >   vfprintf(stderr, fmt, va);
> > >   va_end(va);
> > > - fprintf(stderr, " at line %d\n", yylval.lineno + 1);
> > > + fprintf(stderr, " at line %d, column %d\n", yylval.lineno + 1,
> > > yylval.colno);
> > >   parse_errors++;
> > >  }
> >
> > I don't see the point of this verboseness, and doubt our yacc and lexer
> > cooperate well enough to provide a correct colno.
>
> Agreed. I prefer to have the parser print the unexpected token. This
> is on my to-do list, but give it a shot if you want.  You can use
> bc/bc.y as an exmaple of a possible approach.

I still don't see the point.  In 30 years, I've gotten by with parsers
that say "syntax error", and had very bad experiences with programs
that do a poor job anticipating where the parse error is.  Of course
there are programs which do a good job of detecting the error, but
generally those don't use yacc...

Reply | Threaded
Open this post in threaded view
|

Re: Patch for column number in doas error messages

Otto Moerbeek
On Mon, Jul 09, 2018 at 01:52:29AM -0600, Theo de Raadt wrote:

> Otto Moerbeek <[hidden email]> wrote:
>
> > On Mon, Jul 09, 2018 at 01:21:25AM -0600, Theo de Raadt wrote:
> >
> > > Phil Eaton <[hidden email]> wrote:
> > >
> > > > Hey,
> > > >
> > > > Doas currently tells you the line but not the column for syntax errors. In
> > > > the case of a missing newline at the end of a line I was confused. So I
> > > > added the column number to the message as well.
> > > >
> > > > Also, is there any interest in relaxing the grammar so a trailing rule
> > > > without a newline is ok?
> > > >
> > > > Let me know what you think.
> > > >
> > > > diff --git parse.y parse.y
> > > > index fde406bcf5a..f98deb81706 100644
> > > > --- parse.y
> > > > +++ parse.y
> > > > @@ -195,7 +195,7 @@ yyerror(const char *fmt, ...)
> > > >   va_start(va, fmt);
> > > >   vfprintf(stderr, fmt, va);
> > > >   va_end(va);
> > > > - fprintf(stderr, " at line %d\n", yylval.lineno + 1);
> > > > + fprintf(stderr, " at line %d, column %d\n", yylval.lineno + 1,
> > > > yylval.colno);
> > > >   parse_errors++;
> > > >  }
> > >
> > > I don't see the point of this verboseness, and doubt our yacc and lexer
> > > cooperate well enough to provide a correct colno.
> >
> > Agreed. I prefer to have the parser print the unexpected token. This
> > is on my to-do list, but give it a shot if you want.  You can use
> > bc/bc.y as an exmaple of a possible approach.
>
> I still don't see the point.  In 30 years, I've gotten by with parsers
> that say "syntax error", and had very bad experiences with programs
> that do a poor job anticipating where the parse error is.  Of course
> there are programs which do a good job of detecting the error, but
> generally those don't use yacc...

I'm not trying to guess anything. yacc uses one-token lookahead. If it
cannot continue, the lexical value of the token that could not be
processed gives a pretty clear indication of the error spot.

        -Otto

Reply | Threaded
Open this post in threaded view
|

Re: Patch for column number in doas error messages

Phil Eaton
Thanks for the comments. Happy to take a stab at going the bc/bc.y route if
that seems acceptable. That yyerror handler looks much more helpful.

On Mon, Jul 9, 2018 at 4:04 AM Otto Moerbeek <[hidden email]> wrote:

> On Mon, Jul 09, 2018 at 01:52:29AM -0600, Theo de Raadt wrote:
>
> > Otto Moerbeek <[hidden email]> wrote:
> >
> > > On Mon, Jul 09, 2018 at 01:21:25AM -0600, Theo de Raadt wrote:
> > >
> > > > Phil Eaton <[hidden email]> wrote:
> > > >
> > > > > Hey,
> > > > >
> > > > > Doas currently tells you the line but not the column for syntax
> errors. In
> > > > > the case of a missing newline at the end of a line I was confused.
> So I
> > > > > added the column number to the message as well.
> > > > >
> > > > > Also, is there any interest in relaxing the grammar so a trailing
> rule
> > > > > without a newline is ok?
> > > > >
> > > > > Let me know what you think.
> > > > >
> > > > > diff --git parse.y parse.y
> > > > > index fde406bcf5a..f98deb81706 100644
> > > > > --- parse.y
> > > > > +++ parse.y
> > > > > @@ -195,7 +195,7 @@ yyerror(const char *fmt, ...)
> > > > >   va_start(va, fmt);
> > > > >   vfprintf(stderr, fmt, va);
> > > > >   va_end(va);
> > > > > - fprintf(stderr, " at line %d\n", yylval.lineno + 1);
> > > > > + fprintf(stderr, " at line %d, column %d\n", yylval.lineno + 1,
> > > > > yylval.colno);
> > > > >   parse_errors++;
> > > > >  }
> > > >
> > > > I don't see the point of this verboseness, and doubt our yacc and
> lexer
> > > > cooperate well enough to provide a correct colno.
> > >
> > > Agreed. I prefer to have the parser print the unexpected token. This
> > > is on my to-do list, but give it a shot if you want.  You can use
> > > bc/bc.y as an exmaple of a possible approach.
> >
> > I still don't see the point.  In 30 years, I've gotten by with parsers
> > that say "syntax error", and had very bad experiences with programs
> > that do a poor job anticipating where the parse error is.  Of course
> > there are programs which do a good job of detecting the error, but
> > generally those don't use yacc...
>
> I'm not trying to guess anything. yacc uses one-token lookahead. If it
> cannot continue, the lexical value of the token that could not be
> processed gives a pretty clear indication of the error spot.
>
>         -Otto
>


--
Phil Eaton
Reply | Threaded
Open this post in threaded view
|

Re: Patch for column number in doas error messages

Laurence Tratt
In reply to this post by Otto Moerbeek
On Mon, Jul 09, 2018 at 10:04:35AM +0200, Otto Moerbeek wrote:

Hello Otto,

>> I still don't see the point.  In 30 years, I've gotten by with parsers
>> that say "syntax error", and had very bad experiences with programs that
>> do a poor job anticipating where the parse error is.  Of course there are
>> programs which do a good job of detecting the error, but generally those
>> don't use yacc...
> I'm not trying to guess anything. yacc uses one-token lookahead. If it
> cannot continue, the lexical value of the token that could not be processed
> gives a pretty clear indication of the error spot.

We can state something even a little stronger than that. An LR parser (such
as Yacc) will always stop at the first possible point an error can be
guaranteed to have occurred. Often that point is the same as where the error
was made (e.g. "x = 1 + ;" will report an error at the semi-colon), although
sometimes it isn't (a simple example is "fi (x) { ... }": the error will be
reported at the open curly, even though a human would consider the error to
have occurred at "fi"). In my opinion, when the error location is correct
it's a genuine help (and, in practise, most errors seem to fall into this
category); when it's wrong, it tends to be very obvious, and you're in no
worse a situation than you were if it just says "syntax error".

[My personal opinion is that a lot of the ill feeling towards error recovery
is to do with when it gets it wrong, tries to keep on parsing, and fills the
terminal up with incorrect errors -- the cascading error problem. Anyone with
too much time on their hands can read my attempted solutions to that at
https://arxiv.org/abs/1804.07133]


Laurie
--
Personal                                             http://tratt.net/laurie/
Software Development Team                                http://soft-dev.org/
   https://github.com/ltratt              http://twitter.com/laurencetratt