From 673b4be3d7809198cd6a13a476fef270e430977d Mon Sep 17 00:00:00 2001 From: Jason Rhinelander Date: Sat, 20 Nov 2021 20:01:57 -0400 Subject: [PATCH] Fix py::kw_only when used before the first arg of a method (#3488) * Fix py::kw_only when used before the first arg of a method The implicit space for the `self` argument isn't added until we hit the first argument, but this wasn't being done for kw_only or pos_only, and so a kw_only before the first argument would break. This fixes it by properly checking whether we need to add the self arg. (The pos_only issue here was extremely mild -- you didn't get the `/` in the docstring, but AFAICT it has no other effect since there are no meaningful arguments before it anyway). * Style changes - rename check_have_self_arg -> append_self_arg_if_needed - move the argument name inline comments before the args instead of after --- include/pybind11/attr.h | 12 +++++++++--- tests/test_kwargs_and_defaults.cpp | 17 +++++++++++++++++ tests/test_kwargs_and_defaults.py | 21 +++++++++++++++++++++ 3 files changed, 47 insertions(+), 3 deletions(-) diff --git a/include/pybind11/attr.h b/include/pybind11/attr.h index b95c84904..f1b66fb80 100644 --- a/include/pybind11/attr.h +++ b/include/pybind11/attr.h @@ -414,11 +414,15 @@ inline void check_kw_only_arg(const arg &a, function_record *r) { pybind11_fail("arg(): cannot specify an unnamed argument after a kw_only() annotation or args() argument"); } +inline void append_self_arg_if_needed(function_record *r) { + if (r->is_method && r->args.empty()) + r->args.emplace_back("self", nullptr, handle(), /*convert=*/ true, /*none=*/ false); +} + /// Process a keyword argument attribute (*without* a default value) template <> struct process_attribute : process_attribute_default { static void init(const arg &a, function_record *r) { - if (r->is_method && r->args.empty()) - r->args.emplace_back("self", nullptr, handle(), true /*convert*/, false /*none not allowed*/); + append_self_arg_if_needed(r); r->args.emplace_back(a.name, nullptr, handle(), !a.flag_noconvert, a.flag_none); check_kw_only_arg(a, r); @@ -429,7 +433,7 @@ template <> struct process_attribute : process_attribute_default { template <> struct process_attribute : process_attribute_default { static void init(const arg_v &a, function_record *r) { if (r->is_method && r->args.empty()) - r->args.emplace_back("self", nullptr /*descr*/, handle() /*parent*/, true /*convert*/, false /*none not allowed*/); + r->args.emplace_back("self", /*descr=*/ nullptr, /*parent=*/ handle(), /*convert=*/ true, /*none=*/ false); if (!a.value) { #if !defined(NDEBUG) @@ -461,6 +465,7 @@ template <> struct process_attribute : process_attribute_default { /// Process a keyword-only-arguments-follow pseudo argument template <> struct process_attribute : process_attribute_default { static void init(const kw_only &, function_record *r) { + append_self_arg_if_needed(r); if (r->has_args && r->nargs_pos != static_cast(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(r->args.size()); @@ -470,6 +475,7 @@ template <> struct process_attribute : process_attribute_default struct process_attribute : process_attribute_default { static void init(const pos_only &, function_record *r) { + append_self_arg_if_needed(r); r->nargs_pos_only = static_cast(r->args.size()); if (r->nargs_pos_only > r->nargs_pos) pybind11_fail("pos_only(): cannot follow a py::args() argument"); diff --git a/tests/test_kwargs_and_defaults.cpp b/tests/test_kwargs_and_defaults.cpp index 312596dab..34ad2a864 100644 --- a/tests/test_kwargs_and_defaults.cpp +++ b/tests/test_kwargs_and_defaults.cpp @@ -167,4 +167,21 @@ TEST_SUBMODULE(kwargs_and_defaults, m) { "class_default_argument", [](py::object a) { return py::repr(std::move(a)); }, "a"_a = py::module_::import("decimal").attr("Decimal")); + + // Initial implementation of kw_only was broken when used on a method/constructor before any + // other arguments + // https://github.com/pybind/pybind11/pull/3402#issuecomment-963341987 + + struct first_arg_kw_only {}; + py::class_(m, "first_arg_kw_only") + .def(py::init([](int) { return first_arg_kw_only(); }), + py::kw_only(), // This being before any args was broken + py::arg("i") = 0) + .def("method", [](first_arg_kw_only&, int, int) {}, + py::kw_only(), // and likewise here + py::arg("i") = 1, py::arg("j") = 2) + // Closely related: pos_only marker didn't show up properly when it was before any other + // arguments (although that is fairly useless in practice). + .def("pos_only", [](first_arg_kw_only&, int, int) {}, + py::pos_only{}, py::arg("i"), py::arg("j")); } diff --git a/tests/test_kwargs_and_defaults.py b/tests/test_kwargs_and_defaults.py index 68903bde1..d61cf2aa5 100644 --- a/tests/test_kwargs_and_defaults.py +++ b/tests/test_kwargs_and_defaults.py @@ -229,6 +229,19 @@ def test_keyword_only_args(msg): """ ) + # https://github.com/pybind/pybind11/pull/3402#issuecomment-963341987 + x = m.first_arg_kw_only(i=1) + x.method() + x.method(i=1, j=2) + assert ( + m.first_arg_kw_only.__init__.__doc__ + == "__init__(self: pybind11_tests.kwargs_and_defaults.first_arg_kw_only, *, i: int = 0) -> None\n" # noqa: E501 line too long + ) + assert ( + m.first_arg_kw_only.method.__doc__ + == "method(self: pybind11_tests.kwargs_and_defaults.first_arg_kw_only, *, i: int = 1, j: int = 2) -> None\n" # noqa: E501 line too long + ) + def test_positional_only_args(msg): assert m.pos_only_all(1, 2) == (1, 2) @@ -310,6 +323,14 @@ def test_positional_only_args(msg): {"i": 5, "m": 8}, ) + # pos_only at the beginning of the argument list was "broken" in how it was displayed (though + # this is fairly useless in practice). Related to: + # https://github.com/pybind/pybind11/pull/3402#issuecomment-963341987 + assert ( + m.first_arg_kw_only.pos_only.__doc__ + == "pos_only(self: pybind11_tests.kwargs_and_defaults.first_arg_kw_only, /, i: int, j: int) -> None\n" # noqa: E501 line too long + ) + def test_signatures(): assert "kw_only_all(*, i: int, j: int) -> tuple\n" == m.kw_only_all.__doc__