diff --git a/docs/advanced/cast/stl.rst b/docs/advanced/cast/stl.rst index 6abe8b467d..271dc8b29f 100644 --- a/docs/advanced/cast/stl.rst +++ b/docs/advanced/cast/stl.rst @@ -25,6 +25,19 @@ In particular, containers of C++ string views have additional Arbitrary nesting of any of these types is possible. +.. note:: + + The optional headers that provide type casters (:file:`pybind11/stl.h`, + :file:`pybind11/complex.h`, ...) must be visible consistently wherever the + casters or traits they provide affect a conversion. If such a header + affects the conversion of a type in one translation unit of an extension + module, it must be included in every translation unit that converts that + type. Otherwise the C++ One Definition Rule (ODR) is violated. This applies + to :file:`pybind11/complex.h` for a ``std::array<>`` of ``std::complex<>`` + even if ``std::complex<>`` is :ref:`opaque ` or has a custom type + caster, because the conversion of such an array depends on a trait that + the header specializes. + .. seealso:: The file :file:`tests/test_stl.cpp` contains a complete diff --git a/include/pybind11/detail/common.h b/include/pybind11/detail/common.h index b35147add7..a1b7155e6a 100644 --- a/include/pybind11/detail/common.h +++ b/include/pybind11/detail/common.h @@ -1363,6 +1363,8 @@ PYBIND11_NAMESPACE_BEGIN(detail) // complex float,double,long double. Note that the long double types only participate when long // double is actually longer than double (it isn't under MSVC). // NB: not only the string below but also complex.h and numpy.h rely on this order. +// `array_caster` in stl.h relies on `value`: it assumes that a numeric type is a plain value, for +// which default-constructing, copying and assigning have no observable effect beyond the value. template struct is_fmt_numeric { static constexpr bool value = false; diff --git a/include/pybind11/stl.h b/include/pybind11/stl.h index 0790074534..884696b7d9 100644 --- a/include/pybind11/stl.h +++ b/include/pybind11/stl.h @@ -394,49 +394,98 @@ ArrayType vector_to_array(V &&v) { return vector_to_array_impl(std::forward(v), make_index_sequence{}); } +// The element types for which `array_caster` assigns the elements of a `std::array`, instead of +// constructing them: types for which that makes no difference. +// * Scalar types, which have no constructors, assignment operators or destructors. +// * The numeric types (`is_fmt_numeric`), which are assumed to be plain values. They include +// `std::complex` (with pybind11/complex.h). +// * `std::array`s of such types. +// Other class types are always constructed. Type traits cannot identify the ones for which +// assigning would be equivalent: they miss constructor templates that take over from the copy +// and move constructors, and the difference between direct- and copy-initialization. +template +struct is_plain_array_element : any_of, is_fmt_numeric> {}; +template +struct is_plain_array_element> : is_plain_array_element {}; + template struct array_caster { using value_conv = make_caster; private: + // On the heap because `ArrayType` is not necessarily default constructible. std::unique_ptr value; - template = 0> + // A `std::array` is aggregate-initialized from a temporary `std::vector`, which needs an + // `index_sequence` of `Size` elements: very slow to compile for large `Size` (#5765). + // Assigning to the elements of a default-constructed array avoids that, but is used only + // for the `Value` types for which it makes no difference: `is_plain_array_element`. + // (In general it does: `boost::multi_array`, for example, does not resize on assignment.) bool convert_elements(handle seq, bool convert) { + // Not in class scope: a `value_conv` that is only used for `cast()` does not need to + // have a `cast_op_type`. + using value_cast_op_type = typename value_conv::template cast_op_type; + // The caster also has to yield a `Value`, rather than something that converts to it, and + // the assignment has to be possible: it is not for a `std::array` of `const` elements. + using assign_elements = bool_constant< + Resizable + || (is_plain_array_element::value + && std::is_same>, + remove_cv_t>::value + && std::is_assignable::value)>; + return convert_elements(seq, convert, assign_elements{}); + } + + // Returns a default-constructed array for `size` elements, or `nullptr` if `size` is not + // acceptable. `ArrayType()` rather than `ArrayType{}`: the latter is ill-formed for `Value` + // types with an explicit default constructor. + static std::unique_ptr make_array(size_t size, std::true_type /*Resizable*/) { + std::unique_ptr array(new ArrayType()); + array->resize(size); + return array; + } + static std::unique_ptr make_array(size_t size, std::false_type /*Resizable*/) { + return std::unique_ptr(size == Size ? new ArrayType() : nullptr); + } + + // Both overloads below index `l` up to a size that is determined only once, rather than + // iterating over `l`: the accesses stay in bounds even if the length of `l` changes + // underneath (e.g. if another thread resizes a list in a free-threaded build). + // `item` is an accessor: the item is fetched by `item.ptr()`, that is after `conv` is + // constructed, as when iterating over `l`. It keeps the item alive while `conv` is in use. + // Passing `item.ptr()` rather than `item` avoids a temporary `object`. + + bool convert_elements(handle seq, bool convert, std::true_type /*assign_elements*/) { auto l = reinterpret_borrow(seq); - value.reset(new ArrayType{}); - // 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: - // https://en.cppreference.com/w/cpp/named_req/NumericType - value->resize(l.size()); - size_t ctr = 0; - for (const auto &it : l) { + const size_t size = l.size(); + auto result = make_array(size, bool_constant{}); + if (!result) { + return false; + } + for (size_t i = 0; i < size; i++) { + const auto item = l[i]; value_conv conv; - if (!conv.load(it, convert)) { + if (!conv.load(item.ptr(), convert)) { return false; } - (*value)[ctr++] = cast_op(std::move(conv)); + (*result)[i] = cast_op(std::move(conv)); } + value = std::move(result); return true; } - template = 0> - bool convert_elements(handle seq, bool convert) { + bool convert_elements(handle seq, bool convert, std::false_type /*assign_elements*/) { auto l = reinterpret_borrow(seq); - if (l.size() != Size) { + const size_t size = l.size(); + if (size != Size) { return false; } - // The `temp` storage is needed to support `Value` types that are not - // default-constructible. - // 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. std::vector temp; - temp.reserve(l.size()); - for (auto it : l) { + temp.reserve(size); + for (size_t i = 0; i < size; i++) { + const auto item = l[i]; value_conv conv; - if (!conv.load(it, convert)) { + if (!conv.load(item.ptr(), convert)) { return false; } temp.emplace_back(cast_op(std::move(conv))); diff --git a/tests/test_stl.cpp b/tests/test_stl.cpp index b5d1979d04..e359fa7a97 100644 --- a/tests/test_stl.cpp +++ b/tests/test_stl.cpp @@ -7,6 +7,7 @@ BSD-style license that can be found in the LICENSE file. */ +#include #include #include "constructor_stats.h" @@ -18,7 +19,12 @@ #include +#include +#include +#include +#include #include +#include #include #if defined(PYBIND11_TEST_BOOST) @@ -62,6 +68,29 @@ struct visit_helper { PYBIND11_MAKE_OPAQUE(std::vector>) +// test_array_nested: an array with `const` elements cannot be assigned. As an element of another +// array it has to be constructed. +using OpaqueConstIntArray = std::array; +PYBIND11_MAKE_OPAQUE(OpaqueConstIntArray) + +// An element type with a caster that only supports the C++ to Python direction. +struct CastOnlyElement { + int val; +}; + +namespace PYBIND11_NAMESPACE { +namespace detail { +template <> +struct type_caster { + static constexpr auto name = const_name("int"); + + static handle cast(const CastOnlyElement &src, return_value_policy, handle) { + return int_(src.val).release(); + } +}; +} // namespace detail +} // namespace PYBIND11_NAMESPACE + /// Issue #528: templated constructor struct TplCtorClass { template @@ -169,6 +198,23 @@ struct type_caster> } // namespace detail } // namespace PYBIND11_NAMESPACE +// test_array_element_types (at namespace scope because local classes cannot have member templates) +// Trivially copyable, but a constructor template takes over the construction from a non-const +// lvalue, which is what the caster for a registered type yields. It adds 100, idempotently. +struct CtorTemplateLvalue { + CtorTemplateLvalue() = default; + explicit CtorTemplateLvalue(int val) : val{val} {} + template + // NOLINTNEXTLINE(bugprone-forwarding-reference-overload, google-explicit-constructor) + CtorTemplateLvalue(U &&other) : val{other.val % 100 + 100} {} + int val; +}; + +template +std::vector array_element_vals(const std::array &a) { + return {a[0].val, a[1].val}; +} + int pass_std_vector_int(const std::vector &v) { int zum = 100; for (const int i : v) { @@ -215,16 +261,100 @@ TEST_SUBMODULE(stl, m) { }; // test_array_no_default_ctor - py::class_(m, "NoDefaultCtor").def_readonly("val", &NoDefaultCtor::val); + py::class_(m, "NoDefaultCtor") + .def(py::init()) + .def_readonly("val", &NoDefaultCtor::val); py::class_(m, "NoDefaultCtorArray") .def(py::init()) .def_readwrite("arr", &NoDefaultCtorArray::arr); + // test_array_element_types + // `array_caster` constructs the elements of a `std::array`, except for the element types + // for which assigning them instead cannot make a difference: `is_plain_array_element`. + // The class types below must all be constructed. They are trivially copyable, though, apart + // from `AssignIsNoOp`. + enum class ArrayElementEnum {}; + static_assert(py::detail::is_plain_array_element::value, ""); + static_assert(py::detail::is_plain_array_element::value, ""); + static_assert(py::detail::is_plain_array_element::value, ""); + static_assert(py::detail::is_plain_array_element>::value, ""); + static_assert(py::detail::is_plain_array_element< + std::array, 2>, 3>>::value, + ""); + static_assert(!py::detail::is_plain_array_element::value, ""); + static_assert(!py::detail::is_plain_array_element::value, ""); + static_assert(!py::detail::is_plain_array_element>::value, ""); + // Move assignable, but the elements of an array cannot be assigned: the caster for a type + // registered with `py::class_` yields an lvalue reference. + struct NotCopyAssignable { + NotCopyAssignable() = default; + explicit NotCopyAssignable(int val) : val{val} {} + NotCopyAssignable(const NotCopyAssignable &) = default; + NotCopyAssignable &operator=(const NotCopyAssignable &) = delete; + NotCopyAssignable &operator=(NotCopyAssignable &&) = default; + int val; + }; + // Assigning is not equivalent to copy constructing, in the manner of `boost::multi_array`, + // which does not resize on assignment. + struct AssignIsNoOp { + AssignIsNoOp() = default; + explicit AssignIsNoOp(int val) : val{val} {} + AssignIsNoOp(const AssignIsNoOp &) = default; + AssignIsNoOp &operator=(const AssignIsNoOp &) { return *this; } + int val; + }; + // Trivially copyable, but the default constructor must not be called. + struct ThrowingDefaultCtor { + ThrowingDefaultCtor() { throw std::runtime_error("ThrowingDefaultCtor"); } + explicit ThrowingDefaultCtor(int val) : val{val} {} + int val = 0; + }; + py::enum_(m, "ArrayElementEnum"); + py::class_(m, "NotCopyAssignable").def(py::init()); + py::class_(m, "AssignIsNoOp").def(py::init()); + py::class_(m, "ThrowingDefaultCtor").def(py::init()); + py::class_(m, "CtorTemplateLvalue").def(py::init()); + m.def("load_array_enum", [](const std::array &a) { + return std::vector{static_cast(a[0]), static_cast(a[1])}; + }); + m.def("load_array_not_copy_assignable", &array_element_vals); + m.def("load_array_assign_is_no_op", &array_element_vals); + m.def("load_array_throwing_default_ctor", &array_element_vals); + m.def("load_array_no_default_ctor", &array_element_vals); + m.def("load_array_ctor_template_lvalue", &array_element_vals); + + m.def("cast_array_cast_only_element", []() { + return std::array{{CastOnlyElement{1}, CastOnlyElement{2}}}; + }); + + // test_array_nested + py::class_(m, "OpaqueConstIntArray"); + m.def("make_opaque_const_int_array", []() { return OpaqueConstIntArray{{3, 4}}; }); + m.def("load_array_opaque_const_int_array", + [](const std::array &a) { return a[0][0] + a[0][1]; }); + m.def("nested_numeric_array_cast_sequence", + [](std::array, 2>, 2> x) { return x; }); + m.def("nested_array_cast_sequence", + [](std::array, 2> x) { return x; }); + + // test_array_large + // #5765: this was very slow to compile when `array_caster` needed an `index_sequence` of + // `Size` elements for all element types. + m.def("sum_large_array", [](const std::array &a) { + return std::accumulate(a.begin(), a.end(), 0LL); + }); + // The elements of all numeric types (`is_fmt_numeric`) are assigned. `std::complex` is the + // one numeric type that is not trivially default constructible. + m.def("sum_large_complex_array", [](const std::array, 102400> &a) { + return std::accumulate(a.begin(), a.end(), std::complex()); + }); + // test_valarray m.def("cast_valarray", []() { return std::valarray{1, 4, 9}; }); m.def("load_valarray", [](const std::valarray &v) { return v.size() == 3 && v[0] == 1 && v[1] == 4 && v[2] == 9; }); + m.def("valarray_cast_sequence", [](std::valarray x) { return x; }); // test_map m.def("cast_map", []() { return std::map{{"key", "value"}}; }); diff --git a/tests/test_stl.py b/tests/test_stl.py index 32ee539f50..459dab3bf7 100644 --- a/tests/test_stl.py +++ b/tests/test_stl.py @@ -193,6 +193,88 @@ def test_array_no_default_ctor(): assert [e.val for e in lst.arr] == [13, 23] lst.arr = m.NoDefaultCtorArray(4).arr assert [e.val for e in lst.arr] == [14, 24] + lst.arr = (m.NoDefaultCtor(i) for i in (5, 6)) + assert [e.val for e in lst.arr] == [5, 6] + with pytest.raises(TypeError): + lst.arr = [m.NoDefaultCtor(7), 8] + assert [e.val for e in lst.arr] == [5, 6] + + +@pytest.mark.parametrize( + ("load", "item_type", "offset"), + [ + pytest.param( + m.load_array_not_copy_assignable, + m.NotCopyAssignable, + 0, + id="not_copy_assignable", + ), + pytest.param( + m.load_array_assign_is_no_op, m.AssignIsNoOp, 0, id="assign_is_no_op" + ), + pytest.param( + m.load_array_throwing_default_ctor, + m.ThrowingDefaultCtor, + 0, + id="throwing_default_ctor", + ), + pytest.param( + m.load_array_no_default_ctor, m.NoDefaultCtor, 0, id="no_default_ctor" + ), + pytest.param( + m.load_array_ctor_template_lvalue, + m.CtorTemplateLvalue, + 100, + id="ctor_template_lvalue", + ), + ], +) +def test_array_element_types(load, item_type, offset): + # `offset` is what the constructor template adds when the elements are constructed. + assert load([item_type(1), item_type(2)]) == [1 + offset, 2 + offset] + with pytest.raises(TypeError): + load([item_type(1)]) + with pytest.raises(TypeError): + load([item_type(1), 2]) + + +def test_array_cast_only_element(): + assert m.cast_array_cast_only_element() == [1, 2] + + +def test_array_nested(): + assert m.nested_array_cast_sequence([("a", "b"), ["c", "d"]]) == [ + ["a", "b"], + ["c", "d"], + ] + with pytest.raises(TypeError): + m.nested_array_cast_sequence([("a", "b"), ("c",)]) + assert m.nested_numeric_array_cast_sequence([(1, 2j), [3.5, 4]]) == [ + [1, 2j], + [3.5, 4], + ] + with pytest.raises(TypeError): + m.nested_numeric_array_cast_sequence([(1, 2j), [3.5]]) + assert m.load_array_opaque_const_int_array([m.make_opaque_const_int_array()]) == 7 + + +def test_array_large(): + size = 102400 + assert m.sum_large_array(range(size)) == (size - 1) * size // 2 + with pytest.raises(TypeError): + m.sum_large_array(range(size - 1)) + with pytest.raises(TypeError): + m.sum_large_array([0] * (size - 1) + ["x"]) + + total = (size - 1) * size // 2 + assert m.sum_large_complex_array([complex(i, -i) for i in range(size)]) == complex( + total, -total + ) + assert m.sum_large_complex_array(range(size)) == total + with pytest.raises(TypeError): + m.sum_large_complex_array(range(size - 1)) + with pytest.raises(TypeError): + m.sum_large_complex_array([0j] * (size - 1) + ["x"]) def test_valarray(doc): @@ -201,6 +283,10 @@ def test_valarray(doc): assert lst == [1, 4, 9] assert m.load_valarray(lst) assert m.load_valarray(tuple(lst)) + assert m.valarray_cast_sequence(i * i for i in range(4)) == [0, 1, 4, 9] + assert m.valarray_cast_sequence([]) == [] + with pytest.raises(TypeError): + m.valarray_cast_sequence([1, "x"]) assert doc(m.cast_valarray) == "cast_valarray() -> list[int]" assert ( @@ -585,6 +671,65 @@ def test_array_cast_sequence(): assert m.array_cast_sequence((1, 2, 3)) == [1, 2, 3] +# (load, item_type, size): one entry for each of the element loops in `array_caster`. +ARRAY_CASTER_LOADS = [ + pytest.param(m.load_array_enum, m.ArrayElementEnum, 2, id="array_assign"), + pytest.param(m.valarray_cast_sequence, int, 3, id="valarray"), + pytest.param( + m.load_array_no_default_ctor, m.NoDefaultCtor, 2, id="array_construct" + ), +] + + +class SequenceWithChangingLen: + """Reports `lens[0]` when first asked for its length, `lens[1]` after that.""" + + def __init__(self, items, lens): + self.items = items + self.lens = list(lens) + self.indices = [] + + def __len__(self): + return self.lens.pop(0) if len(self.lens) > 1 else self.lens[0] + + def __getitem__(self, index): + self.indices.append(index) + return self.items[index] + + +@pytest.mark.parametrize(("load", "item_type", "size"), ARRAY_CASTER_LOADS) +@pytest.mark.parametrize("len_change", [-1, 1]) +def test_array_caster_len_changes(load, item_type, size, len_change): + """The length is determined only once, which rules out out-of-bounds accesses.""" + items = [item_type(i) for i in range(size + 1)] + seq = SequenceWithChangingLen(items, [size, size + len_change]) + assert load(seq) == list(range(size)) + assert seq.indices == list(range(size)) + + +@pytest.mark.parametrize(("load", "item_type", "size"), ARRAY_CASTER_LOADS) +def test_array_caster_len_exceeds_items(load, item_type, size): + items = [item_type(i) for i in range(size - 1)] + seq = SequenceWithChangingLen(items, [size]) + with pytest.raises(IndexError): + load(seq) + assert seq.indices == list(range(size)) + + +@pytest.mark.parametrize(("load", "item_type", "size"), ARRAY_CASTER_LOADS) +def test_array_caster_items_created_on_the_fly(load, item_type, size): + """Each item is kept alive until its value was copied (no use-after-free).""" + + class OnTheFly: + def __len__(self): + return size + + def __getitem__(self, index): + return item_type(index + 10) # The sequence does not retain the item. + + assert load(OnTheFly()) == list(range(10, 10 + size)) + + def test_issue_1561(): """check fix for issue #1561""" bar = m.Issue1561Outer()