search

codedownio / aeson-typescript

Generate TypeScript definition files from your ADTs
BSD 3-Clause "New" or "Revised" License
60 stars 27 forks source link

Dispose of Unnecessary OVERLAPPING Pragmas and Deprecated OverlappingInstances Extension #16

Open prednaz opened 4 years ago

prednaz commented 4 years ago

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

thomasjm commented 4 years ago

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 commented 4 years ago

@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 commented 4 years ago

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

thomasjm commented 4 years ago

@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 commented 4 years ago

@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 commented 4 years ago

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 #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 commented 4 years ago

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.