Fix stl_bind to support movable, non-copyable value types (#490)

This commit includes the following changes:

* Don't provide make_copy_constructor for non-copyable container

make_copy_constructor currently fails for various stl containers (e.g.
std::vector, std::unordered_map, std::deque, etc.) when the container's
value type (e.g. the "T" or the std::pair<K,T> for a map) is
non-copyable.  This adds an override that, for types that look like
containers, also requires that the value_type be copyable.

* stl_bind.h: make bind_{vector,map} work for non-copy-constructible types

Most stl_bind modifiers require copying, so if the type isn't copy
constructible, we provide a read-only interface instead.

In practice, this means that if the type is non-copyable, it will be,
for all intents and purposes, read-only from the Python side (but
currently it simply fails to compile with such a container).

It is still possible for the caller to provide an interface manually
(by defining methods on the returned class_ object), but this isn't
something stl_bind can handle because the C++ code to construct values
is going to be highly dependent on the container value_type.

* stl_bind: copy only for arithmetic value types

For non-primitive types, we may well be copying some complex type, when
returning by reference is more appropriate.  This commit returns by
internal reference for all but basic arithmetic types.

* Return by reference whenever possible

Only if we definitely can't--i.e. std::vector<bool>--because v[i]
returns something that isn't a T& do we copy; for everything else, we
return by reference.

For the map case, we can always return by reference (at least for the
default stl map/unordered_map).

include/pybind11/cast.h

@@ -346,6 +346,18 @@ using cast_op_type = typename std::conditional<std::is_pointer<typename std::rem
     typename std::add_pointer<intrinsic_t<T>>::type,
     typename std::add_lvalue_reference<intrinsic_t<T>>::type>::type;
 
+// std::is_copy_constructible isn't quite enough: it lets std::vector<T> (and similar) through when
+// T is non-copyable, but code containing such a copy constructor fails to actually compile.
+template <typename T, typename SFINAE = void> struct is_copy_constructible : std::is_copy_constructible<T> {};
+
+// Specialization for types that appear to be copy constructible but also look like stl containers
+// (we specifically check for: has `value_type` and `reference` with `reference = value_type&`): if
+// so, copy constructability depends on whether the value_type is copy constructible.
+template <typename Container> struct is_copy_constructible<Container, enable_if_t<
+        std::is_copy_constructible<Container>::value &&
+        std::is_same<typename Container::value_type &, typename Container::reference>::value
+    >> : std::is_copy_constructible<typename Container::value_type> {};
+
 /// Generic type caster for objects stored on the heap
 template <typename type> class type_caster_base : public type_caster_generic {
     using itype = intrinsic_t<type>;
@@ -383,20 +395,21 @@ protected:
 #if !defined(_MSC_VER)
     /* Only enabled when the types are {copy,move}-constructible *and* when the type
        does not have a private operator new implementaton. */
-    template <typename T = type> static auto make_copy_constructor(const T *value) -> decltype(new T(*value), Constructor(nullptr)) {
+    template <typename T = type, typename = enable_if_t<is_copy_constructible<T>::value>> static auto make_copy_constructor(const T *value) -> decltype(new T(*value), Constructor(nullptr)) {
         return [](const void *arg) -> void * { return new T(*((const T *) arg)); }; }
     template <typename T = type> static auto make_move_constructor(const T *value) -> decltype(new T(std::move(*((T *) value))), Constructor(nullptr)) {
         return [](const void *arg) -> void * { return (void *) new T(std::move(*((T *) arg))); }; }
 #else
     /* Visual Studio 2015's SFINAE implementation doesn't yet handle the above robustly in all situations.
        Use a workaround that only tests for constructibility for now. */
-    template <typename T = type, typename = enable_if_t<std::is_copy_constructible<T>::value>>
+    template <typename T = type, typename = enable_if_t<is_copy_constructible<T>::value>>
     static Constructor make_copy_constructor(const T *value) {
         return [](const void *arg) -> void * { return new T(*((const T *)arg)); }; }
     template <typename T = type, typename = enable_if_t<std::is_move_constructible<T>::value>>
     static Constructor make_move_constructor(const T *value) {
         return [](const void *arg) -> void * { return (void *) new T(std::move(*((T *)arg))); }; }
 #endif
+
     static Constructor make_copy_constructor(...) { return nullptr; }
     static Constructor make_move_constructor(...) { return nullptr; }
 };

include/pybind11/stl_bind.h

@@ -60,18 +60,21 @@ struct is_comparable<T, enable_if_t<container_traits<T>::is_pair>> {
 };
 
 /* Fallback functions */
-template <typename, typename, typename... Args> void vector_if_copy_constructible(const Args&...) { }
+template <typename, typename, typename... Args> void vector_if_copy_constructible(const Args &...) { }
-template <typename, typename, typename... Args> void vector_if_equal_operator(const Args&...) { }
+template <typename, typename, typename... Args> void vector_if_equal_operator(const Args &...) { }
-template <typename, typename, typename... Args> void vector_if_insertion_operator(const Args&...) { }
+template <typename, typename, typename... Args> void vector_if_insertion_operator(const Args &...) { }
+template <typename, typename, typename... Args> void vector_modifiers(const Args &...) { }
 
-template<typename Vector, typename Class_, enable_if_t<std::is_copy_constructible<typename Vector::value_type>::value, int> = 0>
+template<typename Vector, typename Class_>
-void vector_if_copy_constructible(Class_ &cl) {
+void vector_if_copy_constructible(enable_if_t<
-    cl.def(pybind11::init<const Vector &>(),
+    std::is_copy_constructible<Vector>::value &&
-           "Copy constructor");
+    std::is_copy_constructible<typename Vector::value_type>::value, Class_> &cl) {
+
+    cl.def(pybind11::init<const Vector &>(), "Copy constructor");
 }
 
-template<typename Vector, typename Class_, enable_if_t<is_comparable<Vector>::value, int> = 0>
+template<typename Vector, typename Class_>
-void vector_if_equal_operator(Class_ &cl) {
+void vector_if_equal_operator(enable_if_t<is_comparable<Vector>::value, Class_> &cl) {
     using T = typename Vector::value_type;
 
     cl.def(self == self);
@@ -106,71 +109,34 @@ void vector_if_equal_operator(Class_ &cl) {
     );
 }
 
-template <typename Vector, typename Class_> auto vector_if_insertion_operator(Class_ &cl, std::string const &name)
+// Vector modifiers -- requires a copyable vector_type:
-    -> decltype(std::declval<std::ostream&>() << std::declval<typename Vector::value_type>(), void()) {
+// (Technically, some of these (pop and __delitem__) don't actually require copyability, but it seems
-    using size_type = typename Vector::size_type;
+// silly to allow deletion but not insertion, so include them here too.)
-
+template <typename Vector, typename Class_>
-    cl.def("__repr__",
+void vector_modifiers(enable_if_t<std::is_copy_constructible<typename Vector::value_type>::value, Class_> &cl) {
-           [name](Vector &v) {
-            std::ostringstream s;
-            s << name << '[';
-            for (size_type i=0; i < v.size(); ++i) {
-                s << v[i];
-                if (i != v.size() - 1)
-                    s << ", ";
-            }
-            s << ']';
-            return s.str();
-        },
-        "Return the canonical string representation of this list."
-    );
-}
-
-NAMESPACE_END(detail)
-
-//
-// std::vector
-//
-template <typename Vector, typename holder_type = std::unique_ptr<Vector>, typename... Args>
-pybind11::class_<Vector, holder_type> bind_vector(pybind11::module &m, std::string const &name, Args&&... args) {
     using T = typename Vector::value_type;
     using SizeType = typename Vector::size_type;
     using DiffType = typename Vector::difference_type;
-    using ItType   = typename Vector::iterator;
-    using Class_ = pybind11::class_<Vector, holder_type>;
-
-    Class_ cl(m, name.c_str(), std::forward<Args>(args)...);
-
-    cl.def(pybind11::init<>());
-
-    // Register copy constructor (if possible)
-    detail::vector_if_copy_constructible<Vector, Class_>(cl);
-
-    // Register comparison-related operators and functions (if possible)
-    detail::vector_if_equal_operator<Vector, Class_>(cl);
-
-    // Register stream insertion operator (if possible)
-    detail::vector_if_insertion_operator<Vector, Class_>(cl, name);
-
-    cl.def("__init__", [](Vector &v, iterable it) {
-        new (&v) Vector();
-        try {
-            v.reserve(len(it));
-            for (handle h : it)
-               v.push_back(h.cast<typename Vector::value_type>());
-        } catch (...) {
-            v.~Vector();
-            throw;
-        }
-    });
 
     cl.def("append",
            [](Vector &v, const T &value) { v.push_back(value); },
            arg("x"),
            "Add an item to the end of the list");
 
+    cl.def("__init__", [](Vector &v, iterable it) {
+        new (&v) Vector();
+        try {
+            v.reserve(len(it));
+            for (handle h : it)
+               v.push_back(h.cast<T>());
+        } catch (...) {
+            v.~Vector();
+            throw;
+        }
+    });
+
     cl.def("extend",
-       [](Vector &v, Vector &src) {
+       [](Vector &v, const Vector &src) {
            v.reserve(v.size() + src.size());
            v.insert(v.end(), src.begin(), src.end());
        },
@@ -209,21 +175,6 @@ pybind11::class_<Vector, holder_type> bind_vector(pybind11::module &m, std::stri
         "Remove and return the item at index ``i``"
     );
 
-    cl.def("__bool__",
-        [](const Vector &v) -> bool {
-            return !v.empty();
-        },
-        "Check whether the list is nonempty"
-    );
-
-    cl.def("__getitem__",
-        [](const Vector &v, SizeType i) -> T {
-            if (i >= v.size())
-                throw pybind11::index_error();
-            return v[i];
-        }
-    );
-
     cl.def("__setitem__",
         [](Vector &v, SizeType i, const T &t) {
             if (i >= v.size())
@@ -232,26 +183,6 @@ pybind11::class_<Vector, holder_type> bind_vector(pybind11::module &m, std::stri
         }
     );
 
-    cl.def("__delitem__",
-        [](Vector &v, SizeType i) {
-            if (i >= v.size())
-                throw pybind11::index_error();
-            v.erase(v.begin() + typename Vector::difference_type(i));
-        },
-        "Delete list elements using a slice object"
-    );
-
-    cl.def("__len__", &Vector::size);
-
-    cl.def("__iter__",
-           [](Vector &v) {
-               return pybind11::make_iterator<
-                   return_value_policy::reference_internal, ItType, ItType, T>(
-                   v.begin(), v.end());
-           },
-           pybind11::keep_alive<0, 1>() /* Essential: keep list alive while iterator exists */
-    );
-
     /// Slicing protocol
     cl.def("__getitem__",
         [](const Vector &v, slice slice) -> Vector * {
@@ -290,6 +221,15 @@ pybind11::class_<Vector, holder_type> bind_vector(pybind11::module &m, std::stri
         "Assign list elements using a slice object"
     );
 
+    cl.def("__delitem__",
+        [](Vector &v, SizeType i) {
+            if (i >= v.size())
+                throw pybind11::index_error();
+            v.erase(v.begin() + DiffType(i));
+        },
+        "Delete the list elements at index ``i``"
+    );
+
     cl.def("__delitem__",
         [](Vector &v, slice slice) {
             size_t start, stop, step, slicelength;
@@ -309,6 +249,123 @@ pybind11::class_<Vector, holder_type> bind_vector(pybind11::module &m, std::stri
         "Delete list elements using a slice object"
     );
 
+}
+
+// If the type has an operator[] that doesn't return a reference (most notably std::vector<bool>),
+// we have to access by copying; otherwise we return by reference.
+template <typename Vector> using vector_needs_copy = bool_constant<
+    !std::is_same<decltype(std::declval<Vector>()[typename Vector::size_type()]), typename Vector::value_type &>::value>;
+
+// The usual case: access and iterate by reference
+template <typename Vector, typename Class_>
+void vector_accessor(enable_if_t<!vector_needs_copy<Vector>::value, Class_> &cl) {
+    using T = typename Vector::value_type;
+    using SizeType = typename Vector::size_type;
+    using ItType   = typename Vector::iterator;
+
+    cl.def("__getitem__",
+        [](Vector &v, SizeType i) -> T & {
+            if (i >= v.size())
+                throw pybind11::index_error();
+            return v[i];
+        },
+        return_value_policy::reference_internal // ref + keepalive
+    );
+
+    cl.def("__iter__",
+           [](Vector &v) {
+               return pybind11::make_iterator<
+                   return_value_policy::reference_internal, ItType, ItType, T&>(
+                   v.begin(), v.end());
+           },
+           keep_alive<0, 1>() /* Essential: keep list alive while iterator exists */
+    );
+}
+
+// The case for special objects, like std::vector<bool>, that have to be returned-by-copy:
+template <typename Vector, typename Class_>
+void vector_accessor(enable_if_t<vector_needs_copy<Vector>::value, Class_> &cl) {
+    using T = typename Vector::value_type;
+    using SizeType = typename Vector::size_type;
+    using ItType   = typename Vector::iterator;
+    cl.def("__getitem__",
+        [](const Vector &v, SizeType i) -> T {
+            if (i >= v.size())
+                throw pybind11::index_error();
+            return v[i];
+        }
+    );
+
+    cl.def("__iter__",
+           [](Vector &v) {
+               return pybind11::make_iterator<
+                   return_value_policy::copy, ItType, ItType, T>(
+                   v.begin(), v.end());
+           },
+           keep_alive<0, 1>() /* Essential: keep list alive while iterator exists */
+    );
+}
+
+template <typename Vector, typename Class_> auto vector_if_insertion_operator(Class_ &cl, std::string const &name)
+    -> decltype(std::declval<std::ostream&>() << std::declval<typename Vector::value_type>(), void()) {
+    using size_type = typename Vector::size_type;
+
+    cl.def("__repr__",
+           [name](Vector &v) {
+            std::ostringstream s;
+            s << name << '[';
+            for (size_type i=0; i < v.size(); ++i) {
+                s << v[i];
+                if (i != v.size() - 1)
+                    s << ", ";
+            }
+            s << ']';
+            return s.str();
+        },
+        "Return the canonical string representation of this list."
+    );
+}
+
+NAMESPACE_END(detail)
+
+//
+// std::vector
+//
+template <typename Vector, typename holder_type = std::unique_ptr<Vector>, typename... Args>
+pybind11::class_<Vector, holder_type> bind_vector(pybind11::module &m, std::string const &name, Args&&... args) {
+    using Class_ = pybind11::class_<Vector, holder_type>;
+
+    Class_ cl(m, name.c_str(), std::forward<Args>(args)...);
+
+    cl.def(pybind11::init<>());
+
+    // Register copy constructor (if possible)
+    detail::vector_if_copy_constructible<Vector, Class_>(cl);
+
+    // Register comparison-related operators and functions (if possible)
+    detail::vector_if_equal_operator<Vector, Class_>(cl);
+
+    // Register stream insertion operator (if possible)
+    detail::vector_if_insertion_operator<Vector, Class_>(cl, name);
+
+    // Modifiers require copyable vector value type
+    detail::vector_modifiers<Vector, Class_>(cl);
+
+    // Accessor and iterator; return by value if copyable, otherwise we return by ref + keep-alive
+    detail::vector_accessor<Vector, Class_>(cl);
+
+    cl.def("__bool__",
+        [](const Vector &v) -> bool {
+            return !v.empty();
+        },
+        "Check whether the list is nonempty"
+    );
+
+    cl.def("__len__", &Vector::size);
+
+
+
+
 #if 0
     // C++ style functions deprecated, leaving it here as an example
     cl.def(pybind11::init<size_type>());
@@ -361,9 +418,12 @@ pybind11::class_<Vector, holder_type> bind_vector(pybind11::module &m, std::stri
 NAMESPACE_BEGIN(detail)
 
 /* Fallback functions */
-template <typename, typename, typename... Args> void map_if_insertion_operator(const Args&...) { }
+template <typename, typename, typename... Args> void map_if_insertion_operator(const Args &...) { }
+template <typename, typename, typename... Args> void map_assignment(const Args &...) { }
 
-template <typename Map, typename Class_, typename... Args> void map_if_copy_assignable(Class_ &cl, const Args&...) {
+// Map assignment when copy-assignable: just copy the value
+template <typename Map, typename Class_>
+void map_assignment(enable_if_t<std::is_copy_assignable<typename Map::mapped_type>::value, Class_> &cl) {
     using KeyType = typename Map::key_type;
     using MappedType = typename Map::mapped_type;
 
@@ -376,19 +436,23 @@ template <typename Map, typename Class_, typename... Args> void map_if_copy_assi
     );
 }
 
-template<typename Map, typename Class_, enable_if_t<!std::is_copy_assignable<typename Map::mapped_type>::value, int> = 0>
+// Not copy-assignable, but still copy-constructible: we can update the value by erasing and reinserting
-void map_if_copy_assignable(Class_ &cl) {
+template<typename Map, typename Class_>
+void map_assignment(enable_if_t<
+        !std::is_copy_assignable<typename Map::mapped_type>::value &&
+        std::is_copy_constructible<typename Map::mapped_type>::value,
+        Class_> &cl) {
     using KeyType = typename Map::key_type;
     using MappedType = typename Map::mapped_type;
 
     cl.def("__setitem__",
            [](Map &m, const KeyType &k, const MappedType &v) {
                // We can't use m[k] = v; because value type might not be default constructable
-               auto r = m.insert(std::make_pair(k, v));
+               auto r = m.emplace(k, v);
                if (!r.second) {
-                   // value type might be const so the only way to insert it is to erase it first...
+                   // value type is not copy assignable so the only way to insert it is to erase it first...
                    m.erase(r.first);
-                   m.insert(std::make_pair(k, v));
+                   m.emplace(k, v);
                }
            }
     );
@@ -415,6 +479,8 @@ template <typename Map, typename Class_> auto map_if_insertion_operator(Class_ &
         "Return the canonical string representation of this map."
     );
 }
+
+
 NAMESPACE_END(detail)
 
 template <typename Map, typename holder_type = std::unique_ptr<Map>, typename... Args>
@@ -446,15 +512,17 @@ pybind11::class_<Map, holder_type> bind_map(module &m, const std::string &name,
     );
 
     cl.def("__getitem__",
-           [](Map &m, const KeyType &k) -> MappedType {
+        [](Map &m, const KeyType &k) -> MappedType & {
-               auto it = m.find(k);
+            auto it = m.find(k);
-               if (it == m.end())
+            if (it == m.end())
-                  throw pybind11::key_error();
+              throw pybind11::key_error();
-               return it->second;
+           return it->second;
-           }
+        },
+        return_value_policy::reference_internal // ref + keepalive
     );
 
-    detail::map_if_copy_assignable<Map, Class_>(cl);
+    // Assignment provided only if the type is copyable
+    detail::map_assignment<Map, Class_>(cl);
 
     cl.def("__delitem__",
            [](Map &m, const KeyType &k) {

tests/test_stl_binders.cpp

@@ -11,6 +11,7 @@
 
 #include <pybind11/stl_bind.h>
 #include <map>
+#include <deque>
 #include <unordered_map>
 
 class El {
@@ -26,6 +27,32 @@ std::ostream & operator<<(std::ostream &s, El const&v) {
     return s;
 }
 
+/// Issue #487: binding std::vector<E> with E non-copyable
+class E_nc {
+public:
+    explicit E_nc(int i) : value{i} {}
+    E_nc(const E_nc &) = delete;
+    E_nc &operator=(const E_nc &) = delete;
+    E_nc(E_nc &&) = default;
+    E_nc &operator=(E_nc &&) = default;
+
+    int value;
+};
+
+template <class Container> Container *one_to_n(int n) {
+    auto v = new Container();
+    for (int i = 1; i <= n; i++)
+        v->emplace_back(i);
+    return v;
+}
+
+template <class Map> Map *times_ten(int n) {
+    auto m = new Map();
+    for (int i = 1; i <= n; i++)
+        m->emplace(int(i), E_nc(10*i));
+    return m;
+}
+
 test_initializer stl_binder_vector([](py::module &m) {
     py::class_<El>(m, "El")
         .def(py::init<int>());
@@ -36,6 +63,7 @@ test_initializer stl_binder_vector([](py::module &m) {
     py::bind_vector<std::vector<El>>(m, "VectorEl");
 
     py::bind_vector<std::vector<std::vector<El>>>(m, "VectorVectorEl");
+
 });
 
 test_initializer stl_binder_map([](py::module &m) {
@@ -44,4 +72,24 @@ test_initializer stl_binder_map([](py::module &m) {
 
     py::bind_map<std::map<std::string, double const>>(m, "MapStringDoubleConst");
     py::bind_map<std::unordered_map<std::string, double const>>(m, "UnorderedMapStringDoubleConst");
+
 });
+
+test_initializer stl_binder_noncopyable([](py::module &m) {
+    py::class_<E_nc>(m, "ENC")
+        .def(py::init<int>())
+        .def_readwrite("value", &E_nc::value);
+
+    py::bind_vector<std::vector<E_nc>>(m, "VectorENC");
+    m.def("get_vnc", &one_to_n<std::vector<E_nc>>, py::return_value_policy::reference);
+
+    py::bind_vector<std::deque<E_nc>>(m, "DequeENC");
+    m.def("get_dnc", &one_to_n<std::deque<E_nc>>, py::return_value_policy::reference);
+
+    py::bind_map<std::map<int, E_nc>>(m, "MapENC");
+    m.def("get_mnc", &times_ten<std::map<int, E_nc>>, py::return_value_policy::reference);
+
+    py::bind_map<std::unordered_map<int, E_nc>>(m, "UmapENC");
+    m.def("get_umnc", &times_ten<std::unordered_map<int, E_nc>>, py::return_value_policy::reference);
+});
+

tests/test_stl_binders.py

@@ -50,7 +50,6 @@ def test_vector_bool():
         assert vv_c[i] == (i % 2 == 0)
     assert str(vv_c) == "VectorBool[1, 0, 1, 0, 1, 0, 1, 0, 1, 0]"
 
-
 def test_map_string_double():
     from pybind11_tests import MapStringDouble, UnorderedMapStringDouble
 
@@ -97,3 +96,58 @@ def test_map_string_double_const():
     umc['b'] = 21.5
 
     str(umc)
+
+def test_noncopyable_vector():
+    from pybind11_tests import ENC, get_vnc
+
+    vnc = get_vnc(5)
+    for i in range(0, 5):
+        assert(vnc[i].value == i+1)
+
+    i = 1
+    for j in vnc:
+        assert(j.value == i)
+        i += 1
+
+def test_noncopyable_deque():
+    from pybind11_tests import ENC, get_dnc
+
+    dnc = get_dnc(5)
+    for i in range(0, 5):
+        assert(dnc[i].value == i+1)
+
+    i = 1
+    for j in dnc:
+        assert(j.value == i)
+        i += 1
+
+def test_noncopyable_map():
+    from pybind11_tests import ENC, get_mnc
+
+    mnc = get_mnc(5)
+    for i in range(1, 6):
+        assert(mnc[i].value == 10*i)
+
+    i = 1
+    vsum = 0
+    for k, v in mnc.items():
+        assert(v.value == 10*k)
+        vsum += v.value
+
+    assert(vsum == 150)
+
+def test_noncopyable_unordered_map():
+    from pybind11_tests import ENC, get_umnc
+
+    mnc = get_umnc(5)
+    for i in range(1, 6):
+        assert(mnc[i].value == 10*i)
+
+    i = 1
+    vsum = 0
+    for k, v in mnc.items():
+        assert(v.value == 10*k)
+        vsum += v.value
+
+    assert(vsum == 150)
+