Closed parsonsmatt closed 1 year ago
Thanks for pointing me to the exception backtraces proposal—I hadn't seen that before. If I've read that proposal correctly, then that proposes to add HasCallStack
constraints to throw
and friends, which suggests that exceptions
should follow suit after the proposal is accepted. (The proposal hasn't been accepted yet, but it appears to be close.)
A handful of thoughts:
At first, I wondered why this PR didn't just give HasCallStack
superclasses to MonadThrow
et al. instead of adding a HasCallStack
constraint to every method. I've answered my own question after trying this locally: it's because HasCallStack
is implemented using ImplicitParams
, and you can't use an implicit parameter constraint as a superclass in GHC:
error:
* Illegal implicit parameter `?callStack::CallStack'
* In the context: HasCallStack
...
HasCallStack
constraint itself has only been around since GHC 8.0. We should ensure that we don't break the build for older GHCs, either by using CPP or by using the call-stack
compatibility shim on older GHCs.throwNoBacktrace :: Exception e => e -> a
function for users of exceptions in non-exceptional control flow who want to explicitly opt out of backtrace collection. I wonder if we should introduce something similar in exceptions
?This patch would be helpful right now for users of annotated-exception
,or any user that uses a CallStack
aware exception type. Before I generalized things into annotated-exception
, we had a few exception types that would grab the current CallStack
and report location information. We could not use throwM
directly since it didn't have the HasCallStack
constraint.
I pushed a change that uses Data.CallStack
from call-stack
instead, thanks for the suggestion!
The exception backtraces proposal mentions adding a throwNoBacktrace :: Exception e => e -> a function for users of exceptions in non-exceptional control flow who want to explicitly opt out of backtrace collection. I wonder if we should introduce something similar in exceptions?
Hmm - I think the purpose of that is to avoid a potentially costly backtrace collection in some situations. As far as I can tell, the implicit parameter has some performance implications, but I would be surprised if super perf sensitive code is using a type class method for throwing exceptions in the first place.
Thanks! I've pushed some additional tweaks to make the new call-stack
dependency suitable for use in a GHC boot library dependency like exceptions
.
One more question: I noticed that there are two functions that don't have HasCallStack
constraints after this patch: try
and tryJust
. Is this intentional?
I would be surprised if super perf sensitive code is using a type class method for throwing exceptions in the first place.
A fair point. In that case, I won't bother with adding something like throwNoBacktrace
right now. We can always add it later if someone specifically asks for it.
One more question: I noticed that there are two functions that don't have HasCallStack constraints after this patch: try and tryJust. Is this intentional?
Hmm - the reason I originally put a HasCallStack
on catch
for annotated-exception
is because the handler may itself throw an exception. In hindsight, though, it would be nice to have a HasCallStack
on the try
also - it wouldn't be useful for the cases when try
catches the exception, but it would be good to have the callstack information on exceptions that escape the try
.
Another thing I am noticing: it'd be good to do withFrozenCallStack
on the 'combinators'. That way you don't get an entry for try
and then also catch
immediately - just the try
that is relevant to your app code. I'll make this change as well.
This PR adds
HasCallStack
constraints to the class methods on this library. This allows for callsites to be populated with interesting information.The primary use case I can imagine here is supporting
annotated-exception
, and allowing you to define an instance ofMonadThrow
that always annotates with callstack:With this, anything that uses
throwM
at theAppM
type gets a call stack on the attached exception.While this only benefits users of
annotated-exception
currently, when the GHC proposal to decorate all exceptions with backtrace information is implemented, then everyone benefits from this.Without this constraint, the
CallStack
used by the above instance points to the instance definition site - not terribly useful.