improve display of keyword argument methods in stack traces #33118
Conversation
|
Huge improvement! ❤️ |
JeffBezanson
force-pushed
the
jb/kwstacktrace
branch
3 times, most recently
from
2c5ba85 to
105f847
Compare
There was a problem hiding this comment.
nkw is my new favorite thing. Coupled with the lowering of kw methods in more recent Julia versions (which have the
$(Expr(:method, :f))
$(Expr(:method, Symbol("#f#10")))header) this might be enough info to effectively close #30908. @KristofferC, can you think of anything else we'd really need? I haven't checked what this does to some of the more complicated cases, like kwarg-@generated methods, kwarg methods that define anonymous functions, etc, but presumably those don't introduce any extra unsolvable problems.
| bt = stacktrace(catch_backtrace()) | ||
| end | ||
| @test any(s->startswith(string(s), "f33065(::Float32; b::Symbol, a::String)"), bt) | ||
| end |
There was a problem hiding this comment.
should add some tests for a vararg kwfunc too (e.g. f33065(x; c..., b=1.0, a=""))
There was a problem hiding this comment.
I'll be darned, that triggers a lowering error. I guess everybody puts the keyword vararg last 😂 Maybe an unintentional feature?
JeffBezanson
force-pushed
the
jb/kwstacktrace
branch
from
105f847 to
99e8721
Compare
| @@ -602,6 +601,11 @@ function show_backtrace(io::IO, t::Vector{Any}) | |||
| end | |||
| end | |||
|
|
|||
| function is_kw_sorter_name(name::Symbol) | |||
| sn = string(name) | |||
| return !startswith(sn, '#') && endswith(sn, "##kw") | |||
There was a problem hiding this comment.
Is this a problem?
julia> module Toplevel
fname = Symbol("#hidden")
@eval ($fname)(x; y=1, z="hi") = 1
end
julia> names(Toplevel, all=true)
11-element Array{Symbol,1}:
Symbol("###hidden#1")
Symbol("##hidden")
Symbol("##hidden#1")
Symbol("#eval")
Symbol("#hidden")
Symbol("#include")
Symbol("#kw###hidden")
:Toplevel
:eval
:fname
:include There was a problem hiding this comment.
Could be, but there are probably worse collisions that can happen if user identifiers contain #, so unofficially that is just not supported.
There was a problem hiding this comment.
@timholy we now have this syntax for your example 😬
module Toplevel
var"#hidden"(x; y=1, z="hi") = 1
endThere was a problem hiding this comment.
@JeffBezanson indeed... unintended consequences.
There was a problem hiding this comment.
It's still worth having var I suspect. As this reveals, making important decisions on the basis of naming has some potential downsides. One could potentially solve this and #30908 by introducing more fields linking the various lowered representation to one another. Then you wouldn't be beholden to a naming convention.
There was a problem hiding this comment.
Agreed. I mean, we knew var could be used for this kind of thing... just I'd originally envisaged it as more of an internal thing or occasional escape hatch for naming variables in FFIs. Seeing it becoming directly "useful" is kind of disturbing. The most surprising use so far has been the DataFrames people using it to refer to columns with poorly normalized names.
|
Bump. |
part of #33065
Before:
after:
Before:
after:
I did several tweaks to get this to work. First, I made some of the name mangling more regular. Now the implementations of kwarg methods (where the actual code goes) have names like
f#3and the argument sorting functions have names likef#kw. Previously the#kwpart was removed from the function name (kept only in the type name), which made it hard to identify kw sorters by name alone, which is needed for inlined functions. This change is then used to (1) demangle these names during display and (2) remove kw sorter functions from stack traces entirely.Second, I added metadata to implementation methods (like
#lu!#133above). It's sufficient just to know that some number of leading arguments are actually keyword arguments. The original function is passed as an argument after those, so we can recover the signature of the original positional-only method.Note that this still shows the extra dispatch when you call a method that has kw arguments without passing any:
The extra
f()is tricky to remove. Fortunately it's not so bad, since that frame ([3]) reflects what occurred at the call site so isn't horribly useless/confusing.