Skip to content

Conversation

timholy
Copy link
Owner

@timholy timholy commented Mar 13, 2025

The aim here is to provide full support for struct/const revision. This first commit just adds simple tests, but it already reveals something that needs to be addressed: when a struct gets revised, methods defined for that struct aren't automatically re-evaluated for the new type. Specifically, in the added tests, after revising the definition of Point we get

julia> methods(StructConst.firstval)
# 1 method for generic function "firstval" from StructConst:
 [1] firstval(p::@world(StructConst.Point, 40598:40739))
     @ /tmp/2wfLvVmwY7/StructConst/src/StructConst.jl:11

and there is no defined method for a currently-valid StructConst.Point.

@Keno, if you have a moment, let me check my understanding of the current situation:

  • method applicability is assessed by type-intersection, as usual, but now types (via their bindings) can have a world-age
  • the MethodInstances and CodeInstances involving the old type are still valid (their world age is not capped, but of course they require inputs with old types)
  • there is no invalidation step needed for changes of the kind tested with the revision of Point (invalidate_code_for_globalref! is not called?)
  • we can't count on binding.backedges to list everything that uses this binding
  • the right solution is for Revise to:
    • detect that we're about to redefine a struct
    • search the entire system for any methods where that type appears in a signature
    • parse the source files, if necessary
    • redefine the struct
    • re-evaluate the method definitions

Presumably, waiting to do the last step as the last stage of a revision would be wise, as it is possible that more than one struct will be revised at the same time, and one might as well do each evaluation only once.

@Keno
Copy link
Collaborator

Keno commented Mar 13, 2025

  • method applicability is assessed by type-intersection, as usual, but now types (via their bindings) can have a world-age

I think that's a valid way to say it, but just to clarify, nothing about the type object itself changes, just the name by which you have to access it

  • the MethodInstances and CodeInstances involving the old type are still valid (their world age is not capped, but of course they require inputs with old types)

yes

  • there is no invalidation step needed for changes of the kind tested with the revision of Point (invalidate_code_for_globalref! is not called?)

I don't understand what you're asking

  • we can't count on binding.backedges to list everything that uses this binding

It lists all cross-module edges to get everything, you also need to iterate all methods in the same module as the binding.

  • the right solution is for Revise to:

    • detect that we're about to redefine a struct
    • search the entire system for any methods where that type appears in a signature
    • parse the source files, if necessary
    • redefine the struct
    • re-evaluate the method definitions

In the fullness of time, the correct way to implement Revise is to run the toplevel expressions in a non-standard evaluation mode that tracks dependency edges for any evaluated constants. However, that should maybe wait until after the JuliaLowering changes and the interpreter rewrite. I think your proposed strategy is reasonable in the meantime although not fully complete.

@timholy timholy force-pushed the teh/struct_revision branch from e1a5084 to 3f878e3 Compare March 13, 2025 12:15
@timholy
Copy link
Owner Author

timholy commented Mar 13, 2025

there is no invalidation step needed for changes of the kind tested with the revision of Point (invalidate_code_for_globalref! is not called?)

I don't understand what you're asking

I was wondering whether some kind of callback in there could be helpful in short-circuiting the process of determining which methoddefs need to be re-evaluated, but if it's not called then that won't help. From what I can tell, that's not going to work anyway because it only traverses the module containing the type definition. But your comment

It lists all cross-module edges to get everything, you also need to iterate all methods in the same module as the binding.

seems to be what I was looking for.

This seems fairly straightforward, thanks for the excellent groundwork.

@Keno
Copy link
Collaborator

Keno commented Mar 13, 2025

I was wondering whether some kind of callback in there could be helpful in short-circuiting the process of determining which methoddefs need to be re-evaluated

No, there's no edges of any kind for signatures. You need to scan the entire system. However, an earlier version of that code did the same whole-system scan, so you can steal some code for whole-system scanning.

@timholy
Copy link
Owner Author

timholy commented Mar 13, 2025

I added a cross-module test (the StructConstUser module), expecting this might populate b.bindings, but I get

julia> b = convert(Core.Binding, GlobalRef(StructConst, :Point))
Binding StructConst.Point
   40743:- constant binding to StructConst.Point
   40598:40742 - constant binding to @world(StructConst.Point, 40598:40742)
   1:0 - backdated constant binding to @world(StructConst.Point, 40598:40742)
   1:0 - backdated constant binding to @world(StructConst.Point, 40598:40742)

julia> b.backedges
ERROR: UndefRefError: access to undefined reference
Stacktrace:
 [1] getproperty(x::Core.Binding, f::Symbol)
   @ Base ./Base_compiler.jl:55
 [2] top-level scope
   @ REPL[6]:1

so I don't think I understand what

It lists all cross-module edges

really means. It also doesn't populate if I add

struct PointWrapper
    p::StructConst.Point
end

to that module.

I understand that I have to traverse the whole system, I'm just curious about what b.backedges stores.

@Keno
Copy link
Collaborator

Keno commented Mar 13, 2025

