Use return_value_policy::reference for mutable lvalues.

Author: EricCousineau-TRI

docs/advanced/functions.rst

@@ -92,15 +92,17 @@ The following table provides an overview of available policies:
 +--------------------------------------------------+----------------------------------------------------------------------------+
 | :enum:`return_value_policy::automatic`           | **Default policy.** This policy falls back to the policy                   |
 |                                                  | :enum:`return_value_policy::take_ownership` when the return value is a     |
-|                                                  | pointer. Otherwise, it uses :enum:`return_value_policy::move` or           |
-|                                                  | :enum:`return_value_policy::copy` for rvalue and lvalue references,        |
-|                                                  | respectively. See above for a description of what all of these different   |
-|                                                  | policies do.                                                               |
+|                                                  | pointer or :enum:`return_value_policy:reference` when the return value is  |
+|                                                  | a mutable lvalue reference. Otherwise, it uses                             |
+|                                                  | :enum:`return_value_policy::move` or :enum:`return_value_policy::copy` for |
+|                                                  | rvalue and const lvalue references, respectively. See above for a          |
+|                                                  | description of what all of these different policies do.                    |
 +--------------------------------------------------+----------------------------------------------------------------------------+
 | :enum:`return_value_policy::automatic_reference` | As above, but use policy :enum:`return_value_policy::reference` when the   |
-|                                                  | return value is a pointer. This is the default conversion policy for       |
-|                                                  | function arguments when calling Python functions manually from C++ code    |
-|                                                  | (i.e. via handle::operator()). You probably won't need to use this.        |
+|                                                  | return value is a pointer or mutable lvalue reference. This is the default |
+|                                                  | conversion policy for function arguments when calling Python functions     |
+|                                                  | manually from C++ code (i.e. via handle::operator()). You probably won't   |
+|                                                  | need to use this.                                                          |
 +--------------------------------------------------+----------------------------------------------------------------------------+
 
 Return value policies can also be applied to properties:

include/pybind11/cast.h

@@ -789,6 +789,12 @@ template <typename type> class type_caster_base : public type_caster_generic {
         return cast(&src, policy, parent);
     }
 
+    static handle cast(itype &src, return_value_policy policy, handle parent) {
+        if (policy == return_value_policy::automatic || policy == return_value_policy::automatic_reference)
+            policy = return_value_policy::reference;
+        return cast(&src, policy, parent);
+    }
+
     static handle cast(itype &&src, return_value_policy, handle parent) {
         return cast(&src, return_value_policy::move, parent);
     }

tests/test_callbacks.cpp

@@ -159,8 +159,7 @@ TEST_SUBMODULE(callbacks, m) {
         [](std::function<void(CppCopyable&)> f, CppCopyable& obj) {
             f(obj);
         });
