From 5e915244dfe700d4a708aa0b067f2a65b566427b Mon Sep 17 00:00:00 2001 From: Irit Katriel Date: Thu, 1 Jun 2023 19:11:51 +0100 Subject: [PATCH 01/13] gh-105201: Add PyIter_NextItem to replace PyIter_Next which has an ambiguous return value --- Include/abstract.h | 7 +++++++ Lib/test/test_capi/test_misc.py | 35 +++++++++++++++++++++++++++++++ Modules/_testcapimodule.c | 37 +++++++++++++++++++++++++++++++++ Modules/_xxtestfuzz/fuzzer.c | 2 +- Objects/abstract.c | 33 ++++++++++++++++++++++------- 5 files changed, 105 insertions(+), 9 deletions(-) diff --git a/Include/abstract.h b/Include/abstract.h index b4c2bedef442bf..3b4ba2d10142de 100644 --- a/Include/abstract.h +++ b/Include/abstract.h @@ -397,6 +397,13 @@ PyAPI_FUNC(int) PyIter_Check(PyObject *); This function always succeeds. */ PyAPI_FUNC(int) PyAIter_Check(PyObject *); +/* Takes an iterator object and calls its tp_iternext slot, + setting *item to the next value, or to NULL if the iterator + is exhausted. + + returns 0 on success and -1 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 037c8112d53e7a..15155afb6d68d5 100644 --- a/Lib/test/test_capi/test_misc.py +++ b/Lib/test/test_capi/test_misc.py @@ -410,6 +410,41 @@ def __delitem__(self, index): _testcapi.sequence_del_slice(mapping, 1, 3) self.assertEqual(mapping, {1: 'a', 2: 'b', 3: 'c'}) + 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.call_pyiter_next) + + def test_iter_nextitem(self): + self.run_iter_api_test(_testcapi.call_pyiter_nextitem) + @unittest.skipUnless(hasattr(_testcapi, 'negative_refcount'), 'need _testcapi.negative_refcount') def test_negative_refcount(self): diff --git a/Modules/_testcapimodule.c b/Modules/_testcapimodule.c index d7c89f48f792ed..a662053a99e41d 100644 --- a/Modules/_testcapimodule.c +++ b/Modules/_testcapimodule.c @@ -282,6 +282,41 @@ dict_getitem_knownhash(PyObject *self, PyObject *args) return Py_XNewRef(result); } +static PyObject* +call_pyiter_next(PyObject* self, PyObject *args) +{ + PyObject *iter; + if (!PyArg_ParseTuple(args, "O:call_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* +call_pyiter_nextitem(PyObject* self, PyObject *args) +{ + PyObject *iter; + if (!PyArg_ParseTuple(args, "O:call_pyiter_nextitem", &iter)) { + return NULL; + } + assert(PyIter_Check(iter) || PyAIter_Check(iter)); + PyObject *item = NULL; + int ret = PyIter_NextItem(iter, &item); + if (ret < 0) { + return NULL; + } + if (item == NULL && !PyErr_Occurred()) { + Py_RETURN_NONE; + } + return item; +} + + /* Issue #4701: Check that PyObject_Hash implicitly calls * PyType_Ready if it hasn't already been called */ @@ -3286,6 +3321,8 @@ static PyMethodDef TestMethods[] = { {"test_list_api", test_list_api, METH_NOARGS}, {"test_dict_iteration", test_dict_iteration, METH_NOARGS}, {"dict_getitem_knownhash", dict_getitem_knownhash, METH_VARARGS}, + {"call_pyiter_next", call_pyiter_next, METH_VARARGS}, + {"call_pyiter_nextitem", call_pyiter_nextitem, METH_VARARGS}, {"test_lazy_hash_inheritance", test_lazy_hash_inheritance,METH_NOARGS}, {"test_xincref_doesnt_leak",test_xincref_doesnt_leak, METH_NOARGS}, {"test_incref_doesnt_leak", test_incref_doesnt_leak, METH_NOARGS}, diff --git a/Modules/_xxtestfuzz/fuzzer.c b/Modules/_xxtestfuzz/fuzzer.c index 37d402824853f0..c7f8244c5d4d4f 100644 --- a/Modules/_xxtestfuzz/fuzzer.c +++ b/Modules/_xxtestfuzz/fuzzer.c @@ -377,7 +377,7 @@ static int fuzz_csv_reader(const char* data, size_t size) { if (reader) { /* Consume all of the reader as an iterator */ PyObject* parsed_line; - while ((parsed_line = PyIter_Next(reader))) { + while (PyIter_NextItem(reader, &parsed_line) == 0) { Py_DECREF(parsed_line); } } diff --git a/Objects/abstract.c b/Objects/abstract.c index e95785900c9c5f..3fdb5ed9a9b9e2 100644 --- a/Objects/abstract.c +++ b/Objects/abstract.c @@ -2833,6 +2833,29 @@ PyAIter_Check(PyObject *obj) tp->tp_as_async->am_anext != &_PyObject_NextNotImplemented); } +/* Set *item to the next item. Return 0 on success and -1 on error. + * If the iteration terminates normally, set *item to NULL and clear + * the PyExc_StopIteration exception (if it was set). + */ +int +PyIter_NextItem(PyObject *iter, PyObject **item) +{ + *item = (*Py_TYPE(iter)->tp_iternext)(iter); + if (*item == NULL) { + PyThreadState *tstate = _PyThreadState_GET(); + if (_PyErr_Occurred(tstate)) { + if (_PyErr_ExceptionMatches(tstate, PyExc_StopIteration)) { + _PyErr_Clear(tstate); + *item = NULL; + } + else { + return -1; + } + } + } + return 0; +} + /* Return next item. * If an error occurs, return NULL. PyErr_Occurred() will be true. * If the iteration terminates normally, return NULL and clear the @@ -2844,14 +2867,8 @@ 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); - } + if (PyIter_NextItem(iter, &result) < 0) { + return NULL; } return result; } From 79051257a28f83f562231c6ba85ef1deeada1e1c Mon Sep 17 00:00:00 2001 From: Irit Katriel Date: Thu, 1 Jun 2023 19:33:00 +0100 Subject: [PATCH 02/13] add new function to the stable ABI --- Misc/stable_abi.toml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Misc/stable_abi.toml b/Misc/stable_abi.toml index 2209f6e02cfcd1..7ff516c5787c69 100644 --- a/Misc/stable_abi.toml +++ b/Misc/stable_abi.toml @@ -895,6 +895,8 @@ added = '3.2' [function.PyIter_Next] added = '3.2' +[function.PyIter_NextItem] + added = '3.13' [data.PyListIter_Type] added = '3.2' [data.PyListRevIter_Type] From cc293756cde2c4185166d76f96882ad263760c95 Mon Sep 17 00:00:00 2001 From: Irit Katriel Date: Thu, 1 Jun 2023 20:41:53 +0100 Subject: [PATCH 03/13] code review comments from Jella nd Erlend --- Include/abstract.h | 4 ++-- Modules/_testcapimodule.c | 3 ++- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/Include/abstract.h b/Include/abstract.h index 3b4ba2d10142de..baac6e14be1677 100644 --- a/Include/abstract.h +++ b/Include/abstract.h @@ -397,11 +397,11 @@ PyAPI_FUNC(int) PyIter_Check(PyObject *); This function always succeeds. */ PyAPI_FUNC(int) PyAIter_Check(PyObject *); -/* Takes an iterator object and calls its tp_iternext slot, +/* Take an iterator object and call its tp_iternext slot, setting *item to the next value, or to NULL if the iterator is exhausted. - returns 0 on success and -1 on error. */ + Return 0 on success and -1 on error. */ PyAPI_FUNC(int) PyIter_NextItem(PyObject *iter, PyObject **item); /* Takes an iterator object and calls its tp_iternext slot, diff --git a/Modules/_testcapimodule.c b/Modules/_testcapimodule.c index a662053a99e41d..c04179c25f9d46 100644 --- a/Modules/_testcapimodule.c +++ b/Modules/_testcapimodule.c @@ -310,7 +310,8 @@ call_pyiter_nextitem(PyObject* self, PyObject *args) if (ret < 0) { return NULL; } - if (item == NULL && !PyErr_Occurred()) { + if (item == NULL) { + assert(!PyErr_Occurred()); Py_RETURN_NONE; } return item; From 9eb3a79272cd2e537157a09a13f7aff3d8f76a30 Mon Sep 17 00:00:00 2001 From: Irit Katriel Date: Thu, 1 Jun 2023 21:08:34 +0100 Subject: [PATCH 04/13] update doc --- Doc/c-api/iter.rst | 34 ++++++++++++++++++++++++---------- 1 file changed, 24 insertions(+), 10 deletions(-) diff --git a/Doc/c-api/iter.rst b/Doc/c-api/iter.rst index 434d2021cea8e6..d9a5a1bc925d91 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` (or the legacy :c:func:`PyIter_Next`), + and ``0`` otherwise. This function always succeeds. .. c:function:: int PyAIter_Check(PyObject *o) @@ -19,25 +20,27 @@ There are two functions specifically for working with iterators. .. versionadded:: 3.10 -.. c:function:: PyObject* PyIter_Next(PyObject *o) +.. c:function:: int PyIter_NextItem(PyObject *o, PyObject **item) - 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. + Set ``*item`` to the next value from the iterator *o*. The object *o* + 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, set ``*item`` to ``NULL``. + + Return 0 on success and -1 on error. To write a loop which iterates over an iterator, the C code should look something like this:: PyObject *iterator = PyObject_GetIter(obj); - PyObject *item; if (iterator == NULL) { /* propagate error */ } - while ((item = PyIter_Next(iterator))) { + PyObject *item; + int res; + while ((res = PyIter_Next(iterator, &item)) == 0 && item != NULL) { /* do something with item */ ... /* release reference when done */ @@ -46,7 +49,7 @@ something like this:: Py_DECREF(iterator); - if (PyErr_Occurred()) { + if (res < 0) { /* propagate error */ } else { @@ -54,6 +57,17 @@ something like this:: } +.. c:function:: PyObject* PyIter_Next(PyObject *o) + + 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 The enum value used to represent different results of :c:func:`PyIter_Send`. From 4dcb5d840fefaf3b62d6970cd858512992a6da00 Mon Sep 17 00:00:00 2001 From: Irit Katriel <1055913+iritkatriel@users.noreply.github.com> Date: Thu, 1 Jun 2023 21:33:30 +0100 Subject: [PATCH 05/13] typo --- Doc/c-api/iter.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Doc/c-api/iter.rst b/Doc/c-api/iter.rst index d9a5a1bc925d91..cd30e641db7672 100644 --- a/Doc/c-api/iter.rst +++ b/Doc/c-api/iter.rst @@ -40,7 +40,7 @@ something like this:: PyObject *item; int res; - while ((res = PyIter_Next(iterator, &item)) == 0 && item != NULL) { + while ((res = PyIter_NextItem(iterator, &item)) == 0 && item != NULL) { /* do something with item */ ... /* release reference when done */ From 5aaf37e4255be2e272f589ca2a284e5113f4fba7 Mon Sep 17 00:00:00 2001 From: Irit Katriel Date: Thu, 1 Jun 2023 21:37:07 +0100 Subject: [PATCH 06/13] regen --- Doc/data/stable_abi.dat | 1 + Lib/test/test_stable_abi_ctypes.py | 1 + PC/python3dll.c | 1 + 3 files changed, 3 insertions(+) diff --git a/Doc/data/stable_abi.dat b/Doc/data/stable_abi.dat index 767025b76f8b11..2a33631a5963ec 100644 --- a/Doc/data/stable_abi.dat +++ b/Doc/data/stable_abi.dat @@ -326,6 +326,7 @@ function,PyInterpreterState_GetID,3.7,, function,PyInterpreterState_New,3.2,, function,PyIter_Check,3.8,, function,PyIter_Next,3.2,, +function,PyIter_NextItem,3.13,, function,PyIter_Send,3.10,, var,PyListIter_Type,3.2,, var,PyListRevIter_Type,3.2,, diff --git a/Lib/test/test_stable_abi_ctypes.py b/Lib/test/test_stable_abi_ctypes.py index 60ad3603ae9223..66f00a7b442df9 100644 --- a/Lib/test/test_stable_abi_ctypes.py +++ b/Lib/test/test_stable_abi_ctypes.py @@ -356,6 +356,7 @@ def test_windows_feature_macros(self): "PyInterpreterState_New", "PyIter_Check", "PyIter_Next", + "PyIter_NextItem", "PyIter_Send", "PyListIter_Type", "PyListRevIter_Type", diff --git a/PC/python3dll.c b/PC/python3dll.c index f2c0d9dee883d9..4a22b05f354b1f 100755 --- a/PC/python3dll.c +++ b/PC/python3dll.c @@ -318,6 +318,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 edc54c8507f0de242c7743452ab488c011591bdf Mon Sep 17 00:00:00 2001 From: Irit Katriel Date: Thu, 1 Jun 2023 21:59:03 +0100 Subject: [PATCH 07/13] make PyIter_NextItem return 0 for exhausted iterator and 1 when it provides a new value --- Doc/c-api/iter.rst | 2 +- Include/abstract.h | 3 ++- Modules/_testcapimodule.c | 6 +++++- Modules/_xxtestfuzz/fuzzer.c | 2 +- Objects/abstract.c | 10 ++++++---- 5 files changed, 15 insertions(+), 8 deletions(-) diff --git a/Doc/c-api/iter.rst b/Doc/c-api/iter.rst index cd30e641db7672..6d90f235e4626f 100644 --- a/Doc/c-api/iter.rst +++ b/Doc/c-api/iter.rst @@ -40,7 +40,7 @@ something like this:: PyObject *item; int res; - while ((res = PyIter_NextItem(iterator, &item)) == 0 && item != NULL) { + while ((res = PyIter_NextItem(iterator, &item)) == 1) { /* do something with item */ ... /* release reference when done */ diff --git a/Include/abstract.h b/Include/abstract.h index baac6e14be1677..ebeaa9afb09040 100644 --- a/Include/abstract.h +++ b/Include/abstract.h @@ -401,7 +401,8 @@ PyAPI_FUNC(int) PyAIter_Check(PyObject *); setting *item to the next value, or to NULL if the iterator is exhausted. - Return 0 on success and -1 on error. */ + Return 1 if *item was set to a new value, 0 if the iterator is + exhausted and -1 on error. */ PyAPI_FUNC(int) PyIter_NextItem(PyObject *iter, PyObject **item); /* Takes an iterator object and calls its tp_iternext slot, diff --git a/Modules/_testcapimodule.c b/Modules/_testcapimodule.c index c04179c25f9d46..31c15943457880 100644 --- a/Modules/_testcapimodule.c +++ b/Modules/_testcapimodule.c @@ -310,10 +310,14 @@ call_pyiter_nextitem(PyObject* self, PyObject *args) if (ret < 0) { return NULL; } - if (item == NULL) { + else if (ret == 0) { + assert(item == NULL); assert(!PyErr_Occurred()); Py_RETURN_NONE; } + assert(ret == 1); + assert(item != NULL); + assert(!PyErr_Occurred()); return item; } diff --git a/Modules/_xxtestfuzz/fuzzer.c b/Modules/_xxtestfuzz/fuzzer.c index c7f8244c5d4d4f..f49d56b34ea93c 100644 --- a/Modules/_xxtestfuzz/fuzzer.c +++ b/Modules/_xxtestfuzz/fuzzer.c @@ -377,7 +377,7 @@ static int fuzz_csv_reader(const char* data, size_t size) { if (reader) { /* Consume all of the reader as an iterator */ PyObject* parsed_line; - while (PyIter_NextItem(reader, &parsed_line) == 0) { + while (PyIter_NextItem(reader, &parsed_line) == 1) { Py_DECREF(parsed_line); } } diff --git a/Objects/abstract.c b/Objects/abstract.c index 3fdb5ed9a9b9e2..fe566f6c873f72 100644 --- a/Objects/abstract.c +++ b/Objects/abstract.c @@ -2833,9 +2833,10 @@ PyAIter_Check(PyObject *obj) tp->tp_as_async->am_anext != &_PyObject_NextNotImplemented); } -/* Set *item to the next item. Return 0 on success and -1 on error. - * If the iteration terminates normally, set *item to NULL and clear - * the PyExc_StopIteration exception (if it was set). +/* If the iterator has another value to return, set *item to this value + * and return 1. + * If the iterator is exhausted, set *item to NULL and return 0. + * On error (other than StopIteration) from tp_iternext, return -1. */ int PyIter_NextItem(PyObject *iter, PyObject **item) @@ -2852,8 +2853,9 @@ PyIter_NextItem(PyObject *iter, PyObject **item) return -1; } } + return 0; } - return 0; + return 1; } /* Return next item. From 8d93533a71c3f7286136ec87aa8960c3691f99ad Mon Sep 17 00:00:00 2001 From: Irit Katriel Date: Thu, 1 Jun 2023 22:11:43 +0100 Subject: [PATCH 08/13] fix doc --- Doc/c-api/iter.rst | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Doc/c-api/iter.rst b/Doc/c-api/iter.rst index 6d90f235e4626f..67f2028af408ab 100644 --- a/Doc/c-api/iter.rst +++ b/Doc/c-api/iter.rst @@ -27,7 +27,8 @@ There are two functions specifically for working with iterators. caller to check this). If there are no remaining values, set ``*item`` to ``NULL``. - Return 0 on success and -1 on error. + Return 1 if an item was returned from the iterator, 0 if the iterator + is exhausted, and -1 on error. To write a loop which iterates over an iterator, the C code should look something like this:: From 02b64d4ad657fb9845288024981cd81750882b2c Mon Sep 17 00:00:00 2001 From: Irit Katriel Date: Thu, 1 Jun 2023 22:57:48 +0100 Subject: [PATCH 09/13] Revert "fix doc" This reverts commit 8d93533a71c3f7286136ec87aa8960c3691f99ad. --- Doc/c-api/iter.rst | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/Doc/c-api/iter.rst b/Doc/c-api/iter.rst index 67f2028af408ab..6d90f235e4626f 100644 --- a/Doc/c-api/iter.rst +++ b/Doc/c-api/iter.rst @@ -27,8 +27,7 @@ There are two functions specifically for working with iterators. caller to check this). If there are no remaining values, set ``*item`` to ``NULL``. - Return 1 if an item was returned from the iterator, 0 if the iterator - is exhausted, and -1 on error. + Return 0 on success and -1 on error. To write a loop which iterates over an iterator, the C code should look something like this:: From 1ee8618ce9e69a1e29acc578473afb2548968193 Mon Sep 17 00:00:00 2001 From: Irit Katriel Date: Thu, 1 Jun 2023 22:57:55 +0100 Subject: [PATCH 10/13] Revert "make PyIter_NextItem return 0 for exhausted iterator and 1 when it provides a new value" This reverts commit edc54c8507f0de242c7743452ab488c011591bdf. --- Doc/c-api/iter.rst | 2 +- Include/abstract.h | 3 +-- Modules/_testcapimodule.c | 6 +----- Modules/_xxtestfuzz/fuzzer.c | 2 +- Objects/abstract.c | 10 ++++------ 5 files changed, 8 insertions(+), 15 deletions(-) diff --git a/Doc/c-api/iter.rst b/Doc/c-api/iter.rst index 6d90f235e4626f..cd30e641db7672 100644 --- a/Doc/c-api/iter.rst +++ b/Doc/c-api/iter.rst @@ -40,7 +40,7 @@ something like this:: PyObject *item; int res; - while ((res = PyIter_NextItem(iterator, &item)) == 1) { + while ((res = PyIter_NextItem(iterator, &item)) == 0 && item != NULL) { /* do something with item */ ... /* release reference when done */ diff --git a/Include/abstract.h b/Include/abstract.h index ebeaa9afb09040..baac6e14be1677 100644 --- a/Include/abstract.h +++ b/Include/abstract.h @@ -401,8 +401,7 @@ PyAPI_FUNC(int) PyAIter_Check(PyObject *); setting *item to the next value, or to NULL if the iterator is exhausted. - Return 1 if *item was set to a new value, 0 if the iterator is - exhausted and -1 on error. */ + Return 0 on success and -1 on error. */ PyAPI_FUNC(int) PyIter_NextItem(PyObject *iter, PyObject **item); /* Takes an iterator object and calls its tp_iternext slot, diff --git a/Modules/_testcapimodule.c b/Modules/_testcapimodule.c index 31c15943457880..c04179c25f9d46 100644 --- a/Modules/_testcapimodule.c +++ b/Modules/_testcapimodule.c @@ -310,14 +310,10 @@ call_pyiter_nextitem(PyObject* self, PyObject *args) if (ret < 0) { return NULL; } - else if (ret == 0) { - assert(item == NULL); + if (item == NULL) { assert(!PyErr_Occurred()); Py_RETURN_NONE; } - assert(ret == 1); - assert(item != NULL); - assert(!PyErr_Occurred()); return item; } diff --git a/Modules/_xxtestfuzz/fuzzer.c b/Modules/_xxtestfuzz/fuzzer.c index f49d56b34ea93c..c7f8244c5d4d4f 100644 --- a/Modules/_xxtestfuzz/fuzzer.c +++ b/Modules/_xxtestfuzz/fuzzer.c @@ -377,7 +377,7 @@ static int fuzz_csv_reader(const char* data, size_t size) { if (reader) { /* Consume all of the reader as an iterator */ PyObject* parsed_line; - while (PyIter_NextItem(reader, &parsed_line) == 1) { + while (PyIter_NextItem(reader, &parsed_line) == 0) { Py_DECREF(parsed_line); } } diff --git a/Objects/abstract.c b/Objects/abstract.c index fe566f6c873f72..3fdb5ed9a9b9e2 100644 --- a/Objects/abstract.c +++ b/Objects/abstract.c @@ -2833,10 +2833,9 @@ PyAIter_Check(PyObject *obj) tp->tp_as_async->am_anext != &_PyObject_NextNotImplemented); } -/* If the iterator has another value to return, set *item to this value - * and return 1. - * If the iterator is exhausted, set *item to NULL and return 0. - * On error (other than StopIteration) from tp_iternext, return -1. +/* Set *item to the next item. Return 0 on success and -1 on error. + * If the iteration terminates normally, set *item to NULL and clear + * the PyExc_StopIteration exception (if it was set). */ int PyIter_NextItem(PyObject *iter, PyObject **item) @@ -2853,9 +2852,8 @@ PyIter_NextItem(PyObject *iter, PyObject **item) return -1; } } - return 0; } - return 1; + return 0; } /* Return next item. From 028cef464873e97fce96b909ab17c17dcda6c11c Mon Sep 17 00:00:00 2001 From: Irit Katriel Date: Fri, 2 Jun 2023 13:24:03 +0100 Subject: [PATCH 11/13] flip signature around --- Doc/c-api/iter.rst | 19 +++++++++---------- Include/abstract.h | 11 +++++++---- Modules/_testcapimodule.c | 6 +++--- Modules/_xxtestfuzz/fuzzer.c | 3 ++- Objects/abstract.c | 23 +++++++++++------------ 5 files changed, 32 insertions(+), 30 deletions(-) diff --git a/Doc/c-api/iter.rst b/Doc/c-api/iter.rst index cd30e641db7672..ecdc010083db4c 100644 --- a/Doc/c-api/iter.rst +++ b/Doc/c-api/iter.rst @@ -20,14 +20,13 @@ There are two functions specifically for working with iterators. .. versionadded:: 3.10 -.. c:function:: int PyIter_NextItem(PyObject *o, PyObject **item) +.. c:function:: PyObject* PyIter_NextItem(PyObject *o, int *err) - Set ``*item`` to the next value from the iterator *o*. The object *o* - 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, set ``*item`` to ``NULL``. - - Return 0 on success and -1 on error. + 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 + and ``*err`` set to 0. If an error occurs while retrieving the item, + returns ``NULL`` and sets ``*err`` to 1. To write a loop which iterates over an iterator, the C code should look something like this:: @@ -39,8 +38,8 @@ something like this:: } PyObject *item; - int res; - while ((res = PyIter_NextItem(iterator, &item)) == 0 && item != NULL) { + int err; + while (item = PyIter_NextItem(iterator, &err)) { /* do something with item */ ... /* release reference when done */ @@ -49,7 +48,7 @@ something like this:: Py_DECREF(iterator); - if (res < 0) { + if (err < 0) { /* propagate error */ } else { diff --git a/Include/abstract.h b/Include/abstract.h index baac6e14be1677..ea57fdafd196e4 100644 --- a/Include/abstract.h +++ b/Include/abstract.h @@ -398,11 +398,14 @@ PyAPI_FUNC(int) PyIter_Check(PyObject *); PyAPI_FUNC(int) PyAIter_Check(PyObject *); /* Take an iterator object and call its tp_iternext slot, - setting *item to the next value, or to NULL if the iterator - is exhausted. + returning the next value. + + If the iterator is exhausted, this returns NULL without setting an + exception, and sets *err to 0. - Return 0 on success and -1 on error. */ -PyAPI_FUNC(int) PyIter_NextItem(PyObject *iter, PyObject **item); + NULL with *err == -1 means an error occurred, and an exception has + been set. */ +PyAPI_FUNC(PyObject*) PyIter_NextItem(PyObject *iter, int *err); /* Takes an iterator object and calls its tp_iternext slot, returning the next value. diff --git a/Modules/_testcapimodule.c b/Modules/_testcapimodule.c index c04179c25f9d46..fe13298060bdbe 100644 --- a/Modules/_testcapimodule.c +++ b/Modules/_testcapimodule.c @@ -305,9 +305,9 @@ call_pyiter_nextitem(PyObject* self, PyObject *args) return NULL; } assert(PyIter_Check(iter) || PyAIter_Check(iter)); - PyObject *item = NULL; - int ret = PyIter_NextItem(iter, &item); - if (ret < 0) { + int err; + PyObject *item = PyIter_NextItem(iter, &err); + if (err < 0) { return NULL; } if (item == NULL) { diff --git a/Modules/_xxtestfuzz/fuzzer.c b/Modules/_xxtestfuzz/fuzzer.c index c7f8244c5d4d4f..cc6db8011f06a1 100644 --- a/Modules/_xxtestfuzz/fuzzer.c +++ b/Modules/_xxtestfuzz/fuzzer.c @@ -377,7 +377,8 @@ static int fuzz_csv_reader(const char* data, size_t size) { if (reader) { /* Consume all of the reader as an iterator */ PyObject* parsed_line; - while (PyIter_NextItem(reader, &parsed_line) == 0) { + int err; + while ((parsed_line = PyIter_NextItem(reader, &err))) { Py_DECREF(parsed_line); } } diff --git a/Objects/abstract.c b/Objects/abstract.c index 3fdb5ed9a9b9e2..9e6edcc7237960 100644 --- a/Objects/abstract.c +++ b/Objects/abstract.c @@ -2837,23 +2837,25 @@ PyAIter_Check(PyObject *obj) * If the iteration terminates normally, set *item to NULL and clear * the PyExc_StopIteration exception (if it was set). */ -int -PyIter_NextItem(PyObject *iter, PyObject **item) +PyObject* +PyIter_NextItem(PyObject *iter, int *err) { - *item = (*Py_TYPE(iter)->tp_iternext)(iter); - if (*item == NULL) { + PyObject *item = (*Py_TYPE(iter)->tp_iternext)(iter); + if (item == NULL) { PyThreadState *tstate = _PyThreadState_GET(); if (_PyErr_Occurred(tstate)) { if (_PyErr_ExceptionMatches(tstate, PyExc_StopIteration)) { _PyErr_Clear(tstate); - *item = NULL; + item = NULL; } else { - return -1; + *err = -1; + return NULL; } } } - return 0; + *err = 0; + return item; } /* Return next item. @@ -2866,11 +2868,8 @@ PyIter_NextItem(PyObject *iter, PyObject **item) PyObject * PyIter_Next(PyObject *iter) { - PyObject *result; - if (PyIter_NextItem(iter, &result) < 0) { - return NULL; - } - return result; + int err; + return PyIter_NextItem(iter, &err); } PySendResult From 2c14604153372ec0ecd16dc6f38446bc5ee958b2 Mon Sep 17 00:00:00 2001 From: Irit Katriel Date: Fri, 2 Jun 2023 14:05:49 +0100 Subject: [PATCH 12/13] if(err) --- Doc/c-api/iter.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Doc/c-api/iter.rst b/Doc/c-api/iter.rst index ecdc010083db4c..0f79496940e37b 100644 --- a/Doc/c-api/iter.rst +++ b/Doc/c-api/iter.rst @@ -48,7 +48,7 @@ something like this:: Py_DECREF(iterator); - if (err < 0) { + if (err) { /* propagate error */ } else { From a6d58abb6bb94214ffabad1af4729550fdaece60 Mon Sep 17 00:00:00 2001 From: Irit Katriel Date: Fri, 2 Jun 2023 14:24:11 +0100 Subject: [PATCH 13/13] Revert "if(err)" This reverts commit 2c14604153372ec0ecd16dc6f38446bc5ee958b2. --- Doc/c-api/iter.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Doc/c-api/iter.rst b/Doc/c-api/iter.rst index 0f79496940e37b..ecdc010083db4c 100644 --- a/Doc/c-api/iter.rst +++ b/Doc/c-api/iter.rst @@ -48,7 +48,7 @@ something like this:: Py_DECREF(iterator); - if (err) { + if (err < 0) { /* propagate error */ } else {