Skip to content

Fix struct/const revision #894

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 21 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 18 additions & 1 deletion src/lowered.jl
Original file line number Diff line number Diff line change
Expand Up @@ -458,7 +458,24 @@
pc = step_expr!(recurse, frame, stmt, true)
elseif head === :call
f = @lookup(frame, stmt.args[1])
if isdefined(Core, :_defaultctors) && f === Core._defaultctors && length(stmt.args) == 3
if __bpart__ && f === Core._typebody! && length(stmt.args) >= 3
# Handle type redefinition
newtype = Base.unwrap_unionall(@lookup(frame, stmt.args[3]))
newtypename = newtype.name
oldtype = isdefinedglobal(newtypename.module, newtypename.name) ? getglobal(newtypename.module, newtypename.name) : nothing
if oldtype !== nothing
if !Core._equiv_typedef(oldtype, newtype)

Check warning on line 467 in src/lowered.jl

View check run for this annotation

Codecov / codecov/patch

src/lowered.jl#L463-L467

Added lines #L463 - L467 were not covered by tests
# Find all methods that use `oldtype`
meths = methods_with(oldtype)

Check warning on line 469 in src/lowered.jl

View check run for this annotation

Codecov / codecov/patch

src/lowered.jl#L469

Added line #L469 was not covered by tests
# For any modules that have not yet been parsed and had their signatures extracted,
# we need to do this now, before the binding changes to the new type
maybe_extract_sigs_for_meths(meths)
union!(reeval_methods, meths)

Check warning on line 473 in src/lowered.jl

View check run for this annotation

Codecov / codecov/patch

src/lowered.jl#L472-L473

Added lines #L472 - L473 were not covered by tests
end
end
pc = step_expr!(recurse, frame, stmt, true)

Check warning on line 476 in src/lowered.jl

View check run for this annotation

Codecov / codecov/patch

src/lowered.jl#L476

Added line #L476 was not covered by tests
elseif isdefined(Core, :_defaultctors) && f === Core._defaultctors && length(stmt.args) == 3
# Create the constructors for a type (i.e., a method definition)
T = @lookup(frame, stmt.args[2])
lnn = @lookup(frame, stmt.args[3])
if T isa Type && lnn isa LineNumberNode
Expand Down
39 changes: 38 additions & 1 deletion src/packagedef.jl
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,7 @@
include("utils.jl")
include("parsing.jl")
include("lowered.jl")
include("visit.jl")
include("pkgs.jl")
include("git.jl")
include("recipes.jl")
Expand Down Expand Up @@ -136,6 +137,12 @@
"""
const queue_errors = Dict{Tuple{PkgData,String},Tuple{Exception, Any}}()

# Can we revise types?
const __bpart__ = Base.VERSION >= v"1.12.0-DEV.2047"
# Julia 1.12+: when bindings switch to a new type, we need to re-evaluate method
# definitions using the new binding resolution.
const reeval_methods = Set{Method}()

"""
Revise.NOPACKAGE

Expand Down Expand Up @@ -748,7 +755,7 @@
error(file, " is not currently being tracked.")
end
mexsnew, mexsold = handle_deletions(pkgdata, file)
if mexsnew != nothing
if mexsnew !== nothing
_, includes = eval_new!(mexsnew, mexsold)
fi = fileinfo(pkgdata, i)
pkgdata.fileinfos[i] = FileInfo(mexsnew, fi)
Expand Down Expand Up @@ -805,6 +812,7 @@
sleep(0.01) # in case the file system isn't quite done writing out the new files
lock(revise_lock) do
have_queue_errors = !isempty(queue_errors)
empty!(reeval_methods)

# Do all the deletion first. This ensures that a method that moved from one file to another
# won't get redefined first and deleted second.
Expand Down Expand Up @@ -874,6 +882,33 @@
queue_errors[(pkgdata, file)] = (err, catch_backtrace())
end
end
# Handle binding invalidations
if !isempty(reeval_methods)
handled = typeof(reeval_methods)()
while !isempty(reeval_methods)
m = pop!(reeval_methods)
methinfo = get(CodeTracking.method_info, m.sig, missing)
methinfo === missing && continue
if length(methinfo) != 1
@warn "Multiple definitions for $(m.sig) found, skipping reevaluation"
continue

