Improve C API guidelines for return values

[erlend-aasland]

Clarify that returning an error indicator should be done iff an
exception is raised.

For functions returning an int, a negative value should be returned.
Suggest -1 unless there is a need to distinguish between error types.

---

📚 Documentation preview 📚: https://cpython-devguide--1129.org.readthedocs.build/

[erlend-aasland]

@encukou, are you ok with this change?

[vstinner]

LGTM

[erlend-aasland]

Thanks, Victor and Adam.

[vstinner]

Sadly, the newly added PyLong_AsInt() doesn't respect new guidelines, but it was a deliberate choice. I like the new PyDict_GetItemRef() API, it's pleasant to use it.

[encukou]

Well, I've thought about this, and realized I haven't seen an actual use case for distinguishing between error types. Is there one?
It seems that if an “error” is expected, like with “missing dict key”, that should be handled as another “success” (positive return value), so we don't need to allocate/init an exception object.
That leaves negative return values for “unexpected” errors, where the function can't really distinguish.

I'd love to see a counterexample :)

the newly added PyLong_AsInt() doesn't respect new guidelines

Well, it doesn't respect the old guidelines either, so that's unrelated to this PR :)

[vstinner]

Well, I've thought about this, and realized I haven't seen an actual use case for distinguishing between error types. Is there one?

It's not common to report different error cases, but it happens, especially if the API does not raise an exception.

On error, Py_DecodeLocale() sets size to (size_t)-1 on memory allocation failure and (size_t)-2 on decoding error.

_Py_DecodeLocaleEx() has 3 error cases:

• return -1: memory allocation failure
• return -2: decoding error
• return -3: unsupported error handler

For the caller, it's important to be able to distinguish the different errors. Extract of unicode_decode_locale():

    int res = _Py_DecodeLocaleEx(str, &wstr, &wlen, &reason,
                                 current_locale, errors);
    if (res != 0) {
        if (res == -2) {
            PyObject *exc;
            exc = PyObject_CallFunction(PyExc_UnicodeDecodeError, "sy#nns",
                                        "locale", str, len,
                                        (Py_ssize_t)wlen,
                                        (Py_ssize_t)(wlen + 1),
                                        reason);
            if (exc != NULL) {
                PyCodec_StrictErrors(exc);
                Py_DECREF(exc);
            }
        }
        else if (res == -3) {
            PyErr_SetString(PyExc_ValueError, "unsupported error handler");
        }
        else {
            PyErr_NoMemory();
        }
        return NULL;
    }

[erlend-aasland]

Well, I've thought about this, and realized I haven't seen an actual use case for distinguishing between error types. Is there one?

I'd prefer if the guideline simply said "return -1 on error"; after all a guideline is meant for the general case, not the special case. For the extraordinary cases, it is ok to not follow the general guideline (after a discussion, of course).

[encukou]

It's not common to report different error cases, but it happens, especially if the API does not raise an exception.

Well, if an exception isn't raised, then the function should not return a negative value (or NULL) at all.
That's the main consistency win we're going for: you should be able to tell whether an exception was raised from the return value alone.

---

There are two parts of the question of “Does a negative value other than -1 signal that an exception was raised?”

• what should functions return? (we can say -1 only for now, it's easy to allow more values later)
• what should callers check for, == -1 or < 0? (on this we can't really change our minds later)

[erlend-aasland]

There are two parts of the question of “Does a negative value other than -1 signal that an exception was raised?”

• what should functions return? (we can say -1 only for now, it's easy to allow more values later)
• what should callers check for, == -1 or < 0? (on this we can't really change our minds later)

I would prefer callers to check for < 0 (any negative value implies an exception was raised). For C API implementers, I'd prefer simply to suggest using the return value is -1 (we're only talking about int returns here) if an exception is raised. If it makes it easier to formulate this, I'm fine with simply saying "return a negative value" instead of "return -1".

[encukou]

Yeah, return -1 and if (... < 0) is probably the best solution. It means all the negative return values except -1 probably can't be repurposed for anything useful in the future, but that's probably OK.

But perhaps this shouldn't be decided in an issue, especially since there'll be lots of API discussions at the sprint in about a month.

[erlend-aasland]

But perhaps this shouldn't be decided in an issue, especially since there'll be lots of API discussions at the sprint in about a month.

Well, this PR has been up for two months already; there's no harm in waiting another month :)

[vstinner]

Even when I'm 100% sure that a function can only return -1 on error, I prefer to write if (func() < 0). For me, it's a way to say: "if the function fails".

In the C library, it's unclear if a function returns 0 or 1 on success. It depends. But in the Python C API, it's likely that a function returns "a negative number" on error.

[erlend-aasland]

Even when I'm 100% sure that a function can only return -1 on error, I prefer to write if (func() < 0). For me, it's a way to say: "if the function fails".

Yeah, that's the C idiom, so that would be my preferred style.

[vstinner]

If a function can raise an exeeption, I don't see the point of having more than one negative value. The exception should contain the details: exception type and exception message.

In the worst case, if a very important additonal information cannot be passed in the exception, i would prefer to add a separated argument, than break the rule: return 1, 0 or -1.

[vstinner]

My Py_DecodeLocale() example is unrelated: it doesn't raise an exception, and so uses a different API. I'm talking about public C API which raise exceptions.

[erlend-aasland]

I'll leave this for the C API workgroup.