Try make REPL error easier to visually parse #19569

KristofferC · 2016-12-12T21:43:46Z

This PR tries to make it a bit easier to visually parse error messages that are printed in the REPL. It does this by no longer printing the stack trace in red, numbers each stack frame and makes the function name and file/line info bold since I believe they are often more interesting than the types the functions are being called it.

For a lot of background info and back and forth on design see #18228. Some more ambitious attempts were tried there but in the end I settled for something that is quite similar to what we have now.

Before and after pictures:

For a a future PR, the last stacktrace could be saved on error and the user can write the stack number, press a hotkey, and that file would be opened in the editor at the correct line.

This current PR fails on the Apple build because it inserts an extra space in the test at https://github.com/JuliaLang/julia/compare/kc/error_msgs_2?expand=1#diff-e44967823c8efd2b46505406ac7e9e7cR212. I have no idea why and can't really test it well since I don't have a mac.

tkelman · 2016-12-12T22:26:32Z

base/replutil.jl

+    n_frames = 0
+    frame_counter = 0
+    process_backtrace((a,b) -> n_frames += 1, t)
+    n_frames != 0 && print(io, "\n\nStacktrace:")


maybe only one newline here?

tkelman · 2016-12-12T22:27:59Z

Having both the frame numbering and REPL inputs numbered in [n] format could be a bit confusing, maybe change to parens for one of them?

StefanKarpinski · 2016-12-12T23:07:40Z

I think they're visually different enough here to be clear.

StefanKarpinski · 2016-12-12T23:08:37Z

Also, let's not let relatively trivial style bikeshedding derail this again. This is a huge improvement over the current state of affairs and we can tweak it after the major part of the change is merged.

MichaelHatherly · 2016-12-13T06:27:53Z

The doctests in docstrings containing stacktraces will need updating to the new style. Probably best to rebase over #19562 to avoid seeing doctest errors that I've already fixed.

KristofferC · 2016-12-13T15:37:33Z

I'm having some troubles with the doctests. For example:

!! Test error in the following code block in 'docs/helpdb/Base.jl':

        julia> convert(Int, 3.0)
        3
        
        julia> convert(Int, 3.5)
        ERROR: InexactError()
        Stacktrace
         [1] convert(::Type{Int64}, ::Float64) at ./float.jl:656

    in expression:

        convert(Int, 3.5)

    expected:

        ERROR: InexactError()
        Stacktrace
         [1] convert(::Type{Int64}, ::Float64) at ./float.jl:656

    returned:

        ERROR: InexactError()
        Stacktrace:
         [1] convert(::Type{Int64}, ::Float64) at ./float.jl:656

I can't see any difference between them. Is it whitespace sensitive or?

MichaelHatherly · 2016-12-13T15:45:14Z

Missing : after Stacktrace, but yes having a visual diff would be really nice.

KristofferC · 2016-12-13T15:49:30Z

How could I not notice!? I looked at it for like a minute...

MichaelHatherly · 2016-12-13T16:09:50Z

How could I not notice!? I looked at it for like a minute...

I just have plenty of practise reading that kind of output... really needs to be improved though.

KristofferC · 2016-12-13T20:34:46Z

Tests pass, ready for a second, third etc. set of eyes.

tkelman · 2016-12-13T20:50:16Z

Do the tests pass when color is disabled? Are the env vars documented anywhere?

KristofferC · 2016-12-13T21:05:00Z

Color is always disabled in tests, therefore I have to "fool" julia that they are enabled with the eval stuff in the tests.

The env vars are not documented, because I am not sure if that level of customization is needed. @StefanKarpinski also expressed concerns that the number of env variables are getting maybe a bit too many. They don't really bloat the code so should be fine to keep.

StefanKarpinski · 2016-12-13T22:42:24Z

I'm fine with using environment variables as an interim thing, but we need a better approach.

KristofferC · 2016-12-14T18:40:57Z

If possible I would like to wait with documenting / removing the env variables. If people hate this and we revert it, it is work for nothing.

cossio · 2016-12-14T21:07:09Z

