No line numbers for errors discovered in lrpar/ctbuilder.rs

[ltratt]

The error messages we get from yacc/parser.rs are really helpful, but if the text gets through to ctbuilder.rs the errors are less helpful. Consider:

FuncAttrs -> Result<Vec<AstFuncAttr>, Box<dyn Error>>:
    FuncAttrs FuncAttr { flattenr($1, $) }

The error one gets is:

Unknown text following '$' operator: $)

without a line number (and also repeating the $ character; I think the relevant code &pre_action[last + off..] is missing a +1). That can make tracking the problem down a little frustrating. Given we have token_span in YaccGrammar can we also record and pass through the span of action text? Then ctbuilder.rs could give line/column warnings for such errors.

[ratmice]

I believe we should be able to add a function like action_span() perhaps, but I'll have to look into it!

[ltratt]

Thanks!

[ratmice]

I think the biggest difficulty here is SemVer compatibility, it looks like both
GrammarAST::add_prod and struct Production need changes.

I think the right thing to do is pub action: Option<String>, into pub action: Option<(String, Span)>,

I think if we tried to separate them into distinct fields, such as adding pub action_span: Option<Span> to Production we end up with cases like Some(span) but no action: None.

I don't think YaccGrammar requires any incompatible changes though, there we can just add a new fn action_span. But unfortunately (as per the above) it doesn't look like we currently store that span in the AST anywhere.

I'll probably look at getting some simpler changes before attempting this one.

[ltratt]

I think the biggest difficulty here is SemVer compatibility, it looks like both
GrammarAST::add_prod and struct Production need changes.

I think that's a bullet worth biting. Maybe we can find the other bits where we give errors without line numbers and do all of them in one go?

[ratmice]

Yeah, the other thing we can do is look through GrammarAST for other places that still lack Span information.
There are quite a few in Production itself, e.g. precedence. It could be that it is not used in an error, but maybe it's worth adding as much span info as eagerly as possible in this case?

Finding all the CT*Builder places that error and panic is going to be a bit of a project,
apparenty quote primarily panics e.g. in format_ident!. A lot of these (I think almost all of these) panics are going to be unreachable because we are always generating valid ident e.g. "__gt_wrapper_{}",pidx.

It's definitely worth have a look through obvious places we throw errors in CTParserBuilder in case that guides us to places we need more span info though, I'm just not certain I can hunt down every embedded panic.

[ltratt]

I'm of the opinion that anything we can do to improve the situation is worth it: the perfect is the enemy of the good etc :)

[ratmice]

One more that can be improved (This one was fixed by #625):

In the following commit we note the rule has a production missing action,
We don't currently have a way to get the span from ctparserbuilder though.
ababb51

The spans are in the AST but don't appear to escape the parser,
https://docs.rs/cfgrammar/latest/cfgrammar/yacc/ast/struct.Production.html#structfield.prod_span

Seems like it could be improved by a fn YaccGrammar::prod_span(PIdx) -> Span, that is the last thing I see in gen_user_actions.

[ltratt]

Seems like it could be improved by a fn YaccGrammar::prod_span(PIdx) -> Span, that is the last thing I see in gen_user_actions.

Good idea!

[ratmice]

While doing another look through of CTParserBuilder I did notice some cases that appear to need some work.

There are more than just this call, but Value::try_from which is part of the %grmtools section parsing is just using ? to throw errors.

These aren't currently using a SpannedDiagnosticFormatter for formatting errors with line numbers. This will require some work because these errors may or may not have Span data associated with them, since they can come from https://docs.rs/cfgrammar/latest/cfgrammar/span/enum.Location.html which could alternately be a Location::CommandLine or Location::Other("CTParserBuilder") With only data coming from a %grmtools section having a span.

The data to obtain line numbers is there, but we still need to build out the mechanisms to print them nicely in this case.

[ltratt]

Good spot!

[ratmice]

There is an aspect of it that is surprisingly complex,

In Spanned
Typically e.g. in cfgrammar errors the error itself stores a Vec<Span> The trait then returns an &[Span].

However this really doesn't work for one error, MergeError, it's more like a normal data structure error, (a conflict arising from when two trees are merged together (The trees from %grmtools section, or the command line, or the CTParserBuilder options). It just so happens that both trees can contain a Location via a MergeError<HeaderValue<Location>>.

I assume we have two choices:

1. Convert the error to one where the Location fields are stored contiguously in a Vec<Location>.
2. Trying to make the trait return an Iterator<Item=Location> to work with non-contiguous fields, and a bunch of heartache around deriving the trait (new type, or deriving it based on the generics Location type parameters.

I feel like I'm strongly leaning towards the first as the allocation really doesn't matter given there is one place the error can happen, and that keeps the new trait similar to/the same as Spanned.

[ltratt]

I also tend to prefer the first option.

[ratmice]

That kind of just threw me directly into another problem. In particular figuring out a good way to actually format the errors.

Currently, Spanned errors format the message, totally separately from the Span.
Spans kind of carry an exact source reference to the data that they refer to.

Duplicate start declarations
5| %start foo                                         
                  ^^^ The first declaration.
6| %start bar                                           
                  ^^^ The second declaration.

So with spanned duplicates, the span always carries a reference to the value,
the span printing can follow to the original source string.
Currently with Location, only the Span variant carries that, CommandLine is unitary,
and Other("CTParserBuilder") carries a logical location, but no value, or reference obtainable from that location.

I can think of two options,

1. Copy the value or a reference into the Location, this could either be a string representation, or for argv it could perhaps also be the usize argc value for the argument.
2. Alternately integrate the error formatting into the span print formatting, currently the error holds the associated values, so if you gave it the index of the span, it could format using both the value and the span.

Like ideally if feels like we want something like the following:

Duplicate yacckind values found
1| %grmtools{yacckind: Foo}
                          ^^^^^^^ The first instance.
...
     Bar 
     ^^^  Given on the command line

As the data is organized right now I think the best I can do is:

Duplicate yacckind values found Foo and Bar.  <--- the error formatting
1| %grmtools{yacckind: Foo}                               <--- Span formatting
                          ^^^^^^^ The first instance.         <--- Span formatting
...
     On the command line.                                                                        <--- Span formatting
                  ^^^^^^^^^^^^  The location of the second instance       <--- Span formatting

The first option seems like it might lead to duplicate data,
since we're storing (Value, Location), and we'd now be storing a location including the value.
But it doesn't seem like a deal breaker, the total amount of data in the grmtools section is fairly small.

In the second case we'd need to add a function to the Location variation of the Spanned trait:

fn fmt_span_idx(&self, usize) -> String

This would work, because the error actually already contains the values, we just can't reach it from the Location printing code currently.

I'm not sure that I do have any strong opinions here, both give me a "try it you might not like it feeling"