Skip to content
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

Fine-tune stacktrace printing #37773

Merged
merged 4 commits into from
Oct 20, 2020
Merged

Fine-tune stacktrace printing #37773

merged 4 commits into from
Oct 20, 2020

Conversation

mcabbott
Copy link
Contributor

This is a small tweak to #36134, as discussed there. This gist has the same changes.

base/errorshow.jl Outdated Show resolved Hide resolved
show_method_params(io, tv)
nothing
end

function print_type_stacktrace(io, type; color=:normal)
str = sprint(show, type, context=io)
i = findfirst('{', str)
Copy link
Contributor Author

@mcabbott mcabbott Sep 27, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems a bit crude, but I got lost trying to find the right printing function to overload for this purpose (of printing only the type parameters dimly, not the outermost type). It should still use aliases like Matrix{Int}.

It's also possible this should be a method of print_within_stacktrace instead of its own function.

@mcabbott
Copy link
Contributor Author

mcabbott commented Oct 1, 2020

A question raised in this Discourse thread is whether the stack trace should also truncate very long types. The example there has lines over 7000 characters long. It would be trivial to make print_type_stacktrace truncate this, but are there be cases where the whole type is needed? Maybe it could do this only for interactive use?

(cc @tomerarnon)

@fredrikekre fredrikekre added the error messages Better, more actionable error messages label Oct 1, 2020
base/errorshow.jl Outdated Show resolved Hide resolved
@tomerarnon
Copy link
Contributor

tomerarnon commented Oct 1, 2020

whether the stack trace should also truncate very long types.

While I'm personally of the opinion that this should definitely happen, it wasn't clear to me that the discussion had settled on a precise course of action (and while there did seem to be some support for the idea, it was less than I expected, judging based on small numbers of likes and participants).

We brought up a handful of behaviors, (i.e. opt-in/out, and/or toggleable through an ENV variable, and/or a new overloadable _showtype(T) for stacktrace purposes) and I'm not sure which would be best. If you want to run with any of those ideas, I'm more than happy to lend a hand, either here or in a separate pr.

@mcabbott
Copy link
Contributor Author

mcabbott commented Oct 18, 2020

Latest commit restores bold function names. I don't see precisely how these were disabled in #36134, so they may have been put back differently, torture tests welcomed. (I have updated the gist linked above, too.)

1.5.2, master, and this PR (with Fira Code, whose bold is pretty strong):
Screenshot 2020-10-18 at 14 04 10

(Merge conflict is with #37750, this line: https://github.com/JuliaLang/julia/pull/37750/files#diff-c616894fef3ba781d1e1cf77a08e69f366ffd9e5b1cfe1220e7bdae85377e0b2R2067)

@KristofferC
Copy link
Member

Could you perhaps rebase this on master to get a fresh CI run and get rid of the merge conflict?

@KristofferC
Copy link
Member

I'd like to (again) bring up that the red color for the Base module is non-ideal. I tried this out and got:

image

and immediately my brain went, "Oh what's that error message, Base.Math? That's weird". If anything, it makes those modules stand out more than other colors. Perhaps just make them gray instead? For inlined methods, there are already no module printed so it isn't that much of a missing tooth perhaps?

@mcabbott
Copy link
Contributor Author

mcabbott commented Oct 18, 2020

Thanks for the screenshot, maybe they are too close. (Notice also :bright_red on master, at [14] in screenshot just above.) And maybe grey looks less odd with the bold to keep the rhythm. Like this, or :light_black to match the paths?

 @eval Base STACKTRACE_FIXEDCOLORS = IdDict(Base => :normal, Core => :normal)

Will rebase once I've figured out what #37750 did & how not to break it.

Here's the current look, VScode's default dark theme, JuliaMono, with Base now in :light_black:

Screenshot 2020-10-18 at 20 10 36

@jkrumbiegel
Copy link
Contributor

Cool, I like it much better with bold. Especially for light themes, as black is not that much more contrasting with white compared to dark gray.

@KristofferC
Copy link
Member

Let's try this out then :)

@KristofferC KristofferC merged commit 51f8716 into JuliaLang:master Oct 20, 2020
@mcabbott mcabbott deleted the error branch October 20, 2020 10:00
base/show.jl Show resolved Hide resolved
@mcabbott
Copy link
Contributor Author

Some constructors don't get printed in bold, but perhaps they should:

julia> PermutedDimsArray{Float64,2,(2,1),(1,2),Array{Float64,2}}(ones(2,2))
ERROR: ArgumentError: (2, 1) and (1, 2) must be inverses
Stacktrace:
 [1] PermutedDimsArray{Float64, 2, (2, 1), (1, 2), Matrix{Float64}}(data::Matrix{Float64})  # <-- not in bold
   @ Base.PermutedDimsArrays ./permuteddimsarray.jl:15
 [2] top-level scope
   @ REPL[54]:1

julia> Matrix{Int}([1 2; 3 4.5])
ERROR: InexactError: Int64(4.5)
Stacktrace:
  [1] Int64
    @ ./float.jl:607 [inlined]
...
 [10] Matrix{Int64}(x::Matrix{Float64})   # <-- not in bold
    @ Base ./array.jl:562
 [11] top-level scope
    @ REPL[57]:1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
error messages Better, more actionable error messages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants