Remove `MonadFail (ST s)` instance(s)

[phadej]

These instances are now implemented as

instance MonadFail (ST s) where
    fail s = errorWithoutStackTrace s

for both strict and lazy as ST, and these are very much against the spirit of MonadFail proposal.

[chessai]

Agree. +1 for removal

[treeowl]

Get them out! I thought I remembered having proposed such a thing a few years ago.

[thielema]

On Fri, 21 Jan 2022, Oleg Grenrus wrote:

These instances are now implemented as

instance MonadFail (ST s) where fail s = errorWithoutStackTrace s

for both strict and lazy as ST, and these are very much against the spirit of MonadFail proposal.

Right. So please remove them!

[Bodigrim]

I hate to say it, but we need an impact assessment. Someone needs to prepare GHC 9.0 with the proposed patch and then build https://github.com/Bodigrim/clc-stackage against it.

[phadej]

I hate to say it, but we need an impact assessment. Someone needs to prepare GHC 9.0 with the proposed patch and then build https://github.com/Bodigrim/clc-stackage against it.

I won't be able to do it anytime soon, so any help is welcome.

[ulysses4ever]

@Bodigrim I tried that and clc-stackage fails for vanilla GHC 9.0.1 with

❯ cabal build
Resolving dependencies...
cabal: Could not resolve dependencies:
[__0] trying: clc-stackage-0.1.0.0 (user goal)
[__1] next goal: hfsevents (dependency of clc-stackage)
[__1] rejecting: hfsevents-0.1.6 (library is not buildable in the current
environment, but it is required by clc-stackage)
[__1] rejecting: hfsevents-0.1.5 (conflict: clc-stackage => hfsevents==0.1.6)
[__1] skipping: hfsevents-0.1.4, hfsevents-0.1.3, hfsevents-0.1.2,
hfsevents-0.1.1, hfsevents-0.1 (has the same characteristics that caused the
previous version to fail: excluded by constraint '==0.1.6' from
'clc-stackage')
[__1] fail (backjumping, conflict set: clc-stackage, hfsevents)
After searching the rest of the dependency tree exhaustively, these were the
goals I've had most trouble fulfilling: clc-stackage, hfsevents

hfeevents not on the list in the cabal file. Any suggestions how to fix it?

[Bodigrim]

Just remove it from clc-stackage.cabal, it's OS X specific package.

[ulysses4ever]

I've been trying to build clc-stackage, and here's a quick recap. First, it goes slowly because I keep hitting "Missing dependencies on foreign libraries" like OpenGL stuff, and restarting the process every time is slow.

Interesting breakages so far (with links to particular break points):

• double-conversion
• fgl
• lzma

I'm currently stuck because even after removing the said packages, the solver insists on building them. They're probably dependencies of some other packages (e.g. fgl has ~115 reverse dependencies), so I'm not sure how to proceed at this point.

I'd guess, that a better process would be to (try to) build every package separately, but I'd need to get around to write some bash for that. Even better would be to employ Nix to avoid the missing native dependencies problem, I guess.

[phadej]

@ulysses4ever nice!

