Skip to content
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

Merged
merged 6 commits into from
May 23, 2023

Conversation

rwgk
Copy link
Collaborator

@rwgk rwgk commented May 21, 2023

Description

The original motivation for this PR was to account for a a fairly major behavior change in Python 3.12:

Python 3.12 PyErr_Fetch() normalizes exceptions immediately and tracks any normalization errors under __notes__:

__notes__ were introduced already with Python 3.11, but this was not reflected in pybind11 until now. Adding them to the error_already_set::what() output therefore catches up with Python developments in general, and ensures that exception normalization errors continue to be obvious.

A highly technical detail for completeness:

The FAILURE obtaining and FAILURE formatting conditions are impractical to exercise in the usual way via unit tests, but a manual pass was made exercising them with temporary tweaks.

Suggested changelog entry:

Python exception ``__notes__`` (introduced with Python 3.11) are now added to the ``error_already_set::what()`` output.

@rwgk rwgk added the python dev Working on development versions of Python label May 21, 2023
@rwgk rwgk marked this pull request as ready for review May 22, 2023 12:13
Copy link
Collaborator

@EthanSteinberg EthanSteinberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks mostly good, with two possible flaws

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>(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be reinterpret_borrow?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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.

Copy link
Collaborator

Choose a reason for hiding this comment

The 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.

@@ -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
Copy link
Collaborator

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.

Copy link
Collaborator Author

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 with PyErr_Clear().

@rwgk
Copy link
Collaborator Author

rwgk commented May 23, 2023

Thanks for the review @lalaland!

I just tested this PR also with Python 3.12.0b1 + two patches to make that possible (references below). I'll merge this now so that it'll be easier to keep up with 3.12 developments.


  1. Locally applied https://patch-diff.githubusercontent.com/raw/python/cpython/pull/104742.diff (unfortunately that came too late for the Python 3.12.0b1 cut).

  2. Related to gh-94673: More Per-Interpreter Fields for Builtin Static Types python/cpython#103912:

commit b66198c8345614df7ddf34402cbf5670ae298a6f (HEAD -> wrk)
Author: Ralf W. Grosse-Kunstleve <[email protected]>
Date:   Tue May 23 09:46:48 2023 -0700

    Check for `t->tp_bases == nullptr`

diff --git a/include/pybind11/detail/type_caster_base.h b/include/pybind11/detail/type_caster_base.h
index 16387506..eb7ec7a3 100644
--- a/include/pybind11/detail/type_caster_base.h
+++ b/include/pybind11/detail/type_caster_base.h
@@ -104,6 +104,10 @@ all_type_info_get_cache(PyTypeObject *type);

 // Populates a just-created cache entry.
 PYBIND11_NOINLINE void all_type_info_populate(PyTypeObject *t, std::vector<type_info *> &bases) {
+    if (t->tp_bases == nullptr) {
+        // Since https://github.com/python/cpython/pull/103912 (Python 3.12.0b1)
+        return;
+    }
     std::vector<PyTypeObject *> check;
     for (handle parent : reinterpret_borrow<tuple>(t->tp_bases)) {
         check.push_back((PyTypeObject *) parent.ptr());
diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h
index 28ebc222..db1c5a99 100644
--- a/include/pybind11/pybind11.h
+++ b/include/pybind11/pybind11.h
@@ -1363,6 +1363,10 @@ protected:

     /// Helper function which tags all parents of a type using mult. inheritance
     void mark_parents_nonsimple(PyTypeObject *value) {
+        if (value->tp_bases == nullptr) {
+            // Since https://github.com/python/cpython/pull/103912 (Python 3.12.0b1)
+            return;
+        }
         auto t = reinterpret_borrow<tuple>(value->tp_bases);
         for (handle h : t) {
             auto *tinfo2 = get_type_info((PyTypeObject *) h.ptr());

@rwgk rwgk merged commit ce9bbc0 into pybind:master May 23, 2023
@rwgk rwgk deleted the py312_error_already_set branch May 23, 2023 17:03
@github-actions github-actions bot added the needs changelog Possibly needs a changelog entry label May 23, 2023
@rwgk rwgk removed the needs changelog Possibly needs a changelog entry label Jul 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
python dev Working on development versions of Python
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants