change the type of generalBracket, fixes #63 #64

gelisam · 2018-03-07T21:48:16Z

I made the changes discussed in #63, added some tests, updated the documentation, and added a new onError method which, unlike onException, also runs its handler when the computation aborts which an error which is not an exception.

While we're making breaking changes, would it be worthwhile to also generalize the methods in MonadCatch so they can act on those non-exception errors?

'MSpec' was an existential in 'm', but I want to construct another type which has both an 'MSpec' and some other 'm'-related fields. So I am exposing the 'm' in 'MSpec' and creating another existential which abstracts over it.

I want to reuse them

To make sure that the all the effects performed while releasing a resource are preserved, not just the IO effects. I'm writing the tests first, so the tests currently fail.

It is easy to accidentally implement the 'ExceptT' instance in a way which causes the error message thrown during 'use' to get propagated instead of the one thrown by 'release'. When both 'use' and 'release' throw an IO exception, the one thrown by 'release' wins, so I think we should follow suit.

Now that we have documentation for individual instances, there is no need to repeat this seldom-used information here.

The exceptions package used to have an undocumented advantage over lifted-base, namely that the non-IO effects are not discarded, but then exceptions-0.9.0 made a breaking change which lost this advantage. In this PR, I am restoring this advantage, and I am documenting it, both to help users decide which package to pick, and so we don't foolishly repeat that mistake.

This is the PR's main change. I make the type of 'generalBracket' even more general, which allows me to fix the implementations of 'StateT' and 'WriterT' so they don't discard their state changes.

if ExceptT has one, MaybeT should have one too

Update the documentation to reflect the difference between an exception and an error, calling out the cases in which the user might expect to catch all errors but will only catch all exceptions. In particular, 'onException' will only run the handler when an exception is thrown but not when any other kind of error is thrown; this is not particularly useful, but we can't do better without changing its type signature. A new function 'onError' is introduced which does not have this flaw.

to match the style of the existing codebase

RyanGlScott · 2018-03-08T00:24:50Z

src/Control/Monad/Catch.hs

+-- exception, or abort for some other reason. For example, in @ExceptT e IO@
+-- you can use 'throwM' to abort with an exception or
+-- 'Control.Monad.Trans.Except.throwE' to abort with a value of type 'e'.
+data ExitCase a


How about a derived Show instance for this, for debugging purposes?

RyanGlScott · 2018-03-08T00:25:35Z

src/Control/Monad/Catch.hs

+-- | A 'MonadMask' computation may either succeed with a value, abort with an
+-- exception, or abort for some other reason. For example, in @ExceptT e IO@
+-- you can use 'throwM' to abort with an exception or
+-- 'Control.Monad.Trans.Except.throwE' to abort with a value of type 'e'.


Explicitly state which constructors of ExitCase correspond to throwE and throwM.

RyanGlScott · 2018-03-08T00:30:01Z

src/Control/Monad/Catch.hs

@@ -194,61 +214,95 @@ class MonadCatch m => MonadMask m where
  -- tranformers from having @MonadMask@ instances (notably
  -- multi-exit-point transformers like 'ExceptT'). If you are a
  -- library author, you'll now need to provide an implementation for
-  -- this method. As two examples, here is a @ReaderT@ implementation:
+  -- this method. The @StateT@ implementation demonstrates most of the
+  -- subtelties:


s/subtelties/subtleties

RyanGlScott · 2018-03-08T00:30:15Z

src/Control/Monad/Catch.hs

+  --
+  -- The only effect which is intentionally not incorporated in the @release@
+  -- action is the effect of throwing an error. In that case, the error must be
+  -- re-thrown. One subtelty which is easy to miss is that in the case in which


s/subtelty/subtlety

RyanGlScott · 2018-03-08T00:36:19Z

src/Control/Monad/Catch.hs

@@ -653,44 +783,56 @@ catches a hs = a `catch` handler

 -- | Run an action only if an exception is thrown in the main action. The
 -- exception is not caught, simply rethrown.
+--
+-- /NOTE/ The action is only run if an /exception/ is throw. If the monad


s/is throw/is thrown/

RyanGlScott · 2018-03-08T00:39:21Z

CHANGELOG.markdown

@@ -1,3 +1,13 @@
+?????


Defintely version 0.10.0, as this will require a major version bump.

RyanGlScott · 2018-03-08T00:40:47Z

I've left some comments inline. I'd definitely like @snoyberg to look over this is as well.

While we're making breaking changes, would it be worthwhile to also generalize the methods in MonadCatch so they can act on those non-exception errors?

