Refactor to clean up property descriptor macros

.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"

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
 

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)

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_DATE: &'static str = "2020-01-20";
-const MIN_VERSION: &'static str = "1.37.0-nightly";
+const MIN_VERSION: &'static str = "1.42.0-nightly";
 //const PYTHON_INTERPRETER: &'static str = "python3";
 
 lazy_static! {

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::method::FnType;
-use crate::pymethod::{impl_py_getter_def, impl_py_setter_def, impl_wrap_getter, impl_wrap_setter};
+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 => {
+                        FnType::Getter => Ok(impl_py_getter_def(
-                            let spec = FnSpec {
+                            &name,
-                                tp: FnType::Getter,
+                            &doc,
-                                name: &name,
+                            &impl_wrap_getter(&cls, PropertyType::Descriptor(&field))?,
-                                python_name: name.unraw(),
+                        )),
-                                attrs: Vec::new(),
+                        FnType::Setter => Ok(impl_py_setter_def(
-                                args: Vec::new(),
+                            &name,
-                                output: parse_quote!(PyResult<#field_ty>),
+                            &doc,
-                                doc,
+                            &impl_wrap_setter(&cls, PropertyType::Descriptor(&field))?,
-                            };
+                        )),
-                            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(()) }),
-                                ),
-                            ))
-                        }
                         _ => unreachable!(),
                     }
                 })

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,
+            &spec.python_name,
-            &impl_wrap_getter(cls, &spec, impl_call_getter(&spec)?),
+            &spec.doc,
+            &impl_wrap_getter(cls, PropertyType::Function(&spec))?,
         ),
         FnType::Setter => impl_py_setter_def(
-            &spec,
+            &spec.python_name,
-            &impl_wrap_setter(cls, &spec, impl_call_setter(&spec)?),
+            &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<TokenStream> {
-    let takes_py = match &*spec.args {
+    let (py_arg, args) = split_off_python_arg(&spec.args);
-        [] => false,
+    if !args.is_empty() {
-        [arg] if utils::if_type_is_python(arg.ty) => true,
+        return Err(syn::Error::new_spanned(
-        _ => {
+            args[0].ty,
-            return Err(syn::Error::new_spanned(
+            "Getter function can only have one argument of type pyo3::Python",
-                spec.args[0].ty,
+        ));
-                "Getter function can only have one argument of type pyo3::Python!",
+    }
-            ));
-        }
-    };
 
     let name = &spec.name;
-
+    let fncall = if py_arg.is_some() {
-    let fncall = if takes_py {
         quote! { _slf.#name(_py) }
     } else {
         quote! { _slf.#name() }
@@ -274,9 +278,29 @@ fn impl_call_getter(spec: &FnSpec) -> syn::Result<TokenStream> {
 }
 
 /// Generate functiona wrapper (PyCFunction, PyCFunctionWithKeywords)
-pub(crate) fn impl_wrap_getter(cls: &syn::Type, spec: &FnSpec, fncall: TokenStream) -> TokenStream {
+pub(crate) fn impl_wrap_getter(
-    let python_name = &spec.python_name;
+    cls: &syn::Type,
-    quote! {
+    property_type: PropertyType,
+) -> syn::Result<TokenStream> {
+    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<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 {
+    Ok(fncall)
-    //     [] => {
-    //         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))))
 }
 
 /// Generate functiona wrapper (PyCFunction, PyCFunctionWithKeywords)
 pub(crate) fn impl_wrap_setter(
     cls: &syn::Type,
-    spec: &FnSpec<'_>,
+    property_type: PropertyType,
-    fncall: TokenStream,
+) -> syn::Result<TokenStream> {
-) -> TokenStream {
+    let python_name;
-    let python_name = &spec.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 {
+pub(crate) fn impl_py_setter_def(
-    let python_name = &&spec.python_name;
+    python_name: &syn::Ident,
-    let doc = &spec.doc;
+    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 {
+pub(crate) fn impl_py_getter_def(
-    let python_name = &&spec.python_name;
+    python_name: &syn::Ident,
-    let doc = &spec.doc;
+    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),
+    }
+}

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");
 }

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() {}

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) {}
+   |                                                                        ^^^

tests/ui/too_many_args_to_getter.rs

@@ -1,14 +0,0 @@
-use pyo3::prelude::*;
-
-#[pyclass]
-struct ClassWithGetter {
-    a: u32,
-}
-
-#[pymethods]
-impl ClassWithGetter {
-    #[getter]
-    fn get_num(&self, index: u32) {}
-}
-
-fn main() {}

tests/ui/too_many_args_to_getter.stderr

@@ -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) {}
-   |                              ^^^