From c57e07e3969ce46e10f6165edd7215fb4526c69f Mon Sep 17 00:00:00 2001 From: Xuehai Pan Date: Wed, 9 Oct 2024 15:46:43 +0800 Subject: [PATCH 1/8] feat: allow annotate methods with `pos_only` when only have the `self` argument --- include/pybind11/pybind11.h | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index 2527d25faf..fc68b5b091 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -300,10 +300,12 @@ class cpp_function : public function { { constexpr bool has_kw_only_args = any_of...>::value, has_pos_only_args = any_of...>::value, - has_arg_annotations = any_of...>::value; + has_arg_annotations = any_of...>::value, + has_is_method = any_of...>::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, + static_assert(has_arg_annotations || !has_pos_only_args + || (has_is_method /* method has at least one argument `self` */), "py::pos_only requires the use of argument annotations (for docstrings " "and aligning the annotations to the argument)"); From fa887bd8a10ab92c87def7530d0a892ce8faad46 Mon Sep 17 00:00:00 2001 From: Xuehai Pan Date: Wed, 9 Oct 2024 16:24:42 +0800 Subject: [PATCH 2/8] chore(typing): make arguments for auto-generated dunder methods positional-only --- include/pybind11/detail/init.h | 2 +- include/pybind11/pybind11.h | 45 ++++++++++++++++++++++------------ 2 files changed, 30 insertions(+), 17 deletions(-) diff --git a/include/pybind11/detail/init.h b/include/pybind11/detail/init.h index 79cc930c8d..3eeeaaf970 100644 --- a/include/pybind11/detail/init.h +++ b/include/pybind11/detail/init.h @@ -410,7 +410,7 @@ struct pickle_factory { template void execute(Class &cl, const Extra &...extra) && { - cl.def("__getstate__", std::move(get)); + cl.def("__getstate__", std::move(get), pos_only()); #if defined(PYBIND11_CPP14) cl.def( diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index fc68b5b091..9b237a9984 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -2024,9 +2024,11 @@ struct enum_base { .format(std::move(type_name), enum_name(arg), int_(arg)); }, name("__repr__"), - is_method(m_base)); + is_method(m_base), + pos_only()); - m_base.attr("name") = property(cpp_function(&enum_name, name("name"), is_method(m_base))); + m_base.attr("name") + = property(cpp_function(&enum_name, name("name"), is_method(m_base), pos_only())); m_base.attr("__str__") = cpp_function( [](handle arg) -> str { @@ -2034,7 +2036,8 @@ struct enum_base { return pybind11::str("{}.{}").format(std::move(type_name), enum_name(arg)); }, name("__str__"), - is_method(m_base)); + is_method(m_base), + pos_only()); if (options::show_enum_members_docstring()) { m_base.attr("__doc__") = static_property( @@ -2089,7 +2092,8 @@ struct enum_base { }, \ name(op), \ is_method(m_base), \ - arg("other")) + arg("other"), \ + pos_only()) #define PYBIND11_ENUM_OP_CONV(op, expr) \ m_base.attr(op) = cpp_function( \ @@ -2099,7 +2103,8 @@ struct enum_base { }, \ name(op), \ is_method(m_base), \ - arg("other")) + arg("other"), \ + pos_only()) #define PYBIND11_ENUM_OP_CONV_LHS(op, expr) \ m_base.attr(op) = cpp_function( \ @@ -2109,7 +2114,8 @@ struct enum_base { }, \ name(op), \ is_method(m_base), \ - arg("other")) + arg("other"), \ + pos_only()) if (is_convertible) { PYBIND11_ENUM_OP_CONV_LHS("__eq__", !b.is_none() && a.equal(b)); @@ -2129,7 +2135,8 @@ struct enum_base { m_base.attr("__invert__") = cpp_function([](const object &arg) { return ~(int_(arg)); }, name("__invert__"), - is_method(m_base)); + is_method(m_base), + pos_only()); } } else { PYBIND11_ENUM_OP_STRICT("__eq__", int_(a).equal(int_(b)), return false); @@ -2149,11 +2156,15 @@ struct enum_base { #undef PYBIND11_ENUM_OP_CONV #undef PYBIND11_ENUM_OP_STRICT - m_base.attr("__getstate__") = cpp_function( - [](const object &arg) { return int_(arg); }, name("__getstate__"), is_method(m_base)); + m_base.attr("__getstate__") = cpp_function([](const object &arg) { return int_(arg); }, + name("__getstate__"), + is_method(m_base), + pos_only()); - m_base.attr("__hash__") = cpp_function( - [](const object &arg) { return int_(arg); }, name("__hash__"), is_method(m_base)); + m_base.attr("__hash__") = cpp_function([](const object &arg) { return int_(arg); }, + name("__hash__"), + is_method(m_base), + pos_only()); } PYBIND11_NOINLINE void value(char const *name_, object value, const char *doc = nullptr) { @@ -2245,9 +2256,9 @@ class enum_ : public class_ { m_base.init(is_arithmetic, is_convertible); def(init([](Scalar i) { return static_cast(i); }), arg("value")); - def_property_readonly("value", [](Type value) { return (Scalar) value; }); - def("__int__", [](Type value) { return (Scalar) value; }); - def("__index__", [](Type value) { return (Scalar) value; }); + def_property_readonly("value", [](Type value) { return (Scalar) value; }, pos_only()); + def("__int__", [](Type value) { return (Scalar) value; }, pos_only()); + def("__index__", [](Type value) { return (Scalar) value; }, pos_only()); attr("__setstate__") = cpp_function( [](detail::value_and_holder &v_h, Scalar arg) { detail::initimpl::setstate( @@ -2256,7 +2267,8 @@ class enum_ : public class_ { detail::is_new_style_constructor(), pybind11::name("__setstate__"), is_method(*this), - arg("state")); + arg("state"), + pos_only()); } /// Export enumeration entries into the parent scope @@ -2435,7 +2447,8 @@ iterator make_iterator_impl(Iterator first, Sentinel last, Extra &&...extra) { if (!detail::get_type_info(typeid(state), false)) { class_(handle(), "iterator", pybind11::module_local()) - .def("__iter__", [](state &s) -> state & { return s; }) + .def( + "__iter__", [](state &s) -> state & { return s; }, pos_only()) .def( "__next__", [](state &s) -> ValueType { From 09a4992f18c79ace4c636e9026807b6ab74e07bf Mon Sep 17 00:00:00 2001 From: Xuehai Pan Date: Sat, 9 Nov 2024 17:29:02 +0800 Subject: [PATCH 3/8] docs: add more comments to improve readability --- include/pybind11/pybind11.h | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index 3068e4c360..8eb23c6b92 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -300,12 +300,21 @@ class cpp_function : public function { { constexpr bool has_kw_only_args = any_of...>::value, has_pos_only_args = any_of...>::value, - has_arg_annotations = any_of...>::value, - has_is_method = any_of...>::value; + has_arg_annotations = any_of...>::value; + constexpr bool has_is_method = any_of...>::value; + // The implicit `self` argument is not present and counted in method definitions. + constexpr bool has_args = cast_in::args_pos >= 0; + constexpr bool is_method_with_self_arg_only = has_is_method && !has_args; 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 - || (has_is_method /* method has at least one argument `self` */), + static_assert(((/* Need `py::arg("arg_name")` annotation in function/method. */ + has_arg_annotations) + || (/* Allow methods with no arguments `def method(self, /): ...`. + * A method has at least one argument `self`. There can be no + * `py::arg` annotation. E.g. `class.def("method", py::pos_only())`. + * */ + is_method_with_self_arg_only)) + || !has_pos_only_args, "py::pos_only requires the use of argument annotations (for docstrings " "and aligning the annotations to the argument)"); From 5366b051d7e4a750d2035c03b12f0ff1f06e61b3 Mon Sep 17 00:00:00 2001 From: Xuehai Pan Date: Sun, 10 Nov 2024 14:01:42 +0800 Subject: [PATCH 4/8] style: fix nit suggestions --- include/pybind11/pybind11.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index 8eb23c6b92..f6ce716529 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -302,7 +302,7 @@ class cpp_function : public function { has_pos_only_args = any_of...>::value, has_arg_annotations = any_of...>::value; constexpr bool has_is_method = any_of...>::value; - // The implicit `self` argument is not present and counted in method definitions. + // The implicit `self` argument is not present and not counted in method definitions. constexpr bool has_args = cast_in::args_pos >= 0; constexpr bool is_method_with_self_arg_only = has_is_method && !has_args; static_assert(has_arg_annotations || !has_kw_only_args, @@ -312,7 +312,7 @@ class cpp_function : public function { || (/* Allow methods with no arguments `def method(self, /): ...`. * A method has at least one argument `self`. There can be no * `py::arg` annotation. E.g. `class.def("method", py::pos_only())`. - * */ + */ is_method_with_self_arg_only)) || !has_pos_only_args, "py::pos_only requires the use of argument annotations (for docstrings " From 34e808f6990d4bb46fc7bc16f88c72b2a819488e Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Sun, 10 Nov 2024 12:00:31 -0800 Subject: [PATCH 5/8] Add test_self_only_pos_only() in tests/test_methods_and_attributes --- tests/test_methods_and_attributes.cpp | 2 +- tests/test_methods_and_attributes.py | 7 +++++++ 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/tests/test_methods_and_attributes.cpp b/tests/test_methods_and_attributes.cpp index f433847c76..e324c8bdd4 100644 --- a/tests/test_methods_and_attributes.cpp +++ b/tests/test_methods_and_attributes.cpp @@ -294,7 +294,7 @@ TEST_SUBMODULE(methods_and_attributes, m) { static_cast( &ExampleMandA::overloaded)); }) - .def("__str__", &ExampleMandA::toString) + .def("__str__", &ExampleMandA::toString, py::pos_only()) .def_readwrite("value", &ExampleMandA::value); // test_copy_method diff --git a/tests/test_methods_and_attributes.py b/tests/test_methods_and_attributes.py index 91c7b7751e..cecc184647 100644 --- a/tests/test_methods_and_attributes.py +++ b/tests/test_methods_and_attributes.py @@ -19,6 +19,13 @@ ) +def test_self_only_pos_only(): + assert ( + m.ExampleMandA.__str__.__doc__ + == "__str__(self: pybind11_tests.methods_and_attributes.ExampleMandA, /) -> str\n" + ) + + def test_methods_and_attributes(): instance1 = m.ExampleMandA() instance2 = m.ExampleMandA(32) From 601e49920127f63c9bafd4acb99355f48191a5df Mon Sep 17 00:00:00 2001 From: Xuehai Pan Date: Mon, 11 Nov 2024 23:02:02 +0800 Subject: [PATCH 6/8] test: add docstring tests for generated dunder methods --- include/pybind11/pybind11.h | 1 + tests/test_enum.py | 60 +++++++++++++++++++++++++++ tests/test_pickling.py | 16 +++++++ tests/test_sequences_and_iterators.py | 30 +++++++++++--- 4 files changed, 102 insertions(+), 5 deletions(-) diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index f6ce716529..fe8384e57f 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -2481,6 +2481,7 @@ iterator make_iterator_impl(Iterator first, Sentinel last, Extra &&...extra) { // NOLINTNEXTLINE(readability-const-return-type) // PR #3263 }, std::forward(extra)..., + pos_only(), Policy); } diff --git a/tests/test_enum.py b/tests/test_enum.py index 03cd1c1a64..b8c8891612 100644 --- a/tests/test_enum.py +++ b/tests/test_enum.py @@ -1,6 +1,8 @@ # ruff: noqa: SIM201 SIM300 SIM202 from __future__ import annotations +import re + import pytest import env # noqa: F401 @@ -271,3 +273,61 @@ def test_docstring_signatures(): def test_str_signature(): for enum_type in [m.ScopedEnum, m.UnscopedEnum]: assert enum_type.__str__.__doc__.startswith("__str__") + + +def test_generated_dunder_methods_pos_only(): + for enum_type in [m.ScopedEnum, m.UnscopedEnum]: + for binary_op in [ + "__eq__", + "__ne__", + "__ge__", + "__gt__", + "__lt__", + "__le__", + "__and__", + "__rand__", + "__or__", + "__ror__", + "__xor__", + "__rxor__", + "__rxor__", + ]: + method = getattr(enum_type, binary_op, None) + if method is not None: + assert ( + re.match( + rf"^{binary_op}\(self: [\w\.]+, other: [\w\.]+, /\)", + method.__doc__, + ) + is not None + ) + for unary_op in [ + "__int__", + "__index__", + "__hash__", + "__str__", + "__repr__", + ]: + method = getattr(enum_type, unary_op, None) + if method is not None: + assert ( + re.match( + rf"^{unary_op}\(self: [\w\.]+, /\)", + method.__doc__, + ) + is not None + ) + assert ( + re.match( + r"^__getstate__\(self: [\w\.]+, /\)", + enum_type.__getstate__.__doc__, + ) + is not None + ) + assert ( + re.match( + r"^__setstate__\(self: [\w\.]+, state: [\w\.]+, /\)", + enum_type.__setstate__.__doc__, + ) + is not None + ) diff --git a/tests/test_pickling.py b/tests/test_pickling.py index 3e2e453969..55720e0af1 100644 --- a/tests/test_pickling.py +++ b/tests/test_pickling.py @@ -93,3 +93,19 @@ def test_roundtrip_simple_cpp_derived(): # Issue #3062: pickleable base C++ classes can incur object slicing # if derived typeid is not registered with pybind11 assert not m.check_dynamic_cast_SimpleCppDerived(p2) + + +def test_new_style_pickle_getstate_pos_only(): + assert ( + re.match( + r"^__getstate__\(self: [\w\.]+, /\)", m.PickleableNew.__getstate__.__doc__ + ) + is not None + ) + assert ( + re.match( + r"^__getstate__\(self: [\w\.]+, /\)", + m.PickleableWithDictNew.__getstate__.__doc__, + ) + is not None + ) diff --git a/tests/test_sequences_and_iterators.py b/tests/test_sequences_and_iterators.py index 20abd29d59..6fba6fba34 100644 --- a/tests/test_sequences_and_iterators.py +++ b/tests/test_sequences_and_iterators.py @@ -1,5 +1,7 @@ from __future__ import annotations +import re + import pytest from pytest import approx # noqa: PT013 @@ -253,16 +255,12 @@ def bad_next_call(): def test_iterator_passthrough(): """#181: iterator passthrough did not compile""" - from pybind11_tests.sequences_and_iterators import iterator_passthrough - values = [3, 5, 7, 9, 11, 13, 15] - assert list(iterator_passthrough(iter(values))) == values + assert list(m.iterator_passthrough(iter(values))) == values def test_iterator_rvp(): """#388: Can't make iterators via make_iterator() with different r/v policies""" - import pybind11_tests.sequences_and_iterators as m - assert list(m.make_iterator_1()) == [1, 2, 3] assert list(m.make_iterator_2()) == [1, 2, 3] assert not isinstance(m.make_iterator_1(), type(m.make_iterator_2())) @@ -274,3 +272,25 @@ def test_carray_iterator(): arr_h = m.CArrayHolder(*args_gt) args = list(arr_h) assert args_gt == args + + +def test_generated_dunder_methods_pos_only(): + string_map = m.StringMap({"hi": "bye", "black": "white"}) + for it in ( + m.make_iterator_1(), + m.make_iterator_2(), + m.iterator_passthrough(iter([3, 5, 7])), + iter(m.Sequence(5)), + iter(string_map), + string_map.items(), + string_map.values(), + iter(m.CArrayHolder(*[float(i) for i in range(3)])), + ): + assert ( + re.match(r"^__iter__\(self: [\w\.]+, /\)", type(it).__iter__.__doc__) + is not None + ) + assert ( + re.match(r"^__next__\(self: [\w\.]+, /\)", type(it).__next__.__doc__) + is not None + ) From ebaf3c8a9b3e3acc7a32936d05e99b0b967470e0 Mon Sep 17 00:00:00 2001 From: Xuehai Pan Date: Mon, 11 Nov 2024 23:36:18 +0800 Subject: [PATCH 7/8] test: remove failed tests --- tests/test_enum.py | 4 ++-- tests/test_pickling.py | 13 +++++++------ 2 files changed, 9 insertions(+), 8 deletions(-) diff --git a/tests/test_enum.py b/tests/test_enum.py index b8c8891612..044ef1803e 100644 --- a/tests/test_enum.py +++ b/tests/test_enum.py @@ -286,8 +286,8 @@ def test_generated_dunder_methods_pos_only(): "__le__", "__and__", "__rand__", - "__or__", - "__ror__", + # "__or__", # fail with some compilers (__doc__ = "Return self|value.") + # "__ror__", # fail with some compilers (__doc__ = "Return value|self.") "__xor__", "__rxor__", "__rxor__", diff --git a/tests/test_pickling.py b/tests/test_pickling.py index 55720e0af1..d3551efc12 100644 --- a/tests/test_pickling.py +++ b/tests/test_pickling.py @@ -102,10 +102,11 @@ def test_new_style_pickle_getstate_pos_only(): ) is not None ) - assert ( - re.match( - r"^__getstate__\(self: [\w\.]+, /\)", - m.PickleableWithDictNew.__getstate__.__doc__, + if hasattr(m, "PickleableWithDictNew"): + assert ( + re.match( + r"^__getstate__\(self: [\w\.]+, /\)", + m.PickleableWithDictNew.__getstate__.__doc__, + ) + is not None ) - is not None - ) From 609c4a3872c884728ac028a98c580f0a1ab9dbc0 Mon Sep 17 00:00:00 2001 From: Xuehai Pan Date: Tue, 12 Nov 2024 00:37:33 +0800 Subject: [PATCH 8/8] fix(test): run `gc.collect()` three times for refcount tests --- tests/conftest.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/conftest.py b/tests/conftest.py index 9e7ca88120..8eb803d2f3 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -198,10 +198,11 @@ def pytest_assertrepr_compare(op, left, right): # noqa: ARG001 def gc_collect(): - """Run the garbage collector twice (needed when running + """Run the garbage collector three times (needed when running reference counting tests with PyPy)""" gc.collect() gc.collect() + gc.collect() def pytest_configure():