Towards a solution for rvalue holder arguments in wrapped functions

docs/compiling.rst

@@ -287,3 +287,11 @@ code by introspecting existing C++ codebases using LLVM/Clang. See the
 [binder]_ documentation for details.
 
 .. [binder] http://cppbinder.readthedocs.io/en/latest/about.html
+
+[AutoWIG]_ is a Python library that wraps automatically compiled libraries into
+high-level languages. It parses C++ code using LLVM/Clang technologies and
+generates the wrappers using the Mako templating engine. The approach is automatic,
+extensible, and applies to very complex C++ libraries, composed of thousands of
+classes or incorporating modern meta-programming constructs.
+
+.. [AutoWIG] https://github.com/StatisKit/AutoWIG

include/pybind11/cast.h

@@ -567,23 +567,9 @@ class type_caster_generic {
 
     // Base methods for generic caster; there are overridden in copyable_holder_caster
     void load_value(value_and_holder &&v_h) {
-        auto *&vptr = v_h.value_ptr();
-        // Lazy allocation for unallocated values:
-        if (vptr == nullptr) {
-            auto *type = v_h.type ? v_h.type : typeinfo;
-            if (type->operator_new) {
-                vptr = type->operator_new(type->type_size);
-            } else {
-                #if defined(PYBIND11_CPP17)
-                    if (type->type_align > __STDCPP_DEFAULT_NEW_ALIGNMENT__)
-                        vptr = ::operator new(type->type_size,
-                                              (std::align_val_t) type->type_align);
-                    else
-                #endif
-                vptr = ::operator new(type->type_size);
-            }
-        }
-        value = vptr;
+        value = v_h.value_ptr();
+        if (value == nullptr)
+            throw cast_error("Invalid object instance");
     }
     bool try_implicit_casts(handle src, bool convert) {
         for (auto &cast : typeinfo->implicit_casts) {
@@ -1515,15 +1501,81 @@ template <typename 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_assert(std::is_base_of<type_caster_base<type>, type_caster<type>>::value,
+struct move_only_holder_caster : public type_caster_base<type> {
+public:
+    using base = type_caster_base<type>;
+    static_assert(std::is_base_of<base, type_caster<type>>::value,
             "Holder classes are only supported for custom types");
+    using base::base;
+    using base::cast;
+    using base::typeinfo;
+    using base::value;
 
-    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, std::addressof(src));
+    bool load(handle& src, bool convert) {
+        bool success = base::template load_impl<move_only_holder_caster<type, holder_type>>(src, convert);
+        if (success) // On success, remember src pointer to withdraw later
+            this->v_h = reinterpret_cast<instance *>(src.ptr())->get_value_and_holder();
+        return success;
     }
-    static constexpr auto name = type_caster_base<type>::name;
+
+    template <typename T> using cast_op_type = detail::movable_cast_op_type<T>;
+
+    // Workaround for Intel compiler bug
+    // see pybind11 issue 94
+    #if !defined(__ICC) && !defined(__INTEL_COMPILER)
+    explicit
+    #endif
+    operator holder_type&&() {
+        // In load_value() value_ptr was still valid. If it's invalid now, another argument consumed the same value before.
+        if (!v_h.value_ptr())
+            throw cast_error("Multiple access to moved argument");
+        v_h.value_ptr() = nullptr;
+        // TODO: release object instance in python
+        // clear_instance(src_handle->ptr()); ???
+
+        return std::move(*holder_ptr);
+    }
+
+    static handle cast(const holder_type &src, return_value_policy, handle) {
+        const auto *ptr = holder_helper<holder_type>::get(src);
+        return type_caster_base<type>::cast_holder(ptr, &src);
+    }
+
+protected:
+    friend class type_caster_generic;
+    void check_holder_compat() {
+//        if (typeinfo->default_holder)
+//            throw cast_error("Unable to load a custom holder type from a default-holder instance");
+    }
+
+    bool load_value(value_and_holder &&v_h) {
+        if (v_h.holder_constructed()) {
+            holder_ptr = std::addressof(v_h.template holder<holder_type>());
+            value = const_cast<void*>(reinterpret_cast<const void*>(holder_helper<holder_type>::get(*holder_ptr)));
+            if (!value)
+                throw cast_error("Invalid object instance");
+            return true;
+        } else {
+            throw cast_error("Unable to cast from non-held to held instance (T& to Holder<T>) "
+#if defined(NDEBUG)
+                             "(compile in debug mode for type information)");
+#else
+                             "of type '" + type_id<holder_type>() + "''");
+#endif
+        }
+    }
+
+    template <typename T = holder_type, detail::enable_if_t<!std::is_constructible<T, const T &, type*>::value, int> = 0>
+    bool try_implicit_casts(handle, bool) { return false; }
+
+    template <typename T = holder_type, detail::enable_if_t<std::is_constructible<T, const T &, type*>::value, int> = 0>
+    bool try_implicit_casts(handle, bool) { return false; }
+
+    static bool try_direct_conversions(handle) { return false; }
+
+
+    holder_type* holder_ptr = nullptr;
+    value_and_holder v_h;
 };
 
 template <typename type, typename deleter>
@@ -1924,6 +1976,8 @@ class argument_loader {
 
     template <size_t... Is>
     bool load_impl_sequence(function_call &call, index_sequence<Is...>) {
+        // BUG: When loading the arguments, the actual argument type (pointer, lvalue reference, rvalue reference)
+        // is already lost (argcasters only know the intrinsic type), while the behaviour should differ for them, e.g. for rvalue references.
         for (bool r : {std::get<Is>(argcasters).load(call.args[Is], call.args_convert[Is])...})
             if (!r)
                 return false;

tests/CMakeLists.txt

@@ -151,7 +151,7 @@ function(pybind11_enable_warnings target_name)
   endif()
 endfunction()
 
-set(test_targets pybind11_tests)
+set(test_targets pybind11_tests test_move_arg)
 
 # Build pybind11_cross_module_tests if any test_whatever.py are being built that require it
 foreach(t ${PYBIND11_CROSS_MODULE_TESTS})

tests/test_move_arg.cpp

@@ -0,0 +1,66 @@
+#include "test_move_arg.h"
+#include <pybind11/pybind11.h>
+#include <pybind11/iostream.h>
+#include <sstream>
+
+namespace py = pybind11;
+
+#if 1
+template <typename T>
+class my_ptr {
+public:
+	my_ptr(T* p = nullptr) : ptr_(p) {}
+	my_ptr(my_ptr<T>&& other) : ptr_(other.ptr_) { other.ptr_ = nullptr; }
+	~my_ptr() { delete ptr_; }
+	my_ptr<T>& operator=(my_ptr<T>&& other) { ptr_ = other.ptr_; other.ptr_ = nullptr; return *this; }
+	const T* get() const { return ptr_; }
+	const T* verbose_get() const {
+		std::cout << " [" << ptr_ << "] "; return ptr_;
+	}
+private:
+	T* ptr_;
+};
+PYBIND11_DECLARE_HOLDER_TYPE(T, my_ptr<T>)
+namespace pybind11 { namespace detail {
+    template <typename T>
+    struct holder_helper<my_ptr<T>> { // <-- specialization
+        static const T *get(const my_ptr<T> &p) { return p.verbose_get(); }
+    };
+}}
+#else
+template <typename T>
+using my_ptr = std::unique_ptr<T>;
+#endif
+
+PYBIND11_MODULE(test_move_arg, m) {
+	py::class_<Item, my_ptr<Item>>(m, "Item")
+		.def(py::init<int>(), py::call_guard<py::scoped_ostream_redirect>())
+		.def("__repr__", [](const Item& item) {
+			std::stringstream ss;
+			ss << "py " << item;
+			return ss.str();
+		}, py::call_guard<py::scoped_ostream_redirect>());
+
+	m.def("access", [](const Item& item) {
+		std::cout << "access " << item << "\n";
+	}, py::call_guard<py::scoped_ostream_redirect>());
+
+#if 0 // rvalue arguments fail during compilation
+	m.def("consume", [](Item&& item) {
+		std::cout << "consume " << item << "\n  ";
+		Item sink(std::move(item));
+		std::cout << "  old: " << item << "\n  new: " << sink << "\n";
+	}, py::call_guard<py::scoped_ostream_redirect>());
+#endif
+
+	m.def("consume", [](my_ptr<Item>&& item) {
+		std::cout << "consume " << *item.get() << "\n";
+		my_ptr<Item> sink(std::move(item));
+		std::cout << "  old: " << item.get() << "\n  new: " << *sink.get() << "\n";
+	}, py::call_guard<py::scoped_ostream_redirect>());
+
+	m.def("consume_twice", [](my_ptr<Item>&& /*first*/, my_ptr<Item>&& /*second*/) {
+	}, py::call_guard<py::scoped_ostream_redirect>());
+
+	m.def("consume_str", [](std::string&& s) { std::string o(std::move(s)); });
+}

tests/test_move_arg.h

@@ -0,0 +1,34 @@
+#pragma once
+#include <memory>
+#include <vector>
+#include <iostream>
+
+class Item;
+std::ostream& operator<<(std::ostream& os, const Item& item);
+
+/** Item class requires unique instances, i.e. only supports move construction */
+class Item
+{
+public:
+  Item(int value) : value_(std::make_unique<int>(value)) {
+    std::cout<< "new " << *this << "\n";
+  }
+  ~Item() {
+    std::cout << "destroy " << *this << "\n";
+  }
+  Item(const Item&) = delete;
+  Item(Item&& other) {
+    std::cout << "move " << other << " -> ";
+    value_ = std::move(other.value_);
+    std::cout << *this << "\n";
+  }
+
+  std::unique_ptr<int> value_;
+};
+
+std::ostream& operator<<(std::ostream& os, const Item& item) {
+  os << "item " << &item << "(";
+  if (item.value_) os << *item.value_;
+  os << ")";
+  return os;
+}

tests/test_move_arg.py

@@ -0,0 +1,26 @@
+import pytest
+from test_move_arg import *
+
+
+def test():
+    item = Item(42)
+    other = item
+    access(item)
+    print(item)
+    del item
+    print(other)
+
+def test_consume():
+    item = Item(42)
+    consume(item)
+    access(item)  # should raise, because item is accessed after consumption
+
+def test_consume_twice():
+    item = Item(42)
+    consume_twice(item, item) # should raise, because item is consumed twice
+
+def test_foo():
+    foo()
+
+if __name__ == "__main__":
+    test_consume()