Track GlobalRef consistently

[Keno]

Currently dependencies for GlobalRef and Symbol are tracked separately. Unfortunately, our lowering is not consistent about which one it uses (after lowering, all Symbols refer to globals in the current module), so the same variable can appear twice in the same expression, once as a globalref, once as a symbol. Currently, in that situation, this package misses a dependency, because it considers them two separate objects. Fix that by always normalizing to GlobalRef. This does require the caller to pass through a Module, but the alternative, would be for the caller to keep track of these dependencies implicitly, which would be painful. I think consistently usign GlobalRef here is cleanest.

[JamesWrigley]

I think I may have hit this too on Julia 1.11:

┌ Error: Failed to revise /home/james/git/CImGui.jl/src/CImGui.jl
│   exception =
│    TypeError: in typeassert, expected Symbol, got a value of type GlobalRef
│    Stacktrace:
│     [1] identify_framemethod_calls(frame::JuliaInterpreter.Frame)
│       @ LoweredCodeUtils ~/.julia/packages/LoweredCodeUtils/yox6n/src/signatures.jl:149
└ @ Revise ~/.julia/packages/Revise/bAgL0/src/packagedef.jl:724

There's a global variable in the package with type Union{Symbol, Nothing}.

[timholy]

LGTM once tests pass.

[timholy]

Is this transient or permanent? I'm fine with it either way, just curious.

[Keno]

Should be unreachable in current julia, but intended to fail loudly if julia base changes.

[timholy]

Tests on nightly are known to fail, you don't have to get those working.

[timholy]

