Elide to-python conversion of setter return values (#30027)

* 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.

Author: rwgk

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 {};
 
@@ -190,8 +193,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.. */
@@ -232,6 +235,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;
 
@@ -433,6 +439,12 @@ struct process_attribute<is_method> : process_attribute_default<is_method> {
     }
 };
 
+/// Process an attribute which indicates that this function is a setter
+template <>
+struct process_attribute<is_setter> : process_attribute_default<is_setter> {
+    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<scope> : process_attribute_default<scope> {

include/pybind11/pybind11.h

@@ -86,6 +86,7 @@ class cpp_function : public function {
     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 <typename Return, typename... Args, typename... Extra>
@@ -246,8 +247,16 @@ class cpp_function : public function {
             using Guard = extract_guard_t<Extra...>;
 
             /* Perform the function call */
-            handle result = cast_out::cast(
-                std::move(args_converter).template call<Return, Guard>(cap->f), rvpp, call.parent);
+            handle result;
+            if (call.func.is_setter) {
+                (void) std::move(args_converter).template call<Return, Guard>(cap->f);
+                result = none().release();
+            } else {
+                result = cast_out::cast(
+                    std::move(args_converter).template call<Return, Guard>(cap->f),
+                    rvpp,
+                    call.parent);
+            }
 
             /* Invoke call policy post-call hook */
             process_attributes<Extra...>::postcall(call, result);
@@ -1982,7 +1991,8 @@ class class_ : public detail::generic_type {
     template <typename Getter, typename Setter, typename... Extra>
     class_ &
     def_property(const char *name, const Getter &fget, const Setter &fset, const Extra &...extra) {
-        return def_property(name, fget, cpp_function(method_adaptor<type>(fset)), extra...);
+        return def_property(
+            name, fget, cpp_function(method_adaptor<type>(fset), is_setter()), extra...);
     }
     template <typename Getter, typename... Extra>
     class_ &def_property(const char *name,

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_<Field>(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_<ExampleMandA> emna(m, "ExampleMandA");
@@ -460,4 +492,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);
 }

tests/test_methods_and_attributes.py

@@ -524,3 +524,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