From 4c6bee3514679cde3b506d5552a3a11fd09cc6d5 Mon Sep 17 00:00:00 2001 From: Aaron Gokaslan Date: Thu, 9 Sep 2021 14:06:33 -0400 Subject: [PATCH 1/7] fix: Set __file__ constant when using eval_file (#1300) (#3233) * Set __file__ constant when using eval_file * Use const ref * Use a move instead * Revert * Improve test * Guard test with Python version * Fix tests * Dont support Python2 API * Drop Python2 eval __file__ support * Hack * Semisupport Python2 * Take2 * Remove Python2 support --- include/pybind11/eval.h | 9 +++++++++ tests/test_cmake_build/test.py | 3 +++ 2 files changed, 12 insertions(+) diff --git a/include/pybind11/eval.h b/include/pybind11/eval.h index 33fcdc09d..e0f58bcf4 100644 --- a/include/pybind11/eval.h +++ b/include/pybind11/eval.h @@ -136,6 +136,15 @@ object eval_file(str fname, object global = globals(), object local = object()) pybind11_fail("File \"" + fname_str + "\" could not be opened!"); } + // In Python2, this should be encoded by getfilesystemencoding. + // We don't boher setting it since Python2 is past EOL anyway. + // See PR#3233 +#if PY_VERSION_HEX >= 0x03000000 + if (!global.contains("__file__")) { + global["__file__"] = std::move(fname); + } +#endif + #if PY_VERSION_HEX < 0x03000000 && defined(PYPY_VERSION) PyObject *result = PyRun_File(f, fname_str.c_str(), start, global.ptr(), local.ptr()); diff --git a/tests/test_cmake_build/test.py b/tests/test_cmake_build/test.py index d1a290dcc..972a27bea 100644 --- a/tests/test_cmake_build/test.py +++ b/tests/test_cmake_build/test.py @@ -3,5 +3,8 @@ import sys import test_cmake_build +if str is not bytes: # If not Python2 + assert isinstance(__file__, str) # Test this is properly set + assert test_cmake_build.add(1, 2) == 3 print("{} imports, runs, and adds: 1 + 2 = 3".format(sys.argv[1])) From 4d5ad03e1fd09b8be26843f7cb165051ab9a9c05 Mon Sep 17 00:00:00 2001 From: Jeremy Maitin-Shepard Date: Thu, 9 Sep 2021 12:56:10 -0700 Subject: [PATCH 2/7] Avoid use of temporary `bytes` object in string_caster for UTF-8 (#3257) Fixes #3252 --- include/pybind11/cast.h | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h index db79f57cd..3e621eba7 100644 --- a/include/pybind11/cast.h +++ b/include/pybind11/cast.h @@ -377,6 +377,22 @@ template struct string_caster { #endif } +#if PY_VERSION_HEX >= 0x03030000 + // On Python >= 3.3, for UTF-8 we avoid the need for a temporary `bytes` + // object by using `PyUnicode_AsUTF8AndSize`. + if (PYBIND11_SILENCE_MSVC_C4127(UTF_N == 8)) { + Py_ssize_t size = -1; + const auto *buffer + = reinterpret_cast(PyUnicode_AsUTF8AndSize(load_src.ptr(), &size)); + if (!buffer) { + PyErr_Clear(); + return false; + } + value = StringType(buffer, static_cast(size)); + return true; + } +#endif + auto utfNbytes = reinterpret_steal(PyUnicode_AsEncodedString( load_src.ptr(), UTF_N == 8 ? "utf-8" : UTF_N == 16 ? "utf-16" : "utf-32", nullptr)); if (!utfNbytes) { PyErr_Clear(); return false; } From ae07d4c6c663f8cc2590d28ecd6baa09b055537f Mon Sep 17 00:00:00 2001 From: Aaron Gokaslan Date: Fri, 10 Sep 2021 00:27:36 -0400 Subject: [PATCH 3/7] 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 --- .clang-tidy | 3 ++- include/pybind11/cast.h | 7 +++++-- include/pybind11/detail/descr.h | 3 ++- include/pybind11/pybind11.h | 4 ++-- include/pybind11/pytypes.h | 6 +++--- tests/test_eigen.cpp | 4 ++++ 6 files changed, 18 insertions(+), 9 deletions(-) diff --git a/.clang-tidy b/.clang-tidy index a6437dd49..dbe85a8b4 100644 --- a/.clang-tidy +++ b/.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, diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h index 3e621eba7..17b10157b 100644 --- a/include/pybind11/cast.h +++ b/include/pybind11/cast.h @@ -1194,13 +1194,15 @@ public: } template + // NOLINTNEXTLINE(readability-const-return-type) enable_if_t::value, Return> call(Func &&f) && { - return std::move(*this).template call_impl(std::forward(f), indices{}, Guard{}); + return std::move(*this).template call_impl>(std::forward(f), indices{}, Guard{}); } template + // NOLINTNEXTLINE(readability-const-return-type) enable_if_t::value, void_type> call(Func &&f) && { - std::move(*this).template call_impl(std::forward(f), indices{}, Guard{}); + std::move(*this).template call_impl>(std::forward(f), indices{}, Guard{}); return void_type(); } @@ -1222,6 +1224,7 @@ private: } template + // NOLINTNEXTLINE(readability-const-return-type) Return call_impl(Func &&f, index_sequence, Guard &&) && { return std::forward(f)(cast_op(std::move(std::get(argcasters)))...); } diff --git a/include/pybind11/detail/descr.h b/include/pybind11/detail/descr.h index 0b498e5e7..c62e541bd 100644 --- a/include/pybind11/detail/descr.h +++ b/include/pybind11/detail/descr.h @@ -77,7 +77,8 @@ constexpr enable_if_t _(const T1 &d, const T2 &) { return d; } template constexpr enable_if_t _(const T1 &, const T2 &d) { return d; } -template auto constexpr _() -> decltype(int_to_str::digits) { +template +auto constexpr _() -> remove_cv_t::digits)> { return int_to_str::digits; } diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index 82d339751..c4e5ea6e1 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -2002,13 +2002,13 @@ template ()).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; if (!detail::get_type_info(typeid(state), false)) { class_(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 { if (!s.first_or_done) ++s.it; else diff --git a/include/pybind11/pytypes.h b/include/pybind11/pytypes.h index 971db85f3..2de3f5f10 100644 --- a/include/pybind11/pytypes.h +++ b/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; 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; - using reference = const value_type; + using reference = value_type; using pointer = arrow_proxy; dict_readonly() = default; @@ -966,7 +966,7 @@ public: 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) diff --git a/tests/test_eigen.cpp b/tests/test_eigen.cpp index 651be0575..ad572b2ad 100644 --- a/tests/test_eigen.cpp +++ b/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; }); From 121b91f99c9044e12ea1f746d7f3e586c41529c8 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Fri, 10 Sep 2021 07:16:09 -0700 Subject: [PATCH 4/7] Fixing NOLINT mishap (#3260) * Removing NOLINT pointed out by Aaron. * Removing another NOLINT. --- include/pybind11/cast.h | 2 -- 1 file changed, 2 deletions(-) diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h index 17b10157b..1ec2080f8 100644 --- a/include/pybind11/cast.h +++ b/include/pybind11/cast.h @@ -1200,7 +1200,6 @@ public: } template - // NOLINTNEXTLINE(readability-const-return-type) enable_if_t::value, void_type> call(Func &&f) && { std::move(*this).template call_impl>(std::forward(f), indices{}, Guard{}); return void_type(); @@ -1224,7 +1223,6 @@ private: } template - // NOLINTNEXTLINE(readability-const-return-type) Return call_impl(Func &&f, index_sequence, Guard &&) && { return std::forward(f)(cast_op(std::move(std::get(argcasters)))...); } From 0e599589fe821f86d18635c13636f3042d4c06b9 Mon Sep 17 00:00:00 2001 From: Laramie Leavitt Date: Fri, 10 Sep 2021 09:29:21 -0700 Subject: [PATCH 5/7] Fix thread safety for pybind11 loader_life_support (#3237) * Fix thread safety for pybind11 loader_life_support Fixes issue: https://github.com/pybind/pybind11/issues/2765 This converts the vector of PyObjects to either a single void* or a per-thread void* depending on the WITH_THREAD define. The new field is used by each thread to construct a stack of loader_life_support frames that can extend the life of python objects. The pointer is updated when the loader_life_support object is allocated (which happens before a call) as well as on release. Each loader_life_support maintains a set of PyObject references that need to be lifetime extended; this is done by storing them in a c++ std::unordered_set and clearing the references when the method completes. * Also update the internals version as the internal struct is no longer compatible * Add test demonstrating threading works correctly. It may be appropriate to run this under msan/tsan/etc. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Update test to use lifetime-extended references rather than std::string_view, as that's a C++ 17 feature. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Make loader_life_support members private * Update version to dev2 * Update test to use python threading rather than concurrent.futures * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Remove unnecessary env in test * Remove unnecessary pytest in test * Use native C++ thread_local in place of python per-thread data structures to retain compatability * clang-format test_thread.cpp * Add a note about debugging the py::cast() error * thread_test.py now propagates exceptions on join() calls. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * remove unused sys / merge * Update include order in test_thread.cpp * Remove spurious whitespace * Update comment / whitespace. * Address review comments * lint cleanup * Fix test IntStruct constructor. * Add explicit to constructor Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Aaron Gokaslan --- include/pybind11/detail/common.h | 4 +- include/pybind11/detail/internals.h | 6 +- include/pybind11/detail/type_caster_base.h | 55 ++++++++++-------- pybind11/_version.py | 2 +- tests/CMakeLists.txt | 1 + tests/test_thread.cpp | 66 ++++++++++++++++++++++ tests/test_thread.py | 44 +++++++++++++++ 7 files changed, 148 insertions(+), 30 deletions(-) create mode 100644 tests/test_thread.cpp create mode 100644 tests/test_thread.py diff --git a/include/pybind11/detail/common.h b/include/pybind11/detail/common.h index a8fd7d917..8aeb79fb7 100644 --- a/include/pybind11/detail/common.h +++ b/include/pybind11/detail/common.h @@ -11,11 +11,11 @@ #define PYBIND11_VERSION_MAJOR 2 #define PYBIND11_VERSION_MINOR 8 -#define PYBIND11_VERSION_PATCH 0.dev1 +#define PYBIND11_VERSION_PATCH 0.dev2 // Similar to Python's convention: https://docs.python.org/3/c-api/apiabiversion.html // Additional convention: 0xD = dev -#define PYBIND11_VERSION_HEX 0x020800D1 +#define PYBIND11_VERSION_HEX 0x020800D2 #define PYBIND11_NAMESPACE_BEGIN(name) namespace name { #define PYBIND11_NAMESPACE_END(name) } diff --git a/include/pybind11/detail/internals.h b/include/pybind11/detail/internals.h index b177801a1..39c28e773 100644 --- a/include/pybind11/detail/internals.h +++ b/include/pybind11/detail/internals.h @@ -106,7 +106,7 @@ struct internals { std::unordered_map> patients; std::forward_list registered_exception_translators; std::unordered_map shared_data; // Custom data to be shared across extensions - std::vector loader_patient_stack; // Used by `loader_life_support` + std::vector unused_loader_patient_stack_remove_at_v5; std::forward_list static_strings; // Stores the std::strings backing detail::c_str() PyTypeObject *static_property_type; PyTypeObject *default_metaclass; @@ -298,12 +298,12 @@ PYBIND11_NOINLINE internals &get_internals() { #if PY_VERSION_HEX >= 0x03070000 internals_ptr->tstate = PyThread_tss_alloc(); if (!internals_ptr->tstate || (PyThread_tss_create(internals_ptr->tstate) != 0)) - pybind11_fail("get_internals: could not successfully initialize the TSS key!"); + pybind11_fail("get_internals: could not successfully initialize the tstate TSS key!"); PyThread_tss_set(internals_ptr->tstate, tstate); #else internals_ptr->tstate = PyThread_create_key(); if (internals_ptr->tstate == -1) - pybind11_fail("get_internals: could not successfully initialize the TLS key!"); + pybind11_fail("get_internals: could not successfully initialize the tstate TLS key!"); PyThread_set_key_value(internals_ptr->tstate, tstate); #endif internals_ptr->istate = tstate->interp; diff --git a/include/pybind11/detail/type_caster_base.h b/include/pybind11/detail/type_caster_base.h index 4c9ac7b8f..fa8433cfe 100644 --- a/include/pybind11/detail/type_caster_base.h +++ b/include/pybind11/detail/type_caster_base.h @@ -31,47 +31,54 @@ PYBIND11_NAMESPACE_BEGIN(detail) /// A life support system for temporary objects created by `type_caster::load()`. /// Adding a patient will keep it alive up until the enclosing function returns. class loader_life_support { +private: + loader_life_support* parent = nullptr; + std::unordered_set keep_alive; + + static loader_life_support** get_stack_pp() { +#if defined(WITH_THREAD) + thread_local static loader_life_support* per_thread_stack = nullptr; + return &per_thread_stack; +#else + static loader_life_support* global_stack = nullptr; + return &global_stack; +#endif + } + public: /// A new patient frame is created when a function is entered loader_life_support() { - get_internals().loader_patient_stack.push_back(nullptr); + loader_life_support** stack = get_stack_pp(); + parent = *stack; + *stack = this; } /// ... and destroyed after it returns ~loader_life_support() { - auto &stack = get_internals().loader_patient_stack; - if (stack.empty()) + loader_life_support** stack = get_stack_pp(); + if (*stack != this) pybind11_fail("loader_life_support: internal error"); - - auto ptr = stack.back(); - stack.pop_back(); - Py_CLEAR(ptr); - - // A heuristic to reduce the stack's capacity (e.g. after long recursive calls) - if (stack.capacity() > 16 && !stack.empty() && stack.capacity() / stack.size() > 2) - stack.shrink_to_fit(); + *stack = parent; + for (auto* item : keep_alive) + Py_DECREF(item); } /// This can only be used inside a pybind11-bound function, either by `argument_loader` /// at argument preparation time or by `py::cast()` at execution time. PYBIND11_NOINLINE static void add_patient(handle h) { - auto &stack = get_internals().loader_patient_stack; - if (stack.empty()) + loader_life_support* frame = *get_stack_pp(); + if (!frame) { + // NOTE: It would be nice to include the stack frames here, as this indicates + // use of pybind11::cast<> outside the normal call framework, finding such + // a location is challenging. Developers could consider printing out + // stack frame addresses here using something like __builtin_frame_address(0) throw cast_error("When called outside a bound function, py::cast() cannot " "do Python -> C++ conversions which require the creation " "of temporary values"); - - auto &list_ptr = stack.back(); - if (list_ptr == nullptr) { - list_ptr = PyList_New(1); - if (!list_ptr) - pybind11_fail("loader_life_support: error allocating list"); - PyList_SET_ITEM(list_ptr, 0, h.inc_ref().ptr()); - } else { - auto result = PyList_Append(list_ptr, h.ptr()); - if (result == -1) - pybind11_fail("loader_life_support: error adding patient"); } + + if (frame->keep_alive.insert(h.ptr()).second) + Py_INCREF(h.ptr()); } }; diff --git a/pybind11/_version.py b/pybind11/_version.py index 610d39bf1..d212f1dfb 100644 --- a/pybind11/_version.py +++ b/pybind11/_version.py @@ -8,5 +8,5 @@ def _to_int(s): return s -__version__ = "2.8.0.dev1" +__version__ = "2.8.0.dev2" version_info = tuple(_to_int(s) for s in __version__.split(".")) diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index d71a51e6a..f014771d5 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -129,6 +129,7 @@ set(PYBIND11_TEST_FILES test_stl.cpp test_stl_binders.cpp test_tagbased_polymorphic.cpp + test_thread.cpp test_union.cpp test_virtual_functions.cpp) diff --git a/tests/test_thread.cpp b/tests/test_thread.cpp new file mode 100644 index 000000000..19d91768b --- /dev/null +++ b/tests/test_thread.cpp @@ -0,0 +1,66 @@ +/* + tests/test_thread.cpp -- call pybind11 bound methods in threads + + Copyright (c) 2021 Laramie Leavitt (Google LLC) + + All rights reserved. Use of this source code is governed by a + BSD-style license that can be found in the LICENSE file. +*/ + +#include +#include + +#include +#include + +#include "pybind11_tests.h" + +namespace py = pybind11; + +namespace { + +struct IntStruct { + explicit IntStruct(int v) : value(v) {}; + ~IntStruct() { value = -value; } + IntStruct(const IntStruct&) = default; + IntStruct& operator=(const IntStruct&) = default; + + int value; +}; + +} // namespace + +TEST_SUBMODULE(thread, m) { + + py::class_(m, "IntStruct").def(py::init([](const int i) { return IntStruct(i); })); + + // implicitly_convertible uses loader_life_support when an implicit + // conversion is required in order to lifetime extend the reference. + // + // This test should be run with ASAN for better effectiveness. + py::implicitly_convertible(); + + m.def("test", [](int expected, const IntStruct &in) { + { + py::gil_scoped_release release; + std::this_thread::sleep_for(std::chrono::milliseconds(5)); + } + + if (in.value != expected) { + throw std::runtime_error("Value changed!!"); + } + }); + + m.def( + "test_no_gil", + [](int expected, const IntStruct &in) { + std::this_thread::sleep_for(std::chrono::milliseconds(5)); + if (in.value != expected) { + throw std::runtime_error("Value changed!!"); + } + }, + py::call_guard()); + + // NOTE: std::string_view also uses loader_life_support to ensure that + // the string contents remain alive, but that's a C++ 17 feature. +} diff --git a/tests/test_thread.py b/tests/test_thread.py new file mode 100644 index 000000000..f9db1baba --- /dev/null +++ b/tests/test_thread.py @@ -0,0 +1,44 @@ +# -*- coding: utf-8 -*- + +import threading + +from pybind11_tests import thread as m + + +class Thread(threading.Thread): + def __init__(self, fn): + super(Thread, self).__init__() + self.fn = fn + self.e = None + + def run(self): + try: + for i in range(10): + self.fn(i, i) + except Exception as e: + self.e = e + + def join(self): + super(Thread, self).join() + if self.e: + raise self.e + + +def test_implicit_conversion(): + a = Thread(m.test) + b = Thread(m.test) + c = Thread(m.test) + for x in [a, b, c]: + x.start() + for x in [c, b, a]: + x.join() + + +def test_implicit_conversion_no_gil(): + a = Thread(m.test_no_gil) + b = Thread(m.test_no_gil) + c = Thread(m.test_no_gil) + for x in [a, b, c]: + x.start() + for x in [c, b, a]: + x.join() From 9978ed588baa2cf804794b8ee9f765a316ab16e2 Mon Sep 17 00:00:00 2001 From: Aaron Gokaslan Date: Fri, 10 Sep 2021 14:23:32 -0400 Subject: [PATCH 6/7] Fix capsule bug (#3261) Thanks Aaron for jumping in fixing this! --- include/pybind11/pytypes.h | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/include/pybind11/pytypes.h b/include/pybind11/pytypes.h index 2de3f5f10..a1cf6bef0 100644 --- a/include/pybind11/pytypes.h +++ b/include/pybind11/pytypes.h @@ -1414,14 +1414,19 @@ public: T* get_pointer() const { auto name = this->name(); T *result = static_cast(PyCapsule_GetPointer(m_ptr, name)); - if (!result) pybind11_fail("Unable to extract capsule contents!"); + if (!result) { + PyErr_Clear(); + pybind11_fail("Unable to extract capsule contents!"); + } return result; } /// Replaces a capsule's pointer *without* calling the destructor on the existing one. void set_pointer(const void *value) { - if (PyCapsule_SetPointer(m_ptr, const_cast(value)) != 0) + if (PyCapsule_SetPointer(m_ptr, const_cast(value)) != 0) { + PyErr_Clear(); pybind11_fail("Could not set capsule pointer"); + } } const char *name() const { return PyCapsule_GetName(m_ptr); } From 6c65ab5950a75271bddced3f4238fff2c9a427a5 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Sun, 12 Sep 2021 19:53:26 -0700 Subject: [PATCH 7/7] 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. --- include/pybind11/pybind11.h | 1 + include/pybind11/pytypes.h | 11 +++++++--- tests/test_pytypes.cpp | 43 +++++++++++++++++++++++++++++++++++++ tests/test_pytypes.py | 6 ++++++ 4 files changed, 58 insertions(+), 3 deletions(-) diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index c4e5ea6e1..07d2d0436 100644 --- a/include/pybind11/pybind11.h +++ b/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)..., Policy); } diff --git a/include/pybind11/pytypes.h b/include/pybind11/pytypes.h index a1cf6bef0..d1d3dcb05 100644 --- a/include/pybind11/pytypes.h +++ b/include/pybind11/pytypes.h @@ -733,7 +733,9 @@ public: 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; 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; - using reference = value_type; + using reference = const value_type; // PR #3263 using pointer = arrow_proxy; 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 @@ public: 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 @@ public: return rv; } + // NOLINTNEXTLINE(readability-const-return-type) // PR #3263 reference operator*() const { if (m_ptr && !value.ptr()) { auto& self = const_cast(*this); diff --git a/tests/test_pytypes.cpp b/tests/test_pytypes.cpp index 96c97351e..2157dc097 100644 --- a/tests/test_pytypes.cpp +++ b/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(); + } + 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() * 100 + it.second.cast(); + } + 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(); + } + 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) { diff --git a/tests/test_pytypes.py b/tests/test_pytypes.py index 18847550f..070538cc9 100644 --- a/tests/test_pytypes.py +++ b/tests/test_pytypes.py @@ -623,6 +623,12 @@ def test_weakref(create_weakref, create_weakref_with_callback): 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)