From 65374c8e62488c557dccf70c9276e3f9fe04b007 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Thu, 8 Dec 2022 22:06:51 -0800 Subject: [PATCH 1/4] `pybind11::handle` `inc_ref()` & `dec_ref()` `PyGILState_Check()` **excluding** `nullptr` (#4246) * pybind11/pytypes.h `inc_ref()`, `dec_ref()` `PyGILState_Check()` **excluding** `nullptr` Guarded by `PYBIND11_ASSERT_GIL_HELD_INCREF_DECREF` * Disable `PYBIND11_ASSERT_GIL_HELD_INCREF_DECREF` for PyPy under Windows. * Add reference to PR #4268 (PyPy Windows) --- include/pybind11/detail/common.h | 9 +++++++++ include/pybind11/pytypes.h | 10 ++++++++++ 2 files changed, 19 insertions(+) diff --git a/include/pybind11/detail/common.h b/include/pybind11/detail/common.h index 6e295fd2f..9acf10466 100644 --- a/include/pybind11/detail/common.h +++ b/include/pybind11/detail/common.h @@ -323,6 +323,15 @@ PYBIND11_WARNING_POP # define PYBIND11_HAS_U8STRING #endif +// See description of PR #4246: +#if !defined(NDEBUG) && !defined(PY_ASSERT_GIL_HELD_INCREF_DECREF) \ + && !(defined(PYPY_VERSION) \ + && defined(_MSC_VER)) /* PyPy Windows: pytest hangs indefinitely at the end of the \ + process (see PR #4268) */ \ + && !defined(PYBIND11_ASSERT_GIL_HELD_INCREF_DECREF) +# define PYBIND11_ASSERT_GIL_HELD_INCREF_DECREF +#endif + // #define PYBIND11_STR_LEGACY_PERMISSIVE // If DEFINED, pybind11::str can hold PyUnicodeObject or PyBytesObject // (probably surprising and never documented, but this was the diff --git a/include/pybind11/pytypes.h b/include/pybind11/pytypes.h index 328287931..56e0423ac 100644 --- a/include/pybind11/pytypes.h +++ b/include/pybind11/pytypes.h @@ -249,6 +249,11 @@ public: const handle &inc_ref() const & { #ifdef PYBIND11_HANDLE_REF_DEBUG inc_ref_counter(1); +#endif +#if defined(PYBIND11_ASSERT_GIL_HELD_INCREF_DECREF) + if (m_ptr != nullptr && !PyGILState_Check()) { + throw std::runtime_error("pybind11::handle::inc_ref() PyGILState_Check() failure."); + } #endif Py_XINCREF(m_ptr); return *this; @@ -260,6 +265,11 @@ public: this function automatically. Returns a reference to itself. \endrst */ const handle &dec_ref() const & { +#if defined(PYBIND11_ASSERT_GIL_HELD_INCREF_DECREF) + if (m_ptr != nullptr && !PyGILState_Check()) { + throw std::runtime_error("pybind11::handle::dec_ref() PyGILState_Check() failure."); + } +#endif Py_XDECREF(m_ptr); return *this; } From 00126859a550d10fb9f5bc0d4fdf86988c2012f4 Mon Sep 17 00:00:00 2001 From: Frank Date: Fri, 9 Dec 2022 08:10:10 +0100 Subject: [PATCH 2/4] Add option for enable/disable enum members in docstring. (#2768) * Add option for enable/disable enum members in docstring * Add tests for disable enum members docstring option * Add docstring options to documentation * style: pre-commit fixes * Fix typos in documentation * Improve documentation wording * Apply suggestions by @Skylion007 Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Ralf W. Grosse-Kunstleve --- docs/advanced/misc.rst | 9 ++++++ include/pybind11/options.h | 16 ++++++++++ include/pybind11/pybind11.h | 50 +++++++++++++++++------------- tests/test_docstring_options.cpp | 53 ++++++++++++++++++++++++++++++++ tests/test_docstring_options.py | 23 ++++++++++++++ 5 files changed, 129 insertions(+), 22 deletions(-) diff --git a/docs/advanced/misc.rst b/docs/advanced/misc.rst index 71960b803..35a6ebcd6 100644 --- a/docs/advanced/misc.rst +++ b/docs/advanced/misc.rst @@ -324,6 +324,15 @@ The class ``options`` allows you to selectively suppress auto-generated signatur m.def("add", [](int a, int b) { return a + b; }, "A function which adds two numbers"); } +pybind11 also appends all members of an enum to the resulting enum docstring. +This default behavior can be disabled by using the ``disable_enum_members_docstring()`` +function of the ``options`` class. + +With ``disable_user_defined_docstrings()`` all user defined docstrings of +``module_::def()``, ``class_::def()`` and ``enum_()`` are disabled, but the +function signatures and enum members are included in the docstring, unless they +are disabled separately. + Note that changes to the settings affect only function bindings created during the lifetime of the ``options`` instance. When it goes out of scope at the end of the module's init function, the default settings are restored to prevent unwanted side effects. diff --git a/include/pybind11/options.h b/include/pybind11/options.h index 1e493bdcc..1b2122522 100644 --- a/include/pybind11/options.h +++ b/include/pybind11/options.h @@ -47,6 +47,16 @@ public: return *this; } + options &disable_enum_members_docstring() & { + global_state().show_enum_members_docstring = false; + return *this; + } + + options &enable_enum_members_docstring() & { + global_state().show_enum_members_docstring = true; + return *this; + } + // Getter methods (return the global state): static bool show_user_defined_docstrings() { @@ -55,6 +65,10 @@ public: static bool show_function_signatures() { return global_state().show_function_signatures; } + static bool show_enum_members_docstring() { + return global_state().show_enum_members_docstring; + } + // This type is not meant to be allocated on the heap. void *operator new(size_t) = delete; @@ -63,6 +77,8 @@ private: bool show_user_defined_docstrings = true; //< Include user-supplied texts in docstrings. bool show_function_signatures = true; //< Include auto-generated function signatures // in docstrings. + bool show_enum_members_docstring = true; //< Include auto-generated member list in enum + // docstrings. }; static state &global_state() { diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index a7310dfd4..6205effd6 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -1972,29 +1972,35 @@ struct enum_base { name("name"), is_method(m_base)); - m_base.attr("__doc__") = static_property( - cpp_function( - [](handle arg) -> std::string { - std::string docstring; - dict entries = arg.attr("__entries"); - if (((PyTypeObject *) arg.ptr())->tp_doc) { - docstring += std::string(((PyTypeObject *) arg.ptr())->tp_doc) + "\n\n"; - } - docstring += "Members:"; - for (auto kv : entries) { - auto key = std::string(pybind11::str(kv.first)); - auto comment = kv.second[int_(1)]; - docstring += "\n\n " + key; - if (!comment.is_none()) { - docstring += " : " + (std::string) pybind11::str(comment); + if (options::show_enum_members_docstring()) { + m_base.attr("__doc__") = static_property( + cpp_function( + [](handle arg) -> std::string { + std::string docstring; + dict entries = arg.attr("__entries"); + if (((PyTypeObject *) arg.ptr())->tp_doc) { + docstring += std::string( + reinterpret_cast(arg.ptr())->tp_doc); + docstring += "\n\n"; } - } - return docstring; - }, - name("__doc__")), - none(), - none(), - ""); + docstring += "Members:"; + for (auto kv : entries) { + auto key = std::string(pybind11::str(kv.first)); + auto comment = kv.second[int_(1)]; + docstring += "\n\n "; + docstring += key; + if (!comment.is_none()) { + docstring += " : "; + docstring += pybind11::str(comment).cast(); + } + } + return docstring; + }, + name("__doc__")), + none(), + none(), + ""); + } m_base.attr("__members__") = static_property(cpp_function( [](handle arg) -> dict { diff --git a/tests/test_docstring_options.cpp b/tests/test_docstring_options.cpp index 4d44f4e20..dda1cf6e4 100644 --- a/tests/test_docstring_options.cpp +++ b/tests/test_docstring_options.cpp @@ -85,4 +85,57 @@ TEST_SUBMODULE(docstring_options, m) { &DocstringTestFoo::setValue, "This is a property docstring"); } + + { + enum class DocstringTestEnum1 { Member1, Member2 }; + + py::enum_(m, "DocstringTestEnum1", "Enum docstring") + .value("Member1", DocstringTestEnum1::Member1) + .value("Member2", DocstringTestEnum1::Member2); + } + + { + py::options options; + options.enable_enum_members_docstring(); + + enum class DocstringTestEnum2 { Member1, Member2 }; + + py::enum_(m, "DocstringTestEnum2", "Enum docstring") + .value("Member1", DocstringTestEnum2::Member1) + .value("Member2", DocstringTestEnum2::Member2); + } + + { + py::options options; + options.disable_enum_members_docstring(); + + enum class DocstringTestEnum3 { Member1, Member2 }; + + py::enum_(m, "DocstringTestEnum3", "Enum docstring") + .value("Member1", DocstringTestEnum3::Member1) + .value("Member2", DocstringTestEnum3::Member2); + } + + { + py::options options; + options.disable_user_defined_docstrings(); + + enum class DocstringTestEnum4 { Member1, Member2 }; + + py::enum_(m, "DocstringTestEnum4", "Enum docstring") + .value("Member1", DocstringTestEnum4::Member1) + .value("Member2", DocstringTestEnum4::Member2); + } + + { + py::options options; + options.disable_user_defined_docstrings(); + options.disable_enum_members_docstring(); + + enum class DocstringTestEnum5 { Member1, Member2 }; + + py::enum_(m, "DocstringTestEnum5", "Enum docstring") + .value("Member1", DocstringTestEnum5::Member1) + .value("Member2", DocstringTestEnum5::Member2); + } } diff --git a/tests/test_docstring_options.py b/tests/test_docstring_options.py index fcd16b89f..e6f5a9d98 100644 --- a/tests/test_docstring_options.py +++ b/tests/test_docstring_options.py @@ -39,3 +39,26 @@ def test_docstring_options(): # Suppression of user-defined docstrings for non-function objects assert not m.DocstringTestFoo.__doc__ assert not m.DocstringTestFoo.value_prop.__doc__ + + # Check existig behaviour of enum docstings + assert ( + m.DocstringTestEnum1.__doc__ + == "Enum docstring\n\nMembers:\n\n Member1\n\n Member2" + ) + + # options.enable_enum_members_docstring() + assert ( + m.DocstringTestEnum2.__doc__ + == "Enum docstring\n\nMembers:\n\n Member1\n\n Member2" + ) + + # options.disable_enum_members_docstring() + assert m.DocstringTestEnum3.__doc__ == "Enum docstring" + + # options.disable_user_defined_docstrings() + assert m.DocstringTestEnum4.__doc__ == "Members:\n\n Member1\n\n Member2" + + # options.disable_user_defined_docstrings() + # options.disable_enum_members_docstring() + # When all options are disabled, no docstring (instead of an empty one) should be generated + assert m.DocstringTestEnum5.__doc__ is None From 9db988013ceb54ab15fb775b229ac9180fd08fbe Mon Sep 17 00:00:00 2001 From: aimir <48388610+aimir@users.noreply.github.com> Date: Fri, 9 Dec 2022 09:15:11 +0200 Subject: [PATCH 3/4] Correct class names for KeysView, ValuesView and ItemsView in bind_map (#4353) * Create templated abstract classes KeysView, ValuesView and ItemsView, and implement them on-the-fly when wrapping any specific map type * We don't want to wrap different ValuesView objects for double values and const double, for example, as both wrappers will be named ValuesView[float] * Fallback to C++ names if key or values types are not wrapped * Added a test for .keys(), .values() and .items() returning the same types for similarly-typed maps * Fixed wrong use of auto in a declarator list: the two descriptions might have different types * Fixes for clang-tidy issues: explicit single-argument constructor, using the 'override' keyword when overriding functions * Bugfix for old versions of clang++, which seem to have trouble with the struct being defined inside a module, which was also needlessly ugly anyway * Bugfix for clang++, which doesn't have some of the names in runtime uness they are specified to be static * A fix for clang-tidy performance-inefficient-string-concatenation issues - I personally think this looks uglier, but it's probably worth it for clang-tidy to be happy * Possible fix for clang++ linking issues - make the descriptions static constexpr to make sure they are known before linking * Correct names for previously-wrapped types as keys/values of maps * Bugfix - typo in type info names which caused things to segfault * Apply suggestions from code review Co-authored-by: Aaron Gokaslan * Use detail::remove_cvref_t instead of doing remove_cv and remove_reference separately * Avoid names with double underscore, as they are reserved * Improved testing for KeysView, ValuesView and ItemsView: check type names + stricter asserts * Moved description logic to helper function in type_caster_base.h * style: pre-commit fixes * Fix a clang-tidy issue: do not use 'else' after 'return' * Apply suggestion by @Skylion007, with additional trivial simplification. Co-authored-by: Amir Co-authored-by: aimir Co-authored-by: Aaron Gokaslan Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Ralf W. Grosse-Kunstleve --- include/pybind11/detail/type_caster_base.h | 9 ++ include/pybind11/stl_bind.h | 154 ++++++++++++++------- tests/test_stl_binders.py | 26 ++++ 3 files changed, 142 insertions(+), 47 deletions(-) diff --git a/include/pybind11/detail/type_caster_base.h b/include/pybind11/detail/type_caster_base.h index 21f69c289..0b710d7e4 100644 --- a/include/pybind11/detail/type_caster_base.h +++ b/include/pybind11/detail/type_caster_base.h @@ -1006,5 +1006,14 @@ protected: static Constructor make_move_constructor(...) { return nullptr; } }; +PYBIND11_NOINLINE std::string type_info_description(const std::type_info &ti) { + if (auto *type_data = get_type_info(ti)) { + handle th((PyObject *) type_data->type); + return th.attr("__module__").cast() + '.' + + th.attr("__qualname__").cast(); + } + return clean_type_id(ti.name()); +} + PYBIND11_NAMESPACE_END(detail) PYBIND11_NAMESPACE_END(PYBIND11_NAMESPACE) diff --git a/include/pybind11/stl_bind.h b/include/pybind11/stl_bind.h index 22a29b476..0c634597e 100644 --- a/include/pybind11/stl_bind.h +++ b/include/pybind11/stl_bind.h @@ -10,10 +10,13 @@ #pragma once #include "detail/common.h" +#include "detail/type_caster_base.h" +#include "cast.h" #include "operators.h" #include #include +#include PYBIND11_NAMESPACE_BEGIN(PYBIND11_NAMESPACE) PYBIND11_NAMESPACE_BEGIN(detail) @@ -636,18 +639,52 @@ auto map_if_insertion_operator(Class_ &cl, std::string const &name) "Return the canonical string representation of this map."); } -template +template struct keys_view { - Map ↦ + virtual size_t len() = 0; + virtual iterator iter() = 0; + virtual bool contains(const KeyType &k) = 0; + virtual bool contains(const object &k) = 0; + virtual ~keys_view() = default; }; -template +template struct values_view { + virtual size_t len() = 0; + virtual iterator iter() = 0; + virtual ~values_view() = default; +}; + +template +struct items_view { + virtual size_t len() = 0; + virtual iterator iter() = 0; + virtual ~items_view() = default; +}; + +template +struct KeysViewImpl : public KeysView { + explicit KeysViewImpl(Map &map) : map(map) {} + size_t len() override { return map.size(); } + iterator iter() override { return make_key_iterator(map.begin(), map.end()); } + bool contains(const typename Map::key_type &k) override { return map.find(k) != map.end(); } + bool contains(const object &) override { return false; } Map ↦ }; -template -struct items_view { +template +struct ValuesViewImpl : public ValuesView { + explicit ValuesViewImpl(Map &map) : map(map) {} + size_t len() override { return map.size(); } + iterator iter() override { return make_value_iterator(map.begin(), map.end()); } + Map ↦ +}; + +template +struct ItemsViewImpl : public ItemsView { + explicit ItemsViewImpl(Map &map) : map(map) {} + size_t len() override { return map.size(); } + iterator iter() override { return make_iterator(map.begin(), map.end()); } Map ↦ }; @@ -657,9 +694,11 @@ template , typename... class_ bind_map(handle scope, const std::string &name, Args &&...args) { using KeyType = typename Map::key_type; using MappedType = typename Map::mapped_type; - using KeysView = detail::keys_view; - using ValuesView = detail::values_view; - using ItemsView = detail::items_view; + using StrippedKeyType = detail::remove_cvref_t; + using StrippedMappedType = detail::remove_cvref_t; + using KeysView = detail::keys_view; + using ValuesView = detail::values_view; + using ItemsView = detail::items_view; using Class_ = class_; // If either type is a non-module-local bound type then make the map binding non-local as well; @@ -673,12 +712,57 @@ class_ bind_map(handle scope, const std::string &name, Args && } Class_ cl(scope, name.c_str(), pybind11::module_local(local), std::forward(args)...); - class_ keys_view( - scope, ("KeysView[" + name + "]").c_str(), pybind11::module_local(local)); - class_ values_view( - scope, ("ValuesView[" + name + "]").c_str(), pybind11::module_local(local)); - class_ items_view( - scope, ("ItemsView[" + name + "]").c_str(), pybind11::module_local(local)); + static constexpr auto key_type_descr = detail::make_caster::name; + static constexpr auto mapped_type_descr = detail::make_caster::name; + std::string key_type_name(key_type_descr.text), mapped_type_name(mapped_type_descr.text); + + // If key type isn't properly wrapped, fall back to C++ names + if (key_type_name == "%") { + key_type_name = detail::type_info_description(typeid(KeyType)); + } + // Similarly for value type: + if (mapped_type_name == "%") { + mapped_type_name = detail::type_info_description(typeid(MappedType)); + } + + // Wrap KeysView[KeyType] if it wasn't already wrapped + if (!detail::get_type_info(typeid(KeysView))) { + class_ keys_view( + scope, ("KeysView[" + key_type_name + "]").c_str(), pybind11::module_local(local)); + keys_view.def("__len__", &KeysView::len); + keys_view.def("__iter__", + &KeysView::iter, + keep_alive<0, 1>() /* Essential: keep view alive while iterator exists */ + ); + keys_view.def("__contains__", + static_cast(&KeysView::contains)); + // Fallback for when the object is not of the key type + keys_view.def("__contains__", + static_cast(&KeysView::contains)); + } + // Similarly for ValuesView: + if (!detail::get_type_info(typeid(ValuesView))) { + class_ values_view(scope, + ("ValuesView[" + mapped_type_name + "]").c_str(), + pybind11::module_local(local)); + values_view.def("__len__", &ValuesView::len); + values_view.def("__iter__", + &ValuesView::iter, + keep_alive<0, 1>() /* Essential: keep view alive while iterator exists */ + ); + } + // Similarly for ItemsView: + if (!detail::get_type_info(typeid(ItemsView))) { + class_ items_view( + scope, + ("ItemsView[" + key_type_name + ", ").append(mapped_type_name + "]").c_str(), + pybind11::module_local(local)); + items_view.def("__len__", &ItemsView::len); + items_view.def("__iter__", + &ItemsView::iter, + keep_alive<0, 1>() /* Essential: keep view alive while iterator exists */ + ); + } cl.def(init<>()); @@ -698,19 +782,25 @@ class_ bind_map(handle scope, const std::string &name, Args && cl.def( "keys", - [](Map &m) { return KeysView{m}; }, + [](Map &m) { + return std::unique_ptr(new detail::KeysViewImpl(m)); + }, keep_alive<0, 1>() /* Essential: keep map alive while view exists */ ); cl.def( "values", - [](Map &m) { return ValuesView{m}; }, + [](Map &m) { + return std::unique_ptr(new detail::ValuesViewImpl(m)); + }, keep_alive<0, 1>() /* Essential: keep map alive while view exists */ ); cl.def( "items", - [](Map &m) { return ItemsView{m}; }, + [](Map &m) { + return std::unique_ptr(new detail::ItemsViewImpl(m)); + }, keep_alive<0, 1>() /* Essential: keep map alive while view exists */ ); @@ -749,36 +839,6 @@ class_ bind_map(handle scope, const std::string &name, Args && cl.def("__len__", &Map::size); - keys_view.def("__len__", [](KeysView &view) { return view.map.size(); }); - keys_view.def( - "__iter__", - [](KeysView &view) { return make_key_iterator(view.map.begin(), view.map.end()); }, - keep_alive<0, 1>() /* Essential: keep view alive while iterator exists */ - ); - keys_view.def("__contains__", [](KeysView &view, const KeyType &k) -> bool { - auto it = view.map.find(k); - if (it == view.map.end()) { - return false; - } - return true; - }); - // Fallback for when the object is not of the key type - keys_view.def("__contains__", [](KeysView &, const object &) -> bool { return false; }); - - values_view.def("__len__", [](ValuesView &view) { return view.map.size(); }); - values_view.def( - "__iter__", - [](ValuesView &view) { return make_value_iterator(view.map.begin(), view.map.end()); }, - keep_alive<0, 1>() /* Essential: keep view alive while iterator exists */ - ); - - items_view.def("__len__", [](ItemsView &view) { return view.map.size(); }); - items_view.def( - "__iter__", - [](ItemsView &view) { return make_iterator(view.map.begin(), view.map.end()); }, - keep_alive<0, 1>() /* Essential: keep view alive while iterator exists */ - ); - return cl; } diff --git a/tests/test_stl_binders.py b/tests/test_stl_binders.py index d5e9ccced..9eb906f06 100644 --- a/tests/test_stl_binders.py +++ b/tests/test_stl_binders.py @@ -309,3 +309,29 @@ def test_map_delitem(): del um["ua"] assert sorted(list(um)) == ["ub"] assert sorted(list(um.items())) == [("ub", 2.6)] + + +def test_map_view_types(): + map_string_double = m.MapStringDouble() + unordered_map_string_double = m.UnorderedMapStringDouble() + map_string_double_const = m.MapStringDoubleConst() + unordered_map_string_double_const = m.UnorderedMapStringDoubleConst() + + assert map_string_double.keys().__class__.__name__ == "KeysView[str]" + assert map_string_double.values().__class__.__name__ == "ValuesView[float]" + assert map_string_double.items().__class__.__name__ == "ItemsView[str, float]" + + keys_type = type(map_string_double.keys()) + assert type(unordered_map_string_double.keys()) is keys_type + assert type(map_string_double_const.keys()) is keys_type + assert type(unordered_map_string_double_const.keys()) is keys_type + + values_type = type(map_string_double.values()) + assert type(unordered_map_string_double.values()) is values_type + assert type(map_string_double_const.values()) is values_type + assert type(unordered_map_string_double_const.values()) is values_type + + items_type = type(map_string_double.items()) + assert type(unordered_map_string_double.items()) is items_type + assert type(map_string_double_const.items()) is items_type + assert type(unordered_map_string_double_const.items()) is items_type From 663b86c26ca07c51f2981681ba79e2595e17fbb9 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Fri, 9 Dec 2022 10:53:03 -0800 Subject: [PATCH 4/4] Add flake8 `B905` to `extend-ignore` in setup.cfg (#4391) * Add flake8 `--ignore=B905,N818,W503` * Add B905 to `extend-ignore` in setup.cfg (thanks @Skylion007), leave .pre-commit-config.yaml as-is on master. --- setup.cfg | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/setup.cfg b/setup.cfg index 9af50ea48..16479ab00 100644 --- a/setup.cfg +++ b/setup.cfg @@ -46,5 +46,5 @@ zip_safe = False max-line-length = 120 show_source = True exclude = .git, __pycache__, build, dist, docs, tools, venv -extend-ignore = E203, E722, B903, B950 +extend-ignore = E203, E722, B903, B905, B950 extend-select = B9