There's two kinds of edges that are tracked there. One is explicit import/using. The other is to lowered code of method definitions (but only after the first inference). At no point is an edge ever added for an evaluation of a binding, only for compilation of code that references the binding.

@timholy
Copy link
Owner Author

timholy commented Mar 13, 2025

Just to cross-check:

                module StructConstUser
                using StructConst: Fixed, Point
                struct PointWrapper
                    p::Point
                end
                scuf(f::Fixed) = 33 * f.x
                scup(p::Point) = 44 * p.x
                end

yields a backedge for the using StructConst: Fixed, Point statement, but I don't see anything related to scup even if I evaluate it:

julia> e = only(b.backedges)
Binding StructConstUser.Point
   40598:- explicit `using` from StructConst.Point
   1:0 - undefined binding - guard entry

@Keno
Copy link
Collaborator

Keno commented Mar 13, 2025

but I don't see anything related to scup even if I evaluate it:

That's because scup doesn't use any bindings other than * and getproperty in its lowered code (as I said, no edges for signatures). However, even if you had like mkscup() = Point(), that binding would be to StructConstUser. To actually get a cross-module edge is not that easy, but you could do @eval mkscup() = $(GlobalRef(StructConst, :Point))(). The most common case to get cross-module edges is macros. Also, not a lot of edges is good, that is the design goal, since we don't want to store them :).

@timholy
Copy link
Owner Author

timholy commented Mar 14, 2025

This is getting close, but there's a philosophical question to answer: should Revise try to hew to Base behavior as closely as possible, or should it add/modify behaviors for what might be a nicer interactive experience?

In this case, the issue is the following: when I redefine Point, in Base you can still call previously-defined functions on "old data." However, in this PR, calling functions on old data throws (or is intended to throw) a MethodError. Why might you want that? Because I imagine that a common user-error will be to forget to refresh your data with updated types, and if your whole code pipeline runs (using all the old definitions), I imagine people will bang their heads against walls trying to figure out why their edits aren't having the intended effect. In contrast, if they get a no method foo(::@world(Point, m:n)), they'll pretty quickly learn how you fix it.

