Avoid blindly shadowing existing exceptions through `PyErr_Format`

[picnixz]

Feature or enhancement

Proposal:

TL;DR: I am not suggesting adding the assert at runtime. I am only suggesting that we avoid setting exceptions if another exception is already set, unless we explicitly clear that one before calling PyErr_Format.

TL;DR 2: This is NOT an easy work and I plan to work on it myself.

I've seen many times where PyErr_* functions are called to set an exception while another exception is already set. It doesn't cause a crash because we always clear the error indicator in PyErr_Format since we need to use PyUnicode_FromFormatV. However, this means that we may possibly be hiding some exceptions that actually shouldn't be hidden.

One example is PyLong_FromUnicodeObject which calls PyLong_FromString and sets an exception on failure that is identical to the one that could be set by PyLong_FromString. In particular, we actually do not propagate a MemoryError from PyLong_FromString (which happens if PyUnicode_FromStringAndSize called in PyLong_FromString fails).

I am currently investigating the other cases, but many tests would be failing if I add an assert(!PyErr_Occurred()); inside PyErr_Format() (mostly because of double-setting the same exception). I would suggest that we first try to change the functions that actually set double exceptions, and then explicitly document that PyErr_Format shouldn't be called with an exception set as it will replace the latter.

Analysis

I was more or less able to fix assert(!PyErr_Occurred()); and the patterns I've seen until are the following:

Exact matching exceptions overwritten

Sometimes, we explicitly match some exception types and then overwrite the exception. This happens in the numerical API with the _range_error function.

I've created something locally for that one but won't open a PR for now. The idea is to just look at places that match some specific exceptions, clear it, and then set the new exception. The intent here was anyway to clear any existing exception.

Exceptions redundantly overwritten

Sometimes, we overwrite TypeError or ValueError exceptions with either the ... same exception message (because one C API function calls another C API, and both of them need to set exceptions as they can be used distinctly). The solution to that is to read the code and only set a "fresh" exception in the caller if no error is already set (it's cheaper than checking if the callee's exception matches a specific type).

Exceptions in Python/getargs.c

The parsing process in getargs.c is messy on the exception handling side. Whenever there is an error, we use some converterr function, which is only here for creating error messages. However, we don't "chain" possible exceptions here. If during the conversion, the C API is called and failed, then we overwrite that error with a more general error saying "cannot convert" or something like this. I think I'll need to rewrite the entire logic of setting exceptions to know whether we should set a new exception message or not depending on what we are currently parsing. This will be a separate PR.

Exceptions from PyBuffer_GetObject

