fix: review points from @YannickJadoul

Author: henryiii

include/pybind11/attr.h

@@ -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_pos_only_args(false) { }
+          has_args(false), has_kwargs(false), has_kwonly_args(false) { }
 
     /// Function name
     char *name = nullptr; /* why no C++ strings? They generate heavier code.. */
@@ -188,16 +188,13 @@ struct function_record {
     /// True once a 'py::kwonly' is encountered (any following args are keyword-only)
     bool has_kwonly_args : 1;
 
-    /// True once a 'py::pos_only' is encountered (any previous args are pos-only)
-    bool has_pos_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;
 
-    /// Number of trailing arguments (counted in `nargs`) that are positional-only
+    /// Number of leading arguments (counted in `nargs`) that are positional-only
     std::uint16_t nargs_pos_only = 0;
 
     /// Python method object
@@ -432,7 +429,6 @@ template <> struct process_attribute<kwonly> : process_attribute_default<kwonly>
 /// 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->has_pos_only_args = true;
         r->nargs_pos_only = static_cast<std::uint16_t>(r->args.size());
     }
 };

include/pybind11/pybind11.h

@@ -10,8 +10,6 @@
 
 #pragma once
 
-#include <iostream>
-
 #if defined(__INTEL_COMPILER)
 #  pragma warning push
 #  pragma warning disable 68    // integer conversion resulted in a change of sign
@@ -194,7 +192,7 @@ class cpp_function : public function {
                            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_arg_annotations || !has_pos_only_args, "py::pos_only requires the use of argument annotations (for doc string generation)");
+            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 arguments)");
             static_assert(!(has_args && has_kwonly_args), "py::kwonly cannot be combined with a py::args argument");
         }
 
@@ -261,7 +259,8 @@ class cpp_function : public function {
                 // Write arg name for everything except *args and **kwargs.
                 if (*(pc + 1) == '*')
                     continue;
-                // Seperator for keyword only arguments
+                // Separator for keyword-only arguments, placed before the kw
+                // arguments start
                 if (rec->nargs_kwonly > 0 && arg_index + rec->nargs_kwonly == args)
                     signature += "*, ";
                 if (arg_index < rec->args.size() && rec->args[arg_index].name) {
@@ -278,7 +277,8 @@ class cpp_function : public function {
                     signature += " = ";
                     signature += rec->args[arg_index].descr;
                 }
-                // Seperator for positional only arguments
+                // 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++;

tests/test_kwargs_and_defaults.cpp

@@ -107,18 +107,20 @@ TEST_SUBMODULE(kwargs_and_defaults, m) {
             return py::make_tuple(i, j, k, kwargs); },
             py::arg() /* positional */, py::arg("j") = -1 /* both */, py::kwonly(), 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);
+    });
+
     // 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::kwonly(), py::arg("k"));
-
-    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("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

tests/test_kwargs_and_defaults.py

@@ -175,13 +175,26 @@ def test_positional_only_args(msg):
         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)
+    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():
-    assert "kwonly_all(*, i: int, j: int) -> tuple" in m.kwonly_all.__doc__
-    assert "kwonly_mixed(i: int, *, j: int) -> tuple" in m.kwonly_mixed.__doc__
-    assert "pos_only_all(i: int, j: int, /) -> tuple" in m.pos_only_all.__doc__
-    assert "pos_only_mix(i: int, /, j: int) -> tuple" in m.pos_only_mix.__doc__
-    assert "pos_kw_only_mix(i: int, /, j: int, *, k: int) -> tuple" in m.pos_kw_only_mix.__doc__
+    assert "kwonly_all(*, i: int, j: int) -> tuple\n" == m.kwonly_all.__doc__
+    assert "kwonly_mixed(i: int, *, j: int) -> tuple\n" == m.kwonly_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")