-
Notifications
You must be signed in to change notification settings - Fork 2.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Python 3.11+: Add __notes__
to error_already_set::what()
output.
#4678
Changes from all commits
6eace1a
5d349f6
5751217
6081628
732fc7b
edd6026
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -471,13 +471,24 @@ inline const char *obj_class_name(PyObject *obj) { | |
|
||
std::string error_string(); | ||
|
||
// The code in this struct is very unusual, to minimize the chances of | ||
// masking bugs (elsewhere) by errors during the error handling (here). | ||
// This is meant to be a lifeline for troubleshooting long-running processes | ||
// that crash under conditions that are virtually impossible to reproduce. | ||
// Low-level implementation alternatives are preferred to higher-level ones | ||
// that might raise cascading exceptions. Last-ditch-kind-of attempts are made | ||
// to report as much of the original error as possible, even if there are | ||
// secondary issues obtaining some of the details. | ||
struct error_fetch_and_normalize { | ||
// Immediate normalization is long-established behavior (starting with | ||
// https://github.com/pybind/pybind11/commit/135ba8deafb8bf64a15b24d1513899eb600e2011 | ||
// from Sep 2016) and safest. Normalization could be deferred, but this could mask | ||
// errors elsewhere, the performance gain is very minor in typical situations | ||
// (usually the dominant bottleneck is EH unwinding), and the implementation here | ||
// would be more complex. | ||
// This comment only applies to Python <= 3.11: | ||
// Immediate normalization is long-established behavior (starting with | ||
// https://github.com/pybind/pybind11/commit/135ba8deafb8bf64a15b24d1513899eb600e2011 | ||
// from Sep 2016) and safest. Normalization could be deferred, but this could mask | ||
// errors elsewhere, the performance gain is very minor in typical situations | ||
// (usually the dominant bottleneck is EH unwinding), and the implementation here | ||
// would be more complex. | ||
// Starting with Python 3.12, PyErr_Fetch() normalizes exceptions immediately. | ||
// Any errors during normalization are tracked under __notes__. | ||
explicit error_fetch_and_normalize(const char *called) { | ||
PyErr_Fetch(&m_type.ptr(), &m_value.ptr(), &m_trace.ptr()); | ||
if (!m_type) { | ||
|
@@ -492,6 +503,14 @@ struct error_fetch_and_normalize { | |
"of the original active exception type."); | ||
} | ||
m_lazy_error_string = exc_type_name_orig; | ||
#if PY_VERSION_HEX >= 0x030C0000 | ||
// The presence of __notes__ is likely due to exception normalization | ||
// errors, although that is not necessarily true, therefore insert a | ||
// hint only: | ||
if (PyObject_HasAttrString(m_value.ptr(), "__notes__")) { | ||
m_lazy_error_string += "[WITH __notes__]"; | ||
} | ||
#else | ||
// PyErr_NormalizeException() may change the exception type if there are cascading | ||
// failures. This can potentially be extremely confusing. | ||
PyErr_NormalizeException(&m_type.ptr(), &m_value.ptr(), &m_trace.ptr()); | ||
|
@@ -506,12 +525,12 @@ struct error_fetch_and_normalize { | |
+ " failed to obtain the name " | ||
"of the normalized active exception type."); | ||
} | ||
#if defined(PYPY_VERSION_NUM) && PYPY_VERSION_NUM < 0x07030a00 | ||
# if defined(PYPY_VERSION_NUM) && PYPY_VERSION_NUM < 0x07030a00 | ||
// This behavior runs the risk of masking errors in the error handling, but avoids a | ||
// conflict with PyPy, which relies on the normalization here to change OSError to | ||
// FileNotFoundError (https://github.com/pybind/pybind11/issues/4075). | ||
m_lazy_error_string = exc_type_name_norm; | ||
#else | ||
# else | ||
if (exc_type_name_norm != m_lazy_error_string) { | ||
std::string msg = std::string(called) | ||
+ ": MISMATCH of original and normalized " | ||
|
@@ -523,6 +542,7 @@ struct error_fetch_and_normalize { | |
msg += ": " + format_value_and_trace(); | ||
pybind11_fail(msg); | ||
} | ||
# endif | ||
#endif | ||
} | ||
|
||
|
@@ -558,6 +578,40 @@ struct error_fetch_and_normalize { | |
} | ||
} | ||
} | ||
#if PY_VERSION_HEX >= 0x030B0000 | ||
auto notes | ||
= reinterpret_steal<object>(PyObject_GetAttrString(m_value.ptr(), "__notes__")); | ||
if (!notes) { | ||
PyErr_Clear(); // No notes is good news. | ||
} else { | ||
auto len_notes = PyList_Size(notes.ptr()); | ||
if (len_notes < 0) { | ||
result += "\nFAILURE obtaining len(__notes__): " + detail::error_string(); | ||
} else { | ||
result += "\n__notes__ (len=" + std::to_string(len_notes) + "):"; | ||
for (ssize_t i = 0; i < len_notes; i++) { | ||
PyObject *note = PyList_GET_ITEM(notes.ptr(), i); | ||
auto note_bytes = reinterpret_steal<object>( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn't this be reinterpret_borrow? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is copy-pasted from the existing code further up (btw I didn't see enough meat to factor out). https://docs.python.org/3/c-api/unicode.html#c.PyUnicode_AsEncodedString returns a new reference, we have to take ownership. (Unfortuantely IMO) it's called "steal" instead of "take_ownership", I think this is correct. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oops, nvm. Misread this and didn't notice the PyUnicode_AsEncodedString. You are 100% right. |
||
PyUnicode_AsEncodedString(note, "utf-8", "backslashreplace")); | ||
if (!note_bytes) { | ||
result += "\nFAILURE obtaining __notes__[" + std::to_string(i) | ||
+ "]: " + detail::error_string(); | ||
} else { | ||
char *buffer = nullptr; | ||
Py_ssize_t length = 0; | ||
if (PyBytes_AsStringAndSize(note_bytes.ptr(), &buffer, &length) | ||
== -1) { | ||
result += "\nFAILURE formatting __notes__[" + std::to_string(i) | ||
+ "]: " + detail::error_string(); | ||
} else { | ||
result += '\n'; | ||
result += std::string(buffer, static_cast<std::size_t>(length)); | ||
} | ||
} | ||
} | ||
} | ||
} | ||
#endif | ||
} else { | ||
result = "<MESSAGE UNAVAILABLE>"; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this version check necessary? I know it should only be true for python 3.11 and above, but the hasattrstring is implicitly a version check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm thinking of it as a very easy runtime optimization, eliding the runtime overhead for the attribute lookup. — A while ago I looked at the implementation and it's actually quite involved. It basically does a full
getattr()
equivalent withPyErr_Clear()
.