core-libraries-committee icon indicating copy to clipboard operation
core-libraries-committee copied to clipboard

`base` changes for exception backtrace proposal

Open bgamari opened this issue 2 years ago • 34 comments

In https://github.com/ghc-proposals/ghc-proposals/pull/330 we propose a significant number of interface additions to base, as well as changes to a few central abstractions (namely, the Exception class and SomeException type). These are articulated in the "Proposed Change Specification" section of the proposal.

I have broken the GHC Proposal into five subproposals:

  • #198 proposes changing the top-level exception handler to use displayException
  • #200 describes the general-purpose exception annotation mechanism
  • #199 describes how this mechanism is used to capture backtraces
  • #201 proposes changes to ensure that HasCallStack backtraces are propagated such that they can be attached as context
  • #202 describes how exception context is preserved during rethrowing

The operative sections of the GHC proposal are reflected in the following CLC proposals:

Section Description Proposal
2.1 Annotations part 1
2.2 Representing backtraces part 2
2.3 Representing ExceptionContext part 1
2.4 Attaching context to exceptions part 1
2.5 Providing context to handlers part 1
2.6 Rethrowing part 4
2.7 Capturing backtraces part 2
2.8 HasCallStack improvements part 3
2.9 Asynchronous exceptions part 2
2.10 Modifying top-level handler #198
N/A Collecting nested exceptions and annotations part 5

bgamari avatar May 09 '23 22:05 bgamari

@parsonsmatt since the proposal quotes annotated-exception as prior art, could you please comment?

Bodigrim avatar May 10 '23 23:05 Bodigrim

@haskell/core-language-committee, I would also appreciate guidance on how we can address the issue brought up in #145.

To summarize: in order to provide stack trace decoding for the Haskell stack, we will need to make some or all of ghc-heap (which is exposes internal implementation with a high likelihood of changing even across minor GHC versions) available to base. Naturally, this is complicated by the fact that ghc-heap rather unavoidably depends upon base. Given the constraints of our packaging system, I see a few ways of accomplishing this:

  1. moving the relevant modules into base, advertising them loudly as internal and not subject to the PVP, and reexporting them from ghc-heap (which will continue to be their canonical home)
  2. initiating a split such as suggested in #145, moving the relevant modules and reexporting them from ghc-heap (which will continue to be their canonical home). Naturally, this would allow us to continue to version these declarations under the PVP.
  3. option (1) but retire ghc-heap
  4. option (2) but retire ghc-heap

Thoughts?

While we could in principle proceed with https://github.com/ghc-proposals/ghc-proposals/pull/330 without addressing this, I worry that the utility of the proposal would be severely compromised. Afterall, Haskell stack decoding (that is, IPEBacktrace) is very likely to be the most widely-used of the four backtrace mechanisms introduced in the proposal.

bgamari avatar May 11 '23 17:05 bgamari

I helped a small amount with the proposal and am in favor. While there are some design quibbles re exactly what information gets retained and how to handle duplication, they aren't going to alter my approval of it - if I don't get my favorite design in base, then I'll continue to maintain annotated-exception with the behavior that I want.

parsonsmatt avatar May 11 '23 18:05 parsonsmatt

moving the relevant modules into base, advertising them loudly as internal and not subject to the PVP

I think I've said it too many times by now, but -1 to any PVP exclusion. Please provide evidence that it has been a problem in the past and proper motivation why we can't achieve compliance. "Makes our workflow easier" is not sufficient motivation.

hasufell avatar May 12 '23 02:05 hasufell

I think I've said it too many times by now, but -1 to any PVP exclusion. Please provide evidence that it has been a problem in the past and proper motivation why we can't achieve compliance. "Makes our workflow easier" is not sufficient motivation.

GHC as a compiler explicitly provides no ABI compatibility guarantees, even across minor releases. The reason that the ghc-heap package exists is so that it can truthfully reflect the ABI implemented by GHC (in particular, heap structure) while being versioned independently. Making the major version of base depend upon something that we explicitly claim to be an unstable implementation detail which the user cannot otherwise observe is simply not tenable.

