From 7f94f24d6467f14276c9f4e72fae09a364aa0b6c Mon Sep 17 00:00:00 2001 From: Xuehai Pan Date: Tue, 12 Nov 2024 07:35:28 +0800 Subject: [PATCH] feat(typing): allow annotate methods with `pos_only` when only have the `self` argument (#5403) * feat: allow annotate methods with `pos_only` when only have the `self` argument * chore(typing): make arguments for auto-generated dunder methods positional-only * docs: add more comments to improve readability * style: fix nit suggestions * Add test_self_only_pos_only() in tests/test_methods_and_attributes * test: add docstring tests for generated dunder methods * test: remove failed tests * fix(test): run `gc.collect()` three times for refcount tests --------- Co-authored-by: Ralf W. Grosse-Kunstleve --- include/pybind11/detail/init.h | 2 +- include/pybind11/pybind11.h | 59 ++++++++++++++++++-------- tests/conftest.py | 3 +- tests/test_enum.py | 60 +++++++++++++++++++++++++++ tests/test_methods_and_attributes.cpp | 2 +- tests/test_methods_and_attributes.py | 7 ++++ tests/test_pickling.py | 17 ++++++++ tests/test_sequences_and_iterators.py | 30 +++++++++++--- 8 files changed, 155 insertions(+), 25 deletions(-) diff --git a/include/pybind11/detail/init.h b/include/pybind11/detail/init.h index 79cc930c8..3eeeaaf97 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 b4f93f1a6..fe8384e57 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -301,9 +301,20 @@ protected: constexpr bool has_kw_only_args = any_of...>::value, 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 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, "py::kw_only requires the use of argument annotations"); - static_assert(has_arg_annotations || !has_pos_only_args, + 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)"); @@ -2022,9 +2033,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 { @@ -2032,7 +2045,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( @@ -2087,7 +2101,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( \ @@ -2097,7 +2112,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( \ @@ -2107,7 +2123,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)); @@ -2127,7 +2144,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); @@ -2147,11 +2165,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) { @@ -2243,9 +2265,9 @@ public: 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( @@ -2254,7 +2276,8 @@ public: detail::is_new_style_constructor(), pybind11::name("__setstate__"), is_method(*this), - arg("state")); + arg("state"), + pos_only()); } /// Export enumeration entries into the parent scope @@ -2440,7 +2463,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 { @@ -2457,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/conftest.py b/tests/conftest.py index 9e7ca8812..8eb803d2f 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(): diff --git a/tests/test_enum.py b/tests/test_enum.py index 03cd1c1a6..044ef1803 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__", # fail with some compilers (__doc__ = "Return self|value.") + # "__ror__", # fail with some compilers (__doc__ = "Return value|self.") + "__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_methods_and_attributes.cpp b/tests/test_methods_and_attributes.cpp index f433847c7..e324c8bdd 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 91c7b7751..cecc18464 100644 --- a/tests/test_methods_and_attributes.py +++ b/tests/test_methods_and_attributes.py @@ -19,6 +19,13 @@ NO_DELETER_MSG = ( ) +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) diff --git a/tests/test_pickling.py b/tests/test_pickling.py index 3e2e45396..d3551efc1 100644 --- a/tests/test_pickling.py +++ b/tests/test_pickling.py @@ -93,3 +93,20 @@ 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 + ) + if hasattr(m, "PickleableWithDictNew"): + 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 20abd29d5..6fba6fba3 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 test_python_iterator_in_cpp(): 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 + )