Merge pull request #1275 from davidhewitt/pyerr-debug

pyerr: improve debug & display impls

CHANGELOG.md

@@ -24,6 +24,8 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
 - `#[pyclass(subclass)]` is now required for subclassing from Rust (was previously just required for subclassing from Python). [#1152](https://github.com/PyO3/pyo3/pull/1152)
 - Change `PyIterator` to be consistent with other native types: it is now used as `&PyIterator` instead of `PyIterator<'a>`. [#1176](https://github.com/PyO3/pyo3/pull/1176)
 - Change formatting of `PyDowncastError` messages to be closer to Python's builtin error messages. [#1212](https://github.com/PyO3/pyo3/pull/1212)
+- Change `Debug` and `Display` impls for `PyException` to be consistent with `PyAny`. [#1275](https://github.com/PyO3/pyo3/pull/1275)
+- Change `Debug` impl of `PyErr` to output more helpful information (acquiring the GIL if necessary). [#1275](https://github.com/PyO3/pyo3/pull/1275)
 
 ### Removed
 - Remove deprecated ffi definitions `PyUnicode_AsUnicodeCopy`, `PyUnicode_GetMax`, `_Py_CheckRecursionLimit`, `PyObject_AsCharBuffer`, `PyObject_AsReadBuffer`, `PyObject_CheckReadBuffer` and `PyObject_AsWriteBuffer`, which will be removed in Python 3.10. [#1217](https://github.com/PyO3/pyo3/pull/1217)

src/err/mod.rs

@@ -176,7 +176,8 @@ impl PyErr {
     /// use pyo3::{Python, PyErr, exceptions::PyTypeError, types::PyType};
     /// Python::with_gil(|py| {
     ///     let err = PyTypeError::new_err(("some type error",));
-    ///     assert_eq!(err.pvalue(py).to_string(), "TypeError: some type error");
+    ///     assert!(err.is_instance::<PyTypeError>(py));
+    ///     assert_eq!(err.pvalue(py).to_string(), "some type error");
     /// });
     /// ```
     pub fn pvalue<'py>(&'py self, py: Python<'py>) -> &'py PyBaseException {
@@ -447,13 +448,28 @@ impl PyErr {
 
 impl std::fmt::Debug for PyErr {
     fn fmt(&self, f: &mut std::fmt::Formatter) -> Result<(), std::fmt::Error> {
-        f.write_str(format!("PyErr {{ type: {:?} }}", self.ptype_ptr()).as_str())
+        Python::with_gil(|py| {
+            f.debug_struct("PyErr")
+                .field("type", self.ptype(py))
+                .field("value", self.pvalue(py))
+                .field("traceback", &self.ptraceback(py))
+                .finish()
+        })
     }
 }
 
 impl std::fmt::Display for PyErr {
-    fn fmt(&self, f: &mut std::fmt::Formatter) -> Result<(), std::fmt::Error> {
+    fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result {
-        Python::with_gil(|py| self.instance(py).fmt(f))
+        Python::with_gil(|py| {
+            let instance = self.instance(py);
+            let type_name = instance.get_type().name().map_err(|_| std::fmt::Error)?;
+            write!(f, "{}", type_name)?;
+            if let Ok(s) = instance.str() {
+                write!(f, ": {}", &s.to_string_lossy())
+            } else {
+                write!(f, ": <exception str() failed>")
+            }
+        })
     }
 }
 
@@ -559,6 +575,55 @@ mod tests {
         let _ = PyErr::fetch(py);
     }
 
+    #[test]
+    fn err_debug() {
+        // Debug representation should be like the following (without the newlines):
+        // PyErr {
+        //     type: <class 'Exception'>,
+        //     value: Exception('banana'),
+        //     traceback: Some(<traceback object at 0x..)"
+        // }
+
+        let gil = Python::acquire_gil();
+        let py = gil.python();
+        let err = py
+            .run("raise Exception('banana')", None, None)
+            .expect_err("raising should have given us an error");
+
+        let debug_str = format!("{:?}", err);
+        assert!(debug_str.starts_with("PyErr { "));
+        assert!(debug_str.ends_with(" }"));
+
+        let mut fields = debug_str
+            .strip_prefix("PyErr { ")
+            .unwrap()
+            .strip_suffix(" }")
+            .unwrap()
+            .split(", ");
+
+        assert_eq!(fields.next().unwrap(), "type: <class 'Exception'>");
+        #[cfg(not(Py_3_7))] // Python 3.6 and below formats the repr differently
+        assert_eq!(fields.next().unwrap(), ("value: Exception('banana',)"));
+        #[cfg(Py_3_7)]
+        assert_eq!(fields.next().unwrap(), "value: Exception('banana')");
+
+        let traceback = fields.next().unwrap();
+        assert!(traceback.starts_with("traceback: Some(<traceback object at 0x"));
+        assert!(traceback.ends_with(">)"));
+
+        assert!(fields.next().is_none());
+    }
+
+    #[test]
+    fn err_display() {
+        let gil = Python::acquire_gil();
+        let py = gil.python();
+        let err = py
+            .run("raise Exception('banana')", None, None)
+            .expect_err("raising should have given us an error");
+        assert_eq!(err.to_string(), "Exception: banana");
+    }
+
     #[test]
     fn test_pyerr_send_sync() {
         fn is_send<T: Send>() {}

src/exceptions.rs

@@ -12,6 +12,8 @@ use std::os::raw::c_char;
 #[macro_export]
 macro_rules! impl_exception_boilerplate {
     ($name: ident) => {
+        $crate::pyobject_native_type_fmt!($name);
+
         impl std::convert::From<&$name> for $crate::PyErr {
             fn from(err: &$name) -> $crate::PyErr {
                 $crate::PyErr::from_instance(err)
@@ -27,27 +29,6 @@ macro_rules! impl_exception_boilerplate {
             }
         }
 
-        impl std::fmt::Debug for $name {
-            fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result {
-                let type_name = self.get_type().name().map_err(|_| std::fmt::Error)?;
-                f.debug_struct(type_name)
-                    // TODO: print out actual fields!
-                    .finish()
-            }
-        }
-
-        impl std::fmt::Display for $name {
-            fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result {
-                let type_name = self.get_type().name().map_err(|_| std::fmt::Error)?;
-                write!(f, "{}", type_name)?;
-                if let Ok(s) = self.str() {
-                    write!(f, ": {}", &s.to_string_lossy())
-                } else {
-                    write!(f, ": <exception str() failed>")
-                }
-            }
-        }
-
         impl std::error::Error for $name {
             fn source(&self) -> Option<&(dyn std::error::Error + 'static)> {
                 unsafe {
@@ -493,7 +474,6 @@ mod test {
     use super::{PyException, PyUnicodeDecodeError};
     use crate::types::{IntoPyDict, PyDict};
     use crate::{PyErr, Python};
-    use std::error::Error;
 
     import_exception!(socket, gaierror);
     import_exception!(email.errors, MessageError);
@@ -580,8 +560,12 @@ mod test {
         let exc = py
             .run("raise Exception('banana')", None, None)
             .expect_err("raising should have given us an error")
-            .into_instance(py);
+            .into_instance(py)
-        assert_eq!(format!("{:?}", exc.as_ref(py)), "Exception");
+            .into_ref(py);
+        assert_eq!(
+            format!("{:?}", exc),
+            exc.repr().unwrap().extract::<String>().unwrap()
+        );
     }
 
     #[test]
@@ -591,12 +575,18 @@ mod test {
         let exc = py
             .run("raise Exception('banana')", None, None)
             .expect_err("raising should have given us an error")
-            .into_instance(py);
+            .into_instance(py)
-        assert_eq!(exc.to_string(), "Exception: banana");
+            .into_ref(py);
+        assert_eq!(
+            exc.to_string(),
+            exc.str().unwrap().extract::<String>().unwrap()
+        );
     }
 
     #[test]
     fn native_exception_chain() {
+        use std::error::Error;
+
         let gil = Python::acquire_gil();
         let py = gil.python();
         let exc = py
@@ -606,10 +596,21 @@ mod test {
                 None,
             )
             .expect_err("raising should have given us an error")
-            .into_instance(py);
+            .into_instance(py)
-        assert_eq!(exc.to_string(), "Exception: banana");
+            .into_ref(py);
-        let source = exc.as_ref(py).source().expect("cause should exist");
+
-        assert_eq!(source.to_string(), "TypeError: peach");
+        #[cfg(Py_3_7)]
+        assert_eq!(format!("{:?}", exc), "Exception('banana')");
+        #[cfg(not(Py_3_7))]
+        assert_eq!(format!("{:?}", exc), "Exception('banana',)");
+
+        let source = exc.source().expect("cause should exist");
+
+        #[cfg(Py_3_7)]
+        assert_eq!(format!("{:?}", source), "TypeError('peach')");
+        #[cfg(not(Py_3_7))]
+        assert_eq!(format!("{:?}", source), "TypeError('peach',)");
+
         let source_source = source.source();
         assert!(source_source.is_none(), "source_source should be None");
     }
@@ -621,8 +622,8 @@ mod test {
         Python::with_gil(|py| {
             let decode_err = PyUnicodeDecodeError::new_utf8(py, invalid_utf8, err).unwrap();
             assert_eq!(
-                decode_err.to_string(),
+                format!("{:?}", decode_err),
-                "UnicodeDecodeError: \'utf-8\' codec can\'t decode byte 0xd8 in position 2: invalid utf-8"
+                "UnicodeDecodeError('utf-8', b'fo\\xd8o', 2, 3, 'invalid utf-8')"
             );
 
             // Restoring should preserve the same error

tests/test_inheritance.rs

@@ -99,10 +99,7 @@ fn mutation_fails() {
     let e = py
         .run("obj.base_set(lambda: obj.sub_set_and_ret(1))", global, None)
         .unwrap_err();
-    assert_eq!(
+    assert_eq!(&e.to_string(), "RuntimeError: Already borrowed")
-        &e.instance(py).to_string(),
-        "RuntimeError: Already borrowed"
-    )
 }
 
 #[pyclass(subclass)]