use `PyErr_SetRaisedException` in `raise_lazy` by bschoenmaeckers · Pull Request #5876 · PyO3/pyo3

bschoenmaeckers · 2026-03-13T10:49:59Z

This fixes a TODO in raise_lazy

src/err/err_state.rs

davidhewitt

Thanks for working on this! I've wanted to move forward with this for some time, it will allow us to (hopefully) improve the exception story piece by piece.

I think last time I looked at this I thought it was hard. See #4669 where I once pushed something similar, I forget why exactly I never got back to finishing it off...

src/err/err_state.rs

bschoenmaeckers · 2026-03-13T15:33:08Z

Thanks for working on this! I've wanted to move forward with this for some time, it will allow us to (hopefully) improve the exception story piece by piece.

I think last time I looked at this I thought it was hard. See #4669 where I once pushed something similar, I forget why exactly I never got back to finishing it off...

Reading through #4669 makes me remember we have safe api's to do things like this. 😅

davidhewitt

I think as long as we're confident that the subtle difference with exception chaining will not cause us problems, I'm happy to proceed with this.

davidhewitt · 2026-03-13T16:09:15Z

src/err/err_state.rs

+            #[cfg(not(Py_3_12))]
+            ffi::PyErr_SetObject(ptype.as_ptr(), pvalue.as_ptr());


To be more sure that the difference in exception chaining on the legacy path can't lead to diverging behavior, we could change the legacy path to call PyErr_Restore after creating a fully normalized exception object?

I think it is valuable to compare the PyErr_SetObject behaviour in our test suite so can verify our code path with PyErr_SetRaisedException is correct.

src/err/err_state.rs

bschoenmaeckers · 2026-03-16T12:48:36Z

Tests are passing, so could you please take another look.

davidhewitt

Thanks, good to test all cases here and be sure we're not accidentally regressing something. Looks like this is shaping up well, just a few smaller thoughts...

src/err/err_state.rs

newsfragments/5876.added.md

davidhewitt · 2026-03-17T10:43:45Z

src/err/err_state.rs

+            let context = err.context(py).unwrap();
+            assert!(context.is_instance_of::<PyRuntimeError>(py))
+        })
+    }


I wonder if we should have a test which directly compares the result against against ffi::PyErr_SetObject so that we can keep up to date with any future changes too?

I've added a test that compare's all the different variants I'm aware of.

Add context test fix context test & use safe api's Changelog Remove `compat::PyObject_CallOneArg` fix clippy Add test for 'PyErr::set_context' Return `Bound<'py, PyBaseException>` from `create_normalized_exception`

davidhewitt

Thanks, I would like to see a couple more tests around the context case when comparing to PyErr_SetObject, and then hopefully we're good to merge 👍

davidhewitt · 2026-03-17T14:59:11Z

src/err/err_state.rs

+            let err2 = PyErr::from_value(exc2.into_any());
+
+            assert!(err1.get_type(py).is(err2.get_type(py)));
+            assert!(err1.context(py).xor(err2.context(py)).is_none());


I guess there is another test where if we call PyErr_SetHandledException first, we expect that value to be placed in context? Looking at PyErr_SetObject implementation, there is some logic to avoid creating cycles in the context too, I feel like we should probably be careful to behave similarly?

I wonder, thinking about the context cycles, does that imply this approach is getting too complex? Maybe there is an alternative where we take control of the exception object creation as this PR already does (as that'll be helpful for improving PyErr::new as well as eventually removing the "lazy" state as per #4859), but we continue to use PyErr_SetObject to populate the interpreter state? Not as efficient as PyErr_SetRaisedException but would guarantee that we always use the standard behavior.

Maybe we should just be calling the new create_normalized_exception directly from PyErrStateInner::normalize, as a first step towards #4859? (And avoid touching the interpreter's exception state at all.)

I think that would be slightly breaking in that creating a PyErr would no longer automatically pick up any __context__ (you'd have to actually call PyErr::restore to do that). But maybe that's actually more correct?

(Sorry to keep having slightly new / different ideas here.)

but we continue to use PyErr_SetObject to populate the interpreter state? Not as efficient as PyErr_SetRaisedException but would guarantee that we always use the standard behavior.

Could be an option, I will play around with cycle detection first to see if it's that horrible to implement. Maybe I will find a better solution on the way.

bschoenmaeckers added the CI-skip-changelog Skip checking changelog entry label Mar 13, 2026

LilyFirefly reviewed Mar 13, 2026

View reviewed changes

src/err/err_state.rs Outdated Show resolved Hide resolved

bschoenmaeckers force-pushed the update_raise_lazy branch 3 times, most recently from c155ece to b727b3d Compare March 13, 2026 13:42

davidhewitt reviewed Mar 13, 2026

View reviewed changes

src/err/err_state.rs Show resolved Hide resolved

davidhewitt approved these changes Mar 13, 2026

View reviewed changes

bschoenmaeckers force-pushed the update_raise_lazy branch 4 times, most recently from 5c6319e to 0648d71 Compare March 16, 2026 11:01

bschoenmaeckers removed the CI-skip-changelog Skip checking changelog entry label Mar 16, 2026

davidhewitt reviewed Mar 17, 2026

View reviewed changes

bschoenmaeckers mentioned this pull request Mar 17, 2026

Add PyErr::set_context, PyErr::context #5887

Merged

bschoenmaeckers force-pushed the update_raise_lazy branch from 77314c1 to 6f55717 Compare March 17, 2026 13:03

use PyErr_SetRaisedException in raise_lazy

6ef5328

Add context test fix context test & use safe api's Changelog Remove `compat::PyObject_CallOneArg` fix clippy Add test for 'PyErr::set_context' Return `Bound<'py, PyBaseException>` from `create_normalized_exception`

bschoenmaeckers force-pushed the update_raise_lazy branch from 6f55717 to 6ef5328 Compare March 17, 2026 13:04

bschoenmaeckers added the CI-skip-changelog Skip checking changelog entry label Mar 17, 2026

bschoenmaeckers added 2 commits March 17, 2026 14:58

Only PyException_SetContext when PyErr_GetHandledException != null

9e92b21

Compare create_normalized_exception with PyErr_SetObject

8194fe0

bschoenmaeckers force-pushed the update_raise_lazy branch from 8c9ec99 to 8194fe0 Compare March 17, 2026 13:58

davidhewitt reviewed Mar 17, 2026

View reviewed changes

		#[cfg(not(Py_3_12))]
		ffi::PyErr_SetObject(ptype.as_ptr(), pvalue.as_ptr());

Conversation

bschoenmaeckers commented Mar 13, 2026

Uh oh!

Uh oh!

davidhewitt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bschoenmaeckers commented Mar 13, 2026

Uh oh!

davidhewitt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bschoenmaeckers commented Mar 16, 2026

Uh oh!

davidhewitt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidhewitt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants