[smart_holder] Enable properties for non-owning holders (#4586)

rwgk · web-flow · commit 37d0bf42891c · 2023-03-23T17:21:41.000-07:00
* Add test_class_sh_property_non_owning.cpp,py

Failing:

```
__________________________________________________________ test_persistent_holder __________________________________________________________

    def test_persistent_holder():
        h = m.DataFieldsHolder(2)
&gt;       c = h.vec_at(0).core_fld
E       RuntimeError: Non-owning holder (loaded_as_shared_ptr).

h          = &lt;pybind11_tests.class_sh_property_non_owning.DataFieldsHolder object at 0x7fabab516470&gt;

test_class_sh_property_non_owning.py:6: RuntimeError
__________________________________________________________ test_temporary_holder ___________________________________________________________

    def test_temporary_holder():
        d = m.DataFieldsHolder(2).vec_at(1)
&gt;       c = d.core_fld
E       RuntimeError: Non-owning holder (loaded_as_shared_ptr).

d          = &lt;pybind11_tests.class_sh_property_non_owning.DataField object at 0x7fabab548770&gt;

test_class_sh_property_non_owning.py:13: RuntimeError
```

* Introduce `shared_ptr_from_python(responsible_parent)` and use in all `property_cpp_function`s with `const shared_ptr&lt;T&gt; &amp;` arguments.

Tests are incomplete.

* Complete tests.

* Add comment for `smart_holder_type_caster_load&lt;T&gt;::shared_ptr_from_python`
diff --git 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<T> loaded_as_shared_ptr() const {
+    std::shared_ptr<T> make_shared_ptr_with_responsible_parent(handle parent) const {
+        return std::shared_ptr<T>(loaded_as_raw_ptr_unowned(),
+                                  shared_ptr_parent_life_support(parent.ptr()));
+    }
+
+    std::shared_ptr<T> 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<void>();
@@ -579,6 +605,17 @@ 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<T> shared_ptr_from_python(handle responsible_parent) {
+        smart_holder_type_caster_load<T> 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
@@ -1586,7 +1586,8 @@ struct property_cpp_function<
     template <typename PM, must_be_member_function_pointer<PM> = 0>
     static cpp_function readonly(PM pm, const handle &hdl) {
         return cpp_function(
-            [pm](const std::shared_ptr<T> &c_sp) -> std::shared_ptr<drp> {
+            [pm](handle c_hdl) -> std::shared_ptr<drp> {
+                std::shared_ptr<T> c_sp = detail::type_caster<T>::shared_ptr_from_python(c_hdl);
                 D ptr = (*c_sp).*pm;
                 return std::shared_ptr<drp>(c_sp, ptr);
             },
@@ -1622,8 +1623,8 @@ struct property_cpp_function<
     template <typename PM, must_be_member_function_pointer<PM> = 0>
     static cpp_function readonly(PM pm, const handle &hdl) {
         return cpp_function(
-            [pm](const std::shared_ptr<T> &c_sp)
-                -> std::shared_ptr<typename std::add_const<D>::type> {
+            [pm](handle c_hdl) -> std::shared_ptr<typename std::add_const<D>::type> {
+                std::shared_ptr<T> c_sp = detail::type_caster<T>::shared_ptr_from_python(c_hdl);
                 return std::shared_ptr<typename std::add_const<D>::type>(c_sp, &(c_sp.get()->*pm));
             },
             is_method(hdl));
@@ -1632,7 +1633,8 @@ struct property_cpp_function<
     template <typename PM, must_be_member_function_pointer<PM> = 0>
     static cpp_function read(PM pm, const handle &hdl) {
         return cpp_function(
-            [pm](const std::shared_ptr<T> &c_sp) -> std::shared_ptr<D> {
+            [pm](handle c_hdl) -> std::shared_ptr<D> {
+                std::shared_ptr<T> c_sp = detail::type_caster<T>::shared_ptr_from_python(c_hdl);
                 return std::shared_ptr<D>(c_sp, &(c_sp.get()->*pm));
             },
             is_method(hdl));
@@ -1669,7 +1671,10 @@ struct property_cpp_function<
     template <typename PM, must_be_member_function_pointer<PM> = 0>
     static cpp_function read(PM pm, const handle &hdl) {
         return cpp_function(
-            [pm](const std::shared_ptr<T> &c_sp) -> D { return D{std::move(c_sp.get()->*pm)}; },
+            [pm](handle c_hdl) -> D {
+                std::shared_ptr<T> c_sp = detail::type_caster<T>::shared_ptr_from_python(c_hdl);
+                return D{std::move(c_sp.get()->*pm)};
+            },
             is_method(hdl));
     }
 
diff --git a/tests/test_class_sh_property_non_owning.cpp b/tests/test_class_sh_property_non_owning.cpp
@@ -0,0 +1,68 @@
+#include "pybind11/smart_holder.h"
+#include "pybind11_tests.h"
+
+#include <cstddef>
+#include <vector>
+
+namespace test_class_sh_property_non_owning {
+
+struct CoreField {
+    explicit CoreField(int int_value = -99) : int_value{int_value} {}
+    int int_value;
+};
+
+struct DataField {
+    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<CoreField> core_fld_shared_ptr;
+    CoreField *core_fld_raw_ptr;
+    std::unique_ptr<CoreField> core_fld_unique_ptr;
+};
+
+struct DataFieldsHolder {
+private:
+    std::vector<DataField> vec;
+
+public:
+    DataFieldsHolder(std::size_t vec_size) {
+        for (std::size_t i = 0; i < vec_size; i++) {
+            int i11 = static_cast<int>(i) * 11;
+            vec.push_back(DataField(13 + i11, 14 + i11, 15 + i11));
+        }
+    }
+
+    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<CoreField>(m, "CoreField").def_readwrite("int_value", &CoreField::int_value);
+
+    py::classh<DataField>(m, "DataField")
+        .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<DataFieldsHolder>(m, "DataFieldsHolder")
+        .def(py::init<std::size_t>())
+        .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
@@ -0,0 +1,28 @@
+import pytest
+
+from pybind11_tests import class_sh_property_non_owning as m
+
+
+@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