Can you be more specific on what your intent is?

snoyberg · 2018-03-08T09:35:22Z

src/Control/Monad/Catch.hs

 onException :: MonadCatch m => m a -> m b -> m a
 onException action handler = action `catchAll` \e -> handler >> throwM e

+-- | Run an action only if an error is thrown in the main action. Unlike


It may be worth spelling out that the difference is for transformers like EitherT and MaybeT that have multiple exit points.

snoyberg · 2018-03-08T09:36:08Z

src/Control/Monad/Catch.hs

    -> (a -> m b)
    -- ^ inner action to perform with the resource
-    -> m b
+    -> (a -> ExitCase b -> m c)


I should have commented on this earlier. I think it would make more sense to move the release action to before the use action, to match more closely the type signature of bracket in base.

snoyberg · 2018-03-08T09:37:30Z

src/Control/Monad/Catch.hs

-  -- functions are ignored.
+  -- | A generalized version of 'bracket' which uses 'ExitCase' to distinguish
+  -- the different exit cases, and returns the values of both the 'use' and
+  -- 'release' actions. You should probably use 'bracket' instead.


I'd probably clarify why bracket is better to use (more ergonomic), and comment that many other functions (like finally) are built on top of this method.

snoyberg · 2018-03-08T09:56:24Z

src/Control/Monad/Catch.hs

-    -> m b
+    -> (a -> ExitCase b -> m c)
+    -- ^ release the resource, observing the outcome of the inner action
+    -> m (b, c)


I think this signature makes things much easier to understand.

…and throwM.

gelisam · 2018-03-08T15:44:52Z

While we're making breaking changes, would it be worthwhile to also generalize the methods in MonadCatch so they can act on those non-exception errors?

Can you be more specific on what your intent is?

With MonadMask, we can now handle two kinds of error cases: ExitCaseException and ExitCaseAbort. MonadCatch can only handle the ExitCaseException case. I thought it might be more consistent if MonadCatch was also extended to support both cases, by adding a method which could look like this, with Nothing representing the ExitCaseAbort case:

generalCatch :: Exception e => m a -> (Maybe e -> m a) -> m a

But now that I'm thinking about this more, I don't think it makes a lot of sense to add a variant to the catch and handle family of functions which can catch the ExitCaseAbort case, because we don't have a concrete value to give to this handler. The user would be better-served by a monad-specific catching function like catchE.

RyanGlScott · 2018-03-08T15:59:38Z

OK. I'm not exactly sure what form your suggestion is going to take on, but I'd prefer to keep this PR about only the changes needed to replace the type signature for generalBracket. If there are additional changes needed for catching things, those would be better served as a separate PR (as this one is already quite hefty).

gelisam · 2018-03-08T16:09:35Z

I'm not exactly sure what form your suggestion is going to take on

To clarify: I no longer have a suggestion for changing MonadCatch; I had one but I have retracted it.

the Either instance has one, so MaybeT and ExceptT should have one as well

gelisam · 2018-03-08T16:20:41Z

I have now addressed all the inline comments. I have also added an entry to the changelog about the new onError function, added comments around the StateT and ExceptT implementations as a reminder to keep the documentation's copy of their implementation in sync, and I have added a few more @since annotations.

RyanGlScott · 2018-03-08T16:30:30Z

src/Control/Monad/Catch.hs

+-- example, if @f@ is an 'ExceptT' computation which aborts with a 'Left', the
+-- computation @onError f g@ will execute @g@, while @onException f g@ will not.
+--
+-- For monads which, unlike 'ExceptT' and 'MaybeT', only have a single exit


The phrasing of this sentence is a little awkward. I would prefer stating it like:

"This distinction is only meaningful for monads which have multiple exit points, such as 'Except' and 'MaybeT'. For monads that only have a single exit point, there is no difference between 'onException' and 'onError', except that 'onError' has a more constrained type."

RyanGlScott · 2018-03-08T16:37:41Z

src/Control/Monad/Catch.hs

    -> (a -> m b)
    -- ^ inner action to perform with the resource
-    -> m b
+    -> m (b, c)


How do we feel about the order of type variables in generalBracket? Our primary motivator for picking this order of arguments was to match what bracket does, but now we have:

bracket :: IO a -> (a -> IO b) -> (a -> IO c) -> IO c generalBracket :: m a -> (a -> ExitCase b -> m c) -> (a -> m b) -> m (b, c)

Now, the type variables b and c are in the opposite order as in bracket. We could conceivably change this to:

generalBracket :: m a -> (a -> ExitCase c -> m b) -> (a -> m c) -> m (b, c)

