Bringing back ValueError: "... instance cannot safely be transferred to C++.", but based on dynamic_cast<AliasType>.

Author: rwgk

include/pybind11/detail/smart_holder_type_casters.h

@@ -263,15 +263,13 @@ struct smart_holder_type_caster_class_hooks : smart_holder_type_caster_base_tag
         return &modified_type_caster_generic_load_impl::local_load;
     }
 
-    template <typename T>
-    static void init_instance_for_type(detail::instance *inst,
-                                       const void *holder_const_void_ptr,
-                                       bool has_alias) {
+    template <typename WrappedType, typename AliasType>
+    static void init_instance_for_type(detail::instance *inst, const void *holder_const_void_ptr) {
         // Need for const_cast is a consequence of the type_info::init_instance type:
         // void (*init_instance)(instance *, const void *);
         auto holder_void_ptr = const_cast<void *>(holder_const_void_ptr);
 
-        auto v_h = inst->get_value_and_holder(detail::get_type_info(typeid(T)));
+        auto v_h = inst->get_value_and_holder(detail::get_type_info(typeid(WrappedType)));
         if (!v_h.instance_registered()) {
             register_instance(inst, v_h.value_ptr(), v_h.type);
             v_h.set_instance_registered();
@@ -282,13 +280,14 @@ struct smart_holder_type_caster_class_hooks : smart_holder_type_caster_base_tag
             auto holder_ptr = static_cast<holder_type *>(holder_void_ptr);
             new (std::addressof(v_h.holder<holder_type>())) holder_type(std::move(*holder_ptr));
         } else if (inst->owned) {
-            new (std::addressof(v_h.holder<holder_type>()))
-                holder_type(holder_type::from_raw_ptr_take_ownership(v_h.value_ptr<T>()));
+            new (std::addressof(v_h.holder<holder_type>())) holder_type(
+                holder_type::from_raw_ptr_take_ownership(v_h.value_ptr<WrappedType>()));
         } else {
             new (std::addressof(v_h.holder<holder_type>()))
-                holder_type(holder_type::from_raw_ptr_unowned(v_h.value_ptr<T>()));
+                holder_type(holder_type::from_raw_ptr_unowned(v_h.value_ptr<WrappedType>()));
         }
-        v_h.holder<holder_type>().pointee_depends_on_holder_owner = has_alias;
+        v_h.holder<holder_type>().pointee_depends_on_holder_owner
+            = dynamic_raw_ptr_cast_if_possible<AliasType>(v_h.value_ptr<WrappedType>()) != nullptr;
         v_h.set_holder_constructed();
     }
 
@@ -391,7 +390,12 @@ struct smart_holder_type_caster_load {
         T *raw_type_ptr = convert_type(raw_void_ptr);
 
         auto *self_life_support
-            = dynamic_cast_virtual_overrider_self_life_support_ptr(raw_type_ptr);
+            = dynamic_raw_ptr_cast_if_possible<virtual_overrider_self_life_support>(raw_type_ptr);
+        if (self_life_support == nullptr && holder().pointee_depends_on_holder_owner) {
+            throw value_error("Alias class (also known as trampoline) does not inherit from "
+                              "py::detail::virtual_overrider_self_life_support, therefore the "
+                              "ownership of this instance cannot safely be transferred to C++.");
+        }
 
         // Critical transfer-of-ownership section. This must stay together.
         if (self_life_support != nullptr) {

include/pybind11/detail/virtual_overrider_self_life_support.h

@@ -46,16 +46,18 @@ struct virtual_overrider_self_life_support {
         = default;
 };
 
-template <typename T, detail::enable_if_t<!std::is_polymorphic<T>::value, int> = 0>
-virtual_overrider_self_life_support *
-dynamic_cast_virtual_overrider_self_life_support_ptr(T * /*raw_type_ptr*/) {
+template <typename To,
+          typename From,
+          detail::enable_if_t<!(std::is_polymorphic<From>::value), int> = 0>
+To *dynamic_raw_ptr_cast_if_possible(From * /*ptr*/) {
     return nullptr;
 }
 
-template <typename T, detail::enable_if_t<std::is_polymorphic<T>::value, int> = 0>
-virtual_overrider_self_life_support *
-dynamic_cast_virtual_overrider_self_life_support_ptr(T *raw_type_ptr) {
-    return dynamic_cast<virtual_overrider_self_life_support *>(raw_type_ptr);
+template <typename To,
+          typename From,
+          detail::enable_if_t<std::is_polymorphic<From>::value, int> = 0>
+To *dynamic_raw_ptr_cast_if_possible(From *ptr) {
+    return dynamic_cast<To *>(ptr);
 }
 
 PYBIND11_NAMESPACE_END(detail)

include/pybind11/pybind11.h

@@ -1612,9 +1612,10 @@ class class_ : public detail::generic_type {
 
     // clang-format on
     template <typename T = type,
+              typename A = type_alias,
               detail::enable_if_t<detail::type_uses_smart_holder_type_caster<T>::value, int> = 0>
     static void init_instance(detail::instance *inst, const void *holder_ptr) {
-        detail::type_caster<T>::template init_instance_for_type<type>(inst, holder_ptr, has_alias);
+        detail::type_caster<T>::template init_instance_for_type<T, A>(inst, holder_ptr);
     }
     // clang-format off
 

tests/test_class_sh_trampoline_shared_ptr_cpp_arg.py

@@ -58,15 +58,10 @@ def test_shared_ptr_arg_identity():
     del obj
     pytest.gc_collect()
 
-    # python reference is still around since C++ has it
-    assert objref() is not None
-    assert tester.get_object() is objref()
-    assert tester.has_python_instance() is True
-
-    # python reference disappears once the C++ object releases it
-    tester.set_object(None)
-    pytest.gc_collect()
+    # SMART_HOLDER_WIP: the behavior below is DIFFERENT from PR #2839
+    # python reference is gone because it is not an Alias instance
     assert objref() is None
+    assert tester.has_python_instance() is False
 
 
 def test_shared_ptr_alias_nonpython():
@@ -90,7 +85,6 @@ def test_shared_ptr_alias_nonpython():
     assert tester.has_instance() is True
     assert tester.has_python_instance() is False
 
-    # SMART_HOLDER_WIP: the behavior below is DIFFERENT from PR #2839
     # When we pass it as an arg to a new tester the python instance should
     # disappear because it wasn't created with an alias
     new_tester = m.SpBaseTester()
@@ -107,9 +101,9 @@ def test_shared_ptr_alias_nonpython():
 
     # Gone!
     assert tester.has_instance() is True
-    assert tester.has_python_instance() is True  # False in PR #2839
+    assert tester.has_python_instance() is False
     assert new_tester.has_instance() is True
-    assert new_tester.has_python_instance() is True  # False in PR #2839
+    assert new_tester.has_python_instance() is False
 
 
 def test_shared_ptr_goaway():

tests/test_class_sh_with_alias.py

@@ -40,8 +40,14 @@ def test_drvd0_add_in_cpp_shared_ptr():
 def test_drvd0_add_in_cpp_unique_ptr():
     while True:
         drvd = PyDrvd0(0)
-        with pytest.raises(RuntimeError):
+        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::detail::virtual_overrider_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).