haskell / core-libraries-committee

95 stars 15 forks source link

`base` changes for exception backtrace proposal #164

Closed bgamari closed 3 months ago

bgamari commented 1 year ago

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:

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
Bodigrim commented 1 year ago

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

bgamari commented 1 year ago

@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.

parsonsmatt commented 1 year ago

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.

hasufell commented 1 year ago

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.

bgamari commented 1 year ago

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.

Bodigrim commented 1 year ago

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).

bgamari commented 1 year ago

@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.

hasufell commented 1 year ago

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.

Bodigrim commented 1 year ago

@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.

hasufell commented 1 year ago

@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.

bgamari commented 1 year ago

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.

Bodigrim commented 1 year ago

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.

parsonsmatt commented 1 year ago

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).

chshersh commented 1 year ago

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:

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):


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:

simonpj commented 1 year ago

@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?

Bodigrim commented 1 year ago

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

bgamari commented 1 year ago

Indeed it is on my list.

bgamari commented 1 year ago

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

bgamari commented 1 year ago

This proposal has been partitioned into four sub-proposals:

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 commented 1 year ago

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.

adamgundry commented 1 year ago

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?

bgamari commented 1 year ago

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 
adamgundry commented 1 year ago

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.

bgamari commented 1 year ago

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 commented 12 months ago

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

hasufell commented 11 months ago

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.

bgamari commented 11 months ago

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).

adamgundry commented 11 months ago

@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.

hasufell commented 11 months ago

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?

Bodigrim commented 11 months ago

Yes, we shall vote subproposals one by one.

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

bgamari commented 10 months ago

@hasufell, thank you! I would be very happy to have a call with any of the CLC members who are interested to walk through the design and consequences of the proposal.

bgamari commented 9 months ago

Part 1 (#200) has been approved.

It would be appreciated if we could get some eyes on Part 2 (#199); GHC 9.10 is approaching quite quickly and it would be a shame to see this feature slip yet another release cycle.

bgamari commented 8 months ago

Can I suggest that we begin consideration of Steps 3 and 4 in parallel? These are small changes which are mutually orthogonal and I'm afraid we are running quite low on time before 9.10.

mixphix commented 5 months ago

It seems only parts 4 and 5 remain for consideration. @bgamari could you please update this ticket with the details for Part 5 (Annotation collection) from #250? Are the implementations complete, or detailed enough that we can approve an implementation plan?

Bodigrim commented 3 months ago

All subproposals except #202 has been approved, while #202 is dormant to be reanimated later. Let me close this meta ticket.