ndmitchell / cmdargs

Haskell library for command line argument processing
Other
91 stars 12 forks source link

Added Semigroup instances, in preparation for base 4.11 #47

Closed ElvishJerricco closed 6 years ago

ElvishJerricco commented 6 years ago

Base 4.11 will have Semigroup as a superclass of Monoid

https://github.com/ghc/ghc/commit/8ae263ceb3566a7c82336400b09cb8f381217405

ndmitchell commented 6 years ago

Thanks!

ElvishJerricco commented 6 years ago

@hvr pointed out a slightly better way to do this. @ndmitchell, would you like me to make the same change for this?

hvr commented 6 years ago

Basically, the reason for doing it as recommended on the wiki is that the way #47 was done will break again once the semigroup proposal reaches the final stage; whereas the recommended way was designed to be future-compatible (and that's encoded in the -Wcompat warning) and allows to avoid CPP in (most) cases.

ElvishJerricco commented 6 years ago

@hvr how will it break once he semigroup proposal reaches the final stage?

hvr commented 6 years ago

@ElvishJerricco when mappend moves out of Monoid and becomes an ordinary top-level function (as a compat shim). Then then GHC will tolerate only leaving off the mappend = ... definition, or using the "canonical definition" (mappend = <> which will basically be ignored by GHC as if it wasn't there; everything else will trigger a compile error); that's the whole idea behind the migration scheme encoded in those -Wnoncanonical-*-instances warnings (we have 3 of those; for Monoid, Monad & MonadFail).

ndmitchell commented 6 years ago

I'm happy with it as is for the moment - we can add more CPP in future versions when it's required. I also switched to using __GLASGOW_HASKELL__ rather than the other macros, as that means it works directly in GHC, and doesn't require Cabal.

@ElvishJerricco Would it be useful to have a release with this change included?

hvr commented 6 years ago

I also switched to using __GLASGOW_HASKELL__ rather than the other macros, as that means it works directly in GHC, and doesn't require Cabal.

You may not know this, but that's not true anymore :-)

Starting with GHC 8.0, GHC natively injects MIN_VERSION_xxx() macros. And at some point in the future we'll hopefully be able to break the __GLASGOW_HASKELL__ <-> base-version correlation, at which point __GLASGOW_HASKELL__ will cease to be a proxy for the base library version...

ndmitchell commented 6 years ago

@hvr Cool, that makes way more sense. Once I'm not supporting < 8.0 anymore for my packages I'll move to that (a long way in the future, but I suspect before you break the GHC base-version correlation!)

ElvishJerricco commented 6 years ago

Would it be useful to have a release with this change included?

@ndmitchell yes, I think so. But it's worth noting that if someone writes a library depending on these semigroup instances, that library will not compile with GHC < 8.0. Not sure if that's a dealbreaker or not.

ndmitchell commented 6 years ago

Released. I'll let people complain about the semigroup thing pre 8.0 as and when they want. Generally people who are using 8.0 or above aren't all that worried about going back too far.

hvr commented 6 years ago

But it's worth noting that if someone writes a library depending on these semigroup instances, that library will not compile with GHC < 8.0. Not sure if that's a dealbreaker or not.

Yeah, conditional APIs are quite annoying. It would help if cmdargs documentation would at least sign-post clearly in the instance-haddocks that a specific instance requires a certain minimum base version to be present so users can set their lower base bounds accordingly. I'm already worried a bit about the amount of .cabal Hackage revisions I'm going to have to make to fixup the fallout from packages defining conditional Semigroup instances.

PS: For an example, see the NFData instance for e.g. the TyCon type at http://hackage.haskell.org/package/deepseq-1.4.3.0/docs/Control-DeepSeq.html#t:NFData

ndmitchell commented 6 years ago

@hvr I changed my mind after fixing up all my other packages and seeing how easy the semigroup package solution was and reflecting upon your conditional instances notes (many of which have bit me!). I'll put out a new release shortly.