mirror of
https://github.com/pybind/pybind11.git
synced 2024-11-11 08:03:55 +00:00
Elide to-python conversion of setter return values (#4621)
* Reproducer for property setter with return type that is not wrapped. * Use `py::class_<OptionsBase>()` 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_<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 &...) {} | ^~~~~~~~~~~~ ``` * 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.
This commit is contained in:
parent
90312a6ee8
commit
e9b961d9b9
@ -26,6 +26,9 @@ struct is_method {
|
|||||||
explicit is_method(const handle &c) : class_(c) {}
|
explicit is_method(const handle &c) : class_(c) {}
|
||||||
};
|
};
|
||||||
|
|
||||||
|
/// Annotation for setters
|
||||||
|
struct is_setter {};
|
||||||
|
|
||||||
/// Annotation for operators
|
/// Annotation for operators
|
||||||
struct is_operator {};
|
struct is_operator {};
|
||||||
|
|
||||||
@ -188,8 +191,8 @@ struct argument_record {
|
|||||||
struct function_record {
|
struct function_record {
|
||||||
function_record()
|
function_record()
|
||||||
: is_constructor(false), is_new_style_constructor(false), is_stateless(false),
|
: is_constructor(false), is_new_style_constructor(false), is_stateless(false),
|
||||||
is_operator(false), is_method(false), has_args(false), has_kwargs(false),
|
is_operator(false), is_method(false), is_setter(false), has_args(false),
|
||||||
prepend(false) {}
|
has_kwargs(false), prepend(false) {}
|
||||||
|
|
||||||
/// Function name
|
/// Function name
|
||||||
char *name = nullptr; /* why no C++ strings? They generate heavier code.. */
|
char *name = nullptr; /* why no C++ strings? They generate heavier code.. */
|
||||||
@ -230,6 +233,9 @@ struct function_record {
|
|||||||
/// True if this is a method
|
/// True if this is a method
|
||||||
bool is_method : 1;
|
bool is_method : 1;
|
||||||
|
|
||||||
|
/// True if this is a setter
|
||||||
|
bool is_setter : 1;
|
||||||
|
|
||||||
/// True if the function has a '*args' argument
|
/// True if the function has a '*args' argument
|
||||||
bool has_args : 1;
|
bool has_args : 1;
|
||||||
|
|
||||||
@ -426,6 +432,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
|
/// Process an attribute which indicates the parent scope of a method
|
||||||
template <>
|
template <>
|
||||||
struct process_attribute<scope> : process_attribute_default<scope> {
|
struct process_attribute<scope> : process_attribute_default<scope> {
|
||||||
|
@ -84,6 +84,7 @@ public:
|
|||||||
cpp_function() = default;
|
cpp_function() = default;
|
||||||
// NOLINTNEXTLINE(google-explicit-constructor)
|
// NOLINTNEXTLINE(google-explicit-constructor)
|
||||||
cpp_function(std::nullptr_t) {}
|
cpp_function(std::nullptr_t) {}
|
||||||
|
cpp_function(std::nullptr_t, const is_setter &) {}
|
||||||
|
|
||||||
/// Construct a cpp_function from a vanilla function pointer
|
/// Construct a cpp_function from a vanilla function pointer
|
||||||
template <typename Return, typename... Args, typename... Extra>
|
template <typename Return, typename... Args, typename... Extra>
|
||||||
@ -244,10 +245,16 @@ protected:
|
|||||||
using Guard = extract_guard_t<Extra...>;
|
using Guard = extract_guard_t<Extra...>;
|
||||||
|
|
||||||
/* Perform the function call */
|
/* Perform the function call */
|
||||||
handle result
|
handle result;
|
||||||
= cast_out::cast(std::move(args_converter).template call<Return, Guard>(cap->f),
|
if (call.func.is_setter) {
|
||||||
policy,
|
(void) std::move(args_converter).template call<Return, Guard>(cap->f);
|
||||||
call.parent);
|
result = none().release();
|
||||||
|
} else {
|
||||||
|
result = cast_out::cast(
|
||||||
|
std::move(args_converter).template call<Return, Guard>(cap->f),
|
||||||
|
policy,
|
||||||
|
call.parent);
|
||||||
|
}
|
||||||
|
|
||||||
/* Invoke call policy post-call hook */
|
/* Invoke call policy post-call hook */
|
||||||
process_attributes<Extra...>::postcall(call, result);
|
process_attributes<Extra...>::postcall(call, result);
|
||||||
@ -1729,7 +1736,8 @@ public:
|
|||||||
template <typename Getter, typename Setter, typename... Extra>
|
template <typename Getter, typename Setter, typename... Extra>
|
||||||
class_ &
|
class_ &
|
||||||
def_property(const char *name, const Getter &fget, const Setter &fset, const Extra &...extra) {
|
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>
|
template <typename Getter, typename... Extra>
|
||||||
class_ &def_property(const char *name,
|
class_ &def_property(const char *name,
|
||||||
|
@ -177,6 +177,38 @@ struct RValueRefParam {
|
|||||||
std::size_t func4(std::string &&s) const & { return s.size(); }
|
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_SUBMODULE(methods_and_attributes, m) {
|
||||||
// test_methods_and_attributes
|
// test_methods_and_attributes
|
||||||
py::class_<ExampleMandA> emna(m, "ExampleMandA");
|
py::class_<ExampleMandA> emna(m, "ExampleMandA");
|
||||||
@ -456,4 +488,6 @@ TEST_SUBMODULE(methods_and_attributes, m) {
|
|||||||
.def("func2", &RValueRefParam::func2)
|
.def("func2", &RValueRefParam::func2)
|
||||||
.def("func3", &RValueRefParam::func3)
|
.def("func3", &RValueRefParam::func3)
|
||||||
.def("func4", &RValueRefParam::func4);
|
.def("func4", &RValueRefParam::func4);
|
||||||
|
|
||||||
|
pybind11_tests::exercise_is_setter::add_bindings(m);
|
||||||
}
|
}
|
||||||
|
@ -522,3 +522,12 @@ def test_rvalue_ref_param():
|
|||||||
assert r.func2("1234") == 4
|
assert r.func2("1234") == 4
|
||||||
assert r.func3("12345") == 5
|
assert r.func3("12345") == 5
|
||||||
assert r.func4("123456") == 6
|
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
|
||||||
|
Loading…
Reference in New Issue
Block a user