From 50c3c1e89ea7ff9f4a1a452b675268c7e62c0ee7 Mon Sep 17 00:00:00 2001 From: Noah Oblath Date: Tue, 15 Apr 2025 17:25:54 -0700 Subject: [PATCH 01/28] Added weak_ptr test (currently failing) --- tests/CMakeLists.txt | 1 + tests/test_class_sh_trampoline_weak_ptr.cpp | 68 +++++++++++++++++++++ tests/test_class_sh_trampoline_weak_ptr.py | 36 +++++++++++ 3 files changed, 105 insertions(+) create mode 100644 tests/test_class_sh_trampoline_weak_ptr.cpp create mode 100644 tests/test_class_sh_trampoline_weak_ptr.py diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 9ee8131b1a..d0bb65be61 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -130,6 +130,7 @@ set(PYBIND11_TEST_FILES test_class_sh_trampoline_shared_from_this test_class_sh_trampoline_shared_ptr_cpp_arg test_class_sh_trampoline_unique_ptr + test_class_sh_trampoline_weak_ptr test_class_sh_unique_ptr_custom_deleter test_class_sh_unique_ptr_member test_class_sh_virtual_py_cpp_mix diff --git a/tests/test_class_sh_trampoline_weak_ptr.cpp b/tests/test_class_sh_trampoline_weak_ptr.cpp new file mode 100644 index 0000000000..9d251b081c --- /dev/null +++ b/tests/test_class_sh_trampoline_weak_ptr.cpp @@ -0,0 +1,68 @@ +// Copyright (c) 2021 The Pybind Development Team. +// 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 + +namespace pybind11_tests { +namespace class_sh_trampoline_weak_ptr { + +// // For testing whether a python subclass of a C++ object can be accessed from a C++ weak_ptr +struct WpBase { + // returns true if the base virtual function is called + virtual bool is_base_used() { return true; } + + // returns true if there's an associated python instance + bool has_python_instance() { + auto *tinfo = py::detail::get_type_info(typeid(WpBase)); + return (bool) py::detail::get_object_handle(this, tinfo); + } + + WpBase() = default; + WpBase(const WpBase &) = delete; + virtual ~WpBase() = default; +}; + +struct PyWpBase : WpBase { + using WpBase::WpBase; + bool is_base_used() override { PYBIND11_OVERRIDE(bool, WpBase, is_base_used); } +}; + +struct WpBaseTester { + std::shared_ptr get_object() const { return m_obj.lock(); } + void set_object(std::shared_ptr obj) { m_obj = obj; } + bool is_expired() { return m_obj.expired(); } + bool is_base_used() { return m_obj.lock()->is_base_used(); } + // bool has_instance() { return (bool) m_obj.lock(); } + // bool has_python_instance() { return m_obj.lock() && m_obj.lock()->has_python_instance(); + // } void set_nonpython_instance() { m_obj = std::make_shared(); } + std::weak_ptr m_obj; +}; + +} // namespace class_sh_trampoline_weak_ptr +} // namespace pybind11_tests + +using namespace pybind11_tests::class_sh_trampoline_weak_ptr; + +TEST_SUBMODULE(class_sh_trampoline_weak_ptr, m) { + // For testing whether a python subclass of a C++ object can be accessed from a C++ weak_ptr + + py::classh(m, "WpBase") + .def(py::init<>()) + .def(py::init([](int) { return std::make_shared(); })) + .def("is_base_used", &WpBase::is_base_used) + .def("has_python_instance", &WpBase::has_python_instance); + + py::classh(m, "WpBaseTester") + .def(py::init<>()) + .def("get_object", &WpBaseTester::get_object) + .def("set_object", &WpBaseTester::set_object) + .def("is_expired", &WpBaseTester::is_expired) + .def("is_base_used", &WpBaseTester::is_base_used); + // .def("has_instance", &WpBaseTester::has_instance) + // .def("has_python_instance", &WpBaseTester::has_python_instance) + // .def("set_nonpython_instance", &WpBaseTester::set_nonpython_instance) + // .def_readwrite("obj", &WpBaseTester::m_obj); +} diff --git a/tests/test_class_sh_trampoline_weak_ptr.py b/tests/test_class_sh_trampoline_weak_ptr.py new file mode 100644 index 0000000000..545c1ad2e1 --- /dev/null +++ b/tests/test_class_sh_trampoline_weak_ptr.py @@ -0,0 +1,36 @@ +from __future__ import annotations + +import pytest + +import env # noqa: F401 +import pybind11_tests.class_sh_trampoline_weak_ptr as m + + +@pytest.mark.skipif("env.GRAALPY", reason="Cannot reliably trigger GC") +def test_weak_ptr_base(): + tester = m.WpBaseTester() + + obj = m.WpBase() + + tester.set_object(obj) + + assert tester.is_expired() is False + assert tester.is_base_used() is True + assert tester.get_object().is_base_used() is True + + +@pytest.mark.skipif("env.GRAALPY", reason="Cannot reliably trigger GC") +def test_weak_ptr_child(): + class PyChild(m.WpBase): + def is_base_used(self): + return False + + tester = m.WpBaseTester() + + obj = PyChild() + + tester.set_object(obj) + + assert tester.is_expired() is False + assert tester.is_base_used() is False + assert tester.get_object().is_base_used() is False From b160f6c2b106a7f234bd59e216e287c35f6ad892 Mon Sep 17 00:00:00 2001 From: Noah Oblath Date: Thu, 17 Apr 2025 15:17:22 -0700 Subject: [PATCH 02/28] Cleanup --- tests/test_class_sh_trampoline_weak_ptr.cpp | 7 ------- 1 file changed, 7 deletions(-) diff --git a/tests/test_class_sh_trampoline_weak_ptr.cpp b/tests/test_class_sh_trampoline_weak_ptr.cpp index 9d251b081c..dd53206e4c 100644 --- a/tests/test_class_sh_trampoline_weak_ptr.cpp +++ b/tests/test_class_sh_trampoline_weak_ptr.cpp @@ -35,9 +35,6 @@ struct WpBaseTester { void set_object(std::shared_ptr obj) { m_obj = obj; } bool is_expired() { return m_obj.expired(); } bool is_base_used() { return m_obj.lock()->is_base_used(); } - // bool has_instance() { return (bool) m_obj.lock(); } - // bool has_python_instance() { return m_obj.lock() && m_obj.lock()->has_python_instance(); - // } void set_nonpython_instance() { m_obj = std::make_shared(); } std::weak_ptr m_obj; }; @@ -61,8 +58,4 @@ TEST_SUBMODULE(class_sh_trampoline_weak_ptr, m) { .def("set_object", &WpBaseTester::set_object) .def("is_expired", &WpBaseTester::is_expired) .def("is_base_used", &WpBaseTester::is_base_used); - // .def("has_instance", &WpBaseTester::has_instance) - // .def("has_python_instance", &WpBaseTester::has_python_instance) - // .def("set_nonpython_instance", &WpBaseTester::set_nonpython_instance) - // .def_readwrite("obj", &WpBaseTester::m_obj); } From 2fe82b2fdf94e0b17747a30a05b63d830cd7d790 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Sat, 19 Apr 2025 00:18:23 -0700 Subject: [PATCH 03/28] [skip ci] Simplify test case. --- tests/test_class_sh_trampoline_weak_ptr.cpp | 62 +++++++++------------ tests/test_class_sh_trampoline_weak_ptr.py | 35 +++++------- 2 files changed, 39 insertions(+), 58 deletions(-) diff --git a/tests/test_class_sh_trampoline_weak_ptr.cpp b/tests/test_class_sh_trampoline_weak_ptr.cpp index dd53206e4c..9b7651cce9 100644 --- a/tests/test_class_sh_trampoline_weak_ptr.cpp +++ b/tests/test_class_sh_trampoline_weak_ptr.cpp @@ -1,41 +1,37 @@ -// Copyright (c) 2021 The Pybind Development Team. +// Copyright (c) 2025 The Pybind Development Team. // 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 +#include namespace pybind11_tests { namespace class_sh_trampoline_weak_ptr { -// // For testing whether a python subclass of a C++ object can be accessed from a C++ weak_ptr -struct WpBase { - // returns true if the base virtual function is called - virtual bool is_base_used() { return true; } - - // returns true if there's an associated python instance - bool has_python_instance() { - auto *tinfo = py::detail::get_type_info(typeid(WpBase)); - return (bool) py::detail::get_object_handle(this, tinfo); - } - - WpBase() = default; - WpBase(const WpBase &) = delete; - virtual ~WpBase() = default; +struct VirtBase { + virtual ~VirtBase() = default; + virtual int get_code() { return 100; } }; -struct PyWpBase : WpBase { - using WpBase::WpBase; - bool is_base_used() override { PYBIND11_OVERRIDE(bool, WpBase, is_base_used); } +struct PyVirtBase : VirtBase, py::trampoline_self_life_support { + using VirtBase::VirtBase; + int get_code() override { PYBIND11_OVERRIDE(int, VirtBase, get_code); } }; -struct WpBaseTester { - std::shared_ptr get_object() const { return m_obj.lock(); } - void set_object(std::shared_ptr obj) { m_obj = obj; } - bool is_expired() { return m_obj.expired(); } - bool is_base_used() { return m_obj.lock()->is_base_used(); } - std::weak_ptr m_obj; +struct WpOwner { + void set_wp(std::shared_ptr sp) { wp = sp; } + + int get_code() { + auto sp = wp.lock(); + if (!sp) { + return -999; + } + return sp->get_code(); + } + +private: + std::weak_ptr wp; }; } // namespace class_sh_trampoline_weak_ptr @@ -44,18 +40,12 @@ struct WpBaseTester { using namespace pybind11_tests::class_sh_trampoline_weak_ptr; TEST_SUBMODULE(class_sh_trampoline_weak_ptr, m) { - // For testing whether a python subclass of a C++ object can be accessed from a C++ weak_ptr - - py::classh(m, "WpBase") + py::classh(m, "VirtBase") .def(py::init<>()) - .def(py::init([](int) { return std::make_shared(); })) - .def("is_base_used", &WpBase::is_base_used) - .def("has_python_instance", &WpBase::has_python_instance); + .def("get_code", &VirtBase::get_code); - py::classh(m, "WpBaseTester") + py::classh(m, "WpOwner") .def(py::init<>()) - .def("get_object", &WpBaseTester::get_object) - .def("set_object", &WpBaseTester::set_object) - .def("is_expired", &WpBaseTester::is_expired) - .def("is_base_used", &WpBaseTester::is_base_used); + .def("set_wp", &WpOwner::set_wp) + .def("get_code", &WpOwner::get_code); } diff --git a/tests/test_class_sh_trampoline_weak_ptr.py b/tests/test_class_sh_trampoline_weak_ptr.py index 545c1ad2e1..0f24f01580 100644 --- a/tests/test_class_sh_trampoline_weak_ptr.py +++ b/tests/test_class_sh_trampoline_weak_ptr.py @@ -6,31 +6,22 @@ import pybind11_tests.class_sh_trampoline_weak_ptr as m -@pytest.mark.skipif("env.GRAALPY", reason="Cannot reliably trigger GC") -def test_weak_ptr_base(): - tester = m.WpBaseTester() - - obj = m.WpBase() - - tester.set_object(obj) - - assert tester.is_expired() is False - assert tester.is_base_used() is True - assert tester.get_object().is_base_used() is True +class PyDrvd(m.VirtBase): + def get_code(self): + return 200 @pytest.mark.skipif("env.GRAALPY", reason="Cannot reliably trigger GC") -def test_weak_ptr_child(): - class PyChild(m.WpBase): - def is_base_used(self): - return False - - tester = m.WpBaseTester() +@pytest.mark.parametrize(("vtype", "expected_code"), [(m.VirtBase, 100), (PyDrvd, 200)]) +def test_weak_ptr_base(vtype, expected_code): + wpo = m.WpOwner() + assert wpo.get_code() == -999 - obj = PyChild() + obj = vtype() + assert obj.get_code() == expected_code - tester.set_object(obj) + wpo.set_wp(obj) + assert wpo.get_code() == expected_code - assert tester.is_expired() is False - assert tester.is_base_used() is False - assert tester.get_object().is_base_used() is False + del obj + assert wpo.get_code() == -999 From ff7065723fe133f046529ab30d146a9c1779879e Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Sat, 19 Apr 2025 12:34:34 -0700 Subject: [PATCH 04/28] 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. --- tests/CMakeLists.txt | 1 + tests/test_class_sh_trampoline_weak_ptr.py | 5 +- tests/test_class_sp_trampoline_weak_ptr.cpp | 51 +++++++++++++++++++++ tests/test_class_sp_trampoline_weak_ptr.py | 27 +++++++++++ 4 files changed, 83 insertions(+), 1 deletion(-) create mode 100644 tests/test_class_sp_trampoline_weak_ptr.cpp create mode 100644 tests/test_class_sp_trampoline_weak_ptr.py diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 1b67c19eaa..267c242f03 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -134,6 +134,7 @@ set(PYBIND11_TEST_FILES test_class_sh_unique_ptr_custom_deleter test_class_sh_unique_ptr_member test_class_sh_virtual_py_cpp_mix + test_class_sp_trampoline_weak_ptr test_const_name test_constants_and_functions test_copy_move diff --git a/tests/test_class_sh_trampoline_weak_ptr.py b/tests/test_class_sh_trampoline_weak_ptr.py index 0f24f01580..741bb2804f 100644 --- a/tests/test_class_sh_trampoline_weak_ptr.py +++ b/tests/test_class_sh_trampoline_weak_ptr.py @@ -21,7 +21,10 @@ def test_weak_ptr_base(vtype, expected_code): assert obj.get_code() == expected_code wpo.set_wp(obj) - assert wpo.get_code() == expected_code + if vtype is m.VirtBase: + assert wpo.get_code() == expected_code + else: + assert wpo.get_code() == -999 # THIS NEEDS FIXING (issue #5623) del obj assert wpo.get_code() == -999 diff --git a/tests/test_class_sp_trampoline_weak_ptr.cpp b/tests/test_class_sp_trampoline_weak_ptr.cpp new file mode 100644 index 0000000000..9665983a05 --- /dev/null +++ b/tests/test_class_sp_trampoline_weak_ptr.cpp @@ -0,0 +1,51 @@ +// Copyright (c) 2025 The Pybind Development Team. +// 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 + +namespace pybind11_tests { +namespace class_sp_trampoline_weak_ptr { + +struct VirtBase { + virtual ~VirtBase() = default; + virtual int get_code() { return 100; } +}; + +struct PyVirtBase : VirtBase, py::trampoline_self_life_support { + using VirtBase::VirtBase; + int get_code() override { PYBIND11_OVERRIDE(int, VirtBase, get_code); } +}; + +struct WpOwner { + void set_wp(std::shared_ptr sp) { wp = sp; } + + int get_code() { + auto sp = wp.lock(); + if (!sp) { + return -999; + } + return sp->get_code(); + } + +private: + std::weak_ptr wp; +}; + +} // namespace class_sp_trampoline_weak_ptr +} // namespace pybind11_tests + +using namespace pybind11_tests::class_sp_trampoline_weak_ptr; + +TEST_SUBMODULE(class_sp_trampoline_weak_ptr, m) { + py::class_, PyVirtBase>(m, "VirtBase") + .def(py::init<>()) + .def("get_code", &VirtBase::get_code); + + py::class_(m, "WpOwner") + .def(py::init<>()) + .def("set_wp", &WpOwner::set_wp) + .def("get_code", &WpOwner::get_code); +} diff --git a/tests/test_class_sp_trampoline_weak_ptr.py b/tests/test_class_sp_trampoline_weak_ptr.py new file mode 100644 index 0000000000..2fc36bac12 --- /dev/null +++ b/tests/test_class_sp_trampoline_weak_ptr.py @@ -0,0 +1,27 @@ +from __future__ import annotations + +import pytest + +import env # noqa: F401 +import pybind11_tests.class_sp_trampoline_weak_ptr as m + + +class PyDrvd(m.VirtBase): + def get_code(self): + return 200 + + +@pytest.mark.skipif("env.GRAALPY", reason="Cannot reliably trigger GC") +@pytest.mark.parametrize(("vtype", "expected_code"), [(m.VirtBase, 100), (PyDrvd, 200)]) +def test_weak_ptr_base(vtype, expected_code): + wpo = m.WpOwner() + assert wpo.get_code() == -999 + + obj = vtype() + assert obj.get_code() == expected_code + + wpo.set_wp(obj) + assert wpo.get_code() == expected_code + + del obj + assert wpo.get_code() == -999 From a4816471e1eec6665d5507356f07528b1cf1f455 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Sat, 19 Apr 2025 13:38:54 -0700 Subject: [PATCH 05/28] 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 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 sp) { wp = sp; } | ^ | const & 4430 warnings generated. ``` --- tests/test_class_sh_trampoline_weak_ptr.cpp | 2 +- tests/test_class_sp_trampoline_weak_ptr.cpp | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/test_class_sh_trampoline_weak_ptr.cpp b/tests/test_class_sh_trampoline_weak_ptr.cpp index 9b7651cce9..e0036a08a6 100644 --- a/tests/test_class_sh_trampoline_weak_ptr.cpp +++ b/tests/test_class_sh_trampoline_weak_ptr.cpp @@ -20,7 +20,7 @@ struct PyVirtBase : VirtBase, py::trampoline_self_life_support { }; struct WpOwner { - void set_wp(std::shared_ptr sp) { wp = sp; } + void set_wp(const std::shared_ptr &sp) { wp = sp; } int get_code() { auto sp = wp.lock(); diff --git a/tests/test_class_sp_trampoline_weak_ptr.cpp b/tests/test_class_sp_trampoline_weak_ptr.cpp index 9665983a05..4821fd7113 100644 --- a/tests/test_class_sp_trampoline_weak_ptr.cpp +++ b/tests/test_class_sp_trampoline_weak_ptr.cpp @@ -20,7 +20,7 @@ struct PyVirtBase : VirtBase, py::trampoline_self_life_support { }; struct WpOwner { - void set_wp(std::shared_ptr sp) { wp = sp; } + void set_wp(const std::shared_ptr &sp) { wp = sp; } int get_code() { auto sp = wp.lock(); From 32858da8a6ec5fa746a8f62ed06cb47c818a4b77 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Sat, 19 Apr 2025 13:47:42 -0700 Subject: [PATCH 06/28] PYPY, GRAALPY: skip last part of test_weak_ptr_base --- tests/test_class_sh_trampoline_weak_ptr.py | 5 +++-- tests/test_class_sp_trampoline_weak_ptr.py | 5 +++-- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/tests/test_class_sh_trampoline_weak_ptr.py b/tests/test_class_sh_trampoline_weak_ptr.py index 741bb2804f..c83b7fcfd3 100644 --- a/tests/test_class_sh_trampoline_weak_ptr.py +++ b/tests/test_class_sh_trampoline_weak_ptr.py @@ -2,7 +2,7 @@ import pytest -import env # noqa: F401 +import env import pybind11_tests.class_sh_trampoline_weak_ptr as m @@ -11,7 +11,6 @@ def get_code(self): return 200 -@pytest.mark.skipif("env.GRAALPY", reason="Cannot reliably trigger GC") @pytest.mark.parametrize(("vtype", "expected_code"), [(m.VirtBase, 100), (PyDrvd, 200)]) def test_weak_ptr_base(vtype, expected_code): wpo = m.WpOwner() @@ -27,4 +26,6 @@ def test_weak_ptr_base(vtype, expected_code): assert wpo.get_code() == -999 # THIS NEEDS FIXING (issue #5623) del obj + if env.PYPY or env.GRAALPY: + pytest.skip("Cannot reliably trigger GC") assert wpo.get_code() == -999 diff --git a/tests/test_class_sp_trampoline_weak_ptr.py b/tests/test_class_sp_trampoline_weak_ptr.py index 2fc36bac12..43db1b7d77 100644 --- a/tests/test_class_sp_trampoline_weak_ptr.py +++ b/tests/test_class_sp_trampoline_weak_ptr.py @@ -2,7 +2,7 @@ import pytest -import env # noqa: F401 +import env import pybind11_tests.class_sp_trampoline_weak_ptr as m @@ -11,7 +11,6 @@ def get_code(self): return 200 -@pytest.mark.skipif("env.GRAALPY", reason="Cannot reliably trigger GC") @pytest.mark.parametrize(("vtype", "expected_code"), [(m.VirtBase, 100), (PyDrvd, 200)]) def test_weak_ptr_base(vtype, expected_code): wpo = m.WpOwner() @@ -24,4 +23,6 @@ def test_weak_ptr_base(vtype, expected_code): assert wpo.get_code() == expected_code del obj + if env.PYPY or env.GRAALPY: + pytest.skip("Cannot reliably trigger GC") assert wpo.get_code() == -999 From e978d93cd180c8aedc1f44c2df1ca804d4f84de1 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Sat, 19 Apr 2025 13:50:00 -0700 Subject: [PATCH 07/28] =?UTF-8?q?Rename=20test=5Fweak=5Fptr=5Fbase=20?= =?UTF-8?q?=E2=86=92=20test=5Fweak=5Fptr=5Fowner?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- tests/test_class_sh_trampoline_weak_ptr.py | 2 +- tests/test_class_sp_trampoline_weak_ptr.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/test_class_sh_trampoline_weak_ptr.py b/tests/test_class_sh_trampoline_weak_ptr.py index c83b7fcfd3..a60a2e7b47 100644 --- a/tests/test_class_sh_trampoline_weak_ptr.py +++ b/tests/test_class_sh_trampoline_weak_ptr.py @@ -12,7 +12,7 @@ def get_code(self): @pytest.mark.parametrize(("vtype", "expected_code"), [(m.VirtBase, 100), (PyDrvd, 200)]) -def test_weak_ptr_base(vtype, expected_code): +def test_weak_ptr_owner(vtype, expected_code): wpo = m.WpOwner() assert wpo.get_code() == -999 diff --git a/tests/test_class_sp_trampoline_weak_ptr.py b/tests/test_class_sp_trampoline_weak_ptr.py index 43db1b7d77..3782cd8aaa 100644 --- a/tests/test_class_sp_trampoline_weak_ptr.py +++ b/tests/test_class_sp_trampoline_weak_ptr.py @@ -12,7 +12,7 @@ def get_code(self): @pytest.mark.parametrize(("vtype", "expected_code"), [(m.VirtBase, 100), (PyDrvd, 200)]) -def test_weak_ptr_base(vtype, expected_code): +def test_weak_ptr_owner(vtype, expected_code): wpo = m.WpOwner() assert wpo.get_code() == -999 From 1c0b700fdc9266ec024111337ab89eed64ce699d Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Sun, 20 Apr 2025 10:54:36 -0700 Subject: [PATCH 08/28] Add SpOwner, test_with_sp_owner, test_with_sp_and_wp_owners --- tests/test_class_sp_trampoline_weak_ptr.cpp | 19 +++++++++ tests/test_class_sp_trampoline_weak_ptr.py | 43 ++++++++++++++++++++- 2 files changed, 61 insertions(+), 1 deletion(-) diff --git a/tests/test_class_sp_trampoline_weak_ptr.cpp b/tests/test_class_sp_trampoline_weak_ptr.cpp index 4821fd7113..06250b3036 100644 --- a/tests/test_class_sp_trampoline_weak_ptr.cpp +++ b/tests/test_class_sp_trampoline_weak_ptr.cpp @@ -34,6 +34,20 @@ struct WpOwner { std::weak_ptr wp; }; +struct SpOwner { + void set_sp(const std::shared_ptr &sp) { this->sp = sp; } + + int get_code() { + if (!sp) { + return -888; + } + return sp->get_code(); + } + +private: + std::shared_ptr sp; +}; + } // namespace class_sp_trampoline_weak_ptr } // namespace pybind11_tests @@ -48,4 +62,9 @@ TEST_SUBMODULE(class_sp_trampoline_weak_ptr, m) { .def(py::init<>()) .def("set_wp", &WpOwner::set_wp) .def("get_code", &WpOwner::get_code); + + py::class_(m, "SpOwner") + .def(py::init<>()) + .def("set_sp", &SpOwner::set_sp) + .def("get_code", &SpOwner::get_code); } diff --git a/tests/test_class_sp_trampoline_weak_ptr.py b/tests/test_class_sp_trampoline_weak_ptr.py index 3782cd8aaa..5c427f9804 100644 --- a/tests/test_class_sp_trampoline_weak_ptr.py +++ b/tests/test_class_sp_trampoline_weak_ptr.py @@ -12,7 +12,7 @@ def get_code(self): @pytest.mark.parametrize(("vtype", "expected_code"), [(m.VirtBase, 100), (PyDrvd, 200)]) -def test_weak_ptr_owner(vtype, expected_code): +def test_with_wp_owner(vtype, expected_code): wpo = m.WpOwner() assert wpo.get_code() == -999 @@ -26,3 +26,44 @@ def test_weak_ptr_owner(vtype, expected_code): if env.PYPY or env.GRAALPY: pytest.skip("Cannot reliably trigger GC") assert wpo.get_code() == -999 + + +@pytest.mark.parametrize(("vtype", "expected_code"), [(m.VirtBase, 100), (PyDrvd, 200)]) +def test_with_sp_owner(vtype, expected_code): + spo = m.SpOwner() + assert spo.get_code() == -888 + + obj = vtype() + assert obj.get_code() == expected_code + + spo.set_sp(obj) + assert spo.get_code() == expected_code + + del obj + if env.PYPY or env.GRAALPY: + pytest.skip("Cannot reliably trigger GC") + assert spo.get_code() == 100 # Inheritance slicing (issue #1333) + + +@pytest.mark.parametrize(("vtype", "expected_code"), [(m.VirtBase, 100), (PyDrvd, 200)]) +def test_with_sp_and_wp_owners(vtype, expected_code): + spo = m.SpOwner() + wpo = m.WpOwner() + + obj = vtype() + spo.set_sp(obj) + wpo.set_wp(obj) + + assert spo.get_code() == expected_code + assert wpo.get_code() == expected_code + + del obj + if env.PYPY or env.GRAALPY: + pytest.skip("Cannot reliably trigger GC") + + # Inheritance slicing (issue #1333) + assert spo.get_code() == 100 + assert wpo.get_code() == 100 + + del spo + assert wpo.get_code() == -999 From 4638e017b6e7bbd246c034cb1f828d912fc7b407 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Sun, 20 Apr 2025 14:14:47 -0700 Subject: [PATCH 09/28] 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 d4f9a41e..f3d45301 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(released_ptr, type_raw_ptr); } + auto *self_life_support + = dynamic_raw_ptr_cast_if_possible(type_raw_ptr); + if (self_life_support == nullptr) { + std::shared_ptr void_shd_ptr = hld.template as_shared_ptr(); + std::shared_ptr to_be_released(void_shd_ptr, type_raw_ptr); + vptr_gd_ptr->released_ptr = to_be_released; + return to_be_released; + } std::shared_ptr to_be_released( type_raw_ptr, shared_ptr_trampoline_self_life_support(loaded_v_h.inst)); vptr_gd_ptr->released_ptr = to_be_released; ``` --- include/pybind11/detail/type_caster_base.h | 8 ++++++++ include/pybind11/pybind11.h | 9 +++++++++ ...class_sh_trampoline_shared_ptr_cpp_arg.cpp | 2 +- tests/test_class_sh_trampoline_weak_ptr.cpp | 14 ++++++++++++- tests/test_class_sh_trampoline_weak_ptr.py | 20 +++++++++++++++---- tests/test_class_sp_trampoline_weak_ptr.cpp | 12 +++++++++++ tests/test_class_sp_trampoline_weak_ptr.py | 17 ++++++++++++++++ 7 files changed, 76 insertions(+), 6 deletions(-) diff --git a/include/pybind11/detail/type_caster_base.h b/include/pybind11/detail/type_caster_base.h index d4f9a41e0c..f3d4530146 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(released_ptr, type_raw_ptr); } + auto *self_life_support + = dynamic_raw_ptr_cast_if_possible(type_raw_ptr); + if (self_life_support == nullptr) { + std::shared_ptr void_shd_ptr = hld.template as_shared_ptr(); + std::shared_ptr to_be_released(void_shd_ptr, type_raw_ptr); + vptr_gd_ptr->released_ptr = to_be_released; + return to_be_released; + } std::shared_ptr to_be_released( type_raw_ptr, shared_ptr_trampoline_self_life_support(loaded_v_h.inst)); vptr_gd_ptr->released_ptr = to_be_released; diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index c84b24d3b7..338a9af375 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -3416,14 +3416,23 @@ PYBIND11_NAMESPACE_END(detail) template function get_override(const T *this_ptr, const char *name) { auto *tinfo = detail::get_type_info(typeid(T)); + fflush(stderr); + printf("\nLOOOK get_override tinfo truthy = %s\n", tinfo ? "YES" : "NO"); + fflush(stdout); return tinfo ? detail::get_type_override(this_ptr, tinfo, name) : function(); } #define PYBIND11_OVERRIDE_IMPL(ret_type, cname, name, ...) \ do { \ pybind11::gil_scoped_acquire gil; \ + fflush(stderr); \ + printf("\nLOOOK BEFORE static_cast(this)\n"); \ + fflush(stdout); \ pybind11::function override \ = pybind11::get_override(static_cast(this), name); \ + fflush(stderr); \ + printf("\nLOOOK AFTER static_cast(this)\n"); \ + fflush(stdout); \ if (override) { \ auto o = override(__VA_ARGS__); \ PYBIND11_WARNING_PUSH \ diff --git a/tests/test_class_sh_trampoline_shared_ptr_cpp_arg.cpp b/tests/test_class_sh_trampoline_shared_ptr_cpp_arg.cpp index 49e1ac885d..5580848c6e 100644 --- a/tests/test_class_sh_trampoline_shared_ptr_cpp_arg.cpp +++ b/tests/test_class_sh_trampoline_shared_ptr_cpp_arg.cpp @@ -28,7 +28,7 @@ struct SpBase { std::shared_ptr pass_through_shd_ptr(const std::shared_ptr &obj) { return obj; } -struct PySpBase : SpBase { +struct PySpBase : SpBase, py::trampoline_self_life_support { using SpBase::SpBase; bool is_base_used() override { PYBIND11_OVERRIDE(bool, SpBase, is_base_used); } }; diff --git a/tests/test_class_sh_trampoline_weak_ptr.cpp b/tests/test_class_sh_trampoline_weak_ptr.cpp index e0036a08a6..a0cd19d446 100644 --- a/tests/test_class_sh_trampoline_weak_ptr.cpp +++ b/tests/test_class_sh_trampoline_weak_ptr.cpp @@ -14,9 +14,15 @@ struct VirtBase { virtual int get_code() { return 100; } }; -struct PyVirtBase : VirtBase, py::trampoline_self_life_support { +struct PyVirtBase : VirtBase /*, py::trampoline_self_life_support */ { using VirtBase::VirtBase; int get_code() override { PYBIND11_OVERRIDE(int, VirtBase, get_code); } + + ~PyVirtBase() override { + fflush(stderr); + printf("\nLOOOK ~PyVirtBase()\n"); + fflush(stdout); + } }; struct WpOwner { @@ -34,6 +40,10 @@ struct WpOwner { std::weak_ptr wp; }; +std::shared_ptr pass_through_sp_VirtBase(const std::shared_ptr &sp) { + return sp; +} + } // namespace class_sh_trampoline_weak_ptr } // namespace pybind11_tests @@ -48,4 +58,6 @@ TEST_SUBMODULE(class_sh_trampoline_weak_ptr, m) { .def(py::init<>()) .def("set_wp", &WpOwner::set_wp) .def("get_code", &WpOwner::get_code); + + m.def("pass_through_sp_VirtBase", pass_through_sp_VirtBase); } diff --git a/tests/test_class_sh_trampoline_weak_ptr.py b/tests/test_class_sh_trampoline_weak_ptr.py index a60a2e7b47..784cd0178f 100644 --- a/tests/test_class_sh_trampoline_weak_ptr.py +++ b/tests/test_class_sh_trampoline_weak_ptr.py @@ -1,5 +1,7 @@ from __future__ import annotations +import gc + import pytest import env @@ -20,12 +22,22 @@ def test_weak_ptr_owner(vtype, expected_code): assert obj.get_code() == expected_code wpo.set_wp(obj) - if vtype is m.VirtBase: - assert wpo.get_code() == expected_code - else: - assert wpo.get_code() == -999 # THIS NEEDS FIXING (issue #5623) + assert wpo.get_code() == expected_code del obj if env.PYPY or env.GRAALPY: pytest.skip("Cannot reliably trigger GC") assert wpo.get_code() == -999 + + +@pytest.mark.parametrize(("vtype", "expected_code"), [(m.VirtBase, 100), (PyDrvd, 200)]) +def test_pass_through_sp_VirtBase(vtype, expected_code): + obj = vtype() + ptr = m.pass_through_sp_VirtBase(obj) + print("\nLOOOK BEFORE del obj", flush=True) + del obj + print("\nLOOOK AFTER del obj", flush=True) + gc.collect() + print("\nLOOOK AFTER gc.collect()", flush=True) + assert ptr.get_code() == expected_code + print("\nLOOOK AFTER ptr.get_code()", flush=True) diff --git a/tests/test_class_sp_trampoline_weak_ptr.cpp b/tests/test_class_sp_trampoline_weak_ptr.cpp index 06250b3036..a9295fcf2a 100644 --- a/tests/test_class_sp_trampoline_weak_ptr.cpp +++ b/tests/test_class_sp_trampoline_weak_ptr.cpp @@ -17,6 +17,12 @@ struct VirtBase { struct PyVirtBase : VirtBase, py::trampoline_self_life_support { using VirtBase::VirtBase; int get_code() override { PYBIND11_OVERRIDE(int, VirtBase, get_code); } + + ~PyVirtBase() override { + fflush(stderr); + printf("\nLOOOK ~PyVirtBase()\n"); + fflush(stdout); + } }; struct WpOwner { @@ -48,6 +54,10 @@ struct SpOwner { std::shared_ptr sp; }; +std::shared_ptr pass_through_sp_VirtBase(const std::shared_ptr &sp) { + return sp; +} + } // namespace class_sp_trampoline_weak_ptr } // namespace pybind11_tests @@ -67,4 +77,6 @@ TEST_SUBMODULE(class_sp_trampoline_weak_ptr, m) { .def(py::init<>()) .def("set_sp", &SpOwner::set_sp) .def("get_code", &SpOwner::get_code); + + m.def("pass_through_sp_VirtBase", pass_through_sp_VirtBase); } diff --git a/tests/test_class_sp_trampoline_weak_ptr.py b/tests/test_class_sp_trampoline_weak_ptr.py index 5c427f9804..f41eedccb4 100644 --- a/tests/test_class_sp_trampoline_weak_ptr.py +++ b/tests/test_class_sp_trampoline_weak_ptr.py @@ -1,5 +1,7 @@ from __future__ import annotations +import gc + import pytest import env @@ -42,7 +44,9 @@ def test_with_sp_owner(vtype, expected_code): del obj if env.PYPY or env.GRAALPY: pytest.skip("Cannot reliably trigger GC") + print("\nLOOOK BEFORE spo.get_code() AFTER del obj", flush=True) assert spo.get_code() == 100 # Inheritance slicing (issue #1333) + print("\nLOOOK AFTER spo.get_code() AFTER del obj", flush=True) @pytest.mark.parametrize(("vtype", "expected_code"), [(m.VirtBase, 100), (PyDrvd, 200)]) @@ -67,3 +71,16 @@ def test_with_sp_and_wp_owners(vtype, expected_code): del spo assert wpo.get_code() == -999 + + +@pytest.mark.parametrize(("vtype", "expected_code"), [(m.VirtBase, 100), (PyDrvd, 200)]) +def test_pass_through_sp_VirtBase(vtype, expected_code): + obj = vtype() + ptr = m.pass_through_sp_VirtBase(obj) + print("\nLOOOK BEFORE del obj", flush=True) + del obj + print("\nLOOOK AFTER del obj", flush=True) + gc.collect() + print("\nLOOOK AFTER gc.collect()", flush=True) + assert ptr.get_code() == expected_code + print("\nLOOOK AFTER ptr.get_code()", flush=True) From 6cc6368cda75aa26feafff5e7f672a066c0d749a Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Sun, 20 Apr 2025 14:22:47 -0700 Subject: [PATCH 10/28] Remove debug printf in include/pybind11/pybind11.h --- include/pybind11/pybind11.h | 9 --------- 1 file changed, 9 deletions(-) diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index 338a9af375..c84b24d3b7 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -3416,23 +3416,14 @@ PYBIND11_NAMESPACE_END(detail) template function get_override(const T *this_ptr, const char *name) { auto *tinfo = detail::get_type_info(typeid(T)); - fflush(stderr); - printf("\nLOOOK get_override tinfo truthy = %s\n", tinfo ? "YES" : "NO"); - fflush(stdout); return tinfo ? detail::get_type_override(this_ptr, tinfo, name) : function(); } #define PYBIND11_OVERRIDE_IMPL(ret_type, cname, name, ...) \ do { \ pybind11::gil_scoped_acquire gil; \ - fflush(stderr); \ - printf("\nLOOOK BEFORE static_cast(this)\n"); \ - fflush(stdout); \ pybind11::function override \ = pybind11::get_override(static_cast(this), name); \ - fflush(stderr); \ - printf("\nLOOOK AFTER static_cast(this)\n"); \ - fflush(stdout); \ if (override) { \ auto o = override(__VA_ARGS__); \ PYBIND11_WARNING_PUSH \ From 62d32657eac4b5728e0b432433a1ae972d9536ee Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Sun, 20 Apr 2025 14:31:51 -0700 Subject: [PATCH 11/28] 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' ``` --- tests/test_class_sp_trampoline_weak_ptr.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_class_sp_trampoline_weak_ptr.cpp b/tests/test_class_sp_trampoline_weak_ptr.cpp index a9295fcf2a..db9c447fee 100644 --- a/tests/test_class_sp_trampoline_weak_ptr.cpp +++ b/tests/test_class_sp_trampoline_weak_ptr.cpp @@ -41,7 +41,7 @@ struct WpOwner { }; struct SpOwner { - void set_sp(const std::shared_ptr &sp) { this->sp = sp; } + void set_sp(const std::shared_ptr &sp_) { sp = sp_; } int get_code() { if (!sp) { From 2f672ebb2a89efe9caa6f71d45add2753fc5fbc7 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Sat, 26 Apr 2025 12:08:05 -0700 Subject: [PATCH 12/28] [skip ci] Undo the production code change under 4638e017b6e7bbd246c034cb1f828d912fc7b407 Also undo the corresponding test change in test_class_sh_trampoline_weak_ptr.py But keep all extra debugging code for now. --- include/pybind11/detail/type_caster_base.h | 8 -------- tests/test_class_sh_trampoline_shared_ptr_cpp_arg.cpp | 2 +- tests/test_class_sh_trampoline_weak_ptr.py | 5 ++++- 3 files changed, 5 insertions(+), 10 deletions(-) diff --git a/include/pybind11/detail/type_caster_base.h b/include/pybind11/detail/type_caster_base.h index f3d4530146..d4f9a41e0c 100644 --- a/include/pybind11/detail/type_caster_base.h +++ b/include/pybind11/detail/type_caster_base.h @@ -776,14 +776,6 @@ struct load_helper : value_and_holder_helper { if (released_ptr) { return std::shared_ptr(released_ptr, type_raw_ptr); } - auto *self_life_support - = dynamic_raw_ptr_cast_if_possible(type_raw_ptr); - if (self_life_support == nullptr) { - std::shared_ptr void_shd_ptr = hld.template as_shared_ptr(); - std::shared_ptr to_be_released(void_shd_ptr, type_raw_ptr); - vptr_gd_ptr->released_ptr = to_be_released; - return to_be_released; - } std::shared_ptr to_be_released( type_raw_ptr, shared_ptr_trampoline_self_life_support(loaded_v_h.inst)); vptr_gd_ptr->released_ptr = to_be_released; diff --git a/tests/test_class_sh_trampoline_shared_ptr_cpp_arg.cpp b/tests/test_class_sh_trampoline_shared_ptr_cpp_arg.cpp index 5580848c6e..49e1ac885d 100644 --- a/tests/test_class_sh_trampoline_shared_ptr_cpp_arg.cpp +++ b/tests/test_class_sh_trampoline_shared_ptr_cpp_arg.cpp @@ -28,7 +28,7 @@ struct SpBase { std::shared_ptr pass_through_shd_ptr(const std::shared_ptr &obj) { return obj; } -struct PySpBase : SpBase, py::trampoline_self_life_support { +struct PySpBase : SpBase { using SpBase::SpBase; bool is_base_used() override { PYBIND11_OVERRIDE(bool, SpBase, is_base_used); } }; diff --git a/tests/test_class_sh_trampoline_weak_ptr.py b/tests/test_class_sh_trampoline_weak_ptr.py index 784cd0178f..cfefca4b94 100644 --- a/tests/test_class_sh_trampoline_weak_ptr.py +++ b/tests/test_class_sh_trampoline_weak_ptr.py @@ -22,7 +22,10 @@ def test_weak_ptr_owner(vtype, expected_code): assert obj.get_code() == expected_code wpo.set_wp(obj) - assert wpo.get_code() == expected_code + if vtype is m.VirtBase: + assert wpo.get_code() == expected_code + else: + assert wpo.get_code() == -999 # THIS NEEDS FIXING (issue #5623) del obj if env.PYPY or env.GRAALPY: From 888a29573215522491eeb70ba0f125488931b24d Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Sat, 26 Apr 2025 12:18:14 -0700 Subject: [PATCH 13/28] [skip ci] Introduce lambda in `WpOwner::set_wp` bindings, but simply cast to `std::shared_ptr` for now. --- tests/test_class_sh_trampoline_weak_ptr.cpp | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/tests/test_class_sh_trampoline_weak_ptr.cpp b/tests/test_class_sh_trampoline_weak_ptr.cpp index a0cd19d446..1410b130b8 100644 --- a/tests/test_class_sh_trampoline_weak_ptr.cpp +++ b/tests/test_class_sh_trampoline_weak_ptr.cpp @@ -56,7 +56,10 @@ TEST_SUBMODULE(class_sh_trampoline_weak_ptr, m) { py::classh(m, "WpOwner") .def(py::init<>()) - .def("set_wp", &WpOwner::set_wp) + .def("set_wp", + [](WpOwner &self, py::handle obj) { + self.set_wp(obj.cast>()); + }) .def("get_code", &WpOwner::get_code); m.def("pass_through_sp_VirtBase", pass_through_sp_VirtBase); From b20f557a7311074d92a2ea5b923ef53c04672026 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Sat, 26 Apr 2025 12:22:40 -0700 Subject: [PATCH 14/28] Add `py::potentially_slicing_shared_ptr()` --- include/pybind11/cast.h | 25 +++++++++++++++++++++ include/pybind11/detail/type_caster_base.h | 6 +++-- tests/test_class_sh_trampoline_weak_ptr.cpp | 2 +- tests/test_class_sh_trampoline_weak_ptr.py | 5 +---- 4 files changed, 31 insertions(+), 7 deletions(-) diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h index 8952cf0943..7cfbb7cc3d 100644 --- a/include/pybind11/cast.h +++ b/include/pybind11/cast.h @@ -983,6 +983,16 @@ struct copyable_holder_caster< return shared_ptr_storage; } + std::shared_ptr &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 &src, return_value_policy policy, handle parent) { const auto *ptr = src.get(); @@ -1077,6 +1087,21 @@ struct copyable_holder_caster< template class type_caster> : public copyable_holder_caster> {}; +PYBIND11_NAMESPACE_END(detail) + +template +std::shared_ptr potentially_slicing_shared_ptr(handle obj) { + detail::make_caster> caster; + if (caster.load(obj, /*convert=*/true)) { + return caster.potentially_slicing_shared_ptr(); + } + const char *type_name_obj = detail::obj_class_name(obj.ptr()); + throw type_error("\"" + std::string(type_name_obj) + + "\" object is not convertible to std::shared_ptr"); +} + +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 diff --git a/include/pybind11/detail/type_caster_base.h b/include/pybind11/detail/type_caster_base.h index d4f9a41e0c..564354c414 100644 --- a/include/pybind11/detail/type_caster_base.h +++ b/include/pybind11/detail/type_caster_base.h @@ -754,7 +754,9 @@ struct load_helper : value_and_holder_helper { } std::shared_ptr 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; } @@ -769,7 +771,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(void_raw_ptr); - if (python_instance_is_alias) { + if (python_instance_is_alias && !force_potentially_slicing_shared_ptr) { auto *vptr_gd_ptr = std::get_deleter(hld.vptr); if (vptr_gd_ptr != nullptr) { std::shared_ptr released_ptr = vptr_gd_ptr->released_ptr.lock(); diff --git a/tests/test_class_sh_trampoline_weak_ptr.cpp b/tests/test_class_sh_trampoline_weak_ptr.cpp index 1410b130b8..ea2f75c67e 100644 --- a/tests/test_class_sh_trampoline_weak_ptr.cpp +++ b/tests/test_class_sh_trampoline_weak_ptr.cpp @@ -58,7 +58,7 @@ TEST_SUBMODULE(class_sh_trampoline_weak_ptr, m) { .def(py::init<>()) .def("set_wp", [](WpOwner &self, py::handle obj) { - self.set_wp(obj.cast>()); + self.set_wp(py::potentially_slicing_shared_ptr(obj)); }) .def("get_code", &WpOwner::get_code); diff --git a/tests/test_class_sh_trampoline_weak_ptr.py b/tests/test_class_sh_trampoline_weak_ptr.py index cfefca4b94..784cd0178f 100644 --- a/tests/test_class_sh_trampoline_weak_ptr.py +++ b/tests/test_class_sh_trampoline_weak_ptr.py @@ -22,10 +22,7 @@ def test_weak_ptr_owner(vtype, expected_code): assert obj.get_code() == expected_code wpo.set_wp(obj) - if vtype is m.VirtBase: - assert wpo.get_code() == expected_code - else: - assert wpo.get_code() == -999 # THIS NEEDS FIXING (issue #5623) + assert wpo.get_code() == expected_code del obj if env.PYPY or env.GRAALPY: From 3b4b28cd46c7406e61d78ef6ec5fe51f4cd66d50 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Sat, 26 Apr 2025 21:13:03 -0700 Subject: [PATCH 15/28] Add `type_id()` to `py::potentially_slicing_shared_ptr()` error message and add test. --- include/pybind11/cast.h | 7 ++++--- tests/test_class_sh_trampoline_weak_ptr.py | 10 ++++++++++ 2 files changed, 14 insertions(+), 3 deletions(-) diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h index 7cfbb7cc3d..700ada6d4b 100644 --- a/include/pybind11/cast.h +++ b/include/pybind11/cast.h @@ -1095,9 +1095,10 @@ std::shared_ptr potentially_slicing_shared_ptr(handle obj) { if (caster.load(obj, /*convert=*/true)) { return caster.potentially_slicing_shared_ptr(); } - const char *type_name_obj = detail::obj_class_name(obj.ptr()); - throw type_error("\"" + std::string(type_name_obj) - + "\" object is not convertible to std::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 (with T = " + + type_id() + ")"); } PYBIND11_NAMESPACE_BEGIN(detail) diff --git a/tests/test_class_sh_trampoline_weak_ptr.py b/tests/test_class_sh_trampoline_weak_ptr.py index 784cd0178f..466aa95dd6 100644 --- a/tests/test_class_sh_trampoline_weak_ptr.py +++ b/tests/test_class_sh_trampoline_weak_ptr.py @@ -41,3 +41,13 @@ def test_pass_through_sp_VirtBase(vtype, expected_code): print("\nLOOOK AFTER gc.collect()", flush=True) assert ptr.get_code() == expected_code print("\nLOOOK AFTER ptr.get_code()", flush=True) + + +def test_potentially_slicing_shared_ptr_not_convertible_error(): + wpo = m.WpOwner() + with pytest.raises(Exception) as excinfo: + wpo.set_wp("") + assert str(excinfo.value) == ( + '"str" object is not convertible to std::shared_ptr' + " (with T = pybind11_tests::class_sh_trampoline_weak_ptr::VirtBase)" + ) From 56d23dc47816a2a9f54c1da818c7d45e1034e421 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Sat, 26 Apr 2025 23:58:29 -0700 Subject: [PATCH 16/28] test_potentially_slicing_shared_ptr.cpp,py (for smart_holder only) --- tests/CMakeLists.txt | 3 +- tests/test_class_sh_trampoline_weak_ptr.cpp | 66 -------------- tests/test_class_sh_trampoline_weak_ptr.py | 53 ------------ tests/test_class_sp_trampoline_weak_ptr.py | 86 ------------------- ...> test_potentially_slicing_shared_ptr.cpp} | 63 +++++++------- tests/test_potentially_slicing_shared_ptr.py | 69 +++++++++++++++ 6 files changed, 103 insertions(+), 237 deletions(-) delete mode 100644 tests/test_class_sh_trampoline_weak_ptr.cpp delete mode 100644 tests/test_class_sh_trampoline_weak_ptr.py delete mode 100644 tests/test_class_sp_trampoline_weak_ptr.py rename tests/{test_class_sp_trampoline_weak_ptr.cpp => test_potentially_slicing_shared_ptr.cpp} (59%) create mode 100644 tests/test_potentially_slicing_shared_ptr.py diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 267c242f03..e0a12cae7e 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -130,11 +130,9 @@ set(PYBIND11_TEST_FILES test_class_sh_trampoline_shared_from_this test_class_sh_trampoline_shared_ptr_cpp_arg test_class_sh_trampoline_unique_ptr - test_class_sh_trampoline_weak_ptr test_class_sh_unique_ptr_custom_deleter test_class_sh_unique_ptr_member test_class_sh_virtual_py_cpp_mix - test_class_sp_trampoline_weak_ptr test_const_name test_constants_and_functions test_copy_move @@ -163,6 +161,7 @@ set(PYBIND11_TEST_FILES test_opaque_types test_operator_overloading test_pickling + test_potentially_slicing_shared_ptr test_python_multiple_inheritance test_pytypes test_sequences_and_iterators diff --git a/tests/test_class_sh_trampoline_weak_ptr.cpp b/tests/test_class_sh_trampoline_weak_ptr.cpp deleted file mode 100644 index ea2f75c67e..0000000000 --- a/tests/test_class_sh_trampoline_weak_ptr.cpp +++ /dev/null @@ -1,66 +0,0 @@ -// Copyright (c) 2025 The Pybind Development Team. -// 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 - -namespace pybind11_tests { -namespace class_sh_trampoline_weak_ptr { - -struct VirtBase { - virtual ~VirtBase() = default; - virtual int get_code() { return 100; } -}; - -struct PyVirtBase : VirtBase /*, py::trampoline_self_life_support */ { - using VirtBase::VirtBase; - int get_code() override { PYBIND11_OVERRIDE(int, VirtBase, get_code); } - - ~PyVirtBase() override { - fflush(stderr); - printf("\nLOOOK ~PyVirtBase()\n"); - fflush(stdout); - } -}; - -struct WpOwner { - void set_wp(const std::shared_ptr &sp) { wp = sp; } - - int get_code() { - auto sp = wp.lock(); - if (!sp) { - return -999; - } - return sp->get_code(); - } - -private: - std::weak_ptr wp; -}; - -std::shared_ptr pass_through_sp_VirtBase(const std::shared_ptr &sp) { - return sp; -} - -} // namespace class_sh_trampoline_weak_ptr -} // namespace pybind11_tests - -using namespace pybind11_tests::class_sh_trampoline_weak_ptr; - -TEST_SUBMODULE(class_sh_trampoline_weak_ptr, m) { - py::classh(m, "VirtBase") - .def(py::init<>()) - .def("get_code", &VirtBase::get_code); - - py::classh(m, "WpOwner") - .def(py::init<>()) - .def("set_wp", - [](WpOwner &self, py::handle obj) { - self.set_wp(py::potentially_slicing_shared_ptr(obj)); - }) - .def("get_code", &WpOwner::get_code); - - m.def("pass_through_sp_VirtBase", pass_through_sp_VirtBase); -} diff --git a/tests/test_class_sh_trampoline_weak_ptr.py b/tests/test_class_sh_trampoline_weak_ptr.py deleted file mode 100644 index 466aa95dd6..0000000000 --- a/tests/test_class_sh_trampoline_weak_ptr.py +++ /dev/null @@ -1,53 +0,0 @@ -from __future__ import annotations - -import gc - -import pytest - -import env -import pybind11_tests.class_sh_trampoline_weak_ptr as m - - -class PyDrvd(m.VirtBase): - def get_code(self): - return 200 - - -@pytest.mark.parametrize(("vtype", "expected_code"), [(m.VirtBase, 100), (PyDrvd, 200)]) -def test_weak_ptr_owner(vtype, expected_code): - wpo = m.WpOwner() - assert wpo.get_code() == -999 - - obj = vtype() - assert obj.get_code() == expected_code - - wpo.set_wp(obj) - assert wpo.get_code() == expected_code - - del obj - if env.PYPY or env.GRAALPY: - pytest.skip("Cannot reliably trigger GC") - assert wpo.get_code() == -999 - - -@pytest.mark.parametrize(("vtype", "expected_code"), [(m.VirtBase, 100), (PyDrvd, 200)]) -def test_pass_through_sp_VirtBase(vtype, expected_code): - obj = vtype() - ptr = m.pass_through_sp_VirtBase(obj) - print("\nLOOOK BEFORE del obj", flush=True) - del obj - print("\nLOOOK AFTER del obj", flush=True) - gc.collect() - print("\nLOOOK AFTER gc.collect()", flush=True) - assert ptr.get_code() == expected_code - print("\nLOOOK AFTER ptr.get_code()", flush=True) - - -def test_potentially_slicing_shared_ptr_not_convertible_error(): - wpo = m.WpOwner() - with pytest.raises(Exception) as excinfo: - wpo.set_wp("") - assert str(excinfo.value) == ( - '"str" object is not convertible to std::shared_ptr' - " (with T = pybind11_tests::class_sh_trampoline_weak_ptr::VirtBase)" - ) diff --git a/tests/test_class_sp_trampoline_weak_ptr.py b/tests/test_class_sp_trampoline_weak_ptr.py deleted file mode 100644 index f41eedccb4..0000000000 --- a/tests/test_class_sp_trampoline_weak_ptr.py +++ /dev/null @@ -1,86 +0,0 @@ -from __future__ import annotations - -import gc - -import pytest - -import env -import pybind11_tests.class_sp_trampoline_weak_ptr as m - - -class PyDrvd(m.VirtBase): - def get_code(self): - return 200 - - -@pytest.mark.parametrize(("vtype", "expected_code"), [(m.VirtBase, 100), (PyDrvd, 200)]) -def test_with_wp_owner(vtype, expected_code): - wpo = m.WpOwner() - assert wpo.get_code() == -999 - - obj = vtype() - assert obj.get_code() == expected_code - - wpo.set_wp(obj) - assert wpo.get_code() == expected_code - - del obj - if env.PYPY or env.GRAALPY: - pytest.skip("Cannot reliably trigger GC") - assert wpo.get_code() == -999 - - -@pytest.mark.parametrize(("vtype", "expected_code"), [(m.VirtBase, 100), (PyDrvd, 200)]) -def test_with_sp_owner(vtype, expected_code): - spo = m.SpOwner() - assert spo.get_code() == -888 - - obj = vtype() - assert obj.get_code() == expected_code - - spo.set_sp(obj) - assert spo.get_code() == expected_code - - del obj - if env.PYPY or env.GRAALPY: - pytest.skip("Cannot reliably trigger GC") - print("\nLOOOK BEFORE spo.get_code() AFTER del obj", flush=True) - assert spo.get_code() == 100 # Inheritance slicing (issue #1333) - print("\nLOOOK AFTER spo.get_code() AFTER del obj", flush=True) - - -@pytest.mark.parametrize(("vtype", "expected_code"), [(m.VirtBase, 100), (PyDrvd, 200)]) -def test_with_sp_and_wp_owners(vtype, expected_code): - spo = m.SpOwner() - wpo = m.WpOwner() - - obj = vtype() - spo.set_sp(obj) - wpo.set_wp(obj) - - assert spo.get_code() == expected_code - assert wpo.get_code() == expected_code - - del obj - if env.PYPY or env.GRAALPY: - pytest.skip("Cannot reliably trigger GC") - - # Inheritance slicing (issue #1333) - assert spo.get_code() == 100 - assert wpo.get_code() == 100 - - del spo - assert wpo.get_code() == -999 - - -@pytest.mark.parametrize(("vtype", "expected_code"), [(m.VirtBase, 100), (PyDrvd, 200)]) -def test_pass_through_sp_VirtBase(vtype, expected_code): - obj = vtype() - ptr = m.pass_through_sp_VirtBase(obj) - print("\nLOOOK BEFORE del obj", flush=True) - del obj - print("\nLOOOK AFTER del obj", flush=True) - gc.collect() - print("\nLOOOK AFTER gc.collect()", flush=True) - assert ptr.get_code() == expected_code - print("\nLOOOK AFTER ptr.get_code()", flush=True) diff --git a/tests/test_class_sp_trampoline_weak_ptr.cpp b/tests/test_potentially_slicing_shared_ptr.cpp similarity index 59% rename from tests/test_class_sp_trampoline_weak_ptr.cpp rename to tests/test_potentially_slicing_shared_ptr.cpp index db9c447fee..10e7bfe399 100644 --- a/tests/test_class_sp_trampoline_weak_ptr.cpp +++ b/tests/test_potentially_slicing_shared_ptr.cpp @@ -7,7 +7,7 @@ #include namespace pybind11_tests { -namespace class_sp_trampoline_weak_ptr { +namespace potentially_slicing_shared_ptr { struct VirtBase { virtual ~VirtBase() = default; @@ -17,66 +17,69 @@ struct VirtBase { struct PyVirtBase : VirtBase, py::trampoline_self_life_support { using VirtBase::VirtBase; int get_code() override { PYBIND11_OVERRIDE(int, VirtBase, get_code); } - - ~PyVirtBase() override { - fflush(stderr); - printf("\nLOOOK ~PyVirtBase()\n"); - fflush(stdout); - } }; -struct WpOwner { - void set_wp(const std::shared_ptr &sp) { wp = sp; } +std::shared_ptr rtrn_obj_cast_shared_ptr(py::handle obj) { + return obj.cast>(); +} + +std::shared_ptr rtrn_potentially_slicing_shared_ptr(py::handle obj) { + return py::potentially_slicing_shared_ptr(obj); +} + +struct SpOwner { + void set_sp(const std::shared_ptr &sp_) { sp = sp_; } int get_code() { - auto sp = wp.lock(); if (!sp) { - return -999; + return -888; } return sp->get_code(); } private: - std::weak_ptr wp; + std::shared_ptr sp; }; -struct SpOwner { - void set_sp(const std::shared_ptr &sp_) { sp = sp_; } +struct WpOwner { + void set_wp(const std::shared_ptr &sp) { wp = sp; } int get_code() { + auto sp = wp.lock(); if (!sp) { - return -888; + return -999; } return sp->get_code(); } private: - std::shared_ptr sp; + std::weak_ptr wp; }; -std::shared_ptr pass_through_sp_VirtBase(const std::shared_ptr &sp) { - return sp; -} - -} // namespace class_sp_trampoline_weak_ptr +} // namespace potentially_slicing_shared_ptr } // namespace pybind11_tests -using namespace pybind11_tests::class_sp_trampoline_weak_ptr; +using namespace pybind11_tests::potentially_slicing_shared_ptr; -TEST_SUBMODULE(class_sp_trampoline_weak_ptr, m) { - py::class_, PyVirtBase>(m, "VirtBase") +TEST_SUBMODULE(potentially_slicing_shared_ptr, m) { + py::classh(m, "VirtBase") .def(py::init<>()) .def("get_code", &VirtBase::get_code); - py::class_(m, "WpOwner") - .def(py::init<>()) - .def("set_wp", &WpOwner::set_wp) - .def("get_code", &WpOwner::get_code); + m.def("rtrn_obj_cast_shared_ptr", rtrn_obj_cast_shared_ptr); + m.def("rtrn_potentially_slicing_shared_ptr", rtrn_potentially_slicing_shared_ptr); - py::class_(m, "SpOwner") + py::classh(m, "SpOwner") .def(py::init<>()) .def("set_sp", &SpOwner::set_sp) .def("get_code", &SpOwner::get_code); - m.def("pass_through_sp_VirtBase", pass_through_sp_VirtBase); + py::classh(m, "WpOwner") + .def(py::init<>()) + .def("set_wp", &WpOwner::set_wp) + .def("set_wp_potentially_slicing", + [](WpOwner &self, py::handle obj) { + self.set_wp(py::potentially_slicing_shared_ptr(obj)); + }) + .def("get_code", &WpOwner::get_code); } diff --git a/tests/test_potentially_slicing_shared_ptr.py b/tests/test_potentially_slicing_shared_ptr.py new file mode 100644 index 0000000000..2456d1a46f --- /dev/null +++ b/tests/test_potentially_slicing_shared_ptr.py @@ -0,0 +1,69 @@ +from __future__ import annotations + +import gc +import weakref + +import pytest + +import env +import pybind11_tests.potentially_slicing_shared_ptr as m + + +class PyDrvd(m.VirtBase): + def get_code(self): + return 200 + + +@pytest.mark.parametrize(("vtype", "expected_code"), [(m.VirtBase, 100), (PyDrvd, 200)]) +@pytest.mark.parametrize( + "rtrn_meth", ["rtrn_obj_cast_shared_ptr", "rtrn_potentially_slicing_shared_ptr"] +) +def test_rtrn_obj_cast_shared_ptr(vtype, rtrn_meth, expected_code): + obj = vtype() + ptr = getattr(m, rtrn_meth)(obj) + assert ptr.get_code() == expected_code + objref = weakref.ref(obj) + del obj + gc.collect() + assert ptr.get_code() == expected_code # the ptr Python object keeps obj alive + assert objref() is not None + del ptr + gc.collect() + assert objref() is None + + +@pytest.mark.parametrize(("vtype", "expected_code"), [(m.VirtBase, 100), (PyDrvd, 200)]) +def test_with_sp_owner(vtype, expected_code): + spo = m.SpOwner() + assert spo.get_code() == -888 + + obj = vtype() + assert obj.get_code() == expected_code + + spo.set_sp(obj) + assert spo.get_code() == expected_code + + del obj + gc.collect() + assert spo.get_code() == expected_code + + +@pytest.mark.parametrize(("vtype", "expected_code"), [(m.VirtBase, 100), (PyDrvd, 200)]) +@pytest.mark.parametrize("set_meth", ["set_wp", "set_wp_potentially_slicing"]) +def test_with_wp_owner(vtype, set_meth, expected_code): + wpo = m.WpOwner() + assert wpo.get_code() == -999 + + obj = vtype() + assert obj.get_code() == expected_code + + getattr(wpo, set_meth)(obj) + if vtype is m.VirtBase or set_meth == "set_wp_potentially_slicing": + assert wpo.get_code() == expected_code + else: + assert wpo.get_code() == -999 # see PR #5624 + + del obj + gc.collect() + if not (env.PYPY or env.GRAALPY): + assert wpo.get_code() == -999 From ff0792e7aa45a3112482e90faf77637cf926ddb1 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Sun, 27 Apr 2025 01:48:03 -0700 Subject: [PATCH 17/28] Generalize test_potentially_slicing_shared_ptr.cpp,py for testing with smart_holder and std::shared_ptr as holder. --- tests/test_potentially_slicing_shared_ptr.cpp | 92 +++++++++++++------ tests/test_potentially_slicing_shared_ptr.py | 87 +++++++++++++----- 2 files changed, 130 insertions(+), 49 deletions(-) diff --git a/tests/test_potentially_slicing_shared_ptr.cpp b/tests/test_potentially_slicing_shared_ptr.cpp index 10e7bfe399..069966cd93 100644 --- a/tests/test_potentially_slicing_shared_ptr.cpp +++ b/tests/test_potentially_slicing_shared_ptr.cpp @@ -9,26 +9,38 @@ namespace pybind11_tests { namespace potentially_slicing_shared_ptr { +template // Using int as a trick to easily generate multiple types. struct VirtBase { virtual ~VirtBase() = default; virtual int get_code() { return 100; } }; -struct PyVirtBase : VirtBase, py::trampoline_self_life_support { - using VirtBase::VirtBase; - int get_code() override { PYBIND11_OVERRIDE(int, VirtBase, get_code); } +using VirtBaseSH = VirtBase<0>; // for testing with py::smart_holder +using VirtBaseSP = VirtBase<1>; // for testing with std::shared_ptr as holder + +struct PyVirtBaseSH : VirtBaseSH, py::trampoline_self_life_support { + using VirtBaseSH::VirtBaseSH; + int get_code() override { PYBIND11_OVERRIDE(int, VirtBaseSH, get_code); } +}; + +struct PyVirtBaseSP : VirtBaseSP { // self-life-support not available + using VirtBaseSP::VirtBaseSP; + int get_code() override { PYBIND11_OVERRIDE(int, VirtBaseSP, get_code); } }; -std::shared_ptr rtrn_obj_cast_shared_ptr(py::handle obj) { - return obj.cast>(); +template +std::shared_ptr rtrn_obj_cast_shared_ptr(py::handle obj) { + return obj.cast>(); } -std::shared_ptr rtrn_potentially_slicing_shared_ptr(py::handle obj) { - return py::potentially_slicing_shared_ptr(obj); +template +std::shared_ptr rtrn_potentially_slicing_shared_ptr(py::handle obj) { + return py::potentially_slicing_shared_ptr(obj); } +template struct SpOwner { - void set_sp(const std::shared_ptr &sp_) { sp = sp_; } + void set_sp(const std::shared_ptr &sp_) { sp = sp_; } int get_code() { if (!sp) { @@ -38,11 +50,12 @@ struct SpOwner { } private: - std::shared_ptr sp; + std::shared_ptr sp; }; +template struct WpOwner { - void set_wp(const std::shared_ptr &sp) { wp = sp; } + void set_wp(const std::shared_ptr &sp) { wp = sp; } int get_code() { auto sp = wp.lock(); @@ -53,33 +66,56 @@ struct WpOwner { } private: - std::weak_ptr wp; + std::weak_ptr wp; }; +template +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); + m.def(rps_pyname, rtrn_potentially_slicing_shared_ptr); + + py::classh>(m, spo_pyname) + .def(py::init<>()) + .def("set_sp", &SpOwner::set_sp) + .def("get_code", &SpOwner::get_code); + + py::classh>(m, wpo_pyname) + .def(py::init<>()) + .def("set_wp", &WpOwner::set_wp) + .def("set_wp_potentially_slicing", + [](WpOwner &self, py::handle obj) { + self.set_wp(py::potentially_slicing_shared_ptr(obj)); + }) + .def("get_code", &WpOwner::get_code); +} + } // 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(m, "VirtBase") - .def(py::init<>()) - .def("get_code", &VirtBase::get_code); - - m.def("rtrn_obj_cast_shared_ptr", rtrn_obj_cast_shared_ptr); - m.def("rtrn_potentially_slicing_shared_ptr", rtrn_potentially_slicing_shared_ptr); - - py::classh(m, "SpOwner") + py::classh(m, "VirtBaseSH") .def(py::init<>()) - .def("set_sp", &SpOwner::set_sp) - .def("get_code", &SpOwner::get_code); + .def("get_code", &VirtBaseSH::get_code); - py::classh(m, "WpOwner") + py::class_, PyVirtBaseSP>(m, "VirtBaseSP") .def(py::init<>()) - .def("set_wp", &WpOwner::set_wp) - .def("set_wp_potentially_slicing", - [](WpOwner &self, py::handle obj) { - self.set_wp(py::potentially_slicing_shared_ptr(obj)); - }) - .def("get_code", &WpOwner::get_code); + .def("get_code", &VirtBaseSP::get_code); + + wrap(m, + "SH_rtrn_obj_cast_shared_ptr", + "SH_rtrn_potentially_slicing_shared_ptr", + "SH_SpOwner", + "SH_WpOwner"); + + wrap(m, + "SP_rtrn_obj_cast_shared_ptr", + "SP_rtrn_potentially_slicing_shared_ptr", + "SP_SpOwner", + "SP_WpOwner"); } diff --git a/tests/test_potentially_slicing_shared_ptr.py b/tests/test_potentially_slicing_shared_ptr.py index 2456d1a46f..e87adba224 100644 --- a/tests/test_potentially_slicing_shared_ptr.py +++ b/tests/test_potentially_slicing_shared_ptr.py @@ -9,18 +9,51 @@ import pybind11_tests.potentially_slicing_shared_ptr as m -class PyDrvd(m.VirtBase): +class PyDrvdSH(m.VirtBaseSH): def get_code(self): return 200 -@pytest.mark.parametrize(("vtype", "expected_code"), [(m.VirtBase, 100), (PyDrvd, 200)]) -@pytest.mark.parametrize( - "rtrn_meth", ["rtrn_obj_cast_shared_ptr", "rtrn_potentially_slicing_shared_ptr"] -) -def test_rtrn_obj_cast_shared_ptr(vtype, rtrn_meth, expected_code): - obj = vtype() - ptr = getattr(m, rtrn_meth)(obj) +class PyDrvdSP(m.VirtBaseSP): + def get_code(self): + return 200 + + +VIRT_BASE_TYPES = { + "SH": {100: m.VirtBaseSH, 200: PyDrvdSH}, + "SP": {100: m.VirtBaseSP, 200: PyDrvdSP}, +} + +RTRN_FUNCS = { + "SH": { + "oc": m.SH_rtrn_obj_cast_shared_ptr, + "ps": m.SH_rtrn_potentially_slicing_shared_ptr, + }, + "SP": { + "oc": m.SP_rtrn_obj_cast_shared_ptr, + "ps": m.SP_rtrn_potentially_slicing_shared_ptr, + }, +} + +SP_OWNER_TYPES = { + "SH": m.SH_SpOwner, + "SP": m.SP_SpOwner, +} + +WP_OWNER_TYPES = { + "SH": m.SH_WpOwner, + "SP": m.SP_WpOwner, +} + +GC_IS_RELIABLE = not (env.PYPY or env.GRAALPY) + + +@pytest.mark.parametrize("expected_code", [100, 200]) +@pytest.mark.parametrize("rtrn_kind", ["oc", "ps"]) +@pytest.mark.parametrize("holder_kind", ["SH", "SP"]) +def test_rtrn_obj_cast_shared_ptr(holder_kind, rtrn_kind, expected_code): + obj = VIRT_BASE_TYPES[holder_kind][expected_code]() + ptr = RTRN_FUNCS[holder_kind][rtrn_kind](obj) assert ptr.get_code() == expected_code objref = weakref.ref(obj) del obj @@ -29,15 +62,17 @@ def test_rtrn_obj_cast_shared_ptr(vtype, rtrn_meth, expected_code): assert objref() is not None del ptr gc.collect() - assert objref() is None + if GC_IS_RELIABLE: + assert objref() is None -@pytest.mark.parametrize(("vtype", "expected_code"), [(m.VirtBase, 100), (PyDrvd, 200)]) -def test_with_sp_owner(vtype, expected_code): - spo = m.SpOwner() +@pytest.mark.parametrize("expected_code", [100, 200]) +@pytest.mark.parametrize("holder_kind", ["SH", "SP"]) +def test_with_sp_owner(holder_kind, expected_code): + spo = SP_OWNER_TYPES[holder_kind]() assert spo.get_code() == -888 - obj = vtype() + obj = VIRT_BASE_TYPES[holder_kind][expected_code]() assert obj.get_code() == expected_code spo.set_sp(obj) @@ -45,25 +80,35 @@ def test_with_sp_owner(vtype, expected_code): del obj gc.collect() - assert spo.get_code() == expected_code + if holder_kind == "SH": + assert spo.get_code() == expected_code + elif GC_IS_RELIABLE: + assert ( + spo.get_code() == 100 + ) # see issue #1333 (inheritance slicing) and PR #5624 -@pytest.mark.parametrize(("vtype", "expected_code"), [(m.VirtBase, 100), (PyDrvd, 200)]) +@pytest.mark.parametrize("expected_code", [100, 200]) @pytest.mark.parametrize("set_meth", ["set_wp", "set_wp_potentially_slicing"]) -def test_with_wp_owner(vtype, set_meth, expected_code): - wpo = m.WpOwner() +@pytest.mark.parametrize("holder_kind", ["SH", "SP"]) +def test_with_wp_owner(holder_kind, set_meth, expected_code): + wpo = WP_OWNER_TYPES[holder_kind]() assert wpo.get_code() == -999 - obj = vtype() + obj = VIRT_BASE_TYPES[holder_kind][expected_code]() assert obj.get_code() == expected_code getattr(wpo, set_meth)(obj) - if vtype is m.VirtBase or set_meth == "set_wp_potentially_slicing": + if ( + holder_kind == "SP" + or expected_code == 100 + or set_meth == "set_wp_potentially_slicing" + ): assert wpo.get_code() == expected_code else: - assert wpo.get_code() == -999 # see PR #5624 + assert wpo.get_code() == -999 # see issue #5623 (weak_ptr expired) and PR #5624 del obj gc.collect() - if not (env.PYPY or env.GRAALPY): + if GC_IS_RELIABLE: assert wpo.get_code() == -999 From eb1787cc9b3679e15b00e54170b6fc3195a5df0d Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Sun, 27 Apr 2025 01:58:25 -0700 Subject: [PATCH 18/28] Add back test_potentially_slicing_shared_ptr_not_convertible_error(), it got lost accidentally in commit 56d23dc47816a2a9f54c1da818c7d45e1034e421 --- tests/test_potentially_slicing_shared_ptr.py | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/tests/test_potentially_slicing_shared_ptr.py b/tests/test_potentially_slicing_shared_ptr.py index e87adba224..9fd9bd6bbc 100644 --- a/tests/test_potentially_slicing_shared_ptr.py +++ b/tests/test_potentially_slicing_shared_ptr.py @@ -112,3 +112,18 @@ def test_with_wp_owner(holder_kind, set_meth, expected_code): gc.collect() if GC_IS_RELIABLE: assert wpo.get_code() == -999 + + +def test_potentially_slicing_shared_ptr_not_convertible_error(): + with pytest.raises(Exception) as excinfo: + m.SH_rtrn_potentially_slicing_shared_ptr("") + assert str(excinfo.value) == ( + '"str" object is not convertible to std::shared_ptr' + " (with T = pybind11_tests::potentially_slicing_shared_ptr::VirtBase<0>)" + ) + with pytest.raises(Exception) as excinfo: + m.SP_rtrn_potentially_slicing_shared_ptr([]) + assert str(excinfo.value) == ( + '"list" object is not convertible to std::shared_ptr' + " (with T = pybind11_tests::potentially_slicing_shared_ptr::VirtBase<1>)" + ) From 10f6e78c3dcbeba7561004a604bd47d49ef39d78 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Sun, 27 Apr 2025 21:28:10 -0700 Subject: [PATCH 19/28] Add simple trampoline state assertions. --- tests/test_potentially_slicing_shared_ptr.cpp | 56 +++++++++++++++++-- tests/test_potentially_slicing_shared_ptr.py | 14 +++++ 2 files changed, 64 insertions(+), 6 deletions(-) diff --git a/tests/test_potentially_slicing_shared_ptr.cpp b/tests/test_potentially_slicing_shared_ptr.cpp index 069966cd93..ad628e9a96 100644 --- a/tests/test_potentially_slicing_shared_ptr.cpp +++ b/tests/test_potentially_slicing_shared_ptr.cpp @@ -18,12 +18,50 @@ struct VirtBase { using VirtBaseSH = VirtBase<0>; // for testing with py::smart_holder using VirtBaseSP = VirtBase<1>; // for testing with std::shared_ptr as holder -struct PyVirtBaseSH : VirtBaseSH, py::trampoline_self_life_support { +// 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) { + magic_token = other.magic_token; + } + 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 +const char *determine_trampoline_state(const std::shared_ptr &sp) { + if (!sp) { + return "sp nullptr"; + } + auto *tias = dynamic_cast(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 { // self-life-support not available +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); } }; @@ -42,13 +80,15 @@ template struct SpOwner { void set_sp(const std::shared_ptr &sp_) { sp = sp_; } - int get_code() { + 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 sp; }; @@ -57,7 +97,7 @@ template struct WpOwner { void set_wp(const std::shared_ptr &sp) { wp = sp; } - int get_code() { + int get_code() const { auto sp = wp.lock(); if (!sp) { return -999; @@ -65,6 +105,8 @@ struct WpOwner { return sp->get_code(); } + const char *get_trampoline_state() const { return determine_trampoline_state(wp.lock()); } + private: std::weak_ptr wp; }; @@ -81,7 +123,8 @@ void wrap(py::module_ &m, py::classh>(m, spo_pyname) .def(py::init<>()) .def("set_sp", &SpOwner::set_sp) - .def("get_code", &SpOwner::get_code); + .def("get_code", &SpOwner::get_code) + .def("get_trampoline_state", &SpOwner::get_trampoline_state); py::classh>(m, wpo_pyname) .def(py::init<>()) @@ -90,7 +133,8 @@ void wrap(py::module_ &m, [](WpOwner &self, py::handle obj) { self.set_wp(py::potentially_slicing_shared_ptr(obj)); }) - .def("get_code", &WpOwner::get_code); + .def("get_code", &WpOwner::get_code) + .def("get_trampoline_state", &WpOwner::get_trampoline_state); } } // namespace potentially_slicing_shared_ptr diff --git a/tests/test_potentially_slicing_shared_ptr.py b/tests/test_potentially_slicing_shared_ptr.py index 9fd9bd6bbc..dd3d736a8e 100644 --- a/tests/test_potentially_slicing_shared_ptr.py +++ b/tests/test_potentially_slicing_shared_ptr.py @@ -71,12 +71,17 @@ def test_rtrn_obj_cast_shared_ptr(holder_kind, rtrn_kind, expected_code): def test_with_sp_owner(holder_kind, expected_code): spo = SP_OWNER_TYPES[holder_kind]() assert spo.get_code() == -888 + assert spo.get_trampoline_state() == "sp nullptr" obj = VIRT_BASE_TYPES[holder_kind][expected_code]() assert obj.get_code() == expected_code spo.set_sp(obj) assert spo.get_code() == expected_code + expected_trampoline_state = ( + "dynamic_cast failed" if expected_code == 100 else "trampoline alive" + ) + assert spo.get_trampoline_state() == expected_trampoline_state del obj gc.collect() @@ -86,6 +91,7 @@ def test_with_sp_owner(holder_kind, expected_code): assert ( spo.get_code() == 100 ) # see issue #1333 (inheritance slicing) and PR #5624 + assert spo.get_trampoline_state() == expected_trampoline_state @pytest.mark.parametrize("expected_code", [100, 200]) @@ -94,6 +100,7 @@ def test_with_sp_owner(holder_kind, expected_code): def test_with_wp_owner(holder_kind, set_meth, expected_code): wpo = WP_OWNER_TYPES[holder_kind]() assert wpo.get_code() == -999 + assert wpo.get_trampoline_state() == "sp nullptr" obj = VIRT_BASE_TYPES[holder_kind][expected_code]() assert obj.get_code() == expected_code @@ -107,6 +114,13 @@ def test_with_wp_owner(holder_kind, set_meth, expected_code): assert wpo.get_code() == expected_code else: assert wpo.get_code() == -999 # see issue #5623 (weak_ptr expired) and PR #5624 + if expected_code == 100: + expected_trampoline_state = "dynamic_cast failed" + elif holder_kind == "SH" and set_meth == "set_wp": + expected_trampoline_state = "sp nullptr" + else: + expected_trampoline_state = "trampoline alive" + assert wpo.get_trampoline_state() == expected_trampoline_state del obj gc.collect() From 121c8cfe9831c3ffc4da1f4712838bce6289de4d Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Sun, 27 Apr 2025 23:22:26 -0700 Subject: [PATCH 20/28] 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; | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ``` --- tests/test_potentially_slicing_shared_ptr.cpp | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/tests/test_potentially_slicing_shared_ptr.cpp b/tests/test_potentially_slicing_shared_ptr.cpp index ad628e9a96..0c528e334a 100644 --- a/tests/test_potentially_slicing_shared_ptr.cpp +++ b/tests/test_potentially_slicing_shared_ptr.cpp @@ -26,11 +26,9 @@ struct trampoline_is_alive_simple { ~trampoline_is_alive_simple() { magic_token = 20380118191407u; } - trampoline_is_alive_simple(const trampoline_is_alive_simple &other) { - magic_token = other.magic_token; - } - trampoline_is_alive_simple(trampoline_is_alive_simple &&other) noexcept { - magic_token = other.magic_token; + 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; } From b42bd111b2c296e04e5b3bbd7fee7d2549d40e52 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Mon, 28 Apr 2025 00:20:35 -0700 Subject: [PATCH 21/28] Add a (long) C++ comment for py::potentially_slicing_shared_ptr<>() --- include/pybind11/cast.h | 32 ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h index 700ada6d4b..12f071ca73 100644 --- a/include/pybind11/cast.h +++ b/include/pybind11/cast.h @@ -1089,6 +1089,38 @@ class type_caster> : public copyable_holder_caster>() +/// - py::potentially_slicing_shared_ptr(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 std::shared_ptr potentially_slicing_shared_ptr(handle obj) { detail::make_caster> caster; From f160450e46b6899b98f0ef3911c1e84d81d10a96 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Mon, 28 Apr 2025 09:36:28 -0700 Subject: [PATCH 22/28] [skip ci] Add new "Avoiding Inheritance Slicing and ``std::weak_ptr`` surprises" section in advanced/classes.rst --- docs/advanced/classes.rst | 45 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 45 insertions(+) diff --git a/docs/advanced/classes.rst b/docs/advanced/classes.rst index 0ada0cb60c..33bc00c653 100644 --- a/docs/advanced/classes.rst +++ b/docs/advanced/classes.rst @@ -422,6 +422,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``. Depending on whether the class uses a plain +``std::shared_ptr`` holder or ``py::smart_holder``, the resulting pointer +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`` (e.g., ``obj.cast>()``, or simply +passing the Python object as an argument to a ``.def()``-ed function) returns +a pointer 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 `_). + +In contrast, with ``py::smart_holder``, converting a Python object to +a ``std::shared_ptr`` 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 `_). + +If it is necessary to obtain a ``std::shared_ptr`` that shares the control block +with the ``smart_holder``—at the cost of reintroducing potential inheritance +slicing—you can use ``py::potentially_slicing_shared_ptr(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_shared_ptr` C++ documentation + * :file:`tests/test_potentially_slicing_shared_ptr.cpp` + .. _custom_constructors: From 78178d75ea9808cca08cfbb50d49faf1b7ade2df Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Mon, 28 Apr 2025 10:51:13 -0700 Subject: [PATCH 23/28] [skip ci] Add introductory comment to test_potentially_slicing_shared_ptr.py --- tests/test_potentially_slicing_shared_ptr.cpp | 2 +- tests/test_potentially_slicing_shared_ptr.py | 31 +++++++++++++++++++ 2 files changed, 32 insertions(+), 1 deletion(-) diff --git a/tests/test_potentially_slicing_shared_ptr.cpp b/tests/test_potentially_slicing_shared_ptr.cpp index 0c528e334a..77a74e1129 100644 --- a/tests/test_potentially_slicing_shared_ptr.cpp +++ b/tests/test_potentially_slicing_shared_ptr.cpp @@ -1,4 +1,4 @@ -// Copyright (c) 2025 The Pybind Development Team. +// 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. diff --git a/tests/test_potentially_slicing_shared_ptr.py b/tests/test_potentially_slicing_shared_ptr.py index dd3d736a8e..1f43e94157 100644 --- a/tests/test_potentially_slicing_shared_ptr.py +++ b/tests/test_potentially_slicing_shared_ptr.py @@ -1,3 +1,34 @@ +# 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. + +# This module tests the interaction of pybind11's shared_ptr and smart_holder +# mechanisms with trampoline object lifetime management and inheritance slicing. +# +# The following combinations are covered: +# +# - Holder type: std::shared_ptr (class_ holder) vs. +# py::smart_holder +# - Conversion function: obj.cast>() vs. +# py::potentially_slicing_shared_ptr(obj) +# - Python object type: C++ base class vs. +# Python-derived trampoline class +# +# The tests verify +# +# - that casting or passing Python objects into functions returns usable +# std::shared_ptr instances. +# - that inheritance slicing occurs as expected in controlled cases +# (issue #1333). +# - that surprising weak_ptr behavior (issue #5623) can be reproduced when +# smart_holder is used. +# - that the trampoline object remains alive in all situations +# (no use-after-free) as long as the C++ shared_ptr exists. +# +# Where applicable, trampoline state is introspected to confirm whether the +# C++ object retains knowledge of the Python override or has fallen back to +# the base implementation. + from __future__ import annotations import gc From 222b4a7339cacfbbf5b0e8f7fc50a0b8f85a1f08 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Thu, 1 May 2025 08:56:14 -0700 Subject: [PATCH 24/28] Minimal (!) changes to have py::potentially_slicing_weak_ptr(handle) as the public API. For CI testing, before changing the names around more widely, and the documentation. --- include/pybind11/cast.h | 10 +++++----- tests/test_potentially_slicing_shared_ptr.cpp | 11 +++++++---- tests/test_potentially_slicing_shared_ptr.py | 4 ++-- 3 files changed, 14 insertions(+), 11 deletions(-) diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h index 12f071ca73..c432a5e57f 100644 --- a/include/pybind11/cast.h +++ b/include/pybind11/cast.h @@ -983,7 +983,7 @@ struct copyable_holder_caster< return shared_ptr_storage; } - std::shared_ptr &potentially_slicing_shared_ptr() { + std::weak_ptr potentially_slicing_weak_ptr() { if (typeinfo->holder_enum_v == detail::holder_enum_t::smart_holder) { shared_ptr_storage = sh_load_helper.load_as_shared_ptr(value, @@ -1122,15 +1122,15 @@ PYBIND11_NAMESPACE_END(detail) /// - the std::shared_ptr would own a reference to the derived Python object, /// completing the cycle template -std::shared_ptr potentially_slicing_shared_ptr(handle obj) { +std::weak_ptr potentially_slicing_weak_ptr(handle obj) { detail::make_caster> caster; if (caster.load(obj, /*convert=*/true)) { - return caster.potentially_slicing_shared_ptr(); + 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::shared_ptr (with T = " - + type_id() + ")"); + + "\" object is not convertible to std::weak_ptr (with T = " + type_id() + + ")"); } PYBIND11_NAMESPACE_BEGIN(detail) diff --git a/tests/test_potentially_slicing_shared_ptr.cpp b/tests/test_potentially_slicing_shared_ptr.cpp index 77a74e1129..fbec211757 100644 --- a/tests/test_potentially_slicing_shared_ptr.cpp +++ b/tests/test_potentially_slicing_shared_ptr.cpp @@ -71,7 +71,7 @@ std::shared_ptr rtrn_obj_cast_shared_ptr(py::handle obj) { template std::shared_ptr rtrn_potentially_slicing_shared_ptr(py::handle obj) { - return py::potentially_slicing_shared_ptr(obj); + return py::potentially_slicing_weak_ptr(obj).lock(); } template @@ -93,7 +93,7 @@ struct SpOwner { template struct WpOwner { - void set_wp(const std::shared_ptr &sp) { wp = sp; } + void set_wp(const std::weak_ptr &wp_) { wp = wp_; } int get_code() const { auto sp = wp.lock(); @@ -126,10 +126,13 @@ void wrap(py::module_ &m, py::classh>(m, wpo_pyname) .def(py::init<>()) - .def("set_wp", &WpOwner::set_wp) + .def("set_wp", + [](WpOwner &self, py::handle obj) { + self.set_wp(obj.cast>()); + }) .def("set_wp_potentially_slicing", [](WpOwner &self, py::handle obj) { - self.set_wp(py::potentially_slicing_shared_ptr(obj)); + self.set_wp(py::potentially_slicing_weak_ptr(obj)); }) .def("get_code", &WpOwner::get_code) .def("get_trampoline_state", &WpOwner::get_trampoline_state); diff --git a/tests/test_potentially_slicing_shared_ptr.py b/tests/test_potentially_slicing_shared_ptr.py index 1f43e94157..3c5cf675f9 100644 --- a/tests/test_potentially_slicing_shared_ptr.py +++ b/tests/test_potentially_slicing_shared_ptr.py @@ -163,12 +163,12 @@ def test_potentially_slicing_shared_ptr_not_convertible_error(): with pytest.raises(Exception) as excinfo: m.SH_rtrn_potentially_slicing_shared_ptr("") assert str(excinfo.value) == ( - '"str" object is not convertible to std::shared_ptr' + '"str" object is not convertible to std::weak_ptr' " (with T = pybind11_tests::potentially_slicing_shared_ptr::VirtBase<0>)" ) with pytest.raises(Exception) as excinfo: m.SP_rtrn_potentially_slicing_shared_ptr([]) assert str(excinfo.value) == ( - '"list" object is not convertible to std::shared_ptr' + '"list" object is not convertible to std::weak_ptr' " (with T = pybind11_tests::potentially_slicing_shared_ptr::VirtBase<1>)" ) From 03a8981a14e2235859a5ee39668481d0d946febe Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Thu, 8 May 2025 14:14:21 -0700 Subject: [PATCH 25/28] =?UTF-8?q?Rename=20test=5Fpotentially=5Fslicing=5Fs?= =?UTF-8?q?hared=5Fptr.cpp,py=20=E2=86=92=20test=5Fpotentially=5Fslicing?= =?UTF-8?q?=5Fweak=5Fptr.cpp,py?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- tests/CMakeLists.txt | 2 +- ...d_ptr.cpp => test_potentially_slicing_weak_ptr.cpp} | 8 ++++---- ...red_ptr.py => test_potentially_slicing_weak_ptr.py} | 10 +++++----- 3 files changed, 10 insertions(+), 10 deletions(-) rename tests/{test_potentially_slicing_shared_ptr.cpp => test_potentially_slicing_weak_ptr.cpp} (96%) rename tests/{test_potentially_slicing_shared_ptr.py => test_potentially_slicing_weak_ptr.py} (93%) diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index a188c28638..29172b6ce6 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -161,7 +161,7 @@ set(PYBIND11_TEST_FILES test_opaque_types test_operator_overloading test_pickling - test_potentially_slicing_shared_ptr + test_potentially_slicing_weak_ptr test_python_multiple_inheritance test_pytypes test_sequences_and_iterators diff --git a/tests/test_potentially_slicing_shared_ptr.cpp b/tests/test_potentially_slicing_weak_ptr.cpp similarity index 96% rename from tests/test_potentially_slicing_shared_ptr.cpp rename to tests/test_potentially_slicing_weak_ptr.cpp index fbec211757..bc0a010420 100644 --- a/tests/test_potentially_slicing_shared_ptr.cpp +++ b/tests/test_potentially_slicing_weak_ptr.cpp @@ -7,7 +7,7 @@ #include namespace pybind11_tests { -namespace potentially_slicing_shared_ptr { +namespace potentially_slicing_weak_ptr { template // Using int as a trick to easily generate multiple types. struct VirtBase { @@ -138,12 +138,12 @@ void wrap(py::module_ &m, .def("get_trampoline_state", &WpOwner::get_trampoline_state); } -} // namespace potentially_slicing_shared_ptr +} // namespace potentially_slicing_weak_ptr } // namespace pybind11_tests -using namespace pybind11_tests::potentially_slicing_shared_ptr; +using namespace pybind11_tests::potentially_slicing_weak_ptr; -TEST_SUBMODULE(potentially_slicing_shared_ptr, m) { +TEST_SUBMODULE(potentially_slicing_weak_ptr, m) { py::classh(m, "VirtBaseSH") .def(py::init<>()) .def("get_code", &VirtBaseSH::get_code); diff --git a/tests/test_potentially_slicing_shared_ptr.py b/tests/test_potentially_slicing_weak_ptr.py similarity index 93% rename from tests/test_potentially_slicing_shared_ptr.py rename to tests/test_potentially_slicing_weak_ptr.py index 3c5cf675f9..d442f99a7f 100644 --- a/tests/test_potentially_slicing_shared_ptr.py +++ b/tests/test_potentially_slicing_weak_ptr.py @@ -10,7 +10,7 @@ # - Holder type: std::shared_ptr (class_ holder) vs. # py::smart_holder # - Conversion function: obj.cast>() vs. -# py::potentially_slicing_shared_ptr(obj) +# py::potentially_slicing_weak_ptr(obj) # - Python object type: C++ base class vs. # Python-derived trampoline class # @@ -37,7 +37,7 @@ import pytest import env -import pybind11_tests.potentially_slicing_shared_ptr as m +import pybind11_tests.potentially_slicing_weak_ptr as m class PyDrvdSH(m.VirtBaseSH): @@ -159,16 +159,16 @@ def test_with_wp_owner(holder_kind, set_meth, expected_code): assert wpo.get_code() == -999 -def test_potentially_slicing_shared_ptr_not_convertible_error(): +def test_potentially_slicing_weak_ptr_not_convertible_error(): with pytest.raises(Exception) as excinfo: m.SH_rtrn_potentially_slicing_shared_ptr("") assert str(excinfo.value) == ( '"str" object is not convertible to std::weak_ptr' - " (with T = pybind11_tests::potentially_slicing_shared_ptr::VirtBase<0>)" + " (with T = pybind11_tests::potentially_slicing_weak_ptr::VirtBase<0>)" ) with pytest.raises(Exception) as excinfo: m.SP_rtrn_potentially_slicing_shared_ptr([]) assert str(excinfo.value) == ( '"list" object is not convertible to std::weak_ptr' - " (with T = pybind11_tests::potentially_slicing_shared_ptr::VirtBase<1>)" + " (with T = pybind11_tests::potentially_slicing_weak_ptr::VirtBase<1>)" ) From bad0c12b79391e0207c4c4c2bd67af670145450d Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Thu, 8 May 2025 14:33:52 -0700 Subject: [PATCH 26/28] =?UTF-8?q?Update=20docs/advanced/classes.rst=20and?= =?UTF-8?q?=20C++=20comments=20=E2=86=92=20potentially=5Fslicing=5Fweak=5F?= =?UTF-8?q?ptr?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- docs/advanced/classes.rst | 8 ++++---- include/pybind11/cast.h | 9 +++++---- tests/test_potentially_slicing_weak_ptr.cpp | 2 ++ 3 files changed, 11 insertions(+), 8 deletions(-) diff --git a/docs/advanced/classes.rst b/docs/advanced/classes.rst index 05cfcd812a..7fcf503315 100644 --- a/docs/advanced/classes.rst +++ b/docs/advanced/classes.rst @@ -459,9 +459,9 @@ inheritance slicing but can lead to unintended behavior when creating ``std::weak_ptr`` instances (see `#5623 `_). -If it is necessary to obtain a ``std::shared_ptr`` that shares the control block +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_shared_ptr(obj)``. +slicing—you can use ``py::potentially_slicing_weak_ptr(obj)``. When precise lifetime management of derived Python objects is important, using a Python-side ``weakref`` is the most reliable approach, as it avoids @@ -470,8 +470,8 @@ semantics in C++. .. seealso:: - * :func:`potentially_slicing_shared_ptr` C++ documentation - * :file:`tests/test_potentially_slicing_shared_ptr.cpp` + * :func:`potentially_slicing_weak_ptr` C++ documentation + * :file:`tests/test_potentially_slicing_weak_ptr.cpp` .. _custom_constructors: diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h index c432a5e57f..3bedb9d1a3 100644 --- a/include/pybind11/cast.h +++ b/include/pybind11/cast.h @@ -985,6 +985,7 @@ struct copyable_holder_caster< std::weak_ptr potentially_slicing_weak_ptr() { if (typeinfo->holder_enum_v == detail::holder_enum_t::smart_holder) { + // Reusing shared_ptr code to minimize code complexity. shared_ptr_storage = sh_load_helper.load_as_shared_ptr(value, /*responsible_parent=*/nullptr, @@ -1094,17 +1095,17 @@ PYBIND11_NAMESPACE_END(detail) /// 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): +/// produce equivalent results (see tests/test_potentially_slicing_weak_ptr.cpp,py): /// /// - obj.cast>() -/// - py::potentially_slicing_shared_ptr(obj) +/// - py::potentially_slicing_weak_ptr(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_shared_ptr<>(obj), -/// as exercised in tests/test_potentially_slicing_shared_ptr.cpp,py (look for +/// (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. diff --git a/tests/test_potentially_slicing_weak_ptr.cpp b/tests/test_potentially_slicing_weak_ptr.cpp index bc0a010420..01b147faff 100644 --- a/tests/test_potentially_slicing_weak_ptr.cpp +++ b/tests/test_potentially_slicing_weak_ptr.cpp @@ -69,6 +69,8 @@ std::shared_ptr rtrn_obj_cast_shared_ptr(py::handle obj) { return obj.cast>(); } +// There is no type_caster>, and to minimize code complexity +// we do not want to add one, therefore we have to return a shared_ptr here. template std::shared_ptr rtrn_potentially_slicing_shared_ptr(py::handle obj) { return py::potentially_slicing_weak_ptr(obj).lock(); From 683fff4d28f02a29158735b6f554cf01723f8acf Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Thu, 8 May 2025 14:42:44 -0700 Subject: [PATCH 27/28] Write "shared_ptr" instead of just "pointer" in a couple places in docs/advanced/classes.rst --- docs/advanced/classes.rst | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/docs/advanced/classes.rst b/docs/advanced/classes.rst index 7fcf503315..5f11215d06 100644 --- a/docs/advanced/classes.rst +++ b/docs/advanced/classes.rst @@ -434,9 +434,9 @@ 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``. Depending on whether the class uses a plain -``std::shared_ptr`` holder or ``py::smart_holder``, the resulting pointer -may either allow inheritance slicing or lead to potentially surprising behavior -when constructing ``std::weak_ptr`` instances. +``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 @@ -446,9 +446,9 @@ desired behavior. When using ``std::shared_ptr`` as the holder type, converting a Python object to a ``std::shared_ptr`` (e.g., ``obj.cast>()``, or simply passing the Python object as an argument to a ``.def()``-ed function) returns -a pointer 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 +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 `_). From b349e84d860c338ffb7347a514e3dbf67bbb684c Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Sat, 10 May 2025 12:39:06 -0700 Subject: [PATCH 28/28] Add comment for force_potentially_slicing_shared_ptr in type_caster_base.h, to make a direct connection py::potentially_slicing_weak_ptr --- include/pybind11/detail/type_caster_base.h | 2 ++ 1 file changed, 2 insertions(+) diff --git a/include/pybind11/detail/type_caster_base.h b/include/pybind11/detail/type_caster_base.h index 698df9b2cc..ceef18cb4a 100644 --- a/include/pybind11/detail/type_caster_base.h +++ b/include/pybind11/detail/type_caster_base.h @@ -760,6 +760,8 @@ struct load_helper : value_and_holder_helper { std::shared_ptr load_as_shared_ptr(void *void_raw_ptr, 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()) {