-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Display exception stacks #30900
Display exception stacks #30900
Conversation
FreeBSD CI error seems to be network related. |
Linux failures are #27788. Again. |
base/errorshow.jl
Outdated
print(io, "\nduring initialization of module ", ex.mod) | ||
end | ||
showerror(io::IO, ex::InitError) = showerror(io, ex, []) | ||
showerror(io::IO, ex::LoadError) = print(io, "LoadError in expression starting at ", ex.file, ":", ex.line) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be clearer to say "Error while loading expression starting at ..."; LoadError
is just a derived exception and not really a kind of error on its own. It also seems to be unnecessary to show the backtrace for a LoadError; I think it's always a prefix of the nested exception?
Also, it's nice to note that our backtrace machinery is now good enough that LoadError may be entirely unnecessary 🎉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be clearer to say "Error while loading expression starting at ..."
Sure.
It also seems to be unnecessary to show the backtrace for a LoadError; I think it's always a prefix of the nested exception?
Yes. This isn't really specific to LoadError and @vtjnash had a good suggestion over at #29901 (comment).
This also applies to But in general perhaps we should have a construct like struct ExceptionContext <: Exception
message
vars
end
ExceptionContext(message=nothing; kwargs...) = ExceptionContext(message, kwargs.data)
function Base.showerror(io::IO, ex::ExceptionContext)
print(io, "Exception context:")
if !isnothing(ex.message)
print(io, " ", ex.message)
end
for (k,v) in pairs(ex.vars)
print(io, "\n ", k, " = ", v)
end
end Fake example: bar(y) = baz(y)
baz(y) = sqrt(y)
function foo(x)
try
bar(x^2 - 4)
catch
throw(ExceptionContext(x=x))
end
end
foo(1.5) There seems to be two rather different uses people might have for the system:
Unfortunately the best ordering for printing the stack seems somewhat at odds in these cases. For (1), you'd presumably like to print the root cause first and truncate the common prefix of its stack trace, interspersing the ExceptionContext and printing more of the stack as you go. Something like the following mockup for the example above: For (2) I have a little firsthand experience in digging through java exception stacks to find the root cause. I remember it was always annoying to have to scroll to the top of the log file to find the root cause rather than just finding it at the bottom. The current ordering is designed around the idea that people would like to see the root cause next to their REPL prompt. |
The thoughts? This does make it very clear that the information bundled into For the "exception while handling exception" use case, the stack traces might diverge a lot more than what we see here. |
I like the printing order the way you have it now; it matches the way we print stack traces (with the chronologically first thing at the bottom). LoadError is common enough that I think it's worth special-casing for now and just suppressing its backtrace. Not a big deal though. For other cases it's fine to just print everything. |
Comments addressed (I think!) and CI has passed. |
Beautiful, this is great! |
These are printed as a list, with the top of the stack first, down to the root cause.
This required a bit of refactoring to disentangle and factor out some of the error handling in the various REPL code paths.
* Use display_error with the exception stack to provide more complete root cause info. * Provide a way to define the error stream in the fallback REPL so it can be tested. * Use the logging system for "out of band" system errors in fallback REPL * Add some actual tests
This is now somewhat redundant, given the exception stack system keeps the root cause live until the LoadError/InitError is dealt with. Also simplify the printing of these exceptions, allowing the exception stack mechanism to tell the story instead.
95066d5
to
d3dbe86
Compare
Thanks for the review Jeff! |
Did this break showing the stacktrace in some cases? Im getting something like:
now, while previously, I was sure I got a backtrace of the error in the testset. |
@KristofferC Yes it looks like that problem was probably brought on by this PR, because this PR changed But this apparently has a failure mode when a piece of code captures the
This is reproduced by the code: file2.jl error("Blah") file1.jl using Test
@testset "x" begin
include("file2.jl")
end |
Sounds like the right answer? |
True, that's something which should be done regardless of whether |
This PR contains the display-related changes from #29901, in particular:
display_error
to print exception stacks coming fromBase.catch_stack()
.display_error
to show the full exception stack in both the REPL and when running scripts via_start
.jl_throw
rather thanjl_rethrow_other
forLoadError
andInitError
simplifying the printing of these exceptions while allowing the exception stack to tell the story of the root cause. This will allow us to remove the captured exception fields fromLoadError
andInitError
in the future (Refine exception stack API #29901 (comment))Here's a picture showing how
display_error
currently formats the exception stack:Currently the formatting has the properties:
display_error
caused by [exception $n]
withexcstack[n]
followingHere's an example showing how
LoadError
prints with these changes: