Skip to content

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

Merged
merged 34 commits into from
May 13, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
34 commits
Select commit Hold shift + click to select a range
50c3c1e
Added weak_ptr test (currently failing)
nsoblath Apr 16, 2025
528d1cc
Merge branch 'master' into weak_ptr_test_only
nsoblath Apr 17, 2025
b160f6c
Cleanup
nsoblath Apr 17, 2025
2fe82b2
[skip ci] Simplify test case.
rwgk Apr 19, 2025
ff70657
Add test_class_sp_trampoline_weak_ptr.cpp,py (using std::shared_ptr a…
rwgk Apr 19, 2025
a481647
Resolve clang-tidy errors
rwgk Apr 19, 2025
32858da
PYPY, GRAALPY: skip last part of test_weak_ptr_base
rwgk Apr 19, 2025
e978d93
Rename test_weak_ptr_base → test_weak_ptr_owner
rwgk Apr 19, 2025
1c0b700
Add SpOwner, test_with_sp_owner, test_with_sp_and_wp_owners
rwgk Apr 20, 2025
4638e01
Modify py::trampoline_self_life_support semantics: if trampoline clas…
rwgk Apr 20, 2025
6cc6368
Remove debug printf in include/pybind11/pybind11.h
rwgk Apr 20, 2025
62d3265
Resolve MSVC error
rwgk Apr 20, 2025
2f672eb
[skip ci] Undo the production code change under 4638e017b6e7bbd246c03…
rwgk Apr 26, 2025
888a295
[skip ci] Introduce lambda in `WpOwner::set_wp` bindings, but simply …
rwgk Apr 26, 2025
b20f557
Add `py::potentially_slicing_shared_ptr()`
rwgk Apr 26, 2025
3b4b28c
Add `type_id<T>()` to `py::potentially_slicing_shared_ptr()` error me…
rwgk Apr 27, 2025
56d23dc
test_potentially_slicing_shared_ptr.cpp,py (for smart_holder only)
rwgk Apr 27, 2025
ff0792e
Generalize test_potentially_slicing_shared_ptr.cpp,py for testing wit…
rwgk Apr 27, 2025
eb1787c
Add back test_potentially_slicing_shared_ptr_not_convertible_error(),…
rwgk Apr 27, 2025
10f6e78
Add simple trampoline state assertions.
rwgk Apr 28, 2025
121c8cf
Resolve clang-tidy errors.
rwgk Apr 28, 2025
b42bd11
Add a (long) C++ comment for py::potentially_slicing_shared_ptr<>()
rwgk Apr 28, 2025
f160450
[skip ci] Add new "Avoiding Inheritance Slicing and ``std::weak_ptr``…
rwgk Apr 28, 2025
78178d7
[skip ci] Add introductory comment to test_potentially_slicing_shared…
rwgk Apr 28, 2025
62d2242
[skip ci] Merge branch 'master' into weak_ptr_test_only
rwgk Apr 28, 2025
0ecb396
Merge branch 'master' into weak_ptr_test_only
rwgk Apr 30, 2025
faf1e61
Merge branch 'master' into weak_ptr_test_only
rwgk May 1, 2025
222b4a7
Minimal (!) changes to have py::potentially_slicing_weak_ptr<T>(handl…
rwgk May 1, 2025
7c4a8bc
Merge branch 'master' into weak_ptr_test_only
rwgk May 8, 2025
03a8981
Rename test_potentially_slicing_shared_ptr.cpp,py → test_potentially_…
rwgk May 8, 2025
bad0c12
Update docs/advanced/classes.rst and C++ comments → potentially_slici…
rwgk May 8, 2025
683fff4
Write "shared_ptr" instead of just "pointer" in a couple places in do…
rwgk May 8, 2025
9f379ea
Merge branch 'master' into weak_ptr_test_only
rwgk May 9, 2025
b349e84
Add comment for force_potentially_slicing_shared_ptr in type_caster_b…
rwgk May 10, 2025
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
45 changes: 45 additions & 0 deletions docs/advanced/classes.rst
Original file line number Diff line number Diff line change
Expand Up @@ -428,6 +428,51 @@ Python side by allowing the Python function to return ``None`` or an ``int``:
return false; // Alternatively return MyClass::myMethod(value);
}

Avoiding Inheritance Slicing and ``std::weak_ptr`` surprises
------------------------------------------------------------

When working with classes that use virtual functions and are subclassed
in Python, special care must be taken when converting Python objects to
``std::shared_ptr<T>``. Depending on whether the class uses a plain
``std::shared_ptr`` holder or ``py::smart_holder``, the resulting
``shared_ptr`` may either allow inheritance slicing or lead to potentially
surprising behavior when constructing ``std::weak_ptr`` instances.

This section explains how ``std::shared_ptr`` and ``py::smart_holder`` manage
object lifetimes differently, how these differences affect trampoline-derived
objects, and what options are available to achieve the situation-specific
desired behavior.

When using ``std::shared_ptr`` as the holder type, converting a Python object
to a ``std::shared_ptr<T>`` (e.g., ``obj.cast<std::shared_ptr<T>>()``, or simply
passing the Python object as an argument to a ``.def()``-ed function) returns
a ``shared_ptr`` that shares ownership with the original ``class_`` holder,
usually preserving object lifetime. However, for Python classes that derive from
a trampoline, if the Python object is destroyed, only the base C++ object may
remain alive, leading to inheritance slicing
(see `#1333 <https://github.com/pybind/pybind11/issues/1333>`_).

In contrast, with ``py::smart_holder``, converting a Python object to
a ``std::shared_ptr<T>`` returns a new ``shared_ptr`` with an independent
control block that keeps the derived Python object alive. This avoids
inheritance slicing but can lead to unintended behavior when creating
``std::weak_ptr`` instances
(see `#5623 <https://github.com/pybind/pybind11/issues/5623>`_).

If it is necessary to obtain a ``std::weak_ptr`` that shares the control block
with the ``smart_holder``—at the cost of reintroducing potential inheritance
slicing—you can use ``py::potentially_slicing_weak_ptr<T>(obj)``.

When precise lifetime management of derived Python objects is important,
using a Python-side ``weakref`` is the most reliable approach, as it avoids
both inheritance slicing and unintended interactions with ``std::weak_ptr``
semantics in C++.

.. seealso::

* :func:`potentially_slicing_weak_ptr` C++ documentation
* :file:`tests/test_potentially_slicing_weak_ptr.cpp`


.. _custom_constructors:

Expand Down
59 changes: 59 additions & 0 deletions include/pybind11/cast.h
Original file line number Diff line number Diff line change
Expand Up @@ -983,6 +983,17 @@ struct copyable_holder_caster<
return shared_ptr_storage;
}

