alt-romes
commented
3 days ago I'm going to ping those involved in the original message design to initiate discussion: @tomjaguarpaw @tbidne @adamgundry @parsonsmatt
Open alt-romes opened 6 days ago
alt-romes
commented
3 days ago I'm going to ping those involved in the original message design to initiate discussion: @tomjaguarpaw @tbidne @adamgundry @parsonsmatt
tomjaguarpaw
commented
3 days ago That seems fine by me.
tbidne
commented
2 days ago The rethrown example is compelling. Agreed that this is an improvement. To keep discussion centralized, I'll respond to @bgamari's questions on the original MR here:
Exception type data added to
SomeException'sdisplayException, rather than the handler.it is not clear to me why this implementation approach was taken. This doesn't appear to be consistent with what was proposed in the CLC proposal and means that users have no way of opting out of this output. Perhaps you can shed some light on the reasoning here?
That decision was based on the discussion in the 2nd issue. The (light) consensus was that changing the format from:
<msg>
<backtraces>
<type>to
<msg>
<type>
<backtraces>improved clarity, and doing so required moving the type info to the instance, since that is where msg and backtraces were handled.
This PR changes it to
<type>
<msg>
<backtraces>so AFAICT it would be possible now to move the <type> to the handler instead of the instance. But perhaps this is less important now since the message is far more compact. Moreover, given that we can have multiple exceptions in play with WhileHandling, I think it makes sense to leave it on the instance since that would ensure each exception receives the type info.
As far as "opting out", that would of course be possible with a global mechanism like setBacktraceMechanismState, but it's more complexity that no one asked for at the time, and hopefully it's less important with the proposed concise output.
adamgundry
commented
2 days ago Thanks for taking a careful look at the quality of exception messages @alt-romes, I think this is very valuable.
On reflection, it does seem to me problematic for displayException to show anything other than the user-friendly message, because it is the normal mechanism for developers to use when rendering exceptions for end users, and they may do so in contexts where backtraces/types are not desired. So I think displayException for SomeException should give only the displayException of the underlying exception, but we could have another function that displays the full glorious details, expose that function to users and call it in the default handler. I regret not picking up on this in the previous discussion.
alt-romes
commented
2 days ago I'm currently making an effort to get exceptions as a whole in a better state before the 9.12 release.
As I've come to realize when implementing WhileHandling/re-throwing of exceptions, there are a few other minor bugs and improvements hanging around that I believe we should act on swiftly, but which do require GHC proposals.
@adamgundry suggested I try to keep the discussion within a single CLC proposal for all the incremental improvements to the exceptions. Accordingly, I've updated this proposal with additional part 2 to 5.
Thank you for commenting on the proposal for better exception messages already. The part 1 of the exception (which you originally read) is kept unchanged. I would appreciate it if you could comment on the new parts of the proposal for improving the exceptions as a whole.
I've implemented these suggestions in this MR
tomjaguarpaw
commented
2 days ago I don't use exceptions myself[^1] so I'll defer to exception users regarding what they need. However, I am surprised about this:
[
displayException] is the normal mechanism for developers to use when rendering exceptions for end users
I always understood than an uncaught exception was considered hard abort, if not programmer error: something has gone unexpectedly wrong. In such cases one typically wants as much information as possible dumped to the screen to help with debugging or bug reporting. That seems to me approximately what displayException is currently providing, and I thought that was good. But am I now to understand that displayException is used for formatting error messages intended to be read by end users? If so, should it really be? Why not instead catch the exception and format a nice message of your own choosing?
[^1]: At least not in the standard throw/throwIO and then catch in IO (or not) – instead I use well-scoped exceptions, i.e. Either/MTL/Bluefin etc.
parsonsmatt
commented
2 days ago If so, should it really be? Why not instead catch the exception and format a nice message of your own choosing?
The dynamic nature of SomeException means that you can do this, if you explicitly list out the types you expect to catch. And if you missed one, you need a different message.
foo `catches`
[ Handler (\KnownException -> print0)
, Handler (\(OtherKnownException a) -> print1 a)
, Handler (\(SomeException unknown) -> putStrLn $ displayException unknown)
]In that fallback case, all we "know" is (forall e. Exception e => e), which means we only have the methods on Exception and Show to do anything useful with it.
alt-romes
commented
2 days ago You implement displayException as part of instance Exception that describes how to print an exception. Then, when an exception is not caught, it will be handled by uncaughtExceptionHandler (which can be set with setUncaughtExceptionHandler).
By default, the uncaught excp. handler prints out as much information as possible, as you said. At the moment, this includes the type of the exception and the callstack.
However, if you have business-logic exceptions being thrown in your program, you may want to override the exception handler to print out the exception cleanly to the user in a way custom to your program.
The problem is the handler receives the exception wrapped in SomeException. You'd want to have displayException (SomeException e) = displayException e so that your definition of displayException is the one printed out.
But at the moment it's not. displayException @SomeException will also display type and backtrace.
The proposal is to define displayExceptionWithInfo in terms of displayException, but which wraps it with the type and callstack. Then, the default handler uses displayExceptionWithInfo which will print all the details. However, you can override the handler and call displayException -- opting out of the extra details
But am I now to understand that
displayExceptionis used for formatting error messages intended to be read by end users? If so, should it really be? Why not instead catch the exception and format a nice message of your own choosing?
You could have a top-level handler that tries to catch all exceptions relevant to the user and manually prints them differently. But I also think it is clean to have proper exceptions relevant to the user simply being surfaced to the top level and handled by the exception handler.
tomjaguarpaw
commented
2 days ago By default, the uncaught excp. handler prints out as much information as possible, as you said. At the moment, this includes the type of the exception and the callstack.
Right, so far I understand.
However, if you have business-logic exceptions being thrown in your program, you may want to override the exception handler to print out the exception cleanly to the user in a way custom to your program.
But now I don't understand. Why override the exception handler? Why not catch the exception, and then do whatever you like with it? I feel I must be missing something that's very clear to everyone else.
You could have a top-level handler that tries to catch all exceptions relevant to the user and manually prints them differently. But I also think it is clean to have proper exceptions relevant to the user simply being surfaced to the top level and handled by the exception handler.
That doesn't seem clean to me at all. Since I don't use exceptions like this I may be missing something that seems obvious to others, but letting unhandled exceptions get to the default, top-level, handler, and expecting them to be printed nicely for users, doesn't seem, to me, like something we should be supporting.
alt-romes
commented
2 days ago That doesn't seem clean to me at all. Since I don't use exceptions like this I may be missing something that seems obvious to others, but letting unhandled exceptions get to the default, top-level, handler, and expecting them to be printed nicely for users, doesn't seem, to me, like something we should be supporting.
(Even if there is disagreement on the merits of this style of programming, the proposed solution enables strictly more styles while keeping everything the same for any user who doesn't touch the default uncaught exception handler).
I think SomeException should ideally be "transparent", since it's just an existential wrapper over an Exception. And with the proposal we do indeed get:
displayException (SomeException e) = displayException eWhile keeping exactly the same all of the extra information when exceptions are uncaught and reported.
adamgundry
commented
2 days ago But now I don't understand. Why override the exception handler? Why not catch the exception, and then do whatever you like with it?
One answer: the exception may be thrown in library code I don't control, on a thread forked by the library. There's no way AFAIK to add an exception handler in such a case. Obviously this is a bad/buggy library, but such libraries exist! Thus the application programmer needs to be able to specify a top-level exception handler to report the exception using whatever logging mechanism is appropriate for their application.
For (a not entirely hypothetical) example, I'm writing a concurrent web server with a multitude of request-handling and background worker threads. I can add a top-level handler so that if any of these threads dies with an exception, the exception gets serialised to JSON and sent to a cloud logging service. With @alt-romes's proposal, in the logging code I can just call displayException to get the human-friendly description to populate one field of the log object (plus populate other fields as needed e.g. with types/backtraces). Whereas with the status quo, I have to be careful to call displayException not at type SomeException but only on the inner exception object (otherwise I'll end up accidentally logging the backtraces in the "description" field).
tomjaguarpaw
commented
1 day ago @alt-romes
(Even if there is disagreement on the merits of this style of programming, the proposed solution enables strictly more styles while keeping everything the same for any user who doesn't touch the default uncaught exception handler).
That sounds great! But I don't understand. This proposal suggests a change to the behaviour of the default uncaught exception handler, doesn't it?
I think
SomeExceptionshould ideally be "transparent", since it's just an existential wrapper over anException. And with the proposal we do indeed get:
displayException (SomeException e) = displayException eThis seems pretty reasonable to me, because the thing that is thrown an caught is always a SomeException, right? And then we only check what instance of Exception it is by casting it with fromException. So it seems to me to never make sense to show the top-level SomeException wrapper. But I'm yet again confused: why was the wrapper ever shown? Is this a long-standing issue?
(To be honest I'm beginning to think that it would be easier to deal with this proposal in separate parts. Some parts, like this one, seem like they would be quite quick to resolve.)
@adamgundry
the exception may be thrown in library code I don't control, on a thread forked by the library. There's no way AFAIK to add an exception handler in such a case
I don't follow. Could you elaborate? Maybe with an example?
alt-romes
commented
1 day ago The change to the behavior of the default handler together with the change to displayException @SomeException keeps the program output on an uncaught exception exactly the same.
It's about moving the part adding the call stack to the handler from the instance, achieving all our goals without changing the default output.
tomjaguarpaw
commented
1 day ago why was the wrapper ever shown?
Ah, looks like the wrapper was never shown, but the difference is that recently we added more stuff to the displayException of SomeException.
Is the idea that should "more stuff" shouldn't be in the displayException of SomeException, but should rather be added by a handler? If so I think things are becoming clearer for me. Could you confirm?
The change to the behavior of the default handler together with the change to
displayException @SomeExceptionkeeps the program output on an uncaught exception exactly the same.
Great, I must have missed that. I think that's worth calling out prominently and explicitly in the proposal.
alt-romes
commented
1 day ago Is the idea that should "more stuff" shouldn't be in the
displayExceptionofSomeException, but should rather be added by a handler? If so I think things are becoming clearer for me. Could you confirm?
Yes, that's exactly right.
michaelpj
commented
1 day ago This looks good to me. I only have one small comment on Part 1 and the interaction with rethrown exceptions. It would be nice if the WhileHandling annotations were the last thing printed. As it is, you can see in your example that you have a substantial block for the WhileHandling exception, and then you have to "pop" your reading context back out to the top-level exception to read the backtraces. That is, I think it would be nice if we could get the following formatting for you example:
cgrun025: Uncaught exception ghc-internal:GHC.Internal.Exception.ErrorCall:
hello, error
CallStack (from HasCallStack):
error, called at cgrun025.hs:25:75 in main:Main
HasCallStack backtrace:
collectBacktraces, called at libraries/ghc-internal/src/GHC/Internal/Exception.hs:92:13 in ghc-internal:GHC.Internal.Exception
toExceptionWithBacktrace, called at libraries/ghc-internal/src/GHC/Internal/Exception.hs:127:5 in ghc-internal:GHC.Internal.Exception
error, called at cgrun025.hs:25:75 in main:Main
While handling ghc-internal:GHC.Internal.IO.Exception.IOException:
__WURBLE__: getEnv: does not exist (no environment variable)
HasCallStack backtrace:
collectBacktraces, called at libraries/ghc-internal/src/GHC/Internal/Exception.hs:92:13 in ghc-internal:GHC.Internal.Exception
toExceptionWithBacktrace, called at libraries/ghc-internal/src/GHC/Internal/IO.hs:284:11 in ghc-internal:GHC.Internal.IO
throwIO, called at libraries/ghc-internal/src/GHC/Internal/IO/Exception.hs:315:19 in ghc-internal:GHC.Internal.IO.Exception
ioException, called at libraries/ghc-internal/src/GHC/Internal/System/Environment.hs:192:26 in ghc-internal:GHC.Internal.System.EnvironmentIs the idea that should "more stuff" shouldn't be in the
displayExceptionofSomeException, but should rather be added by a handler? If so I think things are becoming clearer for me. Could you confirm?Yes, that's exactly right.
OK, great. I see this is actually spelled out in the following sentence:
Updating the default handler and the instance guarantees that exceptions by default still get printed with the type and backtrace, as they currently do.
but I think it would be worth calling out more explicitly that this change doesn't change the behaviour of the default handler.
I propose that
ErrorCallstops propagating a callstack manually
Currently,
data ErrorCall = ErrorCallWithLocation String String(see https://www.stackage.org/haddock/lts-22.35/base-4.18.2.1/Control-Exception.html#t:ErrorCall) Are you suggesting changing this constructor? Perhaps changing it back to just ErrorCall String? (There is a already a pattern synonym ErrorCall.)
Since I'm already changing this code, let's finally remove
errorWithStackTrace, which has been deprecated since GHC 8.0 (2015), i.e. for 9 years.
I'm against this. errorWithStackTrace does not require any ongoing support or maintenance. There is a chance (albeit probably tiny) that removing it could break someone's code and make them hate Haskell, and I don't see why we should take that risk. I appreciate the desire to "tidy up", so as a compromise I would accept the following, which at the point of use tells the user exactly what they have to do to fix their code:
errorWithStackTrace ::
Unsatisfiable
(Text "'errorWithStackTrace' no longer exists. Use 'error' instead.") =>
String ->
a
errorWithStackTrace = unsatisfiabletest28.hs:12:7: error: [GHC-22250]
• 'errorWithStackTrace' no longer exists. Use 'error' instead.
• In the expression: errorWithStackTrace "error message"
In an equation for ‘foo’: foo = errorWithStackTrace "error message"
|
12 | foo = errorWithStackTrace "error message"
| ^^^^^^^^^^^^^^^^^^^(And to be clear, I'm +1 on the rest of the proposal.)
alt-romes
commented
15 hours ago Currently,
data ErrorCall = ErrorCallWithLocation String String(see https://www.stackage.org/haddock/lts-22.35/base-4.18.2.1/Control-Exception.html#t:ErrorCall) Are you suggesting changing this constructor? Perhaps changing it back to just
ErrorCall String? (There is an already a pattern synonymErrorCall.)
That's right. The commit is https://gitlab.haskell.org/ghc/ghc/-/merge_requests/13301/diffs?commit_id=c0a2cb447c3fec71edc6653fe7f26619b751ce5f.
We're deleting the ErrorCall pattern synonym, deleting the ErrorCallWithLocation constructor, and instead making ErrorCall :: String -> ErrorCall the only constructor of ErrorCall.
This means that nothing changes for those already using the pattern synonym.
Since I'm already changing this code, let's finally remove
errorWithStackTrace, which has been deprecated since GHC 8.0 (2015), i.e. for 9 years.I'm against this.
errorWithStackTracedoes not require any ongoing support or maintenance. There is a chance (albeit probably tiny) that removing it could break someone's code and make them hate Haskell, and I don't see why we should take that risk. I appreciate the desire to "tidy up", so as a compromise I would accept the following, which at the point of use tells the user exactly what they have to do to fix their code:
In that case I've amended the proposal to not touch errorWithStackTrace. I had no strong motivation to do this other than clean up. FWIW, the 9-year-old deprecation message already refers error as the supposed substitute:
-- | Like the function 'error', but appends a stack trace to the error
-- message if one is available.
--
-- @since base-4.7.0.0
{-# DEPRECATED errorWithStackTrace "'error' appends the call stack now" #-}
-- DEPRECATED in 8.0.1
errorWithStackTrace :: String -> a
errorWithStackTrace x = unsafeDupablePerformIO $ throwIO (ErrorCall x)@michaelpj I think your suggestion would look better, but it's not clear to me how to achieve this with the exception annotations.
However, for the WhileHandling display I intend to use vertical bars | to better distinguish nested levels:
ghc: Uncaught exception ghc-9.11-inplace:GHC.Utils.Panic.GhcException:
default output name would overwrite the input file; must specify -o explicitly
Usage: For basic information, try the `--help' option.
While handling ghc-9.11-inplace:GHC.Utils.Panic.GhcException:
|
| default output name would overwrite the input file; must specify -o explicitly
| Usage: For basic information, try the `--help' option.
|
| While handling ghc-9.11-inplace:GHC.Utils.Panic.GhcException:
| |
| | default output name would overwrite the input file; must specify -o explicitly
| | Usage: For basic information, try the `--help' option.
| |
| | While handling ghc-9.11-inplace:GHC.Utils.Panic.GhcException:
| | |
| | | default output name would overwrite the input file; must specify -o explicitly
| | | Usage: For basic information, try the `--help' option.
| | |
| | | HasCallStack backtrace:
| | | collectBacktraces, called at libraries/ghc-internal/src/GHC/Internal/Exception.hs:91:13 in ghc-internal:GHC.Internal.Exception
| | | toExceptionWithBacktrace, called at libraries/ghc-internal/src/GHC/Internal/Exception.hs:83:32 in ghc-internal:GHC.Internal.Exception
| | | throw, called at compiler/GHC/Utils/Panic.hs:180:21 in ghc-9.11-inplace:GHC.Utils.Panic
| |
| | HasCallStack backtrace:
| | bracket_, called at libraries/semaphore-compat/src/System/Semaphore.hs:320:23 in semaphore-compat-1.0.0-inplace:System.Semaphore
|
| HasCallStack backtrace:
| throwIO, called at libraries/exceptions/src/Control/Monad/Catch.hs:371:12 in exceptions-0.10.7-inplace:Control.Monad.Catch
| throwM, called at libraries/exceptions/src/Control/Monad/Catch.hs:860:84 in exceptions-0.10.7-inplace:Control.Monad.Catch
| onException, called at compiler/GHC/Driver/Make.hs:2986:23 in ghc-9.11-inplace:GHC.Driver.Make
HasCallStack backtrace:
bracket, called at compiler/GHC/Driver/Make.hs:2953:3 in ghc-9.11-inplace:GHC.Driver.MakeThis makes it more-OK that the call stack appears at the bottom only. Also, the Haskell call stack is read from bottom-to-top, so that's also aligned with the outermost exception showing at the bottom.
CLC Proposal for Exceptions Redesign
Part 1
Recently, https://github.com/haskell/core-libraries-committee/issues/231 and https://github.com/haskell/core-libraries-committee/issues/261, two proposals regarding exceptions and backtraces, have been accepted and implemented. These proposals display new information on thrown exceptions: type, module, and package of the exception that was thrown.
However, I believe the formatting/layout of this new information, together with the changes added to the header of the exception message, cause the exception message as a whole to be confusing, unhelpful, and noisy -- undoing the benefits of adding this information.
Example
Compiled with GHC head (with all implemented proposals), the program will output the following:
Diagnosis
I think this message is problematic
Package: ghc-internal,Module: GHC.Internal.Exception, orType: ErrorCallto the program that we wrote. What does it mean?!BlockedIndefinitelyOnMVar,ErrorCallis a generic name of a much more common exception type which to the user likely reads as something internal leaking -- it's hard to connect it to the type of the exception that is being thrown.Exception-- it's hard to link these two bits. It relates more easily with theHasCallStack backtracebelow which at least also refersghc-internal.Package,Module, andTypeis also redundant since this information is only for developers who would just as well understand the triple<package>:<module>.<type>.Main: Exception:is also confusingly short when compared with how much whitespace and other blocks of text there are -- it's just hanging up there instead of relating to the exception type!This confusion is exacerbated when exceptions are re-thrown according to https://github.com/haskell/core-libraries-committee/issues/202 (example below)
Proposal
Considering this, I believe the solution is straightforward and both makes the message prettier and responds to my critiques above. Here are the key points:
<package>:<module>.<type>ErrorCall, and would be perceived as such)So here's a suggestion for what the message should instead look like:
Suggestion applied to example from the original proposal
Interaction with rethrown exceptions
Impact
The originally accepted proposal is yet unreleased. It will become available in 9.12 unless this proposal is accepted in the meantime.
Testsuites logging the stderr of haskell programs could suffer from changes in the exception messages, even if minor, so it would be ideal, if agreed upon, to get this change in before 9.12 is released.
Implementation
Done in https://gitlab.haskell.org/ghc/ghc/-/merge_requests/13280. Note the differences in the testsuite accepted tests. Net reduction in 600 lines in exceptions printing.
Part 2
Secondly, I propose the implementation of
displayExceptionforSomeExceptionto be:This provides transparency of SomeException when working with the
Exceptionclass methods. Currently,displayExceptionofSomeExceptionwill additionally:This information should be moved to a separate method,
displayExceptionWithInfo, which is used by default byuncaughtExceptionHandler-- the handler responsible for printing to the user uncaught exceptions.Updating the default handler and the instance guarantees that exceptions by default still get printed with the type and backtrace, as they currently do. However, developers who want to provide user-facing exceptions can override the default exception handler to opt-out of this information irrelevant to the user.
With catching/re-throwing exceptions, we just have to make sure that the instance of
Exception WhileHandlingusesdisplayExceptionWithInfoto print callstacks and type information of WhileHandling exceptions.The examples listed in Part 1 don't change according to Part 2 by default.
Part 3
Since #164 was implemented, we display by default the backtrace of all exceptions. However, the
ErrorCallexception type used in the implementation oferrorandundefinedkeeps a callstack manually. This results in a buggy output where we get a redundant callstack (seen in the first example of Part 1).I propose that
ErrorCallstops propagating a callstack manually and completely delegate the callstack handling to the backtrace mechanism.Applied to the first example resulting from Part 1:
With Part 3, we'd instead get:
GHC-issue: 25283 Implemetation: I've already got a commit which does this, but essentially we drop the
ErrorCallcall pattern synonym and makeErrorCallthe only constructor ofErrorCall.Part 4
Currently, the callstacks include unnecessary internal details of functions called in the implementation of
error,undefined,throwIO,ioException,ioError.I propose we freeze the call stack at these functions. The example above would now stop the callstack at
error, hiding the unnecessary traces of toExceptionWithBacktrace and collectBacktraces.Part 5Since I'm already changing this code, let's finally removeerrorWithStackTrace, which has been deprecated since GHC 8.0 (2015), i.e. for 9 years.This is not uncontentious, so I'm dropping this suggestion.