diff --git a/ci/travis/cover.sh b/ci/travis/cover.sh index 1b685005..85b02617 100755 --- a/ci/travis/cover.sh +++ b/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 diff --git a/pyo3-derive-backend/src/method.rs b/pyo3-derive-backend/src/method.rs index 818a89c7..9cc02414 100644 --- a/pyo3-derive-backend/src/method.rs +++ b/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 { diff --git a/pyo3-derive-backend/src/module.rs b/pyo3-derive-backend/src/module.rs index 87117a8b..b9133533 100644 --- a/pyo3-derive-backend/src/module.rs +++ b/pyo3-derive-backend/src/module.rs @@ -64,16 +64,7 @@ fn wrap_fn_argument<'a>(input: &'a syn::FnArg, name: &'a Ident) -> Option 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())); diff --git a/pyo3-derive-backend/src/pyclass.rs b/pyo3-derive-backend/src/pyclass.rs index cbd4fbfc..0d9a334a 100644 --- a/pyo3-derive-backend/src/pyclass.rs +++ b/pyo3-derive-backend/src/pyclass.rs @@ -417,9 +417,12 @@ fn impl_descriptors(cls: &syn::Type, descriptors: Vec<(syn::Field, Vec)> 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()); diff --git a/pyo3-derive-backend/src/pyimpl.rs b/pyo3-derive-backend/src/pyimpl.rs index 5af616c9..64abab02 100644 --- a/pyo3-derive-backend/src/pyimpl.rs +++ b/pyo3-derive-backend/src/pyimpl.rs @@ -16,11 +16,11 @@ pub fn build_py_methods(ast: &mut syn::ItemImpl) -> syn::Result { "#[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) -> TokenStream { +pub fn impl_methods(ty: &syn::Type, impls: &mut Vec) -> syn::Result { // 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) -> 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; ::new(&[#(#methods),*]) } } - } + }) } diff --git a/pyo3-derive-backend/src/pymethod.rs b/pyo3-derive-backend/src/pymethod.rs index 994ab003..612df9f7 100644 --- a/pyo3-derive-backend/src/pymethod.rs +++ b/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, -) -> TokenStream { - check_generic(name, sig); +) -> syn::Result { + 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) => { diff --git a/pyo3-derive-backend/src/utils.rs b/pyo3-derive-backend/src/utils.rs index 886fae4c..a6a9eb26 100644 --- a/pyo3-derive-backend/src/utils.rs +++ b/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(); diff --git a/tests/test_compile_error.rs b/tests/test_compile_error.rs old mode 100644 new mode 100755 index 0437d5e3..5524ec41 --- a/tests/test_compile_error.rs +++ b/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"); } diff --git a/tests/test_getter_setter.rs b/tests/test_getter_setter.rs index a82d1ef4..cacf5652 100644 --- a/tests/test_getter_setter.rs +++ b/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::())].into_py_dict(py); py.run( diff --git a/tests/ui/too_many_args_to_getter.rs b/tests/ui/too_many_args_to_getter.rs new file mode 100644 index 00000000..f28b53f6 --- /dev/null +++ b/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() {} diff --git a/tests/ui/too_many_args_to_getter.stderr b/tests/ui/too_many_args_to_getter.stderr new file mode 100644 index 00000000..a81bb1be --- /dev/null +++ b/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) {} + | ^^^