[JuliaLowering] Fix is_ambiguous_local in permeable scopes

JuliaLowering/src/desugaring.jl

@@ -7,6 +7,7 @@ struct DesugaringContext{Attrs} <: AbstractLoweringContext
     mod::Module
     expr_compat_mode::Bool
     ssa_mapping::Dict{Int, IdTag}
+    world::UInt
 end
 
 # Translate a K"ssavalue" node from pre-lowered code into a normal SSA binding.
@@ -4473,7 +4474,7 @@ ensure_desugaring_attributes!(graph) = ensure_attributes!(
     ex = reparent(graph, ex)
     ctx_out = DesugaringContext(graph, ctx.bindings, ctx.scope_layers,
                                 current_layer(ctx).mod, ctx.expr_compat_mode,
-                                Dict{Int, IdTag}())
+                                Dict{Int, IdTag}(), ctx.macro_world)
     vr = valid_st1(ex)
     # surface only one error until we have pretty-printing for multiple
     if !vr.ok

JuliaLowering/src/runtime.jl

@@ -402,8 +402,8 @@ end
 # Has no side effects, unlike isdefined()
 #
 # (This should do what fl_defined_julia_global does for flisp lowering)
-function is_defined_and_owned_global(mod, name)
-    Base.binding_kind(mod, name) === Base.PARTITION_KIND_GLOBAL
+function is_defined_and_owned_global(mod, name, world::UInt=Base.get_world_counter())
+    return Base.invoke_in_world(world, Base.binding_kind, mod, name) === Base.PARTITION_KIND_GLOBAL
 end
 
 # "Reserve" a binding: create the binding if it doesn't exist but do not assign
@@ -414,7 +414,7 @@ function reserve_module_binding(mod, name)
     # lock is only accessible from C. See also the C code in
     # `fl_module_unique_name`.
     if _get_module_binding(mod, name; create=false) === nothing
-        _get_module_binding(mod, name; create=true) !== nothing
+        return _get_module_binding(mod, name; create=true) !== nothing
     else
         return false
     end

JuliaLowering/src/scope_analysis.jl

@@ -84,6 +84,7 @@ struct ScopeResolutionContext{Attrs} <: AbstractLoweringContext
     soft_assignable_globals::Set{NameKey}
     enable_soft_scopes::Bool
     expr_compat_mode::Bool
+    world::UInt
 end
 
 function contains_softscope_marker(ex)
@@ -299,7 +300,7 @@ function enter_scope!(ctx, ex)
                 push!(ctx.soft_assignable_globals, vk)
                 declare_in_scope!(ctx, top_scope(ctx), ex, :global)
             elseif scope.is_permeable && is_defined_and_owned_global(
-                ctx.scope_layers[vk.layer].mod, Symbol(vk.name))
+                ctx.scope_layers[vk.layer].mod, Symbol(vk.name), ctx.world)
                 # special soft scope rules: existing global variables are assigned to
                 if ctx.enable_soft_scopes
                     push!(ctx.soft_assignable_globals, vk)
@@ -320,7 +321,7 @@ function enter_scope!(ctx, ex)
                 # assign-existing-global if this is an explicit global that
                 # isn't at top level, or if the soft scope exception applies
             else
-                declare_in_scope!(ctx, scope, ex, :local)
+                declare_in_scope!(ctx, scope, ex, :local; is_ambiguous_local = scope.is_permeable)
             end
         elseif b.kind === :static_parameter
             throw(LoweringError(ex, "cannot overwrite a static parameter"))
