Correctly revise methods defined in external method tables (#904)

* Revise external method tables

* Bump compat for CodeTracking

* Use MethodInfoKey, rename a few `sigs` to `mt_sigs`

* Start updating docs a bit

* Patch CI to test with upstream dependencies

* Update outdated comment

* Attempt to fix Git tests

* Refactor tests and increase coverage

* Add tests for method deletion

* Bump JuliaInterpreter compat bound

* Fix tests for 1.10

* Update documentation, more `mt_sigs` renames

* Add NEWS.md entry

* Bump package version to 3.8.0

* Update CI patches

* Update CI dependency patch

* Don't test `(::Method).deleted_world` on 1.12+

* Remove CI patches

* Raise compat bound for LoweredCodeUtils

Author: serenity4

NEWS.md

@@ -3,6 +3,10 @@
 This file describes only major changes, and does not include bug fixes,
 cleanups, or minor enhancements.
 
+## Revise 3.8
+
+* Methods defined on external method tables via `Base.Experimental.@overlay` can now be correctly revised ([#904]).
+
 ## Revise 3.3
 
 * Upgrade to JuliaInterpreter 0.9 and drop support for Julia prior to 1.6 (the new LTS).
@@ -151,3 +155,4 @@ New features:
 [#243]: https://github.com/timholy/Revise.jl/pull/243
 [#245]: https://github.com/timholy/Revise.jl/pull/245
 [#278]: https://github.com/timholy/Revise.jl/pull/278
+[#904]: https://github.com/timholy/Revise.jl/pull/904

Project.toml

@@ -21,10 +21,10 @@ Distributed = "8ba89e20-285c-5b6f-9357-94700520ee1b"
 DistributedExt = "Distributed"
 
 [compat]
-CodeTracking = "1.2"
+CodeTracking = "2"
 Distributed = "1"
 JuliaInterpreter = "0.10"
-LoweredCodeUtils = "3.3"
+LoweredCodeUtils = "3.4.3"
 OrderedCollections = "1"
 # Exclude Requires-1.1.0 - see https://github.com/JuliaPackaging/Requires.jl/issues/94
 Requires = "~1.0, ^1.1.1"

docs/src/debugging.md

@@ -159,13 +159,13 @@ julia> rlogger.logs
           #= /tmp/revisetest.jl:9 =#
           x ^ 4
       end))))
- Revise.LogRecord(Debug, LineOffset, Action, Revise_fb38a7f7, "/home/tim/.julia/dev/Revise/src/Revise.jl", 296, (time=1.557996459331061e9, deltainfo=(Any[Tuple{typeof(mult2),Any}], :(#= /tmp/revisetest.jl:11 =#) => :(#= /tmp/revisetest.jl:13 =#))))
+ Revise.LogRecord(Debug, LineOffset, Action, Revise_fb38a7f7, "/home/tim/.julia/dev/Revise/src/Revise.jl", 296, (time=1.557996459331061e9, deltainfo=(Pair{Union{Nothing, MethodTable}, Type}[nothing => Tuple{typeof(mult2),Any}], :(#= /tmp/revisetest.jl:11 =#) => :(#= /tmp/revisetest.jl:13 =#))))
  Revise.LogRecord(Debug, Eval, Action, Revise_9147188b, "/home/tim/.julia/dev/Revise/src/Revise.jl", 276, (time=1.557996459391182e9, deltainfo=(Main.ReviseTest.Internal, :(mult3(x) = begin
           #= /tmp/revisetest.jl:14 =#
           3x
       end))))
- Revise.LogRecord(Debug, LineOffset, Action, Revise_fb38a7f7, "/home/tim/.julia/dev/Revise/src/Revise.jl", 296, (time=1.557996459391642e9, deltainfo=(Any[Tuple{typeof(unchanged),Any}], :(#= /tmp/revisetest.jl:18 =#) => :(#= /tmp/revisetest.jl:19 =#))))
- Revise.LogRecord(Debug, LineOffset, Action, Revise_fb38a7f7, "/home/tim/.julia/dev/Revise/src/Revise.jl", 296, (time=1.557996459391695e9, deltainfo=(Any[Tuple{typeof(unchanged2),Any}], :(#= /tmp/revisetest.jl:20 =#) => :(#= /tmp/revisetest.jl:21 =#))))
+ Revise.LogRecord(Debug, LineOffset, Action, Revise_fb38a7f7, "/home/tim/.julia/dev/Revise/src/Revise.jl", 296, (time=1.557996459391642e9, deltainfo=(Pair{Union{Nothing, MethodTable}, Type}[nothing => Tuple{typeof(unchanged),Any}], :(#= /tmp/revisetest.jl:18 =#) => :(#= /tmp/revisetest.jl:19 =#))))
+ Revise.LogRecord(Debug, LineOffset, Action, Revise_fb38a7f7, "/home/tim/.julia/dev/Revise/src/Revise.jl", 296, (time=1.557996459391695e9, deltainfo=(Pair{Union{Nothing, MethodTable}, Type}[nothing => Tuple{typeof(unchanged2),Any}], :(#= /tmp/revisetest.jl:20 =#) => :(#= /tmp/revisetest.jl:21 =#))))
 ```
 
 You can see that Revise started by deleting three methods, followed by evaluating three new versions of those methods. Interspersed are various changes to the line numbering.

docs/src/internals.md

@@ -137,6 +137,7 @@ Tuple{typeof(print_item),IO,Any,Integer,String}     # print_item(io, item, 2, "
 ```
 
 In Revise's internal code, a definition is often represented with a variable `def`, and a signature-type with `sigt`.
+The method table for which the method was defined is also represented, to form a `mt_sigt` pair.
 Recent versions of Revise do not make extensive use of signature expressions.
 
 ### Computing signatures
@@ -226,9 +227,9 @@ Most of Revise's magic comes down to just three internal variables:
 
 Two "maps" are central to Revise's inner workings: `ExprsSigs` maps link
 definition=>signature-types (the forward workflow), while `CodeTracking` (specifically,
-its internal variable `method_info`) links from
-signature-type=>definition (the backward workflow).
-Concretely, `CodeTracking.method_info` is just an `IdDict` mapping `sigt=>(locationinfo, def)`.
+its internal variable `method_info`) links from a
+method table/signature-type pair to the corresponding definition (the backward workflow).
+Concretely, `CodeTracking.method_info` is just an `IdDict` mapping `(mt => sigt) => (locationinfo, def)`.
 Of note, a stack frame typically contains a link to a method, which stores the equivalent
 of `sigt`; consequently, this information allows one to look up the corresponding
 `locationinfo` and `def`. (When methods move, the location information stored by CodeTracking
@@ -237,8 +238,8 @@ gets updated by Revise.)
 Some additional notes about Revise's `ExprsSigs` maps:
 
 - For expressions that do not define a method, it is just `def=>nothing`
-- For expressions that do define a method, it is `def=>[sigt1, ...]`.
-  `[sigt1, ...]` is the list of signature-types generated from `def` (often just one,
+- For expressions that do define a method, it is `def=>[mt_sigt1, ...]`.
+  `[mt_sigt1, ...]` is the list of method table/signature-type pairs generated from `def` (often just one,
   but more in the case of methods with default arguments or keyword arguments).
 - They are represented as an `OrderedDict` so as to preserve the sequence in which expressions
   occur in the file.
@@ -255,7 +256,7 @@ Some additional notes about Revise's `ExprsSigs` maps:
   the location information stored by `CodeTracking`.
 
 `ExprsSigs` are organized by module and then file, so that one can map
-`filename`=>`module`=>`def`=>`sigts`.
+`filename`=>`module`=>`def`=>`mt_sigts`.
 Importantly, single-file modules can be "reconstructed" from the keys of the corresponding
 `ExprsSigs` (and multi-file modules from a collection of such items), since they hold
 the complete ordered set of expressions that would be `eval`ed to define the module.
@@ -334,13 +335,13 @@ FileInfo(Items=>ExprsSigs with the following expressions:
       end), )
 ```
 
-This is just a summary; to see the actual `def=>sigts` map, do the following:
+This is just a summary; to see the actual `def=>mt_sigts` map, do the following:
 
 ```julia
 julia> pkgdata.fileinfos[2].modexsigs[Items]
-OrderedCollections.OrderedDict{Revise.RelocatableExpr,Union{Nothing, Array{Any,1}}} with 2 entries:
-  :(indent(::UInt16) = begin…                       => Any[Tuple{typeof(indent),UInt16}]
-  :(indent(::UInt8) = begin…                        => Any[Tuple{typeof(indent),UInt8}]
+OrderedCollections.OrderedDict{Module, OrderedCollections.OrderedDict{Revise.RelocatableExpr, Union{Nothing, Vector{Pair{Union{Nothing, Core.MethodTable}, Type}}}}} with 2 entries:
+  :(indent(::UInt16) = begin…                       => Pair{Union{Nothing, MethodTable}, Type}[nothing => Tuple{typeof(indent),UInt16}]
+  :(indent(::UInt8) = begin…                        => Pair{Union{Nothing, MethodTable}, Type}[nothing => Tuple{typeof(indent),UInt8}]
 ```
 
 These are populated now because we specified `__precompile__(false)`, which forces

src/Revise.jl

@@ -35,7 +35,7 @@ module Revise
 
 using OrderedCollections, CodeTracking, JuliaInterpreter, LoweredCodeUtils
 
-using CodeTracking: PkgFiles, basedir, srcfiles, basepath
+using CodeTracking: PkgFiles, basedir, srcfiles, basepath, MethodInfoKey
 using JuliaInterpreter: Compiled, Frame, Interpreter, LineTypes, RecursiveInterpreter
 using JuliaInterpreter: codelocs, finish_and_return!, get_return, is_doc_expr, isassign,
                         isidentical, is_quotenode_egal, linetable, lookup, moduleof,

src/lowered.jl

@@ -9,7 +9,7 @@ end
 # This defines the API needed to store signatures using methods_by_execution!
 # This default version is simple and only used for testing purposes.
 # The "real" one is CodeTrackingMethodInfo in Revise.jl.
-const MethodInfo = IdDict{Type,LineNumberNode}
+const MethodInfo = IdDict{MethodInfoKey,LineNumberNode}
 add_signature!(methodinfo, @nospecialize(sig), ln) = push!(methodinfo, sig=>ln)
 push_expr!(methodinfo, mod::Module, ex::Expr) = methodinfo
 pop_expr!(methodinfo) = methodinfo
@@ -309,7 +309,7 @@ function _methods_by_execution!(interp::Interpreter, methodinfo, frame::Frame, i
     mod = moduleof(frame)
     # Hoist this lookup for performance. Don't throw even when `mod` is a baremodule:
     modinclude = isdefined(mod, :include) ? getfield(mod, :include) : nothing
-    signatures = []  # temporary for method signature storage
+    signatures = MethodInfoKey[]  # temporary for method signature storage
     pc = frame.pc
     while true
         JuliaInterpreter.is_leaf(frame) || (@warn("not a leaf"); break)
@@ -457,13 +457,13 @@ function _methods_by_execution!(interp::Interpreter, methodinfo, frame::Frame, i
                         uT = Base.unwrap_unionall(T)::DataType
                         ft = uT.types
                         sig1 = Tuple{Base.rewrap_unionall(Type{uT}, T), Any[Any for i in 1:length(ft)]...}
-                        push!(signatures, sig1)
+                        push!(signatures, nothing => sig1)
                         sig2 = Base.rewrap_unionall(Tuple{Type{T}, ft...}, T)
                         while T isa UnionAll
                             sig2 isa UnionAll || (sig2 = sig1; break) # sig2 doesn't define all parameters, so drop it
                             T = T.body
                         end
-                        sig1 == sig2 || push!(signatures, sig2)
+                        sig1 == sig2 || push!(signatures, nothing => sig2)
                         for sig in signatures
                             add_signature!(methodinfo, sig, lnn)
                         end