gh-105201: Add PyIter_NextItem() (#122331)

erlend-aasland · web-flow · commit e006c7371d8e · 2024-08-08T00:47:15.000+02:00
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 exhaustion and errors.

Co-authered-by: Irit Katriel &lt;iritkatriel@yahoo.com&gt;
diff --git 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,41 +20,27 @@ There are two functions specifically for working with iterators.
 
    .. versionadded:: 3.10
 
+.. c:function:: int PyIter_NextItem(PyObject *iter, PyObject **item)
+
+   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.
+
+   .. versionadded:: 3.14
+
 .. 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.
 
-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))) {
-       /* do something with item */
-       ...
-       /* release reference when done */
-       Py_DECREF(item);
-   }
-
-   Py_DECREF(iterator);
-
-   if (PyErr_Occurred()) {
-       /* propagate error */
-   }
-   else {
-       /* continue doing useful work */
-   }
-
-
 .. c:type:: PySendResult
 
    The enum value used to represent different results of :c:func:`PyIter_Send`.
diff --git a/Doc/data/refcounts.dat b/Doc/data/refcounts.dat
@@ -1132,6 +1132,10 @@ PyAIter_Check:PyObject*:o:0:
 PyIter_Next:PyObject*::+1:
 PyIter_Next:PyObject*:o:0:
 
+PyIter_NextItem:int:::
+PyIter_NextItem:PyObject*:iter:0:
+PyIter_NextItem: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
diff --git a/Doc/whatsnew/3.14.rst b/Doc/whatsnew/3.14.rst
@@ -404,6 +404,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/Include/abstract.h b/Include/abstract.h
@@ -397,13 +397,23 @@ 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.
 
    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.
+
+   Prefer 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_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_stable_abi_ctypes.py b/Lib/test/test_stable_abi_ctypes.py
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
@@ -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.
diff --git 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.14'
diff --git 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/Objects/abstract.c b/Objects/abstract.c
@@ -2881,7 +2881,50 @@ PyAIter_Check(PyObject *obj)
             tp->tp_as_async->am_anext != &_PyObject_NextNotImplemented);
 }
 
+static int
+iternext(PyObject *iter, PyObject **item)
+{
+    iternextfunc tp_iternext = Py_TYPE(iter)->tp_iternext;
+    if ((*item = tp_iternext(iter))) {
+        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 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 (Py_TYPE(iter)->tp_iternext == NULL) {
+        *item = NULL;
+        PyErr_Format(PyExc_TypeError, "expected an iterator, got '%T'", iter);
+        return -1;
+    }
+
+    return iternext(iter, item);
+}
+
 /* Return next item.
+ *
  * 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 +2934,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

Original file line number	Diff line number	Diff line change
`@@ -0,0 +1,2 @@`
	`1`	+Add :c:func:`PyIter_NextItem` to replace :c:func:`PyIter_Next`, which has an
	`2`	`+ambiguous return value. Patch by Irit Katriel and Erlend Aasland.`