Turn on MonadFail desugaring by default
AbandonedPublic

Authored by hvr on Jul 31 2018, 9:03 AM.

Details

Summary

This contains two commits:


Make GHC's code-base compatible w/ MonadFail

There were a couple of use-sites which implicitly used pattern-matches
in do-notation even though the underlying Monad didn't explicitly
support fail

This refactoring turns those use-sites into explicit case
discrimations and adds an MonadFail instance for UniqSM
(UniqSM was the worst offender so this has been postponed for a
follow-up refactoring)


Turn on MonadFail desugaring by default

This finally implements the phase scheduled for GHC 8.6 according to

https://prime.haskell.org/wiki/Libraries/Proposals/MonadFail#Transitionalstrategy

This also preserves some tests that assumed MonadFail desugaring to be
active; all ghc boot libs were already made compatible with this
MonadFail long ago, so no changes were needed there.

Test Plan

Locally performed ./validate --fast

hvr created this revision.Jul 31 2018, 9:03 AM
bgamari accepted this revision.Jul 31 2018, 9:44 AM

Looks pretty sensible to me. Frankly, I'm pleasantly surprised by how few sites needed to change here.

This revision is now accepted and ready to land.Jul 31 2018, 9:44 AM
RyanGlScott requested changes to this revision.Jul 31 2018, 9:55 AM
RyanGlScott added a subscriber: RyanGlScott.

Wait, is this really intended to land in GHC 8.6? If so, we desperately need some additions to the release notes!

This revision now requires changes to proceed.Jul 31 2018, 9:55 AM
hvr updated this revision to Diff 17528.Jul 31 2018, 10:11 AM
  • Release notes entry doc update for MonadFailDesugaring
In D5028#138180, @hvr wrote:
  • Release notes entry doc update for MonadFailDesugaring

Wait, you wanted this for 8.6? English is terrible: I read the "scheduled" in the summary as past tense; that is, "we previously wanted to get this in to 8.6 but have since realized this isn't practical".

Frankly, I'm not keen on adding any more language changes to 8.6. If this were prior to alpha2 this would be a different story. Even in alpha1 I gave a bit of push-back against NoStarIsType. However, now we have a good fraction of Hackage building on head.hackage. Merging this would mean that we would have to do yet another pass over the libraries to make the beta usable for real-world testing. I personally don't have the bandwidth to make this happen.

docs/users_guide/glasgow_exts.rst
1597

Sigh, I missed this.

hvr added a comment.Jul 31 2018, 11:05 AM

Wait, you wanted this for 8.6? English is terrible: I read the "scheduled" in the summary as past tense; that is, "we previously wanted to get this in to 8.6 but have since realized this isn't practical".

Yes, I stayed up late yesterday and also spent a significant amount of time today to make this happen in time for the first beta; this was always scheduled to be done for GHC 8.6; this shouldn't have been any news to anyone!

Frankly, I'm not keen on adding any more language changes to 8.6. If this were prior to alpha2 this would be a different story. Even in alpha1 I gave a bit of push-back against NoStarIsType. However, now we have a good fraction of Hackage building on head.hackage.

We've got a terrible case of miscommunication then; quoting

https://ghc.haskell.org/trac/ghc/wiki/WorkingConventions/Releases/NewSchedule

Pre-release stability

  • Alpha release: This is a newly cut release branch that is guaranteed to build on all Tier 1 platforms and passes fast validation. Beyond that, no guarantees: some features slated for this release may not have fully landed.
  • Beta release: The release has seen some testing beyond ./validate. Only previously agreed on, stable and tested new functionality is allowed in. The focus is on bug fixing.
  • Release candidate (RC) release: Strictly no new functionality. Only bug fixes are allowed.

And landing this feature now is in compliance with that communicated scheme; before the first beta everything's fair game.

In D5028#138195, @hvr wrote:

Wait, you wanted this for 8.6? English is terrible: I read the "scheduled" in the summary as past tense; that is, "we previously wanted to get this in to 8.6 but have since realized this isn't practical".

Yes, I stayed up late yesterday and also spent a significant amount of time today to make this happen in time for the first beta; this was always scheduled to be done for GHC 8.6; this shouldn't have been any news to anyone!

Frankly, I'm not keen on adding any more language changes to 8.6. If this were prior to alpha2 this would be a different story. Even in alpha1 I gave a bit of push-back against NoStarIsType. However, now we have a good fraction of Hackage building on head.hackage.

We've got a terrible case of miscommunication then; quoting

https://ghc.haskell.org/trac/ghc/wiki/WorkingConventions/Releases/NewSchedule