I should say that initially I wasn't aiming in this direction, and this current proposal arose from noticing some unexpected subtleties about the order in which Revise's cache files get set up: the order in which you parse the source, lower the code, and rebind the name matters quite a lot, and some of the choices in this version are designed to compensate for the fact that Revise normally doesn't parse a file unless it's been edited. (Now, we'll also have to parse any file with a method whose signature references an updated type.) I think it's possible to hew to the Base behavior, if we decide that's better, but the initial bugs I faced led me to ask what behavior we actually want, and I came to the conclusion that it's likely a better user experience if we invalidate methods that only work on old types.

@timholy timholy changed the title [WIP] Fix struct/const revision Fix struct/const revision Mar 15, 2025
@Keno
Copy link
Collaborator

Keno commented Mar 15, 2025

However, in this PR, calling functions on old data throws (or is intended to throw) a MethodError. Why might you want that?

I think that's fine. Conceptually Revise does both adding new methods and deleting old ones. Base is always "append only".

@timholy
Copy link
Owner Author

timholy commented Mar 16, 2025

The collateral damage to tests is something I'm looking into; if one runs all the tests, there appear to be some failures that predate this effort, including an incomplete updating of the ecosystem to JuliaLang/julia#52415. I'll work on that.

But meanwhile, this seems to be working:

tim@diva:~/.julia/dev/Revise$ ~/src/juliaw/julia --startup=no --project -e 'using Pkg; Pkg.test(; test_args=["struct/const revision"], coverage=false)'
     Testing Revise
      Status `/tmp/jl_4EbO8B/Project.toml`
# Pkg output deleted
     Testing Running tests...
Test Summary: | Pass  Total   Time
Revise        |   30     30  10.3s
beginning cleanup

and may be ready for anyone who wants to review it.

@timholy timholy force-pushed the teh/struct_revision branch 2 times, most recently from e8ba7ab to 7b6d113 Compare April 11, 2025 17:27
@timholy
Copy link
Owner Author

timholy commented Apr 12, 2025

@Keno @vtjnash @aviatesk @JeffBezanson here's a fun one:

julia> using Revise
Precompiling Revise finished.
  1 dependency successfully precompiled in 6 seconds. 19 already precompiled.

julia> Revise.track(Core.Compiler)

julia> fieldnames(Core.Compiler.NativeInterpreter)
(:world, :method_table, :inf_cache, :codegen, :inf_params, :opt_params)

#
# edited Compiler/src/types.jl to add a field `dummy` to `NativeInterpreter`
#

julia> 1+1
WARNING: Detected access to binding `Compiler.#NativeInterpreter#476` in a world prior to its definition world.
  Julia 1.12 has introduced more strict world age semantics for global bindings.
  !!! This code may malfunction under Revise.
  !!! This code will error in future versions of Julia.
Hint: Add an appropriate `invokelatest` around the access to this binding.
     # lots more like this
Compiling the compiler. This may take several minutes ...
Base.Compiler ──── 1.72376 seconds
2

julia> fieldnames(Core.Compiler.NativeInterpreter)
(:world, :method_table, :inf_cache, :codegen, :inf_params, :opt_params, :dummy)

julia> function f(::Integer)
           Base.Experimental.@force_compile
           return 1
       end
f (generic function with 1 method)

julia> f(5)
1

@aviatesk
Copy link
Collaborator

This is interesting.

So it looks like this PR tracks edges not only from binding to method signature, but also from binding to method body?

I think a similar mechanism will be needed when developing a new language server. If it's already implemented in Revise, I'd like to reuse it.

I haven't had a chance to look at this PR closely yet, but I'll read it later.

@timholy
Copy link
Owner Author

timholy commented Apr 12, 2025

Yes, when a type gets rebound, Revise will scan the entire system for methods m::Method where that type appears in m.sig. It then tries to find the expression that defined that method, deletes the old method, and re-evaluates the expression. That way it regenerates methods for the new meaning of the type.

There's a separate bit of novelty for Core.Compiler: this PR also checks whether any of the entry points to type inference have been invalidated, and if so calls bootstrap!

@timholy
Copy link
Owner Author

timholy commented Apr 12, 2025

I'll be curious to see whether it improves the efficiency of hacking on inference.

@Keno
Copy link
Collaborator

Keno commented Apr 12, 2025

There's a separate bit of novelty for Core.Compiler: this PR also checks whether any of the entry points to type inference have been invalidated, and if so calls bootstrap!

I don't know if I like that. We already have @activate Compiler[:codegen] for this use case.

@timholy
Copy link
Owner Author

timholy commented Apr 13, 2025

I didn't know about @activate. Is there a demo of the workflow somewhere? I've seen the docs, and they're good, but I suspect there's a bit more wisdom around effective usage than they convey.

I can back that commit out, if there are other mechanisms to achieve the same aim. Are you saying that Revise.track(Core.Compiler) should be deprecated?

@KristofferC
Copy link
Collaborator

KristofferC commented Apr 16, 2025

I tried this. On:

  • Revise (this PR)
  • LoweredCodeUtils master
  • JuliaInterpreter master
  • Julia 1.12.0-beta1

What I did was:

  • Load Revise, Tensors.
  • Make the following revision to Tensors:
    diff --git a/src/Tensors.jl b/src/Tensors.jl
    index 4cd677b..5f04312 100644
    --- a/src/Tensors.jl
    +++ b/src/Tensors.jl
    @@ -63,6 +63,7 @@ julia> Tensor{1,3,Float64}((1.0, 2.0, 3.0))
     """
     struct Tensor{order, dim, T, M} <: AbstractTensor{order, dim, T}
         data::NTuple{M, T}
    +    x::Int
         Tensor{order, dim, T, M}(data::NTuple) where {order, dim, T, M} = new{order, dim, T, M}(data)
     end
  • Press 1 in the REPL
  • Press 1 in the REPL again

with the following result:

julia> using Revise, Tensors

# updates Tensors file here

julia> 1
ERROR: UndefVarError: `Broadcasted` not defined in `StaticArrays.StableFlatten`
Suggestion: check for spelling errors or missing imports.
Stacktrace:
 [1] top-level scope
   @ ~/.julia/packages/StaticArrays/LSPcF/src/broadcast.jl:180
Revise evaluation error at /Users/kristoffercarlsson/.julia/packages/StaticArrays/LSPcF/src/broadcast.jl:180

Stacktrace:
  [1] methods_by_execution!(recurse::Any, methodinfo::Revise.CodeTrackingMethodInfo, docexprs::Dict{…}, mod::Module, ex::Expr; mode::Symbol, disablebp::Bool, always_rethrow::Bool, kwargs::@Kwargs{})
    @ Revise ~/JuliaPkgs/Revise.jl/src/lowered.jl:306
  [2] eval_with_signatures(mod::Module, ex::Expr; mode::Symbol, kwargs::@Kwargs{})
    @ Revise ~/JuliaPkgs/Revise.jl/src/packagedef.jl:556
  [3] eval_with_signatures
    @ ~/JuliaPkgs/Revise.jl/src/packagedef.jl:553 [inlined]
  [4] instantiate_sigs!(modexsigs::OrderedCollections.OrderedDict{…}; mode::Symbol, kwargs::@Kwargs{})
    @ Revise ~/JuliaPkgs/Revise.jl/src/packagedef.jl:564
  [5] instantiate_sigs!
    @ ~/JuliaPkgs/Revise.jl/src/packagedef.jl:560 [inlined]
  [6] maybe_extract_sigs_for_meths(meths::Set{Method})
    @ Revise ~/JuliaPkgs/Revise.jl/src/pkgs.jl:162
  [7] (::Revise.var"#107#108"{Bool})()
    @ Revise ~/JuliaPkgs/Revise.jl/src/packagedef.jl:941
  [8] lock(f::Revise.var"#107#108"{Bool}, l::ReentrantLock)
    @ Base ./lock.jl:335
  [9] #revise#104
    @ ~/JuliaPkgs/Revise.jl/src/packagedef.jl:844 [inlined]
 [10] revise()
    @ Revise ~/JuliaPkgs/Revise.jl/src/packagedef.jl:842
 [11] top-level scope
    @ REPL:1

caused by: UndefVarError: `Broadcasted` not defined in `StaticArrays.StableFlatten`
Suggestion: check for spelling errors or missing imports.
Stacktrace:
  [1] lookup_var
    @ ~/.julia/packages/JuliaInterpreter/J7J9G/src/interpret.jl:5 [inlined]
  [2] step_expr!(recurse::Any, frame::JuliaInterpreter.Frame, node::Any, istoplevel::Bool)
    @ JuliaInterpreter ~/.julia/packages/JuliaInterpreter/J7J9G/src/interpret.jl:44
  [3] signature(recurse::Any, frame::JuliaInterpreter.Frame, stmt::Any, pc::Int64)
    @ LoweredCodeUtils ~/.julia/packages/LoweredCodeUtils/BIVzf/src/signatures.jl:50
  [4] methoddef!(recurse::Any, signatures::Vector{Any}, frame::JuliaInterpreter.Frame, stmt::Any, pc::Int64; define::Bool)
    @ LoweredCodeUtils ~/.julia/packages/LoweredCodeUtils/BIVzf/src/signatures.jl:595
  [5] methoddef!
    @ ~/.julia/packages/LoweredCodeUtils/BIVzf/src/signatures.jl:534 [inlined]
  [6] methods_by_execution!(recurse::Any, methodinfo::Revise.CodeTrackingMethodInfo, docexprs::Dict{…}, frame::JuliaInterpreter.Frame, isrequired::Vector{…}; mode::Symbol, skip_include::Bool)
    @ Revise ~/JuliaPkgs/Revise.jl/src/lowered.jl:354
  [7] methods_by_execution!
    @ ~/JuliaPkgs/Revise.jl/src/lowered.jl:317 [inlined]
  [8] methods_by_execution!(recurse::Any, methodinfo::Revise.CodeTrackingMethodInfo, docexprs::Dict{…}, mod::Module, ex::Expr; mode::Symbol, disablebp::Bool, always_rethrow::Bool, kwargs::@Kwargs{})
    @ Revise ~/JuliaPkgs/Revise.jl/src/lowered.jl:296
  [9] eval_with_signatures(mod::Module, ex::Expr; mode::Symbol, kwargs::@Kwargs{})
    @ Revise ~/JuliaPkgs/Revise.jl/src/packagedef.jl:556
 [10] eval_with_signatures
    @ ~/JuliaPkgs/Revise.jl/src/packagedef.jl:553 [inlined]
 [11] instantiate_sigs!(modexsigs::OrderedCollections.OrderedDict{…}; mode::Symbol, kwargs::@Kwargs{})
    @ Revise ~/JuliaPkgs/Revise.jl/src/packagedef.jl:564
 [12] instantiate_sigs!
    @ ~/JuliaPkgs/Revise.jl/src/packagedef.jl:560 [inlined]
 [13] maybe_extract_sigs_for_meths(meths::Set{Method})
    @ Revise ~/JuliaPkgs/Revise.jl/src/pkgs.jl:162
 [14] (::Revise.var"#107#108"{Bool})()
    @ Revise ~/JuliaPkgs/Revise.jl/src/packagedef.jl:941
 [15] lock(f::Revise.var"#107#108"{Bool}, l::ReentrantLock)
    @ Base ./lock.jl:335
 [16] #revise#104
    @ ~/JuliaPkgs/Revise.jl/src/packagedef.jl:844 [inlined]
 [17] revise()
    @ Revise ~/JuliaPkgs/Revise.jl/src/packagedef.jl:842
 [18] top-level scope
    @ REPL:1
Some type information was truncated. Use `show(err)` to see complete types.

julia> 1
Compiling the compiler. This may take several minutes ...
Base.Compiler ──── 3.43502 seconds
1

The UndefVarError seems correct:

julia> Tensors.StaticArrays.StableFlatten.Broadcasted
ERROR: UndefVarError: `Broadcasted` not defined in `StaticArrays.StableFlatten`
Suggestion: check for spelling errors or missing imports.
Stacktrace:
 [1] getproperty(x::Module, f::Symbol)
   @ Base ./Base_compiler.jl:48
 [2] top-level scope
   @ REPL[4]:1

so I guess the question is why it is tried to be looked up. And also, why does the Compiler recompile when I press another time 1 with no new modifications?

timholy added a commit that referenced this pull request Apr 19, 2025
Fixes #903

This is split out from #894.
timholy added a commit that referenced this pull request Apr 19, 2025
@aviatesk
Copy link
Collaborator

I updated against the latest master branch.

I tested this branch with the simple case, but it seems that for exported bindings, additional edge tracking is needed?
In particular, the world ages of bindings that are usinged need to be updated, it seems.
For example, let's say we have an Example package like this:

module Example
export hello, Hello

struct Hello
    who::String
end

hello(x::Hello) = hello(x.who)

"""
    hello(who::String)

Return "Hello, `who`".
"""
hello(who::String) = "Hello, $who"

end

We get the following error:

julia> using Revise

julia> using Example

julia> hello(Hello("world"))
"Hello, world"

julia> # Apply the following diff
       # ```diff
       # diff --git a/src/Example.jl b/src/Example.jl
       # index 65c7eae..c631814 100644
       # --- a/src/Example.jl
       # +++ b/src/Example.jl
       # @@ -2,10 +2,10 @@ module Example
       #  export hello, Hello
       #  
       #  struct Hello
       # -    who::String
       # +    who2::String
       #  end
       #  
       # -hello(x::Hello) = hello(x.who)
       # +hello(x::Hello) = hello(x.who2 * " (changed)")
       #  
       #  """
       #      hello(who::String)
       # ```

julia> hello(Hello("world"))
ERROR: MethodError: no method matching @world(Example.Hello, 38355:38383)(::String)
The type `@world(Example.Hello, 38355:38383)` exists, but no method is defined for this combination of argument types when trying to construct it.
Stacktrace:
 [1] top-level scope
   @ REPL[5]:1

julia> hello(Example.Hello("world")) # but this works
"Hello, world (changed)"

@KristofferC
Copy link
Collaborator

You can also build from the latest backports-1.12 branch if you need v1.12.

Or just juliaup add the PR (prXYZ).

@timholy
Copy link
Owner Author

timholy commented Aug 13, 2025

@lassepe, this behavior observed at the end of your demo is quite surprising:

julia> fieldnames(Bar)
()

julia> fieldnames(Foo.Bar)
(:x,)

julia> Foo.Bar(1)
Main.Foo.Bar(1)

@Keno, is this a Julia bug failing to propagate the updated binding to Main?

@timholy
Copy link
Owner Author

timholy commented Aug 14, 2025

JuliaLang/julia#59272

@timholy
Copy link
Owner Author

timholy commented Sep 9, 2025

@lassepe your issue is fixed in julia +1.12-rc2 (just released)

@xlxs4
Copy link

xlxs4 commented Sep 9, 2025

┌ Revise
│  ┌ Warning: precompile directive
│  │     precompile(Tuple{mbody.sig.parameters[1], Symbol, Bool, Bool, Iterators.Pairs{Symbol, Bool, Tuple{Symbol}, NamedTuple{(:skip_include,), Tuple{Bool}}}, typeof(methods_by_execution!), Compiled, MI, Module, Expr})
│  │ failed. Please report an issue in Revise (after checking for duplicates) or remove this directive.
│  └ @ Revise ~/.julia/packages/Revise/FV0Wc/src/precompile.jl:65
└
REPL output
julia> Pkg.add(name="Revise"; rev="teh/struct_revision")
    Updating git-repo `https://github.com/timholy/Revise.jl.git`
   Resolving package versions...
    Updating `~/.julia/environments/v1.12/Project.toml`
  [295af30f] + Revise v3.9.0 `https://github.com/timholy/Revise.jl.git#teh/struct_revision`
    Updating `~/.julia/environments/v1.12/Manifest.toml`
  [da1fd8a2] + CodeTracking v2.0.0
  [807dbc54] + Compiler v0.1.1
  [aa1ae85d] + JuliaInterpreter v0.10.5
  [6f1432cf] + LoweredCodeUtils v3.4.3
  [bac558e1] + OrderedCollections v1.8.1
  [ae029012] + Requires v1.3.1
  [295af30f] + Revise v3.9.0 `https://github.com/timholy/Revise.jl.git#teh/struct_revision`
  [56f22d72] + Artifacts v1.11.0
  [2a0f44e3] + Base64 v1.11.0
  [7b1f6079] + FileWatching v1.11.0
  [b77e0a4c] + InteractiveUtils v1.11.0
  [ac6e5ff7] + JuliaSyntaxHighlighting v1.12.0
  [76f85450] + LibGit2 v1.11.0
  [8f399da3] + Libdl v1.11.0
  [d6f4376e] + Markdown v1.11.0
  [ca575930] + NetworkOptions v1.3.0
  [de0858da] + Printf v1.11.0
  [3fa0cd96] + REPL v1.11.0
  [9a3f8284] + Random v1.11.0
  [ea8e919c] + SHA v0.7.0
  [6462fe0b] + Sockets v1.11.0
  [f489334b] + StyledStrings v1.11.0
  [cf7118a7] + UUIDs v1.11.0
  [4ec0a83e] + Unicode v1.11.0
  [e37daf67] + LibGit2_jll v1.9.0+0
  [29816b5a] + LibSSH2_jll v1.11.3+1
  [458c3c95] + OpenSSL_jll v3.5.1+0
Precompiling packages finished.
  1 dependency successfully precompiled in 12 seconds. 20 already precompiled.
  1 dependency had output during precompilation:
┌ Revise
│  ┌ Warning: precompile directive
│  │     precompile(Tuple{mbody.sig.parameters[1], Symbol, Bool, Bool, Iterators.Pairs{Symbol, Bool, Tuple{Symbol}, NamedTuple{(:skip_include,), Tuple{Bool}}}, typeof(methods_by_execution!), Compiled, MI, Module, Expr})
│  │ failed. Please report an issue in Revise (after checking for duplicates) or remove this directive.
│  └ @ Revise ~/.julia/packages/Revise/FV0Wc/src/precompile.jl:65
└
versioninfo
julia> versioninfo()
Julia Version 1.12.0-rc2
Commit 72cbf019d04 (2025-09-06 12:00 UTC)
Build Info:
  Official https://julialang.org release
Platform Info:
  OS: Linux (x86_64-linux-gnu)
  CPU: 16 × AMD Ryzen 7 5700U with Radeon Graphics
  WORD_SIZE: 64
  LLVM: libLLVM-18.1.7 (ORCJIT, znver2)
  GC: Built with stock GC
Threads: 1 default, 1 interactive, 1 GC (on 16 virtual cores)
Pkg status
(@v1.12) pkg> st
Status `~/.julia/environments/v1.12/Project.toml`
  [295af30f] Revise v3.9.0 `https://github.com/timholy/Revise.jl.git#teh/struct_revision`

(@v1.12) pkg> st -m
Status `~/.julia/environments/v1.12/Manifest.toml`
  [da1fd8a2] CodeTracking v2.0.0
  [807dbc54] Compiler v0.1.1
  [aa1ae85d] JuliaInterpreter v0.10.5
  [6f1432cf] LoweredCodeUtils v3.4.3
  [bac558e1] OrderedCollections v1.8.1
  [ae029012] Requires v1.3.1
  [295af30f] Revise v3.9.0 `https://github.com/timholy/Revise.jl.git#teh/struct_revision`
  [56f22d72] Artifacts v1.11.0
  [2a0f44e3] Base64 v1.11.0
  [7b1f6079] FileWatching v1.11.0
  [b77e0a4c] InteractiveUtils v1.11.0
  [ac6e5ff7] JuliaSyntaxHighlighting v1.12.0
  [76f85450] LibGit2 v1.11.0
  [8f399da3] Libdl v1.11.0
  [d6f4376e] Markdown v1.11.0
  [ca575930] NetworkOptions v1.3.0
  [de0858da] Printf v1.11.0
  [3fa0cd96] REPL v1.11.0
  [9a3f8284] Random v1.11.0
  [ea8e919c] SHA v0.7.0
  [6462fe0b] Sockets v1.11.0
  [f489334b] StyledStrings v1.11.0
  [cf7118a7] UUIDs v1.11.0
  [4ec0a83e] Unicode v1.11.0
  [e37daf67] LibGit2_jll v1.9.0+0
  [29816b5a] LibSSH2_jll v1.11.3+1
  [458c3c95] OpenSSL_jll v3.5.1+0

@timholy
Copy link
Owner Author

timholy commented Sep 9, 2025

Thanks for the report. I need to fix that, but FYI it's nothing to worry about in terms of Revise working properly.

@jakobnissen

This comment was marked as resolved.

@lassepe
Copy link

lassepe commented Sep 9, 2025

@jakobnissen, isn't the problem simply that both your module and the struct are called Foo? I think that never worked.

@lassepe
Copy link

lassepe commented Sep 9, 2025

A modified version of my original setting above still seems to be broken (in a different way) on julia 1.12-rc2 and the lateset version of this branch

Steps to reproduce

  1. Generate a package: ]generate StructRevision
  2. Add the following content to src/StructRevision
