-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Add py::potentially_slicing_weak_ptr(handle)
function
#5624
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 27 commits
50c3c1e
528d1cc
b160f6c
2fe82b2
ff70657
a481647
32858da
e978d93
1c0b700
4638e01
6cc6368
62d3265
2f672eb
888a295
b20f557
3b4b28c
56d23dc
ff0792e
eb1787c
10f6e78
121c8cf
b42bd11
f160450
78178d7
62d2242
0ecb396
faf1e61
222b4a7
7c4a8bc
03a8981
bad0c12
683fff4
9f379ea
b349e84
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -983,6 +983,16 @@ struct copyable_holder_caster< | |
return shared_ptr_storage; | ||
} | ||
|
||
std::shared_ptr<type> &potentially_slicing_shared_ptr() { | ||
if (typeinfo->holder_enum_v == detail::holder_enum_t::smart_holder) { | ||
shared_ptr_storage | ||
= sh_load_helper.load_as_shared_ptr(value, | ||
/*responsible_parent=*/nullptr, | ||
/*force_potentially_slicing_shared_ptr=*/true); | ||
} | ||
return shared_ptr_storage; | ||
} | ||
|
||
static handle | ||
cast(const std::shared_ptr<type> &src, return_value_policy policy, handle parent) { | ||
const auto *ptr = src.get(); | ||
|
@@ -1077,6 +1087,54 @@ struct copyable_holder_caster< | |
template <typename T> | ||
class type_caster<std::shared_ptr<T>> : public copyable_holder_caster<T, std::shared_ptr<T>> {}; | ||
|
||
PYBIND11_NAMESPACE_END(detail) | ||
|
||
/// Return a std::shared_ptr with the SAME CONTROL BLOCK as the std::shared_ptr owned by the | ||
/// class_ holder. For class_-wrapped types with trampolines, the returned std::shared_ptr | ||
/// does NOT keep any derived Python objects alive (see issue #1333). | ||
/// | ||
/// For class_-wrapped types using std::shared_ptr as the holder, the following expressions | ||
/// produce equivalent results (see tests/test_potentially_slicing_shared_ptr.cpp,py): | ||
/// | ||
/// - obj.cast<std::shared_ptr<T>>() | ||
/// - py::potentially_slicing_shared_ptr<T>(obj) | ||
/// | ||
/// For class_-wrapped types with trampolines and using py::smart_holder, obj.cast<>() | ||
/// produces a std::shared_ptr that keeps any derived Python objects alive for its own lifetime, | ||
/// but this is achieved by introducing a std::shared_ptr control block that is independent of | ||
/// the one owned by the py::smart_holder. This can lead to surprising std::weak_ptr behavior | ||
/// (see issue #5623). An easy solution is to use py::potentially_slicing_shared_ptr<>(obj), | ||
/// as exercised in tests/test_potentially_slicing_shared_ptr.cpp,py (look for | ||
/// "set_wp_potentially_slicing"). Note, however, that this reintroduces the inheritance | ||
/// slicing issue (see issue #1333). The ideal — but usually more involved — solution is to use | ||
/// a Python weakref to the derived Python object, instead of a C++ base-class std::weak_ptr. | ||
/// | ||
/// It is not possible (at least no known approach exists at the time of this writing) to | ||
/// simultaneously achieve both desirable properties: | ||
/// | ||
/// - the same std::shared_ptr control block as the class_ holder | ||
/// - automatic lifetime extension of any derived Python objects | ||
/// | ||
/// The reason is that this would introduce a reference cycle that cannot be garbage collected: | ||
/// | ||
/// - the derived Python object owns the class_ holder | ||
/// - the class_ holder owns the std::shared_ptr | ||
/// - the std::shared_ptr would own a reference to the derived Python object, | ||
/// completing the cycle | ||
template <typename T> | ||
std::shared_ptr<T> potentially_slicing_shared_ptr(handle obj) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This API is weird to me. If the sole reason for this API to exist is to obtain a weak_ptr that doesn't accidentally expire, why not just return a weak_ptr directly instead? Are there other reasons one might want to get this problematic shared_ptr? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think you're right. Trying it out right now: 222b4a7 (that's minimal changes only; more will be needed if testing goes well (as expected)) The implementation and the testing is a little more awkward, because we cannot pass There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
detail::make_caster<std::shared_ptr<T>> caster; | ||
if (caster.load(obj, /*convert=*/true)) { | ||
return caster.potentially_slicing_shared_ptr(); | ||
} | ||
const char *obj_type_name = detail::obj_class_name(obj.ptr()); | ||
throw type_error("\"" + std::string(obj_type_name) | ||
+ "\" object is not convertible to std::shared_ptr<T> (with T = " | ||
+ type_id<T>() + ")"); | ||
} | ||
|
||
PYBIND11_NAMESPACE_BEGIN(detail) | ||
|
||
// SMART_HOLDER_BAKEIN_FOLLOW_ON: Rewrite comment, with reference to unique_ptr specialization. | ||
/// Type caster for holder types like std::unique_ptr. | ||
/// Please consider the SFINAE hook an implementation detail, as explained | ||
|
Original file line number | Diff line number | Diff line change | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -759,7 +759,9 @@ struct load_helper : value_and_holder_helper { | |||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
std::shared_ptr<T> load_as_shared_ptr(void *void_raw_ptr, | ||||||||||||||||||
handle responsible_parent = nullptr) const { | ||||||||||||||||||
handle responsible_parent = nullptr, | ||||||||||||||||||
bool force_potentially_slicing_shared_ptr | ||||||||||||||||||
= false) const { | ||||||||||||||||||
if (!have_holder()) { | ||||||||||||||||||
return nullptr; | ||||||||||||||||||
} | ||||||||||||||||||
|
@@ -774,7 +776,7 @@ struct load_helper : value_and_holder_helper { | |||||||||||||||||
throw std::runtime_error("Non-owning holder (load_as_shared_ptr)."); | ||||||||||||||||||
} | ||||||||||||||||||
auto *type_raw_ptr = static_cast<T *>(void_raw_ptr); | ||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider adding a comment here to explain the purpose of the new parameter 'force_potentially_slicing_shared_ptr' and how it affects the alias check, so future maintainers understand why this conditional branch bypasses the slicing protection.
Suggested change
Copilot uses AI. Check for mistakes. Positive FeedbackNegative Feedback There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I chose this less verbose, and less redundant, alternative: commit b349e84 |
||||||||||||||||||
if (python_instance_is_alias) { | ||||||||||||||||||
if (python_instance_is_alias && !force_potentially_slicing_shared_ptr) { | ||||||||||||||||||
auto *vptr_gd_ptr = std::get_deleter<memory::guarded_delete>(hld.vptr); | ||||||||||||||||||
if (vptr_gd_ptr != nullptr) { | ||||||||||||||||||
std::shared_ptr<void> released_ptr = vptr_gd_ptr->released_ptr.lock(); | ||||||||||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,163 @@ | ||
// Copyright (c) 2025 The pybind Community. | ||
// All rights reserved. Use of this source code is governed by a | ||
// BSD-style license that can be found in the LICENSE file. | ||
|
||
#include "pybind11_tests.h" | ||
|
||
#include <memory> | ||
|
||
namespace pybind11_tests { | ||
namespace potentially_slicing_shared_ptr { | ||
|
||
template <int> // Using int as a trick to easily generate multiple types. | ||
struct VirtBase { | ||
virtual ~VirtBase() = default; | ||
virtual int get_code() { return 100; } | ||
}; | ||
|
||
using VirtBaseSH = VirtBase<0>; // for testing with py::smart_holder | ||
using VirtBaseSP = VirtBase<1>; // for testing with std::shared_ptr as holder | ||
|
||
// Similar to trampoline_self_life_support | ||
struct trampoline_is_alive_simple { | ||
std::uint64_t magic_token = 197001010000u; | ||
|
||
trampoline_is_alive_simple() = default; | ||
|
||
~trampoline_is_alive_simple() { magic_token = 20380118191407u; } | ||
|
||
trampoline_is_alive_simple(const trampoline_is_alive_simple &other) = default; | ||
trampoline_is_alive_simple(trampoline_is_alive_simple &&other) noexcept | ||
: magic_token(other.magic_token) { | ||
other.magic_token = 20380118191407u; | ||
} | ||
|
||
trampoline_is_alive_simple &operator=(const trampoline_is_alive_simple &) = delete; | ||
trampoline_is_alive_simple &operator=(trampoline_is_alive_simple &&) = delete; | ||
}; | ||
|
||
template <typename VB> | ||
const char *determine_trampoline_state(const std::shared_ptr<VB> &sp) { | ||
if (!sp) { | ||
return "sp nullptr"; | ||
} | ||
auto *tias = dynamic_cast<trampoline_is_alive_simple *>(sp.get()); | ||
if (!tias) { | ||
return "dynamic_cast failed"; | ||
} | ||
if (tias->magic_token == 197001010000u) { | ||
return "trampoline alive"; | ||
} | ||
if (tias->magic_token == 20380118191407u) { | ||
return "trampoline DEAD"; | ||
} | ||
return "UNDEFINED BEHAVIOR"; | ||
} | ||
|
||
struct PyVirtBaseSH : VirtBaseSH, py::trampoline_self_life_support, trampoline_is_alive_simple { | ||
using VirtBaseSH::VirtBaseSH; | ||
int get_code() override { PYBIND11_OVERRIDE(int, VirtBaseSH, get_code); } | ||
}; | ||
|
||
struct PyVirtBaseSP : VirtBaseSP, trampoline_is_alive_simple { // self-life-support not available | ||
using VirtBaseSP::VirtBaseSP; | ||
int get_code() override { PYBIND11_OVERRIDE(int, VirtBaseSP, get_code); } | ||
}; | ||
|
||
template <typename VB> | ||
std::shared_ptr<VB> rtrn_obj_cast_shared_ptr(py::handle obj) { | ||
return obj.cast<std::shared_ptr<VB>>(); | ||
} | ||
|
||
template <typename VB> | ||
std::shared_ptr<VB> rtrn_potentially_slicing_shared_ptr(py::handle obj) { | ||
return py::potentially_slicing_shared_ptr<VB>(obj); | ||
} | ||
|
||
template <typename VB> | ||
struct SpOwner { | ||
void set_sp(const std::shared_ptr<VB> &sp_) { sp = sp_; } | ||
|
||
int get_code() const { | ||
if (!sp) { | ||
return -888; | ||
} | ||
return sp->get_code(); | ||
} | ||
|
||
const char *get_trampoline_state() const { return determine_trampoline_state(sp); } | ||
|
||
private: | ||
std::shared_ptr<VB> sp; | ||
}; | ||
|
||
template <typename VB> | ||
struct WpOwner { | ||
void set_wp(const std::shared_ptr<VB> &sp) { wp = sp; } | ||
|
||
int get_code() const { | ||
auto sp = wp.lock(); | ||
if (!sp) { | ||
return -999; | ||
} | ||
return sp->get_code(); | ||
} | ||
|
||
const char *get_trampoline_state() const { return determine_trampoline_state(wp.lock()); } | ||
|
||
private: | ||
std::weak_ptr<VB> wp; | ||
}; | ||
|
||
template <typename VB> | ||
void wrap(py::module_ &m, | ||
const char *roc_pyname, | ||
const char *rps_pyname, | ||
const char *spo_pyname, | ||
const char *wpo_pyname) { | ||
m.def(roc_pyname, rtrn_obj_cast_shared_ptr<VB>); | ||
m.def(rps_pyname, rtrn_potentially_slicing_shared_ptr<VB>); | ||
|
||
py::classh<SpOwner<VB>>(m, spo_pyname) | ||
.def(py::init<>()) | ||
.def("set_sp", &SpOwner<VB>::set_sp) | ||
.def("get_code", &SpOwner<VB>::get_code) | ||
.def("get_trampoline_state", &SpOwner<VB>::get_trampoline_state); | ||
|
||
py::classh<WpOwner<VB>>(m, wpo_pyname) | ||
.def(py::init<>()) | ||
.def("set_wp", &WpOwner<VB>::set_wp) | ||
.def("set_wp_potentially_slicing", | ||
[](WpOwner<VB> &self, py::handle obj) { | ||
self.set_wp(py::potentially_slicing_shared_ptr<VB>(obj)); | ||
}) | ||
.def("get_code", &WpOwner<VB>::get_code) | ||
.def("get_trampoline_state", &WpOwner<VB>::get_trampoline_state); | ||
} | ||
|
||
} // namespace potentially_slicing_shared_ptr | ||
} // namespace pybind11_tests | ||
|
||
using namespace pybind11_tests::potentially_slicing_shared_ptr; | ||
|
||
TEST_SUBMODULE(potentially_slicing_shared_ptr, m) { | ||
py::classh<VirtBaseSH, PyVirtBaseSH>(m, "VirtBaseSH") | ||
.def(py::init<>()) | ||
.def("get_code", &VirtBaseSH::get_code); | ||
|
||
py::class_<VirtBaseSP, std::shared_ptr<VirtBaseSP>, PyVirtBaseSP>(m, "VirtBaseSP") | ||
.def(py::init<>()) | ||
.def("get_code", &VirtBaseSP::get_code); | ||
|
||
wrap<VirtBaseSH>(m, | ||
"SH_rtrn_obj_cast_shared_ptr", | ||
"SH_rtrn_potentially_slicing_shared_ptr", | ||
"SH_SpOwner", | ||
"SH_WpOwner"); | ||
|
||
wrap<VirtBaseSP>(m, | ||
"SP_rtrn_obj_cast_shared_ptr", | ||
"SP_rtrn_potentially_slicing_shared_ptr", | ||
"SP_SpOwner", | ||
"SP_WpOwner"); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] A short inline comment explaining why the call to load_as_shared_ptr uses force_potentially_slicing_shared_ptr=true would help maintain clarity for future readers.
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done:
// Reusing shared_ptr code to minimize code complexity.