Skip to content

Allow kw-only args after a py::args #3402

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Oct 29, 2021
Merged

Allow kw-only args after a py::args #3402

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
44f1011
Simply has_kw_only_args handling
jagerman Oct 23, 2021
bf1c991
Allow keyword-only arguments to follow py::args
jagerman Oct 24, 2021
eeb2239
Take args/kwargs by const lvalue ref
jagerman Oct 27, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 14 additions & 2 deletions docs/advanced/functions.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -306,8 +306,9 @@ The class ``py::args`` derives from ``py::tuple`` and ``py::kwargs`` derives
from ``py::dict``.

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.
arguments. Note, however, that ``py::kwargs`` must always be the last argument
of the function, and ``py::args`` implies that any further arguments are
keyword-only (see :ref:`keyword_only_arguments`).

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
Expand Down Expand Up @@ -366,6 +367,8 @@ like so:
py::class_<MyClass>("MyClass")
.def("myFunction", py::arg("arg") = static_cast<SomeType *>(nullptr));

.. _keyword_only_arguments:

Keyword-only arguments
======================

Expand Down Expand Up @@ -397,6 +400,15 @@ feature does *not* require Python 3 to work.

.. versionadded:: 2.6

As of pybind11 2.9, a ``py::args`` argument implies that any following arguments
are keyword-only, as if ``py::kw_only()`` had been specified in the same
relative location of the argument list as the ``py::args`` argument. The
``py::kw_only()`` may be included to be explicit about this, but is not
required. (Prior to 2.9 ``py::args`` may only occur at the end of the argument
list, or immediately before a ``py::kwargs`` argument at the end).

.. versionadded:: 2.9

Positional-only arguments
=========================

Expand Down
28 changes: 15 additions & 13 deletions include/pybind11/attr.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,7 +174,7 @@ 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), has_kw_only_args(false), prepend(false) { }
has_kwargs(false), prepend(false) { }

/// Function name
char *name = nullptr; /* why no C++ strings? They generate heavier code.. */
Expand Down Expand Up @@ -221,17 +221,15 @@ struct function_record {
/// True if the function has a '**kwargs' argument
bool has_kwargs : 1;

/// True once a 'py::kw_only' is encountered (any following args are keyword-only)
bool has_kw_only_args : 1;

/// True if this function is to be inserted at the beginning of the overload resolution chain
bool prepend : 1;

/// Number of arguments (including py::args and/or py::kwargs, if present)
std::uint16_t nargs;

/// Number of trailing arguments (counted in `nargs`) that are keyword-only
std::uint16_t nargs_kw_only = 0;
/// Number of leading positional arguments, which are terminated by a py::args or py::kwargs
/// argument or by a py::kw_only annotation.
std::uint16_t nargs_pos = 0;

/// Number of leading arguments (counted in `nargs`) that are positional-only
std::uint16_t nargs_pos_only = 0;
Expand Down Expand Up @@ -411,10 +409,9 @@ template <> struct process_attribute<is_new_style_constructor> : process_attribu
static void init(const is_new_style_constructor &, function_record *r) { r->is_new_style_constructor = true; }
};

inline void process_kw_only_arg(const arg &a, function_record *r) {
if (!a.name || a.name[0] == '\0')
pybind11_fail("arg(): cannot specify an unnamed argument after an kw_only() annotation");
++r->nargs_kw_only;
inline void check_kw_only_arg(const arg &a, function_record *r) {
if (r->args.size() > r->nargs_pos && (!a.name || a.name[0] == '\0'))
pybind11_fail("arg(): cannot specify an unnamed argument after a kw_only() annotation or args() argument");
}

/// Process a keyword argument attribute (*without* a default value)
Expand All @@ -424,7 +421,7 @@ template <> struct process_attribute<arg> : process_attribute_default<arg> {
r->args.emplace_back("self", nullptr, handle(), true /*convert*/, false /*none not allowed*/);
r->args.emplace_back(a.name, nullptr, handle(), !a.flag_noconvert, a.flag_none);

if (r->has_kw_only_args) process_kw_only_arg(a, r);
check_kw_only_arg(a, r);
}
};

