[smart_holder] .def_readonly, .def_readwrite adaptors

include/pybind11/detail/smart_holder_sfinae_hooks_only.h

@@ -6,6 +6,7 @@
 
 #include "common.h"
 
+#include <memory>
 #include <type_traits>
 
 #ifndef PYBIND11_USE_SMART_HOLDER_AS_DEFAULT
@@ -29,5 +30,17 @@ struct smart_holder_type_caster_base_tag {};
 template <typename T>
 struct type_uses_smart_holder_type_caster;
 
+// Simple helpers that may eventually be a better fit for another header file:
+
+template <typename T>
+struct is_std_unique_ptr : std::false_type {};
+template <typename T, typename D>
+struct is_std_unique_ptr<std::unique_ptr<T, D>> : std::true_type {};
+
+template <typename T>
+struct is_std_shared_ptr : std::false_type {};
+template <typename T>
+struct is_std_shared_ptr<std::shared_ptr<T>> : std::true_type {};
+
 PYBIND11_NAMESPACE_END(detail)
 PYBIND11_NAMESPACE_END(PYBIND11_NAMESPACE)

include/pybind11/pybind11.h

@@ -1377,6 +1377,155 @@ using default_holder_type = smart_holder;
         }
 
 #endif
+
+// Helper for the xetter_cpp_function static member functions below. xetter_cpp_function is
+// a shortcut for 'getter & setter adapters for .def_readonly & .def_readwrite'.
+// The only purpose of these adapters is to support .def_readonly & .def_readwrite.
+// In this context, the PM template parameter is certain to be a Pointer to a Member.
+// The main purpose of must_be_member_function_pointer is to make this obvious, and to guard
+// against accidents. As a side-effect, it also explains why the syntactical clutter for
+// perfect forwarding is not needed.
+template <typename PM>
+using must_be_member_function_pointer
+    = detail::enable_if_t<std::is_member_pointer<PM>::value, int>;
+
+// Note that xetter_cpp_function is intentionally in the main pybind11 namespace,
+// because user-defined specializations could be useful.
+
+// Classic (non-smart_holder) implementations for .def_readonly and .def_readwrite
+// getter and setter functions.
+// WARNING: This classic implementation can lead to dangling pointers for raw pointer members.
+// See test_ptr() in tests/test_class_sh_property.py
+// This implementation works as-is for smart_holder std::shared_ptr members.
+template <typename T, typename D, typename SFINAE = void>
+struct xetter_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 T &c) -> const D & { return c.*pm; }, is_method(hdl));
+    }
+
+    template <typename PM, must_be_member_function_pointer<PM> = 0>
+    static cpp_function read(PM pm, const handle &hdl) {
+        return readonly(pm, hdl);
+    }
+
+    template <typename PM, must_be_member_function_pointer<PM> = 0>
+    static cpp_function write(PM pm, const handle &hdl) {
+        return cpp_function([pm](T &c, const D &value) { c.*pm = value; }, is_method(hdl));
+    }
+};
+
+// smart_holder specializations for raw pointer members.
+// WARNING: Like the classic implementation, this implementation can lead to dangling pointers.
+// See test_ptr() in tests/test_class_sh_property.py
+// However, the read functions return a shared_ptr to the member, emulating PyCLIF approach:
+// https://github.com/google/clif/blob/c371a6d4b28d25d53a16e6d2a6d97305fb1be25a/clif/python/instance.h#L233
+// This prevents disowning of the Python object owning the raw pointer member.
+template <typename T, typename D>
+struct xetter_cpp_function<
+    T,
+    D,
+    detail::enable_if_t<detail::all_of<detail::type_uses_smart_holder_type_caster<T>,
+                                       detail::type_uses_smart_holder_type_caster<D>,
+                                       std::is_pointer<D>>::value>> {
+
+    using drp = typename std::remove_pointer<D>::type;
+
+    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> {
+                D ptr = (*c_sp).*pm;
+                return std::shared_ptr<drp>(c_sp, ptr);
+            },
+            is_method(hdl));
+    }
+
+    template <typename PM, must_be_member_function_pointer<PM> = 0>
+    static cpp_function read(PM pm, const handle &hdl) {
+        return readonly(pm, hdl);
+    }
+
+    template <typename PM, must_be_member_function_pointer<PM> = 0>
+    static cpp_function write(PM pm, const handle &hdl) {
+        return cpp_function([pm](T &c, D value) { c.*pm = value; }, is_method(hdl));
+    }
+};
+
+// smart_holder specializations for members held by-value.
+// The read functions return a shared_ptr to the member, emulating PyCLIF approach:
+// https://github.com/google/clif/blob/c371a6d4b28d25d53a16e6d2a6d97305fb1be25a/clif/python/instance.h#L233
+// This prevents disowning of the Python object owning the member.
+template <typename T, typename D>
+struct xetter_cpp_function<
+    T,
+    D,
+    detail::enable_if_t<detail::all_of<detail::type_uses_smart_holder_type_caster<T>,
+                                       detail::type_uses_smart_holder_type_caster<D>,
+                                       detail::none_of<std::is_pointer<D>,
+                                                       detail::is_std_unique_ptr<D>,
+                                                       detail::is_std_shared_ptr<D>>>::value>> {
+
+    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> {
+                return std::shared_ptr<typename std::add_const<D>::type>(c_sp, &(c_sp.get()->*pm));
+            },
+            is_method(hdl));
+    }
+
+    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> {
+                return std::shared_ptr<D>(c_sp, &(c_sp.get()->*pm));
+            },
+            is_method(hdl));
+    }
+
+    template <typename PM, must_be_member_function_pointer<PM> = 0>
+    static cpp_function write(PM pm, const handle &hdl) {
+        return cpp_function([pm](T &c, const D &value) { c.*pm = value; }, is_method(hdl));
+    }
+};
+
+// smart_holder specializations for std::unique_ptr members.
+// read disowns the member unique_ptr.
+// write disowns the passed Python object.
+// readonly is disabled (static_assert) because there is no safe & intuitive way to make the member
+// accessible as a Python object without disowning the member unique_ptr. A .def_readonly disowning
+// the unique_ptr member is deemed highly prone to misunderstandings.
+template <typename T, typename D>
+struct xetter_cpp_function<
+    T,
+    D,
+    detail::enable_if_t<detail::all_of<
+        detail::type_uses_smart_holder_type_caster<T>,
+        detail::is_std_unique_ptr<D>,
+        detail::type_uses_smart_holder_type_caster<typename D::element_type>>::value>> {
+
+    template <typename PM, must_be_member_function_pointer<PM> = 0>
+    static cpp_function readonly(PM, const handle &) {
+        static_assert(!detail::is_std_unique_ptr<D>::value,
+                      "def_readonly cannot be used for std::unique_ptr members.");
+        return cpp_function{}; // Unreachable.
+    }
+
+    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)}; },
+            is_method(hdl));
+    }
+
+    template <typename PM, must_be_member_function_pointer<PM> = 0>
+    static cpp_function write(PM pm, const handle &hdl) {
+        return cpp_function([pm](T &c, D &&value) { c.*pm = std::move(value); }, is_method(hdl));
+    }
+};
+
 // clang-format off
 
 template <typename type_, typename... options>
