RFC / WIP: no longer warn in code_warntype for variables not used in the body #23280
Conversation
KristofferC
force-pushed
the
kc/no_red_unused
branch
3 times, most recently
from
0190354
to
59d0059
Compare
base/interactiveutil.jl
Outdated
| return false | ||
| end | ||
|
|
||
| is_variable_used(arg::SlotNumber, slotnames, v) = slotnames[arg.id] == v |
There was a problem hiding this comment.
Also should be ::Slotinstead.
base/interactiveutil.jl
Outdated
| @@ -347,14 +347,35 @@ problematic for performance, so the results need to be used judiciously. | |||
| See [`@code_warntype`](@ref man-code-warntype) for more information. | |||
| """ | |||
| function code_warntype(io::IO, f, @nospecialize(t)) | |||
| function is_variable_used(bodies::Vector, slotnames, v) | |||
There was a problem hiding this comment.
This is also wrong. This can incorrectly catch constant arrays spliced into the code.
|
Would it make sense to make things yellow if unused in body? I feel like that would be a useful piece of info (although code warntype might not be a good place to show it) |
|
What's this info (unused vars) useful for? It's just a signature of the optimization. It might also be better to just do a single scan of slots uses instead of multiple ones and there's no need to use dispatch for that. |
|
I changed to only doing a single scan. I am not sure how I would avoid using dispatch and if the Vector dispatch function is still problematic. |
function scan_exprs(exprs, used)
for ex in exprs
if isa(ex, Slot)
used[ex.id] = true
elseif isa(ex, Expr)
scan_exprs(ex.args, used)
end
end
end
function slot_usaged(ci, slotnames)
used = falses(length(slotnames))
scan_exprs(ci.code, used)
return used
end |
base/interactiveutil.jl
Outdated
| @@ -346,15 +346,33 @@ This serves as a warning of potential type instability. Not all non-leaf types a | |||
| problematic for performance, so the results need to be used judiciously. | |||
| See [`@code_warntype`](@ref man-code-warntype) for more information. | |||
| """ | |||
| function code_warntype(io::IO, f, @nospecialize(t)) | |||
| function code_warntype(io::IO, f, t::ANY) #@nospecialize(t)) | |||
There was a problem hiding this comment.
Also, I imagine this is for testing only but this should be changed back....
|
Note that the code above rely on the AST to be valid. You can add |
KristofferC
force-pushed
the
kc/no_red_unused
branch
from
c3c50b9
to
b2c4e5f
Compare
|
Thanks! Updated with that code. Also added a test. Now for the details. I currently avoid printing the inferred type and just print |
KristofferC
force-pushed
the
kc/no_red_unused
branch
from
b2c4e5f
to
5c0c392
Compare
|
With some inspiration from GDB I went with:
Any opinions on that? |
|
Not sure if eligible for backport but tentatively putting the label there. |
|
Will merge tomorrow unless [the usual stuff]. |
|
Could we do something like this for the red |
|
Yeah, should be fairly easy. |
(cherry picked from commit 33fbfad)
(cherry picked from commit 33fbfad)
(cherry picked from commit 33fbfad)
(cherry picked from commit 33fbfad)
(cherry picked from commit 33fbfad)
The fact that
@code_warntypeshows unused variables in red is something that keeps tripping up users (both new and experienced), see e.g. https://discourse.julialang.org/t/question-about-type-stability-of-arrays-in-julia-0-6/5389.This PR traverses the codeinfo to check if the variable is actually used somewhere in the body. If not, we no longer print the variable in red even if it is inferred as a non concrete type.
Before:
After:
Maybe the
(Unused in body)is too verbose?Will add test / docs later if people think this is a good idea. Also, I'm new to the codeinfo data structure so someone should probably look over that code. CI skipped as to not waste resources so don't merge.