Error handler trait

[ratmice]

Here is the best attempt I think I can muster for implementing the patch in #426 as a trait instead of plain callback functions, I figured it might be better to push it to a separate branch since @DavidHVernon has been doing some test on that, and this switches the API quite a bit.

This allows for the ErrorHandler to be owned by the outer thing, so values in it's Self type can escape. So that is the benefit over the plain callbacks approach which can only return via the Box<dyn Error>.

I've pushed a branch of error_callbacks_test repo which implement this trait First a very quick/dirty implementation using String as a backing store. https://github.com/ratmice/error_callbacks_test/blob/implement_as_trait/build.rs#L273-L287 The second one is much nicer using the ariadne crate to produce nice messages. https://github.com/ratmice/error_callbacks_test/tree/ariadne_trait_impl

In the second (the work in progress ariadne errors) these errors look something like:

     ╭─[src/erroneous.y:4:1]
     │
   1 │ %token a b
     │        ┬
     │        ╰── Shifted
     │
   4 │ B -> (): a | a a {};
     │ ┬        ┬
     │ ╰─────────── Reduced rule
     │          │
     │          ╰── Reduced production
  ───╯

It is outside the repo because it's a failing build.rs, and ctfails tests can't quite handle it. I was hoping that this would make calling it easier than having a bunch of individual functions for each callback. It sort of succeeds from the CT*Builder perspective, but the lifetimes semantics require a few differences from the normal build.rs e.g. a move |...| closure to lrpar_config. This is why LexErrorHandler can be a &mut, while GrammarErrorHandler has to be the Rc<RefCell<_>>.

I believe that I am happy with this approach in general, I think it can both be used by tools like nimbleparse to cut down some code duplication internally (But I don't try to do that here), and useful for outside projects (Like custom error printing, and my future project of producing SARIF output). As a trait these things should be easier to put into a crates and used.

With the holidays coming up, I'm not certain how quickly I will be able to respond to any requested changes. If you don't happen to get to it before then either, I wish you happy holidays.

[ltratt]

I am broadly in favour of this line of attack: thanks! This part of grmtools is currently undercooked, and I must admit that I had never really worked out in my mind what a better way of doing things might be. I think the trait approach pioneered here is broadly the right one. Of course, there are many minor details about the API that we can think about, but at the very least this feels like we're making a substantial move in the right direction.

Here are some half-formed general questions I have:

1. Is/should the error API intended to be incremental? i.e. it calls the functions as soon as problems are detected? Or are they bundled up at the end (and/or in well-defined stages, in the sense of rustc, where type checking happens first and only when fully complete is the borrow checker run)?
2. Should warnings be differentiated from errors? If so, should they be part of one trait or ... ?
3. Should "analysis" be a separate thing than errors/warnings?

My immediate reaction is that I think I like the sound of: incremental; warnings are separate but can be part of one trait; and "analysis" doesn't need separating from errors/warnings. But my immediate reactions rarely stand up to long-term scrutiny, so don't read too much into that :)

[ratmice]

Being fully incremental might require some work,

We'd need to make a trait for e.g. cfgrammar, to replace the vector here: https://github.com/softdevteam/grmtools/blob/master/cfgrammar/src/lib/yacc/parser.rs#L306

The tension being that cfgrammar wants to parse produce as many errors as it can, even after previous errors have occurred. So currently it stores those in a Vec and returns them all at once.

[ltratt]

Being fully incremental might require some work,

Ah, sorry, I wasn't very clear. So we can be "incremental" (in the sense I intended, but didn't clarify, oops!) API without really being very incremental in the way we use the API (at least yet). But I've realised my question wasn't very well formed. The API you're proposing is inherently incremental (in the sense that it's not a return type for build or similar), which I think is good. So we can ignore that high-level question :)

[ratmice]

Sorry I haven't answered all of your questions, I want to think about them and don't feel I have clear answers without tinkering with things.

[ratmice]

3. Should "analysis" be a separate thing than errors/warnings?

The feeling I'm getting is that, *ErrorHandler is in 1 to 1 correspondence to a RTParserBuilder or CTParserBuilder. While analysis is in many to one, one of the things keeping missing_from_parser/lexer, and conflicts from being just forms of analysis is that 1 to 1 correspondence. So it might be good to try to replace those with analysis passes.

Which would entail making it separate and having multiple analysis passes, presumably feeding into an *ErrorHandler But I'm still not really sure about the specific interfaces they might need to interact, like when an analysis pass succeeds with no errors, I assume you might want to record that?

In counter fashion I find it difficult to see how we can get the very detailed error messages from conflicts if it is going through a generic analysis interface. It seems like an analysis/error handler could be pretty difficult to get right with both ideas as abstract as they are.

