Merge pull request #1212 from Askaholic/issue/#1055-add-arg-name-to-conversion-error

Enhance error messages of conversion errors

CHANGELOG.md

@@ -6,8 +6,12 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/)
 and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.html).
 
 ## [Unreleased]
+### Added
+- Add argument names to `TypeError` messages generated by pymethod wrappers. [#1212](https://github.com/PyO3/pyo3/pull/1212)
+
 ### Changed
 - 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)
 
 ### 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)

guide/src/conversions/traits.md

@@ -171,8 +171,7 @@ enum RustyEnum {
 ```
 
 If the input is neither a string nor an integer, the error message will be:
-`"Can't convert <INPUT> to Union[str, int]"`, where `<INPUT>` is replaced by the type name and
-`repr()` of the input object.
+`"'<INPUT_TYPE>' cannot be converted to 'Union[str, int]'"`.
 
 #### `#[derive(FromPyObject)]` Container Attributes
 - `pyo3(transparent)`

pyo3-derive-backend/src/from_pyobject.rs

@@ -73,11 +73,7 @@ impl<'a> Enum<'a> {
         quote!(
             #(#var_extracts)*
             let type_name = obj.get_type().name();
-            let from = obj
-                .repr()
-                .map(|s| format!("{} ({})", s.to_string_lossy(), type_name))
-                .unwrap_or_else(|_| type_name.to_string());
-            let err_msg = format!("Can't convert {} to {}", from, #error_names);
+            let err_msg = format!("'{}' object cannot be converted to '{}'", type_name, #error_names);
             Err(::pyo3::exceptions::PyTypeError::new_err(err_msg))
         )
     }

pyo3-derive-backend/src/pymethod.rs

@@ -475,10 +475,14 @@ fn impl_arg_param(
 
     let ty = arg.ty;
     let name = arg.name;
+    let transform_error = quote! {
+        |e| pyo3::derive_utils::argument_extraction_error(_py, stringify!(#name), e)
+    };
 
     if spec.is_args(&name) {
         return quote! {
-            let #arg_name = <#ty as pyo3::FromPyObject>::extract(_args.as_ref())?;
+            let #arg_name = <#ty as pyo3::FromPyObject>::extract(_args.as_ref())
+                .map_err(#transform_error)?;
         };
     } else if spec.is_kwargs(&name) {
         return quote! {
@@ -518,7 +522,7 @@ fn impl_arg_param(
 
         quote! {
             let #mut_ _tmp: #target_ty = match #arg_value {
-                Some(_obj) => _obj.extract()?,
+                Some(_obj) => _obj.extract().map_err(#transform_error)?,
                 None => #default,
             };
             let #arg_name = #borrow_tmp;
@@ -526,7 +530,7 @@ fn impl_arg_param(
     } else {
         quote! {
             let #arg_name = match #arg_value {
-                Some(_obj) => _obj.extract()?,
+                Some(_obj) => _obj.extract().map_err(#transform_error)?,
                 None => #default,
             };
         }

src/derive_utils.rs

@@ -8,7 +8,7 @@ use crate::err::{PyErr, PyResult};
 use crate::exceptions::PyTypeError;
 use crate::instance::PyNativeType;
 use crate::pyclass::{PyClass, PyClassThreadChecker};
-use crate::types::{PyAny, PyDict, PyModule, PyTuple};
+use crate::types::{PyAny, PyDict, PyModule, PyString, PyTuple};
 use crate::{ffi, GILPool, IntoPy, PyCell, Python};
 use std::cell::UnsafeCell;
 
@@ -111,6 +111,19 @@ pub fn parse_fn_args<'p>(
     Ok((args, kwargs))
 }
 
+/// Add the argument name to the error message of an error which occurred during argument extraction
+pub fn argument_extraction_error(py: Python, arg_name: &str, error: PyErr) -> PyErr {
+    if error.ptype(py) == py.get_type::<PyTypeError>() {
+        let reason = error
+            .instance(py)
+            .str()
+            .unwrap_or_else(|_| PyString::new(py, ""));
+        PyTypeError::new_err(format!("argument '{}': {}", arg_name, reason))
+    } else {
+        error
+    }
+}
+
 /// `Sync` wrapper of `ffi::PyModuleDef`.
 #[doc(hidden)]
 pub struct ModuleDef(UnsafeCell<ffi::PyModuleDef>);

src/err/mod.rs

@@ -490,11 +490,8 @@ impl<'a> std::fmt::Display for PyDowncastError<'a> {
     fn fmt(&self, f: &mut std::fmt::Formatter) -> Result<(), std::fmt::Error> {
         write!(
             f,
-            "Can't convert {} to {}",
-            self.from
-                .repr()
-                .map(|s| s.to_string_lossy())
-                .unwrap_or_else(|_| self.from.get_type().name()),
+            "'{}' object cannot be converted to '{}'",
+            self.from.get_type().name(),
             self.to
         )
     }

tests/common.rs

@@ -16,9 +16,22 @@ macro_rules! py_expect_exception {
         let d = [(stringify!($val), &$val)].into_py_dict($py);
 
         let res = $py.run($code, None, Some(d));
-        let err = res.unwrap_err();
+        let err = res.expect_err(&format!("Did not raise {}", stringify!($err)));
         if !err.matches($py, $py.get_type::<pyo3::exceptions::$err>()) {
             panic!("Expected {} but got {:?}", stringify!($err), err)
         }
+        err
+    }};
+    ($py:expr, $val:ident, $code:expr, $err:ident, $err_msg:expr) => {{
+        let err = py_expect_exception!($py, $val, $code, $err);
+        assert_eq!(
+            err.instance($py)
+                .str()
+                .expect("error str() failed")
+                .to_str()
+                .expect("message was not valid utf8"),
+            $err_msg
+        );
+        err
     }};
 }

tests/test_arithmetics.rs

@@ -604,7 +604,7 @@ mod return_not_implemented {
             c2,
             &format!("class Other: pass\nc2 {} Other()", operator),
             PyTypeError
-        )
+        );
     }
 
     fn _test_inplace_binary_operator(operator: &str, dunder: &str) {

tests/test_frompyobject.rs

@@ -296,6 +296,6 @@ fn test_err_rename() {
     assert!(f.is_err());
     assert_eq!(
         f.unwrap_err().to_string(),
-        "TypeError: Can't convert {} (dict) to Union[str, uint, int]"
+        "TypeError: 'dict' object cannot be converted to 'Union[str, uint, int]'"
     );
 }

tests/test_pyfunction.rs

@@ -119,3 +119,54 @@ fn test_raw_function() {
         .unwrap();
     assert_eq!(res, "Some(true)");
 }
+
+#[pyfunction]
+fn conversion_error(str_arg: &str, int_arg: i64, tuple_arg: (&str, f64), option_arg: Option<i64>) {
+    println!(
+        "{:?} {:?} {:?} {:?}",
+        str_arg, int_arg, tuple_arg, option_arg
+    );
+}
+
+#[test]
+fn test_conversion_error() {
+    let gil = Python::acquire_gil();
+    let py = gil.python();
+
+    let conversion_error = wrap_pyfunction!(conversion_error)(py).unwrap();
+    py_expect_exception!(
+        py,
+        conversion_error,
+        "conversion_error(None, None, None, None)",
+        PyTypeError,
+        "argument 'str_arg': 'NoneType' object cannot be converted to 'PyString'"
+    );
+    py_expect_exception!(
+        py,
+        conversion_error,
+        "conversion_error(100, None, None, None)",
+        PyTypeError,
+        "argument 'str_arg': 'int' object cannot be converted to 'PyString'"
+    );
+    py_expect_exception!(
+        py,
+        conversion_error,
+        "conversion_error('string1', 'string2', None, None)",
+        PyTypeError,
+        "argument 'int_arg': 'str' object cannot be interpreted as an integer"
+    );
+    py_expect_exception!(
+        py,
+        conversion_error,
+        "conversion_error('string1', -100, 'string2', None)",
+        PyTypeError,
+        "argument 'tuple_arg': 'str' object cannot be converted to 'PyTuple'"
+    );
+    py_expect_exception!(
+        py,
+        conversion_error,
+        "conversion_error('string1', -100, ('string2', 10.), 'string3')",
+        PyTypeError,
+        "argument 'option_arg': 'str' object cannot be interpreted as an integer"
+    );
+}

tests/test_string.rs

@@ -1,5 +1,4 @@
 use pyo3::prelude::*;
-use pyo3::py_run;
 use pyo3::wrap_pyfunction;
 
 mod common;
@@ -15,15 +14,11 @@ fn test_unicode_encode_error() {
     let py = gil.python();
 
     let take_str = wrap_pyfunction!(take_str)(py).unwrap();
-    py_run!(
+    py_expect_exception!(
         py,
         take_str,
-        r#"
-        try:
-            take_str('\ud800')
-        except UnicodeEncodeError as e:
-            error_msg = "'utf-8' codec can't encode character '\\ud800' in position 0: surrogates not allowed"
-            assert str(e) == error_msg
-        "#
+        "take_str('\\ud800')",
+        PyUnicodeEncodeError,
+        "'utf-8' codec can't encode character '\\ud800' in position 0: surrogates not allowed"
     );
 }