std::weak_ptr<type> potentially_slicing_weak_ptr() {
if (typeinfo->holder_enum_v == detail::holder_enum_t::smart_holder) {
Copy link
Preview

Copilot AI May 1, 2025

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.

Suggested change
if (typeinfo->holder_enum_v == detail::holder_enum_t::smart_holder) {
if (typeinfo->holder_enum_v == detail::holder_enum_t::smart_holder) {
// Use force_potentially_slicing_shared_ptr=true to handle cases where slicing
// might occur, ensuring the shared_ptr is created even if the type is not exact.

Copilot uses AI. Check for mistakes.

Copy link
Collaborator

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.

// Reusing shared_ptr code to minimize code complexity.
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();
Expand Down Expand Up @@ -1077,6 +1088,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_weak_ptr.cpp,py):
///
/// - obj.cast<std::shared_ptr<T>>()
/// - py::potentially_slicing_weak_ptr<T>(obj).lock()
///
/// 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_weak_ptr<>(obj),
/// as exercised in tests/test_potentially_slicing_weak_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::weak_ptr<T> potentially_slicing_weak_ptr(handle obj) {
detail::make_caster<std::shared_ptr<T>> caster;
if (caster.load(obj, /*convert=*/true)) {
return caster.potentially_slicing_weak_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::weak_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
Expand Down
8 changes: 6 additions & 2 deletions include/pybind11/detail/type_caster_base.h
Original file line number Diff line number Diff line change
Expand Up @@ -759,7 +759,11 @@ 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,
// to support py::potentially_slicing_weak_ptr
// with minimal added code complexity:
bool force_potentially_slicing_shared_ptr
= false) const {
if (!have_holder()) {
return nullptr;
}
Expand All @@ -774,7 +778,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);
Copy link
Preview

Copilot AI May 10, 2025

Choose a reason for hiding this comment

The 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
auto *type_raw_ptr = static_cast<T *>(void_raw_ptr);
auto *type_raw_ptr = static_cast<T *>(void_raw_ptr);
// The `force_potentially_slicing_shared_ptr` parameter allows bypassing the slicing
// protection mechanism when `python_instance_is_alias` is true. This is necessary
// in scenarios where the caller explicitly requests a shared_ptr that may result
// in slicing, typically for advanced use cases or performance optimizations.
// Without this flag, the function enforces strict aliasing rules to prevent
// potential slicing issues.

Copilot uses AI. Check for mistakes.

Copy link
Collaborator

Choose a reason for hiding this comment

The 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();
Expand Down
1 change: 1 addition & 0 deletions tests/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,7 @@ set(PYBIND11_TEST_FILES
test_opaque_types
test_operator_overloading
test_pickling
test_potentially_slicing_weak_ptr
test_python_multiple_inheritance
test_pytypes
test_sequences_and_iterators
Expand Down
168 changes: 168 additions & 0 deletions tests/test_potentially_slicing_weak_ptr.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,168 @@
// 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_weak_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>>();
}

// There is no type_caster<std::weak_ptr<VB>>, and to minimize code complexity
// we do not want to add one, therefore we have to return a shared_ptr here.
template <typename VB>
std::shared_ptr<VB> rtrn_potentially_slicing_shared_ptr(py::handle obj) {
return py::potentially_slicing_weak_ptr<VB>(obj).lock();
}

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::weak_ptr<VB> &wp_) { wp = wp_; }

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> &self, py::handle obj) {
self.set_wp(obj.cast<std::shared_ptr<VB>>());
})
.def("set_wp_potentially_slicing",
[](WpOwner<VB> &self, py::handle obj) {
self.set_wp(py::potentially_slicing_weak_ptr<VB>(obj));
})
.def("get_code", &WpOwner<VB>::get_code)
.def("get_trampoline_state", &WpOwner<VB>::get_trampoline_state);
}

} // namespace potentially_slicing_weak_ptr
} // namespace pybind11_tests

using namespace pybind11_tests::potentially_slicing_weak_ptr;

TEST_SUBMODULE(potentially_slicing_weak_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");
}
Loading
Loading