maint(Clang-Tidy): readability-const-return (#3254)

* Enable clang-tidy readability-const-return

* PyTest functional

* Fix regression

* Fix actual regression

* Remove one more NOLINT

* Update comment

Author: Skylion007

.clang-tidy

@@ -31,9 +31,10 @@ modernize-use-override,
 modernize-use-using,
 *performance*,
 readability-avoid-const-params-in-decls,
+readability-const-return-type,
 readability-container-size-empty,
-readability-else-after-return,
 readability-delete-null-pointer,
+readability-else-after-return,
 readability-implicit-bool-conversion,
 readability-make-member-function-const,
 readability-misplaced-array-index,

include/pybind11/cast.h

@@ -1194,13 +1194,15 @@ class argument_loader {
     }
 
     template <typename Return, typename Guard, typename Func>
+    // NOLINTNEXTLINE(readability-const-return-type)
     enable_if_t<!std::is_void<Return>::value, Return> call(Func &&f) && {
-        return std::move(*this).template call_impl<Return>(std::forward<Func>(f), indices{}, Guard{});
+        return std::move(*this).template call_impl<remove_cv_t<Return>>(std::forward<Func>(f), indices{}, Guard{});
     }
 
     template <typename Return, typename Guard, typename Func>
+    // NOLINTNEXTLINE(readability-const-return-type)
     enable_if_t<std::is_void<Return>::value, void_type> call(Func &&f) && {
-        std::move(*this).template call_impl<Return>(std::forward<Func>(f), indices{}, Guard{});
+        std::move(*this).template call_impl<remove_cv_t<Return>>(std::forward<Func>(f), indices{}, Guard{});
         return void_type();
     }
 
@@ -1222,6 +1224,7 @@ class argument_loader {
     }
 
     template <typename Return, typename Func, size_t... Is, typename Guard>
+    // NOLINTNEXTLINE(readability-const-return-type)
     Return call_impl(Func &&f, index_sequence<Is...>, Guard &&) && {
         return std::forward<Func>(f)(cast_op<Args>(std::move(std::get<Is>(argcasters)))...);
     }

include/pybind11/detail/descr.h

@@ -77,7 +77,8 @@ constexpr enable_if_t<B, T1> _(const T1 &d, const T2 &) { return d; }
 template <bool B, typename T1, typename T2>
 constexpr enable_if_t<!B, T2> _(const T1 &, const T2 &d) { return d; }
 
-template <size_t Size> auto constexpr _() -> decltype(int_to_str<Size / 10, Size % 10>::digits) {
+template <size_t Size>
+auto constexpr _() -> remove_cv_t<decltype(int_to_str<Size / 10, Size % 10>::digits)> {
     return int_to_str<Size / 10, Size % 10>::digits;
 }
 

include/pybind11/pybind11.h

@@ -2002,13 +2002,13 @@ template <return_value_policy Policy = return_value_policy::reference_internal,
           typename KeyType = decltype((*std::declval<Iterator>()).first),
 #endif
           typename... Extra>
-iterator make_key_iterator(Iterator first, Sentinel last, Extra &&... 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) -> KeyType {
+            .def("__next__", [](state &s) -> detail::remove_cv_t<KeyType> {
                 if (!s.first_or_done)
                     ++s.it;
                 else

include/pybind11/pytypes.h

@@ -773,7 +773,7 @@ class sequence_fast_readonly {
 protected:
     using iterator_category = std::random_access_iterator_tag;
     using value_type = handle;
-    using reference = const handle;
+    using reference = handle;
     using pointer = arrow_proxy<const handle>;
 
     sequence_fast_readonly(handle obj, ssize_t n) : ptr(PySequence_Fast_ITEMS(obj.ptr()) + n) { }
@@ -816,7 +816,7 @@ class dict_readonly {
 protected:
     using iterator_category = std::forward_iterator_tag;
     using value_type = std::pair<handle, handle>;
-    using reference = const value_type;
+    using reference = value_type;
     using pointer = arrow_proxy<const value_type>;
 
     dict_readonly() = default;
@@ -966,7 +966,7 @@ class iterator : public object {
     using iterator_category = std::input_iterator_tag;
     using difference_type = ssize_t;
     using value_type = handle;
-    using reference = const handle;
+    using reference = handle;
     using pointer = const handle *;
 
     PYBIND11_OBJECT_DEFAULT(iterator, object, PyIter_Check)

tests/test_eigen.cpp

@@ -178,6 +178,7 @@ TEST_SUBMODULE(eigen, m) {
         ReturnTester() { print_created(this); }
         ~ReturnTester() { print_destroyed(this); }
         static Eigen::MatrixXd create() { return Eigen::MatrixXd::Ones(10, 10); }
+        // NOLINTNEXTLINE(readability-const-return-type)
         static const Eigen::MatrixXd createConst() { return Eigen::MatrixXd::Ones(10, 10); }
         Eigen::MatrixXd &get() { return mat; }
         Eigen::MatrixXd *getPtr() { return &mat; }
@@ -244,6 +245,9 @@ TEST_SUBMODULE(eigen, m) {
 
     // test_fixed, and various other tests
     m.def("fixed_r", [mat]() -> FixedMatrixR { return FixedMatrixR(mat); });
+    // Our Eigen does a hack which respects constness through the numpy writeable flag.
+    // Therefore, the const return actually affects this type despite being an rvalue.
+    // NOLINTNEXTLINE(readability-const-return-type)
     m.def("fixed_r_const", [mat]() -> const FixedMatrixR { return FixedMatrixR(mat); });
     m.def("fixed_c", [mat]() -> FixedMatrixC { return FixedMatrixC(mat); });
     m.def("fixed_copy_r", [](const FixedMatrixR &m) -> FixedMatrixR { return m; });