From cea8a9a2b053854d53fa5ad323b1be8a90eaf829 Mon Sep 17 00:00:00 2001 From: David Hewitt <1939362+davidhewitt@users.noreply.github.com> Date: Sat, 8 Feb 2020 18:50:55 +0000 Subject: [PATCH] Refactor to clean up property descriptor macros --- .travis.yml | 2 +- CHANGELOG.md | 3 +- README.md | 2 +- build.rs | 4 +- pyo3-derive-backend/src/pyclass.rs | 73 +++-------- pyo3-derive-backend/src/pymethod.rs | 156 ++++++++++++++++-------- tests/test_compile_error.rs | 2 +- tests/ui/invalid_property_args.rs | 27 ++++ tests/ui/invalid_property_args.stderr | 17 +++ tests/ui/too_many_args_to_getter.rs | 14 --- tests/ui/too_many_args_to_getter.stderr | 5 - 11 files changed, 168 insertions(+), 137 deletions(-) create mode 100644 tests/ui/invalid_property_args.rs create mode 100644 tests/ui/invalid_property_args.stderr delete mode 100644 tests/ui/too_many_args_to_getter.rs delete mode 100644 tests/ui/too_many_args_to_getter.stderr diff --git a/.travis.yml b/.travis.yml index 93f98dea..4540b1ee 100644 --- a/.travis.yml +++ b/.travis.yml @@ -20,7 +20,7 @@ matrix: python: "3.7" # Keep this synced up with build.rs and ensure that the nightly version does have clippy available # https://static.rust-lang.org/dist/YYYY-MM-DD/clippy-nightly-x86_64-unknown-linux-gnu.tar.gz exists - env: TRAVIS_RUST_VERSION=nightly-2019-07-19 + env: TRAVIS_RUST_VERSION=nightly-2020-01-21 - name: PyPy3.5 7.0 # Tested via anaconda PyPy (since travis's PyPy version is too old) python: "3.7" env: FEATURES="pypy" PATH="$PATH:/opt/anaconda/envs/pypy3/bin" diff --git a/CHANGELOG.md b/CHANGELOG.md index 28716856..0dbb6e3a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -22,13 +22,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)]` [#XXX](https://github.com/PyO3/pyo3/pull/XXX) +* Usage of `PyObject` with `#[pyo3(get)]` [#760](https://github.com/PyO3/pyo3/pull/760) ### Removed diff --git a/README.md b/README.md index 50b99258..f5ecc925 100644 --- a/README.md +++ b/README.md @@ -16,7 +16,7 @@ A comparison with rust-cpython can be found [in the guide](https://pyo3.rs/maste ## Usage -PyO3 supports Python 3.5 and up. The minimum required Rust version is 1.37.0-nightly 2019-07-19. +PyO3 supports Python 3.5 and up. The minimum required Rust version is 1.42.0-nightly 2020-01-21. If you have never used nightly Rust, the official guide has [a great section](https://doc.rust-lang.org/book/appendix-07-nightly-rust.html#rustup-and-the-role-of-rust-nightly) diff --git a/build.rs b/build.rs index cffdccf2..9007c284 100644 --- a/build.rs +++ b/build.rs @@ -19,8 +19,8 @@ use version_check::{Channel, Date, Version}; /// Specifies the minimum nightly version needed to compile pyo3. /// Keep this synced up with the travis ci config, /// But note that this is the rustc version which can be lower than the nightly version -const MIN_DATE: &'static str = "2019-07-18"; -const MIN_VERSION: &'static str = "1.37.0-nightly"; +const MIN_DATE: &'static str = "2020-01-20"; +const MIN_VERSION: &'static str = "1.42.0-nightly"; //const PYTHON_INTERPRETER: &'static str = "python3"; lazy_static! { diff --git a/pyo3-derive-backend/src/pyclass.rs b/pyo3-derive-backend/src/pyclass.rs index bbf44328..07d15b4f 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; @@ -426,66 +428,21 @@ fn impl_descriptors( .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, - quote!({ - use pyo3::derive_utils::GetPropertyValue; - (&_slf.#name).get_property_value(_py) - }), - ), - )) - } - 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, - quote!({ _slf.#name = _val; Ok(()) }), - ), - )) - } + 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!(), } }) diff --git a/pyo3-derive-backend/src/pymethod.rs b/pyo3-derive-backend/src/pymethod.rs index e43b63d7..469b8660 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, @@ -22,12 +28,14 @@ pub fn gen_py_method( 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, impl_call_getter(&spec)?), + &spec.python_name, + &spec.doc, + &impl_wrap_getter(cls, PropertyType::Function(&spec))?, ), FnType::Setter => impl_py_setter_def( - &spec, - &impl_wrap_setter(cls, &spec, impl_call_setter(&spec)?), + &spec.python_name, + &spec.doc, + &impl_wrap_setter(cls, PropertyType::Function(&spec))?, ), }) } @@ -251,20 +259,16 @@ pub fn impl_wrap_static(cls: &syn::Type, spec: &FnSpec<'_>) -> TokenStream { } fn impl_call_getter(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!", - )); - } - }; + 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 fncall = if takes_py { + let fncall = if py_arg.is_some() { quote! { _slf.#name(_py) } } else { quote! { _slf.#name() } @@ -274,9 +278,29 @@ fn impl_call_getter(spec: &FnSpec) -> syn::Result { } /// Generate functiona wrapper (PyCFunction, PyCFunctionWithKeywords) -pub(crate) fn impl_wrap_getter(cls: &syn::Type, spec: &FnSpec, fncall: TokenStream) -> TokenStream { - let python_name = &spec.python_name; - quote! { +pub(crate) fn impl_wrap_getter( + cls: &syn::Type, + property_type: PropertyType, +) -> syn::Result { + let python_name; + let getter_impl; + + match property_type { + PropertyType::Descriptor(field) => { + let name = field.ident.as_ref().unwrap(); + python_name = name.unraw(); + getter_impl = quote!({ + use pyo3::derive_utils::GetPropertyValue; + (&_slf.#name).get_property_value(_py) + }); + } + PropertyType::Function(spec) => { + python_name = spec.python_name.clone(); + getter_impl = 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 { @@ -286,7 +310,7 @@ pub(crate) fn impl_wrap_getter(cls: &syn::Type, spec: &FnSpec, fncall: TokenStre 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) => { @@ -298,41 +322,55 @@ pub(crate) fn impl_wrap_getter(cls: &syn::Type, spec: &FnSpec, fncall: TokenStre } } } - } + }) } 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(quote!(pyo3::derive_utils::IntoPyResult::into_py_result(_slf.#name(_val)))) + Ok(fncall) } /// Generate functiona wrapper (PyCFunction, PyCFunctionWithKeywords) pub(crate) fn impl_wrap_setter( cls: &syn::Type, - spec: &FnSpec<'_>, - fncall: TokenStream, -) -> TokenStream { - let python_name = &spec.python_name; + property_type: PropertyType, +) -> syn::Result { + let python_name; + let setter_impl; - quote! { + match property_type { + PropertyType::Descriptor(field) => { + let name = field.ident.as_ref().unwrap(); + python_name = name.unraw(); + setter_impl = quote!({ _slf.#name = _val; Ok(()) }); + } + PropertyType::Function(spec) => { + python_name = spec.python_name.clone(); + setter_impl = impl_call_setter(&spec)?; + } + }; + + Ok(quote! { #[allow(unused_mut)] unsafe extern "C" fn __wrap( _slf: *mut pyo3::ffi::PyObject, @@ -346,7 +384,7 @@ pub(crate) fn impl_wrap_setter( let _result = match pyo3::FromPyObject::extract(_value) { Ok(_val) => { - #fncall + #setter_impl } Err(e) => Err(e) }; @@ -358,7 +396,7 @@ pub(crate) fn impl_wrap_setter( } } } - } + }) } /// This function abstracts away some copied code and can propably be simplified itself @@ -637,10 +675,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 @@ -654,10 +693,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 @@ -670,3 +710,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/tests/test_compile_error.rs b/tests/test_compile_error.rs index 661176c0..b316e011 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 00000000..5982add7 --- /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 00000000..b411ef42 --- /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 f28b53f6..00000000 --- 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 a81bb1be..00000000 --- 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) {} - | ^^^