pybind11
pybind11 copied to clipboard
[BUG]: pybind11 should not expect there is always a shared_ptr when detecting enable_shared_from_this
Required prerequisites
- [X] Make sure you've read the documentation. Your issue may be addressed there.
- [X] Search the issue tracker and Discussions to verify that this hasn't already been reported. +1 or comment there if it has.
- [X] Consider asking first in the Gitter chat room or in a Discussion.
Problem description
Currently, when pybind11 detects a class inheriting std::enable_shared_from_this
, it asserts users always uses std::shared_ptr
to hold it. Specifically, it checks std::shared_ptr::element_type
at this line:
https://github.com/pybind/pybind11/blob/47079b9e7b81a4b6d93c46a452115d7d8fd3488f/include/pybind11/pybind11.h#L1784
However, even when a class inherits std::enable_shared_from_this
, an object of its does not necessarily be held in a std::shared_ptr
. It can be held by std::shared_ptr
in just some part of the code, and held by other holders when passing to pybind11. In my case, I warped around std::shared_ptr
to build my own holder, which does not have an element_type
member type.
A possible fix is to check the type of holder_type
first before assuming it is a std::shared_ptr
.
Reproducible example code
No response
@roastduck, if you have a fix in mind a PR would be appreciated. @rwgk something to consider here for smart holder casting.
@roastduck, did you already try the smart_holder branch, using classh
for the type(s) that give you trouble on master?
https://github.com/pybind/pybind11/blob/smart_holder/README_smart_holder.rst
A reproducer would be useful (draft PR).
If it doesn't work with the smart_holder branch, I'll look into fixing.
@Skylion007, in my understanding, changing the following declaration
https://github.com/pybind/pybind11/blob/47079b9e7b81a4b6d93c46a452115d7d8fd3488f/include/pybind11/pybind11.h#L1778-L1782
to
template <typename T, typename U, typename V,
typename std::enable_if_t<std::is_base_of_v<std::shared_ptr<V>, U>> * = nullptr>
static void init_holder(detail::instance *inst,
detail::value_and_holder &v_h,
const U * /* unused */,
const std::enable_shared_from_this<T> * /* dummy */) {
will work. But I am not sure how to safely write these template arguments for maximal compatibility.
I am also not sure how to write a test in pybind11, but I come up with a reproducible code, as follows:
#include <pybind11/pybind11.h>
#include <memory>
template <class T>
class Ref {
std::shared_ptr<T> ptr_;
public:
Ref(const std::shared_ptr<T> &ptr) : ptr_(ptr) {}
Ref(T *ptr) : ptr_(ptr) {}
T &operator*() const { return *ptr_; }
T *operator->() const { return ptr_.get(); }
T *get() const { return ptr_.get(); }
};
class A : public std::enable_shared_from_this<A> { // comment this line
// class A {// and uncomment this line and then it compiles
int x_;
};
PYBIND11_DECLARE_HOLDER_TYPE(T, Ref<T>);
PYBIND11_MODULE(my_module, m) {
pybind11::class_<A, Ref<A>>(m, "A").def(pybind11::init<>());
}
It will not compile unless you remove : public std::enable_shared_from_this<A>
.
Could you make a PR like this?
@rwgk, I have not tried smart_holder
yet, because this problem is related to a custom pointer, while the README of smart_holder
says it "does not have a well-lit path for including interoperability with custom smart-pointers".
will work. But I am not sure how to safely write these template arguments for maximal compatibility.
"None of us is" is probably much closer to the truth than "we (the maintainers) are". At least speaking for myself, I usually try-fail-try until I have what I need. Recommended here.
"does not have a well-lit path for including interoperability with custom smart-pointers"
Something that crossed my mind, but it's just a wild idea atm, is to convert the custom smart-pointer to a shared_ptr with custom deleter, from there the smart_holder may work as-is. — Do you have a pointer to your code?