pyapp-kit / psygnal

Python observer pattern (callback/event system). Modeled after Qt Signals & Slots (but independent of Qt)
https://psygnal.readthedocs.io/
BSD 3-Clause "New" or "Revised" License
84 stars 13 forks source link

Copy operator of evented sets lead to sharing inner structures between copies #254

Closed Czaki closed 8 months ago

Czaki commented 9 months ago

Original issue

https://github.com/napari/napari/issues/6620

Description

When debugging linked issue, I noticed that copu of pysygnal.containers.Selection shares internal structures with the original one. As I understand the code of the copy operator, it does only shallow copy of internal structures.

https://github.com/pyapp-kit/psygnal/blob/b0d2057025e6b8b9facc0210d01222bfdc356a8a/src/psygnal/containers/_evented_set.py#L92-L95

There is any reason to not implement copy as:

    def __copy__(self: _Cls) -> _Cls:
        return self.copy()

Where copy() is implemented as:

https://github.com/pyapp-kit/psygnal/blob/b0d2057025e6b8b9facc0210d01222bfdc356a8a/src/psygnal/containers/_evented_set.py#L97-L98

Current copy may also copy callbacks connected to signals.

tlambert03 commented 9 months ago

yeah, I agree that's a tricky result, and I agree it's more surprising to retain any of the mutable characteristics of one set (including callbacks) in the other set. However, in order to mimic native set behavior as much as possible, I don't think it can simply be self.copy(), because the members of the set should be copied over (as they would in set.copy()). But, perhaps it could be

    def __copy__(self: _Cls) -> _Cls:
        new = self.copy()
        new._data.update(self._data)
        return new

thoughts?

Czaki commented 9 months ago

but


 def copy(self: _Cls) -> _Cls: 
     return self.__class__(self) 

Also copy elements, as it takes iterable as constructor arguments.

tlambert03 commented 9 months ago

oh right! Sorry, completely missed that. So, yep, I agree then with your original proposal