fix(stl): rework array_caster element loading (compile time, bounds)

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 <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

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 <typename T, typename SFINAE = void>
 struct is_fmt_numeric {
     static constexpr bool value = false;

include/pybind11/stl.h

@@ -394,49 +394,98 @@ ArrayType vector_to_array(V &&v) {
     return vector_to_array_impl<ArrayType, V>(std::forward<V>(v), make_index_sequence<N>{});
 }
 
+// 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 <typename T>
+struct is_plain_array_element : any_of<std::is_scalar<T>, is_fmt_numeric<T>> {};
+template <typename T, size_t N>
+struct is_plain_array_element<std::array<T, N>> : is_plain_array_element<T> {};
+
 template <typename ArrayType, typename Value, bool Resizable, size_t Size = 0>
 struct array_caster {
     using value_conv = make_caster<Value>;
 
 private:
+    // On the heap because `ArrayType` is not necessarily default constructible.
     std::unique_ptr<ArrayType> value;
 
-    template <bool R = Resizable, enable_if_t<R, int> = 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<Value &&>;
+        // 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>::value
+                && std::is_same<remove_cv_t<remove_reference_t<value_cast_op_type>>,
+                                remove_cv_t<Value>>::value
+                && std::is_assignable<Value &, value_cast_op_type>::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<ArrayType> make_array(size_t size, std::true_type /*Resizable*/) {
+        std::unique_ptr<ArrayType> array(new ArrayType());
+        array->resize(size);
+        return array;
+    }
+    static std::unique_ptr<ArrayType> make_array(size_t size, std::false_type /*Resizable*/) {
+        return std::unique_ptr<ArrayType>(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<sequence>(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<Resizable>{});
+        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<Value &&>(std::move(conv));
+            (*result)[i] = cast_op<Value &&>(std::move(conv));
         }
+        value = std::move(result);
         return true;
     }
 
-    template <bool R = Resizable, enable_if_t<!R, int> = 0>
-    bool convert_elements(handle seq, bool convert) {
+    bool convert_elements(handle seq, bool convert, std::false_type /*assign_elements*/) {
         auto l = reinterpret_borrow<sequence>(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<Value> 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<Value &&>(std::move(conv)));

tests/test_stl.cpp

@@ -7,6 +7,7 @@
     BSD-style license that can be found in the LICENSE file.
 */
 
+#include <pybind11/complex.h>
 #include <pybind11/stl.h>
 
 #include "constructor_stats.h"
@@ -18,7 +19,12 @@
 
 #include <pybind11/typing.h>
 
+#include <array>
+#include <complex>
+#include <numeric>
+#include <stdexcept>
 #include <string>
+#include <valarray>
 #include <vector>
 
 #if defined(PYBIND11_TEST_BOOST)
@@ -62,6 +68,29 @@ struct visit_helper<boost::variant> {
 
 PYBIND11_MAKE_OPAQUE(std::vector<std::string, std::allocator<std::string>>)
 
+// 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<const int, 2>;
+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<CastOnlyElement> {
+    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 <typename T>
@@ -169,6 +198,23 @@ struct type_caster<ReferenceSensitiveOptional<T>>
 } // 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 <typename U>
+    // NOLINTNEXTLINE(bugprone-forwarding-reference-overload, google-explicit-constructor)
+    CtorTemplateLvalue(U &&other) : val{other.val % 100 + 100} {}
+    int val;
+};
+
+template <typename T>
+std::vector<int> array_element_vals(const std::array<T, 2> &a) {
+    return {a[0].val, a[1].val};
+}
+
 int pass_std_vector_int(const std::vector<int> &v) {
     int zum = 100;
     for (const int i : v) {
@@ -215,16 +261,100 @@ TEST_SUBMODULE(stl, m) {
     };
 
     // test_array_no_default_ctor
-    py::class_<NoDefaultCtor>(m, "NoDefaultCtor").def_readonly("val", &NoDefaultCtor::val);
+    py::class_<NoDefaultCtor>(m, "NoDefaultCtor")
+        .def(py::init<int>())
+        .def_readonly("val", &NoDefaultCtor::val);
     py::class_<NoDefaultCtorArray>(m, "NoDefaultCtorArray")
         .def(py::init<int>())
         .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<int>::value, "");
+    static_assert(py::detail::is_plain_array_element<ArrayElementEnum>::value, "");
+    static_assert(py::detail::is_plain_array_element<NoDefaultCtor *>::value, "");
+    static_assert(py::detail::is_plain_array_element<std::complex<double>>::value, "");
+    static_assert(py::detail::is_plain_array_element<
+                      std::array<std::array<std::complex<float>, 2>, 3>>::value,
+                  "");
+    static_assert(!py::detail::is_plain_array_element<std::string>::value, "");
+    static_assert(!py::detail::is_plain_array_element<NoDefaultCtor>::value, "");
+    static_assert(!py::detail::is_plain_array_element<std::array<std::string, 2>>::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_<ArrayElementEnum>(m, "ArrayElementEnum");
+    py::class_<NotCopyAssignable>(m, "NotCopyAssignable").def(py::init<int>());
+    py::class_<AssignIsNoOp>(m, "AssignIsNoOp").def(py::init<int>());
+    py::class_<ThrowingDefaultCtor>(m, "ThrowingDefaultCtor").def(py::init<int>());
+    py::class_<CtorTemplateLvalue>(m, "CtorTemplateLvalue").def(py::init<int>());
+    m.def("load_array_enum", [](const std::array<ArrayElementEnum, 2> &a) {
+        return std::vector<int>{static_cast<int>(a[0]), static_cast<int>(a[1])};
+    });
+    m.def("load_array_not_copy_assignable", &array_element_vals<NotCopyAssignable>);
+    m.def("load_array_assign_is_no_op", &array_element_vals<AssignIsNoOp>);
+    m.def("load_array_throwing_default_ctor", &array_element_vals<ThrowingDefaultCtor>);
+    m.def("load_array_no_default_ctor", &array_element_vals<NoDefaultCtor>);
+    m.def("load_array_ctor_template_lvalue", &array_element_vals<CtorTemplateLvalue>);
+
+    m.def("cast_array_cast_only_element", []() {
+        return std::array<CastOnlyElement, 2>{{CastOnlyElement{1}, CastOnlyElement{2}}};
+    });
+
+    // test_array_nested
+    py::class_<OpaqueConstIntArray>(m, "OpaqueConstIntArray");
+    m.def("make_opaque_const_int_array", []() { return OpaqueConstIntArray{{3, 4}}; });
+    m.def("load_array_opaque_const_int_array",
+          [](const std::array<OpaqueConstIntArray, 1> &a) { return a[0][0] + a[0][1]; });
+    m.def("nested_numeric_array_cast_sequence",
+          [](std::array<std::array<std::complex<double>, 2>, 2> x) { return x; });
+    m.def("nested_array_cast_sequence",
+          [](std::array<std::array<std::string, 2>, 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<int, 102400> &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<std::complex<double>, 102400> &a) {
+        return std::accumulate(a.begin(), a.end(), std::complex<double>());
+    });
+
     // test_valarray
     m.def("cast_valarray", []() { return std::valarray<int>{1, 4, 9}; });
     m.def("load_valarray", [](const std::valarray<int> &v) {
         return v.size() == 3 && v[0] == 1 && v[1] == 4 && v[2] == 9;
     });
+    m.def("valarray_cast_sequence", [](std::valarray<int> x) { return x; });
 
     // test_map
     m.def("cast_map", []() { return std::map<std::string, std::string>{{"key", "value"}}; });