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

Problematic IsString instance #515

Open
j-mie6 opened this issue Mar 2, 2023 · 4 comments
Open

Problematic IsString instance #515

j-mie6 opened this issue Mar 2, 2023 · 4 comments

Comments

@j-mie6
Copy link

j-mie6 commented Mar 2, 2023

Megaparsec defines an instance:

instance (a ~ Token s, IsString a, Eq a, Stream s, Ord e) => IsString (ParsecT e s m a)

Which allows for convenient syntax for parsing string symbols in a parser. However, the existence of this instance means that it is not possible to define a specialised instance for use with lexing logic (see this paper, page 8, section 3.3) without resorting to newtype hackery. This is arguably a more useful instance to have, because it can actually be used to clean up the logic of a completed parser.

Obviously, the issue here is that the instance is given in the same module as ParsecT's definition (as to not orphan it), but I would suggest actually orphaning it in a module where it is the only thing inside. That way, users can opt into this (indeed very sensible) default, and remove the import when they want to define their own specialised version. If orphaning really is to be avoided, I'd instead just recommend removing it.

In either case, this is a backwards incompatible change.

Keen to know your thoughts!

@mrkkrp
Copy link
Owner

mrkkrp commented Mar 2, 2023

Orphan instances is a source of all kinds of problems. On the other hand, defining a newtype is one of the most common idioms in Haskell.

@j-mie6
Copy link
Author

j-mie6 commented Mar 2, 2023

Fair enough! In which case, I would advocate for the instance to either be removed, or instead placed behind a newtype itself. Library authors should, for sure, not create an orphan (as to not impact downstream users), but downstream end-users should be free to if they wish: it avoids a whole bunch of unnecessary boilerplate.

@mrkkrp
Copy link
Owner

mrkkrp commented Mar 2, 2023

So, you are requesting a backward incompatible change that might break other people's code so that you could avoid writing "boilerplate" in the form of a single-liner defining a newtype?

@j-mie6
Copy link
Author

j-mie6 commented Mar 2, 2023

Backwards incompatible changes by definition break code: I wouldn't be saying do it immediately, but if you're making a different breaking change that already necessitates changes in user code, then I'd be in favour of removing the instance at the same time. I.e. whenever you'd have otherwise made megaparsec 10.

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

2 participants