Or even:

generalBracket :: m a -> (a -> ExitCase c -> m b) -> (a -> m c) -> m (c, b)

I don't have a strong opinion one way or the other. Thoughts?

I don't have a strong opinion either. I'll change it to your first suggestion in order to at least match bracket's (a -> m c), then change it back if someone feels strongly about this :)

OK. The return type is interesting because it might hint at an order of evaluation. That is, the action which returns c runs before the action which returns b, hence m (c, b). On the other hand, this puts us back into a state where the type variables are out-of-order again. Perhaps the order of evaluation is already obvious because ExitCase c appears as an argument to the function which returns an m b?

Now that I am attempting the change, I feel like changing the variable names is a bad idea. It's important to thread the state in the correct order: from acquire to use, then from use to release. By swapping the order of use and release to match bracket, we have already made it less obvious that this is what implementors need to do. Let's at least keep these variable names as is in order to give a hint of the right ordering.

Would it be crazy to change change the type variables in bracket to match generalBracket, to make it clearer that even though use is last so bracket curries better, it is executed in the middle of the a and c bits?

bracket :: IO a -> (a -> IO c) -> (a -> IO b) -> IO b

That sounds like a reasonable suggestion too (short of actually swapping the order of those arguments).

RyanGlScott · 2018-03-08T20:50:11Z

Looking good! At this point, the only issue is that this doesn't build on old GHCs due to <$> and <*> not being in scope. I think you can fix both of these issues with:

import Control.Applicative

At around line 106 or so (within the #elif __GLASGOW_HASKELL__ < 710 ... #endif CPP).

to match the order in which the actions will be executed, like in generalBracket.

RyanGlScott · 2018-03-09T18:43:23Z

So close :)

Travis is now complaining:

src/Control/Monad/Catch.hs:835:27:
    Could not deduce (Functor m) arising from a use of ‘fmap’
    from the context (MonadMask m)
      bound by the type signature for
                 bracket :: MonadMask m => m a -> (a -> m c) -> (a -> m b) -> m b
      at src/Control/Monad/Catch.hs:834:12-64
    Possible fix:
      add (Functor m) to the context of
        the type signature for
          bracket :: MonadMask m => m a -> (a -> m c) -> (a -> m b) -> m b
    In the first argument of ‘(.)’, namely ‘fmap fst’
    In the expression:
      fmap fst . generalBracket acquire (\ a _exitCase -> release a)
    In an equation for ‘bracket’:
        bracket acquire release
          = fmap fst . generalBracket acquire (\ a _exitCase -> release a)

src/Control/Monad/Catch.hs:852:34:
    Could not deduce (Functor m) arising from a use of ‘fmap’
    from the context (MonadMask m)
      bound by the type signature for
                 bracketOnError :: MonadMask m =>
                                   m a -> (a -> m c) -> (a -> m b) -> m b
      at src/Control/Monad/Catch.hs:851:19-71
    Possible fix:
      add (Functor m) to the context of
        the type signature for
          bracketOnError :: MonadMask m =>
                            m a -> (a -> m c) -> (a -> m b) -> m b
    In the first argument of ‘(.)’, namely ‘fmap fst’
    In the expression:
      fmap fst
      . generalBracket
          acquire
          (\ a exitCase
             -> case exitCase of {
                  ExitCaseSuccess _ -> return ()
                  _ -> do { _ <- release a;
                            .... } })
    In an equation for ‘bracketOnError’:
        bracketOnError acquire release
          = fmap fst
            . generalBracket
                acquire
                (\ a exitCase
                   -> case exitCase of {
                        ExitCaseSuccess _ -> return ()
                        _ -> do { ... } })

I think replacing these uses of fmap with liftM will do the trick.

RyanGlScott · 2018-03-09T19:24:58Z

Oh, right. Currently, the Control.Applicative import is only in scope between GHC 7.6 and 7.10, which is why it's now only failing on GHC 7.4. Try taking out that import and putting it in its own:

#if __GLASGOW_HASKELL__ < 710
import Control.Applicative
#endif

gelisam · 2018-03-09T20:00:07Z

Slowly but surely!

RyanGlScott · 2018-03-10T13:37:41Z

Thanks, @gelisam! I'll try to get a new release out next week.

gelisam added 11 commits March 6, 2018 16:57

SomeMSpec

ca70699

'MSpec' was an existential in 'm', but I want to construct another type which has both an 'MSpec' and some other 'm'-related fields. So I am exposing the 'm' in 'MSpec' and creating another existential which abstracts over it.

