From ab078258d1909f2845b95b1af35793bec6574f43 Mon Sep 17 00:00:00 2001 From: David Hewitt <1939362+davidhewitt@users.noreply.github.com> Date: Mon, 10 Jul 2023 23:41:20 +0100 Subject: [PATCH] fix exception handling on Python 3.12 --- newsfragments/3306.changed.md | 1 + src/err/err_state.rs | 108 +++++++++++++++++++++++++++++++--- src/err/mod.rs | 95 ++++++++++++++++-------------- 3 files changed, 151 insertions(+), 53 deletions(-) create mode 100644 newsfragments/3306.changed.md diff --git a/newsfragments/3306.changed.md b/newsfragments/3306.changed.md new file mode 100644 index 00000000000..a2bbc985512 --- /dev/null +++ b/newsfragments/3306.changed.md @@ -0,0 +1 @@ +Update `PyErr` for 3.12 betas to avoid deprecated ffi methods. diff --git a/src/err/err_state.rs b/src/err/err_state.rs index abf1817cd57..a7408efdba2 100644 --- a/src/err/err_state.rs +++ b/src/err/err_state.rs @@ -7,11 +7,37 @@ use crate::{ #[derive(Clone)] pub(crate) struct PyErrStateNormalized { + #[cfg(not(Py_3_12))] pub ptype: Py, pub pvalue: Py, + #[cfg(not(Py_3_12))] pub ptraceback: Option>, } +impl PyErrStateNormalized { + #[cfg(not(Py_3_12))] + pub(crate) fn ptype<'py>(&'py self, py: Python<'py>) -> &'py PyType { + self.ptype.as_ref(py) + } + + #[cfg(Py_3_12)] + pub(crate) fn ptype<'py>(&'py self, py: Python<'py>) -> &'py PyType { + self.pvalue.as_ref(py).get_type() + } + + #[cfg(not(Py_3_12))] + pub(crate) fn ptraceback<'py>(&'py self, py: Python<'py>) -> Option<&'py PyTraceback> { + self.ptraceback + .as_ref() + .map(|traceback| traceback.as_ref(py)) + } + + #[cfg(Py_3_12)] + pub(crate) fn ptraceback<'py>(&'py self, py: Python<'py>) -> Option<&'py PyTraceback> { + unsafe { py.from_owned_ptr_or_opt(ffi::PyException_GetTraceback(self.pvalue.as_ptr())) } + } +} + pub(crate) struct PyErrStateLazyFnOutput { pub(crate) ptype: PyObject, pub(crate) pvalue: PyObject, @@ -22,6 +48,7 @@ pub(crate) type PyErrStateLazyFn = pub(crate) enum PyErrState { Lazy(Box), + #[cfg(not(Py_3_12))] FfiTuple { ptype: PyObject, pvalue: Option, @@ -53,9 +80,23 @@ impl PyErrState { pvalue: args.arguments(py), })) } -} -impl PyErrState { + pub(crate) fn normalized(pvalue: &PyBaseException) -> Self { + Self::Normalized(PyErrStateNormalized { + #[cfg(not(Py_3_12))] + ptype: pvalue.get_type().into(), + pvalue: pvalue.into(), + #[cfg(not(Py_3_12))] + ptraceback: unsafe { + Py::from_owned_ptr_or_opt( + pvalue.py(), + ffi::PyException_GetTraceback(pvalue.as_ptr()), + ) + }, + }) + } + + #[cfg(not(Py_3_12))] pub(crate) fn into_ffi_tuple( self, py: Python<'_>, @@ -64,7 +105,11 @@ impl PyErrState { PyErrState::Lazy(lazy) => { let PyErrStateLazyFnOutput { ptype, pvalue } = lazy(py); if unsafe { ffi::PyExceptionClass_Check(ptype.as_ptr()) } == 0 { - Self::exceptions_must_derive_from_base_exception(py).into_ffi_tuple(py) + PyErrState::lazy( + PyTypeError::type_object(py), + "exceptions must derive from BaseException", + ) + .into_ffi_tuple(py) } else { (ptype.into_ptr(), pvalue.into_ptr(), std::ptr::null_mut()) } @@ -82,10 +127,57 @@ impl PyErrState { } } - fn exceptions_must_derive_from_base_exception(py: Python<'_>) -> Self { - PyErrState::lazy( - PyTypeError::type_object(py), - "exceptions must derive from BaseException", - ) + #[cfg(not(Py_3_12))] + pub(crate) fn normalize(self, py: Python<'_>) -> PyErrStateNormalized { + let (mut ptype, mut pvalue, mut ptraceback) = self.into_ffi_tuple(py); + + unsafe { + ffi::PyErr_NormalizeException(&mut ptype, &mut pvalue, &mut ptraceback); + PyErrStateNormalized { + ptype: Py::from_owned_ptr_or_opt(py, ptype).expect("Exception type missing"), + pvalue: Py::from_owned_ptr_or_opt(py, pvalue).expect("Exception value missing"), + ptraceback: Py::from_owned_ptr_or_opt(py, ptraceback), + } + } + } + + #[cfg(Py_3_12)] + pub(crate) fn normalize(self, py: Python<'_>) -> PyErrStateNormalized { + // To keep the implementation simple, just write the exception into the interpreter, + // which will cause it to be normalized + self.restore(py); + // Safety: self.restore(py) will set the raised exception + let pvalue = unsafe { Py::from_owned_ptr(py, ffi::PyErr_GetRaisedException()) }; + PyErrStateNormalized { pvalue } + } + + #[cfg(not(Py_3_12))] + pub(crate) fn restore(self, py: Python<'_>) { + let (ptype, pvalue, ptraceback) = self.into_ffi_tuple(py); + unsafe { ffi::PyErr_Restore(ptype, pvalue, ptraceback) } + } + + #[cfg(Py_3_12)] + pub(crate) fn restore(self, py: Python<'_>) { + match self { + PyErrState::Lazy(lazy) => { + let PyErrStateLazyFnOutput { ptype, pvalue } = lazy(py); + unsafe { + if ffi::PyExceptionClass_Check(ptype.as_ptr()) == 0 { + ffi::PyErr_SetString( + PyTypeError::type_object_raw(py).cast(), + "exceptions must derive from BaseException\0" + .as_ptr() + .cast(), + ) + } else { + ffi::PyErr_SetObject(ptype.as_ptr(), pvalue.as_ptr()) + } + } + } + PyErrState::Normalized(PyErrStateNormalized { pvalue }) => unsafe { + ffi::PyErr_SetRaisedException(pvalue.into_ptr()) + }, + } } } diff --git a/src/err/mod.rs b/src/err/mod.rs index 1cfd403564e..f50b9568763 100644 --- a/src/err/mod.rs +++ b/src/err/mod.rs @@ -177,17 +177,7 @@ impl PyErr { /// ``` pub fn from_value(obj: &PyAny) -> PyErr { let state = if let Ok(obj) = obj.downcast::() { - let pvalue: Py = obj.into(); - - let ptraceback = unsafe { - Py::from_owned_ptr_or_opt(obj.py(), ffi::PyException_GetTraceback(pvalue.as_ptr())) - }; - - PyErrState::Normalized(PyErrStateNormalized { - ptype: obj.get_type().into(), - pvalue, - ptraceback, - }) + PyErrState::normalized(obj) } else { // Assume obj is Type[Exception]; let later normalization handle if this // is not the case @@ -209,7 +199,7 @@ impl PyErr { /// }); /// ``` pub fn get_type<'py>(&'py self, py: Python<'py>) -> &'py PyType { - self.normalized(py).ptype.as_ref(py) + self.normalized(py).ptype(py) } /// Returns the value of this exception. @@ -236,7 +226,7 @@ impl PyErr { // complexity. let normalized = self.normalized(py); let exc = normalized.pvalue.clone_ref(py); - if let Some(tb) = normalized.ptraceback.as_ref() { + if let Some(tb) = normalized.ptraceback(py) { unsafe { ffi::PyException_SetTraceback(exc.as_ptr(), tb.as_ptr()); } @@ -256,10 +246,7 @@ impl PyErr { /// }); /// ``` pub fn traceback<'py>(&'py self, py: Python<'py>) -> Option<&'py PyTraceback> { - self.normalized(py) - .ptraceback - .as_ref() - .map(|obj| obj.as_ref(py)) + self.normalized(py).ptraceback(py) } /// Gets whether an error is present in the Python interpreter's global state. @@ -278,6 +265,11 @@ impl PyErr { /// expected to have been set, for example from [`PyErr::occurred`] or by an error return value /// from a C FFI function, use [`PyErr::fetch`]. pub fn take(py: Python<'_>) -> Option { + Self::_take(py) + } + + #[cfg(not(Py_3_12))] + fn _take(py: Python<'_>) -> Option { let (ptype, pvalue, ptraceback) = unsafe { let mut ptype: *mut ffi::PyObject = std::ptr::null_mut(); let mut pvalue: *mut ffi::PyObject = std::ptr::null_mut(); @@ -316,17 +308,12 @@ impl PyErr { .map(|py_str| py_str.to_string_lossy().into()) .unwrap_or_else(|| String::from("Unwrapped panic from Python code")); - eprintln!( - "--- PyO3 is resuming a panic after fetching a PanicException from Python. ---" - ); - eprintln!("Python stack trace below:"); - - unsafe { - ffi::PyErr_Restore(ptype.into_ptr(), pvalue.into_ptr(), ptraceback.into_ptr()); - ffi::PyErr_PrintEx(0); - } - - std::panic::resume_unwind(Box::new(msg)) + let state = PyErrState::FfiTuple { + ptype, + pvalue, + ptraceback, + }; + Self::print_panic_and_unwind(py, state, msg) } Some(PyErr::from_state(PyErrState::FfiTuple { @@ -336,6 +323,35 @@ impl PyErr { })) } + #[cfg(Py_3_12)] + fn _take(py: Python<'_>) -> Option { + let pvalue = unsafe { + py.from_owned_ptr_or_opt::(ffi::PyErr_GetRaisedException()) + }?; + if pvalue.get_type().as_ptr() == PanicException::type_object_raw(py).cast() { + let msg: String = pvalue + .str() + .map(|py_str| py_str.to_string_lossy().into()) + .unwrap_or_else(|_| String::from("Unwrapped panic from Python code")); + Self::print_panic_and_unwind(py, PyErrState::normalized(pvalue), msg) + } + + Some(PyErr::from_state(PyErrState::normalized(pvalue))) + } + + fn print_panic_and_unwind(py: Python<'_>, state: PyErrState, msg: String) -> ! { + eprintln!("--- PyO3 is resuming a panic after fetching a PanicException from Python. ---"); + eprintln!("Python stack trace below:"); + + state.restore(py); + + unsafe { + ffi::PyErr_PrintEx(0); + } + + std::panic::resume_unwind(Box::new(msg)) + } + /// Equivalent to [PyErr::take], but when no error is set: /// - Panics in debug mode. /// - Returns a `SystemError` in release mode. @@ -457,15 +473,10 @@ impl PyErr { /// This is the opposite of `PyErr::fetch()`. #[inline] pub fn restore(self, py: Python<'_>) { - let state = match self.state.into_inner() { - Some(state) => state, - // Safety: restore takes `self` by value so nothing else is accessing this err - // and the invariant is that state is always defined except during make_normalized - None => unsafe { std::hint::unreachable_unchecked() }, - }; - - let (ptype, pvalue, ptraceback) = state.into_ffi_tuple(py); - unsafe { ffi::PyErr_Restore(ptype, pvalue, ptraceback) } + self.state + .into_inner() + .expect("PyErr state should never be invalid outside of normalization") + .restore(py) } /// Reports the error as unraisable. @@ -649,17 +660,10 @@ impl PyErr { .take() .expect("Cannot normalize a PyErr while already normalizing it.") }; - let (mut ptype, mut pvalue, mut ptraceback) = state.into_ffi_tuple(py); unsafe { - ffi::PyErr_NormalizeException(&mut ptype, &mut pvalue, &mut ptraceback); let self_state = &mut *self.state.get(); - *self_state = Some(PyErrState::Normalized(PyErrStateNormalized { - ptype: Py::from_owned_ptr_or_opt(py, ptype).expect("Exception type missing"), - pvalue: Py::from_owned_ptr_or_opt(py, pvalue).expect("Exception value missing"), - ptraceback: Py::from_owned_ptr_or_opt(py, ptraceback), - })); - + *self_state = Some(PyErrState::Normalized(state.normalize(py))); match self_state { Some(PyErrState::Normalized(n)) => n, _ => unreachable!(), @@ -826,6 +830,7 @@ mod tests { assert!(err.is_instance_of::(py)); err.restore(py); let err = PyErr::fetch(py); + assert!(err.is_instance_of::(py)); assert_eq!( err.to_string(),