-
Notifications
You must be signed in to change notification settings - Fork 6
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Change array_caster
in pybind11/stl.h to support value types that are not default-constructible.
#30034
Conversation
…` 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.
error: call to implicitly-deleted default constructor
array_caster
in pybind11/stl.h to support value types that are not default-constructible.
// 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()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
} | ||
if (policy == return_value_policy::take_ownership) { | ||
auto h = cast(std::move(*src), policy, parent); | ||
delete src; // WARNING: Assumes `src` was allocated with `new`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you know typically from whom is src allocated? If so, might be helpful to mention that here for more context.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or is this a typical requirement for any take_ownership policy on casters with pointer type src?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -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] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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]
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 Last to work: First broken: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the changes!
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
Description
Loosely related: pybind/pybind11#864
However, this PR is specific to
array_caster
; practically speaking, specific tostd::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: