Skip to content
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.

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

feat: py::pos_only #2459

Merged
merged 4 commits into from
Sep 5, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
26 changes: 22 additions & 4 deletions docs/advanced/functions.rst
Original file line number Diff line number Diff line change
Expand Up @@ -378,17 +378,35 @@ argument in a function definition:
f(1, b=2) # good
f(1, 2) # TypeError: f() takes 1 positional argument but 2 were given

Pybind11 provides a ``py::kwonly`` object that allows you to implement
Pybind11 provides a ``py::kw_only`` object that allows you to implement
YannickJadoul marked this conversation as resolved.
Show resolved Hide resolved
the same behaviour by specifying the object between positional and keyword-only
argument annotations when registering the function:

.. code-block:: cpp

m.def("f", [](int a, int b) { /* ... */ },
py::arg("a"), py::kwonly(), py::arg("b"));
py::arg("a"), py::kw_only(), py::arg("b"));

Note that, as in Python, you cannot combine this with a ``py::args`` argument.
This feature does *not* require Python 3 to work.
Note that you currently cannot combine this with a ``py::args`` argument. This
feature does *not* require Python 3 to work.

.. versionadded:: 2.6

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

Python 3.8 introduced a new positional-only argument syntax, using ``/`` in the
function definition (note that this has been a convention for CPython
positional arguments, such as in ``pow()``, since Python 2). You can
do the same thing in any version of Python using ``py::pos_only()``:

.. code-block:: cpp

m.def("f", [](int a, int b) { /* ... */ },
py::arg("a"), py::pos_only(), py::arg("b"));

You now cannot give argument ``a`` by keyword. This can be combined with
keyword-only arguments, as well.

.. versionadded:: 2.6

Expand Down
4 changes: 3 additions & 1 deletion docs/changelog.rst
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,11 @@ v2.6.0 (IN PROGRESS)

See :ref:`upgrade-guide-2.6` for help upgrading to the new version.

* Keyword only argument supported in Python 2 or 3 with ``py::kwonly()``.
* Keyword-only argument supported in Python 2 or 3 with ``py::kw_only()``.
`#2100 <https://github.com/pybind/pybind11/pull/2100>`_

* Positional-only argument supported in Python 2 or 3 with ``py::pos_only()``.

* Perfect forwarding support for methods.
`#2048 <https://github.com/pybind/pybind11/pull/2048>`_

Expand Down
34 changes: 22 additions & 12 deletions include/pybind11/attr.h
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,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_kwonly_args(false) { }
has_args(false), has_kwargs(false), has_kw_only_args(false) { }

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

/// True once a 'py::kwonly' is encountered (any following args are keyword-only)
bool has_kwonly_args : 1;
/// True once a 'py::kw_only' is encountered (any following args are keyword-only)
bool has_kw_only_args : 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_kwonly = 0;
std::uint16_t nargs_kw_only = 0;

/// Number of leading arguments (counted in `nargs`) that are positional-only
std::uint16_t nargs_pos_only = 0;

/// Python method object
PyMethodDef *def = nullptr;
Expand Down Expand Up @@ -366,10 +369,10 @@ 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_kwonly_arg(const arg &a, function_record *r) {
inline void process_kw_only_arg(const arg &a, function_record *r) {
if (!a.name || strlen(a.name) == 0)
pybind11_fail("arg(): cannot specify an unnamed argument after an kwonly() annotation");
++r->nargs_kwonly;
pybind11_fail("arg(): cannot specify an unnamed argument after an kw_only() annotation");
++r->nargs_kw_only;
}

/// Process a keyword argument attribute (*without* a default value)
Expand All @@ -379,7 +382,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_kwonly_args) process_kwonly_arg(a, r);
if (r->has_kw_only_args) process_kw_only_arg(a, r);
}
};

Expand Down Expand Up @@ -412,14 +415,21 @@ 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_kwonly_args) process_kwonly_arg(a, r);
if (r->has_kw_only_args) process_kw_only_arg(a, r);
}
};

/// Process a keyword-only-arguments-follow pseudo argument
template <> struct process_attribute<kwonly> : process_attribute_default<kwonly> {
static void init(const kwonly &, function_record *r) {
r->has_kwonly_args = true;
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;
}
};

/// 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());
}
};

