diff --git a/CHANGELOG.md b/CHANGELOG.md index 0c30f49ad8c..7f9ad9e364a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -32,7 +32,6 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0. - Rework PyErr to be compatible with the `std::error::Error` trait: [#1115](https://github.com/PyO3/pyo3/pull/1115) - Implement `Display` and `Error` for `PyErr`. - Add `PyErr::instance()` which returns `&PyBaseException`. - - Add `PyErr::from_err_args`. - `PyErr`'s fields are now an implementation detail. The equivalent values can be accessed with `PyErr::ptype()`, `PyErr::pvalue()` and `PyErr::ptraceback()`. - Change `PyErr::print()` and `PyErr::print_and_set_sys_last_vars()` to take `&self` instead of `self`. - Remove `PyErr::into_normalized()` and `PyErr::normalize()`. @@ -41,6 +40,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0. - Remove `PyException::into()` and `Into>` for `PyErr` and `PyException`. - Rename `PyException::py_err()` to `PyException::new_err()`. - Rename `PyUnicodeDecodeErr::new_err()` to `PyUnicodeDecodeErr::new()`. + - Remove `PyStopIteration::stop_iteration`. - `PyModule::add` now uses `IntoPy` instead of `ToPyObject`. #[1124](https://github.com/PyO3/pyo3/pull/1124) diff --git a/guide/src/exception.md b/guide/src/exception.md index 1d02b86eb43..cea079c5025 100644 --- a/guide/src/exception.md +++ b/guide/src/exception.md @@ -61,7 +61,7 @@ use pyo3::exceptions::PyTypeError; fn main() { let gil = Python::acquire_gil(); let py = gil.python(); - PyErr::new::("Error").restore(py); + PyTypeError::new_err("Error").restore(py); assert!(PyErr::occurred(py)); drop(PyErr::fetch(py)); } diff --git a/guide/src/migration.md b/guide/src/migration.md index bd0dddbac49..2ec8c9997be 100644 --- a/guide/src/migration.md +++ b/guide/src/migration.md @@ -85,7 +85,6 @@ let list_ref: &PyList = list_py.as_ref(py); # }) ``` - ## from 0.10.* to 0.11 ### Stable Rust diff --git a/src/class/basic.rs b/src/class/basic.rs index d95053bf828..e082405a882 100644 --- a/src/class/basic.rs +++ b/src/class/basic.rs @@ -9,7 +9,7 @@ //! [typeobj docs](https://docs.python.org/3/c-api/typeobj.html) use crate::callback::{HashCallbackOutput, IntoPyCallbackOutput}; -use crate::{exceptions, ffi, FromPyObject, PyAny, PyCell, PyClass, PyErr, PyObject, PyResult}; +use crate::{exceptions, ffi, FromPyObject, PyAny, PyCell, PyClass, PyObject, PyResult}; use std::os::raw::c_int; /// Operators for the __richcmp__ method @@ -260,7 +260,7 @@ where ffi::Py_NE => Ok(CompareOp::Ne), ffi::Py_GT => Ok(CompareOp::Gt), ffi::Py_GE => Ok(CompareOp::Ge), - _ => Err(PyErr::new::( + _ => Err(exceptions::PyValueError::new_err( "tp_richcompare called with invalid comparison operator", )), } diff --git a/src/class/macros.rs b/src/class/macros.rs index 2ad6a974ab0..4a126a61d00 100644 --- a/src/class/macros.rs +++ b/src/class/macros.rs @@ -228,7 +228,7 @@ macro_rules! py_func_set { let slf = py.from_borrowed_ptr::<$crate::PyCell<$generic>>(slf); if value.is_null() { - Err($crate::PyErr::new::( + Err($crate::exceptions::PyNotImplementedError::new_err( format!( "Subscript deletion not supported by {:?}", stringify!($generic) @@ -264,7 +264,7 @@ macro_rules! py_func_del { .extract()?; slf.try_borrow_mut()?.$fn_del(name).convert(py) } else { - Err(PyErr::new::( + Err(exceptions::PyNotImplementedError::new_err( "Subscript assignment not supported", )) } diff --git a/src/class/mapping.rs b/src/class/mapping.rs index d20547e18b8..a8c30651f9c 100644 --- a/src/class/mapping.rs +++ b/src/class/mapping.rs @@ -4,7 +4,6 @@ //! Trait and support implementation for implementing mapping support use crate::callback::IntoPyCallbackOutput; -use crate::err::PyErr; use crate::{exceptions, ffi, FromPyObject, PyClass, PyObject}; /// Mapping interface diff --git a/src/class/sequence.rs b/src/class/sequence.rs index a9c62f03d0b..d2f29a7f216 100644 --- a/src/class/sequence.rs +++ b/src/class/sequence.rs @@ -223,7 +223,7 @@ mod sq_ass_item_impl { let slf = py.from_borrowed_ptr::>(slf); if value.is_null() { - return Err(PyErr::new::(format!( + return Err(exceptions::PyNotImplementedError::new_err(format!( "Item deletion is not supported by {:?}", stringify!(T) ))); diff --git a/src/err/err_state.rs b/src/err/err_state.rs index 1ad4343249a..382c2d5d81d 100644 --- a/src/err/err_state.rs +++ b/src/err/err_state.rs @@ -1,13 +1,8 @@ use crate::{ exceptions::PyBaseException, ffi, types::PyType, IntoPyPointer, Py, PyObject, Python, - ToPyObject, + IntoPy, }; -pub(crate) enum PyErrValue { - ToArgs(Box), - ToObject(Box), -} - #[derive(Clone)] pub(crate) struct PyErrStateNormalized { pub ptype: Py, @@ -18,7 +13,7 @@ pub(crate) struct PyErrStateNormalized { pub(crate) enum PyErrState { Lazy { ptype: Py, - pvalue: Option, + pvalue: Box PyObject + Send + Sync>, }, FfiTuple { ptype: Option, @@ -28,10 +23,23 @@ pub(crate) enum PyErrState { Normalized(PyErrStateNormalized), } -/// Helper conversion trait that allows to use custom arguments for exception constructor. -pub trait PyErrArguments { +/// Helper conversion trait that allows to use custom arguments for lazy exception construction. +pub trait PyErrArguments: Send + Sync { /// Arguments for exception - fn arguments(&self, _: Python) -> PyObject; + fn arguments(self, py: Python) -> PyObject; +} + +impl PyErrArguments for T +where + T: IntoPy + Send + Sync, +{ + fn arguments(self, py: Python) -> PyObject { + self.into_py(py) + } +} + +pub(crate) fn boxed_args(args: impl PyErrArguments + 'static) -> Box PyObject + Send + Sync> { + Box::new(|py| args.arguments(py)) } impl PyErrState { @@ -40,14 +48,11 @@ impl PyErrState { py: Python, ) -> (*mut ffi::PyObject, *mut ffi::PyObject, *mut ffi::PyObject) { match self { - PyErrState::Lazy { ptype, pvalue } => { - let pvalue = match pvalue { - Some(PyErrValue::ToArgs(ob)) => ob.arguments(py).into_ptr(), - Some(PyErrValue::ToObject(ob)) => ob.to_object(py).into_ptr(), - None => std::ptr::null_mut(), - }; - (ptype.into_ptr(), pvalue, std::ptr::null_mut()) - } + PyErrState::Lazy { ptype, pvalue } => ( + ptype.into_ptr(), + pvalue(py).into_ptr(), + std::ptr::null_mut(), + ), PyErrState::FfiTuple { ptype, pvalue, diff --git a/src/err/impls.rs b/src/err/impls.rs index 05a9d612b6e..d52b82aee37 100644 --- a/src/err/impls.rs +++ b/src/err/impls.rs @@ -4,7 +4,7 @@ use std::io; /// Convert `PyErr` to `io::Error` impl std::convert::From for io::Error { fn from(err: PyErr) -> Self { - io::Error::new(io::ErrorKind::Other, format!("Python exception: {:?}", err)) + io::Error::new(io::ErrorKind::Other, format!("Python exception: {}", err)) } } @@ -13,34 +13,34 @@ impl std::convert::From for PyErr { fn from(err: io::Error) -> PyErr { match err.kind() { io::ErrorKind::BrokenPipe => { - PyErr::from_err_args::(err) + exceptions::PyBrokenPipeError::new_err(err) } io::ErrorKind::ConnectionRefused => { - PyErr::from_err_args::(err) + exceptions::PyConnectionRefusedError::new_err(err) } io::ErrorKind::ConnectionAborted => { - PyErr::from_err_args::(err) + exceptions::PyConnectionAbortedError::new_err(err) } io::ErrorKind::ConnectionReset => { - PyErr::from_err_args::(err) + exceptions::PyConnectionResetError::new_err(err) } io::ErrorKind::Interrupted => { - PyErr::from_err_args::(err) + exceptions::PyInterruptedError::new_err(err) } io::ErrorKind::NotFound => { - PyErr::from_err_args::(err) + exceptions::PyFileNotFoundError::new_err(err) } io::ErrorKind::WouldBlock => { - PyErr::from_err_args::(err) + exceptions::PyBlockingIOError::new_err(err) } - io::ErrorKind::TimedOut => PyErr::from_err_args::(err), - _ => PyErr::from_err_args::(err), + io::ErrorKind::TimedOut => exceptions::PyTimeoutError::new_err(err), + _ => exceptions::PyOSError::new_err(err), } } } impl PyErrArguments for io::Error { - fn arguments(&self, py: Python) -> PyObject { + fn arguments(self, py: Python) -> PyObject { self.to_string().into_py(py) } } @@ -49,39 +49,33 @@ impl std::convert::From) -> PyErr { - PyErr::from_err_args::(err) + exceptions::PyOSError::new_err(err) } } impl PyErrArguments for std::io::IntoInnerError { - fn arguments(&self, py: Python) -> PyObject { + fn arguments(self, py: Python) -> PyObject { self.to_string().into_py(py) } } -impl PyErrArguments for std::convert::Infallible { - fn arguments(&self, py: Python) -> PyObject { - "Infalliable!".into_py(py) - } -} - impl std::convert::From for PyErr { fn from(_: std::convert::Infallible) -> PyErr { - PyErr::new::("Infalliable!") + unreachable!() } } macro_rules! impl_to_pyerr { ($err: ty, $pyexc: ty) => { impl PyErrArguments for $err { - fn arguments(&self, py: Python) -> PyObject { + fn arguments(self, py: Python) -> PyObject { self.to_string().into_py(py) } } impl std::convert::From<$err> for PyErr { fn from(err: $err) -> PyErr { - PyErr::from_err_args::<$pyexc, _>(err) + <$pyexc>::new_err(err) } } }; @@ -105,3 +99,30 @@ impl_to_pyerr!( exceptions::PyUnicodeDecodeError ); impl_to_pyerr!(std::net::AddrParseError, exceptions::PyValueError); + +#[cfg(test)] +mod tests { + use crate::PyErr; + use std::io; + + #[test] + fn io_errors() { + let check_err = |kind, expected_ty| { + let py_err: PyErr = io::Error::new(kind, "some error msg").into(); + let err_msg = format!("{}: some error msg", expected_ty); + assert_eq!(py_err.to_string(), err_msg); + + let os_err: io::Error = py_err.into(); + assert_eq!(os_err.to_string(), format!("Python exception: {}", err_msg)); + }; + + check_err(io::ErrorKind::BrokenPipe, "BrokenPipeError"); + check_err(io::ErrorKind::ConnectionRefused, "ConnectionRefusedError"); + check_err(io::ErrorKind::ConnectionAborted, "ConnectionAbortedError"); + check_err(io::ErrorKind::ConnectionReset, "ConnectionResetError"); + check_err(io::ErrorKind::Interrupted, "InterruptedError"); + check_err(io::ErrorKind::NotFound, "FileNotFoundError"); + check_err(io::ErrorKind::WouldBlock, "BlockingIOError"); + check_err(io::ErrorKind::TimedOut, "TimeoutError"); + } +} diff --git a/src/err/mod.rs b/src/err/mod.rs index 2671a2ec2ba..37a53ae3afc 100644 --- a/src/err/mod.rs +++ b/src/err/mod.rs @@ -1,6 +1,5 @@ // Copyright (c) 2017-present PyO3 Project and Contributors -use crate::gil::ensure_gil; use crate::panic::PanicException; use crate::type_object::PyTypeObject; use crate::types::PyType; @@ -23,7 +22,7 @@ mod err_state; mod impls; pub use err_state::PyErrArguments; -use err_state::{PyErrState, PyErrStateNormalized, PyErrValue}; +use err_state::{PyErrState, PyErrStateNormalized, boxed_args}; /// Represents a Python exception that was raised. pub struct PyErr { @@ -77,20 +76,19 @@ impl PyErr { /// ```ignore /// return Err(exceptions::PyTypeError::new_err("Error message")); /// ``` - pub fn new(value: V) -> PyErr + pub fn new(args: A) -> PyErr where T: PyTypeObject, - V: ToPyObject + Send + Sync + 'static, + A: PyErrArguments + Send + Sync + 'static, { - let gil = ensure_gil(); - let py = unsafe { gil.python() }; - - let ty = T::type_object(py); - assert_ne!(unsafe { ffi::PyExceptionClass_Check(ty.as_ptr()) }, 0); + Python::with_gil(|py| { + let ty = T::type_object(py); + assert_ne!(unsafe { ffi::PyExceptionClass_Check(ty.as_ptr()) }, 0); - PyErr::from_state(PyErrState::Lazy { - ptype: ty.into(), - pvalue: Some(PyErrValue::ToObject(Box::new(value))), + PyErr::from_state(PyErrState::Lazy { + ptype: ty.into(), + pvalue: boxed_args(args), + }) }) } @@ -101,29 +99,11 @@ impl PyErr { /// `args` is the a tuple of arguments to pass to the exception constructor. pub fn from_type(exc: &PyType, args: A) -> PyErr where - A: ToPyObject + Send + Sync + 'static, - { - PyErr::from_state(PyErrState::Lazy { - ptype: exc.into(), - pvalue: Some(PyErrValue::ToObject(Box::new(args))), - }) - } - - /// Creates a new PyErr of type `T`, with the corresponding arguments to lazily instantiate it. - pub fn from_err_args(args: A) -> PyErr - where - T: PyTypeObject, A: PyErrArguments + Send + Sync + 'static, { - let gil = ensure_gil(); - let py = unsafe { gil.python() }; - - let ty = T::type_object(py); - assert_ne!(unsafe { ffi::PyExceptionClass_Check(ty.as_ptr()) }, 0); - PyErr::from_state(PyErrState::Lazy { - ptype: ty.into(), - pvalue: Some(PyErrValue::ToArgs(Box::new(args))), + ptype: exc.into(), + pvalue: boxed_args(args), }) } @@ -133,6 +113,24 @@ impl PyErr { /// If `obj` is a Python exception type object, the PyErr will (lazily) create a new /// instance of that type. /// Otherwise, a `TypeError` is created instead. + /// + /// # Example + /// ```rust + /// use pyo3::{Python, PyErr, IntoPy, exceptions::PyTypeError, types::PyType}; + /// Python::with_gil(|py| { + /// // Case #1: Exception instance + /// let err = PyErr::from_instance(PyTypeError::new_err("some type error",).instance(py)); + /// assert_eq!(err.to_string(), "TypeError: some type error"); + /// + /// // Case #2: Exception type + /// let err = PyErr::from_instance(PyType::new::(py)); + /// assert_eq!(err.to_string(), "TypeError: "); + /// + /// // Case #3: Invalid exception value + /// let err = PyErr::from_instance("foo".into_py(py).as_ref(py)); + /// assert_eq!(err.to_string(), "TypeError: exceptions must derive from BaseException"); + /// }); + /// ``` pub fn from_instance(obj: &PyAny) -> PyErr { let ptr = obj.as_ptr(); @@ -145,16 +143,15 @@ impl PyErr { ptraceback: None, }) } else if unsafe { ffi::PyExceptionClass_Check(obj.as_ptr()) } != 0 { - PyErrState::Lazy { - ptype: unsafe { Py::from_borrowed_ptr(obj.py(), ptr) }, + PyErrState::FfiTuple { + ptype: unsafe { Some(Py::from_borrowed_ptr(obj.py(), ptr)) }, pvalue: None, + ptraceback: None, } } else { PyErrState::Lazy { ptype: exceptions::PyTypeError::type_object(obj.py()).into(), - pvalue: Some(PyErrValue::ToObject(Box::new( - "exceptions must derive from BaseException", - ))), + pvalue: boxed_args("exceptions must derive from BaseException"), } }; diff --git a/src/exceptions.rs b/src/exceptions.rs index 2a5b92c1503..332645646b8 100644 --- a/src/exceptions.rs +++ b/src/exceptions.rs @@ -3,27 +3,26 @@ //! Exception types defined by Python. use crate::type_object::PySizedLayout; -use crate::types::PyTuple; -use crate::{ffi, AsPyPointer, PyResult, Python}; +use crate::{ffi, PyResult, Python}; use std::ffi::CStr; use std::ops; -use std::os::raw::c_char; /// The boilerplate to convert between a Rust type and a Python exception. #[macro_export] macro_rules! impl_exception_boilerplate { ($name: ident) => { - impl std::convert::From<$name> for $crate::PyErr { - fn from(_err: $name) -> $crate::PyErr { - $crate::PyErr::new::<$name, _>(()) + impl std::convert::From<&$name> for $crate::PyErr { + fn from(err: &$name) -> $crate::PyErr { + $crate::PyErr::from_instance(err) } } impl $name { - pub fn new_err( - args: V, - ) -> $crate::PyErr { - $crate::PyErr::new::<$name, V>(args) + pub fn new_err(args: A) -> $crate::PyErr + where + A: $crate::PyErrArguments + Send + Sync + 'static + { + $crate::PyErr::new::<$name, A>(args) } } @@ -444,10 +443,9 @@ impl PyUnicodeDecodeError { reason: &CStr, ) -> PyResult<&'p PyUnicodeDecodeError> { unsafe { - let input: &[c_char] = &*(input as *const [u8] as *const [c_char]); py.from_owned_ptr_or_err(ffi::PyUnicodeDecodeError_Create( encoding.as_ptr(), - input.as_ptr(), + input.as_ptr() as *const i8, input.len() as ffi::Py_ssize_t, range.start as ffi::Py_ssize_t, range.end as ffi::Py_ssize_t, @@ -456,7 +454,6 @@ impl PyUnicodeDecodeError { } } - #[allow(clippy::range_plus_one)] // False positive, ..= returns the wrong type pub fn new_utf8<'p>( py: Python<'p>, input: &[u8], @@ -467,23 +464,12 @@ impl PyUnicodeDecodeError { py, CStr::from_bytes_with_nul(b"utf-8\0").unwrap(), input, - pos..pos + 1, + pos..(pos + 1), CStr::from_bytes_with_nul(b"invalid utf-8\0").unwrap(), ) } } -impl PyStopIteration { - pub fn stop_iteration(_py: Python, args: &PyTuple) { - unsafe { - ffi::PyErr_SetObject( - ffi::PyExc_StopIteration as *mut ffi::PyObject, - args.as_ptr(), - ); - } - } -} - /// Exceptions defined in `asyncio` module pub mod asyncio { import_exception!(asyncio, CancelledError); @@ -504,7 +490,7 @@ pub mod socket { #[cfg(test)] mod test { - use crate::exceptions::PyException; + use super::{PyException, PyUnicodeDecodeError}; use crate::types::{IntoPyDict, PyDict}; use crate::{PyErr, Python}; use std::error::Error; @@ -587,6 +573,17 @@ mod test { .unwrap(); } + #[test] + fn native_exception_debug() { + let gil = Python::acquire_gil(); + let py = gil.python(); + let exc = py + .run("raise Exception('banana')", None, None) + .expect_err("raising should have given us an error") + .into_instance(py); + assert_eq!(format!("{:?}", exc.as_ref(py)), "Exception"); + } + #[test] fn native_exception_display() { let gil = Python::acquire_gil(); @@ -616,4 +613,26 @@ mod test { let source_source = source.source(); assert!(source_source.is_none(), "source_source should be None"); } + + #[test] + fn unicode_decode_error() { + let invalid_utf8 = b"fo\xd8o"; + let err = std::str::from_utf8(invalid_utf8).expect_err("should be invalid utf8"); + Python::with_gil(|py| { + let decode_err = PyUnicodeDecodeError::new_utf8(py, invalid_utf8, err).unwrap(); + assert_eq!( + decode_err.to_string(), + "UnicodeDecodeError: \'utf-8\' codec can\'t decode byte 0xd8 in position 2: invalid utf-8" + ); + + // Restoring should preserve the same error + let e: PyErr = decode_err.into(); + e.restore(py); + + assert_eq!( + PyErr::fetch(py).to_string(), + "UnicodeDecodeError: \'utf-8\' codec can\'t decode byte 0xd8 in position 2: invalid utf-8" + ); + }); + } } diff --git a/src/gil.rs b/src/gil.rs index 989f1540d0d..fb7851e8b37 100644 --- a/src/gil.rs +++ b/src/gil.rs @@ -365,10 +365,8 @@ fn decrement_gil_count() { }); } -/// Ensure the GIL is held, useful in implementation of APIs like PyErr::new where it's -/// inconvenient to force the user to acquire the GIL. -#[doc(hidden)] -pub fn ensure_gil() -> EnsureGIL { +/// Ensure the GIL is held, used in the implementation of Python::with_gil +pub(crate) fn ensure_gil() -> EnsureGIL { if gil_is_acquired() { EnsureGIL(None) } else { @@ -377,8 +375,7 @@ pub fn ensure_gil() -> EnsureGIL { } /// Struct used internally which avoids acquiring the GIL where it's not necessary. -#[doc(hidden)] -pub struct EnsureGIL(Option); +pub(crate) struct EnsureGIL(Option); impl EnsureGIL { /// Get the GIL token. diff --git a/src/lib.rs b/src/lib.rs index c741610802a..8a9665dddb6 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -196,12 +196,6 @@ mod python; pub mod type_object; pub mod types; -/// Internal utilities exposed for rust-numpy -#[doc(hidden)] -pub mod internal_utils { - pub use crate::gil::{ensure_gil, EnsureGIL}; -} - /// The proc macros, which are also part of the prelude. #[cfg(feature = "macros")] pub mod proc_macro { diff --git a/src/python.rs b/src/python.rs index 901a426bcf0..fd736e99812 100644 --- a/src/python.rs +++ b/src/python.rs @@ -126,7 +126,7 @@ impl<'p> Python<'p> { /// .collect(); /// let mut sum = 0; /// for t in threads { - /// sum += t.join().map_err(|_| PyErr::new::(()))?; + /// sum += t.join().map_err(|_| PyRuntimeError::new_err(()))?; /// } /// Ok(sum) /// }) diff --git a/tests/test_dunder.rs b/tests/test_dunder.rs index 5cc56f61575..2633aff43ef 100644 --- a/tests/test_dunder.rs +++ b/tests/test_dunder.rs @@ -170,7 +170,7 @@ impl PySequenceProtocol for Sequence { if let Some(s) = self.fields.get(idx) { Ok(s.clone()) } else { - Err(PyErr::new::(())) + Err(PyIndexError::new_err(())) } } @@ -180,7 +180,7 @@ impl PySequenceProtocol for Sequence { *elem = value; Ok(()) } else { - Err(PyErr::new::(())) + Err(PyIndexError::new_err(())) } } } @@ -436,7 +436,7 @@ impl<'p> PyMappingProtocol<'p> for Test { return Ok("int"); } } - Err(PyErr::new::("error")) + Err(PyValueError::new_err("error")) } }