From 7f4bae152b9680fcaa4e33954e6db18d6879190a Mon Sep 17 00:00:00 2001 From: Joshua Oreman Date: Wed, 13 Nov 2024 16:26:31 +0800 Subject: [PATCH] make_iterator: accept args by value, not forwarding reference, to reduce dangling risk --- docs/api_extra.rst | 6 +++--- docs/changelog.rst | 5 +++++ include/nanobind/make_iterator.h | 16 +++++++++------- tests/test_make_iterator.cpp | 15 +++++++++++++++ tests/test_make_iterator.py | 2 ++ tests/test_make_iterator_ext.pyi.ref | 4 ++++ 6 files changed, 38 insertions(+), 10 deletions(-) diff --git a/docs/api_extra.rst b/docs/api_extra.rst index 9a523830..903aa586 100644 --- a/docs/api_extra.rst +++ b/docs/api_extra.rst @@ -549,7 +549,7 @@ include directive: #include -.. cpp:function:: template auto make_iterator(handle scope, const char * name, Iterator &&first, Iterator &&last, Extra &&...extra) +.. cpp:function:: template auto make_iterator(handle scope, const char * name, Iterator first, Sentinel last, Extra &&...extra) Create a Python iterator wrapping the C++ iterator represented by the range ``[first, last)``. The `Extra` parameter can be used to pass additional @@ -619,7 +619,7 @@ include directive: ``first`` and ``last`` set to ``std::begin(value)`` and ``std::end(value)``, respectively. -.. cpp:function:: template iterator make_key_iterator(handle scope, const char * name, Iterator &&first, Iterator &&last, Extra &&...extra) +.. cpp:function:: template iterator make_key_iterator(handle scope, const char * name, Iterator first, Sentinel last, Extra &&...extra) :cpp:func:`make_iterator` specialization for C++ iterators that return key-value pairs. `make_key_iterator` returns the first pair element to @@ -630,7 +630,7 @@ include directive: ``(*first).first``. -.. cpp:function:: template iterator make_value_iterator(handle scope, const char * name, Iterator &&first, Iterator &&last, Extra &&...extra) +.. cpp:function:: template iterator make_value_iterator(handle scope, const char * name, Iterator first, Sentinel last, Extra &&...extra) :cpp:func:`make_iterator` specialization for C++ iterators that return key-value pairs. `make_value_iterator` returns the second pair element to diff --git a/docs/changelog.rst b/docs/changelog.rst index d5ffba74..7919691a 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -33,6 +33,11 @@ Version TBD (unreleased) documentation for more details, important caveats, and an example policy. (PR `#767 `__) +- :cpp:func:`make_iterator` now accepts its iterator arguments by value, + rather than by forwarding reference, in order to eliminate the hazard + of storing a dangling C++ iterator reference in the returned Python + iterator object. (PR `#788 `__) + Version 2.2.0 (October 3, 2024) ------------------------------- diff --git a/include/nanobind/make_iterator.h b/include/nanobind/make_iterator.h index 9d54689e..22e125ff 100644 --- a/include/nanobind/make_iterator.h +++ b/include/nanobind/make_iterator.h @@ -59,7 +59,7 @@ template struct iterator_value_access { template typed make_iterator_impl(handle scope, const char *name, - Iterator &&first, Sentinel &&last, + Iterator first, Sentinel last, Extra &&...extra) { using State = iterator_state; @@ -103,8 +103,9 @@ template ::result_type, - typename... Extra> -auto make_iterator(handle scope, const char *name, Iterator &&first, Sentinel &&last, Extra &&...extra) { + typename... Extra, + typename = decltype(std::declval() == std::declval())> +auto make_iterator(handle scope, const char *name, Iterator first, Sentinel last, Extra &&...extra) { return detail::make_iterator_impl, Policy, Iterator, Sentinel, ValueType, Extra...>( scope, name, std::forward(first), @@ -118,8 +119,8 @@ template ::result_type, typename... Extra> -auto make_key_iterator(handle scope, const char *name, Iterator &&first, - Sentinel &&last, Extra &&...extra) { +auto make_key_iterator(handle scope, const char *name, Iterator first, + Sentinel last, Extra &&...extra) { return detail::make_iterator_impl, Policy, Iterator, Sentinel, KeyType, Extra...>( @@ -134,7 +135,7 @@ template ::result_type, typename... Extra> -auto make_value_iterator(handle scope, const char *name, Iterator &&first, Sentinel &&last, Extra &&...extra) { +auto make_value_iterator(handle scope, const char *name, Iterator first, Sentinel last, Extra &&...extra) { return detail::make_iterator_impl, Policy, Iterator, Sentinel, ValueType, Extra...>( @@ -145,7 +146,8 @@ auto make_value_iterator(handle scope, const char *name, Iterator &&first, Senti /// Makes an iterator over values of a container supporting `std::begin()`/`std::end()` template + typename... Extra, + typename = decltype(std::begin(std::declval()))> auto make_iterator(handle scope, const char *name, Type &value, Extra &&...extra) { return make_iterator(scope, name, std::begin(value), std::end(value), diff --git a/tests/test_make_iterator.cpp b/tests/test_make_iterator.cpp index fe149e9e..79d07df7 100644 --- a/tests/test_make_iterator.cpp +++ b/tests/test_make_iterator.cpp @@ -28,6 +28,14 @@ NB_MODULE(test_make_iterator_ext, m) { map.begin(), map.end()); }, nb::keep_alive<0, 1>()) + .def("items_l", + [](const StringMap &map) { + // Make sure iterators don't dangle even if passed as lvalue + auto begin = map.begin(), end = map.end(); + return nb::make_iterator(nb::type(), + "item_iterator_l", + begin, end); + }, nb::keep_alive<0, 1>()) .def("values", [](const StringMap &map) { return nb::make_value_iterator(nb::type(), "value_iterator", @@ -72,6 +80,13 @@ NB_MODULE(test_make_iterator_ext, m) { map.begin(), map.end()); }, nb::keep_alive<0, 1>()) + .def("items_l", + [](const IdentityMap &map) { + auto begin = map.begin(), end = map.end(); + return nb::make_iterator(nb::type(), + "item_iterator_l", + begin, end); + }, nb::keep_alive<0, 1>()) .def("values", [](const IdentityMap &map) { return nb::make_value_iterator(nb::type(), "value_iterator", diff --git a/tests/test_make_iterator.py b/tests/test_make_iterator.py index 5d45f37b..ebda4dfc 100644 --- a/tests/test_make_iterator.py +++ b/tests/test_make_iterator.py @@ -27,6 +27,7 @@ def test03_items_iterator(): for d in data: m = t.StringMap(d) assert sorted(list(m.items())) == sorted(list(d.items())) + assert sorted(list(m.items_l())) == sorted(list(d.items())) def test04_passthrough_iterator(): @@ -40,3 +41,4 @@ def test05_iterator_returning_temporary(): assert list(im) == list(range(10)) assert list(im.values()) == list(range(10)) assert list(im.items()) == list(zip(range(10), range(10))) + assert list(im.items_l()) == list(zip(range(10), range(10))) diff --git a/tests/test_make_iterator_ext.pyi.ref b/tests/test_make_iterator_ext.pyi.ref index 80d9fb88..e89da22f 100644 --- a/tests/test_make_iterator_ext.pyi.ref +++ b/tests/test_make_iterator_ext.pyi.ref @@ -9,6 +9,8 @@ class IdentityMap: def items(self) -> Iterator[tuple[int, int]]: ... + def items_l(self) -> Iterator[tuple[int, int]]: ... + def values(self) -> Iterator[int]: ... class StringMap: @@ -22,6 +24,8 @@ class StringMap: def items(self) -> Iterator[tuple[str, str]]: ... + def items_l(self) -> Iterator[tuple[str, str]]: ... + def values(self) -> Iterator[str]: ... __all__: list = ['iterator_passthrough', 'StringMap', 'IdentityMap']