MonadFail proposal, phase 1
ClosedPublic

Authored by quchen on Sep 16 2015, 6:04 PM.
Tags
Subscribers
Tokens
"The World Burns" token, awarded by austin."Mountain of Wealth" token, awarded by thomie."Mountain of Wealth" token, awarded by hvr.

Details

Summary

This implements phase 1 of the MonadFail proposal (MFP, Trac #10751).

  • MonadFail warnings are all issued as desired, tunable with two new flags
  • GHC was *not* made warning-free with -fwarn-missing-monadfail-warnings (but it's disabled by default right now)

Credits/thanks to

  • Franz Thoma, whose help was crucial to implementing this
  • My employer TNG Technology Consulting GmbH for partially funding us for this work

Diff Detail

Repository
rGHC Glasgow Haskell Compiler
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
There are a very large number of changes, so older changes are hidden. Show Older Changes
quchen updated this revision to Diff 4213.Sep 20 2015, 7:34 AM
quchen removed rGHC Glasgow Haskell Compiler as the repository for this revision.

Fix RebindableSyntax issues, rebase onto HEAD

quchen set the repository for this revision to rGHC Glasgow Haskell Compiler.Sep 20 2015, 7:48 AM
austin requested changes to this revision.Sep 21 2015, 7:04 PM
austin edited edge metadata.

Okay, so the general thrust of the patch is good! But it's going to need some clean up.

The biggest red flag is all the irrefutable bindings. I really, really really really do not want any of this going on:

  1. Irrefutable bindings almost never do what you think. Pop quiz: what's the expected result of this expression: do y@(~True, x) <- return (False, 10); return (fst y)? And what happens if you remove the ~? Note that you do this in places like CmmUtils. Stick that in GHCi and see if you're correct.
  2. We can avoid it anyway, see below.

I imagine you introduced these because the alternative is that you have to do things like add MonadFail to the constraint of some function, but MonadFail doesn't always exist because it we might be bootstrapping with stage1 vs stage2. Here are two utilities you can use as a solution

  • Introduce a constraint alias with -XConstraintKinds, and use this anywhere we mention a polymorphic m which needs a pattern match.
#if __GLASGOW_HASKELL__ > 710
type MonadFailish m = MonadFail m
#else
type MonadFailish m = ()
#endif

f :: (Monad m, MonadFailish m) => ... -- works during bootstrap and stage1
  • Introduce a utility module (call it MonadFailCompat for example) that either A) simply defines it's own bullish typeclass in the case we're not using GHC > 7.10, and otherwise just re-exports MonadFail from Control.Monad.Fail. This should Do The Right thing I think. This will also reduce the #ifery elsewhere, which is great.
  • Write out these MonadFail instances for a lot of these types, like the tests for example.

I'd really like for this stuff to be fixed. Other comments below.

compiler/prelude/PrelRules.hs
663–667

sadface.jpg

libraries/base/Control/Monad/Fail.hs
4–6 ↗(On Diff #4213)

Add some module documentation boilerplate (for example, the standard haddock headers and a quick blurb).

7–9 ↗(On Diff #4213)

Kill dead whitespace.

12–14 ↗(On Diff #4213)

More whitespace.

This revision now requires changes to proceed.Sep 21 2015, 7:04 PM
quchen marked an inline comment as done.Sep 22 2015, 4:28 PM
In D1248#35570, @austin wrote:

The biggest red flag is all the irrefutable bindings

Agreed, those are not pretty, and should be replaced by something better. Their sole purpose was silencing the warnings about missing MonadFail instances during GHC's compilation (which, with -Werror, makes validate fail).

In D1248#35570, @austin wrote:

I imagine you introduced these because the alternative is that you have to do things like add MonadFail to the constraint of some function

I think I implemented MonadFail instances for all the Monads that had an overridden fail function. The remaining patterns should be of the form "by looking at the code you can see they always match", e.g. for things like [a,b] <- sequence [ma,mb]. I was not sure how to solve the problem here. You're right that silencing the errors with tildes is a bad idea and I shouldn't have done it. Now, what would you say would be the right way to do things? The truest way I could think of, in terms of retaining semantics,

do { pat <- action; stuff } -- pat failable
-- ===>
do { x <- action; case x of pat -> stuff }

This would get us something similar to an error on pattern match failure, which is what the previous version would result in. Not sure how to silence the "incomplete pattern" warning though.

In D1248#35570, @austin wrote:

but MonadFail doesn't always exist because it we might be bootstrapping with stage1 vs stage2. Here are two utilities you can use as a solution [ConstraintKinds]

I hadn't even considered this case, since I did not see any failing monad-generic functions. Not considered means I should go back and revisit each site. Good thing they're all marked in the diffs!

In D1248#35570, @austin wrote:

Write out these MonadFail instances for a lot of these types, like the tests for example.

Alright, I admit for the tests I was just lazy, and thought the strictness should be orthogonal to the test subject.

compiler/prelude/PrelRules.hs
663–667

I can live with very local very temporary ugliness. The alternative solution would be disabling the warnings module-wide, which is probably worse.

compiler/typecheck/TcMatches.hs
897

This comment is in the wrong place after a few edits. It was referring to the expression

lookupInstEnv True instEnvs typeclass [zonkedTypeHead]

which you can find elsewhere in the module now.

libraries/base/Control/Monad/Fail.hs
7–9 ↗(On Diff #4213)

This is intentional whitespace to separate things. I'll get rid of it if you don't like the style, but I think it adds a lot of readability.

quchen marked 4 inline comments as done.Sep 25 2015, 3:47 AM

I've hit a wall implementing this. The remaining issue is that for

foo :: MonadFail m => m a
foo = do
    Just x <- undefined
    undefined

a warning is generated, "no MonadFail for m".

The current mechanism simply takes the type constructor of the do statement we're typechecking, and looks up whether there is a MonadFail instance in the global environment for it. This works well, since e.g. Maybe does have such an instance. If our block is polymorphic in the monad though, this lookup simply asks "does m have a MonadFail instance?", which is of course not the case: the "instance" is not a given, but demanded by the overall type of the do block. It seems the mechanism of looking for an existing MonadFail constraint is wholly unsuitable for the general case.

I'm sure I'll have the opportunity to talk to some more experienced GHC programmers next week in London, but maybe this brief descriptions helps someone (help me) until then.

quchen updated this object.Nov 13 2015, 12:07 PM
quchen edited edge metadata.

Main functionality is done, what's left to do is mostly cleanup work! I'll update the diff when I'm done, right now it's full of rebasing residue and WIP names.

quchen updated this revision to Diff 5085.Nov 14 2015, 10:22 AM
quchen updated this object.
quchen edited edge metadata.

Alright, this is now ready for another review. I've updated the ticket's description to reflect the features; please have a look at the comment I made in TcError.hs, where there is still a potential issue left open.

compiler/typecheck/TcErrors.hs
470

I'm not sure what happens or should happen if this list is non-empty, but contains a FailablePattern entry.

  • Should we ignore MonadFail and issue the other thing as a potential error?
  • Should we issue both?
  • Under what circumstances could this be a problem?
quchen updated this object.Nov 14 2015, 10:48 AM
quchen edited edge metadata.
hvr requested changes to this revision.Nov 14 2015, 10:50 AM
hvr edited edge metadata.
hvr added inline comments.
libraries/base/Control/Monad.hs
23

as per https://phabricator.haskell.org/D1424#41140 it's been decided to not re-export MonadFail() just yet with GHC 8.0 to avoid "giving a pretext for someone to cry foul." ;-)

78

see above

libraries/base/Prelude.hs
76 ↗(On Diff #5085)

see above

This revision now requires changes to proceed.Nov 14 2015, 10:50 AM
quchen updated this revision to Diff 5088.Nov 14 2015, 11:05 AM
quchen edited edge metadata.

Implement HVR's code review (remove MonadFail reexports)

quchen marked 3 inline comments as done.Nov 14 2015, 11:05 AM
hvr accepted this revision.Nov 14 2015, 11:17 AM
hvr edited edge metadata.

I don't see anything obviously wrong
Macro thumbsup2:

quchen marked 2 inline comments as done.Nov 14 2015, 12:22 PM

Removed obsolete comments

quchen updated this revision to Diff 5092.Nov 14 2015, 1:22 PM

Fix the two failed tests

fmthoma requested changes to this revision.Nov 15 2015, 6:31 AM
fmthoma edited edge metadata.
fmthoma added inline comments.
compiler/rename/RnExpr.hs
14

I think we forgot to delete the {-# LANGUAGE RecordWildCards #-} again, and I'm not sure about the ScopedTypeVariables either.

compiler/typecheck/TcErrors.hs
470

I think we really need to rework this. mk_err reordors (to be precise: partitions) the list cts of missing constraints, and generates an error for the first element. This could lead to the case that there are two missing contstraints, the first of them being MonadFail, and GHC would issue a No instance arising error instead of a warning. This is definitely not intended.

compiler/typecheck/TcMatches.hs
897

I actually think it is gone by now. Instead of looking up whether or not the tyHead is an instance of MonadFail, we now just add a MonadFail constraint and let the constraint solver do all the dirty work.

921

That still looks like an awful lot of panic cases here. Is this safe (enough)?

docs/users_guide/using-warnings.rst
193–206

*Used to cause […] in GHC before 7.10

testsuite/tests/monadfail/MonadFailErrors.stderr
3

Just an idea: Wouldn't it be nice to have the arising from the failable pattern […] here as well, like we have when the warnings are turned on? {We would have to rework parts of the desugaring mechanism, though).

testsuite/tests/monadfail/MonadFailWarnings.stderr
5

I think we should add a line break here before the parentheses, in case the pattern becomes longer.

testsuite/tests/rebindable/rebindable6.hs
4

This flag should still be redundant, since the warnings are still off by default.

This revision now requires changes to proceed.Nov 15 2015, 6:31 AM
fmthoma added inline comments.Nov 15 2015, 9:07 AM
compiler/typecheck/TcErrors.hs
470

Submitted you a pull request :-)

quchen marked an inline comment as done.Nov 15 2015, 12:44 PM
quchen added inline comments.
compiler/typecheck/TcMatches.hs
897

Yup, that's why it's marked as done :-)

testsuite/tests/monadfail/MonadFailErrors.stderr
3

Good idea, but orthogonal to this patch. Depending on when people want to merge we might put it in, or submit it later as a separate patch.

testsuite/tests/rebindable/rebindable6.hs
4

Forgotten residue, yup

quchen marked 7 inline comments as done.Nov 15 2015, 1:12 PM
This comment was removed by quchen.
quchen updated this revision to Diff 5106.Nov 15 2015, 1:14 PM
quchen edited edge metadata.

Implement @fmthoma's suggestions (that are marked as done), merge his pull request

quchen updated this revision to Diff 5115.Nov 16 2015, 3:16 AM
quchen edited edge metadata.
quchen marked an inline comment as done.Nov 16 2015, 3:32 AM
fmthoma accepted this revision.Nov 16 2015, 12:21 PM
fmthoma edited edge metadata.

A few optional and cleanup things, and I'm not quite sure about the panic cases in TcMatches.hs. Otherwise I'm good :-)

compiler/rename/RnExpr.hs
14

My bad. On the first glance I thought we had just enabled the RecordWildCards extension, but this was just formatting.
On the other hand, RecordWildCards is unused here and can be safely deleted.

bgamari accepted this revision.Nov 17 2015, 8:11 AM
bgamari edited edge metadata.

Looks good. Just two comments inline. I'll happily take patches for them (and @fmthoma's remaining comments) post-merge.

Great work @quchen!

compiler/rename/RnExpr.hs
1105

The repetition here is a bit unfortunate. Might it make sense to factor this out?

compiler/typecheck/TcRnTypes.hs
2250

This could use a docstring.

quchen updated this revision to Diff 5147.Nov 17 2015, 9:54 AM
quchen edited edge metadata.
quchen marked an inline comment as done.

Rebase onto HEAD, implement CR suggestions

quchen marked 2 inline comments as done.Nov 17 2015, 9:55 AM
quchen updated this revision to Diff 5149.Nov 17 2015, 10:06 AM

rebase onto HEAD again

quchen marked an inline comment as done.Nov 17 2015, 10:06 AM
This revision was automatically updated to reflect the committed changes.