Pre-release stability

  • Alpha release: This is a newly cut release branch that is guaranteed to build on all Tier 1 platforms and passes fast validation. Beyond that, no guarantees: some features slated for this release may not have fully landed.
  • Beta release: The release has seen some testing beyond ./validate. Only previously agreed on, stable and tested new functionality is allowed in. The focus is on bug fixing.
  • Release candidate (RC) release: Strictly no new functionality. Only bug fixes are allowed.

And landing this feature now is in compliance with that communicated scheme; before the first beta everything's fair game.

The canonical source for the list of features that we expect to be in a new release is that release's status page. I send emails well in advance of the release asking contributors to add their features to this list and put patches up on Phabricator, speaking with me if they think they think their patches aren't going to be up well before the release. The first such email for 8.6 was sent in April.

The trouble is there is no indication on the status page that this was a feature that was intended to land in 8.6. If it were listed there, I would have insisted that the patch be ready earlier so that we can stabilize the release. Given that this change is due to the CLC I would even have prepared the patch myself if need be. However, to do so I need to now about these things earlier than the day before the beta.

Given that we are now here I don't want to hold up the CLC's plans unnecessarily. If we can demonstrate that the change has minimal effect on Hackage builds or someone is willing to do the work to update head.hackage to bring it back to its current state then I would be willing to merge this. However, in the future let's please try to get these changes in earlier. I know we are all busy, but these late changes just make life generally harder for everyone involved.

I tried using this patch to build a subset of widely known Haskell packages, and I've found six packages thus far that are broken by this:

  • criterion
  • fgl
  • polyparse
  • texmath
  • xmonad
  • yi-core
hvr added a comment.Jul 31 2018, 5:10 PM

I tried using this patch to build a subset of widely known Haskell packages, and I've found six packages thus far that are broken by this:

  • criterion
  • fgl
  • polyparse
  • texmath
  • xmonad
  • yi-core

You found more or less the same ones I and Oleg found as well (and I've already included workarounds in head.hackage for those I found)

We specifically tried building stuff like pandoc, xmonad, darcs, criterion, lens, warp

Otoh, somewhere ~150 packages were compatible w/ MFP out of the box, see P181 for a list of packages that got installed into my ~/.cabal/store/ghc-8.6....

I'll admit, 6 problems out of 160 packages does sound pretty reasonable.

If that is representative then I'm okay with merging. I'll try building some of my projects tonight.

In D5028#138215, @hvr wrote:

You found more or less the same ones I and Oleg found as well (and I've already included workarounds in head.hackage for those I found)

Ah, I didn't realize you had already wrote patches! For reference, here is the commit which contains them: https://github.com/hvr/head.hackage/commit/3c15fe8766d1619f71f201ce086fe4602104d14b

So to be more accurate, there's 9 known packages which need to be patched.

hvr added a comment.Aug 1 2018, 8:00 AM

So to be more accurate, there's 9 known packages which need to be patched.

btw, stringsearch is not one of them; I merely pinned down the auto-flags there for convenience and it slipped into that commit by accident

for the record, workarounding MFP-incompat packages locally which haven't yet been covered by head.hackage is as simple as adding two lines to the cabal.project file:

package pandoc
  ghc-options: -XNoMonadFailDesugaring
In D5028#138225, @hvr wrote:

btw, stringsearch is not one of them; I merely pinned down the auto-flags there for convenience and it slipped into that commit by accident

Oops! In that case, there's 8, plus the 8 I found in https://github.com/hvr/head.hackage/pull/79, for a total of 16 thus far.

In D5028#138225, @hvr wrote:

btw, stringsearch is not one of them; I merely pinned down the auto-flags there for convenience and it slipped into that commit by accident

Oops! In that case, there's 8, plus the 8 I found in https://github.com/hvr/head.hackage/pull/79, for a total of 16 thus far.

16 out of roughly how many?

16 out of roughly how many?

I don't know how to determine that exact number, but P182 contains the contents of my ~/.cabal/store/... directory, so perhaps that would be a useful approximation.

bgamari accepted this revision.Aug 6 2018, 11:45 AM

16 out of roughly how many?

I don't know how to determine that exact number, but P182 contains the contents of my ~/.cabal/store/... directory, so perhaps that would be a useful approximation.

Alright, then I would eyeball that this will regress roughly 5%. I suppose this is acceptable. It looks like I'll need to fix annrun001 during merge, however.

This commit appears to only be present in ghc-8.6, and not in master?

It's in the branch that I'm about to push.