diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h index 891f1c9d18..6bc87a1854 100644 --- a/include/pybind11/cast.h +++ b/include/pybind11/cast.h @@ -188,6 +188,9 @@ class type_caster> { template using cast_op_type = std::reference_wrapper; explicit operator std::reference_wrapper() { return cast_op(subcaster); } + // The loaded reference points into `subcaster` unless `type` is a registered class. + static constexpr bool value_refers_to_caster + = !std::is_base_of::value; }; #define PYBIND11_TYPE_CASTER(type, py_name) \ @@ -1619,15 +1622,27 @@ struct move_if_unreferenced< template using move_never = none_of, move_if_unreferenced>; +// Whether a value loaded by make_caster can refer to data owned by that caster, so that the +// caster must outlive the value. Casters opt in with `static constexpr bool +// value_refers_to_caster = true;`: type_caster> when U is converted by +// value, and the STL casters (optional, variant, containers) holding such elements. +template +struct loaded_value_refers_to_caster : std::false_type {}; +template +struct loaded_value_refers_to_caster::value_refers_to_caster>> + : std::true_type {}; + // Detect whether returning a `type` from a cast on type's type_caster is going to result in a // reference or pointer to a local variable of the type_caster. Basically, only // non-reference/pointer `type`s and reference/pointers from a type_caster_generic are safe; -// everything else returns a reference/pointer to a local variable. +// everything else returns a reference/pointer to a local variable. The same holds for values +// that refer into their caster, e.g. `std::optional>`. template using cast_is_temporary_value_reference - = bool_constant<(std::is_reference::value || std::is_pointer::value) - && !std::is_base_of>::value - && !std::is_same, void>::value>; + = bool_constant<((std::is_reference::value || std::is_pointer::value) + && !std::is_base_of>::value + && !std::is_same, void>::value) + || loaded_value_refers_to_caster::value>; // 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 diff --git a/include/pybind11/stl.h b/include/pybind11/stl.h index 01be0b47c6..851a22543e 100644 --- a/include/pybind11/stl.h +++ b/include/pybind11/stl.h @@ -15,6 +15,7 @@ #include "detail/type_caster_base.h" #include +#include #include #include #include @@ -155,6 +156,55 @@ constexpr forwarded_type forward_like(U &&u) { template using has_reserve_method = std::is_same().reserve(0)), void>; +/// Loads values of type T with make_caster. Usually each caster is a local, discarded after its +/// value has been moved out. If the loaded value can refer into its caster +/// (loaded_value_refers_to_caster, e.g. std::reference_wrapper), the casters are +/// kept here instead, so they live as long as the caster that owns this store. +template ::value> +class subcaster_store { +public: + /// Loads `src` and, on success, calls `use(caster)` to take the value out of the caster. + template + bool load(handle src, bool convert, Use &&use) { + make_caster caster; + if (!caster.load(src, convert)) { + return false; + } + use(caster); + return true; + } + void clear() {} +}; + +template +class subcaster_store { +public: + template + bool load(handle src, bool convert, Use &&use) { + casters.emplace_front(); + if (!casters.front().load(src, convert)) { + casters.pop_front(); + return false; + } + use(casters.front()); + return true; + } + void clear() { casters.clear(); } + + subcaster_store() = default; + subcaster_store(subcaster_store &&) = default; + subcaster_store &operator=(subcaster_store &&) = default; + // A copy would hold values that still refer into the casters of the original. + subcaster_store(const subcaster_store &) = delete; + subcaster_store &operator=(const subcaster_store &) = delete; + ~subcaster_store() = default; + +private: + // Node-based: casters are constructed in place (they need not be movable), and their + // addresses do not change when the store is moved. + std::forward_list> casters; +}; + template struct set_caster { using type = Type; @@ -169,35 +219,42 @@ struct set_caster { bool convert_iterable(const iterable &itbl, bool convert) { for (const auto &it : itbl) { - key_conv conv; - if (!conv.load(it, convert)) { + if (!key_casters.load(it, convert, [this](key_conv &conv) { + value.insert(cast_op(std::move(conv))); + })) { return false; } - value.insert(cast_op(std::move(conv))); } return true; } bool convert_anyset(const anyset &s, bool convert) { - value.clear(); + clear(); reserve_maybe(s, &value); return convert_iterable(s, convert); } + void clear() { + value.clear(); + key_casters.clear(); + } + + subcaster_store key_casters; + public: bool load(handle src, bool convert) { if (!object_is_convertible_to_std_set(src, convert)) { return false; } if (isinstance(src)) { - value.clear(); + clear(); return convert_anyset(reinterpret_borrow(src), convert); } if (!convert) { return false; } assert(isinstance(src)); - value.clear(); + clear(); return convert_iterable(reinterpret_borrow(src), convert); } @@ -220,6 +277,7 @@ struct set_caster { PYBIND11_TYPE_CASTER(type, io_name("collections.abc.Set", "set") + const_name("[") + key_conv::name + const_name("]")); + static constexpr bool value_refers_to_caster = loaded_value_refers_to_caster::value; }; template @@ -236,18 +294,30 @@ struct map_caster { bool convert_elements(const dict &d, bool convert) { value.clear(); + key_casters.clear(); + value_casters.clear(); reserve_maybe(d, &value); for (const auto &it : d) { - key_conv kconv; - value_conv vconv; - if (!kconv.load(it.first.ptr(), convert) || !vconv.load(it.second.ptr(), convert)) { + bool value_loaded = false; + if (!key_casters.load(it.first.ptr(), + convert, + [&](key_conv &kconv) { + value_loaded = value_casters.load( + it.second.ptr(), convert, [&](value_conv &vconv) { + value.emplace(cast_op(std::move(kconv)), + cast_op(std::move(vconv))); + }); + }) + || !value_loaded) { return false; } - value.emplace(cast_op(std::move(kconv)), cast_op(std::move(vconv))); } return true; } + subcaster_store key_casters; + subcaster_store value_casters; + public: bool load(handle src, bool convert) { if (!object_is_convertible_to_std_map(src, convert)) { @@ -293,6 +363,8 @@ struct map_caster { io_name("collections.abc.Mapping", "dict") + const_name("[") + key_conv::name + const_name(", ") + value_conv::name + const_name("]")); + static constexpr bool value_refers_to_caster + = loaded_value_refers_to_caster::value || loaded_value_refers_to_caster::value; }; template @@ -327,17 +399,20 @@ struct list_caster { bool convert_elements(handle seq, bool convert) { auto s = reinterpret_borrow(seq); value.clear(); + value_casters.clear(); reserve_maybe(s, &value); for (const auto &it : seq) { - value_conv conv; - if (!conv.load(it, convert)) { + if (!value_casters.load(it, convert, [this](value_conv &conv) { + value.push_back(cast_op(std::move(conv))); + })) { return false; } - value.push_back(cast_op(std::move(conv))); } return true; } + subcaster_store value_casters; + public: template static handle cast(T &&src, return_value_policy policy, handle parent) { @@ -360,6 +435,7 @@ struct list_caster { PYBIND11_TYPE_CASTER(Type, io_name("collections.abc.Sequence", "list") + const_name("[") + value_conv::name + const_name("]")); + static constexpr bool value_refers_to_caster = loaded_value_refers_to_caster::value; }; template @@ -388,11 +464,13 @@ struct array_caster { private: std::unique_ptr value; + subcaster_store value_casters; template = 0> bool convert_elements(handle seq, bool convert) { auto l = reinterpret_borrow(seq); value.reset(new ArrayType{}); + value_casters.clear(); // Using `resize` to preserve the behavior exactly as it was before PR #5305 // For the `resize` to work, `Value` must be default constructible. // For `std::valarray`, this is a requirement: @@ -400,11 +478,11 @@ struct array_caster { value->resize(l.size()); size_t ctr = 0; for (const auto &it : l) { - value_conv conv; - if (!conv.load(it, convert)) { + if (!value_casters.load(it, convert, [&](value_conv &conv) { + (*value)[ctr++] = cast_op(std::move(conv)); + })) { return false; } - (*value)[ctr++] = cast_op(std::move(conv)); } return true; } @@ -420,14 +498,16 @@ struct array_caster { // Deliberate choice: no template specializations, for simplicity, and // because the compile time overhead for the specializations is deemed // more significant than the runtime overhead for the `temp` storage. + value.reset(); + value_casters.clear(); std::vector temp; temp.reserve(l.size()); for (auto it : l) { - value_conv conv; - if (!conv.load(it, convert)) { + if (!value_casters.load(it, convert, [&](value_conv &conv) { + temp.emplace_back(cast_op(std::move(conv))); + })) { return false; } - temp.emplace_back(cast_op(std::move(conv))); } value.reset(new ArrayType(vector_to_array(std::move(temp)))); return true; @@ -499,6 +579,7 @@ struct array_caster { + const_name(const_name(""), const_name(", \"FixedSize(") + const_name() + const_name(")\"]")); + static constexpr bool value_refers_to_caster = loaded_value_refers_to_caster::value; }; template @@ -545,19 +626,28 @@ struct optional_caster { if (!src) { return false; } - if (src.is_none()) { - return true; // default-constructed value is already empty + if (value) { + // Empty it without assigning: Value need not be assignable, and + // std::experimental::optional has no reset(). + value.~Type(); + new (&value) Type(); } - value_conv inner_caster; - if (!inner_caster.load(src, convert)) { - return false; + value_casters.clear(); + if (src.is_none()) { + return true; } - - value.emplace(cast_op(std::move(inner_caster))); - return true; + // The inner caster is only constructed for non-None values: some casters have costly + // or failing constructors (e.g. py::array_t imports NumPy). + return value_casters.load(src, convert, [this](value_conv &conv) { + value.emplace(cast_op(std::move(conv))); + }); } PYBIND11_TYPE_CASTER(Type, value_conv::name | make_caster::name); + static constexpr bool value_refers_to_caster = loaded_value_refers_to_caster::value; + +private: + subcaster_store value_casters; }; #if defined(PYBIND11_HAS_OPTIONAL) @@ -613,9 +703,10 @@ struct variant_caster> { template bool load_alternative(handle src, bool convert, type_list) { - auto caster = make_caster(); - if (caster.load(src, convert)) { - value = cast_op(std::move(caster)); + auto &casters = std::get(alternative_casters); + if (casters.load(src, convert, [this](make_caster &caster) { + value = cast_op(std::move(caster)); + })) { return true; } return load_alternative(src, convert, type_list{}); @@ -624,6 +715,7 @@ struct variant_caster> { bool load_alternative(handle, bool, type_list<>) { return false; } bool load(handle src, bool convert) { + clear_alternative_casters(make_index_sequence{}); // Do a first pass without conversions to improve constructor resolution. // E.g. `py::int_(1).cast>()` needs to fill the `int` // slot of the variant. Without two-pass loading `double` would be filled @@ -642,6 +734,16 @@ struct variant_caster> { using Type = V; PYBIND11_TYPE_CASTER(Type, ::pybind11::detail::union_concat(make_caster::name...)); + static constexpr bool value_refers_to_caster + = any_of...>::value; + +private: + template + void clear_alternative_casters(index_sequence) { + PYBIND11_EXPAND_SIDE_EFFECTS(std::get(alternative_casters).clear()); + } + + std::tuple...> alternative_casters; }; #if defined(PYBIND11_HAS_VARIANT) diff --git a/tests/test_stl.cpp b/tests/test_stl.cpp index b5d1979d04..73d09b0f88 100644 --- a/tests/test_stl.cpp +++ b/tests/test_stl.cpp @@ -18,6 +18,10 @@ #include +#include +#include +#include +#include #include #include @@ -35,6 +39,120 @@ struct type_caster : void_caster {}; } // namespace PYBIND11_NAMESPACE #endif +// Converted by value (not a registered class), to check that a loaded +// std::reference_wrapper (in an optional, variant or container) does not refer to a +// destroyed object. Each instance gets a new id; the live instances are tracked by address and id, +// so a reference is checked without reading the object, and an address reused by a later +// instance is not mistaken for the original. Instances are created and destroyed on the thread +// of the call, hence thread_local rather than shared state. +struct LiveTracked { + static std::map &live() { + static thread_local std::map instances; + return instances; + } + // Ids of the instances filled by type_caster::load(), in load order. + static std::vector &loaded() { + static thread_local std::vector ids; + return ids; + } + static unsigned next_id() { + static thread_local unsigned id = 0; + return ++id; + } + int value = 0; + LiveTracked() { live()[this] = next_id(); } + LiveTracked(const LiveTracked &other) : value(other.value) { live()[this] = next_id(); } + LiveTracked &operator=(const LiveTracked &) = default; + ~LiveTracked() { live().erase(this); } +}; + +static bool operator<(const LiveTracked &a, const LiveTracked &b) { return a.value < b.value; } + +// The values of `refs`, or "dangling" unless each one refers to a distinct live instance that was +// loaded since the previous check. +static py::object live_values(const std::vector &refs) { + std::vector loaded; + loaded.swap(LiveTracked::loaded()); + std::set seen; + py::list values; + for (const auto *ref : refs) { + auto it = LiveTracked::live().find(ref); + if (it == LiveTracked::live().end() + || std::find(loaded.begin(), loaded.end(), it->second) == loaded.end() + || !seen.insert(it->second).second) { + return py::str("dangling"); + } + values.append(ref->value); + } + return py::object(std::move(values)); +} + +// live_values() of a container of std::reference_wrapper +template +py::object live_values_of(const Container &refs) { + std::vector ptrs; + ptrs.reserve(refs.size()); + for (const auto &ref : refs) { + ptrs.push_back(&ref.get()); + } + return live_values(ptrs); +} + +namespace PYBIND11_NAMESPACE { +namespace detail { +template <> +struct type_caster { + PYBIND11_TYPE_CASTER(LiveTracked, const_name("int")); + bool load(handle src, bool) { + if (!isinstance(src)) { + return false; + } + value.value = src.cast(); + LiveTracked::loaded().push_back(LiveTracked::live().at(&value)); + return true; + } + static handle cast(const LiveTracked &src, return_value_policy, handle) { + return int_(src.value).release(); + } +}; +} // namespace detail +} // namespace PYBIND11_NAMESPACE + +// Converted by value with a caster that counts its constructions and is not movable, to check +// that std::optional constructs the inner caster only for non-None values and stays movable. +struct NonMovableCasterItem { + int value = 0; +}; + +namespace PYBIND11_NAMESPACE { +namespace detail { +template <> +struct type_caster { + PYBIND11_TYPE_CASTER(NonMovableCasterItem, const_name("int")); + static int &constructions() { + static thread_local int count = 0; + return count; + } + type_caster() { ++constructions(); } + type_caster(const type_caster &) = delete; + type_caster(type_caster &&) = delete; + type_caster &operator=(const type_caster &) = delete; + type_caster &operator=(type_caster &&) = delete; + ~type_caster() = default; + bool load(handle src, bool) { + if (!isinstance(src)) { + return false; + } + value.value = src.cast(); + return true; + } + static handle cast(const NonMovableCasterItem &src, return_value_policy, handle) { + return int_(src.value).release(); + } +}; +} // namespace detail +} // namespace PYBIND11_NAMESPACE + // Test with `std::variant` in C++17 mode, or with `boost::variant` in C++11/14 #if defined(PYBIND11_HAS_VARIANT) using std::variant; @@ -357,6 +475,59 @@ TEST_SUBMODULE(stl, m) { .def(pybind11::init<>()) .def_property_readonly("access_by_ref", &opt_props::access_by_ref) .def_property_readonly("access_by_copy", &opt_props::access_by_copy); + + // #5637: the referenced value must outlive the inner caster used during load + m.def("optional_refwrap_value", + [](std::optional> x) -> py::object { + if (!x) { + return py::none(); + } + return live_values({&x->get()}); + }); + m.def("vector_optional_refwrap_values", + [](const std::vector>> &v) { + std::vector refs; + for (const auto &x : v) { + if (x) { + refs.push_back(&x->get()); + } + } + return live_values(refs); + }); + + // Loading None into a caster that holds a value releases the value and its inner caster + m.def("optional_refwrap_reload", []() { + auto live_before = LiveTracked::live().size(); + py::detail::make_caster>> caster; + auto &value + = py::detail::cast_op> &>(caster); + bool loaded_int = caster.load(py::int_(5), true) && value && value->get().value == 5; + bool loaded_none = caster.load(py::none(), true) && !value; + LiveTracked::loaded().clear(); + return py::make_tuple(loaded_int, loaded_none, LiveTracked::live().size() == live_before); + }); + + // Without a reference_wrapper element, the optional caster holds no inner caster and stays + // copyable (e.g. for user casters derived from it). + static_assert(std::is_copy_constructible>>::value, + ""); + static_assert( + !py::detail::cast_is_temporary_value_reference>::value, ""); + // py::cast() would return a value referring into a destroyed caster, so it is rejected at + // compile time (by the same static_assert as for references to by-value types). + static_assert(py::detail::cast_is_temporary_value_reference< + std::optional>>::value, + ""); + + // The inner caster is constructed lazily (not for None) and the optional caster stays + // movable, as required by cast>(), even if the inner caster is not. + m.def("optional_nonmovable_caster_constructions", + []() { return py::detail::make_caster::constructions(); }); + m.def("optional_nonmovable_caster_arg", [](std::optional x) { + return x ? py::object(py::int_(x->value)) : py::object(py::none()); + }); + m.def("optional_nonmovable_caster_cast", + [](const py::object &o) { return o.cast>(); }); #endif #ifdef PYBIND11_HAS_EXP_OPTIONAL @@ -537,6 +708,18 @@ TEST_SUBMODULE(stl, m) { }); # if defined(PYBIND11_HAS_VARIANT) + // #5637: same for a std::reference_wrapper alternative + m.def("variant_refwrap_value", + [](const std::variant> &v) { + if (v.index() == 0) { + return py::object(py::str(std::get<0>(v))); + } + return live_values({&std::get<1>(v).get()}); + }); + static_assert(py::detail::cast_is_temporary_value_reference< + std::variant>>::value, + ""); + // std::monostate tests. m.def("load_monostate_variant", [](const variant &v) -> const char * { @@ -572,6 +755,38 @@ TEST_SUBMODULE(stl, m) { std::ref(p1), std::ref(p2), std::ref(p3), p4}; }); + // #5637: containers of std::reference_wrapper to by-value types keep the element casters + m.def("vector_refwrap_values", [](const std::vector> &v) { + return live_values_of(v); + }); + m.def("array_refwrap_values", [](const std::array, 3> &a) { + return live_values_of(a); + }); + m.def("set_refwrap_values", [](const std::set> &s) { + return live_values_of(s); + }); + m.def("map_refwrap_values", + [](const std::map> &d) { + std::vector refs; + refs.reserve(d.size()); + for (const auto &kv : d) { + refs.push_back(&kv.second.get()); + } + return live_values(refs); + }); + static_assert(py::detail::cast_is_temporary_value_reference< + std::vector>>::value, + ""); + static_assert(py::detail::cast_is_temporary_value_reference< + std::map>>::value, + ""); + // References to registered types point to the Python object, not into the caster + static_assert(!py::detail::cast_is_temporary_value_reference< + std::vector>>::value, + ""); + static_assert(std::is_copy_constructible>>::value, + ""); + // test_stl_pass_by_pointer m.def("stl_pass_by_pointer", [](std::vector *v) { return *v; }, "v"_a = nullptr); diff --git a/tests/test_stl.py b/tests/test_stl.py index f3f4ccc736..774f1c177e 100644 --- a/tests/test_stl.py +++ b/tests/test_stl.py @@ -293,6 +293,28 @@ def test_optional(): assert int(props.access_by_copy) == 42 +@pytest.mark.skipif(not hasattr(m, "has_optional"), reason="no ") +def test_optional_reference_wrapper_to_converted_value(): + # #5637: used to refer to a value owned by an already destroyed caster + assert m.optional_refwrap_value(None) is None + assert m.optional_refwrap_value(7) == [7] + assert m.vector_optional_refwrap_values([1, None, 2, 3]) == [1, 2, 3] + # loading None after a value empties the optional and drops the inner caster + assert m.optional_refwrap_reload() == (True, True, True) + + +@pytest.mark.skipif(not hasattr(m, "has_optional"), reason="no ") +def test_optional_inner_caster_lazy_and_nonmovable(): + count = m.optional_nonmovable_caster_constructions + before = count() + assert m.optional_nonmovable_caster_arg(None) is None + assert m.optional_nonmovable_caster_cast(None) is None + assert count() == before # no inner caster for None + assert m.optional_nonmovable_caster_arg(3) == 3 + assert m.optional_nonmovable_caster_cast(4) == 4 + assert count() == before + 2 + + @pytest.mark.skipif( not hasattr(m, "has_exp_optional"), reason="no " ) @@ -500,6 +522,23 @@ def test_variant_monostate(doc): ) +def test_containers_of_reference_wrapper_to_converted_value(): + # #5637: same as for std::optional, for each element + assert m.vector_refwrap_values([1, 2, 3]) == [1, 2, 3] + assert m.vector_refwrap_values([]) == [] + assert m.array_refwrap_values([4, 5, 6]) == [4, 5, 6] + assert m.set_refwrap_values({9, 7, 8}) == [7, 8, 9] + assert m.map_refwrap_values({"a": 1, "b": 2}) == [1, 2] + with pytest.raises(TypeError): + m.vector_refwrap_values([1, "x"]) + + +@pytest.mark.skipif(not hasattr(m, "variant_refwrap_value"), reason="no ") +def test_variant_reference_wrapper_to_converted_value(): + assert m.variant_refwrap_value(5) == [5] + assert m.variant_refwrap_value("s") == "s" + + def test_vec_of_reference_wrapper(): """#171: Can't return reference wrappers (or STL structures containing them)""" assert (