RFC: print error messages in REPL with colors and structure #18228

KristofferC · 2016-08-24T19:49:36Z

Follow up to #18215 which after discussion was closed because hiding relevant information is not a good thing. This instead tries to show the same information as we currently do but with a bit more structure.

For the same example as in #18215:

  type ThisisALongTypeThatWeMightNotWantToSee end
    typealias TL ThisisALongTypeThatWeMightNotWantToSee

    @noinline ff(x) = (a = rand(5); a[6])
    @noinline function g(a::TL, b::TL, c, d::Float64, e::Float64, f::TL, n)
        if n == 0
            return ff(2)
        else
            g(a, b, c, d, e, f, n-1)
        end
        return 0
    end

    @noinline h(x) = g(TL(), TL(), 2, 1.0, 2.0, TL(), 4)


    function test_it(args...)
        h(2.0)
    end

   test_it()

I am not sure that the colors I chose are the best but I am not an artist... Previous way of showing errors are retrieved by an ENV variable:

Things to bikeshed over

Do something like this at all?
I reversed the backtrace.. is that good? In my view yes because it keeps you from scrolling up to see where the error actually got thrown.
Colors?
Is the extra space necessary. In my opinion it actually makes things significantly less "cramped"

JeffBezanson · 2016-08-24T19:59:42Z

I think the extra space and showing the function and filename in different colors are good; makes it much easier to spot the source locations. I'd rather not use too many different colors though. Maybe something like bold for the function name and source location?

KristofferC · 2016-08-24T20:20:56Z

@JeffBezanson like so?

JeffBezanson · 2016-08-24T20:56:13Z

I like that a bit better; will see what others think.

I think if we're going to reverse the order of the stack trace the ins no longer make sense to me; it used to read that ff was inside g which was inside h, which was true, but the other way around doesn't read right to me.

StefanKarpinski · 2016-08-24T23:01:58Z

"from" instead of "in"?

StefanKarpinski · 2016-08-24T23:06:00Z

Nevermind, just bullets is better.

martinholters · 2016-08-25T07:05:23Z

Maybe just the function name bold, not its arguments? E.g. h(::Float64) instead of h(::Float64).

KristofferC · 2016-08-25T07:05:42Z

Yes, I have a new version coming up that I think wil be better.

mauro3 · 2016-08-25T07:18:59Z

Having the file and line-number in a different color would be helpful. Or maybe just bold if the arguments become non-bold as per @martinholters suggestion.

KristofferC · 2016-08-25T07:33:08Z

New suggestion (thx to @jakebolewski for inspiration):

gets away with the "in"
less fruit salad with the colors
no longer bolds the arguments

martinholters · 2016-08-25T07:40:33Z

Very readable. The backtrace ordering needs some getting-used-to, but IMHO makes a lot of sense this way.

KristofferC · 2016-08-25T07:40:59Z

FWIW this also exists now by just using the package at https://github.com/KristofferC/PimpMyREPL.jl so it is possible to try it out without having to mess with rebuilding and merging into a local branch.

oxinabox · 2016-08-25T09:10:07Z

I think it would be a good idea to add a space between filename, and line number.
They blur together for me.

Its looking real good.

So REPL[3]: 1 rather than REPL[3]:1

martinholters · 2016-08-25T09:21:59Z

NEWS.md

-    Also, the stacktrace is printed with the in the reverse order from previously and with the error message at the very bottom. 
-    The rationale behind this is that it puts the relevant info closer to the cursor so one does not need to scroll up. 
+  * The REPL now prints errors with more structure and more color.
+    Also, the stacktrace is printed with the in the reverse order from previously and with the error message at the very bottom.


"with the in the"
Maybe "in reverse order compared to previous releases"?

nalimilan · 2016-08-25T10:00:33Z

+1 in general!

I'm not a fan of the horizontal line at the top, though. If people really want to keep it, at least use the Unicode box drawing character ─ instead of dashes.

Also, how about printing numbered bullets? They would make it easier to sport the most recent call (though "most recent call last" is already printed).

Ismael-VC · 2016-08-25T10:28:31Z

