Simplify enums as suggested

CHANGELOG.md

@@ -31,9 +31,11 @@ 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()`.
+  - Remove `PyErr::from_value`, `PyErr::into_normalized()` and `PyErr::normalize()`.
+  - Change `PyErrValue` to be a private type.
 
 
 ### Removed

src/err/err_state.rs

@@ -4,23 +4,22 @@ use crate::{
 };
 
 #[derive(Clone)]
-pub struct PyErrStateNormalized {
+pub(crate) struct PyErrStateNormalized {
     pub ptype: Py<PyType>,
     pub pvalue: Py<PyBaseException>,
     pub ptraceback: Option<PyObject>,
 }
 
-pub enum PyErrState {
+pub(crate) enum PyErrState {
     Lazy {
         ptype: Py<PyType>,
-        pvalue: PyErrValue,
+        pvalue: Option<PyErrValue>,
     },
     FfiTuple {
         ptype: Option<PyObject>,
         pvalue: Option<PyObject>,
         ptraceback: Option<PyObject>,
     },
-    Normalizing,
     Normalized(PyErrStateNormalized),
 }
 
@@ -32,10 +31,9 @@ impl PyErrState {
         match self {
             PyErrState::Lazy { ptype, pvalue } => {
                 let pvalue = match pvalue {
-                    PyErrValue::None => std::ptr::null_mut(),
-                    PyErrValue::Value(ob) => ob.into_ptr(),
-                    PyErrValue::ToArgs(ob) => ob.arguments(py).into_ptr(),
-                    PyErrValue::ToObject(ob) => ob.to_object(py).into_ptr(),
+                    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())
             }
@@ -44,9 +42,6 @@ impl PyErrState {
                 pvalue,
                 ptraceback,
             } => (ptype.into_ptr(), pvalue.into_ptr(), ptraceback.into_ptr()),
-            PyErrState::Normalizing => {
-                panic!("Cannot get ffi tuple from PyErr whilst normalizing it.")
-            }
             PyErrState::Normalized(PyErrStateNormalized {
                 ptype,
                 pvalue,

src/err/err_value.rs

@@ -1,22 +1,10 @@
 use crate::{PyObject, Python, ToPyObject};
 
-/// Represents a `PyErr` value.
-pub enum PyErrValue {
-    None,
-    Value(PyObject),
+pub(crate) enum PyErrValue {
     ToArgs(Box<dyn PyErrArguments + Send + Sync>),
     ToObject(Box<dyn ToPyObject + Send + Sync>),
 }
 
-impl PyErrValue {
-    pub fn from_err_args<T>(value: T) -> Self
-    where
-        T: PyErrArguments + Send + Sync + 'static,
-    {
-        PyErrValue::ToArgs(Box::new(value))
-    }
-}
-
 /// Helper conversion trait that allows to use custom arguments for exception constructor.
 pub trait PyErrArguments {
     /// Arguments for exception

src/err/impls.rs

@@ -1,4 +1,4 @@
-use crate::{err::PyErrArguments, exceptions, IntoPy, PyErr, PyErrValue, PyObject, Python};
+use crate::{err::PyErrArguments, exceptions, IntoPy, PyErr, PyObject, Python};
 use std::io;
 
 /// Convert `PyErr` to `io::Error`
@@ -11,37 +11,30 @@ impl std::convert::From<PyErr> for io::Error {
 /// Create `OSError` from `io::Error`
 impl std::convert::From<io::Error> for PyErr {
     fn from(err: io::Error) -> PyErr {
-        macro_rules! err_value {
-            () => {
-                PyErrValue::from_err_args(err)
-            };
-        }
         match err.kind() {
             io::ErrorKind::BrokenPipe => {
-                PyErr::from_value::<exceptions::PyBrokenPipeError>(err_value!())
+                PyErr::from_err_args::<exceptions::PyBrokenPipeError, _>(err)
             }
             io::ErrorKind::ConnectionRefused => {
-                PyErr::from_value::<exceptions::PyConnectionRefusedError>(err_value!())
+                PyErr::from_err_args::<exceptions::PyConnectionRefusedError, _>(err)
             }
             io::ErrorKind::ConnectionAborted => {
-                PyErr::from_value::<exceptions::PyConnectionAbortedError>(err_value!())
+                PyErr::from_err_args::<exceptions::PyConnectionAbortedError, _>(err)
             }
             io::ErrorKind::ConnectionReset => {
-                PyErr::from_value::<exceptions::PyConnectionResetError>(err_value!())
+                PyErr::from_err_args::<exceptions::PyConnectionResetError, _>(err)
             }
             io::ErrorKind::Interrupted => {
-                PyErr::from_value::<exceptions::PyInterruptedError>(err_value!())
+                PyErr::from_err_args::<exceptions::PyInterruptedError, _>(err)
             }
             io::ErrorKind::NotFound => {
-                PyErr::from_value::<exceptions::PyFileNotFoundError>(err_value!())
+                PyErr::from_err_args::<exceptions::PyFileNotFoundError, _>(err)
             }
             io::ErrorKind::WouldBlock => {
-                PyErr::from_value::<exceptions::PyBlockingIOError>(err_value!())
-            }
-            io::ErrorKind::TimedOut => {
-                PyErr::from_value::<exceptions::PyTimeoutError>(err_value!())
+                PyErr::from_err_args::<exceptions::PyBlockingIOError, _>(err)
             }
-            _ => PyErr::from_value::<exceptions::PyOSError>(err_value!()),
+            io::ErrorKind::TimedOut => PyErr::from_err_args::<exceptions::PyTimeoutError, _>(err),
+            _ => PyErr::from_err_args::<exceptions::PyOSError, _>(err),
         }
     }
 }
@@ -56,7 +49,7 @@ 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_value::<exceptions::PyOSError>(PyErrValue::from_err_args(err))
+        PyErr::from_err_args::<exceptions::PyOSError, _>(err)
     }
 }
 
@@ -88,7 +81,7 @@ macro_rules! impl_to_pyerr {
 
         impl std::convert::From<$err> for PyErr {
             fn from(err: $err) -> PyErr {
-                PyErr::from_value::<$pyexc>(PyErrValue::from_err_args(err))
+                PyErr::from_err_args::<$pyexc, _>(err)
             }
         }
     };

src/err/mod.rs

@@ -24,18 +24,22 @@ mod err_value;
 mod impls;
 
 use err_state::{PyErrState, PyErrStateNormalized};
+use err_value::PyErrValue;
 
-pub use err_value::{PyErrArguments, PyErrValue};
+pub use err_value::PyErrArguments;
 
 /// Represents a Python exception that was raised.
 pub struct PyErr {
     // Safety: can only hand out references when in the "normalized" state. Will never change
     // after normalization.
-    state: UnsafeCell<PyErrState>,
+    //
+    // The state is temporarily removed from the PyErr during normalization, to avoid
+    // concurrent modifications.
+    state: UnsafeCell<Option<PyErrState>>,
 }
 
-unsafe impl Send for PyErr where PyErrState: Send {}
-unsafe impl Sync for PyErr where PyErrState: Sync {}
+unsafe impl Send for PyErr {}
+unsafe impl Sync for PyErr {}
 
 /// Represents the result of a Python call.
 pub type PyResult<T> = Result<T, PyErr>;
@@ -63,7 +67,7 @@ impl PyErr {
     /// * a tuple: the exception instance will be created using Python `T(*tuple)`
     /// * any other value: the exception instance will be created using Python `T(value)`
     ///
-    /// Note: if `value` is not `Send` or `Sync`, consider using `PyErr::from_value` instead.
+    /// Note: if `value` is not `Send` or `Sync`, consider using `PyErr::from_instance` instead.
     ///
     /// Panics if `T` is not a Python class derived from `BaseException`.
     ///
@@ -91,7 +95,7 @@ impl PyErr {
 
         PyErr::from_state(PyErrState::Lazy {
             ptype: ty.into(),
-            pvalue: PyErrValue::ToObject(Box::new(value)),
+            pvalue: Some(PyErrValue::ToObject(Box::new(value))),
         })
     }
 
@@ -106,14 +110,15 @@ impl PyErr {
     {
         PyErr::from_state(PyErrState::Lazy {
             ptype: exc.into(),
-            pvalue: PyErrValue::ToObject(Box::new(args)),
+            pvalue: Some(PyErrValue::ToObject(Box::new(args))),
         })
     }
 
-    /// Creates a new PyErr of type `T`.
-    pub fn from_value<T>(value: PyErrValue) -> PyErr
+    /// Creates a new PyErr of type `T`, with the corresponding arguments to lazily instantiate it.
+    pub fn from_err_args<T, A>(args: A) -> PyErr
     where
         T: PyTypeObject,
+        A: PyErrArguments + Send + Sync + 'static,
     {
         let gil = ensure_gil();
         let py = unsafe { gil.python() };
@@ -123,7 +128,7 @@ impl PyErr {
 
         PyErr::from_state(PyErrState::Lazy {
             ptype: ty.into(),
-            pvalue: value,
+            pvalue: Some(PyErrValue::ToArgs(Box::new(args))),
         })
     }
 
@@ -147,12 +152,14 @@ impl PyErr {
         } else if unsafe { ffi::PyExceptionClass_Check(obj.as_ptr()) } != 0 {
             PyErrState::Lazy {
                 ptype: unsafe { Py::from_borrowed_ptr(obj.py(), ptr) },
-                pvalue: PyErrValue::None,
+                pvalue: None,
             }
         } else {
             PyErrState::Lazy {
                 ptype: exceptions::PyTypeError::type_object(obj.py()).into(),
-                pvalue: PyErrValue::ToObject(Box::new("exceptions must derive from BaseException")),
+                pvalue: Some(PyErrValue::ToObject(Box::new(
+                    "exceptions must derive from BaseException",
+                ))),
             }
         };
 
@@ -356,7 +363,11 @@ impl PyErr {
     /// This is the opposite of `PyErr::fetch()`.
     #[inline]
     pub fn restore(self, py: Python) {
-        let (ptype, pvalue, ptraceback) = self.state.into_inner().into_ffi_tuple(py);
+        let (ptype, pvalue, ptraceback) = self
+            .state
+            .into_inner()
+            .expect("Cannot restore a PyErr while normalizing it")
+            .into_ffi_tuple(py);
         unsafe { ffi::PyErr_Restore(ptype, pvalue, ptraceback) }
     }
 
@@ -382,32 +393,36 @@ impl PyErr {
 
     fn from_state(state: PyErrState) -> PyErr {
         PyErr {
-            state: UnsafeCell::new(state),
+            state: UnsafeCell::new(Some(state)),
         }
     }
 
     /// Returns borrowed reference to this Err's type
     fn ptype_ptr(&self) -> *mut ffi::PyObject {
         match unsafe { &*self.state.get() } {
-            PyErrState::Lazy { ptype, .. } => ptype.as_ptr(),
-            PyErrState::FfiTuple { ptype, .. } => ptype.as_ptr(),
-            PyErrState::Normalizing => panic!("Cannot access exception type while normalizing"),
-            PyErrState::Normalized(n) => n.ptype.as_ptr(),
+            Some(PyErrState::Lazy { ptype, .. }) => ptype.as_ptr(),
+            Some(PyErrState::FfiTuple { ptype, .. }) => ptype.as_ptr(),
+            Some(PyErrState::Normalized(n)) => n.ptype.as_ptr(),
+            None => panic!("Cannot access exception type while normalizing"),
         }
     }
 
     fn normalized(&self, py: Python) -> &PyErrStateNormalized {
-        if let PyErrState::Normalized(n) = unsafe { &*self.state.get() } {
+        if let Some(PyErrState::Normalized(n)) = unsafe { &*self.state.get() } {
             return n;
         }
 
-        let state = unsafe { std::mem::replace(&mut *self.state.get(), PyErrState::Normalizing) };
+        let state = unsafe {
+            (*self.state.get())
+                .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 = PyErrState::Normalized(PyErrStateNormalized {
+            *self_state = Some(PyErrState::Normalized(PyErrStateNormalized {
                 ptype: Py::from_owned_ptr_or_opt(py, ptype)
                     .unwrap_or_else(|| exceptions::PySystemError::type_object(py).into()),
                 pvalue: Py::from_owned_ptr_or_opt(py, pvalue).unwrap_or_else(|| {
@@ -416,10 +431,10 @@ impl PyErr {
                         .into_py(py)
                 }),
                 ptraceback: PyObject::from_owned_ptr_or_opt(py, ptraceback),
-            });
+            }));
 
             match self_state {
-                PyErrState::Normalized(n) => n,
+                Some(PyErrState::Normalized(n)) => n,
                 _ => unreachable!(),
             }
         }
@@ -507,6 +522,7 @@ pub fn error_on_minusone(py: Python, result: c_int) -> PyResult<()> {
 
 #[cfg(test)]
 mod tests {
+    use super::PyErrState;
     use crate::exceptions;
     use crate::panic::PanicException;
     use crate::{PyErr, Python};
@@ -548,5 +564,8 @@ mod tests {
 
         is_send::<PyErr>();
         is_sync::<PyErr>();
+
+        is_send::<PyErrState>();
+        is_sync::<PyErrState>();
     }
 }

src/lib.rs

@@ -138,7 +138,7 @@ pub use crate::conversion::{
     AsPyPointer, FromPyObject, FromPyPointer, IntoPy, IntoPyPointer, PyTryFrom, PyTryInto,
     ToBorrowedObject, ToPyObject,
 };
-pub use crate::err::{PyDowncastError, PyErr, PyErrArguments, PyErrValue, PyResult};
+pub use crate::err::{PyDowncastError, PyErr, PyErrArguments, PyResult};
 pub use crate::gil::{GILGuard, GILPool};
 pub use crate::instance::{Py, PyNativeType, PyObject};
 pub use crate::pycell::{PyCell, PyRef, PyRefMut};