Merge pull request google#30018 from rwgk/pywrapcc_merge_sh

git merge smart_holder & test_class_sh_property_stl adjustments

Author: rwgk

include/pybind11/detail/descr.h

@@ -238,12 +238,26 @@ constexpr descr<N, Ts...> concat(const descr<N, Ts...> &descr) {
     return descr;
 }
 
+#if defined(PYBIND11_CPP17)
+template <size_t N1, size_t N2, typename... Ts1, typename... Ts2>
+constexpr descr<N1 + N2 + 2, Ts1..., Ts2...> operator,(const descr<N1, Ts1...> &a,
+                                                       const descr<N2, Ts2...> &b) {
+    // Ensure that src_loc of existing descr is used.
+    return a + const_name(", ", src_loc{nullptr, 0}) + b;
+}
+
+template <size_t N, typename... Ts, typename... Args>
+constexpr auto concat(const descr<N, Ts...> &d, const Args &...args) {
+    return (d, ..., args);
+}
+#else
 template <size_t N, typename... Ts, typename... Args>
 constexpr auto concat(const descr<N, Ts...> &d, const Args &...args)
     -> decltype(std::declval<descr<N + 2, Ts...>>() + concat(args...)) {
     // Ensure that src_loc of existing descr is used.
     return d + const_name(", ", src_loc{nullptr, 0}) + concat(args...);
 }
+#endif
 
 template <size_t N, typename... Ts>
 constexpr descr<N + 2, Ts...> type_descr(const descr<N, Ts...> &descr) {

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;
 

include/pybind11/pybind11.h

@@ -1594,7 +1594,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);
             },
@@ -1630,8 +1631,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));
@@ -1640,7 +1641,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));
@@ -1677,7 +1679,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));
     }
 

tests/CMakeLists.txt

@@ -130,6 +130,7 @@ set(PYBIND11_TEST_FILES
     test_class_sh_mi_thunks
     test_class_sh_module_local.py
     test_class_sh_property
+    test_class_sh_property_non_owning
     test_class_sh_property_stl
     test_class_sh_shared_ptr_copy_move
     test_class_sh_trampoline_basic

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:
+    explicit DataFieldsHolder(std::size_t vec_size) {
+        for (std::size_t i = 0; i < vec_size; i++) {
+            int i11 = static_cast<int>(i) * 11;
+            vec.emplace_back(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);
+}

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

tests/test_class_sh_property_stl.cpp

@@ -3,6 +3,7 @@
 
 #include "pybind11_tests.h"
 
+#include <cstddef>
 #include <vector>
 
 namespace test_class_sh_property_stl {
@@ -20,6 +21,11 @@ struct FieldHolder {
 struct VectorFieldHolder {
     std::vector<FieldHolder> vec_fld_hld;
     VectorFieldHolder() { vec_fld_hld.push_back(FieldHolder{Field{300}}); }
+    void reset_at(std::size_t index, int wrapped_int) {
+        if (index < vec_fld_hld.size()) {
+            vec_fld_hld[index].fld.wrapped_int = wrapped_int;
+        }
+    }
 };
 
 } // namespace test_class_sh_property_stl
@@ -37,6 +43,7 @@ TEST_SUBMODULE(class_sh_property_stl, m) {
 
     py::classh<VectorFieldHolder>(m, "VectorFieldHolder")
         .def(py::init<>())
+        .def("reset_at", &VectorFieldHolder::reset_at)
         .def_readwrite("vec_fld_hld_ref", &VectorFieldHolder::vec_fld_hld)
         .def_readwrite("vec_fld_hld_cpy",
                        &VectorFieldHolder::vec_fld_hld,

tests/test_class_sh_property_stl.py

@@ -1,17 +1,31 @@
-import pytest
-
 from pybind11_tests import class_sh_property_stl as m
 
 
-def test_vec_fld_hld_ref():
-    vfh = m.VectorFieldHolder()
-    vfh0 = vfh.vec_fld_hld_ref[0]
-    with pytest.raises(RuntimeError) as exc_info:
-        vfh0.fld
-    assert str(exc_info.value) == "Non-owning holder (loaded_as_shared_ptr)."
+def test_cpy_after_ref():
+    h = m.VectorFieldHolder()
+    c1 = h.vec_fld_hld_cpy
+    c2 = h.vec_fld_hld_cpy
+    assert repr(c2) != repr(c1)
+    r1 = h.vec_fld_hld_ref
+    assert repr(r1) != repr(c2)
+    assert repr(r1) != repr(c1)
+    r2 = h.vec_fld_hld_ref
+    assert repr(r2) == repr(r1)
+    c3 = h.vec_fld_hld_cpy
+    assert repr(c3) == repr(r1)  # SURPRISE!
+
+
+def test_persistent_holder():
+    h = m.VectorFieldHolder()
+    c0 = h.vec_fld_hld_cpy[0]  # Must be first. See test_cpy_after_ref().
+    r0 = h.vec_fld_hld_ref[0]  # Must be second.
+    assert c0.fld.wrapped_int == 300
+    assert r0.fld.wrapped_int == 300
+    h.reset_at(0, 400)
+    assert c0.fld.wrapped_int == 300
+    assert r0.fld.wrapped_int == 400
 
 
-def test_vec_fld_hld_cpy():
-    vfh = m.VectorFieldHolder()
-    vfh0 = vfh.vec_fld_hld_cpy[0]
-    assert vfh0.fld.wrapped_int == 300
+def test_temporary_holder_keep_alive():
+    r0 = m.VectorFieldHolder().vec_fld_hld_ref[0]
+    assert r0.fld.wrapped_int == 300