Merge pull request #128 from zopefoundation/issue112

Fix slicing and copying of PersistentList and PersistentMapping.

CHANGES.rst

@@ -4,6 +4,13 @@
 4.5.2 (unreleased)
 ------------------
 
+- Fix slicing of ``PersistentList`` to always return instances of the
+  same class. It was broken on Python 3 prior to 3.7.4.
+
+- Fix copying  of ``PersistentList`` and ``PersistentMapping`` using
+  ``copy.copy`` to also copy the underlying data object. This was
+  broken prior to Python 3.7.4.
+
 - Fix ``PersistentList`` to mark itself as changed after calling
   ``clear``. See `PR 115 <https://github.com/zopefoundation/persistent/pull/115/>`_.
 

persistent/list.py

@@ -31,10 +31,13 @@ class PersistentList(UserList, persistent.Persistent):
     as changed and automatically persisted.
 
     .. versionchanged:: 4.5.2
-       Using the `clear` method, or writing ``del inst[:]`` now only
-       results in marking the instance as changed if it actually removed
+       Using the `clear` method, or deleting a slice (e.g., ``del inst[:]`` or ``del inst[x:x]``)
+       now only results in marking the instance as changed if it actually removed
        items.
+    .. versionchanged:: 4.5.2
+       The `copy` method is available on Python 2.
     """
+    __super_getitem = UserList.__getitem__
     __super_setitem = UserList.__setitem__
     __super_delitem = UserList.__delitem__
     __super_iadd = UserList.__iadd__
@@ -49,43 +52,62 @@ class PersistentList(UserList, persistent.Persistent):
     __super_clear = (
         UserList.clear
         if hasattr(UserList, 'clear')
-        else lambda inst: inst.__delitem__(slice(None, None, None))
+        else lambda inst: inst.__delitem__(_SLICE_ALL)
     )
 
+    if not PYTHON2 and sys.version_info[:3] < (3, 7, 4):
+        # Prior to 3.7.4, Python 3 (but not Python 2) failed to properly
+        # return an instance of the same class.
+        # See https://bugs.python.org/issue27639
+        # and https://github.com/zopefoundation/persistent/issues/112.
+        # We only define the special method on the necessary versions to avoid
+        # any speed penalty.
+        def __getitem__(self, item):
+            result = self.__super_getitem(item)
+            if isinstance(item, slice):
+                result = self.__class__(result)
+            return result
+
+    if sys.version_info[:3] < (3, 7, 4):
+        # Likewise for __copy__, except even Python 2 needs it.
+        # See https://github.com/python/cpython/commit/3645d29a1dc2102fdb0f5f0c0129ff2295bcd768
+        def __copy__(self):
+            inst = self.__class__.__new__(self.__class__)
+            inst.__dict__.update(self.__dict__)
+            # Create a copy and avoid triggering descriptors
+            inst.__dict__["data"] = self.__dict__["data"][:]
+            return inst
+
     def __setitem__(self, i, item):
         self.__super_setitem(i, item)
         self._p_changed = 1
 
     def __delitem__(self, i):
         # If they write del list[:] but we're empty,
-        # no need to mark us changed.
-        needs_changed = i != _SLICE_ALL or bool(self)
+        # no need to mark us changed. Likewise with
+        # a slice that's empty, like list[1:1].
+        len_before = len(self.data)
         self.__super_delitem(i)
-        if needs_changed:
+        if len(self.data) != len_before:
             self._p_changed = 1
 
     if PYTHON2:  # pragma: no cover
         __super_setslice = UserList.__setslice__
         __super_delslice = UserList.__delslice__
 
+        def copy(self):
+            return self.__class__(self)
+
         def __setslice__(self, i, j, other):
             self.__super_setslice(i, j, other)
             self._p_changed = 1
 
         def __delslice__(self, i, j):
-            # On Python 2, the slice dunders, like ``__delslice__``,
-            # are documented as getting ``(0, sys.maxsize)`` as the
-            # arguments for list[:]. Prior to 2.7.10, it was
-            # incorrectly documented as ``(0, sys.maxint)``, but that
-            # usually worked because ``sys.maxsize`` and
-            # ``sys.maxint`` are usually the same. But on 64-bit
-            # Windown, where ``sizeof(int) == sizeof(long) == 4``,
-            # they're different (``sys.maxsize`` is bigger). See
-            # https://bugs.python.org/issue23645
-            needs_changed = i == 0 and j == sys.maxsize and bool(self)
-            self.__super_delslice(i, j)
-            if needs_changed:
-                self._p_changed = 1
+            # In the past we just called super, but we want to apply the
+            # same _p_changed optimization logic that __delitem__ does. Don't
+            # call it as ``self.__delitem__``, though, because user code in subclasses
+            # on Python 2 may not be expecting to get a slice.
+            PersistentList.__delitem__(self, slice(i, j))
 
     def __iadd__(self, other):
         L = self.__super_iadd(other)

persistent/mapping.py

@@ -16,6 +16,8 @@
 
 $Id$"""
 
