stl.h: propagate return value policies to type-specific casters (#1455)

* stl.h: propagate return value policies to type-specific casters

Return value policies for containers like those handled in in 'stl.h'
are currently broken.

The problem is that detail::return_value_policy_override<C>::policy()
always returns 'move' when given a non-pointer/reference type, e.g.
'std::vector<...>'.

This is sensible behavior for custom types that are exposed via
'py::class_<>', but it does not make sense for types that are handled by
other type casters (STL containers, Eigen matrices, etc.).

This commit changes the behavior so that
detail::return_value_policy_override only becomes active when the type
caster derives from type_caster_generic.

Furthermore, the override logic is called recursively in STL type
casters to enable key/value-specific behavior.

Author: wjakob

include/pybind11/cast.h

@@ -1606,9 +1606,13 @@ template <typename type> using cast_is_temporary_value_reference = bool_constant
 
 // When a value returned from a C++ function is being cast back to Python, we almost always want to
 // force `policy = move`, regardless of the return value policy the function/method was declared
-// with.  Some classes (most notably Eigen::Ref and related) need to avoid this, and so can do so by
-// specializing this struct.
+// with.
 template <typename Return, typename SFINAE = void> struct return_value_policy_override {
+    static return_value_policy policy(return_value_policy p) { return p; }
+};
+
+template <typename Return> struct return_value_policy_override<Return,
+        detail::enable_if_t<std::is_base_of<type_caster_generic, make_caster<Return>>::value, void>> {
     static return_value_policy policy(return_value_policy p) {
         return !std::is_lvalue_reference<Return>::value && !std::is_pointer<Return>::value
             ? return_value_policy::move : p;

include/pybind11/eigen.h

@@ -353,14 +353,6 @@ struct type_caster<Type, enable_if_t<is_eigen_dense_plain<Type>::value>> {
     Type value;
 };
 
-// Eigen Ref/Map classes have slightly different policy requirements, meaning we don't want to force
-// `move` when a Ref/Map rvalue is returned; we treat Ref<> sort of like a pointer (we care about
-// the underlying data, not the outer shell).
-template <typename Return>
-struct return_value_policy_override<Return, enable_if_t<is_eigen_dense_map<Return>::value>> {
-    static return_value_policy policy(return_value_policy p) { return p; }
-};
-
 // Base class for casting reference/map/block/etc. objects back to python.
 template <typename MapType> struct eigen_map_caster {
 private:

include/pybind11/pybind11.h

@@ -145,7 +145,7 @@ class cpp_function : public function {
             capture *cap = const_cast<capture *>(reinterpret_cast<const capture *>(data));
 
             /* Override policy for rvalues -- usually to enforce rvp::move on an rvalue */
-            const auto policy = return_value_policy_override<Return>::policy(call.func.policy);
+            return_value_policy policy = return_value_policy_override<Return>::policy(call.func.policy);
 
             /* Function scope guard -- defaults to the compile-to-nothing `void_type` */
             using Guard = extract_guard_t<Extra...>;

include/pybind11/stl.h

@@ -83,6 +83,7 @@ template <typename Type, typename Key> struct set_caster {
 
     template <typename T>
     static handle cast(T &&src, return_value_policy policy, handle parent) {
+        policy = return_value_policy_override<Key>::policy(policy);
         pybind11::set s;
         for (auto &&value : src) {
             auto value_ = reinterpret_steal<object>(key_conv::cast(forward_like<T>(value), policy, parent));
@@ -118,9 +119,11 @@ template <typename Type, typename Key, typename Value> struct map_caster {
     template <typename T>
     static handle cast(T &&src, return_value_policy policy, handle parent) {
         dict d;
+        return_value_policy policy_key = return_value_policy_override<Key>::policy(policy);
+        return_value_policy policy_value = return_value_policy_override<Value>::policy(policy);
         for (auto &&kv : src) {
-            auto key = reinterpret_steal<object>(key_conv::cast(forward_like<T>(kv.first), policy, parent));
-            auto value = reinterpret_steal<object>(value_conv::cast(forward_like<T>(kv.second), policy, parent));
+            auto key = reinterpret_steal<object>(key_conv::cast(forward_like<T>(kv.first), policy_key, parent));
+            auto value = reinterpret_steal<object>(value_conv::cast(forward_like<T>(kv.second), policy_value, parent));
             if (!key || !value)
                 return handle();
             d[key] = value;
@@ -158,6 +161,7 @@ template <typename Type, typename Value> struct list_caster {
 public:
     template <typename T>
     static handle cast(T &&src, return_value_policy policy, handle parent) {
+        policy = return_value_policy_override<Value>::policy(policy);
         list l(src.size());
         size_t index = 0;
         for (auto &&value : src) {
@@ -252,6 +256,7 @@ template<typename T> struct optional_caster {
     static handle cast(T_ &&src, return_value_policy policy, handle parent) {
         if (!src)
             return none().inc_ref();
+        policy = return_value_policy_override<typename T::value_type>::policy(policy);
         return value_conv::cast(*std::forward<T_>(src), policy, parent);
     }
 
@@ -356,6 +361,7 @@ struct variant_caster<V<Ts...>> {
 template <typename... Ts>
 struct type_caster<std::variant<Ts...>> : variant_caster<std::variant<Ts...>> { };
 #endif
+
 NAMESPACE_END(detail)
 
 inline std::ostream &operator<<(std::ostream &os, const handle &obj) {

tests/test_stl.cpp

@@ -8,6 +8,7 @@
 */
 
 #include "pybind11_tests.h"
+#include "constructor_stats.h"
 #include <pybind11/stl.h>
 
 // Test with `std::variant` in C++17 mode, or with `boost::variant` in C++11/14
@@ -235,4 +236,21 @@ TEST_SUBMODULE(stl, m) {
 
     // test_stl_pass_by_pointer
     m.def("stl_pass_by_pointer", [](std::vector<int>* v) { return *v; }, "v"_a=nullptr);
+
+    class Placeholder {
+    public:
+        Placeholder() { print_created(this); }
+        Placeholder(const Placeholder &) = delete;
+        ~Placeholder() { print_destroyed(this); }
+    };
+    py::class_<Placeholder>(m, "Placeholder");
+
+    /// test_stl_vector_ownership
+    m.def("test_stl_ownership",
+          []() {
+              std::vector<Placeholder *> result;
+              result.push_back(new Placeholder());
+              return result;
+          },
+          py::return_value_policy::take_ownership);
 }

tests/test_stl.py

@@ -2,6 +2,7 @@
 
 from pybind11_tests import stl as m
 from pybind11_tests import UserType
+from pybind11_tests import ConstructorStats
 
 
 def test_vector(doc):
@@ -198,3 +199,12 @@ def test_missing_header_message():
     with pytest.raises(TypeError) as excinfo:
         cm.missing_header_return()
     assert expected_message in str(excinfo.value)
+
+
+def test_stl_ownership():
+    cstats = ConstructorStats.get(m.Placeholder)
+    assert cstats.alive() == 0
+    r = m.test_stl_ownership()
+    assert len(r) == 1
+    del r
+    assert cstats.alive() == 0