Redesign for exceptions #285
Comments
alt-romes
changed the title
|
I'm going to ping those involved in the original message design to initiate discussion: @tomjaguarpaw @tbidne @adamgundry @parsonsmatt |
|
That seems fine by me. |
|
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:
|
|
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 |
|
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 @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 |
|
I don't use exceptions myself1 so I'll defer to exception users regarding what they need. However, I am surprised about this:
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 Footnotes |
The dynamic nature of foo `catches`
[ Handler (\KnownException -> print0)
, Handler (\(OtherKnownException a) -> print1 a)
, Handler (\(SomeException unknown) -> putStrLn $ displayException unknown)
]In that fallback case, all we "know" is |
|
You implement 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 But at the moment it's not. The proposal is to define
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. |
Right, so far I understand.
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.
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 While keeping exactly the same all of the extra information when exceptions are uncaught and reported. |
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 |
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?
This seems pretty reasonable to me, because the thing that is thrown an caught is always a (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.)
I don't follow. Could you elaborate? Maybe with an example? |
|
The change to the behavior of the default handler together with the change to 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. |
Ah, looks like the wrapper was never shown, but the difference is that recently we added more stuff to the Is the idea that should "more stuff" shouldn't be in the
Great, I must have missed that. I think that's worth calling out prominently and explicitly in the proposal. |
Yes, that's exactly right. |
|
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 |
OK, great. I see this is actually spelled out in the following sentence:
but I think it would be worth calling out more explicitly that this change doesn't change the behaviour of the default handler.
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
I'm against this. errorWithStackTrace ::
Unsatisfiable
(Text "'errorWithStackTrace' no longer exists. Use 'error' instead.") =>
String ->
a
errorWithStackTrace = unsatisfiable |
|
(And to be clear, I'm +1 on the rest of the proposal.) |
That's right. The commit is https://gitlab.haskell.org/ghc/ghc/-/merge_requests/13301/diffs?commit_id=c0a2cb447c3fec71edc6653fe7f26619b751ce5f. We're deleting the This means that nothing changes for those already using the pattern synonym.
In that case I've amended the proposal to not touch -- | 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 This makes it more-OK that the call stack appears at the bottom only. |
|
To add a datapoint, today we got a user bug report about the duplicate call stacks part of this proposal (Part 3): https://gitlab.haskell.org/ghc/ghc/-/issues/25311 |
|
Hey @alt-romes . Following your invitation to comment on the proposal, I have one suggestion regarding presentation. In particular, the example in the top post has the following formatting on the "Proposal" part: I think the message and the call stack are two different paragraphs in the text, semantically. Therefore, I'd expect them to be separated by an empty line: Bonus points would gain an extra indentation for the message because morally it's a quote. The reason I think of it as a quote is because I read the whole output as authored by the RTS, whereas the error message was written by the programmer. |
the two lines being together is determined by the instance of Exception for Therefore, there's no line to add since the As for the exception context callstack, there's already a line between it and the user message (example from part 3): Thanks! |
Sounds like we should add a blank line there when there's no |
After removing the which eventually becomes by freezing the callstack of |
|
@alt-romes any chance "this list should never be empty" can be indented two or four spaces so that it looks like a quote? |
|
It could. What do you suggest would happen to the "WhileHandling" messages? In any case, the behavior for whileHandling could differ from the top level handler message. Of course, if this is not unambiguous I won't, since I don't intend to add any friction to this proposal. |
|
I'm personally sympathetic with Part 1. However, in my role I must make a point of order. There was time and place to raise concerns about #261. The CLC proposal was open for 3+ months. The corresponding MR was available for review as well. We cannot bikeshed this ad infinitum every time someone disagrees on aesthetic. Is there a material change validating re-evaluation? What would prevent someone to raise another proposal to change formatting in a week or two? How long shall we keep this proposal open to make sure that all interested parties had voiced their opinions? |
These changes are motivated by a holistic assessment of what exceptions as a whole have become in GHC, in light of all the recent proposals that have changed them. Unlike other MRs, this is not a standalone step in a direction, but rather a correction to exceptions as they stand whole. This could only have been done now, after the incremental ideas made their way in.
I don't think this is just about changing formatting (see above), but in any case, if they did, we would:
I intend the implementation to be 100% finished next week, or the week after in the worst case scenario. 100% here means full CI pipeline finished, since the code implementation is essentially complete. The proposal is ideally voted on at that time. |
|
Some related feedback from me in #GHC chat:
|
|
To be clear, when developing user-facing applications we want brief errors without stack traces by default, and I don't think we should have to set up a special exception handler to achieve that. |
|
Have you tried the following? I didn't see it in the chat: main = do
-- HasCallStackBacktrace is the only backtrace on by default
setBacktraceMechanismState HasCallStackBacktrace False
...IMO a bit easier than messing with the handler. |
That's the problem of picking the color for the bike shed. Everyone has different opinions. Your opinion is fair, but also saying that application should give enough information so when a user can copy paste an error and report a bug, the information is there. The proper solution(s) is the ones which allow changing the behavior to suit different needs; then the default can be chosen based on about anything, gut feeling, perception of elegance etc. AFAICT, moving stuff between
These are not coupled code parts. I could also argue that default exception handler design should not alone affect |
|
On Sun, 13 Oct 2024, Oleg Grenrus wrote:
To be clear, when developing user-facing applications we want
brief errors without stack traces by default, and I don't think we
should have to set up a special exception handler to achieve that.
That's the problem of picking the color for the bike shed. Everyone has
different opinions. Your opinion is fair, but also saying that
application should give enough information so when a user can copy paste
an error and report a bug, the information is there.
An exception must be handled by the programmer and e.g. turned into a
user-readable message. Of course, standard functions to do that would be
nice. Unhandled exceptions are programming errors and programming errors
should be reported in a developer-readable way.
|
|
Hello ghc friends!
I have not (I wasn't aware of it). Thank you - I confirm that it works and is relatively easy if you know about it and if you add an import of Control.Exception.Backtrace, but only when building with base 4.20+ / ghc 9.10+ (which will mean CPP most likely, since base-compat lags and adds complexity.)
I agree that options are nice and this is about the choice of default behaviour. I think you didn't say which default you'd prefer phadej (or I missed it).
I'm not sure this is a programming error. At least, it used not to be. It sounds like you'd be saying that from ghc 9.10 on, it is a programming error to not explicitly override ghc's default error output. It's hard for one person to see the whole picture, and I have only skimmed this long thread, but I think the viewpoint of ordinary developers building end user software with GHC, might be underrepresented here. Allow me to expand a little, and excuse the verbosity. I feel this is mainly about
|
|
For me, there are at least two major reasons why (some form of) backtraces on is the right default.
I could be wrong, but I don't think exceptions were absent from Haskell because of a conscious choice. It was because the implementation was really hard. Simon Marlow gave a talk in 2012 about the difficulties, describing it as a "very old problem, I've been struggling with for many, many years." This fits my impression, that the direction of travel has always been to add more information to errors by default when possible (see: As you point out, it is clearly a good idea to give users friendly error messages, even if they are not actionable. That means making it easy for developers to get the behavior they want, and communicating these methods effectively. It seems |
My line of reasoning would be: if an exception can be thrown that the developer is not aware of, then that is a programming error. Therefore, if there is no programming error then the developer is aware of all exceptions that can be thrown, and thus they should catch all of them, reporting them to the user in an appropriate way, rather than defer to the default exception handler. Now, in practice I'm lazy and I often do actually let the default exception handler print exceptions. But I consider it "my fault" if I don't like it when the default exception handler changes to show more information. |
|
@frasertweedale and the Haskell Security team, any thoughts on showing stack traces by default ? |
|
On Tue, 15 Oct 2024, tomjaguarpaw wrote:
I'm not sure this is a programming error. At least, it used not to be.
My line of reasoning would be: if an exception can be thrown that the
developer is not aware of, then that is a programming error. Therefore,
if there is no programming error then the developer is aware of all
exceptions that can be thrown, and thus they should catch all of them,
reporting them to the user in an appropriate way, rather than defer to
the default exception handler.
It is unfortunate that the IO type does not show the exceptions that can
be thrown. Suggestions were made for new monad and transformers that add
exceptions explicitly in types, however we would still need an
exceptionless I/O monad. 'main' should be of this type. Then you cannot
accidentally miss to handle exceptions.
|
We'll discuss it at this week's meeting and follow up soon! |
|
The implementation is ready at https://gitlab.haskell.org/ghc/ghc/-/merge_requests/13301#note_590084 Given the implementation is ready, we should wait for the last comments and then proceed to a vote (which looks like waiting @frasertweedale's meeting). Regarding the behaviour of the default uncaught exception handler, I do not wish to change this proposal further. I believe the alternative suggestions are too contentious to be part of this proposal, regardless of their merits. This proposal liberates the |
|
SRT discussed at our meeting yesterday: https://github.com/haskell/security-advisories/blob/main/meeting-notes/2024-10-16.md#stack-traces-proposal SRT has no objection to this proposal. We note that many languages and frameworks display full stack traces in a variety of situations. We also note that in most cases the program structure revealed by the call stack is not sensitive. We have some small uncertainties about when stack traces would be displayed by default (but it should be clear to people familiar with this proposal). If this proposal results in situations where stack trace will be shown where previously it was not, this should be clearly communicated via release notes, users guide and other documentation. We have no problem with the behaviour per se - only want to avoid unpleasant surprises for users. |
|
@Bodigrim, I would like to trigger a vote. |
This is a problem that Footnotes
|
|
I'm having hard time to grasp the proposal in its entirety, but it's high time to vote. Dear CLC members, last chance to offer comments / suggestions / objections before we open the vote early next week. |
|
Dear CLC members, let's vote on the proposal to redesign certain aspects of exception display and handling as detailed in the top post. There are no breaking changes, the proposal is mostly about displaying stack traces and exception provenance in a more user-friendly manner. Please cast your vote separately on each of 4 parts. @tomjaguarpaw @hasufell @parsonsmatt @angerman @velveteer @mixphix +1 from me. I re-read the proposal attentively and I agree with the arguments. |
|
+1 to all four parts from me, especially part 4! |
|
+1 Thanks, this is a very helpful, detailed and well-motivated proposal. |
|
+1 |
|
Thanks all, that's enough votes to approve. |
Bodigrim
added
approved
|
Thank you all for the discussion and voting. The patch implementing these changes has landed now: !13301. |
CLC Proposal for Exceptions Redesign
Part 1
Recently, #231 and #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 #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.
The text was updated successfully, but these errors were encountered: