Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improvements to property macros #760

Merged
merged 6 commits into from
Feb 10, 2020
Merged

Improvements to property macros #760

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
f8c8b8e
Fix for PyObject with `#[pyo3(get)]`
davidhewitt Feb 7, 2020
cea8a9a
Refactor to clean up property descriptor macros
davidhewitt Feb 8, 2020
50bb41f
Add CHANGELOG entry for msrv bump
davidhewitt Feb 8, 2020
de9698e
Changes from PR#760
davidhewitt Feb 9, 2020
0eb4eb4
Merge branch 'master' into property-fixes
kngwyu Feb 10, 2020
7b3de17
Run cargo fmt
kngwyu Feb 10, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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,
Copy link
Member Author

@davidhewitt davidhewitt Feb 9, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was wondering whether the Clone bound should really be Copy, so that we don't let people accidentally do really expensive properties with #[pyo3(get)] (such as Vec<T>).

Users can always write their own #[getter] functions for types which aren't Copy.

{
fn get_property_value(&self, py: Python) -> PyObject {
(*self).clone().into_py(py)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why Deref here... ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

&self is &&T, so self.clone() makes &T. We need to deref like (*self).clone() to get a T.

}
}

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