Expand Down Expand Up @@ -457,21 +454,26 @@ template <> struct process_attribute<arg_v> : process_attribute_default<arg_v> {
}
r->args.emplace_back(a.name, a.descr, a.value.inc_ref(), !a.flag_noconvert, a.flag_none);

if (r->has_kw_only_args) process_kw_only_arg(a, r);
check_kw_only_arg(a, r);
}
};

/// Process a keyword-only-arguments-follow pseudo argument
template <> struct process_attribute<kw_only> : process_attribute_default<kw_only> {
static void init(const kw_only &, function_record *r) {
r->has_kw_only_args = true;
if (r->has_args && r->nargs_pos != static_cast<std::uint16_t>(r->args.size()))
pybind11_fail("Mismatched args() and kw_only(): they must occur at the same relative argument location (or omit kw_only() entirely)");
r->nargs_pos = static_cast<std::uint16_t>(r->args.size());
}
};

/// Process a positional-only-argument maker
template <> struct process_attribute<pos_only> : process_attribute_default<pos_only> {
static void init(const pos_only &, function_record *r) {
r->nargs_pos_only = static_cast<std::uint16_t>(r->args.size());
if (r->nargs_pos_only > r->nargs_pos)
pybind11_fail("pos_only(): cannot follow a py::args() argument");
// It also can't follow a kw_only, but a static_assert in pybind11.h checks that
}
};

Expand Down
20 changes: 12 additions & 8 deletions include/pybind11/cast.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1130,6 +1130,9 @@ constexpr arg operator"" _a(const char *name, size_t) { return arg(name); }

PYBIND11_NAMESPACE_BEGIN(detail)

template <typename T> using is_kw_only = std::is_same<intrinsic_t<T>, kw_only>;
template <typename T> using is_pos_only = std::is_same<intrinsic_t<T>, pos_only>;

// forward declaration (definition in attr.h)
struct function_record;