This now puts us in a difficult situation: in order to implement a standard library feature we now find that we need access to ghc-heap. The natural way to accomplish this would be to introduce a package --- call it, for instance, ghc-base --- which we can freely bump across GHC versions without affecting users of base. This package would contain both ghc-heap as well as the bits of base that require ghc-heap. This is option (2), which we are discussing in #145, and is my preferred option.

However, as there appears to be some resistance to #145, I feel obligated to suggest alternatives. The most plausible alternative I can think of is option (3), which sadly requires a targeted PVP exemption. In practice, I don't believe this would be problematic as these definitions are only exposed to be reexported by ghc-heap; users should not be using them directly.. While our package system doesn't give us the tools to prevent users from using them entirely, I think a {-# HADDOCK hide #-} pragma would be a sufficient deterrent.

If we nevertheless think that this is unacceptable, then we are left only with options (2) and (4). I simply do not see a way around this.

bgamari avatar May 12 '23 14:05 bgamari

GHC as a compiler explicitly provides no ABI compatibility guarantees, even across minor releases. The reason that the ghc-heap package exists is so that it can truthfully reflect the ABI implemented by GHC (in particular, heap structure) while being versioned independently.

Could you possibly elaborate a bit?

ghc-heap releases have versions 9.0.1, 9.2.1, 9.2.2, 9.4.1, 9.6.1, which means that major version bumps of ghc-heap perfectly match major version bumps of base. Is ghc-heap itself PVP-compliant?

AFAICT the only difference between ghc-heap-9.2.1 and ghc-heap-9.2.2 is adding

#if __GLASGOW_HASKELL__ >= 811 && __GLASGOW_HASKELL__ < 902
  | BlockedOnIOCompletion
#endif

which seems to be fixing an outright bug and not reflecting an ABI difference between GHC 9.2.1 and 9.2.2 (in which case I would expect to see __GLASGOW_HASKELL_PATCHLEVEL1__ checked).

Bodigrim avatar May 13 '23 11:05 Bodigrim

@Bodigrim, sure.

