Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MTL doesn't adhere to PVP as it re-exports e.g. Control.Monad #68

Closed
phadej opened this issue Sep 15, 2019 · 14 comments · Fixed by #99
Closed

MTL doesn't adhere to PVP as it re-exports e.g. Control.Monad #68

phadej opened this issue Sep 15, 2019 · 14 comments · Fixed by #99
Milestone

Comments

@phadej
Copy link
Contributor

phadej commented Sep 15, 2019

This is subtle issue. But when you do

import Prelude ()
import Prelude.Compat --base-compat-0.11

import Control.Monad.Reader -- mtl-2.2.2

Then the fail is ambiguous.

it's my own fault, that I imported Control.Monad.Reader unqualified, but still this is surprising and nasty.

@chessai
Copy link
Member

chessai commented Dec 9, 2019

would it be okay to just remove this?

@phadej
Copy link
Contributor Author

phadej commented Dec 9, 2019

Remove re-export? I'd be very fine with that, the added convenience is minuscule IMO.

@ekmett
Copy link
Member

ekmett commented Oct 18, 2020

I think we should just pull off the bandaid and do this.

@phadej
Copy link
Contributor Author

phadej commented Jan 28, 2022

As the #99 is reverted this should be re-opened.

@ekmett
Copy link
Member

ekmett commented Jan 28, 2022

So much for pulling off the bandaid. sigh

@ekmett ekmett reopened this Jan 28, 2022
@ekmett
Copy link
Member

ekmett commented Jan 28, 2022

I do want to go squarely on record as saying that reverting this change is the wrong decision.

@sjakobi
Copy link
Member

sjakobi commented Jan 28, 2022

@ekmett Are you aware of the discussion in #101 about how to best respond to the breakage that pulling off this bandaid would cause?

The decision to revert it certainly wasn't done lightly.

@ekmett
Copy link
Member

ekmett commented Jan 28, 2022

Yes. That level of impact is well within my expectation.

This has been a wart in the design of this library that we've suffered with for 15 years, because at no point has it ever been "the right time to fix it." The thing is, waiting to fix it ever longer will simply increases our exposure without end.

If our stated intention is to do this in 3.0 then all that we do by pushing this off is make sure that the substantive changes we want to make there to improve are conflated with this and ensuring we have even more code to unbreak.

If our intention is not to do this in 3.0 then I really don't think this is maintenance, it is life support.

@ekmett
Copy link
Member

ekmett commented Jan 28, 2022

The patches consist of adding imports, not making complicated semantic changes.

The cost of the status quo is that adding any new combinators to Control.Monad or Control.Monad.Fix or Control.Monad.Trans means that any existing user of MTL modules that uses any of the new names we take in base may have to hide the same import a dozen times or more to keep using their code.

With perfect hindsight, pulling this bandaid off when it was causing this pain to users more actively during the MonadFail switch-over would have been better.

@ekmett
Copy link
Member

ekmett commented Jan 28, 2022

Why is it rational that import Control.Monad.RWS should bring into scope fix?

Meanwhile, import Control.Monad.RWS.Class spuriously does not.

@chessai chessai changed the title MTL doesn't adher to PVP as it re-exports e.g. Control.Monad MTL doesn't adhere to PVP as it re-exports e.g. Control.Monad Jan 28, 2022
@chessai
Copy link
Member

chessai commented Jan 28, 2022

(fixed a typo)

See #103 (comment)

@Bodigrim
Copy link

This has been a wart in the design of this library that we've suffered with for 15 years, because at no point has it ever been "the right time to fix it." The thing is, waiting to fix it ever longer will simply increases our exposure without end.

I do not see many developers suffering from this wart for 15 years. If that was the case, we'd see lots of packages choosing to import mtl qualified or with explicit import list, but this is not so.

The breakage allows for backwards-compatible patches, which proponents of the change could have provided beforehand to alleviate the pain of migration. Yet I've seen no such effort underway in 2.5 years this ticket is open.

I believe such a nuclear change, breaking all downstream dependencies, deserves a public presentation and community discussion before being made. A brief line in a changelog does not make it justice.

@ekmett
Copy link
Member

ekmett commented Jan 31, 2022

There has been no effort underway on anything around the mtl in the 2.5 years this ticket has been open.

@emilypi
Copy link
Member

emilypi commented May 8, 2022

with mtl-2.3, i believe we addressed this. Please hmu to reopen if there's more non-compliance.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants