Repository navigation
No line numbers for errors discovered in lrpar/ctbuilder.rs #623
Description
Activity
I believe we should be able to add a function like
action_span()perhaps, but I'll have to look into it!Thanks!
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>,intopub action: Option<(String, Span)>,I think if we tried to separate them into distinct fields, such as adding
pub action_span: Option<Span>toProductionwe end up with cases likeSome(span)but noaction: None.I don't think
YaccGrammarrequires any incompatible changes though, there we can just add a newfn 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.
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?
Yeah, the other thing we can do is look through
GrammarASTfor other places that still lackSpaninformation.
There are quite a few inProductionitself, 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*Builderplaces that error and panic is going to be a bit of a project,
apparentyquoteprimarily panics e.g. informat_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
CTParserBuilderin case that guides us to places we need more span info though, I'm just not certain I can hunt down every embedded panic.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 :)
Reacted by matt riceOne 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.
ababb51The 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_spanSeems like it could be improved by a
fn YaccGrammar::prod_span(PIdx) -> Span, that is the last thing I see ingen_user_actions.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!
While doing another look through of
CTParserBuilderI 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
%grmtoolssection parsing is just using?to throw errors.These aren't currently using a
SpannedDiagnosticFormatterfor formatting errors with line numbers. This will require some work because these errors may or may not haveSpandata associated with them, since they can come from https://docs.rs/cfgrammar/latest/cfgrammar/span/enum.Location.html which could alternately be aLocation::CommandLineorLocation::Other("CTParserBuilder")With only data coming from a%grmtoolssection 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.
Good spot!
There is an aspect of it that is surprisingly complex,
In Spanned
Typically e.g. incfgrammarerrors the error itself stores aVec<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
CTParserBuilderoptions). It just so happens that both trees can contain aLocationvia aMergeError<HeaderValue<Location>>.I assume we have two choices:
- Convert the error to one where the
Locationfields are stored contiguously in aVec<Location>. - 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.- Convert the error to one where the
I also tend to prefer the first option.
That kind of just threw me directly into another problem. In particular figuring out a good way to actually format the errors.
Currently,
Spannederrors format the message, totally separately from theSpan.
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 withLocation, only theSpanvariant carries that,CommandLineis unitary,
andOther("CTParserBuilder")carries a logical location, but no value, or reference obtainable from that location.I can think of two options,
- Copy the value or a reference into the
Location, this could either be a string representation, or for argv it could perhaps also be theusizeargc value for the argument. - 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
Locationvariation of theSpannedtrait:fn fmt_span_idx(&self, usize) -> StringThis would work, because the error actually already contains the values, we just can't reach it from the
Locationprinting 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"
- Copy the value or a reference into the
The error messages we get from yacc/parser.rs are really helpful, but if the text gets through to
ctbuilder.rsthe errors are less helpful. Consider:The error one gets is:
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 havetoken_spaninYaccGrammarcan we also record and pass through the span of action text? Then ctbuilder.rs could give line/column warnings for such errors.