PyIter_Next has ambiguous return value

As discussed in capi-workgroup/problems#1, we have some C API functions that have ambiguous return values, requiring the caller to query PyErr_Occurred() to find out whether there was an error.

We will try to move away from those APIs to alternative ones whose return values non-ambiguously indicate whether there has been an error, without requiring the user to call PyErr_Occurred().

In this issue we will discuss the iterator API. PyIter_Next return NULL for both error and for the iterator being exhausted. PyErr_Occurred() distinguishes between the cases.

Linked PRs

pythongh-105201: Add PyIter_NextItem to replace PyIter_Next which has…

Before we rush to introduce new APIs, can we clarify the following:

We will try to move away from those APIs to alternative ones whose return values non-ambiguously indicate whether there has been an error.

I interpret this as:

for function retuning int, 0 is a successful call, -1 is an error
for function returning PyObject pointer, NULL is an error, anything else is ok

Error means a raised exception.

I don't think it's a good API for an iterator if you need to check 2 things just to know whether iteration completed, and then after the loop you need to check again why it was completed.

Perhaps iterator APIs should get their own issue, since they require ambiguous return values? It seems to me they don't fit in the premises established by the OP.

Sure, let make this issue about iterators.

My proposal is to add a new PyObject *PyIter_NextItem(PyObject* iter, int *err) which returns the same as PyIter_Next, but also sets *err to 0 in success and -1 on error.

The reason to do this and not int PyIter_NextItem(PyObject* iter, PyObject **item) is because of how this function is used while looping over an iterator: for each iteration we want to know whether we got another value or not. Only after (when we got a NULL) we want to check the error value to see how the iteration exited.

So I am proposing:

PyObject *item;
int err;
while (item = PyIter_NextItem(iterator, &err)) {
    /* do something with item */
    ...
    /* release reference when done */
    Py_DECREF(item);
}
Py_DECREF(iterator);
if (err < 0) {
    /* error */
}
else {
    /* no error */
}

rather than

PyObject *item;
int err;
while ((err = PyIter_NextItem(iterator, &item) ) == 0 && *item != NULL) {
  /* do something with item */
  ...
  /* release reference when done */
  Py_DECREF(item);
}
Py_DECREF(iterator);
if (err < 0) {
  /* error */
}
else {
  /* no error */
}

@encukou

CC @encukou

I still fail to see how the problem "API A has ambiguous return value" is solved by introducing API B which also has an ambiguous return value.

I’m giving up on incrementally fixing this in the current c api and will turn my attention to the new c api work. Those who warned me were right, unfortunately.

(See discussion on the attached PR for the full picture).

I’m giving up on incrementally fixing this in the current c api and will turn my attention to the new c api work. Those who warned me were right, unfortunately.

Isn't this overreacting? Ignoring Raymond's knee-jerk reaction, Erlend's feedback points to a real issue with how we've been talking about this, but is not a reason to give up on incremental fixes. The way I read Erlend's feedback is that the problem isn't that the result or return value is ambiguous, but that it requires calling PyErr_Occurred() to get the full picture. There are a few ways we can design the replacement for PyIter_Next() to avoid PyErr_Occurred(), and we simply have to look at the ergonomics to choose one. That seems a rational choice we can make.

How would you fix PyUnicode_Compare?

PyUnicode_Compare can only fail because it has the wrong type signature.
Change it from int PyUnicode_Compare(PyObject *left, PyObject *right) to
int PyUnicode_Compare(PyUnicodeObject *left, PyUnicodeObject *right) and it becomes infallible.

Assuming you want to keep the more general signature, then yes it does need more return values.
There are in fact five possible return values: error, less, equal, more, unordered. The last for sets, floats, etc.
In which case I would use the following enum:

enum {
     ERROR = -1,
     UNORDERED = 1,
     LESS_THAN = 2,
     GREATER_THAN = 4
     EQUAL = 8
}

See https://github.com/python/cpython/blob/main/Include/internal/pycore_code.h#L469 for why this seemingly odd choice makes sense.

As to what the return values should be:

-1 for an error. As we use that convention everywhere

0 for a "lesser" result, which in this case is termination (for PyDict_GetItem, the case where the key is absent).

1 for the "greater" result, the presence of a value (for PyDict_GetItem, the case where the key is present).

That sounds like a good direction.
A general guideline should also suggest when *result should be set. For the “lesser” case, do we leave it untouched, set it to NULL, or say that users shouldn't use it? What about the “error” case?

For PyDict_GetItem, it seems ergonomic enough to do:

-1 for an error, everywhere. *result is undefined, may be garbage, don't touch it.
0 for success
- result is NULL, where the key is absent
- result is non-NULL if found

In a PyWeakref_GetRef-style function, determining whether to return 1 or 0 is a (admittedly tiny) bit of extra work that most callers could ignore. Is it worth the consistency? Or should __next__-style function be the odd ones that return 1 for convenience in a while loop?

For PyDict_GetItem, I'd prefer to return 0 for not found, and 1 for found.
This seems more consistent; not found is the "lesser" result.
It also reduces memory traffic a bit, as *result is only touched if the value is found.

For example, implementing a version of dict.get, where the key is expected to be missing:

PyObject *
dict_get_probably_missing(PyDictObject *self, PyObject *key, PyObject *default)
{
   
   PyObject *val;
   int res = PyDict_GetItem(self, key, &val);
   if (res == 0) {
       return Py_NewRef(default);
   }
   if (res > 0) {
        return val;
   }
   else {
       return NULL;
   }
}

PyWeakref_GetRef does a lot of work to check that the weakref is not NULL and is a weak ref.
See capi-workgroup/problems#31

Can we please open a devguide issue for establishing and documenting these API guidelines, and then move the general discussion we're now having over there? When that discussion has landed, and guidelines are documented, we can ~~start~~ continue fixing existing problematic APIs by adding new APIs according to the established guidelines.

I'm not going to pursue this.

The C API WG voted on and agreed on the following API:

int PyIter_NextItem(PyObject *iter, PyObject **item)

Return -1 on error; raise an exception and set *item to NULL
Return 0 and set *item to NULL on StopIteration
Return 1 and set *item to a strong reference to the iterator item on success

iter and item cannot be NULL.

pythongh-105201: Add PyIter_NextItem()

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

pythongh-105201: Add PyIter_NextItem() (python#122331)

Navigation Menu

Repository navigation

PyIter_Next has ambiguous return value #105201

Linked PRs

6 remaining items

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

PyIter_Next has ambiguous return value #105201

Description

Linked PRs

Activity

erlend-aasland commented on Jun 2, 2023

iritkatriel commented on Jun 2, 2023

erlend-aasland commented on Jun 2, 2023

iritkatriel commented on Jun 2, 2023

iritkatriel commented on Jun 2, 2023

iritkatriel commented on Jun 2, 2023

erlend-aasland commented on Jun 2, 2023

iritkatriel commented on Jun 4, 2023

iritkatriel commented on Jun 4, 2023

gvanrossum commented on Jun 5, 2023

6 remaining items

markshannon commented on Jun 6, 2023

markshannon commented on Jun 6, 2023

encukou commented on Jun 21, 2023

markshannon commented on Jun 22, 2023

markshannon commented on Jun 22, 2023

erlend-aasland commented on Jun 22, 2023

iritkatriel commented on Aug 23, 2023

erlend-aasland commented on Jul 24, 2024

Metadata

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Issue actions