Refactor, simplify, add tests

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<PyResult<T>>` 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<PyObject>` instead of `ToPyObject`. #[1124](https://github.com/PyO3/pyo3/pull/1124)
 
 

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::<PyTypeError, _>("Error").restore(py);
+    PyTypeError::new_err("Error").restore(py);
     assert!(PyErr::occurred(py));
     drop(PyErr::fetch(py));
 }

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

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::<exceptions::PyValueError, _>(
+            _ => Err(exceptions::PyValueError::new_err(
                 "tp_richcompare called with invalid comparison operator",
             )),
         }

src/class/macros.rs

@@ -228,12 +228,10 @@ macro_rules! py_func_set {
                 let slf = py.from_borrowed_ptr::<$crate::PyCell<$generic>>(slf);
 
                 if value.is_null() {
-                    Err($crate::PyErr::new::<exceptions::PyNotImplementedError, _>(
-                        format!(
-                            "Subscript deletion not supported by {:?}",
-                            stringify!($generic)
-                        ),
-                    ))
+                    Err($crate::exceptions::PyNotImplementedError::new_err(format!(
+                        "Subscript deletion not supported by {:?}",
+                        stringify!($generic)
+                    )))
                 } else {
                     let name = py.from_borrowed_ptr::<$crate::PyAny>(name);
                     let value = py.from_borrowed_ptr::<$crate::PyAny>(value);
@@ -264,7 +262,7 @@ macro_rules! py_func_del {
                         .extract()?;
                     slf.try_borrow_mut()?.$fn_del(name).convert(py)
                 } else {
-                    Err(PyErr::new::<exceptions::PyNotImplementedError, _>(
+                    Err(exceptions::PyNotImplementedError::new_err(
                         "Subscript assignment not supported",
                     ))
                 }

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

src/class/sequence.rs

@@ -223,7 +223,7 @@ mod sq_ass_item_impl {
                 let slf = py.from_borrowed_ptr::<PyCell<T>>(slf);
 
                 if value.is_null() {
-                    return Err(PyErr::new::<exceptions::PyNotImplementedError, _>(format!(
+                    return Err(exceptions::PyNotImplementedError::new_err(format!(
                         "Item deletion is not supported by {:?}",
                         stringify!(T)
                     )));

src/err/err_state.rs

@@ -1,13 +1,7 @@
 use crate::{
-    exceptions::PyBaseException, ffi, types::PyType, IntoPyPointer, Py, PyObject, Python,
-    ToPyObject,
+    exceptions::PyBaseException, ffi, types::PyType, IntoPy, IntoPyPointer, Py, PyObject, Python,
 };
 
-pub(crate) enum PyErrValue {
-    ToArgs(Box<dyn PyErrArguments + Send + Sync>),
-    ToObject(Box<dyn ToPyObject + Send + Sync>),
-}
-
 #[derive(Clone)]
 pub(crate) struct PyErrStateNormalized {
     pub ptype: Py<PyType>,
@@ -18,7 +12,7 @@ pub(crate) struct PyErrStateNormalized {
 pub(crate) enum PyErrState {
     Lazy {
         ptype: Py<PyType>,
-        pvalue: Option<PyErrValue>,
+        pvalue: Box<dyn FnOnce(Python) -> PyObject + Send + Sync>,
     },
     FfiTuple {
         ptype: Option<PyObject>,
@@ -28,10 +22,25 @@ 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<T> PyErrArguments for T
+where
+    T: IntoPy<PyObject> + Send + Sync,
+{
+    fn arguments(self, py: Python) -> PyObject {
+        self.into_py(py)
+    }
+}
+
+pub(crate) fn boxed_args(
+    args: impl PyErrArguments + 'static,
+) -> Box<dyn FnOnce(Python) -> PyObject + Send + Sync> {
+    Box::new(|py| args.arguments(py))
 }
 
 impl PyErrState {
@@ -40,14 +49,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,

src/err/impls.rs

@@ -4,43 +4,29 @@ use std::io;
 /// Convert `PyErr` to `io::Error`
 impl std::convert::From<PyErr> 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))
     }
 }
 
 /// Create `OSError` from `io::Error`
 impl std::convert::From<io::Error> for PyErr {
     fn from(err: io::Error) -> PyErr {
         match err.kind() {
-            io::ErrorKind::BrokenPipe => {
-                PyErr::from_err_args::<exceptions::PyBrokenPipeError, _>(err)
-            }
-            io::ErrorKind::ConnectionRefused => {
-                PyErr::from_err_args::<exceptions::PyConnectionRefusedError, _>(err)
-            }
-            io::ErrorKind::ConnectionAborted => {
-                PyErr::from_err_args::<exceptions::PyConnectionAbortedError, _>(err)
-            }
-            io::ErrorKind::ConnectionReset => {
-                PyErr::from_err_args::<exceptions::PyConnectionResetError, _>(err)
-            }
-            io::ErrorKind::Interrupted => {
-                PyErr::from_err_args::<exceptions::PyInterruptedError, _>(err)
-            }
-            io::ErrorKind::NotFound => {
-                PyErr::from_err_args::<exceptions::PyFileNotFoundError, _>(err)
-            }
-            io::ErrorKind::WouldBlock => {
-                PyErr::from_err_args::<exceptions::PyBlockingIOError, _>(err)
-            }
-            io::ErrorKind::TimedOut => PyErr::from_err_args::<exceptions::PyTimeoutError, _>(err),
-            _ => PyErr::from_err_args::<exceptions::PyOSError, _>(err),
+            io::ErrorKind::BrokenPipe => exceptions::PyBrokenPipeError::new_err(err),
+            io::ErrorKind::ConnectionRefused => exceptions::PyConnectionRefusedError::new_err(err),
+            io::ErrorKind::ConnectionAborted => exceptions::PyConnectionAbortedError::new_err(err),
+            io::ErrorKind::ConnectionReset => exceptions::PyConnectionResetError::new_err(err),
+            io::ErrorKind::Interrupted => exceptions::PyInterruptedError::new_err(err),
+            io::ErrorKind::NotFound => exceptions::PyFileNotFoundError::new_err(err),
+            io::ErrorKind::WouldBlock => exceptions::PyBlockingIOError::new_err(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 +35,33 @@ impl<W: 'static + Send + Sync + std::fmt::Debug> std::convert::From<std::io::Int
     for PyErr
 {
     fn from(err: std::io::IntoInnerError<W>) -> PyErr {
-        PyErr::from_err_args::<exceptions::PyOSError, _>(err)
+        exceptions::PyOSError::new_err(err)
     }
 }
 
 impl<W: Send + Sync + std::fmt::Debug> PyErrArguments for std::io::IntoInnerError<W> {
-    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<std::convert::Infallible> for PyErr {
     fn from(_: std::convert::Infallible) -> PyErr {
-        PyErr::new::<exceptions::PyValueError, _>("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 +85,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");
+    }
+}