VinylRecords / Vinyl

Extensible Records for Haskell. Pull requests welcome! Come visit us on #vinyl on freenode.
http://hackage.haskell.org/package/vinyl
MIT License
260 stars 49 forks source link

Fix building on GHC 8.8 #130

Closed mstksg closed 4 years ago

mstksg commented 4 years ago

GHC 8.8 changes the way that kinds are quantified with respect to -XTypeApplications. Now, for a typeclass like

class MyClass (f :: k -> *) a  where
  myMethod :: f a

myMethod will now expect k as an argument with TypeApplications, myMethod @k @f @a, even if it isn't explicitly listed as a parameter of the typeclass.

Because of this, a lot of internal code will no longer work because many typeclasses mention a kind variable without explicitly bringing it as a TC parameter. This also changes the external API in a fundamental way.

There are two ways to fix this, I think:

  1. Keep the typeclasses the way they are -- their external API's will now be different between 8.6 and 8.8, and we have to use CPP on type applications or clever type annotations to make code backwards compatible
  2. Change the typeclass definitions to no longer explicitly mention k. This preserves the external API, and is backwards compatible from an external sense, at the cost of documentation being a little less clear.

This PR implements the second way. Let me know if there's anything I can add, or if you decide to go down another route :)

acowley commented 4 years ago

Thank you! Ignorant question: does this preserve poly-kindedness? It was written that way because I think it was (at the time) necessary if you wanted to have indices at kinds other than *.

acowley commented 4 years ago

And woof does the TravisCI configuration need updating. That's a bag of uselessness at the moment.

mstksg commented 4 years ago

Definitely not a bad question -- it's something that I hadn't thought to check myself!

It looks like polykindedness is preserved for the most part from GHC 8.0 and onwards (the library as a whole doesn't build before this, because TypeApplications was not yet supported); I tested this for RecElem for non-* kinds.

However, one typeclass is an exception. IsoHKD still needs the kind signature for GHC 8.0 and GHC 8.2 to define instances. So, to go along the path that this PR sets upon, either you can:

  1. Drop support for GHC 8.2 and below by removing the kind signature for the IsoHKD typeclass. This is the state as of 779cf3f522bb5be82e66a96d8d8819517cdddf2c .
  2. Keep the kind signature for the IsoHKD typeclass, but then 8.6 and 8.4 will have a breaking external API change. However, this doesn't seem to be too bad in the case of IsoHKD, since people probably would not be using its methods directly, I think.

    (At least, the library will still build internally because this doesn't affect anything it currently uses internally)

acowley commented 4 years ago

Dropping <= 8.2 is okay especially now that 8.8 is out. I’ll merge this later today or tomorrow when I get a chance to build, if that’s okay.

I’d like to try to fix up the CI situation; I think there’s some code that exercises the poly kinds in a test or example somewhere that would have made this easier to verify. I will get to it today or tomorrow.

mstksg commented 4 years ago

Of course, no rush :) Yeah, if the polymorphism is still preserved, the main 'downside', i think, is the documentation being different. Let me know if there's anything else I can help fix up with these compatibility changes.

acowley commented 4 years ago

Thank you for the help with this!

mstksg commented 4 years ago

Glad to have been able to help! :)

mstksg commented 4 years ago

Ah, it might be worth changing the cabal metadata for 0.11.0.0 on hackage to limit base < 0.14, to help the solver pick the right versions for different GHC's. That way people can leave have their vinyl version bounds (and be wider than just >= 0.12) be independent of GHC version (so without requiring if/then/else cabal stanzas).

mstksg commented 4 years ago

Ah, also the base bounds for the new version should also be >= 4.11 -- sorry for not updating them!

acowley commented 4 years ago

Can you clarify what you mean for the upper bound on base for vinyl-0.11.0.0? Did you mean base < 4.14 as a precautionary bound, or should we make it base < 4.13 to prevent an attempted build with GHC-8.8?

mstksg commented 4 years ago

Ah yes, sorry, that was a numerical error. base < 4.13, to prevent an attempted build with GHC 8.8.