[WIP] Towards a solution for custom-type rvalue arguments

include/pybind11/cast.h

@@ -50,14 +50,15 @@ PYBIND11_NAMESPACE_BEGIN(detail)
 template <typename type, typename SFINAE = void> class type_caster : public type_caster_base<type> { };
 template <typename type> using make_caster = type_caster<intrinsic_t<type>>;
 
-// Shortcut for calling a caster's `cast_op_type` cast operator for casting a type_caster to a T
+// Shortcuts for calling a caster's `cast_op_type` cast operator for casting a type_caster to a T
+// cast_op operating on an lvalue-reference caster, enables moving only if required by the actual function argument
 template <typename T> typename make_caster<T>::template cast_op_type<T> cast_op(make_caster<T> &caster) {
     return caster.operator typename make_caster<T>::template cast_op_type<T>();
 }
+// cast_op operating on an rvalue-referenced caster enforces an rvalue-reference for the cast_op type as well
 template <typename T> typename make_caster<T>::template cast_op_type<typename std::add_rvalue_reference<T>::type>
 cast_op(make_caster<T> &&caster) {
-    return std::move(caster).operator
-        typename make_caster<T>::template cast_op_type<typename std::add_rvalue_reference<T>::type>();
+    return caster.operator typename make_caster<T>::template cast_op_type<typename std::add_rvalue_reference<T>::type>();
 }
 
 template <typename type> class type_caster<std::reference_wrapper<type>> {
@@ -101,7 +102,7 @@ template <typename type> class type_caster<std::reference_wrapper<type>> {
         } \
         operator type*() { return &value; } \
         operator type&() { return value; } \
-        operator type&&() && { return std::move(value); } \
+        operator type&&() { return std::move(value); } \
         template <typename T_> using cast_op_type = pybind11::detail::movable_cast_op_type<T_>
 
 
@@ -644,6 +645,10 @@ struct copyable_holder_caster : public type_caster_base<type> {
     // static_cast works around compiler error with MSVC 17 and CUDA 10.2
     // see issue #2180
     explicit operator type&() { return *(static_cast<type *>(this->value)); }
+
+    // holders only support copying, not moving
+    template <typename T> using cast_op_type = detail::cast_op_type<T>;
+
     explicit operator holder_type*() { return std::addressof(holder); }
     explicit operator holder_type&() { return holder; }
 
@@ -791,8 +796,8 @@ class type_caster<T, enable_if_t<is_pyobject<T>::value>> : public pyobject_caste
 // - type_caster<T>::operator T&() must exist
 // - the type must be move constructible (obviously)
 // At run-time:
-// - if the type is non-copy-constructible, the object must be the sole owner of the type (i.e. it
-//   must have ref_count() == 1)h
+// - if the type is non-copy-constructible, the object must be the sole owner of the type
+//   (i.e. it must have ref_count() == 1)
 // If any of the above are not satisfied, we fall back to copying.
 template <typename T> using move_is_plain_type = satisfies_none_of<T,
     std::is_void, std::is_pointer, std::is_reference, std::is_const
@@ -851,12 +856,6 @@ template <typename T, typename SFINAE> type_caster<T, SFINAE> &load_type(type_ca
     }
     return conv;
 }
-// Wrapper around the above that also constructs and returns a type_caster
-template <typename T> make_caster<T> load_type(const handle &handle) {
-    make_caster<T> conv;
-    load_type(conv, handle);
-    return conv;
-}
 
 PYBIND11_NAMESPACE_END(detail)
 
@@ -866,7 +865,9 @@ T cast(const handle &handle) {
     using namespace detail;
     static_assert(!cast_is_temporary_value_reference<T>::value,
             "Unable to cast type to reference: value is local to type caster");
-    return cast_op<T>(load_type<T>(handle));
+    make_caster<T> conv;
+    load_type(conv, handle);
+    return cast_op<T>(conv);
 }
 
 // pytype -> pytype (calls converting constructor)
@@ -902,7 +903,9 @@ detail::enable_if_t<!detail::move_never<T>::value, T> move(object &&obj) {
 #endif
 
     // Move into a temporary and return that, because the reference may be a local value of `conv`
-    T ret = std::move(detail::load_type<T>(obj).operator T&());
+    detail::make_caster<T> conv;
+    load_type(conv, obj);
+    T ret = std::move(conv.operator T &());
     return ret;
 }
 
@@ -1161,7 +1164,7 @@ class argument_loader {
 
     template <typename Return, typename Func, size_t... Is, typename Guard>
     Return call_impl(Func &&f, index_sequence<Is...>, Guard &&) && {
-        return std::forward<Func>(f)(cast_op<Args>(std::move(std::get<Is>(argcasters)))...);
+        return std::forward<Func>(f)(cast_op<Args>(std::get<Is>(argcasters))...);
     }
 
     std::tuple<make_caster<Args>...> argcasters;

include/pybind11/detail/type_caster_base.h

@@ -760,29 +760,44 @@ class type_caster_generic {
  * Determine suitable casting operator for pointer-or-lvalue-casting type casters.  The type caster
  * needs to provide `operator T*()` and `operator T&()` operators.
  *
- * If the type supports moving the value away via an `operator T&&() &&` method, it should use
- * `movable_cast_op_type` instead.
+ * If the type supports moving, it should use `movable_cast_op_type` instead.
  */
 template <typename T>
 using cast_op_type =
     conditional_t<std::is_pointer<remove_reference_t<T>>::value,
         typename std::add_pointer<intrinsic_t<T>>::type,
         typename std::add_lvalue_reference<intrinsic_t<T>>::type>;
 
+// A by-value argument will use lvalue references by default (triggering copy construction)
+// except the type is move-only and thus should use move semantics.
+template <typename T> using cast_op_type_for_byvalue =
+    conditional_t<std::is_copy_constructible<intrinsic_t<T>>::value,
+        typename std::add_lvalue_reference<intrinsic_t<T>>::type,
+        typename std::add_rvalue_reference<intrinsic_t<T>>::type >;
+
 /**
- * Determine suitable casting operator for a type caster with a movable value.  Such a type caster
- * needs to provide `operator T*()`, `operator T&()`, and `operator T&&() &&`.  The latter will be
- * called in appropriate contexts where the value can be moved rather than copied.
+ * Determine suitable casting operator for movable types.
+ * While C++ decides about copy vs. move semantics based on the type of the passed argument,
+ * Python doesn't know about lvalue and rvalue references and thus cannot decide based on the
+ * passed argument. Instead, the wrapper needs to define the desired semantics of argument passing
+ * explicitly:
+ * - By specifying an rvalue reference (T&&), move semantics is enforced.
+ * - By default, lvalue references (T&) are passed, triggering copy semantics if necessary.
+ *   An exception are move-only types: These are passed via move semantics.
  *
- * These operator are automatically provided when using the PYBIND11_TYPE_CASTER macro.
+ * The corresponding caster needs to provide the following operators:
+ * `operator T*()`, `operator T&()`, and `operator T&&()`.
+ * These operators are automatically provided when using the PYBIND11_TYPE_CASTER macro.
  */
 template <typename T>
 using movable_cast_op_type =
     conditional_t<std::is_pointer<typename std::remove_reference<T>::type>::value,
         typename std::add_pointer<intrinsic_t<T>>::type,
-    conditional_t<std::is_rvalue_reference<T>::value,
-        typename std::add_rvalue_reference<intrinsic_t<T>>::type,
-        typename std::add_lvalue_reference<intrinsic_t<T>>::type>>;
+        conditional_t<std::is_rvalue_reference<T>::value,
+            typename std::add_rvalue_reference<intrinsic_t<T>>::type,
+            conditional_t<std::is_lvalue_reference<T>::value,
+                typename std::add_lvalue_reference<intrinsic_t<T>>::type,
+                cast_op_type_for_byvalue<T> > > >;
 
 // std::is_copy_constructible isn't quite enough: it lets std::vector<T> (and similar) through when
 // T is non-copyable, but code containing such a copy constructor fails to actually compile.
@@ -912,10 +927,12 @@ template <typename type> class type_caster_base : public type_caster_generic {
             nullptr, nullptr, holder);
     }
 
-    template <typename T> using cast_op_type = detail::cast_op_type<T>;
+    // allow moving of custom types
+    template <typename T> using cast_op_type = detail::movable_cast_op_type<T>;
 
     operator itype*() { return (type *) value; }
     operator itype&() { if (!value) throw reference_cast_error(); return *((itype *) value); }
+    operator itype&&() { if (!value) throw reference_cast_error(); return std::move(*((itype *) value)); }
 
 protected:
     using Constructor = void *(*)(const void *);

include/pybind11/eigen.h

@@ -346,7 +346,7 @@ struct type_caster<Type, enable_if_t<is_eigen_dense_plain<Type>::value>> {
 
     operator Type*() { return &value; }
     operator Type&() { return value; }
-    operator Type&&() && { return std::move(value); }
+    operator Type&&() { return std::move(value); }
     template <typename T> using cast_op_type = movable_cast_op_type<T>;
 
 private:

tests/test_class.cpp

@@ -421,6 +421,7 @@ TEST_SUBMODULE(class_, m) {
     // test_exception_rvalue_abort
     struct PyPrintDestructor {
         PyPrintDestructor() = default;
+        PyPrintDestructor(const PyPrintDestructor&) = default;
         ~PyPrintDestructor() {
             py::print("Print from destructor");
         }

tests/test_copy_move.cpp

@@ -34,40 +34,38 @@ struct lacking_move_ctor : public empty<lacking_move_ctor> {
 template <> lacking_move_ctor empty<lacking_move_ctor>::instance_ = {};
 
 /* Custom type caster move/copy test classes */
-class MoveOnlyInt {
-public:
-    MoveOnlyInt() { print_default_created(this); }
-    MoveOnlyInt(int v) : value{std::move(v)} { print_created(this, value); }
-    MoveOnlyInt(MoveOnlyInt &&m) { print_move_created(this, m.value); std::swap(value, m.value); }
+struct MoveOnlyInt {
+    MoveOnlyInt() : value{42} { print_default_created(this); }
+    MoveOnlyInt(int v) : value{v} { print_created(this, value); }
+    MoveOnlyInt(MoveOnlyInt &&m) : value{-1} { print_move_created(this, m.value); std::swap(value, m.value); }
     MoveOnlyInt &operator=(MoveOnlyInt &&m) { print_move_assigned(this, m.value); std::swap(value, m.value); return *this; }
     MoveOnlyInt(const MoveOnlyInt &) = delete;
     MoveOnlyInt &operator=(const MoveOnlyInt &) = delete;
     ~MoveOnlyInt() { print_destroyed(this); }
 
     int value;
 };
-class MoveOrCopyInt {
-public:
-    MoveOrCopyInt() { print_default_created(this); }
-    MoveOrCopyInt(int v) : value{std::move(v)} { print_created(this, value); }
-    MoveOrCopyInt(MoveOrCopyInt &&m) { print_move_created(this, m.value); std::swap(value, m.value); }
+struct MoveOrCopyInt {
+    MoveOrCopyInt() : value{42} { print_default_created(this); }
+    MoveOrCopyInt(int v) : value{v} { print_created(this, value); }
+    MoveOrCopyInt(MoveOrCopyInt &&m) : value{-1} { print_move_created(this, m.value); std::swap(value, m.value); }
     MoveOrCopyInt &operator=(MoveOrCopyInt &&m) { print_move_assigned(this, m.value); std::swap(value, m.value); return *this; }
     MoveOrCopyInt(const MoveOrCopyInt &c) { print_copy_created(this, c.value); value = c.value; }
     MoveOrCopyInt &operator=(const MoveOrCopyInt &c) { print_copy_assigned(this, c.value); value = c.value; return *this; }
     ~MoveOrCopyInt() { print_destroyed(this); }
 
     int value;
 };
-class CopyOnlyInt {
-public:
-    CopyOnlyInt() { print_default_created(this); }
-    CopyOnlyInt(int v) : value{std::move(v)} { print_created(this, value); }
+struct CopyOnlyInt {
+    CopyOnlyInt() : value{42} { print_default_created(this); }
+    CopyOnlyInt(int v) : value{v} { print_created(this, value); }
     CopyOnlyInt(const CopyOnlyInt &c) { print_copy_created(this, c.value); value = c.value; }
     CopyOnlyInt &operator=(const CopyOnlyInt &c) { print_copy_assigned(this, c.value); value = c.value; return *this; }
     ~CopyOnlyInt() { print_destroyed(this); }
 
     int value;
 };
+
 PYBIND11_NAMESPACE_BEGIN(pybind11)
 PYBIND11_NAMESPACE_BEGIN(detail)
 template <> struct type_caster<MoveOnlyInt> {
@@ -113,10 +111,10 @@ TEST_SUBMODULE(copy_move_policies, m) {
     // test_move_and_copy_casts
     m.def("move_and_copy_casts", [](py::object o) {
         int r = 0;
-        r += py::cast<MoveOrCopyInt>(o).value; /* moves */
+        r += py::cast<MoveOrCopyInt>(o).value; /* copies */
         r += py::cast<MoveOnlyInt>(o).value; /* moves */
         r += py::cast<CopyOnlyInt>(o).value; /* copies */
-        auto m1(py::cast<MoveOrCopyInt>(o)); /* moves */
+        auto m1(py::cast<MoveOrCopyInt>(o)); /* copies */
         auto m2(py::cast<MoveOnlyInt>(o)); /* moves */
         auto m3(py::cast<CopyOnlyInt>(o)); /* copies */
         r += m1.value + m2.value + m3.value;
@@ -126,7 +124,8 @@ TEST_SUBMODULE(copy_move_policies, m) {
 
     // test_move_and_copy_loads
     m.def("move_only", [](MoveOnlyInt m) { return m.value; });
-    m.def("move_or_copy", [](MoveOrCopyInt m) { return m.value; });
+    m.def("move", [](MoveOrCopyInt&& m) { return m.value; });
+    m.def("copy", [](MoveOrCopyInt m) { return m.value; });
     m.def("copy_only", [](CopyOnlyInt m) { return m.value; });
     m.def("move_pair", [](std::pair<MoveOnlyInt, MoveOrCopyInt> p) {
         return p.first.value + p.second.value;
@@ -156,18 +155,40 @@ TEST_SUBMODULE(copy_move_policies, m) {
         d["CopyOnlyInt"] = py::cast(co, py::return_value_policy::reference);
         return d;
     });
+
+    // test_move_copy_class_loads: similar tests as above, but with types exposed to python
+    struct MoveOnlyIntC : MoveOnlyInt { using MoveOnlyInt::MoveOnlyInt; };
+    py::class_<MoveOnlyIntC>(m, "MoveOnlyInt")
+        .def(py::init<int>())
+        .def_readonly("value", &MoveOnlyIntC::value)
+        .def_static("val", [](MoveOnlyIntC v) { return v.value; })
+        .def_static("rref", [](MoveOnlyIntC&& v) { MoveOnlyIntC sink(std::move(v)); return sink.value; })
+        .def_static("lref", [](const MoveOnlyIntC& v) { return v.value; })
+        ;
+    struct MoveOrCopyIntC : MoveOrCopyInt { using MoveOrCopyInt::MoveOrCopyInt; };
+    py::class_<MoveOrCopyIntC>(m, "MoveOrCopyInt")
+        .def(py::init<int>())
+        .def_readonly("value", &MoveOrCopyIntC::value)
+        .def_static("val", [](MoveOrCopyIntC v) { return v.value; })
+        .def_static("rref", [](MoveOrCopyIntC&& v) {  MoveOrCopyIntC sink(std::move(v)); return sink.value; })
+        .def_static("lref", [](const MoveOrCopyIntC& v) { return v.value; })
+        ;
+    struct CopyOnlyIntC : CopyOnlyInt { using CopyOnlyInt::CopyOnlyInt; };
+    py::class_<CopyOnlyIntC>(m, "CopyOnlyInt")
+        .def(py::init<int>())
+        .def_readonly("value", &CopyOnlyIntC::value)
+        .def_static("val", [](CopyOnlyIntC v) { return v.value; })
+        .def_static("rref", [](CopyOnlyIntC&& v) {  CopyOnlyIntC sink(std::move(v)); return sink.value; })
+        .def_static("lref", [](const CopyOnlyIntC& v) { return v.value; })
+        ;
+
 #ifdef PYBIND11_HAS_OPTIONAL
     // test_move_and_copy_load_optional
     m.attr("has_optional") = true;
-    m.def("move_optional", [](std::optional<MoveOnlyInt> o) {
-        return o->value;
-    });
-    m.def("move_or_copy_optional", [](std::optional<MoveOrCopyInt> o) {
-        return o->value;
-    });
-    m.def("copy_optional", [](std::optional<CopyOnlyInt> o) {
-        return o->value;
-    });
+    m.def("move_optional", [](std::optional<MoveOnlyInt> o) { return o->value; });
+    m.def("mc_move_optional", [](std::optional<MoveOrCopyInt>&& o) { return o->value; });
+    m.def("mc_copy_optional", [](std::optional<MoveOrCopyInt> o) { return o->value; });
+    m.def("copy_optional", [](std::optional<CopyOnlyInt> o) { return o->value; });
     m.def("move_optional_tuple", [](std::optional<std::tuple<MoveOrCopyInt, MoveOnlyInt, CopyOnlyInt>> x) {
         return std::get<0>(*x).value + std::get<1>(*x).value + std::get<2>(*x).value;
     });