pythongh-111262: Add PyDict_Pop() function

Change the API of the internal _PyDict_Pop_KnownHash() function
to remove the default value and return an int.

Co-Authored-By: Stefan Behnel <stefan_ml@behnel.de>
Co-authored-by: Antoine Pitrou <pitrou@free.fr>

Author: 3 people

Doc/c-api/dict.rst

@@ -173,6 +173,20 @@ Dictionary Objects
 
    .. versionadded:: 3.4
 
+
+.. c:function:: int PyDict_Pop(PyObject *p, PyObject *key, PyObject **result)
+
+   Remove *key* from dictionary *p* and return the removed value.
+
+   - If the key is present, set *\*result* to a new reference to the removed
+     value, and return ``1``.
+   - If the key is missing, set *\*result* to ``NULL``, and return ``0``.
+   - On error, raise an exception and return ``-1``.
+
+   This is the similar to :meth:`dict.pop` without the default value.
+
+   .. versionadded:: 3.13
+
 .. c:function:: PyObject* PyDict_Items(PyObject *p)
 
    Return a :c:type:`PyListObject` containing all the items from the dictionary.

Doc/data/stable_abi.dat

@@ -1164,6 +1164,11 @@ New Features
   :c:func:`PyErr_WriteUnraisable`, but allow to customize the warning mesage.
   (Contributed by Serhiy Storchaka in :gh:`108082`.)
 
+* Add :c:func:`PyDict_Pop` function: remove a key from a dictionary and return
+  the removed value. This is the similar to :meth:`dict.pop` without the
+  default value.
+  (Contributed by Victor Stinner in :gh:`111262`.)
+
 
 Porting to Python 3.13
 ----------------------

Doc/whatsnew/3.13.rst

@@ -66,6 +66,8 @@ PyAPI_FUNC(int) PyDict_DelItemString(PyObject *dp, const char *key);
 // - On error, raise an exception and return -1.
 PyAPI_FUNC(int) PyDict_GetItemRef(PyObject *mp, PyObject *key, PyObject **result);
 PyAPI_FUNC(int) PyDict_GetItemStringRef(PyObject *mp, const char *key, PyObject **result);
+
+PyAPI_FUNC(int) PyDict_Pop(PyObject *dict, PyObject *key, PyObject **result);
 #endif
 
 #if !defined(Py_LIMITED_API) || Py_LIMITED_API+0 >= 0x030A0000

Include/dictobject.h

@@ -116,7 +116,11 @@ extern PyObject *_PyDict_LoadGlobal(PyDictObject *, PyDictObject *, PyObject *);
 extern int _PyDict_SetItem_Take2(PyDictObject *op, PyObject *key, PyObject *value);
 extern int _PyObjectDict_SetItem(PyTypeObject *tp, PyObject **dictptr, PyObject *name, PyObject *value);
 
-extern PyObject *_PyDict_Pop_KnownHash(PyObject *, PyObject *, Py_hash_t, PyObject *);
+extern int _PyDict_Pop_KnownHash(
+    PyDictObject *dict,
+    PyObject *key,
+    Py_hash_t hash,
+    PyObject **result);
 
 #define DKIX_EMPTY (-1)
 #define DKIX_DUMMY (-2)  /* Used internally */

Include/internal/pycore_dict.h

@@ -432,6 +432,45 @@ def test_dict_mergefromseq2(self):
         # CRASHES mergefromseq2({}, NULL, 0)
         # CRASHES mergefromseq2(NULL, {}, 0)
 
+    def test_dict_pop(self):
+        # Test PyDict_Pop()
+        dict_pop = _testcapi.dict_pop
+        default = object()
+
+        def expect_value(mydict, key, expected_value):
+            self.assertEqual(dict_pop(mydict.copy(), key),
+                             (1, expected_value))
+
+        def expect_missing(mydict, key):
+            self.assertEqual(dict_pop(mydict, key, default), (0, None))
+
+        # key present
+        mydict = {"key": 2, "key2": 5}
+        expect_value(mydict, "key", 2)
+        expect_value(mydict, "key2", 5)
+
+        # key missing
+        expect_missing({}, "key")  # empty dict has a fast path
+        expect_missing({"a": 1}, "key")
+        self.assertRaises(KeyError, dict_pop, {}, "key", NULL)
+        self.assertRaises(KeyError, dict_pop, {"a": 1}, "key", NULL)
+
+        # dict error
+        not_dict = "string"
+        self.assertRaises(SystemError, dict_pop, not_dict, "key", default)
+
+        # key error
+        not_hashable_key = ["list"]
+        expect_missing({}, not_hashable_key)  # don't hash key if dict is empty
+        with self.assertRaises(TypeError):
+            dict_pop({'key': 1}, not_hashable_key, NULL)
+        with self.assertRaises(TypeError):
+            dict_pop({'key': 1}, not_hashable_key, default)
+        dict_pop({}, NULL, default)  # don't check key if dict is empty
+
+        # CRASHES dict_pop(NULL, "key")
+        # CRASHES dict_pop({"a": 1}, NULL, default)
+
 
 if __name__ == "__main__":
     unittest.main()

