From d9957016bf9f9c362183d8058a0bb07c43cb077f Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Wed, 22 Mar 2023 18:06:02 -0700 Subject: [PATCH 1/4] Add test_class_sh_property_non_owning.cpp,py Failing: ``` __________________________________________________________ test_persistent_holder __________________________________________________________ def test_persistent_holder(): h = m.DataFieldsHolder(2) > c = h.vec_at(0).core_fld E RuntimeError: Non-owning holder (loaded_as_shared_ptr). h = test_class_sh_property_non_owning.py:6: RuntimeError __________________________________________________________ test_temporary_holder ___________________________________________________________ def test_temporary_holder(): d = m.DataFieldsHolder(2).vec_at(1) > c = d.core_fld E RuntimeError: Non-owning holder (loaded_as_shared_ptr). d = test_class_sh_property_non_owning.py:13: RuntimeError ``` --- tests/test_class_sh_property_non_owning.cpp | 58 +++++++++++++++++++++ tests/test_class_sh_property_non_owning.py | 14 +++++ 2 files changed, 72 insertions(+) create mode 100644 tests/test_class_sh_property_non_owning.cpp create mode 100644 tests/test_class_sh_property_non_owning.py diff --git a/tests/test_class_sh_property_non_owning.cpp b/tests/test_class_sh_property_non_owning.cpp new file mode 100644 index 0000000000..e384a4e16c --- /dev/null +++ b/tests/test_class_sh_property_non_owning.cpp @@ -0,0 +1,58 @@ +#include "pybind11/smart_holder.h" +#include "pybind11_tests.h" + +#include +#include + +namespace test_class_sh_property_non_owning { + +struct CoreField { + CoreField(int int_value = -99) : int_value{int_value} {} + int int_value; +}; + +struct DataField { + DataField(const CoreField &core_fld = CoreField{}) : core_fld{core_fld} {} + CoreField core_fld; +}; + +struct DataFieldsHolder { +private: + std::vector vec; + +public: + DataFieldsHolder(std::size_t vec_size) { + for (std::size_t i = 0; i < vec_size; i++) { + vec.push_back(DataField{CoreField{13 + static_cast(i) * 11}}); + } + } + + DataField *vec_at(std::size_t index) { + if (index >= vec.size()) { + return nullptr; + } + return &vec[index]; + } +}; + +} // namespace test_class_sh_property_non_owning + +using namespace test_class_sh_property_non_owning; + +PYBIND11_SMART_HOLDER_TYPE_CASTERS(CoreField) +PYBIND11_SMART_HOLDER_TYPE_CASTERS(DataField) +PYBIND11_SMART_HOLDER_TYPE_CASTERS(DataFieldsHolder) + +TEST_SUBMODULE(class_sh_property_non_owning, m) { + py::classh(m, "CoreField") + .def(py::init<>()) + .def_readwrite("int_value", &CoreField::int_value); + + py::classh(m, "DataField") + .def(py::init<>()) + .def_readwrite("core_fld", &DataField::core_fld); + + py::classh(m, "DataFieldsHolder") + .def(py::init()) + .def("vec_at", &DataFieldsHolder::vec_at, py::return_value_policy::reference_internal); +} diff --git a/tests/test_class_sh_property_non_owning.py b/tests/test_class_sh_property_non_owning.py new file mode 100644 index 0000000000..1a67054861 --- /dev/null +++ b/tests/test_class_sh_property_non_owning.py @@ -0,0 +1,14 @@ +from pybind11_tests import class_sh_property_non_owning as m + + +def test_persistent_holder(): + h = m.DataFieldsHolder(2) + c = h.vec_at(0).core_fld + assert c.int_value == 13 + assert h.vec_at(1).core_fld.int_value == 24 + + +def test_temporary_holder(): + d = m.DataFieldsHolder(2).vec_at(1) + c = d.core_fld + assert c.int_value == 24 From e7fbb8644fb2a2da57ba7d26de948339ed0800d4 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Thu, 23 Mar 2023 12:17:20 -0700 Subject: [PATCH 2/4] Introduce `shared_ptr_from_python(responsible_parent)` and use in all `property_cpp_function`s with `const shared_ptr &` arguments. Tests are incomplete. --- .../detail/smart_holder_type_casters.h | 34 ++++++++++++++++++- include/pybind11/pybind11.h | 15 +++++--- 2 files changed, 43 insertions(+), 6 deletions(-) diff --git a/include/pybind11/detail/smart_holder_type_casters.h b/include/pybind11/detail/smart_holder_type_casters.h index ba2b5bc040..cc16ad6cd0 100644 --- a/include/pybind11/detail/smart_holder_type_casters.h +++ b/include/pybind11/detail/smart_holder_type_casters.h @@ -408,6 +408,18 @@ struct smart_holder_type_caster_class_hooks : smart_holder_type_caster_base_tag } }; +struct shared_ptr_parent_life_support { + PyObject *parent; + explicit shared_ptr_parent_life_support(PyObject *parent) : parent{parent} { + Py_INCREF(parent); + } + // NOLINTNEXTLINE(readability-make-member-function-const) + void operator()(void *) { + gil_scoped_acquire gil; + Py_DECREF(parent); + } +}; + struct shared_ptr_trampoline_self_life_support { PyObject *self; explicit shared_ptr_trampoline_self_life_support(instance *inst) @@ -462,12 +474,23 @@ struct smart_holder_type_caster_load { return *raw_ptr; } - std::shared_ptr loaded_as_shared_ptr() const { + std::shared_ptr make_shared_ptr_with_responsible_parent(handle parent) const { + return std::shared_ptr(loaded_as_raw_ptr_unowned(), + shared_ptr_parent_life_support(parent.ptr())); + } + + std::shared_ptr loaded_as_shared_ptr(handle responsible_parent = nullptr) const { if (load_impl.unowned_void_ptr_from_void_ptr_capsule) { + if (responsible_parent) { + return make_shared_ptr_with_responsible_parent(responsible_parent); + } throw cast_error("Unowned pointer from a void pointer capsule cannot be converted to a" " std::shared_ptr."); } if (load_impl.unowned_void_ptr_from_direct_conversion != nullptr) { + if (responsible_parent) { + return make_shared_ptr_with_responsible_parent(responsible_parent); + } throw cast_error("Unowned pointer from direct conversion cannot be converted to a" " std::shared_ptr."); } @@ -478,6 +501,9 @@ struct smart_holder_type_caster_load { holder_type &hld = holder(); hld.ensure_is_not_disowned("loaded_as_shared_ptr"); if (hld.vptr_is_using_noop_deleter) { + if (responsible_parent) { + return make_shared_ptr_with_responsible_parent(responsible_parent); + } throw std::runtime_error("Non-owning holder (loaded_as_shared_ptr)."); } auto *void_raw_ptr = hld.template as_raw_ptr_unowned(); @@ -579,6 +605,12 @@ struct smart_holder_type_caster_load { return result; } + static std::shared_ptr shared_ptr_from_python(handle responsible_parent) { + smart_holder_type_caster_load loader; + loader.load(responsible_parent, false); + return loader.loaded_as_shared_ptr(responsible_parent); + } + private: modified_type_caster_generic_load_impl load_impl; diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index eef03cf74f..93de6ec9c3 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -1586,7 +1586,8 @@ struct property_cpp_function< template = 0> static cpp_function readonly(PM pm, const handle &hdl) { return cpp_function( - [pm](const std::shared_ptr &c_sp) -> std::shared_ptr { + [pm](handle c_hdl) -> std::shared_ptr { + std::shared_ptr c_sp = detail::type_caster::shared_ptr_from_python(c_hdl); D ptr = (*c_sp).*pm; return std::shared_ptr(c_sp, ptr); }, @@ -1622,8 +1623,8 @@ struct property_cpp_function< template = 0> static cpp_function readonly(PM pm, const handle &hdl) { return cpp_function( - [pm](const std::shared_ptr &c_sp) - -> std::shared_ptr::type> { + [pm](handle c_hdl) -> std::shared_ptr::type> { + std::shared_ptr c_sp = detail::type_caster::shared_ptr_from_python(c_hdl); return std::shared_ptr::type>(c_sp, &(c_sp.get()->*pm)); }, is_method(hdl)); @@ -1632,7 +1633,8 @@ struct property_cpp_function< template = 0> static cpp_function read(PM pm, const handle &hdl) { return cpp_function( - [pm](const std::shared_ptr &c_sp) -> std::shared_ptr { + [pm](handle c_hdl) -> std::shared_ptr { + std::shared_ptr c_sp = detail::type_caster::shared_ptr_from_python(c_hdl); return std::shared_ptr(c_sp, &(c_sp.get()->*pm)); }, is_method(hdl)); @@ -1669,7 +1671,10 @@ struct property_cpp_function< template = 0> static cpp_function read(PM pm, const handle &hdl) { return cpp_function( - [pm](const std::shared_ptr &c_sp) -> D { return D{std::move(c_sp.get()->*pm)}; }, + [pm](handle c_hdl) -> D { + std::shared_ptr c_sp = detail::type_caster::shared_ptr_from_python(c_hdl); + return D{std::move(c_sp.get()->*pm)}; + }, is_method(hdl)); } From 838643b1fdf14794caa087e03d98f8c7f16c1c18 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Thu, 23 Mar 2023 16:36:24 -0700 Subject: [PATCH 3/4] Complete tests. --- tests/test_class_sh_property_non_owning.cpp | 28 ++++++++++------ tests/test_class_sh_property_non_owning.py | 36 ++++++++++++++------- 2 files changed, 44 insertions(+), 20 deletions(-) diff --git a/tests/test_class_sh_property_non_owning.cpp b/tests/test_class_sh_property_non_owning.cpp index e384a4e16c..5be72007be 100644 --- a/tests/test_class_sh_property_non_owning.cpp +++ b/tests/test_class_sh_property_non_owning.cpp @@ -7,13 +7,19 @@ namespace test_class_sh_property_non_owning { struct CoreField { - CoreField(int int_value = -99) : int_value{int_value} {} + explicit CoreField(int int_value = -99) : int_value{int_value} {} int int_value; }; struct DataField { - DataField(const CoreField &core_fld = CoreField{}) : core_fld{core_fld} {} - CoreField core_fld; + DataField(int i_value, int i_shared, int i_unique) + : core_fld_value{i_value}, core_fld_shared_ptr{new CoreField{i_shared}}, + core_fld_raw_ptr{core_fld_shared_ptr.get()}, core_fld_unique_ptr{ + new CoreField{i_unique}} {} + CoreField core_fld_value; + std::shared_ptr core_fld_shared_ptr; + CoreField *core_fld_raw_ptr; + std::unique_ptr core_fld_unique_ptr; }; struct DataFieldsHolder { @@ -23,7 +29,8 @@ struct DataFieldsHolder { public: DataFieldsHolder(std::size_t vec_size) { for (std::size_t i = 0; i < vec_size; i++) { - vec.push_back(DataField{CoreField{13 + static_cast(i) * 11}}); + int i11 = static_cast(i) * 11; + vec.push_back(DataField(13 + i11, 14 + i11, 15 + i11)); } } @@ -44,13 +51,16 @@ PYBIND11_SMART_HOLDER_TYPE_CASTERS(DataField) PYBIND11_SMART_HOLDER_TYPE_CASTERS(DataFieldsHolder) TEST_SUBMODULE(class_sh_property_non_owning, m) { - py::classh(m, "CoreField") - .def(py::init<>()) - .def_readwrite("int_value", &CoreField::int_value); + py::classh(m, "CoreField").def_readwrite("int_value", &CoreField::int_value); py::classh(m, "DataField") - .def(py::init<>()) - .def_readwrite("core_fld", &DataField::core_fld); + .def_readonly("core_fld_value_ro", &DataField::core_fld_value) + .def_readwrite("core_fld_value_rw", &DataField::core_fld_value) + .def_readonly("core_fld_shared_ptr_ro", &DataField::core_fld_shared_ptr) + .def_readwrite("core_fld_shared_ptr_rw", &DataField::core_fld_shared_ptr) + .def_readonly("core_fld_raw_ptr_ro", &DataField::core_fld_raw_ptr) + .def_readwrite("core_fld_raw_ptr_rw", &DataField::core_fld_raw_ptr) + .def_readwrite("core_fld_unique_ptr_rw", &DataField::core_fld_unique_ptr); py::classh(m, "DataFieldsHolder") .def(py::init()) diff --git a/tests/test_class_sh_property_non_owning.py b/tests/test_class_sh_property_non_owning.py index 1a67054861..8b6f5f4bd3 100644 --- a/tests/test_class_sh_property_non_owning.py +++ b/tests/test_class_sh_property_non_owning.py @@ -1,14 +1,28 @@ -from pybind11_tests import class_sh_property_non_owning as m - +import pytest -def test_persistent_holder(): - h = m.DataFieldsHolder(2) - c = h.vec_at(0).core_fld - assert c.int_value == 13 - assert h.vec_at(1).core_fld.int_value == 24 +from pybind11_tests import class_sh_property_non_owning as m -def test_temporary_holder(): - d = m.DataFieldsHolder(2).vec_at(1) - c = d.core_fld - assert c.int_value == 24 +@pytest.mark.parametrize("persistent_holder", [True, False]) +@pytest.mark.parametrize( + ("core_fld", "expected"), + [ + ("core_fld_value_ro", (13, 24)), + ("core_fld_value_rw", (13, 24)), + ("core_fld_shared_ptr_ro", (14, 25)), + ("core_fld_shared_ptr_rw", (14, 25)), + ("core_fld_raw_ptr_ro", (14, 25)), + ("core_fld_raw_ptr_rw", (14, 25)), + ("core_fld_unique_ptr_rw", (15, 26)), + ], +) +def test_core_fld_common(core_fld, expected, persistent_holder): + if persistent_holder: + h = m.DataFieldsHolder(2) + for i, exp in enumerate(expected): + c = getattr(h.vec_at(i), core_fld) + assert c.int_value == exp + else: + for i, exp in enumerate(expected): + c = getattr(m.DataFieldsHolder(2).vec_at(i), core_fld) + assert c.int_value == exp From 7c4607dd25670f17366a46d29516da17c5a5e3c8 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Thu, 23 Mar 2023 16:43:16 -0700 Subject: [PATCH 4/4] Add comment for `smart_holder_type_caster_load::shared_ptr_from_python` --- include/pybind11/detail/smart_holder_type_casters.h | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/include/pybind11/detail/smart_holder_type_casters.h b/include/pybind11/detail/smart_holder_type_casters.h index cc16ad6cd0..5e0f791c3c 100644 --- a/include/pybind11/detail/smart_holder_type_casters.h +++ b/include/pybind11/detail/smart_holder_type_casters.h @@ -605,6 +605,11 @@ struct smart_holder_type_caster_load { return result; } + // This function will succeed even if the `responsible_parent` does not own the + // wrapped C++ object directly. + // It is the responsibility of the caller to ensure that the `responsible_parent` + // has a `keep_alive` relationship with the owner of the wrapped C++ object, or + // that the wrapped C++ object lives for the duration of the process. static std::shared_ptr shared_ptr_from_python(handle responsible_parent) { smart_holder_type_caster_load loader; loader.load(responsible_parent, false);