Skip to content

Commit 74b5242

Browse files
nsoblathrwgk
andauthored
feat: add py::potentially_slicing_weak_ptr(handle) function (#5624)
* Added weak_ptr test (currently failing) * Cleanup * [skip ci] Simplify test case. * Add test_class_sp_trampoline_weak_ptr.cpp,py (using std::shared_ptr as holder). Tweak test_class_sh_trampoline_weak_ptr.py to pass, with `# THIS NEEDS FIXING` comment. * Resolve clang-tidy errors ``` /__w/pybind11/pybind11/tests/test_class_sh_trampoline_weak_ptr.cpp:23:43: error: the parameter 'sp' is copied for each invocation but only used as a const reference; consider making it a const reference [performance-unnecessary-value-param,-warnings-as-errors] 23 | void set_wp(std::shared_ptr<VirtBase> sp) { wp = sp; } | ^ | const & 4443 warnings generated. ``` ``` /__w/pybind11/pybind11/tests/test_class_sp_trampoline_weak_ptr.cpp:23:43: error: the parameter 'sp' is copied for each invocation but only used as a const reference; consider making it a const reference [performance-unnecessary-value-param,-warnings-as-errors] 23 | void set_wp(std::shared_ptr<VirtBase> sp) { wp = sp; } | ^ | const & 4430 warnings generated. ``` * PYPY, GRAALPY: skip last part of test_weak_ptr_base * Rename test_weak_ptr_base → test_weak_ptr_owner * Add SpOwner, test_with_sp_owner, test_with_sp_and_wp_owners * Modify py::trampoline_self_life_support semantics: if trampoline class does not inherit from this class, preserve established Inheritance Slicing behavior. rwgk reached this point with the help of ChatGPT: * https://chatgpt.com/share/68056498-7d94-8008-8ff0-232e2aba451c The only production code change in this commit is: ``` diff --git a/include/pybind11/detail/type_caster_base.h b/include/pybind11/detail/type_caster_base.h index d4f9a41..f3d4530 100644 --- a/include/pybind11/detail/type_caster_base.h +++ b/include/pybind11/detail/type_caster_base.h @@ -776,6 +776,14 @@ struct load_helper : value_and_holder_helper { if (released_ptr) { return std::shared_ptr<T>(released_ptr, type_raw_ptr); } + auto *self_life_support + = dynamic_raw_ptr_cast_if_possible<trampoline_self_life_support>(type_raw_ptr); + if (self_life_support == nullptr) { + std::shared_ptr<void> void_shd_ptr = hld.template as_shared_ptr<void>(); + std::shared_ptr<T> to_be_released(void_shd_ptr, type_raw_ptr); + vptr_gd_ptr->released_ptr = to_be_released; + return to_be_released; + } std::shared_ptr<T> to_be_released( type_raw_ptr, shared_ptr_trampoline_self_life_support(loaded_v_h.inst)); vptr_gd_ptr->released_ptr = to_be_released; ``` * Remove debug printf in include/pybind11/pybind11.h * Resolve MSVC error ``` 11>D:\a\pybind11\pybind11\tests\test_class_sp_trampoline_weak_ptr.cpp(44,50): error C2220: the following warning is treated as an error [D:\a\pybind11\pybind11\build\tests\pybind11_tests.vcxproj] 11>D:\a\pybind11\pybind11\tests\test_class_sp_trampoline_weak_ptr.cpp(44,50): warning C4458: declaration of 'sp' hides class member [D:\a\pybind11\pybind11\build\tests\pybind11_tests.vcxproj] D:\a\pybind11\pybind11\tests\test_class_sp_trampoline_weak_ptr.cpp(54,31): see declaration of 'pybind11_tests::class_sp_trampoline_weak_ptr::SpOwner::sp' ``` * [skip ci] Undo the production code change under 4638e01 Also undo the corresponding test change in test_class_sh_trampoline_weak_ptr.py But keep all extra debugging code for now. * [skip ci] Introduce lambda in `WpOwner::set_wp` bindings, but simply cast to `std::shared_ptr<VirtBase>` for now. * Add `py::potentially_slicing_shared_ptr()` * Add `type_id<T>()` to `py::potentially_slicing_shared_ptr()` error message and add test. * test_potentially_slicing_shared_ptr.cpp,py (for smart_holder only) * Generalize test_potentially_slicing_shared_ptr.cpp,py for testing with smart_holder and std::shared_ptr as holder. * Add back test_potentially_slicing_shared_ptr_not_convertible_error(), it got lost accidentally in commit 56d23dc * Add simple trampoline state assertions. * Resolve clang-tidy errors. ``` /__w/pybind11/pybind11/tests/test_potentially_slicing_shared_ptr.cpp:30:9: error: 'magic_token' should be initialized in a member initializer of the constructor [cppcoreguidelines-prefer-member-initializer,-warnings-as-errors] 29 | trampoline_is_alive_simple(const trampoline_is_alive_simple &other) { | : magic_token(other.magic_token) 30 | magic_token = other.magic_token; | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ /__w/pybind11/pybind11/tests/test_potentially_slicing_shared_ptr.cpp:33:9: error: 'magic_token' should be initialized in a member initializer of the constructor [cppcoreguidelines-prefer-member-initializer,-warnings-as-errors] 32 | trampoline_is_alive_simple(trampoline_is_alive_simple &&other) noexcept { | : magic_token(other.magic_token) 33 | magic_token = other.magic_token; | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ``` * Add a (long) C++ comment for py::potentially_slicing_shared_ptr<>() * [skip ci] Add new "Avoiding Inheritance Slicing and ``std::weak_ptr`` surprises" section in advanced/classes.rst * [skip ci] Add introductory comment to test_potentially_slicing_shared_ptr.py * Minimal (!) changes to have py::potentially_slicing_weak_ptr<T>(handle) as the public API. For CI testing, before changing the names around more widely, and the documentation. * Rename test_potentially_slicing_shared_ptr.cpp,py → test_potentially_slicing_weak_ptr.cpp,py * Update docs/advanced/classes.rst and C++ comments → potentially_slicing_weak_ptr * Write "shared_ptr" instead of just "pointer" in a couple places in docs/advanced/classes.rst * Add comment for force_potentially_slicing_shared_ptr in type_caster_base.h, to make a direct connection py::potentially_slicing_weak_ptr --------- Co-authored-by: Ralf W. Grosse-Kunstleve <[email protected]>
1 parent ce42c4d commit 74b5242

File tree

6 files changed

+453
-2
lines changed

6 files changed

+453
-2
lines changed

docs/advanced/classes.rst

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -428,6 +428,51 @@ Python side by allowing the Python function to return ``None`` or an ``int``:
428428
return false; // Alternatively return MyClass::myMethod(value);
429429
}
430430
431+
Avoiding Inheritance Slicing and ``std::weak_ptr`` surprises
432+
------------------------------------------------------------
433+
434+
When working with classes that use virtual functions and are subclassed
435+
in Python, special care must be taken when converting Python objects to
436+
``std::shared_ptr<T>``. Depending on whether the class uses a plain
437+
``std::shared_ptr`` holder or ``py::smart_holder``, the resulting
438+
``shared_ptr`` may either allow inheritance slicing or lead to potentially
439+
surprising behavior when constructing ``std::weak_ptr`` instances.
440+
441+
This section explains how ``std::shared_ptr`` and ``py::smart_holder`` manage
442+
object lifetimes differently, how these differences affect trampoline-derived
443+
objects, and what options are available to achieve the situation-specific
444+
desired behavior.
445+
446+
When using ``std::shared_ptr`` as the holder type, converting a Python object
447+
to a ``std::shared_ptr<T>`` (e.g., ``obj.cast<std::shared_ptr<T>>()``, or simply
448+
passing the Python object as an argument to a ``.def()``-ed function) returns
449+
a ``shared_ptr`` that shares ownership with the original ``class_`` holder,
450+
usually preserving object lifetime. However, for Python classes that derive from
451+
a trampoline, if the Python object is destroyed, only the base C++ object may
452+
remain alive, leading to inheritance slicing
453+
(see `#1333 <https://github.com/pybind/pybind11/issues/1333>`_).
454+
455+
In contrast, with ``py::smart_holder``, converting a Python object to
456+
a ``std::shared_ptr<T>`` returns a new ``shared_ptr`` with an independent
457+
control block that keeps the derived Python object alive. This avoids
458+
inheritance slicing but can lead to unintended behavior when creating
459+
``std::weak_ptr`` instances
460+
(see `#5623 <https://github.com/pybind/pybind11/issues/5623>`_).
461+
462+
If it is necessary to obtain a ``std::weak_ptr`` that shares the control block
463+
with the ``smart_holder``—at the cost of reintroducing potential inheritance
464+
slicing—you can use ``py::potentially_slicing_weak_ptr<T>(obj)``.
465+
466+
When precise lifetime management of derived Python objects is important,
467+
using a Python-side ``weakref`` is the most reliable approach, as it avoids
468+
both inheritance slicing and unintended interactions with ``std::weak_ptr``
469+
semantics in C++.
470+
471+
.. seealso::
472+
473+
* :func:`potentially_slicing_weak_ptr` C++ documentation
474+
* :file:`tests/test_potentially_slicing_weak_ptr.cpp`
475+
431476

