diff --git a/docs/advanced/functions.rst b/docs/advanced/functions.rst index 7c97a0f7e..7ffdfaa80 100644 --- a/docs/advanced/functions.rst +++ b/docs/advanced/functions.rst @@ -256,16 +256,21 @@ Such functions can also be created using pybind11: m.def("generic", &generic); The class ``py::args`` derives from ``py::tuple`` and ``py::kwargs`` derives -from ``py::dict``. Note that the ``kwargs`` argument is invalid if no keyword -arguments were actually provided. Please refer to the other examples for -details on how to iterate over these, and on how to cast their entries into -C++ objects. A demonstration is also available in -``tests/test_kwargs_and_defaults.cpp``. +from ``py::dict``. -.. warning:: +You may also use just one or the other, and may combine these with other +arguments as long as the ``py::args`` and ``py::kwargs`` arguments are the last +arguments accepted by the function. - Unlike Python, pybind11 does not allow combining normal parameters with the - ``args`` / ``kwargs`` special parameters. +Please refer to the other examples for details on how to iterate over these, +and on how to cast their entries into C++ objects. A demonstration is also +available in ``tests/test_kwargs_and_defaults.cpp``. + +.. note:: + + When combining \*args or \*\*kwargs with :ref:`keyword_args` you should + *not* include ``py::arg`` tags for the ``py::args`` and ``py::kwargs`` + arguments. Default arguments revisited =========================== diff --git a/include/pybind11/attr.h b/include/pybind11/attr.h index 41f59e78b..c07cd6c94 100644 --- a/include/pybind11/attr.h +++ b/include/pybind11/attr.h @@ -69,7 +69,7 @@ struct undefined_t; template struct op_; template struct init; template struct init_alias; -inline void keep_alive_impl(int Nurse, int Patient, handle args, handle ret); +inline void keep_alive_impl(int Nurse, int Patient, function_arguments args, handle ret); /// Internal data structure which holds metadata about a keyword argument struct argument_record { @@ -100,7 +100,7 @@ struct function_record { std::vector args; /// Pointer to lambda function which converts arguments and performs the actual call - handle (*impl) (function_record *, handle, handle, handle) = nullptr; + handle (*impl) (function_record *, function_arguments, handle) = nullptr; /// Storage for the wrapped function pointer and captured data, if any void *data[3] = { }; @@ -129,7 +129,7 @@ struct function_record { /// True if this is a method bool is_method : 1; - /// Number of arguments + /// Number of arguments (including py::args and/or py::kwargs, if present) uint16_t nargs; /// Python method object @@ -233,8 +233,8 @@ template struct process_attribute_default { /// Default implementation: do nothing static void init(const T &, function_record *) { } static void init(const T &, type_record *) { } - static void precall(handle) { } - static void postcall(handle, handle) { } + static void precall(function_arguments) { } + static void postcall(function_arguments, handle) { } }; /// Process an attribute specifying the function's name @@ -362,13 +362,13 @@ struct process_attribute : process_attribute_default {}; */ template struct process_attribute> : public process_attribute_default> { template = 0> - static void precall(handle args) { keep_alive_impl(Nurse, Patient, args, handle()); } + static void precall(function_arguments args) { keep_alive_impl(Nurse, Patient, args, handle()); } template = 0> - static void postcall(handle, handle) { } + static void postcall(function_arguments, handle) { } template = 0> - static void precall(handle) { } + static void precall(function_arguments) { } template = 0> - static void postcall(handle args, handle ret) { keep_alive_impl(Nurse, Patient, args, ret); } + static void postcall(function_arguments args, handle ret) { keep_alive_impl(Nurse, Patient, args, ret); } }; /// Recursively iterate over variadic template arguments @@ -381,11 +381,11 @@ template struct process_attributes { int unused[] = { 0, (process_attribute::type>::init(args, r), 0) ... }; ignore_unused(unused); } - static void precall(handle fn_args) { + static void precall(function_arguments fn_args) { int unused[] = { 0, (process_attribute::type>::precall(fn_args), 0) ... }; ignore_unused(unused); } - static void postcall(handle fn_args, handle fn_ret) { + static void postcall(function_arguments fn_args, handle fn_ret) { int unused[] = { 0, (process_attribute::type>::postcall(fn_args, fn_ret), 0) ... }; ignore_unused(unused); } @@ -395,8 +395,8 @@ template struct process_attributes { template ::value...), size_t self = constexpr_sum(std::is_same::value...)> -constexpr bool expected_num_args(size_t nargs) { - return named == 0 || (self + named) == nargs; +constexpr bool expected_num_args(size_t nargs, bool has_args, bool has_kwargs) { + return named == 0 || (self + named + has_args + has_kwargs) == nargs; } NAMESPACE_END(detail) diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h index c9312de79..5f8eaf87a 100644 --- a/include/pybind11/cast.h +++ b/include/pybind11/cast.h @@ -1245,22 +1245,42 @@ constexpr arg operator"" _a(const char *name, size_t) { return arg(name); } NAMESPACE_BEGIN(detail) +// forward declaration +struct function_record; + +// Helper struct to only allow py::args and/or py::kwargs at the end of the function arguments +template struct assert_args_kwargs_must_be_last { + static constexpr bool has_args = args, has_kwargs = kwargs; + static_assert(args_kwargs_are_last, "py::args/py::kwargs are only permitted as the last argument(s) of a function"); +}; +template struct args_kwargs_must_be_last; +template struct args_kwargs_must_be_last + : args_kwargs_must_be_last {}; +template struct args_kwargs_must_be_last + : assert_args_kwargs_must_be_last {}; +template struct args_kwargs_must_be_last + : assert_args_kwargs_must_be_last {}; +template struct args_kwargs_must_be_last + : assert_args_kwargs_must_be_last {}; +template <> struct args_kwargs_must_be_last<> : assert_args_kwargs_must_be_last {}; + +using function_arguments = const std::vector &; + /// Helper class which loads arguments for C++ functions called from Python template class argument_loader { - using itypes = type_list...>; using indices = make_index_sequence; -public: - argument_loader() : value() {} // Helps gcc-7 properly initialize value + using check_args_kwargs = args_kwargs_must_be_last...>; - static constexpr auto has_kwargs = std::is_same>::value; - static constexpr auto has_args = has_kwargs || std::is_same>::value; +public: + static constexpr bool has_kwargs = check_args_kwargs::has_kwargs; + static constexpr bool has_args = check_args_kwargs::has_args; static PYBIND11_DESCR arg_names() { return detail::concat(make_caster::name()...); } - bool load_args(handle args, handle kwargs) { - return load_impl(args, kwargs, itypes{}); + bool load_args(function_arguments args) { + return load_impl_sequence(args, indices{}); } template @@ -1275,26 +1295,12 @@ public: } private: - bool load_impl(handle args_, handle, type_list) { - std::get<0>(value).load(args_, true); - return true; - } - bool load_impl(handle args_, handle kwargs_, type_list) { - std::get<0>(value).load(args_, true); - std::get<1>(value).load(kwargs_, true); - return true; - } - - bool load_impl(handle args, handle, ... /* anything else */) { - return load_impl_sequence(args, indices{}); - } - - static bool load_impl_sequence(handle, index_sequence<>) { return true; } + static bool load_impl_sequence(function_arguments, index_sequence<>) { return true; } template - bool load_impl_sequence(handle src, index_sequence) { - for (bool r : {std::get(value).load(PyTuple_GET_ITEM(src.ptr(), Is), true)...}) + bool load_impl_sequence(function_arguments args, index_sequence) { + for (bool r : {std::get(value).load(args[Is], true)...}) if (!r) return false; return true; diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index 420e18e3c..dc21caf60 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -82,8 +82,6 @@ protected: /// Special internal constructor for functors, lambda functions, etc. template void initialize(Func &&f, Return (*)(Args...) PYBIND11_NOEXCEPT_SPECIFIER, const Extra&... extra) { - static_assert(detail::expected_num_args(sizeof...(Args)), - "The number of named arguments does not match the function signature"); struct capture { typename std::remove_reference::type f; }; @@ -116,12 +114,15 @@ protected: detail::conditional_t::value, detail::void_type, Return> >; + static_assert(detail::expected_num_args(sizeof...(Args), cast_in::has_args, cast_in::has_kwargs), + "The number of named arguments does not match the function signature"); + /* Dispatch code which converts function arguments and performs the actual function call */ - rec->impl = [](detail::function_record *rec, handle args, handle kwargs, handle parent) -> handle { + rec->impl = [](detail::function_record *rec, detail::function_arguments args, handle parent) -> handle { cast_in args_converter; /* Try to cast the function arguments into the C++ domain */ - if (!args_converter.load_args(args, kwargs)) + if (!args_converter.load_args(args)) return PYBIND11_TRY_NEXT_OVERLOAD; /* Invoke call policy pre-call hook */ @@ -379,66 +380,144 @@ protected: } /// Main dispatch logic for calls to functions bound using pybind11 - static PyObject *dispatcher(PyObject *self, PyObject *args, PyObject *kwargs) { + static PyObject *dispatcher(PyObject *self, PyObject *args_in, PyObject *kwargs_in) { /* Iterator over the list of potentially admissible overloads */ detail::function_record *overloads = (detail::function_record *) PyCapsule_GetPointer(self, nullptr), *it = overloads; /* Need to know how many arguments + keyword arguments there are to pick the right overload */ - size_t nargs = (size_t) PyTuple_GET_SIZE(args), - nkwargs = kwargs ? (size_t) PyDict_Size(kwargs) : 0; + const size_t n_args_in = (size_t) PyTuple_GET_SIZE(args_in); - handle parent = nargs > 0 ? PyTuple_GET_ITEM(args, 0) : nullptr, + handle parent = n_args_in > 0 ? PyTuple_GET_ITEM(args_in, 0) : nullptr, result = PYBIND11_TRY_NEXT_OVERLOAD; try { for (; it != nullptr; it = it->next) { - auto args_ = reinterpret_borrow(args); - size_t kwargs_consumed = 0; - /* For each overload: - 1. If the required list of arguments is longer than the - actually provided amount, create a copy of the argument - list and fill in any available keyword/default arguments. - 2. Ensure that all keyword arguments were "consumed" - 3. Call the function call dispatcher (function_record::impl) + 1. Copy all positional arguments we were given, also checking to make sure that + named positional arguments weren't *also* specified via kwarg. + 2. If we weren't given enough, try to make up the ommitted ones by checking + whether they were provided by a kwarg matching the `py::arg("name")` name. If + so, use it (and remove it from kwargs; if not, see if the function binding + provided a default that we can use. + 3. Ensure that either all keyword arguments were "consumed", or that the function + takes a kwargs argument to accept unconsumed kwargs. + 4. Any positional arguments still left get put into a tuple (for args), and any + leftover kwargs get put into a dict. + 5. Pack everything into a vector; if we have py::args or py::kwargs, they are an + extra tuple or dict at the end of the positional arguments. + 6. Call the function call dispatcher (function_record::impl) + + If one of these fail, move on to the next overload and keep trying until we get a + result other than PYBIND11_TRY_NEXT_OVERLOAD. */ - size_t nargs_ = nargs; - if (nargs < it->args.size()) { - nargs_ = it->args.size(); - args_ = tuple(nargs_); - for (size_t i = 0; i < nargs; ++i) { - handle item = PyTuple_GET_ITEM(args, i); - PyTuple_SET_ITEM(args_.ptr(), i, item.inc_ref().ptr()); - } - int arg_ctr = 0; - for (auto const &it2 : it->args) { - int index = arg_ctr++; - if (PyTuple_GET_ITEM(args_.ptr(), index)) - continue; + size_t pos_args = it->nargs; // Number of positional arguments that we need + if (it->has_args) --pos_args; // (but don't count py::args + if (it->has_kwargs) --pos_args; // or py::kwargs) - handle value; - if (kwargs) - value = PyDict_GetItemString(kwargs, it2.name); + if (!it->has_args && n_args_in > pos_args) + continue; // Too many arguments for this overload + if (n_args_in < pos_args && it->args.size() < pos_args) + continue; // Not enough arguments given, and not enough defaults to fill in the blanks + + std::vector pass_args; + pass_args.reserve(it->nargs); + + size_t args_to_copy = std::min(pos_args, n_args_in); + size_t args_copied = 0; + + // 1. Copy any position arguments given. + for (; args_copied < args_to_copy; ++args_copied) { + // If we find a given positional argument that also has a named kwargs argument, + // raise a TypeError like Python does. (We could also continue with the next + // overload, but this seems highly likely to be a caller mistake rather than a + // legitimate overload). + if (kwargs_in && args_copied < it->args.size()) { + handle value = PyDict_GetItemString(kwargs_in, it->args[args_copied].name); if (value) - kwargs_consumed++; - else if (it2.value) - value = it2.value; - - if (value) { - PyTuple_SET_ITEM(args_.ptr(), index, value.inc_ref().ptr()); - } else { - kwargs_consumed = (size_t) -1; /* definite failure */ - break; - } + throw type_error(std::string(it->name) + "(): got multiple values for argument '" + + std::string(it->args[args_copied].name) + "'"); } + + pass_args.push_back(PyTuple_GET_ITEM(args_in, args_copied)); } + // We'll need to copy this if we steal some kwargs for defaults + dict kwargs = reinterpret_borrow(kwargs_in); + + // 2. Check kwargs and, failing that, defaults that may help complete the list + if (args_copied < pos_args) { + bool copied_kwargs = false; + + for (; args_copied < pos_args; ++args_copied) { + const auto &arg = it->args[args_copied]; + + handle value; + if (kwargs_in) + value = PyDict_GetItemString(kwargs.ptr(), arg.name); + + if (value) { + // Consume a kwargs value + if (!copied_kwargs) { + kwargs = reinterpret_steal(PyDict_Copy(kwargs.ptr())); + copied_kwargs = true; + } + PyDict_DelItemString(kwargs.ptr(), arg.name); + } + else if (arg.value) { + value = arg.value; + } + + if (value) + pass_args.push_back(value); + else + break; + } + + if (args_copied < pos_args) + continue; // Not enough arguments, defaults, or kwargs to fill the positional arguments + } + + // 3. Check everything was consumed (unless we have a kwargs arg) + if (kwargs && kwargs.size() > 0 && !it->has_kwargs) + continue; // Unconsumed kwargs, but no py::kwargs argument to accept them + + // 4a. If we have a py::args argument, create a new tuple with leftovers + tuple extra_args; + if (it->has_args) { + if (args_to_copy == 0) { + // We didn't copy out any position arguments from the args_in tuple, so we + // can reuse it directly without copying: + extra_args = reinterpret_borrow(args_in); + } + else if (args_copied >= n_args_in) { + extra_args = tuple(0); + } + else { + size_t args_size = n_args_in - args_copied; + extra_args = tuple(args_size); + for (size_t i = 0; i < args_size; ++i) { + handle item = PyTuple_GET_ITEM(args_in, args_copied + i); + extra_args[i] = item.inc_ref().ptr(); + } + } + pass_args.push_back(extra_args); + } + + // 4b. If we have a py::kwargs, pass on any remaining kwargs + if (it->has_kwargs) { + if (!kwargs.ptr()) + kwargs = dict(); // If we didn't get one, send an empty one + pass_args.push_back(kwargs); + } + + // 5. Put everything in a big tuple. Not technically step 5, we've been building it + // in `pass_args` all along. + + // 6. Call the function. try { - if ((kwargs_consumed == nkwargs || it->has_kwargs) && - (nargs_ == it->nargs || it->has_args)) - result = it->impl(it, args_, kwargs, parent); + result = it->impl(it, pass_args, parent); } catch (reference_cast_error &) { result = PYBIND11_TRY_NEXT_OVERLOAD; } @@ -512,7 +591,7 @@ protected: msg += "\n"; } msg += "\nInvoked with: "; - auto args_ = reinterpret_borrow(args); + auto args_ = reinterpret_borrow(args_in); for (size_t ti = overloads->is_constructor ? 1 : 0; ti < args_.size(); ++ti) { msg += pybind11::repr(args_[ti]); if ((ti + 1) != args_.size() ) @@ -530,9 +609,8 @@ protected: if (overloads->is_constructor) { /* When a constructor ran successfully, the corresponding holder type (e.g. std::unique_ptr) must still be initialized. */ - PyObject *inst = PyTuple_GET_ITEM(args, 0); - auto tinfo = detail::get_type_info(Py_TYPE(inst)); - tinfo->init_holder(inst, nullptr); + auto tinfo = detail::get_type_info(Py_TYPE(parent.ptr())); + tinfo->init_holder(parent.ptr(), nullptr); } return result.ptr(); } @@ -1416,11 +1494,11 @@ inline void keep_alive_impl(handle nurse, handle patient) { (void) wr.release(); } -PYBIND11_NOINLINE inline void keep_alive_impl(int Nurse, int Patient, handle args, handle ret) { - handle nurse (Nurse > 0 ? PyTuple_GetItem(args.ptr(), Nurse - 1) : ret.ptr()); - handle patient(Patient > 0 ? PyTuple_GetItem(args.ptr(), Patient - 1) : ret.ptr()); - - keep_alive_impl(nurse, patient); +PYBIND11_NOINLINE inline void keep_alive_impl(int Nurse, int Patient, function_arguments args, handle ret) { + keep_alive_impl( + Nurse == 0 ? ret : Nurse > 0 && (size_t) Nurse <= args.size() ? args[Nurse - 1] : handle(), + Patient == 0 ? ret : Patient > 0 && (size_t) Patient <= args.size() ? args[Patient - 1] : handle() + ); } template diff --git a/include/pybind11/pytypes.h b/include/pybind11/pytypes.h index 0090e6638..80b802fcc 100644 --- a/include/pybind11/pytypes.h +++ b/include/pybind11/pytypes.h @@ -149,14 +149,14 @@ public: preferable to use the `object` class which derives from `handle` and calls this function automatically. Returns a reference to itself. \endrst */ - const handle& inc_ref() const { Py_XINCREF(m_ptr); return *this; } + const handle& inc_ref() const & { Py_XINCREF(m_ptr); return *this; } /** \rst Manually decrease the reference count of the Python object. Usually, it is preferable to use the `object` class which derives from `handle` and calls this function automatically. Returns a reference to itself. \endrst */ - const handle& dec_ref() const { Py_XDECREF(m_ptr); return *this; } + const handle& dec_ref() const & { Py_XDECREF(m_ptr); return *this; } /** \rst Attempt to cast the Python object into the given C++ type. A `cast_error` diff --git a/tests/test_kwargs_and_defaults.cpp b/tests/test_kwargs_and_defaults.cpp index 24fc0cd5b..3180123df 100644 --- a/tests/test_kwargs_and_defaults.cpp +++ b/tests/test_kwargs_and_defaults.cpp @@ -28,6 +28,27 @@ py::tuple args_kwargs_function(py::args args, py::kwargs kwargs) { return py::make_tuple(args, kwargs); } +py::tuple mixed_plus_args(int i, double j, py::args args) { + return py::make_tuple(i, j, args); +} + +py::tuple mixed_plus_kwargs(int i, double j, py::kwargs kwargs) { + return py::make_tuple(i, j, kwargs); +} + +py::tuple mixed_plus_args_kwargs(int i, double j, py::args args, py::kwargs kwargs) { + return py::make_tuple(i, j, args, kwargs); +} + +// pybind11 won't allow these to be bound: args and kwargs, if present, must be at the end. +void bad_args1(py::args, int) {} +void bad_args2(py::kwargs, int) {} +void bad_args3(py::kwargs, py::args) {} +void bad_args4(py::args, int, py::kwargs) {} +void bad_args5(py::args, py::kwargs, int) {} +void bad_args6(py::args, py::args) {} +void bad_args7(py::kwargs, py::kwargs) {} + struct KWClass { void foo(int, float) {} }; @@ -53,4 +74,20 @@ test_initializer arg_keywords_and_defaults([](py::module &m) { py::class_(m, "KWClass") .def("foo0", &KWClass::foo) .def("foo1", &KWClass::foo, "x"_a, "y"_a); + + m.def("mixed_plus_args", &mixed_plus_args); + m.def("mixed_plus_kwargs", &mixed_plus_kwargs); + m.def("mixed_plus_args_kwargs", &mixed_plus_args_kwargs); + + m.def("mixed_plus_args_kwargs_defaults", &mixed_plus_args_kwargs, + py::arg("i") = 1, py::arg("j") = 3.14159); + + // Uncomment these to test that the static_assert is indeed working: +// m.def("bad_args1", &bad_args1); +// m.def("bad_args2", &bad_args2); +// m.def("bad_args3", &bad_args3); +// m.def("bad_args4", &bad_args4); +// m.def("bad_args5", &bad_args5); +// m.def("bad_args6", &bad_args6); +// m.def("bad_args7", &bad_args7); }); diff --git a/tests/test_kwargs_and_defaults.py b/tests/test_kwargs_and_defaults.py index 852d03c6e..d1777a4ee 100644 --- a/tests/test_kwargs_and_defaults.py +++ b/tests/test_kwargs_and_defaults.py @@ -55,3 +55,52 @@ def test_arg_and_kwargs(): args = 'a1', 'a2' kwargs = dict(arg3='a3', arg4=4) assert args_kwargs_function(*args, **kwargs) == (args, kwargs) + + +def test_mixed_args_and_kwargs(msg): + from pybind11_tests import (mixed_plus_args, mixed_plus_kwargs, mixed_plus_args_kwargs, + mixed_plus_args_kwargs_defaults) + mpa = mixed_plus_args + mpk = mixed_plus_kwargs + mpak = mixed_plus_args_kwargs + mpakd = mixed_plus_args_kwargs_defaults + + assert mpa(1, 2.5, 4, 99.5, None) == (1, 2.5, (4, 99.5, None)) + assert mpa(1, 2.5) == (1, 2.5, ()) + with pytest.raises(TypeError) as excinfo: + assert mpa(1) + assert msg(excinfo.value) == """ + mixed_plus_args(): incompatible function arguments. The following argument types are supported: + 1. (arg0: int, arg1: float, *args) -> tuple + + Invoked with: 1 + """ # noqa: E501 + with pytest.raises(TypeError) as excinfo: + assert mpa() + assert msg(excinfo.value) == """ + mixed_plus_args(): incompatible function arguments. The following argument types are supported: + 1. (arg0: int, arg1: float, *args) -> tuple + + Invoked with: + """ # noqa: E501 + + assert mpk(-2, 3.5, pi=3.14159, e=2.71828) == (-2, 3.5, {'e': 2.71828, 'pi': 3.14159}) + assert mpak(7, 7.7, 7.77, 7.777, 7.7777, minusseven=-7) == ( + 7, 7.7, (7.77, 7.777, 7.7777), {'minusseven': -7}) + assert mpakd() == (1, 3.14159, (), {}) + assert mpakd(3) == (3, 3.14159, (), {}) + assert mpakd(j=2.71828) == (1, 2.71828, (), {}) + assert mpakd(k=42) == (1, 3.14159, (), {'k': 42}) + assert mpakd(1, 1, 2, 3, 5, 8, then=13, followedby=21) == ( + 1, 1, (2, 3, 5, 8), {'then': 13, 'followedby': 21}) + # Arguments specified both positionally and via kwargs is an error: + with pytest.raises(TypeError) as excinfo: + assert mpakd(1, i=1) + assert msg(excinfo.value) == """ + mixed_plus_args_kwargs_defaults(): got multiple values for argument 'i' + """ + with pytest.raises(TypeError) as excinfo: + assert mpakd(1, 2, j=1) + assert msg(excinfo.value) == """ + mixed_plus_args_kwargs_defaults(): got multiple values for argument 'j' + """