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

Fix cloning issue with container traits #1624

Merged
merged 5 commits into from
Mar 31, 2022
Merged

Conversation

mdickinson
Copy link
Member

@mdickinson mdickinson commented Mar 30, 2022

This PR makes a shallow fix for problems with clone_traits applied to List, Dict and Set traits. It doesn't try to touch deeper issues of disconnection of TraitsListObject, TraitDictObject and friends from their owning HasTraits object.

Closes #1622. The cause of that issue is that we were using a function lambda: None for object where a HasTraits object was expected. Inside TraitListObject we then take a weakref to that function. In most cases, the lambda function has no other references to it, so it's garbage collected immediately and when the weakref is dereferenced, it returns None. But in the deepcopy case the weakref target is kept alive for long enough that we try to use the actual lambda: None function as a HasTraits object. The solution is to allow and special-case an object of None in the TraitListObject constructor.

@mdickinson
Copy link
Member Author

@sallenEnth Interested in reviewing? (Feel free to unassign yourself from review if not.)

Copy link
Contributor

@sallenEnth sallenEnth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The solution is actually simpler than I thought. I noticed that TraitListObject had already situation where object was lambda: None. Thank you for the work @mdickinson.
LGTM, but I still does not have a very deep knowledge of Traits. I have some minor suggestions for changes/additions to the regression tests.

traits/tests/test_regression.py Outdated Show resolved Hide resolved
traits/tests/test_regression.py Show resolved Hide resolved
mdickinson and others added 2 commits March 31, 2022 11:06
Co-authored-by: Steve Allen <sallen@enthought.com>
Co-authored-by: Steve Allen <sallen@enthought.com>
@mdickinson
Copy link
Member Author

That's unfortunately the part that's not fixed by this PR, and that's much harder to fix in general.

Apologies - looks like I'm wrong about this. So let's keep that test (thank you!), and I need to figure out how this is working when I expected it to fail ...

@mdickinson
Copy link
Member Author

I need to figure out how this is working when I expected it to fail ...

Got it: the resulting value from applying deepcopy to the original TraitListObject is indeed disconnected from the object, but when we assign that copied value back to the object using setattr (here), we create a new TraitListObject that's connected to the newly cloned HasTraits object.

@mdickinson mdickinson merged commit a307f57 into main Mar 31, 2022
@mdickinson mdickinson deleted the fix/clone-traits-enum branch March 31, 2022 10:39
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.

Clone traits does not seem to work with dynamic Enum
2 participants