From c9a56ac21bd830304df990d8d9667cd7e3b53f90 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Tue, 25 Jul 2023 16:15:39 -0700 Subject: [PATCH 01/27] Equivalent of https://github.com/google/clif/commit/5718e4d0807fd3b6a8187dde140069120b81ecef --- tests/CMakeLists.txt | 1 + tests/test_python_multiple_inheritance.cpp | 45 ++++++++++++++++++++++ tests/test_python_multiple_inheritance.py | 39 +++++++++++++++++++ 3 files changed, 85 insertions(+) create mode 100644 tests/test_python_multiple_inheritance.cpp create mode 100644 tests/test_python_multiple_inheritance.py diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 80ee9c1f11..d68067df6d 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -144,6 +144,7 @@ set(PYBIND11_TEST_FILES test_opaque_types test_operator_overloading test_pickling + test_python_multiple_inheritance test_pytypes test_sequences_and_iterators test_smart_ptr diff --git a/tests/test_python_multiple_inheritance.cpp b/tests/test_python_multiple_inheritance.cpp new file mode 100644 index 0000000000..e3a66fb842 --- /dev/null +++ b/tests/test_python_multiple_inheritance.cpp @@ -0,0 +1,45 @@ +#include "pybind11_tests.h" + +namespace test_python_multiple_inheritance { + +// Copied from: +// https://github.com/google/clif/blob/5718e4d0807fd3b6a8187dde140069120b81ecef/clif/testing/python_multiple_inheritance.h + +struct CppBase { + CppBase(int value) : base_value(value) {} + int get_base_value() const { return base_value; } + void reset_base_value(int new_value) { base_value = new_value; } + +private: + int base_value; +}; + +struct CppDrvd : CppBase { + CppDrvd(int value) : CppBase(value), drvd_value(value * 3) {} + int get_drvd_value() const { return drvd_value; } + void reset_drvd_value(int new_value) { drvd_value = new_value; } + + int get_base_value_from_drvd() const { return get_base_value(); } + void reset_base_value_from_drvd(int new_value) { reset_base_value(new_value); } + +private: + int drvd_value; +}; + +} // namespace test_python_multiple_inheritance + +TEST_SUBMODULE(python_multiple_inheritance, m) { + using namespace test_python_multiple_inheritance; + + py::class_(m, "CppBase") + .def(py::init()) + .def("get_base_value", &CppBase::get_base_value) + .def("reset_base_value", &CppBase::reset_base_value); + + py::class_(m, "CppDrvd") + .def(py::init()) + .def("get_drvd_value", &CppDrvd::get_drvd_value) + .def("reset_drvd_value", &CppDrvd::reset_drvd_value) + .def("get_base_value_from_drvd", &CppDrvd::get_base_value_from_drvd) + .def("reset_base_value_from_drvd", &CppDrvd::reset_base_value_from_drvd); +} diff --git a/tests/test_python_multiple_inheritance.py b/tests/test_python_multiple_inheritance.py new file mode 100644 index 0000000000..57c196030b --- /dev/null +++ b/tests/test_python_multiple_inheritance.py @@ -0,0 +1,39 @@ +# Adapted from: +# https://github.com/google/clif/blob/5718e4d0807fd3b6a8187dde140069120b81ecef/clif/testing/python/python_multiple_inheritance_test.py + +from pybind11_tests import python_multiple_inheritance as m + + +class PC(m.CppBase): + pass + + +class PPCCInit(PC, m.CppDrvd): + def __init__(self, value): + PC.__init__(self, value) + m.CppDrvd.__init__(self, value + 1) + + +def test_PC(): + d = PC(11) + assert d.get_base_value() == 11 + d.reset_base_value(13) + assert d.get_base_value() == 13 + + +def test_PPCCInit(): + d = PPCCInit(11) + assert d.get_drvd_value() == 36 + d.reset_drvd_value(55) + assert d.get_drvd_value() == 55 + + # CppBase is initialized and used when CppBase methods are called, but + # CppDrvd is used when CppDrvd methods are called. + assert d.get_base_value() == 11 + assert d.get_base_value_from_drvd() == 12 + d.reset_base_value(20) + assert d.get_base_value() == 20 + assert d.get_base_value_from_drvd() == 12 + d.reset_base_value_from_drvd(30) + assert d.get_base_value() == 20 + assert d.get_base_value_from_drvd() == 30 From bdd938ad7e0e52eb749e3e58555328f6f595d815 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Tue, 25 Jul 2023 21:00:38 -0700 Subject: [PATCH 02/27] Resolve clang-tidy errors. --- tests/test_python_multiple_inheritance.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/test_python_multiple_inheritance.cpp b/tests/test_python_multiple_inheritance.cpp index e3a66fb842..6899171585 100644 --- a/tests/test_python_multiple_inheritance.cpp +++ b/tests/test_python_multiple_inheritance.cpp @@ -6,7 +6,7 @@ namespace test_python_multiple_inheritance { // https://github.com/google/clif/blob/5718e4d0807fd3b6a8187dde140069120b81ecef/clif/testing/python_multiple_inheritance.h struct CppBase { - CppBase(int value) : base_value(value) {} + explicit CppBase(int value) : base_value(value) {} int get_base_value() const { return base_value; } void reset_base_value(int new_value) { base_value = new_value; } @@ -15,7 +15,7 @@ struct CppBase { }; struct CppDrvd : CppBase { - CppDrvd(int value) : CppBase(value), drvd_value(value * 3) {} + explicit CppDrvd(int value) : CppBase(value), drvd_value(value * 3) {} int get_drvd_value() const { return drvd_value; } void reset_drvd_value(int new_value) { drvd_value = new_value; } From 61f75293050e12dd396c0dd183c8c2dc5033bc27 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Wed, 26 Jul 2023 11:51:01 -0700 Subject: [PATCH 03/27] Moving test_PPCCInit() first changes the behavior! --- tests/test_python_multiple_inheritance.py | 23 +++++++++++------------ 1 file changed, 11 insertions(+), 12 deletions(-) diff --git a/tests/test_python_multiple_inheritance.py b/tests/test_python_multiple_inheritance.py index 57c196030b..cdd34d9ceb 100644 --- a/tests/test_python_multiple_inheritance.py +++ b/tests/test_python_multiple_inheritance.py @@ -14,26 +14,25 @@ def __init__(self, value): m.CppDrvd.__init__(self, value + 1) -def test_PC(): - d = PC(11) - assert d.get_base_value() == 11 - d.reset_base_value(13) - assert d.get_base_value() == 13 - - +# Moving this test after test_PC() changes the behavior! def test_PPCCInit(): d = PPCCInit(11) assert d.get_drvd_value() == 36 d.reset_drvd_value(55) assert d.get_drvd_value() == 55 - # CppBase is initialized and used when CppBase methods are called, but - # CppDrvd is used when CppDrvd methods are called. - assert d.get_base_value() == 11 + assert d.get_base_value() == 12 assert d.get_base_value_from_drvd() == 12 d.reset_base_value(20) assert d.get_base_value() == 20 - assert d.get_base_value_from_drvd() == 12 + assert d.get_base_value_from_drvd() == 20 d.reset_base_value_from_drvd(30) - assert d.get_base_value() == 20 + assert d.get_base_value() == 30 assert d.get_base_value_from_drvd() == 30 + + +def test_PC(): + d = PC(11) + assert d.get_base_value() == 11 + d.reset_base_value(13) + assert d.get_base_value() == 13 From 1fb7dc3c649b4b7c446258a2a8c28ad90a2bad60 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Wed, 26 Jul 2023 12:22:55 -0700 Subject: [PATCH 04/27] Resolve new Clang dev C++11 errors: ``` The CXX compiler identification is Clang 17.0.0 ``` ``` pytypes.h:1615:23: error: identifier '_s' preceded by whitespace in a literal operator declaration is deprecated [-Werror,-Wdeprecated-literal-operator] ``` ``` cast.h:1380:26: error: identifier '_a' preceded by whitespace in a literal operator declaration is deprecated [-Werror,-Wdeprecated-literal-operator] ``` --- include/pybind11/cast.h | 2 +- include/pybind11/pytypes.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h index db39341180..0ce9c5faa3 100644 --- a/include/pybind11/cast.h +++ b/include/pybind11/cast.h @@ -1377,7 +1377,7 @@ inline namespace literals { /** \rst String literal version of `arg` \endrst */ -constexpr arg operator"" _a(const char *name, size_t) { return arg(name); } +constexpr arg operator""_a(const char *name, size_t) { return arg(name); } } // namespace literals PYBIND11_NAMESPACE_BEGIN(detail) diff --git a/include/pybind11/pytypes.h b/include/pybind11/pytypes.h index 64aad63476..6615063100 100644 --- a/include/pybind11/pytypes.h +++ b/include/pybind11/pytypes.h @@ -1612,7 +1612,7 @@ inline namespace literals { /** \rst String literal version of `str` \endrst */ -inline str operator"" _s(const char *s, size_t size) { return {s, size}; } +inline str operator""_s(const char *s, size_t size) { return {s, size}; } } // namespace literals /// \addtogroup pytypes From b9f33801308dc3f32dcc02103706a8413824496e Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Wed, 26 Jul 2023 13:16:42 -0700 Subject: [PATCH 05/27] Resolve gcc 4.8.5 error: ``` pytypes.h:1615:12: error: missing space between '""' and suffix identifier ``` --- include/pybind11/cast.h | 10 +++++++++- include/pybind11/pytypes.h | 10 +++++++++- 2 files changed, 18 insertions(+), 2 deletions(-) diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h index 0ce9c5faa3..8be4d12d68 100644 --- a/include/pybind11/cast.h +++ b/include/pybind11/cast.h @@ -1377,7 +1377,15 @@ inline namespace literals { /** \rst String literal version of `arg` \endrst */ -constexpr arg operator""_a(const char *name, size_t) { return arg(name); } +constexpr arg +#if defined(__GNUC__) && __GNUC__ < 5 +operator"" _a // gcc 4.8.5 insists on having a space (hard error). +#else +operator""_a // clang 17 generates a deprecation warning if there is a space. +#endif + (const char *name, size_t) { + return arg(name); +} } // namespace literals PYBIND11_NAMESPACE_BEGIN(detail) diff --git a/include/pybind11/pytypes.h b/include/pybind11/pytypes.h index 6615063100..394f571288 100644 --- a/include/pybind11/pytypes.h +++ b/include/pybind11/pytypes.h @@ -1612,7 +1612,15 @@ inline namespace literals { /** \rst String literal version of `str` \endrst */ -inline str operator""_s(const char *s, size_t size) { return {s, size}; } +inline str +#if defined(__GNUC__) && __GNUC__ < 5 +operator"" _s // gcc 4.8.5 insists on having a space (hard error). +#else +operator""_s // clang 17 generates a deprecation warning if there is a space. +#endif + (const char *s, size_t size) { + return {s, size}; +} } // namespace literals /// \addtogroup pytypes From 8ea1b8cda79258d32ef4239ecd595cb503a230d7 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Wed, 26 Jul 2023 13:28:49 -0700 Subject: [PATCH 06/27] Specifically exclude `__clang__` --- include/pybind11/cast.h | 2 +- include/pybind11/pytypes.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h index 8be4d12d68..b3c8ebe17e 100644 --- a/include/pybind11/cast.h +++ b/include/pybind11/cast.h @@ -1378,7 +1378,7 @@ inline namespace literals { String literal version of `arg` \endrst */ constexpr arg -#if defined(__GNUC__) && __GNUC__ < 5 +#if !defined(__clang__) && defined(__GNUC__) && __GNUC__ < 5 operator"" _a // gcc 4.8.5 insists on having a space (hard error). #else operator""_a // clang 17 generates a deprecation warning if there is a space. diff --git a/include/pybind11/pytypes.h b/include/pybind11/pytypes.h index 394f571288..580a4658ef 100644 --- a/include/pybind11/pytypes.h +++ b/include/pybind11/pytypes.h @@ -1613,7 +1613,7 @@ inline namespace literals { String literal version of `str` \endrst */ inline str -#if defined(__GNUC__) && __GNUC__ < 5 +#if !defined(__clang__) && defined(__GNUC__) && __GNUC__ < 5 operator"" _s // gcc 4.8.5 insists on having a space (hard error). #else operator""_s // clang 17 generates a deprecation warning if there is a space. From 1d4f9ff2632b32ddcb0dc7ecd0ab7a4ce4c15a4e Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Thu, 27 Jul 2023 13:04:03 -0700 Subject: [PATCH 07/27] Snapshot of debugging code (does NOT pass pre-commit checks). --- include/pybind11/detail/class.h | 6 +++- include/pybind11/detail/type_caster_base.h | 27 ++++++++++++--- include/pybind11/pybind11.h | 2 ++ tests/test_python_multiple_inheritance.py | 40 ++++++++++++++++++++-- 4 files changed, 68 insertions(+), 7 deletions(-) diff --git a/include/pybind11/detail/class.h b/include/pybind11/detail/class.h index bc2b40c50a..057a867bb7 100644 --- a/include/pybind11/detail/class.h +++ b/include/pybind11/detail/class.h @@ -184,6 +184,7 @@ extern "C" inline PyObject *pybind11_meta_getattro(PyObject *obj, PyObject *name extern "C" inline PyObject *pybind11_meta_call(PyObject *type, PyObject *args, PyObject *kwargs) { // use the default metaclass call to create/initialize the object +printf("\nLOOOK %s:%d\n", __FILE__, __LINE__); fflush(stdout); PyObject *self = PyType_Type.tp_call(type, args, kwargs); if (self == nullptr) { return nullptr; @@ -366,7 +367,10 @@ inline PyObject *make_new_instance(PyTypeObject *type) { /// Instance creation function for all pybind11 types. It only allocates space for the /// C++ object, but doesn't call the constructor -- an `__init__` function must do that. extern "C" inline PyObject *pybind11_object_new(PyTypeObject *type, PyObject *, PyObject *) { - return make_new_instance(type); +printf("\nLOOOK [pybind11_object_new (called via tp_new) %s:%d\n", __FILE__, __LINE__); fflush(stdout); + PyObject *retval = make_new_instance(type); +printf("\nLOOOK ]pybind11_object_new (called via tp_new) %s:%d\n", __FILE__, __LINE__); fflush(stdout); + return retval; } /// An `__init__` function constructs the C++ object. Users should provide at least one diff --git a/include/pybind11/detail/type_caster_base.h b/include/pybind11/detail/type_caster_base.h index 16387506cf..5e182cb4dd 100644 --- a/include/pybind11/detail/type_caster_base.h +++ b/include/pybind11/detail/type_caster_base.h @@ -104,14 +104,16 @@ all_type_info_get_cache(PyTypeObject *type); // Populates a just-created cache entry. PYBIND11_NOINLINE void all_type_info_populate(PyTypeObject *t, std::vector &bases) { +printf("\nLOOOK all_type_info_populate[ %s:%d\n", __FILE__, __LINE__); fflush(stdout); std::vector check; for (handle parent : reinterpret_borrow(t->tp_bases)) { check.push_back((PyTypeObject *) parent.ptr()); } +printf("\nLOOOK:REG:POP tp_name=%s check.size()=%lu %s:%d\n", t->tp_name, (unsigned long) check.size(), __FILE__, __LINE__); fflush(stdout); auto const &type_dict = get_internals().registered_types_py; for (size_t i = 0; i < check.size(); i++) { - auto *type = check[i]; + PyTypeObject *type = check[i]; // Ignore Python2 old-style class super type: if (!PyType_Check((PyObject *) type)) { continue; @@ -119,23 +121,25 @@ PYBIND11_NOINLINE void all_type_info_populate(PyTypeObject *t, std::vectortp_name, (it != type_dict.end() ? "yes" : "no"), __FILE__, __LINE__); fflush(stdout); if (it != type_dict.end()) { // We found a cache entry for it, so it's either pybind-registered or has pre-computed // pybind bases, but we have to make sure we haven't already seen the type(s) before: // we want to follow Python/virtual C++ rules that there should only be one instance of // a common base. - for (auto *tinfo : it->second) { + for (type_info *tinfo : it->second) { // NB: Could use a second set here, rather than doing a linear search, but since // having a large number of immediate pybind11-registered types seems fairly // unlikely, that probably isn't worthwhile. bool found = false; - for (auto *known : bases) { + for (type_info *known : bases) { if (known == tinfo) { found = true; break; } } if (!found) { +printf("\nLOOOK:REG:ADD bases.push_back(tinfo) %s %s:%d\n", tinfo->cpptype->name(), __FILE__, __LINE__); fflush(stdout); bases.push_back(tinfo); } } @@ -154,6 +158,7 @@ PYBIND11_NOINLINE void all_type_info_populate(PyTypeObject *t, std::vectorsimple_layout ? inst->simple_value_holder - : &inst->nonsimple.values_and_holders[vpos]} {} + : &inst->nonsimple.values_and_holders[vpos]} { +if (type && type->cpptype) { +const char *nm = type->cpptype->name(); +if (strcmp(nm, "N32test_python_multiple_inheritance7CppBaseE") == 0 || strcmp(nm, "N32test_python_multiple_inheritance7CppDrvdE") == 0) { +printf("\nLOOOK %s value_and_holder ctor %s:%d\n", nm, __FILE__, __LINE__); fflush(stdout); +} +} + } // Default constructor (used to signal a value-and-holder not found by get_value_and_holder()) value_and_holder() = default; @@ -286,11 +298,18 @@ struct value_and_holder { } // NOLINTNEXTLINE(readability-make-member-function-const) void set_holder_constructed(bool v = true) { +//printf("\nLOOOK set_holder_constructed inst=%lu %s %s:%d\n", reinterpret_cast(inst), type->cpptype->name(), __FILE__, __LINE__); fflush(stdout); +if (strcmp("N32test_python_multiple_inheritance7CppBaseE", type->cpptype->name()) == 0) { + //long *BAD = nullptr; *BAD = 101; +} if (inst->simple_layout) { +//printf("\nLOOOK %s set_holder_constructed simple_layout %s:%d\n", type->cpptype->name(), __FILE__, __LINE__); fflush(stdout); inst->simple_holder_constructed = v; } else if (v) { +//printf("\nLOOOK %s set_holder_constructed v %s:%d\n", type->cpptype->name(), __FILE__, __LINE__); fflush(stdout); inst->nonsimple.status[index] |= instance::status_holder_constructed; } else { +//printf("\nLOOOK %s set_holder_constructed not v %s:%d\n", type->cpptype->name(), __FILE__, __LINE__); fflush(stdout); inst->nonsimple.status[index] &= (std::uint8_t) ~instance::status_holder_constructed; } } diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index 3bce1a01ba..1acde6f461 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -1142,8 +1142,10 @@ class cpp_function : public function { return nullptr; } if (overloads->is_constructor && !self_value_and_holder.holder_constructed()) { +printf("\nLOOOK %s BEFORE self_value_and_holder.type->init_instance %s:%d\n", self_value_and_holder.type->cpptype->name(), __FILE__, __LINE__); fflush(stdout); auto *pi = reinterpret_cast(parent.ptr()); self_value_and_holder.type->init_instance(pi, nullptr); +printf("\nLOOOK %s AFTER self_value_and_holder.type->init_instance %s:%d\n", self_value_and_holder.type->cpptype->name(), __FILE__, __LINE__); fflush(stdout); } return result.ptr(); } diff --git a/tests/test_python_multiple_inheritance.py b/tests/test_python_multiple_inheritance.py index cdd34d9ceb..035a14b9ff 100644 --- a/tests/test_python_multiple_inheritance.py +++ b/tests/test_python_multiple_inheritance.py @@ -10,13 +10,28 @@ class PC(m.CppBase): class PPCCInit(PC, m.CppDrvd): def __init__(self, value): + print("\nLOOOK PPCCInit PC", flush=True) PC.__init__(self, value) + print("\nLOOOK PPCCInit CppDrvd", flush=True) m.CppDrvd.__init__(self, value + 1) + print("\nLOOOK PPCCInit Done", flush=True) + + +def NOtest_PC_AAA(): + print("\nLOOOK BEFORE PC(11) AAA", flush=True) + d = PC(11) + print("\nLOOOK AFTER PC(11) AAA", flush=True) + assert d.get_base_value() == 11 + d.reset_base_value(13) + assert d.get_base_value() == 13 # Moving this test after test_PC() changes the behavior! -def test_PPCCInit(): +def test_PPCCInit_BBB(): + print("\nLOOOK BEFORE PPCCInit(11) BBB", flush=True) d = PPCCInit(11) + print("\nLOOOK AFTER PPCCInit(11) BBB", flush=True) + print("\nLOOOK", flush=True) assert d.get_drvd_value() == 36 d.reset_drvd_value(55) assert d.get_drvd_value() == 55 @@ -31,8 +46,29 @@ def test_PPCCInit(): assert d.get_base_value_from_drvd() == 30 -def test_PC(): +def NOtest_PC_CCC(): + print("\nLOOOK BEFORE PC(11) CCC", flush=True) d = PC(11) + print("\nLOOOK AFTER PC(11) CCC", flush=True) assert d.get_base_value() == 11 d.reset_base_value(13) assert d.get_base_value() == 13 + +# Moving this test after test_PC() changes the behavior! +def NOtest_PPCCInit_DDD(): + print("\nLOOOK BEFORE PPCCInit(11) DDD", flush=True) + d = PPCCInit(11) + print("\nLOOOK AFTER PPCCInit(11) DDD", flush=True) + print("\nLOOOK", flush=True) + assert d.get_drvd_value() == 36 + d.reset_drvd_value(55) + assert d.get_drvd_value() == 55 + + assert d.get_base_value() == 12 + assert d.get_base_value_from_drvd() == 12 + d.reset_base_value(20) + assert d.get_base_value() == 20 + assert d.get_base_value_from_drvd() == 20 + d.reset_base_value_from_drvd(30) + assert d.get_base_value() == 30 + assert d.get_base_value_from_drvd() == 30 From 775aab5722bc8c64a6078b1d0cb8f2ce9e159dcb Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Thu, 27 Jul 2023 13:04:23 -0700 Subject: [PATCH 08/27] Revert "Snapshot of debugging code (does NOT pass pre-commit checks)." This reverts commit 1d4f9ff2632b32ddcb0dc7ecd0ab7a4ce4c15a4e. --- include/pybind11/detail/class.h | 6 +--- include/pybind11/detail/type_caster_base.h | 27 +++------------ include/pybind11/pybind11.h | 2 -- tests/test_python_multiple_inheritance.py | 40 ++-------------------- 4 files changed, 7 insertions(+), 68 deletions(-) diff --git a/include/pybind11/detail/class.h b/include/pybind11/detail/class.h index 057a867bb7..bc2b40c50a 100644 --- a/include/pybind11/detail/class.h +++ b/include/pybind11/detail/class.h @@ -184,7 +184,6 @@ extern "C" inline PyObject *pybind11_meta_getattro(PyObject *obj, PyObject *name extern "C" inline PyObject *pybind11_meta_call(PyObject *type, PyObject *args, PyObject *kwargs) { // use the default metaclass call to create/initialize the object -printf("\nLOOOK %s:%d\n", __FILE__, __LINE__); fflush(stdout); PyObject *self = PyType_Type.tp_call(type, args, kwargs); if (self == nullptr) { return nullptr; @@ -367,10 +366,7 @@ inline PyObject *make_new_instance(PyTypeObject *type) { /// Instance creation function for all pybind11 types. It only allocates space for the /// C++ object, but doesn't call the constructor -- an `__init__` function must do that. extern "C" inline PyObject *pybind11_object_new(PyTypeObject *type, PyObject *, PyObject *) { -printf("\nLOOOK [pybind11_object_new (called via tp_new) %s:%d\n", __FILE__, __LINE__); fflush(stdout); - PyObject *retval = make_new_instance(type); -printf("\nLOOOK ]pybind11_object_new (called via tp_new) %s:%d\n", __FILE__, __LINE__); fflush(stdout); - return retval; + return make_new_instance(type); } /// An `__init__` function constructs the C++ object. Users should provide at least one diff --git a/include/pybind11/detail/type_caster_base.h b/include/pybind11/detail/type_caster_base.h index 5e182cb4dd..16387506cf 100644 --- a/include/pybind11/detail/type_caster_base.h +++ b/include/pybind11/detail/type_caster_base.h @@ -104,16 +104,14 @@ all_type_info_get_cache(PyTypeObject *type); // Populates a just-created cache entry. PYBIND11_NOINLINE void all_type_info_populate(PyTypeObject *t, std::vector &bases) { -printf("\nLOOOK all_type_info_populate[ %s:%d\n", __FILE__, __LINE__); fflush(stdout); std::vector check; for (handle parent : reinterpret_borrow(t->tp_bases)) { check.push_back((PyTypeObject *) parent.ptr()); } -printf("\nLOOOK:REG:POP tp_name=%s check.size()=%lu %s:%d\n", t->tp_name, (unsigned long) check.size(), __FILE__, __LINE__); fflush(stdout); auto const &type_dict = get_internals().registered_types_py; for (size_t i = 0; i < check.size(); i++) { - PyTypeObject *type = check[i]; + auto *type = check[i]; // Ignore Python2 old-style class super type: if (!PyType_Check((PyObject *) type)) { continue; @@ -121,25 +119,23 @@ printf("\nLOOOK:REG:POP tp_name=%s check.size()=%lu %s:%d\n", t->tp_name, (unsig // Check `type` in the current set of registered python types: auto it = type_dict.find(type); -printf("\nLOOOK type_dict.find(type) tp_name=%s found=%s %s:%d\n", type->tp_name, (it != type_dict.end() ? "yes" : "no"), __FILE__, __LINE__); fflush(stdout); if (it != type_dict.end()) { // We found a cache entry for it, so it's either pybind-registered or has pre-computed // pybind bases, but we have to make sure we haven't already seen the type(s) before: // we want to follow Python/virtual C++ rules that there should only be one instance of // a common base. - for (type_info *tinfo : it->second) { + for (auto *tinfo : it->second) { // NB: Could use a second set here, rather than doing a linear search, but since // having a large number of immediate pybind11-registered types seems fairly // unlikely, that probably isn't worthwhile. bool found = false; - for (type_info *known : bases) { + for (auto *known : bases) { if (known == tinfo) { found = true; break; } } if (!found) { -printf("\nLOOOK:REG:ADD bases.push_back(tinfo) %s %s:%d\n", tinfo->cpptype->name(), __FILE__, __LINE__); fflush(stdout); bases.push_back(tinfo); } } @@ -158,7 +154,6 @@ printf("\nLOOOK:REG:ADD bases.push_back(tinfo) %s %s:%d\n", tinfo->cpptype->name } } } -printf("\nLOOOK all_type_info_populate] %s:%d\n", __FILE__, __LINE__); fflush(stdout); } /** @@ -265,14 +260,7 @@ struct value_and_holder { value_and_holder(instance *i, const detail::type_info *type, size_t vpos, size_t index) : inst{i}, index{index}, type{type}, vh{inst->simple_layout ? inst->simple_value_holder - : &inst->nonsimple.values_and_holders[vpos]} { -if (type && type->cpptype) { -const char *nm = type->cpptype->name(); -if (strcmp(nm, "N32test_python_multiple_inheritance7CppBaseE") == 0 || strcmp(nm, "N32test_python_multiple_inheritance7CppDrvdE") == 0) { -printf("\nLOOOK %s value_and_holder ctor %s:%d\n", nm, __FILE__, __LINE__); fflush(stdout); -} -} - } + : &inst->nonsimple.values_and_holders[vpos]} {} // Default constructor (used to signal a value-and-holder not found by get_value_and_holder()) value_and_holder() = default; @@ -298,18 +286,11 @@ printf("\nLOOOK %s value_and_holder ctor %s:%d\n", nm, __FILE__, __LINE__); fflu } // NOLINTNEXTLINE(readability-make-member-function-const) void set_holder_constructed(bool v = true) { -//printf("\nLOOOK set_holder_constructed inst=%lu %s %s:%d\n", reinterpret_cast(inst), type->cpptype->name(), __FILE__, __LINE__); fflush(stdout); -if (strcmp("N32test_python_multiple_inheritance7CppBaseE", type->cpptype->name()) == 0) { - //long *BAD = nullptr; *BAD = 101; -} if (inst->simple_layout) { -//printf("\nLOOOK %s set_holder_constructed simple_layout %s:%d\n", type->cpptype->name(), __FILE__, __LINE__); fflush(stdout); inst->simple_holder_constructed = v; } else if (v) { -//printf("\nLOOOK %s set_holder_constructed v %s:%d\n", type->cpptype->name(), __FILE__, __LINE__); fflush(stdout); inst->nonsimple.status[index] |= instance::status_holder_constructed; } else { -//printf("\nLOOOK %s set_holder_constructed not v %s:%d\n", type->cpptype->name(), __FILE__, __LINE__); fflush(stdout); inst->nonsimple.status[index] &= (std::uint8_t) ~instance::status_holder_constructed; } } diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index 1acde6f461..3bce1a01ba 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -1142,10 +1142,8 @@ class cpp_function : public function { return nullptr; } if (overloads->is_constructor && !self_value_and_holder.holder_constructed()) { -printf("\nLOOOK %s BEFORE self_value_and_holder.type->init_instance %s:%d\n", self_value_and_holder.type->cpptype->name(), __FILE__, __LINE__); fflush(stdout); auto *pi = reinterpret_cast(parent.ptr()); self_value_and_holder.type->init_instance(pi, nullptr); -printf("\nLOOOK %s AFTER self_value_and_holder.type->init_instance %s:%d\n", self_value_and_holder.type->cpptype->name(), __FILE__, __LINE__); fflush(stdout); } return result.ptr(); } diff --git a/tests/test_python_multiple_inheritance.py b/tests/test_python_multiple_inheritance.py index 035a14b9ff..cdd34d9ceb 100644 --- a/tests/test_python_multiple_inheritance.py +++ b/tests/test_python_multiple_inheritance.py @@ -10,28 +10,13 @@ class PC(m.CppBase): class PPCCInit(PC, m.CppDrvd): def __init__(self, value): - print("\nLOOOK PPCCInit PC", flush=True) PC.__init__(self, value) - print("\nLOOOK PPCCInit CppDrvd", flush=True) m.CppDrvd.__init__(self, value + 1) - print("\nLOOOK PPCCInit Done", flush=True) - - -def NOtest_PC_AAA(): - print("\nLOOOK BEFORE PC(11) AAA", flush=True) - d = PC(11) - print("\nLOOOK AFTER PC(11) AAA", flush=True) - assert d.get_base_value() == 11 - d.reset_base_value(13) - assert d.get_base_value() == 13 # Moving this test after test_PC() changes the behavior! -def test_PPCCInit_BBB(): - print("\nLOOOK BEFORE PPCCInit(11) BBB", flush=True) +def test_PPCCInit(): d = PPCCInit(11) - print("\nLOOOK AFTER PPCCInit(11) BBB", flush=True) - print("\nLOOOK", flush=True) assert d.get_drvd_value() == 36 d.reset_drvd_value(55) assert d.get_drvd_value() == 55 @@ -46,29 +31,8 @@ def test_PPCCInit_BBB(): assert d.get_base_value_from_drvd() == 30 -def NOtest_PC_CCC(): - print("\nLOOOK BEFORE PC(11) CCC", flush=True) +def test_PC(): d = PC(11) - print("\nLOOOK AFTER PC(11) CCC", flush=True) assert d.get_base_value() == 11 d.reset_base_value(13) assert d.get_base_value() == 13 - -# Moving this test after test_PC() changes the behavior! -def NOtest_PPCCInit_DDD(): - print("\nLOOOK BEFORE PPCCInit(11) DDD", flush=True) - d = PPCCInit(11) - print("\nLOOOK AFTER PPCCInit(11) DDD", flush=True) - print("\nLOOOK", flush=True) - assert d.get_drvd_value() == 36 - d.reset_drvd_value(55) - assert d.get_drvd_value() == 55 - - assert d.get_base_value() == 12 - assert d.get_base_value_from_drvd() == 12 - d.reset_base_value(20) - assert d.get_base_value() == 20 - assert d.get_base_value_from_drvd() == 20 - d.reset_base_value_from_drvd(30) - assert d.get_base_value() == 30 - assert d.get_base_value_from_drvd() == 30 From d37b5409d4e5b835620ccbb321a4e1ba89af315c Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Thu, 27 Jul 2023 13:45:26 -0700 Subject: [PATCH 09/27] [ci skip] Order Dependence Demo --- include/pybind11/detail/type_caster_base.h | 14 ++++++++++++++ tests/test_python_multiple_inheritance.py | 9 +++++++-- 2 files changed, 21 insertions(+), 2 deletions(-) diff --git a/include/pybind11/detail/type_caster_base.h b/include/pybind11/detail/type_caster_base.h index 16387506cf..38b5fcf5d0 100644 --- a/include/pybind11/detail/type_caster_base.h +++ b/include/pybind11/detail/type_caster_base.h @@ -156,6 +156,19 @@ PYBIND11_NOINLINE void all_type_info_populate(PyTypeObject *t, std::vectortp_name); + for (const auto &base : item.second) { + printf(" %s\n", base->type->tp_name); + } + } + printf("}\n"); + fflush(stdout); +} + /** * Extracts vector of type_info pointers of pybind-registered roots of the given Python type. Will * be just 1 pybind type for the Python type of a pybind-registered class, or for any Python-side @@ -171,6 +184,7 @@ inline const std::vector &all_type_info(PyTypeObject *type) if (ins.second) { // New cache entry: populate it all_type_info_populate(type, ins.first->second); + dump_registered_types_py(); } return ins.first->second; diff --git a/tests/test_python_multiple_inheritance.py b/tests/test_python_multiple_inheritance.py index cdd34d9ceb..5b18cfea1a 100644 --- a/tests/test_python_multiple_inheritance.py +++ b/tests/test_python_multiple_inheritance.py @@ -15,7 +15,7 @@ def __init__(self, value): # Moving this test after test_PC() changes the behavior! -def test_PPCCInit(): +def NOtest_PPCCInit(): d = PPCCInit(11) assert d.get_drvd_value() == 36 d.reset_drvd_value(55) @@ -31,8 +31,13 @@ def test_PPCCInit(): assert d.get_base_value_from_drvd() == 30 -def test_PC(): +def NOtest_PC(): d = PC(11) assert d.get_base_value() == 11 d.reset_base_value(13) assert d.get_base_value() == 13 + + +def testOrderDependenceDemo(): + PC(0) + PPCCInit(0) From 7f8b2081ffdd74340459ae7b0c40ac7fce231d5b Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Thu, 27 Jul 2023 15:16:59 -0700 Subject: [PATCH 10/27] Revert "[ci skip] Order Dependence Demo" This reverts commit d37b5409d4e5b835620ccbb321a4e1ba89af315c. --- include/pybind11/detail/type_caster_base.h | 14 -------------- tests/test_python_multiple_inheritance.py | 9 ++------- 2 files changed, 2 insertions(+), 21 deletions(-) diff --git a/include/pybind11/detail/type_caster_base.h b/include/pybind11/detail/type_caster_base.h index 38b5fcf5d0..16387506cf 100644 --- a/include/pybind11/detail/type_caster_base.h +++ b/include/pybind11/detail/type_caster_base.h @@ -156,19 +156,6 @@ PYBIND11_NOINLINE void all_type_info_populate(PyTypeObject *t, std::vectortp_name); - for (const auto &base : item.second) { - printf(" %s\n", base->type->tp_name); - } - } - printf("}\n"); - fflush(stdout); -} - /** * Extracts vector of type_info pointers of pybind-registered roots of the given Python type. Will * be just 1 pybind type for the Python type of a pybind-registered class, or for any Python-side @@ -184,7 +171,6 @@ inline const std::vector &all_type_info(PyTypeObject *type) if (ins.second) { // New cache entry: populate it all_type_info_populate(type, ins.first->second); - dump_registered_types_py(); } return ins.first->second; diff --git a/tests/test_python_multiple_inheritance.py b/tests/test_python_multiple_inheritance.py index 5b18cfea1a..cdd34d9ceb 100644 --- a/tests/test_python_multiple_inheritance.py +++ b/tests/test_python_multiple_inheritance.py @@ -15,7 +15,7 @@ def __init__(self, value): # Moving this test after test_PC() changes the behavior! -def NOtest_PPCCInit(): +def test_PPCCInit(): d = PPCCInit(11) assert d.get_drvd_value() == 36 d.reset_drvd_value(55) @@ -31,13 +31,8 @@ def NOtest_PPCCInit(): assert d.get_base_value_from_drvd() == 30 -def NOtest_PC(): +def test_PC(): d = PC(11) assert d.get_base_value() == 11 d.reset_base_value(13) assert d.get_base_value() == 13 - - -def testOrderDependenceDemo(): - PC(0) - PPCCInit(0) From eb09c6c1b978208ceee40f05bbe75491b6ff8ad6 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Thu, 27 Jul 2023 21:27:44 -0700 Subject: [PATCH 11/27] One way to deal with the order dependency issue. This is not the best way, more like a proof of concept. --- include/pybind11/detail/type_caster_base.h | 27 +++++++++++++++++----- 1 file changed, 21 insertions(+), 6 deletions(-) diff --git a/include/pybind11/detail/type_caster_base.h b/include/pybind11/detail/type_caster_base.h index 16387506cf..a6a8e968ca 100644 --- a/include/pybind11/detail/type_caster_base.h +++ b/include/pybind11/detail/type_caster_base.h @@ -713,13 +713,28 @@ class type_caster_generic { // if we can find an exact match (or, for a simple C++ type, an inherited match); if // so, we can safely reinterpret_cast to the relevant pointer. if (bases.size() > 1) { - for (auto *base : bases) { - if (no_cpp_mi ? PyType_IsSubtype(base->type, typeinfo->type) - : base->type == typeinfo->type) { - this_.load_value( - reinterpret_cast(src.ptr())->get_value_and_holder(base)); - return true; + type_info *best_base = nullptr; + if (no_cpp_mi) { + for (auto *base : bases) { + if (PyType_IsSubtype(base->type, typeinfo->type)) { + if (best_base == nullptr + || PyType_IsSubtype(base->type, best_base->type)) { + best_base = base; + } + } } + } else { + for (auto *base : bases) { + if (base->type == typeinfo->type) { + best_base = base; + break; + } + } + } + if (best_base != nullptr) { + this_.load_value( + reinterpret_cast(src.ptr())->get_value_and_holder(best_base)); + return true; } } From a77c1c6fb61eccee032d4803e5bee308d9fac3a1 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Thu, 27 Jul 2023 21:28:14 -0700 Subject: [PATCH 12/27] Move test_PC() first again. --- tests/test_python_multiple_inheritance.py | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/tests/test_python_multiple_inheritance.py b/tests/test_python_multiple_inheritance.py index cdd34d9ceb..97ee24f696 100644 --- a/tests/test_python_multiple_inheritance.py +++ b/tests/test_python_multiple_inheritance.py @@ -14,7 +14,13 @@ def __init__(self, value): m.CppDrvd.__init__(self, value + 1) -# Moving this test after test_PC() changes the behavior! +def test_PC(): + d = PC(11) + assert d.get_base_value() == 11 + d.reset_base_value(13) + assert d.get_base_value() == 13 + + def test_PPCCInit(): d = PPCCInit(11) assert d.get_drvd_value() == 36 @@ -29,10 +35,3 @@ def test_PPCCInit(): d.reset_base_value_from_drvd(30) assert d.get_base_value() == 30 assert d.get_base_value_from_drvd() == 30 - - -def test_PC(): - d = PC(11) - assert d.get_base_value() == 11 - d.reset_base_value(13) - assert d.get_base_value() == 13 From eec2d81d828b47a3d51809d95e1bc3fa093aadcb Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Fri, 28 Jul 2023 13:26:14 -0700 Subject: [PATCH 13/27] Add `all_type_info_add_base_most_derived_first()`, use in `all_type_info_populate()` --- include/pybind11/detail/type_caster_base.h | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/include/pybind11/detail/type_caster_base.h b/include/pybind11/detail/type_caster_base.h index a6a8e968ca..9f4891d01a 100644 --- a/include/pybind11/detail/type_caster_base.h +++ b/include/pybind11/detail/type_caster_base.h @@ -102,8 +102,21 @@ class loader_life_support { inline std::pair all_type_info_get_cache(PyTypeObject *type); +inline void all_type_info_add_base_most_derived_first(std::vector &bases, + type_info *addl_base) { + for (std::vector::const_iterator it = bases.begin(); it != bases.end(); it++) { + type_info *existing_base = *it; + if (PyType_IsSubtype(addl_base->type, existing_base->type)) { + bases.insert(it, addl_base); + return; + } + } + bases.push_back(addl_base); +} + // Populates a just-created cache entry. PYBIND11_NOINLINE void all_type_info_populate(PyTypeObject *t, std::vector &bases) { + assert(bases.empty()); std::vector check; for (handle parent : reinterpret_borrow(t->tp_bases)) { check.push_back((PyTypeObject *) parent.ptr()); @@ -136,7 +149,7 @@ PYBIND11_NOINLINE void all_type_info_populate(PyTypeObject *t, std::vectortp_bases) { From 9c7d2676c95ee54bf5430b69f208a1e46078e6a3 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Fri, 28 Jul 2023 13:26:24 -0700 Subject: [PATCH 14/27] Revert "One way to deal with the order dependency issue. This is not the best way, more like a proof of concept." This reverts commit eb09c6c1b978208ceee40f05bbe75491b6ff8ad6. --- include/pybind11/detail/type_caster_base.h | 27 +++++----------------- 1 file changed, 6 insertions(+), 21 deletions(-) diff --git a/include/pybind11/detail/type_caster_base.h b/include/pybind11/detail/type_caster_base.h index 9f4891d01a..0fb15a162c 100644 --- a/include/pybind11/detail/type_caster_base.h +++ b/include/pybind11/detail/type_caster_base.h @@ -726,28 +726,13 @@ class type_caster_generic { // if we can find an exact match (or, for a simple C++ type, an inherited match); if // so, we can safely reinterpret_cast to the relevant pointer. if (bases.size() > 1) { - type_info *best_base = nullptr; - if (no_cpp_mi) { - for (auto *base : bases) { - if (PyType_IsSubtype(base->type, typeinfo->type)) { - if (best_base == nullptr - || PyType_IsSubtype(base->type, best_base->type)) { - best_base = base; - } - } + for (auto *base : bases) { + if (no_cpp_mi ? PyType_IsSubtype(base->type, typeinfo->type) + : base->type == typeinfo->type) { + this_.load_value( + reinterpret_cast(src.ptr())->get_value_and_holder(base)); + return true; } - } else { - for (auto *base : bases) { - if (base->type == typeinfo->type) { - best_base = base; - break; - } - } - } - if (best_base != nullptr) { - this_.load_value( - reinterpret_cast(src.ptr())->get_value_and_holder(best_base)); - return true; } } From 7c7d78d87f219d7ccc3083582ec86d79b5ce6069 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Fri, 28 Jul 2023 14:13:14 -0700 Subject: [PATCH 15/27] clang-tidy fixes (automatic) --- include/pybind11/detail/type_caster_base.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/include/pybind11/detail/type_caster_base.h b/include/pybind11/detail/type_caster_base.h index 0fb15a162c..98137a7f98 100644 --- a/include/pybind11/detail/type_caster_base.h +++ b/include/pybind11/detail/type_caster_base.h @@ -104,9 +104,9 @@ all_type_info_get_cache(PyTypeObject *type); inline void all_type_info_add_base_most_derived_first(std::vector &bases, type_info *addl_base) { - for (std::vector::const_iterator it = bases.begin(); it != bases.end(); it++) { + for (auto it = bases.begin(); it != bases.end(); it++) { type_info *existing_base = *it; - if (PyType_IsSubtype(addl_base->type, existing_base->type)) { + if (PyType_IsSubtype(addl_base->type, existing_base->type) != 0) { bases.insert(it, addl_base); return; } From d7088364f992ce1195846f385c658787d7d9e6df Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Fri, 28 Jul 2023 15:20:03 -0700 Subject: [PATCH 16/27] Add `is_redundant_value_and_holder()` and use to avoid forcing `__init__` overrides when they are not needed. --- include/pybind11/detail/class.h | 23 +++++++++++++++++----- include/pybind11/detail/type_caster_base.h | 1 + tests/test_python_multiple_inheritance.py | 16 +++++++-------- 3 files changed, 26 insertions(+), 14 deletions(-) diff --git a/include/pybind11/detail/class.h b/include/pybind11/detail/class.h index bc2b40c50a..89a205bbe7 100644 --- a/include/pybind11/detail/class.h +++ b/include/pybind11/detail/class.h @@ -180,6 +180,17 @@ extern "C" inline PyObject *pybind11_meta_getattro(PyObject *obj, PyObject *name return PyType_Type.tp_getattro(obj, name); } +// Band-aid workaround to fix a subtle but serious bug in a minimalistic fashion. See PR #4762. +inline bool is_redundant_value_and_holder(const std::vector &bases, + std::size_t ix_base) { + for (std::size_t i = 0; i < ix_base; i++) { + if (PyType_IsSubtype(bases[i]->type, bases[ix_base]->type) != 0) { + return true; + } + } + return false; +} + /// metaclass `__call__` function that is used to create all pybind11 objects. extern "C" inline PyObject *pybind11_meta_call(PyObject *type, PyObject *args, PyObject *kwargs) { @@ -189,18 +200,20 @@ extern "C" inline PyObject *pybind11_meta_call(PyObject *type, PyObject *args, P return nullptr; } - // This must be a pybind11 instance - auto *instance = reinterpret_cast(self); - // Ensure that the base __init__ function(s) were called - for (const auto &vh : values_and_holders(instance)) { - if (!vh.holder_constructed()) { + const auto &bases = all_type_info((PyTypeObject *) type); + values_and_holders vhs(reinterpret_cast(self)); + assert(bases.size() == vhs.size()); + std::size_t ix_base = 0; + for (const auto &vh : vhs) { + if (!vh.holder_constructed() && !is_redundant_value_and_holder(bases, ix_base)) { PyErr_Format(PyExc_TypeError, "%.200s.__init__() must be called when overriding __init__", get_fully_qualified_tp_name(vh.type->type).c_str()); Py_DECREF(self); return nullptr; } + ix_base++; } return self; diff --git a/include/pybind11/detail/type_caster_base.h b/include/pybind11/detail/type_caster_base.h index 98137a7f98..b43c227de6 100644 --- a/include/pybind11/detail/type_caster_base.h +++ b/include/pybind11/detail/type_caster_base.h @@ -102,6 +102,7 @@ class loader_life_support { inline std::pair all_type_info_get_cache(PyTypeObject *type); +// Band-aid workaround to fix a subtle but serious bug in a minimalistic fashion. See PR #4762. inline void all_type_info_add_base_most_derived_first(std::vector &bases, type_info *addl_base) { for (auto it = bases.begin(); it != bases.end(); it++) { diff --git a/tests/test_python_multiple_inheritance.py b/tests/test_python_multiple_inheritance.py index 97ee24f696..3bddd67dfb 100644 --- a/tests/test_python_multiple_inheritance.py +++ b/tests/test_python_multiple_inheritance.py @@ -8,10 +8,8 @@ class PC(m.CppBase): pass -class PPCCInit(PC, m.CppDrvd): - def __init__(self, value): - PC.__init__(self, value) - m.CppDrvd.__init__(self, value + 1) +class PPCC(PC, m.CppDrvd): + pass def test_PC(): @@ -21,14 +19,14 @@ def test_PC(): assert d.get_base_value() == 13 -def test_PPCCInit(): - d = PPCCInit(11) - assert d.get_drvd_value() == 36 +def test_PPCC(): + d = PPCC(11) + assert d.get_drvd_value() == 33 d.reset_drvd_value(55) assert d.get_drvd_value() == 55 - assert d.get_base_value() == 12 - assert d.get_base_value_from_drvd() == 12 + assert d.get_base_value() == 11 + assert d.get_base_value_from_drvd() == 11 d.reset_base_value(20) assert d.get_base_value() == 20 assert d.get_base_value_from_drvd() == 20 From cf5958d51682af9e6a22643406f75d733cc5ea4c Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Sun, 30 Jul 2023 23:27:50 -0700 Subject: [PATCH 17/27] Streamline implementation and avoid unsafe `reinterpret_cast()` introduced with PR #2152 The `reinterpret_cast(self)` is unsafe if `__new__` is mocked, which was actually found in the wild: the mock returned `None` for `self`. This was inconsequential because `inst` is currently cast straight back to `PyObject *` to compute `all_type_info()`, which is empty if `self` is not a pybind11 `instance`, and then `inst` is never dereferenced. However, the unsafe detour through `instance *` is easily avoided and the updated implementation is less prone to accidents while debugging or refactoring. --- include/pybind11/detail/class.h | 19 ++----------------- include/pybind11/detail/type_caster_base.h | 17 +++++++++++++++++ 2 files changed, 19 insertions(+), 17 deletions(-) diff --git a/include/pybind11/detail/class.h b/include/pybind11/detail/class.h index 89a205bbe7..7b0b7d3b0d 100644 --- a/include/pybind11/detail/class.h +++ b/include/pybind11/detail/class.h @@ -180,17 +180,6 @@ extern "C" inline PyObject *pybind11_meta_getattro(PyObject *obj, PyObject *name return PyType_Type.tp_getattro(obj, name); } -// Band-aid workaround to fix a subtle but serious bug in a minimalistic fashion. See PR #4762. -inline bool is_redundant_value_and_holder(const std::vector &bases, - std::size_t ix_base) { - for (std::size_t i = 0; i < ix_base; i++) { - if (PyType_IsSubtype(bases[i]->type, bases[ix_base]->type) != 0) { - return true; - } - } - return false; -} - /// metaclass `__call__` function that is used to create all pybind11 objects. extern "C" inline PyObject *pybind11_meta_call(PyObject *type, PyObject *args, PyObject *kwargs) { @@ -201,19 +190,15 @@ extern "C" inline PyObject *pybind11_meta_call(PyObject *type, PyObject *args, P } // Ensure that the base __init__ function(s) were called - const auto &bases = all_type_info((PyTypeObject *) type); - values_and_holders vhs(reinterpret_cast(self)); - assert(bases.size() == vhs.size()); - std::size_t ix_base = 0; + values_and_holders vhs(self); for (const auto &vh : vhs) { - if (!vh.holder_constructed() && !is_redundant_value_and_holder(bases, ix_base)) { + if (!vh.holder_constructed() && !vhs.is_redundant_value_and_holder(vh)) { PyErr_Format(PyExc_TypeError, "%.200s.__init__() must be called when overriding __init__", get_fully_qualified_tp_name(vh.type->type).c_str()); Py_DECREF(self); return nullptr; } - ix_base++; } return self; diff --git a/include/pybind11/detail/type_caster_base.h b/include/pybind11/detail/type_caster_base.h index b43c227de6..ac1014d281 100644 --- a/include/pybind11/detail/type_caster_base.h +++ b/include/pybind11/detail/type_caster_base.h @@ -336,6 +336,13 @@ struct values_and_holders { explicit values_and_holders(instance *inst) : inst{inst}, tinfo(all_type_info(Py_TYPE(inst))) {} + explicit values_and_holders(PyObject *obj) + : inst{nullptr}, tinfo(all_type_info(Py_TYPE(obj))) { + if (!tinfo.empty()) { + inst = reinterpret_cast(obj); + } + } + struct iterator { private: instance *inst = nullptr; @@ -378,6 +385,16 @@ struct values_and_holders { } size_t size() { return tinfo.size(); } + + // Band-aid workaround to fix a subtle but serious bug in a minimalistic fashion. See PR #4762. + bool is_redundant_value_and_holder(const value_and_holder &vh) { + for (size_t i = 0; i < vh.index; i++) { + if (PyType_IsSubtype(tinfo[i]->type, tinfo[vh.index]->type) != 0) { + return true; + } + } + return false; + } }; /** From c89561f77aeea99d1149822430ab3a0e6e190daa Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Mon, 31 Jul 2023 10:26:13 -0700 Subject: [PATCH 18/27] Fix actual undefined behavior exposed by previous changes. It turns out the previous commit message is incorrect, the `inst` pointer is actually dereferenced, in the `value_and_holder` ctor here: https://github.com/pybind/pybind11/blob/f3e0602802c7840992c97f4960515777cad6a5c7/include/pybind11/detail/type_caster_base.h#L262-L263 ``` 259 // Main constructor for a found value/holder: 260 value_and_holder(instance *i, const detail::type_info *type, size_t vpos, size_t index) 261 : inst{i}, index{index}, type{type}, 262 vh{inst->simple_layout ? inst->simple_value_holder 263 : &inst->nonsimple.values_and_holders[vpos]} {} ``` --- include/pybind11/detail/type_caster_base.h | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/include/pybind11/detail/type_caster_base.h b/include/pybind11/detail/type_caster_base.h index ac1014d281..346e366da6 100644 --- a/include/pybind11/detail/type_caster_base.h +++ b/include/pybind11/detail/type_caster_base.h @@ -349,12 +349,16 @@ struct values_and_holders { const type_vec *types = nullptr; value_and_holder curr; friend struct values_and_holders; - iterator(instance *inst, const type_vec *tinfo) - : inst{inst}, types{tinfo}, - curr(inst /* instance */, - types->empty() ? nullptr : (*types)[0] /* type info */, - 0, /* vpos: (non-simple types only): the first vptr comes first */ - 0 /* index */) {} + iterator(instance *inst, const type_vec *tinfo) : inst{inst}, types{tinfo} { + if (inst != nullptr) { + assert(!types->empty()); + curr = value_and_holder( + inst /* instance */, + (*types)[0] /* type info */, + 0, /* vpos: (non-simple types only): the first vptr comes first */ + 0 /* index */); + } + } // Past-the-end iterator: explicit iterator(size_t end) : curr(end) {} From a2f95e1d9419ca713eab0b8b93eb7d540c81de60 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Mon, 31 Jul 2023 10:55:54 -0700 Subject: [PATCH 19/27] Add test_mock_new() --- tests/test_class.py | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/tests/test_class.py b/tests/test_class.py index ee7467cf8b..73a48309e8 100644 --- a/tests/test_class.py +++ b/tests/test_class.py @@ -1,3 +1,5 @@ +from unittest import mock + import pytest import env @@ -203,6 +205,18 @@ def __init__(self): assert msg(exc_info.value) == expected +@pytest.mark.parametrize( + "mock_return_value", [None, (1, 2, 3), m.Pet("Polly", "parrot"), m.Dog("Molly")] +) +def test_mock_new(mock_return_value): + with mock.patch.object( + m.Pet, "__new__", return_value=mock_return_value + ) as mock_new: + obj = m.Pet("Noname", "Nospecies") + assert obj is mock_return_value + mock_new.assert_called_once_with(m.Pet, "Noname", "Nospecies") + + def test_automatic_upcasting(): assert type(m.return_class_1()).__name__ == "DerivedClass1" assert type(m.return_class_2()).__name__ == "DerivedClass2" From 4f90d85f9fc15290d6be54d5ae9417bd131b84d9 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Mon, 31 Jul 2023 16:16:46 -0700 Subject: [PATCH 20/27] Experiment: specify indirect bases --- tests/test_python_multiple_inheritance.cpp | 16 ++++++++++++++++ tests/test_python_multiple_inheritance.py | 11 +++++++++++ 2 files changed, 27 insertions(+) diff --git a/tests/test_python_multiple_inheritance.cpp b/tests/test_python_multiple_inheritance.cpp index 6899171585..d8fdcf1cf2 100644 --- a/tests/test_python_multiple_inheritance.cpp +++ b/tests/test_python_multiple_inheritance.cpp @@ -26,6 +26,14 @@ struct CppDrvd : CppBase { int drvd_value; }; +struct CppDrv2 : CppDrvd { + explicit CppDrv2(int value) : CppDrvd(value), drv2_value(value * 5) {} + int get_drv2_value() const { return drv2_value; } + +private: + int drv2_value; +}; + } // namespace test_python_multiple_inheritance TEST_SUBMODULE(python_multiple_inheritance, m) { @@ -42,4 +50,12 @@ TEST_SUBMODULE(python_multiple_inheritance, m) { .def("reset_drvd_value", &CppDrvd::reset_drvd_value) .def("get_base_value_from_drvd", &CppDrvd::get_base_value_from_drvd) .def("reset_base_value_from_drvd", &CppDrvd::reset_base_value_from_drvd); + +#if 1 // This works. + py::class_(m, "CppDrv2") +#else // Apparent undefined behavior. + py::class_(m, "CppDrv2") +#endif + .def(py::init()) + .def("get_drv2_value", &CppDrv2::get_drv2_value); } diff --git a/tests/test_python_multiple_inheritance.py b/tests/test_python_multiple_inheritance.py index 3bddd67dfb..4acc5202e2 100644 --- a/tests/test_python_multiple_inheritance.py +++ b/tests/test_python_multiple_inheritance.py @@ -12,6 +12,10 @@ class PPCC(PC, m.CppDrvd): pass +class PPCC2(PC, m.CppDrv2): + pass + + def test_PC(): d = PC(11) assert d.get_base_value() == 11 @@ -33,3 +37,10 @@ def test_PPCC(): d.reset_base_value_from_drvd(30) assert d.get_base_value() == 30 assert d.get_base_value_from_drvd() == 30 + + +def test_PPCC2(): + d = PPCC2(13) + assert d.get_drv2_value() == 65 + assert d.get_drvd_value() == 39 + assert d.get_base_value() == 13 From 52b799343fc5d80a49c81c03dc6bcfc94de8a45e Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Fri, 4 Aug 2023 12:36:36 -0700 Subject: [PATCH 21/27] Revert "Experiment: specify indirect bases" This reverts commit 4f90d85f9fc15290d6be54d5ae9417bd131b84d9. --- tests/test_python_multiple_inheritance.cpp | 16 ---------------- tests/test_python_multiple_inheritance.py | 11 ----------- 2 files changed, 27 deletions(-) diff --git a/tests/test_python_multiple_inheritance.cpp b/tests/test_python_multiple_inheritance.cpp index d8fdcf1cf2..6899171585 100644 --- a/tests/test_python_multiple_inheritance.cpp +++ b/tests/test_python_multiple_inheritance.cpp @@ -26,14 +26,6 @@ struct CppDrvd : CppBase { int drvd_value; }; -struct CppDrv2 : CppDrvd { - explicit CppDrv2(int value) : CppDrvd(value), drv2_value(value * 5) {} - int get_drv2_value() const { return drv2_value; } - -private: - int drv2_value; -}; - } // namespace test_python_multiple_inheritance TEST_SUBMODULE(python_multiple_inheritance, m) { @@ -50,12 +42,4 @@ TEST_SUBMODULE(python_multiple_inheritance, m) { .def("reset_drvd_value", &CppDrvd::reset_drvd_value) .def("get_base_value_from_drvd", &CppDrvd::get_base_value_from_drvd) .def("reset_base_value_from_drvd", &CppDrvd::reset_base_value_from_drvd); - -#if 1 // This works. - py::class_(m, "CppDrv2") -#else // Apparent undefined behavior. - py::class_(m, "CppDrv2") -#endif - .def(py::init()) - .def("get_drv2_value", &CppDrv2::get_drv2_value); } diff --git a/tests/test_python_multiple_inheritance.py b/tests/test_python_multiple_inheritance.py index 4acc5202e2..3bddd67dfb 100644 --- a/tests/test_python_multiple_inheritance.py +++ b/tests/test_python_multiple_inheritance.py @@ -12,10 +12,6 @@ class PPCC(PC, m.CppDrvd): pass -class PPCC2(PC, m.CppDrv2): - pass - - def test_PC(): d = PC(11) assert d.get_base_value() == 11 @@ -37,10 +33,3 @@ def test_PPCC(): d.reset_base_value_from_drvd(30) assert d.get_base_value() == 30 assert d.get_base_value_from_drvd() == 30 - - -def test_PPCC2(): - d = PPCC2(13) - assert d.get_drv2_value() == 65 - assert d.get_drvd_value() == 39 - assert d.get_base_value() == 13 From 0a9599f775bfd3ca196c5e23a3fcf2890cbf6e82 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Thu, 2 Nov 2023 17:03:20 -0700 Subject: [PATCH 22/27] Add `all_type_info_check_for_divergence()` and some tests. --- include/pybind11/detail/type_caster_base.h | 35 +++++++++++++++++++++ tests/test_python_multiple_inheritance.cpp | 19 ++++++++++++ tests/test_python_multiple_inheritance.py | 36 ++++++++++++++++++++++ 3 files changed, 90 insertions(+) diff --git a/include/pybind11/detail/type_caster_base.h b/include/pybind11/detail/type_caster_base.h index 476646ee8f..e305066db7 100644 --- a/include/pybind11/detail/type_caster_base.h +++ b/include/pybind11/detail/type_caster_base.h @@ -115,6 +115,40 @@ inline void all_type_info_add_base_most_derived_first(std::vector & bases.push_back(addl_base); } +inline void all_type_info_check_for_divergence(const std::vector &bases) { + using sz_t = std::size_t; + sz_t n = bases.size(); + if (n < 3) { + return; + } + std::vector cluster_ids; + cluster_ids.reserve(n); + for (sz_t ci = 0; ci < n; ci++) { + cluster_ids.push_back(ci); + } + for (sz_t i = 0; i < n - 1; i++) { + if (cluster_ids[i] != i) { + continue; + } + for (sz_t j = i + 1; j < n; j++) { + if (PyType_IsSubtype(bases[i]->type, bases[j]->type) != 0) { + sz_t k = cluster_ids[j]; + if (k == j) { + cluster_ids[j] = i; + } else { + PyErr_Format( + PyExc_TypeError, + "bases include diverging derived types: base=%s, derived1=%s, derived2=%s", + bases[j]->type->tp_name, + bases[k]->type->tp_name, + bases[i]->type->tp_name); + throw error_already_set(); + } + } + } + } +} + // Populates a just-created cache entry. PYBIND11_NOINLINE void all_type_info_populate(PyTypeObject *t, std::vector &bases) { assert(bases.empty()); @@ -168,6 +202,7 @@ PYBIND11_NOINLINE void all_type_info_populate(PyTypeObject *t, std::vector(m, "CppDrvd2") + .def(py::init()) + .def("get_drvd2_value", &CppDrvd2::get_drvd2_value) + .def("reset_drvd2_value", &CppDrvd2::reset_drvd2_value) + .def("get_base_value_from_drvd2", &CppDrvd2::get_base_value_from_drvd2) + .def("reset_base_value_from_drvd2", &CppDrvd2::reset_base_value_from_drvd2); } diff --git a/tests/test_python_multiple_inheritance.py b/tests/test_python_multiple_inheritance.py index 3bddd67dfb..6669ac339e 100644 --- a/tests/test_python_multiple_inheritance.py +++ b/tests/test_python_multiple_inheritance.py @@ -1,6 +1,8 @@ # Adapted from: # https://github.com/google/clif/blob/5718e4d0807fd3b6a8187dde140069120b81ecef/clif/testing/python/python_multiple_inheritance_test.py +import pytest + from pybind11_tests import python_multiple_inheritance as m @@ -12,6 +14,22 @@ class PPCC(PC, m.CppDrvd): pass +class PPPCCC(PPCC, m.CppDrvd2): + pass + + +class PC1(m.CppDrvd): + pass + + +class PC2(m.CppDrvd2): + pass + + +class PCD(PC1, PC2): + pass + + def test_PC(): d = PC(11) assert d.get_base_value() == 11 @@ -33,3 +51,21 @@ def test_PPCC(): d.reset_base_value_from_drvd(30) assert d.get_base_value() == 30 assert d.get_base_value_from_drvd() == 30 + + +def NOtest_PPPCCC(): + # terminate called after throwing an instance of 'pybind11::error_already_set' + # what(): TypeError: bases include diverging derived types: + # base=pybind11_tests.python_multiple_inheritance.CppBase, + # derived1=pybind11_tests.python_multiple_inheritance.CppDrvd, + # derived2=pybind11_tests.python_multiple_inheritance.CppDrvd2 + PPPCCC(11) + + +def test_PCD(): + # This escapes all_type_info_check_for_divergence() because CppBase does not appear in bases. + with pytest.raises( + TypeError, + match=r"CppDrvd2\.__init__\(\) must be called when overriding __init__$", + ): + PCD(11) From 5f5fd6a68e3cff1726628f6dea8e1c0754636a23 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Tue, 7 Nov 2023 08:45:52 -0800 Subject: [PATCH 23/27] Call `all_type_info_check_for_divergence()` also from `type_caster_generic::load_impl<>` --- include/pybind11/detail/type_caster_base.h | 14 +++++++++++--- tests/test_python_multiple_inheritance.cpp | 2 ++ tests/test_python_multiple_inheritance.py | 12 ++++++++++++ 3 files changed, 25 insertions(+), 3 deletions(-) diff --git a/include/pybind11/detail/type_caster_base.h b/include/pybind11/detail/type_caster_base.h index e305066db7..50a3b87950 100644 --- a/include/pybind11/detail/type_caster_base.h +++ b/include/pybind11/detail/type_caster_base.h @@ -785,14 +785,22 @@ class type_caster_generic { // if we can find an exact match (or, for a simple C++ type, an inherited match); if // so, we can safely reinterpret_cast to the relevant pointer. if (bases.size() > 1) { + std::vector matching_bases; for (auto *base : bases) { if (no_cpp_mi ? PyType_IsSubtype(base->type, typeinfo->type) : base->type == typeinfo->type) { - this_.load_value( - reinterpret_cast(src.ptr())->get_value_and_holder(base)); - return true; + matching_bases.push_back(base); } } + if (matching_bases.size() != 0) { + if (matching_bases.size() > 1) { + matching_bases.push_back(const_cast(typeinfo)); + all_type_info_check_for_divergence(matching_bases); + } + this_.load_value(reinterpret_cast(src.ptr())->get_value_and_holder( + matching_bases[0])); + return true; + } } // Case 2c: C++ multiple inheritance is involved and we couldn't find an exact type diff --git a/tests/test_python_multiple_inheritance.cpp b/tests/test_python_multiple_inheritance.cpp index 664a2c23a0..15b1f3360e 100644 --- a/tests/test_python_multiple_inheritance.cpp +++ b/tests/test_python_multiple_inheritance.cpp @@ -61,4 +61,6 @@ TEST_SUBMODULE(python_multiple_inheritance, m) { .def("reset_drvd2_value", &CppDrvd2::reset_drvd2_value) .def("get_base_value_from_drvd2", &CppDrvd2::get_base_value_from_drvd2) .def("reset_base_value_from_drvd2", &CppDrvd2::reset_base_value_from_drvd2); + + m.def("pass_CppBase", [](const CppBase *) {}); } diff --git a/tests/test_python_multiple_inheritance.py b/tests/test_python_multiple_inheritance.py index 6669ac339e..6b941cb8d5 100644 --- a/tests/test_python_multiple_inheritance.py +++ b/tests/test_python_multiple_inheritance.py @@ -30,6 +30,12 @@ class PCD(PC1, PC2): pass +class PCDI(PC1, PC2): + def __init__(self): + PC1.__init__(self, 11) + PC2.__init__(self, 12) + + def test_PC(): d = PC(11) assert d.get_base_value() == 11 @@ -69,3 +75,9 @@ def test_PCD(): match=r"CppDrvd2\.__init__\(\) must be called when overriding __init__$", ): PCD(11) + + +def test_PCDI(): + obj = PCDI() + with pytest.raises(TypeError, match="^bases include diverging derived types: "): + m.pass_CppBase(obj) From df27188dc6d6cf333145755543f56b2f6657aa5e Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Tue, 7 Nov 2023 09:19:24 -0800 Subject: [PATCH 24/27] Resolve clang-tidy error: ``` include/pybind11/detail/type_caster_base.h:795:21: error: the 'empty' method should be used to check for emptiness instead of 'size' [readability-container-size-empty,-warnings-as-errors] if (matching_bases.size() != 0) { ^~~~~~~~~~~~~~~~~~~~~~~~~~ !matching_bases.empty() ``` --- include/pybind11/detail/type_caster_base.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/pybind11/detail/type_caster_base.h b/include/pybind11/detail/type_caster_base.h index 50a3b87950..aaf191a60e 100644 --- a/include/pybind11/detail/type_caster_base.h +++ b/include/pybind11/detail/type_caster_base.h @@ -792,7 +792,7 @@ class type_caster_generic { matching_bases.push_back(base); } } - if (matching_bases.size() != 0) { + if (!matching_bases.empty()) { if (matching_bases.size() > 1) { matching_bases.push_back(const_cast(typeinfo)); all_type_info_check_for_divergence(matching_bases); From 1b157fcde95e3e9bb08410120728d1a2c2a50275 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Wed, 8 Nov 2023 11:52:46 -0800 Subject: [PATCH 25/27] Revert "Resolve clang-tidy error:" This reverts commit df27188dc6d6cf333145755543f56b2f6657aa5e. --- include/pybind11/detail/type_caster_base.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/pybind11/detail/type_caster_base.h b/include/pybind11/detail/type_caster_base.h index aaf191a60e..50a3b87950 100644 --- a/include/pybind11/detail/type_caster_base.h +++ b/include/pybind11/detail/type_caster_base.h @@ -792,7 +792,7 @@ class type_caster_generic { matching_bases.push_back(base); } } - if (!matching_bases.empty()) { + if (matching_bases.size() != 0) { if (matching_bases.size() > 1) { matching_bases.push_back(const_cast(typeinfo)); all_type_info_check_for_divergence(matching_bases); From 7a6c436affd64cba2a6381f1d19ea2475291582c Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Wed, 8 Nov 2023 11:53:06 -0800 Subject: [PATCH 26/27] Revert "Call `all_type_info_check_for_divergence()` also from `type_caster_generic::load_impl<>`" This reverts commit 5f5fd6a68e3cff1726628f6dea8e1c0754636a23. --- include/pybind11/detail/type_caster_base.h | 14 +++----------- tests/test_python_multiple_inheritance.cpp | 2 -- tests/test_python_multiple_inheritance.py | 12 ------------ 3 files changed, 3 insertions(+), 25 deletions(-) diff --git a/include/pybind11/detail/type_caster_base.h b/include/pybind11/detail/type_caster_base.h index 50a3b87950..e305066db7 100644 --- a/include/pybind11/detail/type_caster_base.h +++ b/include/pybind11/detail/type_caster_base.h @@ -785,22 +785,14 @@ class type_caster_generic { // if we can find an exact match (or, for a simple C++ type, an inherited match); if // so, we can safely reinterpret_cast to the relevant pointer. if (bases.size() > 1) { - std::vector matching_bases; for (auto *base : bases) { if (no_cpp_mi ? PyType_IsSubtype(base->type, typeinfo->type) : base->type == typeinfo->type) { - matching_bases.push_back(base); + this_.load_value( + reinterpret_cast(src.ptr())->get_value_and_holder(base)); + return true; } } - if (matching_bases.size() != 0) { - if (matching_bases.size() > 1) { - matching_bases.push_back(const_cast(typeinfo)); - all_type_info_check_for_divergence(matching_bases); - } - this_.load_value(reinterpret_cast(src.ptr())->get_value_and_holder( - matching_bases[0])); - return true; - } } // Case 2c: C++ multiple inheritance is involved and we couldn't find an exact type diff --git a/tests/test_python_multiple_inheritance.cpp b/tests/test_python_multiple_inheritance.cpp index 15b1f3360e..664a2c23a0 100644 --- a/tests/test_python_multiple_inheritance.cpp +++ b/tests/test_python_multiple_inheritance.cpp @@ -61,6 +61,4 @@ TEST_SUBMODULE(python_multiple_inheritance, m) { .def("reset_drvd2_value", &CppDrvd2::reset_drvd2_value) .def("get_base_value_from_drvd2", &CppDrvd2::get_base_value_from_drvd2) .def("reset_base_value_from_drvd2", &CppDrvd2::reset_base_value_from_drvd2); - - m.def("pass_CppBase", [](const CppBase *) {}); } diff --git a/tests/test_python_multiple_inheritance.py b/tests/test_python_multiple_inheritance.py index 6b941cb8d5..6669ac339e 100644 --- a/tests/test_python_multiple_inheritance.py +++ b/tests/test_python_multiple_inheritance.py @@ -30,12 +30,6 @@ class PCD(PC1, PC2): pass -class PCDI(PC1, PC2): - def __init__(self): - PC1.__init__(self, 11) - PC2.__init__(self, 12) - - def test_PC(): d = PC(11) assert d.get_base_value() == 11 @@ -75,9 +69,3 @@ def test_PCD(): match=r"CppDrvd2\.__init__\(\) must be called when overriding __init__$", ): PCD(11) - - -def test_PCDI(): - obj = PCDI() - with pytest.raises(TypeError, match="^bases include diverging derived types: "): - m.pass_CppBase(obj) From 6ab605bd25b5156c52fcc13ba3432e3e54575f46 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Wed, 8 Nov 2023 11:53:25 -0800 Subject: [PATCH 27/27] Revert "Add `all_type_info_check_for_divergence()` and some tests." This reverts commit 0a9599f775bfd3ca196c5e23a3fcf2890cbf6e82. --- include/pybind11/detail/type_caster_base.h | 35 --------------------- tests/test_python_multiple_inheritance.cpp | 19 ------------ tests/test_python_multiple_inheritance.py | 36 ---------------------- 3 files changed, 90 deletions(-) diff --git a/include/pybind11/detail/type_caster_base.h b/include/pybind11/detail/type_caster_base.h index e305066db7..476646ee8f 100644 --- a/include/pybind11/detail/type_caster_base.h +++ b/include/pybind11/detail/type_caster_base.h @@ -115,40 +115,6 @@ inline void all_type_info_add_base_most_derived_first(std::vector & bases.push_back(addl_base); } -inline void all_type_info_check_for_divergence(const std::vector &bases) { - using sz_t = std::size_t; - sz_t n = bases.size(); - if (n < 3) { - return; - } - std::vector cluster_ids; - cluster_ids.reserve(n); - for (sz_t ci = 0; ci < n; ci++) { - cluster_ids.push_back(ci); - } - for (sz_t i = 0; i < n - 1; i++) { - if (cluster_ids[i] != i) { - continue; - } - for (sz_t j = i + 1; j < n; j++) { - if (PyType_IsSubtype(bases[i]->type, bases[j]->type) != 0) { - sz_t k = cluster_ids[j]; - if (k == j) { - cluster_ids[j] = i; - } else { - PyErr_Format( - PyExc_TypeError, - "bases include diverging derived types: base=%s, derived1=%s, derived2=%s", - bases[j]->type->tp_name, - bases[k]->type->tp_name, - bases[i]->type->tp_name); - throw error_already_set(); - } - } - } - } -} - // Populates a just-created cache entry. PYBIND11_NOINLINE void all_type_info_populate(PyTypeObject *t, std::vector &bases) { assert(bases.empty()); @@ -202,7 +168,6 @@ PYBIND11_NOINLINE void all_type_info_populate(PyTypeObject *t, std::vector(m, "CppDrvd2") - .def(py::init()) - .def("get_drvd2_value", &CppDrvd2::get_drvd2_value) - .def("reset_drvd2_value", &CppDrvd2::reset_drvd2_value) - .def("get_base_value_from_drvd2", &CppDrvd2::get_base_value_from_drvd2) - .def("reset_base_value_from_drvd2", &CppDrvd2::reset_base_value_from_drvd2); } diff --git a/tests/test_python_multiple_inheritance.py b/tests/test_python_multiple_inheritance.py index 6669ac339e..3bddd67dfb 100644 --- a/tests/test_python_multiple_inheritance.py +++ b/tests/test_python_multiple_inheritance.py @@ -1,8 +1,6 @@ # Adapted from: # https://github.com/google/clif/blob/5718e4d0807fd3b6a8187dde140069120b81ecef/clif/testing/python/python_multiple_inheritance_test.py -import pytest - from pybind11_tests import python_multiple_inheritance as m @@ -14,22 +12,6 @@ class PPCC(PC, m.CppDrvd): pass -class PPPCCC(PPCC, m.CppDrvd2): - pass - - -class PC1(m.CppDrvd): - pass - - -class PC2(m.CppDrvd2): - pass - - -class PCD(PC1, PC2): - pass - - def test_PC(): d = PC(11) assert d.get_base_value() == 11 @@ -51,21 +33,3 @@ def test_PPCC(): d.reset_base_value_from_drvd(30) assert d.get_base_value() == 30 assert d.get_base_value_from_drvd() == 30 - - -def NOtest_PPPCCC(): - # terminate called after throwing an instance of 'pybind11::error_already_set' - # what(): TypeError: bases include diverging derived types: - # base=pybind11_tests.python_multiple_inheritance.CppBase, - # derived1=pybind11_tests.python_multiple_inheritance.CppDrvd, - # derived2=pybind11_tests.python_multiple_inheritance.CppDrvd2 - PPPCCC(11) - - -def test_PCD(): - # This escapes all_type_info_check_for_divergence() because CppBase does not appear in bases. - with pytest.raises( - TypeError, - match=r"CppDrvd2\.__init__\(\) must be called when overriding __init__$", - ): - PCD(11)