Skip to content
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

deepcopy with non-trivial circular references fails in 1.11.2 #56775

Closed
DrChainsaw opened this issue Dec 7, 2024 · 4 comments · Fixed by #56990
Closed

deepcopy with non-trivial circular references fails in 1.11.2 #56775

DrChainsaw opened this issue Dec 7, 2024 · 4 comments · Fixed by #56990
Labels
bug Indicates an unexpected problem or unintended behavior regression Regression in behavior compared to a previous version regression 1.11 Regression in the 1.11 release regression 1.12 Regression in the 1.12 release

Comments

@DrChainsaw
Copy link

Not sure if this is just a symptom of deepcopy not having much in terms of guaranteed behaviour, but it is no longer consistent with serialize => deserialize.

I tried to recreate it with a simpler structure (e.g. just Arrays which contain themselves) but couldn't. This is the most stripped down version I could create:

struct ProblemStruct{T}
    label::String # Just helps a bit when debugging
    inputs::T
end

using Serialization

let 
    problem = ProblemStruct("problem", ProblemStruct[])

    next = ProblemStruct("next", [problem])
    push!(problem.inputs, next) # Make a circular reference

    println("Original:")
    @show problem === next.inputs[1]
    @show problem.inputs[1] === next

    problemc, nextc = deepcopy((problem, next))
    println("\ndeepcopy:")
    @show problemc === nextc.inputs[1]
    @show problemc.inputs[1] === nextc

    println("\nserde:")
    problemds, nextds = let 
        io = PipeBuffer()
        serialize(io, (problem, next))
        deserialize(io)
    end
    @show problemds === nextds.inputs[1]
    @show problemds.inputs[1] === nextds

end;

Prints the following in Julia 1.11.2:

Original:
problem === next.inputs[1] = true    
problem.inputs[1] === next = true    

deepcopy:
problemc === nextc.inputs[1] = false 
problemc.inputs[1] === nextc = true  

serde:
problemds === nextds.inputs[1] = true
problemds.inputs[1] === nextds = true

And this in Julia 1.10.7:

Original:
problem === next.inputs[1] = true
problem.inputs[1] === next = true

deepcopy:
problemc === nextc.inputs[1] = true
problemc.inputs[1] === nextc = true

serde:
problemds === nextds.inputs[1] = true
problemds.inputs[1] === nextds = true

Fwiw: I can work around the problem on my side with the following:

function Base.deepcopy_internal(v::Vector{ProblemStruct}, stackdict::IdDict)
    v in keys(stackdict) && return stackdict[v]
    newv = similar(v)
    stackdict[v] = newv
    for (i, vi) in zip(eachindex(newv), v)
        newv[i] = Base.deepcopy_internal(vi, stackdict)
    end
    return newv
end

Versioninfo 1.11.2:

Julia Version 1.11.2
Commit 5e9a32e7af (2024-12-01 20:02 UTC)
Build Info:
  Official https://julialang.org/ release
Platform Info:
  OS: Windows (x86_64-w64-mingw32)
  CPU: 12 × Intel(R) Core(TM) i7-5820K CPU @ 3.30GHz
  WORD_SIZE: 64
  LLVM: libLLVM-16.0.6 (ORCJIT, haswell)
Threads: 12 default, 0 interactive, 6 GC (on 12 virtual cores)
Environment:
  JULIA_DEPOT_PATH = E:/Programs/julia/.julia
  JULIA_PKG_DEVDIR = E:/Programs/julia/.julia/dev
  JULIA_EDITOR = code

Versioninfo 1.10.7:

Julia Version 1.10.7
Commit 4976d05258 (2024-11-26 15:57 UTC)
Build Info:
  Official https://julialang.org/ release
Platform Info:
  OS: Windows (x86_64-w64-mingw32)
  CPU: 12 × Intel(R) Core(TM) i7-5820K CPU @ 3.30GHz
  WORD_SIZE: 64
  LIBM: libopenlibm
  LLVM: libLLVM-15.0.7 (ORCJIT, haswell)
Threads: 1 default, 0 interactive, 1 GC (on 12 virtual cores)
Environment:
  JULIA_DEPOT_PATH = E:/Programs/julia/.julia
  JULIA_PKG_DEVDIR = E:/Programs/julia/.julia/dev
@nsajko
Copy link
Contributor

nsajko commented Dec 7, 2024

Not sure if related: #26020

@willtebbutt
Copy link
Contributor

To offer another example, this caused me some trouble when handling arrays containing circular references:

On 1.10.7 I see

julia> function make_circular_reference_array()
           p = Any[nothing]
           p[1] = p
           return p
       end
make_circular_reference_array (generic function with 1 method)

julia> p = make_circular_reference_array()
1-element Vector{Any}:
 1-element Vector{Any}:#= circular reference @-1 =#

julia> p2 = deepcopy(p)
1-element Vector{Any}:
1-element Vector{Any}:#= circular reference @-1 =#

julia> p === p[1]
true

julia> p2 === p2[1]
true

which is what I had expected. On 1.11.2 I see

julia> function make_circular_reference_array()
           p = Any[nothing]
           p[1] = p
           return p
       end
make_circular_reference_array (generic function with 1 method)

julia> p = make_circular_reference_array()
1-element Vector{Any}:
 1-element Vector{Any}:#= circular reference @-1 =#

julia> p2 = deepcopy(p)
1-element Vector{Any}:
 Any[Any[#= circular reference @-1 =#]]

julia> p === p[1]
true

julia> p2 === p2[1]
false

@bbrehm
Copy link

bbrehm commented Dec 20, 2024

Good reproducer @willtebbutt !
The issue is in

stackdict[x] = $(Expr(:new, :(Array{T, N}), :(deepcopy_internal(x.ref, stackdict)), :(x.size)))
from commit 909bcea

The deepcopy_internal construction should register the new copied item in the stackdict before recursing to the children. Failing that, we need to check again when ascending from the stack. So the fix is

julia> @eval Base begin
       function deepcopy_internal(x::Array{T, N}, stackdict::IdDict) where {T, N}
           if haskey(stackdict, x)
               return stackdict[x]::typeof(x)
           end
           copied = $(Expr(:new, :(Array{T, N}), :(deepcopy_internal(x.ref, stackdict)), :(x.size)))
           #we need to check again, in case that we just recursed back to x
           get!(stackdict, x, copied)::typeof(x)
           end
           end

@nsajko nsajko added bug Indicates an unexpected problem or unintended behavior regression Regression in behavior compared to a previous version regression 1.11 Regression in the 1.11 release labels Dec 26, 2024
@willtebbutt
Copy link
Contributor

Thanks for figuring this out @bbrehm -- are you interested in turning this fix into a PR?

@nsajko nsajko added the regression 1.12 Regression in the 1.12 release label Jan 6, 2025
KristofferC pushed a commit that referenced this issue Jan 13, 2025
Resolves #56775 . Credit for this fix rests entirely with bbrehm .

---------

Co-authored-by: Lilith Orion Hafner <lilithhafner@gmail.com>
Co-authored-by: Neven Sajko <4944410+nsajko@users.noreply.github.com>
(cherry picked from commit 1ebacac)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Indicates an unexpected problem or unintended behavior regression Regression in behavior compared to a previous version regression 1.11 Regression in the 1.11 release regression 1.12 Regression in the 1.12 release
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants