From cfb91057affa158888e082ad24d25c42a8864212 Mon Sep 17 00:00:00 2001 From: David Hewitt <1939362+davidhewitt@users.noreply.github.com> Date: Wed, 1 Jun 2022 07:43:42 +0100 Subject: [PATCH 1/3] frompyobject: improve error message for tuple case --- pyo3-macros-backend/src/frompyobject.rs | 35 +++++++------------------ src/types/tuple.rs | 3 ++- tests/test_frompyobject.rs | 32 ++++++++++++++++++++++ 3 files changed, 44 insertions(+), 26 deletions(-) diff --git a/pyo3-macros-backend/src/frompyobject.rs b/pyo3-macros-backend/src/frompyobject.rs index 2af520797d5..604fd6b0f50 100644 --- a/pyo3-macros-backend/src/frompyobject.rs +++ b/pyo3-macros-backend/src/frompyobject.rs @@ -3,7 +3,7 @@ use crate::{ utils::get_pyo3_crate, }; use proc_macro2::TokenStream; -use quote::quote; +use quote::{quote, format_ident}; use syn::{ parenthesized, parse::{Parse, ParseStream}, @@ -244,48 +244,33 @@ impl<'a> Container<'a> { fn build_tuple_struct(&self, tups: &[FieldPyO3Attributes]) -> TokenStream { let self_ty = &self.path; - let mut fields: Punctuated = Punctuated::new(); - for (index, attrs) in tups.iter().enumerate() { + let field_idents: Vec<_> = (0..tups.len()).into_iter().map(|i| format_ident!("arg{}", i)).collect(); + let fields = tups.iter().zip(&field_idents).enumerate().map(|(index, (attrs, ident))| { let error_msg = format!("failed to extract field {}.{}", quote!(#self_ty), index); let parsed_item = match &attrs.from_py_with { None => quote!( - obj.get_item(#index)?.extract() + _pyo3::PyAny::extract(#ident) ), Some(FromPyWithAttribute { value: expr_path, .. }) => quote! ( - #expr_path(obj.get_item(#index)?) + #expr_path(#ident) ), }; - let extractor = quote!( + quote!( #parsed_item.map_err(|inner| { let py = _pyo3::PyNativeType::py(obj); let new_err = _pyo3::exceptions::PyTypeError::new_err(#error_msg); new_err.set_cause(py, ::std::option::Option::Some(inner)); new_err })? - ); - - fields.push(quote!(#extractor)); - } - let len = tups.len(); - let msg = if self.is_enum_variant { - quote!(::std::format!( - "expected tuple of length {}, but got length {}", - #len, - s.len() - )) - } else { - quote!("") - }; + ) + }); quote!( - let s = <_pyo3::types::PyTuple as _pyo3::conversion::PyTryFrom>::try_from(obj)?; - if s.len() != #len { - return ::std::result::Result::Err(_pyo3::exceptions::PyValueError::new_err(#msg)) - } - ::std::result::Result::Ok(#self_ty(#fields)) + let (#(#field_idents),*) = obj.extract()?; + ::std::result::Result::Ok(#self_ty(#(#fields),*)) ) } diff --git a/src/types/tuple.rs b/src/types/tuple.rs index c9f9ba1df0e..4baa6901141 100644 --- a/src/types/tuple.rs +++ b/src/types/tuple.rs @@ -291,9 +291,10 @@ impl<'a> IntoIterator for &'a PyTuple { } } +#[cold] fn wrong_tuple_length(t: &PyTuple, expected_length: usize) -> PyErr { let msg = format!( - "Expected tuple of length {}, but got tuple of length {}.", + "expected tuple of length {}, but got tuple of length {}", expected_length, t.len() ); diff --git a/tests/test_frompyobject.rs b/tests/test_frompyobject.rs index b93be185087..d1aea8dd76f 100644 --- a/tests/test_frompyobject.rs +++ b/tests/test_frompyobject.rs @@ -399,6 +399,21 @@ TypeError: failed to extract enum Foo ('TupleVar | StructVar | TransparentTuple - variant StructWithGetItem (StructWithGetItem): 'a' - variant StructWithGetItemArg (StructWithGetItemArg): 'foo'" ); + + let tup = PyTuple::empty(py); + let err = Foo::extract(tup.as_ref()).unwrap_err(); + assert_eq!( + err.to_string(), + "\ +TypeError: failed to extract enum Foo ('TupleVar | StructVar | TransparentTuple | TransparentStructVar | StructVarGetAttrArg | StructWithGetItem | StructWithGetItemArg') +- variant TupleVar (TupleVar): expected tuple of length 2, but got tuple of length 0 +- variant StructVar (StructVar): 'tuple' object has no attribute 'test' +- variant TransparentTuple (TransparentTuple): 'tuple' object cannot be interpreted as an integer +- variant TransparentStructVar (TransparentStructVar): failed to extract field Foo :: TransparentStructVar.a +- variant StructVarGetAttrArg (StructVarGetAttrArg): 'tuple' object has no attribute 'bla' +- variant StructWithGetItem (StructWithGetItem): tuple indices must be integers or slices, not str +- variant StructWithGetItemArg (StructWithGetItemArg): tuple indices must be integers or slices, not str" + ); }); } @@ -500,6 +515,23 @@ fn test_from_py_with_tuple_struct() { }); } +#[test] +fn test_from_py_with_tuple_struct_error() { + Python::with_gil(|py| { + let py_zap = py + .eval(r#"("whatever", [1, 2, 3], "third")"#, None, None) + .expect("failed to create tuple"); + + let f = ZapTuple::extract(py_zap); + + assert!(f.is_err()); + assert_eq!( + f.unwrap_err().to_string(), + "ValueError: expected tuple of length 2, but got tuple of length 3" + ); + }); +} + #[derive(Debug, FromPyObject, PartialEq)] pub enum ZapEnum { Zip(#[pyo3(from_py_with = "PyAny::len")] usize), From e4ec720d51145b12759363ee13dad66d329e1b54 Mon Sep 17 00:00:00 2001 From: David Hewitt <1939362+davidhewitt@users.noreply.github.com> Date: Wed, 1 Jun 2022 08:51:48 +0100 Subject: [PATCH 2/3] frompyobject: tidy up generated code --- pyo3-macros-backend/src/frompyobject.rs | 84 ++++++++----------- src/impl_/frompyobject.rs | 102 +++++++++++++++++++++++- 2 files changed, 133 insertions(+), 53 deletions(-) diff --git a/pyo3-macros-backend/src/frompyobject.rs b/pyo3-macros-backend/src/frompyobject.rs index 604fd6b0f50..ffa4dc6d2d8 100644 --- a/pyo3-macros-backend/src/frompyobject.rs +++ b/pyo3-macros-backend/src/frompyobject.rs @@ -3,7 +3,7 @@ use crate::{ utils::get_pyo3_crate, }; use proc_macro2::TokenStream; -use quote::{quote, format_ident}; +use quote::{format_ident, quote}; use syn::{ parenthesized, parse::{Parse, ParseStream}, @@ -213,18 +213,12 @@ impl<'a> Container<'a> { fn build_newtype_struct(&self, field_ident: Option<&Ident>) -> TokenStream { let self_ty = &self.path; if let Some(ident) = field_ident { - let error_msg = format!( - "failed to extract field {}.{}", - quote!(#self_ty), - quote!(#ident) - ); + let struct_name = quote!(#self_ty).to_string(); + let field_name = ident.to_string(); quote!( - ::std::result::Result::Ok(#self_ty{#ident: obj.extract().map_err(|inner| { - let py = _pyo3::PyNativeType::py(obj); - let new_err = _pyo3::exceptions::PyTypeError::new_err(#error_msg); - new_err.set_cause(py, ::std::option::Option::Some(inner)); - new_err - })?}) + ::std::result::Result::Ok(#self_ty{ + #ident: _pyo3::impl_::frompyobject::extract_struct_field(obj, #struct_name, #field_name)? + }) ) } else if !self.is_enum_variant { let error_msg = format!("failed to extract inner field of {}", quote!(#self_ty)); @@ -244,33 +238,28 @@ impl<'a> Container<'a> { fn build_tuple_struct(&self, tups: &[FieldPyO3Attributes]) -> TokenStream { let self_ty = &self.path; - let field_idents: Vec<_> = (0..tups.len()).into_iter().map(|i| format_ident!("arg{}", i)).collect(); + let field_idents: Vec<_> = (0..tups.len()) + .into_iter() + .map(|i| format_ident!("arg{}", i)) + .collect(); + let struct_name = "e!(#self_ty).to_string(); let fields = tups.iter().zip(&field_idents).enumerate().map(|(index, (attrs, ident))| { - let error_msg = format!("failed to extract field {}.{}", quote!(#self_ty), index); - - let parsed_item = match &attrs.from_py_with { + match &attrs.from_py_with { None => quote!( - _pyo3::PyAny::extract(#ident) + _pyo3::impl_::frompyobject::extract_tuple_struct_field(#ident, #struct_name, #index)? ), Some(FromPyWithAttribute { value: expr_path, .. }) => quote! ( - #expr_path(#ident) + _pyo3::impl_::frompyobject::extract_tuple_struct_field_with(#expr_path, #ident, #struct_name, #index)? ), - }; - - quote!( - #parsed_item.map_err(|inner| { - let py = _pyo3::PyNativeType::py(obj); - let new_err = _pyo3::exceptions::PyTypeError::new_err(#error_msg); - new_err.set_cause(py, ::std::option::Option::Some(inner)); - new_err - })? - ) + } }); quote!( - let (#(#field_idents),*) = obj.extract()?; - ::std::result::Result::Ok(#self_ty(#(#fields),*)) + match obj.extract() { + ::std::result::Result::Ok((#(#field_idents),*)) => ::std::result::Result::Ok(#self_ty(#(#fields),*)), + ::std::result::Result::Err(err) => ::std::result::Result::Err(err), + } ) } @@ -278,36 +267,27 @@ impl<'a> Container<'a> { let self_ty = &self.path; let mut fields: Punctuated = Punctuated::new(); for (ident, attrs) in tups { + let struct_name = quote!(#self_ty).to_string(); + let field_name = ident.to_string(); let getter = match &attrs.getter { - FieldGetter::GetAttr(Some(name)) => quote!(getattr(_pyo3::intern!(py, #name))), + FieldGetter::GetAttr(Some(name)) => { + quote!(getattr(_pyo3::intern!(obj.py(), #name))) + } FieldGetter::GetAttr(None) => { - quote!(getattr(_pyo3::intern!(py, stringify!(#ident)))) + quote!(getattr(_pyo3::intern!(obj.py(), #field_name))) } FieldGetter::GetItem(Some(key)) => quote!(get_item(#key)), - FieldGetter::GetItem(None) => quote!(get_item(stringify!(#ident))), + FieldGetter::GetItem(None) => quote!(get_item(#field_name)), }; - let conversion_error_msg = - format!("failed to extract field {}.{}", quote!(#self_ty), ident); - let get_field = quote!(obj.#getter?); let extractor = match &attrs.from_py_with { - None => quote!({ - let py = _pyo3::PyNativeType::py(obj); - #get_field.extract().map_err(|inner| { - let new_err = _pyo3::exceptions::PyTypeError::new_err(#conversion_error_msg); - new_err.set_cause(py, ::std::option::Option::Some(inner)); - new_err - })? - }), + None => { + quote!(_pyo3::impl_::frompyobject::extract_struct_field(obj.#getter?, #struct_name, #field_name)?) + } Some(FromPyWithAttribute { value: expr_path, .. - }) => quote! ( - #expr_path(#get_field).map_err(|inner| { - let py = _pyo3::PyNativeType::py(obj); - let new_err = _pyo3::exceptions::PyTypeError::new_err(#conversion_error_msg); - new_err.set_cause(py, ::std::option::Option::Some(inner)); - new_err - })? - ), + }) => { + quote! (_pyo3::impl_::frompyobject::extract_struct_field_with(#expr_path, obj.#getter?, #struct_name, #field_name)?) + } }; fields.push(quote!(#ident: #extractor)); diff --git a/src/impl_/frompyobject.rs b/src/impl_/frompyobject.rs index 8c0db79833a..3d68ba55507 100644 --- a/src/impl_/frompyobject.rs +++ b/src/impl_/frompyobject.rs @@ -1,4 +1,4 @@ -use crate::{exceptions::PyTypeError, PyErr, Python}; +use crate::{exceptions::PyTypeError, FromPyObject, PyAny, PyErr, PyResult, Python}; #[cold] pub fn failed_to_extract_enum( @@ -24,3 +24,103 @@ pub fn failed_to_extract_enum( } PyTypeError::new_err(err_msg) } + +pub fn extract_struct_field<'py, T>( + obj: &'py PyAny, + struct_name: &str, + field_name: &str, +) -> PyResult +where + T: FromPyObject<'py>, +{ + match obj.extract() { + ok @ Ok(_) => ok, + Err(err) => Err(failed_to_extract_struct_field( + obj.py(), + err, + struct_name, + field_name, + )), + } +} + +pub fn extract_struct_field_with<'py, T>( + extractor: impl FnOnce(&'py PyAny) -> PyResult, + obj: &'py PyAny, + struct_name: &str, + field_name: &str, +) -> PyResult { + match extractor(obj) { + ok @ Ok(_) => ok, + Err(err) => Err(failed_to_extract_struct_field( + obj.py(), + err, + struct_name, + field_name, + )), + } +} + +#[cold] +fn failed_to_extract_struct_field( + py: Python<'_>, + inner_err: PyErr, + struct_name: &str, + field_name: &str, +) -> PyErr { + let new_err = PyTypeError::new_err(format!( + "failed to extract field {}.{}", + struct_name, field_name + )); + new_err.set_cause(py, ::std::option::Option::Some(inner_err)); + new_err +} + +pub fn extract_tuple_struct_field<'py, T>( + obj: &'py PyAny, + struct_name: &str, + index: usize, +) -> PyResult +where + T: FromPyObject<'py>, +{ + match obj.extract() { + ok @ Ok(_) => ok, + Err(err) => Err(failed_to_extract_tuple_struct_field( + obj.py(), + err, + struct_name, + index, + )), + } +} + +pub fn extract_tuple_struct_field_with<'py, T>( + extractor: impl FnOnce(&'py PyAny) -> PyResult, + obj: &'py PyAny, + struct_name: &str, + index: usize, +) -> PyResult { + match extractor(obj) { + ok @ Ok(_) => ok, + Err(err) => Err(failed_to_extract_tuple_struct_field( + obj.py(), + err, + struct_name, + index, + )), + } +} + +#[cold] +fn failed_to_extract_tuple_struct_field( + py: Python<'_>, + inner_err: PyErr, + struct_name: &str, + index: usize, +) -> PyErr { + let new_err = + PyTypeError::new_err(format!("failed to extract field {}.{}", struct_name, index)); + new_err.set_cause(py, ::std::option::Option::Some(inner_err)); + new_err +} From 4090f6b1ae023e1ed0bc48e40b2c09b794aabb77 Mon Sep 17 00:00:00 2001 From: David Hewitt <1939362+davidhewitt@users.noreply.github.com> Date: Wed, 1 Jun 2022 09:20:17 +0100 Subject: [PATCH 3/3] CHANGELOG: document #2414 --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 402e4d42e48..5f8c5b0463f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -34,6 +34,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Fixed incorrectly disabled FFI definition `PyThreadState_DeleteCurrent`. [#2357](https://github.com/PyO3/pyo3/pull/2357) - Correct FFI definition `PyEval_EvalCodeEx` to take `*const *mut PyObject` array arguments instead of `*mut *mut PyObject` (this was changed in CPython 3.6). [#2368](https://github.com/PyO3/pyo3/pull/2368) - Fix "raw-ident" structs (e.g. `#[pyclass] struct r#RawName`) incorrectly having `r#` at the start of the class name created in Python. [#2395](https://github.com/PyO3/pyo3/pull/2395) +- Fix case where `ValueError` without message could be raised by the `#[derive(FromPyObject)]` generated implementation for a tuple struct. [#2414](https://github.com/PyO3/pyo3/pull/2414) +- Fix compile failure when using `#[pyo3(from_py_with = "pouf")]` in on a field in a `#[derive(FromPyObject)]` struct. [#2414](https://github.com/PyO3/pyo3/pull/2414) ## [0.16.5] - 2022-05-15