Skip to content

GH-106485: Handle dict subclasses correctly when dematerializing __dict__ #107837

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Include/internal/pycore_dict.h
Original file line number Diff line number Diff line change
Expand Up @@ -199,7 +199,7 @@ _PyDict_NotifyEvent(PyInterpreterState *interp,
}

extern PyObject *_PyObject_MakeDictFromInstanceAttributes(PyObject *obj, PyDictValues *values);
extern int _PyObject_MakeInstanceAttributesFromDict(PyObject *obj, PyDictOrValues *dorv);
extern bool _PyObject_MakeInstanceAttributesFromDict(PyObject *obj, PyDictOrValues *dorv);
extern PyObject *_PyDict_FromItems(
PyObject *const *keys, Py_ssize_t keys_offset,
PyObject *const *values, Py_ssize_t values_offset,
Expand Down
1 change: 1 addition & 0 deletions Include/internal/pycore_global_objects_fini_generated.h

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions Include/internal/pycore_global_strings.h
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ struct _Py_global_strings {
STRUCT_FOR_STR(anon_lambda, "<lambda>")
STRUCT_FOR_STR(anon_listcomp, "<listcomp>")
STRUCT_FOR_STR(anon_module, "<module>")
STRUCT_FOR_STR(anon_null, "<NULL>")
STRUCT_FOR_STR(anon_setcomp, "<setcomp>")
STRUCT_FOR_STR(anon_string, "<string>")
STRUCT_FOR_STR(anon_unknown, "<unknown>")
Expand Down
1 change: 1 addition & 0 deletions Include/internal/pycore_runtime_init_generated.h

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

121 changes: 121 additions & 0 deletions Lib/test/test_opcache.py
Original file line number Diff line number Diff line change
@@ -1,8 +1,11 @@
import copy
import pickle
import dis
import threading
import types
import unittest
from test.support import threading_helper
import _testinternalcapi


class TestLoadSuperAttrCache(unittest.TestCase):
Expand Down Expand Up @@ -997,6 +1000,124 @@ def write(items):
opname = "UNPACK_SEQUENCE_LIST"
self.assert_races_do_not_crash(opname, get_items, read, write)

class C:
pass

class TestInstanceDict(unittest.TestCase):

def setUp(self):
c = C()
c.a, c.b, c.c = 0,0,0

def test_values_on_instance(self):
c = C()
c.a = 1
C().b = 2
c.c = 3
self.assertEqual(
_testinternalcapi.get_object_dict_values(c),
(1, '<NULL>', 3)
)

def test_dict_materialization(self):
c = C()
c.a = 1
c.b = 2
c.__dict__
self.assertIs(
_testinternalcapi.get_object_dict_values(c),
None
)

def test_dict_dematerialization(self):
c = C()
c.a = 1
c.b = 2
c.__dict__
self.assertIs(
_testinternalcapi.get_object_dict_values(c),
None
)
for _ in range(100):
c.a
self.assertEqual(
_testinternalcapi.get_object_dict_values(c),
(1, 2, '<NULL>')
)

def test_dict_dematerialization_multiple_refs(self):
c = C()
c.a = 1
c.b = 2
d = c.__dict__
for _ in range(100):
c.a
self.assertIs(
_testinternalcapi.get_object_dict_values(c),
None
)
self.assertIs(c.__dict__, d)

def test_dict_dematerialization_copy(self):
c = C()
c.a = 1
c.b = 2
c2 = copy.copy(c)
for _ in range(100):
c.a
c2.a
self.assertEqual(
_testinternalcapi.get_object_dict_values(c),
(1, 2, '<NULL>')
)
self.assertEqual(
_testinternalcapi.get_object_dict_values(c2),
(1, 2, '<NULL>')
)
c3 = copy.deepcopy(c)
for _ in range(100):
c.a
c3.a
self.assertEqual(
_testinternalcapi.get_object_dict_values(c),
(1, 2, '<NULL>')
)
#NOTE -- c3.__dict__ does not de-materialize

def test_dict_dematerialization_pickle(self):
c = C()
c.a = 1
c.b = 2
c2 = pickle.loads(pickle.dumps(c))
for _ in range(100):
c.a
c2.a
self.assertEqual(
_testinternalcapi.get_object_dict_values(c),
(1, 2, '<NULL>')
)
self.assertEqual(
_testinternalcapi.get_object_dict_values(c2),
(1, 2, '<NULL>')
)

def test_dict_dematerialization_subclass(self):
class D(dict): pass
c = C()
c.a = 1
c.b = 2
c.__dict__ = D(c.__dict__)
for _ in range(100):
c.a
self.assertIs(
_testinternalcapi.get_object_dict_values(c),
None
)
self.assertEqual(
c.__dict__,
{'a':1, 'b':2}
)


if __name__ == "__main__":
unittest.main()
36 changes: 36 additions & 0 deletions Modules/_testinternalcapi.c
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
#include "pycore_bytesobject.h" // _PyBytes_Find()
#include "pycore_compile.h" // _PyCompile_CodeGen, _PyCompile_OptimizeCfg, _PyCompile_Assemble, _PyCompile_CleanDoc
#include "pycore_ceval.h" // _PyEval_AddPendingCall
#include "pycore_dict.h" // _PyDictOrValues_GetValues
#include "pycore_fileutils.h" // _Py_normpath
#include "pycore_frame.h" // _PyInterpreterFrame
#include "pycore_gc.h" // PyGC_Head
Expand Down Expand Up @@ -1530,6 +1531,40 @@ test_pymem_getallocatorsname(PyObject *self, PyObject *args)
return PyUnicode_FromString(name);
}

static PyObject *
get_object_dict_values(PyObject *self, PyObject *obj)
{
PyTypeObject *type = Py_TYPE(obj);
if (!_PyType_HasFeature(type, Py_TPFLAGS_MANAGED_DICT)) {
Py_RETURN_NONE;
}
PyDictOrValues dorv = *_PyObject_DictOrValuesPointer(obj);
if (!_PyDictOrValues_IsValues(dorv)) {
Py_RETURN_NONE;
}
PyDictValues *values = _PyDictOrValues_GetValues(dorv);
PyDictKeysObject *keys = ((PyHeapTypeObject *)type)->ht_cached_keys;
assert(keys != NULL);
int size = (int)keys->dk_nentries;
assert(size >= 0);
PyObject *res = PyTuple_New(size);
if (res == NULL) {
return NULL;
}
_Py_DECLARE_STR(anon_null, "<NULL>");
for(int i = 0; i < size; i++) {
PyObject *item = values->values[i];
if (item == NULL) {
item = &_Py_STR(anon_null);
}
else {
Py_INCREF(item);
}
PyTuple_SET_ITEM(res, i, item);
}
return res;
}


static PyMethodDef module_functions[] = {
{"get_configs", get_configs, METH_NOARGS},
Expand Down Expand Up @@ -1594,6 +1629,7 @@ static PyMethodDef module_functions[] = {
{"check_pyobject_uninitialized_is_freed",
check_pyobject_uninitialized_is_freed, METH_NOARGS},
{"pymem_getallocatorsname", test_pymem_getallocatorsname, METH_NOARGS},
{"get_object_dict_values", get_object_dict_values, METH_O},
{NULL, NULL} /* sentinel */
};

Expand Down
14 changes: 8 additions & 6 deletions Objects/dictobject.c
Original file line number Diff line number Diff line change
Expand Up @@ -5464,22 +5464,24 @@ _PyObject_MakeDictFromInstanceAttributes(PyObject *obj, PyDictValues *values)
return make_dict_from_instance_attributes(interp, keys, values);
}

// Return 1 if the dict was dematerialized, 0 otherwise.
int
// Return true if the dict was dematerialized, false otherwise.
bool
_PyObject_MakeInstanceAttributesFromDict(PyObject *obj, PyDictOrValues *dorv)
{
assert(_PyObject_DictOrValuesPointer(obj) == dorv);
assert(!_PyDictOrValues_IsValues(*dorv));
PyDictObject *dict = (PyDictObject *)_PyDictOrValues_GetDict(*dorv);
if (dict == NULL) {
return 0;
return false;
}
// It's likely that this dict still shares its keys (if it was materialized
// on request and not heavily modified):
assert(PyDict_CheckExact(dict));
if (!PyDict_CheckExact(dict)) {
return false;
}
assert(_PyType_HasFeature(Py_TYPE(obj), Py_TPFLAGS_HEAPTYPE));
if (dict->ma_keys != CACHED_KEYS(Py_TYPE(obj)) || Py_REFCNT(dict) != 1) {
return 0;
return false;
}
assert(dict->ma_values);
// We have an opportunity to do something *really* cool: dematerialize it!
Expand All @@ -5490,7 +5492,7 @@ _PyObject_MakeInstanceAttributesFromDict(PyObject *obj, PyDictOrValues *dorv)
dict->ma_keys = NULL;
dict->ma_values = NULL;
Py_DECREF(dict);
return 1;
return true;
}

int
Expand Down