PVP adherence: Stop re-exporting from other packages (`base`)

[chessai]

• Stop re-exporting Control.Monad and Control.Monad.Fix from each non-*.Class module
• Stop re-exporting Control.Monad.IO.Class from Control.Monad.Trans
• Stop re-exporting Data.Monoid from Control.Monad.Writer.*

Resolves #68

cc @phadej @ekmett

[cartazio]

@chessai should it instead explicitly reexport the things we expect to be provided? (aka , reexport MonadIO etc, explicitily rather than whatever is in the whole module?)

cc @RyanGlScott @ekmett

[chessai]

Hmm @cartazio isn't that still in violation of PVP? Or am I mistaken?

[cartazio]

@chessai you can https://pvp.haskell.org/ see in the PVP, that its about "invisibly varying" reexports I think? Like if you did an explicit import list aka

import Foo (MyClass,myFun) as MyQFoo and then export module MQFoo,you avoid that problem? though maybe @hvr or someone else can opine. I could be missing something.

My concern is mostly, this alternative would a) fix the leaky exports issue and b) avoid the needlessly breaking stuff that was picking up those classes via the MTL reexports? i could be totally missing something though

[cartazio]

@chessai Alternatively, if we can grep all uses of MTL on hackage, for most recent releases on hackage, to determine what would be broken possibly, that would be fine too. Just its good to do mix of some sort of empirical what would be broken by this impact assement.

[chessai]

@cartazio i think that grep should happen at the very least. im about to divert my attention to something else for the rest of the day

[Bodigrim]

I tried three random packages and this change breaks all three of them (see PRs above). To be honest, I don't think it's reasonable to push this any further.

[Bodigrim]

Another option is to re-export not original functions, but wrappers with deprecation warnings like this:

fix :: (a -> a) -> a
fix = Data.Function.fix 
{-# DEPRECATED fix "Export fix directly from Data.Function" #-}

Still not sure whether this justifies the amount of work involved.

[jkachmar]

Per the recent discussion in #69, would this be worth the effort if mtl ends up being decoupled from base (and therefore no longer shipped with GHC as a core library)?

[ekmett]

@Bodigrim: Pushing wrappers will break even more folks in a much much worse way, because if you import them from the right place they'll collide, whereas no interim wrappers means you just add a couple of extra imports and you work across old and new versions of the library.

Without those shims they have a reasonably straightforward path to add a couple of imports to get things from the canonical locations. We'd put very loud warnings on the package, and folks would grumble and fix it.

But with them there is no option but to use qualified imports for every import of almost any module from the mtl, which is completely unacceptable.

The breakage from mtl 1 to mtl 2 was considerably worse than the breakage would be here with no mitigation. This would definitely be a major revision, but it'd fix one of the largest long standing warts in the core libraries.

[ekmett]

This item has been lurking near the top of my TODO list for years now. I do consider it worth suffering the disruption of a first-digit major release, and loud announcement process.

[Bodigrim]

@ekmett There are 3700+ reverse dependencies of mtl and 2300+ do not specify an upper bound. Removing re-exports, given an empirical evidence above, means that most of these packages would have to cut a new release, plus half of them would have to revise bounds in all previous releases. All for comparably small tangible benefit IMHO.

(This is why I suggested that ugly hack with wrappers: at least there would be no build errors. At a price of much more churn to fix deprecation warnings, agreed.)

I mean, this is probably a good showcase why upper bounds for core libraries are so important. One can start a preliminary campaign of preventive PRs (put upper bound + import stuff from home packages), probably.

[phadej]

I'd rather not add bounds pre-emptively. There are massive amount of packages lacking base upper bounds, but yet SMP and MonadFail (and even AMP) was solved without revising ~everything (we = hackage trustees - had to do plenty of revisions, but not everything without base upper bounds broke).

[ekmett]

(This is why I suggested that ugly hack with wrappers: at least there would be no build errors. At a price of much more churn to fix deprecation warnings, agreed.)

If I import Control.Monad.Fix explicitly like we want people to do and then import Control.Monad.State to which we er.. affix your shim, I get actively penalized, because now those two different definitions that were the same collide as they are two unrelated functions that want the same name. Try it. To prevent the error I must explicitly hide every one of your shims from every one of the mtl modules I import or take the warning and actively NOT import the correct Control.Monad.Fix or move to qualified imports for all uses of the mtl. These are not the directions we want people to move in, and even the last one should be allowed, but not required.

This is the same thing as happens with Prelude when you import Control.Category and they both go to supply competing definitions of (.) and id. You still have to import Prelude hiding (id,(.))

So, er.. your fix doesn't work. ;)

Keep in mind any release would be tied to a major GHC release, a time at which users who already have bad upper bounds on base are fixing a half a dozen minor issues anyways, so adding 2-3 unqualified imports isn't out of line with what GHC has done to users every couple of years.

To do your fix justice, we'd need a warning about a given provenance of a symbol, which is something we've wanted for years, which doesn't seem to be forthcoming, so I don't think we can meaningfully tie any release plan to the existence of such a tool.

[Bodigrim]

@ekmett agreed, I forgot that Control.Monad might be already imported as well. fix withdrawn :)

