Structural error

[ryo33]

I think we should have a dedicated error type something like the following instead of miette::Report.

enum ParserError {
    Scanner(ScannerError),
    Syntax(SyntaxError),
}

pub struct Span(Range<usize>);

enum ScannerError {
    UnexpectedToken {
        span: Span,
        token: Token,
        expected: Vec<TokenDef>,
    },
    ...
}

enum SyntaxError {
    ...
}

[ryo33]

I may be going to work on this feature, as it is something I want right now.

[jsinger67]

You can find error types in errors.rs files in basic crates.

[jsinger67]

@ryo33,
I would like to start at parol_runtime with the removal of miette.
I believe that further effort will be necessary to modify parol and any clients like examples and the language server accordingly.
I'm not sure if you are already working on it and I would like to avoid double work here.
So, please let me know if I can start.

[ryo33]

@jsinger67 That's great! Fortunately or unfortunately, I hadn't started on it yet.

[jsinger67]

Ok, fine. Thank you for the quick reply. 👍

[jsinger67]

@ryo33
parol_runtime and generated parsers now (5e33504) return anyhow::Error objects.
All error types are built using the thiserror crate.
miette support was extracted to crates\parol\src\utils\miette_support.rs and is currently only used by parol's binary to leverage miette's fancy error messages and by example basic_interpreter, as proof of concept.
I would like to think about a way to optionally bring miette support to user applications, maybe using parol new with a new switch and only for binary crates.
Please give me feedback how these changes feel from your perspective and if there are still requirements left before closing this issue.

Edit: I didn't succeed with re-exporting thiserror and anyhow from parol_runtime. There were build errors when I tried this. I simply postponed this problem by now.

[ryo33]

Thank you very much. I've created #53 as PoC using your changes.
I believe anyhow is not suitable for a generated parser. Please see the basic example in #53.
I have not yet come up with a concrete way to do it, but by using such as codespan or ariadne along with ParolError, we can generate fancy errors without miette. I assume there is a way to stop using miette while keep the current easiness. This way we can drop mierre_support.

[jsinger67]

I'm not sure if this can work.
Actually I was trying to go exactly this way.
But which I didn't get any further with was the fact that a generated parser must also be able to return user errors, not only errors from parol_runtime.
parol's own binary, is an example for this. It returns both, error types from parol_runtime and its own ParolParserError (crates/parol/src/parser/errors.rs). And I believe that most users want to return their own error type from parse, too.
Therefore I decided to wrap this error in an opaque error type - here anyhow.

Maybe this is not optimal but currently I have no idea how to solve this (perhaps by some clever template mechanisms). But I always strive for the simplest solution.

I agree with you, that we could achieve good error reporting with other libraries, like codespan or ariadne, which look indeed very promising. I will have a closer look at both of them.

[ryo33]

I think we have a way to do that.

