softdevteam / grmtools

Rust grammar tool libraries and binaries
Other
494 stars 32 forks source link

Add error callbacks to CTBuilders. #426

Closed ratmice closed 7 months ago

ratmice commented 7 months ago

Here is an initial draft of the patch I was thinking in #425 I still need to add tests, and an example with pretty errors.

But in case @DavidHVernon wanted to test it out, and see if it helps solve his problem in a cleaner fashion, I wanted to get it out as a draft.

Edit: This is still not catch a few things,

"Missing from lexer", "Missing from parser", aren't grammar errors, or lexer errors, but some kind of error relation between the two. It also doesn't get called for warnings.

ratmice commented 7 months ago

Edit: Going back to the drawing board regarding the rest of this comment, because of implementation complexity and compatibility.

So, looking forward to other things I am inclined to try implementing 2 changes to this basic API, despite the fact that I like how simple it is, these changes described are in-order (so building upon one another).

  1. If we pass a trait to the builder, and the trait implements all the functions
  2. adding an arbitrary user-supplied argument to it kind of the equivalent of %parse_param.
  3. add an extra call to_error(T) -> Box<dyn Error>
  4. CTParserBuilder then gets a fn error_handler(GrammarErrorHandler) instead of separate on_* functions.

So it would look something to the effect of the following (which probably doesn't compile):

trait<T> GrammarErrorHandler {
   fn on_grammar_error() -> Option<for<'a> Fn (&mut T, YaccGrammarError)>
   fn on_unexpected_conflicts() -> Option<for<'a> Fn(&mut T, ...)>
   fn to_error(T) -> Box<dyn Error>
}

Here is a playground link, with the basic idea after some renaming https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=2a6137cd8ecd9da8c564e3d1b89ff38e

The purpose of this is to open the door for a separate call from build, which i'll call check for now. This would basically do what build does, except it wouldn't write the generated sources to disk, and instead of returning an error as a result, it could return T.

The motivation for this extra parameter and new check phase, is that T could be something which generates reports such as those in the SARIF static analysis results interchange format. In addition to the use case here for customizing error output.

I think from a build.rs perspective at least, settings a single field on the builder, rather than one for each callback may be an improvement, in that it is much easier to provide a reusable libraries which take care of these things. I think having to supply a T is probably negative. I think there will be a way around requiring users to provide it by requiring it to implement Default.

ratmice commented 7 months ago

Reminder to self, I probably should consider passing in the lex/yacc source string to their various callbacks.

ratmice commented 7 months ago

I put up an alternative trait-ized implementation in #428, which is perhaps worth looking at instead?

ratmice commented 7 months ago

It seems to me like this one can be closed for now, that we're at least going to try and go for the trait based impl instead.