Add HasCallStack to functions (resolves #40) #41
Conversation
There was a problem hiding this comment.
There are a few places that I'd change the frozen call stack behavior. It's a tricky function to apply correctly - IMO it's almost easier to write with parens explicitly to make it clearer what's happening.
withFrozenCallStack foo bar baz
withFrozenCallStack $ foo bar baz
(withFrozenCallStack foo) bar baz
withFrozenCallStack (foo bar baz)Usually these are equivalent, but the implicit param obscures how these differ.
| throw :: HAS_CALL_STACK => (C.MonadThrow m, Exception e) => e -> m a | ||
| throw = C.throwM . toSyncException |
There was a problem hiding this comment.
We could put a withFrozenCallStack here, which would "hide" that Control.Monad.Catch.throwM is the underlying throwing mechanism. Not sure if that's desired or not.
src/Control/Exception/Safe.hs
Outdated
| withException :: HAS_CALL_STACK => (C.MonadMask m, E.Exception e) => m a -> (e -> m b) -> m a | ||
| withException thing after = withFrozenCallStack $ C.uninterruptibleMask $ \restore -> do |
There was a problem hiding this comment.
There's a subtle thing here - withFrozenCallStack $ is going to freeze the CallStack for thing and after, while withFrozenCallStack C.uninterruptibleMask is going to only freeze the CallStack for that function. I suspect we want the former, though I also suspect that it's going to play weirdly with the types. May be worth not freezing here.
There was a problem hiding this comment.
I suspect we want the former
Did you mean to say latter here, by any chance? If so I agree with your comment: withFrozenCallStack C.uninterruptibleMask seems closer to what we want (freeze the handler only), and it is consistent with other functions. And you are right about it misbehaving. Naively changing it to
withFrozenCallStack C.uninterruptibleMask $ \restoregives
• Couldn't match type: m a -> m a
with: forall a1. m a1 -> m a1
Expected: (forall a1. m a1 -> m a1) -> m a
Actual: (m a -> m a) -> m a
There was a problem hiding this comment.
Fun fact, ghc can be placated with -XImpredicativeTypes
-- requires -XImpredicativeTypes
uninterruptibleMaskFrozen :: forall m b. C.MonadMask m => ((forall a. m a -> m a) -> m b) -> m b
uninterruptibleMaskFrozen = withFrozenCallStack C.uninterruptibleMask
withException thing after = uninterruptibleMaskFrozen $ \restore -> do ...I'm not sure how to do this without it, however (-XRankNTypes is not enough).
- Add withFrozenCallStack to simple aliases. - Remove HasCallStack from throwTo as it is unused. - Rewrite functions such that withFrozenCallStack is applied to the handler only (e.g. catch), not the action. - Remove withFrozenCallStack from withException as desired semantics are unclear.
|
@parsonsmatt Thanks for the review! Indeed, "
The same problem for Edit: I went ahead and made the same change to |
Removes withFrozenCallStack from finally for consistency with withException. Adds explanatory comment.
Also remove windows CI workaround now that issue has been resolved.
|
Hey @snoyberg, I believe this is ready for your review, now that stackage nightly has 9.6 and thus this PR can be tested on CI. In addition to the
Let me know if you want something else. Thanks! |
| onException :: HAS_CALL_STACK => C.MonadMask m => m a -> m b -> m a | ||
| onException thing after = withFrozenCallStack withException thing (\(_ :: SomeException) -> after) | ||
|
|
||
| -- Note: [withFrozenCallStack impredicativity] |
There was a problem hiding this comment.
I'm curious if DeepSubsumption helps here? https://github.com/ghc-proposals/ghc-proposals/blob/master/proposals/0511-deep-subsumption.rst
There was a problem hiding this comment.
@parsonsmatt Good idea! Unfortunately, I just tried it and received the same error:
{-# LANGUAGE DeepSubsumption #-}
...
withException thing after = withFrozenCallStack C.uninterruptibleMask $ ...
-- src/Control/Exception/Safe.hs:441:49: error: [GHC-83865]
-- • Couldn't match type: m a -> m a
-- with: forall a1. m a1 -> m a1
-- Expected: (forall a1. m a1 -> m a1) -> m a
-- Actual: (m a -> m a) -> m a
-- • In the first argument of ‘withFrozenCallStack’, namely
-- ‘C.uninterruptibleMask’This however inspired me to try manually eta-expanding All The Things, and it actually worked without any new extensions!
diff --git a/src/Control/Exception/Safe.hs b/src/Control/Exception/Safe.hs
index 0c4b5e9..8872d3c 100644
--- a/src/Control/Exception/Safe.hs
+++ b/src/Control/Exception/Safe.hs
@@ -437,7 +437,7 @@ onException thing after = withFrozenCallStack withException thing (\(_ :: SomeEx
--
-- @since 0.1.0.0
withException :: HAS_CALL_STACK => (C.MonadMask m, E.Exception e) => m a -> (e -> m b) -> m a
-withException thing after = C.uninterruptibleMask $ \restore -> do
+withException thing after = withFrozenCallStack (\io -> C.uninterruptibleMask (\r -> io r)) $ \restore -> do
fmap fst $ C.generalBracket (pure ()) cAfter (const $ restore thing)
where
-- ignore the exception from after, see bracket for explanation
@@ -464,7 +464,7 @@ bracket_ before after thing = withFrozenCallStack bracket before (const after) (
--
-- @since 0.1.0.0
finally :: HAS_CALL_STACK => C.MonadMask m => m a -> m b -> m a
-finally thing after = C.uninterruptibleMask $ \restore -> do
+finally thing after = withFrozenCallStack (\io -> C.uninterruptibleMask (\r -> io r)) $ \restore -> do
fmap fst $ C.generalBracket (pure ()) cAfter (const $ restore thing)
where
-- ignore the exception from after, see bracket for explanationI missed this when I was experimenting earlier. I am curious what others think of this idea.
|
Thanks everyone! |
Resolves #40. I'm putting this up now for visibility, though it's probably best to wait until GHC 9.6 can be added to CI so the new code can actually be compiled on CI (9.6 is the first GHC using
exceptions >= 0.10.6that this PR utilizes).Couple notes:
In addition to the
MIN_VERSION_base(4,9,0)guard forHasCallStack, I added a condition forMIN_VERSION_exceptions(0,10,6), since there is no reason to addHasCallStackfor any lower versions (also it would trip-Wredundant-constraints, though that doesn't appear to be on).It is not entirely clear to me when it is best to use
withFrozenCallStack, so I followed the original PR's lead. The following table summarizes the situation:exceptionsfunctions usingwithFrozenCallStacksafe-exceptionscounterpart, or a note that the originalexceptionsfunction was re-exported, so we do not have to do anything.Additionally,
safe-exceptionshas a few functions that do not exist inexceptions, yet seem they like they should also usewithFrozenCallStack, so I added those as well, and listed them asN/A.Click to expand table
exceptionssafe-exceptionsmask_uninterruptibleMask_catchAllcatchAnycatchIOErrorcatchIfcatchJustcatchJusthandlehandlehandleIOErrorhandleAllhandleAnyhandleIfhandleJusthandleJusttrytrytryJusttryJustcatchescatchesonExceptiononExceptiononErrorbracketbracketbracket_bracket_finallyfinallybracketOnErrorcatchIOcatchDeepcatchAnyDeephandleIOhandleAnyhandleDeephandleAnyDeeptryIOtryAnytryDeeptryAnyDeepwithExceptionbracketOnError_bracketWithErrorcatchesDeepPerhaps @parsonsmatt would be kind enough to glance over the
withFrozenCallStacklogic?