haskell / mtl

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

Remove re-exports of Control.Monad, ... #99

Closed phadej closed 2 years ago

phadej commented 2 years ago

... Control.Monad.Fix and Data.Monoid modules.

Import Identity and MonadIO explicitly in Control.Monad.Identity and Control.Monad.Trans to avoid potential future API fluctuations.

Resolves #68

phadej commented 2 years ago

I'd actually prefer to not re-export MonadTrans either, especially as whether it has QuantifiedConstraint depends on the underlying transformers version (as non-transformers-0.6 are also allowed). But maybe that can wait for the next major version.

emilypi commented 2 years ago

@phadej approved with one small grammatical wibble

chessai commented 2 years ago

LGTM pending Emily's comments.

sjakobi commented 2 years ago

I've noticed some breakage due to this change. See #101.

phadej commented 2 years ago

E.g. lucid imports MonadIO through Control.Monad.Reader. This is IMHO surprising.

Having an explicit set list of re-exports is an option too. Though #68 would repeat when e.g. base-compat exports MonadFail.fail and Control.Monad.Reader would export old Control.Monad.fail (on base where it exists).

phadej commented 2 years ago

Re explicit export list: Note that Control.Monad has functions like:

msum :: MonadPlus m => [m a] -> m a

which changed its type to

msum :: (MonadPlus m, Foldable t) => f (m a) -> m a

also foldM is similarly Foldable-generalized. Also forever, guard are generalized to Applicative / Alternative.

Also the <$!> is present since base-4.8.0.0 only.

We could either:

  1. not re-export these functions
  2. redefine them to always have "new" types
  3. drop support from pre-AMP GHCs.

All of these would still preserve a potential problem that a future base change would force a major version of mtl due change in type. (Or mtl could continue to re-export the name with an old type, if possible).


TBH, I looks like there are only bad options

Long term the latter is better, but right now is quite a bad moment for that :(

sjakobi commented 2 years ago

I think it would be fine to drop support for some older GHCs, eg 7.x. If type signatures still shift, we should try to find the most compatible one, probably by testing with some affected packages.

Oleg Grenrus @.***> schrieb am Fr., 19. Nov. 2021, 16:35:

Re explicit export list: Note that Control.Monad has functions like:

msum :: MonadPlus m => [m a] -> m a

which changed its type to

msum :: (MonadPlus m, Foldable t) => f (m a) -> m a

also foldM is similarly Foldable-generalized. Also forever, guard are generalized to Applicative / Alternative.

Also the <$!> is present since base-4.8.0.0 only.

We could either:

  1. not re-export these functions
  2. redefine them to always have "new" types
  3. drop support from pre-AMP GHCs.

All of these would still preserve a potential problem that a future base change would force a major version of mtl due change in type. (Or mtl could continue to re-export the name with an old type, if possible).

TBH, I looks like there are only bad options

  • Try to not break downstream, making maintenance harder (the base <5 bound won't cut, has to be reviewed every major base release, if there are explicit re-export lists)
  • Annoy folks buy cleaning up the API surface.

Long term the latter is better, but right now is quite a bad moment for that :(

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/haskell/mtl/pull/99#issuecomment-974174484, or unsubscribe https://github.com/notifications/unsubscribe-auth/AA36VCZM27BRPS7JE4S5PV3UMZVDXANCNFSM5IAF776A . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

phadej commented 2 years ago

As I said, this will just delay the "inevitable". Folks shouldn't import Control.Monad.State etc unqualified and also expect to get MonadIO from there. That's unhygienic and bad API design.

I'd rather do this right at once. If now is a bad moment (due Eq drama and no decision on how much breakage is acceptable, e.g.) I'd rather revert this commit (an re-open #68) than go for suboptimal middle-ground. Still re-exporting something is a baby step which doesn't really bring closer to resolving #68.

sjakobi commented 2 years ago

Still re-exporting something is a baby step which doesn't really bring closer to resolving #68.

I don't understand this. It seems quite viable to find a set of re-exports that would substantially reduce the breakage I reported in #101, and at the same time avoid the PVP problems reported in #68.

In principle I agree that mtl shouldn't have these re-exports at all, but the current situation is that lots of packages depend on these now.

I'm not fundamentally opposed to getting rid of the re-exports eventually, but I think it should be done separately from the migration to transformers-0.6. Otherwise this migration will be extremely slow and painful.

phadej commented 2 years ago

Otherwise this migration will be extremely slow and painful.

This is "break big but rarely" vs. "break often in small parts" trade-off. In general people prefer no breakages at all.

Otherwise this migration will be extremely slow and painful.

For the removal of re-exports change the migration is adding

import Control.Monad

like import on top of

import Control.Monad.Reader

Not painful. Requires more packages to be patched, but if we are going to do that anyway, why not now?

Anyway. I won't write a patch for a set of re-exports that..., and I'm fine with this patch being reverted if it's a blocker for release of mtl. But this is up to actual maintainers of mtl whom have been silent for now. I think we discussed this enough so they can make a call what they would prefer to do.