Skip to content

Add error callbacks to CTBuilders. #426

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from

Conversation

ratmice
Copy link
Collaborator

@ratmice ratmice commented Dec 10, 2023

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
Copy link
Collaborator Author

ratmice commented Dec 11, 2023

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
Copy link
Collaborator Author

ratmice commented Dec 14, 2023

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

@ratmice ratmice mentioned this pull request Dec 15, 2023
@ratmice
Copy link
Collaborator Author

ratmice commented Dec 15, 2023

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

@ratmice
Copy link
Collaborator Author

ratmice commented Dec 16, 2023

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.

@ratmice ratmice closed this Dec 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants