Merge pull request #1957 from davidhewitt/fetch-if-set

err: add `PyErr::take`

CHANGELOG.md

@@ -28,10 +28,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - Add `abi3-py310` feature. [#1889](https://github.com/PyO3/pyo3/pull/1889)
 - Add `PyCFunction::new_closure` to create a Python function from a Rust closure. [#1901](https://github.com/PyO3/pyo3/pull/1901)
 - Add support for positional-only arguments in `#[pyfunction]` [#1925](https://github.com/PyO3/pyo3/pull/1925)
+- Add `PyErr::take` to attempt to fetch a Python exception if present. [#1957](https://github.com/PyO3/pyo3/pull/1957)
 
 ### Changed
 
-- Change `PyErr::fetch` to return `Option<PyErr>`. [#1717](https://github.com/PyO3/pyo3/pull/1717)
 - `PyList`, `PyTuple` and `PySequence`'s APIs now accepts only `usize` indices instead of `isize`.
   [#1733](https://github.com/PyO3/pyo3/pull/1733), [#1802](https://github.com/PyO3/pyo3/pull/1802),
   [#1803](https://github.com/PyO3/pyo3/pull/1803)
@@ -48,6 +48,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - Remove function PyTuple_ClearFreeList from python 3.9 above. [#1887](https://github.com/PyO3/pyo3/pull/1887)
 - Deprecate `#[call]` attribute in favor of using `fn __call__`. [#1929](https://github.com/PyO3/pyo3/pull/1929)
 - Fix missing FFI definition `_PyImport_FindExtensionObject` on Python 3.10. [#1942](https://github.com/PyO3/pyo3/pull/1942)
+- Change `PyErr::fetch` to panic in debug mode if no exception is present. [#1957](https://github.com/PyO3/pyo3/pull/1957)
 
 ### Fixed
 

src/conversion.rs

@@ -494,7 +494,7 @@ pub unsafe trait FromPyPointer<'p>: Sized {
     ///
     /// Relies on [`from_owned_ptr_or_opt`](#method.from_owned_ptr_or_opt).
     unsafe fn from_owned_ptr_or_err(py: Python<'p>, ptr: *mut ffi::PyObject) -> PyResult<&'p Self> {
-        Self::from_owned_ptr_or_opt(py, ptr).ok_or_else(|| err::PyErr::api_call_failed(py))
+        Self::from_owned_ptr_or_opt(py, ptr).ok_or_else(|| err::PyErr::fetch(py))
     }
     /// Convert from an arbitrary borrowed `PyObject`.
     ///
@@ -528,7 +528,7 @@ pub unsafe trait FromPyPointer<'p>: Sized {
         py: Python<'p>,
         ptr: *mut ffi::PyObject,
     ) -> PyResult<&'p Self> {
-        Self::from_borrowed_ptr_or_opt(py, ptr).ok_or_else(|| err::PyErr::api_call_failed(py))
+        Self::from_borrowed_ptr_or_opt(py, ptr).ok_or_else(|| err::PyErr::fetch(py))
     }
 }
 

src/conversions/num_bigint.rs

@@ -108,19 +108,16 @@ macro_rules! bigint_conversion {
             fn extract(ob: &'source PyAny) -> PyResult<$rust_ty> {
                 let py = ob.py();
                 unsafe {
-                    let num = ffi::PyNumber_Index(ob.as_ptr());
-                    if num.is_null() {
-                        return Err(PyErr::api_call_failed(py));
-                    }
-                    let n_bits = ffi::_PyLong_NumBits(num);
-                    let n_bytes = if n_bits < 0 {
-                        return Err(PyErr::api_call_failed(py));
+                    let num: Py<PyLong> =
+                        Py::from_owned_ptr_or_err(py, ffi::PyNumber_Index(ob.as_ptr()))?;
+                    let n_bits = ffi::_PyLong_NumBits(num.as_ptr());
+                    let n_bytes = if n_bits == -1 {
+                        return Err(PyErr::fetch(py));
                     } else if n_bits == 0 {
                         0
                     } else {
                         (n_bits as usize - 1 + $is_signed) / 8 + 1
                     };
-                    let num: Py<PyLong> = Py::from_owned_ptr(py, num);
                     if n_bytes <= 128 {
                         let mut buffer = [0; 128];
                         extract(num.as_ref(py), &mut buffer[..n_bytes], $is_signed)?;

src/conversions/num_complex.rs

@@ -143,19 +143,22 @@ macro_rules! complex_conversion {
                 #[cfg(not(any(Py_LIMITED_API, PyPy)))]
                 unsafe {
                     let val = ffi::PyComplex_AsCComplex(obj.as_ptr());
-                    if val.real == -1.0 && PyErr::occurred(obj.py()) {
-                        Err(PyErr::api_call_failed(obj.py()))
-                    } else {
-                        Ok(Complex::new(val.real as $float, val.imag as $float))
+                    if val.real == -1.0 {
+                        if let Some(err) = PyErr::take(obj.py()) {
+                            return Err(err);
+                        }
                     }
+                    Ok(Complex::new(val.real as $float, val.imag as $float))
                 }
 
                 #[cfg(any(Py_LIMITED_API, PyPy))]
                 unsafe {
                     let ptr = obj.as_ptr();
                     let real = ffi::PyComplex_RealAsDouble(ptr);
-                    if real == -1.0 && PyErr::occurred(obj.py()) {
-                        return Err(PyErr::api_call_failed(obj.py()));
+                    if real == -1.0 {
+                        if let Some(err) = PyErr::take(obj.py()) {
+                            return Err(err);
+                        }
                     }
                     let imag = ffi::PyComplex_ImagAsDouble(ptr);
                     Ok(Complex::new(real as $float, imag as $float))

src/conversions/osstr.rs

@@ -94,7 +94,7 @@ impl FromPyObject<'_> for OsString {
             let size =
                 unsafe { ffi::PyUnicode_AsWideChar(pystring.as_ptr(), std::ptr::null_mut(), 0) };
             if size == -1 {
-                return Err(PyErr::api_call_failed(ob.py()));
+                return Err(PyErr::fetch(ob.py()));
             }
 
             let mut buffer = vec![0; size as usize];

src/err/err_state.rs

@@ -23,7 +23,7 @@ pub(crate) enum PyErrState {
         pvalue: Box<dyn FnOnce(Python) -> PyObject + Send + Sync>,
     },
     FfiTuple {
-        ptype: Option<PyObject>,
+        ptype: PyObject,
         pvalue: Option<PyObject>,
         ptraceback: Option<PyObject>,
     },

src/err/mod.rs

@@ -7,9 +7,7 @@ use crate::{
     exceptions::{self, PyBaseException},
     ffi,
 };
-use crate::{
-    AsPyPointer, FromPyPointer, IntoPy, Py, PyAny, PyObject, Python, ToBorrowedObject, ToPyObject,
-};
+use crate::{AsPyPointer, IntoPy, Py, PyAny, PyObject, Python, ToBorrowedObject, ToPyObject};
 use std::borrow::Cow;
 use std::cell::UnsafeCell;
 use std::ffi::CString;
@@ -137,15 +135,13 @@ impl PyErr {
 
         let state = if unsafe { ffi::PyExceptionInstance_Check(ptr) } != 0 {
             PyErrState::Normalized(PyErrStateNormalized {
-                ptype: unsafe {
-                    Py::from_borrowed_ptr(obj.py(), ffi::PyExceptionInstance_Class(ptr))
-                },
+                ptype: obj.get_type().into(),
                 pvalue: unsafe { Py::from_borrowed_ptr(obj.py(), obj.as_ptr()) },
                 ptraceback: None,
             })
         } else if unsafe { ffi::PyExceptionClass_Check(obj.as_ptr()) } != 0 {
             PyErrState::FfiTuple {
-                ptype: unsafe { Some(Py::from_borrowed_ptr(obj.py(), ptr)) },
+                ptype: obj.into(),
                 pvalue: None,
                 ptraceback: None,
             }
@@ -218,61 +214,95 @@ impl PyErr {
         unsafe { !ffi::PyErr_Occurred().is_null() }
     }
 
-    /// Retrieves the current error from the Python interpreter's global state.
+    /// Takes the current error from the Python interpreter's global state and clears the global
+    /// state. If no error is set, returns `None`.
     ///
-    /// The error is cleared from the Python interpreter.
-    /// If no error is set, returns a `None`.
+    /// If the error is a `PanicException` (which would have originated from a panic in a pyo3
+    /// callback) then this function will resume the panic.
     ///
-    /// If the error fetched is a `PanicException` (which would have originated from a panic in a
-    /// pyo3 callback) then this function will resume the panic.
-    pub fn fetch(py: Python) -> Option<PyErr> {
-        unsafe {
+    /// Use this function when it is not known if an error should be present. If the error is
+    /// 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<PyErr> {
+        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();
             let mut ptraceback: *mut ffi::PyObject = std::ptr::null_mut();
             ffi::PyErr_Fetch(&mut ptype, &mut pvalue, &mut ptraceback);
 
-            // If the error indicator is not set, all three variables are set to NULL
-            if ptype.is_null() && pvalue.is_null() && ptraceback.is_null() {
-                return None;
-            }
-
-            let err = PyErr::new_from_ffi_tuple(py, ptype, pvalue, ptraceback);
+            // Convert to Py immediately so that any references are freed by early return.
+            let ptype = Py::from_owned_ptr_or_opt(py, ptype);
+            let pvalue = Py::from_owned_ptr_or_opt(py, pvalue);
+            let ptraceback = Py::from_owned_ptr_or_opt(py, ptraceback);
+
+            // A valid exception state should always have a non-null ptype, but the other two may be
+            // null.
+            let ptype = match ptype {
+                Some(ptype) => ptype,
+                None => {
+                    debug_assert!(
+                        pvalue.is_none(),
+                        "Exception type was null but value was not null"
+                    );
+                    debug_assert!(
+                        ptraceback.is_none(),
+                        "Exception type was null but traceback was not null"
+                    );
+                    return None;
+                }
+            };
+
+            (ptype, pvalue, ptraceback)
+        };
 
-            if ptype == PanicException::type_object(py).as_ptr() {
-                let msg: String = PyAny::from_borrowed_ptr_or_opt(py, pvalue)
-                    .and_then(|obj| obj.extract().ok())
-                    .unwrap_or_else(|| String::from("Unwrapped panic from Python code"));
+        if ptype.as_ptr() == PanicException::type_object(py).as_ptr() {
+            let msg: String = pvalue
+                .as_ref()
+                .and_then(|obj| obj.extract(py).ok())
+                .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:");
-                err.print(py);
+            eprintln!(
+                "--- PyO3 is resuming a panic after fetching a PanicException from Python. ---"
+            );
+            eprintln!("Python stack trace below:");
 
-                std::panic::resume_unwind(Box::new(msg))
+            unsafe {
+                use crate::conversion::IntoPyPointer;
+                ffi::PyErr_Restore(ptype.into_ptr(), pvalue.into_ptr(), ptraceback.into_ptr());
+                ffi::PyErr_PrintEx(0);
             }
 
-            Some(err)
+            std::panic::resume_unwind(Box::new(msg))
         }
+
+        Some(PyErr::from_state(PyErrState::FfiTuple {
+            ptype,
+            pvalue,
+            ptraceback,
+        }))
     }
 
-    /// Retrieves the current error from the Python interpreter's global state.
+    /// Equivalent to [PyErr::take], but when no error is set:
+    ///  - Panics in debug mode.
+    ///  - Returns a `SystemError` in release mode.
     ///
-    /// The error is cleared from the Python interpreter.
-    /// If no error is set, returns a `SystemError` in release mode,
-    /// panics in debug mode.
-    pub(crate) fn api_call_failed(py: Python) -> PyErr {
-        #[cfg(debug_assertions)]
-        {
-            PyErr::fetch(py).expect("error return without exception set")
-        }
-        #[cfg(not(debug_assertions))]
-        {
-            use crate::exceptions::PySystemError;
-
-            PyErr::fetch(py)
-                .unwrap_or_else(|| PySystemError::new_err("error return without exception set"))
+    /// This behavior is consistent with Python's internal handling of what happens when a C return
+    /// value indicates an error occurred but the global error state is empty. (A lack of exception
+    /// should be treated as a bug in the code which returned an error code but did not set an
+    /// exception.)
+    ///
+    /// Use this function when the error is expected to have been set, for example from
+    /// [PyErr::occurred] or by an error return value from a C FFI function.
+    #[cfg_attr(all(debug_assertions, track_caller), track_caller)]
+    #[inline]
+    pub fn fetch(py: Python) -> PyErr {
+        const FAILED_TO_FETCH: &str = "attempted to fetch exception but none was set";
+        match PyErr::take(py) {
+            Some(err) => err,
+            #[cfg(debug_assertions)]
+            None => panic!("{}", FAILED_TO_FETCH),
+            #[cfg(not(debug_assertions))]
+            None => exceptions::PySystemError::new_err(FAILED_TO_FETCH),
         }
     }
 
@@ -309,27 +339,6 @@ impl PyErr {
         }
     }
 
-    /// Create a PyErr from an ffi tuple
-    ///
-    /// # Safety
-    /// - `ptype` must be a pointer to valid Python exception type object.
-    /// - `pvalue` must be a pointer to a valid Python object, or NULL.
-    /// - `ptraceback` must be a pointer to a valid Python traceback object, or NULL.
-    unsafe fn new_from_ffi_tuple(
-        py: Python,
-        ptype: *mut ffi::PyObject,
-        pvalue: *mut ffi::PyObject,
-        ptraceback: *mut ffi::PyObject,
-    ) -> PyErr {
-        // Note: must not panic to ensure all owned pointers get acquired correctly,
-        // and because we mustn't panic in normalize().
-        PyErr::from_state(PyErrState::FfiTuple {
-            ptype: Py::from_owned_ptr_or_opt(py, ptype),
-            pvalue: Py::from_owned_ptr_or_opt(py, pvalue),
-            ptraceback: Py::from_owned_ptr_or_opt(py, ptraceback),
-        })
-    }
-
     /// Prints a standard traceback to `sys.stderr`.
     pub fn print(&self, py: Python) {
         self.clone_ref(py).restore(py);
@@ -579,7 +588,7 @@ pub fn error_on_minusone(py: Python, result: c_int) -> PyResult<()> {
     if result != -1 {
         Ok(())
     } else {
-        Err(PyErr::api_call_failed(py))
+        Err(PyErr::fetch(py))
     }
 }
 
@@ -596,9 +605,7 @@ mod tests {
 
     #[test]
     fn no_error() {
-        Python::with_gil(|py| {
-            assert!(PyErr::fetch(py).is_none());
-        });
+        assert!(Python::with_gil(PyErr::take).is_none());
     }
 
     #[test]
@@ -608,7 +615,7 @@ mod tests {
             assert!(err.is_instance::<exceptions::PyValueError>(py));
             err.restore(py);
             assert!(PyErr::occurred(py));
-            let err = PyErr::fetch(py).unwrap();
+            let err = PyErr::fetch(py);
             assert!(err.is_instance::<exceptions::PyValueError>(py));
             assert_eq!(err.to_string(), "ValueError: some exception message");
         })
@@ -620,7 +627,7 @@ mod tests {
             let err: PyErr = PyErr::new::<crate::types::PyString, _>(());
             assert!(err.is_instance::<exceptions::PyTypeError>(py));
             err.restore(py);
-            let err = PyErr::fetch(py).unwrap();
+            let err = PyErr::fetch(py);
             assert!(err.is_instance::<exceptions::PyTypeError>(py));
             assert_eq!(
                 err.to_string(),

src/exceptions.rs

@@ -829,7 +829,7 @@ mod tests {
             e.restore(py);
 
             assert_eq!(
-                PyErr::api_call_failed(py).to_string(),
+                PyErr::fetch(py).to_string(),
                 "UnicodeDecodeError: \'utf-8\' codec can\'t decode byte 0xd8 in position 2: invalid utf-8"
             );
         });

src/instance.rs

@@ -589,7 +589,7 @@ impl<T> Py<T> {
             let kwargs = kwargs.into_ptr();
             let ptr = ffi::PyObject_GetAttr(self.as_ptr(), name);
             if ptr.is_null() {
-                return Err(PyErr::api_call_failed(py));
+                return Err(PyErr::fetch(py));
             }
             let result = PyObject::from_owned_ptr_or_err(py, ffi::PyObject_Call(ptr, args, kwargs));
             ffi::Py_DECREF(ptr);
@@ -656,7 +656,7 @@ impl<T> Py<T> {
     pub unsafe fn from_owned_ptr_or_err(py: Python, ptr: *mut ffi::PyObject) -> PyResult<Py<T>> {
         match NonNull::new(ptr) {
             Some(nonnull_ptr) => Ok(Py(nonnull_ptr, PhantomData)),
-            None => Err(PyErr::api_call_failed(py)),
+            None => Err(PyErr::fetch(py)),
         }
     }
 
@@ -694,7 +694,7 @@ impl<T> Py<T> {
     /// `ptr` must be a pointer to a Python object of type T.
     #[inline]
     pub unsafe fn from_borrowed_ptr_or_err(py: Python, ptr: *mut ffi::PyObject) -> PyResult<Self> {
-        Self::from_borrowed_ptr_or_opt(py, ptr).ok_or_else(|| PyErr::api_call_failed(py))
+        Self::from_borrowed_ptr_or_opt(py, ptr).ok_or_else(|| PyErr::fetch(py))
     }
 
     /// Create a `Py<T>` instance by creating a new reference from the given FFI pointer.

src/pyclass.rs

@@ -122,7 +122,7 @@ where
 
     let type_object = unsafe { ffi::PyType_FromSpec(&mut spec) };
     if type_object.is_null() {
-        Err(PyErr::api_call_failed(py))
+        Err(PyErr::fetch(py))
     } else {
         tp_init_additional::<T>(type_object as _);
         Ok(type_object as _)