Make reference(_internal) the default return value policy for properties (#473)

* Make reference(_internal) the default return value policy for properties

Before this, all `def_property*` functions used `automatic` as their
default return value policy. This commit makes it so that:

 * Non-static properties use `reference_interal` by default, thus
   matching `def_readonly` and `def_readwrite`.

 * Static properties use `reference` by default, thus matching
   `def_readonly_static` and `def_readwrite_static`.

In case `cpp_function` is passed to any `def_property*`, its policy will
be used instead of any defaults. User-defined arguments in `extras`
still have top priority and will override both the default policies and
the ones from `cpp_function`.

Resolves #436.

* Almost always use return_value_policy::move for rvalues

For functions which return rvalues or rvalue references, the only viable
return value policies are `copy` and `move`. `reference(_internal)` and
`take_ownership` would take the address of a temporary which is always
an error.

This commit prevents possible user errors by overriding the bad rvalue
policies with `move`. Besides `move`, only `copy` is allowed, and only
if it's explicitly selected by the user.

This is also a necessary safety feature to support the new default
return value policies for properties: `reference(_internal)`.

docs/advanced/functions.rst

@@ -100,13 +100,23 @@ The following table provides an overview of available policies:
 |                                                  | (i.e. via handle::operator()). You probably won't need to use this.        |
 +--------------------------------------------------+----------------------------------------------------------------------------+
 
-Return value policies can also be applied to properties, in which case the
+Return value policies can also be applied to properties:
-arguments must be passed through the :class:`cpp_function` constructor:
 
 .. code-block:: cpp
 
     class_<MyClass>(m, "MyClass")
-        def_property("data"
+        .def_property("data", &MyClass::getData, &MyClass::setData,
+                      py::return_value_policy::copy);
+
+Technically, the code above applies the policy to both the getter and the
+setter function, however, the setter doesn't really care about *return*
+value policies which makes this a convenient terse syntax. Alternatively,
+targeted arguments can be passed through the :class:`cpp_function` constructor:
+
+.. code-block:: cpp
+
+    class_<MyClass>(m, "MyClass")
+        .def_property("data"
             py::cpp_function(&MyClass::getData, py::return_value_policy::copy),
             py::cpp_function(&MyClass::setData)
         );

docs/changelog.rst

@@ -57,6 +57,8 @@ Breaking changes queued for v2.0.0 (Not yet released)
   to chain attributes ``obj.attr("a")[key].attr("b").attr("method")(1, 2, 3)```.
 * Added built-in support for ``std::shared_ptr`` holder type. There is no more need
   to do it manually via ``PYBIND11_DECLARE_HOLDER_TYPE(T, std::shared_ptr<T>)``.
+* Default return values policy changes: non-static properties now use ``reference_internal``
+  and static properties use ``reference`` (previous default was ``automatic``, i.e. ``copy``).
 * Various minor improvements of library internals (no user-visible changes)
 
 1.8.1 (July 12, 2016)

include/pybind11/cast.h

@@ -345,7 +345,7 @@ public:
     }
 
     static handle cast(itype &&src, return_value_policy policy, handle parent) {
-        if (policy == return_value_policy::automatic || policy == return_value_policy::automatic_reference)
+        if (policy != return_value_policy::copy)
             policy = return_value_policy::move;
         return cast(&src, policy, parent);
     }

include/pybind11/pybind11.h

@@ -1054,23 +1054,49 @@ public:
         return *this;
     }
 
+    /// Uses return_value_policy::reference_internal by default
+    template <typename Getter, typename... Extra>
+    class_ &def_property_readonly(const char *name, const Getter &fget, const Extra& ...extra) {
+        return def_property_readonly(name, cpp_function(fget), return_value_policy::reference_internal, extra...);
+    }
+
+    /// Uses cpp_function's return_value_policy by default
     template <typename... Extra>
     class_ &def_property_readonly(const char *name, const cpp_function &fget, const Extra& ...extra) {
-        def_property(name, fget, cpp_function(), extra...);
+        return def_property(name, fget, cpp_function(), extra...);
-        return *this;
     }
 
+    /// Uses return_value_policy::reference by default
+    template <typename Getter, typename... Extra>
+    class_ &def_property_readonly_static(const char *name, const Getter &fget, const Extra& ...extra) {
+        return def_property_readonly_static(name, cpp_function(fget), return_value_policy::reference, extra...);
+    }
+
+    /// Uses cpp_function's return_value_policy by default
     template <typename... Extra>
     class_ &def_property_readonly_static(const char *name, const cpp_function &fget, const Extra& ...extra) {
-        def_property_static(name, fget, cpp_function(), extra...);
+        return def_property_static(name, fget, cpp_function(), extra...);
-        return *this;
     }
 
+    /// Uses return_value_policy::reference_internal by default
+    template <typename Getter, typename... Extra>
+    class_ &def_property(const char *name, const Getter &fget, const cpp_function &fset, const Extra& ...extra) {
+        return def_property(name, cpp_function(fget), fset, return_value_policy::reference_internal, extra...);
+    }
+
+    /// Uses cpp_function's return_value_policy by default
     template <typename... Extra>
     class_ &def_property(const char *name, const cpp_function &fget, const cpp_function &fset, const Extra& ...extra) {
         return def_property_static(name, fget, fset, is_method(*this), extra...);
     }
 
+    /// Uses return_value_policy::reference by default
+    template <typename Getter, typename... Extra>
+    class_ &def_property_static(const char *name, const Getter &fget, const cpp_function &fset, const Extra& ...extra) {
+        return def_property_static(name, cpp_function(fget), fset, return_value_policy::reference, extra...);
+    }
+
+    /// Uses cpp_function's return_value_policy by default
     template <typename... Extra>
     class_ &def_property_static(const char *name, const cpp_function &fget, const cpp_function &fset, const Extra& ...extra) {
         auto rec_fget = get_function_record(fget), rec_fset = get_function_record(fset);

tests/test_methods_and_attributes.cpp

@@ -66,6 +66,24 @@ struct TestProperties {
 
 int TestProperties::static_value = 1;
 
+struct SimpleValue { int value = 1; };
+
+struct TestPropRVP {
+    SimpleValue v1;
+    SimpleValue v2;
+    static SimpleValue sv1;
+    static SimpleValue sv2;
+
+    const SimpleValue &get1() const { return v1; }
+    const SimpleValue &get2() const { return v2; }
+    SimpleValue get_rvalue() const { return v2; }
+    void set1(int v) { v1.value = v; }
+    void set2(int v) { v2.value = v; }
+};
+
+SimpleValue TestPropRVP::sv1{};
+SimpleValue TestPropRVP::sv2{};
+
 class DynamicClass {
 public:
     DynamicClass() { print_default_created(this); }
@@ -117,6 +135,32 @@ test_initializer methods_and_attributes([](py::module &m) {
                              [](py::object) { return TestProperties::static_get(); },
                              [](py::object, int v) { return TestProperties::static_set(v); });
 
+    py::class_<SimpleValue>(m, "SimpleValue")
+        .def_readwrite("value", &SimpleValue::value);
+
+    auto static_get1 = [](py::object) -> const SimpleValue & { return TestPropRVP::sv1; };
+    auto static_get2 = [](py::object) -> const SimpleValue & { return TestPropRVP::sv2; };
+    auto static_set1 = [](py::object, int v) { TestPropRVP::sv1.value = v; };
+    auto static_set2 = [](py::object, int v) { TestPropRVP::sv2.value = v; };
+    auto rvp_copy = py::return_value_policy::copy;
+
+    py::class_<TestPropRVP>(m, "TestPropRVP")
+        .def(py::init<>())
+        .def_property_readonly("ro_ref", &TestPropRVP::get1)
+        .def_property_readonly("ro_copy", &TestPropRVP::get2, rvp_copy)
+        .def_property_readonly("ro_func", py::cpp_function(&TestPropRVP::get2, rvp_copy))
+        .def_property("rw_ref", &TestPropRVP::get1, &TestPropRVP::set1)
+        .def_property("rw_copy", &TestPropRVP::get2, &TestPropRVP::set2, rvp_copy)
+        .def_property("rw_func", py::cpp_function(&TestPropRVP::get2, rvp_copy), &TestPropRVP::set2)
+        .def_property_readonly_static("static_ro_ref", static_get1)
+        .def_property_readonly_static("static_ro_copy", static_get2, rvp_copy)
+        .def_property_readonly_static("static_ro_func", py::cpp_function(static_get2, rvp_copy))
+        .def_property_static("static_rw_ref", static_get1, static_set1)
+        .def_property_static("static_rw_copy", static_get2, static_set2, rvp_copy)
+        .def_property_static("static_rw_func", py::cpp_function(static_get2, rvp_copy), static_set2)
+        .def_property_readonly("rvalue", &TestPropRVP::get_rvalue)
+        .def_property_readonly_static("static_rvalue", [](py::object) { return SimpleValue(); });
+
     py::class_<DynamicClass>(m, "DynamicClass", py::dynamic_attr())
         .def(py::init());
 

tests/test_methods_and_attributes.py

@@ -85,6 +85,44 @@ def test_static_properties():
     assert Type.def_property_static == 3
 
 
+@pytest.mark.parametrize("access", ["ro", "rw", "static_ro", "static_rw"])
+def test_property_return_value_policies(access):
+    from pybind11_tests import TestPropRVP
+
+    if not access.startswith("static"):
+        obj = TestPropRVP()
+    else:
+        obj = TestPropRVP
+
+    ref = getattr(obj, access + "_ref")
+    assert ref.value == 1
+    ref.value = 2
+    assert getattr(obj, access + "_ref").value == 2
+    ref.value = 1  # restore original value for static properties
+
+    copy = getattr(obj, access + "_copy")
+    assert copy.value == 1
+    copy.value = 2
+    assert getattr(obj, access + "_copy").value == 1
+
+    copy = getattr(obj, access + "_func")
+    assert copy.value == 1
+    copy.value = 2
+    assert getattr(obj, access + "_func").value == 1
+
+
+def test_property_rvalue_policy():
+    """When returning an rvalue, the return value policy is automatically changed from
+       `reference(_internal)` to `move`. The following would not work otherwise."""
+    from pybind11_tests import TestPropRVP
+
+    instance = TestPropRVP()
+    o = instance.rvalue
+    assert o.value == 1
+    o = TestPropRVP.static_rvalue
+    assert o.value == 1
+
+
 def test_dynamic_attributes():
     from pybind11_tests import DynamicClass, CppDerivedDynamicClass