From 4fbf6646453e9cda0cb552d2093cb2119674216d Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Tue, 30 Jul 2024 12:04:13 -0600 Subject: [PATCH 1/8] Simplify the Fix For Builtin Types Slot Wrappers --- Include/internal/pycore_typeobject.h | 1 - Lib/test/test_embed.py | 1 - Objects/typeobject.c | 136 +++++++++++++++++++++------ 3 files changed, 107 insertions(+), 31 deletions(-) diff --git a/Include/internal/pycore_typeobject.h b/Include/internal/pycore_typeobject.h index df6bfef715dd34..32bd19d968b917 100644 --- a/Include/internal/pycore_typeobject.h +++ b/Include/internal/pycore_typeobject.h @@ -33,7 +33,6 @@ struct _types_runtime_state { struct { struct { PyTypeObject *type; - PyTypeObject def; int64_t interp_count; } types[_Py_MAX_MANAGED_STATIC_TYPES]; } managed_static; diff --git a/Lib/test/test_embed.py b/Lib/test/test_embed.py index 916a9a79887dfc..4083044e743324 100644 --- a/Lib/test/test_embed.py +++ b/Lib/test/test_embed.py @@ -416,7 +416,6 @@ def test_datetime_reset_strptime(self): out, err = self.run_embedded_interpreter("test_repeated_init_exec", code) self.assertEqual(out, '20000101\n' * INIT_LOOPS) - @unittest.skip('inheritance across re-init is currently broken; see gh-117482') def test_static_types_inherited_slots(self): script = textwrap.dedent(""" import test.support diff --git a/Objects/typeobject.c b/Objects/typeobject.c index 00f0dc9849b5c8..d4932cbeac8d41 100644 --- a/Objects/typeobject.c +++ b/Objects/typeobject.c @@ -314,15 +314,6 @@ managed_static_type_state_clear(PyInterpreterState *interp, PyTypeObject *self, } } -static PyTypeObject * -managed_static_type_get_def(PyTypeObject *self, int isbuiltin) -{ - size_t index = managed_static_type_index_get(self); - size_t full_index = isbuiltin - ? index - : index + _Py_MAX_MANAGED_STATIC_BUILTIN_TYPES; - return &_PyRuntime.types.managed_static.types[full_index].def; -} // Also see _PyStaticType_InitBuiltin() and _PyStaticType_FiniBuiltin(). @@ -5859,6 +5850,7 @@ fini_static_type(PyInterpreterState *interp, PyTypeObject *type, _PyStaticType_ClearWeakRefs(interp, type); managed_static_type_state_clear(interp, type, isbuiltin, final); + /* We leave _Py_TPFLAGS_STATIC_BUILTIN set on tp_flags. */ } void @@ -7871,7 +7863,7 @@ inherit_slots(PyTypeObject *type, PyTypeObject *base) return 0; } -static int add_operators(PyTypeObject *, PyTypeObject *); +static int add_operators(PyTypeObject *); static int add_tp_new_wrapper(PyTypeObject *type); #define COLLECTION_FLAGS (Py_TPFLAGS_SEQUENCE | Py_TPFLAGS_MAPPING) @@ -8036,10 +8028,10 @@ type_dict_set_doc(PyTypeObject *type) static int -type_ready_fill_dict(PyTypeObject *type, PyTypeObject *def) +type_ready_fill_dict(PyTypeObject *type) { /* Add type-specific descriptors to tp_dict */ - if (add_operators(type, def) < 0) { + if (add_operators(type) < 0) { return -1; } if (type_add_methods(type) < 0) { @@ -8358,7 +8350,7 @@ type_ready_post_checks(PyTypeObject *type) static int -type_ready(PyTypeObject *type, PyTypeObject *def, int initial) +type_ready(PyTypeObject *type, int initial) { ASSERT_TYPE_LOCK_HELD(); @@ -8397,7 +8389,7 @@ type_ready(PyTypeObject *type, PyTypeObject *def, int initial) if (type_ready_set_new(type, initial) < 0) { goto error; } - if (type_ready_fill_dict(type, def) < 0) { + if (type_ready_fill_dict(type) < 0) { goto error; } if (initial) { @@ -8454,7 +8446,7 @@ PyType_Ready(PyTypeObject *type) int res; BEGIN_TYPE_LOCK(); if (!(type->tp_flags & Py_TPFLAGS_READY)) { - res = type_ready(type, NULL, 1); + res = type_ready(type, 1); } else { res = 0; assert(_PyType_CheckConsistency(type)); @@ -8490,20 +8482,14 @@ init_static_type(PyInterpreterState *interp, PyTypeObject *self, managed_static_type_state_init(interp, self, isbuiltin, initial); - PyTypeObject *def = managed_static_type_get_def(self, isbuiltin); - if (initial) { - memcpy(def, self, sizeof(PyTypeObject)); - } - int res; BEGIN_TYPE_LOCK(); - res = type_ready(self, def, initial); + res = type_ready(self, initial); END_TYPE_LOCK(); if (res < 0) { _PyStaticType_ClearWeakRefs(interp, self); managed_static_type_state_clear(interp, self, isbuiltin, initial); } - return res; } @@ -11060,6 +11046,83 @@ recurse_down_subclasses(PyTypeObject *type, PyObject *attr_name, return 0; } +static int +expect_manually_inherited(PyTypeObject *type, void **slot) +{ + PyObject *typeobj = (PyObject *)type; + if (slot == (void *)&type->tp_init) { + /* This is a best-effort list of builtin exception types + that have their own tp_init function. */ + if (typeobj != PyExc_BaseException + && typeobj != PyExc_BaseExceptionGroup + && typeobj != PyExc_ImportError + && typeobj != PyExc_NameError + && typeobj != PyExc_OSError + && typeobj != PyExc_StopIteration + && typeobj != PyExc_SyntaxError + && typeobj != PyExc_UnicodeDecodeError + && typeobj != PyExc_UnicodeEncodeError + + && type != &PyBool_Type + && type != &PyMemoryView_Type + && type != &PyBytes_Type + && type != &PyComplex_Type + && type != &PyEnum_Type + && type != &PyFilter_Type + && type != &PyFloat_Type + && type != &PyFrozenSet_Type + && type != &PyLong_Type + && type != &PyMap_Type + && type != &PyRange_Type + && type != &PyReversed_Type + && type != &PySlice_Type + && type != &PyUnicode_Type + && type != &PyTuple_Type + && type != &PyZip_Type) + { + return 1; + } + } + else if (slot == (void *)&type->tp_str) { + /* This is a best-effort list of builtin exception types + that have their own tp_str function. */ + if (typeobj == PyExc_AttributeError || typeobj == PyExc_NameError) { + return 1; + } + } + else if (slot == (void *)&type->tp_getattr + || slot == (void *)&type->tp_getattro) + { + /* This is a best-effort list of builtin types + that have their own tp_getattr function. */ + if (typeobj == PyExc_BaseException + || type == &PyByteArray_Type + || type == &PyBytes_Type + || type == &PyComplex_Type + || type == &PyDict_Type + || type == &PyEnum_Type + || type == &PyFilter_Type + || type == &PyLong_Type + || type == &PyList_Type + || type == &PyMap_Type + || type == &PyMemoryView_Type + || type == &PyProperty_Type + || type == &PyRange_Type + || type == &PyReversed_Type + || type == &PySet_Type + || type == &PySlice_Type + || type == &PySuper_Type + || type == &PyTuple_Type + || type == &PyZip_Type) + { + return 1; + } + } + + /* It must be inherited (see type_ready_inherit()).. */ + return 0; +} + /* This function is called by PyType_Ready() to populate the type's dictionary with method descriptors for function slots. For each function slot (like tp_repr) that's defined in the type, one or more @@ -11091,24 +11154,39 @@ recurse_down_subclasses(PyTypeObject *type, PyObject *attr_name, infinite recursion here.) */ static int -add_operators(PyTypeObject *type, PyTypeObject *def) +add_operators(PyTypeObject *type) { PyObject *dict = lookup_tp_dict(type); pytype_slotdef *p; PyObject *descr; void **ptr; - assert(def == NULL || (type->tp_flags & _Py_TPFLAGS_STATIC_BUILTIN)); - if (def == NULL) { - def = type; - } - for (p = slotdefs; p->name; p++) { if (p->wrapper == NULL) continue; - ptr = slotptr(def, p->offset); + ptr = slotptr(type, p->offset); if (!ptr || !*ptr) continue; + if (type->tp_flags & _Py_TPFLAGS_STATIC_BUILTIN + && type->tp_base != NULL) + { + /* Also ignore when the type slot has been inherited. */ + void **ptr_base = slotptr(type->tp_base, p->offset); + if (ptr_base && *ptr == *ptr_base) { + /* Ideally we would always ignore any manually inherited + slots, Which would mean inheriting the slot wrapper + using normal attribute lookup rather than keeping + a distinct copy. However, that would introduce + a slight change in behavior that could break + existing code. + + In the meantime, look the other way when the definition + explicitly inherits the slot. */ + if (!expect_manually_inherited(type, ptr)) { + continue; + } + } + } int r = PyDict_Contains(dict, p->name_strobj); if (r > 0) continue; From 7d635f5b35b1954645364d7d7218bd55f39c4975 Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Tue, 30 Jul 2024 13:30:43 -0600 Subject: [PATCH 2/8] Fix tp_init. --- Objects/typeobject.c | 35 +---------------------------------- 1 file changed, 1 insertion(+), 34 deletions(-) diff --git a/Objects/typeobject.c b/Objects/typeobject.c index d4932cbeac8d41..e084db28c97494 100644 --- a/Objects/typeobject.c +++ b/Objects/typeobject.c @@ -11050,40 +11050,7 @@ static int expect_manually_inherited(PyTypeObject *type, void **slot) { PyObject *typeobj = (PyObject *)type; - if (slot == (void *)&type->tp_init) { - /* This is a best-effort list of builtin exception types - that have their own tp_init function. */ - if (typeobj != PyExc_BaseException - && typeobj != PyExc_BaseExceptionGroup - && typeobj != PyExc_ImportError - && typeobj != PyExc_NameError - && typeobj != PyExc_OSError - && typeobj != PyExc_StopIteration - && typeobj != PyExc_SyntaxError - && typeobj != PyExc_UnicodeDecodeError - && typeobj != PyExc_UnicodeEncodeError - - && type != &PyBool_Type - && type != &PyMemoryView_Type - && type != &PyBytes_Type - && type != &PyComplex_Type - && type != &PyEnum_Type - && type != &PyFilter_Type - && type != &PyFloat_Type - && type != &PyFrozenSet_Type - && type != &PyLong_Type - && type != &PyMap_Type - && type != &PyRange_Type - && type != &PyReversed_Type - && type != &PySlice_Type - && type != &PyUnicode_Type - && type != &PyTuple_Type - && type != &PyZip_Type) - { - return 1; - } - } - else if (slot == (void *)&type->tp_str) { + if (slot == (void *)&type->tp_str) { /* This is a best-effort list of builtin exception types that have their own tp_str function. */ if (typeobj == PyExc_AttributeError || typeobj == PyExc_NameError) { From 7fa8cebd6c79db7556c5f44174dc7090fccf2843 Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Mon, 12 Aug 2024 12:31:36 -0600 Subject: [PATCH 3/8] Revert "Fix tp_init." This reverts commit 7d635f5b35b1954645364d7d7218bd55f39c4975. --- Objects/typeobject.c | 35 ++++++++++++++++++++++++++++++++++- 1 file changed, 34 insertions(+), 1 deletion(-) diff --git a/Objects/typeobject.c b/Objects/typeobject.c index e084db28c97494..d4932cbeac8d41 100644 --- a/Objects/typeobject.c +++ b/Objects/typeobject.c @@ -11050,7 +11050,40 @@ static int expect_manually_inherited(PyTypeObject *type, void **slot) { PyObject *typeobj = (PyObject *)type; - if (slot == (void *)&type->tp_str) { + if (slot == (void *)&type->tp_init) { + /* This is a best-effort list of builtin exception types + that have their own tp_init function. */ + if (typeobj != PyExc_BaseException + && typeobj != PyExc_BaseExceptionGroup + && typeobj != PyExc_ImportError + && typeobj != PyExc_NameError + && typeobj != PyExc_OSError + && typeobj != PyExc_StopIteration + && typeobj != PyExc_SyntaxError + && typeobj != PyExc_UnicodeDecodeError + && typeobj != PyExc_UnicodeEncodeError + + && type != &PyBool_Type + && type != &PyMemoryView_Type + && type != &PyBytes_Type + && type != &PyComplex_Type + && type != &PyEnum_Type + && type != &PyFilter_Type + && type != &PyFloat_Type + && type != &PyFrozenSet_Type + && type != &PyLong_Type + && type != &PyMap_Type + && type != &PyRange_Type + && type != &PyReversed_Type + && type != &PySlice_Type + && type != &PyUnicode_Type + && type != &PyTuple_Type + && type != &PyZip_Type) + { + return 1; + } + } + else if (slot == (void *)&type->tp_str) { /* This is a best-effort list of builtin exception types that have their own tp_str function. */ if (typeobj == PyExc_AttributeError || typeobj == PyExc_NameError) { From b20d62768c6f81d18eeba50caeb76ad598373e5b Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Fri, 9 Aug 2024 12:25:35 -0600 Subject: [PATCH 4/8] Fix the tests. --- Lib/test/test_types.py | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/Lib/test/test_types.py b/Lib/test/test_types.py index 2ee46601f4adc6..3c9e33e3c9dbfc 100644 --- a/Lib/test/test_types.py +++ b/Lib/test/test_types.py @@ -2410,9 +2410,6 @@ def test_static_types_inherited_slots(self): def collate_results(raw): results = {} for cls, attr, wrapper in raw: - # XXX This should not be necessary. - if cls == repr(bool) and attr in self.NUMERIC_METHODS: - continue key = cls, attr assert key not in results, (results, key, wrapper) results[key] = wrapper @@ -2433,14 +2430,7 @@ def collate_results(raw): cls, attr = key with self.subTest(cls=cls, slotattr=attr): actual = interp_results.pop(key) - # XXX This should not be necessary. - if cls == "" and attr == '__len__': - continue self.assertEqual(actual, expected) - # XXX This should not be necessary. - interp_results = {k: v for k, v in interp_results.items() if k[1] != '__hash__'} - # XXX This should not be necessary. - interp_results.pop(("", '__getitem__'), None) self.maxDiff = None self.assertEqual(interp_results, {}) From 338914ba155316897517e04d5d28d0bb77ecb473 Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Mon, 12 Aug 2024 14:14:13 -0600 Subject: [PATCH 5/8] Factor out slot_inherited() and drop expect_manually_inherited(). --- Objects/typeobject.c | 45 ++++++++++++++++++++++---------------------- 1 file changed, 23 insertions(+), 22 deletions(-) diff --git a/Objects/typeobject.c b/Objects/typeobject.c index be246d3c4cd801..debe96eb02d8b1 100644 --- a/Objects/typeobject.c +++ b/Objects/typeobject.c @@ -11088,8 +11088,22 @@ recurse_down_subclasses(PyTypeObject *type, PyObject *attr_name, } static int -expect_manually_inherited(PyTypeObject *type, void **slot) +slot_inherited(PyTypeObject *type, pytype_slotdef *slotdef, void **slot) { + void **slot_base = slotptr(type->tp_base, slotdef->offset); + if (slot_base == NULL || *slot != *slot_base) { + return 0; + } + + /* Ideally we would always ignore any manually inherited + slots, Which would mean inheriting the slot wrapper + using normal attribute lookup rather than keeping + a distinct copy. However, that would introduce + a slight change in behavior that could break + existing code. + + In the meantime, look the other way when the definition + explicitly inherits the slot. */ PyObject *typeobj = (PyObject *)type; if (slot == (void *)&type->tp_init) { /* This is a best-effort list of builtin exception types @@ -11121,14 +11135,14 @@ expect_manually_inherited(PyTypeObject *type, void **slot) && type != &PyTuple_Type && type != &PyZip_Type) { - return 1; + return 0; } } else if (slot == (void *)&type->tp_str) { /* This is a best-effort list of builtin exception types that have their own tp_str function. */ if (typeobj == PyExc_AttributeError || typeobj == PyExc_NameError) { - return 1; + return 0; } } else if (slot == (void *)&type->tp_getattr @@ -11156,12 +11170,12 @@ expect_manually_inherited(PyTypeObject *type, void **slot) || type == &PyTuple_Type || type == &PyZip_Type) { - return 1; + return 0; } } /* It must be inherited (see type_ready_inherit()).. */ - return 0; + return 1; } /* This function is called by PyType_Ready() to populate the type's @@ -11208,25 +11222,12 @@ add_operators(PyTypeObject *type) ptr = slotptr(type, p->offset); if (!ptr || !*ptr) continue; + /* Also ignore when the type slot has been inherited. */ if (type->tp_flags & _Py_TPFLAGS_STATIC_BUILTIN - && type->tp_base != NULL) + && type->tp_base != NULL + && slot_inherited(type, p, ptr)) { - /* Also ignore when the type slot has been inherited. */ - void **ptr_base = slotptr(type->tp_base, p->offset); - if (ptr_base && *ptr == *ptr_base) { - /* Ideally we would always ignore any manually inherited - slots, Which would mean inheriting the slot wrapper - using normal attribute lookup rather than keeping - a distinct copy. However, that would introduce - a slight change in behavior that could break - existing code. - - In the meantime, look the other way when the definition - explicitly inherits the slot. */ - if (!expect_manually_inherited(type, ptr)) { - continue; - } - } + continue; } int r = PyDict_Contains(dict, p->name_strobj); if (r > 0) From d8dcbe71f720ac9499b87989abdbd7baf8be1a08 Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Mon, 12 Aug 2024 16:17:49 -0600 Subject: [PATCH 6/8] Drop the special-casing for explicitly inherited slots. --- Lib/test/test_doctest/test_doctest.py | 2 +- Objects/typeobject.c | 83 ++------------------------- 2 files changed, 6 insertions(+), 79 deletions(-) diff --git a/Lib/test/test_doctest/test_doctest.py b/Lib/test/test_doctest/test_doctest.py index 3e93a3bba283c2..171412cb7cb08c 100644 --- a/Lib/test/test_doctest/test_doctest.py +++ b/Lib/test/test_doctest/test_doctest.py @@ -738,7 +738,7 @@ def non_Python_modules(): r""" >>> import builtins >>> tests = doctest.DocTestFinder().find(builtins) - >>> 830 < len(tests) < 860 # approximate number of objects with docstrings + >>> 750 < len(tests) < 800 # approximate number of objects with docstrings True >>> real_tests = [t for t in tests if len(t.examples) > 0] >>> len(real_tests) # objects that actually have doctests diff --git a/Objects/typeobject.c b/Objects/typeobject.c index debe96eb02d8b1..e14aa6994e0e9f 100644 --- a/Objects/typeobject.c +++ b/Objects/typeobject.c @@ -11094,87 +11094,14 @@ slot_inherited(PyTypeObject *type, pytype_slotdef *slotdef, void **slot) if (slot_base == NULL || *slot != *slot_base) { return 0; } - - /* Ideally we would always ignore any manually inherited - slots, Which would mean inheriting the slot wrapper - using normal attribute lookup rather than keeping - a distinct copy. However, that would introduce - a slight change in behavior that could break - existing code. - - In the meantime, look the other way when the definition - explicitly inherits the slot. */ - PyObject *typeobj = (PyObject *)type; - if (slot == (void *)&type->tp_init) { - /* This is a best-effort list of builtin exception types - that have their own tp_init function. */ - if (typeobj != PyExc_BaseException - && typeobj != PyExc_BaseExceptionGroup - && typeobj != PyExc_ImportError - && typeobj != PyExc_NameError - && typeobj != PyExc_OSError - && typeobj != PyExc_StopIteration - && typeobj != PyExc_SyntaxError - && typeobj != PyExc_UnicodeDecodeError - && typeobj != PyExc_UnicodeEncodeError - - && type != &PyBool_Type - && type != &PyMemoryView_Type - && type != &PyBytes_Type - && type != &PyComplex_Type - && type != &PyEnum_Type - && type != &PyFilter_Type - && type != &PyFloat_Type - && type != &PyFrozenSet_Type - && type != &PyLong_Type - && type != &PyMap_Type - && type != &PyRange_Type - && type != &PyReversed_Type - && type != &PySlice_Type - && type != &PyUnicode_Type - && type != &PyTuple_Type - && type != &PyZip_Type) - { - return 0; - } + if (slot == (void *)&type->tp_hash) { + return (type->tp_richcompare == type->tp_base->tp_richcompare); } - else if (slot == (void *)&type->tp_str) { - /* This is a best-effort list of builtin exception types - that have their own tp_str function. */ - if (typeobj == PyExc_AttributeError || typeobj == PyExc_NameError) { - return 0; - } - } - else if (slot == (void *)&type->tp_getattr - || slot == (void *)&type->tp_getattro) - { - /* This is a best-effort list of builtin types - that have their own tp_getattr function. */ - if (typeobj == PyExc_BaseException - || type == &PyByteArray_Type - || type == &PyBytes_Type - || type == &PyComplex_Type - || type == &PyDict_Type - || type == &PyEnum_Type - || type == &PyFilter_Type - || type == &PyLong_Type - || type == &PyList_Type - || type == &PyMap_Type - || type == &PyMemoryView_Type - || type == &PyProperty_Type - || type == &PyRange_Type - || type == &PyReversed_Type - || type == &PySet_Type - || type == &PySlice_Type - || type == &PySuper_Type - || type == &PyTuple_Type - || type == &PyZip_Type) - { - return 0; - } + else if (slot == (void *)&type->tp_richcompare) { + return (type->tp_hash == type->tp_base->tp_hash); } - /* It must be inherited (see type_ready_inherit()).. */ + /* It must be inherited (see type_ready_inherit()). */ return 1; } From 71990239b72b465bb1adb461cb1144dbfabc4dd4 Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Tue, 13 Aug 2024 10:11:17 -0600 Subject: [PATCH 7/8] Add a comment. --- Objects/typeobject.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Objects/typeobject.c b/Objects/typeobject.c index e14aa6994e0e9f..0d3737f953e0d1 100644 --- a/Objects/typeobject.c +++ b/Objects/typeobject.c @@ -11094,6 +11094,8 @@ slot_inherited(PyTypeObject *type, pytype_slotdef *slotdef, void **slot) if (slot_base == NULL || *slot != *slot_base) { return 0; } + + /* Some slots are inherited in pairs. */ if (slot == (void *)&type->tp_hash) { return (type->tp_richcompare == type->tp_base->tp_richcompare); } From 7c3903cf43c532e0efc2b67dd41ff8986cb5a6ae Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Tue, 13 Aug 2024 11:13:36 -0600 Subject: [PATCH 8/8] Fix a forward declaration. --- Objects/typeobject.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Objects/typeobject.c b/Objects/typeobject.c index 0d3737f953e0d1..eff2be0065aa4b 100644 --- a/Objects/typeobject.c +++ b/Objects/typeobject.c @@ -7886,7 +7886,7 @@ inherit_slots(PyTypeObject *type, PyTypeObject *base) return 0; } -static int add_operators(PyTypeObject *); +static int add_operators(PyTypeObject *type); static int add_tp_new_wrapper(PyTypeObject *type); #define COLLECTION_FLAGS (Py_TPFLAGS_SEQUENCE | Py_TPFLAGS_MAPPING)