Refine exception stack API

[c42f]

Updated Description

This PR has been simplified with some parts merged separately (#30900, #30899). However, the catch_stack API has not been finalized. In the current version (as of 2019-02-06), I've got two things here:

• Rename catch_stack() -> current_exceptions() as this seems more descriptive.
• Introduce a new type ExceptionStack so that we can have a proper exception-and-backtrace specific type we can dispatch on. For example, this allows us to have a show implemented for exception and backtrace data which pretty prints the information in a much more readable way than the simple Vector of tuples that catch_stack returns right now.

Original Description

Here's a few refinements to the API introduced in #28878. I'm not quite happy with that API for various reasons so I want to revisit it before 1.1 feature freeze (or unexport it before 1.1).

IMO this is a big improvement, but still a bit WIP so feedback would be great.

• Rename catch_stack() -> current_exceptions() as this seems far more transparent, and is in fact how the function is summarized in the docs. I wasn't happy with the previous name, it's got an analogy to catch_backtrace but other than that seemes too obscure. Alternative name proposals gladly welcome (the current_exceptions(task) variant is perhaps less natural).

• Introduce a new type ExceptionStack so that we have a proper exception-and-backtrace specific type we can dispatch on. This allows us to treat showerror as more of an implementation detail, and simply override show(::IO,::ExceptionStack) in the usual way to get integrated pretty printing of exceptions and backtraces. I'm not quite certain about how this is a wrapper for a Vector; perhaps it should be an AbstractVector.

• For PARTR sanity, prevent reading of current exceptions of a concurrently running task, at least for now.

• Pretty printing of ExceptionStack in the REPL by overloading display_error. Could go into a separate PR if people want this simplified (or just review the commit 074e39b in isolation).

• Pretty printing of ExceptionStack via show(). Examples follow:

try
    1 + [1,2]
catch exc
    try
        1÷0
    catch
        show(current_exceptions())
    end
end

Notice that by having proper show integration, we automatically get things like the following:

try
    1 + [1,2]
catch exc
    try
        1÷0
    catch
        @info "Ignoring an error" exception=current_exceptions()
    end
end

[c42f]

Regarding ExceptionStack, we could alternatively model this as

struct ExceptionState
    exception
    backtrace
end

const ExceptionStack = Vector{ExceptionState}

which might make more or less sense. It's a little nicer because it doesn't introduce a container type for this one small thing. It's a little worse because it would assume that any Vector{ExceptionState} has the semantics of an exception stack rather than a simple array.

[c42f]

Travis failure on linux32 is failure of a single test, due to #29923

The appveyor failure on 64 bit is really odd (a timeout? but with no error message or other indication of what went wrong?) I've restarted that and we'll see.

[c42f]

Win64 CI problem has symptoms the same as #29603

[StefanKarpinski]

Would it make sense to have each exception have next::Union{Nothing,Exception} field instead? That way you can ask for the current exception what, if any, the previous exception was. My reasoning being that it may be less disruptive to just add a field to each exception type since code that isn't prepared to get an exception stack still gets an exception object while code that is prepared can still access the stack easily enough.

[c42f]

It does make sense and in some ways would be more natural and less disruptive. But the problem is user exception types: how do we ensure they actually have such a field? Basically, I don't think we can, at least not without the change being hugely breaking. We even allow people to throw things which aren't <:Exception! I tried to argue that this was a bad idea before but I'm not sure people were convinced (see #21886).

Given we don't have structural inheritance, that leaves us with composition:

struct ExceptionState
    exception
    backtrace
    next::Union{Nothing,ExceptionState}
end

Which I think would be quite a natural way of expressing this. But unfortunately no less breaking and somewhat less flexible to use than just doing what I've done in this PR, and shoving them all in a Vector. Thoughts?

[StefanKarpinski]

Hmm. That's interesting: this may be the first compelling case I've encountered where allowing abstract types to have fields would be really useful—it would allow us to insert a next field into every subtype of Exception.

[c42f]

By the way, my feeling was that there's no harm in providing a simple function-based interface to the exception stack system and seeing how that pans out.

Clearly it's not the end game and you'd like it to integrate better with the syntax. But the best syntax also depends on deeper design questions about how exceptions should be matched.

... having said that I didn't want to talk syntax, now I can't resist suggesting the following which popped into my head:

try
    # blah
catch es...  # equivalent to `es = current_exceptions()`
    # whatever
end

[JeffBezanson]

This type doesn't seem necessary to me. Since showing errors goes through display_error anyway, having a type with a show method is a bit redundant (and does have some duplicated code here).

[c42f]

Since showing errors goes through display_error anyway

That's only true for the REPL where we pass exceptions and backtraces directly to display_error rather than dispatching them to some generic machinery which needs to match on their type.

In the generic case, there's currently no nice way to pass around a package of exceptions and backtraces together such that their type can be dispatched on in a non-clumsy way. I'm thinking particularly of Loggers, where I'd like to be able to have things like:

@error "Something bad happened" arbitrary_key_name=current_exceptions()

and have this actually display something sensible without clumsy matching of types in the Logger backend. We currently resort to such matching, for example

julia/stdlib/Logging/src/ConsoleLogger.jl

Lines 45 to 52 in 7a5042a

	# Formatting of values in key value pairs
	showvalue(io, msg) = show(io, "text/plain", msg)
	function showvalue(io, e::Tuple{Exception,Any})
	ex,bt = e
	showerror(io, ex, bt; backtrace = bt!=nothing)
	end
	showvalue(io, ex::Exception) = showerror(io, ex)

[c42f]

Not that I love ExceptionStack as a type, it seems annoying and superfluous on the surface, and it's one of the main reasons I wanted some more eyes on this.

What I'd really like are suggestions about how to replace or improve it, while also having a way to pass these to logging macros and have them robustly recognized by the logger backend.

[c42f]

It's true that there's a little duplication of code between show and display_error. Previously I tried making display_error dispatch to show and adding some IOContext handling to turn on the simplifications we currently use in display_error. That turned out somewhat complex though so I punted in the current iteration and left it with two concrete implementations.

I could give it another go.

[c42f]

I've rebased, fixed the minor conflict and renamed include_bt.

The remaining question in my mind is what to do about ExceptionStack. It's annoying to have a new type, but there's also reasons to want a type for this so that it can be show()n (and otherwise dispatched on) in contexts which are more general than REPL error reporting.

[codecov-io]

Codecov Report

❗ No coverage uploaded for pull request base (master@7bc3742). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master   #29901   +/-   ##
=========================================
  Coverage          ?   39.06%           
=========================================
  Files             ?      344           
  Lines             ?    61454           
  Branches          ?        0           
=========================================
  Hits              ?    24008           
  Misses            ?    37446           
  Partials          ?        0

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7bc3742...ff25fee. Read the comment docs.

[c42f]

Win64 issue appears to be #29603 again, restarted.

[JeffBezanson]

My impression is that an exception or exception stack needs to be passed to a context that expects it --- it usually doesn't make sense to have a context that expects an arbitrary value, and then sometimes treats it as an error based on its type.

[c42f]

Yes logging backends can have magic autodetection of "I think this vector looks structurally like it contains exceptions and backtrace pairs" and they can dig exception stacks out of the arbitrary list of user-supplied key=value pairs. I'm just somewhat uncomfortable having to re-infer this information when it was present in the first place.

I'm also not sure why showerror isn't just show with some special IOContext variable set, and this would be much easier to do if we had a special purpose type.

Putting those concerns aside in the interest of getting this done, would it be ok to define a type alias

const ExceptionStack = Array{NamedTuple{(:exception, :backtrace),T}, 1} where {T <: Tuple}

and to remove the special show for ExceptionStack (people can use showerror if they like)? In this case we will also need special handling inside ConsoleLogger so that we can call showerror there rather than show.

[c42f]

@JeffBezanson As food for thought I've added a commit c886fef which removes both ExceptionStack and the specialized show, but keeps a type alias. Because show was removed, I had to add something to ConsoleLogger so that exception stacks can be displayed in log messages in a useful form.

It's not too bad but on the whole it seems like a semantic step backward. On the upside it removes a small amount of code.

[vtjnash]

Hm, yeah, that doesn't seem great. I think wrapping the vector in an <:Exception makes sense, since the point is that you want to be able to treat this as a single exception. In that way, it's similar to other existing cases of the same, such as CompositeException, RemoteException, and LoadError. In various cases, that is exactly the point of this new object too. Consider for instance a case like notify(error=true) where we know we're going to lose the backtrace, so we're using current_exceptions to build an explicit representation of the state of all exceptions—and then packaging that up to use as an Exception object itself too.

[JeffBezanson]

Yes logging backends can have magic autodetection of "I think this vector looks structurally like it contains exceptions and backtrace pairs" and they can dig exception stacks out of the arbitrary list of user-supplied key=value pairs. I'm just somewhat uncomfortable having to re-infer this information when it was present in the first place.

That's not at all what I meant to suggest. Looking at the overall type of the value, or the element type, or the types of objects contained in it are all variations of the same thing: examining the value itself to determine how to use it. I'm saying there needs to be some information outside the value itself to tell you what it represents. For example as @vtjnash mentioned notify(cond, val, error=true) treats val as an exception. We don't do that based on the type of val. Similarly a special key could be used in logging e.g. exception=x.

[c42f]

Sorry @JeffBezanson, I just couldn't catch your meaning before. I've reverted that last commit (will be rebased away later).

Similarly a special key could be used in logging e.g. exception=x

Right, this is exactly what the logging documentation already says :-) However, I'm worried about how to show the value of exception stacks to the user, especially when they pass it as a different key... let's say for purely informational purposes:

