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

Remove MonadFail (ST s) instance(s) #33

Closed
phadej opened this issue Jan 22, 2022 · 40 comments
Closed

Remove MonadFail (ST s) instance(s) #33

phadej opened this issue Jan 22, 2022 · 40 comments
Labels
approved Approved by CLC vote base-4.17 Implemented in base-4.17 (GHC 9.4)

Comments

@phadej
Copy link

phadej commented Jan 22, 2022

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
Copy link
Member

chessai commented Jan 22, 2022

Agree. +1 for removal

@treeowl
Copy link

treeowl commented Jan 22, 2022

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

@thielema
Copy link

thielema commented Jan 22, 2022 via email

@Bodigrim
Copy link
Collaborator

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
Copy link
Author

phadej commented Jan 22, 2022

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
Copy link
Contributor

@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
Copy link
Collaborator

Bodigrim commented Jan 23, 2022

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

@ulysses4ever
Copy link
Contributor

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):

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
Copy link
Author

phadej commented Jan 24, 2022

@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
Copy link
Contributor

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
Copy link
Contributor

ulysses4ever commented Jan 28, 2022

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 added a commit to haskell-hvr/regex-tdfa that referenced this issue Jan 28, 2022
There was a use of `fail` for `Maybe` which is entirely pointless.
@andreasabel
Copy link
Member

andreasabel commented Jan 28, 2022

@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: haskell-hvr/regex-tdfa#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
Copy link
Member

@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
Copy link
Author

phadej commented Jan 28, 2022

@andreasabel: and people try to improve that in

andreasabel added a commit to haskell-hvr/regex-tdfa that referenced this issue Jan 28, 2022
There was a use of `fail` for `Maybe` which is entirely pointless.
@ulysses4ever
Copy link
Contributor

@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
Copy link
Contributor

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 added a commit to agda/agda that referenced this issue Jan 31, 2022
Preparing for a potential removal of `instance MonadFail (ST _)`, see
haskell/core-libraries-committee#33.
@andreasabel
Copy link
Member

andreasabel commented Jan 31, 2022

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

UPDATE: Now in master: agda/agda@c7499fb

andreasabel added a commit to haskell-hvr/regex-tdfa that referenced this issue Jan 31, 2022
There was a use of `fail` for `Maybe` which is entirely pointless.
andreasabel added a commit to agda/agda that referenced this issue Feb 1, 2022
Preparing for a potential removal of `instance MonadFail (ST _)`, see
haskell/core-libraries-committee#33.
@ulysses4ever
Copy link
Contributor

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
Copy link
Author

phadej commented Feb 6, 2022

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

@ulysses4ever
Copy link
Contributor

@phadej
Copy link
Author

phadej commented Feb 9, 2022

The MR passes CI, thanks @ulysses4ever.

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

@tomjaguarpaw
Copy link
Member

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)".

@phadej
Copy link
Author

phadej commented Feb 14, 2022

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

@Bodigrim
Copy link
Collaborator

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

@mixphix
Copy link
Collaborator

mixphix commented Feb 15, 2022

+1

@emilypi
Copy link
Member

emilypi commented Feb 17, 2022

🔥 +1

@Bodigrim Bodigrim added the approved Approved by CLC vote label Feb 17, 2022
@Bodigrim
Copy link
Collaborator

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
Copy link
Contributor

@Bodigrim #42

ghc-mirror-bot pushed a commit to ghc/ghc that referenced this issue Mar 4, 2022
CLC proposal: haskell/core-libraries-committee#33

The instances had `fail` implemented in terms of `error`, whereas the
idea of the `MonadFail` class is that the `fail` method should be
implemented in terms of the monad itself.
ghc-mirror-bot pushed a commit to ghc/ghc that referenced this issue Mar 5, 2022
CLC proposal: haskell/core-libraries-committee#33

The instances had `fail` implemented in terms of `error`, whereas the
idea of the `MonadFail` class is that the `fail` method should be
implemented in terms of the monad itself.
@Bodigrim
Copy link
Collaborator

Bodigrim commented Mar 5, 2022

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

@Bodigrim Bodigrim closed this as completed Mar 5, 2022
@mpickering
Copy link

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

@chshersh
Copy link
Member

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 🙂

@chshersh chshersh added the base-4.17 Implemented in base-4.17 (GHC 9.4) label Mar 19, 2023
FPtje added a commit to FPtje/uuagc that referenced this issue Aug 5, 2023
The MonadFail instance of ST was removed, see
haskell/core-libraries-committee#33
FPtje added a commit to FPtje/uuagc that referenced this issue Aug 5, 2023
The MonadFail instance of ST was removed, see
haskell/core-libraries-committee#33
FPtje added a commit to FPtje/uuagc that referenced this issue Aug 5, 2023
The MonadFail instance of ST was removed, see
haskell/core-libraries-committee#33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Approved by CLC vote base-4.17 Implemented in base-4.17 (GHC 9.4)
Projects
None yet
Development

No branches or pull requests