diff --git a/CHANGELOG.md b/CHANGELOG.md index 866d4169108..9722cd2dcdf 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -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 diff --git a/pyo3-derive-backend/src/pyclass.rs b/pyo3-derive-backend/src/pyclass.rs index 7404993f8fb..688d9a31cf2 100644 --- a/pyo3-derive-backend/src/pyclass.rs +++ b/pyo3-derive-backend/src/pyclass.rs @@ -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; @@ -421,90 +423,26 @@ fn impl_descriptors( cls: &syn::Type, descriptors: Vec<(syn::Field, Vec)>, ) -> syn::Result { - let methods: Vec = 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::>() - }) - .collect(); - let py_methods: Vec = 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!(), } }) @@ -513,7 +451,6 @@ fn impl_descriptors( .collect::>()?; Ok(quote! { - #(#methods)* pyo3::inventory::submit! { #![crate = pyo3] { diff --git a/pyo3-derive-backend/src/pymethod.rs b/pyo3-derive-backend/src/pymethod.rs index 97bb1f161f0..5827f43cc23 100644 --- a/pyo3-derive-backend/src/pymethod.rs +++ b/pyo3-derive-backend/src/pymethod.rs @@ -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, @@ -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))?, + ), }) } @@ -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 { - 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 { + 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 { + 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 @@ -276,7 +306,7 @@ pub(crate) fn impl_wrap_getter(cls: &syn::Type, spec: &FnSpec) -> syn::Result(_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) => { @@ -291,25 +321,42 @@ pub(crate) fn impl_wrap_getter(cls: &syn::Type, spec: &FnSpec) -> syn::Result) -> syn::Result { +fn impl_call_setter(spec: &FnSpec) -> syn::Result { + 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 { + 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! { @@ -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) }; @@ -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 @@ -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 @@ -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), + } +} diff --git a/src/derive_utils.rs b/src/derive_utils.rs index a5e284b5f9a..62e3e2efaca 100644 --- a/src/derive_utils.rs +++ b/src/derive_utils.rs @@ -199,3 +199,22 @@ impl>> IntoPyNewResult for PyRes self } } + +pub trait GetPropertyValue { + fn get_property_value(&self, py: Python) -> PyObject; +} + +impl GetPropertyValue for &T +where + T: IntoPy + 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) + } +} diff --git a/tests/test_class_basics.rs b/tests/test_class_basics.rs index caf763b4498..69caa5594b7 100644 --- a/tests/test_class_basics.rs +++ b/tests/test_class_basics.rs @@ -138,3 +138,28 @@ fn empty_class_in_module() { // than using whatever calls init first. assert_eq!(module, "builtins"); } + +#[pyclass] +struct ClassWithObjectField { + // PyObject has special case for derive_utils::GetPropertyValue, + // so this test is making sure it compiles! + #[pyo3(get, set)] + value: PyObject, +} + +#[pymethods] +impl ClassWithObjectField { + #[new] + fn new(value: PyObject) -> ClassWithObjectField { + Self { value } + } +} + +#[test] +fn class_with_object_field() { + let gil = Python::acquire_gil(); + let py = gil.python(); + let ty = py.get_type::(); + py_assert!(py, ty, "ty(5).value == 5"); + py_assert!(py, ty, "ty(None).value == None"); +} diff --git a/tests/test_compile_error.rs b/tests/test_compile_error.rs index 661176c0e40..b316e011aaf 100644 --- a/tests/test_compile_error.rs +++ b/tests/test_compile_error.rs @@ -1,8 +1,8 @@ #[test] fn test_compile_errors() { let t = trybuild::TestCases::new(); + t.compile_fail("tests/ui/invalid_property_args.rs"); t.compile_fail("tests/ui/invalid_pymethod_names.rs"); t.compile_fail("tests/ui/missing_clone.rs"); t.compile_fail("tests/ui/reject_generics.rs"); - t.compile_fail("tests/ui/too_many_args_to_getter.rs"); } diff --git a/tests/ui/invalid_property_args.rs b/tests/ui/invalid_property_args.rs new file mode 100644 index 00000000000..5982add7e22 --- /dev/null +++ b/tests/ui/invalid_property_args.rs @@ -0,0 +1,27 @@ +use pyo3::prelude::*; + +#[pyclass] +struct ClassWithGetter {} + +#[pymethods] +impl ClassWithGetter { + #[getter] + fn getter_with_arg(&self, py: Python, index: u32) {} +} + +#[pyclass] +struct ClassWithSetter {} + +#[pymethods] +impl ClassWithSetter { + #[setter] + fn setter_with_no_arg(&mut self, py: Python) {} +} + +#[pymethods] +impl ClassWithSetter { + #[setter] + fn setter_with_too_many_args(&mut self, py: Python, foo: u32, bar: u32) {} +} + +fn main() {} diff --git a/tests/ui/invalid_property_args.stderr b/tests/ui/invalid_property_args.stderr new file mode 100644 index 00000000000..b411ef421de --- /dev/null +++ b/tests/ui/invalid_property_args.stderr @@ -0,0 +1,17 @@ +error: Getter function can only have one argument of type pyo3::Python + --> $DIR/invalid_property_args.rs:9:50 + | +9 | fn getter_with_arg(&self, py: Python, index: u32) {} + | ^^^ + +error: Setter function expected to have one argument + --> $DIR/invalid_property_args.rs:18:8 + | +18 | fn setter_with_no_arg(&mut self, py: Python) {} + | ^^^^^^^^^^^^^^^^^^ + +error: Setter function can have at most two arguments: one of pyo3::Python, and one other + --> $DIR/invalid_property_args.rs:24:72 + | +24 | fn setter_with_too_many_args(&mut self, py: Python, foo: u32, bar: u32) {} + | ^^^ diff --git a/tests/ui/too_many_args_to_getter.rs b/tests/ui/too_many_args_to_getter.rs deleted file mode 100644 index f28b53f6e2b..00000000000 --- a/tests/ui/too_many_args_to_getter.rs +++ /dev/null @@ -1,14 +0,0 @@ -use pyo3::prelude::*; - -#[pyclass] -struct ClassWithGetter { - a: u32, -} - -#[pymethods] -impl ClassWithGetter { - #[getter] - fn get_num(&self, index: u32) {} -} - -fn main() {} diff --git a/tests/ui/too_many_args_to_getter.stderr b/tests/ui/too_many_args_to_getter.stderr deleted file mode 100644 index a81bb1be1f9..00000000000 --- a/tests/ui/too_many_args_to_getter.stderr +++ /dev/null @@ -1,5 +0,0 @@ -error: Getter function can only have one argument of type pyo3::Python! - --> $DIR/too_many_args_to_getter.rs:11:30 - | -11 | fn get_num(&self, index: u32) {} - | ^^^