From 6a308a428d1c58760cb9fbd56601917f810c4a91 Mon Sep 17 00:00:00 2001 From: Bruce Merry Date: Sun, 14 May 2017 12:05:03 +0200 Subject: [PATCH 1/3] 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 #847. --- docs/advanced/cast/stl.rst | 19 +++++++++++++++++-- include/pybind11/stl.h | 34 ++++++++++++++++++++++++++++++++-- tests/test_python_types.cpp | 24 ++++++++++++++++++++++++ tests/test_python_types.py | 16 ++++++++++++++-- 4 files changed, 87 insertions(+), 6 deletions(-) diff --git a/docs/advanced/cast/stl.rst b/docs/advanced/cast/stl.rst index 47c2a96705..304c8cc315 100644 --- a/docs/advanced/cast/stl.rst +++ b/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 struct optional_reset> { + static void reset(std::experimental::optional &value) { + value = std::experimental::nullopt; + } + }; + }} // namespace pybind11::detail + +Similarly, a specialization can be provided for custom variant types: .. code-block:: cpp diff --git a/include/pybind11/stl.h b/include/pybind11/stl.h index f37dd795e3..1dc3826b82 100644 --- a/include/pybind11/stl.h +++ b/include/pybind11/stl.h @@ -17,6 +17,7 @@ #include #include #include +#include #if defined(_MSC_VER) #pragma warning(push) @@ -228,6 +229,26 @@ template struct template struct type_caster> : map_caster, Key, Value> { }; +/// Helper class to reset an std::optional-like class in the best way. +template 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 + static decltype(std::declval().reset()) reset_impl(T &value, int) { + return value.reset(); + } + + template + static decltype(std::declval() = {}) 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 struct optional_caster { using value_conv = make_caster; @@ -242,14 +263,14 @@ template struct optional_caster { if (!src) { return false; } else if (src.is_none()) { - value = {}; // nullopt + optional_reset::reset(value); return true; } value_conv inner_caster; if (!inner_caster.load(src, convert)) return false; - value.emplace(cast_op(inner_caster)); + value.emplace(std::move(cast_op(inner_caster))); return true; } @@ -265,11 +286,20 @@ template<> struct type_caster #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 struct optional_reset> { + static void reset(std::experimental::optional &value) { + value = std::experimental::nullopt; + } +}; + template struct type_caster> : public optional_caster> {}; template<> struct type_caster : public void_caster {}; + #endif /// Visit a variant and cast any found type to Python diff --git a/tests/test_python_types.cpp b/tests/test_python_types.cpp index f9bda617cd..276d5ad2ee 100644 --- a/tests/test_python_types.cpp +++ b/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_(m, "NoMoveAssign", "Class that can only be move constructed") + .def(py::init<>()) + .def(py::init()); + 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; + using opt_no_move_assign = std::optional; 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; + using exp_opt_no_move_assign = std::experimental::optional; 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; diff --git a/tests/test_python_types.py b/tests/test_python_types.py index e427c4677b..e4aea4717a 100644 --- a/tests/test_python_types.py +++ b/tests/test_python_types.py @@ -337,7 +337,8 @@ def func(self, x, *args): @pytest.mark.skipif(not has_optional, reason='no ') 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 ') 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 ') def test_variant(doc): From 9ca6290d8eb8ad54ad03caf6d5182b97b8500910 Mon Sep 17 00:00:00 2001 From: Bruce Merry Date: Sun, 14 May 2017 12:56:57 +0200 Subject: [PATCH 2/3] Fix typos in std::optional test --- tests/test_python_types.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/test_python_types.cpp b/tests/test_python_types.cpp index 276d5ad2ee..7a3fefb38f 100644 --- a/tests/test_python_types.cpp +++ b/tests/test_python_types.cpp @@ -353,8 +353,8 @@ test_initializer python_types([](py::module &m) { 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")); + return x ? x->value : 42; + }, py::arg_v("x", std::nullopt, "None")); #endif #ifdef PYBIND11_HAS_EXP_OPTIONAL From 607915c4538a0a086d7e42e45f7455de456ec7ed Mon Sep 17 00:00:00 2001 From: Bruce Merry Date: Sun, 14 May 2017 19:48:31 +0200 Subject: [PATCH 3/3] Undo the attempt to support non-copyable std::optional It was moving the output of a caster, which is not safe because a caster can return an lvalue reference which it doesn't own. The wrapped type must now be copy-constructible, but does not need to be either move- or copy-assignable. --- include/pybind11/stl.h | 2 +- tests/test_python_types.cpp | 24 ++++++++++++------------ tests/test_python_types.py | 20 ++++++++++---------- 3 files changed, 23 insertions(+), 23 deletions(-) diff --git a/include/pybind11/stl.h b/include/pybind11/stl.h index 1dc3826b82..740d94bf58 100644 --- a/include/pybind11/stl.h +++ b/include/pybind11/stl.h @@ -270,7 +270,7 @@ template struct optional_caster { if (!inner_caster.load(src, convert)) return false; - value.emplace(std::move(cast_op(inner_caster))); + value.emplace(cast_op(inner_caster)); return true; } diff --git a/tests/test_python_types.cpp b/tests/test_python_types.cpp index 7a3fefb38f..f3375c67e5 100644 --- a/tests/test_python_types.cpp +++ b/tests/test_python_types.cpp @@ -188,16 +188,16 @@ struct MoveOutContainer { struct UnregisteredType { }; -// Class that can be move-constructed, but not copied or move-assigned -struct NoMoveAssign { +// Class that can be move- and copy-constructed, but not assigned +struct NoAssign { int value; - explicit NoMoveAssign(int value = 0) : value(value) {} - NoMoveAssign(NoMoveAssign &&) = default; + explicit NoAssign(int value = 0) : value(value) {} + NoAssign(const NoAssign &) = default; + NoAssign(NoAssign &&) = default; - NoMoveAssign(const NoMoveAssign &) = delete; - NoMoveAssign &operator=(const NoMoveAssign &) = delete; - NoMoveAssign &operator=(NoMoveAssign &&) = delete; + NoAssign &operator=(const NoAssign &) = delete; + NoAssign &operator=(NoAssign &&) = delete; }; test_initializer python_types([](py::module &m) { @@ -248,7 +248,7 @@ 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_(m, "NoMoveAssign", "Class that can only be move constructed") + py::class_(m, "NoAssign", "Class with no C++ assignment operators") .def(py::init<>()) .def(py::init()); @@ -342,7 +342,7 @@ test_initializer python_types([](py::module &m) { #ifdef PYBIND11_HAS_OPTIONAL has_optional = true; using opt_int = std::optional; - using opt_no_move_assign = std::optional; + using opt_no_assign = std::optional; m.def("double_or_zero", [](const opt_int& x) -> int { return x.value_or(0) * 2; }); @@ -352,7 +352,7 @@ 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) { + m.def("test_no_assign", [](const opt_no_assign &x) { return x ? x->value : 42; }, py::arg_v("x", std::nullopt, "None")); #endif @@ -360,7 +360,7 @@ test_initializer python_types([](py::module &m) { #ifdef PYBIND11_HAS_EXP_OPTIONAL has_exp_optional = true; using exp_opt_int = std::experimental::optional; - using exp_opt_no_move_assign = std::experimental::optional; + using exp_opt_no_assign = std::experimental::optional; m.def("double_or_zero_exp", [](const exp_opt_int& x) -> int { return x.value_or(0) * 2; }); @@ -370,7 +370,7 @@ 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) { + m.def("test_no_assign_exp", [](const exp_opt_no_assign &x) { return x ? x->value : 42; }, py::arg_v("x", std::experimental::nullopt, "None")); #endif diff --git a/tests/test_python_types.py b/tests/test_python_types.py index e4aea4717a..f7c3d5555c 100644 --- a/tests/test_python_types.py +++ b/tests/test_python_types.py @@ -338,7 +338,7 @@ def func(self, x, *args): @pytest.mark.skipif(not has_optional, reason='no ') def test_optional(): from pybind11_tests import (double_or_zero, half_or_none, test_nullopt, - test_no_move_assign, NoMoveAssign) + test_no_assign, NoAssign) assert double_or_zero(None) == 0 assert double_or_zero(42) == 84 @@ -353,16 +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) + assert test_no_assign() == 42 + assert test_no_assign(None) == 42 + assert test_no_assign(NoAssign(43)) == 43 + pytest.raises(TypeError, test_no_assign, 43) @pytest.mark.skipif(not has_exp_optional, reason='no ') def test_exp_optional(): from pybind11_tests import (double_or_zero_exp, half_or_none_exp, test_nullopt_exp, - test_no_move_assign_exp, NoMoveAssign) + test_no_assign_exp, NoAssign) assert double_or_zero_exp(None) == 0 assert double_or_zero_exp(42) == 84 @@ -377,10 +377,10 @@ 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) + assert test_no_assign_exp() == 42 + assert test_no_assign_exp(None) == 42 + assert test_no_assign_exp(NoAssign(43)) == 43 + pytest.raises(TypeError, test_no_assign_exp, 43) @pytest.mark.skipif(not hasattr(pybind11_tests, "load_variant"), reason='no ')