Add support for positional args with args/kwargs

jagerman · jagerman · commit 80b04cb17dff · 2017-01-22T01:01:30.000-05:00
This commit rewrites the function dispatcher code to support mixing
regular arguments with py::args/py::kwargs arguments.  It also
simplifies the argument loader noticeably as it no longer has to worry
about args/kwargs: all of that is now sorted out in the dispatcher,
which now simply appends a tuple/dict if the function takes
py::args/py::kwargs, then passes all the arguments in a single tuple.

Some (intentional) restrictions:
- you may not bind a function that has args/kwargs somewhere other than
  the end (this somewhat matches Python, and keeps the dispatch code a
  little cleaner by being able to not worry about where to inject the
  args/kwargs in the argument list).
- If you specify an argument both positionally and via a keyword
  argument, you get a TypeError (as you do in Python).
diff --git 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.
+
+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::
 
-   Unlike Python, pybind11 does not allow combining normal parameters with the
-   ``args`` / ``kwargs`` special parameters.
+    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
@@ -95,7 +95,7 @@ struct function_record {
     std::vector<argument_record> 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 *, handle, handle) = nullptr;
 
     /// Storage for the wrapped function pointer and captured data, if any
     void *data[3] = { };
@@ -124,7 +124,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
@@ -378,8 +378,8 @@ template <typename... Args> struct process_attributes {
 template <typename... Extra,
           size_t named = constexpr_sum(std::is_base_of<arg, Extra>::value...),
           size_t self  = constexpr_sum(std::is_same<is_method, Extra>::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
@@ -1217,22 +1217,40 @@ 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 <bool args, bool kwargs, bool args_kwargs_are_last> 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 <typename... T> struct args_kwargs_must_be_last;
+template <typename T1, typename... Tmore> struct args_kwargs_must_be_last<T1, Tmore...>
+    : args_kwargs_must_be_last<Tmore...> {};
+template <typename... T> struct args_kwargs_must_be_last<args, T...>
+    : assert_args_kwargs_must_be_last<true, false, sizeof...(T) == 0> {};
+template <typename... T> struct args_kwargs_must_be_last<kwargs, T...>
+    : assert_args_kwargs_must_be_last<false, true, sizeof...(T) == 0> {};
+template <typename... T> struct args_kwargs_must_be_last<args, kwargs, T...>
+    : assert_args_kwargs_must_be_last<true, true, sizeof...(T) == 0> {};
+template <> struct args_kwargs_must_be_last<> : assert_args_kwargs_must_be_last<false, false, true> {};
+
 /// Helper class which loads arguments for C++ functions called from Python
 template <typename... Args>
 class argument_loader {
-    using itypes = type_list<intrinsic_t<Args>...>;
     using indices = make_index_sequence<sizeof...(Args)>;
 
-public:
-    argument_loader() : value() {} // Helps gcc-7 properly initialize value
+    using check_args_kwargs = args_kwargs_must_be_last<intrinsic_t<Args>...>;
 
-    static constexpr auto has_kwargs = std::is_same<itypes, type_list<args, kwargs>>::value;
-    static constexpr auto has_args = has_kwargs || std::is_same<itypes, type_list<args>>::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<Args>::name()...); }
 
-    bool load_args(handle args, handle kwargs) {
-        return load_impl(args, kwargs, itypes{});
+    bool load_args(handle args) {
+        return load_impl_sequence(args, indices{});
     }
 
     template <typename Return, typename Func>
@@ -1247,26 +1265,12 @@ class argument_loader {
     }
 
 private:
-    bool load_impl(handle args_, handle, type_list<args>) {
-        std::get<0>(value).load(args_, true);
-        return true;
-    }
-
-    bool load_impl(handle args_, handle kwargs_, type_list<args, kwargs>) {
-        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; }
 
     template <size_t... Is>
-    bool load_impl_sequence(handle src, index_sequence<Is...>) {
-        for (bool r : {std::get<Is>(value).load(PyTuple_GET_ITEM(src.ptr(), Is), true)...})
+    bool load_impl_sequence(handle args, index_sequence<Is...>) {
+        for (bool r : {std::get<Is>(value).load(PyTuple_GET_ITEM(args.ptr(), Is), true)...})
             if (!r)
                 return false;
         return true;
diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h
@@ -82,7 +82,9 @@ class cpp_function : public function {
     /// Special internal constructor for functors, lambda functions, etc.
     template <typename Func, typename Return, typename... Args, typename... Extra /*,*/ PYBIND11_NOEXCEPT_TPL_ARG>
     void initialize(Func &&f, Return (*)(Args...) PYBIND11_NOEXCEPT_SPECIFIER, const Extra&... extra) {
-        static_assert(detail::expected_num_args<Extra...>(sizeof...(Args)),
+        constexpr bool have_args   = detail::any_of<std::is_same<args,   detail::intrinsic_t<Args>>...>::value,
+                       have_kwargs = detail::any_of<std::is_same<kwargs, detail::intrinsic_t<Args>>...>::value;
+        static_assert(detail::expected_num_args<Extra...>(sizeof...(Args), have_args, have_kwargs),
                       "The number of named arguments does not match the function signature");
 
         struct capture { typename std::remove_reference<Func>::type f; };
@@ -117,11 +119,11 @@ class cpp_function : public function {
         >;
 
         /* 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, handle 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 +381,144 @@ class cpp_function : public function {
     }
 
     /// 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<tuple>(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 tuple; 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
 
+                tuple pass_args(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;
+                            throw type_error(std::string(it->name) + "(): got multiple values for argument '" +
+                                    std::string(it->args[args_copied].name) + "'");
+                    }
+
+                    handle item = PyTuple_GET_ITEM(args_in, args_copied);
+                    PyTuple_SET_ITEM(pass_args.ptr(), args_copied, item.inc_ref().ptr());
+                }
+
+                // We'll need to copy this if we steal some kwargs for defaults
+                dict kwargs = reinterpret_borrow<dict>(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) {
-                            PyTuple_SET_ITEM(args_.ptr(), index, value.inc_ref().ptr());
-                        } else {
-                            kwargs_consumed = (size_t) -1; /* definite failure */
+                            // Consume a kwargs value
+                            if (!copied_kwargs) {
+                                kwargs = reinterpret_steal<dict>(PyDict_Copy(kwargs.ptr()));
+                                copied_kwargs = true;
+                            }
+                            PyDict_DelItemString(kwargs.ptr(), arg.name);
+                        }
+                        else if (arg.value) {
+                            value = arg.value;
+                        }
+
+                        if (value)
+                            PyTuple_SET_ITEM(pass_args.ptr(), args_copied, value.inc_ref().ptr());
+                        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
+                if (it->has_args) {
+                    tuple extra_args;
+                    if (args_to_copy == 0) {
+                        // We didn't copy out any position arguments from the args_in tuple, so we
+                        // can use it directly:
+                        extra_args = reinterpret_borrow<tuple>(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();
                         }
                     }
+                    PyTuple_SET_ITEM(pass_args.ptr(), args_copied++, extra_args.release().ptr());
                 }
 
+                // 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
+                    PyTuple_SET_ITEM(pass_args.ptr(), args_copied++, kwargs.inc_ref().ptr());
+                }
+
+                // 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 +592,7 @@ class cpp_function : public function {
                 msg += "\n";
             }
             msg += "\nInvoked with: ";
-            auto args_ = reinterpret_borrow<tuple>(args);
+            auto args_ = reinterpret_borrow<tuple>(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 +610,8 @@ class cpp_function : public function {
             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();
         }
diff --git a/include/pybind11/pytypes.h b/include/pybind11/pytypes.h
@@ -85,8 +85,8 @@ class handle : public detail::object_api<handle> {
 
     PyObject *ptr() const { return m_ptr; }
     PyObject *&ptr() { return m_ptr; }
-    const handle& inc_ref() const { Py_XINCREF(m_ptr); return *this; }
-    const handle& dec_ref() const { Py_XDECREF(m_ptr); return *this; }
+    const handle& inc_ref() const & { Py_XINCREF(m_ptr); return *this; }
+    const handle& dec_ref() const & { Py_XDECREF(m_ptr); return *this; }
 
     template <typename T> T cast() const;
     explicit operator bool() const { return m_ptr != nullptr; }
diff --git a/tests/test_kwargs_and_defaults.cpp b/tests/test_kwargs_and_defaults.cpp
diff --git a/tests/test_kwargs_and_defaults.py b/tests/test_kwargs_and_defaults.py