Expand Down
7 changes: 6 additions & 1 deletion include/pybind11/cast.h
Original file line number Diff line number Diff line change
Expand Up @@ -1910,7 +1910,12 @@ struct arg_v : arg {
/// \ingroup annotations
/// Annotation indicating that all following arguments are keyword-only; the is the equivalent of an
/// unnamed '*' argument (in Python 3)
struct kwonly {};
struct kw_only {};

/// \ingroup annotations
/// Annotation indicating that all previous arguments are positional-only; the is the equivalent of an
/// unnamed '/' argument (in Python 3.8)
struct pos_only {};

template <typename T>
arg_v arg::operator=(T &&value) const { return {std::move(*this), std::forward<T>(value)}; }
Expand Down
40 changes: 35 additions & 5 deletions include/pybind11/pybind11.h
Original file line number Diff line number Diff line change
Expand Up @@ -187,11 +187,13 @@ class cpp_function : public function {
process_attributes<Extra...>::init(extra..., rec);

{
constexpr bool has_kwonly_args = any_of<std::is_same<kwonly, Extra>...>::value,
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_kwonly_args, "py::kwonly requires the use of argument annotations");
static_assert(!(has_args && has_kwonly_args), "py::kwonly cannot be combined with a py::args argument");
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");
}

/* Generate a readable signature describing the function's arguments and return value types */
Expand Down Expand Up @@ -257,7 +259,10 @@ class cpp_function : public function {
// Write arg name for everything except *args and **kwargs.
if (*(pc + 1) == '*')
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)
signature += "*, ";
henryiii marked this conversation as resolved.
Show resolved Hide resolved
if (arg_index < rec->args.size() && rec->args[arg_index].name) {
signature += rec->args[arg_index].name;
} else if (arg_index == 0 && rec->is_method) {
Expand All @@ -272,6 +277,10 @@ class cpp_function : public function {
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++;
} else if (c == '%') {
const std::type_info *t = types[type_index++];
Expand All @@ -297,6 +306,7 @@ class cpp_function : public function {
signature += c;
}
}

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

Expand Down Expand Up @@ -512,7 +522,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_kwonly;
size_t pos_args = num_args - func.nargs_kw_only;

if (!func.has_args && n_args_in > pos_args)
continue; // Too many positional arguments for this overload
Expand Down Expand Up @@ -561,6 +571,26 @@ class cpp_function : public function {
// We'll need to copy this if we steal some kwargs for defaults
dict kwargs = reinterpret_borrow<dict>(kwargs_in);

// 1.5. Fill in any missing pos_only args from defaults if they exist
if (args_copied < func.nargs_pos_only) {
for (; args_copied < func.nargs_pos_only; ++args_copied) {
const auto &arg = func.args[args_copied];
handle value;

if (arg.value) {
value = arg.value;
}
if (value) {
call.args.push_back(value);
call.args_convert.push_back(arg.convert);
} else
break;
YannickJadoul marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Collaborator

Choose a reason for hiding this comment

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

I haven't checked the rest of the codebase, so if this is consistent, leave it. I'm just a little bothered by else block not having braces while the if block does.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's done the same way just below, in the keyword checking.

}

if (args_copied < func.nargs_pos_only)
continue; // Not enough defaults to fill the positional arguments
}

// 2. Check kwargs and, failing that, defaults that may help complete the list
if (args_copied < num_args) {
bool copied_kwargs = false;
Expand Down
45 changes: 28 additions & 17 deletions tests/test_kwargs_and_defaults.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -95,28 +95,39 @@ TEST_SUBMODULE(kwargs_and_defaults, m) {
// m.def("bad_args7", [](py::kwargs, py::kwargs) {});

// test_keyword_only_args
m.def("kwonly_all", [](int i, int j) { return py::make_tuple(i, j); },
py::kwonly(), py::arg("i"), py::arg("j"));
m.def("kwonly_some", [](int i, int j, int k) { return py::make_tuple(i, j, k); },
py::arg(), py::kwonly(), py::arg("j"), py::arg("k"));
m.def("kwonly_with_defaults", [](int i, int j, int k, int z) { return py::make_tuple(i, j, k, z); },
py::arg() = 3, "j"_a = 4, py::kwonly(), "k"_a = 5, "z"_a);
m.def("kwonly_mixed", [](int i, int j) { return py::make_tuple(i, j); },
"i"_a, py::kwonly(), "j"_a);
m.def("kwonly_plus_more", [](int i, int j, int k, py::kwargs kwargs) {
m.def("kw_only_all", [](int i, int j) { return py::make_tuple(i, j); },
py::kw_only(), py::arg("i"), py::arg("j"));
m.def("kw_only_some", [](int i, int j, int k) { return py::make_tuple(i, j, k); },
py::arg(), py::kw_only(), py::arg("j"), py::arg("k"));
m.def("kw_only_with_defaults", [](int i, int j, int k, int z) { return py::make_tuple(i, j, k, z); },
py::arg() = 3, "j"_a = 4, py::kw_only(), "k"_a = 5, "z"_a);
m.def("kw_only_mixed", [](int i, int j) { return py::make_tuple(i, j); },
"i"_a, py::kw_only(), "j"_a);
m.def("kw_only_plus_more", [](int i, int j, int k, py::kwargs kwargs) {
return py::make_tuple(i, j, k, kwargs); },
py::arg() /* positional */, py::arg("j") = -1 /* both */, py::kwonly(), py::arg("k") /* kw-only */);
py::arg() /* positional */, py::arg("j") = -1 /* both */, py::kw_only(), py::arg("k") /* kw-only */);

m.def("register_invalid_kwonly", [](py::module m) {
m.def("bad_kwonly", [](int i, int j) { return py::make_tuple(i, j); },
py::kwonly(), py::arg() /* invalid unnamed argument */, "j"_a);
m.def("register_invalid_kw_only", [](py::module m) {
m.def("bad_kw_only", [](int i, int j) { return py::make_tuple(i, j); },
py::kw_only(), py::arg() /* invalid unnamed argument */, "j"_a);
});

// test_positional_only_args
m.def("pos_only_all", [](int i, int j) { return py::make_tuple(i, j); },
py::arg("i"), py::arg("j"), py::pos_only());
m.def("pos_only_mix", [](int i, int j) { return py::make_tuple(i, j); },
py::arg("i"), py::pos_only(), py::arg("j"));
m.def("pos_kw_only_mix", [](int i, int j, int k) { return py::make_tuple(i, j, k); },
py::arg("i"), py::pos_only(), py::arg("j"), py::kw_only(), py::arg("k"));
m.def("pos_only_def_mix", [](int i, int j, int k) { return py::make_tuple(i, j, k); },
py::arg("i"), py::arg("j") = 2, py::pos_only(), py::arg("k") = 3);


// These should fail to compile:
// argument annotations are required when using kwonly
// m.def("bad_kwonly1", [](int) {}, py::kwonly());
// can't specify both `py::kwonly` and a `py::args` argument
// m.def("bad_kwonly2", [](int i, py::args) {}, py::kwonly(), "i"_a);
// argument annotations are required when using kw_only
// m.def("bad_kw_only1", [](int) {}, py::kw_only());
// can't specify both `py::kw_only` and a `py::args` argument
// m.def("bad_kw_only2", [](int i, py::args) {}, py::kw_only(), "i"_a);

// test_function_signatures (along with most of the above)
struct KWClass { void foo(int, float) {} };
Expand Down
84 changes: 66 additions & 18 deletions tests/test_kwargs_and_defaults.py
Original file line number Diff line number Diff line change
Expand Up @@ -112,43 +112,91 @@ def test_mixed_args_and_kwargs(msg):


def test_keyword_only_args(msg):
assert m.kwonly_all(i=1, j=2) == (1, 2)
assert m.kwonly_all(j=1, i=2) == (2, 1)
assert m.kw_only_all(i=1, j=2) == (1, 2)
assert m.kw_only_all(j=1, i=2) == (2, 1)

with pytest.raises(TypeError) as excinfo:
assert m.kwonly_all(i=1) == (1,)
assert m.kw_only_all(i=1) == (1,)
assert "incompatible function arguments" in str(excinfo.value)

with pytest.raises(TypeError) as excinfo:
assert m.kwonly_all(1, 2) == (1, 2)
assert m.kw_only_all(1, 2) == (1, 2)
assert "incompatible function arguments" in str(excinfo.value)

assert m.kwonly_some(1, k=3, j=2) == (1, 2, 3)
assert m.kw_only_some(1, k=3, j=2) == (1, 2, 3)

assert m.kwonly_with_defaults(z=8) == (3, 4, 5, 8)
assert m.kwonly_with_defaults(2, z=8) == (2, 4, 5, 8)
assert m.kwonly_with_defaults(2, j=7, k=8, z=9) == (2, 7, 8, 9)
assert m.kwonly_with_defaults(2, 7, z=9, k=8) == (2, 7, 8, 9)
assert m.kw_only_with_defaults(z=8) == (3, 4, 5, 8)
assert m.kw_only_with_defaults(2, z=8) == (2, 4, 5, 8)
assert m.kw_only_with_defaults(2, j=7, k=8, z=9) == (2, 7, 8, 9)
assert m.kw_only_with_defaults(2, 7, z=9, k=8) == (2, 7, 8, 9)

assert m.kwonly_mixed(1, j=2) == (1, 2)
assert m.kwonly_mixed(j=2, i=3) == (3, 2)
assert m.kwonly_mixed(i=2, j=3) == (2, 3)
assert m.kw_only_mixed(1, j=2) == (1, 2)
assert m.kw_only_mixed(j=2, i=3) == (3, 2)
assert m.kw_only_mixed(i=2, j=3) == (2, 3)

assert m.kwonly_plus_more(4, 5, k=6, extra=7) == (4, 5, 6, {'extra': 7})
assert m.kwonly_plus_more(3, k=5, j=4, extra=6) == (3, 4, 5, {'extra': 6})
assert m.kwonly_plus_more(2, k=3, extra=4) == (2, -1, 3, {'extra': 4})
assert m.kw_only_plus_more(4, 5, k=6, extra=7) == (4, 5, 6, {'extra': 7})
assert m.kw_only_plus_more(3, k=5, j=4, extra=6) == (3, 4, 5, {'extra': 6})
assert m.kw_only_plus_more(2, k=3, extra=4) == (2, -1, 3, {'extra': 4})

with pytest.raises(TypeError) as excinfo:
assert m.kwonly_mixed(i=1) == (1,)
assert m.kw_only_mixed(i=1) == (1,)
assert "incompatible function arguments" in str(excinfo.value)

with pytest.raises(RuntimeError) as excinfo:
m.register_invalid_kwonly(m)
m.register_invalid_kw_only(m)
assert msg(excinfo.value) == """
arg(): cannot specify an unnamed argument after an kwonly() annotation
arg(): cannot specify an unnamed argument after an kw_only() annotation
"""


def test_positional_only_args(msg):
assert m.pos_only_all(1, 2) == (1, 2)
assert m.pos_only_all(2, 1) == (2, 1)

with pytest.raises(TypeError) as excinfo:
m.pos_only_all(i=1, j=2)
assert "incompatible function arguments" in str(excinfo.value)

assert m.pos_only_mix(1, 2) == (1, 2)
assert m.pos_only_mix(2, j=1) == (2, 1)

with pytest.raises(TypeError) as excinfo:
m.pos_only_mix(i=1, j=2)
assert "incompatible function arguments" in str(excinfo.value)

assert m.pos_kw_only_mix(1, 2, k=3) == (1, 2, 3)
assert m.pos_kw_only_mix(1, j=2, k=3) == (1, 2, 3)

with pytest.raises(TypeError) as excinfo:
m.pos_kw_only_mix(i=1, j=2, k=3)
assert "incompatible function arguments" in str(excinfo.value)

with pytest.raises(TypeError) as excinfo:
m.pos_kw_only_mix(1, 2, 3)
assert "incompatible function arguments" in str(excinfo.value)

with pytest.raises(TypeError) as excinfo:
m.pos_only_def_mix()
assert "incompatible function arguments" in str(excinfo.value)

assert m.pos_only_def_mix(1) == (1, 2, 3)
assert m.pos_only_def_mix(1, 4) == (1, 4, 3)
assert m.pos_only_def_mix(1, 4, 7) == (1, 4, 7)
henryiii marked this conversation as resolved.
Show resolved Hide resolved
assert m.pos_only_def_mix(1, 4, k=7) == (1, 4, 7)

with pytest.raises(TypeError) as excinfo:
m.pos_only_def_mix(1, j=4)
assert "incompatible function arguments" in str(excinfo.value)


def test_signatures():
henryiii marked this conversation as resolved.
Show resolved Hide resolved
assert "kw_only_all(*, i: int, j: int) -> tuple\n" == m.kw_only_all.__doc__
assert "kw_only_mixed(i: int, *, j: int) -> tuple\n" == m.kw_only_mixed.__doc__
assert "pos_only_all(i: int, j: int, /) -> tuple\n" == m.pos_only_all.__doc__
assert "pos_only_mix(i: int, /, j: int) -> tuple\n" == m.pos_only_mix.__doc__
assert "pos_kw_only_mix(i: int, /, j: int, *, k: int) -> tuple\n" == m.pos_kw_only_mix.__doc__


@pytest.mark.xfail("env.PYPY and env.PY2", reason="PyPy2 doesn't double count")
def test_args_refcount():
"""Issue/PR #1216 - py::args elements get double-inc_ref()ed when combined with regular
Expand Down