Expand Down Expand Up @@ -1165,17 +1168,18 @@ class argument_loader {

template <typename Arg> using argument_is_args = std::is_same<intrinsic_t<Arg>, args>;
template <typename Arg> using argument_is_kwargs = std::is_same<intrinsic_t<Arg>, kwargs>;
// Get args/kwargs argument positions relative to the end of the argument list:
static constexpr auto args_pos = constexpr_first<argument_is_args, Args...>() - (int) sizeof...(Args),
kwargs_pos = constexpr_first<argument_is_kwargs, Args...>() - (int) sizeof...(Args);

static constexpr bool args_kwargs_are_last = kwargs_pos >= - 1 && args_pos >= kwargs_pos - 1;
// Get kwargs argument position, or -1 if not present:
static constexpr auto kwargs_pos = constexpr_last<argument_is_kwargs, Args...>();

static_assert(args_kwargs_are_last, "py::args/py::kwargs are only permitted as the last argument(s) of a function");
static_assert(kwargs_pos == -1 || kwargs_pos == (int) sizeof...(Args) - 1, "py::kwargs is only permitted as the last argument of a function");

public:
static constexpr bool has_kwargs = kwargs_pos < 0;
static constexpr bool has_args = args_pos < 0;
static constexpr bool has_kwargs = kwargs_pos != -1;

// py::args argument position; -1 if not present.
static constexpr int args_pos = constexpr_last<argument_is_args, Args...>();

static_assert(args_pos == -1 || args_pos == constexpr_first<argument_is_args, Args...>(), "py::args cannot be specified more than once");

static constexpr auto arg_names = concat(type_descr(make_caster<Args>::name)...);

Expand Down
57 changes: 39 additions & 18 deletions include/pybind11/pybind11.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -203,7 +203,7 @@ class cpp_function : public function {
conditional_t<std::is_void<Return>::value, void_type, Return>
>;

static_assert(expected_num_args<Extra...>(sizeof...(Args), cast_in::has_args, cast_in::has_kwargs),
static_assert(expected_num_args<Extra...>(sizeof...(Args), cast_in::args_pos >= 0, cast_in::has_kwargs),
"The number of argument annotations does not match the number of function arguments");

/* Dispatch code which converts function arguments and performs the actual function call */
Expand Down Expand Up @@ -238,17 +238,27 @@ class cpp_function : public function {
return result;
};

rec->nargs_pos = cast_in::args_pos >= 0
? static_cast<std::uint16_t>(cast_in::args_pos)
: sizeof...(Args) - cast_in::has_kwargs; // Will get reduced more if we have a kw_only
rec->has_args = cast_in::args_pos >= 0;
rec->has_kwargs = cast_in::has_kwargs;

/* Process any user-provided function attributes */
process_attributes<Extra...>::init(extra..., rec);

{
constexpr bool has_kw_only_args = any_of<std::is_same<kw_only, Extra>...>::value,
has_pos_only_args = any_of<std::is_same<pos_only, Extra>...>::value,
has_args = any_of<std::is_same<args, Args>...>::value,
has_arg_annotations = any_of<is_keyword<Extra>...>::value;
static_assert(has_arg_annotations || !has_kw_only_args, "py::kw_only requires the use of argument annotations");
static_assert(has_arg_annotations || !has_pos_only_args, "py::pos_only requires the use of argument annotations (for docstrings and aligning the annotations to the argument)");
static_assert(!(has_args && has_kw_only_args), "py::kw_only cannot be combined with a py::args argument");

static_assert(constexpr_sum(is_kw_only<Extra>::value...) <= 1, "py::kw_only may be specified only once");
static_assert(constexpr_sum(is_pos_only<Extra>::value...) <= 1, "py::pos_only may be specified only once");
constexpr auto kw_only_pos = constexpr_first<is_kw_only, Extra...>();
constexpr auto pos_only_pos = constexpr_first<is_pos_only, Extra...>();
static_assert(!(has_kw_only_args && has_pos_only_args) || pos_only_pos < kw_only_pos, "py::pos_only must come before py::kw_only");
}

/* Generate a readable signature describing the function's arguments and return value types */
Expand All @@ -259,9 +269,6 @@ class cpp_function : public function {
// Pass on the ownership over the `unique_rec` to `initialize_generic`. `rec` stays valid.
initialize_generic(std::move(unique_rec), signature.text, types.data(), sizeof...(Args));

if (cast_in::has_args) rec->has_args = true;
if (cast_in::has_kwargs) rec->has_kwargs = true;

/* Stash some additional information used by an important optimization in 'functional.h' */
using FunctionType = Return (*)(Args...);
constexpr bool is_function_ptr =
Expand Down Expand Up @@ -340,16 +347,18 @@ class cpp_function : public function {
/* Generate a proper function signature */
std::string signature;
size_t type_index = 0, arg_index = 0;
bool is_starred = false;
for (auto *pc = text; *pc != '\0'; ++pc) {
const auto c = *pc;

if (c == '{') {
// Write arg name for everything except *args and **kwargs.
if (*(pc + 1) == '*')
is_starred = *(pc + 1) == '*';
if (is_starred)
continue;
// Separator for keyword-only arguments, placed before the kw
// arguments start
if (rec->nargs_kw_only > 0 && arg_index + rec->nargs_kw_only == args)
// arguments start (unless we are already putting an *args)
if (!rec->has_args && arg_index == rec->nargs_pos)
signature += "*, ";
if (arg_index < rec->args.size() && rec->args[arg_index].name) {
signature += rec->args[arg_index].name;
Expand All @@ -361,15 +370,16 @@ class cpp_function : public function {
signature += ": ";
} else if (c == '}') {
// Write default value if available.
if (arg_index < rec->args.size() && rec->args[arg_index].descr) {
if (!is_starred && arg_index < rec->args.size() && rec->args[arg_index].descr) {
signature += " = ";
signature += rec->args[arg_index].descr;
}
// Separator for positional-only arguments (placed after the
// argument, rather than before like *
if (rec->nargs_pos_only > 0 && (arg_index + 1) == rec->nargs_pos_only)
signature += ", /";
arg_index++;
if (!is_starred)
arg_index++;
} else if (c == '%') {
const std::type_info *t = types[type_index++];
if (!t)
Expand All @@ -395,7 +405,7 @@ class cpp_function : public function {
}
}

if (arg_index != args || types[type_index] != nullptr)
if (arg_index != args - rec->has_args - rec->has_kwargs || types[type_index] != nullptr)
pybind11_fail("Internal error while parsing type signature (2)");

#if PY_MAJOR_VERSION < 3
Expand Down Expand Up @@ -631,7 +641,7 @@ class cpp_function : public function {
named positional arguments weren't *also* specified via kwarg.
2. If we weren't given enough, try to make up the omitted 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
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.
Expand All @@ -649,7 +659,7 @@ class cpp_function : public function {
size_t num_args = func.nargs; // Number of positional arguments that we need
if (func.has_args) --num_args; // (but don't count py::args
if (func.has_kwargs) --num_args; // or py::kwargs)
size_t pos_args = num_args - func.nargs_kw_only;
size_t pos_args = func.nargs_pos;

if (!func.has_args && n_args_in > pos_args)
continue; // Too many positional arguments for this overload
Expand Down Expand Up @@ -695,6 +705,10 @@ class cpp_function : public function {
if (bad_arg)
continue; // Maybe it was meant for another overload (issue #688)

// Keep track of how many position args we copied out in case we need to come back
// to copy the rest into a py::args argument.
size_t positional_args_copied = args_copied;

// We'll need to copy this if we steal some kwargs for defaults
dict kwargs = reinterpret_borrow<dict>(kwargs_in);

Expand Down Expand Up @@ -747,6 +761,10 @@ class cpp_function : public function {
}

if (value) {
// If we're at the py::args index then first insert a stub for it to be replaced later
if (func.has_args && call.args.size() == func.nargs_pos)
call.args.push_back(none());

call.args.push_back(value);
call.args_convert.push_back(arg_rec.convert);
}
Expand All @@ -769,16 +787,19 @@ class cpp_function : public function {
// 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<tuple>(args_in);
} else if (args_copied >= n_args_in) {
} else if (positional_args_copied >= n_args_in) {
extra_args = tuple(0);
} else {
size_t args_size = n_args_in - args_copied;
size_t args_size = n_args_in - positional_args_copied;
extra_args = tuple(args_size);
for (size_t i = 0; i < args_size; ++i) {
extra_args[i] = PyTuple_GET_ITEM(args_in, args_copied + i);
extra_args[i] = PyTuple_GET_ITEM(args_in, positional_args_copied + i);
}
}
call.args.push_back(extra_args);
if (call.args.size() <= func.nargs_pos)
call.args.push_back(extra_args);
else
call.args[func.nargs_pos] = extra_args;
call.args_convert.push_back(false);
call.args_ref = std::move(extra_args);
}
Expand Down
17 changes: 17 additions & 0 deletions tests/test_kwargs_and_defaults.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,23 @@ TEST_SUBMODULE(kwargs_and_defaults, m) {
m.def("mixed_plus_args_kwargs_defaults", mixed_plus_both,
py::arg("i") = 1, py::arg("j") = 3.14159);

m.def("args_kwonly",
Copy link
Collaborator

@Skylion007 Skylion007 Oct 24, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These tests should all pass py::args and py::kwargs by ref through the lambda since they are not trivially copyable. (This is the clang-tidy rule that fails).

[](int i, double j, const py::args &args, int z) { return py::make_tuple(i, j, args, z); },
"i"_a, "j"_a, "z"_a);
m.def("args_kwonly_kwargs",
[](int i, double j, const py::args &args, int z, const py::kwargs &kwargs) {
return py::make_tuple(i, j, args, z, kwargs); },
"i"_a, "j"_a, py::kw_only{}, "z"_a);
m.def("args_kwonly_kwargs_defaults",
[](int i, double j, const py::args &args, int z, const py::kwargs &kwargs) {
return py::make_tuple(i, j, args, z, kwargs); },
"i"_a = 1, "j"_a = 3.14159, "z"_a = 42);
m.def("args_kwonly_full_monty",
[](int h, int i, double j, const py::args &args, int z, const py::kwargs &kwargs) {
return py::make_tuple(h, i, j, args, z, kwargs); },
py::arg() = 1, py::arg() = 2, py::pos_only{}, "j"_a = 3.14159, "z"_a = 42);


// test_args_refcount
// PyPy needs a garbage collection to get the reference count values to match CPython's behaviour
#ifdef PYPY_VERSION
Expand Down
Loading