• In lzma we should use error. Using fail there is just wrong. (I'll fix that, being a co-maintainer).
• double-conversion is also using fail instead of error
• fgl is bad:

class MonadFail m => GraphM m gr where

is just a such big lie. See https://hackage.haskell.org/package/fgl-5.7.0.3/docs/src/Data.Graph.Inductive.Monad.html#GraphM There authors are using fail for error. Yes, it behaves better in IO, but it's wrong in ST.

These breakages pin point poorly written code, which is my intention with this proposal.

[ulysses4ever]

I'm sorry but cabal-install giving me hard time today. I'm trying to build Bodigrim/clc-stackage after today's update where we removed several packages with native dependencies, for example: bindings-GLFW, OpenGLRaw. Yet, the build fails citing (among others) these very packages, and (most surprising) clc-stackage as its dependent:

cabal: Failed to build OpenGLRaw-3.3.4.1 (which is required by
exe:clc-stackage from clc-stackage-0.1.0.0). See the build log above for
details.
Failed to build bindings-GLFW-3.3.2.0 (which is required by exe:clc-stackage
from clc-stackage-0.1.0.0). See the build log above for details.

Maybe it's a wrong place to ask, but why is this happening? To be sure, I checked with grep that the said packages are not mentioned in the clc-stackage's cabal file. I tried cabal clean and removing dist-newstyle. Full output of cabal build is here.

---

Meanwhile, another breakage is in regex-tdfa (failable patterns).

[ulysses4ever]

Quick update on building Stackage (~2600 packages) under this change. It's done, and here's a list of 8 packages (including 4 mentioned above) that failed to build on their own because of the change; via a click on the package name you can get the build output. For posterity, the list is:

• cborg-0.2.6.0
• double-conversion-2.0.2.0
• fgl-5.7.0.3
• lzma-0.0.0.3
• regex-tdfa-1.3.1.1
• union-find-0.2
• xeno-0.4.3
• zlib-0.6.2.3

There's about 400 of build failures. Some are due to environment (missing native libs) but many of them are due to dependency on (some of) these 8 (about 350 on zlib, it seems). To investigate into these 400 I'd need quick patches for these 8 packages (I already had patches for 3 of them though, fgl, lzma, double-conversion). Not sure when I get around to do it, but probably no earlier than Sunday.

[andreasabel]

@ulysses4ever

Meanwhile, another breakage is in regex-tdfa (failable patterns).

How should we communicate the fixed package to you? I can easily fix this error, but there may be more; GHC aborts at the first error. In the meantime, the fix is here: https://github.com/haskell-hvr/regex-tdfa/pull/29 Location: https://github.com/haskell-hvr/regex-tdfa/pull/29/files#diff-32adf9fa2b97b309ce49d4c8198f514329aaf2bfc737262f4d4e72dd8dfb14dcR243

Update: fix also a second occurrence of the same pattern: https://github.com/haskell-hvr/regex-tdfa/pull/29/files#diff-6f1582818f0d271f987905c7d77395bf709a0853462d45d236a6b24e9442afa3R288

[andreasabel]

@phadej

These breakages pin point poorly written code, which is my intention with this proposal.

In case of regex-tdfa is more like a poorly written compiler (for historic reasons); translating

  x : xs <- foo

into a use of fail rather than a pattern match failure (warning / runtime error).

[phadej]

@andreasabel: and people try to improve that in

• https://github.com/ghc-proposals/ghc-proposals/pull/319
• https://github.com/ghc-proposals/ghc-proposals/pull/327

[ulysses4ever]

@andreasabel thanks for pitching in! A link to a branch with the fix or to the corresponding commit, if it's already merged into master/main would be best (posted either here or to my email a.pelenitsyn on gmail.com) (e.g. https://github.com/haskell-hvr/regex-tdfa/tree/monadfail-ST). I can work with diffs too. The fix for regex-tdfa is already good!

[ulysses4ever]

Three more failures:

• Agda-2.6.2.1
• vty-5.33
• zenacy-html-2.0.4

Build output is here. There are 5 packages depending on these and failing to build.

[andreasabel]

The fix for the above error in Agda is here: https://github.com/agda/agda/tree/fail-ST (https://github.com/agda/agda/pull/5766).

UPDATE: Now in master: https://github.com/agda/agda/commit/c7499fb8a85eb8be160246ad5a2f0996c43eea9f

[ulysses4ever]

Dear all, I successfully built the whole Stackage (the version presented on Bodigrim/clc-stackage). The set of failing packages has not changed — it's 11 packages:

Agda, cborg, double-conversion, fgl, lzma, regex-tdfa, union-find, vty, xeno, zenacy-html, zlib

(details are above). Two of these (Agda and regex-tdfa) have patches submitted by the maintainer (thanks Andreas!). The rest 9 I patched myself and uploaded to this GH org: https://github.com/hs-monadfail-st-remove. Most of the patches are trivial, except for fgl (see above).

If CLC and @phadej are fine with it, I can submit a patch to GHC. And optionally to the affected packages.

[phadej]

@ulysses4ever it would be great if you can make a patch, then CLC could vote (if I understand the process correctly).

[ulysses4ever]

Here we go: https://gitlab.haskell.org/ghc/ghc/-/merge_requests/7501

[phadej]

The MR passes CI, thanks @ulysses4ever.

I therefore submit this proposal for vote to CLC. ping @Bodigrim

[tomjaguarpaw]

these are very much against the spirit of MonadFail proposal.

I'm not sure what this spirit is. Could the spirit be documented, at the same time as the removal of the instances, so that no one considers adding similar instances in the future?

I suspect the spirit is something like "fail s should be something actually in the monad itself, not an exception (except in instances of MonadIO)".

[Bodigrim]

The documentation says "When a value is bound in do-notation, the pattern on the left hand side of <- might not match. In this case, this class provides a function to recover". And errorWithoutStackTrace does not really provides any opportunity to "recover".

failIOis actually in the IO monad itself, so not an exception to the rule.

I agree that it's worth to clarify that fail = error is not an option. @ulysses4ever may I ask you to update the PR? Hopefully, @phadej does not mind such addition.

[ulysses4ever]

@Bodigrim I'm happy to. Do you mean to put the explanation in the changelog or what? I'm afraid the changelog will be lost in the internets pretty fast (I always have trouble looking up changelogs for past releases).

Btw, thanks, Tom, for bringing this up. I agree that an explanation is in order.

[Bodigrim]

I mean, let's put the explanation into haddock for MonadFail.

[phadej]

@Bodigrim I'd prefer deliberately avoiding "scope creep". If the CLC will need one or two things to approve the proposal I'd prefer them to come in a single review, not one-by-one.

I'd suggest @ulysses4ever to wait until others CLC members have raised their opinion.

[tomjaguarpaw]

I'd prefer deliberately avoiding "scope creep". If the CLC will need one or two things to approve the proposal I'd prefer them to come in a single review, not one-by-one.

As far as I'm concerned, putting in guard rails to reduce the chance that the fix regresses in the future is an essential part of the fix.

[phadej]

I'd prefer deliberately avoiding "scope creep". If the CLC will need one or two things to approve the proposal I'd prefer them to come in a single review, not one-by-one.

As far as I'm concerned, putting in guard rails to reduce the chance that the fix regresses in the future is an essential part of the fix.

I'm in favor of adding the documentation, but as a contributor I'm get frustrated when there are "could you do one more thing" "sure" "could you do one more thing" "sure" loop. This is why I ask CLC to vote on the change.

The process is too heavy if the PR has to be perfect before CLC bothers to vote.

[tomjaguarpaw]

I sympathise with that. I'm in favour of PRs doing something as simple as possible (but no simpler). I don't know the full details of how the CLC votes, but perhaps it's possible to vote on three possibilities (no change, remove the instances only, remove the instances and add docs).

[Bodigrim]

Dear CLC members. Let's vote on https://gitlab.haskell.org/ghc/ghc/-/merge_requests/7501. As long as we agree on it, the justification for the change (that fail = error is not an option) can be added and bikeshedded separately as a part of documentation process.

CC @emilypi @tomjaguarpaw @cgibbard @chessai @cigsender

---

+1 from me.

[tomjaguarpaw]

+1

[phadej]

Btw, this was discussed already in 2019: https://mail.haskell.org/pipermail/libraries/2019-November/030087.html but

[Bodigrim]

@emilypi @cgibbard @chessai @cigsender just a gentle reminder about the vote.

[mixphix]

+1

[emilypi]

🔥 +1

[Bodigrim]

4 votes out of 6 are enough for approval, thanks all.

@ulysses4ever @phadej since this is a breaking change, could you please prepare a migration guide along the lines of https://github.com/haskell/core-libraries-committee/blob/main/guides/no-noneq-in-eq.md ?

[ulysses4ever]

@Bodigrim #42

[Bodigrim]

Thanks all, the change has been merged in GHC, and I advertised the migration guide on Reddit.

[mpickering]

Thanks for the little migration guide. I have updated head.hackage with the relevant patches.

[chshersh]

I'm trying to summarise the state of this proposal as part of my volunteering effort to track the progress of all approved CLC proposals.

Field	Value
Authors	@phadej, @ulysses4ever
Status	merged
base version	4.17.0.0
Merge Request (MR)	https://gitlab.haskell.org/ghc/ghc/-/merge_requests/7501
Blocked by	nothing
CHANGELOG entry	present
Migration guide	https://github.com/haskell/core-libraries-committee/blob/main/guides/no-monadfail-st-inst.md

Please, let me know if you find any mistakes 🙂