Address reviews and fix a bug

the bug: 'item' was not set to NULL when an incorrect type was given
python · erlend-aasland · Aug 7, 2024 · Jul 26, 2024 · Jul 26, 2024 · Jul 27, 2024
commit c5800e6e03cc382cfbc549516746f86654eac418
diff --git a/Doc/c-api/iter.rst b/Doc/c-api/iter.rst
@@ -37,13 +37,16 @@ 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);
           Py_DECREF(item);
       }
       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
@@ -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
@@ -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
@@ -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
@@ -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,15 +2915,15 @@ 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;
     }
 
     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