From 91ca4e6d60088bd8a48ba38359330a52c28124db Mon Sep 17 00:00:00 2001 From: Sergei Izmailov Date: Thu, 14 May 2020 23:16:01 +0300 Subject: [PATCH 1/8] Render typed iterators (e.g. Iterator[int]) in docstrings This commit introduces minor breaking change: `make_iterator` and `make_key_iterator` now return `iterator_state` instance instead of `py::iterator`. It doesn't affect regular use of those functions (immediate return from __iter__ lambda), but requires changes in user code with implicit assignments/conversions, e.g.: py::iterator it = make_iterator(...); # requires explicit py::cast() --- include/pybind11/pybind11.h | 35 ++++++++++++++++++++------ tests/test_sequences_and_iterators.cpp | 2 +- tests/test_stl_binders.py | 29 +++++++++++++++++++++ 3 files changed, 58 insertions(+), 8 deletions(-) diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index d34c92c24a..ff38f29a7c 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -1712,6 +1712,22 @@ struct iterator_state { bool first_or_done; }; +template +struct type_caster> : + type_caster_base> { + using ValueType = decltype(*std::declval()); +public: + static constexpr auto name = _("Iterator[") + make_caster::name + _("]"); +}; + +template +struct type_caster> : + type_caster_base> { + using ValueType = decltype((*std::declval()).first); +public: + static constexpr auto name = _("Iterator[") + make_caster::name + _("]"); +}; + PYBIND11_NAMESPACE_END(detail) /// Makes a python iterator from a first and past-the-end C++ InputIterator. @@ -1720,7 +1736,8 @@ template ()), typename... Extra> -iterator make_iterator(Iterator first, Sentinel last, Extra &&... extra) { +detail::iterator_state + make_iterator(Iterator first, Sentinel last, Extra &&... extra) { typedef detail::iterator_state state; if (!detail::get_type_info(typeid(state), false)) { @@ -1738,8 +1755,7 @@ iterator make_iterator(Iterator first, Sentinel last, Extra &&... extra) { return *s.it; }, std::forward(extra)..., Policy); } - - return cast(state{first, last, true}); + return state{first, last, true}; } /// Makes an python iterator over the keys (`.first`) of a iterator over pairs from a @@ -1749,7 +1765,8 @@ template ()).first), typename... Extra> -iterator make_key_iterator(Iterator first, Sentinel last, Extra &&... extra) { +detail::iterator_state + make_key_iterator(Iterator first, Sentinel last, Extra &&... extra) { typedef detail::iterator_state state; if (!detail::get_type_info(typeid(state), false)) { @@ -1768,20 +1785,24 @@ iterator make_key_iterator(Iterator first, Sentinel last, Extra &&... extra) { }, std::forward(extra)..., Policy); } - return cast(state{first, last, true}); + return state{first, last, true}; } /// Makes an iterator over values of an stl container or other container supporting /// `std::begin()`/`std::end()` template iterator make_iterator(Type &value, Extra&&... extra) { + typename Type, typename... Extra> +detail::iterator_state())), decltype(std::end(std::declval())), false, Policy> +make_iterator(Type &value, Extra&&... extra) { return make_iterator(std::begin(value), std::end(value), extra...); } /// Makes an iterator over the keys (`.first`) of a stl map-like container supporting /// `std::begin()`/`std::end()` template iterator make_key_iterator(Type &value, Extra&&... extra) { + typename Type, typename... Extra> +detail::iterator_state())), decltype(std::end(std::declval())), true, Policy> +make_key_iterator(Type &value, Extra&&... extra) { return make_key_iterator(std::begin(value), std::end(value), extra...); } diff --git a/tests/test_sequences_and_iterators.cpp b/tests/test_sequences_and_iterators.cpp index 05f999bb3b..70999ffd84 100644 --- a/tests/test_sequences_and_iterators.cpp +++ b/tests/test_sequences_and_iterators.cpp @@ -344,7 +344,7 @@ TEST_SUBMODULE(sequences_and_iterators, m) { // test_iterator_passthrough // #181: iterator passthrough did not compile - m.def("iterator_passthrough", [](py::iterator s) -> py::iterator { + m.def("iterator_passthrough", [](py::iterator s) { return py::make_iterator(std::begin(s), std::end(s)); }); diff --git a/tests/test_stl_binders.py b/tests/test_stl_binders.py index 27b326f070..a16d028d38 100644 --- a/tests/test_stl_binders.py +++ b/tests/test_stl_binders.py @@ -277,3 +277,32 @@ def test_map_delitem(): del um['ua'] assert sorted(list(um)) == ['ub'] assert sorted(list(um.items())) == [('ub', 2.6)] + + +def test_map_docstrings(doc): + assert (doc(m.MapStringDouble.__iter__) == + "__iter__(self: m.stl_binders.MapStringDouble)" + " -> Iterator[str]") + assert (doc(m.MapStringDouble.items) == + "items(self: m.stl_binders.MapStringDouble)" + " -> Iterator[Tuple[str, float]]") + assert (doc(m.UnorderedMapStringDouble.__iter__) == + "__iter__(self: m.stl_binders.UnorderedMapStringDouble)" + " -> Iterator[str]\n") + assert (doc(m.UnorderedMapStringDouble.items) == + "items(self: m.stl_binders.UnorderedMapStringDouble)" + " -> Iterator[Tuple[str, float]]\n") + + +def test_vector_docstrings(doc): + assert (doc(m.VectorInt.__iter__) == + "__iter__(self: m.stl_binders.VectorInt)" + " -> Iterator[int]\n") + + +@pytest.unsupported_on_pypy +@pytest.requires_numpy +def test_vector_docstring2(doc): + assert (doc(m.VectorStruct.__iter__) == + "__iter__(self: m.stl_binders.VectorStruct)" + " -> Iterator[m.stl_binders.VStruct]") From 8bbfeea340775a8937a78083e6988bb6ab8adbea Mon Sep 17 00:00:00 2001 From: Sergei Izmailov Date: Fri, 7 Aug 2020 14:07:29 +0300 Subject: [PATCH 2/8] Introduce make_iterator_ng/make_key_iterator_ng, deprecate make_iterator,make_key_iterator --- include/pybind11/pybind11.h | 51 +++++++++++++++++++++++++++++++------ include/pybind11/stl_bind.h | 8 +++--- 2 files changed, 47 insertions(+), 12 deletions(-) diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index ff38f29a7c..0feba5e810 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -1737,7 +1737,7 @@ template ()), typename... Extra> detail::iterator_state - make_iterator(Iterator first, Sentinel last, Extra &&... extra) { +[[deprecated]] make_iterator_ng(Iterator first, Sentinel last, Extra &&... extra) { typedef detail::iterator_state state; if (!detail::get_type_info(typeid(state), false)) { @@ -1766,7 +1766,7 @@ template ()).first), typename... Extra> detail::iterator_state - make_key_iterator(Iterator first, Sentinel last, Extra &&... extra) { + make_key_iterator_ng(Iterator first, Sentinel last, Extra &&... extra) { typedef detail::iterator_state state; if (!detail::get_type_info(typeid(state), false)) { @@ -1788,24 +1788,59 @@ detail::iterator_state return state{first, last, true}; } +/// Makes a python iterator from a first and past-the-end C++ InputIterator. +template ()), + typename... Extra> +[[deprecated("Superseded by make_iterator_ng")]] +auto make_iterator(Iterator first, Sentinel last, Extra &&... extra) { + return cast(make_iterator_ng(first, last, std::forward(extra)...)); +} + +/// Makes a python iterator from a first and past-the-end C++ InputIterator. +template ()), + typename... Extra> +[[deprecated("Superseded by make_key_iterator_ng")]] +auto make_key_iterator(Iterator first, Sentinel last, Extra &&... extra) { + return cast(make_key_iterator_ng(first, last, std::forward(extra)...)); +} + +template +auto make_iterator_ng(Type &value, Extra&&... extra) { + return make_iterator_ng(std::begin(value), std::end(value), std::forward(extra)...); +} + +/// Makes an iterator over the keys (`.first`) of a stl map-like container supporting +/// `std::begin()`/`std::end()` +template +auto make_key_iterator_ng(Type &value, Extra&&... extra) { + return make_key_iterator_ng(std::begin(value), std::end(value), std::forward(extra)...); +} + /// Makes an iterator over values of an stl container or other container supporting /// `std::begin()`/`std::end()` template -detail::iterator_state())), decltype(std::end(std::declval())), false, Policy> -make_iterator(Type &value, Extra&&... extra) { - return make_iterator(std::begin(value), std::end(value), extra...); +auto make_iterator(Type &value, Extra&&... extra) { + return cast(make_iterator_ng(value, std::forward(extra)...)); } /// Makes an iterator over the keys (`.first`) of a stl map-like container supporting /// `std::begin()`/`std::end()` template -detail::iterator_state())), decltype(std::end(std::declval())), true, Policy> -make_key_iterator(Type &value, Extra&&... extra) { - return make_key_iterator(std::begin(value), std::end(value), extra...); +auto make_key_iterator(Type &value, Extra&&... extra) { + return cast(make_key_iterator(value, std::forward(extra)...)); } + template void implicitly_convertible() { struct set_flag { bool &flag; diff --git a/include/pybind11/stl_bind.h b/include/pybind11/stl_bind.h index 61b94b6220..fa319199c0 100644 --- a/include/pybind11/stl_bind.h +++ b/include/pybind11/stl_bind.h @@ -313,7 +313,7 @@ void vector_accessor(enable_if_t::value, Class_> &cl) cl.def("__iter__", [](Vector &v) { - return make_iterator< + return make_iterator_ng< return_value_policy::reference_internal, ItType, ItType, T&>( v.begin(), v.end()); }, @@ -340,7 +340,7 @@ void vector_accessor(enable_if_t::value, Class_> &cl) cl.def("__iter__", [](Vector &v) { - return make_iterator< + return make_iterator_ng< return_value_policy::copy, ItType, ItType, T>( v.begin(), v.end()); }, @@ -608,12 +608,12 @@ class_ bind_map(handle scope, const std::string &name, Args&&. ); cl.def("__iter__", - [](Map &m) { return make_key_iterator(m.begin(), m.end()); }, + [](Map &m) { return make_key_iterator_ng(m.begin(), m.end()); }, keep_alive<0, 1>() /* Essential: keep list alive while iterator exists */ ); cl.def("items", - [](Map &m) { return make_iterator(m.begin(), m.end()); }, + [](Map &m) { return make_iterator_ng(m.begin(), m.end()); }, keep_alive<0, 1>() /* Essential: keep list alive while iterator exists */ ); From 0e360b73fb053b03d2918d2b563458cb027436f7 Mon Sep 17 00:00:00 2001 From: Sergei Izmailov Date: Fri, 7 Aug 2020 14:07:45 +0300 Subject: [PATCH 3/8] Revert `iterator_passthrough` test change --- tests/test_sequences_and_iterators.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_sequences_and_iterators.cpp b/tests/test_sequences_and_iterators.cpp index 70999ffd84..05f999bb3b 100644 --- a/tests/test_sequences_and_iterators.cpp +++ b/tests/test_sequences_and_iterators.cpp @@ -344,7 +344,7 @@ TEST_SUBMODULE(sequences_and_iterators, m) { // test_iterator_passthrough // #181: iterator passthrough did not compile - m.def("iterator_passthrough", [](py::iterator s) { + m.def("iterator_passthrough", [](py::iterator s) -> py::iterator { return py::make_iterator(std::begin(s), std::end(s)); }); From 036add2b022649273957e708dda9625b57118e44 Mon Sep 17 00:00:00 2001 From: Sergei Izmailov Date: Fri, 7 Aug 2020 14:18:46 +0300 Subject: [PATCH 4/8] Use make_iterator_ng/make_key_iterator_ng in tests --- tests/test_opaque_types.cpp | 2 +- tests/test_sequences_and_iterators.cpp | 14 +++++++------- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/tests/test_opaque_types.cpp b/tests/test_opaque_types.cpp index 0d20d9a01c..499da18b8d 100644 --- a/tests/test_opaque_types.cpp +++ b/tests/test_opaque_types.cpp @@ -30,7 +30,7 @@ TEST_SUBMODULE(opaque_types, m) { .def("back", (std::string &(StringList::*)()) &StringList::back) .def("__len__", [](const StringList &v) { return v.size(); }) .def("__iter__", [](StringList &v) { - return py::make_iterator(v.begin(), v.end()); + return py::make_iterator_ng(v.begin(), v.end()); }, py::keep_alive<0, 1>()); class ClassWithSTLVecProperty { diff --git a/tests/test_sequences_and_iterators.cpp b/tests/test_sequences_and_iterators.cpp index 05f999bb3b..d7db2a4d88 100644 --- a/tests/test_sequences_and_iterators.cpp +++ b/tests/test_sequences_and_iterators.cpp @@ -189,7 +189,7 @@ TEST_SUBMODULE(sequences_and_iterators, m) { }) .def("__len__", &Sequence::size) /// Optional sequence protocol operations - .def("__iter__", [](const Sequence &s) { return py::make_iterator(s.begin(), s.end()); }, + .def("__iter__", [](const Sequence &s) { return py::make_iterator_ng(s.begin(), s.end()); }, py::keep_alive<0, 1>() /* Essential: keep object alive while iterator exists */) .def("__contains__", [](const Sequence &s, float v) { return s.contains(v); }) .def("__reversed__", [](const Sequence &s) -> Sequence { return s.reversed(); }) @@ -249,9 +249,9 @@ TEST_SUBMODULE(sequences_and_iterators, m) { }) .def("__setitem__", &StringMap::set) .def("__len__", &StringMap::size) - .def("__iter__", [](const StringMap &map) { return py::make_key_iterator(map.begin(), map.end()); }, + .def("__iter__", [](const StringMap &map) { return py::make_key_iterator_ng(map.begin(), map.end()); }, py::keep_alive<0, 1>()) - .def("items", [](const StringMap &map) { return py::make_iterator(map.begin(), map.end()); }, + .def("items", [](const StringMap &map) { return py::make_iterator_ng(map.begin(), map.end()); }, py::keep_alive<0, 1>()) ; @@ -266,10 +266,10 @@ TEST_SUBMODULE(sequences_and_iterators, m) { py::class_(m, "IntPairs") .def(py::init>>()) .def("nonzero", [](const IntPairs& s) { - return py::make_iterator(NonZeroIterator>(s.begin()), NonZeroSentinel()); + return py::make_iterator_ng(NonZeroIterator>(s.begin()), NonZeroSentinel()); }, py::keep_alive<0, 1>()) .def("nonzero_keys", [](const IntPairs& s) { - return py::make_key_iterator(NonZeroIterator>(s.begin()), NonZeroSentinel()); + return py::make_key_iterator_ng(NonZeroIterator>(s.begin()), NonZeroSentinel()); }, py::keep_alive<0, 1>()) ; @@ -351,6 +351,6 @@ TEST_SUBMODULE(sequences_and_iterators, m) { // test_iterator_rvp // #388: Can't make iterators via make_iterator() with different r/v policies static std::vector list = { 1, 2, 3 }; - m.def("make_iterator_1", []() { return py::make_iterator(list); }); - m.def("make_iterator_2", []() { return py::make_iterator(list); }); + m.def("make_iterator_1", []() { return py::make_iterator_ng(list); }); + m.def("make_iterator_2", []() { return py::make_iterator_ng(list); }); } From 96bdc5fc0e5b37a6bc8499bf5b198436c2386f4c Mon Sep 17 00:00:00 2001 From: Sergei Izmailov Date: Fri, 7 Aug 2020 15:05:09 +0300 Subject: [PATCH 5/8] Use concrete return types instead of auto --- include/pybind11/pybind11.h | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index 0feba5e810..c5b87a3b0c 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -1795,7 +1795,7 @@ template ()), typename... Extra> [[deprecated("Superseded by make_iterator_ng")]] -auto make_iterator(Iterator first, Sentinel last, Extra &&... extra) { +iterator make_iterator(Iterator first, Sentinel last, Extra &&... extra) { return cast(make_iterator_ng(first, last, std::forward(extra)...)); } @@ -1806,21 +1806,23 @@ template ()), typename... Extra> [[deprecated("Superseded by make_key_iterator_ng")]] -auto make_key_iterator(Iterator first, Sentinel last, Extra &&... extra) { +iterator make_key_iterator(Iterator first, Sentinel last, Extra &&... extra) { return cast(make_key_iterator_ng(first, last, std::forward(extra)...)); } template -auto make_iterator_ng(Type &value, Extra&&... extra) { +detail::iterator_state())), decltype(std::end(std::declval())), false, Policy> +make_iterator_ng(Type &value, Extra &&... extra) { return make_iterator_ng(std::begin(value), std::end(value), std::forward(extra)...); } /// Makes an iterator over the keys (`.first`) of a stl map-like container supporting /// `std::begin()`/`std::end()` -template -auto make_key_iterator_ng(Type &value, Extra&&... extra) { +detail::iterator_state())), decltype(std::end(std::declval())), true, Policy> +make_key_iterator_ng(Type &value, Extra &&... extra) { return make_key_iterator_ng(std::begin(value), std::end(value), std::forward(extra)...); } @@ -1828,7 +1830,7 @@ auto make_key_iterator_ng(Type &value, Extra&&... extra) { /// `std::begin()`/`std::end()` template -auto make_iterator(Type &value, Extra&&... extra) { +iterator make_iterator(Type &value, Extra&&... extra) { return cast(make_iterator_ng(value, std::forward(extra)...)); } @@ -1836,7 +1838,7 @@ auto make_iterator(Type &value, Extra&&... extra) { /// `std::begin()`/`std::end()` template -auto make_key_iterator(Type &value, Extra&&... extra) { +iterator make_key_iterator(Type &value, Extra&&... extra) { return cast(make_key_iterator(value, std::forward(extra)...)); } From b1dbc30f0d4dec857d3b8abe1c619644012da1be Mon Sep 17 00:00:00 2001 From: Sergei Izmailov Date: Fri, 7 Aug 2020 15:10:29 +0300 Subject: [PATCH 6/8] fix: typo --- include/pybind11/pybind11.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index c5b87a3b0c..4b8d2e121d 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -1737,7 +1737,7 @@ template ()), typename... Extra> detail::iterator_state -[[deprecated]] make_iterator_ng(Iterator first, Sentinel last, Extra &&... extra) { +make_iterator_ng(Iterator first, Sentinel last, Extra &&... extra) { typedef detail::iterator_state state; if (!detail::get_type_info(typeid(state), false)) { From 98763aae85cc3ede888769a2c459c2758cbe815f Mon Sep 17 00:00:00 2001 From: Sergei Izmailov Date: Fri, 7 Aug 2020 15:15:07 +0300 Subject: [PATCH 7/8] Avoid deprecation warning in iterator_passthrough --- tests/test_sequences_and_iterators.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_sequences_and_iterators.cpp b/tests/test_sequences_and_iterators.cpp index d7db2a4d88..7a01d32287 100644 --- a/tests/test_sequences_and_iterators.cpp +++ b/tests/test_sequences_and_iterators.cpp @@ -345,7 +345,7 @@ TEST_SUBMODULE(sequences_and_iterators, m) { // test_iterator_passthrough // #181: iterator passthrough did not compile m.def("iterator_passthrough", [](py::iterator s) -> py::iterator { - return py::make_iterator(std::begin(s), std::end(s)); + return py::cast(py::make_iterator_ng(std::begin(s), std::end(s))); }); // test_iterator_rvp From c5500d835f0712eeb378b9f1f2fcdacc0f63742a Mon Sep 17 00:00:00 2001 From: Sergei Izmailov Date: Fri, 7 Aug 2020 15:28:17 +0300 Subject: [PATCH 8/8] Make use of PYBIND11_DEPRECATED macro --- 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 4b8d2e121d..a1f7759fca 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -1794,7 +1794,7 @@ template ()), typename... Extra> -[[deprecated("Superseded by make_iterator_ng")]] +PYBIND11_DEPRECATED("Superseded by make_iterator_ng") iterator make_iterator(Iterator first, Sentinel last, Extra &&... extra) { return cast(make_iterator_ng(first, last, std::forward(extra)...)); } @@ -1805,7 +1805,7 @@ template ()), typename... Extra> -[[deprecated("Superseded by make_key_iterator_ng")]] +PYBIND11_DEPRECATED("Superseded by make_key_iterator_ng") iterator make_key_iterator(Iterator first, Sentinel last, Extra &&... extra) { return cast(make_key_iterator_ng(first, last, std::forward(extra)...)); } @@ -1830,6 +1830,7 @@ make_key_iterator_ng(Type &value, Extra &&... extra) { /// `std::begin()`/`std::end()` template +PYBIND11_DEPRECATED("Superseded by make_iterator_ng") iterator make_iterator(Type &value, Extra&&... extra) { return cast(make_iterator_ng(value, std::forward(extra)...)); } @@ -1838,6 +1839,7 @@ iterator make_iterator(Type &value, Extra&&... extra) { /// `std::begin()`/`std::end()` template +PYBIND11_DEPRECATED("Superseded by make_key_iterator_ng") iterator make_key_iterator(Type &value, Extra&&... extra) { return cast(make_key_iterator(value, std::forward(extra)...)); }