| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
* PyDict_Copy() now also returns a dict if the argument is a frozendict. * Remove _PyDict_CopyAsDict() function. * Fix frozendict.items() ^ frozendict.items(). Add non-regression test.
|
I propose changing PyDict_Copy() API to always return a dict. Previously, it returned a frozendict if it was called on a frozendict. IMO it's less surprising and more convenient that PyDict_Copy() always return a copy (as a dict). Example in typeobject.c: static PyTypeObject*
type_new_init(type_new_ctx *ctx)
{
PyObject *dict = PyDict_Copy(ctx->orig_dict);
if (dict == NULL) {
goto error;
}
...
set_tp_dict(type, dict);
...
}The function accepts dict or frozendict but the type dictionary (dict variable) must be a dict. Example in Modules/_elementtree.c: static PyObject*
get_attrib_from_keywords(PyObject *kwds)
{
...
if (attrib) {
...
Py_SETREF(attrib, PyDict_Copy(attrib));
}
else {
attrib = PyDict_New();
}
if (attrib != NULL && PyDict_Update(attrib, kwds) < 0) {
Py_DECREF(attrib);
return NULL;
}
return attrib;
}(With my pending change PR gh-145508), the function accepts dict or frozendict but the result must be a dict (for example, we call PyDict_Update() on it). |
Sorry, something went wrong.
|
Currently, PyDict_Copy(frozendict) returns a frozendict copy which is something used internally to modify an immutable frozendict! But I don't think that we should leak such implementation details to the public C API. Existing C extensions calling PyDict_Copy() expect it to return a dict. If it can return a frozendict on Python 3.15, it can trigger bugs. Always returning dict avoids these issues. |
Sorry, something went wrong.
|
I'm not so sure this is a good idea. If I call something named Copy, I'd expect the result to be the same type as the input. If anything, let's disallow the use of frozendict in PyDict_Copy. |
Sorry, something went wrong.
|
The frozendict C API has only 3 functions: PyFrozenDict_Check(), PyFrozenDict_CheckExact(), PyFrozenDict_New(). For all other methods, it reuses the dict C API (PyDict functions). PyDict_Copy() is the only function where the return type can depend on the dictionary type (dict or frozendict). IMO it's convenient to also accept frozendict here and convert it to a dict (create a copy). It would be useful in multiple places as I shown in my previous comment.
It would mean that such code (using this PR): PyObject *dict;
dict = PyDict_Copy(orig_dict);
if (dict == NULL) {
goto error;
}should be replaced with: PyObject *dict;
if (PyFrozenDict_Check(orig_dict)) {
dict = PyDict_New();
if (dict == NULL) {
goto error;
}
if (PyDict_Merge(dict, orig_dict, 1) < 0) {
goto error;
}
}
else {
dict = PyDict_Copy(orig_dict);
if (dict == NULL) {
goto error;
}
}As a temporary solution, I added an internal function _PyDict_CopyAsDict() (in commit 95f56b1). But I would prefer to have a public C API for such code. |
Sorry, something went wrong.
|
I completed PyDict_Copy() documentation. |
Sorry, something went wrong.
|
Another example in PR gh-145123: else if (PyDict_Check(dict)) {
/* Copy __dict__ to avoid mutating it. */
PyObject *temp = PyDict_Copy(dict);
Py_SETREF(dict, temp);
}
else if (PyFrozenDict_Check(dict)) {
/* Convert frozendict to a mutable dict for merging. */
PyObject *temp = PyDict_New();
if (temp != NULL && PyDict_Update(temp, dict) < 0) {
Py_DECREF(temp);
temp = NULL;
}
Py_SETREF(dict, temp);
} |
Sorry, something went wrong.
And here is something completely different! I wrote #145531 to add PyFrozenDict_AsDict() function and modify PyDict_Copy() to only accept dict. |
Sorry, something went wrong.
|
Why is frozen dict needed to be copied? It's immutable. |
Sorry, something went wrong.
|
|
||
| return _PyDict_Copy(o); | ||
| PyObject *res; | ||
| Py_BEGIN_CRITICAL_SECTION(o); |
There was a problem hiding this comment.
Even we want to copy as dict from frozendict, critical section is actually needed for frozendict?
Sorry, something went wrong.
There was a problem hiding this comment.
We should be able to skip the critical section for frozendict, at least for the PyFrozenDict_CheckExact() case. But I would prefer to work on such optimization in a separated PR since this PR (and PR gh-145531) is already quite complex.
Sorry, something went wrong.
Multiple functions accept dict and frozendict (see previous comments), but then only want to work on a dict copy and so use PyDict_Copy(). The question is which API do we want to use to convert a frozendict to a dict. Here I propose to accept frozendict in PyDict_Copy(), but convert it to dict. |
Sorry, something went wrong.
Not disagreeing that it's useful, but it's also a huge footgun. I think it's really counterintuitive for a copy to have a different type than the input. That, and how are users supposed to actually create a copy of frozendict? I'd be okay with it if we did the following:
With the existence of those two, the expected behavior is much more intuitive. |
Sorry, something went wrong.
|
@ZeroIntensity and @StanFromIreland would prefer to not accept frozendict in PyDict_Copy(). I convinced myself that's the right way. For example, copy.copy() returns the same as the input. So I close this PR. Instead, I created PR gh-145542 to no longer accept frozendict in PyDict_Copy(). I suggest to continue the discussion in PR gh-145531 about adding a new function to convert a frozendict to a dict. |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
📚 Documentation preview 📚: https://cpython-previews--145517.org.readthedocs.build/