Improve custom holder support (#607)

* Abstract away some holder functionality (resolve #585)

Custom holder types which don't have `.get()` can select the correct
function to call by specializing `holder_traits`.

* Add support for move-only holders (fix #605)

docs/advanced/smart_ptrs.rst

@@ -149,6 +149,27 @@ situation where ``true`` should be passed is when the ``T`` instances use
 
 Please take a look at the :ref:`macro_notes` before using this feature.
 
+By default, pybind11 assumes that your custom smart pointer has a standard
+interface, i.e. provides a ``.get()`` member function to access the underlying
+raw pointer. If this is not the case, pybind11's ``holder_helper`` must be
+specialized:
+
+.. code-block:: cpp
+
+    // Always needed for custom holder types
+    PYBIND11_DECLARE_HOLDER_TYPE(T, SmartPtr<T>);
+
+    // Only needed if the type's `.get()` goes by another name
+    namespace pybind11 { namespace detail {
+        template <typename T>
+        struct holder_helper<SmartPtr<T>> { // <-- specialization
+            static const T *get(const SmartPtr<T> &p) { return p.getPointer(); }
+        };
+    }}
+
+The above specialization informs pybind11 that the custom ``SmartPtr`` class
+provides ``.get()`` functionality via ``.getPointer()``.
+
 .. seealso::
 
     The file :file:`tests/test_smart_ptr.cpp` contains a complete example

include/pybind11/cast.h

@@ -402,6 +402,13 @@ public:
             make_copy_constructor(src), make_move_constructor(src));
     }
 
+    static handle cast_holder(const itype *src, const void *holder) {
+        return type_caster_generic::cast(
+            src, return_value_policy::take_ownership, {},
+            src ? &typeid(*src) : nullptr, &typeid(type),
+            nullptr, nullptr, holder);
+    }
+
     template <typename T> using cast_op_type = pybind11::detail::cast_op_type<T>;
 
     operator itype*() { return (type *) value; }
@@ -636,17 +643,6 @@ protected:
     bool success = false;
 };
 
-template <typename type, typename deleter> class type_caster<std::unique_ptr<type, deleter>> {
-public:
-    static handle cast(std::unique_ptr<type, deleter> &&src, return_value_policy policy, handle parent) {
-        handle result = type_caster_base<type>::cast(src.get(), policy, parent);
-        if (result)
-            src.release();
-        return result;
-    }
-    static PYBIND11_DESCR name() { return type_caster_base<type>::name(); }
-};
-
 template <> class type_caster<std::wstring> {
 public:
     bool load(handle src, bool) {
@@ -837,8 +833,16 @@ protected:
     std::tuple<make_caster<Tuple>...> value;
 };
 
+/// Helper class which abstracts away certain actions. Users can provide specializations for
+/// custom holders, but it's only necessary if the type has a non-standard interface.
+template <typename T>
+struct holder_helper {
+    static auto get(const T &p) -> decltype(p.get()) { return p.get(); }
+};
+
 /// Type caster for holder types like std::shared_ptr, etc.
-template <typename type, typename holder_type> class type_caster_holder : public type_caster_base<type> {
+template <typename type, typename holder_type>
+struct copyable_holder_caster : public type_caster_base<type> {
 public:
     using base = type_caster_base<type>;
     using base::base;
@@ -917,7 +921,7 @@ public:
     template <typename T = holder_type, detail::enable_if_t<std::is_constructible<T, const T &, type*>::value, int> = 0>
     bool try_implicit_casts(handle src, bool convert) {
         for (auto &cast : typeinfo->implicit_casts) {
-            type_caster_holder sub_caster(*cast.first);
+            copyable_holder_caster sub_caster(*cast.first);
             if (sub_caster.load(src, convert)) {
                 value = cast.second(sub_caster.value);
                 holder = holder_type(sub_caster.holder, (type *) value);
@@ -940,10 +944,8 @@ public:
     #endif
 
     static handle cast(const holder_type &src, return_value_policy, handle) {
-        return type_caster_generic::cast(
+        const auto *ptr = holder_helper<holder_type>::get(src);
-            src.get(), return_value_policy::take_ownership, handle(),
+        return type_caster_base<type>::cast_holder(ptr, &src);
-            src.get() ? &typeid(*src.get()) : nullptr, &typeid(type),
-            nullptr, nullptr, &src);
     }
 
 protected:
@@ -952,7 +954,25 @@ protected:
 
 /// Specialize for the common std::shared_ptr, so users don't need to
 template <typename T>
-class type_caster<std::shared_ptr<T>> : public type_caster_holder<T, std::shared_ptr<T>> { };
+class type_caster<std::shared_ptr<T>> : public copyable_holder_caster<T, std::shared_ptr<T>> { };
+
+template <typename type, typename holder_type>
+struct move_only_holder_caster {
+    static handle cast(holder_type &&src, return_value_policy, handle) {
+        auto *ptr = holder_helper<holder_type>::get(src);
+        return type_caster_base<type>::cast_holder(ptr, &src);
+    }
+    static PYBIND11_DESCR name() { return type_caster_base<type>::name(); }
+};
+
+template <typename type, typename deleter>
+class type_caster<std::unique_ptr<type, deleter>>
+    : public move_only_holder_caster<type, std::unique_ptr<type, deleter>> { };
+
+template <typename type, typename holder_type>
+using type_caster_holder = conditional_t<std::is_copy_constructible<holder_type>::value,
+                                         copyable_holder_caster<type, holder_type>,
+                                         move_only_holder_caster<type, holder_type>>;
 
 template <typename T, bool Value = false> struct always_construct_holder { static constexpr bool value = Value; };
 

include/pybind11/pybind11.h

@@ -1212,24 +1212,22 @@ private:
         }
     }
 
-    /// Initialize holder object, variant 2: try to construct from existing holder object, if possible
+    static void init_holder_from_existing(instance_type *inst, const holder_type *holder_ptr,
-    template <typename T = holder_type,
+                                          std::true_type /*is_copy_constructible*/) {
-              detail::enable_if_t<std::is_copy_constructible<T>::value, int> = 0>
+        new (&inst->holder) holder_type(*holder_ptr);
-    static void init_holder_helper(instance_type *inst, const holder_type *holder_ptr, const void * /* dummy */) {
-        if (holder_ptr) {
-            new (&inst->holder) holder_type(*holder_ptr);
-            inst->holder_constructed = true;
-        } else if (inst->owned || detail::always_construct_holder<holder_type>::value) {
-            new (&inst->holder) holder_type(inst->value);
-            inst->holder_constructed = true;
-        }
     }
 
-    /// Initialize holder object, variant 3: holder is not copy constructible (e.g. unique_ptr), always initialize from raw pointer
+    static void init_holder_from_existing(instance_type *inst, const holder_type *holder_ptr,
-    template <typename T = holder_type,
+                                          std::false_type /*is_copy_constructible*/) {
-              detail::enable_if_t<!std::is_copy_constructible<T>::value, int> = 0>
+        new (&inst->holder) holder_type(std::move(*const_cast<holder_type *>(holder_ptr)));
-    static void init_holder_helper(instance_type *inst, const holder_type * /* unused */, const void * /* dummy */) {
+    }
-        if (inst->owned || detail::always_construct_holder<holder_type>::value) {
+
+    /// Initialize holder object, variant 2: try to construct from existing holder object, if possible
+    static void init_holder_helper(instance_type *inst, const holder_type *holder_ptr, const void * /* dummy */) {
+        if (holder_ptr) {
+            init_holder_from_existing(inst, holder_ptr, std::is_copy_constructible<holder_type>());
+            inst->holder_constructed = true;
+        } else if (inst->owned || detail::always_construct_holder<holder_type>::value) {
             new (&inst->holder) holder_type(inst->value);
             inst->holder_constructed = true;
         }

tests/object.h

@@ -164,10 +164,10 @@ public:
     operator T* () { return m_ptr; }
 
     /// Return a const pointer to the referenced object
-    T* get() { return m_ptr; }
+    T* get_ptr() { return m_ptr; }
 
     /// Return a pointer to the referenced object
-    const T* get() const { return m_ptr; }
+    const T* get_ptr() const { return m_ptr; }
 private:
     T *m_ptr;
 };

tests/test_smart_ptr.cpp

@@ -90,6 +90,14 @@ PYBIND11_DECLARE_HOLDER_TYPE(T, ref<T>, true);
 PYBIND11_DECLARE_HOLDER_TYPE(T, std::shared_ptr<T>); // Not required any more for std::shared_ptr,
                                                      // but it should compile without error
 
+// Make pybind11 aware of the non-standard getter member function
+namespace pybind11 { namespace detail {
+    template <typename T>
+    struct holder_helper<ref<T>> {
+        static const T *get(const ref<T> &p) { return p.get_ptr(); }
+    };
+}}
+
 Object *make_object_1() { return new MyObject1(1); }
 ref<Object> make_object_2() { return new MyObject1(2); }
 
@@ -206,6 +214,18 @@ struct SharedFromThisRef {
     std::shared_ptr<B> shared = std::make_shared<B>();
 };
 
+template <typename T>
+class CustomUniquePtr {
+    std::unique_ptr<T> impl;
+
+public:
+    CustomUniquePtr(T* p) : impl(p) { }
+    T* get() const { return impl.get(); }
+    T* release_ptr() { return impl.release(); }
+};
+
+PYBIND11_DECLARE_HOLDER_TYPE(T, CustomUniquePtr<T>);
+
 test_initializer smart_ptr_and_references([](py::module &pm) {
     auto m = pm.def_submodule("smart_ptr");
 
@@ -237,4 +257,12 @@ test_initializer smart_ptr_and_references([](py::module &pm) {
                                py::return_value_policy::copy)
         .def("set_ref", [](SharedFromThisRef &, const B &) { return true; })
         .def("set_holder", [](SharedFromThisRef &, std::shared_ptr<B>) { return true; });
+
+    struct C {
+        C() { print_created(this); }
+        ~C() { print_destroyed(this); }
+    };
+
+    py::class_<C, CustomUniquePtr<C>>(m, "TypeWithMoveOnlyHolder")
+        .def_static("make", []() { return CustomUniquePtr<C>(new C); });
 });

tests/test_smart_ptr.py

@@ -201,3 +201,13 @@ def test_shared_ptr_from_this_and_references():
 
     del ref, bad_wp, copy, holder_ref, holder_copy, s
     assert stats.alive() == 0
+
+
+def test_move_only_holder():
+    from pybind11_tests.smart_ptr import TypeWithMoveOnlyHolder
+
+    a = TypeWithMoveOnlyHolder.make()
+    stats = ConstructorStats.get(TypeWithMoveOnlyHolder)
+    assert stats.alive() == 1
+    del a
+    assert stats.alive() == 0