@@ -1577,17 +1726,23 @@ class class_ : public detail::generic_type {
     template <typename C, typename D, typename... Extra>
     class_ &def_readwrite(const char *name, D C::*pm, const Extra&... extra) {
         static_assert(std::is_same<C, type>::value || std::is_base_of<C, type>::value, "def_readwrite() requires a class member (or base class member)");
-        cpp_function fget([pm](const type &c) -> const D &{ return c.*pm; }, is_method(*this)),
-                     fset([pm](type &c, const D &value) { c.*pm = value; }, is_method(*this));
-        def_property(name, fget, fset, return_value_policy::reference_internal, extra...);
+        def_property(
+            name,
+            xetter_cpp_function<type, D>::read(pm, *this),
+            xetter_cpp_function<type, D>::write(pm, *this),
+            return_value_policy::reference_internal,
+            extra...);
         return *this;
     }
 
     template <typename C, typename D, typename... Extra>
     class_ &def_readonly(const char *name, const D C::*pm, const Extra& ...extra) {
         static_assert(std::is_same<C, type>::value || std::is_base_of<C, type>::value, "def_readonly() requires a class member (or base class member)");
-        cpp_function fget([pm](const type &c) -> const D &{ return c.*pm; }, is_method(*this));
-        def_property_readonly(name, fget, return_value_policy::reference_internal, extra...);
+        def_property_readonly(
+            name,
+            xetter_cpp_function<type, D>::readonly(pm, *this),
+            return_value_policy::reference_internal,
+            extra...);
         return *this;
     }
 

tests/CMakeLists.txt

@@ -128,6 +128,7 @@ set(PYBIND11_TEST_FILES
     test_class_sh_factory_constructors
     test_class_sh_inheritance
     test_class_sh_module_local.py
+    test_class_sh_property
     test_class_sh_shared_ptr_copy_move
     test_class_sh_trampoline_basic
     test_class_sh_trampoline_self_life_support

tests/test_class_sh_property.cpp

@@ -0,0 +1,90 @@
+#include "pybind11_tests.h"
+
+#include "pybind11/smart_holder.h"
+
+#include <memory>
+
+namespace test_class_sh_property {
+
+struct ClassicField {
+    int num = -88;
+};
+
+struct ClassicOuter {
+    ClassicField *m_mptr = nullptr;
+    const ClassicField *m_cptr = nullptr;
+};
+
+struct Field {
+    int num = -99;
+};
+
+struct Outer {
+    // The compact 4-character naming matches that in test_class_sh_basic.cpp
+    // (c = const, m = mutable).
+    Field m_valu;
+    Field *m_mptr = nullptr;
+    const Field *m_cptr = nullptr;
+    std::unique_ptr<Field> m_uqmp;
+    std::unique_ptr<const Field> m_uqcp;
+    std::shared_ptr<Field> m_shmp;
+    std::shared_ptr<const Field> m_shcp;
+};
+
+inline void DisownOuter(std::unique_ptr<Outer>) {}
+
+} // namespace test_class_sh_property
+
+PYBIND11_TYPE_CASTER_BASE_HOLDER(test_class_sh_property::ClassicField,
+                                 std::unique_ptr<test_class_sh_property::ClassicField>)
+PYBIND11_TYPE_CASTER_BASE_HOLDER(test_class_sh_property::ClassicOuter,
+                                 std::unique_ptr<test_class_sh_property::ClassicOuter>)
+
+PYBIND11_SMART_HOLDER_TYPE_CASTERS(test_class_sh_property::Field)
+PYBIND11_SMART_HOLDER_TYPE_CASTERS(test_class_sh_property::Outer)
+
+TEST_SUBMODULE(class_sh_property, m) {
+    using namespace test_class_sh_property;
+
+    py::class_<ClassicField, std::unique_ptr<ClassicField>>(m, "ClassicField")
+        .def(py::init<>())                        //
+        .def_readwrite("num", &ClassicField::num) //
+        ;
+
+    py::class_<ClassicOuter, std::unique_ptr<ClassicOuter>>(m, "ClassicOuter")
+        .def(py::init<>())
+        .def_readonly("m_mptr_readonly", &ClassicOuter::m_mptr)
+        .def_readwrite("m_mptr_readwrite", &ClassicOuter::m_mptr)
+        .def_readwrite("m_cptr_readonly", &ClassicOuter::m_cptr)
+        .def_readwrite("m_cptr_readwrite", &ClassicOuter::m_cptr) //
+        ;
+
+    py::classh<Field>(m, "Field")          //
+        .def(py::init<>())                 //
+        .def_readwrite("num", &Field::num) //
+        ;
+
+    py::classh<Outer>(m, "Outer") //
+        .def(py::init<>())
+
+        .def_readonly("m_valu_readonly", &Outer::m_valu)
+        .def_readwrite("m_valu_readwrite", &Outer::m_valu)
+
+        .def_readonly("m_mptr_readonly", &Outer::m_mptr)
+        .def_readwrite("m_mptr_readwrite", &Outer::m_mptr)
+        .def_readonly("m_cptr_readonly", &Outer::m_cptr)
+        .def_readwrite("m_cptr_readwrite", &Outer::m_cptr)
+
+        // .def_readonly("m_uqmp_readonly", &Outer::m_uqmp) // Custom compilation Error.
+        .def_readwrite("m_uqmp_readwrite", &Outer::m_uqmp)
+        // .def_readonly("m_uqcp_readonly", &Outer::m_uqcp) // Custom compilation Error.
+        .def_readwrite("m_uqcp_readwrite", &Outer::m_uqcp)
+
+        .def_readwrite("m_shmp_readonly", &Outer::m_shmp)
+        .def_readwrite("m_shmp_readwrite", &Outer::m_shmp)
+        .def_readwrite("m_shcp_readonly", &Outer::m_shcp)
+        .def_readwrite("m_shcp_readwrite", &Outer::m_shcp) //
+        ;
+
+    m.def("DisownOuter", DisownOuter);
+}