bpo-27275: Change popitem() and pop() methods of collections.OrderedDict (GH-27530)

* Unify the C and Python implementations of OrderedDict.popitem().

The C implementation no longer calls ``__getitem__`` and ``__delitem__``
methods of the OrderedDict subclasses.

* Change popitem() and pop() methods of collections.OrderedDict

For consistency with dict both implementations (pure Python and C)
of these methods in OrderedDict no longer call __getitem__ and
__delitem__ methods of the OrderedDict subclasses.

Previously only the Python implementation of popitem() did not
call them.

Lib/collections/__init__.py

@@ -236,11 +236,19 @@ class OrderedDict(dict):
         is raised.
 
         '''
-        if key in self:
+        marker = self.__marker
-            result = self[key]
+        result = dict.pop(self, key, marker)
-            del self[key]
+        if result is not marker:
+            # The same as in __delitem__().
+            link = self.__map.pop(key)
+            link_prev = link.prev
+            link_next = link.next
+            link_prev.next = link_next
+            link_next.prev = link_prev
+            link.prev = None
+            link.next = None
             return result
-        if default is self.__marker:
+        if default is marker:
             raise KeyError(key)
         return default
 

Lib/test/test_ordered_dict.py

@@ -896,5 +896,88 @@ class CPythonSubclassMappingTests(mapping_tests.BasicTestMappingProtocol):
         self.assertRaises(KeyError, d.popitem)
 
 
+class SimpleLRUCache:
+
+    def __init__(self, size):
+        super().__init__()
+        self.size = size
+        self.counts = dict.fromkeys(('get', 'set', 'del'), 0)
+
+    def __getitem__(self, item):
+        self.counts['get'] += 1
+        value = super().__getitem__(item)
+        self.move_to_end(item)
+        return value
+
+    def __setitem__(self, key, value):
+        self.counts['set'] += 1
+        while key not in self and len(self) >= self.size:
+            self.popitem(last=False)
+        super().__setitem__(key, value)
+        self.move_to_end(key)
+
+    def __delitem__(self, key):
+        self.counts['del'] += 1
+        super().__delitem__(key)
+
+
+class SimpleLRUCacheTests:
+
+    def test_add_after_full(self):
+        c = self.type2test(2)
+        c['t1'] = 1
+        c['t2'] = 2
+        c['t3'] = 3
+        self.assertEqual(c.counts, {'get': 0, 'set': 3, 'del': 0})
+        self.assertEqual(list(c), ['t2', 't3'])
+        self.assertEqual(c.counts, {'get': 0, 'set': 3, 'del': 0})
+
+    def test_popitem(self):
+        c = self.type2test(3)
+        for i in range(1, 4):
+            c[i] = i
+        self.assertEqual(c.popitem(last=False), (1, 1))
+        self.assertEqual(c.popitem(last=True), (3, 3))
+        self.assertEqual(c.counts, {'get': 0, 'set': 3, 'del': 0})
+
+    def test_pop(self):
+        c = self.type2test(3)
+        for i in range(1, 4):
+            c[i] = i
+        self.assertEqual(c.counts, {'get': 0, 'set': 3, 'del': 0})
+        self.assertEqual(c.pop(2), 2)
+        self.assertEqual(c.counts, {'get': 0, 'set': 3, 'del': 0})
+        self.assertEqual(c.pop(4, 0), 0)
+        self.assertEqual(c.counts, {'get': 0, 'set': 3, 'del': 0})
+        self.assertRaises(KeyError, c.pop, 4)
+        self.assertEqual(c.counts, {'get': 0, 'set': 3, 'del': 0})
+
+    def test_change_order_on_get(self):
+        c = self.type2test(3)
+        for i in range(1, 4):
+            c[i] = i
+        self.assertEqual(list(c), list(range(1, 4)))
+        self.assertEqual(c.counts, {'get': 0, 'set': 3, 'del': 0})
+        self.assertEqual(c[2], 2)
+        self.assertEqual(c.counts, {'get': 1, 'set': 3, 'del': 0})
+        self.assertEqual(list(c), [1, 3, 2])
+
+
+class PySimpleLRUCacheTests(SimpleLRUCacheTests, unittest.TestCase):
+
+    class type2test(SimpleLRUCache, py_coll.OrderedDict):
+        pass
+
+
+@unittest.skipUnless(c_coll, 'requires the C version of the collections module')
+class CSimpleLRUCacheTests(SimpleLRUCacheTests, unittest.TestCase):
+
+    @classmethod
+    def setUpClass(cls):
+        class type2test(SimpleLRUCache, c_coll.OrderedDict):
+            pass
+        cls.type2test = type2test
+
+
 if __name__ == "__main__":
     unittest.main()

Misc/NEWS.d/next/Library/2021-08-01-19-49-09.bpo-27275.QsvE0k.rst

@@ -0,0 +1,3 @@
+:meth:`collections.OrderedDict.popitem` and :meth:`collections.OrderedDict.pop`
+no longer call ``__getitem__`` and ``__delitem__`` methods of the OrderedDict
+subclasses.

Objects/odictobject.c

@@ -1047,8 +1047,37 @@ OrderedDict_setdefault_impl(PyODictObject *self, PyObject *key,
 
 /* pop() */
 
-/* forward */
+static PyObject *
-static PyObject * _odict_popkey(PyObject *, PyObject *, PyObject *);
+_odict_popkey_hash(PyObject *od, PyObject *key, PyObject *failobj,
+                   Py_hash_t hash)
+{
+    PyObject *value = NULL;
+
+    _ODictNode *node = _odict_find_node_hash((PyODictObject *)od, key, hash);
+    if (node != NULL) {
+        /* Pop the node first to avoid a possible dict resize (due to
+           eval loop reentrancy) and complications due to hash collision
+           resolution. */
+        int res = _odict_clear_node((PyODictObject *)od, node, key, hash);
+        if (res < 0) {
+            return NULL;
+        }
+        /* Now delete the value from the dict. */
+        value = _PyDict_Pop_KnownHash(od, key, hash, failobj);
+    }
+    else if (value == NULL && !PyErr_Occurred()) {
+        /* Apply the fallback value, if necessary. */
+        if (failobj) {
+            value = failobj;
+            Py_INCREF(failobj);
+        }
+        else {
+            PyErr_SetObject(PyExc_KeyError, key);
+        }
+    }
+
+    return value;
+}
 
 /* Skips __missing__() calls. */
 /*[clinic input]
@@ -1067,81 +1096,11 @@ static PyObject *
 OrderedDict_pop_impl(PyODictObject *self, PyObject *key,
                      PyObject *default_value)
 /*[clinic end generated code: output=7a6447d104e7494b input=7efe36601007dff7]*/
-{
-    return _odict_popkey((PyObject *)self, key, default_value);
-}
-
-static PyObject *
-_odict_popkey_hash(PyObject *od, PyObject *key, PyObject *failobj,
-                   Py_hash_t hash)
-{
-    _ODictNode *node;
-    PyObject *value = NULL;
-
-    /* Pop the node first to avoid a possible dict resize (due to
-       eval loop reentrancy) and complications due to hash collision
-       resolution. */
-    node = _odict_find_node_hash((PyODictObject *)od, key, hash);
-    if (node == NULL) {
-        if (PyErr_Occurred())
-            return NULL;
-    }
-    else {
-        int res = _odict_clear_node((PyODictObject *)od, node, key, hash);
-        if (res < 0) {
-            return NULL;
-        }
-    }
-
-    /* Now delete the value from the dict. */
-    if (PyODict_CheckExact(od)) {
-        if (node != NULL) {
-            value = _PyDict_GetItem_KnownHash(od, key, hash);  /* borrowed */
-            if (value != NULL) {
-                Py_INCREF(value);
-                if (_PyDict_DelItem_KnownHash(od, key, hash) < 0) {
-                    Py_DECREF(value);
-                    return NULL;
-                }
-            }
-        }
-    }
-    else {
-        int exists = PySequence_Contains(od, key);
-        if (exists < 0)
-            return NULL;
-        if (exists) {
-            value = PyObject_GetItem(od, key);
-            if (value != NULL) {
-                if (PyObject_DelItem(od, key) == -1) {
-                    Py_CLEAR(value);
-                }
-            }
-        }
-    }
-
-    /* Apply the fallback value, if necessary. */
-    if (value == NULL && !PyErr_Occurred()) {
-        if (failobj) {
-            value = failobj;
-            Py_INCREF(failobj);
-        }
-        else {
-            PyErr_SetObject(PyExc_KeyError, key);
-        }
-    }
-
-    return value;
-}
-
-static PyObject *
-_odict_popkey(PyObject *od, PyObject *key, PyObject *failobj)
 {
     Py_hash_t hash = PyObject_Hash(key);
     if (hash == -1)
         return NULL;
-
+    return _odict_popkey_hash((PyObject *)self, key, default_value, hash);
-    return _odict_popkey_hash(od, key, failobj, hash);
 }