-    // Does not work as expected, because pybind will copy the instance when
-    // binding.
+    // Works as expected, because pybind will not copy the instance.
     m.def(
         "callback_mutate_copyable_cpp_ref",
         [](std::function<void(CppCopyable&)> f, int value) {

tests/test_callbacks.py

@@ -118,8 +118,6 @@ def incr(obj):
     assert obj.value == 1
 
     obj = m.callback_mutate_copyable_cpp_ref(incr, 10)
-    # WARNING: This this creates a COPY when passing to callback.
-    assert obj.value == 10
-
+    assert obj.value == 11
     obj = m.callback_mutate_copyable_cpp_ptr(incr, 10)
     assert obj.value == 11

tests/test_methods_and_attributes.cpp

@@ -451,4 +451,60 @@ TEST_SUBMODULE(methods_and_attributes, m) {
     m.def("custom_caster_destroy_const", []() -> const DestructionTester * { return new DestructionTester(); },
             py::return_value_policy::take_ownership); // Likewise (const doesn't inhibit destruction)
     m.def("destruction_tester_cstats", &ConstructorStats::get<DestructionTester>, py::return_value_policy::reference);
+
+    // Test mutable lvalue references for return policies and `cast<>()` (#1200).
+    struct Item {
+        Item(int value_in) : value(value_in) { print_created(this, value); }
+        Item(const Item& other) : value(other.value) {
+             print_copy_created(this, value);
+         }
+        ~Item() { print_destroyed(this); }
+        int value{};
+    };
+    py::class_<Item>(m, "Item")
+        .def(py::init<int>())
+        .def_readwrite("value", &Item::value);
+
+    struct Container {
+        Item* new_ptr() { return new Item{100}; }
+        Item* get_ptr() { return &item; }
+        Item& get_ref() { return item; }
+
+        // Test casting behavior.
+        py::object cast_copy(int value) {
+            Item item{value};
+            return py::cast(item);
+        }
+        py::object cast_ptr() { return py::cast(&item); }
+        py::object cast_ref_registered() { return py::cast(item); }
+        py::object cast_ref_unregistered() {
+            // This is different from above in that we have not exposed `item_cast_ref` prior to this.
+            // NB. Writing `py::cast<Item&>(get_cast_ref())` will simply bind to the same overload as
+            // `py::cast(get_cast_ref())`, which is `object cast(const Item&, ...)`.
+            // Unfortunately, there is no way to overload `cast` to return references without danger of
+            // returning references to temporaries (see `cast_copy`).
+            return py::cast(get_cast_ref(), py::return_value_policy::reference);
+        }
+
+    private:
+        Item& get_cast_ref() {
+            // Ensure that we return something that pybind has not seen yet.
+            item_cast_ref_ptr.reset(new Item(20));
+            return *item_cast_ref_ptr;
+        }
+
+        Item item{10};
+        std::unique_ptr<Item> item_cast_ref_ptr;
+    };
+    py::class_<Container>(m, "Container")
+        .def(py::init<>())
+        .def("new_ptr", &Container::new_ptr)
+        .def("get_ptr_unsafe", &Container::get_ptr, py::return_value_policy::automatic_reference)
+        .def("get_ref_unsafe", &Container::get_ref)
+        .def("get_ptr", &Container::get_ptr, py::return_value_policy::reference_internal)
+        .def("get_ref", &Container::get_ref, py::return_value_policy::reference_internal)
+        .def("cast_copy", &Container::cast_copy)
+        .def("cast_ptr", &Container::cast_ptr)
+        .def("cast_ref_registered", &Container::cast_ref_registered)
+        .def("cast_ref_unregistered", &Container::cast_ref_unregistered);
 }

tests/test_methods_and_attributes.py

@@ -510,3 +510,60 @@ def test_custom_caster_destruction():
 
     # Make sure we still only have the original object (from ..._no_destroy()) alive:
     assert cstats.alive() == 1
+
+
+def test_lvalue_reference_policies():
+    cstats = ConstructorStats.get(m.Item)
+
+    c = m.Container()
+    assert cstats.alive() == 1
+    # Try new.
+    obj = c.new_ptr()
+    assert cstats.alive() == 2
+    del obj
+    pytest.gc_collect()
+    assert cstats.alive() == 1
+
+    # Try references (unsafe).
+    obj = c.get_ptr_unsafe()
+    obj.value = 100
+    assert cstats.alive() == 1
+    obj = c.get_ref_unsafe()
+    assert cstats.alive() == 1
+    del obj
+
+    # Try references (safe).
+    obj = c.get_ptr()
+    assert cstats.alive() == 1
+    obj = c.get_ref()
+    assert cstats.alive() == 1
+
+    # Check casting behavior.
+    obj = c.cast_copy(10)
+    assert cstats.alive() == 2
+    assert obj.value == 10
+    del obj
+    pytest.gc_collect()
+    assert cstats.alive() == 1
+
+    obj = c.cast_ptr()
+    assert cstats.alive() == 1
+    assert obj is c.get_ptr()
+    obj = c.cast_ref_registered()
+    assert obj is c.get_ptr()
+    assert cstats.alive() == 1
+    # Check reference casting for an object previously unseen by pybind.
+    obj = c.cast_ref_unregistered()
+    assert cstats.alive() == 2
+    assert obj.value == 20
+    del obj
+    pytest.gc_collect()
+    # Object is owned by `c` internally.
+    assert cstats.alive() == 2
+
+    # Ensure we have our original value.
+    assert c.get_ref().value == 100
+
+    del c
+    pytest.gc_collect()
+    assert cstats.alive() == 0