@@ -756,14 +757,18 @@ metadata about each binding.
 This pass also records the set of binding IDs used locally within the
 enclosing lambda form and information about variables captured by closures.
 """
-@fzone "JL: resolve_scopes" function resolve_scopes(ctx::DesugaringContext, ex)
+@fzone "JL: resolve_scopes" function resolve_scopes(ctx::DesugaringContext, ex;
+                                                    soft_scope::Union{Nothing,Bool}=nothing,
+                                                    world::UInt=ctx.world)
     graph = ensure_scope_attributes!(copy_attrs(ctx.graph))
     ex = reparent(graph, ex)
+    enable_soft_scopes = soft_scope !== nothing ? soft_scope : contains_softscope_marker(ex)
     ctx2 = ScopeResolutionContext(graph, ctx.bindings, ctx.mod,
                                   Vector{ScopeInfo}(), Vector{ScopeId}(),
                                   ctx.scope_layers, Set{NameKey}(),
-                                  contains_softscope_marker(ex),
-                                  ctx.expr_compat_mode)
+                                  enable_soft_scopes,
+                                  ctx.expr_compat_mode,
+                                  world)
     ex2 = resolve_scopes(ctx2, ex)
     ctx3 = VariableAnalysisContext(graph, ctx2.bindings, ctx2.mod,
                                    ctx2.scopes, ex2.lambda_bindings,

JuliaLowering/test/scopes.jl

@@ -343,6 +343,148 @@ end
     end
 end
 
+module ambiguous_local
+    global x::Int = 0
+end
+
+function resolve_and_get_bindings(
+        mod::Module, ex;
+        world::UInt = Base.get_world_counter(),
+        soft_scope::Union{Nothing,Bool} = nothing,
+    )
+    est = JuliaLowering.expr_to_est(ex)
+    ctx1, ex1 = JuliaLowering.expand_forms_1(mod, est, false, world)
+    ctx2, ex2 = JuliaLowering.expand_forms_2(ctx1, ex1)
+    ctx3, _ = JuliaLowering.resolve_scopes(ctx2, ex2; soft_scope)
+    return ctx3.bindings.info
+end
+
+@testset "is_ambiguous_local" begin
+    # Assignment in for loop within begin block after toplevel assignment
+    let bindings = resolve_and_get_bindings(ambiguous_local, :(for _ = 1:10; x = 1; end))
+        binfo = only(filter(b->b.name=="x", bindings))
+        @test binfo.kind === :local
+        @test binfo.is_ambiguous_local
+    end
+
+    # while loop
+    let bindings = resolve_and_get_bindings(ambiguous_local, :(while x < 5; x += 1; break; end))
+        binfos = filter(b->b.name=="x", bindings)
+        @test length(binfos) == 2
+        binfo = only(filter(b->b.kind==:local, binfos))
+        @test binfo.is_ambiguous_local
+        @test count(b->b.kind==:global, binfos) == 1
+    end
+
+    # No ambiguity inside a function (hard scope)
+    let bindings = resolve_and_get_bindings(ambiguous_local, :(function f()
+            for _ = 1:10
+                x = 1
+            end
+        end))
+        binfo = only(filter(b->b.name=="x", bindings))
+        @test binfo.kind === :local
+        @test !binfo.is_ambiguous_local
+    end
+
+    # No ambiguity when shadowing global variable does not exist
+    let bindings = resolve_and_get_bindings(ambiguous_local, :(for _ = 1:10; y = 1; end))
+        binfo = only(filter(b->b.name=="y", bindings))
+        @test binfo.kind === :local
+        @test !binfo.is_ambiguous_local
+    end
+
+    # Explicit `global` should not produce ambiguous local
+    let bindings = resolve_and_get_bindings(ambiguous_local, :(for _ = 1:10; global x = 1; end))
+        binfo = only(filter(b->b.name=="x", bindings))
+        @test binfo.kind === :global
+    end
+
+    # Block containing a toplevel assignment preceding a permeable scope
+    let bindings = resolve_and_get_bindings(Module(), quote
+            x = 0
+            for _ = 1:10
+                x = 1
+            end
+        end)
+        binfos = filter(b->b.name=="x", bindings)
+        @test length(binfos) == 2
+        binfo = only(filter(b->b.kind==:local, binfos))
+        @test binfo.is_ambiguous_local
+        @test count(b->b.kind==:global, binfos) == 1
+    end
+    # Block containing a permeable scope followed by a toplevel assignment
+    let bindings = resolve_and_get_bindings(Module(), quote
+            for _ = 1:10
+                x = 1
+            end
+            x = 0
+        end)
+        binfos = filter(b->b.name=="x", bindings)
+        @test length(binfos) == 2
+        binfo = only(filter(b->b.kind==:local, binfos))
+        @test binfo.is_ambiguous_local
+        @test count(b->b.kind==:global, binfos) == 1
+    end
+
+    # For some reason, flisp can avoid ambiguity when there is an additional `global` annotation.
+    # JuliaLowering may want to follow suit, but it would be better to first decide on the details of this behaviour.
+    let bindings = resolve_and_get_bindings(Module(), quote
+            global x = 0
+            for _ = 1:10
+                x = 1
+            end
+        end)
+        binfos = filter(b->b.name=="x", bindings)
+        @test length(binfos) == 2
+        binfo = only(filter(b->b.kind==:local, binfos))
+        @test_broken !binfo.is_ambiguous_local
+        @test count(b->b.kind==:global, binfos) == 1
+    end
+    let bindings = resolve_and_get_bindings(Module(), quote
+            for _ = 1:10
+                x = 1
+            end
+            global x = 0
+        end)
+        binfos = filter(b->b.name=="x", bindings)
+        @test length(binfos) == 2
+        binfo = only(filter(b->b.kind==:local, binfos))
+        @test_broken !binfo.is_ambiguous_local
+        @test count(b->b.kind==:global, binfos) == 1
+    end
+
+    @testset "soft_scope kwarg override" begin
+        # Without soft_scope, x becomes an ambiguous local
+        let bindings = resolve_and_get_bindings(ambiguous_local, :(for _ = 1:10; x = 1; end))
+            binfo = only(filter(b->b.name=="x", bindings))
+            @test binfo.kind === :local
+            @test binfo.is_ambiguous_local
+        end
+        # With soft_scope=true, x stays global (no local created)
+        let bindings = resolve_and_get_bindings(ambiguous_local, :(for _ = 1:10; x = 1; end); soft_scope=true)
+            binfo = only(filter(b->b.name=="x", bindings))
+            @test binfo.kind === :global
+        end
+    end
+
+    @testset "world-age propagation" begin
+        let m = Module()
+            Core.eval(m, :(global x = 0))
+            bindings = resolve_and_get_bindings(m, :(for _ = 1:10; x = 1; end); world=Base.get_world_counter())
+            binfo = only(filter(b->b.name=="x", bindings))
+            @test binfo.kind === :local
+            @test binfo.is_ambiguous_local
+        end
+        let m = Module()
+            Core.eval(m, :(global x = 0))
+            bindings = resolve_and_get_bindings(m, :(for _ = 1:10; x = 1; end); world=Base.get_world_counter(), soft_scope=true)
+            binfo = only(filter(b->b.name=="x", bindings))
+            @test binfo.kind === :global
+        end
+    end
+end
+
 # Note: Certain flisp (un)hygiene behaviour is yet to be implemented.
 # In flisp, with no escaping:
 # - Top-level functions are unhygienic and declared in the macro's module

JuliaLowering/test/utils.jl

@@ -82,12 +82,6 @@ format_as_ast_macro(ex) = format_as_ast_macro(stdout, ex)
 
 # Test tools
 
-function desugar(mod::Module, src::String)
-    ex = parsestmt(SyntaxTree, src, filename="foo.jl")
-    ctx = JuliaLowering.DesugaringContext(syntax_graph(ex), Bindings(), ScopeLayer[], mod)
-    JuliaLowering.expand_forms_2(ctx, ex)
-end
-
 function uncomment_description(desc)
     replace(desc, r"^# ?"m=>"")
 end