From bde5ed369278f095c82a59f93fbee19f284185cd Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" Date: Sat, 27 Jul 2024 00:17:00 +0200 Subject: [PATCH 1/9] gh-105201: Add PyIter_NextItem() Return -1 and set an exception on error; return 0 if the iterator is exhausted, and return 1 if the next item was fetched successfully. Prefer this API to PyIter_Next(), which requires the caller to use PyErr_Occurred() to differentiate between iterator exhaution and errors. Co-authered-by: Irit Katriel --- Doc/c-api/iter.rst | 58 +++++++++++++++--------------- Doc/data/refcounts.dat | 4 +++ Doc/data/stable_abi.dat | 1 + Include/abstract.h | 6 ++++ Lib/test/test_capi/test_misc.py | 35 ++++++++++++++++++ Lib/test/test_stable_abi_ctypes.py | 1 + Misc/stable_abi.toml | 2 ++ Modules/_testcapimodule.c | 41 +++++++++++++++++++++ Objects/abstract.c | 54 ++++++++++++++++++++++------ PC/python3dll.c | 1 + 10 files changed, 164 insertions(+), 39 deletions(-) diff --git a/Doc/c-api/iter.rst b/Doc/c-api/iter.rst index 434d2021cea8e6..41650c7d3294b5 100644 --- a/Doc/c-api/iter.rst +++ b/Doc/c-api/iter.rst @@ -10,7 +10,8 @@ There are two functions specifically for working with iterators. .. c:function:: int PyIter_Check(PyObject *o) Return non-zero if the object *o* can be safely passed to - :c:func:`PyIter_Next`, and ``0`` otherwise. This function always succeeds. + :c:func:`PyIter_NextItem` and ``0`` otherwise. + This function always succeeds. .. c:function:: int PyAIter_Check(PyObject *o) @@ -19,40 +20,41 @@ There are two functions specifically for working with iterators. .. versionadded:: 3.10 -.. c:function:: PyObject* PyIter_Next(PyObject *o) - - Return the next value from the iterator *o*. The object must be an iterator - according to :c:func:`PyIter_Check` (it is up to the caller to check this). - If there are no remaining values, returns ``NULL`` with no exception set. - If an error occurs while retrieving the item, returns ``NULL`` and passes - along the exception. +.. c:function:: int PyIter_NextItem(PyObject *iter, PyObject **item) -To write a loop which iterates over an iterator, the C code should look -something like this:: + Return ``1`` and set *item* to a :term:`strong reference` of the + next value of the iterator *iter* on success. + Return ``0`` and set *item* to ``NULL`` if there are no remaining values. + Return ``-1``, set *item* to ``NULL`` and set an exception on error. - PyObject *iterator = PyObject_GetIter(obj); - PyObject *item; + Iterate over *iter* using the following pattern:: - if (iterator == NULL) { - /* propagate error */ - } + PyObject *iter = PyObject_GetIter(obj); + if (iter == NULL) { + goto error; + } - while ((item = PyIter_Next(iterator))) { - /* do something with item */ - ... - /* release reference when done */ - Py_DECREF(item); - } + PyObject *item = NULL; + while (PyIter_NextItem(iter, &item)) { + if (item == NULL) { + goto error; + } + do_something(item); + Py_DECREF(item); + } + Py_DECREF(iter); - Py_DECREF(iterator); +.. c:function:: PyObject* PyIter_Next(PyObject *o) - if (PyErr_Occurred()) { - /* propagate error */ - } - else { - /* continue doing useful work */ - } + This is an older version of :c:func:`!PyIter_NextItem`, + which is retained for backwards compatibility. + Prefer :c:func:`PyIter_NextItem`. + Return the next value from the iterator *o*. The object must be an iterator + according to :c:func:`PyIter_Check` (it is up to the caller to check this). + If there are no remaining values, returns ``NULL`` with no exception set. + If an error occurs while retrieving the item, returns ``NULL`` and passes + along the exception. .. c:type:: PySendResult diff --git a/Doc/data/refcounts.dat b/Doc/data/refcounts.dat index a7d06e076a1b55..899f61fdf05c7e 100644 --- a/Doc/data/refcounts.dat +++ b/Doc/data/refcounts.dat @@ -1136,6 +1136,10 @@ PyAIter_Check:PyObject*:o:0: PyIter_Next:PyObject*::+1: PyIter_Next:PyObject*:o:0: +PyIter_NextIter:int::: +PyIter_NextIter:PyObject*:iter:0: +PyIter_NextIter:PyObject**:item:+1: + PyIter_Send:int::: PyIter_Send:PyObject*:iter:0: PyIter_Send:PyObject*:arg:0: diff --git a/Doc/data/stable_abi.dat b/Doc/data/stable_abi.dat index 90ddb3fd8213ca..592e3465824893 100644 --- a/Doc/data/stable_abi.dat +++ b/Doc/data/stable_abi.dat @@ -335,6 +335,7 @@ func,PyInterpreterState_GetID,3.7,, func,PyInterpreterState_New,3.2,, func,PyIter_Check,3.8,, func,PyIter_Next,3.2,, +func,PyIter_NextItem,3.14,, func,PyIter_Send,3.10,, data,PyListIter_Type,3.2,, data,PyListRevIter_Type,3.2,, diff --git a/Include/abstract.h b/Include/abstract.h index f0e49c1afb8164..cbceda4d34c14e 100644 --- a/Include/abstract.h +++ b/Include/abstract.h @@ -397,6 +397,12 @@ PyAPI_FUNC(int) PyIter_Check(PyObject *); This function always succeeds. */ PyAPI_FUNC(int) PyAIter_Check(PyObject *); +/* Return 1 and set 'item' to the next item of iter on success. + * Return 0 and set 'item' to NULL when there are no remaining values. + * Return -1, set 'item' to NULL and set an exception on error. + */ +PyAPI_FUNC(int) PyIter_NextItem(PyObject *iter, PyObject **item); + /* Takes an iterator object and calls its tp_iternext slot, returning the next value. diff --git a/Lib/test/test_capi/test_misc.py b/Lib/test/test_capi/test_misc.py index 9de97c0c2c776a..aadd442e52dac6 100644 --- a/Lib/test/test_capi/test_misc.py +++ b/Lib/test/test_capi/test_misc.py @@ -401,6 +401,41 @@ def check_negative_refcount(self, code): br'_Py_NegativeRefcount: Assertion failed: ' br'object has negative ref count') + def run_iter_api_test(self, next_func): + inputs = [ (), (1,2,3), + [], [1,2,3]] + + for inp in inputs: + items = [] + it = iter(inp) + while (item := next_func(it)) is not None: + items.append(item) + self.assertEqual(items, list(inp)) + + class Broken: + def __init__(self): + self.count = 0 + + def __next__(self): + if self.count < 3: + self.count += 1 + return self.count + else: + raise TypeError('bad type') + + it = Broken() + self.assertEqual(next_func(it), 1) + self.assertEqual(next_func(it), 2) + self.assertEqual(next_func(it), 3) + with self.assertRaisesRegex(TypeError, 'bad type'): + next_func(it) + + def test_iter_next(self): + self.run_iter_api_test(_testcapi.PyIter_Next) + + def test_iter_nextitem(self): + self.run_iter_api_test(_testcapi.PyIter_NextItem) + @unittest.skipUnless(hasattr(_testcapi, 'negative_refcount'), 'need _testcapi.negative_refcount()') def test_negative_refcount(self): diff --git a/Lib/test/test_stable_abi_ctypes.py b/Lib/test/test_stable_abi_ctypes.py index d1d8a967dbe62f..fedad17621cb02 100644 --- a/Lib/test/test_stable_abi_ctypes.py +++ b/Lib/test/test_stable_abi_ctypes.py @@ -371,6 +371,7 @@ def test_windows_feature_macros(self): "PyInterpreterState_New", "PyIter_Check", "PyIter_Next", + "PyIter_NextItem", "PyIter_Send", "PyListIter_Type", "PyListRevIter_Type", diff --git a/Misc/stable_abi.toml b/Misc/stable_abi.toml index 73012193d61485..1da1c0a7450342 100644 --- a/Misc/stable_abi.toml +++ b/Misc/stable_abi.toml @@ -2508,3 +2508,5 @@ [function.Py_TYPE] added = '3.14' +[function.PyIter_NextItem] + added = '3.15' diff --git a/Modules/_testcapimodule.c b/Modules/_testcapimodule.c index 5ebcfef6143e02..0d368abb6fcc6e 100644 --- a/Modules/_testcapimodule.c +++ b/Modules/_testcapimodule.c @@ -281,6 +281,44 @@ test_dict_iteration(PyObject* self, PyObject *Py_UNUSED(ignored)) Py_RETURN_NONE; } +static PyObject * +pyiter_next(PyObject *self, PyObject *args) +{ + PyObject *iter; + if (!PyArg_ParseTuple(args, "O:pyiter_next", &iter)) { + return NULL; + } + assert(PyIter_Check(iter) || PyAIter_Check(iter)); + PyObject *item = PyIter_Next(iter); + if (item == NULL && !PyErr_Occurred()) { + Py_RETURN_NONE; + } + return item; +} + +static PyObject * +pyiter_nextitem(PyObject *self, PyObject *args) +{ + PyObject *iter; + if (!PyArg_ParseTuple(args, "O:pyiter_nextitem", &iter)) { + return NULL; + } + assert(PyIter_Check(iter) || PyAIter_Check(iter)); + PyObject *item; + int rc = PyIter_NextItem(iter, &item); + if (rc < 0) { + assert(PyErr_Occurred()); + assert(item == NULL); + return NULL; + } + assert(!PyErr_Occurred()); + if (item == NULL) { + Py_RETURN_NONE; + } + return item; +} + + /* Issue #4701: Check that PyObject_Hash implicitly calls * PyType_Ready if it hasn't already been called */ @@ -3483,6 +3521,9 @@ static PyMethodDef TestMethods[] = { {"test_weakref_capi", test_weakref_capi, METH_NOARGS}, {"function_set_warning", function_set_warning, METH_NOARGS}, {"test_critical_sections", test_critical_sections, METH_NOARGS}, + + {"PyIter_Next", pyiter_next, METH_VARARGS}, + {"PyIter_NextItem", pyiter_nextitem, METH_VARARGS}, {NULL, NULL} /* sentinel */ }; diff --git a/Objects/abstract.c b/Objects/abstract.c index afb068718bb010..3d0472907e0eb6 100644 --- a/Objects/abstract.c +++ b/Objects/abstract.c @@ -2881,7 +2881,47 @@ PyAIter_Check(PyObject *obj) tp->tp_as_async->am_anext != &_PyObject_NextNotImplemented); } +static int +iternext(PyObject *iter, PyObject **item) +{ + *item = (*Py_TYPE(iter)->tp_iternext)(iter); + if (*item == NULL) { + PyThreadState *tstate = _PyThreadState_GET(); + /* When the iterator is exhausted it must return NULL; + * a StopIteration exception may or may not be set. */ + if (!_PyErr_Occurred(tstate)) { + return 0; + } + if (_PyErr_ExceptionMatches(tstate, PyExc_StopIteration)) { + _PyErr_Clear(tstate); + return 0; + } + return -1; + } + return 1; +} + +/* Return 1 and set 'item' to the next item of iter on success. + * Return 0 and set 'item' to NULL when there are no remaining values. + * Return -1, set 'item' to NULL and set an exception on error. + */ +int +PyIter_NextItem(PyObject *iter, PyObject **item) +{ + assert(iter != NULL); + assert(item != NULL); + + if (!PyIter_Check(iter) && !PyAIter_Check(iter)) { + PyErr_Format(PyExc_TypeError, "expected an iterator, not '%T'", iter); + return -1; + } + + return iternext(iter, item); +} + /* Return next item. + * Deprecated; use PyIter_NextItem() instead. + * * If an error occurs, return NULL. PyErr_Occurred() will be true. * If the iteration terminates normally, return NULL and clear the * PyExc_StopIteration exception (if it was set). PyErr_Occurred() @@ -2891,17 +2931,9 @@ PyAIter_Check(PyObject *obj) PyObject * PyIter_Next(PyObject *iter) { - PyObject *result; - result = (*Py_TYPE(iter)->tp_iternext)(iter); - if (result == NULL) { - PyThreadState *tstate = _PyThreadState_GET(); - if (_PyErr_Occurred(tstate) - && _PyErr_ExceptionMatches(tstate, PyExc_StopIteration)) - { - _PyErr_Clear(tstate); - } - } - return result; + PyObject *item; + (void)iternext(iter, &item); + return item; } PySendResult diff --git a/PC/python3dll.c b/PC/python3dll.c index aa3c3965908ff4..78bcef155f51d5 100755 --- a/PC/python3dll.c +++ b/PC/python3dll.c @@ -326,6 +326,7 @@ EXPORT_FUNC(PyInterpreterState_GetID) EXPORT_FUNC(PyInterpreterState_New) EXPORT_FUNC(PyIter_Check) EXPORT_FUNC(PyIter_Next) +EXPORT_FUNC(PyIter_NextItem) EXPORT_FUNC(PyIter_Send) EXPORT_FUNC(PyList_Append) EXPORT_FUNC(PyList_AsTuple) From 13045076e0b4b57d03a49e3f37fbb83db79a6cbd Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" Date: Sat, 27 Jul 2024 00:28:45 +0200 Subject: [PATCH 2/9] NEWS and What's New --- Doc/whatsnew/3.14.rst | 4 ++++ .../next/C_API/2024-07-27-00-28-35.gh-issue-105201.0-xUWq.rst | 2 ++ 2 files changed, 6 insertions(+) create mode 100644 Misc/NEWS.d/next/C_API/2024-07-27-00-28-35.gh-issue-105201.0-xUWq.rst diff --git a/Doc/whatsnew/3.14.rst b/Doc/whatsnew/3.14.rst index d2ba7ada76733a..ad4c572d9949a9 100644 --- a/Doc/whatsnew/3.14.rst +++ b/Doc/whatsnew/3.14.rst @@ -397,6 +397,10 @@ New Features (Contributed by Victor Stinner in :gh:`119182`.) +* Add :c:func:`PyIter_NextItem` to replace :c:func:`PyIter_Next`, + which has an ambiguous return value. + (Contributed by Irit Katriel and Erlend Aasland in :gh:`105201`.) + Porting to Python 3.14 ---------------------- diff --git a/Misc/NEWS.d/next/C_API/2024-07-27-00-28-35.gh-issue-105201.0-xUWq.rst b/Misc/NEWS.d/next/C_API/2024-07-27-00-28-35.gh-issue-105201.0-xUWq.rst new file mode 100644 index 00000000000000..bf5300b1c5d5f8 --- /dev/null +++ b/Misc/NEWS.d/next/C_API/2024-07-27-00-28-35.gh-issue-105201.0-xUWq.rst @@ -0,0 +1,2 @@ +Add :c:func:`PyIter_NextItem` to replace :c:func:`PyIter_Next`, which has an +ambiguous return value. Patch by Irit Katriel and Erlend Aasland. From 210c0e1e62f87f4b0762e291e3f70bf3753a3aef Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" Date: Sat, 27 Jul 2024 09:02:41 +0200 Subject: [PATCH 3/9] =?UTF-8?q?Address=20B=C3=A9n=C3=A9dikt's=20review?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- Lib/test/test_capi/test_misc.py | 12 +++++++----- Misc/stable_abi.toml | 2 +- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/Lib/test/test_capi/test_misc.py b/Lib/test/test_capi/test_misc.py index aadd442e52dac6..617fd50f6a3816 100644 --- a/Lib/test/test_capi/test_misc.py +++ b/Lib/test/test_capi/test_misc.py @@ -402,15 +402,17 @@ def check_negative_refcount(self, code): br'object has negative ref count') def run_iter_api_test(self, next_func): - inputs = [ (), (1,2,3), - [], [1,2,3]] + dataset = ( + (), (1,2,3), + [], [1,2,3], + ) - for inp in inputs: + for input_ in dataset: items = [] - it = iter(inp) + it = iter(input_) while (item := next_func(it)) is not None: items.append(item) - self.assertEqual(items, list(inp)) + self.assertEqual(items, list(input_)) class Broken: def __init__(self): diff --git a/Misc/stable_abi.toml b/Misc/stable_abi.toml index 1da1c0a7450342..c38671e389ac5e 100644 --- a/Misc/stable_abi.toml +++ b/Misc/stable_abi.toml @@ -2509,4 +2509,4 @@ [function.Py_TYPE] added = '3.14' [function.PyIter_NextItem] - added = '3.15' + added = '3.14' From c5800e6e03cc382cfbc549516746f86654eac418 Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" Date: Sun, 28 Jul 2024 00:12:41 +0200 Subject: [PATCH 4/9] Address reviews and fix a bug the bug: 'item' was not set to NULL when an incorrect type was given --- Doc/c-api/iter.rst | 3 +++ Include/abstract.h | 4 +++- Lib/test/test_capi/test_misc.py | 29 ++++++++++++++------------ Modules/_testcapimodule.c | 18 ++++------------ Objects/abstract.c | 37 ++++++++++++++++++--------------- 5 files changed, 46 insertions(+), 45 deletions(-) diff --git a/Doc/c-api/iter.rst b/Doc/c-api/iter.rst index 41650c7d3294b5..1a4fe514190da9 100644 --- a/Doc/c-api/iter.rst +++ b/Doc/c-api/iter.rst @@ -37,6 +37,7 @@ There are two functions specifically for working with iterators. PyObject *item = NULL; while (PyIter_NextItem(iter, &item)) { if (item == NULL) { + Py_DECREF(iter); goto error; } do_something(item); @@ -44,6 +45,8 @@ There are two functions specifically for working with iterators. } Py_DECREF(iter); + .. versionadded:: 3.14 + .. c:function:: PyObject* PyIter_Next(PyObject *o) This is an older version of :c:func:`!PyIter_NextItem`, diff --git a/Include/abstract.h b/Include/abstract.h index cbceda4d34c14e..8ced2004e75822 100644 --- a/Include/abstract.h +++ b/Include/abstract.h @@ -409,7 +409,9 @@ PyAPI_FUNC(int) PyIter_NextItem(PyObject *iter, PyObject **item); If the iterator is exhausted, this returns NULL without setting an exception. - NULL with an exception means an error occurred. */ + NULL with an exception means an error occurred. + + Deprecated; use PyIter_NextItem() instead. */ PyAPI_FUNC(PyObject *) PyIter_Next(PyObject *); #if !defined(Py_LIMITED_API) || Py_LIMITED_API+0 >= 0x030A0000 diff --git a/Lib/test/test_capi/test_misc.py b/Lib/test/test_capi/test_misc.py index 617fd50f6a3816..c27c0122f55111 100644 --- a/Lib/test/test_capi/test_misc.py +++ b/Lib/test/test_capi/test_misc.py @@ -402,17 +402,13 @@ def check_negative_refcount(self, code): br'object has negative ref count') def run_iter_api_test(self, next_func): - dataset = ( - (), (1,2,3), - [], [1,2,3], - ) - - for input_ in dataset: - items = [] - it = iter(input_) - while (item := next_func(it)) is not None: - items.append(item) - self.assertEqual(items, list(input_)) + for data in (), [], (1, 2, 3), [1 , 2, 3], "123": + with self.subTest(data=data): + items = [] + it = iter(data) + while (item := next_func(it)) is not None: + items.append(item) + self.assertEqual(items, list(data)) class Broken: def __init__(self): @@ -433,10 +429,17 @@ def __next__(self): next_func(it) def test_iter_next(self): - self.run_iter_api_test(_testcapi.PyIter_Next) + from _testcapi import PyIter_Next + self.run_iter_api_test(PyIter_Next) + # CRASHES PyIter_Next(10) def test_iter_nextitem(self): - self.run_iter_api_test(_testcapi.PyIter_NextItem) + from _testcapi import PyIter_NextItem + self.run_iter_api_test(PyIter_NextItem) + + regex = "expected.*iterator.*got.*'int'" + with self.assertRaisesRegex(TypeError, regex): + PyIter_NextItem(10) @unittest.skipUnless(hasattr(_testcapi, 'negative_refcount'), 'need _testcapi.negative_refcount()') diff --git a/Modules/_testcapimodule.c b/Modules/_testcapimodule.c index 0d368abb6fcc6e..3f09657e27f65d 100644 --- a/Modules/_testcapimodule.c +++ b/Modules/_testcapimodule.c @@ -282,13 +282,8 @@ test_dict_iteration(PyObject* self, PyObject *Py_UNUSED(ignored)) } static PyObject * -pyiter_next(PyObject *self, PyObject *args) +pyiter_next(PyObject *self, PyObject *iter) { - PyObject *iter; - if (!PyArg_ParseTuple(args, "O:pyiter_next", &iter)) { - return NULL; - } - assert(PyIter_Check(iter) || PyAIter_Check(iter)); PyObject *item = PyIter_Next(iter); if (item == NULL && !PyErr_Occurred()) { Py_RETURN_NONE; @@ -297,13 +292,8 @@ pyiter_next(PyObject *self, PyObject *args) } static PyObject * -pyiter_nextitem(PyObject *self, PyObject *args) +pyiter_nextitem(PyObject *self, PyObject *iter) { - PyObject *iter; - if (!PyArg_ParseTuple(args, "O:pyiter_nextitem", &iter)) { - return NULL; - } - assert(PyIter_Check(iter) || PyAIter_Check(iter)); PyObject *item; int rc = PyIter_NextItem(iter, &item); if (rc < 0) { @@ -3522,8 +3512,8 @@ static PyMethodDef TestMethods[] = { {"function_set_warning", function_set_warning, METH_NOARGS}, {"test_critical_sections", test_critical_sections, METH_NOARGS}, - {"PyIter_Next", pyiter_next, METH_VARARGS}, - {"PyIter_NextItem", pyiter_nextitem, METH_VARARGS}, + {"PyIter_Next", pyiter_next, METH_O}, + {"PyIter_NextItem", pyiter_nextitem, METH_O}, {NULL, NULL} /* sentinel */ }; diff --git a/Objects/abstract.c b/Objects/abstract.c index 3d0472907e0eb6..9269f05b9b4c82 100644 --- a/Objects/abstract.c +++ b/Objects/abstract.c @@ -2884,24 +2884,27 @@ PyAIter_Check(PyObject *obj) static int iternext(PyObject *iter, PyObject **item) { - *item = (*Py_TYPE(iter)->tp_iternext)(iter); - if (*item == NULL) { - PyThreadState *tstate = _PyThreadState_GET(); - /* When the iterator is exhausted it must return NULL; - * a StopIteration exception may or may not be set. */ - if (!_PyErr_Occurred(tstate)) { - return 0; - } - if (_PyErr_ExceptionMatches(tstate, PyExc_StopIteration)) { - _PyErr_Clear(tstate); - return 0; - } - return -1; + iternextfunc tp_iternext = Py_TYPE(iter)->tp_iternext; + if ((*item = tp_iternext(iter))) { + return 1; } - return 1; + + PyThreadState *tstate = _PyThreadState_GET(); + /* When the iterator is exhausted it must return NULL; + * a StopIteration exception may or may not be set. */ + if (!_PyErr_Occurred(tstate)) { + return 0; + } + if (_PyErr_ExceptionMatches(tstate, PyExc_StopIteration)) { + _PyErr_Clear(tstate); + return 0; + } + + /* Error case: an exception (different than StopIteration) is set. */ + return -1; } -/* Return 1 and set 'item' to the next item of iter on success. +/* Return 1 and set 'item' to the next item of 'iter' on success. * Return 0 and set 'item' to NULL when there are no remaining values. * Return -1, set 'item' to NULL and set an exception on error. */ @@ -2912,7 +2915,8 @@ PyIter_NextItem(PyObject *iter, PyObject **item) assert(item != NULL); if (!PyIter_Check(iter) && !PyAIter_Check(iter)) { - PyErr_Format(PyExc_TypeError, "expected an iterator, not '%T'", iter); + *item = NULL; + PyErr_Format(PyExc_TypeError, "expected an iterator, got '%T'", iter); return -1; } @@ -2920,7 +2924,6 @@ PyIter_NextItem(PyObject *iter, PyObject **item) } /* Return next item. - * Deprecated; use PyIter_NextItem() instead. * * If an error occurs, return NULL. PyErr_Occurred() will be true. * If the iteration terminates normally, return NULL and clear the From f60173b4d102ea395f3ee354089f828843f9b28c Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" Date: Sun, 28 Jul 2024 00:20:47 +0200 Subject: [PATCH 5/9] Move tests to _testcapi/abstract --- Lib/test/test_capi/test_abstract.py | 40 +++++++++++++++++++++++++++++ Lib/test/test_capi/test_misc.py | 40 ----------------------------- Modules/_testcapi/abstract.c | 29 +++++++++++++++++++++ Modules/_testcapimodule.c | 31 ---------------------- 4 files changed, 69 insertions(+), 71 deletions(-) diff --git a/Lib/test/test_capi/test_abstract.py b/Lib/test/test_capi/test_abstract.py index bc39036e90bf8b..3a8c224126a672 100644 --- a/Lib/test/test_capi/test_abstract.py +++ b/Lib/test/test_capi/test_abstract.py @@ -1007,6 +1007,46 @@ def test_object_generichash(self): for obj in object(), 1, 'string', []: self.assertEqual(generichash(obj), object.__hash__(obj)) + def run_iter_api_test(self, next_func): + for data in (), [], (1, 2, 3), [1 , 2, 3], "123": + with self.subTest(data=data): + items = [] + it = iter(data) + while (item := next_func(it)) is not None: + items.append(item) + self.assertEqual(items, list(data)) + + class Broken: + def __init__(self): + self.count = 0 + + def __next__(self): + if self.count < 3: + self.count += 1 + return self.count + else: + raise TypeError('bad type') + + it = Broken() + self.assertEqual(next_func(it), 1) + self.assertEqual(next_func(it), 2) + self.assertEqual(next_func(it), 3) + with self.assertRaisesRegex(TypeError, 'bad type'): + next_func(it) + + def test_iter_next(self): + from _testcapi import PyIter_Next + self.run_iter_api_test(PyIter_Next) + # CRASHES PyIter_Next(10) + + def test_iter_nextitem(self): + from _testcapi import PyIter_NextItem + self.run_iter_api_test(PyIter_NextItem) + + regex = "expected.*iterator.*got.*'int'" + with self.assertRaisesRegex(TypeError, regex): + PyIter_NextItem(10) + if __name__ == "__main__": unittest.main() diff --git a/Lib/test/test_capi/test_misc.py b/Lib/test/test_capi/test_misc.py index c27c0122f55111..9de97c0c2c776a 100644 --- a/Lib/test/test_capi/test_misc.py +++ b/Lib/test/test_capi/test_misc.py @@ -401,46 +401,6 @@ def check_negative_refcount(self, code): br'_Py_NegativeRefcount: Assertion failed: ' br'object has negative ref count') - def run_iter_api_test(self, next_func): - for data in (), [], (1, 2, 3), [1 , 2, 3], "123": - with self.subTest(data=data): - items = [] - it = iter(data) - while (item := next_func(it)) is not None: - items.append(item) - self.assertEqual(items, list(data)) - - class Broken: - def __init__(self): - self.count = 0 - - def __next__(self): - if self.count < 3: - self.count += 1 - return self.count - else: - raise TypeError('bad type') - - it = Broken() - self.assertEqual(next_func(it), 1) - self.assertEqual(next_func(it), 2) - self.assertEqual(next_func(it), 3) - with self.assertRaisesRegex(TypeError, 'bad type'): - next_func(it) - - def test_iter_next(self): - from _testcapi import PyIter_Next - self.run_iter_api_test(PyIter_Next) - # CRASHES PyIter_Next(10) - - def test_iter_nextitem(self): - from _testcapi import PyIter_NextItem - self.run_iter_api_test(PyIter_NextItem) - - regex = "expected.*iterator.*got.*'int'" - with self.assertRaisesRegex(TypeError, regex): - PyIter_NextItem(10) - @unittest.skipUnless(hasattr(_testcapi, 'negative_refcount'), 'need _testcapi.negative_refcount()') def test_negative_refcount(self): diff --git a/Modules/_testcapi/abstract.c b/Modules/_testcapi/abstract.c index b126aee5b9777b..8c2c7137cdce40 100644 --- a/Modules/_testcapi/abstract.c +++ b/Modules/_testcapi/abstract.c @@ -129,6 +129,33 @@ mapping_getoptionalitem(PyObject *self, PyObject *args) } } +static PyObject * +pyiter_next(PyObject *self, PyObject *iter) +{ + PyObject *item = PyIter_Next(iter); + if (item == NULL && !PyErr_Occurred()) { + Py_RETURN_NONE; + } + return item; +} + +static PyObject * +pyiter_nextitem(PyObject *self, PyObject *iter) +{ + PyObject *item; + int rc = PyIter_NextItem(iter, &item); + if (rc < 0) { + assert(PyErr_Occurred()); + assert(item == NULL); + return NULL; + } + assert(!PyErr_Occurred()); + if (item == NULL) { + Py_RETURN_NONE; + } + return item; +} + static PyMethodDef test_methods[] = { {"object_getoptionalattr", object_getoptionalattr, METH_VARARGS}, @@ -138,6 +165,8 @@ static PyMethodDef test_methods[] = { {"mapping_getoptionalitem", mapping_getoptionalitem, METH_VARARGS}, {"mapping_getoptionalitemstring", mapping_getoptionalitemstring, METH_VARARGS}, + {"PyIter_Next", pyiter_next, METH_O}, + {"PyIter_NextItem", pyiter_nextitem, METH_O}, {NULL}, }; diff --git a/Modules/_testcapimodule.c b/Modules/_testcapimodule.c index 3f09657e27f65d..5ebcfef6143e02 100644 --- a/Modules/_testcapimodule.c +++ b/Modules/_testcapimodule.c @@ -281,34 +281,6 @@ test_dict_iteration(PyObject* self, PyObject *Py_UNUSED(ignored)) Py_RETURN_NONE; } -static PyObject * -pyiter_next(PyObject *self, PyObject *iter) -{ - PyObject *item = PyIter_Next(iter); - if (item == NULL && !PyErr_Occurred()) { - Py_RETURN_NONE; - } - return item; -} - -static PyObject * -pyiter_nextitem(PyObject *self, PyObject *iter) -{ - PyObject *item; - int rc = PyIter_NextItem(iter, &item); - if (rc < 0) { - assert(PyErr_Occurred()); - assert(item == NULL); - return NULL; - } - assert(!PyErr_Occurred()); - if (item == NULL) { - Py_RETURN_NONE; - } - return item; -} - - /* Issue #4701: Check that PyObject_Hash implicitly calls * PyType_Ready if it hasn't already been called */ @@ -3511,9 +3483,6 @@ static PyMethodDef TestMethods[] = { {"test_weakref_capi", test_weakref_capi, METH_NOARGS}, {"function_set_warning", function_set_warning, METH_NOARGS}, {"test_critical_sections", test_critical_sections, METH_NOARGS}, - - {"PyIter_Next", pyiter_next, METH_O}, - {"PyIter_NextItem", pyiter_nextitem, METH_O}, {NULL, NULL} /* sentinel */ }; From 36cd91a4f812d535ff2a92dcb9b3cec36bf7cb66 Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" Date: Sun, 28 Jul 2024 00:24:30 +0200 Subject: [PATCH 6/9] Remove PyAIter_Check; this is about tp_iternext, not am_anext --- Objects/abstract.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Objects/abstract.c b/Objects/abstract.c index 9269f05b9b4c82..d92a7144c533c0 100644 --- a/Objects/abstract.c +++ b/Objects/abstract.c @@ -2914,7 +2914,7 @@ PyIter_NextItem(PyObject *iter, PyObject **item) assert(iter != NULL); assert(item != NULL); - if (!PyIter_Check(iter) && !PyAIter_Check(iter)) { + if (!PyIter_Check(iter)) { *item = NULL; PyErr_Format(PyExc_TypeError, "expected an iterator, got '%T'", iter); return -1; From 636e39be3e683663eda983a6efd9fe84b2204fab Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" Date: Mon, 29 Jul 2024 00:10:13 +0200 Subject: [PATCH 7/9] Address review: add PyIter_NextItem to the Limited API in 3.14 --- Include/abstract.h | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Include/abstract.h b/Include/abstract.h index 8ced2004e75822..2350eb89624f3f 100644 --- a/Include/abstract.h +++ b/Include/abstract.h @@ -397,11 +397,13 @@ PyAPI_FUNC(int) PyIter_Check(PyObject *); This function always succeeds. */ PyAPI_FUNC(int) PyAIter_Check(PyObject *); +#if !defined(Py_LIMITED_API) || Py_LIMITED_API+0 >= 0x030e0000 /* Return 1 and set 'item' to the next item of iter on success. * Return 0 and set 'item' to NULL when there are no remaining values. * Return -1, set 'item' to NULL and set an exception on error. */ PyAPI_FUNC(int) PyIter_NextItem(PyObject *iter, PyObject **item); +#endif /* Takes an iterator object and calls its tp_iternext slot, returning the next value. From 9a6b246657ff2c20f9c2e06214b90809012ee672 Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" Date: Mon, 29 Jul 2024 14:26:06 +0200 Subject: [PATCH 8/9] Address reveiws and remove example --- Doc/c-api/iter.rst | 18 ------------------ Include/abstract.h | 4 ++-- Objects/abstract.c | 2 +- 3 files changed, 3 insertions(+), 21 deletions(-) diff --git a/Doc/c-api/iter.rst b/Doc/c-api/iter.rst index 1a4fe514190da9..bf9df62c6f1706 100644 --- a/Doc/c-api/iter.rst +++ b/Doc/c-api/iter.rst @@ -27,24 +27,6 @@ There are two functions specifically for working with iterators. Return ``0`` and set *item* to ``NULL`` if there are no remaining values. Return ``-1``, set *item* to ``NULL`` and set an exception on error. - Iterate over *iter* using the following pattern:: - - PyObject *iter = PyObject_GetIter(obj); - if (iter == NULL) { - goto error; - } - - PyObject *item = NULL; - while (PyIter_NextItem(iter, &item)) { - if (item == NULL) { - Py_DECREF(iter); - goto error; - } - do_something(item); - Py_DECREF(item); - } - Py_DECREF(iter); - .. versionadded:: 3.14 .. c:function:: PyObject* PyIter_Next(PyObject *o) diff --git a/Include/abstract.h b/Include/abstract.h index 2350eb89624f3f..7cfee1332ccaa4 100644 --- a/Include/abstract.h +++ b/Include/abstract.h @@ -398,7 +398,7 @@ PyAPI_FUNC(int) PyIter_Check(PyObject *); PyAPI_FUNC(int) PyAIter_Check(PyObject *); #if !defined(Py_LIMITED_API) || Py_LIMITED_API+0 >= 0x030e0000 -/* Return 1 and set 'item' to the next item of iter on success. +/* Return 1 and set 'item' to the next item of 'iter' on success. * Return 0 and set 'item' to NULL when there are no remaining values. * Return -1, set 'item' to NULL and set an exception on error. */ @@ -413,7 +413,7 @@ PyAPI_FUNC(int) PyIter_NextItem(PyObject *iter, PyObject **item); NULL with an exception means an error occurred. - Deprecated; use PyIter_NextItem() instead. */ + Prefer PyIter_NextItem() instead. */ PyAPI_FUNC(PyObject *) PyIter_Next(PyObject *); #if !defined(Py_LIMITED_API) || Py_LIMITED_API+0 >= 0x030A0000 diff --git a/Objects/abstract.c b/Objects/abstract.c index d92a7144c533c0..8626584e9bf56c 100644 --- a/Objects/abstract.c +++ b/Objects/abstract.c @@ -2914,7 +2914,7 @@ PyIter_NextItem(PyObject *iter, PyObject **item) assert(iter != NULL); assert(item != NULL); - if (!PyIter_Check(iter)) { + if (Py_TYPE(iter)->tp_iternext == NULL) { *item = NULL; PyErr_Format(PyExc_TypeError, "expected an iterator, got '%T'", iter); return -1; From fe41ad7285259c64317537561fa5c36e84870ddf Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" Date: Fri, 2 Aug 2024 23:46:52 +0200 Subject: [PATCH 9/9] Address review: fix misspelling in refcounts.dat --- Doc/data/refcounts.dat | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Doc/data/refcounts.dat b/Doc/data/refcounts.dat index 6b0ac37c5f83f2..65d48f8bea7de8 100644 --- a/Doc/data/refcounts.dat +++ b/Doc/data/refcounts.dat @@ -1132,9 +1132,9 @@ PyAIter_Check:PyObject*:o:0: PyIter_Next:PyObject*::+1: PyIter_Next:PyObject*:o:0: -PyIter_NextIter:int::: -PyIter_NextIter:PyObject*:iter:0: -PyIter_NextIter:PyObject**:item:+1: +PyIter_NextItem:int::: +PyIter_NextItem:PyObject*:iter:0: +PyIter_NextItem:PyObject**:item:+1: PyIter_Send:int::: PyIter_Send:PyObject*:iter:0: