Merge pull request #529 from kngwyu/getter-py

Allow py: Python as an argument of getter

ci/travis/cover.sh

@@ -16,7 +16,8 @@ rm -f target/debug/test_doc-*
 
 # Note: On travis this is run with -P1 because it started failing with
 # `-P $(nproc)`. kcov can probably be run in parallel if used with different CI
-FILES=$(find . -path ./target/debug/pyo3\* -or -path ./target/debug/test_\*)
+# Exclude test_compile_error.rs (See https://github.com/PyO3/pyo3/pull/503)
+FILES=$(find . -path ./target/debug/pyo3\* -or -path ./target/debug/test_\* -not -name '*test_compile_error*')
 echo $FILES | xargs -n1 -P1 sh -c '
   dir="target/cov/$(basename $@)"
   mkdir -p $dir

pyo3-derive-backend/src/method.rs

@@ -85,16 +85,7 @@ impl<'a> FnSpec<'a> {
                         }
                     };
 
-                    let py = match ty {
-                        syn::Type::Path(syn::TypePath { ref path, .. }) => {
-                            if let Some(segment) = path.segments.last() {
-                                segment.value().ident == "Python"
-                            } else {
-                                false
-                            }
-                        }
-                        _ => false,
-                    };
+                    let py = crate::utils::if_type_is_python(ty);
 
                     let opt = check_arg_ty_and_optional(name, ty);
                     arguments.push(FnArg {

pyo3-derive-backend/src/module.rs

@@ -64,16 +64,7 @@ fn wrap_fn_argument<'a>(input: &'a syn::FnArg, name: &'a Ident) -> Option<method
                 _ => panic!("unsupported argument: {:?}", cap.pat),
             };
 
-            let py = match cap.ty {
-                syn::Type::Path(ref typath) => typath
-                    .path
-                    .segments
-                    .last()
-                    .map(|seg| seg.value().ident == "Python")
-                    .unwrap_or(false),
-                _ => false,
-            };
-
+            let py = crate::utils::if_type_is_python(&cap.ty);
             let opt = method::check_arg_ty_and_optional(&name, &cap.ty);
             Some(method::FnArg {
                 name: ident,
@@ -111,7 +102,7 @@ fn extract_pyfn_attrs(
                         }
                         _ => panic!("The first parameter of pyfn must be a MetaItem"),
                     }
-                    // read Python fonction name
+                    // read Python function name
                     match meta[1] {
                         syn::NestedMeta::Literal(syn::Lit::Str(ref lits)) => {
                             fnname = Some(syn::Ident::new(&lits.value(), lits.span()));

pyo3-derive-backend/src/pyclass.rs

@@ -417,9 +417,12 @@ fn impl_descriptors(cls: &syn::Type, descriptors: Vec<(syn::Field, Vec<FnType>)>
 
                     let field_ty = &field.ty;
                     match *desc {
-                        FnType::Getter(ref getter) => {
-                            impl_py_getter_def(&name, doc, getter, &impl_wrap_getter(&cls, &name))
-                        }
+                        FnType::Getter(ref getter) => impl_py_getter_def(
+                            &name,
+                            doc,
+                            getter,
+                            &impl_wrap_getter(&cls, &name, false),
+                        ),
                         FnType::Setter(ref setter) => {
                             let setter_name =
                                 syn::Ident::new(&format!("set_{}", name), Span::call_site());

pyo3-derive-backend/src/pyimpl.rs

@@ -16,11 +16,11 @@ pub fn build_py_methods(ast: &mut syn::ItemImpl) -> syn::Result<TokenStream> {
             "#[pymethods] can not be used with lifetime parameters or generics",
         ))
     } else {
-        Ok(impl_methods(&ast.self_ty, &mut ast.items))
+        impl_methods(&ast.self_ty, &mut ast.items)
     }
 }
 
-pub fn impl_methods(ty: &syn::Type, impls: &mut Vec<syn::ImplItem>) -> TokenStream {
+pub fn impl_methods(ty: &syn::Type, impls: &mut Vec<syn::ImplItem>) -> syn::Result<TokenStream> {
     // get method names in impl block
     let mut methods = Vec::new();
     for iimpl in impls.iter_mut() {
@@ -31,16 +31,16 @@ pub fn impl_methods(ty: &syn::Type, impls: &mut Vec<syn::ImplItem>) -> TokenStre
                 &name,
                 &mut meth.sig,
                 &mut meth.attrs,
-            ));
+            )?);
         }
     }
 
-    quote! {
+    Ok(quote! {
        pyo3::inventory::submit! {
             #![crate = pyo3] {
                 type TyInventory = <#ty as pyo3::class::methods::PyMethodsInventoryDispatch>::InventoryType;
                 <TyInventory as pyo3::class::methods::PyMethodsInventory>::new(&[#(#methods),*])
             }
         }
-    }
+    })
 }

pyo3-derive-backend/src/pymethod.rs

@@ -9,13 +9,13 @@ pub fn gen_py_method(
     name: &syn::Ident,
     sig: &mut syn::MethodSig,
     meth_attrs: &mut Vec<syn::Attribute>,
-) -> TokenStream {
-    check_generic(name, sig);
+) -> syn::Result<TokenStream> {
+    check_generic(name, sig)?;
 
     let doc = utils::get_doc(&meth_attrs, true);
-    let spec = FnSpec::parse(name, sig, meth_attrs).unwrap();
+    let spec = FnSpec::parse(name, sig, meth_attrs)?;
 
-    match spec.tp {
+    Ok(match spec.tp {
         FnType::Fn => impl_py_method_def(name, doc, &spec, &impl_wrap(cls, name, &spec, true)),
         FnType::PySelf(ref self_ty) => impl_py_method_def(
             name,
@@ -31,28 +31,43 @@ pub fn gen_py_method(
             impl_py_method_def_static(name, doc, &impl_wrap_static(cls, name, &spec))
         }
         FnType::Getter(ref getter) => {
-            impl_py_getter_def(name, doc, getter, &impl_wrap_getter(cls, name))
+            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!",
+                    ))
+                }
+            };
+            impl_py_getter_def(name, doc, getter, &impl_wrap_getter(cls, name, takes_py))
         }
         FnType::Setter(ref setter) => {
             impl_py_setter_def(name, doc, setter, &impl_wrap_setter(cls, name, &spec))
         }
-    }
+    })
 }
 
-fn check_generic(name: &syn::Ident, sig: &syn::MethodSig) {
+fn check_generic(name: &syn::Ident, sig: &syn::MethodSig) -> syn::Result<()> {
+    let err_msg = |typ| {
+        format!(
+            "A Python method can't have a generic {} parameter: {}",
+            name, typ
+        )
+    };
     for param in &sig.decl.generics.params {
         match param {
             syn::GenericParam::Lifetime(_) => {}
-            syn::GenericParam::Type(_) => panic!(
-                "A Python method can't have a generic type parameter: {}",
-                name
-            ),
-            syn::GenericParam::Const(_) => panic!(
-                "A Python method can't have a const generic parameter: {}",
-                name
-            ),
+            syn::GenericParam::Type(_) => {
+                return Err(syn::Error::new_spanned(param, err_msg("type")))
+            }
+            syn::GenericParam::Const(_) => {
+                return Err(syn::Error::new_spanned(param, err_msg("const")))
+            }
         }
     }
+    Ok(())
 }
 
 /// Generate function wrapper (PyCFunction, PyCFunctionWithKeywords)
@@ -302,7 +317,12 @@ pub fn impl_wrap_static(cls: &syn::Type, name: &syn::Ident, spec: &FnSpec<'_>) -
 }
 
 /// Generate functiona wrapper (PyCFunction, PyCFunctionWithKeywords)
-pub(crate) fn impl_wrap_getter(cls: &syn::Type, name: &syn::Ident) -> TokenStream {
+pub(crate) fn impl_wrap_getter(cls: &syn::Type, name: &syn::Ident, takes_py: bool) -> TokenStream {
+    let fncall = if takes_py {
+        quote! { _slf.#name(_py) }
+    } else {
+        quote! { _slf.#name() }
+    };
     quote! {
         unsafe extern "C" fn __wrap(
             _slf: *mut pyo3::ffi::PyObject, _: *mut ::std::os::raw::c_void) -> *mut pyo3::ffi::PyObject
@@ -313,7 +333,7 @@ pub(crate) fn impl_wrap_getter(cls: &syn::Type, name: &syn::Ident) -> TokenStrea
             let _py = pyo3::Python::assume_gil_acquired();
             let _slf = _py.mut_from_borrowed_ptr::<#cls>(_slf);
 
-            let result = pyo3::derive_utils::IntoPyResult::into_py_result(_slf.#name());
+            let result = pyo3::derive_utils::IntoPyResult::into_py_result(#fncall);
 
             match result {
                 Ok(val) => {

pyo3-derive-backend/src/utils.rs

@@ -2,12 +2,24 @@
 
 use proc_macro2::Span;
 use proc_macro2::TokenStream;
-use syn;
 
 pub fn print_err(msg: String, t: TokenStream) {
     println!("Error: {} in '{}'", msg, t.to_string());
 }
 
+/// Check if the given type `ty` is `pyo3::Python`.
+pub fn if_type_is_python(ty: &syn::Type) -> bool {
+    match ty {
+        syn::Type::Path(ref typath) => typath
+            .path
+            .segments
+            .last()
+            .map(|seg| seg.value().ident == "Python")
+            .unwrap_or(false),
+        _ => false,
+    }
+}
+
 // FIXME(althonos): not sure the docstring formatting is on par here.
 pub fn get_doc(attrs: &[syn::Attribute], null_terminated: bool) -> syn::Lit {
     let mut doc = Vec::new();

tests/test_compile_error.rs

@@ -1,6 +1,6 @@
 #[test]
-#[cfg(testkcovstopmarker)]
 fn test_compile_errors() {
     let t = trybuild::TestCases::new();
     t.compile_fail("tests/ui/reject_generics.rs");
+    t.compile_fail("tests/ui/too_many_args_to_getter.rs");
 }

tests/test_getter_setter.rs

@@ -1,6 +1,6 @@
 use pyo3::prelude::*;
 use pyo3::py_run;
-use pyo3::types::IntoPyDict;
+use pyo3::types::{IntoPyDict, PyList};
 use std::isize;
 
 mod common;
@@ -32,10 +32,16 @@ impl ClassWithProperties {
     fn get_unwrapped(&self) -> i32 {
         self.num
     }
+
     #[setter]
     fn set_unwrapped(&mut self, value: i32) {
         self.num = value;
     }
+
+    #[getter]
+    fn get_data_list<'py>(&self, py: Python<'py>) -> &'py PyList {
+        PyList::new(py, &[self.num])
+    }
 }
 
 #[test]
@@ -48,12 +54,12 @@ fn class_with_properties() {
     py_run!(py, inst, "assert inst.get_num() == 10");
     py_run!(py, inst, "assert inst.get_num() == inst.DATA");
     py_run!(py, inst, "inst.DATA = 20");
-    py_run!(py, inst, "assert inst.get_num() == 20");
-    py_run!(py, inst, "assert inst.get_num() == inst.DATA");
+    py_run!(py, inst, "assert inst.get_num() == 20 == inst.DATA");
 
     py_run!(py, inst, "assert inst.get_num() == inst.unwrapped == 20");
     py_run!(py, inst, "inst.unwrapped = 42");
     py_run!(py, inst, "assert inst.get_num() == inst.unwrapped == 42");
+    py_run!(py, inst, "assert inst.data_list == [42]");
 
     let d = [("C", py.get_type::<ClassWithProperties>())].into_py_dict(py);
     py.run(

tests/ui/too_many_args_to_getter.rs

@@ -0,0 +1,14 @@
+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

@@ -0,0 +1,5 @@
+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) {}
+   |                              ^^^