Skip to content

Commit

Permalink
Merge pull request #2414 from davidhewitt/frompyobject-fixes
Browse files Browse the repository at this point in the history 
`#[derive(FromPyObject)]`: clean up generated code
  • Loading branch information
@davidhewitt
davidhewitt committed Jun 2, 2022
2 parents 1d50ffd + 4090f6b commit a388901
Show file tree
Hide file tree
Showing 5 changed files with 170 additions and 70 deletions.
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Expand Up @@ -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

Expand Down
101 changes: 33 additions & 68 deletions pyo3-macros-backend/src/frompyobject.rs
View file
Expand Up @@ -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},
Expand Down Expand Up @@ -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));
Expand All @@ -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));
Expand Down
102 changes: 101 additions & 1 deletion src/impl_/frompyobject.rs
View file
@@ -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(
Expand All @@ -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
}
3 changes: 2 additions & 1 deletion src/types/tuple.rs
View file
Expand Up @@ -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()
);
Expand Down
32 changes: 32 additions & 0 deletions tests/test_frompyobject.rs
View file
Expand Up @@ -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"
);
});
}

Expand Down Expand Up @@ -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),
Expand Down

0 comments on commit a388901

Please sign in to comment.