Add MonadFail instance

[k0001]

trafficstars

This instance is necessary in GHC 8.6 for do-notation fail desugaring to work

[phadej]

One need to depend on fail package for older GHC. See https://ghc.haskell.org/trac/ghc/wiki/Migration/8.0#base-4.9.0.0

[phadej]

OTOH, fail = error is just wrong. If you have possibly failing patterns, use error in the user code?

[k0001]

fail = error is the current situation, see https://hackage.haskell.org/package/base-4.11.1.0/docs/src/GHC.Base.html#fail (modulo stack trace details).

Not being able to pattern match when using do notation makes using QuickCheck less ergonomic. This feature has been around for a long while already. Not supporting the now mandatory MonadFail would be a backwards incompatible change.

I'm not sure depending on the fail package is really necessary. See the comments in https://hackage.haskell.org/package/base-4.12.0.0/docs/Control-Monad-Fail.html

[Lysxia]

Control.Monad.Fail is a relatively recent addition to base, you need to depend on fail in other to support versions as old as base-4.3

[sol]

fail = error is the current situation

Hmm, while this is true, it is a problem. Some types with a Monad instance don't have a sensible implementation for fail. This was the motivation for splitting Monad and MonadFail into two classes.

Not supporting the now mandatory MonadFail would be a backwards incompatible change.

I'm not going into the finer details of exceptions in Haskell (as implemented by GHC) now, but how I look at it: error and undefined are for situations of the kind of "this should never happen" or "this is a programming error" (this is actually true for all pure exceptions). Basically for situations where our types are not expressive enough. Now, with MonadFail the compiler can detect some of these situations.

MonadFail is still fairly new and we as a community don't have the collective experience yet on how things will pan out in practice. All I'm saying is: That you get compile-time errors instead of runtime errors in some situations now is an intentional change. It's the whole point of introducing MonadFail. Adding a MonadFail instance to everything that has a Monad instance would defeat that point.

As such, adding MonadFail instances is a case by case decision.

Not being able to pattern match when using do notation makes using QuickCheck less ergonomic.

I think a minimal example that motivates this point could be useful?

[quasicomputational]

An example of this causing build errors in the wild: haskell/containers#570.

I agree that the instance is dubious and I think it's justifiable to leave it out: if MonadFail Gen where fail = error is a legitimate instance, then MonadFail m where fail = error for all Monad m would also be legitimate in the same way.

[k0001]

I think it's important to keep in mind the use case here. QuickCheck is used to write tests which we expect to fail in some way if anything goes wrong. error accomplishes that.

[Lysxia]

How about putting this instance in a separate module? That provides the option to opt in while keeping total pattern matches available by default.

Libraries might have a hard time with that since we can't hide an instance if a dependency uses it but in practice it might be a minor issue.

[sol]

How about putting this instance in a separate module?

Personally I'm not in favor of this. I would even prefer merging as is. Bad practice don't become better if you stack them. Right now we have a pure exception, which is bad practice. After, we would have an orphan instance and a pure exception, which both are bad practice.

nb. the only situation when I even consider orphans is for dependency reasons (when we have package A and B, and neither A wants to depend on B, nor B wants to depend on A).

[Lysxia]

Orphans are problematic because they lead to incoherence. In this case there seems to be only one MonadFail instance might one ever want to write for Gen.

[ocharles]

I think a minimal example that motivates this point could be useful?

I believe @k0001 submitted this PR to fix (at least) the following error in his safe-money package:

Just dec <- pure (MoneyI.rationalToDecimal aprox plus yts ds digs r) 

But this particular one can be rewritten as

dec <- maybe (error "Generated impossible Decimal") return (MoneyI.rationalToDecimal aprox plus yts ds digs r)

For types without fold-like functions, it's a little trickier. I'd probably use case for those:

dec <- 
  case MoneyI.rationalToDecimal aprox plus yts ds digs r of
    Justy ok -> return ok
    Nothingy -> error "Generated impossible Decimal"

I'm also not convinced fail = error should be added.

[andrewthad]

I'm trying to build some tests with GHC head, and QuickCheck won't build because of this. I myself am partial (hehehe) to not providing a MonadFail instance. Users could always write the orphan instance in their test suites if they really needed it. Because of how quickcheck is designed, it already encourages the use of orphan instances in test suites.

One way or another, the decision on this will need to be made before GHC 8.8 comes out in a few weeks.

[phadej]

GHC head doesn't fail due lacking MonadFail instance. It fails because there aren't fail in Monad anymore in base which will ship with GHC-8.8. https://github.com/hvr/head.hackage/blob/82893131da075a480dee6ac9f6f8c939a685fc78/patches/QuickCheck-2.13.1.patch

Please, use head.hackage if you play with GHC HEAD.

[treeowl]

FWIW, I don't think this instance should exist either.

[treeowl]

Test failure management lives somewhere very different from Gen.

[MaximilianAlgehed]

This appears to be both a stale PR and an unwelcome change. I'm going to close the PR. Feel free to re-open it if you think that this requires further discussion.