I did a little reading of the Sarif specification today, to get an idea of how it handles these kinds of things. I'm looking at the example on page 43 of https://docs.oasis-open.org/sarif/sarif/v2.1.0/sarif-v2.1.0.pdf Which i'll snip some relevant bits out of:

{
   message": { "text": "Tainted data was used. The data came from [here](3)." }
   "relatedLocations": [{
         "id": 3,
         "physicalLocation": {
              "uri": "file:///C:/code/input.c",
              "region": {
                   "startLine": 25,
                   "startColumn": 19
              }
         }
    }]
}

So essentially it is linking spans directly into format strings, and using markdown links to associate span information into the message. While that would allow us to encode the current forms of analysis we're doing with one interface between GrammarErrorHandler, and all the analysis passes. It also seems like it would be pretty heavy-weight and entail a lot of dependencies or at least specialized string formatting code.

[ratmice]

2. Should warnings be differentiated from errors? If so, should they be part of one trait or ... ?

I kind of feel like it is fine to keep them in one trait,

One thing to note with warnings is that the printing of them from within cargo is a bit strange. https://github.com/softdevteam/grmtools/blob/master/lrpar/src/lib/ctbuilder.rs#L455C44-L460

Because cargo generally wants to suppress stderr. So CTParserBuilder uses an environment variable to tell if we are running under cargo, and then print them specially, so they get shown regardless of panic. This seems to me like logic we probably don't want everywhere.

I think I'd like to try the simplest thing I can imagine for warnings, changing the results() signature to something like:

/// Under the following conditions this function must return an `Err`
/// ....
/// Otherwise an `Ok(vec!["Warnings", ...])` 
fn results(&self) -> Result<Vec<String>, Box<dyn Error>>

Edit: Or Option<Vec<String>> even to avoid the allocation?

That way we can keep when/how to actually print them specific to CTParserBuilder and each specific tool, GrammarErrorHandler then doesn't have to worry about gaining a show_warnings flag, printing or not is up to the tool in question. I am already imagining though that between Cargo and Ariadne, we might get double "Warning:" text, because both are prefixing their output.

Do you think that is generally worth trying?

[ratmice]

So I would like to focus for a minute on ways to integrate this in an 'imperfect' fashion without hamstringing ourselves to an imperfect trait.

• Include the trait as a Sealed trait trait
• Implement 'DefaultGrammarErrorHandlerandDefaultLexerErrorHandler`s in lrpar and lrlex
• Migrate tools like nimbleparse/lrlex/lrpar, CTParserBuilder, and RTParserBuilder to use those Default*Handlers internally.
• Because the above cross crate boundries the trait methods must be callable by external crates so this would be a supertrait sealed trait in the sealed trait guide above. The methods would be documented unstable, and #[doc(hidden)]
• We could even add something like #[cfg(feature="unstable_api")] pub struct UnstableAPI(); #[cfg(not(feature="unstable_api")] struct UnstableAPI(); argument to the methods, to indicate their instability in code. enabling or disabling the unstable_api feature, then turns these from supertrait sealed trait to method signature sealed trait making them uncallable without the unstable_api feature enabled. (Now that I think about feature additivity this may not actually work, it may see it as a multiply defined struct)
• Once we feel like we have the trait method signatures in good working order and are happy with everything, we could unseal it and either add or unhide the error_handler in the Builders

I think a plan of this sort would allow us to migrate the code to the new trait piecemeal, without having to try getting it perfect on the first try, and getting some real experience using it internally within the codebase, before having to worry about stablizing it.

Curious if you think this sounds like a viable plan?

[ltratt]

So I've been chewing for a bit, and my current thinking is that the "map" approach does, by its nature, lock in a fairly specific kind of behaviour. In many cases it's a very reasonable style of behaviour, but it seems to me that we have a more general kind of behaviour at our fingertips, and might as well see if we can work out exactly what that is. [Part of the reason I'm inclined towards the more general approach is that whatever we do now is an API break. Since grmtools current approach to reporting errors was always short-termist, that break is not just OK, but I've always been expecting it to come. That said, ideally, we'd like to do fewer breaks in the future to the error reporting API because we hope that we're able to soon iterate towards an approach we can more-or-less stick to for the long term.] Of course, maybe the "map" approach will end up being the best --- we can always come back to it if so!

Bringing everything together, I think we can see a possible high-level design principle:

• We want to report errors / warnings / analysis stuff to the user as we discover it. [What I previously called "incremental" though that word might be a bit loaded!]

