Improve support for std::optional-like classes

With older versions of Boost, `value = {}` can fail to compile because
the interpretation of `{}` is ambiguous (could construct `boost::none`
or the value type). It can also fail with `std::experimental::optional`,
at least on GCC 5.4, because it would construct an instance of the
optional type and then move-assign it, and if the value type isn't move
assignable this would fail.

This is replaced by preferring `.reset` if the type supports it, and
also providing a traits class that can be extended to override the
behaviour where necessary (which is done for
std::experimental::optional).

Additionally, the assignment of the value from the inner caster was by
copy rather than move, which prevented use with uncopyable types.

Closes pybind#847.

Author: bmerry

docs/advanced/cast/stl.rst

@@ -46,8 +46,23 @@ types:
     }}
 
 The above should be placed in a header file and included in all translation units
-where automatic conversion is needed. Similarly, a specialization can be provided
-for custom variant types:
+where automatic conversion is needed. If the type provides a ``reset`` member
+function, that is used to clear the stored value; otherwise, it is cleared by
+assigning ``{}`` to it. If neither approach is suitable, ``optional_reset`` can
+be specialized to provide an alternative. Here is how pybind11 specializes it for
+``std::experimental::optional<>`` (which does not have a ``reset`` method):
+
+.. code-block:: cpp
+
+    namespace pybind11 { namespace detail {
+        template<typename T> struct optional_reset<std::experimental::optional<T>> {
+            static void reset(std::experimental::optional<T> &value) {
+                value = std::experimental::nullopt;
+            }
+        };
+    }} // namespace pybind11::detail
+
+Similarly, a specialization can be provided for custom variant types:
 
 .. code-block:: cpp
 

include/pybind11/stl.h

@@ -17,6 +17,7 @@
 #include <iostream>
 #include <list>
 #include <valarray>
+#include <utility>
 
 #if defined(_MSC_VER)
 #pragma warning(push)
@@ -228,6 +229,26 @@ template <typename Key, typename Value, typename Compare, typename Alloc> struct
 template <typename Key, typename Value, typename Hash, typename Equal, typename Alloc> struct type_caster<std::unordered_map<Key, Value, Hash, Equal, Alloc>>
   : map_caster<std::unordered_map<Key, Value, Hash, Equal, Alloc>, Key, Value> { };
 
