From 7c56a03d6475ecf2873d8c020b9f36bb2fb86626 Mon Sep 17 00:00:00 2001 From: David Hewitt <1939362+davidhewitt@users.noreply.github.com> Date: Wed, 8 Jun 2022 06:41:31 +0100 Subject: [PATCH] frompyobject: fix `from_py_with` ignored for transparent structs --- CHANGELOG.md | 1 + pyo3-macros-backend/src/frompyobject.rs | 192 +++++++++++++++--------- tests/test_frompyobject.rs | 21 ++- tests/ui/invalid_frompy_derive.rs | 10 ++ tests/ui/invalid_frompy_derive.stderr | 12 ++ 5 files changed, 162 insertions(+), 74 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ccf5ec61..4ec080b6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -54,6 +54,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Fix FFI definitions `_PyDateTime_BaseTime` and `_PyDateTime_BaseDateTime` incorrectly having `fold` member. [#2432](https://github.com/PyO3/pyo3/pull/2432) - Fix FFI definitions `PyTypeObject`. `PyHeapTypeObject`, and `PyCFunctionObject` having incorrect members on PyPy 3.9. [#2428](https://github.com/PyO3/pyo3/pull/2428) - Fix FFI definition `PyGetSetDef` to have `*const c_char` for `doc` member (not `*mut c_char`). [#2439](https://github.com/PyO3/pyo3/pull/2439) +- Fix `#[pyo3(from_py_with = "...")]` being ignored for 1-element tuple structs and transparent structs. [#2440](https://github.com/PyO3/pyo3/pull/2440) ## [0.16.5] - 2022-05-15 diff --git a/pyo3-macros-backend/src/frompyobject.rs b/pyo3-macros-backend/src/frompyobject.rs index 9e5036a3..9693805f 100644 --- a/pyo3-macros-backend/src/frompyobject.rs +++ b/pyo3-macros-backend/src/frompyobject.rs @@ -14,7 +14,6 @@ use syn::{ }; /// Describes derivation input of an enum. -#[derive(Debug)] struct Enum<'a> { enum_ident: &'a Ident, variants: Vec>, @@ -86,34 +85,42 @@ impl<'a> Enum<'a> { } } +struct NamedStructField<'a> { + ident: &'a syn::Ident, + getter: Option, + from_py_with: Option, +} + +struct TupleStructField { + from_py_with: Option, +} + /// Container Style /// /// Covers Structs, Tuplestructs and corresponding Newtypes. -#[derive(Debug)] enum ContainerType<'a> { /// Struct Container, e.g. `struct Foo { a: String }` /// /// Variant contains the list of field identifiers and the corresponding extraction call. - Struct(Vec<(&'a Ident, FieldPyO3Attributes)>), + Struct(Vec>), /// Newtype struct container, e.g. `#[transparent] struct Foo { a: String }` /// /// The field specified by the identifier is extracted directly from the object. - StructNewtype(&'a Ident), + StructNewtype(&'a syn::Ident, Option), /// Tuple struct, e.g. `struct Foo(String)`. /// /// Variant contains a list of conversion methods for each of the fields that are directly /// extracted from the tuple. - Tuple(Vec), + Tuple(Vec), /// Tuple newtype, e.g. `#[transparent] struct Foo(String)` /// /// The wrapped field is directly extracted from the object. - TupleNewtype, + TupleNewtype(Option), } /// Data container /// /// Either describes a struct or an enum variant. -#[derive(Debug)] struct Container<'a> { path: syn::Path, ty: ContainerType<'a>, @@ -125,55 +132,72 @@ impl<'a> Container<'a> { /// /// Fails if the variant has no fields or incompatible attributes. fn new(fields: &'a Fields, path: syn::Path, options: ContainerOptions) -> Result { - ensure_spanned!( - !fields.is_empty(), - fields.span() => "cannot derive FromPyObject for empty structs and variants" - ); - if options.transparent { - ensure_spanned!( - fields.len() == 1, - fields.span() => "transparent structs and variants can only have 1 field" - ); - } - let style = match (fields, options.transparent) { - (Fields::Unnamed(_), true) => ContainerType::TupleNewtype, - (Fields::Unnamed(unnamed), false) => match unnamed.unnamed.len() { - 1 => ContainerType::TupleNewtype, - _ => { - let fields = unnamed - .unnamed - .iter() - .map(|field| FieldPyO3Attributes::from_attrs(&field.attrs)) - .collect::>>()?; + let style = match fields { + Fields::Unnamed(unnamed) if !unnamed.unnamed.is_empty() => { + let mut tuple_fields = unnamed + .unnamed + .iter() + .map(|field| { + let attrs = FieldPyO3Attributes::from_attrs(&field.attrs)?; + ensure_spanned!( + attrs.getter.is_none(), + field.span() => "`getter` is not permitted on tuple struct elements." + ); + Ok(TupleStructField { + from_py_with: attrs.from_py_with, + }) + }) + .collect::>>()?; - ContainerType::Tuple(fields) + if tuple_fields.len() == 1 { + // Always treat a 1-length tuple struct as "transparent", even without the + // explicit annotation. + let field = tuple_fields.pop().unwrap(); + ContainerType::TupleNewtype(field.from_py_with) + } else if options.transparent { + bail_spanned!( + fields.span() => "transparent structs and variants can only have 1 field" + ); + } else { + ContainerType::Tuple(tuple_fields) } - }, - (Fields::Named(named), true) => { - let field = named + } + Fields::Named(named) if !named.named.is_empty() => { + let mut struct_fields = named .named .iter() - .next() - .expect("Check for len 1 is done above"); - let ident = field - .ident - .as_ref() - .expect("Named fields should have identifiers"); - ContainerType::StructNewtype(ident) - } - (Fields::Named(named), false) => { - let mut fields = Vec::new(); - for field in named.named.iter() { - let ident = field - .ident - .as_ref() - .expect("Named fields should have identifiers"); - let attrs = FieldPyO3Attributes::from_attrs(&field.attrs)?; - fields.push((ident, attrs)) + .map(|field| { + let ident = field + .ident + .as_ref() + .expect("Named fields should have identifiers"); + let attrs = FieldPyO3Attributes::from_attrs(&field.attrs)?; + + Ok(NamedStructField { + ident, + getter: attrs.getter, + from_py_with: attrs.from_py_with, + }) + }) + .collect::>>()?; + if options.transparent { + ensure_spanned!( + struct_fields.len() == 1, + fields.span() => "transparent structs and variants can only have 1 field" + ); + let field = struct_fields.pop().unwrap(); + ensure_spanned!( + field.getter.is_none(), + field.ident.span() => "`transparent` structs may not have a `getter` for the inner field" + ); + ContainerType::StructNewtype(field.ident, field.from_py_with) + } else { + ContainerType::Struct(struct_fields) } - ContainerType::Struct(fields) } - (Fields::Unit, _) => unreachable!(), // covered by length check above + _ => bail_spanned!( + fields.span() => "cannot derive FromPyObject for empty structs and variants" + ), }; let err_name = options.annotation.map_or_else( || path.segments.last().unwrap().ident.to_string(), @@ -202,39 +226,63 @@ impl<'a> Container<'a> { /// Build derivation body for a struct. fn build(&self) -> TokenStream { match &self.ty { - ContainerType::StructNewtype(ident) => self.build_newtype_struct(Some(ident)), - ContainerType::TupleNewtype => self.build_newtype_struct(None), + ContainerType::StructNewtype(ident, from_py_with) => { + self.build_newtype_struct(Some(ident), from_py_with) + } + ContainerType::TupleNewtype(from_py_with) => { + self.build_newtype_struct(None, from_py_with) + } ContainerType::Tuple(tups) => self.build_tuple_struct(tups), ContainerType::Struct(tups) => self.build_struct(tups), } } - fn build_newtype_struct(&self, field_ident: Option<&Ident>) -> TokenStream { + fn build_newtype_struct( + &self, + field_ident: Option<&Ident>, + from_py_with: &Option, + ) -> TokenStream { let self_ty = &self.path; let struct_name = self.name(); if let Some(ident) = field_ident { let field_name = ident.to_string(); - quote!( - ::std::result::Result::Ok(#self_ty{ - #ident: _pyo3::impl_::frompyobject::extract_struct_field(obj, #struct_name, #field_name)? - }) - ) + match from_py_with { + None => quote! { + Ok(#self_ty { + #ident: _pyo3::impl_::frompyobject::extract_struct_field(obj, #struct_name, #field_name)? + }) + }, + Some(FromPyWithAttribute { + value: expr_path, .. + }) => quote! { + Ok(#self_ty { + #ident: _pyo3::impl_::frompyobject::extract_struct_field_with(#expr_path, obj, #struct_name, #field_name)? + }) + }, + } } else { - quote!( - _pyo3::impl_::frompyobject::extract_tuple_struct_field(obj, #struct_name, 0).map(#self_ty) - ) + match from_py_with { + None => quote!( + _pyo3::impl_::frompyobject::extract_tuple_struct_field(obj, #struct_name, 0).map(#self_ty) + ), + Some(FromPyWithAttribute { + value: expr_path, .. + }) => quote! ( + _pyo3::impl_::frompyobject::extract_tuple_struct_field_with(#expr_path, obj, #struct_name, 0).map(#self_ty) + ), + } } } - fn build_tuple_struct(&self, tups: &[FieldPyO3Attributes]) -> TokenStream { + fn build_tuple_struct(&self, struct_fields: &[TupleStructField]) -> TokenStream { let self_ty = &self.path; let struct_name = &self.name(); - let field_idents: Vec<_> = (0..tups.len()) + let field_idents: Vec<_> = (0..struct_fields.len()) .into_iter() .map(|i| format_ident!("arg{}", i)) .collect(); - let fields = tups.iter().zip(&field_idents).enumerate().map(|(index, (attrs, ident))| { - match &attrs.from_py_with { + let fields = struct_fields.iter().zip(&field_idents).enumerate().map(|(index, (field, ident))| { + match &field.from_py_with { None => quote!( _pyo3::impl_::frompyobject::extract_tuple_struct_field(#ident, #struct_name, #index)? ), @@ -253,13 +301,14 @@ impl<'a> Container<'a> { ) } - fn build_struct(&self, tups: &[(&Ident, FieldPyO3Attributes)]) -> TokenStream { + fn build_struct(&self, struct_fields: &[NamedStructField<'_>]) -> TokenStream { let self_ty = &self.path; let struct_name = &self.name(); let mut fields: Punctuated = Punctuated::new(); - for (ident, attrs) in tups { + for field in struct_fields { + let ident = &field.ident; let field_name = ident.to_string(); - let getter = match &attrs.getter { + let getter = match field.getter.as_ref().unwrap_or(&FieldGetter::GetAttr(None)) { FieldGetter::GetAttr(Some(name)) => { quote!(getattr(_pyo3::intern!(obj.py(), #name))) } @@ -269,7 +318,7 @@ impl<'a> Container<'a> { FieldGetter::GetItem(Some(key)) => quote!(get_item(#key)), FieldGetter::GetItem(None) => quote!(get_item(#field_name)), }; - let extractor = match &attrs.from_py_with { + let extractor = match &field.from_py_with { None => { quote!(_pyo3::impl_::frompyobject::extract_struct_field(obj.#getter?, #struct_name, #field_name)?) } @@ -297,7 +346,6 @@ struct ContainerOptions { } /// Attributes for deriving FromPyObject scoped on containers. -#[derive(Debug)] enum ContainerPyO3Attribute { /// Treat the Container as a Wrapper, directly extract its fields from the input object. Transparent(attributes::kw::transparent), @@ -365,7 +413,7 @@ impl ContainerOptions { /// Attributes for deriving FromPyObject scoped on fields. #[derive(Clone, Debug)] struct FieldPyO3Attributes { - getter: FieldGetter, + getter: Option, from_py_with: Option, } @@ -457,7 +505,7 @@ impl FieldPyO3Attributes { } Ok(FieldPyO3Attributes { - getter: getter.unwrap_or(FieldGetter::GetAttr(None)), + getter, from_py_with, }) } diff --git a/tests/test_frompyobject.rs b/tests/test_frompyobject.rs index 454159d2..f429ce26 100644 --- a/tests/test_frompyobject.rs +++ b/tests/test_frompyobject.rs @@ -2,7 +2,7 @@ use pyo3::exceptions::PyValueError; use pyo3::prelude::*; -use pyo3::types::{PyDict, PyString, PyTuple}; +use pyo3::types::{PyDict, PyList, PyString, PyTuple}; #[macro_use] mod common; @@ -546,8 +546,25 @@ fn test_from_py_with_enum() { .expect("failed to create tuple"); let zap = ZapEnum::extract(py_zap).unwrap(); - let expected_zap = ZapEnum::Zap(String::from("whatever"), 3usize); + let expected_zap = ZapEnum::Zip(2); assert_eq!(zap, expected_zap); }); } + +#[derive(Debug, FromPyObject, PartialEq)] +#[pyo3(transparent)] +pub struct TransparentFromPyWith { + #[pyo3(from_py_with = "PyAny::len")] + len: usize, +} + +#[test] +fn test_transparent_from_py_with() { + Python::with_gil(|py| { + let result = TransparentFromPyWith::extract(PyList::new(py, &[1, 2, 3])).unwrap(); + let expected = TransparentFromPyWith { len: 3 }; + + assert_eq!(result, expected); + }); +} diff --git a/tests/ui/invalid_frompy_derive.rs b/tests/ui/invalid_frompy_derive.rs index bdbc9c4d..7134e40b 100644 --- a/tests/ui/invalid_frompy_derive.rs +++ b/tests/ui/invalid_frompy_derive.rs @@ -165,4 +165,14 @@ struct InvalidFromPyWithLiteral { field: String, } +#[derive(FromPyObject)] +struct InvalidTupleGetter(#[pyo3(item("foo"))] String); + +#[derive(FromPyObject)] +#[pyo3(transparent)] +struct InvalidTransparentWithGetter { + #[pyo3(item("foo"))] + field: String, +} + fn main() {} diff --git a/tests/ui/invalid_frompy_derive.stderr b/tests/ui/invalid_frompy_derive.stderr index 04280d73..5f11a8f8 100644 --- a/tests/ui/invalid_frompy_derive.stderr +++ b/tests/ui/invalid_frompy_derive.stderr @@ -181,3 +181,15 @@ error: expected string literal | 164 | #[pyo3(from_py_with = func)] | ^^^^ + +error: `getter` is not permitted on tuple struct elements. + --> tests/ui/invalid_frompy_derive.rs:169:27 + | +169 | struct InvalidTupleGetter(#[pyo3(item("foo"))] String); + | ^ + +error: `transparent` structs may not have a `getter` for the inner field + --> tests/ui/invalid_frompy_derive.rs:175:5 + | +175 | field: String, + | ^^^^^