Keep in mind any release would be tied to a major GHC release, a time at which users who already have bad upper bounds on base are fixing a half a dozen minor issues anyways, so adding 2-3 unqualified imports isn't out of line with what GHC has done to users every couple of years.

IMHO there is a difference. Significant breaking changes in base are smoothed by -Wcompat warnings; and stuff does not usually disappear overnight. Despite all these efforts, even moderate changes like MonadFail cause a considerable disruption. And my impression is that there are many more modules, ab-using Control.Monad.Reader to import Control.Monad, than using fail.

Perhaps my problem here is that I personally do not see much added value in this change. I mean, sure, it would be nice to structure mtl this way from the very beginning. But we live in the world, where return and mappend are still type class members.

[ekmett]

An example that was particularly galling for a long time during the MonadFail switch over was using import Control.Monad.Fail, then explicitly hiding fail from Monad, only to have it dragged back in 20 times by different mtl imports. (Not an issue now, but during the MonadFail switch-over I wanted to scream on multiple occasions.) You particularly feel this if you go to build your own mtl-like classes which lift over other transformers or data types that act as transformers in their own right, and import many mtl modules.

Working on some abstract algebra and want to use the name join? Well, better be very explicit I guess.

The other main issue is PVP issue, where re-exports here effectively tie us to changes to base. On paper significant changes to base may be presaged by -Wcompat, but it seems about once a year I wind up having to push out a significant cross section of all the libraries I maintain due to base changes nonetheless. On paper that sounds like we're doing better, right? I seem to make it through two major releases, but er.. we doubled the frequency of major releases at the same time, so it has been a wash IMHO.

There's also a minor issue of signalling about proper API design. Folks emulate core libraries all the time, and the mtl is very much not matching behavior folks should emulate.

[Bodigrim]

In my experience, updating to newer GHC/base was pretty painless last years. Sure, it takes time to clean up all warnings, but errors are rare.

It would be nice to sweeten such breaking changes by simultaneous improvements / new features in other areas. Like catching up with transformers on account of MonadAccum / MonadSelect. Otherwise it looks like a difficult sell to me: why would an end user switch to newer mtl only to break his existing codebase?

[Bodigrim]

I must emphasize that this is a huge change breaking every other package (and IMO it is not worth to fix a subtle PVP issue in such way, but whatever). I think it deserves to be clearly announced and explained at libraries maillist.

[emilypi]

@Bodigrim we are planning on this months from now, with as much tooling guidance, announcing, and help as possible.

[chessai]

Marking this as MTL 3 milestone

[kozross]

@chessai and @emilypi - did PR #108 fix this, or did I miss something?

[emilypi]

@kozross i believe it did yes

[kozross]

@emilypi Then we should close this.