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

Dispose of Unnecessary OVERLAPPING Pragmas and Deprecated OverlappingInstances Extension #16

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

prednaz
Copy link

@prednaz prednaz commented Mar 8, 2020

By the way, is it my setup or are the tests broken?

@thomasjm
Copy link
Contributor

Hey, thanks for the PR. The tests pass for me locally but seem to be broken on Travis. Not sure why, I've been doing some work on master recently. You need to have npm installed for them to work.

I'm of two minds about this--I like how this avoids the need for OVERLAPPABLE/OVERLAPPING, but I'm not sure it's worth cluttering the core typeclass with getListTypeScriptType/getListParentTypes. Given the choice between the two I'd probably choose the abstruseness of the former over the complexity of the latter. Were the pragmas causing problems for you somehow?

@thomasjm
Copy link
Contributor

@prednaz friendly ping on my question about the pragmas. I'm still not quite sure what to do about this PR -- I might rebase it on #17 after that lands but I wanted to better understand your reasoning for getting rid of the pragmas. Thanks!

@hasufell
Copy link

The main thing is getting rid of OverlappingInstances, which is deprecated. the OVERLAPPABLE pragmas should be safe enough.

@thomasjm
Copy link
Contributor

@hasufell well as the comment in the file mentions, OverlappingInstances is there only to support older GHCs (like <7.8.4). Maybe we can just drop support for those.

@prednaz
Copy link
Author

prednaz commented Apr 15, 2020

@thomasjm , I am very sorry for having kept you waiting. No, the pragmas were not causing me any problems. I opened the pull request just to demonstrate to me and a colleague that in some situations it is possible to avoid overlapping instances completely and to discuss the options.

I, myself, think that the option this pull request puts forward composes poorly because it requires changing the class in order to define instances which a library user could not do. On the other hand, overlapping instances entails a few dangers. One example is the incoherent behavior described in the warning section at the bottom of https://downloads.haskell.org/ghc/latest/docs/html/users_guide/glasgow_exts.html#instance-overlap . But as far as I can see, users of your library could never trigger and observe this one. But I have not looked very closely.

@thomasjm
Copy link
Contributor

Thanks @prednaz ! From what I can tell, the warning you reference applies only to OverlappingInstances and not to the newer OVERLAPPING pragmas. So, it seems like all we really need to do is remove/contain the deprecated pragma. I'm thinking one of the following solutions:

  1. Just get rid of OverlappingInstances and drop support for the older GHCs that need it.
  2. Gate the OverlappingInstances pragma behind a CPP #if check so it's only active for said GHCs. I've never tried to gate a pragma before but it should work.
  3. Land this PR after rebasing on Various bug fixes and improvements. #17 (which I'd like to land more urgently).

I'm leaning toward 1) or 2) but I'll revisit this after the other PR + a little research on what newer GHC adoption looks like.

@prednaz
Copy link
Author

prednaz commented Apr 16, 2020

From what I can tell, the warning you reference applies only to OverlappingInstances and not to the newer OVERLAPPING pragmas.

It applies to the pragmas too. Almost all dangers of OverlappingInstances apply to the pragmas too because they are strictly more powerful.

I personally do not think, using a deprecated feature is worse than losing the support of older GHC versions. But that is clearly your decision.

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.

3 participants