add a vibe coded Test.detect_closure_boxes for closure boxes in loaded code#60478
add a vibe coded Test.detect_closure_boxes for closure boxes in loaded code#60478KristofferC merged 4 commits intomasterfrom
Test.detect_closure_boxes for closure boxes in loaded code#60478Conversation
|
Not sure if this is obvious or not, but can (of course) be used with external packages as well: |
|
@aviatesk maybe you have some thoughts about this? I guess this is kind of the same as https://github.com/aviatesk/JET.jl/blob/cf77ed77e46b1a3bda92494818792272269d1d1a/src/analyzers/optanalyzer.jl#L276-L306? Is there a way to only run the |
contrib that checks for closures boxes in loaded codecontrib that checks for closure boxes in loaded code
|
Sure, we can definitely provide that kind of analysis as a special analyzer for JET.jl. Right now, JET only works with v1.12, but maybe we want to give it a shot? |
|
So you can do something like using JET
struct CapturedVariableFilter{RC<:JET.ReportConfig}
inner::RC
end
function CapturedVariableFilter(; target_modules = nothing, ignored_modules = nothing)
CapturedVariableFilter(JET.ReportConfig(target_modules, ignored_modules))
end
function JET.configured_reports(config::CapturedVariableFilter, reports::Vector{JET.InferenceErrorReport})
reports = filter(r->r isa JET.CapturedVariableReport, reports)
return JET.configured_reports(config.inner, reports)
endjulia> function func(x)
local y
f = function ()
y = x
end
return y
end;
julia> report_opt(func, (Int,), report_config=CapturedVariableFilter())
═════ 1 possible error found ═════
┌ func(x::Int64) @ Main ./REPL[4]:2
│ captured variable `y` detected
└────────────────────
julia> using LibGit2
julia> report_opt(LibGit2.branch!, (LibGit2.GitRepo,String); report_config=CapturedVariableFilter())
═════ 1 possible error found ═════
┌ branch!(repo::GitRepo, branch_name::String) @ LibGit2 /Users/julia/.julia/scratchspaces/a66863c6-20e8-4ff4-8a62-49f30b1f605e/agent-cache/default-honeycrisp-R17H3W25T9.0/build/default-honeycrisp-R17H3W25T9-0/julialang/julia-release-1-dot-12/usr/share/julia/stdlib/v1.12/LibGit2/src/LibGit2.jl:405
│┌ branch!(repo::GitRepo, branch_name::String, commit::String) @ LibGit2 /Users/julia/.julia/scratchspaces/a66863c6-20e8-4ff4-8a62-49f30b1f605e/agent-cache/default-honeycrisp-R17H3W25T9.0/build/default-honeycrisp-R17H3W25T9-0/julialang/julia-release-1-dot-12/usr/share/julia/stdlib/v1.12/LibGit2/src/LibGit2.jl:405
││┌ branch!(repo::GitRepo, branch_name::String, commit::String; track::String, force::Bool, set_head::Bool) @ LibGit2 /Users/julia/.julia/scratchspaces/a66863c6-20e8-4ff4-8a62-49f30b1f605e/agent-cache/default-honeycrisp-R17H3W25T9.0/build/default-honeycrisp-R17H3W25T9-0/julialang/julia-release-1-dot-12/usr/share/julia/stdlib/v1.12/LibGit2/src/LibGit2.jl:411
│││ captured variable `branch_ref` detected
││└──────────────────── |
|
One thing to note is that julia> report_opt(LibGit2.branch!, (LibGit2.GitRepo,AbstractString); report_config=CapturedVariableFilter())
No errors detected
julia> report_opt(LibGit2.branch!, (LibGit2.GitRepo,AbstractString); report_config=CapturedVariableFilter(), skip_noncompileable_calls=false)
═════ 1 possible error found ═════
┌ branch!(repo::GitRepo, branch_name::AbstractString) @ LibGit2 /Users/julia/.julia/scratchspaces/a66863c6-20e8-4ff4-8a62-49f30b1f605e/agent-cache/default-honeycrisp-R17H3W25T9.0/build/default-honeycrisp-R17H3W25T9-0/julialang/julia-release-1-dot-12/usr/share/julia/stdlib/v1.12/LibGit2/src/LibGit2.jl:405
│┌ branch!(repo::GitRepo, branch_name::AbstractString, commit::String) @ LibGit2 /Users/julia/.julia/scratchspaces/a66863c6-20e8-4ff4-8a62-49f30b1f605e/agent-cache/default-honeycrisp-R17H3W25T9.0/build/default-honeycrisp-R17H3W25T9-0/julialang/julia-release-1-dot-12/usr/share/julia/stdlib/v1.12/LibGit2/src/LibGit2.jl:405
││┌ branch!(repo::GitRepo, branch_name::AbstractString, commit::String; track::String, force::Bool, set_head::Bool) @ LibGit2 /Users/julia/.julia/scratchspaces/a66863c6-20e8-4ff4-8a62-49f30b1f605e/agent-cache/default-honeycrisp-R17H3W25T9.0/build/default-honeycrisp-R17H3W25T9-0/julialang/julia-release-1-dot-12/usr/share/julia/stdlib/v1.12/LibGit2/src/LibGit2.jl:411
│││ captured variable `branch_ref` detected
││└──────────────────── |
|
I guess closure boxes are a bit special since you don't even need types to find them so you can just blast through methods and look at the lowered code (like what this PR does). |
|
Yeah I think JET might be too much just to find closure boxes. |
|
Fun! How about using PrettyTables to render the output? Also, may I suggest merging file and line into a single |
Uses JuliaLang/julia#60478 to find `Core.Box` entries in the package and remove them. After this, there are no such closure boxes in the package.
Found using the script in JuliaLang/julia#60478
contrib/scan-closure-boxes.jl
Outdated
| Base.visit(Core.methodtable) do m | ||
| scan_method!(lines, m, modules) | ||
| end | ||
| sort!(lines, by = entry -> entry.mod) |
There was a problem hiding this comment.
The sorting is very unstable, how about using this instead (it works well for me)
| sort!(lines, by = entry -> entry.mod) | |
| sort!(lines, by = entry -> (entry.mod, entry.func, entry.var)) |
|
The improvements derived from this seem like a good idea. However, I question whether it is useful to merge this into contrib, or merely leave it as a closed PR for historical reference. |
|
Yeah, I can make it a Pkg app or something instead. |
I used JuliaLang/julia#60478 to find all the `Core.Box` instances in the package and then fix them.
Found using the script in JuliaLang/julia#60478
Found using the script in JuliaLang/julia#60478
606e0a1 to
bfee356
Compare
Found using the script in JuliaLang/julia#60478
Uses JuliaLang/julia#60478 to find `Core.Box` entries in the package and remove them. After this, there are no such closure boxes in the package.
|
I like |
Detected using script from JuliaLang/julia#60478 This was the only Core.Box detected in VortexPasta.
bfee356 to
7d4c437
Compare
7d4c437 to
4cc0b40
Compare
| allowed_undefineds=nothing) | ||
| @nospecialize mods | ||
| ambs = Set{Method}() | ||
| mods = collect(mods)::Vector{Module} |
There was a problem hiding this comment.
julia> Test.detect_unbound_args()
ERROR: TypeError: in typeassert, expected Vector{Module}, got a value of type Vector{Union{}}
|
It is now in Test. |
contrib that checks for closure boxes in loaded codeTest.detect_closure_boxes for closure boxes in loaded code
Co-authored-by: Shuhei Kadowaki <40514306+aviatesk@users.noreply.github.com>
427f15f to
84ddda0
Compare
|
I dunno if the |
|
With the script, it was very easy to get the but that doesn't have the nice Would be nice to have an easy way to get the warn type printout from the results of the detect function. |
|
Can you not store the tuple of the signature, like you had in the script? |
| @nospecialize | ||
| ambs = Set{Tuple{Method,Method}}() | ||
| mods = collect(mods)::Vector{Module} | ||
| mods = Module[mods...] |
There was a problem hiding this comment.
Unrelated bugfix, should be separated out into independent PR and backported to v1.10, v1.12, v1.13?
|
With the newish argument type deduction for |
Changes recursive closures into proper methods. Best to review without whitespace. Found via #60478: ``` julia> Test.detect_closure_boxes_all_modules() 8-element Vector{Pair{Method, Vector{Symbol}}}: prune(graph1_a::Base.JuliaSyntax.SyntaxGraph, entrypoints_a::Vector{Int64}) @ Base.JuliaSyntax /JuliaSyntax/src/porcelain/syntax_graph.jl:816 => [:get_resolved!] _stm_check_usage(pats::Expr) @ Base.JuliaSyntax /JuliaSyntax/src/porcelain/syntax_graph.jl:1095 => [:_stm_check_pattern] ```
Uses JuliaLang/julia#60478 to find `Core.Box` entries in the package and remove them. After this, there are no such closure boxes in the package.
For example:
and indeed: