diff --git a/CMakeLists.txt b/CMakeLists.txt index 0a52493b2..b698b016d 100644 --- a/CMakeLists.txt +++ b/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) diff --git a/include/pybind11/detail/dynamic_raw_ptr_cast_if_possible.h b/include/pybind11/detail/dynamic_raw_ptr_cast_if_possible.h new file mode 100644 index 000000000..7c00fe98c --- /dev/null +++ b/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) diff --git a/include/pybind11/detail/smart_holder_type_casters.h b/include/pybind11/detail/smart_holder_type_casters.h index 5e6b0c016..14b3ea997 100644 --- a/include/pybind11/detail/smart_holder_type_casters.h +++ b/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. diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index 90d37972f..7ffadcc51 100644 --- a/include/pybind11/pybind11.h +++ b/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 diff --git a/include/pybind11/detail/virtual_overrider_self_life_support.h b/include/pybind11/virtual_overrider_self_life_support.h similarity index 73% rename from include/pybind11/detail/virtual_overrider_self_life_support.h rename to include/pybind11/virtual_overrider_self_life_support.h index 524bfcf54..b4c2437d0 100644 --- a/include/pybind11/detail/virtual_overrider_self_life_support.h +++ b/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) diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index efc548c41..7388589e5 100644 --- a/tests/CMakeLists.txt +++ b/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 diff --git a/tests/extra_python_package/test_files.py b/tests/extra_python_package/test_files.py index cfa8d9ca4..7d2cfd814 100644 --- a/tests/extra_python_package/test_files.py +++ b/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 = { diff --git a/tests/test_class_sh_trampoline_shared_ptr_cpp_arg.py b/tests/test_class_sh_trampoline_shared_ptr_cpp_arg.py index 9304aa0b7..09e72172b 100644 --- a/tests/test_class_sh_trampoline_shared_ptr_cpp_arg.py +++ b/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(): diff --git a/tests/test_class_sh_virtual_py_cpp_mix.cpp b/tests/test_class_sh_virtual_py_cpp_mix.cpp new file mode 100644 index 000000000..74d2db417 --- /dev/null +++ b/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")); +} diff --git a/tests/test_class_sh_virtual_py_cpp_mix.py b/tests/test_class_sh_virtual_py_cpp_mix.py new file mode 100644 index 000000000..e85be05fe --- /dev/null +++ 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 diff --git a/tests/test_class_sh_with_alias.cpp b/tests/test_class_sh_with_alias.cpp index 37372f116..846533ddf 100644 --- a/tests/test_class_sh_with_alias.cpp +++ b/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 { diff --git a/tests/test_class_sh_with_alias.py b/tests/test_class_sh_with_alias.py index efd6af7a4..4650eaa5e 100644 --- a/tests/test_class_sh_with_alias.py +++ b/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).