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

Update from Megaparsec 6.5 #368

Closed
jpdoyle opened this issue Sep 16, 2018 · 7 comments
Closed

Update from Megaparsec 6.5 #368

jpdoyle opened this issue Sep 16, 2018 · 7 comments

Comments

@jpdoyle
Copy link

jpdoyle commented Sep 16, 2018

Currently hnix depends on megaparsec >=6.5 && <7.0, and when running nix-shell I encountered a quickcheck error indicating a bug fixed in megaparsec 7.0 (mrkkrp/megaparsec#323)

I'm not sure which things depend directly on megaparsec (I was doing this installation to just get started contributing), but since this bug is supposedly fixed in 7.0 that should be our target.

@jwiegley
Copy link
Member

I'd be quite happy to upgrade to Megaparsec 7.0.

@Gabriella439
Copy link

I took a stab at this and it's a bit tricky. The main challenge is that megaparsec-7.* changed ParseError to store Int instead of NonEmpty SourcePos which in turn requires refactoring the annotations that are threaded around hnix

@jwiegley
Copy link
Member

@Gabriel439 Thanks for the attempt, Gabriel! Is there any other way to recover that source position information when an error happens? I wonder if the author removed that information to avoid storing something that most people weren't using...

@Gabriella439
Copy link

@jwiegley: I believe it's mainly for performance reasons. This post provides more context for the motivation for the change:

https://markkarpov.com/post/megaparsec-more-speed-more-power.html

The changelog is also pretty detailed, although I haven't read it thoroughly yet:

http://hackage.haskell.org/package/megaparsec-7.0.1/changelog

What I did notice, though, is that the utilities for pretty-printing ParseError actually ignore that Int field now:

http://hackage.haskell.org/package/megaparsec-7.0.1/docs/src/Text.Megaparsec.Error.html#parseErrorTextPretty

My guess is that either:

  • The source position information can be recovered from the Int (perhaps using attachSourcePos)
  • The source position information can be recovered from other fields within the same constructor

@mrkkrp
Copy link

mrkkrp commented Oct 16, 2018

Hi, I haven't looked at the code, but if you want to have SourcePos as before then get it using getSourcePos from a running parser. Alternatively you could use Int offsets and recover SourcePos using attachSourcePos.

The users are mostly expected to work with ParseErrorBundles now, not with ParseErrors directly.

What I did notice, though, is that the utilities for pretty-printing ParseError actually ignore that Int field now

Not quite true, parseErrorTextPretty always ignored that. Look at parseErrorPretty and you'll see that it print the Int as "offset".

If you have any questions please ask, I'm interested to help with the transition. (Because if it doesn't work well for you, then version 7 sucks!)

@domenkozar
Copy link
Collaborator

Fixed by #380

@domenkozar
Copy link
Collaborator

Fixed in master.

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

No branches or pull requests

5 participants