feat: reapply fixed version of #3271 (#3293)

* Add make_value_iterator (#3271)

* Add make_value_iterator

This is the counterpart to make_key_iterator, and will allow
implementing a `value` method in `bind_map` (although doing so is left
for a subsequent PR).

I made a few design changes to reduce copy-and-paste boilerplate.
Previously detail::iterator_state had a boolean template parameter to
indicate whether it was being used for make_iterator or
make_key_iterator. I replaced the boolean with a class that determines
how to dereference the iterator. This allows for a generic
implementation of `__next__`.

I also added the ValueType and Extra... parameters to the iterator_state
template args, because I think it was a bug that they were missing: if
make_iterator is called twice with different values of these, only the
first set has effect (because the state class is only registered once).
There is still a potential issue in that the *values* of the extra
arguments are latched on the first call, but since most policies are
empty classes this should be even less common.

* Add some remove_cv_t to appease clang-tidy

* Make iterator_access and friends take reference

For some reason I'd accidentally made it take a const value, which
caused some issues with third-party packages.

* Another attempt to remove remove_cv_t from iterators

Some of the return types were const (non-reference) types because of the
pecularities of decltype: `decltype((*it).first)` is the *declared* type
of the member of the pair, rather than the type of the expression. So if
the reference type of the iterator is `pair<const int, int> &`, then the
decltype is `const int`. Wrapping an extra set of parentheses to form
`decltype(((*it).first))` would instead give `const int &`.

This means that the existing make_key_iterator actually returns by value
from `__next__`, rather than by reference. Since for mapping types, keys
are always const, this probably hasn't been noticed, but it will affect
make_value_iterator if the Python code tries to mutate the returned
objects. I've changed things to use double parentheses so that
make_iterator, make_key_iterator and make_value_iterator should now all
return the reference type of the iterator. I'll still need to add a test
for that; for now I'm just checking whether I can keep Clang-Tidy happy.

* Add back some NOLINTNEXTLINE to appease Clang-Tidy

This is favoured over using remove_cv_t because in some cases a const
value return type is deliberate (particularly for Eigen).

* Add a unit test for iterator referencing

Ensure that make_iterator, make_key_iterator and make_value_iterator
return references to the container elements, rather than copies. The
test for make_key_iterator fails to compile on master, which gives me
confidence that this branch has fixed it.

* Make the iterator_access etc operator() const

I'm actually a little surprised it compiled at all given that the
operator() is called on a temporary, but I don't claim to fully
understand all the different value types in C++11.

* Attempt to work around compiler bugs

https://godbolt.org/ shows an example where ICC gets the wrong result
for a decltype used as the default for a template argument, and CI also
showed problems with PGI. This is a shot in the dark to see if it fixes
things.

* Make a test constructor explicit (Clang-Tidy)

* Fix unit test on GCC 4.8.5

It seems to require the arguments to the std::pair constructor to be
implicitly convertible to the types in the pair, rather than just
requiring is_constructible.

* Remove DOXYGEN_SHOULD_SKIP_THIS guards

Now that a complex decltype expression has been replaced by a simpler
nested type, I'm hoping Doxygen will be able to build it without issues.

* Add comment to explain iterator_state template params

* fix: regression in #3271

Co-authored-by: Bruce Merry <[email protected]>

Author: henryiii

docs/reference.rst

@@ -63,6 +63,9 @@ Convenience functions converting to Python types
 .. doxygenfunction:: make_key_iterator(Iterator, Sentinel, Extra &&...)
 .. doxygenfunction:: make_key_iterator(Type &, Extra&&...)
 
+.. doxygenfunction:: make_value_iterator(Iterator, Sentinel, Extra &&...)
+.. doxygenfunction:: make_value_iterator(Type &, Extra&&...)
+
 .. _extras:
 
 Passing extra arguments to ``def`` or ``class_``

include/pybind11/pybind11.h

@@ -1955,25 +1955,54 @@ inline std::pair<decltype(internals::registered_types_py)::iterator, bool> all_t
     return res;
 }
 
-template <typename Iterator, typename Sentinel, bool KeyIterator, return_value_policy Policy>
+/* There are a large number of apparently unused template arguments because
+ * each combination requires a separate py::class_ registration.
+ */
+template <typename Access, return_value_policy Policy, typename Iterator, typename Sentinel, typename ValueType, typename... Extra>
 struct iterator_state {
     Iterator it;
     Sentinel end;
     bool first_or_done;
 };
 
-PYBIND11_NAMESPACE_END(detail)
+// Note: these helpers take the iterator by non-const reference because some
+// iterators in the wild can't be dereferenced when const. C++ needs the extra parens in decltype
+// to enforce an lvalue. The & after Iterator is required for MSVC < 16.9. SFINAE cannot be
+// reused for result_type due to bugs in ICC, NVCC, and PGI compilers. See PR #3293.
+template <typename Iterator, typename SFINAE = decltype((*std::declval<Iterator &>()))>
+struct iterator_access {
+    using result_type = decltype((*std::declval<Iterator &>()));
+    // NOLINTNEXTLINE(readability-const-return-type) // PR #3263
+    result_type operator()(Iterator &it) const {
+        return *it;
+    }
+};
 
-/// Makes a python iterator from a first and past-the-end C++ InputIterator.
-template <return_value_policy Policy = return_value_policy::reference_internal,
+template <typename Iterator, typename SFINAE = decltype(((*std::declval<Iterator &>()).first)) >
+struct iterator_key_access {
+    using result_type = decltype(((*std::declval<Iterator &>()).first));
+    result_type operator()(Iterator &it) const {
+        return (*it).first;
+    }
+};
+
+template <typename Iterator, typename SFINAE = decltype(((*std::declval<Iterator &>()).second))>
+struct iterator_value_access {
+    using result_type = decltype(((*std::declval<Iterator &>()).second));
+    result_type operator()(Iterator &it) const {
+        return (*it).second;
+    }
+};
+
+template <typename Access,
+          return_value_policy Policy,
           typename Iterator,
           typename Sentinel,
-#ifndef DOXYGEN_SHOULD_SKIP_THIS  // Issue in breathe 4.26.1
-          typename ValueType = decltype(*std::declval<Iterator>()),
-#endif
+          typename ValueType,
           typename... Extra>
-iterator make_iterator(Iterator first, Sentinel last, Extra &&... extra) {
-    using state = detail::iterator_state<Iterator, Sentinel, false, Policy>;
+iterator make_iterator_impl(Iterator first, Sentinel last, Extra &&... extra) {
+    using state = detail::iterator_state<Access, Policy, Iterator, Sentinel, ValueType, Extra...>;
+    // TODO: state captures only the types of Extra, not the values
 
     if (!detail::get_type_info(typeid(state), false)) {
         class_<state>(handle(), "iterator", pybind11::module_local())
@@ -1987,43 +2016,63 @@ iterator make_iterator(Iterator first, Sentinel last, Extra &&... extra) {
                     s.first_or_done = true;
                     throw stop_iteration();
                 }
-                return *s.it;
+                return Access()(s.it);
             // NOLINTNEXTLINE(readability-const-return-type) // PR #3263
             }, std::forward<Extra>(extra)..., Policy);
     }
 
     return cast(state{first, last, true});
 }
 
-/// Makes an python iterator over the keys (`.first`) of a iterator over pairs from a
+PYBIND11_NAMESPACE_END(detail)
+
+/// Makes a python iterator from a first and past-the-end C++ InputIterator.
+template <return_value_policy Policy = return_value_policy::reference_internal,
+          typename Iterator,
+          typename Sentinel,
+          typename ValueType = typename detail::iterator_access<Iterator>::result_type,
+          typename... Extra>
+iterator make_iterator(Iterator first, Sentinel last, Extra &&... extra) {
+    return detail::make_iterator_impl<
+        detail::iterator_access<Iterator>,
+        Policy,
+        Iterator,
+        Sentinel,
+        ValueType,
+        Extra...>(first, last, std::forward<Extra>(extra)...);
+}
+
+/// Makes a python iterator over the keys (`.first`) of a iterator over pairs from a
 /// first and past-the-end InputIterator.
 template <return_value_policy Policy = return_value_policy::reference_internal,
           typename Iterator,
           typename Sentinel,
-#ifndef DOXYGEN_SHOULD_SKIP_THIS  // Issue in breathe 4.26.1
-          typename KeyType = decltype((*std::declval<Iterator>()).first),
-#endif
+          typename KeyType = typename detail::iterator_key_access<Iterator>::result_type,
           typename... Extra>
 iterator make_key_iterator(Iterator first, Sentinel last, Extra &&...extra) {
-    using state = detail::iterator_state<Iterator, Sentinel, true, Policy>;
-
-    if (!detail::get_type_info(typeid(state), false)) {
-        class_<state>(handle(), "iterator", pybind11::module_local())
-            .def("__iter__", [](state &s) -> state& { return s; })
-            .def("__next__", [](state &s) -> detail::remove_cv_t<KeyType> {
-                if (!s.first_or_done)
-                    ++s.it;
-                else
-                    s.first_or_done = false;
-                if (s.it == s.end) {
-                    s.first_or_done = true;
-                    throw stop_iteration();
-                }
-                return (*s.it).first;
-            }, std::forward<Extra>(extra)..., Policy);
-    }
+    return detail::make_iterator_impl<
+        detail::iterator_key_access<Iterator>,
+        Policy,
+        Iterator,
+        Sentinel,
+        KeyType,
+        Extra...>(first, last, std::forward<Extra>(extra)...);
+}
 
-    return cast(state{first, last, true});
+/// Makes a python iterator over the values (`.second`) of a iterator over pairs from a
+/// first and past-the-end InputIterator.
+template <return_value_policy Policy = return_value_policy::reference_internal,
+          typename Iterator,
+          typename Sentinel,
+          typename ValueType = typename detail::iterator_value_access<Iterator>::result_type,
+          typename... Extra>
+iterator make_value_iterator(Iterator first, Sentinel last, Extra &&...extra) {
+    return detail::make_iterator_impl<
+        detail::iterator_value_access<Iterator>,
+        Policy, Iterator,
+        Sentinel,
+        ValueType,
+        Extra...>(first, last, std::forward<Extra>(extra)...);
 }
 
 /// Makes an iterator over values of an stl container or other container supporting
@@ -2040,6 +2089,13 @@ template <return_value_policy Policy = return_value_policy::reference_internal,
     return make_key_iterator<Policy>(std::begin(value), std::end(value), extra...);
 }
 
+/// Makes an iterator over the values (`.second`) of a stl map-like container supporting
+/// `std::begin()`/`std::end()`
+template <return_value_policy Policy = return_value_policy::reference_internal,
+          typename Type, typename... Extra> iterator make_value_iterator(Type &value, Extra&&... extra) {
+    return make_value_iterator<Policy>(std::begin(value), std::end(value), extra...);
+}
+
 template <typename InputType, typename OutputType> void implicitly_convertible() {
     struct set_flag {
         bool &flag;

tests/test_sequences_and_iterators.cpp

@@ -15,6 +15,7 @@
 
 #include <algorithm>
 #include <utility>
+#include <vector>
 
 #ifdef PYBIND11_HAS_OPTIONAL
 #include <optional>
@@ -37,6 +38,29 @@ bool operator==(const NonZeroIterator<std::pair<A, B>>& it, const NonZeroSentine
     return !(*it).first || !(*it).second;
 }
 
+class NonCopyableInt {
+public:
+    explicit NonCopyableInt(int value) : value_(value) {}
+    NonCopyableInt(const NonCopyableInt &) = delete;
+    NonCopyableInt(NonCopyableInt &&other) noexcept : value_(other.value_) {
+        other.value_ = -1;  // detect when an unwanted move occurs
+    }
+    NonCopyableInt &operator=(const NonCopyableInt &) = delete;
+    NonCopyableInt &operator=(NonCopyableInt &&other) noexcept {
+        value_ = other.value_;
+        other.value_ = -1;  // detect when an unwanted move occurs
+        return *this;
+    }
+    int get() const { return value_; }
+    void set(int value) { value_ = value; }
+    ~NonCopyableInt() = default;
+private:
+    int value_;
+};
+using NonCopyableIntPair = std::pair<NonCopyableInt, NonCopyableInt>;
+PYBIND11_MAKE_OPAQUE(std::vector<NonCopyableInt>);
+PYBIND11_MAKE_OPAQUE(std::vector<NonCopyableIntPair>);
+
 template <typename PythonType>
 py::list test_random_access_iterator(PythonType x) {
     if (x.size() < 5)
@@ -288,6 +312,10 @@ TEST_SUBMODULE(sequences_and_iterators, m) {
         .def(
             "items",
             [](const StringMap &map) { return py::make_iterator(map.begin(), map.end()); },
+            py::keep_alive<0, 1>())
+        .def(
+            "values",
+            [](const StringMap &map) { return py::make_value_iterator(map.begin(), map.end()); },
             py::keep_alive<0, 1>());
 
     // test_generalized_iterators
@@ -308,19 +336,62 @@ TEST_SUBMODULE(sequences_and_iterators, m) {
         .def("nonzero_keys", [](const IntPairs& s) {
             return py::make_key_iterator(NonZeroIterator<std::pair<int, int>>(s.begin()), NonZeroSentinel());
         }, py::keep_alive<0, 1>())
+        .def("nonzero_values", [](const IntPairs& s) {
+            return py::make_value_iterator(NonZeroIterator<std::pair<int, int>>(s.begin()), NonZeroSentinel());
+        }, py::keep_alive<0, 1>())
+
+        // test single-argument make_iterator
         .def("simple_iterator", [](IntPairs& self) {
             return py::make_iterator(self);
         }, py::keep_alive<0, 1>())
         .def("simple_keys", [](IntPairs& self) {
             return py::make_key_iterator(self);
         }, py::keep_alive<0, 1>())
+        .def("simple_values", [](IntPairs& self) {
+            return py::make_value_iterator(self);
+        }, py::keep_alive<0, 1>())
 
-        // test iterator with keep_alive (doesn't work so not used at runtime, but tests compile)
-        .def("make_iterator_keep_alive", [](IntPairs& self) {
-            return py::make_iterator(self, py::keep_alive<0, 1>());
+        // Test iterator with an Extra (doesn't do anything useful, so not used
+        // at runtime, but tests need to be able to compile with the correct
+        // overload. See PR #3293.
+        .def("_make_iterator_extras", [](IntPairs& self) {
+            return py::make_iterator(self, py::call_guard<int>());
+        }, py::keep_alive<0, 1>())
+        .def("_make_key_extras", [](IntPairs& self) {
+            return py::make_key_iterator(self, py::call_guard<int>());
+        }, py::keep_alive<0, 1>())
+        .def("_make_value_extras", [](IntPairs& self) {
+            return py::make_value_iterator(self, py::call_guard<int>());
         }, py::keep_alive<0, 1>())
         ;
 
+    // test_iterater_referencing
+    py::class_<NonCopyableInt>(m, "NonCopyableInt")
+        .def(py::init<int>())
+        .def("set", &NonCopyableInt::set)
+        .def("__int__", &NonCopyableInt::get)
+        ;
+    py::class_<std::vector<NonCopyableInt>>(m, "VectorNonCopyableInt")
+        .def(py::init<>())
+        .def("append", [](std::vector<NonCopyableInt> &vec, int value) {
+            vec.emplace_back(value);
+        })
+        .def("__iter__", [](std::vector<NonCopyableInt> &vec) {
+            return py::make_iterator(vec.begin(), vec.end());
+        })
+        ;
+    py::class_<std::vector<NonCopyableIntPair>>(m, "VectorNonCopyableIntPair")
+        .def(py::init<>())
+        .def("append", [](std::vector<NonCopyableIntPair> &vec, const std::pair<int, int> &value) {
+            vec.emplace_back(NonCopyableInt(value.first), NonCopyableInt(value.second));
+        })
+        .def("keys", [](std::vector<NonCopyableIntPair> &vec) {
+            return py::make_key_iterator(vec.begin(), vec.end());
+        })
+        .def("values", [](std::vector<NonCopyableIntPair> &vec) {
+            return py::make_value_iterator(vec.begin(), vec.end());
+        })
+        ;
 
 #if 0
     // Obsolete: special data structure for exposing custom iterator types to python

tests/test_sequences_and_iterators.py

@@ -36,6 +36,10 @@ def test_generalized_iterators():
     assert list(m.IntPairs([(1, 2), (2, 0), (0, 3), (4, 5)]).nonzero_keys()) == [1]
     assert list(m.IntPairs([(0, 3), (1, 2), (3, 4)]).nonzero_keys()) == []
 
+    assert list(m.IntPairs([(1, 2), (3, 4), (0, 5)]).nonzero_values()) == [2, 4]
+    assert list(m.IntPairs([(1, 2), (2, 0), (0, 3), (4, 5)]).nonzero_values()) == [2]
+    assert list(m.IntPairs([(0, 3), (1, 2), (3, 4)]).nonzero_values()) == []
+
     # __next__ must continue to raise StopIteration
     it = m.IntPairs([(0, 0)]).nonzero()
     for _ in range(3):
@@ -55,6 +59,31 @@ def test_generalized_iterators_simple():
         (0, 5),
     ]
     assert list(m.IntPairs([(1, 2), (3, 4), (0, 5)]).simple_keys()) == [1, 3, 0]
+    assert list(m.IntPairs([(1, 2), (3, 4), (0, 5)]).simple_values()) == [2, 4, 5]
+
+
+def test_iterator_referencing():
+    """Test that iterators reference rather than copy their referents."""
+    vec = m.VectorNonCopyableInt()
+    vec.append(3)
+    vec.append(5)
+    assert [int(x) for x in vec] == [3, 5]
+    # Increment everything to make sure the referents can be mutated
+    for x in vec:
+        x.set(int(x) + 1)
+    assert [int(x) for x in vec] == [4, 6]
+
+    vec = m.VectorNonCopyableIntPair()
+    vec.append([3, 4])
+    vec.append([5, 7])
+    assert [int(x) for x in vec.keys()] == [3, 5]
+    assert [int(x) for x in vec.values()] == [4, 7]
+    for x in vec.keys():
+        x.set(int(x) + 1)
+    for x in vec.values():
+        x.set(int(x) + 10)
+    assert [int(x) for x in vec.keys()] == [4, 6]
+    assert [int(x) for x in vec.values()] == [14, 17]
 
 
 def test_sliceable():
@@ -160,6 +189,7 @@ def test_map_iterator():
         assert sm[k] == expected[k]
     for k, v in sm.items():
         assert v == expected[k]
+    assert list(sm.values()) == [expected[k] for k in sm]
 
     it = iter(m.StringMap({}))
     for _ in range(3):  # __next__ must continue to raise StopIteration