The most common "bad" occurrences I found are related to the usage of PyBuffer_GetObject. As per PEP-3118, any object implementing the Buffer API in C must use PyExc_BuffreError if the request cannot be fulfilled and return -1 (and it's highlighted in the docs https://docs.python.org/3/c-api/buffer.html#c.PyObject_GetBuffer with a huge MUST, so it's not a "maybe", it's explicit). In other words, if a custom implementation uses the C API, all their exceptions must be converted into PyExc_BufferError. I'm not sure that all implementations respect this, as it's easy to simply propagate the exception.

Unfortunately, almost all places that use PyBuffer_GetObject and set an exception afterwards disregard a possible PyExc_BuffreError that could have happened and overwrite any exception by usually raising a simple TypeError. Otherwise, they just return -1 and propagate any existing exception (which is fine). For those overwriting the exception, this is not good. Instead, I decided on the following approach:

• If PyBuffer_GetObject raised a TypeError, we can safely overwrite it. We may lose information though if it was raised not from the base implementation of PyBuffer_GetObject but if it was raised from a custom implementation. However, there is currently no way to distinguish between an exception set outside CPython and an exception set by CPython. We could try checking against the matched messages but if we were to change the error message, then the entire machinery collapses.
• If PyBuffer_GetObject raised a non-TypeError exception and we actually overwrite with a TypeError, I suggest adding that exception as a __context__ of that TypeError. There shouldn't be cycles here because the TypeError would be newly created. I doubt people would be happy if I were to add some eager assert about the fact that only BufferError can be raised from PyBufferProcs.bf_getbuffer so I'll abstain for now. Maybe adding a warning would be a good idea in the first place (but it may become annoying as most of the time, this warning would come up in extension modules that extensively use the Buffer API, and usually outside the user's control).

Has this already been discussed elsewhere?

This is a minor feature, which does not need previous discussion elsewhere

Links to previous discussion of this feature:

There was a previous suggestion here: #67759 where it was mentioned that "PyUnicode_FromFormatV() must not be called with an exception set".

[ronaldoussoren]

None of the APIs for raising an exception currently require that there is no active exception. I'd expect that introducing such a requirement will be problematic for 3th-party code.

Personally I'd be more interested in API variants that (can) automatically chain exceptions (that is, what happens when you raise an exception in an exception handler, and/or raise ... from ...). Doing that currently requires boilerplate (fetch the current exception, raise a new one, then set the cause or context on the newly raised exception).

[sobolevn]

We can try to create a build flag / env var for the internal development that will help us finding these cases by setting an assert. Sounds like a nice debugging tool to me.

[picnixz]

None of the APIs for raising an exception currently require that there is no active exception. I'd expect that introducing such a requirement will be problematic for 3th-party code.

I am not suggestng changjng the API in any way. I am suggesting that we clean up our internal code only. Strictly speaking there is no requirement because we are actually internally clearing the exception already! The reason is that we are calling the C API to set the exception and this cannot be done with an exception set.

[picnixz]

Personally I'd be more interested in API variants that (can) automatically chain exceptions (that is, what happens when you raise an exception in an exception handler, and/or raise ... from ...). Doing that currently requires boilerplate (fetch the current exception, raise a new one, then set the cause or context on the newly raised exception).

The problem is that raising a new exception may require more than just a format call, and all the code that is constructing the new exception must be done after getting the active exception and clearing the error indicator as we could call the C API in-between.

I however wouldn't mind having those new functions but I don't know if we will be able to use them internally as much as we can.

then set the cause or context on the newly raised exception

This is no longer needed if you use _PyErr_ChainExceptions1. You however need to retrieve the active exception beforehand.

[picnixz]

FTR, here are all the test suites that currently fail if I add an assert(!PyErr_Occurred()) in PyErr_Format implementation:

	test.test_asyncio.test_base_events
    test.test_io.test_file test.test_io.test_fileio
    test.test_io.test_general test.test_io.test_memoryio
    test_abstract_numbers test_argparse test_buffer test_builtin
    test_bytes test_capi test_class test_configparser test_contains
    test_coroutines test_crossinterp test_csv test_dataclasses
    test_datetime test_dbm_dumb test_descr test_dis test_enum
    test_float test_format test_fractions test_frame test_funcattrs
    test_grp test_httplib test_int test_interpreters test_ipaddress
    test_iter test_long test_memoryview test_ntpath test_opcache
    test_opcodes test_operator test_os test_pathlib test_pdb
    test_peepholer test_pickle test_pickletools test_pkgutil
    test_poplib test_posix test_posixpath test_profiling test_pwd
    test_random test_re test_regrtest test_select test_shutil
    test_socket test_source_encoding test_sqlite3 test_statistics
    test_str test_strtod test_struct test_sys test_tempfile
    test_timeit test_types test_unicodedata test_unittest test_unpack
    test_unpack_ex test_urllib test_userstring test_uuid test_warnings
    test_weakref test_xml_etree test_xml_etree_c test_xmlrpc
    test_xxtestfuzz test_zoneinfo test_zstd

Note that most of the exceptions are because we don't clear the error indicator after int() or bytes() conversions (usually because of PyNumber_Long or other C API-like functions).

I'm pretty sure that once I fix the numeric API, I'll see other failures that are currently hidden (since the teste early abort) but it is likely a long task.

[sergey-miryanov]

This is no longer needed if you use _PyErr_ChainExceptions1. You however need to retrieve the active exception beforehand.

I did such helper in this PR https://github.com/python/cpython/pull/132970/files#diff-08adf6717de7c662b4e6960c5c428e6c46b1ef5370b562a4b0c92ecb071ce733R48. I think it would be great if it (or similar one) will be used in other parts of CPython because I can remember other places where it will be useful.

[picnixz]

Well, if the non-test files can benefit from it, I think it'd be a fine addition. However, the question is then: should we have a SetString + SetFormat + SetFormatV + SetObject interface as well? at first we could have an internal one though, but most of the time, production-like chained exceptions are not just simple string messages.

[picnixz]

Ok, so I was more or less able to fix assert(!PyErr_Occurred()); and the patterns I've seen until are the following:

Exact matching exceptions overwritten

Sometimes, we explicitly match some exception types and then overwrite the exception. This happens in the numerical API with the _range_error function.

I've created something locally for that one but won't open a PR for now. The idea is to just look at places that match some specific exceptions, clear it, and then set the new exception. The intent here was anyway to clear any existing exception.

Exceptions redundantly overwritten

Sometimes, we overwrite TypeError or ValueError exceptions with either the ... same exception message (because one C API function calls another C API, and both of them need to set exceptions as they can be used distinctly). The solution to that is to read the code and only set a "fresh" exception in the caller if no error is already set (it's cheaper than checking if the callee's exception matches a specific type).

Exceptions in Python/getargs.c

The parsing process in getargs.c is messy on the exception handling side. Whenever there is an error, we use some converterr function, which is only here for creating error messages. However, we don't "chain" possible exceptions here. If during the conversion, the C API is called and failed, then we overwrite that error with a more general error saying "cannot convert" or something like this. I think I'll need to rewrite the entire logic of setting exceptions to know whether we should set a new exception message or not depending on what we are currently parsing. This will be a separate PR.

Exceptions from PyBuffer_GetObject

The most common "bad" occurrences I found are related to the usage of PyBuffer_GetObject. As per PEP-3118, any object implementing the Buffer API in C must use PyExc_BuffreError if the request cannot be fulfilled and return -1 (and it's highlighted in the docs https://docs.python.org/3/c-api/buffer.html#c.PyObject_GetBuffer with a huge MUST, so it's not a "maybe", it's explicit). In other words, if a custom implementation uses the C API, all their exceptions must be converted into PyExc_BufferError. I'm not sure that all implementations respect this, as it's easy to simply propagate the exception.

Unfortunately, almost all places that use PyBuffer_GetObject and set an exception afterwards disregard a possible PyExc_BuffreError that could have happened and overwrite any exception by usually raising a simple TypeError. Otherwise, they just return -1 and propagate any existing exception (which is fine). For those overwriting the exception, this is not good. Instead, I decided on the following approach:

• If PyBuffer_GetObject raised a TypeError, we can safely overwrite it. We may lose information though if it was raised not from the base implementation of PyBuffer_GetObject but if it was raised from a custom implementation. However, there is currently no way to distinguish between an exception set outside CPython and an exception set by CPython. We could try checking against the matched messages but if we were to change the error message, then the entire machinery collapses.
• If PyBuffer_GetObject raised a non-TypeError exception and we actually overwrite with a TypeError, I suggest adding that exception as a __context__ of that TypeError. There shouldn't be cycles here because the TypeError would be newly created. I doubt people would be happy if I were to add some eager assert about the fact that only BufferError can be raised from PyBufferProcs.bf_getbuffer so I'll abstain for now. Maybe adding a warning would be a good idea in the first place (but it may become annoying as most of the time, this warning would come up in extension modules that extensively use the Buffer API, and usually outside the user's control).