diff --git a/docs/advanced/classes.rst b/docs/advanced/classes.rst index 0ada0cb60c..7c0ec10e79 100644 --- a/docs/advanced/classes.rst +++ b/docs/advanced/classes.rst @@ -82,15 +82,21 @@ helper class that is defined as follows: The ``py::trampoline_self_life_support`` base class is needed to ensure that a ``std::unique_ptr`` can safely be passed between Python and C++. To -steer clear of notorious pitfalls (e.g. inheritance slicing), it is best -practice to always use the base class, in combination with +help you steer clear of notorious pitfalls (e.g. inheritance slicing), +pybind11 enforces that trampoline classes inherit from +``py::trampoline_self_life_support`` if used in in combination with ``py::smart_holder``. .. note:: For completeness, the base class has no effect if a holder other than ``py::smart_holder`` used, including the default ``std::unique_ptr``. - Please think twice, though, the pitfalls are very real, and the overhead - for using the safer ``py::smart_holder`` is very likely to be in the noise. + To avoid confusion, pybind11 will fail to compile bindings that combine + ``py::trampoline_self_life_support`` with a holder other than + ``py::smart_holder``. + + Please think twice, though, before deciding to not use the safer + ``py::smart_holder``. The pitfalls associated with avoiding it are very + real, and the overhead for using it is very likely in the noise. The macro :c:macro:`PYBIND11_OVERRIDE_PURE` should be used for pure virtual functions, and :c:macro:`PYBIND11_OVERRIDE` should be used for functions which have diff --git a/include/pybind11/detail/type_caster_base.h b/include/pybind11/detail/type_caster_base.h index d4f9a41e0c..f968c1a3ca 100644 --- a/include/pybind11/detail/type_caster_base.h +++ b/include/pybind11/detail/type_caster_base.h @@ -822,11 +822,8 @@ struct load_helper : value_and_holder_helper { auto *self_life_support = dynamic_raw_ptr_cast_if_possible(raw_type_ptr); - if (self_life_support == nullptr && python_instance_is_alias) { - throw value_error("Alias class (also known as trampoline) does not inherit from " - "py::trampoline_self_life_support, therefore the ownership of this " - "instance cannot safely be transferred to C++."); - } + // This is enforced indirectly by a static_assert in the class_ implementation: + assert(!python_instance_is_alias || self_life_support); std::unique_ptr extracted_deleter = holder().template extract_deleter(context); diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index c84b24d3b7..e05380947c 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -20,6 +20,7 @@ #include "gil.h" #include "gil_safe_call_once.h" #include "options.h" +#include "trampoline_self_life_support.h" #include "typing.h" #include @@ -1982,7 +1983,21 @@ class class_ : public detail::generic_type { "Unknown/invalid class_ template parameters provided"); static_assert(!has_alias || std::is_polymorphic::value, - "Cannot use an alias class with a non-polymorphic type"); + "Cannot use an alias class (aka trampoline) with a non-polymorphic type"); + +#ifndef PYBIND11_RUN_TESTING_WITH_SMART_HOLDER_AS_DEFAULT_BUT_NEVER_USE_IN_PRODUCTION_PLEASE + static_assert(!has_alias || !detail::is_smart_holder::value + || std::is_base_of::value, + "Alias class (aka trampoline) must inherit from" + " pybind11::trampoline_self_life_support if used in combination with" + " pybind11::smart_holder"); +#endif + static_assert(!has_alias || detail::is_smart_holder::value + || !std::is_base_of::value, + "pybind11::trampoline_self_life_support is a smart_holder feature, therefore" + " an alias class (aka trampoline) should inherit from" + " pybind11::trampoline_self_life_support only if used in combination with" + " pybind11::smart_holder"); PYBIND11_OBJECT(class_, generic_type, PyType_Check) diff --git a/tests/test_class_sh_factory_constructors.cpp b/tests/test_class_sh_factory_constructors.cpp index b3a8daea51..574ab26a2b 100644 --- a/tests/test_class_sh_factory_constructors.cpp +++ b/tests/test_class_sh_factory_constructors.cpp @@ -64,7 +64,7 @@ struct with_alias { with_alias &operator=(const with_alias &) = default; with_alias &operator=(with_alias &&) = default; }; -struct with_alias_alias : with_alias {}; +struct with_alias_alias : with_alias, py::trampoline_self_life_support {}; struct sddwaa : std::default_delete {}; } // namespace class_sh_factory_constructors diff --git a/tests/test_class_sh_trampoline_basic.cpp b/tests/test_class_sh_trampoline_basic.cpp index 0f42dcc717..1ceb11ba83 100644 --- a/tests/test_class_sh_trampoline_basic.cpp +++ b/tests/test_class_sh_trampoline_basic.cpp @@ -5,7 +5,6 @@ namespace pybind11_tests { namespace class_sh_trampoline_basic { -template // Using int as a trick to easily generate a series of types. struct Abase { int val = 0; virtual ~Abase() = default; @@ -20,63 +19,39 @@ struct Abase { Abase &operator=(Abase &&) noexcept = default; }; -template -struct AbaseAlias : Abase { - using Abase::Abase; +struct AbaseAlias : Abase, py::trampoline_self_life_support { + using Abase::Abase; int Add(int other_val) const override { - PYBIND11_OVERRIDE_PURE(int, /* Return type */ - Abase, /* Parent class */ - Add, /* Name of function in C++ (must match Python name) */ + PYBIND11_OVERRIDE_PURE(int, /* Return type */ + Abase, /* Parent class */ + Add, /* Name of function in C++ (must match Python name) */ other_val); } }; -template <> -struct AbaseAlias<1> : Abase<1>, py::trampoline_self_life_support { - using Abase<1>::Abase; +int AddInCppRawPtr(const Abase *obj, int other_val) { return obj->Add(other_val) * 10 + 7; } - int Add(int other_val) const override { - PYBIND11_OVERRIDE_PURE(int, /* Return type */ - Abase<1>, /* Parent class */ - Add, /* Name of function in C++ (must match Python name) */ - other_val); - } -}; - -template -int AddInCppRawPtr(const Abase *obj, int other_val) { - return obj->Add(other_val) * 10 + 7; -} - -template -int AddInCppSharedPtr(std::shared_ptr> obj, int other_val) { +int AddInCppSharedPtr(const std::shared_ptr &obj, int other_val) { return obj->Add(other_val) * 100 + 11; } -template -int AddInCppUniquePtr(std::unique_ptr> obj, int other_val) { +int AddInCppUniquePtr(std::unique_ptr obj, int other_val) { return obj->Add(other_val) * 100 + 13; } -template -void wrap(py::module_ m, const char *py_class_name) { - py::classh, AbaseAlias>(m, py_class_name) - .def(py::init(), py::arg("val")) - .def("Get", &Abase::Get) - .def("Add", &Abase::Add, py::arg("other_val")); - - m.def("AddInCppRawPtr", AddInCppRawPtr, py::arg("obj"), py::arg("other_val")); - m.def("AddInCppSharedPtr", AddInCppSharedPtr, py::arg("obj"), py::arg("other_val")); - m.def("AddInCppUniquePtr", AddInCppUniquePtr, py::arg("obj"), py::arg("other_val")); -} - } // namespace class_sh_trampoline_basic } // namespace pybind11_tests using namespace pybind11_tests::class_sh_trampoline_basic; TEST_SUBMODULE(class_sh_trampoline_basic, m) { - wrap<0>(m, "Abase0"); - wrap<1>(m, "Abase1"); + py::classh(m, "Abase") + .def(py::init(), py::arg("val")) + .def("Get", &Abase::Get) + .def("Add", &Abase::Add, py::arg("other_val")); + + m.def("AddInCppRawPtr", AddInCppRawPtr, py::arg("obj"), py::arg("other_val")); + m.def("AddInCppSharedPtr", AddInCppSharedPtr, py::arg("obj"), py::arg("other_val")); + m.def("AddInCppUniquePtr", AddInCppUniquePtr, py::arg("obj"), py::arg("other_val")); } diff --git a/tests/test_class_sh_trampoline_basic.py b/tests/test_class_sh_trampoline_basic.py index eab82121fb..0020a14a70 100644 --- a/tests/test_class_sh_trampoline_basic.py +++ b/tests/test_class_sh_trampoline_basic.py @@ -1,11 +1,9 @@ from __future__ import annotations -import pytest - from pybind11_tests import class_sh_trampoline_basic as m -class PyDrvd0(m.Abase0): +class PyDrvd(m.Abase): def __init__(self, val): super().__init__(val) @@ -13,47 +11,25 @@ def Add(self, other_val): return self.Get() * 100 + other_val -class PyDrvd1(m.Abase1): - def __init__(self, val): - super().__init__(val) - - def Add(self, other_val): - return self.Get() * 200 + other_val - - -def test_drvd0_add(): - drvd = PyDrvd0(74) +def test_drvd_add(): + drvd = PyDrvd(74) assert drvd.Add(38) == (74 * 10 + 3) * 100 + 38 -def test_drvd0_add_in_cpp_raw_ptr(): - drvd = PyDrvd0(52) +def test_drvd_add_in_cpp_raw_ptr(): + drvd = PyDrvd(52) assert m.AddInCppRawPtr(drvd, 27) == ((52 * 10 + 3) * 100 + 27) * 10 + 7 -def test_drvd0_add_in_cpp_shared_ptr(): +def test_drvd_add_in_cpp_shared_ptr(): while True: - drvd = PyDrvd0(36) + drvd = PyDrvd(36) assert m.AddInCppSharedPtr(drvd, 56) == ((36 * 10 + 3) * 100 + 56) * 100 + 11 return # Comment out for manual leak checking (use `top` command). -def test_drvd0_add_in_cpp_unique_ptr(): - while True: - drvd = PyDrvd0(0) - with pytest.raises(ValueError) as exc_info: - m.AddInCppUniquePtr(drvd, 0) - assert ( - str(exc_info.value) - == "Alias class (also known as trampoline) does not inherit from" - " py::trampoline_self_life_support, therefore the ownership of this" - " instance cannot safely be transferred to C++." - ) - return # Comment out for manual leak checking (use `top` command). - - -def test_drvd1_add_in_cpp_unique_ptr(): +def test_drvd_add_in_cpp_unique_ptr(): while True: - drvd = PyDrvd1(25) - assert m.AddInCppUniquePtr(drvd, 83) == ((25 * 10 + 3) * 200 + 83) * 100 + 13 + drvd = PyDrvd(25) + assert m.AddInCppUniquePtr(drvd, 83) == ((25 * 10 + 3) * 100 + 83) * 100 + 13 return # Comment out for manual leak checking (use `top` command). diff --git a/tests/test_class_sh_trampoline_shared_ptr_cpp_arg.cpp b/tests/test_class_sh_trampoline_shared_ptr_cpp_arg.cpp index 49e1ac885d..5580848c6e 100644 --- a/tests/test_class_sh_trampoline_shared_ptr_cpp_arg.cpp +++ b/tests/test_class_sh_trampoline_shared_ptr_cpp_arg.cpp @@ -28,7 +28,7 @@ struct SpBase { std::shared_ptr pass_through_shd_ptr(const std::shared_ptr &obj) { return obj; } -struct PySpBase : SpBase { +struct PySpBase : SpBase, py::trampoline_self_life_support { using SpBase::SpBase; bool is_base_used() override { PYBIND11_OVERRIDE(bool, SpBase, is_base_used); } };