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

Author: jagerman

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<arg> : process_attribute_default<arg> {
     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<arg> : process_attribute_default<arg> {
 template <> struct process_attribute<arg_v> : process_attribute_default<arg_v> {
     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<arg_v> : process_attribute_default<arg_v> {
 /// 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) {
+        append_self_arg_if_needed(r);
         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());
@@ -470,6 +475,7 @@ template <> struct process_attribute<kw_only> : process_attribute_default<kw_onl
 /// 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) {
+        append_self_arg_if_needed(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");

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_<first_arg_kw_only>(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"));
 }

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__