-
Notifications
You must be signed in to change notification settings - Fork 949
use PyErr_SetRaisedException in raise_lazy
#5876
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
base: main
Are you sure you want to change the base?
Changes from 3 commits
6ef5328
9e92b21
8194fe0
4feb153
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 |
|---|---|---|
|
|
@@ -13,6 +13,11 @@ use crate::{ | |
| types::{PyAnyMethods, PyTraceback, PyType}, | ||
| Bound, Py, PyAny, PyErrArguments, PyTypeInfo, Python, | ||
| }; | ||
| #[cfg(Py_3_12)] | ||
| use { | ||
| crate::types::{PyString, PyTuple}, | ||
| std::ptr::NonNull, | ||
| }; | ||
|
|
||
| pub(crate) struct PyErrState { | ||
| // Safety: can only hand out references when in the "normalized" state. Will never change | ||
|
|
@@ -383,29 +388,76 @@ fn lazy_into_normalized_ffi_tuple( | |
| } | ||
|
|
||
| /// Raises a "lazy" exception state into the Python interpreter. | ||
| /// | ||
| /// 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. | ||
| /// | ||
| /// This would require either moving some logic from C to Rust, or requesting a new | ||
| /// API in CPython. | ||
| fn raise_lazy(py: Python<'_>, lazy: Box<PyErrStateLazyFn>) { | ||
| let PyErrStateLazyFnOutput { ptype, pvalue } = lazy(py); | ||
|
|
||
| unsafe { | ||
| #[cfg(not(Py_3_12))] | ||
| if ffi::PyExceptionClass_Check(ptype.as_ptr()) == 0 { | ||
| ffi::PyErr_SetString( | ||
| PyTypeError::type_object_raw(py).cast(), | ||
| c"exceptions must derive from BaseException".as_ptr(), | ||
| ) | ||
| ); | ||
| } else { | ||
| ffi::PyErr_SetObject(ptype.as_ptr(), pvalue.as_ptr()) | ||
| ffi::PyErr_SetObject(ptype.as_ptr(), pvalue.as_ptr()); | ||
| } | ||
|
|
||
| #[cfg(Py_3_12)] | ||
| { | ||
| let exc = create_normalized_exception(ptype.bind(py), pvalue.into_bound(py)); | ||
|
|
||
| ffi::PyErr_SetRaisedException(exc.into_ptr()); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| #[cfg(Py_3_12)] | ||
| fn create_normalized_exception<'py>( | ||
| ptype: &Bound<'py, PyAny>, | ||
| mut pvalue: Bound<'py, PyAny>, | ||
| ) -> Bound<'py, PyBaseException> { | ||
| let py = ptype.py(); | ||
|
|
||
| // 1: check type is a subclass of BaseException | ||
| let ptype: Bound<'py, PyType> = if unsafe { ffi::PyExceptionClass_Check(ptype.as_ptr()) } == 0 { | ||
| pvalue = PyString::new(py, "exceptions must derive from BaseException").into_any(); | ||
| PyTypeError::type_object(py) | ||
| } else { | ||
| // Safety: PyExceptionClass_Check guarantees that ptype is a subclass of BaseException | ||
| unsafe { ptype.cast_unchecked() }.clone() | ||
| }; | ||
|
|
||
| let pvalue = if pvalue.is_exact_instance(&ptype) { | ||
| // Safety: already an exception value of the correct type | ||
| Ok(unsafe { pvalue.cast_into_unchecked::<PyBaseException>() }) | ||
| } else if pvalue.is_none() { | ||
| // None -> no arguments | ||
| ptype.call0().and_then(|pvalue| Ok(pvalue.cast_into()?)) | ||
| } else if let Ok(tup) = pvalue.cast::<PyTuple>() { | ||
| // Tuple -> use as tuple of arguments | ||
| ptype.call1(tup).and_then(|pvalue| Ok(pvalue.cast_into()?)) | ||
| } else { | ||
| // Anything else -> use as single argument | ||
| ptype | ||
| .call1((pvalue,)) | ||
| .and_then(|pvalue| Ok(pvalue.cast_into()?)) | ||
| }; | ||
|
|
||
| match pvalue { | ||
| Ok(pvalue) => { | ||
| unsafe { | ||
| if let Some(context) = NonNull::new(ffi::PyErr_GetHandledException()) { | ||
| ffi::PyException_SetContext(pvalue.as_ptr(), context.as_ptr()) | ||
| } | ||
| }; | ||
| pvalue | ||
| } | ||
| Err(e) => e.value(py).clone(), | ||
| } | ||
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod tests { | ||
|
|
||
| use crate::{ | ||
| exceptions::PyValueError, sync::PyOnceLock, Py, PyAny, PyErr, PyErrArguments, Python, | ||
| }; | ||
|
|
@@ -478,4 +530,90 @@ mod tests { | |
| .is_instance_of::<PyValueError>(py)) | ||
| }); | ||
| } | ||
|
|
||
| #[test] | ||
| #[cfg(feature = "macros")] | ||
| fn test_new_exception_context() { | ||
| use crate::{ | ||
| exceptions::{PyRuntimeError, PyValueError}, | ||
| pyfunction, | ||
| types::{PyDict, PyDictMethods}, | ||
| wrap_pyfunction, PyResult, | ||
| }; | ||
| #[pyfunction(crate = "crate")] | ||
| fn throw_exception() -> PyResult<()> { | ||
| Err(PyValueError::new_err("error happened")) | ||
| } | ||
|
|
||
| Python::attach(|py| { | ||
| let globals = PyDict::new(py); | ||
| let f = wrap_pyfunction!(throw_exception, py).unwrap(); | ||
| globals.set_item("throw_exception", f).unwrap(); | ||
| let err = py | ||
| .run( | ||
| c"try:\n raise RuntimeError()\nexcept:\n throw_exception()\n", | ||
| Some(&globals), | ||
| None, | ||
| ) | ||
| .unwrap_err(); | ||
|
|
||
| let context = err.context(py).unwrap(); | ||
| assert!(context.is_instance_of::<PyRuntimeError>(py)) | ||
| }) | ||
| } | ||
|
Member
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. I wonder if we should have a test which directly compares the result against against
Member
Author
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. I've added a test that compare's all the different variants I'm aware of. |
||
|
|
||
| #[test] | ||
| #[cfg(Py_3_12)] | ||
| fn compare_create_normalized_exception_with_pyerr_setobject() { | ||
| use crate::{ | ||
| conversion::IntoPyObjectExt, err::err_state::PyErrStateNormalized, | ||
| exceptions::PyRuntimeError, ffi, type_object::PyTypeInfo, types::any::PyAnyMethods, | ||
| Bound, | ||
| }; | ||
|
|
||
| fn test_exception<'py>(ptype: &Bound<'py, PyAny>, pvalue: Bound<'py, PyAny>) { | ||
| let py = ptype.py(); | ||
|
|
||
| let exc1 = super::create_normalized_exception(ptype, pvalue.clone()); | ||
|
|
||
| unsafe { | ||
| ffi::PyErr_SetObject(ptype.as_ptr(), pvalue.as_ptr()); | ||
| } | ||
| let exc2 = PyErrStateNormalized::take(py) | ||
| .unwrap() | ||
| .pvalue | ||
| .into_bound(py); | ||
|
|
||
| let err1 = PyErr::from_value(exc1.into_any()); | ||
| 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()); | ||
|
Member
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. I guess there is another test where if we call
Member
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. 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
Member
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. Maybe we should just be calling the new I think that would be slightly breaking in that creating a
Member
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. (Sorry to keep having slightly new / different ideas here.)
Member
Author
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.
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.
Member
Author
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. It got indeed somewhat complex. But I think the test suite is extended sufficiently.
Member
Author
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. Using |
||
| assert!(err1.traceback(py).xor(err2.traceback(py)).is_none()); | ||
| assert!(err1.cause(py).xor(err2.cause(py)).is_none()); | ||
| assert_eq!(err1.to_string(), err2.to_string()); | ||
| } | ||
|
|
||
| Python::attach(|py| { | ||
| test_exception(&PyRuntimeError::type_object(py), py.None().into_bound(py)); | ||
|
|
||
| test_exception( | ||
| &PyRuntimeError::type_object(py), | ||
| "Boom".into_bound_py_any(py).unwrap(), | ||
| ); | ||
|
|
||
| test_exception( | ||
| &PyRuntimeError::type_object(py), | ||
| (3, 2, 1, "Boom").into_bound_py_any(py).unwrap(), | ||
| ); | ||
|
|
||
| test_exception( | ||
| &PyRuntimeError::type_object(py), | ||
| PyRuntimeError::new_err("Boom") | ||
| .into_value(py) | ||
| .into_any() | ||
| .into_bound(py), | ||
| ); | ||
| }) | ||
| } | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.