diff --git a/.codespell-ignore-lines b/.codespell-ignore-lines index e8cbf31447..c03cd2f727 100644 --- a/.codespell-ignore-lines +++ b/.codespell-ignore-lines @@ -2,6 +2,7 @@ template auto &this_ = static_cast(*this); if (load_impl(temp, false)) { + return load_impl(src, false); ssize_t nd = 0; auto trivial = broadcast(buffers, nd, shape); auto ndim = (size_t) nd; diff --git a/CMakeLists.txt b/CMakeLists.txt index a6d619bbdd..8063303938 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -188,6 +188,7 @@ set(PYBIND11_HEADERS include/pybind11/detail/dynamic_raw_ptr_cast_if_possible.h include/pybind11/detail/exception_translation.h include/pybind11/detail/function_record_pyobject.h + include/pybind11/detail/holder_caster_foreign_helpers.h include/pybind11/detail/init.h include/pybind11/detail/internals.h include/pybind11/detail/native_enum_data.h diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h index 7b014fed99..17040c7c5a 100644 --- a/include/pybind11/cast.h +++ b/include/pybind11/cast.h @@ -13,6 +13,7 @@ #include "detail/argument_vector.h" #include "detail/common.h" #include "detail/descr.h" +#include "detail/holder_caster_foreign_helpers.h" #include "detail/native_enum_data.h" #include "detail/type_caster_base.h" #include "detail/typeid.h" @@ -907,6 +908,10 @@ struct copyable_holder_caster : public type_caster_base { } } + bool set_foreign_holder(handle src) { + return holder_caster_foreign_helpers::set_foreign_holder(src, (type *) value, &holder); + } + void load_value(value_and_holder &&v_h) { if (v_h.holder_constructed()) { value = v_h.value_ptr(); @@ -977,7 +982,7 @@ struct copyable_holder_caster< } explicit operator std::shared_ptr *() { - if (typeinfo->holder_enum_v == detail::holder_enum_t::smart_holder) { + if (sh_load_helper.was_populated) { pybind11_fail("Passing `std::shared_ptr *` from Python to C++ is not supported " "(inherently unsafe)."); } @@ -985,14 +990,14 @@ struct copyable_holder_caster< } explicit operator std::shared_ptr &() { - if (typeinfo->holder_enum_v == detail::holder_enum_t::smart_holder) { + if (sh_load_helper.was_populated) { shared_ptr_storage = sh_load_helper.load_as_shared_ptr(typeinfo, value); } return shared_ptr_storage; } std::weak_ptr potentially_slicing_weak_ptr() { - if (typeinfo->holder_enum_v == detail::holder_enum_t::smart_holder) { + if (sh_load_helper.was_populated) { // Reusing shared_ptr code to minimize code complexity. shared_ptr_storage = sh_load_helper.load_as_shared_ptr(typeinfo, @@ -1041,6 +1046,11 @@ struct copyable_holder_caster< } } + bool set_foreign_holder(handle src) { + return holder_caster_foreign_helpers::set_foreign_holder( + src, (type *) value, &shared_ptr_storage); + } + void load_value(value_and_holder &&v_h) { if (typeinfo->holder_enum_v == detail::holder_enum_t::smart_holder) { sh_load_helper.loaded_v_h = v_h; @@ -1078,6 +1088,7 @@ struct copyable_holder_caster< value = cast.second(sub_caster.value); if (typeinfo->holder_enum_v == detail::holder_enum_t::smart_holder) { sh_load_helper.loaded_v_h = sub_caster.sh_load_helper.loaded_v_h; + sh_load_helper.was_populated = true; } else { shared_ptr_storage = std::shared_ptr(sub_caster.shared_ptr_storage, (type *) value); @@ -1224,6 +1235,12 @@ struct move_only_holder_caster< return false; } + bool set_foreign_holder(handle) { + throw cast_error("Foreign instance cannot be converted to std::unique_ptr " + "because we don't know how to make it relinquish " + "ownership"); + } + void load_value(value_and_holder &&v_h) { if (typeinfo->holder_enum_v == detail::holder_enum_t::smart_holder) { sh_load_helper.loaded_v_h = v_h; @@ -1282,6 +1299,7 @@ struct move_only_holder_caster< value = cast.second(sub_caster.value); if (typeinfo->holder_enum_v == detail::holder_enum_t::smart_holder) { sh_load_helper.loaded_v_h = sub_caster.sh_load_helper.loaded_v_h; + sh_load_helper.was_populated = true; } else { pybind11_fail("Expected to be UNREACHABLE: " __FILE__ ":" PYBIND11_TOSTRING(__LINE__)); diff --git a/include/pybind11/detail/holder_caster_foreign_helpers.h b/include/pybind11/detail/holder_caster_foreign_helpers.h new file mode 100644 index 0000000000..f636618e9f --- /dev/null +++ b/include/pybind11/detail/holder_caster_foreign_helpers.h @@ -0,0 +1,86 @@ +/* + pybind11/detail/holder_caster_foreign_helpers.h: Logic to implement + set_foreign_holder() in copyable_ and movable_holder_caster. + + Copyright (c) 2025 Hudson River Trading LLC + + 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 + +#include "common.h" + +PYBIND11_NAMESPACE_BEGIN(PYBIND11_NAMESPACE) +PYBIND11_NAMESPACE_BEGIN(detail) + +struct holder_caster_foreign_helpers { + struct py_deleter { + void operator()(const void *) const noexcept { + // Don't run the deleter if the interpreter has been shut down + if (Py_IsInitialized() == 0) { + return; + } + gil_scoped_acquire guard; + Py_DECREF(o); + } + + PyObject *o; + }; + + template + static auto set_via_shared_from_this(type *value, std::shared_ptr *holder_out) + -> decltype(value->shared_from_this(), bool()) { + // object derives from enable_shared_from_this; + // try to reuse an existing shared_ptr if one is known + if (auto existing = try_get_shared_from_this(value)) { + *holder_out = std::static_pointer_cast(existing); + return true; + } + return false; + } + + template + static bool set_via_shared_from_this(void *, std::shared_ptr *) { + return false; + } + + template + static bool set_foreign_holder(handle src, type *value, std::shared_ptr *holder_out) { + // We only support using std::shared_ptr for foreign T, and + // it's done by creating a new shared_ptr control block that + // owns a reference to the original Python object. + if (value == nullptr) { + *holder_out = {}; + return true; + } + if (set_via_shared_from_this(value, holder_out)) { + return true; + } + *holder_out = std::shared_ptr(value, py_deleter{src.inc_ref().ptr()}); + return true; + } + + template + static bool + set_foreign_holder(handle src, const type *value, std::shared_ptr *holder_out) { + std::shared_ptr holder_mut; + if (set_foreign_holder(src, const_cast(value), &holder_mut)) { + *holder_out = holder_mut; + return true; + } + return false; + } + + template + static bool set_foreign_holder(handle, type *, ...) { + throw cast_error("Unable to cast foreign type to held instance -- " + "only std::shared_ptr is supported in this case"); + } +}; + +PYBIND11_NAMESPACE_END(detail) +PYBIND11_NAMESPACE_END(PYBIND11_NAMESPACE) diff --git a/include/pybind11/detail/type_caster_base.h b/include/pybind11/detail/type_caster_base.h index a2512a5527..2cba6694ab 100644 --- a/include/pybind11/detail/type_caster_base.h +++ b/include/pybind11/detail/type_caster_base.h @@ -1058,6 +1058,7 @@ class type_caster_generic { return false; } void check_holder_compat() {} + bool set_foreign_holder(handle) { return true; } PYBIND11_NOINLINE static void *local_load(PyObject *src, const type_info *ti) { auto caster = type_caster_generic(ti); @@ -1096,14 +1097,14 @@ class type_caster_generic { // logic (without having to resort to virtual inheritance). template PYBIND11_NOINLINE bool load_impl(handle src, bool convert) { + auto &this_ = static_cast(*this); if (!src) { return false; } if (!typeinfo) { - return try_load_foreign_module_local(src); + return try_load_foreign_module_local(src) && this_.set_foreign_holder(src); } - auto &this_ = static_cast(*this); this_.check_holder_compat(); PyTypeObject *srctype = Py_TYPE(src.ptr()); @@ -1169,13 +1170,13 @@ class type_caster_generic { if (typeinfo->module_local) { if (auto *gtype = get_global_type_info(*typeinfo->cpptype)) { typeinfo = gtype; - return load(src, false); + return load_impl(src, false); } } // Global typeinfo has precedence over foreign module_local if (try_load_foreign_module_local(src)) { - return true; + return this_.set_foreign_holder(src); } // Custom converters didn't take None, now we convert None to nullptr. @@ -1189,7 +1190,7 @@ class type_caster_generic { } if (convert && cpptype && this_.try_cpp_conduit(src)) { - return true; + return this_.set_foreign_holder(src); } return false; diff --git a/tests/extra_python_package/test_files.py b/tests/extra_python_package/test_files.py index d3452aa383..1539b171a2 100644 --- a/tests/extra_python_package/test_files.py +++ b/tests/extra_python_package/test_files.py @@ -83,6 +83,7 @@ "include/pybind11/detail/descr.h", "include/pybind11/detail/dynamic_raw_ptr_cast_if_possible.h", "include/pybind11/detail/function_record_pyobject.h", + "include/pybind11/detail/holder_caster_foreign_helpers.h", "include/pybind11/detail/init.h", "include/pybind11/detail/internals.h", "include/pybind11/detail/native_enum_data.h", diff --git a/tests/local_bindings.h b/tests/local_bindings.h index dea1813108..23d46eb264 100644 --- a/tests/local_bindings.h +++ b/tests/local_bindings.h @@ -25,8 +25,9 @@ using MixedLocalGlobal = LocalBase<4>; using MixedGlobalLocal = LocalBase<5>; /// Registered with py::module_local only in the secondary module: -using ExternalType1 = LocalBase<6>; -using ExternalType2 = LocalBase<7>; +using ExternalType1 = LocalBase<6>; // default holder +using ExternalType2 = LocalBase<7>; // held by std::shared_ptr +using ExternalType3 = LocalBase<8>; // held by smart_holder using LocalVec = std::vector; using LocalVec2 = std::vector; @@ -65,11 +66,11 @@ PYBIND11_MAKE_OPAQUE(NonLocalMap) PYBIND11_MAKE_OPAQUE(NonLocalMap2) // Simple bindings (used with the above): -template -py::class_ bind_local(Args &&...args) { - return py::class_(std::forward(args)...).def(py::init()).def("get", [](T &i) { - return i.i + Adjust; - }); +template , typename... Args> +py::class_ bind_local(Args &&...args) { + return py::class_(std::forward(args)...) + .def(py::init()) + .def("get", [](T &i) { return i.i + Adjust; }); } // Simulate a foreign library base class (to match the example in the docs): diff --git a/tests/pybind11_cross_module_tests.cpp b/tests/pybind11_cross_module_tests.cpp index 76f40bfa97..30210194b7 100644 --- a/tests/pybind11_cross_module_tests.cpp +++ b/tests/pybind11_cross_module_tests.cpp @@ -26,7 +26,9 @@ PYBIND11_MODULE(pybind11_cross_module_tests, m, py::mod_gil_not_used()) { // test_load_external bind_local(m, "ExternalType1", py::module_local()); - bind_local(m, "ExternalType2", py::module_local()); + bind_local>( + m, "ExternalType2", py::module_local()); + bind_local(m, "ExternalType3", py::module_local()); // test_exceptions.py py::register_local_exception(m, "LocalSimpleException"); diff --git a/tests/test_local_bindings.cpp b/tests/test_local_bindings.cpp index 1373677447..5118b25da8 100644 --- a/tests/test_local_bindings.cpp +++ b/tests/test_local_bindings.cpp @@ -21,6 +21,31 @@ TEST_SUBMODULE(local_bindings, m) { // test_load_external m.def("load_external1", [](ExternalType1 &e) { return e.i; }); m.def("load_external2", [](ExternalType2 &e) { return e.i; }); + m.def("load_external3", [](ExternalType3 &e) { return e.i; }); + + struct SharedKeepAlive { + std::shared_ptr contents; + int value() const { return contents ? *contents : -20251012; } + long use_count() const { return contents.use_count(); } + }; + py::class_(m, "SharedKeepAlive") + .def_property_readonly("value", &SharedKeepAlive::value) + .def_property_readonly("use_count", &SharedKeepAlive::use_count); + m.def("load_external2_shared", [](const std::shared_ptr &p) { + return SharedKeepAlive{std::shared_ptr(p, &p->i)}; + }); + m.def("load_external3_shared", [](const std::shared_ptr &p) { + return SharedKeepAlive{std::shared_ptr(p, &p->i)}; + }); + m.def("load_external1_unique", [](std::unique_ptr p) { return p->i; }); + m.def("load_external3_unique", [](std::unique_ptr p) { return p->i; }); + + // Aspects of set_foreign_holder that are not covered: + // - loading a foreign instance into a custom holder should fail + // - we're only covering the case where the local module doesn't know + // about the type; the paths where it does (e.g., if both global and + // foreign-module-local bindings exist for the same type) should work + // the same way (they use the same code so they very likely do) // test_local_bindings // Register a class with py::module_local: diff --git a/tests/test_local_bindings.py b/tests/test_local_bindings.py index 57552f7ec5..cac89d0da0 100644 --- a/tests/test_local_bindings.py +++ b/tests/test_local_bindings.py @@ -1,5 +1,8 @@ from __future__ import annotations +import sys +from contextlib import suppress + import pytest from pybind11_tests import local_bindings as m @@ -11,6 +14,7 @@ def test_load_external(): assert m.load_external1(cm.ExternalType1(11)) == 11 assert m.load_external2(cm.ExternalType2(22)) == 22 + assert m.load_external3(cm.ExternalType3(33)) == 33 with pytest.raises(TypeError) as excinfo: assert m.load_external2(cm.ExternalType1(21)) == 21 @@ -20,6 +24,36 @@ def test_load_external(): assert m.load_external1(cm.ExternalType2(12)) == 12 assert "incompatible function arguments" in str(excinfo.value) + def test_shared(val, ctor, loader): + obj = ctor(val) + with suppress(AttributeError): # non-cpython VMs don't have getrefcount + rc_before = sys.getrefcount(obj) + wrapper = loader(obj) + # wrapper holds a shared_ptr that keeps obj alive + assert wrapper.use_count == 1 + assert wrapper.value == val + with suppress(AttributeError): + rc_after = sys.getrefcount(obj) + assert rc_after > rc_before + + test_shared(220, cm.ExternalType2, m.load_external2_shared) + test_shared(330, cm.ExternalType3, m.load_external3_shared) + + with pytest.raises(TypeError, match="incompatible function arguments"): + test_shared(320, cm.ExternalType2, m.load_external3_shared) + with pytest.raises(TypeError, match="incompatible function arguments"): + test_shared(230, cm.ExternalType3, m.load_external2_shared) + + with pytest.raises( + RuntimeError, match="Foreign instance cannot be converted to std::unique_ptr" + ): + m.load_external1_unique(cm.ExternalType1(2200)) + + with pytest.raises( + RuntimeError, match="Foreign instance cannot be converted to std::unique_ptr" + ): + m.load_external3_unique(cm.ExternalType3(3300)) + def test_local_bindings(): """Tests that duplicate `py::module_local` class bindings work across modules"""