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 Fix #56990

Merged
merged 10 commits into from
Jan 9, 2025
Merged

Deepcopy Fix #56990

merged 10 commits into from
Jan 9, 2025

Conversation

willtebbutt
Copy link
Contributor

@willtebbutt willtebbutt commented Jan 8, 2025

Resolves #56775 . Credit for this fix rests entirely with @bbrehm .

I have a regression test -- see #56775 (comment) -- but I'm unsure where to put it. If someone could point me towards an appropriate location for the test, that would be very helpful!

edit: CC @nsajko

base/deepcopy.jl Outdated Show resolved Hide resolved
@LilithHafner
Copy link
Member

LilithHafner commented Jan 8, 2025

Another option which registers the new copied item in the stackdict before recursing to the children is

function deepcopy_internal(x::Array{T, N}, stackdict::IdDict) where {T, N}
    if haskey(stackdict, x)
        return stackdict[x]::typeof(x)
    end
    y = stackdict[x] = Array{T, N}(undef, ntuple(Returns(0), Val{N}()))
    setfield!(y, :ref, deepcopy_internal(x.ref, stackdict))
    setfield!(y, :size, x.size)
    y
end

I'm worried about calling setfield! on an array, though. cc. @oscardssmith for weather that is okay in this context and also because #56775 was introduced by https://github.com/JuliaLang/julia/pull/51319/files#diff-471c7f0385825eae442274e0debe4e88fe50ce6898508130c9c14fbb7c29f255R119-R124

@LilithHafner
Copy link
Member

If someone could point me towards an appropriate location for the test, that would be very helpful!

I'd put tests for this here:

@LilithHafner LilithHafner added bugfix This change fixes an existing bug backport 1.11 Change should be backported to release-1.11 labels Jan 8, 2025
@willtebbutt
Copy link
Contributor Author

Another option which registers the new copied item in the stackdict before recursing to the children is

I'm happy to go with whichever is preferred -- I'll wait for @oscardssmith to weigh in before modifying.

@oscardssmith
Copy link
Member

Calling setfield! on an array is fine (so long as you don't violate the invariant)

@willtebbutt
Copy link
Contributor Author

Cool. I'll change the fix.

@willtebbutt
Copy link
Contributor Author

Fix modified to following @LilithHafner 's suggestion, which I agree is probably cleaner. Test also fixed to actually be a test, rather than just evaluating a Bool.

@vtjnash vtjnash added the merge me PR is reviewed. Merge when all tests are passing label Jan 8, 2025
@vtjnash
Copy link
Member

vtjnash commented Jan 8, 2025

Is this triggering a Revise.jl internal bug?

@vtjnash vtjnash removed the merge me PR is reviewed. Merge when all tests are passing label Jan 8, 2025
@willtebbutt
Copy link
Contributor Author

It looks like the @eval statement I had (accidentally) left in the code was causing some kind of problem. The latest commit appears to resolve.

test/copy.jl Outdated Show resolved Hide resolved
@LilithHafner LilithHafner added backport 1.12 Change should be backported to release-1.12 and removed backport 1.12 Change should be backported to release-1.12 labels Jan 8, 2025
Co-authored-by: Lilith Orion Hafner <[email protected]>
@willtebbutt
Copy link
Contributor Author

It looks to me like the current test failures are unrelated to this PR, but someone who knows more about Julia's CI should probably take a look.

test/copy.jl Show resolved Hide resolved
Co-authored-by: Neven Sajko <[email protected]>
@KristofferC KristofferC merged commit 1ebacac into JuliaLang:master Jan 9, 2025
7 checks passed
@willtebbutt willtebbutt deleted the wct/deepcopy-fix branch January 9, 2025 12:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 1.11 Change should be backported to release-1.11 bugfix This change fixes an existing bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

deepcopy with non-trivial circular references fails in 1.11.2
7 participants