Check warning on line 894 in src/packagedef.jl

View check run for this annotation

Codecov / codecov/patch

src/packagedef.jl#L887-L894

Added lines #L887 - L894 were not covered by tests
end
Base.delete_method(m) # ensure that "old data" doesn't get run with "old methods"
_, ex = methinfo[1]
eval_with_signatures(m.module, ex; mode=:eval)
push!(handled, m)
if isdefinedglobal(m.module, m.name)
f = getglobal(m.module, m.name)
if isa(f, DataType)
newmeths = setdiff(methods_with(f), handled)
maybe_extract_sigs_for_meths(newmeths)
union!(reeval_methods, newmeths)

Check warning on line 905 in src/packagedef.jl

View check run for this annotation

Codecov / codecov/patch

src/packagedef.jl#L896-L905

Added lines #L896 - L905 were not covered by tests
end
else
push!(trouble, m)

Check warning on line 908 in src/packagedef.jl

View check run for this annotation

Codecov / codecov/patch

src/packagedef.jl#L908

Added line #L908 was not covered by tests
end
end

Check warning on line 910 in src/packagedef.jl

View check run for this annotation

Codecov / codecov/patch

src/packagedef.jl#L910

Added line #L910 was not covered by tests
end
if interrupt
for pkgfile in finished
haskey(queue_errors, pkgfile) || delete!(revision_queue, pkgfile)
Expand Down Expand Up @@ -908,6 +943,8 @@
end
revise(backend::REPL.REPLBackend) = revise()

trouble = []

"""
revise(mod::Module; force::Bool=true)

Expand Down
17 changes: 17 additions & 0 deletions src/pkgs.jl
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,23 @@
end
maybe_extract_sigs!(pkgdata::PkgData, file::AbstractString) = maybe_extract_sigs!(fileinfo(pkgdata, file))

function maybe_extract_sigs_for_meths(meths)
for m in meths
methinfo = get(CodeTracking.method_info, m.sig, false)
if methinfo === false
pkgdata = get(pkgdatas, PkgId(m.module), nothing)
pkgdata === nothing && continue
for file in srcfiles(pkgdata)
fi = fileinfo(pkgdata, file)
if (isempty(fi.modexsigs) && !fi.parsed[]) && (!isempty(fi.cachefile) || !isempty(fi.cacheexprs))
fi = maybe_parse_from_cache!(pkgdata, file)
instantiate_sigs!(fi.modexsigs)

Check warning on line 162 in src/pkgs.jl

View check run for this annotation

Codecov / codecov/patch

src/pkgs.jl#L152-L162

Added lines #L152 - L162 were not covered by tests
end
end

Check warning on line 164 in src/pkgs.jl

View check run for this annotation

Codecov / codecov/patch

src/pkgs.jl#L164

Added line #L164 was not covered by tests
end
end

Check warning on line 166 in src/pkgs.jl

View check run for this annotation

Codecov / codecov/patch

src/pkgs.jl#L166

Added line #L166 was not covered by tests
end

function maybe_add_includes_to_pkgdata!(pkgdata::PkgData, file::AbstractString, includes; eval_now::Bool=false)
for (mod, inc) in includes
inc = joinpath(splitdir(file)[1], inc)
Expand Down
50 changes: 50 additions & 0 deletions src/visit.jl
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
function methods_with(@nospecialize(T::Type), world::UInt = Base.get_world_counter())
meths = Method[]
visited = Set{Module}()
for mod in Base.loaded_modules_array()
methods_with!(meths, T, world, mod, visited)
end

Check warning on line 6 in src/visit.jl

View check run for this annotation

Codecov / codecov/patch

src/visit.jl#L1-L6

Added lines #L1 - L6 were not covered by tests
# Also handle Type
mt = methods(Type).mt
Tname = T.name
for method in Base.MethodList(mt)
method.module === Tname.module && method.name === Tname.name && continue # skip constructor
hastype(method.sig, T) && push!(meths, method)
end
return meths

Check warning on line 14 in src/visit.jl

View check run for this annotation

Codecov / codecov/patch

src/visit.jl#L8-L14

Added lines #L8 - L14 were not covered by tests
end

function methods_with!(meths, @nospecialize(T::Type), world, mod::Module, visited::Set{Module})
mod in visited && return
push!(visited, mod)

Check warning on line 19 in src/visit.jl

View check run for this annotation

Codecov / codecov/patch

src/visit.jl#L17-L19

Added lines #L17 - L19 were not covered by tests
# Traverse submodules
for name in names(mod; all=true, imported=false)
isdefined(mod, name) || continue
obj = getglobal(mod, name)
if isa(obj, Module)
methods_with!(meths, T, world, obj, visited)

Check warning on line 25 in src/visit.jl

View check run for this annotation

Codecov / codecov/patch

src/visit.jl#L21-L25

Added lines #L21 - L25 were not covered by tests
end
end
Base.foreach_module_mtable(mod, world) do mt::Core.MethodTable
for method in Base.MethodList(mt)
hastype(method.sig, T) && push!(meths, method)
end
return true

Check warning on line 32 in src/visit.jl

View check run for this annotation

Codecov / codecov/patch

src/visit.jl#L27-L32

Added lines #L27 - L32 were not covered by tests
end
return meths

Check warning on line 34 in src/visit.jl

View check run for this annotation

Codecov / codecov/patch

src/visit.jl#L34

Added line #L34 was not covered by tests
end

function hastype(@nospecialize(S), @nospecialize(T))
isa(S, Type) || return false
S = Base.unwrap_unionall(S)
isa(S, Core.TypeofBottom) && return false
if isa(S, Union)
return hastype(S.a, T) | hastype(S.b, T)

Check warning on line 42 in src/visit.jl

View check run for this annotation

Codecov / codecov/patch

src/visit.jl#L37-L42

Added lines #L37 - L42 were not covered by tests
end
Base.isvarargtype(S) && return hastype(S.T, T)
S === T && return true
for P in S.parameters
hastype(P, T) && return true
end
return false

Check warning on line 49 in src/visit.jl

View check run for this annotation

Codecov / codecov/patch

src/visit.jl#L44-L49

Added lines #L44 - L49 were not covered by tests
end
132 changes: 132 additions & 0 deletions test/runtests.jl
Original file line number Diff line number Diff line change
Expand Up @@ -2432,6 +2432,138 @@ end
pop!(LOAD_PATH)
end

if Revise.__bpart__ && do_test("struct/const revision") # can we revise types and constants?
@testset "struct/const revision" begin
testdir = newtestdir()
dn = joinpath(testdir, "StructConst", "src")
mkpath(dn)
write(joinpath(dn, "StructConst.jl"), """
module StructConst
const __hash__ = 0x71716e828e2d6093
struct Fixed
x::Int
end
Base.hash(f::Fixed, h::UInt) = hash(__hash__, hash(f.x, h))
struct Point
x::Float64
end
firstval(p::Point) = p.x
mynorm(p::Point) = sqrt(p.x^2)
end
""")
# Also create another package that uses it
dn2 = joinpath(testdir, "StructConstUser", "src")
mkpath(dn2)
write(joinpath(dn2, "StructConstUser.jl"), """
module StructConstUser
using StructConst: StructConst, Point
struct PointWrapper
p::Point
end
scuf(f::StructConst.Fixed) = 33 * f.x
scup(p::Point) = 44 * p.x
scup(pw::PointWrapper) = 55 * pw.p.x
end
""")
# ...and one that uses that. This is to check whether the propagation of
# signature extraction works correctly.
dn3 = joinpath(testdir, "StructConstUserUser", "src")
mkpath(dn3)
write(joinpath(dn3, "StructConstUserUser.jl"), """
module StructConstUserUser
using StructConstUser
struct PointWrapperWrapper
pw::StructConstUser.PointWrapper
end
StructConstUser.scup(pw::PointWrapperWrapper) = 2 * StructConstUser.scup(pw.pw)
end
""")
sleep(mtimedelay)
@eval using StructConst
@eval using StructConstUser
@eval using StructConstUserUser
sleep(mtimedelay)
w1 = Base.get_world_counter()
f = StructConst.Fixed(5)
v1 = hash(f)
p = StructConst.Point(5.0)
pw = StructConstUser.PointWrapper(p)
pww = StructConstUserUser.PointWrapperWrapper(pw)
@test StructConst.firstval(p) == 5.0
@test StructConst.mynorm(p) == 5.0
@test StructConstUser.scuf(f) == 33 * 5.0
@test StructConstUser.scup(p) == 44 * 5.0
@test StructConstUser.scup(pw) == 55 * 5.0
@test StructConstUser.scup(pww) == 2 * 55 * 5.0
write(joinpath(dn, "StructConst.jl"), """
module StructConst
const __hash__ = 0xddaab158621d200c
struct Fixed
x::Int
end
Base.hash(f::Fixed, h::UInt) = hash(__hash__, hash(f.x, h))
struct Point
x::Float64
y::Float64
end
firstval(p::Point) = p.x
mynorm(p::Point) = sqrt(p.x^2 + p.y^2)
end
""")
@yry()
@test StructConst.__hash__ == 0xddaab158621d200c
v2 = hash(f)
@test v1 != v2
# Call with old objects---ensure we deleted all the outdated methods to reduce user confusion
@test_throws MethodError StructConst.firstval(p)
@test_throws MethodError StructConst.mynorm(p)
@test StructConstUser.scuf(f) == 33 * 5.0
@test_throws MethodError StructConstUser.scup(p)
@test_throws MethodError StructConstUser.scup(pw)
@test_throws MethodError StructConstUser.scup(pww)
# Call with new objects
p2 = StructConst.Point(3.0, 4.0)
pw2 = @eval(StructConstUser.PointWrapper($p2))
pww2 = @eval(StructConstUserUser.PointWrapperWrapper($pw2))
@test @eval(StructConst.firstval($p2)) == 3.0
@test @eval(StructConst.mynorm($p2)) == 5.0
@test @eval(StructConstUser.scup($p2)) == 44 * 3.0
@test @eval(StructConstUser.scup($pw2)) == 55 * 3.0
@test @eval(StructConstUser.scup($pww2)) == 2 * 55 * 3.0
write(joinpath(dn, "StructConst.jl"), """
module StructConst
const __hash__ = 0x71716e828e2d6093
struct Fixed
x::Int
end
Base.hash(f::Fixed, h::UInt) = hash(__hash__, hash(f.x, h))
struct Point{T<:Real} <: AbstractVector{T}
x::T
y::T
end
firstval(p::Point) = p.x
mynorm(p::Point) = sqrt(p.x^2 + p.y^2)
end
""")
@yry()
@test StructConst.__hash__ == 0x71716e828e2d6093
v3 = hash(f)
@test v1 == v3
p3 = StructConst.Point(3.0, 4.0)
pw3 = @eval(StructConstUser.PointWrapper($p3))
pww3 = @eval(StructConstUserUser.PointWrapperWrapper($pw3))
@test @eval(StructConst.mynorm($p3)) == 5.0
@test @eval(StructConstUser.scup($p3)) == 44 * 3.0
@test @eval(StructConstUser.scup($pw3)) == 55 * 3.0
@test @eval(StructConstUser.scup($pww3)) == 2 * 55 * 3.0

rm_precompile("StructConst")
rm_precompile("StructConstUser")
rm_precompile("StructConstUserUser")
pop!(LOAD_PATH)
end
end

do_test("get_def") && @testset "get_def" begin
testdir = newtestdir()
dn = joinpath(testdir, "GetDef", "src")
Expand Down
Loading