[BUG]: Custom SharedPtr<T> usually should not be required to have a copy constructible T.

[andre-caldas]

Required prerequisites

• Make sure you've read the documentation. Your issue may be addressed there.
• Search the issue tracker and Discussions to verify that this hasn't already been reported. +1 or comment there if it has.
• Consider asking first in the Gitter chat room or in a Discussion.

What version (or hash if on master) of pybind11 are you using?

https://github.com/pybind/pybind11/archive/refs/tags/v2.13.5.tar.gz

Problem description

I have a custom SharedPtr that is basically an std::shared_ptr with some extra functionalities. It actually derives privately from std::shared_ptr.

My custom SharedPtr<T> is declared as a holder type.

PYBIND11_DECLARE_HOLDER_TYPE(T, SharedPtr<T>);

However, when T is not copy-constructible, I get compilation errors when using it: (error happens in "populate" binding)

void init_scene(py::module_& m)
{
  py::class_<SceneRoot, SharedPtr<SceneRoot>>(
      m, "Scene",
      "A scene graph with a message queue that keeps it updated.")
      .def(py::init(&new_scene),
           "Creates an empty scene and associates it to an OGRE SceneManager.")
      .def("populate",
           [](SharedPtr<SceneRoot> self, std::shared_ptr<DocumentTree> doc)
           {self->populate(std::move(self), std::move(doc));},
           "document"_a,
           "Populates the scene with the contents of 'document'.")
      .def("__repr__",
           [](const SceneRoot& s){ return "<SCENE... (put info here)>"; });
}

This happens because since SceneRoot is not copy-constructible (we do not want to inadvertently copy this huge structure!), type_caster<SharedPtr<T>> inherits from move_only_holder_caster. See

pybind11/include/pybind11/cast.h

Lines 862 to 864 in f7e14e9

	using type_caster_holder = conditional_t<is_copy_constructible<holder_type>::value,
	copyable_holder_caster<type, holder_type>,
	move_only_holder_caster<type, holder_type>>;

I do not really know much about use cases, but I do believe this is not correct. In pybind11 we have a custom is_copy_constructible<> trait because pre C++17 stl libraries sometimes report non-copy constructible as copy constructible but later give a compilation error. See commit 7937260. But in case of custom holders, using pybind's own is_copy_constructible is not correct IMO. I have discussed this (#5142) with myself and we both have agreed. :-)

I have worked around the issue with this:

/*
 * A SharedPtr<T> is copy constructible even if T is not!
 * So, PYBIND11_DECLARE_HOLDER_TYPE does not work. :-(
 */
//PYBIND11_DECLARE_HOLDER_TYPE(T, SharedPtr<T>);
PYBIND11_NAMESPACE_BEGIN(PYBIND11_NAMESPACE)
namespace detail {
  template <typename T>
  class type_caster<SharedPtr<T>>
      : public copyable_holder_caster<T, SharedPtr<T>> {};
  template <typename T>
  struct is_holder_type<T, SharedPtr<T>> : std::true_type {};
}
PYBIND11_NAMESPACE_END(PYBIND11_NAMESPACE)

Notice that I also had to declare is_holder_type<T, SharedPtr<T>>.

If I am not missing anything, type_caster_holder should use std::is_copy_constructible instead of the recursive pybind11 version.

This would enable std::shared_ptr to not be treated as a special case here

pybind11/include/pybind11/cast.h

Line 879 in f7e14e9

class type_caster<holder_type, enable_if_t<!is_shared_ptr<holder_type>::value>> \

and here

pybind11/include/pybind11/cast.h

Line 840 in f7e14e9

class type_caster<std::shared_ptr<T>> : public copyable_holder_caster<T, std::shared_ptr<T>> {};

Side Note

I do not understand how std::shared_ptr deals with the fact that is_holder_type<> is IMO supposed to fail (but of course it doesn't!) when T is not copy-constructible:

pybind11/include/pybind11/cast.h

Lines 886 to 887 in f7e14e9

	struct is_holder_type
	: std::is_base_of<detail::type_caster_holder<base, holder>, detail::type_caster<holder>> {};

Reproducible example code

No response

Is this a regression? Put the last known working version here if it is.

Not a regression