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

Add stacktrace colors to showing MethodErrors and method lists #45069

Merged
merged 5 commits into from
Jun 24, 2022

Conversation

Seelengrab
Copy link
Contributor

@Seelengrab Seelengrab commented Apr 23, 2022

This basically takes the module coloring logic from printing stacktraces and applies it to method error printing as well. Also contains some formatting to make the "closest candidates" stand out more by having an empty line precede & follow that block.

Before (master)

screenshot_2022-06-09_19-51-23_027029655

After (This PR)

after1
after2

Code for producing the above

example.jl

f() = 1
f(x,y,z; f=true) = 3
f(x,y,z,k; f=false) = 4

include("subdir/subdir2/module.jl")

h() = f(1)
l() = h()

ambig(x, y) = 1
ambig(x::Integer, y) = 2
ambig(x, y::Integer) = 3
ambig(x::Int, y::Int) = 4
ambig(x::Number, y) = 5

subdir/subdir2/module.jl

module Example
    Main.f(x,y) = 2
    g() = :g
end

The one thing that kind of bothers me though is that the module coloring between the MethodError message and the followed stacktrace is not consistent - they're done seperately & in isolation and there's no easy way to "pass on" the existing coloring that's built in MethodError. The colors between method printing, stacktraces and MethodError are now also consistent. There may be a race condition on the underlying IdDict, should two tasks set a color at the same time. Since only one printing call would be affected and it's a non-functional part, I'd say this is fine.

Opinions, improvement ideas etc. are welcome! I've been bothered by how hard MethodErrors are to read for a long time now, and I think unifying the printing with stacktraces is a good first step to improve this.

Closes #40251 Closes #40913 Closes #30110 Closes #36129

@Seelengrab
Copy link
Contributor Author

Seelengrab commented Apr 23, 2022

I do expect tests to fail, mostly due to doctests. I just wanted to have CI churn away at finding them instead of my laptop 😅 I'll fix them as they occur and after some feedback about the general layout/design.

Copy link
Contributor

@rikhuijzer rikhuijzer left a comment

Choose a reason for hiding this comment

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

In response to your request for comments on Zulip

I like how you spotted and fixed the inconsistency between using @ and at in the stacktrace versus the error message.

base/errorshow.jl Show resolved Hide resolved
base/errorshow.jl Show resolved Hide resolved
@Seelengrab
Copy link
Contributor Author

I was made aware of #40251 and now I'm thinking about unifying the two, since they're (in part) about the same exact thing: making methods & their uses easier to read when displayed.

@Seelengrab Seelengrab force-pushed the methoderror_colors branch from 29ddf22 to 0488abc Compare May 24, 2022 15:28
@Seelengrab
Copy link
Contributor Author

Seelengrab commented Jun 7, 2022

Alright, I incorporated #40251 (with permission) into this PR, since that one kinda stalled and this one also touches method printing (though at a different place). I'll update the images at the top post.

I think this should be about it, maybe just some very minor things and this should be good to go!

@Seelengrab Seelengrab changed the title Add stacktrace colors to showing MethodErrors Add stacktrace colors to showing MethodErrors and method lists Jun 7, 2022
@Seelengrab
Copy link
Contributor Author

Seelengrab commented Jun 7, 2022

Ah of course, doctests - those will have to wait for tomorrow. Trying to fix those results in ugly expanded stacktraces to be printed, which I'll have to investigate why that happens.. I didn't touch any logic regarding that, I don't think.

@Seelengrab
Copy link
Contributor Author

While fixing the failing tests, I noticed that the ambiguity errors have their own code path here - so I'm changing those too.

@giordano giordano added the error messages Better, more actionable error messages label Jun 8, 2022
base/methodshow.jl Outdated Show resolved Hide resolved
base/methodshow.jl Outdated Show resolved Hide resolved
@Seelengrab Seelengrab force-pushed the methoderror_colors branch from 5f14da8 to 1cc22a5 Compare June 10, 2022 14:44
@Seelengrab
Copy link
Contributor Author

Alright, the only thing that's missing now should be the doctests (for real this time). I've also added a NEWS.md entry and rebased onto current master.

I'm still not 100% sure how to fix the doctests best, without someone in the future running into having to specifically exclude the non-existence of [...] everywhere 🤔

Fix error printing issues

Incorporate method list printing

Fix almost all test cases

Remove complete inner module printing from tests

This is to be consistent with stacktrace printing, which now shows the topmost module
that isn't `Main`, to make it easier to identify packages in stacktraces.

Contract home dir in method printing as well

Fix now contracted user dir paths

Fix failing ambiguity test cases as well as other failing test cases

Fix missing newline in ambiguity error
…lists and MethodErrors.

Remove bad detection of "required" keyword arguments

Fix missing line information for methods without a source file

Fix InteractiveUtils tests
Fix last remaining reflection test
@Seelengrab Seelengrab force-pushed the methoderror_colors branch from 26880a3 to 594a9eb Compare June 12, 2022 06:20
@Seelengrab
Copy link
Contributor Author

Alright, I think the doctests are fixed now, so this should be good to go once CI agrees. I took the liberty to squash the commits already, to keep history cleaner.

@Seelengrab Seelengrab force-pushed the methoderror_colors branch from 594a9eb to 95be1a4 Compare June 12, 2022 06:35
@Seelengrab
Copy link
Contributor Author

Seelengrab commented Jun 15, 2022

Since this has been agreed on by triage, can this be merged soon-ish? It'd be unfortunate if this stalls and I have to keep fixing new doctests/other tests..

@oscardssmith
Copy link
Member

LGTM

@Seelengrab
Copy link
Contributor Author

Seelengrab commented Jun 23, 2022

I missed an inconsistency with colors :| Here's what it looks like now:

screenshot_2022-06-23_20-38-30_948777202

Should be perfectly consistent now.

EDIT modulo whitespace 😅

@oscardssmith oscardssmith added the merge me PR is reviewed. Merge when all tests are passing label Jun 23, 2022
@Seelengrab Seelengrab force-pushed the methoderror_colors branch 2 times, most recently from 9dfc9ce to 2dc4e15 Compare June 23, 2022 20:53
@Seelengrab Seelengrab force-pushed the methoderror_colors branch from 2dc4e15 to 2aa6fb5 Compare June 23, 2022 20:59
@Seelengrab
Copy link
Contributor Author

Apologies for the force pushes after you added the mergeme label, fixing the colors broke some tests that should be fixed now (hopefully)..

@Seelengrab
Copy link
Contributor Author

The Apple Darwin failure seems unrelated:

nested task error: rmprocs: pids [5] not terminated after 30 seconds.

as is the PowerPC build failure:

JIT session error: Symbols not found: [ __gnu_f2h_ieee, __gnu_h2f_ieee ]

No idea what's going on with the Profile failure in the "Allow Fail" category. Seems unrelated?

@oscardssmith oscardssmith merged commit b2b8ce8 into JuliaLang:master Jun 24, 2022
@tecosaur
Copy link
Contributor

I have to say, I'm not a fan of the choice to have variable names be greyed out. To quote past me in #40251

I think argument names deserve more emphasis, in my PR I use the magenta colour for them. for example there's the join method with signature join(strings, delim, last) and I think it's worth applying emphasising argument names for cases such as those.

@Seelengrab
Copy link
Contributor Author

The goal here was to make MethodError, methods(f) and stacktraces consistent in color - since it didn't seem like there was consensus about adding more colors there (and some said they dislike more color due to it adding more noise), I left the choice for later.

Perhaps this could be done in a future "customize all the colors" REPL theming PR, like suggested in #41435?

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
7 participants