+/// Helper class to reset an std::optional-like class in the best way.
+template<typename T> struct optional_reset {
+private:
+    // Either or both of these overloads may exist (SFINAE decides), and if
+    // both exist then the .reset version is preferred due to a better match
+    // in the second argument.
+    template<typename T2 = T>
+    static decltype(std::declval<T2>().reset()) reset_impl(T &value, int) {
+        return value.reset();
+    }
+
+    template<typename T2 = T>
+    static decltype(std::declval<T2>() = {}) reset_impl(T &value, long) {
+        return value = {};
+    }
+
+public:
+    static void reset(T &value) { reset_impl(value, 0); }
+};
+
 // This type caster is intended to be used for std::optional and std::experimental::optional
 template<typename T> struct optional_caster {
     using value_conv = make_caster<typename T::value_type>;
@@ -242,14 +263,14 @@ template<typename T> struct optional_caster {
         if (!src) {
             return false;
         } else if (src.is_none()) {
-            value = {};  // nullopt
+            optional_reset<T>::reset(value);
             return true;
         }
         value_conv inner_caster;
         if (!inner_caster.load(src, convert))
             return false;
 
-        value.emplace(cast_op<typename T::value_type>(inner_caster));
+        value.emplace(std::move(cast_op<typename T::value_type>(inner_caster)));
         return true;
     }
 
@@ -265,11 +286,20 @@ template<> struct type_caster<std::nullopt_t>
 #endif
 
 #if PYBIND11_HAS_EXP_OPTIONAL
+// std::experimental::optional doesn't have a reset method, and the value = {}
+// strategy can fail if the underlying type isn't move-assignable.
+template<typename T> struct optional_reset<std::experimental::optional<T>> {
+    static void reset(std::experimental::optional<T> &value) {
+        value = std::experimental::nullopt;
+    }
+};
+
 template<typename T> struct type_caster<std::experimental::optional<T>>
     : public optional_caster<std::experimental::optional<T>> {};
 
 template<> struct type_caster<std::experimental::nullopt_t>
     : public void_caster<std::experimental::nullopt_t> {};
+
 #endif
 
 /// Visit a variant and cast any found type to Python

tests/test_python_types.cpp

@@ -188,6 +188,18 @@ struct MoveOutContainer {
 
 struct UnregisteredType { };
 
+// Class that can be move-constructed, but not copied or move-assigned
+struct NoMoveAssign {
+    int value;
+
+    explicit NoMoveAssign(int value = 0) : value(value) {}
+    NoMoveAssign(NoMoveAssign &&) = default;
+
+    NoMoveAssign(const NoMoveAssign &) = delete;
+    NoMoveAssign &operator=(const NoMoveAssign &) = delete;
+    NoMoveAssign &operator=(NoMoveAssign &&) = delete;
+};
+
 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 */
@@ -236,6 +248,10 @@ test_initializer python_types([](py::module &m) {
         py::print("{a} + {b} = {c}"_s.format("a"_a="py::print", "b"_a="str.format", "c"_a="this"));
     });
 
+    py::class_<NoMoveAssign>(m, "NoMoveAssign", "Class that can only be move constructed")
+        .def(py::init<>())
+        .def(py::init<int>());
+
     m.def("test_print_failure", []() { py::print(42, UnregisteredType()); });
 #if !defined(NDEBUG)
     m.attr("debug_enabled") = true;
@@ -326,6 +342,7 @@ test_initializer python_types([](py::module &m) {
 #ifdef PYBIND11_HAS_OPTIONAL
     has_optional = true;
     using opt_int = std::optional<int>;
+    using opt_no_move_assign = std::optional<NoMoveAssign>;
     m.def("double_or_zero", [](const opt_int& x) -> int {
         return x.value_or(0) * 2;
     });
@@ -335,11 +352,15 @@ test_initializer python_types([](py::module &m) {
     m.def("test_nullopt", [](opt_int x) {
         return x.value_or(42);
     }, py::arg_v("x", std::nullopt, "None"));
+    m.def("test_no_move_assign", [](const opt_no_move_assign &x) {
+        return x ? x.value : 42;
+    }, py::argv_("x", std::nullopt, "None"));
 #endif
 
 #ifdef PYBIND11_HAS_EXP_OPTIONAL
     has_exp_optional = true;
     using exp_opt_int = std::experimental::optional<int>;
+    using exp_opt_no_move_assign = std::experimental::optional<NoMoveAssign>;
     m.def("double_or_zero_exp", [](const exp_opt_int& x) -> int {
         return x.value_or(0) * 2;
     });
@@ -349,6 +370,9 @@ test_initializer python_types([](py::module &m) {
     m.def("test_nullopt_exp", [](exp_opt_int x) {
         return x.value_or(42);
     }, py::arg_v("x", std::experimental::nullopt, "None"));
+    m.def("test_no_move_assign_exp", [](const exp_opt_no_move_assign &x) {
+        return x ? x->value : 42;
+    }, py::arg_v("x", std::experimental::nullopt, "None"));
 #endif
 
     m.attr("has_optional") = has_optional;

tests/test_python_types.py

@@ -337,7 +337,8 @@ def func(self, x, *args):
 
 @pytest.mark.skipif(not has_optional, reason='no <optional>')
 def test_optional():
-    from pybind11_tests import double_or_zero, half_or_none, test_nullopt
+    from pybind11_tests import (double_or_zero, half_or_none, test_nullopt,
+                                test_no_move_assign, NoMoveAssign)
 
     assert double_or_zero(None) == 0
     assert double_or_zero(42) == 84
@@ -352,10 +353,16 @@ def test_optional():
     assert test_nullopt(42) == 42
     assert test_nullopt(43) == 43
 
+    assert test_no_move_assign() == 42
+    assert test_no_move_assign(None) == 42
+    assert test_no_move_assign(NoMoveAssign(43)) == 43
+    pytest.raises(TypeError, test_no_move_assign, 43)
+
 
 @pytest.mark.skipif(not has_exp_optional, reason='no <experimental/optional>')
 def test_exp_optional():
-    from pybind11_tests import double_or_zero_exp, half_or_none_exp, test_nullopt_exp
+    from pybind11_tests import (double_or_zero_exp, half_or_none_exp, test_nullopt_exp,
+                                test_no_move_assign_exp, NoMoveAssign)
 
     assert double_or_zero_exp(None) == 0
     assert double_or_zero_exp(42) == 84
@@ -370,6 +377,11 @@ def test_exp_optional():
     assert test_nullopt_exp(42) == 42
     assert test_nullopt_exp(43) == 43
 
+    assert test_no_move_assign_exp() == 42
+    assert test_no_move_assign_exp(None) == 42
+    assert test_no_move_assign_exp(NoMoveAssign(43)) == 43
+    pytest.raises(TypeError, test_no_move_assign_exp, 43)
+
 
 @pytest.mark.skipif(not hasattr(pybind11_tests, "load_variant"), reason='no <variant>')
 def test_variant(doc):