Skip to content
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

Role of ErrorKind and intent of the variants? #697

Closed
epage opened this issue Jan 14, 2025 · 10 comments · Fixed by #722
Closed

Role of ErrorKind and intent of the variants? #697

epage opened this issue Jan 14, 2025 · 10 comments · Fixed by #722
Labels
A-error Area: error reporting C-question Uncertainty is involved M-breaking-change Meta: Implementing or merging this will introduce a breaking change.
Milestone

Comments

@epage
Copy link
Collaborator

epage commented Jan 14, 2025

Some early expectations of ErrorKind from nom

  • Passed back through FFI to C APIs
  • iirc Infer intent from &[ErrorKind], inspired by another parser library (can't find where I saw this written)

Today in Winnow, none of these apply and the default error type drops ErrorKind. It is hard to infer intent from ErrorKind because which one is reported is an implementation detail in a lot of circumstances.

Also, ErrorKind variants are focused on what operation failed, instead of describing the failure

  • This is confusing (e.g. I keep wanting to use ErrorKind::Eof when an eof is found but its for when an eof isn't found)
  • It doesn't scale beyond built-in parsers and should not be used as a reason to add a built-in parser
  • AddContext can serve a similar role, though isn't applied to built-in parsers

Where should ErrorKind go?

@epage epage added M-breaking-change Meta: Implementing or merging this will introduce a breaking change. A-error Area: error reporting C-question Uncertainty is involved labels Jan 14, 2025
@epage epage added this to the 0.7.0 milestone Jan 14, 2025
@epage
Copy link
Collaborator Author

epage commented Jan 14, 2025

One idea is to remove them completely.

The other is to shift the focus to describing the failure and to focus on the core principles of why something fails:

enum ErrorKind {
    // For `fail`
    Failed,
    // Use for `one_of`, `take_while`/`repeat` ending early besides eof, `verify`, `try_map`
    Invalid,
    // Used for operations that hit `eof` when data was expected, including `complete_err`
    EofFound,
    // Parser pre-condition was not met
    Assert,
}

@Nukesor
Copy link
Contributor

Nukesor commented Jan 14, 2025

That's a pretty good question.

In my mind, it kind of depends on how a user wants to use the library.

One scenario where an ErrorKind might be useful would be when one is trying to implement some kind of error recovery strategy.
Having an ErrorKind that tries to explain the reason for a failure might allow you to perform a better guess on where to jump to, to skip the broken input and continue parsing afterwards.

Personally, I don't really need the ErrorKind yet. I simply had to create a backtrack error by hand, which required any ErrorKind. I decided to just use the Verify kind in the end, as non of the existing ones matched my exact usecase.
So if ErrorKind were to be rewritten, the possibility for a user provided ErrorKind in the form of Custom(String) could be awesome, as that could be used to easily provide some context for error handling.

FYI, I'm currently writing parsers for hand-crafted and very poorly structured file formats in the scope of Arch Linux package management. (Over here in case you're interested :D)

My usecase is that those file formats have no "nice" indicators/syntax on when a certain section is finished, which requires me to peek ahead and throw a backtracking error if I find a new section so that I can escape the current repeat parser and continue with the next section.

@Nukesor
Copy link
Contributor

Nukesor commented Jan 14, 2025

To be honest, error handling in winnow is one of the last things that haven't really clicked for me yet.

I'll probably spend a few hours soon to add good error handling to those libraries, maybe I'll finally understand how to do proper error handling with winnow 😅

@epage
Copy link
Collaborator Author

epage commented Jan 14, 2025

So if ErrorKind were to be rewritten, the possibility for a user provided ErrorKind in the form of Custom(String) could be awesome, as that could be used to easily provide some context for error handling.

Wouldn't AddContext serves that role?

@Nukesor
Copy link
Contributor

Nukesor commented Jan 15, 2025

Good point. I forgot about that!

@Nukesor
Copy link
Contributor

Nukesor commented Jan 27, 2025

Ok. I think I got a pretty good grasp on how to do proper backtracking and error handling now.

In general, I really don't see the need for the current ErrorKind.
The only thing it could be useful for is error recovery, but even than, it's not very descriptive.


I took some time and tought about error recovery specifically and how this would look in winnow on a library level. I guess to have something like error recovery, it would need to be properly baked into winnow. For this to be ergonomic, there would need to be some form of error aggregation mechanic and a dedicated "We found an error, seek to this position and continue with the repeat/take_while" error type, combined with some functionality to convert a backtracking error to that recoverable error in combination with a (fallible) recovery strategy (e.g. seek to next }, but not if we encounter a { first.).

Considering that this would likely require a significant restructurization of winnow's internals, I think that the ErrorKind can be dropped.

IMO error recovery would be a really nice touch, but this is a different topic :)

@epage
Copy link
Collaborator Author

epage commented Jan 27, 2025

We are tracking error recovery in #96 which has an unstable implementation that includes

@Nukesor
Copy link
Contributor

Nukesor commented Jan 27, 2025

Haha, perfect.
In that case, there's really nothing left to desire! Much appreciated.

Let's kick ErrorKind out then :)

@snaggen
Copy link

snaggen commented Jan 28, 2025

I think the Errors are the most confusing thing about nom/winnow. Some issues I have with it is:
The input type is part of the error, hence methods like parse parse_next throw different errors that are incompatible.

Also, looking at the parser example in https://docs.rs/winnow/latest/winnow/_topic/partial/index.html
you have an Error type E: ParserError<Stream<'i>> + AddContext<Stream<'i>, StrContext> . My first thought was to simplify it by using type, but that doesn't work easily due to reasons. And when you have worked with these for a while, copying and pasting this around you suddenly need to throw an external error, just to realize that E doesn't have from_external_error... so a manual error you think, but then there is no way to provide any information.

And I'm sure there are ways to handle my issues, since I'm quite new at winnow, but the rest of the framework feels quite intuitive so the Errors feels very like a mess.

So, I'm happy to see that you are looking into this area, and I hope you find a way to simplify it. And maybe just some better documentation explaining how the Error handling is meant to work.

@epage
Copy link
Collaborator Author

epage commented Jan 28, 2025

@snaggen thank you for sharing your experience! Understanding how people are approaching Winnow and dealing with problems is a big help in figuring out how to improve it. Could you start a Discussion on that topic? It looks different enough from this issue and there are enough points to cover, that having a dedicated, threaded conversation would help in finding how to help solve those problems.

@epage epage closed this as completed in d88c56b Jan 29, 2025
epage added a commit to epage/winnow that referenced this issue Jan 29, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-error Area: error reporting C-question Uncertainty is involved M-breaking-change Meta: Implementing or merging this will introduce a breaking change.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants