From 0e308fea566ba121e48dc94d646accf41ff1fe88 Mon Sep 17 00:00:00 2001 From: Yannick Jadoul Date: Thu, 8 Oct 2020 21:25:34 +0200 Subject: [PATCH 1/7] Demonstrate test_factory_constructors.py failure without functional changes from #2335 --- include/pybind11/pybind11.h | 4 ++-- tests/test_class.cpp | 16 ---------------- tests/test_class.py | 9 --------- 3 files changed, 2 insertions(+), 27 deletions(-) diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index 51fb74a57f..b43cf05428 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -1028,7 +1028,7 @@ class generic_type : public object { PYBIND11_OBJECT_DEFAULT(generic_type, object, PyType_Check) protected: void initialize(const type_record &rec) { - if (rec.scope && hasattr(rec.scope, "__dict__") && rec.scope.attr("__dict__").contains(rec.name)) + if (rec.scope && hasattr(rec.scope, rec.name)) pybind11_fail("generic_type: cannot initialize type \"" + std::string(rec.name) + "\": an object with that name is already defined"); @@ -1946,7 +1946,7 @@ class exception : public object { std::string full_name = scope.attr("__name__").cast() + std::string(".") + name; m_ptr = PyErr_NewException(const_cast(full_name.c_str()), base.ptr(), NULL); - if (hasattr(scope, "__dict__") && scope.attr("__dict__").contains(name)) + if (hasattr(scope, name)) pybind11_fail("Error during initialization: multiple incompatible " "definitions with name \"" + std::string(name) + "\""); scope.attr(name) = *this; diff --git a/tests/test_class.cpp b/tests/test_class.cpp index 890fab736f..d6db6bbdca 100644 --- a/tests/test_class.cpp +++ b/tests/test_class.cpp @@ -429,22 +429,6 @@ TEST_SUBMODULE(class_, m) { py::class_(m, "Empty") .def(py::init<>()); - // test_base_and_derived_nested_scope - struct BaseWithNested { - struct Nested {}; - }; - - struct DerivedWithNested : BaseWithNested { - struct Nested {}; - }; - - py::class_ baseWithNested_class(m, "BaseWithNested"); - py::class_ derivedWithNested_class(m, "DerivedWithNested"); - py::class_(baseWithNested_class, "Nested") - .def_static("get_name", []() { return "BaseWithNested::Nested"; }); - py::class_(derivedWithNested_class, "Nested") - .def_static("get_name", []() { return "DerivedWithNested::Nested"; }); - // test_register_duplicate_class struct Duplicate {}; struct OtherDuplicate {}; diff --git a/tests/test_class.py b/tests/test_class.py index 10e4dd79b0..ab3dcbbfb4 100644 --- a/tests/test_class.py +++ b/tests/test_class.py @@ -385,15 +385,6 @@ def test_multiple_instances_with_same_pointer(capture): # and just completes without crashing, we're good. -# https://github.com/pybind/pybind11/issues/1624 -def test_base_and_derived_nested_scope(): - assert issubclass(m.DerivedWithNested, m.BaseWithNested) - assert m.BaseWithNested.Nested != m.DerivedWithNested.Nested - assert m.BaseWithNested.Nested.get_name() == "BaseWithNested::Nested" - assert m.DerivedWithNested.Nested.get_name() == "DerivedWithNested::Nested" - - -@pytest.mark.skip("See https://github.com/pybind/pybind11/pull/2564") def test_register_duplicate_class(): import types module_scope = types.ModuleType("module_scope") From 2d8a70cf9e6a52cd2b0123d8242465874f2e90d0 Mon Sep 17 00:00:00 2001 From: Yannick Jadoul Date: Fri, 9 Oct 2020 17:57:49 +0200 Subject: [PATCH 2/7] Revert "Demonstrate test_factory_constructors.py failure without functional changes from #2335" This reverts commit ca33a8021fc2a3617c3356b188796528f4594419. --- include/pybind11/pybind11.h | 4 ++-- tests/test_class.cpp | 16 ++++++++++++++++ tests/test_class.py | 8 ++++++++ 3 files changed, 26 insertions(+), 2 deletions(-) diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index b43cf05428..51fb74a57f 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -1028,7 +1028,7 @@ class generic_type : public object { PYBIND11_OBJECT_DEFAULT(generic_type, object, PyType_Check) protected: void initialize(const type_record &rec) { - if (rec.scope && hasattr(rec.scope, rec.name)) + if (rec.scope && hasattr(rec.scope, "__dict__") && rec.scope.attr("__dict__").contains(rec.name)) pybind11_fail("generic_type: cannot initialize type \"" + std::string(rec.name) + "\": an object with that name is already defined"); @@ -1946,7 +1946,7 @@ class exception : public object { std::string full_name = scope.attr("__name__").cast() + std::string(".") + name; m_ptr = PyErr_NewException(const_cast(full_name.c_str()), base.ptr(), NULL); - if (hasattr(scope, name)) + if (hasattr(scope, "__dict__") && scope.attr("__dict__").contains(name)) pybind11_fail("Error during initialization: multiple incompatible " "definitions with name \"" + std::string(name) + "\""); scope.attr(name) = *this; diff --git a/tests/test_class.cpp b/tests/test_class.cpp index d6db6bbdca..890fab736f 100644 --- a/tests/test_class.cpp +++ b/tests/test_class.cpp @@ -429,6 +429,22 @@ TEST_SUBMODULE(class_, m) { py::class_(m, "Empty") .def(py::init<>()); + // test_base_and_derived_nested_scope + struct BaseWithNested { + struct Nested {}; + }; + + struct DerivedWithNested : BaseWithNested { + struct Nested {}; + }; + + py::class_ baseWithNested_class(m, "BaseWithNested"); + py::class_ derivedWithNested_class(m, "DerivedWithNested"); + py::class_(baseWithNested_class, "Nested") + .def_static("get_name", []() { return "BaseWithNested::Nested"; }); + py::class_(derivedWithNested_class, "Nested") + .def_static("get_name", []() { return "DerivedWithNested::Nested"; }); + // test_register_duplicate_class struct Duplicate {}; struct OtherDuplicate {}; diff --git a/tests/test_class.py b/tests/test_class.py index ab3dcbbfb4..add0f6cc84 100644 --- a/tests/test_class.py +++ b/tests/test_class.py @@ -385,6 +385,14 @@ def test_multiple_instances_with_same_pointer(capture): # and just completes without crashing, we're good. +# https://github.com/pybind/pybind11/issues/1624 +def test_base_and_derived_nested_scope(): + assert issubclass(m.DerivedWithNested, m.BaseWithNested) + assert m.BaseWithNested.Nested != m.DerivedWithNested.Nested + assert m.BaseWithNested.Nested.get_name() == "BaseWithNested::Nested" + assert m.DerivedWithNested.Nested.get_name() == "DerivedWithNested::Nested" + + def test_register_duplicate_class(): import types module_scope = types.ModuleType("module_scope") From 55a99148965400f38b5d770301f0a3ce944f2cc8 Mon Sep 17 00:00:00 2001 From: Yannick Jadoul Date: Fri, 9 Oct 2020 18:40:50 +0200 Subject: [PATCH 3/7] Fix test crash where registered Python type gets garbage collected --- include/pybind11/pybind11.h | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index 51fb74a57f..8c8c95926f 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -1037,10 +1037,11 @@ class generic_type : public object { "\" is already registered!"); m_ptr = make_new_python_type(rec); + auto type = (PyTypeObject *) m_ptr; /* Register supplemental type information in C++ dict */ auto *tinfo = new detail::type_info(); - tinfo->type = (PyTypeObject *) m_ptr; + tinfo->type = type; tinfo->cpptype = rec.type; tinfo->type_size = rec.type_size; tinfo->type_align = rec.type_align; @@ -1060,7 +1061,11 @@ class generic_type : public object { registered_local_types_cpp()[tindex] = tinfo; else internals.registered_types_cpp[tindex] = tinfo; - internals.registered_types_py[(PyTypeObject *) m_ptr] = { tinfo }; + internals.registered_types_py[type] = { tinfo }; + weakref(m_ptr, cpp_function([type](handle wr) { + get_internals().registered_types_py.erase(type); + wr.dec_ref(); + })).release(); if (rec.bases.size() > 1 || rec.multiple_inheritance) { mark_parents_nonsimple(tinfo->type); From 704d6a5e6bd19fbab6c7474385f970f54d906f76 Mon Sep 17 00:00:00 2001 From: Yannick Jadoul Date: Tue, 13 Oct 2020 00:51:38 +0200 Subject: [PATCH 4/7] Clean up some more internal structures when class objects go out of scope --- include/pybind11/pybind11.h | 26 +++++++++++++++++++++++--- 1 file changed, 23 insertions(+), 3 deletions(-) diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index 8c8c95926f..5615602314 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -1056,14 +1056,34 @@ class generic_type : public object { auto &internals = get_internals(); auto tindex = std::type_index(*rec.type); + auto module_local = rec.module_local; tinfo->direct_conversions = &internals.direct_conversions[tindex]; - if (rec.module_local) + if (module_local) registered_local_types_cpp()[tindex] = tinfo; else internals.registered_types_cpp[tindex] = tinfo; internals.registered_types_py[type] = { tinfo }; - weakref(m_ptr, cpp_function([type](handle wr) { - get_internals().registered_types_py.erase(type); + + // Clean up our internals after the Python type object gets garbage collected + weakref(m_ptr, cpp_function([type, tindex, module_local](handle wr) { + auto &internals = get_internals(); + internals.direct_conversions.erase(tindex); + if (module_local) + registered_local_types_cpp().erase(tindex); + else + internals.registered_types_cpp.erase(tindex); + internals.registered_types_py.erase(type); + // C++20: std::erase_if(internals.inactive_override_cache, + // [type](const auto &entry) { + // return entry.first == (PyObject *) type; + // }); + auto &cache = internals.inactive_override_cache; + for (auto it = cache.begin(), last = cache.end(); it != last; ) { + if (it->first == (PyObject *) type) + it = cache.erase(it); + else + ++it; + } wr.dec_ref(); })).release(); From 35ce74776d0b98a839a2215d3f875b9760bb701c Mon Sep 17 00:00:00 2001 From: Yannick Jadoul Date: Thu, 15 Oct 2020 00:44:23 +0200 Subject: [PATCH 5/7] Reduce length of std::erase_if-in-C++20 comment --- include/pybind11/pybind11.h | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index 5615602314..75a12dcf43 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -1073,10 +1073,7 @@ class generic_type : public object { else internals.registered_types_cpp.erase(tindex); internals.registered_types_py.erase(type); - // C++20: std::erase_if(internals.inactive_override_cache, - // [type](const auto &entry) { - // return entry.first == (PyObject *) type; - // }); + // Actually just `std::erase_if`, but that's only available in C++20 auto &cache = internals.inactive_override_cache; for (auto it = cache.begin(), last = cache.end(); it != last; ) { if (it->first == (PyObject *) type) From 51978a845edbcc15d1758463de5d796848e31a09 Mon Sep 17 00:00:00 2001 From: Yannick Jadoul Date: Fri, 16 Oct 2020 00:02:37 +0200 Subject: [PATCH 6/7] Clean up code for cleaning up type internals --- include/pybind11/pybind11.h | 50 +++++++++++++++++++++---------------- 1 file changed, 29 insertions(+), 21 deletions(-) diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index 75a12dcf43..b0f3cc1ef6 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -1021,6 +1021,30 @@ inline dict globals() { } PYBIND11_NAMESPACE_BEGIN(detail) +/// Cleanup the type-info for a pybind11-registered type. +PYBIND11_NOINLINE inline void cleanup_type_info(detail::type_info *tinfo) { + auto &internals = get_internals(); + auto tindex = std::type_index(*tinfo->cpptype); + internals.direct_conversions.erase(tindex); + + if (tinfo->module_local) + registered_local_types_cpp().erase(tindex); + else + internals.registered_types_cpp.erase(tindex); + internals.registered_types_py.erase(tinfo->type); + + // Actually just `std::erase_if`, but that's only available in C++20 + auto &cache = internals.inactive_override_cache; + for (auto it = cache.begin(), last = cache.end(); it != last; ) { + if (it->first == (PyObject *) tinfo->type) + it = cache.erase(it); + else + ++it; + } + + delete tinfo; +} + /// Generic support for creating new Python heap types class generic_type : public object { template friend class class_; @@ -1037,11 +1061,10 @@ class generic_type : public object { "\" is already registered!"); m_ptr = make_new_python_type(rec); - auto type = (PyTypeObject *) m_ptr; /* Register supplemental type information in C++ dict */ auto *tinfo = new detail::type_info(); - tinfo->type = type; + tinfo->type = (PyTypeObject *) m_ptr; tinfo->cpptype = rec.type; tinfo->type_size = rec.type_size; tinfo->type_align = rec.type_align; @@ -1056,31 +1079,16 @@ class generic_type : public object { auto &internals = get_internals(); auto tindex = std::type_index(*rec.type); - auto module_local = rec.module_local; tinfo->direct_conversions = &internals.direct_conversions[tindex]; - if (module_local) + if (rec.module_local) registered_local_types_cpp()[tindex] = tinfo; else internals.registered_types_cpp[tindex] = tinfo; - internals.registered_types_py[type] = { tinfo }; + internals.registered_types_py[(PyTypeObject *) m_ptr] = { tinfo }; // Clean up our internals after the Python type object gets garbage collected - weakref(m_ptr, cpp_function([type, tindex, module_local](handle wr) { - auto &internals = get_internals(); - internals.direct_conversions.erase(tindex); - if (module_local) - registered_local_types_cpp().erase(tindex); - else - internals.registered_types_cpp.erase(tindex); - internals.registered_types_py.erase(type); - // Actually just `std::erase_if`, but that's only available in C++20 - auto &cache = internals.inactive_override_cache; - for (auto it = cache.begin(), last = cache.end(); it != last; ) { - if (it->first == (PyObject *) type) - it = cache.erase(it); - else - ++it; - } + weakref(m_ptr, cpp_function([tinfo](handle wr) { + cleanup_type_info(tinfo); wr.dec_ref(); })).release(); From ae96b37866bdb7a2e7f314681707d8407690a639 Mon Sep 17 00:00:00 2001 From: Yannick Jadoul Date: Fri, 16 Oct 2020 00:31:48 +0200 Subject: [PATCH 7/7] Move cleaning up of type info in internals to tp_dealloc on pybind11_metaclass --- include/pybind11/detail/class.h | 40 +++++++++++++++++++++++++++++++++ include/pybind11/pybind11.h | 30 ------------------------- 2 files changed, 40 insertions(+), 30 deletions(-) diff --git a/include/pybind11/detail/class.h b/include/pybind11/detail/class.h index 54a857388d..569c5415d6 100644 --- a/include/pybind11/detail/class.h +++ b/include/pybind11/detail/class.h @@ -193,6 +193,44 @@ extern "C" inline PyObject *pybind11_meta_call(PyObject *type, PyObject *args, P return self; } +/// Cleanup the type-info for a pybind11-registered type. +extern "C" inline void pybind11_meta_dealloc(PyObject *obj) { + auto *type = (PyTypeObject *) obj; + auto &internals = get_internals(); + + // A pybind11-registered type will: + // 1) be found in internals.registered_types_py + // 2) have exactly one associated `detail::type_info` + auto found_type = internals.registered_types_py.find(type); + if (found_type != internals.registered_types_py.end() && + found_type->second.size() == 1 && + found_type->second[0]->type == type) { + + auto *tinfo = found_type->second[0]; + auto tindex = std::type_index(*tinfo->cpptype); + internals.direct_conversions.erase(tindex); + + if (tinfo->module_local) + registered_local_types_cpp().erase(tindex); + else + internals.registered_types_cpp.erase(tindex); + internals.registered_types_py.erase(tinfo->type); + + // Actually just `std::erase_if`, but that's only available in C++20 + auto &cache = internals.inactive_override_cache; + for (auto it = cache.begin(), last = cache.end(); it != last; ) { + if (it->first == (PyObject *) tinfo->type) + it = cache.erase(it); + else + ++it; + } + + delete tinfo; + } + + PyType_Type.tp_dealloc(obj); +} + /** This metaclass is assigned by default to all pybind11 types and is required in order for static properties to function correctly. Users may override this using `py::metaclass`. Return value: New reference. */ @@ -225,6 +263,8 @@ inline PyTypeObject* make_default_metaclass() { type->tp_getattro = pybind11_meta_getattro; #endif + type->tp_dealloc = pybind11_meta_dealloc; + if (PyType_Ready(type) < 0) pybind11_fail("make_default_metaclass(): failure in PyType_Ready()!"); diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index b0f3cc1ef6..51fb74a57f 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -1021,30 +1021,6 @@ inline dict globals() { } PYBIND11_NAMESPACE_BEGIN(detail) -/// Cleanup the type-info for a pybind11-registered type. -PYBIND11_NOINLINE inline void cleanup_type_info(detail::type_info *tinfo) { - auto &internals = get_internals(); - auto tindex = std::type_index(*tinfo->cpptype); - internals.direct_conversions.erase(tindex); - - if (tinfo->module_local) - registered_local_types_cpp().erase(tindex); - else - internals.registered_types_cpp.erase(tindex); - internals.registered_types_py.erase(tinfo->type); - - // Actually just `std::erase_if`, but that's only available in C++20 - auto &cache = internals.inactive_override_cache; - for (auto it = cache.begin(), last = cache.end(); it != last; ) { - if (it->first == (PyObject *) tinfo->type) - it = cache.erase(it); - else - ++it; - } - - delete tinfo; -} - /// Generic support for creating new Python heap types class generic_type : public object { template friend class class_; @@ -1086,12 +1062,6 @@ class generic_type : public object { internals.registered_types_cpp[tindex] = tinfo; internals.registered_types_py[(PyTypeObject *) m_ptr] = { tinfo }; - // Clean up our internals after the Python type object gets garbage collected - weakref(m_ptr, cpp_function([tinfo](handle wr) { - cleanup_type_info(tinfo); - wr.dec_ref(); - })).release(); - if (rec.bases.size() > 1 || rec.multiple_inheritance) { mark_parents_nonsimple(tinfo->type); tinfo->simple_ancestors = false;