+import sys
+
 import persistent
 from persistent._compat import IterableUserDict
 
@@ -55,6 +57,25 @@ class PersistentMapping(IterableUserDict, persistent.Persistent):
     __super_pop = IterableUserDict.pop
     __super_popitem = IterableUserDict.popitem
 
+
+    # Be sure to make a deep copy of our ``data`` (See PersistentList.)
+    # See https://github.com/python/cpython/commit/3645d29a1dc2102fdb0f5f0c0129ff2295bcd768
+    # This was fixed in CPython 3.7.4, but we can't rely on that because it
+    # doesn't handle our old ``_container`` appropriately (it goes directly
+    # to ``self.__dict__``, bypassing the descriptor). The code here was initially
+    # based on the version found in 3.7.4.
+    def __copy__(self):
+        inst = self.__class__.__new__(self.__class__)
+        inst.__dict__.update(self.__dict__)
+        # Create a copy and avoid triggering descriptors
+        if '_container' in inst.__dict__:
+            # BWC for ZODB < 3.3.
+            data = inst.__dict__.pop('_container')
+        else:
+            data = inst.__dict__['data']
+        inst.__dict__["data"] = data.copy()
+        return inst
+
     def __delitem__(self, key):
         self.__super_delitem(key)
         self._p_changed = 1
@@ -121,8 +142,10 @@ def popitem(self):
         self._p_changed = 1
         return result
 
-    # Old implementations used _container rather than data.
-    # Use a descriptor to provide data when we have _container instead
+    # Old implementations (prior to 2001; see
+    # https://github.com/zopefoundation/ZODB/commit/c64281cf2830b569eed4f211630a8a61d22a0f0b#diff-b0f568e20f51129c10a096abad27c64a)
+    # used ``_container`` rather than ``data``. Use a descriptor to provide
+    # ``data`` when we have ``_container`` instead
 
     @default
     def data(self): # pylint:disable=method-hidden

persistent/tests/test_list.py

@@ -16,7 +16,10 @@
 
 import unittest
 
+
 from persistent.tests.utils import TrivialJar
+from persistent.tests.utils import copy_test
+
 
 l0 = []
 l1 = [0]
@@ -268,15 +271,32 @@ def test_setslice(self):
         self.assertEqual(inst, [1, 2, 3])
         self.assertTrue(inst._p_changed)
 
-    def test_delslice_nonempty(self):
+    def test_delslice_all_nonempty_list(self):
+        # Delete everything from a non-empty list
         inst = self._makeOne([1, 2, 3])
         self.assertFalse(inst._p_changed)
         self.assertEqual(inst, [1, 2, 3])
         del inst[:]
         self.assertEqual(inst, [])
         self.assertTrue(inst._p_changed)
 
