search

haskell / mtl

The Monad Transformer Library
http://www.haskell.org/haskellwiki/Monad_Transformers
Other
362 stars 63 forks source link

Support Control.Monad.Trans.{Writer,RWS}.CPS from transformers 0.5.6 #67

Closed lexi-lambda closed 5 years ago

lexi-lambda commented 5 years ago

transformers-0.5.6.0 adds new Control.Monad.Trans.Writer.CPS and Control.Monad.Trans.RWS.CPS monad transformers, so this PR adds support for them in mtl.

I’ve taken the liberty of bumping the version to 2.3, since this seems like a significant enough change to do so, and picking what the next release’s version number will be was necessary for the @since annotations in the docs. Let me know if you’d like me to do something else, and I’ll change it.

fishtreesugar commented 5 years ago

It could fix #38

chessai commented 5 years ago

At a glance, this looks great! But could you remove the version bump? That's something that's typically done prior to a release, and not based on any one contribution alone.

lexi-lambda commented 5 years ago

@chessai Sure, I’ve removed the version bump. I’ve left the Haddock annotations alone, so they still say @since 2.3, since I’m not sure what else to do with those. Let me know if there’s something better.

lexi-lambda commented 5 years ago

@chessai Pinging you about this. Is there anything in the way of getting this merged?

chessai commented 5 years ago

Thanks for the ping. I see one problem, currently: The lower bound of the transformers dependency has been bumped from 4 to 5.6. It's possible that this could disrupt some users. IMO we should keep the old bounds and wrap all this code in CPP.

chessai commented 5 years ago

Also keeping the since pragmas as you currently have them is fine, IMO.

lexi-lambda commented 5 years ago

I’m happy to put the CPP in if you think the version bump on transformers is too much, but I admit I don’t personally see why it would be worth it. transformers has effectively zero dependencies. In what scenario could someone upgrade to a newer version of mtl but couldn’t upgrade to a newer version of transformers?

chessai commented 5 years ago

Personally i don't see a huge issue. Perhaps @hvr or @RyanGlScott could weigh in here?

lexi-lambda commented 5 years ago

In the absolute worst case scenario, if people complain, we could always release a version with the CPP and relaxed bounds after the fact… it isn’t like releasing a version with the tighter bounds locks us into them or anything.

RyanGlScott commented 5 years ago

I think I'm in favor of CPP as well. transformers is a GHC boot library, which means that any build plan that depends on GHC-the-library must use the version of transformers bundled with GHC itself. Combine this with the fact that transformers-0.5.6.0 was only bundled with GHC starting with 8.6 and you can get yourself into situations where the only way to construct valid build plans is to upgrade GHC itself, which is often much more painful than installing a newer version of a library.

This is not to say that we should continue to support all versions of transformers indefinitely—I think it would make sense to drop this CPP at some point in the future. As things stand currently, however, you can end up having build plans that only support GHC 8.6 or 8.8, which isn't as wide of a support window as I'd like for such a widely used library.

chessai commented 5 years ago

I second ryan's comment, the CPP is needed, at least for now.

lexi-lambda commented 5 years ago

transformers is a GHC boot library, which means that any build plan that depends on GHC-the-library must use the version of transformers bundled with GHC itself.

Maybe I’m misunderstanding some distinction, but isn’t mtl also a boot library, as of GHC 8.4? The list of modules at https://downloads.haskell.org/~ghc/latest/docs/html/libraries/index.html seems to include modules from mtl… but on the other hand, https://hackage.haskell.org/package/ghc-8.6.5/dependencies lists a dependency on transformers, as you describe, but not on mtl. So I guess there’s a difference between them I’m not aware of.

In any case, what you said is compelling, so I’m happy to add the CPP. I do have one question, though: is there any way to conditionally include an entire module in exposed-modules based on the version of a library? As far as I can tell, there isn’t, so should I just make Control.Monad.Writer.CPS an empty module on unsupported transformers versions? That seems a bit confusing, but if it’s the only way, I’ll do it.

RyanGlScott commented 5 years ago

Maybe I’m misunderstanding some distinction, but isn’t mtl also a boot library, as of GHC 8.4?

Good point, I was somewhat vague in my use of the phrase "boot library" earlier. Both mtl and transformers are bundled with GHC itself, since various libraries (e.g., Cabal) depend on them. GHC-the-library depends on transformers, but not mtl, so in this sense transformers is "more wired in" than mtl is.

As far as I can tell, there isn’t, so should I just make Control.Monad.Writer.CPS an empty module on unsupported transformers versions?

Yes, that should be fine. Do make sure to leave a mention of this somewhere in the surrounding Haddocks.

lexi-lambda commented 5 years ago

Good point, I was somewhat vague in my use of the phrase "boot library" earlier. Both mtl and transformers are bundled with GHC itself, since various libraries (e.g., Cabal) depend on them. GHC-the-library depends on transformers, but not mtl, so in this sense transformers is "more wired in" than mtl is.

Got it, thanks for the explanation!

Yes, that should be fine. Do make sure to leave a mention of this somewhere in the surrounding Haddocks.

Alright, I’ve pushed an updated set of changes that does that.

RyanGlScott commented 5 years ago

Thanks!