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

Misc refactors to the Elm compiler's parser integration code #748

Merged
merged 12 commits into from
Aug 21, 2021

Conversation

emmabastas
Copy link
Contributor

Apart from small changes and improvements there is one notable refactors in this PR

Refactor the parsec adapter's error type


elm-format doesn't report errors anyways, why care?

  • Good error reporting can aid in debugging.
  • We might want to report errors in some context (jsonAST?) at some point, at least let's not actively dismantle existing error reporting infrastructure.
  • Great effort has been expended to give the Elm compiler good error reporting infrastructure, it would be a shame to not utilize it.

Parsec errors are essentially strings with some additional metadata. The Elm compiler on the other hand defines all possible parse errors as custom types inside Reporting.Error.Syntax. With this approach no information is lost when a parse error is propagated from the lowest level parser through the higher level ones. The higher level ones just construct higher level errors with the lower level errors as children.

393c7ee redefines parsecs error type. Instead of a single string with metadata it's now possible to create a tree structure, kind of like a generic/untyped version of the Elm compilers errors, it looks like this:

data ParseError
= Nil
| Cons Message Row Col ParseError
| OneOf ParseError ParseError

data Message
= Message !String -- raw message
| Expect !String -- expecting something
| UnExpect !String -- unexpected something

9770e47 Integrates the parsec errors with the Elm compilers errors. Additional leaf variants are added to Message so that the error types from the compiler parsing code can be added as children to a parsec error without first needing to convert to a string.

What the Message type looks like now: (note that String, Char and Number refer to custom error types)

data Message
= Message !Prelude.String -- raw message
| Expect !Prelude.String -- expecting something
| UnExpect !Prelude.String -- unexpected something
| CharError Char
| StringError String
| NumberError Number

@emmabastas emmabastas marked this pull request as ready for review August 17, 2021 00:52
errorPos err =
case err of
Nil ->
error "An unexpeced error occured, this is likely a bug. Please report this issue at https://github.com/avh4/elm-format/issues"
Copy link
Contributor Author

@emmabastas emmabastas Aug 17, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

errorPos crashes on empty error. I realize that it's possible to define ParseError to make impossible state impossible. But since Nil isn't exposed outside this module and since it's never used to construct an empty error I figured this was ok. That said I'm not opposed to changing this

@avh4 avh4 self-requested a review August 20, 2021 05:39
@avh4
Copy link
Owner

avh4 commented Aug 21, 2021

This all looks good. And some of these were pretty big cleanups -- thanks!

@avh4 avh4 merged commit 724861e into avh4:new-parser-2021 Aug 21, 2021
@avh4 avh4 removed their request for review August 21, 2021 08:31
kutyel pushed a commit to kutyel/elm-format that referenced this pull request Apr 26, 2022
Misc refactors to the Elm compiler's parser integration code
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.

None yet

2 participants