About the colors, I really liked the yellow, but I'm fine with white bold, @KristofferC would there be an API to change the color scheme and make new themes?

KristofferC · 2016-08-25T10:37:43Z

base/REPL.jl

@@ -110,15 +110,19 @@ function ip_matches_func(ip, func::Symbol)
    return false
 end

+error_file_color = :green
+error_funcdef_color = :yellow


TODO: Remove

KristofferC · 2016-08-25T10:38:25Z

@Ismael-VC There is (yet to be documented, if people think it should be a setting): https://github.com/JuliaLang/julia/pull/18228/files#diff-8d2b8a22d475d81e3a1944aedb57ad42R60

KristofferC · 2016-08-25T10:48:22Z

@nalimilan Why would numbers make it easier to spot the most recent call? It is always at the bottom. Also it is not obvious if a 1 represents the bottom or the top of the stack.

Regarding the line, I kinda like it because without it the last command and the error sort of gets clumped together. Without it, it also makes the Stacktrace (most... feels like it is hanging in mid air:

MichaelHatherly · 2016-08-25T13:32:52Z

base/replutil.jl

+            # Only print the backtrace header if there actually is a printed backtrace
+            if backtrace_str != ""
+                header = string(typeof(ex).name.name)
+                line_len = 76


Should this be dependant of the terminal width rather than a fixed size?

Something like min(displaysize(io)[2], 90)?

Perhaps just without the 90 minimum since otherwise it would print a partially wrapped line of -s when the terminal is narrower that 90.

I meant max...

Err... or wait...

would displaysize(STDOUT) be valid...?

Base.Terminals.width(Base.active_repl.t)...

Probably good with an extra space in header

pabloferz · 2016-08-25T16:20:35Z

I'm not fond of the line, I'd prefer either a blank line or that it'd occupy the same width as the julia prompt for example. That aside, this is looking really good so far!

KristofferC · 2016-08-25T16:58:27Z

I took the line from IPython:

I do think it clearly separates the error from the prompt...

Here they are for reference so the difference is more clear:

tkelman · 2016-08-25T19:44:50Z

The blank line seems like wasted space to me. Also need to double-check whether the unicode symbols will show up properly in Windows prompts. Looks okay in Windows 10 but might not be in 7.

edit: nope, '\u2319' is a question mark on windows 7 with the default font

KristofferC · 2016-08-25T20:08:27Z

Ref JuliaData/DataFrames.jl#953.

KristofferC · 2016-08-25T20:26:39Z

From @nalimilan suggestion about numbering the frames I implemented something fun in PimpMyREPL. After showing the stacktrace you can enter <n><Ctrl + Q>, where <n> is the stackframe number and doing so will take your editor to the file + line corresponding to that stackframe. Ex: https://media.giphy.com/media/3o7TKKlZrKQnWcdGTK/giphy.gif. Sorry for going off topic but programming something that is not only physics simulations / numerical algorithms is really fun :P

ararslan · 2016-08-30T18:17:38Z

What would folks think of putting the descriptive error at the top, e.g.

julia> test_error()
BoundsError: attempt to access 5-element Array{Float64,1} at index [6]
─────────────────────────────────────────────────────────────────────────────────
BoundsError                                   Stack trace (most recent call last)
 - test_error()
    └ at ErrorMessages.jl:172

 - h(::Float64)
    └ at ErrorMessages.jl:162
─────────────────────────────────────────────────────────────────────────────────

julia>

?

Could also draw another line beneath the stack trace to better visually separate it from the user's commands, as shown above.

vtjnash · 2016-09-19T17:39:31Z

base/show.jl

-        print(io, f)
-    else
-        print(io, "(::", ft, ")")
+    isreplerror = get(io, :REPLError, false)


maybe a better name for this would be :hascolor? REPLError sounds both too specific and too vague to me.

vtjnash · 2016-09-19T17:42:21Z

base/stacktraces.jl

@@ -201,15 +201,19 @@ function show_spec_linfo(io::IO, frame::StackFrame)
 end

 function show(io::IO, frame::StackFrame; full_path::Bool=false)
-    print(io, " in ")
+    isreplerror = get(io, :REPLError, false)
+    print(io, isreplerror ? " ─ " : " in ")


This feels unfortunate. I think we could add a second kwarg prefix = " in " instead, since the only place this is called with REPLError = true is show_backtrace -> show_trace_entry?

ararslan · 2016-09-19T18:00:49Z

Please forgive me if I'm wrong, but I thought the consensus was to put the error text at the bottom? In your example, the red KeyError is at the top.

Also I'd still love to see some dividing lines but that's just personal taste. :)

