From 17a1ff9737ea0594f0d54f9ab69d7792a23e6090 Mon Sep 17 00:00:00 2001 From: Joshua Oreman Date: Fri, 10 Oct 2025 20:20:45 -0600 Subject: [PATCH 1/8] type_caster_generic: add set_foreign_holder method for subclasses to implement --- include/pybind11/cast.h | 88 +++++++++++++++++++++- include/pybind11/detail/type_caster_base.h | 11 +-- tests/local_bindings.h | 6 +- tests/pybind11_cross_module_tests.cpp | 4 +- tests/test_local_bindings.cpp | 16 ++++ tests/test_local_bindings.py | 28 +++++++ 6 files changed, 140 insertions(+), 13 deletions(-) diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h index 7b014fed99..c7c2bf3151 100644 --- a/include/pybind11/cast.h +++ b/include/pybind11/cast.h @@ -863,6 +863,71 @@ struct holder_helper { static auto get(const T &p) -> decltype(p.get()) { return p.get(); } }; +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 try_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 try_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 (try_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"); + } +}; + // SMART_HOLDER_BAKEIN_FOLLOW_ON: Rewrite comment, with reference to shared_ptr specialization. /// Type caster for holder types like std::shared_ptr, etc. /// The SFINAE hook is provided to help work around the current lack of support @@ -907,6 +972,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 +1046,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 +1054,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 +1110,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 +1152,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 +1299,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 +1363,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/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/local_bindings.h b/tests/local_bindings.h index dea1813108..273fa392ae 100644 --- a/tests/local_bindings.h +++ b/tests/local_bindings.h @@ -65,9 +65,9 @@ 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) { +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; }); } diff --git a/tests/pybind11_cross_module_tests.cpp b/tests/pybind11_cross_module_tests.cpp index 76f40bfa97..2a9092735d 100644 --- a/tests/pybind11_cross_module_tests.cpp +++ b/tests/pybind11_cross_module_tests.cpp @@ -25,8 +25,8 @@ PYBIND11_MODULE(pybind11_cross_module_tests, m, py::mod_gil_not_used()) { // relevant pybind11_tests submodule from a test_whatever.py // test_load_external - bind_local(m, "ExternalType1", py::module_local()); - bind_local(m, "ExternalType2", py::module_local()); + bind_local>(m, "ExternalType1", py::module_local()); + bind_local(m, "ExternalType2", 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..c65f6e03cf 100644 --- a/tests/test_local_bindings.cpp +++ b/tests/test_local_bindings.cpp @@ -22,6 +22,22 @@ TEST_SUBMODULE(local_bindings, m) { m.def("load_external1", [](ExternalType1 &e) { return e.i; }); m.def("load_external2", [](ExternalType2 &e) { return e.i; }); + struct SharedKeepAlive { + std::shared_ptr contents; + int value() const { return contents ? *contents : -1; } + 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_external1_shared", [](std::shared_ptr p) { + return SharedKeepAlive{std::shared_ptr(p, &p->i)}; + }); + m.def("load_external2_shared", [](std::shared_ptr p) { + return SharedKeepAlive{std::shared_ptr(p, &p->i)}; + }); + m.def("load_external2_unique", [](std::unique_ptr p) { return p->i; }); + // test_local_bindings // Register a class with py::module_local: bind_local(m, "LocalType", py::module_local()).def("get3", [](LocalType &t) { diff --git a/tests/test_local_bindings.py b/tests/test_local_bindings.py index 57552f7ec5..17c01c1707 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 @@ -20,6 +23,31 @@ 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(110, cm.ExternalType1, m.load_external1_shared) + test_shared(220, cm.ExternalType2, m.load_external2_shared) + + with pytest.raises(TypeError, match="incompatible function arguments"): + test_shared(210, cm.ExternalType1, m.load_external2_shared) + with pytest.raises(TypeError, match="incompatible function arguments"): + test_shared(120, cm.ExternalType2, m.load_external1_shared) + + with pytest.raises( + RuntimeError, match="Foreign instance cannot be converted to std::unique_ptr" + ): + m.load_external2_unique(cm.ExternalType2(2200)) + def test_local_bindings(): """Tests that duplicate `py::module_local` class bindings work across modules""" From f971060d425992a0ecb47b5a49a38cff52e81dbd Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Sat, 11 Oct 2025 17:28:55 +0000 Subject: [PATCH 2/8] style: pre-commit fixes --- tests/local_bindings.h | 6 +++--- tests/pybind11_cross_module_tests.cpp | 3 ++- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/tests/local_bindings.h b/tests/local_bindings.h index 273fa392ae..94d0455c41 100644 --- a/tests/local_bindings.h +++ b/tests/local_bindings.h @@ -67,9 +67,9 @@ PYBIND11_MAKE_OPAQUE(NonLocalMap2) // Simple bindings (used with the above): 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; - }); + 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 2a9092735d..5569e797c9 100644 --- a/tests/pybind11_cross_module_tests.cpp +++ b/tests/pybind11_cross_module_tests.cpp @@ -25,7 +25,8 @@ PYBIND11_MODULE(pybind11_cross_module_tests, m, py::mod_gil_not_used()) { // relevant pybind11_tests submodule from a test_whatever.py // test_load_external - bind_local>(m, "ExternalType1", py::module_local()); + bind_local>( + m, "ExternalType1", py::module_local()); bind_local(m, "ExternalType2", py::module_local()); // test_exceptions.py From fa6651180ee7d6531e627d72f782d5b13a442114 Mon Sep 17 00:00:00 2001 From: Joshua Oreman Date: Sat, 11 Oct 2025 13:37:07 -0400 Subject: [PATCH 3/8] Rename try_shared_from_this -> set_via_shared_from_this to avoid confusion against try_get_shared_from_this --- include/pybind11/cast.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h index c7c2bf3151..01ad286ade 100644 --- a/include/pybind11/cast.h +++ b/include/pybind11/cast.h @@ -878,7 +878,7 @@ struct holder_caster_foreign_helpers { }; template - static auto try_shared_from_this(type *value, std::shared_ptr *holder_out) + 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 @@ -890,7 +890,7 @@ struct holder_caster_foreign_helpers { } template - static bool try_shared_from_this(void *, std::shared_ptr *) { + static bool set_via_shared_from_this(void *, std::shared_ptr *) { return false; } @@ -903,7 +903,7 @@ struct holder_caster_foreign_helpers { *holder_out = {}; return true; } - if (try_shared_from_this(value, holder_out)) { + if (set_via_shared_from_this(value, holder_out)) { return true; } *holder_out = std::shared_ptr(value, py_deleter{src.inc_ref().ptr()}); From 8c760c4f30ec3d805c00e50924c9076698a8fec8 Mon Sep 17 00:00:00 2001 From: Joshua Oreman Date: Sat, 11 Oct 2025 13:57:28 -0400 Subject: [PATCH 4/8] Add comment explaining the limits of the test --- tests/test_local_bindings.cpp | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/tests/test_local_bindings.cpp b/tests/test_local_bindings.cpp index c65f6e03cf..9e55fc1836 100644 --- a/tests/test_local_bindings.cpp +++ b/tests/test_local_bindings.cpp @@ -38,6 +38,13 @@ TEST_SUBMODULE(local_bindings, m) { }); m.def("load_external2_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: bind_local(m, "LocalType", py::module_local()).def("get3", [](LocalType &t) { From 524d54dd2de139fafe9ec6d2cb9feb950201e252 Mon Sep 17 00:00:00 2001 From: Joshua Oreman Date: Sat, 11 Oct 2025 15:55:25 -0400 Subject: [PATCH 5/8] CI --- .codespell-ignore-lines | 1 + tests/test_local_bindings.cpp | 4 ++-- 2 files changed, 3 insertions(+), 2 deletions(-) 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/tests/test_local_bindings.cpp b/tests/test_local_bindings.cpp index 9e55fc1836..b497d2fee5 100644 --- a/tests/test_local_bindings.cpp +++ b/tests/test_local_bindings.cpp @@ -30,10 +30,10 @@ TEST_SUBMODULE(local_bindings, m) { py::class_(m, "SharedKeepAlive") .def_property_readonly("value", &SharedKeepAlive::value) .def_property_readonly("use_count", &SharedKeepAlive::use_count); - m.def("load_external1_shared", [](std::shared_ptr p) { + m.def("load_external1_shared", [](const std::shared_ptr& p) { return SharedKeepAlive{std::shared_ptr(p, &p->i)}; }); - m.def("load_external2_shared", [](std::shared_ptr p) { + m.def("load_external2_shared", [](const std::shared_ptr& p) { return SharedKeepAlive{std::shared_ptr(p, &p->i)}; }); m.def("load_external2_unique", [](std::unique_ptr p) { return p->i; }); From 68c8f3a216979ec84159c79eecbe95f03e65df10 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Sat, 11 Oct 2025 19:55:53 +0000 Subject: [PATCH 6/8] style: pre-commit fixes --- tests/test_local_bindings.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/test_local_bindings.cpp b/tests/test_local_bindings.cpp index b497d2fee5..6b7a5d190e 100644 --- a/tests/test_local_bindings.cpp +++ b/tests/test_local_bindings.cpp @@ -30,10 +30,10 @@ TEST_SUBMODULE(local_bindings, m) { py::class_(m, "SharedKeepAlive") .def_property_readonly("value", &SharedKeepAlive::value) .def_property_readonly("use_count", &SharedKeepAlive::use_count); - m.def("load_external1_shared", [](const std::shared_ptr& p) { + m.def("load_external1_shared", [](const std::shared_ptr &p) { return SharedKeepAlive{std::shared_ptr(p, &p->i)}; }); - m.def("load_external2_shared", [](const std::shared_ptr& p) { + m.def("load_external2_shared", [](const std::shared_ptr &p) { return SharedKeepAlive{std::shared_ptr(p, &p->i)}; }); m.def("load_external2_unique", [](std::unique_ptr p) { return p->i; }); From 1a907084a2168bd157f1c783110a4011665d5988 Mon Sep 17 00:00:00 2001 From: Joshua Oreman Date: Mon, 13 Oct 2025 12:20:27 -0600 Subject: [PATCH 7/8] Fixes from code review --- CMakeLists.txt | 1 + include/pybind11/cast.h | 66 +------------- .../detail/holder_caster_foreign_helpers.h | 86 +++++++++++++++++++ tests/extra_python_package/test_files.py | 1 + tests/local_bindings.h | 5 +- tests/pybind11_cross_module_tests.cpp | 7 +- tests/test_local_bindings.cpp | 10 ++- tests/test_local_bindings.py | 14 ++- 8 files changed, 112 insertions(+), 78 deletions(-) create mode 100644 include/pybind11/detail/holder_caster_foreign_helpers.h 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 01ad286ade..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" @@ -863,71 +864,6 @@ struct holder_helper { static auto get(const T &p) -> decltype(p.get()) { return p.get(); } }; -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"); - } -}; - // SMART_HOLDER_BAKEIN_FOLLOW_ON: Rewrite comment, with reference to shared_ptr specialization. /// Type caster for holder types like std::shared_ptr, etc. /// The SFINAE hook is provided to help work around the current lack of support 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/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 94d0455c41..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; diff --git a/tests/pybind11_cross_module_tests.cpp b/tests/pybind11_cross_module_tests.cpp index 5569e797c9..c37d2b3da1 100644 --- a/tests/pybind11_cross_module_tests.cpp +++ b/tests/pybind11_cross_module_tests.cpp @@ -25,9 +25,10 @@ PYBIND11_MODULE(pybind11_cross_module_tests, m, py::mod_gil_not_used()) { // relevant pybind11_tests submodule from a test_whatever.py // test_load_external - bind_local>( - m, "ExternalType1", py::module_local()); - bind_local(m, "ExternalType2", py::module_local()); + bind_local(m, "ExternalType1", 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 6b7a5d190e..5118b25da8 100644 --- a/tests/test_local_bindings.cpp +++ b/tests/test_local_bindings.cpp @@ -21,22 +21,24 @@ 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 : -1; } + 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_external1_shared", [](const std::shared_ptr &p) { + m.def("load_external2_shared", [](const std::shared_ptr &p) { return SharedKeepAlive{std::shared_ptr(p, &p->i)}; }); - m.def("load_external2_shared", [](const std::shared_ptr &p) { + m.def("load_external3_shared", [](const std::shared_ptr &p) { return SharedKeepAlive{std::shared_ptr(p, &p->i)}; }); - m.def("load_external2_unique", [](std::unique_ptr p) { return 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 diff --git a/tests/test_local_bindings.py b/tests/test_local_bindings.py index 17c01c1707..cac89d0da0 100644 --- a/tests/test_local_bindings.py +++ b/tests/test_local_bindings.py @@ -14,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 @@ -35,18 +36,23 @@ def test_shared(val, ctor, loader): rc_after = sys.getrefcount(obj) assert rc_after > rc_before - test_shared(110, cm.ExternalType1, m.load_external1_shared) 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(210, cm.ExternalType1, m.load_external2_shared) + test_shared(320, cm.ExternalType2, m.load_external3_shared) with pytest.raises(TypeError, match="incompatible function arguments"): - test_shared(120, cm.ExternalType2, m.load_external1_shared) + 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_external2_unique(cm.ExternalType2(2200)) + 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(): From 1ba6d643edfb1839f6d40aaccdb02ac3fb5404e3 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Mon, 13 Oct 2025 18:20:54 +0000 Subject: [PATCH 8/8] style: pre-commit fixes --- tests/pybind11_cross_module_tests.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/pybind11_cross_module_tests.cpp b/tests/pybind11_cross_module_tests.cpp index c37d2b3da1..30210194b7 100644 --- a/tests/pybind11_cross_module_tests.cpp +++ b/tests/pybind11_cross_module_tests.cpp @@ -27,7 +27,7 @@ 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()); + m, "ExternalType2", py::module_local()); bind_local(m, "ExternalType3", py::module_local()); // test_exceptions.py