gh-128198: Add missing error checks for usages of PyIter_Next

[WolframAlph]

• Issue: Add missing error checks for usages of PyIter_Next #128198

[ZeroIntensity]

In 3.14, we have a new PyIter_NextItem for exactly this problem. I think it's worth migrating to that in this PR instead of trying to adapt to the bad design choices of PyIter_Next.

[WolframAlph]

👍 Will check it out and update the PR

[WolframAlph]

@ZeroIntensity I can see PyIter_NextItem being used only in couple places. Should I rename the issue to indicate migration from old PyIter_Next to new PyIter_NextItem api and work on replacing old one in the entire codebase?

[ZeroIntensity]

I don't think we need to replace PyIter_Next throughout the entire codebase (though it is probably inevitable), just change it for the cases that are problematic.

Should I rename the issue to indicate migration from old PyIter_Next

Nah, just update the PR title.

[WolframAlph]

@ZeroIntensity so if this is inevitable (otherwise why would new PyIter_NextItem be implemented) and I can see ~63 instances of PyIter_Next usage in the codebase, I would like to spend time on migrating to the new version in this PR. Or at least those instances that do not require significant rewrite and don't introduce more complexity. Any objections from your side? Or if this is still too much for a single PR, maybe create top level issue to track migration and add separate issues for migration in each file where usages occur?

[serhiy-storchaka]

If older Python versions have the same code, it will be needed to backport these changes. In such case it is better to use the older API, unless you want to add a work for yourself and for reviewers.

[ZeroIntensity]

Ah, right. I would be in favor of a follow-up PR to switch to PyIter_NextItem though.

[picnixz]

LGTM (I'll just run the refleaks build bots since the previous run was interrupted, just in case)

[bedevere-bot]

🤖 New build scheduled with the buildbot fleet by @picnixz for commit 3aa46e7 🤖

If you want to schedule another build, you need to add the 🔨 test-with-refleak-buildbots label again.

[WolframAlph]

Regarding migration to PyIter_NextItem, can I create top level issue and tag you guys? @serhiy-storchaka @picnixz @ZeroIntensity ?

[picnixz]

Yes, you can just request my review. But before creating the issue, you should locally verify that:

• the changes would not impact performances (I don't know which one is faster); try to check if PyIter_Next is actually faster overall or if it's PyIter_NextItem).
• some parts of the code directly use tp_iternext under some assumptions and for performance reasons. Those parts should not be modified.
• locally investigate how you want to split the task. Changing a lot of places may or may not be easy to review. There are more than 50 usages my IDE found so maybe splitting it up by file would be better.
• let's first focus on parts that don't need much rewrite; it's fine if some parts are still using the old API though.

FTR, changing the usages could perhaps make future backports harder, so I don't know whether it's worth the shot.

[WolframAlph]

locally investigate how you want to split the task. Changing a lot of places may or may not be easy to review. There are more than 50 usages my IDE found so maybe splitting it up by file would be better.

Yes, my initial thought was to create top level issue to track all sub issues for each file separately to make review process easier.

FTR, changing the usages could perhaps make future backports harder, so I don't know whether it's worth the shot.

I presume this is not first time of migrating to newer API. How were those cases handled?

[WolframAlph]

One more question. When comparing code for PyIter_Next:

cpython/Objects/abstract.c

Lines 2914 to 2919 in d9ed42b

	PyIter_Next(PyObject *iter)
	{
	PyObject *item;
	(void)iternext(iter, &item);
	return item;
	}

and PyIter_NextItem:

cpython/Objects/abstract.c

Lines 2891 to 2903 in d9ed42b

	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);
	}

I can see that latter has Py_TYPE(iter)->tp_iternext == NULL check while former does not. Is it intentional? If so - what is the reason?

[picnixz]

Yes it's intentional. Docs (https://docs.python.org/3/c-api/iter.html#c.PyIter_Next) say:

The object must be an iterator according to PyIter_Check() (it is up to the caller to check this).

The reason is to make the call more efficient when the caller knows that the object is a correct iterator. OTOH PyIter_NextItem makes the check for the caller (if Py_TYPE is a function call, this would add one function call and one pointer comparison; on incorrect inputs, we don't care about performances).

I don't know how less (or more?) efficient it would be but it might not necessarily by much. If you're really interested in preserving performance, we could first glance at the assembly and benchmarks some simple statements, though I suspect that naive benchmarks would hide too much noise (but maybe not?).

Now, if the caller is checking the input to PyIter_Next, then you can maybe remove that check and directly use PyIter_NextItem instead (in the end, you'll move the cost of the check to PyIter_NextItem, though it will cost a function call (I assume that the checks are actually inlined checks but maybe not)).

I presume this is not first time of migrating to newer API. How were those cases handled?

I don't know, I only became active with CPython in June :')

[serhiy-storchaka]

I presume this is not first time of migrating to newer API. How were those cases handled?

We migrate to newer API if it is faster, safer or more convenient. Or if there are plans to remove the older API.

There are no plans to remove PyIter_Next(), it is not broken. In theory, it may be marginally faster because it does not need to call PyErr_Occurred(), but on other hand, additional check Py_TYPE(iter)->tp_iternext == NULL can make it slower.

I would recommend to use a newer API in a new code if it makes the code clearer, and do not touch existing code (unless you have other reasons to rewrite it).

[miss-islington-app]

Thanks @WolframAlph for the PR, and @serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 3.13.
🐍🍒⛏🤖 I'm not a witch! I'm not a witch!

[miss-islington-app]

Thanks @WolframAlph for the PR, and @serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 3.12.
🐍🍒⛏🤖

[miss-islington-app]

Sorry, @WolframAlph and @serhiy-storchaka, I could not cleanly backport this to 3.13 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker 5c814c83cdd3dc42bd9682106ffb7ade7ce6b5b3 3.13

[miss-islington-app]

Sorry, @WolframAlph and @serhiy-storchaka, I could not cleanly backport this to 3.12 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker 5c814c83cdd3dc42bd9682106ffb7ade7ce6b5b3 3.12

[bedevere-app]

GH-128272 is a backport of this pull request to the 3.13 branch.

[bedevere-app]

GH-128273 is a backport of this pull request to the 3.12 branch.