Using dynamic_cast<AliasType> to determine pointee_depends_on_holder_owner. (#2910)

* Adaption of PyCLIF virtual_py_cpp_mix test.

* Removing ValueError: Ownership of instance with virtual overrides in Python cannot be transferred to C++. TODO: static_assert alias class needs to inherit from virtual_overrider_self_life_support.

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

* Fixing oversight: adding test_class_sh_virtual_py_cpp_mix.cpp to cmake file.

* clang <= 3.6 compatibility.

* Fixing oversight: dynamic_raw_ptr_cast_if_possible needs special handling for To = void. Adding corresponding missing test in test_class_sh_virtual_py_cpp_mix. Moving dynamic_raw_ptr_cast_if_possible to separate header.

* Changing py::detail::virtual_overrider_self_life_support to py::virtual_overrider_self_life_support.

CMakeLists.txt

@@ -103,6 +103,7 @@ set(PYBIND11_HEADERS
     include/pybind11/detail/class.h
     include/pybind11/detail/common.h
     include/pybind11/detail/descr.h
+    include/pybind11/detail/dynamic_raw_ptr_cast_if_possible.h
     include/pybind11/detail/init.h
     include/pybind11/detail/internals.h
     include/pybind11/detail/smart_holder_poc.h
@@ -110,7 +111,6 @@ set(PYBIND11_HEADERS
     include/pybind11/detail/smart_holder_type_casters.h
     include/pybind11/detail/type_caster_base.h
     include/pybind11/detail/typeid.h
-    include/pybind11/detail/virtual_overrider_self_life_support.h
     include/pybind11/attr.h
     include/pybind11/buffer_info.h
     include/pybind11/cast.h
@@ -130,7 +130,8 @@ set(PYBIND11_HEADERS
     include/pybind11/pytypes.h
     include/pybind11/smart_holder.h
     include/pybind11/stl.h
-    include/pybind11/stl_bind.h)
+    include/pybind11/stl_bind.h
+    include/pybind11/virtual_overrider_self_life_support.h)
 
 # Compare with grep and warn if mismatched
 if(PYBIND11_MASTER_PROJECT AND NOT CMAKE_VERSION VERSION_LESS 3.12)

include/pybind11/detail/dynamic_raw_ptr_cast_if_possible.h

@@ -0,0 +1,39 @@
+// Copyright (c) 2021 The Pybind Development Team.
+// All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+
+#pragma once
+
+#include "common.h"
+
+#include <type_traits>
+
+PYBIND11_NAMESPACE_BEGIN(PYBIND11_NAMESPACE)
+PYBIND11_NAMESPACE_BEGIN(detail)
+
+template <typename To, typename From, typename SFINAE = void>
+struct dynamic_raw_ptr_cast_is_possible : std::false_type {};
+
+template <typename To, typename From>
+struct dynamic_raw_ptr_cast_is_possible<
+    To,
+    From,
+    detail::enable_if_t<!std::is_same<To, void>::value && std::is_polymorphic<From>::value>>
+    : std::true_type {};
+
+template <typename To,
+          typename From,
+          detail::enable_if_t<!dynamic_raw_ptr_cast_is_possible<To, From>::value, int> = 0>
+To *dynamic_raw_ptr_cast_if_possible(From * /*ptr*/) {
+    return nullptr;
+}
+
+template <typename To,
+          typename From,
+          detail::enable_if_t<dynamic_raw_ptr_cast_is_possible<To, From>::value, int> = 0>
+To *dynamic_raw_ptr_cast_if_possible(From *ptr) {
+    return dynamic_cast<To *>(ptr);
+}
+
+PYBIND11_NAMESPACE_END(detail)
+PYBIND11_NAMESPACE_END(PYBIND11_NAMESPACE)

include/pybind11/detail/smart_holder_type_casters.h

@@ -6,14 +6,15 @@
 
 #include "../gil.h"
 #include "../pytypes.h"
+#include "../virtual_overrider_self_life_support.h"
 #include "common.h"
 #include "descr.h"
+#include "dynamic_raw_ptr_cast_if_possible.h"
 #include "internals.h"
 #include "smart_holder_poc.h"
 #include "smart_holder_sfinae_hooks_only.h"
 #include "type_caster_base.h"
 #include "typeid.h"
-#include "virtual_overrider_self_life_support.h"
 
 #include <cstddef>
 #include <memory>
@@ -263,15 +264,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 +281,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,10 +391,11 @@ 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("Ownership of instance with virtual overrides in Python cannot be "
-                              "transferred to C++.");
+            throw value_error("Alias class (also known as trampoline) does not inherit from "
+                              "py::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.

include/pybind11/pybind11.h

@@ -1612,9 +1612,10 @@ private:
 
     // 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
 

include/pybind11/virtual_overrider_self_life_support.h

@@ -4,24 +4,23 @@
 
 #pragma once
 
-#include "common.h"
-#include "smart_holder_poc.h"
-#include "type_caster_base.h"
-
-#include <type_traits>
+#include "detail/common.h"
+#include "detail/smart_holder_poc.h"
+#include "detail/type_caster_base.h"
 
 PYBIND11_NAMESPACE_BEGIN(PYBIND11_NAMESPACE)
-PYBIND11_NAMESPACE_BEGIN(detail)
 
+PYBIND11_NAMESPACE_BEGIN(detail)
 // SMART_HOLDER_WIP: Needs refactoring of existing pybind11 code.
 inline bool deregister_instance(instance *self, void *valptr, const type_info *tinfo);
+PYBIND11_NAMESPACE_END(detail)
 
 // The original core idea for this struct goes back to PyCLIF:
 // https://github.com/google/clif/blob/07f95d7e69dca2fcf7022978a55ef3acff506c19/clif/python/runtime.cc#L37
 // URL provided here mainly to give proper credit. To fully explain the `HoldPyObj` feature, more
 // context is needed (SMART_HOLDER_WIP).
 struct virtual_overrider_self_life_support {
-    value_and_holder loaded_v_h;
+    detail::value_and_holder loaded_v_h;
     ~virtual_overrider_self_life_support() {
         if (loaded_v_h.inst != nullptr && loaded_v_h.vh != nullptr) {
             void *value_void_ptr = loaded_v_h.value_ptr();
@@ -30,7 +29,7 @@ struct virtual_overrider_self_life_support {
                 Py_DECREF((PyObject *) loaded_v_h.inst);
                 loaded_v_h.value_ptr() = nullptr;
                 loaded_v_h.holder<pybindit::memory::smart_holder>().release_disowned();
-                deregister_instance(loaded_v_h.inst, value_void_ptr, loaded_v_h.type);
+                detail::deregister_instance(loaded_v_h.inst, value_void_ptr, loaded_v_h.type);
                 PyGILState_Release(threadstate);
             }
         }
@@ -46,17 +45,4 @@ 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*/) {
-    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);
-}
-
-PYBIND11_NAMESPACE_END(detail)
 PYBIND11_NAMESPACE_END(PYBIND11_NAMESPACE)

tests/CMakeLists.txt

@@ -106,6 +106,7 @@ set(PYBIND11_TEST_FILES
     test_class_sh_inheritance.cpp
     test_class_sh_trampoline_shared_ptr_cpp_arg.cpp
     test_class_sh_unique_ptr_member.cpp
+    test_class_sh_virtual_py_cpp_mix.cpp
     test_class_sh_with_alias.cpp
     test_constants_and_functions.cpp
     test_copy_move.cpp

tests/extra_python_package/test_files.py

@@ -35,12 +35,14 @@ main_headers = {
     "include/pybind11/smart_holder.h",
     "include/pybind11/stl.h",
     "include/pybind11/stl_bind.h",
+    "include/pybind11/virtual_overrider_self_life_support.h",
 }
 
 detail_headers = {
     "include/pybind11/detail/class.h",
     "include/pybind11/detail/common.h",
     "include/pybind11/detail/descr.h",
+    "include/pybind11/detail/dynamic_raw_ptr_cast_if_possible.h",
     "include/pybind11/detail/init.h",
     "include/pybind11/detail/internals.h",
     "include/pybind11/detail/smart_holder_poc.h",
@@ -48,7 +50,6 @@ detail_headers = {
     "include/pybind11/detail/smart_holder_type_casters.h",
     "include/pybind11/detail/type_caster_base.h",
     "include/pybind11/detail/typeid.h",
-    "include/pybind11/detail/virtual_overrider_self_life_support.h",
 }
 
 cmake_files = {

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_virtual_py_cpp_mix.cpp

@@ -0,0 +1,65 @@
+#include "pybind11_tests.h"
+
+#include <pybind11/smart_holder.h>
+
+#include <memory>
+
+namespace pybind11_tests {
+namespace test_class_sh_virtual_py_cpp_mix {
+
+class Base {
+public:
+    virtual ~Base() = default;
+    virtual int get() const { return 101; }
+
+    // Some compilers complain about implicitly defined versions of some of the following:
+    Base()             = default;
+    Base(const Base &) = default;
+};
+
+class CppDerivedPlain : public Base {
+public:
+    int get() const override { return 202; }
+};
+
+class CppDerived : public Base {
+public:
+    int get() const override { return 212; }
+};
+
+int get_from_cpp_plainc_ptr(const Base *b) { return b->get() + 4000; }
+
+int get_from_cpp_unique_ptr(std::unique_ptr<Base> b) { return b->get() + 5000; }
+
+struct BaseVirtualOverrider : Base, py::virtual_overrider_self_life_support {
+    using Base::Base;
+
+    int get() const override { PYBIND11_OVERRIDE(int, Base, get); }
+};
+
+struct CppDerivedVirtualOverrider : CppDerived, py::virtual_overrider_self_life_support {
+    using CppDerived::CppDerived;
+
+    int get() const override { PYBIND11_OVERRIDE(int, CppDerived, get); }
+};
+
+} // namespace test_class_sh_virtual_py_cpp_mix
+} // namespace pybind11_tests
+
+PYBIND11_SMART_HOLDER_TYPE_CASTERS(pybind11_tests::test_class_sh_virtual_py_cpp_mix::Base)
+PYBIND11_SMART_HOLDER_TYPE_CASTERS(
+    pybind11_tests::test_class_sh_virtual_py_cpp_mix::CppDerivedPlain)
+PYBIND11_SMART_HOLDER_TYPE_CASTERS(pybind11_tests::test_class_sh_virtual_py_cpp_mix::CppDerived)
+
+TEST_SUBMODULE(class_sh_virtual_py_cpp_mix, m) {
+    using namespace pybind11_tests::test_class_sh_virtual_py_cpp_mix;
+
+    py::classh<Base, BaseVirtualOverrider>(m, "Base").def(py::init<>()).def("get", &Base::get);
+
+    py::classh<CppDerivedPlain, Base>(m, "CppDerivedPlain").def(py::init<>());
+
+    py::classh<CppDerived, Base, CppDerivedVirtualOverrider>(m, "CppDerived").def(py::init<>());
+
+    m.def("get_from_cpp_plainc_ptr", get_from_cpp_plainc_ptr, py::arg("b"));
+    m.def("get_from_cpp_unique_ptr", get_from_cpp_unique_ptr, py::arg("b"));
+}

tests/test_class_sh_virtual_py_cpp_mix.py

@@ -0,0 +1,65 @@
+# -*- coding: utf-8 -*-
+import pytest
+
+from pybind11_tests import class_sh_virtual_py_cpp_mix as m
+
+
+class PyBase(m.Base):  # Avoiding name PyDerived, for more systematic naming.
+    def __init__(self):
+        m.Base.__init__(self)
+
+    def get(self):
+        return 323
+
+
+class PyCppDerived(m.CppDerived):
+    def __init__(self):
+        m.CppDerived.__init__(self)
+
+    def get(self):
+        return 434
+
+
+@pytest.mark.parametrize(
+    "ctor, expected",
+    [
+        (m.Base, 101),
+        (PyBase, 323),
+        (m.CppDerivedPlain, 202),
+        (m.CppDerived, 212),
+        (PyCppDerived, 434),
+    ],
+)
+def test_base_get(ctor, expected):
+    obj = ctor()
+    assert obj.get() == expected
+
+
+@pytest.mark.parametrize(
+    "ctor, expected",
+    [
+        (m.Base, 4101),
+        (PyBase, 4323),
+        (m.CppDerivedPlain, 4202),
+        (m.CppDerived, 4212),
+        (PyCppDerived, 4434),
+    ],
+)
+def test_get_from_cpp_plainc_ptr(ctor, expected):
+    obj = ctor()
+    assert m.get_from_cpp_plainc_ptr(obj) == expected
+
+
+@pytest.mark.parametrize(
+    "ctor, expected",
+    [
+        (m.Base, 5101),
+        (PyBase, 5323),
+        (m.CppDerivedPlain, 5202),
+        (m.CppDerived, 5212),
+        (PyCppDerived, 5434),
+    ],
+)
+def test_get_from_cpp_unique_ptr(ctor, expected):
+    obj = ctor()
+    assert m.get_from_cpp_unique_ptr(obj) == expected

tests/test_class_sh_with_alias.cpp

@@ -35,7 +35,7 @@ struct AbaseAlias : Abase<SerNo> {
 };
 
 template <>
-struct AbaseAlias<1> : Abase<1>, py::detail::virtual_overrider_self_life_support {
+struct AbaseAlias<1> : Abase<1>, py::virtual_overrider_self_life_support {
     using Abase<1>::Abase;
 
     int Add(int other_val) const override {

tests/test_class_sh_with_alias.py

@@ -44,8 +44,9 @@ def test_drvd0_add_in_cpp_unique_ptr():
             m.AddInCppUniquePtr(drvd, 0)
         assert (
             str(exc_info.value)
-            == "Ownership of instance with virtual overrides in Python"
-            " cannot be transferred to C++."
+            == "Alias class (also known as trampoline) does not inherit from"
+            " py::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).