#[derive(Error, Debug)]
pub enum ParolError<UserError: std::error::Error> {
    #[error(transparent)]
    ParserError(#[from] ParserError),
    #[error(transparent)]
    LexerError(#[from] LexerError),
    #[error(transparent)]
    UserError(UserError),
}

Pros

• Strongly typed
• We can implement From<T> for ParolError<T>, and users can use ? for returning a user-defined error.
• We can remove anyhow from parol_runtime's dependencies.
• A user can use anyhow like ParolError<anyhow::Error>.

Cons

• ParolError<BasicError> is slightly annoying
  (We can use an alias like type ParolError = parol_runtime::ParolError<BasicError>).
• User must be implement From<BasicError> for ParolError<BasicError> to use ? operator.

[ryo33]

I've updated #53 with the above idea. The code generation is not updated yet, and it needs a new parameter user_error like user_type_name.

[jsinger67]

Ok, I see.
UserError type is a generic type parameter of the parol_runtime::ParolError.
This should work.
The need to implement From<BasicError> for ParolError<BasicError> is acceptable I think and can be documented.

What I'm rather afraid of is that this could destroy the rapid prototyping behaviour which guarantees that a generated parser works out of the box as an acceptor for its grammar without user written code.
So there should be a way to opt this in. Maybe when no user error type is provided the parol_runtime::ParolError will not contain an enum variant for UserError. This would have the advantage that the current behaviour stays intact. Most examples should work and only advanced examples that provide an own error type have to change their parol build configuration.

I think we should head in this direction.
We can agree on who does which part. I could take over this task if you like.

[ryo33]

How about usinguser_error = () as the default? I suppose that user-defined errors will be needed after development has progressed. Or we can use anyhow::Error, but since it seems that std::error::Error is not implemented for it, it will not work as it is now, so a little work is required. Or we can generate an empty enum (or single variant enum withOther(#[from] anyhow::Error) variant) in parol new.
No problem if you take over.

[ryo33]

Instead the generics, UserError(#[from] anyhow::Error) makes the entire code simple. It does not requires the new parameter and helps rapid prototyping. Users can return any error type with ? operator, and no need to implement From for these errors (maybe) we still need to implement From maybe.

[jsinger67]

As a first step we can resort to anyhow::Error for user defined error types. This way the #[from] macro should be able to generate the From trait for it.
If the need arises to bring a more specific user error type into the generated code we can introduce a new parameter in a later step.
I will take over this now so you can focus on your own project.
Thanks alot for your valuable feedback. 👍

[ryo33]

Thank you too!

[jsinger67]

@ryo33
There are new changes on jsinger67/parol:structural-error

I've decided to use codespan_reporting for error reporting in parol's binary. Therefore I created a new crate error-report to separate error reporting. All other libraries are agnostic regarding the actual error reporting approach.
At the beginning I fiddled a bit with ariadne which seems to create slightly nicer reports. But I needed to be able to create reports without any relation to source code (e.g. for errors like parol_runtime::LexerError::DateError - "Error in generated source") which you simply can't do with ariadne. With codespan this was no problem.

To separate error reporting into a new crate I had to extract parol's error data types into a new library crate parol-errors to avoid cyclic references.

In the error-report crate I provide a way for users to plug in error reporting for their own error type.
You can see this in the example basic_interpreter (and of course in parol's binary). The function error-report::reports::report_error takes an Option<&UserErrorReporter> which has an underlying function type. This function can provide the reporting for the anyhow::Error wrapped in ParolError::UserError. For parol's binary this function is error-report::reports::parol_error_reporter, for basic_interpreter it is basic::errors::basic_error_reporter.

Maybe I could make this more idiomatic by defining a trait for the reporting and the user reporting plugin function. This will be a next step.

Currently I want to bring this error story to a state where we can build on for the foreseeable future.

If you have time to have a look at these changes your feedback would be highly appreciated.

For the Hihaheho/Desk issue #50 I want to be able to reference a version of parol where those decisions are made.

Edit: I made error reporting more idiomatic by defining a trait Report for the reporting and a struct ErrorReporter that implements this trait.

[ryo33]

@jsinger67
I think it's great!

I have subtle suggestions:

1. to add fn report_user_error in the Report trait with default blank implementation. Also, fn report_error could have the default implementation with codespan_reporting. Each crate has its own error reporter that implements Report like ParolErrorReporter for parol and BasicErrorReporter for basic_interpreter. Thus, we can remove Option<&UserErrorReporter> from the interface. It is also possible to extract report_parser_error and report_lexer_error from report_error for interface consistency, but this does not benefit the user and might not be required.
2. to define ParolErrorReporter in the parol crate like BasicErrorReporter is in the basic (if possible). It feels more consistent and, although trivial, reduces extra (logical) dependencies for runtime, and we don't need the parol-error crate.
3. to move the Report trait definition into parol_runtime (ideally with the "report" feature). It's easy for users.

[jsinger67]

Yeah, good points 👍
Then I can put codespan_reporting as dependency of parol_runtime and re-export it.
This will remove the need of both new crates, parol-errors and error-report.

[jsinger67]

I incorporated all your suggestions and now the overall handling is much better.
Also the support for user error reporting is much clearer now.

[ryo33]

Thanks a lot!

[jsinger67]

I released new versions on crates.io of parol-macros, parol_runtime, parol and parol-ls today.
Hence I would close this issue.
As always, don't hesitate to contact me if you have any suggestions or questions.
Thanks a lot for your contributions! 💯