Repository navigation
C API: Add PyDict_GetItemRef() function #106004
Description
Activity
Example of
PyDict_GetItemRef()usage of the function, replacingPyDict_GetItemWithError():@@ -5146,13 +5191,12 @@ dictitems_contains(_PyDictViewObject *dv, PyObject *obj) return 0; key = PyTuple_GET_ITEM(obj, 0); value = PyTuple_GET_ITEM(obj, 1); - found = PyDict_GetItemWithError((PyObject *)dv->dv_dict, key); + if (PyDict_GetItemRef((PyObject *)dv->dv_dict, key, &found) < 0) { + return -1; + } if (found == NULL) { - if (PyErr_Occurred()) - return -1; return 0; } - Py_INCREF(found); result = PyObject_RichCompareBool(found, value, Py_EQ); Py_DECREF(found); return result;
PyErr_Occurred()andPy_INCREF()calls can be removed.Reacted by Gregory P. Smith- added a commit that references this issue
on Jun 23, 2023 - added a commit that references this issue
on Jun 23, 2023 - added a commit that references this issue
on Jun 23, 2023 I'm in favor of this because I don't think we should have public APIs that (a) require a value check + PyErr_Occurred() call pattern - a frequent source of lurking bugs - or (b) return borrowed references. Yes I know we already have them, that's missing the point. The point is that with these in place, we can promote their use over the others because these are better in all respects.
One possible long term future would have us deprecate the messy borrowing APIs. Having these already in place sooner rather than later smooths that potential transition.
There is no foreseeable future in which we'd go the other way towards all-borrowing-only APIs.
Reacted by Erlend E. AaslandThere is no foreseeable future in which we'd go the other way towards all-borrowing-only APIs.
FYI I'm tracking the list of C API functions returning borrowed references at: https://pythoncapi.readthedocs.io/bad_api.html#functions
I'm trying to provide a replacement for each function, and if possible an efficient replacement. For example, PyDict_GetItem() can be replaced with PyObject_GetItem(), but it's less efficient (that's what I explained in my first message).
- added a commit that references this issue
on Jun 29, 2023 The only problem is that functions with so similar names have completely different interface. It is pretty confusing. Would not be better to name it
PyDict_LookupItemor like? It may be worth to add alsoPyMapping_LookupItemfor convenience. BTW, I think it is time to make_PyObject_LookupAttrpublic (if we agree about the name).Reacted by Petr ViktorinI replied on the PR, since the discussion is ongoing there.
- added a commit that references this issue
on Jul 21, 2023 - added 3 commits that reference this issue
on Jul 25, 2023 Tests will be added in #107179.
- added a commit that references this issue
on Jul 25, 2023
The PyDict C API has a bad history. PyDict_GetItem() ignores all exception: error on hash(), error on "key == key2", KeyboardInterrupt, etc. PyDict_GetItemWithError() was added to fix this design. Moreover, Python 3.9 and older allowed to call PyDict_GetItem() with the GIL released.
PyDict_GetItem() returns a borrowed reference which is usually safe since the dictionary still contains a strong reference to the request value. But in general, borrowed references are error prone and can likely lead to complex race conditions causing crashes:
While
PyDict_GetItem()calls can be quite easily replaced withPyObject_GetItem()which has a better API (return a new stong reference), developers usually prefer to still use the specialized PyDict API for best performance: avoid the minor overhead of type dispatching,Py_TYPE(obj)->tp_as_mapping->mp_subscript.I propose adding
PyDict_GetItemRef()andPyDict_GetItemStringRef()functions to the limited C API (version 3.13): replacements forPyDict_GetItem(),PyDict_GetItemWithError()andPyDict_GetItemString().API:
PyDict_GetItemWithError()has another API issue: when it returns NULL, it can mean two things. It returns NULL if the key is missing, but it also returns NULL on error. The caller has to checkPyErr_Occurred()to distinguish the two cases (to write correct code). See capi-workgroup/problems#1 Proposed API avoids this by returning anint: return -1 on error, or return 0 otherwise (present or missing key). CheckingPyErr_Occurred()is no longer needed.By the way, the public C API has no PyDict_GetItemStringWithError() function: using
PyDict_GetItemWithError()with achar*key is not convenient. The_PyDict_GetItemStringWithError()function exists but it's a private C API.Linked PRs