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

also use PyErr::SetObject on Python versions before 3.12 #3455

Merged
merged 1 commit into from
Oct 10, 2023

Conversation

davidhewitt
Copy link
Member

@davidhewitt davidhewitt commented Sep 15, 2023

Fixes #3432

This PR changes versions before 3.12 to also use PyErr::SetObject when raising (or normalizing) a lazy PyErr. This helps to unify the code paths between 3.12 and older versions as well as fixes the bugs discussed in those recent issues.

@mejrs
Copy link
Member

mejrs commented Sep 18, 2023

Can you explain how this fixes those issues? I'm reading the code and the C api docs but I don't quite see it.

@davidhewitt
Copy link
Member Author

davidhewitt commented Sep 19, 2023

I can't speak in detail for the PyPy fix, which is a little bit arcane. My intuition there was that before this PR we were not "normalizing" exceptions from our Lazy state before calling PyErr_Restore. Somehow in the StopIteration case with PyPy having the exception be unnormalized was not behaving the same as CPython. Potentially we could argue it's a PyPy bug which we should report upstream, but if switching to PyErr_SetObject fixed it, hey, that's a nice improvement given I was exploring that anyway.

The motivation for calling PyErr_SetObject is that this has the logic for chaining exceptions baked into it, and on 3.12 it also normalizes the exception eagerly (since raised exceptions are always stored in the normalized state).

Given we're forced to use it on 3.12 to make the machinery work, because PyErr_NormalizeException is deprecated, it seems like a simplification to the code to also use it on pre-3.12 given it adds the exception chaining functionality we want.

Comment on lines 187 to 212
/// In principle this could be split in two; first a function to create an exception
/// in a normalized state, and then a call to `PyErr_SetRaisedException` to raise it.
Copy link
Member Author

Choose a reason for hiding this comment

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

See also this comment; I think in principle we could implement an equivalent of PyErr_SetObject in Rust, which would have the upside of allowing us to then refactor and adjust behaviour later if we so desire. I wanted to start by just calling PyErr_SetObject as it avoids moving a lot of complexity to Rust.

@davidhewitt
Copy link
Member Author

davidhewitt commented Sep 23, 2023

Please review #3471 first.

@davidhewitt davidhewitt changed the title fix edge cases with exception handling also use PyErr::SetObject on Python versions before 3.12 Sep 30, 2023
@davidhewitt davidhewitt marked this pull request as ready for review September 30, 2023 22:35
@davidhewitt
Copy link
Member Author

Rebased; this PR is now a very minimal diff which hopefully highlights the way that this adjustment makes the pre-3.12 code paths match 3.12, while also fixing a bug (and adding a test).

@davidhewitt davidhewitt mentioned this pull request Oct 3, 2023
@davidhewitt
Copy link
Member Author

As this makes a behavioural change which I believe we want to bring as part of the 0.20 release, I'm going to merge this soon (probably tomorrow) so I can proceed with preparing the release. Please forgive me if this turns out to be unwanted! 🙏

@davidhewitt davidhewitt added this pull request to the merge queue Oct 10, 2023
Merged via the queue into PyO3:main with commit c0b5004 Oct 10, 2023
34 checks passed
@davidhewitt davidhewitt deleted the normalized-exceptions branch October 10, 2023 08:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"TypeError: No constructor defined" discards exception context
2 participants