Could this be extended to errors in scripts (outside REPL) too?

KristofferC · 2016-12-14T21:36:18Z

If you run the script in the REPL then it will also have this output if that is what you mean. So not only for functions defined in the REPL.

cossio · 2016-12-14T21:44:54Z

@KristofferC Actually, I meant scripts outside the REPL, run from the shell, like:

$ julia myscript.jl

KristofferC · 2016-12-14T21:50:34Z

Ah, I see. I am not sure where the error is actually thrown from in that case. Maybe from the c code?

KristofferC · 2016-12-16T09:41:03Z

Rebased. AV failure is in pkg with some network problems.

KristofferC · 2016-12-21T17:16:50Z

I do think that @cossio has a good point about that backtraces should look as similar as possible no matter if excpetions are thrown (and caught) in the REPL or by the julia runtime (if that is the correct vocabulary) which then makes execution stop. I tried to look a bit in the c-code (jl_throw and friends) but didn't really understand exactly where the backtrace is being printed and what julia functions are being called from that point. Guidance appreciated.

yuyichao · 2016-12-21T17:38:42Z

All errors are thrown by C code and all exceptions with backtraces are printed in julia code by showerror. Crash backtraces are printed by jl_gdblookup.

KristofferC · 2016-12-21T18:35:15Z

@cossio I am a bit confused because this does already seems to work when running files from the shell, i.e.

➜  julia git:(kc/error_msgs_2) ✗ ./julia err.jl
ERROR: LoadError: Erp

Stacktrace:
 [1] error(::String) at ./error.jl:21
 [2] include_from_node1(::String) at ./loading.jl:532
 [3] include(::String) at ./sysimg.jl:14
 [4] process_options(::Base.JLOptions) at ./client.jl:275
 [5] _start() at ./client.jl:339
while loading /home/kristoffer/julia/err.jl, in expression starting on line 2

cossio · 2016-12-22T14:09:56Z

@KristofferC will it be colored as well? I don't have the master branch installed so I cant test here.

KristofferC · 2016-12-22T14:44:41Z

It should now

I completely removed the hascolor symbol from the IOContext and just went with the global variable have_color that is normally used for colored output. Not sure why I didn't do like that in the first place.

KristofferC · 2016-12-23T17:47:04Z

Any further comments here?

tkelman · 2016-12-23T18:02:25Z

I'm still going to propose using different punctuation for the frame numbering, the use of square brackets for both REPL defined functions and frame numbering makes them look interrelated when they aren't.

KristofferC · 2016-12-23T18:09:23Z

"Framing" the stackframe number with a [] makes too much sense to change. Could change the way REPL filename is printed?

KristofferC · 2016-12-28T13:07:46Z

Thinking about it more, I think keeping like this is fine. REPL frames will be color coded (bolded) in the terminal and will typically not exist outside interative use. Distinguishing them should not be a problem. If it gets annoying, could trivially be changed in another PR.

tkelman · 2016-12-28T16:21:33Z

base/array.jl

@@ -737,7 +737,6 @@ julia> deleteat!([6, 5, 4, 3, 2, 1], 1:2:5)

 julia> deleteat!([6, 5, 4, 3, 2, 1], (2, 2))
 ERROR: ArgumentError: indices must be unique and sorted
-Stacktrace:


won't this be needed to pass the doctest?

updated, I messed up with the online rebase tool

tkelman · 2016-12-28T16:23:43Z

bold vs non bold doesn't add any context to where the repl square brackets are coming from. frame numbers are at least obviously ordered, and would look fine with almost any demarcation, pun notwithstanding.

KristofferC · 2016-12-28T17:40:17Z

Ok. I've kinda run out of steam for this PR (and #18228) so if the potential confusion between REPL numbers and frame numbers is merge blocking, I might have another go at it for 1.0.

Keno · 2016-12-28T17:57:34Z

I think it's fine, the REPL as REPL prominently written next to it. If it turns out to be a problem we can revisit. Thanks for all your hard work on this!

KristofferC · 2016-12-29T18:01:53Z

Woot. I'm on standby for the fallout so ping away :)

tkelman · 2016-12-29T18:13:08Z

I'll submit a PR proposing to make the frame numbering 1. etc

(and add NEWS links back for #18453 (comment))

tkelman reviewed Dec 12, 2016

View reviewed changes

kshyatt added error handling Handling of exceptions by Julia or the user REPL Julia's REPL (Read Eval Print Loop) labels Dec 12, 2016

KristofferC mentioned this pull request Dec 13, 2016

Print diff in failing doctest JuliaDocs/Documenter.jl#370

Closed

KristofferC force-pushed the kc/error_msgs_2 branch from 6697dc2 to 2d6643e Compare December 13, 2016 16:00

KristofferC added 4 commits December 16, 2016 09:26

print error messages in REPL with colors and structure

1564eaa

no emptyline before stacktrace

6c80aa8

fix doctests

cbe0aad

no longer align on last digit

8c514ef

KristofferC force-pushed the kc/error_msgs_2 branch from ead09b2 to 8c514ef Compare December 16, 2016 08:26

KristofferC mentioned this pull request Dec 22, 2016

RFC: jump to numbered stackframe with hotkey #19680

Merged

get rid of hascolor iocontext

0736250

Merge branch 'master' into kc/error_msgs_2

3f5a1db

tkelman reviewed Dec 28, 2016

View reviewed changes

add back lost "Stacktrace" in rebase

90d078c

KristofferC added this to the 0.6.0 milestone Dec 29, 2016

StefanKarpinski merged commit dc7b4fb into master Dec 29, 2016

tkelman deleted the kc/error_msgs_2 branch December 31, 2016 06:39

KristofferC mentioned this pull request Jan 2, 2017

RFC: do not print backtrace for TestSetExceptions and Pkg.test() exception. #19750

Merged

KristofferC mentioned this pull request Jan 7, 2020

Where are the stack traces KristofferC/OhMyREPL.jl#189

Closed

Try make REPL error easier to visually parse #19569

Try make REPL error easier to visually parse #19569

Conversation

KristofferC commented Dec 12, 2016

tkelman Dec 12, 2016

Choose a reason for hiding this comment

KristofferC Dec 13, 2016

Choose a reason for hiding this comment

tkelman commented Dec 12, 2016

StefanKarpinski commented Dec 12, 2016

StefanKarpinski commented Dec 12, 2016

MichaelHatherly commented Dec 13, 2016

KristofferC commented Dec 13, 2016

MichaelHatherly commented Dec 13, 2016

KristofferC commented Dec 13, 2016

MichaelHatherly commented Dec 13, 2016

KristofferC commented Dec 13, 2016

tkelman commented Dec 13, 2016

KristofferC commented Dec 13, 2016

StefanKarpinski commented Dec 13, 2016

KristofferC commented Dec 14, 2016

cossio commented Dec 14, 2016

KristofferC commented Dec 14, 2016

cossio commented Dec 14, 2016 • edited Loading

KristofferC commented Dec 14, 2016

KristofferC commented Dec 16, 2016

KristofferC commented Dec 21, 2016 • edited Loading

yuyichao commented Dec 21, 2016

KristofferC commented Dec 21, 2016 • edited Loading

cossio commented Dec 22, 2016

KristofferC commented Dec 22, 2016

KristofferC commented Dec 23, 2016

tkelman commented Dec 23, 2016

KristofferC commented Dec 23, 2016

KristofferC commented Dec 28, 2016

tkelman Dec 28, 2016

Choose a reason for hiding this comment

KristofferC Dec 28, 2016

Choose a reason for hiding this comment

tkelman commented Dec 28, 2016

KristofferC commented Dec 28, 2016

Keno commented Dec 28, 2016

KristofferC commented Dec 29, 2016

tkelman commented Dec 29, 2016 • edited Loading

cossio commented Dec 14, 2016 •

edited

Loading

KristofferC commented Dec 21, 2016 •

edited

Loading

KristofferC commented Dec 21, 2016 •

edited

Loading

tkelman commented Dec 29, 2016 •

edited

Loading