432477
.. _custom_constructors:
433478

include/pybind11/cast.h

Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -983,6 +983,17 @@ struct copyable_holder_caster<
983983
return shared_ptr_storage;
984984
}
985985

986+
std::weak_ptr<type> potentially_slicing_weak_ptr() {
987+
if (typeinfo->holder_enum_v == detail::holder_enum_t::smart_holder) {
988+
// Reusing shared_ptr code to minimize code complexity.
989+
shared_ptr_storage
990+
= sh_load_helper.load_as_shared_ptr(value,
991+
/*responsible_parent=*/nullptr,
992+
/*force_potentially_slicing_shared_ptr=*/true);
993+
}
994+
return shared_ptr_storage;
995+
}
996+
986997
static handle
987998
cast(const std::shared_ptr<type> &src, return_value_policy policy, handle parent) {
988999
const auto *ptr = src.get();
@@ -1077,6 +1088,54 @@ struct copyable_holder_caster<
10771088
template <typename T>
10781089
class type_caster<std::shared_ptr<T>> : public copyable_holder_caster<T, std::shared_ptr<T>> {};
10791090

1091+
PYBIND11_NAMESPACE_END(detail)
1092+
1093+
/// Return a std::shared_ptr with the SAME CONTROL BLOCK as the std::shared_ptr owned by the
1094+
/// class_ holder. For class_-wrapped types with trampolines, the returned std::shared_ptr
1095+
/// does NOT keep any derived Python objects alive (see issue #1333).
1096+
///
1097+
/// For class_-wrapped types using std::shared_ptr as the holder, the following expressions
1098+
/// produce equivalent results (see tests/test_potentially_slicing_weak_ptr.cpp,py):
1099+
///
1100+
/// - obj.cast<std::shared_ptr<T>>()
1101+
/// - py::potentially_slicing_weak_ptr<T>(obj).lock()
1102+
///
1103+
/// For class_-wrapped types with trampolines and using py::smart_holder, obj.cast<>()
1104+
/// produces a std::shared_ptr that keeps any derived Python objects alive for its own lifetime,
1105+
/// but this is achieved by introducing a std::shared_ptr control block that is independent of
1106+
/// the one owned by the py::smart_holder. This can lead to surprising std::weak_ptr behavior
1107+
/// (see issue #5623). An easy solution is to use py::potentially_slicing_weak_ptr<>(obj),
1108+
/// as exercised in tests/test_potentially_slicing_weak_ptr.cpp,py (look for
1109+
/// "set_wp_potentially_slicing"). Note, however, that this reintroduces the inheritance
1110+
/// slicing issue (see issue #1333). The ideal — but usually more involved — solution is to use
1111+
/// a Python weakref to the derived Python object, instead of a C++ base-class std::weak_ptr.
1112+
///
1113+
/// It is not possible (at least no known approach exists at the time of this writing) to
1114+
/// simultaneously achieve both desirable properties:
1115+
///
1116+
/// - the same std::shared_ptr control block as the class_ holder
1117+
/// - automatic lifetime extension of any derived Python objects
1118+
///
1119+
/// The reason is that this would introduce a reference cycle that cannot be garbage collected:
1120+
///
1121+
/// - the derived Python object owns the class_ holder
1122+
/// - the class_ holder owns the std::shared_ptr
1123+
/// - the std::shared_ptr would own a reference to the derived Python object,
1124+
/// completing the cycle
1125+
template <typename T>
1126+
std::weak_ptr<T> potentially_slicing_weak_ptr(handle obj) {
1127+
detail::make_caster<std::shared_ptr<T>> caster;
1128+
if (caster.load(obj, /*convert=*/true)) {
1129+
return caster.potentially_slicing_weak_ptr();
1130+
}
1131+
const char *obj_type_name = detail::obj_class_name(obj.ptr());
1132+
throw type_error("\"" + std::string(obj_type_name)
1133+
+ "\" object is not convertible to std::weak_ptr<T> (with T = " + type_id<T>()
1134+
+ ")");
1135+
}
1136+
1137+
PYBIND11_NAMESPACE_BEGIN(detail)
1138+
10801139
// SMART_HOLDER_BAKEIN_FOLLOW_ON: Rewrite comment, with reference to unique_ptr specialization.
10811140
/// Type caster for holder types like std::unique_ptr.
10821141
/// Please consider the SFINAE hook an implementation detail, as explained

include/pybind11/detail/type_caster_base.h

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -759,7 +759,11 @@ struct load_helper : value_and_holder_helper {
759759
}
760760

761761
std::shared_ptr<T> load_as_shared_ptr(void *void_raw_ptr,
762-
handle responsible_parent = nullptr) const {
762+
handle responsible_parent = nullptr,
763+
// to support py::potentially_slicing_weak_ptr
764+
// with minimal added code complexity:
765+
bool force_potentially_slicing_shared_ptr
766+
= false) const {
763767
if (!have_holder()) {
764768
return nullptr;
765769
}
@@ -774,7 +778,7 @@ struct load_helper : value_and_holder_helper {
774778
throw std::runtime_error("Non-owning holder (load_as_shared_ptr).");
775779
}
776780
auto *type_raw_ptr = static_cast<T *>(void_raw_ptr);
777-
if (python_instance_is_alias) {
781+
if (python_instance_is_alias && !force_potentially_slicing_shared_ptr) {
778782
auto *vptr_gd_ptr = std::get_deleter<memory::guarded_delete>(hld.vptr);
779783
if (vptr_gd_ptr != nullptr) {
780784
std::shared_ptr<void> released_ptr = vptr_gd_ptr->released_ptr.lock();

tests/CMakeLists.txt

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -161,6 +161,7 @@ set(PYBIND11_TEST_FILES
161161
test_opaque_types
162162
test_operator_overloading
163163
test_pickling
164+
test_potentially_slicing_weak_ptr
164165
test_python_multiple_inheritance
165166
test_pytypes
166167
test_sequences_and_iterators
Lines changed: 168 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,168 @@
1+
// Copyright (c) 2025 The pybind Community.
2+
// All rights reserved. Use of this source code is governed by a
3+
// BSD-style license that can be found in the LICENSE file.
4+
5+
#include "pybind11_tests.h"
6+
7+
#include <memory>
8+
9+
namespace pybind11_tests {
10+
namespace potentially_slicing_weak_ptr {
11+
12+
template <int> // Using int as a trick to easily generate multiple types.
13+
struct VirtBase {
14+
virtual ~VirtBase() = default;
15+
virtual int get_code() { return 100; }
16+
};
17+
18+
using VirtBaseSH = VirtBase<0>; // for testing with py::smart_holder
19+
using VirtBaseSP = VirtBase<1>; // for testing with std::shared_ptr as holder
20+
21+
// Similar to trampoline_self_life_support
22+
struct trampoline_is_alive_simple {
23+
std::uint64_t magic_token = 197001010000u;
24+
25+
trampoline_is_alive_simple() = default;
26+
27+
~trampoline_is_alive_simple() { magic_token = 20380118191407u; }
28+
29+
trampoline_is_alive_simple(const trampoline_is_alive_simple &other) = default;
30+
trampoline_is_alive_simple(trampoline_is_alive_simple &&other) noexcept
31+
: magic_token(other.magic_token) {
32+
other.magic_token = 20380118191407u;
33+
}
34+
35+
trampoline_is_alive_simple &operator=(const trampoline_is_alive_simple &) = delete;
36+
trampoline_is_alive_simple &operator=(trampoline_is_alive_simple &&) = delete;
37+
};
38+
39+
template <typename VB>
40+
const char *determine_trampoline_state(const std::shared_ptr<VB> &sp) {
41+
if (!sp) {
42+
return "sp nullptr";
43+
}
44+
auto *tias = dynamic_cast<trampoline_is_alive_simple *>(sp.get());
45+
if (!tias) {
46+
return "dynamic_cast failed";
47+
}
48+
if (tias->magic_token == 197001010000u) {
49+
return "trampoline alive";
50+
}
51+
if (tias->magic_token == 20380118191407u) {
52+
return "trampoline DEAD";
53+
}
54+
return "UNDEFINED BEHAVIOR";
55+
}
56+
57+
struct PyVirtBaseSH : VirtBaseSH, py::trampoline_self_life_support, trampoline_is_alive_simple {
58+
using VirtBaseSH::VirtBaseSH;
59+
int get_code() override { PYBIND11_OVERRIDE(int, VirtBaseSH, get_code); }
60+
};
61+
62+
struct PyVirtBaseSP : VirtBaseSP, trampoline_is_alive_simple { // self-life-support not available
63+
using VirtBaseSP::VirtBaseSP;
64+
int get_code() override { PYBIND11_OVERRIDE(int, VirtBaseSP, get_code); }
65+
};
66+
67+
template <typename VB>
68+
std::shared_ptr<VB> rtrn_obj_cast_shared_ptr(py::handle obj) {
69+
return obj.cast<std::shared_ptr<VB>>();
70+
}
71+
72+
// There is no type_caster<std::weak_ptr<VB>>, and to minimize code complexity
73+
// we do not want to add one, therefore we have to return a shared_ptr here.
74+
template <typename VB>
75+
std::shared_ptr<VB> rtrn_potentially_slicing_shared_ptr(py::handle obj) {
76+
return py::potentially_slicing_weak_ptr<VB>(obj).lock();
77+
}
78+
79+
template <typename VB>
80+
struct SpOwner {
81+
void set_sp(const std::shared_ptr<VB> &sp_) { sp = sp_; }
82+
83+
int get_code() const {
84+
if (!sp) {
85+
return -888;
86+
}
87+
return sp->get_code();
88+
}
89+
90+
const char *get_trampoline_state() const { return determine_trampoline_state(sp); }
91+
92+
private:
93+
std::shared_ptr<VB> sp;
94+
};
95+
96+
template <typename VB>
97+
struct WpOwner {
98+
void set_wp(const std::weak_ptr<VB> &wp_) { wp = wp_; }
99+
100+
int get_code() const {
101+
auto sp = wp.lock();
102+
if (!sp) {
103+
return -999;
104+
}
105+
return sp->get_code();
106+
}
107+
108+
const char *get_trampoline_state() const { return determine_trampoline_state(wp.lock()); }
109+
110+
private:
111+
std::weak_ptr<VB> wp;
112+
};
113+
114+
template <typename VB>
115+
void wrap(py::module_ &m,
116+
const char *roc_pyname,
117+
const char *rps_pyname,
118+
const char *spo_pyname,
119+
const char *wpo_pyname) {
120+
m.def(roc_pyname, rtrn_obj_cast_shared_ptr<VB>);
121+
m.def(rps_pyname, rtrn_potentially_slicing_shared_ptr<VB>);
122+
123+
py::classh<SpOwner<VB>>(m, spo_pyname)
124+
.def(py::init<>())
125+
.def("set_sp", &SpOwner<VB>::set_sp)
126+
.def("get_code", &SpOwner<VB>::get_code)
127+
.def("get_trampoline_state", &SpOwner<VB>::get_trampoline_state);
128+
129+
py::classh<WpOwner<VB>>(m, wpo_pyname)
130+
.def(py::init<>())
131+
.def("set_wp",
132+
[](WpOwner<VB> &self, py::handle obj) {
133+
self.set_wp(obj.cast<std::shared_ptr<VB>>());
134+
})
135+
.def("set_wp_potentially_slicing",
136+
[](WpOwner<VB> &self, py::handle obj) {
137+
self.set_wp(py::potentially_slicing_weak_ptr<VB>(obj));
138+
})
139+
.def("get_code", &WpOwner<VB>::get_code)
140+
.def("get_trampoline_state", &WpOwner<VB>::get_trampoline_state);
141+
}
142+
143+
} // namespace potentially_slicing_weak_ptr
144+
} // namespace pybind11_tests
145+
146+
using namespace pybind11_tests::potentially_slicing_weak_ptr;
147+
148+
TEST_SUBMODULE(potentially_slicing_weak_ptr, m) {
149+
py::classh<VirtBaseSH, PyVirtBaseSH>(m, "VirtBaseSH")
150+
.def(py::init<>())
151+
.def("get_code", &VirtBaseSH::get_code);
152+
153+
py::class_<VirtBaseSP, std::shared_ptr<VirtBaseSP>, PyVirtBaseSP>(m, "VirtBaseSP")
154+
.def(py::init<>())
155+
.def("get_code", &VirtBaseSP::get_code);
156+
157+
wrap<VirtBaseSH>(m,
158+
"SH_rtrn_obj_cast_shared_ptr",
159+
"SH_rtrn_potentially_slicing_shared_ptr",
160+
"SH_SpOwner",
161+
"SH_WpOwner");
162+
163+
wrap<VirtBaseSP>(m,
164+
"SP_rtrn_obj_cast_shared_ptr",
165+
"SP_rtrn_potentially_slicing_shared_ptr",
166+
"SP_SpOwner",
167+
"SP_WpOwner");
168+
}

0 commit comments

Comments
 (0)