From f8c8b8effded330bdf6adadfcea649d1ddf92dd1 Mon Sep 17 00:00:00 2001 From: David Hewitt <1939362+davidhewitt@users.noreply.github.com> Date: Fri, 7 Feb 2020 19:31:13 +0000 Subject: [PATCH 1/5] Fix for PyObject with `#[pyo3(get)]` --- CHANGELOG.md | 1 + pyo3-derive-backend/src/pyclass.rs | 58 +++++++-------------- pyo3-derive-backend/src/pymethod.rs | 78 ++++++++++++++++++----------- src/derive_utils.rs | 21 +++++++- tests/test_class_basics.rs | 25 +++++++++ 5 files changed, 114 insertions(+), 69 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c99b62c0ad7..28716856fbb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -28,6 +28,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0. * 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) ### Removed diff --git a/pyo3-derive-backend/src/pyclass.rs b/pyo3-derive-backend/src/pyclass.rs index e2f94340d78..bbf44328d7e 100644 --- a/pyo3-derive-backend/src/pyclass.rs +++ b/pyo3-derive-backend/src/pyclass.rs @@ -421,42 +421,6 @@ 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)| { @@ -479,7 +443,17 @@ fn impl_descriptors( output: parse_quote!(PyResult<#field_ty>), doc, }; - Ok(impl_py_getter_def(&spec, &impl_wrap_getter(&cls, &spec)?)) + 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( @@ -503,7 +477,14 @@ fn impl_descriptors( output: parse_quote!(PyResult<()>), doc, }; - Ok(impl_py_setter_def(&spec, &impl_wrap_setter(&cls, &spec)?)) + Ok(impl_py_setter_def( + &spec, + &impl_wrap_setter( + &cls, + &spec, + quote!({ _slf.#name = _val; Ok(()) }), + ), + )) } _ => unreachable!(), } @@ -513,7 +494,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..e43b63d758e 100644 --- a/pyo3-derive-backend/src/pymethod.rs +++ b/pyo3-derive-backend/src/pymethod.rs @@ -21,8 +21,14 @@ 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, + &impl_wrap_getter(cls, &spec, impl_call_getter(&spec)?), + ), + FnType::Setter => impl_py_setter_def( + &spec, + &impl_wrap_setter(cls, &spec, impl_call_setter(&spec)?), + ), }) } @@ -244,8 +250,7 @@ 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 { +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, @@ -258,7 +263,6 @@ pub(crate) fn impl_wrap_getter(cls: &syn::Type, spec: &FnSpec) -> syn::Result syn::Result TokenStream { + let python_name = &spec.python_name; + quote! { unsafe extern "C" fn __wrap( _slf: *mut pyo3::ffi::PyObject, _: *mut ::std::os::raw::c_void) -> *mut pyo3::ffi::PyObject { @@ -288,31 +298,41 @@ pub(crate) fn impl_wrap_getter(cls: &syn::Type, spec: &FnSpec) -> syn::Result) -> syn::Result { +fn impl_call_setter(spec: &FnSpec) -> syn::Result { let name = &spec.name; - let python_name = &spec.python_name; + // let python_name = &spec.python_name; + + // 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)))) +} - 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", - )) - } - }; +/// 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; - Ok(quote! { + quote! { #[allow(unused_mut)] unsafe extern "C" fn __wrap( _slf: *mut pyo3::ffi::PyObject, @@ -324,9 +344,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)) + #fncall } Err(e) => Err(e) }; @@ -338,7 +358,7 @@ pub(crate) fn impl_wrap_setter(cls: &syn::Type, spec: &FnSpec<'_>) -> syn::Resul } } } - }) + } } /// This function abstracts away some copied code and can propably be simplified itself diff --git a/src/derive_utils.rs b/src/derive_utils.rs index a5e284b5f9a..87e0d89d8ca 100644 --- a/src/derive_utils.rs +++ b/src/derive_utils.rs @@ -11,7 +11,7 @@ use crate::instance::PyNativeType; use crate::pyclass::PyClass; use crate::pyclass_init::PyClassInitializer; use crate::types::{PyAny, PyDict, PyModule, PyTuple}; -use crate::{ffi, GILPool, IntoPy, PyObject, Python}; +use crate::{ffi, GILPool, IntoPy, PyObject, Python, ToPyObject}; use std::ptr; /// Description of a python parameter; used for `parse_args()`. @@ -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.to_object(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"); +} 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 2/5] 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 | 158 +++++++++++++++--------- 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, 169 insertions(+), 138 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 93f98dea17c..4540b1ee8f8 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 28716856fbb..0dbb6e3abe8 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 50b99258ee8..f5ecc925a61 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 cffdccf2add..9007c284486 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 bbf44328d7e..07d15b4f9bc 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 e43b63d758e..469b86605a4 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 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)))) + 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))) + }; + + 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; + + 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)?; + } + }; - quote! { + 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 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) {} - | ^^^ From 50bb41f3983b4bc1bfd8eee9a743342be00ac308 Mon Sep 17 00:00:00 2001 From: David Hewitt <1939362+davidhewitt@users.noreply.github.com> Date: Sat, 8 Feb 2020 19:02:52 +0000 Subject: [PATCH 3/5] Add CHANGELOG entry for msrv bump --- CHANGELOG.md | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0dbb6e3abe8..e6ae507b506 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0. * The blanket implementations for `FromPyObject` for `&T` and `&mut T` are no longer specializable. Implement `PyTryFrom` for your type to control the behavior of `FromPyObject::extract()` for your types. [#713](https://github.com/PyO3/pyo3/pull/713) * The implementation for `IntoPy for T` where `U: FromPy` is no longer specializable. Control the behavior of this via the implementation of `FromPy`. [#713](https://github.com/PyO3/pyo3/pull/713) * Use `parking_lot::Mutex` instead of `spin::Mutex`. [#734](https://github.com/PyO3/pyo3/pull/734) +* Bumped minimum Rust version to `1.42.0-nightly 2020-01-21`. [#760](https://github.com/PyO3/pyo3/pull/760) ### Added @@ -22,14 +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) +* `#[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) +* Usage of `PyObject` with `#[pyo3(get)]`. [#760](https://github.com/PyO3/pyo3/pull/760) ### Removed From de9698e7a52d6630ead80666becb26adb07e59b6 Mon Sep 17 00:00:00 2001 From: David Hewitt <1939362+davidhewitt@users.noreply.github.com> Date: Sun, 9 Feb 2020 11:06:44 +0000 Subject: [PATCH 4/5] Changes from PR#760 --- pyo3-derive-backend/src/pymethod.rs | 39 +++++++++++------------------ src/derive_utils.rs | 4 +-- 2 files changed, 16 insertions(+), 27 deletions(-) diff --git a/pyo3-derive-backend/src/pymethod.rs b/pyo3-derive-backend/src/pymethod.rs index 469b86605a4..8067aa6cb59 100644 --- a/pyo3-derive-backend/src/pymethod.rs +++ b/pyo3-derive-backend/src/pymethod.rs @@ -277,27 +277,23 @@ fn impl_call_getter(spec: &FnSpec) -> syn::Result { Ok(fncall) } -/// Generate functiona wrapper (PyCFunction, PyCFunctionWithKeywords) +/// 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; - let getter_impl; - - match property_type { + let (python_name, 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)?; + ( + 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! { @@ -350,24 +346,17 @@ fn impl_call_setter(spec: &FnSpec) -> syn::Result { Ok(fncall) } -/// Generate functiona wrapper (PyCFunction, PyCFunctionWithKeywords) +/// 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; - let setter_impl; - - match property_type { + let (python_name, setter_impl) = 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)?; + (name.unraw(), quote!({ _slf.#name = _val; Ok(()) })) } + PropertyType::Function(spec) => (spec.python_name.clone(), impl_call_setter(&spec)?) }; Ok(quote! { diff --git a/src/derive_utils.rs b/src/derive_utils.rs index 87e0d89d8ca..62e3e2efaca 100644 --- a/src/derive_utils.rs +++ b/src/derive_utils.rs @@ -11,7 +11,7 @@ use crate::instance::PyNativeType; use crate::pyclass::PyClass; use crate::pyclass_init::PyClassInitializer; use crate::types::{PyAny, PyDict, PyModule, PyTuple}; -use crate::{ffi, GILPool, IntoPy, PyObject, Python, ToPyObject}; +use crate::{ffi, GILPool, IntoPy, PyObject, Python}; use std::ptr; /// Description of a python parameter; used for `parse_args()`. @@ -215,6 +215,6 @@ where impl GetPropertyValue for PyObject { fn get_property_value(&self, py: Python) -> PyObject { - self.to_object(py) + self.clone_ref(py) } } From 7b3de17d811cf7ab21d425fa24a22103abb32dda Mon Sep 17 00:00:00 2001 From: kngwyu Date: Mon, 10 Feb 2020 18:18:44 +0900 Subject: [PATCH 5/5] Run cargo fmt --- pyo3-derive-backend/src/pymethod.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/pyo3-derive-backend/src/pymethod.rs b/pyo3-derive-backend/src/pymethod.rs index 8067aa6cb59..5827f43cc23 100644 --- a/pyo3-derive-backend/src/pymethod.rs +++ b/pyo3-derive-backend/src/pymethod.rs @@ -290,10 +290,10 @@ pub(crate) fn impl_wrap_getter( quote!({ use pyo3::derive_utils::GetPropertyValue; (&_slf.#name).get_property_value(_py) - }) + }), ) } - PropertyType::Function(spec) => (spec.python_name.clone(), impl_call_getter(&spec)?) + PropertyType::Function(spec) => (spec.python_name.clone(), impl_call_getter(&spec)?), }; Ok(quote! { @@ -356,7 +356,7 @@ pub(crate) fn impl_wrap_setter( 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)?) + PropertyType::Function(spec) => (spec.python_name.clone(), impl_call_setter(&spec)?), }; Ok(quote! {