From 007bfb7ab061cfd3285006a9b059bf967e128c9f Mon Sep 17 00:00:00 2001 From: Askaholic Date: Tue, 13 Oct 2020 14:02:14 -0800 Subject: [PATCH 1/4] Refactor py_expect_exception to also verify error string representation --- tests/common.rs | 15 ++++++++++++++- tests/test_arithmetics.rs | 2 +- 2 files changed, 15 insertions(+), 2 deletions(-) diff --git a/tests/common.rs b/tests/common.rs index 4cd3c084..c110f5d4 100644 --- a/tests/common.rs +++ b/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::()) { 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 }}; } diff --git a/tests/test_arithmetics.rs b/tests/test_arithmetics.rs index f0c38724..5fd81952 100644 --- a/tests/test_arithmetics.rs +++ b/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) { From 19889bc6b9a9c79d03b6b4a11293faf4aa19b73c Mon Sep 17 00:00:00 2001 From: Askaholic Date: Tue, 13 Oct 2020 13:38:21 -0800 Subject: [PATCH 2/4] Add argument name to TypeError messages caused during argument conversion --- pyo3-derive-backend/src/pymethod.rs | 10 ++++-- src/derive_utils.rs | 15 ++++++++- tests/test_pyfunction.rs | 51 +++++++++++++++++++++++++++++ tests/test_string.rs | 13 +++----- 4 files changed, 76 insertions(+), 13 deletions(-) diff --git a/pyo3-derive-backend/src/pymethod.rs b/pyo3-derive-backend/src/pymethod.rs index 6afb66ef..a3d2f4ad 100644 --- a/pyo3-derive-backend/src/pymethod.rs +++ b/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, }; } diff --git a/src/derive_utils.rs b/src/derive_utils.rs index 52e22fac..cd0f5bde 100644 --- a/src/derive_utils.rs +++ b/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::() { + 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); diff --git a/tests/test_pyfunction.rs b/tests/test_pyfunction.rs index 6f6bf44c..affb768a 100644 --- a/tests/test_pyfunction.rs +++ b/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) { + 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': Can't convert None to PyString" + ); + py_expect_exception!( + py, + conversion_error, + "conversion_error(100, None, None, None)", + PyTypeError, + "argument 'str_arg': Can't convert 100 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': Can't convert 'string2' 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" + ); +} diff --git a/tests/test_string.rs b/tests/test_string.rs index 38d375b5..cde88592 100644 --- a/tests/test_string.rs +++ b/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" ); } From 6724783395ddc3e85d2a223be7257ab108df4ac3 Mon Sep 17 00:00:00 2001 From: Askaholic Date: Tue, 13 Oct 2020 13:40:03 -0800 Subject: [PATCH 3/4] Change wording of PyDowncastError display implementation Displays type(obj) instead of repr(obj) and uses `cannot` instead of `can't` to be more consistent with existing python error messages. See discussion at #1212. --- guide/src/conversions/traits.md | 3 +-- pyo3-derive-backend/src/from_pyobject.rs | 6 +----- src/err/mod.rs | 7 ++----- tests/test_frompyobject.rs | 2 +- tests/test_pyfunction.rs | 6 +++--- 5 files changed, 8 insertions(+), 16 deletions(-) diff --git a/guide/src/conversions/traits.md b/guide/src/conversions/traits.md index 7d997538..d09a684f 100644 --- a/guide/src/conversions/traits.md +++ b/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 to Union[str, int]"`, where `` is replaced by the type name and -`repr()` of the input object. +`"'' cannot be converted to 'Union[str, int]'"`. #### `#[derive(FromPyObject)]` Container Attributes - `pyo3(transparent)` diff --git a/pyo3-derive-backend/src/from_pyobject.rs b/pyo3-derive-backend/src/from_pyobject.rs index 8d8dc956..5ea0754d 100644 --- a/pyo3-derive-backend/src/from_pyobject.rs +++ b/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)) ) } diff --git a/src/err/mod.rs b/src/err/mod.rs index 294cbe9b..461a1ad4 100644 --- a/src/err/mod.rs +++ b/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 ) } diff --git a/tests/test_frompyobject.rs b/tests/test_frompyobject.rs index 98f2d2b9..24a7322b 100644 --- a/tests/test_frompyobject.rs +++ b/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]'" ); } diff --git a/tests/test_pyfunction.rs b/tests/test_pyfunction.rs index affb768a..1e088955 100644 --- a/tests/test_pyfunction.rs +++ b/tests/test_pyfunction.rs @@ -139,14 +139,14 @@ fn test_conversion_error() { conversion_error, "conversion_error(None, None, None, None)", PyTypeError, - "argument 'str_arg': Can't convert None to PyString" + "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': Can't convert 100 to PyString" + "argument 'str_arg': 'int' object cannot be converted to 'PyString'" ); py_expect_exception!( py, @@ -160,7 +160,7 @@ fn test_conversion_error() { conversion_error, "conversion_error('string1', -100, 'string2', None)", PyTypeError, - "argument 'tuple_arg': Can't convert 'string2' to PyTuple" + "argument 'tuple_arg': 'str' object cannot be converted to 'PyTuple'" ); py_expect_exception!( py, From 1d7034478c91951bd1ae35178c937b6397843d97 Mon Sep 17 00:00:00 2001 From: Askaholic Date: Thu, 15 Oct 2020 12:02:58 -0800 Subject: [PATCH 4/4] Add entries to changelog --- CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index b6175856..98fceb83 100644 --- a/CHANGELOG.md +++ b/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)