The obvious question then becomes: how does that play out in practise? I think we can see something else emerge from our discussions: there are various layers of errors / warnings / analysis. Fortunately (AFAICS at the moment, at least!) these can compose. So, for example we might have a couple of traits (as you can see, I don't know what good names are!):

trait RTParserBuilderErrorWarningAnalysisTraitThing {
  /// Report a syntax error in the input grammar.
  fn syntax_error(...);
}

trait CTParserBuilderErrorWarningAnalysisTraitThing: RTParserBuilderErrorWarningAnalysisTraitThing {
  /// The grammar is referencing tokens that aren't in the lexer.
  fn missing_in_lexer(...);
}

Both RTParserBuilder and CTParserBuilder can (if we want!) provide default implementations.

Because these things compose, CTParserBuilder can, internally, provide its own implementation that wraps the user's version, so build can still return Err if there are errors. So CTParserBuilder might do something like:

struct ErrorWarningWrapper {
  user_impl: Box<dyn RTParserBuilderErrorWarningAnalysisTraitThing>,
  errors_seen: Vec<Box<dyn Error>>
}

impl RTParserBuilderErrorWarningAnalysisTraitThing for ErrorWarningWrapper {
  fn syntax_error(...) {
    self.errors_seen.push(...);
    self.user_impl.syntax_error(...);
  }
}

and then CTParserBuilder can check error_wrapper.errors_seen in build and report them back to the user (or just return Err("Some errors were seen") or whatever it chooses). In other words, the "standard" case would feel pretty much just like using grmtools today --- but you also have the ability to do fine-grained things with errors / warnings if you want (whether that's printing them to stderr, or popups, or feeding them to a later analysis or whatever).

Please feel free to tell me that I've lost the plot :) You've thought about this more than I have!

[ratmice]

my current thinking is that the "map" approach does, by its nature, lock in a fairly specific kind of behaviour. In many cases it's a very reasonable style of behaviour, but it seems to me that we have a more general kind of behaviour at our fingertips, and might as well see if we can work out exactly what that is.

That seems totally reasonable to want to explore other options, I would say the "map" approach is my best attempt so far with a focus on compatibility. That is it merely adds new optional behavior, and allows for some internal reorganization that didn't affect API but does allow us to remove some duplicated code across a few crates. I'm happy to look at implementations that focus on things like incremental reporting of errors that will require incompatible changes.

Part of the reason I'm inclined towards the more general approach is that whatever we do now is an API break. Since grmtools current approach to reporting errors was always short-termist, that break is not just OK, but I've always been expecting it to come. That said, ideally, we'd like to do fewer breaks in the future to the error reporting API because we hope that we're able to soon iterate towards an approach we can more-or-less stick to for the long term.

Bringing everything together, I think we can see a possible high-level design principle:

• We want to report errors / warnings / analysis stuff to the user as we discover it. [What I previously called "incremental" though that word might be a bit loaded!]

I think another term for this could be an error stream or channel perhaps.
I'm going to enumerate what I consider to be the main areas of difficulty implementing these (in no particular order)

1. RTParserBuilder/CTParserBuilder currently receive buffered errors in batches and the user e.g. as a Vec<*Error>. So I think the streaming must go at a lower level, e.g. One of the main sources of errors is currently ending up here: https://docs.rs/cfgrammar/latest/src/cfgrammar/yacc/ast.rs.html#18 Such that we could consider that vec becoming the write end of a channel.
2. At the origin where most errors occur I don't believe we have anything such as the yacc filename.y, so we need a way to tack this on in the builders, or otherwise pass it (or even just any form FileId ).
3. add_duplicate_occurrence is in tension with streaming, because it is collating duplicate rules/names etc which require caching them and reporting them at the end to squash all duplicates to a single error. https://github.com/softdevteam/grmtools/blob/0f30331f14bbb0d353d533f16e8b2bebeb485a95/cfgrammar/src/lib/yacc/parser.rs#L272-L291C2

I believe that is going to be all the issues we should expect to encounter (maybe an instance of each across the crates). It is definitely possible I've overlooked something though. One and two definitely have the most user visible impact on API (I think in this case ASTWithValidityInfo::new wants a channel and a some File Id or filename). I'm guessing we'd want to remove 3 or move it out so it is something the read end of an error channel would do compacting duplicate errors?

[ltratt]

That is it merely adds new optional behavior, and allows for some internal reorganization that didn't affect API but does allow us to remove some duplicated code across a few crates.

That's a good point and something I might have underplayed. Though this PR does add new pub traits so I think it does also extend the current user-visible API? That said, one thing that might be worth considering is using this PR (or something like it) as a stepping-stone for a minor release while we chug away on a bigger longer-term change for the next major release. I'm open to your thoughts on that!

I think another term for this could be an error stream or channel perhaps.

"Streaming errors/warnings/etc" (is there a term that covers "errors/warnings/etc"? I can't think of one off-hand!) is certainly a better term than "incremental".

RTParserBuilder/CTParserBuilder currently receive buffered errors in batches and the user e.g. as a Vec<*Error>. So I think the streaming must go at a lower level, e.g. One of the main sources of errors is currently ending up here: https://docs.rs/cfgrammar/latest/src/cfgrammar/yacc/ast.rs.html#18 Such that we could consider that vec becoming the write end of a channel.

I think there's a good implicit question here which is: how far should we push streaming errors? We could say "we only really care about RTBuilder/CTBuilder", and as long as they present a nice API we don't mind too much if other parts of grmtools have a slightly cruder error/warnings API. This would probably be a good intermediate step, full stop, and perhaps might be the most pragmatic option anyway?

At the origin where most errors occur I don't believe we have anything such as the yacc filename.y, so we need a way to tack this on in the builders, or otherwise pass it (or even just any form FileId ).

Right, so the streaming error API would probably need some sort of helper methods (or sometimes maybe parameters) that allow it to be told such information. I think some implementations will largely ignore this information, but some will want every last bit of it!

add_duplicate_occurrence is in tension with streaming, because it is collating duplicate rules/names etc which require caching them and reporting them at the end to squash all duplicates to a single error. https://github.com/softdevteam/grmtools/blob/0f30331f14bbb0d353d533f16e8b2bebeb485a95/cfgrammar/src/lib/yacc/parser.rs#L272-L291C2

I think this an instance of the first problem in disguise? Maybe I'm not noticing an important detail though!

Thanks for bearing with me on this. I know it's challenging when good work receives a barrage of questions! But your PRs really have made me think and I'm very grateful that you're willing to bear with me while we work out what to do next!

[ratmice]

add_duplicate_occurrence is in tension with streaming, because it is collating duplicate rules/names etc which require caching them and reporting them at the end to squash all duplicates to a single error. https://github.com/softdevteam/grmtools/blob/0f30331f14bbb0d353d533f16e8b2bebeb485a95/cfgrammar/src/lib/yacc/parser.rs#L272-L291C2

I think this an instance of the first problem in disguise? Maybe I'm not noticing an important detail though!

It's definitely dependent on the first problem, I think the important question here is what do we prioritize between reducing error spam, and streaming errors upon discovery.

• It is the only place we do lookups in the vector of errors.
• We currently prioritize an incompatible property, reducing error spam by combining repeated errors and emitting them once with multiple locations.

In this case the errors aren't created in a single pass e.g. given the following:

A: 'a';
A: 'a2';
B: 'b';
A: 'a3';

we errs.push(duplicate A), parse B successfully, then search through errs and add the span for the a3 rule to the error previously pushed, so it isn't like we do a single pass here over all the duplicate names and emit an error.

There are a couple of ways we could solve it, depending upon which which decide has priority.

Thanks for bearing with me on this. I know it's challenging when good work receives a barrage of questions! But your PRs really have made me think and I'm very grateful that you're willing to bear with me while we work out what to do next!

If it helps, I actually think we aren't too far off from e.g. a fully streaming errors code-wise as the crow flies, but to me the important question is whether it is reasonable from an API compatibility perspective, but I think it is worth making a good faith effort.

[ltratt]

It is the only place we do lookups in the vector of errors.

We currently prioritize an incompatible property, reducing error spam by combining repeated errors and emitting them once with multiple locations.

Good point! I think I'm OK with shoving off to the implementer of the error trait (we could, if we wanted, even provide a de-duper composition trait, but that's a strictly optional extra). That would simplify what we do internally sufficiently to unblock this general idea I think?

If it helps, I actually think we aren't too far off from e.g. a fully streaming errors code-wise as the crow flies, but to me the important question is whether it is reasonable from an API compatibility perspective, but I think it is worth making a good faith effort.

Thank you -- and I agree!

[ratmice]

So, today I focused on whether or not it is possible to implement this in any roughly compatible way, and it seems like it is possible I changed one (rarely used method) from returning a slice to a Vec) . The basic idea is to mirror the current ownership structure. ASTWithValidationInfo, goes from owning a Vec<YaccGrammarError> to owning a (error_sender, Option<error_receiver>) pair. We can then reimplement the functions expecting to return a Vec<YaccGrammarError> result in terms of an interator over the error_receiver endpoint. Alternately there is a new method which returns the error_receiver (basically) Option::take, in which case they can receive errors as they are produced.

I've now pushed this to https://github.com/ratmice/grmtools/tree/error_channels It still doesn't pass the testsuite (it now appears only tests for duplicate errors are left to fix). Because it would either allow us to go through a deprecation cycle, or even just keep the buffered error vecs, around for cases where we want the simpler API?

I think there are a few rough edges, revolving around mixing the channel approach with Vec result approach, e.g. when the user has taken the error_receiver, then subsequently asks for you to convert it into a Vec<YaccGrammarError>, I don't believe we currently have the right return values to indicate this nicely. Anyway, curious about your thoughts on the matter.

[ltratt]

I think this PR might also be in a state to be closed?