Skip to content

Elide to-python conversion of setter return values #4621

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 14 commits into from
May 8, 2023
Merged

Elide to-python conversion of setter return values #4621

merged 14 commits into from
May 8, 2023

Conversation

rwgk
Copy link
Collaborator

@rwgk rwgk commented Apr 12, 2023 •
edited
Loading

Description

Setter return values are inaccessible, at least for all practical purposes (see e.g. https://stackoverflow.com/questions/16615866/python-setter-to-also-return-a-value). pybind11 laboriously converts them from C++ to Python, only to be discarded. This PR elides the to-python conversion of setter return values.

The new test is a reduced example, reflective of a large number of use cases in the wild (Google codebase) in which the unnecessary to-python conversion fails. Eliding the to-python conversion fixes such failures.

For manually written bindings, eliding the to-python conversion is a minor optimization that could also be achieved by wrapping setters in lambda functions, but this small change removes the need completely to 1. think about it and 2. write the lambda function.

For automatically generated bindings (PyCLIF-pybind11) this small PR is a major simplification.

This PR was ported to the pywrapcc repo/pseudo-branch under google/pybind11clif#30027 (merged there already).

Suggested changelog entry:

Setter return values (which are inaccessible for all practical purposes) are no longer converted to Python (only to be discarded).

rwgk added 10 commits April 10, 2023 21:02
@rwgk
Reproducer for property setter with return type that is not wrapped.
cb9dda6
@rwgk
Use py::class_<OptionsBase>() to work around the return value conve…
302ae21 
…rsion issue.
@rwgk
WIP drop_return_value
454e15a
@rwgk
Remove struct drop_return_value
fb06e14
@rwgk
Introduce return_value_policy::return_none for use by setters.
82f0e61
@rwgk
Add is_setter to attr.h and use from .def_property()
b7a47da
@rwgk
Merge the new test into test_methods_and_attributes
7c6a2ec
@rwgk
Remove return_none return_value_policy again.
3a6944d
@rwgk
Fix oversight (NOLINTNEXTLINE placement).
78bb47b
@rwgk
Simplification (for the better) found while searching for a way to re…
047c326 
…solve GCC build failures.

Example of failure resolved by this change:

g++ (Ubuntu 9.4.0-1ubuntu1~20.04.1) 9.4.0

```
cd /build/tests && /usr/bin/c++ -DPYBIND11_TEST_EIGEN -Dpybind11_tests_EXPORTS -I/mounted_pybind11/include -isystem /usr/include/python3.8 -isystem /build/_deps/eigen-src -g -std=c++17 -fPIC -fvisibility=hidden -Wall -Wextra -Wconversion -Wcast-qual -Wdeprecated -Wundef -Wnon-virtual-dtor -MD -MT tests/CMakeFiles/pybind11_tests.dir/test_buffers.cpp.o -MF CMakeFiles/pybind11_tests.dir/test_buffers.cpp.o.d -o CMakeFiles/pybind11_tests.dir/test_buffers.cpp.o -c /mounted_pybind11/tests/test_buffers.cpp
In file included from /mounted_pybind11/include/pybind11/stl.h:12,
                 from /mounted_pybind11/tests/test_buffers.cpp:10:
/mounted_pybind11/include/pybind11/pybind11.h: In instantiation of ‘pybind11::class_<type_, options>& pybind11::class_<type_, options>::def_property(const char*, const Getter&, const Setter&, const Extra& ...) [with Getter = pybind11::cpp_function; Setter = std::nullptr_t; Extra = {pybind11::return_value_policy}; type_ = pybind11::buffer_info; options = {}]’:
/mounted_pybind11/include/pybind11/pybind11.h:1716:58:   required from ‘pybind11::class_<type_, options>& pybind11::class_<type_, options>::def_property_readonly(const char*, const pybind11::cpp_function&, const Extra& ...) [with Extra = {pybind11::return_value_policy}; type_ = pybind11::buffer_info; options = {}]’
/mounted_pybind11/include/pybind11/pybind11.h:1684:9:   required from ‘pybind11::class_<type_, options>& pybind11::class_<type_, options>::def_readonly(const char*, const D C::*, const Extra& ...) [with C = pybind11::buffer_info; D = long int; Extra = {}; type_ = pybind11::buffer_info; options = {}]’
/mounted_pybind11/tests/test_buffers.cpp:209:61:   required from here
/mounted_pybind11/include/pybind11/pybind11.h:1740:25: error: call of overloaded ‘cpp_function(std::nullptr_t&, pybind11::is_setter)’ is ambiguous
 1740 |             name, fget, cpp_function(method_adaptor<type>(fset), is_setter()), extra...);
      |                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/mounted_pybind11/include/pybind11/pybind11.h:101:5: note: candidate: ‘pybind11::cpp_function::cpp_function(Func&&, const Extra& ...) [with Func = std::nullptr_t&; Extra = {pybind11::is_setter}; <template-parameter-1-3> = void]’
  101 |     cpp_function(Func &&f, const Extra &...extra) {
      |     ^~~~~~~~~~~~
In file included from /mounted_pybind11/include/pybind11/stl.h:12,
                 from /mounted_pybind11/tests/test_buffers.cpp:10:
/mounted_pybind11/include/pybind11/pybind11.h:87:5: note: candidate: ‘pybind11::cpp_function::cpp_function(std::nullptr_t, const Extra& ...) [with Extra = {pybind11::is_setter}; std::nullptr_t = std::nullptr_t]’
   87 |     cpp_function(std::nullptr_t, const Extra &...) {}
      |     ^~~~~~~~~~~~
```
rwgk added a commit to rwgk/pybind11clif that referenced this pull request Apr 12, 2023
@rwgk
Snapshot of pybind/pybind11#4621 applied to pywrapcc
2a3e07a
@rwgk rwgk mentioned this pull request Apr 12, 2023
Merged
@Skylion007
Copy link
Collaborator

@rwgk Would this flag help with the __set_state__ problems in #4549?

@rwgk rwgk mentioned this pull request Apr 13, 2023
Open
3 tasks
rwgk added 2 commits April 26, 2023 17:21
@rwgk
Merge branch 'master' into property_setter_return_master
ea85cbf
@rwgk
Bug fix: obvious in hindsight. I thought the original version was inc…
4ff27ef 
…rementing the reference count for None, but no.

Discovered via many failing tests in the wild (10s of thousands).

It is very tricky to construct a meaningful unit test for this bug specifically. It's unlikely to come back, because 10s of thousands of tests will fail again.
rwgk added a commit to google/pybind11clif that referenced this pull request Apr 28, 2023
@rwgk
Elide to-python conversion of setter return values (#30027)
9efd8a1 
* Snapshot of pybind/pybind11#4621 applied to pywrapcc

* Bug fix: obvious in hindsight. I thought the original version was incrementing the reference count for None, but no.

Discovered via many failing tests in the wild (10s of thousands).

It is very tricky to construct a meaningful unit test for this bug specifically. It's unlikely to come back, because 10s of thousands of tests will fail again.
@rwgk rwgk marked this pull request as ready for review April 28, 2023 23:06
@rwgk rwgk requested review from EthanSteinberg and Skylion007 May 8, 2023 15:37
@rwgk
Copy link
Collaborator Author

rwgk commented May 8, 2023

@Skylion007 @lalaland This is a really small tweak removing an obvious stumbling block. It would be great if someone could review.

@henryiii Do you know why we're getting this failure?

docs/readthedocs.org:pybind11 — Read the Docs build failed!

I saw this the first time about a week ago (smart_holder and/or pywrapcc, not sure).

EthanSteinberg
EthanSteinberg approved these changes May 8, 2023
View reviewed changes
Copy link
Collaborator

@EthanSteinberg EthanSteinberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks mostly good. One suggestion for improving this would be to add a test that explicitly verifies that the conversion on the return value isn't triggered. (Register a custom converter and have that custom converter throw an exception).

@rwgk
Copy link
Collaborator Author

rwgk commented May 8, 2023

Looks mostly good. One suggestion for improving this would be to add a test that explicitly verifies that the conversion on the return value isn't triggered. (Register a custom converter and have that custom converter throw an exception).

Thanks Ethan, the new test ensures already that the conversion is not triggered, see this line in test_methods_and_attributes.cpp:

            &Field::SetIntValue // ... the `FieldBase &` return value here cannot be converted.

This test will fail at runtime without the production code change. In this case an exception is thrown from type_caster_base, similar to what you're suggesting IIUC. Could you please let me know if you still think we should add another test?

@EthanSteinberg
Copy link
Collaborator

Oh, oops, missed that part of the test. Looks good to me then.

@rwgk
Copy link
Collaborator Author

rwgk commented May 8, 2023

Thanks a lot Ethan!

@rwgk rwgk merged commit e9b961d into pybind:master May 8, 2023
@github-actions github-actions bot added the needs changelog Possibly needs a changelog entry label May 8, 2023
@rwgk rwgk deleted the property_setter_return_master branch May 8, 2023 17:14
@rwgk rwgk removed the needs changelog Possibly needs a changelog entry label May 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@EthanSteinberg EthanSteinberg EthanSteinberg approved these changes

@Skylion007 Skylion007 Awaiting requested review from Skylion007

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@rwgk @Skylion007 @EthanSteinberg