From e9b961d9b913575c07ba28c038c3706731768da6 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Mon, 8 May 2023 10:13:54 -0700 Subject: [PATCH] Elide to-python conversion of setter return values (#4621) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Reproducer for property setter with return type that is not wrapped. * Use `py::class_()` to work around the return value conversion issue. * WIP drop_return_value * Remove struct drop_return_value * Introduce `return_value_policy::return_none` for use by setters. * Add `is_setter` to attr.h and use from `.def_property()` * Merge the new test into test_methods_and_attributes * Remove return_none return_value_policy again. * Fix oversight (NOLINTNEXTLINE placement). * Simplification (for the better) found while searching for a way to resolve 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_& pybind11::class_::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_& pybind11::class_::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_& pybind11::class_::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(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}; = 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 &...) {} | ^~~~~~~~~~~~ ``` * 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. --- include/pybind11/attr.h | 16 +++++++++++-- include/pybind11/pybind11.h | 18 ++++++++++---- tests/test_methods_and_attributes.cpp | 34 +++++++++++++++++++++++++++ tests/test_methods_and_attributes.py | 9 +++++++ 4 files changed, 70 insertions(+), 7 deletions(-) diff --git a/include/pybind11/attr.h b/include/pybind11/attr.h index b5e3b7b22..1044db94d 100644 --- a/include/pybind11/attr.h +++ b/include/pybind11/attr.h @@ -26,6 +26,9 @@ struct is_method { explicit is_method(const handle &c) : class_(c) {} }; +/// Annotation for setters +struct is_setter {}; + /// Annotation for operators struct is_operator {}; @@ -188,8 +191,8 @@ struct argument_record { struct function_record { function_record() : is_constructor(false), is_new_style_constructor(false), is_stateless(false), - is_operator(false), is_method(false), has_args(false), has_kwargs(false), - prepend(false) {} + is_operator(false), is_method(false), is_setter(false), has_args(false), + has_kwargs(false), prepend(false) {} /// Function name char *name = nullptr; /* why no C++ strings? They generate heavier code.. */ @@ -230,6 +233,9 @@ struct function_record { /// True if this is a method bool is_method : 1; + /// True if this is a setter + bool is_setter : 1; + /// True if the function has a '*args' argument bool has_args : 1; @@ -426,6 +432,12 @@ struct process_attribute : process_attribute_default { } }; +/// Process an attribute which indicates that this function is a setter +template <> +struct process_attribute : process_attribute_default { + static void init(const is_setter &, function_record *r) { r->is_setter = true; } +}; + /// Process an attribute which indicates the parent scope of a method template <> struct process_attribute : process_attribute_default { diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index 6205effd6..28ebc2229 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -84,6 +84,7 @@ public: cpp_function() = default; // NOLINTNEXTLINE(google-explicit-constructor) cpp_function(std::nullptr_t) {} + cpp_function(std::nullptr_t, const is_setter &) {} /// Construct a cpp_function from a vanilla function pointer template @@ -244,10 +245,16 @@ protected: using Guard = extract_guard_t; /* Perform the function call */ - handle result - = cast_out::cast(std::move(args_converter).template call(cap->f), - policy, - call.parent); + handle result; + if (call.func.is_setter) { + (void) std::move(args_converter).template call(cap->f); + result = none().release(); + } else { + result = cast_out::cast( + std::move(args_converter).template call(cap->f), + policy, + call.parent); + } /* Invoke call policy post-call hook */ process_attributes::postcall(call, result); @@ -1729,7 +1736,8 @@ public: template class_ & def_property(const char *name, const Getter &fget, const Setter &fset, const Extra &...extra) { - return def_property(name, fget, cpp_function(method_adaptor(fset)), extra...); + return def_property( + name, fget, cpp_function(method_adaptor(fset), is_setter()), extra...); } template class_ &def_property(const char *name, diff --git a/tests/test_methods_and_attributes.cpp b/tests/test_methods_and_attributes.cpp index 815dd5e98..31d46eb7e 100644 --- a/tests/test_methods_and_attributes.cpp +++ b/tests/test_methods_and_attributes.cpp @@ -177,6 +177,38 @@ struct RValueRefParam { std::size_t func4(std::string &&s) const & { return s.size(); } }; +namespace pybind11_tests { +namespace exercise_is_setter { + +struct FieldBase { + int int_value() const { return int_value_; } + + FieldBase &SetIntValue(int int_value) { + int_value_ = int_value; + return *this; + } + +private: + int int_value_ = -99; +}; + +struct Field : FieldBase {}; + +void add_bindings(py::module &m) { + py::module sm = m.def_submodule("exercise_is_setter"); + // NOTE: FieldBase is not wrapped, therefore ... + py::class_(sm, "Field") + .def(py::init<>()) + .def_property( + "int_value", + &Field::int_value, + &Field::SetIntValue // ... the `FieldBase &` return value here cannot be converted. + ); +} + +} // namespace exercise_is_setter +} // namespace pybind11_tests + TEST_SUBMODULE(methods_and_attributes, m) { // test_methods_and_attributes py::class_ emna(m, "ExampleMandA"); @@ -456,4 +488,6 @@ TEST_SUBMODULE(methods_and_attributes, m) { .def("func2", &RValueRefParam::func2) .def("func3", &RValueRefParam::func3) .def("func4", &RValueRefParam::func4); + + pybind11_tests::exercise_is_setter::add_bindings(m); } diff --git a/tests/test_methods_and_attributes.py b/tests/test_methods_and_attributes.py index a85468575..955a85f67 100644 --- a/tests/test_methods_and_attributes.py +++ b/tests/test_methods_and_attributes.py @@ -522,3 +522,12 @@ def test_rvalue_ref_param(): assert r.func2("1234") == 4 assert r.func3("12345") == 5 assert r.func4("123456") == 6 + + +def test_is_setter(): + fld = m.exercise_is_setter.Field() + assert fld.int_value == -99 + setter_return = fld.int_value = 100 + assert isinstance(setter_return, int) + assert setter_return == 100 + assert fld.int_value == 100