Repository navigation
Ambiguous return values #1
Description
Some C API functions can return a value which may or may not indicate an error, and require disambiguation with PyErr_Occurred(). They should be replaced by new versions with unambiguous return values.
Ambiguous return values
PyCapsule_GetDestructor
PyCapsule_GetContext
PyCapsule_GetName
PyDict_GetItemWithError
PyFloat_AsDouble
PyFloat_Unpack2
PyFloat_Unpack4
PyFloat_Unpack8
PyIter_Next
PyLong_AsLong
PyLong_AsLongAndOverflow
PyLong_AsLongLong
PyLong_AsLongLongAndOverflow
PyLong_AsSsize_t
PyLong_AsUnsignedLong
PyLong_AsSize_t
PyLong_AsUnsignedLongLong
PyLong_AsUnsignedLongMask
PyLong_AsDouble
PyLong_AsVoidPtr
PyMarshal_ReadLongFromFile
PyMarshal_ReadShortFromFile
PyNumber_AsSsize_t
PyUnicode_Compare
no return value, but can set an exception that can only be checked with PyErr_Occurred:
PyMarshal_WriteObjectToFile
Activity
iritkatriel commentedon May 31, 2023
PyUnicode_Compareis an interesting one. It has only 3 possible return values: -1, 0 and 1. And -1 is ambiguous.iritkatriel commentedon Jun 1, 2023
We will track work on this here: python/cpython#105201encukou commentedon Jun 5, 2023
Maybe ambiguous return values are OK themselves, but it's the way we disambiguate that needs improving?
Maybe we should look focus on
PyErr_Occurreditself. Why is it bad? Is it slow? Does it not work well with alternative implementations? Does it prevent optimizations?Why is an
int *errargument better than a variable in the interpreter state?erlend-aasland commentedon Jun 5, 2023
IMO that's a related, but slightly different issue. I'd consider creating a separate issue for those problems.
iritkatriel commentedon Jun 5, 2023
It' not a different issue, it's this issue. And I'm sure we've already discussed it elsewhere.
Ambiguous return values force us to access globals state. This has two problems:
It forces all implementations of the API to maintain this global state, whether it makes sense for them or not.
The usual problem with global state: you can never be sure where the value came from. You think
PyIter_Nextset the error indicator, but it may have been something else in the last iteration of the loop.encukou commentedon Jun 5, 2023
Trouble is, they need to do that anyway if
PyErr_Occurredstays in the API. Which it needs to, unless we overhaul all of error handling. Ambiguous return values themselves aren't the real problem.Yup. Users ideally need assert
!PyErr_Occurred()(or save/restore it). But that's a relatively minor papercut, so, we run into a general problem with incremental improvement: if the next small step is too small relative to the disruption (deprecation cycles or proliferation of similar API), it doesn't make sense to do it (yet). Perfect is the enemy of good :(iritkatriel commentedon Jun 5, 2023
We should have an issue for that, and think how we can avoid being stuck with every mistake forever in the new api.
iritkatriel commentedon Jun 5, 2023
I don't see why. If all functions are explicit in their output on whether there was an error or not, then why should users need
PyErr_Occurred()to be part of the API? Internally the interpreter can continue to have the error in globals state, but it doesn't have to expose that through the API.iritkatriel commentedon Jun 5, 2023
We could change the title of this issue to "PyErr_Occurred() should not be part of the API".
5 remaining items
erlend-aasland commentedon Jun 22, 2023
See also python/devguide#1121
serhiy-storchaka commentedon Jun 22, 2023
I do not think that there is a serious problem urging us to to fix it. In most of these cases the value used to indicate error is an uncommon, unexpected result, so it usually does not have significant performance impact.
PyUnicode_Comparecould return -2 on error, likePyUnicode_Find.PyMarshal_*functions are legacy API, they are almost not used. It is not worth to touch them.Functions like
PyDict_GetItemWithErrorandPyIter_Nextcould return a special value (e.g. a pointer to a singleton or(PyObject*)-1) for absent value or for error. The question is what chose as an error indicator and what as a special case indicator. In any case this is incompatible with existing functions, so needs introducing new functions.An alternate approach is used in
_PyObject_LookupAttr. It returns one of three values (error, absent, exists) and requires providing an address of variable to save the result. In theory the disadvantage of this is that the compiler no longer able to use only register for the result and eliminate the stack variable.vstinner commentedon Jun 23, 2023
I don't think that this project only lists issues which MUST be fixed immediately. It's also about issues which should be avoided when designing new APIs.
markshannon commentedon Jun 23, 2023
markshannon commentedon Jun 23, 2023
Consistency is vital.
Everything should return -1 for an error, and it makes sense for comparison functions to potentially return
unordered, even if string comparisons are always ordered.vstinner commentedon Jun 23, 2023
The Python C API has a big pile of legacy APIs. Many functions with known issues are kept for "backward compatibility": remove them is not worth it. Are you suggesting to quicky shift towards a "sane only" API by eaggerly remove the legacy APIs?
I propose to design a new API which would be smaller and with no known issues: issue #54.
erlend-aasland commentedon Jun 24, 2023
See python/devguide#1123 for proposed guidelines.
encukou commentedon Jul 3, 2023
A 2016 bug for this: python/cpython#70963
encukou commentedon Oct 12, 2023
Guideline proposal: capi-workgroup/api-evolution#13
vstinner commentedon Jun 12, 2024
I propose new PyLong API which returns 0 on success and -1 on error: python/cpython#120389
serhiy-storchaka commentedon Jun 12, 2024
For converters from Python object there is another convention, used with the
O&format unit inPyArg_Parse: returning 0 on failure and non-zero on success (usually 1, but can also be Py_CLEANUP_SUPPORTED for some converters).serhiy-storchaka commentedon Jun 12, 2024
In addition to standard converters in the public C API (
PyUnicode_FSConverter,PyUnicode_FSDecoder), there is a large number of private converters (_PyLong_UnsignedShort_Converter,_Py_convert_optional_to_ssize_t,_PyEval_SliceIndex,_PyLong_FileDescriptor_Converter,_PyUnicode_WideCharString_Converter, etc).