Skip to content

Commit

Permalink
Merge pull request #760 from davidhewitt/property-fixes
Browse files Browse the repository at this point in the history 
Improvements to property macros
  • Loading branch information
@kngwyu
kngwyu authored Feb 10, 2020
2 parents 1d33ac4 + 7b3de17 commit 3c02de2
Show file tree
Hide file tree
Showing 10 changed files with 207 additions and 142 deletions.
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,12 +23,14 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
* Implemented `IntoIterator` for `PySet` and `PyFrozenSet`. [#716](https://github.com/PyO3/pyo3/pull/716)
* `FromPyObject` is now automatically implemented for `T: Clone` pyclasses. [#730](https://github.com/PyO3/pyo3/pull/730)
* `#[pyo3(get)]` and `#[pyo3(set)]` will now use the Rust doc-comment from the field for the Python property. [#755](https://github.com/PyO3/pyo3/pull/755)
* `#[setter]` functions may now take an argument of `Pyo3::Python`. [#760](https://github.com/PyO3/pyo3/pull/760)

### Fixed

* Fixed unsoundness of subclassing. [#683](https://github.com/PyO3/pyo3/pull/683).
* Clear error indicator when the exception is handled on the Rust side. [#719](https://github.com/PyO3/pyo3/pull/719)
* Usage of raw identifiers with `#[pyo3(set)]`. [#745](https://github.com/PyO3/pyo3/pull/745)
* Usage of `PyObject` with `#[pyo3(get)]`. [#760](https://github.com/PyO3/pyo3/pull/760)

### Removed

Expand Down
93 changes: 15 additions & 78 deletions pyo3-derive-backend/src/pyclass.rs
View file
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
// Copyright (c) 2017-present PyO3 Project and Contributors

use crate::method::{FnArg, FnSpec, FnType};
use crate::pymethod::{impl_py_getter_def, impl_py_setter_def, impl_wrap_getter, impl_wrap_setter};
use crate::method::FnType;
use crate::pymethod::{
impl_py_getter_def, impl_py_setter_def, impl_wrap_getter, impl_wrap_setter, PropertyType,
};
use crate::utils;
use proc_macro2::{Span, TokenStream};
use quote::quote;
Expand Down Expand Up @@ -421,90 +423,26 @@ fn impl_descriptors(
cls: &syn::Type,
descriptors: Vec<(syn::Field, Vec<FnType>)>,
) -> syn::Result<TokenStream> {
let methods: Vec<TokenStream> = descriptors
.iter()
.flat_map(|&(ref field, ref fns)| {
fns.iter()
.map(|desc| {
let name = field.ident.as_ref().unwrap();
let field_ty = &field.ty;
match *desc {
FnType::Getter => {
quote! {
impl #cls {
fn #name(&self) -> pyo3::PyResult<#field_ty> {
Ok(self.#name.clone())
}
}
}
}
FnType::Setter => {
let setter_name =
syn::Ident::new(&format!("set_{}", name.unraw()), Span::call_site());
quote! {
impl #cls {
fn #setter_name(&mut self, value: #field_ty) -> pyo3::PyResult<()> {
self.#name = value;
Ok(())
}
}
}
}
_ => unreachable!(),
}
})
.collect::<Vec<TokenStream>>()
})
.collect();

let py_methods: Vec<TokenStream> = descriptors
.iter()
.flat_map(|&(ref field, ref fns)| {
fns.iter()
.map(|desc| {
let name = field.ident.as_ref().unwrap();

let name = field.ident.as_ref().unwrap().unraw();
let doc = utils::get_doc(&field.attrs, None, true)
.unwrap_or_else(|_| syn::LitStr::new(&name.to_string(), name.span()));

let field_ty = &field.ty;
match *desc {
FnType::Getter => {
let spec = FnSpec {
tp: FnType::Getter,
name: &name,
python_name: name.unraw(),
attrs: Vec::new(),
args: Vec::new(),
output: parse_quote!(PyResult<#field_ty>),
doc,
};
Ok(impl_py_getter_def(&spec, &impl_wrap_getter(&cls, &spec)?))
}
FnType::Setter => {
let setter_name = syn::Ident::new(
&format!("set_{}", name.unraw()),
Span::call_site(),
);
let spec = FnSpec {
tp: FnType::Setter,
name: &setter_name,
python_name: name.unraw(),
attrs: Vec::new(),
args: vec![FnArg {
name: &name,
mutability: &None,
by_ref: &None,
ty: field_ty,
optional: None,
py: true,
reference: false,
}],
output: parse_quote!(PyResult<()>),
doc,
};
Ok(impl_py_setter_def(&spec, &impl_wrap_setter(&cls, &spec)?))
}
FnType::Getter => Ok(impl_py_getter_def(
&name,
&doc,
&impl_wrap_getter(&cls, PropertyType::Descriptor(&field))?,
)),
FnType::Setter => Ok(impl_py_setter_def(
&name,
&doc,
&impl_wrap_setter(&cls, PropertyType::Descriptor(&field))?,
)),
_ => unreachable!(),
}
})
Expand All @@ -513,7 +451,6 @@ fn impl_descriptors(
.collect::<syn::Result<_>>()?;

Ok(quote! {
#(#methods)*

pyo3::inventory::submit! {
#![crate = pyo3] {
Expand Down
145 changes: 101 additions & 44 deletions pyo3-derive-backend/src/pymethod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,12 @@ use crate::method::{FnArg, FnSpec, FnType};
use crate::utils;
use proc_macro2::{Span, TokenStream};
use quote::quote;
use syn::ext::IdentExt;

pub enum PropertyType<'a> {
Descriptor(&'a syn::Field),
Function(&'a FnSpec<'a>),
}

pub fn gen_py_method(
cls: &syn::Type,
Expand All @@ -21,8 +27,16 @@ pub fn gen_py_method(
FnType::FnCall => impl_py_method_def_call(&spec, &impl_wrap(cls, &spec, false)),
FnType::FnClass => impl_py_method_def_class(&spec, &impl_wrap_class(cls, &spec)),
FnType::FnStatic => impl_py_method_def_static(&spec, &impl_wrap_static(cls, &spec)),
FnType::Getter => impl_py_getter_def(&spec, &impl_wrap_getter(cls, &spec)?),
FnType::Setter => impl_py_setter_def(&spec, &impl_wrap_setter(cls, &spec)?),
FnType::Getter => impl_py_getter_def(
&spec.python_name,
&spec.doc,
&impl_wrap_getter(cls, PropertyType::Function(&spec))?,
),
FnType::Setter => impl_py_setter_def(
&spec.python_name,
&spec.doc,
&impl_wrap_setter(cls, PropertyType::Function(&spec))?,
),
})
}

Expand Down Expand Up @@ -244,28 +258,44 @@ pub fn impl_wrap_static(cls: &syn::Type, spec: &FnSpec<'_>) -> TokenStream {
}
}

/// Generate functiona wrapper (PyCFunction, PyCFunctionWithKeywords)
pub(crate) fn impl_wrap_getter(cls: &syn::Type, spec: &FnSpec) -> syn::Result<TokenStream> {
let takes_py = match &*spec.args {
[] => false,
[arg] if utils::if_type_is_python(arg.ty) => true,
_ => {
return Err(syn::Error::new_spanned(
spec.args[0].ty,
"Getter function can only have one argument of type pyo3::Python!",
));
}
};
fn impl_call_getter(spec: &FnSpec) -> syn::Result<TokenStream> {
let (py_arg, args) = split_off_python_arg(&spec.args);
if !args.is_empty() {
return Err(syn::Error::new_spanned(
args[0].ty,
"Getter function can only have one argument of type pyo3::Python",
));
}

let name = &spec.name;
let python_name = &spec.python_name;

let fncall = if takes_py {
let fncall = if py_arg.is_some() {
quote! { _slf.#name(_py) }
} else {
quote! { _slf.#name() }
};

Ok(fncall)
}

/// Generate a function wrapper called `__wrap` for a property getter
pub(crate) fn impl_wrap_getter(
cls: &syn::Type,
property_type: PropertyType,
) -> syn::Result<TokenStream> {
let (python_name, getter_impl) = match property_type {
PropertyType::Descriptor(field) => {
let name = field.ident.as_ref().unwrap();
(
name.unraw(),
quote!({
use pyo3::derive_utils::GetPropertyValue;
(&_slf.#name).get_property_value(_py)
}),
)
}
PropertyType::Function(spec) => (spec.python_name.clone(), impl_call_getter(&spec)?),
};

Ok(quote! {
unsafe extern "C" fn __wrap(
_slf: *mut pyo3::ffi::PyObject, _: *mut ::std::os::raw::c_void) -> *mut pyo3::ffi::PyObject
Expand All @@ -276,7 +306,7 @@ pub(crate) fn impl_wrap_getter(cls: &syn::Type, spec: &FnSpec) -> syn::Result<To
let _pool = pyo3::GILPool::new(_py);
let _slf = _py.mut_from_borrowed_ptr::<#cls>(_slf);

let result = pyo3::derive_utils::IntoPyResult::into_py_result(#fncall);
let result = pyo3::derive_utils::IntoPyResult::into_py_result(#getter_impl);

match result {
Ok(val) => {
Expand All @@ -291,25 +321,42 @@ pub(crate) fn impl_wrap_getter(cls: &syn::Type, spec: &FnSpec) -> syn::Result<To
})
}

/// Generate functiona wrapper (PyCFunction, PyCFunctionWithKeywords)
pub(crate) fn impl_wrap_setter(cls: &syn::Type, spec: &FnSpec<'_>) -> syn::Result<TokenStream> {
fn impl_call_setter(spec: &FnSpec) -> syn::Result<TokenStream> {
let (py_arg, args) = split_off_python_arg(&spec.args);

if args.is_empty() {
return Err(syn::Error::new_spanned(
&spec.name,
"Setter function expected to have one argument",
));
} else if args.len() > 1 {
return Err(syn::Error::new_spanned(
&args[1].ty,
"Setter function can have at most two arguments: one of pyo3::Python, and one other",
));
}

let name = &spec.name;
let python_name = &spec.python_name;
let fncall = if py_arg.is_some() {
quote!(pyo3::derive_utils::IntoPyResult::into_py_result(_slf.#name(_py, _val)))
} else {
quote!(pyo3::derive_utils::IntoPyResult::into_py_result(_slf.#name(_val)))
};

let val_ty = match &*spec.args {
[] => {
return Err(syn::Error::new_spanned(
&spec.name,
"Not enough arguments for setter {}::{}",
))
}
[arg] => &arg.ty,
_ => {
return Err(syn::Error::new_spanned(
spec.args[0].ty,
"Setter function must have exactly one argument",
))
Ok(fncall)
}

/// Generate a function wrapper called `__wrap` for a property setter
pub(crate) fn impl_wrap_setter(
cls: &syn::Type,
property_type: PropertyType,
) -> syn::Result<TokenStream> {
let (python_name, setter_impl) = match property_type {
PropertyType::Descriptor(field) => {
let name = field.ident.as_ref().unwrap();
(name.unraw(), quote!({ _slf.#name = _val; Ok(()) }))
}
PropertyType::Function(spec) => (spec.python_name.clone(), impl_call_setter(&spec)?),
};

Ok(quote! {
Expand All @@ -324,9 +371,9 @@ pub(crate) fn impl_wrap_setter(cls: &syn::Type, spec: &FnSpec<'_>) -> syn::Resul
let _slf = _py.mut_from_borrowed_ptr::<#cls>(_slf);
let _value = _py.from_borrowed_ptr(_value);

let _result = match <#val_ty as pyo3::FromPyObject>::extract(_value) {
let _result = match pyo3::FromPyObject::extract(_value) {
Ok(_val) => {
pyo3::derive_utils::IntoPyResult::into_py_result(_slf.#name(_val))
#setter_impl
}
Err(e) => Err(e)
};
Expand Down Expand Up @@ -617,10 +664,11 @@ pub fn impl_py_method_def_call(spec: &FnSpec, wrapper: &TokenStream) -> TokenStr
}
}

pub(crate) fn impl_py_setter_def(spec: &FnSpec, wrapper: &TokenStream) -> TokenStream {
let python_name = &&spec.python_name;
let doc = &spec.doc;

pub(crate) fn impl_py_setter_def(
python_name: &syn::Ident,
doc: &syn::LitStr,
wrapper: &TokenStream,
) -> TokenStream {
quote! {
pyo3::class::PyMethodDefType::Setter({
#wrapper
Expand All @@ -634,10 +682,11 @@ pub(crate) fn impl_py_setter_def(spec: &FnSpec, wrapper: &TokenStream) -> TokenS
}
}

pub(crate) fn impl_py_getter_def(spec: &FnSpec, wrapper: &TokenStream) -> TokenStream {
let python_name = &&spec.python_name;
let doc = &spec.doc;

pub(crate) fn impl_py_getter_def(
python_name: &syn::Ident,
doc: &syn::LitStr,
wrapper: &TokenStream,
) -> TokenStream {
quote! {
pyo3::class::PyMethodDefType::Getter({
#wrapper
Expand All @@ -650,3 +699,11 @@ pub(crate) fn impl_py_getter_def(spec: &FnSpec, wrapper: &TokenStream) -> TokenS
})
}
}

/// Split an argument of pyo3::Python from the front of the arg list, if present
fn split_off_python_arg<'a>(args: &'a [FnArg<'a>]) -> (Option<&FnArg>, &[FnArg]) {
match args {
[py, rest @ ..] if utils::if_type_is_python(&py.ty) => (Some(py), rest),
rest => (None, rest),
}
}
19 changes: 19 additions & 0 deletions src/derive_utils.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -199,3 +199,22 @@ impl<T: PyClass, I: Into<PyClassInitializer<T>>> IntoPyNewResult<T, I> for PyRes
self
}
}

pub trait GetPropertyValue {
fn get_property_value(&self, py: Python) -> PyObject;
}

impl<T> GetPropertyValue for &T
where
T: IntoPy<PyObject> + Clone,
{
fn get_property_value(&self, py: Python) -> PyObject {
(*self).clone().into_py(py)
}
}

impl GetPropertyValue for PyObject {
fn get_property_value(&self, py: Python) -> PyObject {
self.clone_ref(py)
}
}
Loading

0 comments on commit 3c02de2

Please sign in to comment.