[JuliaLowering] Bugfixes - #62862
Conversation
mlechu
added
bugfix
a9cec23 to
e1ac935
Compare
| end | ||
| end | ||
| const _reserve_binding_lock = ReentrantLock() | ||
| const _reserve_binding_next = WeakKeyDict{Module,Dict{String,Int}}() |
There was a problem hiding this comment.
I don't think Claude's changes here are very good: the WeakKeyDict assumes that Modules are cleaned up, but for correctness right now we assume that all Modules are permanently rooted (it's always a bug for a Module to be GC'd) and JL can probably use m->lock instead of its own lock here (might need bindings to use from the Julia side - or alternatively, you could expose the "reservation" operation as a C API used by flisp + JL)
I think we probably want to follow flisp in using m->counter (for the simple case) and scanning through m->bindings for a reservation for the inner function case. If that is not efficient enough, we can add a reservations dict to jl_module_t but flisp has survived so far without it so perhaps not necessary
There was a problem hiding this comment.
I should have caught that. Let me rearrange some C stuff.
There was a problem hiding this comment.
but flisp has survived so far without it so perhaps not necessary
Looking into this, I realize JuliaLang/JuliaLowering.jl#186 is not specific to JL at all, so exposing the reservation API won't fix the slowness issue:
julia> @time JuliaLowering.include_string(Main, raw"""
module M
for i in 1:10000
@eval function f(x); function g(x); end; end
end
end
"""; expr_compat_mode=true)
15.812677 seconds (260.50 M allocations: 8.994 GiB, 4.59% gc time, 0.34% compilation time)
Main.M
julia> @time JuliaLowering.include_string(Main, raw"""
module M
for i in 1:30000
@eval function f(x); function g(x); end; end
end
end
"""; expr_compat_mode=true)
156.136695 seconds (2.28 G allocations: 76.129 GiB, 3.93% gc time, 0.00% compilation time)
Main.M
julia> @time Base.include_string(Main, raw"""
module M
for i in 1:10000
@eval function f(x); function g(x); end; end
end
end
""")
18.177919 seconds (3.39 M allocations: 138.019 MiB, 0.31% gc time)
Main.M
julia> @time Base.include_string(Main, raw"""
module M
for i in 1:30000
@eval function f(x); function g(x); end; end
end
end
""")
152.873729 seconds (10.17 M allocations: 414.350 MiB, 0.07% gc time)
Main.M
it's just that flisp gives up (defers to the module counter) more easily than JL in producing the extra-deterministic names. In the linked issue, that happens when declaring a kwcall method. I can probably just match this for now, although we'll later want to fix the nondeterminism in the module counter case and the quadratic runtime in the parsed-method-stack case.
There was a problem hiding this comment.
Yeah that's a good point - we were perhaps a little over-eager not to fix the quadratic behavior in #53719
We should probably go ahead and fix that up properly soon
|
@aviatesk perhaps I should avoid calling newly-exported |
|
Thanks for pinning this. diff --git a/JuliaLowering/src/desugaring.jl b/JuliaLowering/src/desugaring.jl
index b63aa10219..84ea258864 100644
--- a/JuliaLowering/src/desugaring.jl
+++ b/JuliaLowering/src/desugaring.jl
@@ -4338,7 +4338,11 @@ function expand_forms_2(ctx::DesugaringContext, ex::SyntaxTree, docs=nothing)
[K"tuple" as...] -> (nothing, as, @ast(ctx, sig, "Any"::K"core"))
end
if isnothing(name)
- name = newsym(ctx, sig, string(module_next_counter(ctx.layer.mod)))
+ @static if VERSION < v"1.14.0-DEV.3063"
+ name = newsym(ctx, sig, "#anon#")
+ else
+ name = newsym(ctx, sig, string(module_next_counter(ctx.layer.mod)))
+ end
@ast ctx ex [K"block" [K"local" name] expand_function_def(
ctx, ex, SyntaxList(name, args...), wheres, ex[2], rett)]
else
@@ -4348,7 +4352,11 @@ function expand_forms_2(ctx::DesugaringContext, ex::SyntaxTree, docs=nothing)
elseif k == K"->"
sig, wheres = flatten_wheres(ex[1])
@jl_assert kind(sig) === K"tuple" ex
- name = newsym(ctx, sig, string(module_next_counter(ctx.layer.mod)))
+ @static if VERSION < v"1.14.0-DEV.3063"
+ name = newsym(ctx, sig, "#->#")
+ else
+ name = newsym(ctx, sig, string(module_next_counter(ctx.layer.mod)))
+ end
rett = @ast(ctx, sig, "Any"::K"core")
@ast ctx ex [K"block" [K"local" name] expand_function_def(
ctx, ex, SyntaxList(name, children(sig)...), wheres, ex[2], rett)]
diff --git a/JuliaLowering/src/runtime.jl b/JuliaLowering/src/runtime.jl
index c89cbbdabb..07ff4ff2e1 100644
--- a/JuliaLowering/src/runtime.jl
+++ b/JuliaLowering/src/runtime.jl
@@ -394,13 +394,20 @@ end
# Even less likely to be deterministic than the above, but necessary to avoid
# quadratic behaviour where flisp doesn't already have it.
function reserve_module_binding_simple(mod, hint::String)
- i = module_next_counter(mod)
- name = "$hint#$i"
- b = _get_module_binding(mod, Symbol(name); create=true)
- # @assert !isdefined(b, :partitions) || b.partitions.kind === Base.PARTITION_KIND_GUARD hint
- name
+ # jl_module_next_counter is not exported before Julia 1.14.0-DEV.3063.
+ @static if VERSION < v"1.14.0-DEV.3063"
+ return reserve_module_binding_i(mod, hint)
+ else
+ i = module_next_counter(mod)
+ name = "$hint#$i"
+ b = _get_module_binding(mod, Symbol(name); create=true)
+ # @assert !isdefined(b, :partitions) || b.partitions.kind === Base.PARTITION_KIND_GUARD hint
+ return name
+ end
+end
+@static if VERSION >= v"1.14.0-DEV.3063"
+ module_next_counter(mod::Module) = @ccall(jl_module_next_counter(mod::Module)::UInt32)
end
-module_next_counter(mod) = @ccall(jl_module_next_counter(mod::Module)::Int)
# Return true if a `name` is defined in and *by* the module `mod`.
# Has no side effects, unlike isdefined()(+ we should also merge JuliaCI/julia-buildkite#566) |
Some tests by claude
I'd like to try loosening the requirement on lowering uses this internal name,
so do it in the compat layer instead of desugaring. Also thread context
through `est_to_dst`, since more stuff will probably need to go here.
Note this assumes users do not assign their own variables named `var#self#`,
which I think is a fair expectation. If we can assume it isn't used at
top-level, that would simplify things further.
This also doesn't resolve `#self#` in some default-arg cases, but that doesn't
appear in the ecosystem, and can be fixed if it does.
`var"#self#"` is also used in `isdefined`, and I realized it makes more sense to
allow `thisfunction` in `isdefined` than do anything special here.
With this change (and with undoing the `invoke_in_lowering_world` change due to
world age issues), JuliaLowering is able to lower itself!
Assisted-by: Claude Opus 5
Wrap the MacroExpansionError wrapper in LoadError. Noisy, but probably fine,
and offers "here is your macro call" provenance (which I think was the
point of the MacroExpansionError). There are ~20 pkgeval failures due to
tests checking for an error type.
I haven't changed the new-macro error wrapper. Ideally there would be good
error messages with no wrapper, but that would require provenance to be
read downstream.
Dumps provenance, of course. Tests botted. Assisted-by: Claude Opus 5
Give up and use the module counter for kwbody and anonymous methods. The
problem is still present when all enclosing functions are named, but that's
consistent with flisp.
This change should also address most of
JuliaLang/JuliaLowering.jl#172 by reversing the
name stack, but doesn't aim for parity.
Needs further work. It would be much more convenient for Revise if we didn't do
this.
This change also leaves the old behaviour around for interim 1.13 semi-support
Co-authored-by: Shuhei Kadowaki <aviatesk@gmail.com>
Attempt number three at making this bug work. Used by Makie.
topolarity
left a comment
topolarity
left a comment
There was a problem hiding this comment.
So many bugfixes in, and still just as pleasing to merge! Thanks @mlechu
|
IR tests now failing on master, will fix |
|
Actually, it should be fixed starting with 7b72c53f395 (but I'll keep an eye on it), as that's |
Update the JuliaSyntax/JuliaLowering pins from 2fefd0127e to 537a4c16f1. The JuliaLowering bugfix batch JuliaLang/julia#62862 included in this range changes two behaviors JETLS relied on, so this commit also adapts the affected code paths: - Old-style macro invocation failures are now rethrown wrapped as `LoadError(file, line, MacroExpansionError(...))` instead of a bare `MacroExpansionError`, so the `err isa JL.MacroExpansionError` check in `per_stmt_diagnostics!` no longer matched and macro expansion error diagnostics silently disappeared. The catch handler now unwraps the `LoadError` before dispatching on the error type. - `expand_struct_def`/`expand_typegroup_def` now emit an explicit `global` declaration for the type name ahead of the lowered definition. That node resolves to a `:global` binding distinct from the internal one the type-alias normalization in `compute_binding_occurrences` re-keys onto, so the struct name gained a duplicate `:decl` occurrence. Such same-`(mod, name)` global entries are now folded into the alias target, skipping occurrences already covered at the same byte range. Both regressions were caught by the existing test suite (`test_lowering_diagnostic.jl` and `test_occurrence_analysis.jl`); the full suite passes with this commit. Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
3 participants
More pkgeval findings from #61576. Careful review isn't needed outside of "Fix quadratic gensym behaviour for kwbody/genfunc," which I took from @topolarity's claude experiments. Fixes JuliaLang/JuliaLowering.jl#186.