julia
julia copied to clipboard
JuliaLang
Refactor stacktrace processing and display pipeline for more clarity and less redundancy
The current stacktrace display pipeline has 2 different ways to show repetition:
And each of these uses two separate printing paths, leading to other inconsistencies, like sometimes top-level scope is shown and sometimes it isn't, and sometimes the frame counter includes the hidden frames and sometimes it doesn't.
I merged the two pathways and display the cycles more nicely using line characters, and extracted some of the filtering into separate functions. I also added a comment to document the stacktrace processing pipeline.
~~Keeping as a draft to look at CI results and get feedback.~~
Current state of PR:
Original idea:
Could the first entry be removed? Saying that an error starts at Base ./error.jl is so ... useless.
Could the first entry be removed? Saying that an error starts at
Base ./error.jlis so ... useless.
This PR wouldn't be the spot for a change like that. I'm personally all for removing frames that aren't useful, though formally a stacktrace needs to show where an error was thrown from, and the error function is just a convenience to trigger an error. So it wouldn't be a default even if possible.
That said, there are bigger fish to fry when it comes to stacktrace lengths, and movement on that is... slow.
Just a small suggestion for your consideration: one positive aspect of the first of the two current styles is how it is explicit about which lines are repeated; the style proposed in this PR attempts to capture this information, but I find it a bit more ambiguous, perhaps because of the asymetry between the begin and the end markers of the blocks:
julia> foo1(100)
ERROR:
Stacktrace:
[1] error()
@ Base ./error.jl:53
┌┌ [2] foo1(n::Int64)
││ @ Main ./REPL[2]:1
││ [3] foo2(n::Int64)
││ @ Main ./REPL[3]:1
│└───── repeated 9 more times
│┌ [22] foo3(n::Int64)
││ @ Main ./REPL[4]:1
││ [23] foo2(n::Int64)
││ @ Main ./REPL[3]:1
│└───── repeated 9 more times
└────── repeated 1 more time
┌ [82] foo1(n::Int64)
│ @ Main ./REPL[2]:1
│ [83] foo2(n::Int64)
│ @ Main ./REPL[3]:1
└────── repeated 9 more times
[102] foo1(n::Int64)
@ Main ./REPL[2]:1
[103] top-level scope
@ REPL[14]:1
Perhaps adding a block-ending marker in a similar fashion to the starting one could help?
julia> foo1(100)
ERROR:
Stacktrace:
[1] error()
@ Base ./error.jl:53
┌┌ [2] foo1(n::Int64)
││ @ Main ./REPL[2]:1
││ [3] foo2(n::Int64)
│├ @ Main ./REPL[3]:1
│╰───── repeated 9 more times
│┌ [22] foo3(n::Int64)
││ @ Main ./REPL[4]:1
││ [23] foo2(n::Int64)
├├ @ Main ./REPL[3]:1
│╰───── repeated 9 more times
╰────── repeated 1 more time
┌ [82] foo1(n::Int64)
│ @ Main ./REPL[2]:1
│ [83] foo2(n::Int64)
├ @ Main ./REPL[3]:1
╰────── repeated 9 more times
[102] foo1(n::Int64)
@ Main ./REPL[2]:1
[103] top-level scope
@ REPL[14]:1
In case this seems too busy, perhaps the block delimiters could be distinguished from the "arrow" pointing to the "repeated x times" line by using a different emphasis, e.g.:
julia> foo1(100)
ERROR:
Stacktrace:
[1] error()
@ Base ./error.jl:53
┏┏ [2] foo1(n::Int64)
┃┃ @ Main ./REPL[2]:1
┃┃ [3] foo2(n::Int64)
┃┡ @ Main ./REPL[3]:1
┃╰───── repeated 9 more times
┃┏ [22] foo3(n::Int64)
┃┃ @ Main ./REPL[4]:1
┃┃ [23] foo2(n::Int64)
┡┡ @ Main ./REPL[3]:1
│╰───── repeated 9 more times
╰────── repeated 1 more time
┏ [82] foo1(n::Int64)
┃ @ Main ./REPL[2]:1
┃ [83] foo2(n::Int64)
┡ @ Main ./REPL[3]:1
╰────── repeated 9 more times
[102] foo1(n::Int64)
@ Main ./REPL[2]:1
[103] top-level scope
@ REPL[14]:1
WDYT?
Could the first entry be removed? Saying that an error starts at
Base ./error.jlis so ... useless.
But error(x) is just a function that invokes throw(ErrorException(x)). The stacktrace already doesn't show any entries for throw(...).
While I sympathise, making a special case for just this seems odd to me.
@waldyrious how do you feel about one of these? Adding the end marker inside complicates things a bit, but maybe the bold region is enough of an indicator? (bold start tick, thin start tick, no start tick)
Using bold is certainly an improvement over the initial approach! I personally like the third option conceptually the most, because it's symmetrical in how it represents the start and the end of the relevant selection, but can understand if others prefer the corner.
I do worry that the distinction between bold and thin lines may not be as evident, though. Would using dashed segments for the thin ones be an option? (And maybe curved corners to add to the "softness" aspect :slightly_smiling_face:)
julia> foo1(100)
ERROR:
Stacktrace:
[1] error()
@ Base ./error.jl:53
┃┃ [2] foo1(n::Int64)
┃┃ @ Main ./REPL[2]:1
┃┃ [3] foo2(n::Int64)
┃┃ @ Main ./REPL[3]:1
┃╰╌╌╌╌╌ repeated 9 more times
┃┃ [22] foo3(n::Int64)
┃┃ @ Main ./REPL[4]:1
┃┃ [23] foo2(n::Int64)
┃┃ @ Main ./REPL[3]:1
┊╰╌╌╌╌╌ repeated 9 more times
╰╌╌╌╌╌╌ repeated 1 more time
┃ [82] foo1(n::Int64)
┃ @ Main ./REPL[2]:1
┃ [83] foo2(n::Int64)
┃ @ Main ./REPL[3]:1
╰╌╌╌╌╌╌ repeated 9 more times
[102] foo1(n::Int64)
@ Main ./REPL[2]:1
[103] top-level scope
@ REPL[14]:1
I suppose it's hard to tell, but they are rounded in the screenshots. I'll try some more things out.
I'm not a fan of the bold stuff. It doesn't look clean and I don't understand the motivation: What else should the reader assume is repeated? Is it that people think the "x times repeated" is repeated too?
Yeah... I've tried a couple iterations and nothing quite seems to be better enough to justify itself. I will try adding the closing tick though, despite the extra complexity.
To be clear, it's not that I expect people to actually assume the "repeated X more times" is part of the stack trace; it's just that it adds a small "mental hiccup" that admittedly is insignificant in isolation, but would add up over time (especially due to the small inconsistency in how the opening and closing of the range are marked). If it could be avoided without making the code too complex, it would be ideal IMHO. But sure, it's not a huge deal :)
Yeah, I think maybe that wasn't working well... but what about this? A tick for every frame
I like it! It's explicit and accurate. :heart_eyes:
(Admittedly, it's a bit busier than the approach with only the tick at the first frame, so just to be clear, I do think that one is already a definite improvement over the status quo!)
Great! I'm leaning towards the ticks because it does eliminate the slight mental overhead of the original one I used, and is simple to implement.
And to be fair these kinds of traces seem to be uncommon, and nested cycles vanishingly rare. On Discourse, I'm seeing a dozen or so traces over the last few years that have 2-12 frames repeated arbitrary numbers of times, and no obvious ones with nesting.
Adding triage since this is a fairly big visual change. I'm very much in favor, but it seems like a good idea to get some more input.
I prefer no ticks. "Original idea" from the OP is my favorite of all proposed versions. I find that which lines are repeated is quite clear in the original and it's visually cleaner.
I can make that change back, I see the one comment and three thumbs-up, but I don't want to make further changes without consensus. Did triage discuss it?
Triage discussed it and I left that comment during the meeting but it's not a "triage decision" comment. Triage didn't decide to make a style decision on this.
Personally, I don't understand @waldyrious's reasoning when asking for tick marks.
one positive aspect of the first of the two current styles is how it is explicit about which lines are repeated; the style proposed in this PR attempts to capture this information, but I find it a bit more ambiguous
What is a possible alternative interpretation? I fail to see the ambiguity in the OP's proposal.
What is a possible alternative interpretation? I fail to see the ambiguity in the OP's proposal.
Just to clarify, I explained in an earlier comment that it's not so much that the original approach was undecidable, it's just that the deciding, while indeed obvious, does requires a little more mental work (i.e. mentally subtracting the "repeated X more times" lines) than if the bracket boundaries were symmetric/explicit in how they indicate the start and the end of the repeated block.
That said, I don't feel strongly about this, and can certainly understand that others consider that either the added ticks are too noisy, or that the boundaries are immediately evident even to the untrained eye. If the bold option from this comment would be acceptable, I'd prefer it, but otherwise I'm happy to defer to the broader community consensus.
Triage voted 6-4 against the ticks but is fine with the ticks and issues a decision in support of keeping the ticks. However, one change that triage does want is "n times" instead of "n-1 more times".
EDIT: since this post, votes have changed. The decision stands.
I got it - single repeated frames weren't nesting right within cycles, now they are! Added tests to cover this too. See if they all pass...
# single-frame repeat inside a cycle
foo1(n) = n ≤ 0 ? error() : foo2(n - 1)
foo2(n) = 80 > n > 20 ? foo3(n - 1) : foo1(n - 1)
foo3(n) = n % 10 == 0 ? foo2(n - 1) : foo3(n - 1)
foo1(100)
# multi-frame cycle inside a cycle
foo1(n) = n ≤ 0 ? error() : foo2(n - 1)
foo2(n) = 80 > n > 60 || 40 > n > 20 ? foo3(n - 1) : foo1(n - 1)
foo3(n) = foo2(n - 1)
foo1(100)