name the MSpecs

04a5988

I want to reuse them

a series of tests about detectable effects

dd90823

To make sure that the all the effects performed while releasing a resource are preserved, not just the IO effects. I'm writing the tests first, so the tests currently fail.

move the CatchT warning to the instance documentation

7e9b7df

Now that we have documentation for individual instances, there is no need to repeat this seldom-used information here.

change the type of generalBracket, fixes #63

9e42dd8

This is the PR's main change. I make the type of 'generalBracket' even more general, which allows me to fix the implementations of 'StateT' and 'WriterT' so they don't discard their state changes.

MonadMask instance for MaybeT

6f51794

if ExceptT has one, MaybeT should have one too

use return instead of pure

1cacf63

to match the style of the existing codebase

add the changes to the changelog

5c725b7

RyanGlScott requested changes Mar 8, 2018

View reviewed changes

snoyberg reviewed Mar 8, 2018

View reviewed changes

gelisam added 6 commits March 8, 2018 10:15

add onError to the changelog

82659ea

add Show instance for ExitCase

bd6a570

Explicitly state which constructors of ExitCase correspond to throwE …

4352de7

…and throwM.

typos

3a7e35b

next version is 0.10.0

2aa7dae

onError @SInCE 0.10.0

9d4ee1b

spell out the difference between onError and onException

cd317d8

gelisam added 2 commits March 8, 2018 10:59

swap the arguments of generalBracket to match bracket

562b331

explain why users probably don't want to call generalBracket directly

594477e

gelisam added 2 commits March 8, 2018 11:14

reminder to keep the doc in sync with the implementation

9e168e8

@SInCE annotations for MaybeT and ExceptT

c1a5014

the Either instance has one, so MaybeT and ExceptT should have one as well

RyanGlScott reviewed Mar 8, 2018

View reviewed changes

adjust phrasing

eb13cec

gelisam added 2 commits March 8, 2018 18:26

rename the type variables in bracket*

1bae026

to match the order in which the actions will be executed, like in generalBracket.

import Control.Applicative when required

7d74727

use liftM instead of fmap, for older GHCs

dc3e243

also import Control.Applicative for GHC-7.4

91fe5e5

RyanGlScott approved these changes Mar 9, 2018

View reviewed changes

RyanGlScott merged commit 9fe0a66 into ekmett:master Mar 9, 2018

phadej mentioned this pull request Mar 11, 2018

exceptions-0.9.0 out of bounds commercialhaskell/stackage#3315

Closed

68 tasks

gelisam mentioned this pull request Mar 15, 2018

Fix the build and tests with exceptions 0.9.* haskell-hint/hint#54

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

change the type of generalBracket, fixes #63 #64

change the type of generalBracket, fixes #63 #64

gelisam commented Mar 7, 2018

RyanGlScott Mar 8, 2018

RyanGlScott Mar 8, 2018

RyanGlScott Mar 8, 2018

RyanGlScott Mar 8, 2018

RyanGlScott Mar 8, 2018

RyanGlScott Mar 8, 2018

RyanGlScott commented Mar 8, 2018

snoyberg Mar 8, 2018

snoyberg Mar 8, 2018

snoyberg Mar 8, 2018

snoyberg Mar 8, 2018

gelisam commented Mar 8, 2018

RyanGlScott commented Mar 8, 2018

gelisam commented Mar 8, 2018

gelisam commented Mar 8, 2018 •

edited

RyanGlScott Mar 8, 2018

RyanGlScott Mar 8, 2018

gelisam Mar 8, 2018

RyanGlScott Mar 8, 2018

gelisam Mar 8, 2018

gelisam Mar 8, 2018

RyanGlScott Mar 8, 2018

gelisam Mar 9, 2018

RyanGlScott commented Mar 8, 2018

RyanGlScott commented Mar 9, 2018

RyanGlScott commented Mar 9, 2018

gelisam commented Mar 9, 2018

RyanGlScott commented Mar 10, 2018

change the type of generalBracket, fixes #63 #64

change the type of generalBracket, fixes #63 #64

Conversation

gelisam commented Mar 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RyanGlScott commented Mar 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gelisam commented Mar 8, 2018

RyanGlScott commented Mar 8, 2018

gelisam commented Mar 8, 2018

gelisam commented Mar 8, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RyanGlScott commented Mar 8, 2018

RyanGlScott commented Mar 9, 2018

RyanGlScott commented Mar 9, 2018

gelisam commented Mar 9, 2018

RyanGlScott commented Mar 10, 2018

gelisam commented Mar 8, 2018 •

edited