Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Decorate exceptions with backtrace information #330

Open
wants to merge 39 commits into
base: master
Choose a base branch
from

Conversation

bgamari
Copy link
Contributor

@bgamari bgamari commented May 7, 2020

@bgamari bgamari changed the title Add backtraces to exceptions Decorate exceptions with backtrace information May 7, 2020
@bgamari bgamari force-pushed the stacktraces branch 9 times, most recently from 6fa2aeb to 74ed8ba Compare May 7, 2020
@isovector
Copy link
Contributor

isovector commented May 8, 2020

I love this.

@saurabhnanda
Copy link

saurabhnanda commented May 8, 2020

Add a HasCallStack constraint to toException, incurring potentially unnecessary runtime cost and changing the type of a fairly widely used function (albeit in a backwards compatible way)

@bgamari FWIW I had attempted some benchmarks a long time ago:

With such a type we can easily write a variant of ``throwIO`` that, for
instance, attaches a DWARF backtrace: ::

-- | Throws an exception with a 'HasCallStack' backtrace.
Copy link

@elaforge elaforge May 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this a HasCallStackBacktrace, not a ExecutionBacktrace as implied above? Also it's not clear how the Backtrace type enables this, at least not without a definition for throw.

@jberryman
Copy link

jberryman commented May 8, 2020

Thanks for doing this work! Can you lead with the UX story and clarify a bit?

E.g.: I have a web app with various layers of exception handling with a topmost handler that logs anything that reaches there. Occasionally I get a mysterious head: empty list exception presumably from a dependency, but only in production. Does this help me? Would it have helped me diagnose the issue without me needing to anticipate it, or would I need to fiddle with stuff, release a new version, then hope the error is triggered? Say I've written my app already without being aware of this new backtrace exceptions stuff; what do I need to do exactly to my code to get a better story for the production scenario above?

If this is only really useful with a local profiling build, why not just xc?

I already don't use xc or HasCallStack since I guess I don't find them useful/ergonomic enougg. It sounds like if DWARF stack unwinding were fast enough to use in production then I might be able to use this?

@enobayram
Copy link

enobayram commented May 8, 2020

How much overhead would it bring to introduce a withBacktraceMechanism instead of a setGlobalBacktraceMechanism?

Also, would I ever want to have backtraces from multiple mechanisms for the same exception? My understanding is that HasCallstack gives you lexical traces but other methods give you execution traces, so even if performance weren't a concern, there's no strictly better-worse relationship between these mechanisms. Then, would it make sense to store a [Backtrace] instead of a Maybe Backtrace inside SomeException? Or maybe there are ways to obtain this behavior in other ways?

