Repository navigation
Conversation
Load `std::array` and `std::valarray` elements in one of two ways: * General path: default-construct the array, then assign the elements. * Fallback (`std::array` only): temporary `std::vector` plus an `index_sequence` of `Size` elements, for element types that are not default constructible, or not assignable from what their caster yields. Since pybind#5305 all `std::array` types went through the `index_sequence`, which is very slow to compile for large `Size` (pybind#5765). All element loops now determine the length of the sequence only once and index up to it. Before, a sequence reporting different lengths could cause out-of-bounds accesses (a write for `std::valarray`, a read for `std::array`). Assisted-by: ClaudeCode:claude-fable-5-2
Select the loading path inside a member function instead of in class scope, so that an element caster that only implements `cast()` keeps working for returning a `std::array`, as on master. Assisted-by: ClaudeCode:claude-fable-5-2
|
— Claude Code (Fable 5.2) on behalf of Ralf — The general path changes element semantics: a real case, and a proposed fixSummary: On the general path, this PR default-constructs the elements of a The real case
Without Reproducer#include <pybind11/pybind11.h>
#include <pybind11/stl.h>
#include <boost/multi_array.hpp>
#include <array>
namespace py = pybind11;
using MA = boost::multi_array<double, 2>;
PYBIND11_MODULE(ma_demo, m) {
py::class_<MA>(m, "MA");
m.def("make", []() { return MA(boost::extents[2][3]); });
m.def("shape_of_first", [](const std::array<MA, 1> &a) {
return py::make_tuple(a[0].shape()[0], a[0].shape()[1]);
});
}import ma_demo as m
print(m.shape_of_first([m.make()]))For the description's own example, a copyable type whose default constructor throws in normal use, I did not find a real instance. The general point stands without it: any type whose assignment is not equivalent to copy construction, or whose default constructor has side effects, can tell the two approaches apart. Options1. Narrow the trait (proposed). Take the general path only where the switch cannot be observed: the element type is trivially default constructible, trivially copyable, and trivially assignable from what its caster yields. - || (std::is_default_constructible<ArrayType>::value
- && std::is_assignable<Value &, value_cast_op_type>::value)>;
+ || (std::is_trivially_default_constructible<Value>::value
+ && std::is_trivially_copyable<Value>::value
+ && std::is_trivially_assignable<Value &,
+ value_cast_op_type>::value)>;Everything else takes the fallback, where it behaves and compiles exactly as on master:
The cost is that fewer element types get the fast compile. Checked so far with this change applied (plus making one test type trivial): the table above, and 2. A looser trait. Require "default constructible" instead of "trivially default constructible", and keep the two trivial-copy conditions. That keeps 3. Construct each element in place in raw storage with placement-new. That keeps master's element operations for every type, and would also remove the compile-time explosion for the types that otherwise stay on the fallback. It is the approach the description calls too fragile under "Where the explosion remains" (it leans on implicit-lifetime rules and My preference is option 1: it makes this PR a pure optimization plus the bounds fix, with no change in element semantics. StatusThe PR head (04e6c39) is unchanged and still has this problem. With option 1, the description's "What changes in behavior" section shrinks to the changing-length case, and "Where the explosion remains" widens to all element types that are not trivial. |
henryiii
left a comment
There was a problem hiding this comment.
AI-generated review — OpenCode:GLM-5.3, at Henry Schreiner's direction. All claims below were verified by building and running locally (Apple clang arm64, free-threaded CPython 3.15, PYBIND11_TEST_OVERRIDE=test_stl), not only by reading the code.
Code review: the diff itself is solid
- Bounds fix: length is queried once and
l[i]goes throughPySequence_GetItem(pytypes.h:1111), which is bounds-checked and throws — a shrinking sequence now raises instead of overflowing thestd::valarraywrite / OOB-reading the fallback'sstd::vector. Confirmed the tests fail on master's behavior without relying on UB being caught:test_array_caster_len_changesasserts on the__getitem__indices, andtest_array_caster_len_exceeds_itemsexpects theIndexErrorto propagate (on master it is silently swallowed by old-style iteration and reads out of bounds). - The selection trait is exact.
movable_cast_op_type<Value&&>resolves toValue&&for macro casters andValue&forclass_casters (detail/type_caster_base.h:967), which is exactly whatcast_op<Value &&>(std::move(conv))yields, sois_assignable<Value &, value_cast_op_type>precisely predicts the assignment it gates. This is what saves the copy-constructible-but-not-copy-assignable registered types that #6258 broke (covered byNotCopyAssignable). - Cast-only element casters: keeping the trait inside the function body means
cast_op_typeis only required whenload()is instantiated.test_array_cast_only_elementpasses locally. ArrayType()vsArrayType{}is the right call (copy-list-init would rejectexplicitdefault constructors; covered bytest_array_element_types).- Item lifetime (
const object item = l[i]) and no partial value (result stored only after all elements convert) are correct improvements, and tested.
Local run: full test_stl.py passes (55 passed, 10 skipped, skips unrelated to this PR); CI is green across the matrix.
The multi_array finding: confirmed independently
I built the reproducer from the comment above locally (Homebrew Boost, -DNDEBUG, Apple clang 17 / C++17):
| Headers | result of shape_of_first([m.make()]) |
|---|---|
| this PR as posted (04e6c39) | (0, 0) — silently wrong |
| this PR + option 1 trait | (2, 3) — matches master |
So the concern is real and reproducible, not just theoretical.
Recommendation: option 1
I applied option 1's trait locally and re-ran the full test_stl.py: everything still passes, including:
ExplicitDefaultCtor— its= defaultconstructor is trivial, so it stays on the general path and keeps covering the()vs{}distinction;- the #5765 case —
std::array<int, 102400>is trivial, so the compile-time fix is unaffected (the wholetest_stltest module still builds in seconds); - nested POD arrays (
std::array<std::array<double, 3>, N>style) remain trivially constructible/copyable/assignable and stay on the fast path.
Rationale: option 1 makes this PR a pure optimization plus the bounds fix, with no observable element-semantics change for any type. The cost (non-trivial element types like std::complex keep today's slow fallback compile) is the status quo, not a regression, and can be revisited (option 2) separately if a real std::complex-heavy case shows up.
Two small notes if option 1 is adopted:
- The proposed diff switches the default-constructibility test from
ArrayTypetoValue. Both are correct;Valuesendsstd::array<T, 0>to the fallback, which is cheap (emptyindex_sequence). Just worth making that a deliberate, commented choice. - Consider adding a test that pins the semantics: an element type whose copy assignment is a no-op but whose copy constructor works (the
boost::multi_arraybehavior in miniature — correct on the fallback, all-zeros on the general path). It would fail if the trait is ever loosened again without revisiting this discussion.
The PR description's "What changes in behavior" section will need the matching update.
Default-constructing the elements and then assigning them is not equivalent to copy constructing them for every element type. With `boost::multi_array` elements, whose assignment does not resize, the previous revision loaded empty arrays where master loads them correctly. Assign only if that cannot be told apart from constructing: the element type is trivially default constructible, trivially copyable, and trivially assignable from what its caster yields. All other element types are constructed, exactly as on master. Assisted-by: ClaudeCode:claude-fable-5-2
|
Please don't merge: I'm still validating a change that'll resolve this issue (see comment above):
|
Shorten the array_caster comments, get each item as an owned object directly (one less incref/decref pair per element), and bind array_element_vals for NoDefaultCtor so the tests share one parameter list. Assisted-by: ClaudeCode:claude-fable-5-2
|
🤖 AI text below 🤖 I pushed 5d687c7, a cleanup pass (reuse, simplification, efficiency). It does not change behavior. Changed
One coverage change: the Considered and not changed
Scope limit
|
Assign the elements of a `std::array`, instead of constructing them, for the element types for which that cannot make a difference: scalar types, the numeric types (`is_fmt_numeric`), and `std::array`s of those. The caster has to yield the element type itself, and the assignment has to be possible. All other class types are constructed, as on master. The numeric types are what makes this more than "trivial types": they include `std::complex` (with pybind11/complex.h), whose default constructor is not trivial, although all it does is zero-initialize. A large `std::array` of `std::complex` now compiles as quickly as one of `double`, and numeric types added to `is_fmt_numeric` later are covered without further changes. Trivial class types are no longer assigned. Identifying the ones for which assigning is equivalent to constructing is not feasible with type traits: review found a constructor template that takes over from the copy constructor, and an explicit move constructor, which direct-initialization uses but the copy-initialization of the array elements does not. In both cases master runs a user-defined constructor that assigning would skip. Assisted-by: ClaudeCode:claude-fable-5-2
Master iterates over the sequence with a lazy accessor, so that an item is fetched when its caster loads it, after the caster was constructed. The indexed loops fetched the item first. A caster whose constructor has side effects that `__getitem__` can observe saw the difference, also for a sequence of constant length. Go back to an accessor, which fetches the item when it is first used, and which keeps it alive while the caster is in use. Pass `item.ptr()` to the caster instead of the accessor itself: that keeps the saving of the cleanup commit, which avoided the temporary `object` that master creates for each item. Assisted-by: ClaudeCode:claude-fable-5-2
Add a note to the documentation of the STL container conversions. The general rule in the note already applies on master, but was only documented for custom type casters: the optional caster headers (stl.h, complex.h, ...) must be visible consistently wherever the casters or traits they provide affect a conversion. If one translation unit of an extension module converts a type with such a header taking effect, every translation unit that converts that type has to include it, or the ODR is violated. The last sentence of the note is new with this PR. complex.h now also matters for a `std::array` of `std::complex` if `std::complex` is opaque or has a custom caster, because `array_caster` selects how it loads the elements with `is_fmt_numeric`, a trait that complex.h specializes. Assisted-by: ClaudeCode:claude-fable-5-2
|
— Claude Code (Fable 5.2) on behalf of Ralf — What changed since bf5c31aThree commits are pushed on top of the cleanup commit 5d687c7, and the PR description is rewritten to match. The elements of a The three new commits
The selection now reads: 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> {};
// in array_caster::convert_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)>;The second condition requires the caster to yield the element type itself, rather than something that converts to it. The third is explained under "Two smaller fixes". The cleanup commit 5d687c7 is kept. Two things in it changed again:
Which element types are assigned now
The table was produced by evaluating both selections for these types. "POD" is not the criterion in either direction: scalars and arrays of scalars are PODs and are assigned, POD structs are not, and Why
|
|
I'm done now and all tests pass. I think this is ready for merging. |
— Claude Code (Fable 5.2) on behalf of Ralf —
Description
Fixes #5765. Alternative to #6258, which this PR grew out of (see "Compared with #6258").
array_caster(the caster behindstd::arrayandstd::valarrayinpybind11/stl.h) now loads elements in one of two ways:std::vector, then aggregate-initialize thestd::arraythrough anindex_sequenceofSizeelements, which copy- or move-constructs them. Since feat: changearray_casterin pybind11/stl.h to support value types that are not default-constructible. #5305 allstd::arraytypes go through this code, and most still do.std::valarrayhas always been loaded this way. Astd::arrayis now loaded this way only for element types for which that cannot make a difference: scalar types (int,double, pointers, enums), pybind11's numeric types (is_fmt_numeric, which includestd::complex), andstd::arrays of those. In addition, the caster has to yield the element type itself (not something that converts to it), and the elements have to be assignable (an array ofconstelements is not). Everything else is constructed, as on master.What this fixes
1. Very slow compilation for large
std::arrayof scalar and numeric element types (#5765). Assigning needs noindex_sequence. Compiling the example from the issue (std::array<int, 102400>withdef_readwrite,-O1, C++17):The numeric types include
std::complex(withpybind11/complex.h), whose default constructor is not trivial although all it does is zero-initialize. A function taking astd::array<std::complex<double>, 102400>compiles in 3.5 s (g++) and 4.2 s (clang++) with this PR; with g++ on master it takes 130 s. Numeric types added tois_fmt_numericlater are covered without further changes, and so are nested arrays such asstd::array<std::array<double, 3>, N>.2. Out-of-bounds accesses when the length of the sequence changes during the conversion. On master the length is queried more than once: for the size check, and again by the
end()of the iteration (thestd::arraypath also queries it forreserve). If the answers differ, the loops can run past the end of the buffer. Both of these are reproducible on master with AddressSanitizer, using a sequence whose__len__returns different values:std::valarray: heap-buffer-overflow write if the length grows.std::array: heap-buffer-overflow read if the length shrinks (elements are moved out of astd::vectorthat has fewer thanSizeelements).A user-defined
__len__is the deterministic way to trigger this. In a free-threaded build, another thread resizing alistbetween two of the queries is the realistic way (not reproduced; the window is small).All element loops now query the length once and index up to it. As before, each element caster is constructed before its item is fetched, and the item is kept alive until its value has been copied out.
3. No partially-loaded value. The
std::valarraypath used toresizeand fill the caster'svaluedirectly. The result is now built in a local and stored only if all elements converted.What changes in behavior
std::valarrayas[1, 2, 0]on master and as[1, 2, 3]with this PR. A__len__with side effects can also tell the difference.The first revision of this PR (04e6c39) assigned whenever the element type was default constructible and assignable. That is not equivalent in general: with
boost::multi_arrayelements, whose assignment does not resize, it loaded empty arrays where master loads them correctly. This comment has the reproducer and the options that were considered. The selection above is narrower than option 1 from there, in that trivial class types are constructed rather than assigned, and wider in that it includes the numeric types.A requirement that follows from
is_fmt_numericThe specializations of
is_fmt_numericmust be visible consistently in all translation units of a module that convert such arrays. In practice,pybind11/complex.hhas to be included in all of them or in none, also wherestd::complexis opaque or has a custom caster. Otherwisearray_casteris instantiated differently in different translation units, which violates the ODR, even though both instantiations give the same results.The documentation of the STL conversions gets a note about this. The general part of that note (optional caster headers must be visible consistently wherever the casters or traits they provide affect a conversion) already applies on master, but was only documented for custom type casters.
Where the explosion remains
Element types that are constructed still pay for the
index_sequenceexpansion, exactly as on master. That is every class type except the numeric types andstd::arrays of scalar or numeric types:std::string, registered classes, and other structs and classes, including trivial ones. (The exceptions are constructed as well if their caster yields something other than the element type, or if they cannot be assigned.) With g++ 13.3 at-O1, a function taking astd::arrayof 102400 elements of a registered type without a default constructor takes 95 s to compile with this PR and 98 s on master.Two ways to extend the fast compile to more element types, neither pursued here:
explicitconstructors).std::array. That would cover every element type, but it depends onstd::arraylayout details (including the special cases forSize == 0) and on implicit-lifetime rules, which seems too fragile for pybind11's compiler matrix.Compared with #6258
#6258 has the right idea, which this PR keeps: fill the array in place where that is possible, instead of going through
index_sequence. The differences:boost::multi_arrayelements it loads empty arrays (g++ 13), or does not compile (clang++ 18, see the third item). Here it is limited to scalar and numeric element types, and arrays of those.convert_elementsoverload next to thestd::valarrayone, which does the same thing. Here they are one function;make_array()covers the only difference (resizevs. size check).new ArrayType()instead ofnew ArrayType{}. The latter copy-list-initializes the elements, which is ill-formed for element types with anexplicitdefault constructor (clang++ 18 rejects it; g++ 13 accepts it).boost::multi_arrayis such a type.std::valarrayproblem described above:(*result)[ctr++]is bounded by a second length query, so a growing sequence overflows the heap-allocated array (confirmed with AddressSanitizer at 25e2172).is_move_assignable<Value>, but the assignment gets whatcast_op<Value &&>yields, which isValue &for types registered withpy::class_. A registered type that is copy constructible and move assignable, but not copy assignable, compiles on master and is a hard error with fix: avoid slow compile for large std::array in array_caster #6258. Here such a type is constructed.The large-array test is adapted from #6258.
Testing
New and extended tests in
tests/test_stl.py(bindings intests/test_stl.cpp):test_array_large: the [BUG]: slow compile if very long std::array being used with clang-18 #5765 case, and the same size withstd::complex<double>elements. Buildingpybind11_tests.cpp+test_stl.cppwith these bindings (g++ 13.3,-O0) takes 671 s against master's headers and 20 s with this PR.test_array_caster_len_changes,test_array_caster_len_exceeds_items: a sequence whose reported length shrinks, grows, or exceeds its items, for an assignedstd::array, astd::valarrayand a constructedstd::array. The first asserts on the indices requested through__getitem__, so it fails on master without depending on undefined behavior being detected.test_array_caster_items_created_on_the_fly: items that the sequence does not retain (meaningful under Valgrind/ASan).test_array_element_types: five class types that must be constructed, four of them trivially copyable. Three were loaded differently from master by an earlier revision of the selection: one whose assignment does nothing (theboost::multi_arraybehavior in miniature), one whose default constructor throws, and one with a constructor template that takes over from the copy constructor.static_asserts pin which element types are assigned.test_array_nested: constructed and assigned nested arrays, and an opaque array ofconstelements inside another array.test_array_cast_only_element: an element caster that only implementscast()still works for returning astd::array.Local validation, Linux x86-64, g++ 13.3, C++20, CPython 3.14.4:
test_stl.pyunder AddressSanitizer: 67 passed, 2 skipped, no reports.boost::multi_arrayelements, a constructor template that takes over from the copy constructor, anexplicitmove constructor, an opaque array ofconstelements, and a caster whose constructor has side effects that__getitem__observes.static_asserts on the new trait removed, which master does not have), the sixtest_array_caster_len_changescases fail and the other tests pass.tests/test_stl.cppcompiles without warnings (-Werror) with g++ 13.3 and clang++ 18.1 as C++11, 14, 17, 20 and 23.pre-commitpasses on the changed files.Not tested locally: MSVC, PyPy, GraalPy. CI was green for the first three commits.
AI assistance
This PR was written by Claude Code (Fable 5.2) at Ralf's direction. It started as a review of #6258, followed by the question of how
array_casterwould look if written from scratch. The claims in this description were checked by building and running: AddressSanitizer reproducers for the out-of-bounds cases (on master, on #6258 and with this PR), the compile-time measurements, and the test runs above.The code and this description were reviewed independently with Codex (gpt-6.1-sol, reasoning effort "ultra") in several rounds, before and after posting, in addition to the review on this PR. What those reviews found, and what became of it:
cast(). It is now evaluated inside a member function, with a test.boost::multi_arraycase, which the review on this PR confirmed independently.is_fmt_numericcreates the ODR requirement above. The design is kept, because it is what makes future numeric types work without further changes, and the requirement is documented.constelements, nested in another array, no longer compiled. The assignability condition and a test cover that.A last review of the final state, in a fresh session, ended with no findings for the code, the documentation note and this description.
How
array_castergot here, and how that informed this PRstd::arraycaster added (10c74c6, fixes #97)list. Size check, thenvalue[ctr++] = ...while iterating.std::valarraysupport, #545array_caster<ArrayType, Value, Resizable, Size>. An SFINAE pairrequire_size()either resizes or compares. Still default-construct and assign, shared by both containers.__len__returns: once forrequire_size(), again for the iteration.tuple()first;convert_elements()split out ofload().std::unique_ptr<ArrayType>.std::arrayelements go into a temporarystd::vector, thenvector_to_array()with anindex_sequenceofSize. Deliberately used for allstd::arrayelement types, to have one code path.std::valarraykeeps resize-and-assign.index_sequenceexpansion makesstd::array<int, 102400>take minutes to compile.std::valarrayandstd::array, for all default-constructible, assignable element types.How this shaped the implementation:
array_casterin pybind11/stl.h to support value types that are not default-constructible. #5305. By switchingstd::arrayto constructing its elements, feat: changearray_casterin pybind11/stl.h to support value types that are not default-constructible. #5305 also made the caster correct for types whose assignment is not equivalent to copy construction, although that was not its aim. fix: avoid slow compile for large std::array in array_caster #6258 and the first revision of this PR both undid that for class types.__len__decide how far it runs. So the fix is a pattern for all loops rather than a patch to one.array_casterin pybind11/stl.h to support value types that are not default-constructible. #5305's mechanism is kept. Heap storage stays, because the caster must be default constructible even ifArrayTypeis not. The temporarystd::vectorstays, because it lets each element be converted while its Python item is still alive. What was costly was sendingstd::array<int, N>through it.Also not pursued: holding small arrays by value in the caster, as before #5305, which would need a second storage strategy; and sharing
load()andcast()withlist_caster, which has near-identical copies (worth doing, but separately).Suggested changelog entry:
Fixed very slow compilation when binding large
std::arraytypes withpybind11/stl.h, for scalar and numeric element types, such asint,doubleorstd::complex, and nested arrays of those.Fixed potential out-of-bounds accesses in the
std::arrayandstd::valarraycasters when the length of the Python sequence changes while it is being converted.🤖 Generated with Claude Code
📚 Documentation preview 📚: https://pybind11--6276.org.readthedocs.build/