Change `array_caster` in pybind11/stl.h to support value types that are not default-constructible. #30034

rwgk · 2023-05-04T23:55:08Z

Description

However, this PR is specific to array_caster; practically speaking, specific to std::array conversions.

This PR preserves the existing array_caster behavior as much as possible. The only difference is that it also works for value types that are not default-constructible. — Note that without this PR, the new test results in a compiler error.

This resolves failures when testing globally with PyCLIF-pybind11.

Suggested changelog entry:

…` triggered by stl.h `array_caster`: ``` clang++ -o pybind11/tests/test_stl_no_default_ctor.os -c -std=c++17 -fPIC -fvisibility=hidden -O0 -g -Wall -Wextra -Wconversion -Wcast-qual -Wdeprecated -Wundef -Wnon-virtual-dtor -Wunused-result -Werror -isystem /usr/include/python3.10 -isystem /usr/include/eigen3 -DPYBIND11_STRICT_ASSERTS_CLASS_HOLDER_VS_TYPE_CASTER_MIX -DPYBIND11_ENABLE_TYPE_CASTER_ODR_GUARD_IF_AVAILABLE -DPYBIND11_TEST_BOOST -Ipybind11/include -I/usr/local/google/home/rwgk/forked/pybind11/include -I/usr/local/google/home/rwgk/clone/pybind11/include /usr/local/google/home/rwgk/forked/pybind11/tests/test_stl_no_default_ctor.cpp In file included from /usr/local/google/home/rwgk/forked/pybind11/tests/test_stl_no_default_ctor.cpp:1: In file included from /usr/local/google/home/rwgk/forked/pybind11/include/pybind11/stl.h:12: In file included from /usr/local/google/home/rwgk/forked/pybind11/include/pybind11/pybind11.h:13: In file included from /usr/local/google/home/rwgk/forked/pybind11/include/pybind11/detail/class.h:12: In file included from /usr/local/google/home/rwgk/forked/pybind11/include/pybind11/attr.h:14: In file included from /usr/local/google/home/rwgk/forked/pybind11/include/pybind11/cast.h:18: /usr/local/google/home/rwgk/forked/pybind11/include/pybind11/detail/type_caster_odr_guard.h:111:5: error: call to implicitly-deleted default constructor of 'pybind11::detail::type_caster<std::array<pybind11_tests::stl_no_default_ctor::Node, 2>>' type_caster_odr_guard() { ^ /usr/bin/../lib/gcc/x86_64-linux-gnu/12/../../../../include/c++/12/tuple:190:9: note: in instantiation of member function 'pybind11::detail::type_caster_odr_guard<std::array<pybind11_tests::stl_no_default_ctor::Node, 2>, pybind11::detail::type_caster<std::array<pybind11_tests::stl_no_default_ctor::Node, 2>>>::type_caster_odr_guard' requested here : _M_head_impl() { } ^ /usr/local/google/home/rwgk/forked/pybind11/include/pybind11/pybind11.h:104:9: note: in instantiation of function template specialization 'pybind11::cpp_function::initialize<(lambda at /usr/local/google/home/rwgk/forked/pybind11/include/pybind11/pybind11.h:1583:29), void, pybind11_tests::stl_no_default_ctor::NodeArray &, const std::array<pybind11_tests::stl_no_default_ctor::Node, 2> &, pybind11::is_method>' requested here initialize( ^ /usr/local/google/home/rwgk/forked/pybind11/include/pybind11/pybind11.h:1583:16: note: in instantiation of function template specialization 'pybind11::cpp_function::cpp_function<(lambda at /usr/local/google/home/rwgk/forked/pybind11/include/pybind11/pybind11.h:1583:29), pybind11::is_method, void>' requested here return cpp_function([pm](T &c, const D &value) { c.*pm = value; }, is_method(hdl)); ^ /usr/local/google/home/rwgk/forked/pybind11/include/pybind11/pybind11.h:1926:54: note: in instantiation of function template specialization 'pybind11::property_cpp_function<pybind11_tests::stl_no_default_ctor::NodeArray, std::array<pybind11_tests::stl_no_default_ctor::Node, 2>>::write<std::array<pybind11_tests::stl_no_default_ctor::Node, 2> pybind11_tests::stl_no_default_ctor::NodeArray::*, 0>' requested here property_cpp_function<type, D>::write(pm, *this), ^ /usr/local/google/home/rwgk/forked/pybind11/tests/test_stl_no_default_ctor.cpp:32:10: note: in instantiation of function template specialization 'pybind11::class_<pybind11_tests::stl_no_default_ctor::NodeArray>::def_readwrite<pybind11_tests::stl_no_default_ctor::NodeArray, std::array<pybind11_tests::stl_no_default_ctor::Node, 2>>' requested here .def_readwrite("arr", &NodeArray::arr); ^ /usr/local/google/home/rwgk/forked/pybind11/include/pybind11/stl.h:288:7: note: default constructor of 'type_caster<std::array<pybind11_tests::stl_no_default_ctor::Node, 2>>' is implicitly deleted because base class 'array_caster<std::array<Node, 2UL>, pybind11_tests::stl_no_default_ctor::Node, false, 2UL>' has a deleted default constructor : array_caster<std::array<Type, Size>, Type, false, Size> {}; ^ /usr/local/google/home/rwgk/forked/pybind11/include/pybind11/stl.h:278:5: note: default constructor of 'array_caster<std::array<pybind11_tests::stl_no_default_ctor::Node, 2>, pybind11_tests::stl_no_default_ctor::Node, false, 2>' is implicitly deleted because field 'value' has a deleted default constructor PYBIND11_TYPE_CASTER_RVPP(ArrayType, ^ /usr/local/google/home/rwgk/forked/pybind11/include/pybind11/cast.h:262:5: note: expanded from macro 'PYBIND11_TYPE_CASTER_RVPP' PYBIND11_TYPE_CASTER_IMPL(type, py_name, ::pybind11::return_value_policy_pack) ^ /usr/local/google/home/rwgk/forked/pybind11/include/pybind11/cast.h:232:10: note: expanded from macro 'PYBIND11_TYPE_CASTER_IMPL' type value; \ ^ /usr/bin/../lib/gcc/x86_64-linux-gnu/12/../../../../include/c++/12/array:115:56: note: default constructor of 'array<pybind11_tests::stl_no_default_ctor::Node, 2>' is implicitly deleted because field '_M_elems' has no default constructor typename _AT_Type::_Type _M_elems; ^ 1 error generated. ```

…a default constructor.

…8.5 compilation errors.

rainwoodman · 2024-01-16T17:34:17Z

include/pybind11/stl.h

+        // 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());


Is there a way to use e.g. emplace to construct the elements in place? then you can likely avoid the default constructible requirement. (not sure if the comment above means the requirement is a limitation or always satisfied)

Yes, but I added this to the PR description after seeing your questions:

This PR preserves the existing array_caster behavior as much as possible. The only difference is that it also works for value types that are not default-constructible. — Note that without this PR, the new test results in a compiler error.

I also added a source code comment here:

// Using `resize` to preserve the behavior exactly as it was before google/pywrapcc#30034

rainwoodman · 2024-01-16T17:36:57Z

include/pybind11/stl.h

+        }
+        if (policy == return_value_policy::take_ownership) {
+            auto h = cast(std::move(*src), policy, parent);
+            delete src; // WARNING: Assumes `src` was allocated with `new`.


Do you know typically from whom is src allocated? If so, might be helpful to mention that here for more context.

Or is this a typical requirement for any take_ownership policy on casters with pointer type src?

I also added a source code comment here:

// Intentionally preserving the behavior exactly as it was before google/pywrapcc#30034

Do you know typically from whom is src allocated?

No! :-)

I think this is one of the dark corners of pybind11, specifically the PYBIND11_TYPE_CASTER macro, which is used a lot. But the ecosystem has grown around it ... better don't touch.

rainwoodman · 2024-01-16T17:39:23Z

tests/test_stl.py

@@ -46,6 +46,13 @@ def test_array(doc):
    )


+def test_array_no_default_ctor():
+    lst = m.NoDefaultCtorArray(3)
+    assert [e.val for e in lst.arr] == [13, 23]


Do you allow mutation of members of the array? if so might want to add a test about whether mutating lst.arr affects m.NoDefaultCtorArray(4).arr.

The array_caster copies (or moves) elements to/from a native Python list. The code below demonstrates the behavior. I verified that it works, then undid the change again, because that aspect of the array_caster behavior is tangential to adding the default ctor support.

def test_array_no_default_ctor(): lst = m.NoDefaultCtorArray(3) assert [e.val for e in lst.arr] == [13, 23] lst4 = m.NoDefaultCtorArray(4) lst.arr = lst4.arr assert [e.val for e in lst.arr] == [14, 24] # Tangential to default ctor behavior: lst.arr = m.NoDefaultCtorArray(5).arr assert [e.val for e in lst.arr] == [15, 25] assert [e.val for e in lst4.arr] == [14, 24]

rwgk · 2024-01-16T19:31:33Z

EDIT: This was resolved by pybind/pybind11#5006

The two 🐍 3 • windows-latest • mingw32 CI failures are unrelated and also appear on pybind11 master:

https://github.com/pybind/pybind11/actions/runs/7514690075/job/20457837110
Installing additional packages through pacman...
137
C:\Windows\system32\cmd.exe /D /S /C D:\a_temp\setup-msys2\msys2.cmd -c "'pacman' '--noconfirm' '-S' '--needed' '--overwrite' '*' 'git' 'mingw-w64-i686-gcc' 'mingw-w64-i686-python-pip' 'mingw-w64-i686-python-numpy' 'mingw-w64-i686-python-scipy' 'mingw-w64-i686-cmake' 'mingw-w64-i686-make' 'mingw-w64-i686-python-pytest' 'mingw-w64-i686-eigen3' 'mingw-w64-i686-boost' 'mingw-w64-i686-catch'"
138
error: target not found: mingw-w64-i686-python-scipy
139
Error: The process 'C:\Windows\system32\cmd.exe' failed with exit code 1

Last to work: 2024-01-08T13:40:51.1780929Z

First broken: 2024-01-13T19:31:58.0408995Z

rainwoodman

Thanks for the changes!

rwgk · 2024-01-17T19:07:42Z

Thanks for the review!

For easy direct access: * google/pybind11clif#30034 * google/pybind11clif#30087 * google/pybind11clif#30088 Note regarding the change in std_containers_copy_move_test.py: When the test was added (cl/570839777), the undocumented expectation was that it is hyper-sensitive, but that it will be adjusted or made more permissive (similar to e.g. google3/third_party/clif/testing/python/return_value_policy_test.py;l=25;rcl=534200687) as needed. #MIGRATION_3P_PYBIND11__DEFAULT ``` - 24a3b1c3729401ca661efacfbbd13a83117e1bae Add `case return_value_policy::_clif_automatic` in type_c... by Ralf W. Grosse-Kunstleve <rwgk@google.com> - 015834b13a99d233c15406c36a3a44d27ebfc846 Change `array_caster` in pybind11/stl.h to support value ... by Ralf W. Grosse-Kunstleve <rwgk@google.com> - ce00785ef877f0bb1dba10115f00366111583b3c Add `release_gil_before_calling_cpp_dtor` annotation for ... by Ralf W. Grosse-Kunstleve <rwgk@google.com> ``` PiperOrigin-RevId: 599883612

Ralf W. Grosse-Kunstleve added 12 commits May 4, 2023 16:48

Merge branch 'google_pywrapcc_main' into stl_h_no_default_ctor_pywrapcc

85a6077

Expand PYBIND11_TYPE_CASTER_RVPP macro.

cca21ee

Resolve clang-tidy and clang <= 5 errors.

3d61895

Second try: Resolve clang-tidy and clang <= 5 errors.

aa82e3a

First working version of array_caster that works for types without …

e26769a

…a default constructor.

Try () instead of {} to see if that resolves clang 3.6 and gcc 4.…

080fa2b

…8.5 compilation errors.

Add minor comment.

6dd0c70

Avoid temp storage if Resizable is true.

cb0021c

Merge test_stl_no_default_ctor into test_stl

d2e1222

Minor move of code blocks (no functional change).

3f14ae3

Remove unused default value.

64dcab7

rwgk marked this pull request as ready for review January 12, 2024 18:43

rwgk changed the title ~~WIP: stl.h error: call to implicitly-deleted default constructor~~ Change array_caster in pybind11/stl.h to support value types that are not default-constructible. Jan 12, 2024

IncludeCleaner (IWYU) fixes

f90342a

rainwoodman reviewed Jan 16, 2024

View reviewed changes

Add two comments: preserving existing behavior

8486c9e

Merge branch 'google_pywrapcc_main' into stl_h_no_default_ctor_pywrapcc

7c45242

rainwoodman approved these changes Jan 17, 2024

View reviewed changes

rwgk merged commit 015834b into google:main Jan 17, 2024
147 checks passed

rwgk deleted the stl_h_no_default_ctor_pywrapcc branch January 17, 2024 19:07

rwgk pushed a commit to rwgk/pybind11 that referenced this pull request Aug 13, 2024

Backport of google/pybind11clif#30034

7b0a261

rwgk mentioned this pull request Aug 13, 2024

feat: change array_caster in pybind11/stl.h to support value types that are not default-constructible. pybind/pybind11#5305

Merged

rwgk pushed a commit to pybind/pybind11 that referenced this pull request Aug 15, 2024

Backport of google/pybind11clif#30034 (#5305)

a1d0091

rwgk mentioned this pull request Aug 15, 2024

git merge smart_holder #30147

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change `array_caster` in pybind11/stl.h to support value types that are not default-constructible. #30034

Change `array_caster` in pybind11/stl.h to support value types that are not default-constructible. #30034

rwgk commented May 4, 2023 •

edited

Loading

rainwoodman Jan 16, 2024

rwgk Jan 16, 2024

rainwoodman Jan 16, 2024

rainwoodman Jan 16, 2024

rwgk Jan 16, 2024 •

edited

Loading

rainwoodman Jan 16, 2024

rwgk Jan 16, 2024

rwgk commented Jan 16, 2024 •

edited

Loading

rainwoodman left a comment

rwgk commented Jan 17, 2024

Change array_caster in pybind11/stl.h to support value types that are not default-constructible. #30034

Change array_caster in pybind11/stl.h to support value types that are not default-constructible. #30034

Conversation

rwgk commented May 4, 2023 • edited Loading

Description

Suggested changelog entry:

rainwoodman Jan 16, 2024

Choose a reason for hiding this comment

rwgk Jan 16, 2024

Choose a reason for hiding this comment

rainwoodman Jan 16, 2024

Choose a reason for hiding this comment

rainwoodman Jan 16, 2024

Choose a reason for hiding this comment

rwgk Jan 16, 2024 • edited Loading

Choose a reason for hiding this comment

rainwoodman Jan 16, 2024

Choose a reason for hiding this comment

rwgk Jan 16, 2024

Choose a reason for hiding this comment

rwgk commented Jan 16, 2024 • edited Loading

rainwoodman left a comment

Choose a reason for hiding this comment

rwgk commented Jan 17, 2024

Change `array_caster` in pybind11/stl.h to support value types that are not default-constructible. #30034

Change `array_caster` in pybind11/stl.h to support value types that are not default-constructible. #30034

rwgk commented May 4, 2023 •

edited

Loading

rwgk Jan 16, 2024 •

edited

Loading

rwgk commented Jan 16, 2024 •

edited

Loading