I thought about fixing the conflicts for you and got confused about one element of your intent, please clarify. But tests may pass more easily if you rebase this on top of master (see #108 and #109).

[Keno]

Alright, I fixed the tests. The only issue is that since this changes the API surface that Revise uses, the versioning will be need to be bumped appropriately.

[Keno]

@timholy Can we make a plan to get this in? I think between this and my JuliaInterpreter changes we're most of the way there to get Revise back to working on 1.12.

[timholy]

Thanks, @Keno, this is great.

Before tagging a new release: @aviatesk, any other breaking changes you'd like to make?

[timholy]

In particular, maybe we should fix #99

[Keno]

Something broke in the final version of this. It no longer fixes LCU tests on julia master.

[timholy]

There's a Revise integration test that only runs on CI. But I presume that's not what you mean.

I'll build julia master and give it a whirl too.

[Keno]

It's also possibly related to further changes on master. I looked at it a bit and it looks similar to the case this PR fixed, but for :method. I tried the following, but it didn't fully work. I'll look more in the morning:

diff --git a/src/signatures.jl b/src/signatures.jl
index d6d9615..98dc773 100644
--- a/src/signatures.jl
+++ b/src/signatures.jl
@@ -121,8 +121,8 @@ MethodInfo(start) = MethodInfo(start, -1, Int[])
 struct SelfCall
     linetop::Int
     linebody::Int
-    callee::Symbol
-    caller::Union{Symbol,Bool,Nothing}
+    callee::GlobalRef
+    caller::Union{GlobalRef,Bool,Nothing}
 end

 """
@@ -140,14 +140,17 @@ which will correspond to a 3-argument `:method` expression containing a `CodeInf
 `callee` is the Symbol of the called method.
 """
 function identify_framemethod_calls(frame)
-    refs = Pair{Symbol,Int}[]
-    methodinfos = Dict{Symbol,MethodInfo}()
+    refs = Pair{GlobalRef,Int}[]
+    methodinfos = Dict{GlobalRef,MethodInfo}()
     selfcalls = SelfCall[]
     for (i, stmt) in enumerate(frame.framecode.src.code)
         isa(stmt, Expr) || continue
         if stmt.head === :global && length(stmt.args) == 1
             key = stmt.args[1]
             if isa(key, Symbol)
+                key = GlobalRef(moduleof(frame), key)
+            end
+            if isa(key, GlobalRef)
                 # We don't know for sure if this is a reference to a method, but let's
                 # tentatively cue it
                 push!(refs, key=>i)
@@ -159,7 +162,10 @@ function identify_framemethod_calls(frame)
                 if is_return(tstmt)
                     tex = tstmt.val
                     if isa(tex, Expr)
-                        if tex.head === :method && (methname = tex.args[1]; isa(methname, Symbol))
+                        if tex.head === :method && (methname = tex.args[1]; isa(methname, Union{Symbol, GlobalRef}))
+                            if isa(methname, Symbol)
+                                methname = GlobalRef(moduleof(frame), methname)
+                            end
                             push!(refs, methname=>i)
                         end
                     end
@@ -168,7 +174,7 @@ function identify_framemethod_calls(frame)
         elseif ismethod1(stmt)
             key = stmt.args[1]
             key = normalize_defsig(key, frame)
-            key = key::Symbol
+            key = key::GlobalRef
             mi = get(methodinfos, key, nothing)
             if mi === nothing
                 methodinfos[key] = MethodInfo(i)
@@ -178,7 +184,7 @@ function identify_framemethod_calls(frame)
         elseif ismethod3(stmt)
             key = stmt.args[1]
             key = normalize_defsig(key, frame)
-            if key isa Symbol
+            if key isa GlobalRef
                 # XXX A temporary hack to fix https://github.com/JuliaDebug/LoweredCodeUtils.jl/issues/80
                 #     We should revisit it.
                 mi = get(methodinfos, key, MethodInfo(1))
@@ -188,7 +194,7 @@ function identify_framemethod_calls(frame)
             end
             msrc = stmt.args[3]
             if msrc isa CodeInfo
-                key = key::Union{Symbol,Bool,Nothing}
+                key = key::Union{GlobalRef,Bool,Nothing}
                 for (j, mstmt) in enumerate(msrc.code)
                     isa(mstmt, Expr) || continue
                     jj = j
@@ -196,12 +202,15 @@ function identify_framemethod_calls(frame)
                         mkey = mstmt.args[1]
                         if isa(mkey, SSAValue) || isa(mkey, Core.SSAValue)
                             refstmt = msrc.code[mkey.id]
-                            if isa(refstmt, Symbol)
+                            if isa(refstmt, Union{Symbol, GlobalRef})
                                 jj = mkey.id
                                 mkey = refstmt
                             end
                         end
                         if isa(mkey, Symbol)
+                            mkey = GlobalRef(moduleof(frame), mkey)
+                        end
+                        if isa(mkey, GlobalRef)
                             # Could be a GlobalRef but then it's outside frame
                             haskey(methodinfos, mkey) && push!(selfcalls, SelfCall(i, jj, mkey, key))
                         elseif is_global_ref(mkey, Core, isdefined(Core, :_apply_iterate) ? :_apply_iterate : :_apply)
@@ -212,6 +221,9 @@ function identify_framemethod_calls(frame)
                             end
                             mkey = mstmt.args[end-2]
                             if isa(mkey, Symbol)
+                                mkey = GlobalRef(moduleof(frame), mkey)
+                            end
+                            if isa(mkey, GlobalRef)
                                 haskey(methodinfos, mkey) && push!(selfcalls, SelfCall(i, jj, mkey, key))
                             end
                         end
@@ -219,7 +231,10 @@ function identify_framemethod_calls(frame)
                         newex = mstmt.args[2]
                         if isa(newex, Expr)
                             if newex.head === :new && length(newex.args) >= 2 && is_global_ref(newex.args[1], Core, :GeneratedFunctionStub)
-                                mkey = newex.args[2]::Symbol
+                                mkey = newex.args[2]
+                                if isa(mkey, Symbol)
+                                    mkey = GlobalRef(moduleof(frame), mkey)
+                                end
                                 haskey(methodinfos, mkey) && push!(selfcalls, SelfCall(i, jj, mkey, key))
                             end
                         end
@@ -235,20 +250,21 @@ function identify_framemethod_calls(frame)
     return methodinfos, selfcalls
 end

-# try to normalize `def` to `Symbol` representation
+# try to normalize `def` to `GlobalRef` representation
 function normalize_defsig(@nospecialize(def), frame::Frame)
     if def isa QuoteNode
         def = nameof(def.value)
-    elseif def isa GlobalRef
-        def = def.name
+    end
+    if def isa Symbol
+        def = GlobalRef(moduleof(frame), def)
     end
     return def
 end

 function callchain(selfcalls)
-    calledby = Dict{Symbol,Union{Symbol,Bool,Nothing}}()
+    calledby = Dict{GlobalRef,Union{GlobalRef,Bool,Nothing}}()
     for sc in selfcalls
-        startswith(String(sc.callee), '#') || continue
+        startswith(String(sc.callee.name), '#') || continue
         caller = get(calledby, sc.callee, nothing)
         if caller === nothing
             calledby[sc.callee] = sc.caller
@@ -261,7 +277,7 @@ function callchain(selfcalls)
 end

 function set_to_running_name!(@nospecialize(recurse), replacements, frame, methodinfos, selfcall, calledby, callee, caller)
-    if isa(caller, Symbol) && startswith(String(caller), '#')
+    if isa(caller, GlobalRef) && startswith(String(caller.name), '#')
         rep = get(replacements, caller, nothing)
         if rep === nothing
             parentcaller = get(calledby, caller, nothing)
@@ -313,9 +329,9 @@ the same name in the `start:stop` range.
 """
 function rename_framemethods!(@nospecialize(recurse), frame::Frame, methodinfos, selfcalls, calledby)
     src = frame.framecode.src
-    replacements = Dict{Symbol,Symbol}()
+    replacements = Dict{GlobalRef,GlobalRef}()
     for (callee, caller) in calledby
-        (!startswith(String(callee), '#') || haskey(replacements, callee)) && continue
+        (!startswith(String(callee.name), '#') || haskey(replacements, callee)) && continue
         idx = findfirst(sc->sc.callee === callee && sc.caller === caller, selfcalls)
         idx === nothing && continue
         try
@@ -438,7 +454,7 @@ function get_running_name(@nospecialize(recurse), frame, pc, name, parentname)
     methparent = whichtt(sigtparent)
     methparent === nothing && return name, pc, lastpcparent  # caller isn't defined, no correction is needed
     if isgen
-        cname = nameof(methparent.generator.gen)
+        cname = GlobalRef(moduleof(frame), nameof(methparent.generator.gen))
     else
         bodyparent = Base.uncompressed_ast(methparent)
         bodystmt = bodyparent.code[end-1]
@@ -450,7 +466,7 @@ function get_running_name(@nospecialize(recurse), frame, pc, name, parentname)
         isa(ref, GlobalRef) || @show ref typeof(ref)
         @assert isa(ref, GlobalRef)
         @assert ref.mod == moduleof(frame)
-        cname = ref.name
+        cname = ref
     end
     return cname, pc, lastpcparent
 end
@@ -526,7 +542,7 @@ function methoddef!(@nospecialize(recurse), signatures, frame::Frame, @nospecial
     elseif name === missing
         Base.invokelatest(error, "given invalid definition: ", stmt)
     end
-    name = name::Symbol
+    name = name::GlobalRef
     # Is there any 3-arg method definition with the same name? If not, avoid risk of executing code that
     # we shouldn't (fixes https://github.com/timholy/Revise.jl/issues/758)
     found = false
diff --git a/test/signatures.jl b/test/signatures.jl
index 7beba46..159b260 100644
--- a/test/signatures.jl
+++ b/test/signatures.jl
@@ -103,6 +103,7 @@ bodymethtest5(x, y=Dict(1=>2)) = 5
     modeval, modinclude = getfield(Lowering, :eval), getfield(Lowering, :include)
     failed = []
     for fsym in nms
+        isdefined(Lowering, fsym) || continue
         f = getfield(Lowering, fsym)
         isa(f, Base.Callable) || continue
         (f === modeval || f === modinclude) && continue
@@ -348,10 +349,11 @@ bodymethtest5(x, y=Dict(1=>2)) = 5
     Core.eval(Lowering, ex)
     frame = Frame(Lowering, ex)
     dct = rename_framemethods!(frame)
-    ks = collect(filter(k->startswith(String(k), "#Items#"), keys(dct)))
+    ks = collect(filter(k->startswith(String(k.name), "#Items#"), keys(dct)))
     @test length(ks) == 2
     @test dct[ks[1]] == dct[ks[2]]
-    @test isdefined(Lowering, ks[1]) || isdefined(Lowering, ks[2])
+    @test ks[1].mod === ks[2].mod === Lowering
+    @test isdefined(Lowering, ks[1].name) || isdefined(Lowering, ks[2].name)
     if !isdefined(Core, :kwcall)
         nms = filter(sym->occursin(r"#Items#\d+#\d+", String(sym)), names(Lowering; all=true))
         @test length(nms) == 1