-    def test_delslice_empty(self):
+    def test_delslice_sub_nonempty_list(self):
+        # delete a sub-list from a non-empty list
+        inst = self._makeOne([0, 1, 2, 3])
+        self.assertFalse(inst._p_changed)
+        del inst[1:2]
+        self.assertEqual(inst, [0, 2, 3])
+        self.assertTrue(inst._p_changed)
+
+    def test_delslice_empty_nonempty_list(self):
+        # delete an empty sub-list from a non-empty list
+        inst = self._makeOne([0, 1, 2, 3])
+        self.assertFalse(inst._p_changed)
+        del inst[1:1]
+        self.assertEqual(inst, [0, 1, 2, 3])
+        self.assertFalse(inst._p_changed)
+
+    def test_delslice_all_empty_list(self):
         inst = self._makeOne([])
         self.assertFalse(inst._p_changed)
         self.assertEqual(inst, [])
@@ -347,6 +367,33 @@ def test_reverse(self):
         self.assertEqual(inst, [1, 2])
         self.assertTrue(inst._p_changed)
 
+    def test_getslice_same_class(self):
+        class MyList(self._getTargetClass()):
+            pass
+
+        inst = MyList()
+        inst._p_jar = self._makeJar()
+        # Entire thing, empty.
+        inst2 = inst[:]
+        self.assertIsNot(inst, inst2)
+        self.assertEqual(inst, inst2)
+        self.assertIsInstance(inst2, MyList)
+        # The _p_jar is *not* propagated.
+        self.assertIsNotNone(inst._p_jar)
+        self.assertIsNone(inst2._p_jar)
+
+        # Partial
+        inst.extend((1, 2, 3))
+        inst2 = inst[1:2]
+        self.assertEqual(inst2, [2])
+        self.assertIsInstance(inst2, MyList)
+        self.assertIsNone(inst2._p_jar)
+
+    def test_copy(self):
+        inst = self._makeOne()
+        inst.append(42)
+        copy_test(self, inst)
+
 
 def test_suite():
     return unittest.defaultTestLoader.loadTestsFromName(__name__)

persistent/tests/test_mapping.py

@@ -13,7 +13,9 @@
 ##############################################################################
 import unittest
 
+
 from persistent.tests.utils import TrivialJar
+from persistent.tests.utils import copy_test
 
 # pylint:disable=blacklisted-name, protected-access
 
@@ -281,6 +283,25 @@ def test_clear_empty_legacy_container(self):
         # and we are marked as changed.
         self.assertTrue(pm._p_changed)
 
+    def test_copy(self):
+        pm = self._makeOne()
+        pm['key'] = 42
+        copy = copy_test(self, pm)
+        self.assertEqual(42, copy['key'])
+
+    def test_copy_legacy_container(self):
+        pm = self._makeOne()
+        pm['key'] = 42
+        pm.__dict__['_container'] = pm.__dict__.pop('data')
+
+        self.assertNotIn('data', pm.__dict__)
+        self.assertIn('_container', pm.__dict__)
+
+        copy = copy_test(self, pm)
+        self.assertNotIn('_container', copy.__dict__)
+        self.assertIn('data', copy.__dict__)
+        self.assertEqual(42, copy['key'])
+
 
 class Test_legacy_PersistentDict(unittest.TestCase):
 

persistent/tests/utils.py

@@ -71,3 +71,24 @@ def setstate(self, obj):
         # This isn't what setstate() is supposed to do,
         # but it suffices for the tests.
         obj.__setstate__(self.remembered)
+
+
+def copy_test(self, obj):
+    import copy
+    # Test copy.copy. Do this first, because depending on the
+    # version of Python, `UserDict.copy()` may wind up
+    # mutating the original object's ``data`` (due to our
+    # BWC with ``_container``). This shows up only as a failure
+    # of coverage.
+    obj.test = [1234]  # Make sure instance vars are also copied.
+    obj_copy = copy.copy(obj)
+    self.assertIsNot(obj.data, obj_copy.data)
+    self.assertEqual(obj.data, obj_copy.data)
+    self.assertIs(obj.test, obj_copy.test)
+
+    # Test internal copy
+    obj_copy = obj.copy()
+    self.assertIsNot(obj.data, obj_copy.data)
+    self.assertEqual(obj.data, obj_copy.data)
+
+    return obj_copy