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

Author: markshannon

Include/internal/pycore_dict.h

@@ -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,

Include/internal/pycore_global_objects_fini_generated.h

@@ -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>")

Include/internal/pycore_global_strings.h

@@ -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):
@@ -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()

Include/internal/pycore_runtime_init_generated.h

@@ -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
@@ -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},
@@ -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 */
 };
 

Lib/test/test_opcache.py

@@ -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!
@@ -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