KristofferC · 2016-09-19T18:09:40Z

That only makes sense if the traces are reversed though. And that I feel is a too big change right now.

Thanks for the comments @vtjnash. Lovely to get some input on the implementation. I had a hard time knowing how much to branch on IOContext vs keywords. Will update in a while.

KristofferC · 2016-09-21T18:40:15Z

How about something like this @vtjnash?

KristofferC · 2016-09-21T20:07:52Z

Also, do you think the hascolor IOContext entry might be one of the "official" entries. Like the question in #14052 (comment)

tkelman · 2016-09-23T10:34:41Z

base/client.jl

@@ -61,6 +63,8 @@ warn_color()   = repl_color("JULIA_WARN_COLOR", default_color_warn)
 info_color()   = repl_color("JULIA_INFO_COLOR", default_color_info)
 input_color()  = text_colors[repl_color("JULIA_INPUT_COLOR", default_color_input)]
 answer_color() = text_colors[repl_color("JULIA_ANSWER_COLOR", default_color_answer)]
+stackframe_linfo_color() =    repl_color("JULIA_STACKFRAME_LINFO_COLOR", :bold)


spell out lineinfo ?

tkelman · 2016-09-23T10:35:53Z

It's a bit awkward to have both a global variable for have_color and an IOContext property for it. Maybe we should migrate towards the latter going forward (separately from this).

tkelman

Testing what happens if I leave another neutral "comment" review when I had previously approved an older version of this

StefanKarpinski · 2016-09-23T16:34:01Z

I'm a little concerned about the ballooning number of environment variables. I feel like we should use a TOML config file instead and use an environment variable to indicate which file to load.

KristofferC · 2016-09-23T17:13:18Z

Ref #2716

KristofferC · 2016-09-23T17:39:54Z

I could just take away the config options but for frontends that can do color but not boldness (like jupyter notebook) it might be good to give an option.

I wouldn't mind redoing the whole color customization configuration into a julia functional API but might not be worth it.

vtjnash · 2016-09-23T18:41:58Z

Also, do you think the hascolor IOContext entry might be one of the "official" entries.

yes, I think the global should go away

StefanKarpinski · 2016-10-27T17:47:37Z

This seems to need a rebase and maybe a little design discussion.

KristofferC · 2016-12-11T11:47:38Z

Rebased. Design comments are welcome.

KristofferC · 2016-12-11T12:39:38Z

Mac seems to include an extra space for some reason, hence the test error:

ERROR: LoadError: Declaring __precompile__(false) is not allowed in files that are being precompiled.\n\nStacktrace:\n  [1] __precompile__
ERROR: LoadError: Declaring __precompile__(false) is not allowed in files that are being precompiled.\n\nStacktrace:\n [1] __precompile__

StefanKarpinski · 2016-12-12T20:31:31Z

@KristofferC: I think you should just use your judgement here and pick the style you like. If people hate it, I'm sure we'll hear about it.

KristofferC · 2016-12-12T21:16:25Z

I will open a new PR since there is so much previous discussion here that is no longer directly relevant, and many things have changed along the way.

KristofferC added REPL Julia's REPL (Read Eval Print Loop) needs tests Unit tests are required for this change labels Aug 24, 2016

KristofferC changed the title ~~print error messages in REPL with colors and structure~~ RFC: print error messages in REPL with colors and structure Aug 24, 2016

kshyatt added the error handling Handling of exceptions by Julia or the user label Aug 24, 2016

StefanKarpinski added this to the 0.6.0 milestone Aug 24, 2016

martinholters reviewed Aug 25, 2016
View reviewed changes

