Add static_asserts to enforce that py::smart_holder is combined with py::trampoline_self_life_support (#5633)

* Strictly enforce: trampoline must inherit from trampoline_self_life_support when used in combination with smart_holder

* Simplify test_class_sh_trampoline_basic.cpp,py (only one Abase is needed now)

* Replace obsolete sophisticated `throw value_error()` with a simple `assert()`

* Strictly enforce: trampoline should inherit from trampoline_self_life_support only if used in combination with smart_holder

* 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<Abase> obj, int other_val) {
      |                                              ^
      |                       const                 &
```

* Disable new static_assert if PYBIND11_RUN_TESTING_WITH_SMART_HOLDER_AS_DEFAULT_BUT_NEVER_USE_IN_PRODUCTION_PLEASE is defined.

Author: rwgk

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<T>``.
-    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

include/pybind11/detail/type_caster_base.h

@@ -827,11 +827,8 @@ struct load_helper : value_and_holder_helper {
 
         auto *self_life_support
             = dynamic_raw_ptr_cast_if_possible<trampoline_self_life_support>(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<D> extracted_deleter = holder().template extract_deleter<T, D>(context);
 

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 <cassert>
@@ -1982,7 +1983,21 @@ class class_ : public detail::generic_type {
                   "Unknown/invalid class_ template parameters provided");
 
     static_assert(!has_alias || std::is_polymorphic<type>::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<holder_type>::value
+                      || std::is_base_of<trampoline_self_life_support, type_alias>::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<holder_type>::value
+                      || !std::is_base_of<trampoline_self_life_support, type_alias>::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)
 

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<with_alias_alias> {};
 
 } // namespace class_sh_factory_constructors

tests/test_class_sh_trampoline_basic.cpp

@@ -5,7 +5,6 @@
 namespace pybind11_tests {
 namespace class_sh_trampoline_basic {
 
-template <int SerNo> // 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 <int SerNo>
-struct AbaseAlias : Abase<SerNo> {
-    using Abase<SerNo>::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<SerNo>, /* 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 SerNo>
-int AddInCppRawPtr(const Abase<SerNo> *obj, int other_val) {
-    return obj->Add(other_val) * 10 + 7;
-}
-
-template <int SerNo>
-int AddInCppSharedPtr(std::shared_ptr<Abase<SerNo>> obj, int other_val) {
+int AddInCppSharedPtr(const std::shared_ptr<Abase> &obj, int other_val) {
     return obj->Add(other_val) * 100 + 11;
 }
 
-template <int SerNo>
-int AddInCppUniquePtr(std::unique_ptr<Abase<SerNo>> obj, int other_val) {
+int AddInCppUniquePtr(std::unique_ptr<Abase> obj, int other_val) {
     return obj->Add(other_val) * 100 + 13;
 }
 
-template <int SerNo>
-void wrap(py::module_ m, const char *py_class_name) {
-    py::classh<Abase<SerNo>, AbaseAlias<SerNo>>(m, py_class_name)
-        .def(py::init<int>(), py::arg("val"))
-        .def("Get", &Abase<SerNo>::Get)
-        .def("Add", &Abase<SerNo>::Add, py::arg("other_val"));
-
-    m.def("AddInCppRawPtr", AddInCppRawPtr<SerNo>, py::arg("obj"), py::arg("other_val"));
-    m.def("AddInCppSharedPtr", AddInCppSharedPtr<SerNo>, py::arg("obj"), py::arg("other_val"));
-    m.def("AddInCppUniquePtr", AddInCppUniquePtr<SerNo>, 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<Abase, AbaseAlias>(m, "Abase")
+        .def(py::init<int>(), 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"));
 }

tests/test_class_sh_trampoline_basic.py

@@ -1,59 +1,35 @@
 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)
 
     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).

tests/test_class_sh_trampoline_shared_ptr_cpp_arg.cpp

@@ -28,7 +28,7 @@ struct SpBase {
 
 std::shared_ptr<SpBase> pass_through_shd_ptr(const std::shared_ptr<SpBase> &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); }
 };