module StructRevision

struct Foo
  x::Int
end

bar(::Foo) = 1

end # module StructRevision
  1. With julia 1.12-rc2 from the REPL:
❯ julia +1.12 --startup-file=no --project
               _
   _       _ _(_)_     |  Documentation: https://docs.julialang.org
  (_)     | (_) (_)    |
   _ _   _| |_  __ _   |  Type "?" for help, "]?" for Pkg help.
  | | | | | | |/ _` |  |
  | | |_| | | | (_| |  |  Version 1.12.0-rc2 (2025-09-06)
 _/ |\__'_|_|_|\__'_|  |  Official https://julialang.org release
|__/                   |

julia> using Revise, StructRevision

julia> foo1 = StructRevision.Foo(1)
StructRevision.Foo(1)

julia> StructRevision.bar(foo1)
1

julia> # after changing Foo.x from Int to Float64:

julia> foo2 = StructRevision.Foo(1)
StructRevision.Foo(1.0)

julia> StructRevision.bar(foo1)
1

julia> StructRevision.bar(foo2)
ERROR: MethodError: no method matching bar(::StructRevision.Foo)
The function `bar` exists, but no method is defined for this combination of argument types.

Closest candidates are:
  bar(::@world(StructRevision.Foo, 38519:38547))
   @ StructRevision ~/worktree/BugReports/StructRevision.jl/src/StructRevision.jl:7

Stacktrace:
 [1] top-level scope
   @ REPL[7]:1

@timholy
Copy link
Owner Author

timholy commented Sep 9, 2025

Hmm, there are working tests that are awfully close to your example, e.g.,

Revise.jl/test/runtests.jl

Lines 2453 to 2458 in 0158326

struct Point
x::Float64
end
# Two methods that won't need to be explicitly redefined (but will need re-evaluation for new type)
firstval(p::Point) = p.x
firstvalP(p::P) where P<:Point = p.x

I'll have to check and see what the difference may be.

@jakobnissen

This comment was marked as resolved.

@timholy
Copy link
Owner Author

timholy commented Sep 11, 2025

@lassepe fixed in 962c7cb
@jakobnissen fixed in 1583289

@timholy
Copy link
Owner Author

timholy commented Sep 11, 2025

@xlxs4 fixed in 9e3fd08

@timholy
Copy link
Owner Author

timholy commented Sep 11, 2025

PSA: anyone who is using this via Pkg.add(name="Revise"; rev="teh/struct_revision") will need to re-execute that line to get the latest.

@lassepe
Copy link

lassepe commented Sep 11, 2025

Nice! I stress-tested it a bit more, and all the stuff I tried before now works flawlessly. The only minor oddity I found is that, when a struct is modified from parametric to non-parametric, the old methods don't get deleted and the old instances + dispatches still work. I am not sure if that is considered a bug because everything from the current world age still works as expected (in the sense that it refuses to dispatch on the outdated bar method table)
There seems to be another edge case that occurs when one removes a type parameter and adds it back:

  1. Generate a new package with ]generate StructRevision
  2. Add the following to src/StructRevision.jl
module StructRevision

export Foo, bar

struct Foo{T}
  x::T
end

bar(::Foo{T}) where {T} = "parametric with $T"

end # module StructRevision
  1. Run the following from the REPL with modification instructions in the comments:
worktree/BugReports/StructRevision.jl is 📦 v0.1.0 via ஃ v1.10.10 took 5s
❯ julia +1.12 --project=. --startup-file=no
               _
   _       _ _(_)_     |  Documentation: https://docs.julialang.org
  (_)     | (_) (_)    |
   _ _   _| |_  __ _   |  Type "?" for help, "]?" for Pkg help.
  | | | | | | |/ _` |  |
  | | |_| | | | (_| |  |  Version 1.12.0-rc2 (2025-09-06)
 _/ |\__'_|_|_|\__'_|  |  Official https://julialang.org release
|__/                   |

julia> using Revise, StructRevision

julia> foo = Foo(1)
Foo{Int64}(1)

julia> bar(foo)
"parametric with Int64"

julia> # change Foo to: `struct Foo x::Int end`

julia> foo2 = Foo(1)
Foo(1)

julia> bar(foo)
"parametric with Int64"

julia> bar(foo2)
ERROR: MethodError: no method matching bar(::Foo)
The function `bar` exists, but no method is defined for this combination of argument types.

Closest candidates are:
  bar(::@world(Foo, 38519:38547){T}) where T
   @ StructRevision ~/worktree/BugReports/StructRevision.jl/src/StructRevision.jl:9

Stacktrace:
 [1] top-level scope
   @ REPL[6]:1

julia> # now change Foo back to its original definition

julia> foo = Foo(1)
Foo{Int64}(1)

julia> bar(foo)
ERROR: MethodError: no method matching bar(::Foo{Int64})
The function `bar` exists, but no method is defined for this combination of argument types.

Closest candidates are:
  bar(::@world(Foo, 38519:38547){T}) where T
   @ StructRevision ~/worktree/BugReports/StructRevision.jl/src/StructRevision.jl:9

Stacktrace:
 [1] top-level scope
   @ REPL[8]:1

@jkrumbiegel
Copy link

I tried it on Makie and it immediately failed. With CairoMakie and Makie dev'ed I did using CairoMakie; lines(cumsum(randn(100))) to get a first plot. Then I went to src/makielayout/types.jl and renamed the field reset_timer on ScrollZoom to reset_time just to see what happens. Executing another plot command then printed:

┌ Error: Failed to revise /Users/krumbiegel/.julia/dev/Makie/Makie/src/makielayout/types.jl
│   exception =
│    MethodError: no method matching getfile(::Nothing)
│    The function `getfile` exists, but no method is defined for this combination of argument types.
│    
│    Closest candidates are:getfile(::JuliaInterpreter.Frame)
│       @ JuliaInterpreter ~/.julia/packages/JuliaInterpreter/378J1/src/utils.jl:421getfile(::JuliaInterpreter.FrameCode, ::Any)
│       @ JuliaInterpreter ~/.julia/packages/JuliaInterpreter/378J1/src/utils.jl:416getfile(::JuliaInterpreter.Frame, ::Any)
│       @ JuliaInterpreter ~/.julia/packages/JuliaInterpreter/378J1/src/utils.jl:421...
│    
│    Stacktrace:
│     [1] top-level scope
│       @ ~/.julia/dev/Makie/Makie/src/makielayout/types.jl:190
│    Revise evaluation error at /Users/krumbiegel/.julia/dev/Makie/Makie/src/makielayout/types.jl:190
│    
└ @ Revise ~/.julia/packages/Revise/il844/src/packagedef.jl:783
┌ Warning: The running code does not match the saved version for the following files:
│ 
│   /Users/krumbiegel/.julia/dev/Makie/Makie/src/makielayout/types.jl
│ 
│ If the error was due to evaluation order, it can sometimes be resolved by calling `Revise.retry()`.
│ Use Revise.errors() to report errors again. Only the first error in each file is shown.
│ Your prompt color may be yellow until the errors are resolved.
└ @ Revise ~/.julia/packages/Revise/il844/src/packagedef.jl:957

I then tried to Revise.retry() to see what would happen and that printed something else

julia> Revise.retry()
┌ Error: Failed to revise /Users/krumbiegel/.julia/dev/Makie/Makie/src/makielayout/types.jl
│   exception =
│    UndefVarError: `#1217###mixin#307` not defined in `Makie`
│    Suggestion: check for spelling errors or missing imports.
│    Stacktrace:
│     [1] top-level scope
│       @ ~/.julia/dev/Makie/Makie/src/colorsampler.jl:14
│    Revise evaluation error at /Users/krumbiegel/.julia/dev/Makie/Makie/src/colorsampler.jl:14
│    
│    Stacktrace:
│     [1] top-level scope
│       @ ~/.julia/dev/Makie/Makie/src/makielayout/types.jl:190
│    Revise evaluation error at /Users/krumbiegel/.julia/dev/Makie/Makie/src/makielayout/types.jl:190
│    
└ @ Revise ~/.julia/packages/Revise/il844/src/packagedef.jl:783

@lassepe
Copy link

lassepe commented Sep 11, 2025

Ah, here is a bug: if in my example above you go "full circle" by adding back the parameter T, new instances won't be accepted by bar. I updated my comment above accordingly.

@timholy
Copy link
Owner Author

timholy commented Sep 11, 2025

@jkrumbiegel, thanks for the example. I might need your help, because Makie's recipe system makes it a bit hard for me to look up how types are defined. I created a gist (because it exceeded GitHub's message limit) with a log consisting of:

  • modules and expressions it's "scanning" to extract method signatures
  • types that it thinks have changed their definition, e.g., "### REVISED TYPE ###\n\nAxis"
  • methods that take that type in their signature constraints (and which therefore need to be re-evaluated), e.g., "#### HERE IS A METHOD FOR THAT TYPE #####\nm = kwcall(::NamedTuple, ::typeof(hidedecorations!), la::Axis) @ Makie ~/.julia/environments/v1.12/dev/Makie/Makie/src/makielayout/blocks/axis.jl:1156

