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

Multiple errors #222

Open
jamesdbrock opened this issue Apr 4, 2023 · 7 comments
Open

Multiple errors #222

jamesdbrock opened this issue Apr 4, 2023 · 7 comments

Comments

@jamesdbrock
Copy link
Member

Should we have multiple errors like in Megaparsec? https://hackage.haskell.org/package/megaparsec/docs/Text-Megaparsec-Error.html#t:ParseErrorBundle

@Violet-Codes
Copy link

Violet-Codes commented Apr 4, 2023

I would start this with an "IMO" ("In My Opinion") but I think this is more then an opinion...

Parse errors without context simply do not contain enough information content, especially in recursive parsing where you face arbitrary depth and are not able to represent more and more of the necessary information.

Also the system for bundling, skipping and recovery does not have to be the same; I propose something that lets the user fully handle multiple error parsing that keeps the library simple and easy to use:

-- new combinators at the minimum
recoverWith :: ParserT s m a -> ParserT s m Unit -> ParserT s m (Either ParseError a)
bundleErr :: ParserT s m (Either ParseError a) -> ParserT s m a

-- some potentially easier way to combine errors, fail if any not present, list all errors
-- The binding of (<*>) with the error combination of (<|>)
(<*?>) :: Either [e] (a -> b) -> Either [e] a -> Either [e]

-- type change
data ParseError
    = ParseError String Position
    | ParseContext String Position ParseError -- +
    | ParseBundle String Position [Parse] -- +

This is only going to be breaking for projects that are matching on ParseError, which is going to be some projects however its not going to be a large refactor even in that case.

@JordanMartinez
Copy link
Contributor

I'm all for better error messages. However, don't better errors typically mean slower parsing?

@Violet-Codes
Copy link

I'm all for better error messages. However, don't better errors typically mean slower parsing?

Not at all! ParseContext just wraps all errors in a section which is an incredibly cheep action,
bundleErr just raises the error if its present and otherwise succeeds
and recoverWith will only fire the recovery parser on a failure, and as for the recovery parser, these things are usually incredibly simple, aggressively so.

This would not cause any noticeable performance hit. 👍

@JordanMartinez
Copy link
Contributor

No offense but I'd be more convinced by a benchmark than a claim like above.

@Violet-Codes
Copy link

Violet-Codes commented Apr 4, 2023

I do not know how better to tell you that these operations that do simple manipulations on the ParseError type are trivial from a performance perspective, the only thing that effects performance is recoverWith, however:

  • It changes the behavior so if that tradeoff exists it can be justified.
  • It will only fire if the original parser errors.
  • These parsers are, by design, incredibly simple and that usually means performant. (e.g. next ; at same {} level)

@jamesdbrock
Copy link
Member Author

If you want to try to write a multiple-error feature @Violet-Codes then I would like to try to add that feature to parsing. But I don't want to waste your time so let's try to make sure that what you plan to implement has a good chance of getting merged.

Like Jordan says, we have benchmarks for parsing, and we'll probably need to add some more benchmarks to make sure that the new error machinery doesn't cause too much slowdown.

We also would like to make sure that the new error machinery is simple for the simple case, like you said, for

  1. people who are encountering monadic combinator parsing for the first time
  2. people who will have to upgrade to the breaking change

Can you point me to an example of a library (or a paper or a blog post) which you think describes multiple errors correctly? Maybe Megaparsec, or something else?

Another thing to think about... maybe if we're going to improve the ParseError type then we should improve the Indents module at the same time? #172

@jamesdbrock
Copy link
Member Author

Also if we're going to do multiple errors then maybe we should do lazy error messages at the same time see #158

This was referenced Apr 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

3 participants