Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .codespell-ignore-lines
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ template <op_id id, op_type ot, typename L = undefined_t, typename R = undefined
template <typename ThisT>
auto &this_ = static_cast<ThisT &>(*this);
if (load_impl<ThisT>(temp, false)) {
return load_impl<ThisT>(src, false);
ssize_t nd = 0;
auto trivial = broadcast(buffers, nd, shape);
auto ndim = (size_t) nd;
Expand Down
1 change: 1 addition & 0 deletions CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
24 changes: 21 additions & 3 deletions include/pybind11/cast.h
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -907,6 +908,10 @@ struct copyable_holder_caster : public type_caster_base<type> {
}
}

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();
Expand Down Expand Up @@ -977,22 +982,22 @@ struct copyable_holder_caster<
}

explicit operator std::shared_ptr<type> *() {
if (typeinfo->holder_enum_v == detail::holder_enum_t::smart_holder) {
if (sh_load_helper.was_populated) {
pybind11_fail("Passing `std::shared_ptr<T> *` from Python to C++ is not supported "
"(inherently unsafe).");
}
return std::addressof(shared_ptr_storage);
}

explicit operator std::shared_ptr<type> &() {
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<type> 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,
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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<type>(sub_caster.shared_ptr_storage, (type *) value);
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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__));
Expand Down
86 changes: 86 additions & 0 deletions include/pybind11/detail/holder_caster_foreign_helpers.h
Original file line number Diff line number Diff line change
@@ -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 <opensource@hudson-trading.com>

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 <pybind11/gil.h>

#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 <typename type>
static auto set_via_shared_from_this(type *value, std::shared_ptr<type> *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<type>(existing);
return true;
}
return false;
}

template <typename type>
static bool set_via_shared_from_this(void *, std::shared_ptr<type> *) {
return false;
}

template <typename type>
static bool set_foreign_holder(handle src, type *value, std::shared_ptr<type> *holder_out) {
// We only support using std::shared_ptr<T> 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<type>(value, py_deleter{src.inc_ref().ptr()});
return true;
}

template <typename type>
static bool
set_foreign_holder(handle src, const type *value, std::shared_ptr<const type> *holder_out) {
std::shared_ptr<type> holder_mut;
if (set_foreign_holder(src, const_cast<type *>(value), &holder_mut)) {
*holder_out = holder_mut;
return true;
}
return false;
}

template <typename type>
static bool set_foreign_holder(handle, type *, ...) {
throw cast_error("Unable to cast foreign type to held instance -- "
"only std::shared_ptr<T> is supported in this case");
}
};

PYBIND11_NAMESPACE_END(detail)
PYBIND11_NAMESPACE_END(PYBIND11_NAMESPACE)
11 changes: 6 additions & 5 deletions include/pybind11/detail/type_caster_base.h
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -1096,14 +1097,14 @@ class type_caster_generic {
// logic (without having to resort to virtual inheritance).
template <typename ThisT>
PYBIND11_NOINLINE bool load_impl(handle src, bool convert) {
auto &this_ = static_cast<ThisT &>(*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<ThisT &>(*this);
this_.check_holder_compat();

PyTypeObject *srctype = Py_TYPE(src.ptr());
Expand Down Expand Up @@ -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<ThisT>(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.
Expand All @@ -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;
Expand Down
1 change: 1 addition & 0 deletions tests/extra_python_package/test_files.py
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
15 changes: 8 additions & 7 deletions tests/local_bindings.h
Original file line number Diff line number Diff line change
Expand Up @@ -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<LocalType>;
using LocalVec2 = std::vector<NonLocal2>;
Expand Down Expand Up @@ -65,11 +66,11 @@ PYBIND11_MAKE_OPAQUE(NonLocalMap)
PYBIND11_MAKE_OPAQUE(NonLocalMap2)

// Simple bindings (used with the above):
template <typename T, int Adjust = 0, typename... Args>
py::class_<T> bind_local(Args &&...args) {
return py::class_<T>(std::forward<Args>(args)...).def(py::init<int>()).def("get", [](T &i) {
return i.i + Adjust;
});
template <typename T, int Adjust = 0, typename Holder = std::unique_ptr<T>, typename... Args>
py::class_<T, Holder> bind_local(Args &&...args) {
return py::class_<T, Holder>(std::forward<Args>(args)...)
.def(py::init<int>())
.def("get", [](T &i) { return i.i + Adjust; });
}

// Simulate a foreign library base class (to match the example in the docs):
Expand Down
4 changes: 3 additions & 1 deletion tests/pybind11_cross_module_tests.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,9 @@ PYBIND11_MODULE(pybind11_cross_module_tests, m, py::mod_gil_not_used()) {

// test_load_external
bind_local<ExternalType1>(m, "ExternalType1", py::module_local());
bind_local<ExternalType2>(m, "ExternalType2", py::module_local());
bind_local<ExternalType2, 0, std::shared_ptr<ExternalType2>>(
m, "ExternalType2", py::module_local());
bind_local<ExternalType3, 0, py::smart_holder>(m, "ExternalType3", py::module_local());

// test_exceptions.py
py::register_local_exception<LocalSimpleException>(m, "LocalSimpleException");
Expand Down
25 changes: 25 additions & 0 deletions tests/test_local_bindings.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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<int> contents;
int value() const { return contents ? *contents : -20251012; }
long use_count() const { return contents.use_count(); }
};
py::class_<SharedKeepAlive>(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<ExternalType2> &p) {
return SharedKeepAlive{std::shared_ptr<int>(p, &p->i)};
});
m.def("load_external3_shared", [](const std::shared_ptr<ExternalType3> &p) {
return SharedKeepAlive{std::shared_ptr<int>(p, &p->i)};
});
m.def("load_external1_unique", [](std::unique_ptr<ExternalType1> p) { return p->i; });
m.def("load_external3_unique", [](std::unique_ptr<ExternalType3> 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:
Expand Down
34 changes: 34 additions & 0 deletions tests/test_local_bindings.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
from __future__ import annotations

import sys
from contextlib import suppress

import pytest

from pybind11_tests import local_bindings as m
Expand All @@ -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
Expand All @@ -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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@msimacek I just looked:

$ cd tests/
$ git grep 'Cannot reliably trigger GC' | wc -l
48

Do you think we could enable more test coverage for GraalPy by adopting Joshua's approach here more widely?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem of gc.collect() not reliably GCing things is broader than the problem of not having refcounts. PyPy doesn't have refcounts but its gc.collect() works (if you call it a few times).

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"""
Expand Down
Loading