Currently we version ghc-heap according to the GHC version number for simplicity. Moreover, several minor GHC releases have required no changes to ghc-heap at all (hence there being . However, when we find we need to make a change requiring a major version bump we will revisit this. The important thing here is that we have the ability to make versioning decisions independently of base.

bgamari avatar May 13 '23 12:05 bgamari

Currently we version ghc-heap according to the GHC version number for simplicity.

So there's no particular reason to not be strictly PVP compliant and deviate from the GHC version scheme? Boot packages already correspond to a particular GHC version. I don't see a problem doing the same here.

Is it more work? Maybe.

hasufell avatar May 13 '23 12:05 hasufell

@hasufell AFAIU the GHC ABI wrt heap structure (accidentally) happened to remain stable within each major version of GHC so far, but @bgamari is reluctant to guarantee that it is to remain so in future.

I'm not really sure what I think about it: if the past behaviour is any predictor of the future, there does not seem to be too much pressure to change heap structure in patch versions of GHC. Maybe it's time to commit to keep at least this part of ABI stable within each single major release? Especially if we expect people to write and operate heap-analysing tools: it would be unfortunate if they stop working after a minor GHC update.

Bodigrim avatar May 13 '23 12:05 Bodigrim

@Bodigrim I'm a bit confused here.

I don't see a particular good reason to have a library versioned exactly like GHC (even if it contains internals) other than the fact you don't have to think about the versioning at all.

Do people expect that there are no major PVP bumps of boot libraries in a minor GHC bump? I think that doesn't have to be guaranteed when it's about internals. We can be more aggressive there.

hasufell avatar May 13 '23 13:05 hasufell

I'm not really sure what I think about it: if the past behaviour is any predictor of the future, there does not seem to be too much pressure to change heap structure in patch versions of GHC. Maybe it's time to commit to keep at least this part of ABI stable within each single major release?

This is not an outcome that we would be willing to entertain. Ultimately, heap structure is the very definition of "implementation detail" and one that we need to be able to iterate on freely. Of course, as always, we will attempt to keep breakage to a minimum in minor releases. However, we cannot commit to a situation which forecloses such changes entirely.

bgamari avatar May 14 '23 13:05 bgamari

Do people expect that there are no major PVP bumps of boot libraries in a minor GHC bump?

Yes, this is the expectation. Currently Stackage can accomodate a minor GHC bump almost immediately, within the same LTS series. If a minor GHC bump incurs major bumps of boot libraries (be it base, or ghc-heap, or ghc-base), it would require a new Stackage LTS series.

That said, I'd rather sacrifice the speed of Stackage adoption than PVP compliance. After all, the very point of Stackage LTS evolution is that no one sneaks breaking changes underneath.

Bodigrim avatar May 15 '23 21:05 Bodigrim

FWIW, I'm totally fine with PVP exclusions, provided the modules are named in a manner that suggests that and documented as excluded. While I do prefer to see packages extracted instead, I can't deny that base and other boot packages have a much harder time with that. We have proposals in place to mitigate these issues in the future. I'd rather not hold up good new stuff for ideological purity on a version scheme which essentially no one in the Haskell ecosystem actually follows, anyway (lookin at you, open imports without minor version bump limits).

parsonsmatt avatar May 17 '23 20:05 parsonsmatt

I believe that the "Exceptions backtrace" proposal is highly important and beneficial for the Haskell community. Not having a great observability story in Haskell when debugging problems had hurt many Haskellers in the past (including myself).

However, the proposal is huge. If my summary is correct, in terms of API changes, the proposal has:

  • 3 new modules
  • 7 new data types
  • 1 new constraint synonym
  • 1 new typeclasses
  • 8 new instances
  • 14 new functions
  • 2 breaking changes
    • Adding a new constraint to SomeException
    • Modifying the behaviour of global exception handler to use displayException instead of show
  • 1 new method to Exception (with the default definition)
  • 1 new internal non-exported function (which looks generally useful and just waits for someone to ask to reexport it one day)
  • 3 additions of HasCallStack to existing functions

This looks like a lot just to comprehend 🤯 Not all popular Haskell libraries have such big API, and we're talking only about adding backtraces to exceptions here.

Unlike @parsonsmatt, I wasn't following this proposal closely, and I don't have the capacity to go through it thoroughly and read all the 238 comments in the conversation to catch various subtleties in the API.

To help CLC be more efficient, I propose the proposal authors to write the following (I also think it'll be helpful for the general public as well):

  • A summary of an API
  • Usage examples
  • How much of this API is user-facing?
  • Compilation errors in case of breaking changes
  • Migration guide

I want us to work together to find the best way of introducing this work without disrupting existing Haskell users too much. But this is going to be lots of work. I don't see a fast way here, so a few points from my POV:

  • Advertise the change (and its summary) to a broader audience. People are generally less unhappy about breaking changes when they make sense. Still, some upfront communication will help the proposal.
  • The migration part of the proposal says that the migration is trivial, yet no specific migration guide is provided. Also, I think that using head.hackage for assessing breaking changes proved to be insufficient, so at least a proper CLC assessment is required.
    • image
  • I'm 100% sure some people discover deficiencies in the API and will require changes after it's implemented. I appreciate all the great effort that was put into the proposal, but you can't predict everything, and you can discover shortcomings only after real usage. We can cushion the hit by advertising irrelevant and non-user-facing parts of the API as internal. But this requires a proper summary and a split between user-facing and internal API.
  • Would it be possible to split the proposal into several phases? E.g. add non-controversial parts first (if they still can be useful) and the remaining parts after. For example, there was a separate proposal to add HasCallStack to some list functions. I don't expect much controversy for throwIO, throwTo, and throw but it looks like something that requires some separate consideration.

chshersh avatar May 28 '23 17:05 chshersh

@bgamari could the proposal be more explicit about

  1. What aspects of the existing API of base are changed?
  2. What types and functions are new?

I think that most of the proposal is (2), but there is some impact on (1). Could you make that precise?

simonpj avatar Jun 12 '23 16:06 simonpj

@bgamari a summary of the proposal as suggested above would be incredibly helpful indeed.

Bodigrim avatar Jul 11 '23 21:07 Bodigrim

Indeed it is on my list.

bgamari avatar Jul 12 '23 02:07 bgamari

I have factored out #198 as the first piece of this proposal. A second piece should be coming shortly.

bgamari avatar Aug 28 '23 17:08 bgamari

This proposal has been partitioned into four sub-proposals:

  • #198 proposes changing the top-level exception handler to use displayException
  • #200 describes the general-purpose exception annotation mechanism
  • #199 describes how this mechanism is used to capture backtraces
  • #201 proposes changes to ensure that HasCallStack backtraces are propagated such that they can be attached as context
  • #202 describes how exception context is preserved during rethrowing

The operative sections of the GHC proposal are reflected in the following CLC proposals:

Section Description Proposal
2.1 Annotations part 1
2.2 Representing backtraces part 2
2.3 Representing ExceptionContext part 1
2.4 Attaching context to exceptions part 1
2.5 Providing context to handlers part 1
2.6 Rethrowing part 4
2.7 Capturing backtraces part 2
2.8 HasCallStack improvements part 3
2.9 Asynchronous exceptions part 2
2.10 Modifying top-level handler #198

bgamari avatar Aug 28 '23 19:08 bgamari

On re-reading @chshersh's comment this morning, it became clear to me that I never addressed the impression that the proposal contains breaking changes.

To be clear: this proposal contains no breaking changes. This is no accident; our exception infrastructure is used by a significant fraction of programs and often in subtle ways. Breaking it is simply too high a cost to bear and consequently avoiding breakage was a requirement which dictated the proposal's development. The defaulting mechanism proposed in Section 8.6 is part of the proposed implementation and the complexity that it requires is minor enough that deprecation doesn't seem to be necessary.

As to the size of the interface, I hope that the partitioning given above helps. As you will see, none of the individual parts are particularly large. The majority of the surface area serves the general-purpose annotation mechanism, the design of which seems fairly "obvious" save a few design facets which were dictated by compatibility concerns (e.g. the use of implicit parameters and the HasExceptionContext constraint synonym). I suspect that the rest of the proposals' surface area will not have a significant number of direct users.

bgamari avatar Aug 28 '23 20:08 bgamari

Thanks @bgamari for the tremendous effort you've put in to this proposal. It will be a massive benefit to have ubiquitous exception backtraces available in GHC.

Regarding "breaking changes", I believe you and @chshersh are interpreting the term slightly differently. I understand you to mean that this proposal will not cause any existing code to cease compiling. Which is very impressive in itself!

On the other hand, the combination of #198 and #200 means that the top-level handler will print the exception context for all exceptions to stderr. Which is what we want, but it is potentially a breaking change for users relying on the stderr output (e.g. it may cause testsuite failures).

I think the potential breakage is clearly justified and that it will be sufficient to make sure the change is well publicised (which I suspect will happen naturally, given the massive positive impact of all exceptions having backtraces by default). We could also do with a more explicit migration story. Presumably users wanting the old behaviour back can call setUncaughtExceptionHandler, but do we have a convenient handler for them to install?

adamgundry avatar Aug 29 '23 20:08 adamgundry

I think the potential breakage is clearly justified and that it will be sufficient to make sure the change is well publicised (which I suspect will happen naturally, given the massive positive impact of all exceptions having backtraces by default). We could also do with a more explicit migration story. Presumably users wanting the old behaviour back can call setUncaughtExceptionHandler, but do we have a convenient handler for them to install?

We don't currently and currently it's not possible to write the top-level handler outside of base as it relies internally on the C errorBelch function to write to stderr. We cannot in general use hPutStrLn instead since the exception may be due to the IO manager. We could in general expose a generalization of it like:

mkTopExceptionHandler :: (SomeException -> String)
                      -> (SomeException -> IO ())
mkTopExceptionHandler showExc exc = do
    hFlush stdout `catchAny` (\ _ -> return ())
    withCString "%s" $ \cfmt ->
      withCString (showExc exc) $ \cmsg ->
        errorBelch cfmt cmsg

However, I would argue that relying on the top-level handler when predictable output is wanted should be in general avoided. If the user wants predictable output then they should rather just use handle (and async, in a concurrent program). e.g.:

main = handle onErr $ do ...

onErr :: SomeException -> IO ()
onErr = hPutStrLn stderr . show 

bgamari avatar Aug 30 '23 17:08 bgamari

However, I would argue that relying on the top-level handler when predictable output is wanted should be in general avoided. If the user wants predictable output then they should rather just use handle (and async, in a concurrent program).

Indeed. Unfortunately, I suspect people will have been doing so, and it would be nice if we had a migration story that was essentially "call this function at the start of main to get back the old behaviour" rather than "make sure to install an exception handler on main and every forked thread".

I think mkTopExceptionHandler would be worth providing for that reason. I wonder if it's worth going further and defining

oldDefaultHandler :: SomeException -> IO ()
oldDefaultHandler = mkTopExceptionHandler $ \ se@(SomeException ex) ->
         case cast ex of
               Just Deadlock -> "no threads to run:  infinite loop or deadlock?"
               _             -> show se

so the magic incantation would then be simply

main = do setUncaughtExceptionHandler oldDefaultHandler
          ... 

although it's not so obvious whether maintaining the old behaviour for Deadlock exceptions is really worth the candle.

adamgundry avatar Aug 30 '23 20:08 adamgundry

I think mkTopExceptionHandler would be worth providing for that reason. I wonder if it's worth going further and defining oldDefaultHandler

We can do that. If we do, perhaps we want to provide oldDefaultHandler instead of mkTopExceptionHandler. One concern that I have about mkTopExceptionHandler is that it invites the user to use setUncaughtExceptionHandler, where I would argue that most applications really shouldn't need to do so. Providing oldDefaultHandler instead makes it more clear that the usage is for legacy reasons.

bgamari avatar Aug 31 '23 12:08 bgamari

@Bodigrim, just a gentle ping to ensure that there isn't something I need to do to trigger a vote.

bgamari avatar Sep 13 '23 20:09 bgamari

The proposal is massive. I'm not sure any of us had enough time yet to go through all of it and evaluate impact, breaking changes etc.

I certainly have not yet.

hasufell avatar Sep 14 '23 03:09 hasufell

Sure, no worries at all. I just wanted to make sure this wasn't blocked on me.

One thing I would like to clarify is that the only breaking change (that I can discern) implied by these proposals is in the preparatory work of #198. The remaining body of the proposal was crafted in such a way to avoid affecting the compilation or runtime behavior of the existing programs (beyond having access to additional context when wanted).

bgamari avatar Sep 14 '23 17:09 bgamari

@Bodigrim @hasufell Would you be able to comment on how soon you foresee the CLC being in a position to make a decision about this proposal, or what is needed to make progress?

I certainly understand that it includes significant changes, and that the CLC has limited volunteer time. However, this is a very significant improvement that will benefit many users, has been extensively discussed already through the GHC proposals process, and @bgamari has gone to some effort to break it down into reasonable-sized sub-proposals for more convenient consideration by the CLC. Thus if possible it would be great to be able to get it merged in good time for GHC 9.10.

adamgundry avatar Oct 12 '23 10:10 adamgundry

I will try to review it this weekend. I wonder if it makes sense for @bgamari to give a walkthrough in some form, but my guess is it's better to just do it async and ask specific questions.

Does it make sense to look at the sub-proposals in isolation first? So maybe we start with part 1 first and give the committee a soft deadline of 4 weeks @Bodigrim?

hasufell avatar Oct 12 '23 10:10 hasufell

Yes, we shall vote subproposals one by one.

@adamgundry thanks for the reminder, and I'm sorry for being so slow on this.

Bodigrim avatar Oct 12 '23 17:10 Bodigrim