So it seems to start trying to start redefining Axis (does that wrap ScollZoom?) and methods that have ::Axis in their signatures. Does that make sense? The type that ends up causing it to error is Plot{PlotFunc, T} and the method get(x::Plot, key::Symbol, default) @ Makie ~/.julia/environments/v1.12/dev/Makie/Makie/src/compute-plots.jl:29. It chokes when it gets to @recipe Image and it may have something to do with inconsistent gensyms from your whole DocumentedAttributes/mixin system? Revise has to re-evaluate code and if that means gensyms are created, they probably/certainly won't be created with the same numbers as the first time.

Let me know if these somehow make sense or if Revise is just going off the rails somehow.

@jkrumbiegel
Copy link

jkrumbiegel commented Sep 11, 2025

So it seems to start trying to start redefining Axis (does that wrap ScollZoom?) and methods that have ::Axis in their signatures. Does that make sense?

Hm I'm not sure if it makes sense, the type Axis shouldn't change if ScrollZoom changes. ScrollZoom is used when instantiating an Axis via initialize_block! and register_events! in there but that should only cause recompilation of those functions, no? Not everything that uses Axis in its arguments, as Axis itself should be the same.

Maybe Makie's system where structs are defined via complicated macros just ties too many things together so that one small change ripples outward further than it should. The thing with the mixin kind of makes me think that, as it was definitely not designed with revise-ability in mind and is maybe a bit "weird" in places. Like this macro-within-macro thing here https://github.com/MakieOrg/Makie.jl/blob/46350344714203b165c696947e37051708bf332a/Makie/src/recipes.jl#L506-L508 in which there is indeed a gensym with mixin happening here https://github.com/MakieOrg/Makie.jl/blob/46350344714203b165c696947e37051708bf332a/Makie/src/recipes.jl#L319

@timholy
Copy link
Owner Author

timholy commented Sep 11, 2025

Thanks for the perspective, that's extraordinarily helpful. Merely knowing that I should start at the "head" of this log (why did it redefine Axis?) rather than the "tail" (what to do about mixin?) might save me a couple hours.

Re the gensyms and revisability, do keep in mind that the only reason Revise needs to go to all these shenanigans is because we don't yet have JuliaLowering: to track changes in line numbers and thus "repair" the line numbers in stacktraces, Revise has to map source text ↔ signatures in a manner more dynamic than plain line numbers (in case you insert code earlier in the file). Currently there isn't a way to do that besides reparse/lower/almost-evaluate all the source files, but JuliaLowering should provide a much better way. Since hopefully 🤞 JuliaLowering will land sometime in the Julia 1.13 or 1.14 timeframe, one option is to just wait it out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants