Follow-on to PR #3254, to address user code breakages. (#3263)

* Restoring `const` removed from pytypes.h in PR #3254, adding tests reflective of user code that breaks when those `const` are removed.

* clang-tidy NOLINTs (and one collateral fix).

* Inserting PYBIND11_CONST_FOR_STRICT_PLATFORMS

* Trying `defined(__APPLE__)`

* Trying again: `auto it` for strict platforms.

* Adding NOLINTNEXTLINE(bugprone-macro-parentheses), expanding comments.

* Labeling all changes with `PR #3263`, for easy reference, and to make it easy to undo these changes if we decide to do so in the future.

Author: rwgk

include/pybind11/pybind11.h

@@ -1987,6 +1987,7 @@ iterator make_iterator(Iterator first, Sentinel last, Extra &&... extra) {
                     throw stop_iteration();
                 }
                 return *s.it;
+            // NOLINTNEXTLINE(readability-const-return-type) // PR #3263
             }, std::forward<Extra>(extra)..., Policy);
     }
 

include/pybind11/pytypes.h

@@ -733,7 +733,9 @@ class generic_iterator : public Policy {
     generic_iterator() = default;
     generic_iterator(handle seq, ssize_t index) : Policy(seq, index) { }
 
+    // NOLINTNEXTLINE(readability-const-return-type) // PR #3263
     reference operator*() const { return Policy::dereference(); }
+    // NOLINTNEXTLINE(readability-const-return-type) // PR #3263
     reference operator[](difference_type n) const { return *(*this + n); }
     pointer operator->() const { return **this; }
 
@@ -773,11 +775,12 @@ class sequence_fast_readonly {
 protected:
     using iterator_category = std::random_access_iterator_tag;
     using value_type = handle;
-    using reference = handle;
+    using reference = const handle; // PR #3263
     using pointer = arrow_proxy<const handle>;
 
     sequence_fast_readonly(handle obj, ssize_t n) : ptr(PySequence_Fast_ITEMS(obj.ptr()) + n) { }
 
+    // NOLINTNEXTLINE(readability-const-return-type) // PR #3263
     reference dereference() const { return *ptr; }
     void increment() { ++ptr; }
     void decrement() { --ptr; }
@@ -816,12 +819,13 @@ class dict_readonly {
 protected:
     using iterator_category = std::forward_iterator_tag;
     using value_type = std::pair<handle, handle>;
-    using reference = value_type;
+    using reference = const value_type; // PR #3263
     using pointer = arrow_proxy<const value_type>;
 
     dict_readonly() = default;
     dict_readonly(handle obj, ssize_t pos) : obj(obj), pos(pos) { increment(); }
 
+    // NOLINTNEXTLINE(readability-const-return-type) // PR #3263
     reference dereference() const { return {key, value}; }
     void increment() {
         if (PyDict_Next(obj.ptr(), &pos, &key, &value) == 0) {
@@ -966,7 +970,7 @@ class iterator : public object {
     using iterator_category = std::input_iterator_tag;
     using difference_type = ssize_t;
     using value_type = handle;
-    using reference = handle;
+    using reference = const handle; // PR #3263
     using pointer = const handle *;
 
     PYBIND11_OBJECT_DEFAULT(iterator, object, PyIter_Check)
@@ -982,6 +986,7 @@ class iterator : public object {
         return rv;
     }
 
+    // NOLINTNEXTLINE(readability-const-return-type) // PR #3263
     reference operator*() const {
         if (m_ptr && !value.ptr()) {
             auto& self = const_cast<iterator &>(*this);

tests/test_pytypes.cpp

@@ -462,6 +462,49 @@ TEST_SUBMODULE(pytypes, m) {
     m.def("weakref_from_object_and_function",
           [](py::object o, py::function f) { return py::weakref(std::move(o), std::move(f)); });
 
+// See PR #3263 for background (https://github.com/pybind/pybind11/pull/3263):
+// pytypes.h could be changed to enforce the "most correct" user code below, by removing
+// `const` from iterator `reference` using type aliases, but that will break existing
+// user code.
+#if (defined(__APPLE__) && defined(__clang__)) || defined(PYPY_VERSION)
+// This is "most correct" and enforced on these platforms.
+#    define PYBIND11_AUTO_IT auto it
+#else
+// This works on many platforms and is (unfortunately) reflective of existing user code.
+// NOLINTNEXTLINE(bugprone-macro-parentheses)
+#    define PYBIND11_AUTO_IT auto &it
+#endif
+
+    m.def("tuple_iterator", []() {
+        auto tup = py::make_tuple(5, 7);
+        int tup_sum = 0;
+        for (PYBIND11_AUTO_IT : tup) {
+            tup_sum += it.cast<int>();
+        }
+        return tup_sum;
+    });
+
+    m.def("dict_iterator", []() {
+        py::dict dct;
+        dct[py::int_(3)] = 5;
+        dct[py::int_(7)] = 11;
+        int kv_sum = 0;
+        for (PYBIND11_AUTO_IT : dct) {
+            kv_sum += it.first.cast<int>() * 100 + it.second.cast<int>();
+        }
+        return kv_sum;
+    });
+
+    m.def("passed_iterator", [](const py::iterator &py_it) {
+        int elem_sum = 0;
+        for (PYBIND11_AUTO_IT : py_it) {
+            elem_sum += it.cast<int>();
+        }
+        return elem_sum;
+    });
+
+#undef PYBIND11_AUTO_IT
+
     // Tests below this line are for pybind11 IMPLEMENTATION DETAILS:
 
     m.def("sequence_item_get_ssize_t", [](const py::object &o) {

tests/test_pytypes.py

@@ -623,6 +623,12 @@ def callback(wr):
     assert callback.called
 
 
+def test_cpp_iterators():
+    assert m.tuple_iterator() == 12
+    assert m.dict_iterator() == 305 + 711
+    assert m.passed_iterator(iter((-7, 3))) == -4
+
+
 def test_implementation_details():
     lst = [39, 43, 92, 49, 22, 29, 93, 98, 26, 57, 8]
     tup = tuple(lst)