#[derive(FromPyObject)]: clean up generated code

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
 

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::{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,85 +238,56 @@ impl<'a> Container<'a> {
 
     fn build_tuple_struct(&self, tups: &[FieldPyO3Attributes]) -> TokenStream {
         let self_ty = &self.path;
-        let mut fields: Punctuated<TokenStream, syn::Token![,]> = Punctuated::new();
-        for (index, attrs) in tups.iter().enumerate() {
-            let error_msg = format!("failed to extract field {}.{}", quote!(#self_ty), index);
-
-            let parsed_item = match &attrs.from_py_with {
+        let field_idents: Vec<_> = (0..tups.len())
+            .into_iter()
+            .map(|i| format_ident!("arg{}", i))
+            .collect();
+        let struct_name = &quote!(#self_ty).to_string();
+        let fields = tups.iter().zip(&field_idents).enumerate().map(|(index, (attrs, ident))| {
+            match &attrs.from_py_with {
                 None => quote!(
-                    obj.get_item(#index)?.extract()
+                    _pyo3::impl_::frompyobject::extract_tuple_struct_field(#ident, #struct_name, #index)?
                 ),
                 Some(FromPyWithAttribute {
                     value: expr_path, ..
                 }) => quote! (
-                    #expr_path(obj.get_item(#index)?)
+                    _pyo3::impl_::frompyobject::extract_tuple_struct_field_with(#expr_path, #ident, #struct_name, #index)?
                 ),
-            };
-
-            let extractor = 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))
+            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),
             }
-            ::std::result::Result::Ok(#self_ty(#fields))
         )
     }
 
     fn build_struct(&self, tups: &[(&Ident, FieldPyO3Attributes)]) -> TokenStream {
         let self_ty = &self.path;
         let mut fields: Punctuated<TokenStream, syn::Token![,]> = 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));

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<T>
+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<T>,
+    obj: &'py PyAny,
+    struct_name: &str,
+    field_name: &str,
+) -> PyResult<T> {
+    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<T>
+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<T>,
+    obj: &'py PyAny,
+    struct_name: &str,
+    index: usize,
+) -> PyResult<T> {
+    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
+}

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()
     );

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),