KristofferC reviewed Aug 25, 2016
View reviewed changes

MichaelHatherly reviewed Aug 25, 2016
View reviewed changes

vtjnash requested changes Sep 19, 2016

View reviewed changes

KristofferC force-pushed the kc/error_msgs branch 2 times, most recently from 6a48239 to f546514 Compare September 21, 2016 18:38

KristofferC force-pushed the kc/error_msgs branch from f546514 to 8d15273 Compare September 21, 2016 20:05

tkelman reviewed Sep 23, 2016

View reviewed changes

timholy mentioned this pull request Oct 13, 2016

julep: Base.summary and ShowItLikeYouBuildIt #18909

Closed

print error messages in REPL with colors and structure

4dc6f35

KristofferC force-pushed the kc/error_msgs branch from a248031 to 4dc6f35 Compare December 11, 2016 11:43

spell out linfo as lineinfo

fa82939

KristofferC closed this Dec 12, 2016

KristofferC mentioned this pull request Dec 12, 2016

Try make REPL error easier to visually parse #19569

Merged

tkelman deleted the kc/error_msgs branch December 29, 2016 05:24

KristofferC mentioned this pull request Feb 22, 2017

RFC: make julia prompt non bold #20236

Closed

KristofferC mentioned this pull request Jan 7, 2020

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

Closed

KristofferC mentioned this pull request May 28, 2020

What's needed to include ClearStacktrace.jl in Base? #36026

Closed

RFC: print error messages in REPL with colors and structure #18228

RFC: print error messages in REPL with colors and structure #18228

Conversation

KristofferC commented Aug 24, 2016 • edited Loading

JeffBezanson commented Aug 24, 2016

KristofferC commented Aug 24, 2016

JeffBezanson commented Aug 24, 2016

StefanKarpinski commented Aug 24, 2016

StefanKarpinski commented Aug 24, 2016

martinholters commented Aug 25, 2016

KristofferC commented Aug 25, 2016

mauro3 commented Aug 25, 2016

KristofferC commented Aug 25, 2016

martinholters commented Aug 25, 2016

KristofferC commented Aug 25, 2016 • edited Loading

oxinabox commented Aug 25, 2016

Choose a reason for hiding this comment

nalimilan commented Aug 25, 2016

Ismael-VC commented Aug 25, 2016

Choose a reason for hiding this comment

KristofferC commented Aug 25, 2016 • edited Loading

KristofferC commented Aug 25, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MichaelHatherly Aug 25, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KristofferC Aug 25, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pabloferz commented Aug 25, 2016 • edited Loading

KristofferC commented Aug 25, 2016

tkelman commented Aug 25, 2016 • edited Loading

KristofferC commented Aug 25, 2016 • edited Loading

KristofferC commented Aug 25, 2016

ararslan commented Aug 30, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ararslan commented Sep 19, 2016

KristofferC commented Sep 19, 2016

KristofferC commented Sep 21, 2016

KristofferC commented Sep 21, 2016

Choose a reason for hiding this comment

tkelman commented Sep 23, 2016

tkelman left a comment

Choose a reason for hiding this comment

StefanKarpinski commented Sep 23, 2016

KristofferC commented Sep 23, 2016

KristofferC commented Sep 23, 2016

vtjnash commented Sep 23, 2016

StefanKarpinski commented Oct 27, 2016

KristofferC commented Dec 11, 2016

KristofferC commented Dec 11, 2016 • edited Loading

StefanKarpinski commented Dec 12, 2016

KristofferC commented Dec 12, 2016

KristofferC commented Aug 24, 2016 •

edited

Loading

KristofferC commented Aug 25, 2016 •

edited

Loading

KristofferC commented Aug 25, 2016 •

edited

Loading

MichaelHatherly Aug 25, 2016 •

edited

Loading

KristofferC Aug 25, 2016 •

edited

Loading

pabloferz commented Aug 25, 2016 •

edited

Loading

tkelman commented Aug 25, 2016 •

edited

Loading

KristofferC commented Aug 25, 2016 •

edited

Loading

KristofferC commented Dec 11, 2016 •

edited

Loading