Refactor stacktrace processing and display pipeline for more clarity and less redundancy

[BioTurboNick]

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:

[joa-quim]

Could the first entry be removed? Saying that an error starts at Base ./error.jl is so ... useless.

[BioTurboNick]

Could the first entry be removed? Saying that an error starts at Base ./error.jl is 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.

[waldyrious]

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?

[BioTurboNick]

Yeah I take your point... Could work!

[andyferris]

Could the first entry be removed? Saying that an error starts at Base ./error.jl is 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.

[BioTurboNick]

@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)

[waldyrious]

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 🙂)

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

[BioTurboNick]

I suppose it's hard to tell, but they are rounded in the screenshots. I'll try some more things out.

[laborg]

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?

[BioTurboNick]

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.

[waldyrious]

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 :)

[BioTurboNick]

Yeah, I think maybe that wasn't working well... but what about this? A tick for every frame

[waldyrious]

I like it! It's explicit and accurate. 😍

(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!)

[BioTurboNick]

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.