Lib/test/test_capi/test_dict.py

@@ -0,0 +1,3 @@
+Add :c:func:`PyDict_Pop` function: remove a key from a dictionary and return
+the removed value. This is the same as :meth:`dict.pop`. Patch by Stefan
+Behnel and Victor Stinner.

Lib/test/test_stable_abi_ctypes.py

@@ -2481,3 +2481,5 @@
 [function._Py_SetRefcnt]
     added = '3.13'
     abi_only = true
+[function.PyDict_Pop]
+    added = '3.13'

Misc/NEWS.d/next/C API/2023-11-10-10-21-38.gh-issue-111262.2utB5m.rst

@@ -1087,19 +1087,9 @@ bounded_lru_cache_wrapper(lru_cache_object *self, PyObject *args, PyObject *kwds
        The cache dict holds one reference to the link.
        We created one other reference when the link was created.
        The linked list only has borrowed references. */
-    popresult = _PyDict_Pop_KnownHash(self->cache, link->key,
-                                      link->hash, Py_None);
-    if (popresult == Py_None) {
-        /* Getting here means that the user function call or another
-           thread has already removed the old key from the dictionary.
-           This link is now an orphan.  Since we don't want to leave the
-           cache in an inconsistent state, we don't restore the link. */
-        Py_DECREF(popresult);
-        Py_DECREF(link);
-        Py_DECREF(key);
-        return result;
-    }
-    if (popresult == NULL) {
+    int res = _PyDict_Pop_KnownHash((PyDictObject*)self->cache, link->key,
+                                    link->hash, &popresult);
+    if (res < 0) {
         /* An error arose while trying to remove the oldest key (the one
            being evicted) from the cache.  We restore the link to its
            original position as the oldest link.  Then we allow the
@@ -1110,6 +1100,18 @@ bounded_lru_cache_wrapper(lru_cache_object *self, PyObject *args, PyObject *kwds
         Py_DECREF(result);
         return NULL;
     }
+    if (res == 0) {
+        /* Getting here means that the user function call or another
+           thread has already removed the old key from the dictionary.
+           This link is now an orphan.  Since we don't want to leave the
+           cache in an inconsistent state, we don't restore the link. */
+        assert(popresult == NULL);
+        Py_DECREF(link);
+        Py_DECREF(key);
+        return result;
+    }
+    assert(popresult != NULL);
+
     /* Keep a reference to the old key and old result to prevent their
        ref counts from going to zero during the update. That will
        prevent potentially arbitrary object clean-up code (i.e. __del__)

Misc/stable_abi.toml

@@ -331,6 +331,24 @@ dict_mergefromseq2(PyObject *self, PyObject *args)
 }
 
 
+static PyObject *
+dict_pop(PyObject *self, PyObject *args)
+{
+    PyObject *dict, *key;
+    if (!PyArg_ParseTuple(args, "OO", &dict, &key)) {
+        return NULL;
+    }
+    NULLABLE(dict);
+    NULLABLE(key);
+    PyObject *result = UNINITIALIZED_PTR;
+    int res = PyDict_Pop(dict, key,  &result);
+    if (result == NULL) {
+        return Py_NewRef(Py_None);
+    }
+    return Py_BuildValue("iN", res, result);
+}
+
+
 static PyMethodDef test_methods[] = {
     {"dict_check", dict_check, METH_O},
     {"dict_checkexact", dict_checkexact, METH_O},
@@ -358,7 +376,7 @@ static PyMethodDef test_methods[] = {
     {"dict_merge", dict_merge, METH_VARARGS},
     {"dict_update", dict_update, METH_VARARGS},
     {"dict_mergefromseq2", dict_mergefromseq2, METH_VARARGS},
-
+    {"dict_pop", dict_pop, METH_VARARGS},
     {NULL},
 };
 

Modules/_functoolsmodule.c

@@ -967,12 +967,13 @@ local_clear(localobject *self)
         HEAD_UNLOCK(runtime);
         while (tstate) {
             if (tstate->dict) {
-                PyObject *v = _PyDict_Pop(tstate->dict, self->key, Py_None);
-                if (v != NULL) {
-                    Py_DECREF(v);
+                PyObject *v;
+                if (PyDict_Pop(tstate->dict, self->key, &v) < 0) {
+                    // Silently ignore error
+                    PyErr_Clear();
                 }
                 else {
-                    PyErr_Clear();
+                    Py_XDECREF(v);
                 }
             }
             HEAD_LOCK(runtime);

Modules/_testcapi/dict.c

@@ -396,12 +396,13 @@ remove_unusable_flags(PyObject *m)
             if (flag_name == NULL) {
                 return -1;
             }
-            PyObject *v = _PyDict_Pop(dict, flag_name, Py_None);
-            Py_DECREF(flag_name);
-            if (v == NULL) {
+            PyObject *v;
+            if (PyDict_Pop(dict, flag_name, &v) < 0) {
+                Py_DECREF(flag_name);
                 return -1;
             }
-            Py_DECREF(v);
+            Py_DECREF(flag_name);
+            Py_XDECREF(v);
         }
     }
     return 0;

Modules/_threadmodule.c

@@ -2226,64 +2226,85 @@ PyDict_Next(PyObject *op, Py_ssize_t *ppos, PyObject **pkey, PyObject **pvalue)
     return _PyDict_Next(op, ppos, pkey, pvalue, NULL);
 }
 
+
 /* Internal version of dict.pop(). */
-PyObject *
-_PyDict_Pop_KnownHash(PyObject *dict, PyObject *key, Py_hash_t hash, PyObject *deflt)
+int
+_PyDict_Pop_KnownHash(PyDictObject *mp, PyObject *key, Py_hash_t hash,
+                      PyObject **result)
 {
-    Py_ssize_t ix;
-    PyObject *old_value;
-    PyDictObject *mp;
-    PyInterpreterState *interp = _PyInterpreterState_GET();
-
-    assert(PyDict_Check(dict));
-    mp = (PyDictObject *)dict;
+    assert(PyDict_Check(mp));
 
     if (mp->ma_used == 0) {
-        if (deflt) {
-            return Py_NewRef(deflt);
-        }
-        _PyErr_SetKeyError(key);
-        return NULL;
+        *result = NULL;
+        return 0;
     }
-    ix = _Py_dict_lookup(mp, key, hash, &old_value);
-    if (ix == DKIX_ERROR)
-        return NULL;
+
+    PyObject *old_value;
+    Py_ssize_t ix = _Py_dict_lookup(mp, key, hash, &old_value);
+    if (ix == DKIX_ERROR) {
+        *result = NULL;
+        return -1;
+    }
+
     if (ix == DKIX_EMPTY || old_value == NULL) {
-        if (deflt) {
-            return Py_NewRef(deflt);
-        }
-        _PyErr_SetKeyError(key);
-        return NULL;
+        *result = NULL;
+        return 0;
     }
+
     assert(old_value != NULL);
+    PyInterpreterState *interp = _PyInterpreterState_GET();
     uint64_t new_version = _PyDict_NotifyEvent(
             interp, PyDict_EVENT_DELETED, mp, key, NULL);
     delitem_common(mp, hash, ix, Py_NewRef(old_value), new_version);
 
     ASSERT_CONSISTENT(mp);
-    return old_value;
+    *result = old_value;
+    return 1;
 }
 
-PyObject *
-_PyDict_Pop(PyObject *dict, PyObject *key, PyObject *deflt)
+
+int
+PyDict_Pop(PyObject *op, PyObject *key, PyObject **result)
 {
+    if (!PyDict_Check(op)) {
+        *result = NULL;
+        PyErr_BadInternalCall();
+        return -1;
+    }
+    PyDictObject *dict = (PyDictObject *)op;
+
+    if (dict->ma_used == 0) {
+        *result = NULL;
+        return 0;
+    }
+
     Py_hash_t hash;
+    if (!PyUnicode_CheckExact(key) || (hash = unicode_get_hash(key)) == -1) {
+        hash = PyObject_Hash(key);
+        if (hash == -1) {
+            *result = NULL;
+            return -1;
+        }
+    }
+    return _PyDict_Pop_KnownHash(dict, key, hash, result);
+}
+
 
-    if (((PyDictObject *)dict)->ma_used == 0) {
-        if (deflt) {
-            return Py_NewRef(deflt);
+PyObject *
+_PyDict_Pop(PyObject *dict, PyObject *key, PyObject *default_value)
+{
+    PyObject *result;
+    if (PyDict_Pop(dict, key, &result) == 0) {
+        if (default_value != NULL) {
+            return Py_NewRef(default_value);
         }
         _PyErr_SetKeyError(key);
         return NULL;
     }
-    if (!PyUnicode_CheckExact(key) || (hash = unicode_get_hash(key)) == -1) {
-        hash = PyObject_Hash(key);
-        if (hash == -1)
-            return NULL;
-    }
-    return _PyDict_Pop_KnownHash(dict, key, hash, deflt);
+    return result;
 }
 
+
 /* Internal version of dict.from_keys().  It is subclass-friendly. */
 PyObject *
 _PyDict_FromKeys(PyObject *cls, PyObject *iterable, PyObject *value)

Modules/socketmodule.c

@@ -1049,7 +1049,10 @@ _odict_popkey_hash(PyObject *od, PyObject *key, PyObject *failobj,
             return NULL;
         }
         /* Now delete the value from the dict. */
-        value = _PyDict_Pop_KnownHash(od, key, hash, failobj);
+        if (_PyDict_Pop_KnownHash((PyDictObject *)od, key, hash,
+                                  &value) == 0) {
+            value = Py_NewRef(failobj);
+        }
     }
     else if (value == NULL && !PyErr_Occurred()) {
         /* Apply the fallback value, if necessary. */