@info "Ignoring some error" ignored_error=current_exceptions()

If show doesn't do something useful for the type of object returned by current_exceptions() this will be completely unreadable in the ConsoleLogger (or other logger) output. Which is exactly why I want some kind of distinguishable type for this.

---

it's similar to other existing cases of the same, such as CompositeException, RemoteException, and LoadError

Thanks @vtjnash that's an extremely relevant point. Previously things like LoadError had to eagerly capture the root cause exception to avoid it being lost. Now the exception stack does that automatically for all exceptions so LoadError could just leave the original exception on the stack and this PR means it will be reported to the user. (However we still need RemoteException and CompositeException for out of process exceptions.)

[c42f]

In the spirit of seeing how LoadError/InitError can be made more consistent with the new system, I've changed them from using jl_rethrow_other to jl_throw. With this PR's REPL changes, these would be reported as follows:

The previous exception is still eagerly captured and stored in LoadError. Not sure how that could (or should) be changed without breaking compatibility.

[JeffBezanson]

Yes, I agree LoadError (and InitError) saving the last exception is now redundant; nice! I would say keep everything the same except the printing of LoadError, which now doesn't need to show its inner exception. In 2.0 we can remove those fields.

[vtjnash]

Unrelated to this PR, but just from looking at that screenshot, we should eventually hide all frames (but one) that are printed in a earlier (in printing order) list

[JeffBezanson]

Also the simplest thing to do here is just unexport catch_stack for now.

[Keno]

Triage tag was outdated. If there's something left to triage, re-apply the tag and specify the issue.

[vtjnash]

LGTM, conditional on addressing the needs-tests tag (and needs a rebase).

[vtjnash]

bump. Are you able to rebase?

[c42f]

Wow, this PR has languished for a long time :-(

I've found some time to rebase this and update the tests.

I'll plan to merge it tomorrow, provided CI passes.