[smart_holder] .def_readonly, .def_readwrite adaptors (continuation of PR #3581). (#3844)
* Transferred net diff from PR #3581, as-is.
* Automatic `pre-commit run --all-files` fixes. NO manual changes.
* Removing trailing `//` (originally added to manipulate clang-format), as suggested by @charlesbeattie back in Jan/Feb under PR #3581.
* Renaming `xetter_cpp_function` to `property_cpp_function` as suggested by @rainwoodman
* Fully explain the terse variable naming scheme in test_class_sh_property (as suggested by @rainwoodman)
* Also use parametrize for readonly, readwrite (as suggested by @rainwoodman)
* Apply change suggested by @skylion007 (with clang-format).
diff --git a/.codespell-ignorelines b/.codespell-ignorelines
index 5781de0..01ff575 100644
--- a/.codespell-ignorelines
+++ b/.codespell-ignorelines
@@ -11,3 +11,4 @@
valu = other.valu;
with pytest.raises(ValueError) as excinfo:
with pytest.raises(ValueError) as exc_info:
+// valu(e), ref(erence), ptr or p (pointer), r = rvalue, m = mutable, c = const,
diff --git a/include/pybind11/detail/smart_holder_sfinae_hooks_only.h b/include/pybind11/detail/smart_holder_sfinae_hooks_only.h
index f324854..ca57cb6 100644
--- a/include/pybind11/detail/smart_holder_sfinae_hooks_only.h
+++ b/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 @@
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)
diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h
index 9f00800..989ec49 100644
--- a/include/pybind11/pybind11.h
+++ b/include/pybind11/pybind11.h
@@ -1514,6 +1514,154 @@
#endif
+// Helper for the property_cpp_function static member functions below.
+// The only purpose of these functions 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 overhead 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 property_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 (and safely) for smart_holder std::shared_ptr members.
+template <typename T, typename D, typename SFINAE = void>
+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 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 the 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 property_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 = std::forward<D>(value); },
+ is_method(hdl));
+ }
+};
+
+// smart_holder specializations for members held by-value.
+// The read functions return a shared_ptr to the member, emulating the 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 property_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 property_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));
+ }
+};
+
template <typename type_, typename... options>
class class_ : public detail::generic_type {
template <typename T>
@@ -1727,9 +1875,11 @@
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,
+ property_cpp_function<type, D>::read(pm, *this),
+ property_cpp_function<type, D>::write(pm, *this),
+ return_value_policy::reference_internal,
+ extra...);
return *this;
}
@@ -1737,8 +1887,10 @@
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,
+ property_cpp_function<type, D>::readonly(pm, *this),
+ return_value_policy::reference_internal,
+ extra...);
return *this;
}
diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt
index d9086f3..dd351d6 100644
--- a/tests/CMakeLists.txt
+++ b/tests/CMakeLists.txt
@@ -128,6 +128,7 @@
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
diff --git a/tests/test_class_sh_property.cpp b/tests/test_class_sh_property.cpp
new file mode 100644
index 0000000..35615ce
--- /dev/null
+++ b/tests/test_class_sh_property.cpp
@@ -0,0 +1,86 @@
+// The compact 4-character naming matches that in test_class_sh_basic.cpp
+// Variable names are intentionally terse, to not distract from the more important C++ type names:
+// valu(e), ref(erence), ptr or p (pointer), r = rvalue, m = mutable, c = const,
+// sh = shared_ptr, uq = unique_ptr.
+
+#include "pybind11/smart_holder.h"
+#include "pybind11_tests.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 {
+ 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);
+}
diff --git a/tests/test_class_sh_property.py b/tests/test_class_sh_property.py
new file mode 100644
index 0000000..19dd976
--- /dev/null
+++ b/tests/test_class_sh_property.py
@@ -0,0 +1,161 @@
+# The compact 4-character naming scheme (e.g. mptr, cptr, shcp) is explained at the top of
+# test_class_sh_property.cpp.
+
+import pytest
+
+import env # noqa: F401
+from pybind11_tests import class_sh_property as m
+
+
+@pytest.mark.xfail("env.PYPY", reason="gc after `del field` is apparently deferred")
+@pytest.mark.parametrize("m_attr", ("m_valu_readonly", "m_valu_readwrite"))
+def test_valu_getter(msg, m_attr):
+ # Reduced from PyCLIF test:
+ # https://github.com/google/clif/blob/c371a6d4b28d25d53a16e6d2a6d97305fb1be25a/clif/testing/python/nested_fields_test.py#L56
+ outer = m.Outer()
+ field = getattr(outer, m_attr)
+ assert field.num == -99
+ with pytest.raises(ValueError) as excinfo:
+ m.DisownOuter(outer)
+ assert msg(excinfo.value) == "Cannot disown use_count != 1 (loaded_as_unique_ptr)."
+ del field
+ m.DisownOuter(outer)
+ with pytest.raises(ValueError) as excinfo:
+ getattr(outer, m_attr)
+ assert (
+ msg(excinfo.value)
+ == "Missing value for wrapped C++ type: Python instance was disowned."
+ )
+
+
+def test_valu_setter():
+ outer = m.Outer()
+ assert outer.m_valu_readonly.num == -99
+ assert outer.m_valu_readwrite.num == -99
+ field = m.Field()
+ field.num = 35
+ outer.m_valu_readwrite = field
+ assert outer.m_valu_readonly.num == 35
+ assert outer.m_valu_readwrite.num == 35
+
+
+@pytest.mark.parametrize("m_attr", ("m_shmp", "m_shcp"))
+def test_shp(m_attr):
+ m_attr_readonly = m_attr + "_readonly"
+ m_attr_readwrite = m_attr + "_readwrite"
+ outer = m.Outer()
+ assert getattr(outer, m_attr_readonly) is None
+ assert getattr(outer, m_attr_readwrite) is None
+ field = m.Field()
+ field.num = 43
+ setattr(outer, m_attr_readwrite, field)
+ assert getattr(outer, m_attr_readonly).num == 43
+ assert getattr(outer, m_attr_readwrite).num == 43
+ getattr(outer, m_attr_readonly).num = 57
+ getattr(outer, m_attr_readwrite).num = 57
+ assert field.num == 57
+ del field
+ assert getattr(outer, m_attr_readonly).num == 57
+ assert getattr(outer, m_attr_readwrite).num == 57
+
+
+@pytest.mark.parametrize(
+ "field_type, num_default, outer_type",
+ [
+ (m.ClassicField, -88, m.ClassicOuter),
+ (m.Field, -99, m.Outer),
+ ],
+)
+@pytest.mark.parametrize("m_attr", ("m_mptr", "m_cptr"))
+@pytest.mark.parametrize("r_kind", ("_readonly", "_readwrite"))
+def test_ptr(field_type, num_default, outer_type, m_attr, r_kind):
+ m_attr_r_kind = m_attr + r_kind
+ outer = outer_type()
+ assert getattr(outer, m_attr_r_kind) is None
+ field = field_type()
+ assert field.num == num_default
+ setattr(outer, m_attr + "_readwrite", field)
+ assert getattr(outer, m_attr_r_kind).num == num_default
+ field.num = 76
+ assert getattr(outer, m_attr_r_kind).num == 76
+ # Change to -88 or -99 to demonstrate Undefined Behavior (dangling pointer).
+ if num_default == 88 and m_attr == "m_mptr":
+ del field
+ assert getattr(outer, m_attr_r_kind).num == 76
+
+
+@pytest.mark.parametrize("m_attr_readwrite", ("m_uqmp_readwrite", "m_uqcp_readwrite"))
+def test_uqp(m_attr_readwrite, msg):
+ outer = m.Outer()
+ assert getattr(outer, m_attr_readwrite) is None
+ field_orig = m.Field()
+ field_orig.num = 39
+ setattr(outer, m_attr_readwrite, field_orig)
+ with pytest.raises(ValueError) as excinfo:
+ field_orig.num
+ assert (
+ msg(excinfo.value)
+ == "Missing value for wrapped C++ type: Python instance was disowned."
+ )
+ field_retr1 = getattr(outer, m_attr_readwrite)
+ assert getattr(outer, m_attr_readwrite) is None
+ assert field_retr1.num == 39
+ field_retr1.num = 93
+ setattr(outer, m_attr_readwrite, field_retr1)
+ with pytest.raises(ValueError):
+ field_retr1.num
+ field_retr2 = getattr(outer, m_attr_readwrite)
+ assert field_retr2.num == 93
+
+
+# Proof-of-concept (POC) for safe & intuitive Python access to unique_ptr members.
+# The C++ member unique_ptr is disowned to a temporary Python object for accessing
+# an attribute of the member. After the attribute was accessed, the Python object
+# is disowned back to the C++ member unique_ptr.
+# Productizing this POC is left for a future separate PR, as needed.
+class unique_ptr_field_proxy_poc: # noqa: N801
+ def __init__(self, obj, field_name):
+ object.__setattr__(self, "__obj", obj)
+ object.__setattr__(self, "__field_name", field_name)
+
+ def __getattr__(self, *args, **kwargs):
+ return _proxy_dereference(self, getattr, *args, **kwargs)
+
+ def __setattr__(self, *args, **kwargs):
+ return _proxy_dereference(self, setattr, *args, **kwargs)
+
+ def __delattr__(self, *args, **kwargs):
+ return _proxy_dereference(self, delattr, *args, **kwargs)
+
+
+def _proxy_dereference(proxy, xxxattr, *args, **kwargs):
+ obj = object.__getattribute__(proxy, "__obj")
+ field_name = object.__getattribute__(proxy, "__field_name")
+ field = getattr(obj, field_name) # Disowns the C++ unique_ptr member.
+ assert field is not None
+ try:
+ return xxxattr(field, *args, **kwargs)
+ finally:
+ setattr(obj, field_name, field) # Disowns the temporary Python object (field).
+
+
+@pytest.mark.parametrize("m_attr", ("m_uqmp", "m_uqcp"))
+def test_unique_ptr_field_proxy_poc(m_attr, msg):
+ m_attr_readwrite = m_attr + "_readwrite"
+ outer = m.Outer()
+ field_orig = m.Field()
+ field_orig.num = 45
+ setattr(outer, m_attr_readwrite, field_orig)
+ field_proxy = unique_ptr_field_proxy_poc(outer, m_attr_readwrite)
+ assert field_proxy.num == 45
+ assert field_proxy.num == 45
+ with pytest.raises(AttributeError):
+ field_proxy.xyz
+ assert field_proxy.num == 45
+ field_proxy.num = 82
+ assert field_proxy.num == 82
+ field_proxy = unique_ptr_field_proxy_poc(outer, m_attr_readwrite)
+ assert field_proxy.num == 82
+ with pytest.raises(AttributeError):
+ del field_proxy.num
+ assert field_proxy.num == 82
