From b3b3a54f388e113861bfd51c8962cb417cbcb692 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Mon, 28 Apr 2025 14:32:27 -0700 Subject: [PATCH 1/6] Strictly enforce: trampoline must inherit from trampoline_self_life_support when used in combination with smart_holder --- include/pybind11/pybind11.h | 9 +++++- tests/test_class_sh_factory_constructors.cpp | 2 +- tests/test_class_sh_trampoline_basic.cpp | 19 ++----------- tests/test_class_sh_trampoline_basic.py | 28 ++----------------- ...class_sh_trampoline_shared_ptr_cpp_arg.cpp | 2 +- 5 files changed, 14 insertions(+), 46 deletions(-) diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index c84b24d3b7..7c6e0d233f 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,13 @@ 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"); + + 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 when 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..7a7ccbfbce 100644 --- a/tests/test_class_sh_trampoline_basic.cpp +++ b/tests/test_class_sh_trampoline_basic.cpp @@ -21,7 +21,7 @@ struct Abase { }; template -struct AbaseAlias : Abase { +struct AbaseAlias : Abase, py::trampoline_self_life_support { using Abase::Abase; int Add(int other_val) const override { @@ -32,18 +32,6 @@ struct AbaseAlias : Abase { } }; -template <> -struct AbaseAlias<1> : Abase<1>, py::trampoline_self_life_support { - using Abase<1>::Abase; - - 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; @@ -76,7 +64,4 @@ void wrap(py::module_ m, const char *py_class_name) { using namespace pybind11_tests::class_sh_trampoline_basic; -TEST_SUBMODULE(class_sh_trampoline_basic, m) { - wrap<0>(m, "Abase0"); - wrap<1>(m, "Abase1"); -} +TEST_SUBMODULE(class_sh_trampoline_basic, m) { wrap<0>(m, "Abase0"); } diff --git a/tests/test_class_sh_trampoline_basic.py b/tests/test_class_sh_trampoline_basic.py index eab82121fb..91235a9053 100644 --- a/tests/test_class_sh_trampoline_basic.py +++ b/tests/test_class_sh_trampoline_basic.py @@ -1,7 +1,5 @@ from __future__ import annotations -import pytest - from pybind11_tests import class_sh_trampoline_basic as m @@ -13,14 +11,6 @@ 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) assert drvd.Add(38) == (74 * 10 + 3) * 100 + 38 @@ -40,20 +30,6 @@ def test_drvd0_add_in_cpp_shared_ptr(): 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(): - while True: - drvd = PyDrvd1(25) - assert m.AddInCppUniquePtr(drvd, 83) == ((25 * 10 + 3) * 200 + 83) * 100 + 13 + drvd = PyDrvd0(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); } }; From 34ce2d9baa59f7f0e6feab78ff282bca011d60eb Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Mon, 28 Apr 2025 15:16:05 -0700 Subject: [PATCH 2/6] Simplify test_class_sh_trampoline_basic.cpp,py (only one Abase is needed now) --- tests/test_class_sh_trampoline_basic.cpp | 46 ++++++++++-------------- tests/test_class_sh_trampoline_basic.py | 18 +++++----- 2 files changed, 27 insertions(+), 37 deletions(-) diff --git a/tests/test_class_sh_trampoline_basic.cpp b/tests/test_class_sh_trampoline_basic.cpp index 7a7ccbfbce..2c7123f198 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,48 +19,39 @@ struct Abase { Abase &operator=(Abase &&) noexcept = default; }; -template -struct AbaseAlias : Abase, py::trampoline_self_life_support { - 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 -int AddInCppRawPtr(const Abase *obj, int other_val) { - return obj->Add(other_val) * 10 + 7; -} +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(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"); } +TEST_SUBMODULE(class_sh_trampoline_basic, m) { + 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 91235a9053..0020a14a70 100644 --- a/tests/test_class_sh_trampoline_basic.py +++ b/tests/test_class_sh_trampoline_basic.py @@ -3,7 +3,7 @@ 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) @@ -11,25 +11,25 @@ def Add(self, other_val): return self.Get() * 100 + 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(): +def test_drvd_add_in_cpp_unique_ptr(): while True: - drvd = PyDrvd0(25) + 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). From a502603e0aa6cf91f383448a3905775f343494a9 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Mon, 28 Apr 2025 15:22:29 -0700 Subject: [PATCH 3/6] Replace obsolete sophisticated `throw value_error()` with a simple `assert()` --- include/pybind11/detail/type_caster_base.h | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) 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); From 25108cfe87f96ce78e878017b461fb90d0739393 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Mon, 28 Apr 2025 15:36:31 -0700 Subject: [PATCH 4/6] Strictly enforce: trampoline should inherit from trampoline_self_life_support only if used in combination with smart_holder --- docs/advanced/classes.rst | 14 ++++++++++---- include/pybind11/pybind11.h | 8 +++++++- 2 files changed, 17 insertions(+), 5 deletions(-) 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/pybind11.h b/include/pybind11/pybind11.h index 7c6e0d233f..3407d86754 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -1988,7 +1988,13 @@ class class_ : public detail::generic_type { 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 when used in combination with" + " pybind11::trampoline_self_life_support if used in combination with" + " pybind11::smart_holder"); + 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) From 3863958415f599abcc59a984b4d33ecf09c06c11 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Mon, 28 Apr 2025 20:50:03 -0700 Subject: [PATCH 5/6] Resolve clang-tidy error ``` /__w/pybind11/pybind11/tests/test_class_sh_trampoline_basic.cpp:35:46: error: the parameter 'obj' is copied for each invocation but only used as a const reference; consider making it a const reference [performance-unnecessary-value-param,-warnings-as-errors] 35 | int AddInCppSharedPtr(std::shared_ptr obj, int other_val) { | ^ | const & ``` --- tests/test_class_sh_trampoline_basic.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_class_sh_trampoline_basic.cpp b/tests/test_class_sh_trampoline_basic.cpp index 2c7123f198..1ceb11ba83 100644 --- a/tests/test_class_sh_trampoline_basic.cpp +++ b/tests/test_class_sh_trampoline_basic.cpp @@ -32,7 +32,7 @@ struct AbaseAlias : Abase, py::trampoline_self_life_support { int AddInCppRawPtr(const Abase *obj, int other_val) { return obj->Add(other_val) * 10 + 7; } -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; } From a5e6e0167ee927e716aec90be03ad8e579b2a49e Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Mon, 28 Apr 2025 20:56:06 -0700 Subject: [PATCH 6/6] Disable new static_assert if PYBIND11_RUN_TESTING_WITH_SMART_HOLDER_AS_DEFAULT_BUT_NEVER_USE_IN_PRODUCTION_PLEASE is defined. --- include/pybind11/pybind11.h | 2 ++ 1 file changed, 2 insertions(+) diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index 3407d86754..e05380947c 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -1985,11 +1985,13 @@ class class_ : public detail::generic_type { static_assert(!has_alias || std::is_polymorphic::value, "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"