-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Add py::potentially_slicing_weak_ptr(handle)
function
#5624
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
virtual ~WpBase() = default; | ||
}; | ||
|
||
struct PyWpBase : WpBase { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
struct PyWpBase : WpBase, py::trampoline_self_life_support {
Does that help?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(I just fixed an oversight in my previous comment.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
struct PyWpBase : WpBase { | |
struct PyWpBase : WpBase, py::trampoline_self_life_support { |
I simplified the reproducer and changed the variable names to make them more intuitive (to me). I'm guessing we're building an intermediate I wrote that code four years ago. Quite likely this will take me several hours to re-learn what exactly is happening. Not sure when I'll get to it. @nsoblath How did you arrive at that test? Did you have code that worked with |
…s holder). Tweak test_class_sh_trampoline_weak_ptr.py to pass, with `# THIS NEEDS FIXING` comment.
On that suspicion, I added test_class_sp_trampoline_weak_ptr.cpp,py, and it seems to work: it certainly works locally, but I'm waiting for GitHub Actions to finish. Assuming the new test works reliably on all platforms, I'd guess that it's feasible to fix the smart_holder equivalent. |
``` /__w/pybind11/pybind11/tests/test_class_sh_trampoline_weak_ptr.cpp:23:43: error: the parameter 'sp' is copied for each invocation but only used as a const reference; consider making it a const reference [performance-unnecessary-value-param,-warnings-as-errors] 23 | void set_wp(std::shared_ptr<VirtBase> sp) { wp = sp; } | ^ | const & 4443 warnings generated. ``` ``` /__w/pybind11/pybind11/tests/test_class_sp_trampoline_weak_ptr.cpp:23:43: error: the parameter 'sp' is copied for each invocation but only used as a const reference; consider making it a const reference [performance-unnecessary-value-param,-warnings-as-errors] 23 | void set_wp(std::shared_ptr<VirtBase> sp) { wp = sp; } | ^ | const & 4430 warnings generated. ```
Before I wrote:
That's here (conclusive):
Note that the
So in line 782 we need to return a
Any ideas, please let me know. (I haven't thought about it much yet; the penny didn't want to drop immediately.) @virtuald for visibility |
I copied the test_class_sh_trampoline_shared_ptr_cpp_arg test, modified it to store a |
I've now turned this over in my mind for a couple hours, and consulted ChatGPT, which I believe got very confused/confusing, but please see for yourself here. Stepping out of the box:
Solving this is a real brain teaser. I'm not sure I can help. I'm not sure if there is a viable solution. I have a couple ideas, but each of them might take hours of work only to find out it leads to a dead end. Could you try to work around this limitation? For example, instead of owning the C++ |
…s does not inherit from this class, preserve established Inheritance Slicing behavior. rwgk reached this point with the help of ChatGPT: * https://chatgpt.com/share/68056498-7d94-8008-8ff0-232e2aba451c The only production code change in this commit is: ``` diff --git a/include/pybind11/detail/type_caster_base.h b/include/pybind11/detail/type_caster_base.h index d4f9a41..f3d4530 100644 --- a/include/pybind11/detail/type_caster_base.h +++ b/include/pybind11/detail/type_caster_base.h @@ -776,6 +776,14 @@ struct load_helper : value_and_holder_helper { if (released_ptr) { return std::shared_ptr<T>(released_ptr, type_raw_ptr); } + auto *self_life_support + = dynamic_raw_ptr_cast_if_possible<trampoline_self_life_support>(type_raw_ptr); + if (self_life_support == nullptr) { + std::shared_ptr<void> void_shd_ptr = hld.template as_shared_ptr<void>(); + std::shared_ptr<T> to_be_released(void_shd_ptr, type_raw_ptr); + vptr_gd_ptr->released_ptr = to_be_released; + return to_be_released; + } std::shared_ptr<T> to_be_released( type_raw_ptr, shared_ptr_trampoline_self_life_support(loaded_v_h.inst)); vptr_gd_ptr->released_ptr = to_be_released; ```
I'm testing commit 4638e01 to see if that works on all platforms. See https://chatgpt.com/share/68056498-7d94-8008-8ff0-232e2aba451c for how I got there / rationale. I'm not sure if 4638e01 is a wise twist to add. Fundamentally, it'd be better to support |
``` 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' ```
@nsoblath I feel I'm pretty clear now about the situation. I think it's best not to change the smart_holder code, but to go with the approach outlined before:
I'll explain more when I get to it, probably next weekend. (The only thing I want to change, probably, is to add a |
@rwgk Thank you very much for your time and attention looking into this. I really appreciate it. Though my understanding of the internals of pybind11 is quite incomplete, my thoughts about what might be going wrong were similar to what you suggested: that the control block for the In your later message you said
Does that mean you're preferring to not use the change in 4638e01? Is there a downside to making sure that the control block of the returned Regarding the alternative solution you proposed:
I believe I can implement something to this effect. It's not ideal because the library where I'm using this is sometimes used as a pure-C++ library, sometimes used with its Python bindings. |
Yes.
Yes, it'll be very confusing to many (I think), while only being useful in a few niche cases. It is not obvious at all that the I feel it'll be better to keep the smart_holder feature simpler and the behavior consistent. It will be easier to reason about the bindings. We could probably add an interface that makes it relatively easy for you to get the inheritance-slicing- What I didn't explain: I convinced myself that it is impossible to get a
Reason: This would introduce a hard reference cycle, because the Python object ( |
@nsoblath If you can wait a week or two, I think (although I'm not sure) that I can give you something like this: py::classh<WpOwner>(m, "WpOwner")
...
.def("set_wp", [](WpOwner& self, py::handle obj) {
self.set_wp(py::slicing_shared_ptr_from_smart_holder<VirtBase>(obj));
})
... |
@rwgk I can certainly wait, and if that works, it'd be great. Thank you! Incidentally I have been working on the workaround we talked about, but this would be a cleaner solution (at least on the side of my application). |
I wouldn't call the proposed "slicing" option the cleaner solution. It's only easier. — You might still get a |
Also undo the corresponding test change in test_class_sh_trampoline_weak_ptr.py But keep all extra debugging code for now.
…cast to `std::shared_ptr<VirtBase>` for now.
@nsoblath I'm mostly done, if you want to take a look. The only part missing is an addition to the trampoline documentation. |
… surprises" section in advanced/classes.rst
@nsoblath All done now. Could you please review the added documentation? @virtuald Is there a chance that you could help with a review? — The production code changes are small and nearly trivial. Most of my effort for this PR went into the new stress-test level unit test, documentation, and source code comments. |
@rwgk The added documentation is excellent. I think it clarifies the situation and makes the options clear. I appreciate both the .rst documentation that will be in the RTD site, and the header-file documentation that includes more technical details. With the documentation, it was clear that for my application the My simplified test cases and actual application code all now work. Thank you very much for your time and efforts on this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is definitely an esoteric bug, but I see how you got there. If everyone else is happy with this, I am happy with it too and you can ignore my nitpicky comment.
include/pybind11/cast.h
Outdated
/// - the std::shared_ptr would own a reference to the derived Python object, | ||
/// completing the cycle | ||
template <typename T> | ||
std::shared_ptr<T> potentially_slicing_shared_ptr(handle obj) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This API is weird to me. If the sole reason for this API to exist is to obtain a weak_ptr that doesn't accidentally expire, why not just return a weak_ptr directly instead? Are there other reasons one might want to get this problematic shared_ptr?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you're right. Trying it out right now: 222b4a7 (that's minimal changes only; more will be needed if testing goes well (as expected))
The implementation and the testing is a little more awkward, because we cannot pass std::weak_ptr
around between Python and C++ (and I wouldn't want to add that complexity), but I totally agree that the public-facing API is better at nudging people in the right direction. What I mean is, it'll be even more likely that the new public API will only get used if a std::weak_ptr
is truly needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds a new function, py::potentially_slicing_shared_ptr, to address issues related to inheritance slicing and weak_ptr behavior when using py::smart_holder as the holder type.
- Introduces a new overload in load_as_shared_ptr with an extra force flag to control potentially slicing conversions.
- Adds C++ and Python tests to verify expected behavior and error reporting for both shared_ptr conversion functions.
- Updates documentation in cast.h to explain the behavior and potential trade-offs (such as reintroduced inheritance slicing).
Reviewed Changes
Copilot reviewed 4 out of 6 changed files in this pull request and generated 1 comment.
File | Description |
---|---|
tests/test_potentially_slicing_shared_ptr.py | Added comprehensive tests covering shared_ptr conversion behavior. |
tests/test_potentially_slicing_shared_ptr.cpp | Introduced C++ tests for both obj.cast and potentially slicing path. |
include/pybind11/detail/type_caster_base.h | Extended load_as_shared_ptr() to include a force flag parameter. |
include/pybind11/cast.h | Added the potentially_slicing_shared_ptr() function and its docs. |
Files not reviewed (2)
- docs/advanced/classes.rst: Language not supported
- tests/CMakeLists.txt: Language not supported
Comments suppressed due to low confidence (2)
include/pybind11/detail/type_caster_base.h:763
- [nitpick] Consider adding a brief comment explaining the purpose of the 'force_potentially_slicing_shared_ptr' parameter to clarify its effect on alias handling.
bool force_potentially_slicing_shared_ptr = false) const {
tests/test_potentially_slicing_shared_ptr.py:163
- [nitpick] Using exact string equality for error message checks can be brittle; consider verifying that the error message contains an expected substring rather than matching it exactly.
assert str(excinfo.value) == ( ...
@@ -983,6 +983,16 @@ struct copyable_holder_caster< | |||
return shared_ptr_storage; | |||
} | |||
|
|||
std::shared_ptr<type> &potentially_slicing_shared_ptr() { | |||
if (typeinfo->holder_enum_v == detail::holder_enum_t::smart_holder) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] A short inline comment explaining why the call to load_as_shared_ptr uses force_potentially_slicing_shared_ptr=true would help maintain clarity for future readers.
if (typeinfo->holder_enum_v == detail::holder_enum_t::smart_holder) { | |
if (typeinfo->holder_enum_v == detail::holder_enum_t::smart_holder) { | |
// Use force_potentially_slicing_shared_ptr=true to handle cases where slicing | |
// might occur, ensuring the shared_ptr is created even if the type is not exact. |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done: // Reusing shared_ptr code to minimize code complexity.
By the way, feel free to ignore the co-pilot review, was just curious, and I've had it catch a bug before elsewhere. |
…e) as the public API. For CI testing, before changing the names around more widely, and the documentation.
…slicing_weak_ptr.cpp,py
…cs/advanced/classes.rst
py::potentially_slicing_shared_ptr(handle)
functionpy::potentially_slicing_weak_ptr(handle)
function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces a new function, py::potentially_slicing_weak_ptr(handle), which provides an alternative way to obtain a std::weak_ptr from a Python object while addressing issues related to inheritance slicing when using py::smart_holder.
- Added new tests in Python and C++ to validate both correct behavior and error cases associated with potentially slicing weak pointers.
- Enhanced the underlying type caster implementations in include/pybind11/detail/type_caster_base.h and include/pybind11/cast.h to support the new functionality.
- Updated the documentation to explain the differences in lifetime management between std::shared_ptr and py::smart_holder, and the trade-offs when using potentially slicing weak pointers.
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
tests/test_potentially_slicing_weak_ptr.py | New tests to verify shared and weak pointer behavior. |
tests/test_potentially_slicing_weak_ptr.cpp | Adds C++ test support for the new slicing weak pointer function. |
tests/CMakeLists.txt | Registers the new test suite. |
include/pybind11/detail/type_caster_base.h | Modified load_as_shared_ptr() to accept a force flag for slicing. |
include/pybind11/cast.h | Introduced the potentially_slicing_weak_ptr() API and documentation. |
docs/advanced/classes.rst | Adds a documentation section to explain the new weak_ptr behavior. |
@@ -774,7 +776,7 @@ struct load_helper : value_and_holder_helper { | |||
throw std::runtime_error("Non-owning holder (load_as_shared_ptr)."); | |||
} | |||
auto *type_raw_ptr = static_cast<T *>(void_raw_ptr); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider adding a comment here to explain the purpose of the new parameter 'force_potentially_slicing_shared_ptr' and how it affects the alias check, so future maintainers understand why this conditional branch bypasses the slicing protection.
auto *type_raw_ptr = static_cast<T *>(void_raw_ptr); | |
auto *type_raw_ptr = static_cast<T *>(void_raw_ptr); | |
// The `force_potentially_slicing_shared_ptr` parameter allows bypassing the slicing | |
// protection mechanism when `python_instance_is_alias` is true. This is necessary | |
// in scenarios where the caller explicitly requests a shared_ptr that may result | |
// in slicing, typically for advanced use cases or performance optimizations. | |
// Without this flag, the function enforces strict aliasing rules to prevent | |
// potential slicing issues. |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I chose this less verbose, and less redundant, alternative: commit b349e84
…ase.h, to make a direct connection py::potentially_slicing_weak_ptr
Description
Closes #5623.
Please see the added documentation and source code comments for more information.
For quick access, this is the new section in the documentation: