base: Add new Control.Monad.Fail module
ClosedPublic

Authored by hvr on Nov 2 2015, 7:40 AM.

Details

Summary

This is based on David's initial patch augmented by more extensive
Haddock comments.

This has been broken out of D1248 to reduce the size of D1248
by splitting it into smaller logical pieces.

Diff Detail

Repository
rGHC Glasgow Haskell Compiler
Branch
arcpatch-D1424
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 6495
Build 7503: GHC Patch Validation (amd64/Linux)
Build 7502: arc lint + arc unit
hvr updated this revision to Diff 4881.Nov 2 2015, 7:40 AM
hvr retitled this revision from to base: Add new Control.Monad.Fail module.
hvr updated this object.
hvr edited the test plan for this revision. (Show Details)
hvr added reviewers: quchen, ekmett.
hvr updated the Trac tickets for this revision.
hvr added a comment.Nov 2 2015, 7:44 AM

Here's how the Haddocs currently render:

austin requested changes to this revision.Nov 2 2015, 12:12 PM
austin edited edge metadata.
austin added inline comments.
libraries/base/Control/Monad/Fail.hs
2

This should have a module header block to be consistent with a lot of our other modules (like Data.Bifunctor).

This revision now requires changes to proceed.Nov 2 2015, 12:12 PM
ekmett edited edge metadata.Nov 2 2015, 2:12 PM

In general it looks good to me, except we never explicitly talked about adding MonadFail as a default export to Control.Monad or Prelude in this release.

Given the lengthened timeline it might be better to warn about the fact that they are coming into base next release than to take symbols now?

My proposal would be to remove the re-exports of MonadFail (sans method) from Prelude and Control.Monad, add a warning that they are going to be added, and then bring them in in 8.2 or so.

hvr updated this revision to Diff 4886.Nov 2 2015, 2:56 PM
hvr edited edge metadata.
  • add boilerplate module header
  • drop re-exports from Prelude/Control.Monad again
hvr marked an inline comment as done.EditedNov 2 2015, 2:58 PM

@ekmett while delaying the re-export is no big deal, I wonder if there's really anybody out there who would get a nameclash with the MonadFail *class* name, and whom this cautionary measure benefits :-)

hvr updated this revision to Diff 4887.Nov 2 2015, 3:02 PM
hvr edited edge metadata.

drop now longer true comment from Haddock

hvr updated this revision to Diff 4888.Nov 2 2015, 3:06 PM
hvr edited edge metadata.

Turns out that w/o the re-export we need to qualify MonadFail too

hvr updated this revision to Diff 4889.Nov 2 2015, 3:10 PM
hvr edited edge metadata.

Finally, a changelog entry is nice to have...

ekmett accepted this revision.Nov 3 2015, 3:01 PM
ekmett edited edge metadata.

I too suspect that nobody would be affected in practice, but no point giving a pretext for someone to cry foul.

austin accepted this revision.Nov 3 2015, 3:39 PM
austin edited edge metadata.

LGTM.

This revision is now accepted and ready to land.Nov 3 2015, 3:39 PM
This revision was automatically updated to reflect the committed changes.
quchen edited edge metadata.Nov 4 2015, 11:48 AM

Looks good to me.

When I wrote this as part of the MFP implementation, I was wondering about the INLINE pragmas in the default MonadFail implementations. They seem pretty redundant, since given the size of the functions GHC should easily be able to decide whether to inline the definitions or not.