As a side note, I feel like the most significant feature suggested here is exposing to Haskell the stack trace powered by the DWARF debug information (Don't get me wrong, the change to SomeException is also important and significant). So, would it make sense to introduce it in a preliminary proposal?

P.S: Just a personal note; There have been times when I would sell my soul to the devil on the spot just to have the slightest bit of context for a given exception thrown in production. Any hint whatsoever...

@phadej
Copy link
Contributor

phadej commented May 8, 2020

foo someExc = do
    SomeException exc <- someExc

will desugar using MonadFail, currently it doesn't. See #327 #319. I don't consider PatternSynonyms to be good enough to offer compatibility layer for something as core feature as exceptions. I think this proposal would be better without until the pattern synonym.

Note, exceptions are also thrown in mtl style programs, where m might be on purpose not MonadFail so errors are reported using specific mechanism, potentially building on using exceptions.

@pepeiborra
Copy link

pepeiborra commented May 8, 2020

An alternative to the pattern synonym: leave SomeException alone and introduce a new datatype SomeExceptionWithLocation to become the root of the exception hierarchy, while SomeException would only stay around for backwards compatibility.

One also needs to keep fromException and toException in the Exception type class for backwards compatibility.

data SomeExceptionWithLocation where
    SomeExceptionWithLocation
      :: forall e. Exception e
      => Maybe Backtrace   -- ^ backtrace, if available
      -> e                 -- ^ the exception
      -> SomeExceptionWithLocation

 -- Two new members are added to the Exception type class, with default implementations.
class Exception e where
  toExceptionWithLocation :: e -> SomeExceptionWithLocation
  toExceptionWithLocation = SomeExceptionWithLocation Nothing

  fromExceptionWithLocation :: SomeExceptionWithLocation -> Maybe e
  fromExceptionWithLocation (SomeExceptionWithLocation _ e) = fromException e

  -- we keep the old members for backwards compatibility
  toException :: e -> SomeException
  fromException :: SomeException -> Maybe e

{- NOTE changing the default implementations of toException and fromException to be
    defined via the WithLocation variants will loop for fully default instances.
 
  toException e = case toExceptionWithLocation e of 
    SomeExceptionWithLocation _ someE -> SomeException someE

  fromException e = fromExceptionWithLocation (SomeExceptionWithLocation Nothing e)
-}

instance Exception SomeExceptionWithLocation where
  toExceptionWithLocation = id
  fromExceptionWithLocation = Just

instance Exception SomeException where
  toException = id
  fromException = Just

-- Minor changes are needed to 'throw' and 'catch' 
throw = raise# . toExceptionWithLocation
catch = ...

@Shimuuar
Copy link

Shimuuar commented May 8, 2020

Another problem with SomeException pattern synonym is case exc of SomeException e -> SomeException e is no longer so whenever code inspects exception and then rethrows it it will strip and replace stack trace information.

@Ericson2314
Copy link
Contributor

Ericson2314 commented May 8, 2020

I think it's nice to point out that the extra state lives purely in base using regular synchronization primitives? If one was really worried they could alwaysrebuild base to statically force one sort of backtrace and remove the state.

@cartazio
Copy link

cartazio commented May 8, 2020

is this a ghc proposal or a base / core libraries proposal? this seems to be strictly libraries based ( cc @chessai )

@cartazio
Copy link

cartazio commented May 8, 2020

another question: why are we assuming/presuming that you'll only provide one of the several types of backtrace? what obstacles or reasons for/against having an n-tuple of possible subset products of these various backtraces?

@goldfirere
Copy link
Contributor

goldfirere commented May 8, 2020

I agree about concerns around pattern synonyms. But maybe using one in this way would be a nice forcing function to get GHC to fix weaknesses around them.

I'm still a little unsure of what this all means, though. Here are a few questions:

  • Suppose I don't take any particular action w.r.t. this proposal and my program throws an exception. Will it have tracing info? That is, what's the default behavior?

  • DWARF traces are evidently time-consuming to build. Is it possible to build a DWARF trace only if an exception won't be caught? Or do we not have that information at the right time?

  • Is DWARF available on all platforms? I somehow thought it was only certain Linux flavors.

  • Would this mechanism be used by all exceptions? This worries me. If a library uses lots of exceptions internally (with the expectation of catching them all), then would this introduce a performance regression?

Don't get me wrong -- I love the idea of having stack traces on exceptions. I just don't really understand how this will work in practice.

@Philonous
Copy link

Philonous commented May 8, 2020

As someone has pointed out on reddit¹ there's combinators in base that might need adjustment to preserve backtraces when they re-throw (e.g. tryJust²).

1: https://reddit.com/r/haskell/comments/gfg5ac/ghc_proposal_decorate_exceptions_with_backtrace/fpvtxe8/
2: https://hackage.haskell.org/package/base-4.14.0.0/docs/src/Control.Exception.Base.html#tryJust

Copy link

@parsonsmatt parsonsmatt left a comment

i'm so excited 😄 thanks for the excellent work!

SomeException :: forall a. (Exception a, ?context :: ExceptionContext)
=> a -> SomeException

data ExceptionContext = ExceptionContext [SomeExceptionAnnotation]
Copy link
Contributor

@int-index int-index Oct 24, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the annotations are stored in a list. Do we have a single element per call stack frame, or do we store the entire call stack in a single element?

If it is the latter, then lists (being ordered) seem like a strange choice of a data structure. E.g. what does it mean to have [CostCentreBacktrace ..., HasCallStackBacktrace ...] vs [HasCallStackBacktrace ..., CostCentreBacktrace ...]? Shouldn't it be more like a type-indexed map?

I'm out of my depth here, so I'm asking, not suggesting.

Copy link
Contributor Author

@bgamari bgamari Oct 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A call stack is a single annotation. Until now I had been considered ExceptionContext to be an unordered bag of annotations and indeed through that lens the ordered natured of list is a bit odd.

One way of addressing this would be to state that ExceptionContexts are to be interpreted as stacks of annotations (which would roughly reflect the lexical scoping of the handlers which added the annotations). This isn't strictly true since multiple annotations may be added by the same call to, e.g., withExceptionContext. However, I think it's true enough to be a useful convention and at least somewhat resolve the semantic issue that you raise.

@bgamari
Copy link
Contributor Author

bgamari commented Oct 25, 2022

@int-index, from my perspective this proposal is ready for consideration by the committee.

@int-index int-index added Pending shepherd recommendation The shepherd needs to evaluate the proposal and make a recommendataion. and removed Needs revision labels Oct 25, 2022
@bgamari
Copy link
Contributor Author

bgamari commented Oct 29, 2022

@nomeata, I believe that this proposal is ready for consideration by the committee.

This proposal has evolved significantly since its first iteration, thanks in large part to @int-index and @parsonsmatt. In short, while the initial proposal was narrow in scope but limited in applicability, we now aim to address a more general need (namely, the ability to attach arbitrary provenance metadata to thrown exceptions, the utility of which has been demonstrated by @parsonsmatt's annotated-exception package).

In addition, in response to feedback we now faithfully preserve annotations on rethrowing of exceptions. Finally the proposal has been adjusted to define error messages produced by GHC's top-level exception handler in terms of displayException, fixing a long-standing idiosyncrasy and ensuring that exception backtraces are displayed.

I have a work-in-progress implementation of this proposal in ghc!8869. If the deliberation of this proposal proceeds swiftly then I believe this feature can land in time for GHC 9.6.

@nomeata
Copy link
Contributor

nomeata commented Oct 30, 2022

@int-index, you are still the shepherd, so the next step is a recommendation from you to the committee.

@int-index
Copy link
Contributor

int-index commented Oct 30, 2022

I know, that is why I changed the label from [Needs revision] to [Pending shepherd recommendation]. I just wanted to give the proposal a final thorough read before making a recommendation but haven't gotten around to it yet.

Copy link
Contributor

@adamgundry adamgundry left a comment

I have some comments on the details, but broadly this is looking like a really nice design with minimal backwards compatibility impact and a significant step forward in terms of debugging observability.

I spoke with @bgamari about this earlier and he pointed out that the fork for GHC 9.6 is imminent. The implementation of this proposal is not far off completion, so if we want to get this into 9.6 we should try to make the committee review process swift if possible. 😄

the existing mechanisms for collecting backtraces listed above. Furthermore, we
want to ensure that this information is available for consumption in structured
form by the user program, to allow use by logging libraries (for instance, see
`kaptip-raven #1
Copy link
Contributor

@adamgundry adamgundry Nov 2, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
`kaptip-raven #1
`katip-raven #1

SomeExceptionAnnotation :: forall a. (ExceptionAnnotation a)
=> a -> SomeExceptionAnnotation

class Typeable a => ExceptionAnnotation a where
Copy link
Contributor

@adamgundry adamgundry Nov 2, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exception has a Show superclass, should we do the same for ExceptionAnnotation for consistency? I guess using DefaultSignatures for displayExceptionAnnotation is fine too though.

Copy link
Contributor Author

@bgamari bgamari Nov 3, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose we could add a Show superclass although I am of mixed feelings as to whether it is desireable. Specifically, I see two viewpoints:

  1. That Exception requiring Show was a mistake in light of the fact that we have displayException. In fact, I believe that we only have it today because the original design of Exception didn't include a displayException method.
  2. That Show complements displayException, serving as a machine-readable rendering while displayException is human-readable.

Given that not all types have law-abiding Show instances, my tendency is to agree more with (1). Making Show a superclass of ExceptionAnnotation will only serve to force more non-law-abiding Show instances while carrying little benefit.


-- displayException shows context after the exception itself:
displayException (SomeException e) =
displayException e ++ "\n" ++ displayExceptionContext ?exceptionContext
Copy link
Contributor

@adamgundry adamgundry Nov 2, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even if the exception context is empty, this will add an extra trailing newline. Perhaps make displayExceptionContext add this only if the context is non-empty?

Copy link
Contributor Author

@bgamari bgamari Nov 3, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Quite right. I shall fix this.

.. implemented:: in-progress <https://gitlab.haskell.org/ghc/ghc/-/merge_requests/8869>
.. highlight:: haskell
.. header:: This proposal is `discussed at this pull request <https://github.com/ghc-proposals/ghc-proposals/pull/330>`_.
.. contents::
Copy link
Contributor

@adamgundry adamgundry Nov 2, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This proposal clearly predates the addition of section numbers to the proposal template. 😁

Suggested change
.. contents::
.. sectnum::
.. contents::

Since the ``SomeException``'s ``displayException`` implementation is used to
by GHC's top-level exception handler to display uncaught exceptions, this
change carries the consequence that uncaught exceptions will have their context
automatically printed as part of the error message presented to the user.
Copy link
Contributor

@adamgundry adamgundry Nov 2, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps add a forward reference to the section "Teach top-level handler to use displayException"?

Examples
--------

User programs would typically call ``setEnabledBacktraceMechanisms`` during
Copy link
Contributor

@adamgundry adamgundry Nov 2, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like the current version of the proposal doesn't mention setEnabledBacktraceMechanisms in the API of GHC.Exception.Backtrace?

``throwIO`` and similar functions: ::

module GHC.Exception.Backtrace
( enableMechanism, BacktraceMechanism(..) ) where
Copy link
Contributor

@adamgundry adamgundry Nov 2, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this export list complete? It doesn't mention disableMechanism or collectBacktrace[s].

-- 'BacktraceMechanism's.
collectBacktraces :: HasCallStack => IO ExceptionContext
collectBacktraces = do
mechs <- readIORef currentBacktraceMechanisms
Copy link
Contributor

@adamgundry adamgundry Nov 2, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps expose an API function from GHC.Exception.Backtrace to query the current list of backtrace mechanisms?

One existing use-case which does not break but arguably results in non-ideal
behavior is that of exception re-throwing. For instance, consider the program:
::

catch do_something $ \(e :: MyException) ->
-- Do something
throwIO e

Here the original annotations attached to ``e`` (which may include, e.g.,
backtraces) will be lost when the exception is re-thrown.
Copy link
Contributor

@adamgundry adamgundry Nov 2, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like the proposal text still needs to be adapted now that this has been fixed?

The introduction of the global state for backtrace mechanism selection is quite
ad-hoc. We consider this approach to be a compromise which makes robust
backtraces available by default with minimal additional code. Exception
backtraces are primarily a debugging tool and are a cross-cutting concern. The
global backtrace mechanism selection facility proposed here recognizes this but
it suffers from the usual drawbacks associated with global state: it does not
compose well and may result in surprising behavior when manipulated by more
than one actor.
Copy link
Contributor

@adamgundry adamgundry Nov 2, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a reasonable, pragmatic compromise. Does the proposal mention what the default state for the enabled backtraces is if the user doesn't do anything? I couldn't immediately see this, but I might have missed it.

@int-index
Copy link
Contributor

int-index commented Nov 2, 2022

if we want to get this into 9.6 we should try to make the committee review process swift if possible

My apologies, but I cannot promise swift action. I have made a few suggestions already and I'd like to thank @bgamari for incorporating them, but I believe further improvements are possible:

  • The semantics of annotation lists. Based on the discussion in a thread above, there seems to be no single interpretation for what it means for one annotation to be after another, only a "convention" which is immediately violated by the proposal itself in how it treats various representations of the call stack.
  • Rethrowing. Now that the proposal uses implicit parameters, we could make sure that exception contexts are not only preserved, but also enriched with information about rethrowing points. And this would be possible without any changes to the libraries if we give ?exceptionContext :: ExceptionContext special treatment in the constraint solver (just like HasCallStack enjoys special treatment).
  • We probably want a synonym type HasExceptionContext = (?exceptionContext :: ExceptionContext) to hide the fact that we're using implicit parameters internally. This is how it's done with HasCallStack.
  • The proposal text is incomplete. I skimmed it once again while writing this comment and couldn't tell what the types of throw, throwIO, catch, try, and other exception-related combinators would become if this proposal is implemented. It should spell out the new type signatures explicitly (or the old type signatures if they are unchanged, either way it should be explicit).

This is just off the top of my head. I didn't want to bother @bgamari with individual nitpicks like this and intended to prepare a more comprehensive review. I feel like this is my responsibility to make sure we have the best design possible before I make a recommendation, and I hope you can understand when I say that I cannot promise to do that in a timely manner for the 9.6 release, as I'm doing this in my free time. But if the delays are unacceptable, I'm willing to let someone else shepherd this.

@adamgundry
Copy link
Contributor

adamgundry commented Nov 3, 2022

@int-index that's fair enough, thanks for the update and for working on a comprehensive review. It sounds like we're not quite at a stage to accept the proposal without further polishing. We should certainly spend the time to get this right rather than implement a half-baked version for 9.6 and then need to change the implementation later.

parsonsmatt added a commit to parsonsmatt/exceptions that referenced this pull request Nov 8, 2022
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`](https://hackage.haskell.org/package/annotated-exception-0.2.0.4/docs/Control-Exception-Annotated.html), and allowing you to define an instance of `MonadThrow` that always annotates with callstack:

```haskell
instance MonadThrow AppM where
    throwM = throwWithCallStack
```

With this, anything that uses `throwM` at the `AppM` 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](ghc-proposals/ghc-proposals#330) 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.
RyanGlScott added a commit to ekmett/exceptions that referenced this pull request Nov 27, 2022
* Add `HasCallStack` to classes and functions

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`](https://hackage.haskell.org/package/annotated-exception-0.2.0.4/docs/Control-Exception-Annotated.html), and allowing you to define an instance of `MonadThrow` that always annotates with callstack:

```haskell
instance MonadThrow AppM where
    throwM = throwWithCallStack
```

With this, anything that uses `throwM` at the `AppM` 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](ghc-proposals/ghc-proposals#330) 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.

* Use call-stack for backwards compatibility

* Only depend on call-stack if using an old GHC version

`exceptions` is a GHC boot package, so we want to make sure that it does not
incur any external depedencies when using a recent GHC version.

* Enable FlexibleContexts for the benefit of older GHCs

Older versions of GHC require `FlexibleContexts` to use `call-stack`'s
`type HasCallStack = (?callStack :: CallStack)` compatibility shim.

* Drop support for GHC 7.0 and 7.2

`call-stack`'s `HasCallStack` compatibility shim only supports back to GHC 7.4.
As a result, we can't reasonably support GHC 7.0 and 7.2 in `exceptions`
without a fair bit of grimy CPP. That being said, GHC 7.0 and 7.2 are quite
ancient by this point, so I'm comfortable with simply dropping support for
them. This patch makes the necessary `.cabal` and CI changes to accomplish that.

* Enable ConstraintKinds for the benefit of older GHCs

Older versions of GHC require `ConstraintKinds` to use `call-stack`'s
`type HasCallStack = (?callStack :: CallStack)` compatibility shim.

* Add withFrozenCallStack where possible

* withFrozenCallStack fix for old GHC

* freeze masks

Co-authored-by: Ryan Scott <ryan.gl.scott@gmail.com>
@int-index
Copy link
Contributor

int-index commented Nov 27, 2022

Just to give a heads up, I have not forgotten about this proposal and I finally have some free time to properly think it through. In a few days, I will post some detailed feedback in this thread.

@int-index
Copy link
Contributor

int-index commented Nov 29, 2022

Hi @bgamari! I have finished another round of review and identified a few potential improvements.

  1. Proposal structure. At the moment, the "Proposed Change Specification" section contains a lot of information in addition to the specification itself. It's mixed with exposition and implementation-specific code snippets. Instead, I expect to see a concise list (bulleted or numbered) of proposed changes, as seen from the end-user perspective. Undoubtedly, the rest of the prose is also valuable, so I ask you not to remove it but to distribute it across other sections of the proposal text.

  2. Make ExceptionContext opaque. The proposal define ExceptionContext as a wrapper around [SomeExceptionAnnotation], but should we expose this fact to our users? What if we decide to store additional information there? I believe it would be best not to export the data constructor to allow for future changes.

  3. Remove the Semigroup instance for ExceptionContext. The annotations are stored in a list, so (<>) is O(n), and that's not good. Furthermore, I can't think of a use case for appending annotations from different exceptions, so there's a simple way to address this: do not define Semigroup or Monoid instances.

  4. Semantics of ExceptionContext. The list of SomeExceptionAnnotation is ordered, so we ought to define how users can interpret this order. What does it mean for one annotation to come before another? Here's an idea: introduce an operation similar to checkpoint from annotated-exceptions:

    annotateIO :: ExceptionAnnotation a => a -> IO r -> IO r

    The order of SomeExceptionAnnotation would reflect the nesting of calls to annotateIO, reflecting the call stack in its own way (separately from the collected backtraces).

  5. Group backtraces in a record. If we say that a single element in SomeExceptionAnnotation corresponds to a call to annotateIO, it is somewhat strange to add multiple annotations for a single event of collecting backtraces. After all, what would it mean for the HasCallStack backtraces to come before the CostCentre backtrace or vice versa? So introduce a record to store them together:

      data Backtraces =
        Backtraces {
          costCentreBacktrace :: Maybe (Ptr CostCentreStack),
          hasCallStackBacktrace :: Maybe GHC.Stack.CallStack,
          executionBacktrace :: Maybe [GHC.ExecutionStack.Location],
          ipeBacktrace :: Maybe [StackEntry]
       }

    (I refine this idea in the next suggestion)

  6. Encode the Backtraces record with GADTs. The record type Backtraces that I suggest in the previous point introduces a slightly annoying form of code duplication. The proposal already has an enumeration of backtrace mechanisms:

    data BacktraceMechanism
       = CostCenterBacktraceMech
       | ExecutionStackBacktraceMech
       | IPEBacktraceMech
       | HasCallStackBacktraceMech

    And in the record, we have a field per mechanism, each wrapped in a Maybe. Fortunately, there is an encoding that removes this duplication. Let us index BacktraceMechanism by the representation type of the backtrace:

    data BacktraceMechanism a where
      CostCentreBacktrace   :: BacktraceMechanism (Ptr CostCentreStack)
      HasCallStackBacktrace :: BacktraceMechanism GHC.Stack.CallStack
      ExecutionBacktrace    :: BacktraceMechanism [GHC.ExecutionStack.Location]
      IPEBacktrace          :: BacktraceMechanism [StackEntry]

    Now we can encode the set of enabled mechanisms as a function to Bool and the record of collected backtraces as a function to Maybe a. Something along the lines of:

    type EnabledBacktraceMechanisms = forall a. BacktraceMechanism a -> Bool
    type Backtraces = forall a. BacktraceMechanism a -> Maybe a

    This isn't as low-tech as a an enum and a record, and I realize that low-tech solutions are appealing in their own way, but in this particular case, I find that GADTs offer a more elegant encoding.

  7. Hide the implicit parameter behind a synonym. We choose to pass around the exception context as an implicit parameter, but this should be hidden from the end user. This is the way it's done with HasCallStack, where documentation clearly states:

    NOTE: The implicit parameter ?callStack :: CallStack is an implementation
    detail and should not be considered part of the CallStack API, we may
    decide to change the implementation in the future.

    Let's do the same and introduce a synonym for exception contexts:

    type HasExceptionContext = (?exceptionContext :: ExceptionContext)
  8. Preserve backtraces on rethrowing. The way the proposal is currently written, when an exception is caught and rethrown, its old backtrace is dropped and a new one is constructed. This is very bad, because rethrowing happens all the time (e.g. in bracket)! But it is not hard to fix: catch should provide the context to the handler as an implicit parameter, and throw should make use of it:

    catch :: Exception e => IO a -> (HasExceptionContext => e -> IO a) -> IO a
    throwIO :: (HasExceptionContext, Exception e) => e -> IO a

    What about uses of throwIO where HasExceptionContext has not been provided by catch? Easy: default to an empty context. It shouldn't be hard to add this special case to the solver, since HasCallStack has already set a precedent.

  9. Get rid of toExceptionWithContext. The proposal introduces a new method to Exception:

    toException :: Exception e => e -> SomeException                                 -- old
    toExceptionWithContext :: Exception e => e -> ExceptionContext -> SomeException  -- new

    But if we are passing the context as a constraint, then we could simply add it to the original method:

    toException :: (HasExceptionContext, Exception e) => e -> SomeException

    There is no need for two methods this way.

  10. Improvements to throwIONoBacktrace. Currently the proposal defines NoBacktrace variants of throw and throwIO:

    throwNoBacktrace   :: forall e a. (Exception e) => e -> a
    throwIONoBacktrace :: forall e a. (Exception e) => e -> a

    The idea is to allow users to opt out of backtraces for non-exceptional control flow. But the problem is that this choice is not recorded anywhere in the exception, so when the exception is caught and rethrown, it will have unwanted backtraces added to it.

    One solution is to add a Bool flag to ExceptionContext to record the choice of not collecting the backtraces, so that they are not collected when the exception is rethrown. In fact, we could avoid the duplication of throw and throwIO this way:

    backtracesEnabled :: HasExceptionContext => Bool
    enableBacktraces, disableBacktraces :: (HasExceptionContext => r) -> (HasExceptionContext => r)
    
    throw :: (HasExceptionContext, Exception e) => e -> IO a
    throwIO :: (HasExceptionContext, Exception e) => e -> IO a

    throw and throwIO can check for backtracesEnabled before calling collectBacktraces. The users would write something along the lines of:

    disableBacktraces $
      throwIO MyControlFlowException

    And the choice to disable backtraces would be carried along the exception in its context.

Hopefully, I have not missed anything. In the process of writing this review, I took a stab at rewriting the "Proposed Change Specification" section in accordance with all of the suggestions above, mainly to convince myself that the combination of those suggestions forms a coherent design. You can find it here: https://gist.github.com/int-index/750c6c292eb8266729adc61a5812a581. If you agree with my comments, feel free to incorporate the updated specification (or parts of it) into the proposal.

Thank you!

@int-index int-index added Needs revision and removed Pending shepherd recommendation The shepherd needs to evaluate the proposal and make a recommendataion. labels Nov 29, 2022
@parsonsmatt
Copy link

parsonsmatt commented Nov 29, 2022

Make ExceptionContext opaque.

Perhaps from the main interface but please don't hide it from end users entirely. At the least, exposing a method contextToAnnotations :: ExceptionContext -> [SomeExceptionAnnotation] would be good.

Remove the Semigroup instance for ExceptionContext. The annotations are stored in a list, so (<>) is O(n), and that's not good.

The performance for appending annotations is probably a non-issue, since these things are very unlikely to occur in a Hot Loop. If someone is expecting a forever (catch (throw x) (appendContexts)) to be fast then I expect they will quickly refactor away from exceptions at all, and exception contexts in particular, well before they hit the O(n) on list append.

Furthermore, I can't think of a use case for appending annotations from different exceptions, so there's a simple way to address this: do not define Semigroup or Monoid instances.

Consider ExceptionInLinkedThread from async:

data ExceptionInLinkedThread 
  = forall a. ExceptionInLinkedThread (Async a) SomeException 

constructed = 
  SomeException $
    SomeAsyncException $
      ExceptionInLinkedThread _ $
        SomeException FooException

We have at least two ?exceptionContext implicit params - one for the outer SomeException and another for the inner SomeException. Plus SomeAsyncException may get this context too. It seems reasonable to want ot attach HasExceptionContext to any "wrapper" exception like this.

Then, when you go to report on these - you want to have all the context available for reporting.

reportExceptionInLinkedThread :: SomeException -> IO ()
reportExceptionInLinkedThread se@(SomeException innerError) = do
  let outerContext = ?exceptionContext
  for_ (fromException se) \(ExceptionInLinkedThread errAsync se'@(SomeException _)) -> do
    let innerContext = ?exceptionContext
    reportException (toExceptionWithContext (outerContext <> innerContext) innerError)    

All told I think this is a valuable feature to have, and it is used in annotated-exception and the work codebase.

Or throwTo can have ExceptionContext items in the thrown exception, which would need to get merged somewhere.

The order of SomeExceptionAnnotation would reflect the nesting of calls to annotateIO, reflecting the call stack in its own way (separately from the collected backtraces).

This is generally how it works in annotated-exception and it works nicely.

Group backtraces in a record.

Is the idea to have a Backtraces record in a SomeExceptionAnnotation, stored in the internal [SomeExceptionAnnotation]? Or for ExceptionContext to carry this record separately, like:

data ExceptionContext = ExceptionContext Backtraces [SomeExceptionAnnotation]

If it is the former, then this suggestion feels like a lot more work for little gain. Suppose I'm writing a fold over [SomeExceptionAnnotation], and I want to ensure that each backtrace collection mechanism is handled uniformly. And, since CallStack presumably has a ToExceptionContext instance, that means I need to consider both a Backtraces record and a CallStack, redundantly:

processExceptionContext :: [SomeExceptionAnnotation] -> ExceptionMetadata
processExceptionContext = foldr test mempty
  where
    test ann acc 
      | Just (cs :: CallStack) <- castAnnotation ann
      = addCallStackToMetadata cs acc
      | Just (bt :: BackTraces) <- castAnnotation ann
      = maybe id addCallStackToMetadata (hasCallStackBacktrace bt)
        . maybe id addCostCenterBacktrace (costCenterBacktrace bt)
        . maybe id addExecutionBacktrace (executionBacktrace bt)
        . maybe id addIpeBacktrace (ipeBacktrace bt)
        $ acc
      -- repeat for all the other duplicates

I don't believe that requiring or maintaining an 'ordering' to these annotations is really necessary or desirable. Haskell's exceptions are imprecise, anyway, and execution follows a graph, not a clear stack.

Hmm, which makes me wonder: should we have a pure annotateException :: (ToExceptionAnnotation a) => a -> r -> r? In hindsight, this seems extremely useful for diagnosing or debugging pure code or lazy code, but it would probably require some Trickery. consider:

foo :: Int -> IO ()
foo i = do
  annotateIO (FooBlewUpWithArg i) $ do
    -- some messy work
    let x = if blah then 5 else error "oh no"
    -- more work
    pure $ someComputation x

Since someComputation can throw a exception while evaluating x, but that thunk won't get processed until after foo exits, then we lose the annotation on the pure exception.

(I have not considered this since annotated-exception does not expose a pure throw, but GHC will need to reckon with it).

If the Backtraces record is intended to be separate, then this does have some advantages.

  • The CallStack can be given a TypeError instance for ToExceptionAnnotation, which blocks people from using it in the [SomeExceptionAnnotation] list.
  • Or, ToExceptionAnnotation can instead be defined in terms of how it updates the ExceptionContext.
class ToExceptionAnnotation a where
    updateExceptionContext :: a -> ExceptionContext -> ExceptionContext

instance ToExceptionAnnotation CallStack where
    updateExceptionContext cs ec = 
        let oldCallStack = fromMaybe mempty hasCallStackBacktrace 
        in ec { hasCallStackBacktrace = Just (mergeCallStacks cs oldCallStack) }

instance ToExceptionAnnotation SomeExceptionAnnotation where
    updateExeptionContext =
        addToListOfSomeExceptionAnnotation

Encode the Backtraces record with GADTs.

IMO this seems unnecessarily awkward - I'd much rather have a regular ol' datatype. The awkwardness of the record is there to resolve the ambiguity of "What does it mean for CallStack to come before CostCenter?" and I think it's fine if we just say "ordering isn't really all that important."

IE, I'd want addCallStackToContext to merge the new CallStack with the existing CallStack if it is already defined. In annotated-exception, that means a pass over the list to find any existing CallStack entries - if found, merge, if not, place it in.

Hide the implicit parameter behind a synonym.

This seems like a really good idea 👍🏻

Preserve backtraces on rethrowing.

Yes this is table stakes imo. Seems like the proposal covers it:

Consequently, we propose to that catch and handle be modified to preserve exception context when an exception is thrown from a handler.

I would also like for catch, try, etc to have HasCallStack constraints, so that these can show up usefully in the CallStacks :) But that's already handled in exceptions and annotated-exception and doesn't need to be upstreamed.

Get rid of toExceptionWithContext

I think the benefit of toExceptionWithContext is that it allows an easy way to deal with exception contexts without invoking the ImplicitParams magic. IMO it's much nicer to write toExceptionWithContext e cxt than let ?exceptionContext = cxt in toException e.

This could also be done with explicit helpers, like giveExceptionContext :: ExceptionContext -> (HasExceptionContext => r) -> r and withExceptionContext :: HasExceptionContext => (ExceptionContext -> r) -> r.

Improvements to throwIONoBacktrace

I'm unsure about this. I would be really frustrated if someone used throwIONoBacktrace and it was impossible for me to add annotations or backtraces on it afterwards. Perhaps it's better to have catchNoBacktrace et al instead.

I would be less frustrated if my regular annotateIO calls still worked, but I would definitely be missing the CallStack that I want to preserve.

Another potential problem is the implicit params magic. Right now, HasCallStack is broken if any function call omits it:

a :: HasCallStack => Int
a = error "oh no"

b :: HasCallStack => Int
b = a

c :: Int
c = b

d :: HasCallStack => Bool -> Int
d True = b
d False = c

With backtracesEnabled using an implicit parameter, then you'd need to thread that constraint through, or it may be lost, and users would be confused as to why backtraces were getting collected in a call like:

main = disableBacktraces $ foo

foo :: HasExceptionContext => IO ()
foo = bar

-- no ExceptionContext in scope, what do?
bar :: IO ()
bar = throwIO "oh no"

@int-index
Copy link
Contributor

int-index commented Nov 29, 2022

@parsonsmatt If you read the specification I linked to at the end of my review (https://gist.github.com/int-index/750c6c292eb8266729adc61a5812a581), you'll find that many of your counterpoints do not apply.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment