Always use return_value_policy::move for rvalues (#510)

Fixes #509.

The move policy was already set for rvalues in PR #473, but this only
applied to directly cast user-defined types. The problem is that STL
containers cast values indirectly and the rvalue information is lost.
Therefore the move policy was not set correctly. This commit fixes it.

This also makes an additional adjustment to remove the `copy` policy
exception: rvalues now always use the `move` policy. This is also safe
for copy-only rvalues because the `move` policy has an internal fallback
to copying.

include/pybind11/cast.h

@@ -380,10 +380,8 @@ public:
         return cast(&src, policy, parent);
     }
 
-    static handle cast(itype &&src, return_value_policy policy, handle parent) {
-        if (policy != return_value_policy::copy)
-            policy = return_value_policy::move;
-        return cast(&src, policy, parent);
+    static handle cast(itype &&src, return_value_policy, handle parent) {
+        return cast(&src, return_value_policy::move, parent);
     }
 
     static handle cast(const itype *src, return_value_policy policy, handle parent) {

include/pybind11/pybind11.h

@@ -131,9 +131,14 @@ protected:
             capture *cap = (capture *) (sizeof(capture) <= sizeof(rec->data)
                                         ? &rec->data : rec->data[0]);
 
+            /* Override policy for rvalues -- always move */
+            constexpr auto is_rvalue = !std::is_pointer<Return>::value
+                                       && !std::is_lvalue_reference<Return>::value;
+            const auto policy = is_rvalue ? return_value_policy::move : rec->policy;
+
             /* Perform the function call */
             handle result = cast_out::cast(args_converter.template call<Return>(cap->f),
-                                           rec->policy, parent);
+                                           policy, parent);
 
             /* Invoke call policy post-call hook */
             detail::process_attributes<Extra...>::postcall(args, result);

tests/test_python_types.cpp

@@ -164,6 +164,13 @@ public:
 int ExamplePythonTypes::value = 0;
 const int ExamplePythonTypes::value2 = 5;
 
+struct MoveOutContainer {
+    struct Value { int value; };
+
+    std::list<Value> move_list() const { return {{0}, {1}, {2}}; }
+};
+
+
 test_initializer python_types([](py::module &m) {
     /* No constructor is explicitly defined below. An exception is raised when
        trying to construct it directly from Python */
@@ -363,4 +370,11 @@ test_initializer python_types([](py::module &m) {
             "memoryview"_a=d["memoryview"].cast<py::memoryview>()
         );
     });
+
+    py::class_<MoveOutContainer::Value>(m, "MoveOutContainerValue")
+        .def_readonly("value", &MoveOutContainer::Value::value);
+
+    py::class_<MoveOutContainer>(m, "MoveOutContainer")
+        .def(py::init<>())
+        .def_property_readonly("move_list", &MoveOutContainer::move_list);
 });

tests/test_python_types.py

@@ -356,3 +356,15 @@ def test_constructors():
     expected = {k.__name__: k(v) for k, v in data.items()}
     assert test_converting_constructors(inputs) == expected
     assert test_cast_functions(inputs) == expected
+
+
+def test_move_out_container():
+    """Properties use the `reference_internal` policy by default. If the underlying function
+    returns an rvalue, the policy is automatically changed to `move` to avoid referencing
+    a temporary. In case the return value is a container of user-defined types, the policy
+    also needs to be applied to the elements, not just the container."""
+    from pybind11_tests import MoveOutContainer
+
+    c = MoveOutContainer()
+    moved_out_list = c.move_list
+    assert [x.value for x in moved_out_list] == [0, 1, 2]

tools/libsize.py

@@ -1,4 +1,4 @@
-from __future__ import print_function
+from __future__ import print_function, division
 import os
 import sys