From 1158c08f4223e4c6199f055ad062fa453a084c96 Mon Sep 17 00:00:00 2001 From: David Hewitt <1939362+davidhewitt@users.noreply.github.com> Date: Sat, 30 Sep 2023 22:51:52 +0100 Subject: [PATCH 1/2] error on passing arguments to `#[new]` and similar attributes --- newsfragments/3484.fixed.md | 1 + pyo3-macros-backend/src/method.rs | 351 ++++++++++++++----------- tests/ui/invalid_pymethod_names.rs | 13 + tests/ui/invalid_pymethod_names.stderr | 18 +- tests/ui/invalid_pymethods.rs | 36 ++- tests/ui/invalid_pymethods.stderr | 87 ++++-- 6 files changed, 320 insertions(+), 186 deletions(-) create mode 100644 newsfragments/3484.fixed.md diff --git a/newsfragments/3484.fixed.md b/newsfragments/3484.fixed.md new file mode 100644 index 00000000..c1f27412 --- /dev/null +++ b/newsfragments/3484.fixed.md @@ -0,0 +1 @@ +Emit error on invalid arguments to `#[new]`, `#[classmethod]`, `#[staticmethod]`, and `#[classattr]`. diff --git a/pyo3-macros-backend/src/method.rs b/pyo3-macros-backend/src/method.rs index 9562c6eb..2deec007 100644 --- a/pyo3-macros-backend/src/method.rs +++ b/pyo3-macros-backend/src/method.rs @@ -1,3 +1,5 @@ +use std::fmt::Display; + use crate::attributes::{TextSignatureAttribute, TextSignatureAttributeValue}; use crate::params::impl_arg_params; use crate::pyfunction::{FunctionSignature, PyFunctionArgPyO3Attributes}; @@ -9,7 +11,7 @@ use quote::ToTokens; use quote::{quote, quote_spanned}; use syn::ext::IdentExt; use syn::spanned::Spanned; -use syn::Result; +use syn::{Ident, Result}; #[derive(Clone, Debug)] pub struct FnArg<'a> { @@ -69,24 +71,6 @@ fn handle_argument_error(pat: &syn::Pat) -> syn::Error { syn::Error::new(span, msg) } -#[derive(Clone, PartialEq, Debug, Copy, Eq)] -pub enum MethodTypeAttribute { - /// `#[new]` - New, - /// `#[new]` && `#[classmethod]` - NewClassMethod, - /// `#[classmethod]` - ClassMethod, - /// `#[classattr]` - ClassAttribute, - /// `#[staticmethod]` - StaticMethod, - /// `#[getter]` - Getter, - /// `#[setter]` - Setter, -} - #[derive(Clone, Debug)] pub enum FnType { Getter(SelfType), @@ -278,13 +262,10 @@ impl<'a> FnSpec<'a> { .. } = options; - let MethodAttributes { - ty: fn_type_attr, - mut python_name, - } = parse_method_attributes(meth_attrs, name.map(|name| name.value.0))?; + let mut python_name = name.map(|name| name.value.0); let (fn_type, skip_first_arg, fixed_convention) = - Self::parse_fn_type(sig, fn_type_attr, &mut python_name)?; + Self::parse_fn_type(sig, meth_attrs, &mut python_name)?; ensure_signatures_on_valid_method(&fn_type, signature.as_ref(), text_signature.as_ref())?; let name = &sig.ident; @@ -331,9 +312,11 @@ impl<'a> FnSpec<'a> { fn parse_fn_type( sig: &syn::Signature, - fn_type_attr: Option, + meth_attrs: &mut Vec, python_name: &mut Option, ) -> Result<(FnType, bool, Option)> { + let mut method_attributes = parse_method_attributes(meth_attrs)?; + let name = &sig.ident; let parse_receiver = |msg: &'static str| { let first_arg = sig @@ -351,52 +334,87 @@ impl<'a> FnSpec<'a> { .map(|stripped| syn::Ident::new(stripped, name.span())) }; - let (fn_type, skip_first_arg, fixed_convention) = match fn_type_attr { - Some(MethodTypeAttribute::StaticMethod) => (FnType::FnStatic, false, None), - Some(MethodTypeAttribute::ClassAttribute) => (FnType::ClassAttribute, false, None), - Some(MethodTypeAttribute::New) | Some(MethodTypeAttribute::NewClassMethod) => { - if let Some(name) = &python_name { - bail_spanned!(name.span() => "`name` not allowed with `#[new]`"); - } - *python_name = Some(syn::Ident::new("__new__", Span::call_site())); - if matches!(fn_type_attr, Some(MethodTypeAttribute::New)) { - (FnType::FnNew, false, Some(CallingConvention::TpNew)) - } else { - (FnType::FnNewClass, true, Some(CallingConvention::TpNew)) - } - } - Some(MethodTypeAttribute::ClassMethod) => (FnType::FnClass, true, None), - Some(MethodTypeAttribute::Getter) => { - // Strip off "get_" prefix if needed - if python_name.is_none() { - *python_name = strip_fn_name("get_"); - } - - ( - FnType::Getter(parse_receiver("expected receiver for #[getter]")?), - true, - None, - ) - } - Some(MethodTypeAttribute::Setter) => { - // Strip off "set_" prefix if needed - if python_name.is_none() { - *python_name = strip_fn_name("set_"); - } - - ( - FnType::Setter(parse_receiver("expected receiver for #[setter]")?), - true, - None, - ) - } - None => ( + let (fn_type, skip_first_arg, fixed_convention) = match method_attributes.as_mut_slice() { + [] => ( FnType::Fn(parse_receiver( "static method needs #[staticmethod] attribute", )?), true, None, ), + [MethodTypeAttribute::StaticMethod(_)] => (FnType::FnStatic, false, None), + [MethodTypeAttribute::ClassAttribute(_)] => (FnType::ClassAttribute, false, None), + [MethodTypeAttribute::New(_)] + | [MethodTypeAttribute::New(_), MethodTypeAttribute::ClassMethod(_)] + | [MethodTypeAttribute::ClassMethod(_), MethodTypeAttribute::New(_)] => { + if let Some(name) = &python_name { + bail_spanned!(name.span() => "`name` not allowed with `#[new]`"); + } + *python_name = Some(syn::Ident::new("__new__", Span::call_site())); + if matches!(method_attributes.as_slice(), [MethodTypeAttribute::New(_)]) { + (FnType::FnNew, false, Some(CallingConvention::TpNew)) + } else { + (FnType::FnNewClass, true, Some(CallingConvention::TpNew)) + } + } + [MethodTypeAttribute::ClassMethod(_)] => (FnType::FnClass, true, None), + [MethodTypeAttribute::Getter(_, name)] => { + if let Some(name) = name.take() { + ensure_spanned!( + python_name.replace(name).is_none(), + python_name.span() => "`name` may only be specified once" + ); + } else if python_name.is_none() { + // Strip off "get_" prefix if needed + *python_name = strip_fn_name("get_"); + } + + ( + FnType::Getter(parse_receiver("expected receiver for `#[getter]`")?), + true, + None, + ) + } + [MethodTypeAttribute::Setter(_, name)] => { + if let Some(name) = name.take() { + ensure_spanned!( + python_name.replace(name).is_none(), + python_name.span() => "`name` may only be specified once" + ); + } else if python_name.is_none() { + // Strip off "set_" prefix if needed + *python_name = strip_fn_name("set_"); + } + + ( + FnType::Setter(parse_receiver("expected receiver for `#[setter]`")?), + true, + None, + ) + } + [first, rest @ .., last] => { + // Join as many of the spans together as possible + let span = rest + .iter() + .fold(first.span(), |s, next| s.join(next.span()).unwrap_or(s)); + let span = span.join(last.span()).unwrap_or(span); + // List all the attributes in the error message + let mut msg = format!("`{}` may not be combined with", first); + let mut is_first = true; + for attr in &*rest { + msg.push_str(&format!(" `{}`", attr)); + if is_first { + is_first = false; + } else { + msg.push(','); + } + } + if !rest.is_empty() { + msg.push_str(" and"); + } + msg.push_str(&format!(" `{}`", last)); + bail_spanned!(span => msg) + } }; Ok((fn_type, skip_first_arg, fixed_convention)) } @@ -604,107 +622,128 @@ impl<'a> FnSpec<'a> { } } -#[derive(Debug)] -struct MethodAttributes { - ty: Option, - python_name: Option, +enum MethodTypeAttribute { + New(Span), + ClassMethod(Span), + StaticMethod(Span), + Getter(Span, Option), + Setter(Span, Option), + ClassAttribute(Span), } -fn parse_method_attributes( - attrs: &mut Vec, - mut python_name: Option, -) -> Result { +impl MethodTypeAttribute { + fn span(&self) -> Span { + match self { + MethodTypeAttribute::New(span) + | MethodTypeAttribute::ClassMethod(span) + | MethodTypeAttribute::StaticMethod(span) + | MethodTypeAttribute::Getter(span, _) + | MethodTypeAttribute::Setter(span, _) + | MethodTypeAttribute::ClassAttribute(span) => *span, + } + } + + /// Attempts to parse a method type attribute. + /// + /// If the attribute does not match one of the attribute names, returns `Ok(None)`. + /// + /// Otherwise will either return a parse error or the attribute. + fn parse_if_matching_attribute(attr: &syn::Attribute) -> Result> { + fn ensure_no_arguments(meta: &syn::Meta, ident: &str) -> syn::Result<()> { + match meta { + syn::Meta::Path(_) => Ok(()), + syn::Meta::List(l) => bail_spanned!( + l.span() => format!( + "`#[{ident}]` does not take any arguments\n= help: did you mean `#[{ident}] #[pyo3({meta})]`?", + ident = ident, + meta = l.tokens, + ) + ), + syn::Meta::NameValue(nv) => { + bail_spanned!(nv.eq_token.span() => format!( + "`#[{}]` does not take any arguments\n= note: this was previously accepted and ignored", + ident + )) + } + } + } + + fn extract_name(meta: &syn::Meta, ident: &str) -> Result> { + match meta { + syn::Meta::Path(_) => Ok(None), + syn::Meta::NameValue(nv) => bail_spanned!( + nv.eq_token.span() => format!("expected `#[{}(name)]` to set the name", ident) + ), + syn::Meta::List(l) => { + if let Ok(name) = l.parse_args::() { + Ok(Some(name)) + } else if let Ok(name) = l.parse_args::() { + name.parse().map(Some) + } else { + bail_spanned!(l.tokens.span() => "expected ident or string literal for property name"); + } + } + } + } + + let meta = &attr.meta; + let path = meta.path(); + + if path.is_ident("new") { + ensure_no_arguments(meta, "new")?; + Ok(Some(MethodTypeAttribute::New(path.span()))) + } else if path.is_ident("__new__") { + // TODO deprecate this form? + ensure_no_arguments(meta, "__new__")?; + Ok(Some(MethodTypeAttribute::New(path.span()))) + } else if path.is_ident("classmethod") { + ensure_no_arguments(meta, "classmethod")?; + Ok(Some(MethodTypeAttribute::ClassMethod(path.span()))) + } else if path.is_ident("staticmethod") { + ensure_no_arguments(meta, "staticmethod")?; + Ok(Some(MethodTypeAttribute::StaticMethod(path.span()))) + } else if path.is_ident("classattr") { + ensure_no_arguments(meta, "classattr")?; + Ok(Some(MethodTypeAttribute::ClassAttribute(path.span()))) + } else if path.is_ident("getter") { + let name = extract_name(meta, "getter")?; + Ok(Some(MethodTypeAttribute::Getter(path.span(), name))) + } else if path.is_ident("setter") { + let name = extract_name(meta, "setter")?; + Ok(Some(MethodTypeAttribute::Setter(path.span(), name))) + } else { + Ok(None) + } + } +} + +impl Display for MethodTypeAttribute { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + MethodTypeAttribute::New(_) => "#[new]".fmt(f), + MethodTypeAttribute::ClassMethod(_) => "#[classmethod]".fmt(f), + MethodTypeAttribute::StaticMethod(_) => "#[staticmethod]".fmt(f), + MethodTypeAttribute::Getter(_, _) => "#[getter]".fmt(f), + MethodTypeAttribute::Setter(_, _) => "#[setter]".fmt(f), + MethodTypeAttribute::ClassAttribute(_) => "#[classattr]".fmt(f), + } + } +} + +fn parse_method_attributes(attrs: &mut Vec) -> Result> { let mut new_attrs = Vec::new(); - let mut ty: Option = None; - - macro_rules! set_compound_ty { - ($new_ty:expr, $ident:expr) => { - ty = match (ty, $new_ty) { - (None, new_ty) => Some(new_ty), - (Some(MethodTypeAttribute::ClassMethod), MethodTypeAttribute::New) => Some(MethodTypeAttribute::NewClassMethod), - (Some(MethodTypeAttribute::New), MethodTypeAttribute::ClassMethod) => Some(MethodTypeAttribute::NewClassMethod), - (Some(_), _) => bail_spanned!($ident.span() => "can only combine `new` and `classmethod`"), - }; - }; - } - - macro_rules! set_ty { - ($new_ty:expr, $ident:expr) => { - ensure_spanned!( - ty.replace($new_ty).is_none(), - $ident.span() => "cannot combine these method types" - ); - }; - } + let mut found_attrs = Vec::new(); for attr in attrs.drain(..) { - match attr.meta { - syn::Meta::Path(ref name) => { - if name.is_ident("new") || name.is_ident("__new__") { - set_compound_ty!(MethodTypeAttribute::New, name); - } else if name.is_ident("classmethod") { - set_compound_ty!(MethodTypeAttribute::ClassMethod, name); - } else if name.is_ident("staticmethod") { - set_ty!(MethodTypeAttribute::StaticMethod, name); - } else if name.is_ident("classattr") { - set_ty!(MethodTypeAttribute::ClassAttribute, name); - } else if name.is_ident("setter") || name.is_ident("getter") { - if let syn::AttrStyle::Inner(_) = attr.style { - bail_spanned!( - attr.span() => "inner attribute is not supported for setter and getter" - ); - } - if name.is_ident("setter") { - set_ty!(MethodTypeAttribute::Setter, name); - } else { - set_ty!(MethodTypeAttribute::Getter, name); - } - } else { - new_attrs.push(attr) - } - } - syn::Meta::List(ref ml @ syn::MetaList { ref path, .. }) => { - if path.is_ident("new") { - set_ty!(MethodTypeAttribute::New, path); - } else if path.is_ident("setter") || path.is_ident("getter") { - if let syn::AttrStyle::Inner(_) = attr.style { - bail_spanned!( - attr.span() => "inner attribute is not supported for setter and getter" - ); - } - - if path.is_ident("setter") { - set_ty!(MethodTypeAttribute::Setter, path); - } else { - set_ty!(MethodTypeAttribute::Getter, path); - }; - - ensure_spanned!( - python_name.is_none(), - python_name.span() => "`name` may only be specified once" - ); - - if let Ok(ident) = ml.parse_args::() { - python_name = Some(ident); - } else if let Ok(syn::Lit::Str(s)) = ml.parse_args::() { - python_name = Some(s.parse()?); - } else { - return Err(syn::Error::new_spanned( - ml, - "expected ident or string literal for property name", - )); - } - } else { - new_attrs.push(attr) - } - } - syn::Meta::NameValue(_) => new_attrs.push(attr), + match MethodTypeAttribute::parse_if_matching_attribute(&attr)? { + Some(attr) => found_attrs.push(attr), + None => new_attrs.push(attr), } } *attrs = new_attrs; - Ok(MethodAttributes { ty, python_name }) + Ok(found_attrs) } const IMPL_TRAIT_ERR: &str = "Python functions cannot have `impl Trait` arguments"; diff --git a/tests/ui/invalid_pymethod_names.rs b/tests/ui/invalid_pymethod_names.rs index 337cb2a3..1334bcab 100644 --- a/tests/ui/invalid_pymethod_names.rs +++ b/tests/ui/invalid_pymethod_names.rs @@ -26,4 +26,17 @@ impl TestClass { fn new(&self) -> Self { Self { num: 0 } } } +#[pymethods] +impl TestClass { + #[getter(1)] + fn get_one(&self) -> Self { Self { num: 0 } } +} + +#[pymethods] +impl TestClass { + #[getter = 1] + fn get_two(&self) -> Self { Self { num: 0 } } +} + + fn main() {} diff --git a/tests/ui/invalid_pymethod_names.stderr b/tests/ui/invalid_pymethod_names.stderr index c99c692c..1e7a6f44 100644 --- a/tests/ui/invalid_pymethod_names.stderr +++ b/tests/ui/invalid_pymethod_names.stderr @@ -1,8 +1,8 @@ error: `name` may only be specified once - --> tests/ui/invalid_pymethod_names.rs:10:19 + --> tests/ui/invalid_pymethod_names.rs:11:14 | -10 | #[pyo3(name = "num")] - | ^^^^^ +11 | #[getter(number)] + | ^^^^^^ error: `name` may only be specified once --> tests/ui/invalid_pymethod_names.rs:18:12 @@ -15,3 +15,15 @@ error: `name` not allowed with `#[new]` | 24 | #[pyo3(name = "makenew")] | ^^^^^^^^^ + +error: expected ident or string literal for property name + --> tests/ui/invalid_pymethod_names.rs:31:14 + | +31 | #[getter(1)] + | ^ + +error: expected `#[getter(name)]` to set the name + --> tests/ui/invalid_pymethod_names.rs:37:14 + | +37 | #[getter = 1] + | ^ diff --git a/tests/ui/invalid_pymethods.rs b/tests/ui/invalid_pymethods.rs index 03500dd1..c652748b 100644 --- a/tests/ui/invalid_pymethods.rs +++ b/tests/ui/invalid_pymethods.rs @@ -108,11 +108,45 @@ impl MyClass { #[pymethods] impl MyClass { - #[classattr] + #[new] + #[classmethod] #[staticmethod] + #[classattr] + #[getter(x)] + #[setter(x)] fn multiple_method_types() {} } +#[pymethods] +impl MyClass { + #[new(signature = ())] + fn new_takes_no_arguments(&self) {} +} + +#[pymethods] +impl MyClass { + #[new = ()] // in this form there's no suggestion to move arguments to `#[pyo3()]` attribute + fn new_takes_no_arguments_nv(&self) {} +} + +#[pymethods] +impl MyClass { + #[classmethod(signature = ())] + fn classmethod_takes_no_arguments(&self) {} +} + +#[pymethods] +impl MyClass { + #[staticmethod(signature = ())] + fn staticmethod_takes_no_arguments(&self) {} +} + +#[pymethods] +impl MyClass { + #[classattr(signature = ())] + fn classattr_takes_no_arguments(&self) {} +} + #[pymethods] impl MyClass { fn generic_method(value: T) {} diff --git a/tests/ui/invalid_pymethods.stderr b/tests/ui/invalid_pymethods.stderr index 4453340a..4836bcee 100644 --- a/tests/ui/invalid_pymethods.stderr +++ b/tests/ui/invalid_pymethods.stderr @@ -22,13 +22,13 @@ error: unexpected receiver 26 | fn staticmethod_with_receiver(&self) {} | ^ -error: expected receiver for #[getter] +error: expected receiver for `#[getter]` --> tests/ui/invalid_pymethods.rs:39:5 | 39 | fn getter_without_receiver() {} | ^^ -error: expected receiver for #[setter] +error: expected receiver for `#[setter]` --> tests/ui/invalid_pymethods.rs:45:5 | 45 | fn setter_without_receiver() {} @@ -88,62 +88,97 @@ error: `signature` not allowed with `classattr` 105 | #[pyo3(signature = ())] | ^^^^^^^^^ -error: cannot combine these method types - --> tests/ui/invalid_pymethods.rs:112:7 +error: `#[new]` may not be combined with `#[classmethod]` `#[staticmethod]`, `#[classattr]`, `#[getter]`, and `#[setter]` + --> tests/ui/invalid_pymethods.rs:111:7 | -112 | #[staticmethod] +111 | #[new] + | ^^^ + +error: `#[new]` does not take any arguments + = help: did you mean `#[new] #[pyo3(signature = ())]`? + --> tests/ui/invalid_pymethods.rs:122:7 + | +122 | #[new(signature = ())] + | ^^^ + +error: `#[new]` does not take any arguments + = note: this was previously accepted and ignored + --> tests/ui/invalid_pymethods.rs:128:11 + | +128 | #[new = ()] // in this form there's no suggestion to move arguments to `#[pyo3()]` attribute + | ^ + +error: `#[classmethod]` does not take any arguments + = help: did you mean `#[classmethod] #[pyo3(signature = ())]`? + --> tests/ui/invalid_pymethods.rs:134:7 + | +134 | #[classmethod(signature = ())] + | ^^^^^^^^^^^ + +error: `#[staticmethod]` does not take any arguments + = help: did you mean `#[staticmethod] #[pyo3(signature = ())]`? + --> tests/ui/invalid_pymethods.rs:140:7 + | +140 | #[staticmethod(signature = ())] | ^^^^^^^^^^^^ -error: Python functions cannot have generic type parameters - --> tests/ui/invalid_pymethods.rs:118:23 +error: `#[classattr]` does not take any arguments + = help: did you mean `#[classattr] #[pyo3(signature = ())]`? + --> tests/ui/invalid_pymethods.rs:146:7 | -118 | fn generic_method(value: T) {} +146 | #[classattr(signature = ())] + | ^^^^^^^^^ + +error: Python functions cannot have generic type parameters + --> tests/ui/invalid_pymethods.rs:152:23 + | +152 | fn generic_method(value: T) {} | ^ error: Python functions cannot have `impl Trait` arguments - --> tests/ui/invalid_pymethods.rs:123:48 + --> tests/ui/invalid_pymethods.rs:157:48 | -123 | fn impl_trait_method_first_arg(impl_trait: impl AsRef) {} +157 | fn impl_trait_method_first_arg(impl_trait: impl AsRef) {} | ^^^^ error: Python functions cannot have `impl Trait` arguments - --> tests/ui/invalid_pymethods.rs:128:56 + --> tests/ui/invalid_pymethods.rs:162:56 | -128 | fn impl_trait_method_second_arg(&self, impl_trait: impl AsRef) {} +162 | fn impl_trait_method_second_arg(&self, impl_trait: impl AsRef) {} | ^^^^ error: `async fn` is not yet supported for Python functions. Additional crates such as `pyo3-asyncio` can be used to integrate async Rust and Python. For more information, see https://github.com/PyO3/pyo3/issues/1632 - --> tests/ui/invalid_pymethods.rs:133:5 + --> tests/ui/invalid_pymethods.rs:167:5 | -133 | async fn async_method(&self) {} +167 | async fn async_method(&self) {} | ^^^^^ error: `pass_module` cannot be used on Python methods - --> tests/ui/invalid_pymethods.rs:138:12 + --> tests/ui/invalid_pymethods.rs:172:12 | -138 | #[pyo3(pass_module)] +172 | #[pyo3(pass_module)] | ^^^^^^^^^^^ error: Python objects are shared, so 'self' cannot be moved out of the Python interpreter. Try `&self`, `&mut self, `slf: PyRef<'_, Self>` or `slf: PyRefMut<'_, Self>`. - --> tests/ui/invalid_pymethods.rs:144:29 + --> tests/ui/invalid_pymethods.rs:178:29 | -144 | fn method_self_by_value(self) {} +178 | fn method_self_by_value(self) {} | ^^^^ error: macros cannot be used as items in `#[pymethods]` impl blocks = note: this was previously accepted and ignored - --> tests/ui/invalid_pymethods.rs:179:5 + --> tests/ui/invalid_pymethods.rs:213:5 | -179 | macro_invocation!(); +213 | macro_invocation!(); | ^^^^^^^^^^^^^^^^ error[E0119]: conflicting implementations of trait `pyo3::impl_::pyclass::PyClassNewTextSignature` for type `pyo3::impl_::pyclass::PyClassImplCollector` - --> tests/ui/invalid_pymethods.rs:149:1 + --> tests/ui/invalid_pymethods.rs:183:1 | -149 | #[pymethods] +183 | #[pymethods] | ^^^^^^^^^^^^ | | | first implementation here @@ -152,9 +187,9 @@ error[E0119]: conflicting implementations of trait `pyo3::impl_::pyclass::PyClas = note: this error originates in the attribute macro `pymethods` (in Nightly builds, run with -Z macro-backtrace for more info) error[E0592]: duplicate definitions with name `__pymethod___new____` - --> tests/ui/invalid_pymethods.rs:149:1 + --> tests/ui/invalid_pymethods.rs:183:1 | -149 | #[pymethods] +183 | #[pymethods] | ^^^^^^^^^^^^ | | | duplicate definitions for `__pymethod___new____` @@ -163,9 +198,9 @@ error[E0592]: duplicate definitions with name `__pymethod___new____` = note: this error originates in the attribute macro `pymethods` (in Nightly builds, run with -Z macro-backtrace for more info) error[E0592]: duplicate definitions with name `__pymethod_func__` - --> tests/ui/invalid_pymethods.rs:164:1 + --> tests/ui/invalid_pymethods.rs:198:1 | -164 | #[pymethods] +198 | #[pymethods] | ^^^^^^^^^^^^ | | | duplicate definitions for `__pymethod_func__` From b3ee70db408d06dba75f54951ede924d0d19f737 Mon Sep 17 00:00:00 2001 From: David Hewitt <1939362+davidhewitt@users.noreply.github.com> Date: Mon, 2 Oct 2023 08:26:56 +0100 Subject: [PATCH 2/2] refactor `parse_fn_type` --- pyo3-macros-backend/src/method.rs | 82 +++++++++++++++---------------- 1 file changed, 41 insertions(+), 41 deletions(-) diff --git a/pyo3-macros-backend/src/method.rs b/pyo3-macros-backend/src/method.rs index 2deec007..fbf55629 100644 --- a/pyo3-macros-backend/src/method.rs +++ b/pyo3-macros-backend/src/method.rs @@ -85,6 +85,18 @@ pub enum FnType { } impl FnType { + pub fn skip_first_rust_argument_in_python_signature(&self) -> bool { + match self { + FnType::Getter(_) + | FnType::Setter(_) + | FnType::Fn(_) + | FnType::FnClass + | FnType::FnNewClass + | FnType::FnModule => true, + FnType::FnNew | FnType::FnStatic | FnType::ClassAttribute => false, + } + } + pub fn self_arg(&self, cls: Option<&syn::Type>, error_mode: ExtractErrorMode) -> TokenStream { match self { FnType::Getter(st) | FnType::Setter(st) | FnType::Fn(st) => { @@ -264,26 +276,23 @@ impl<'a> FnSpec<'a> { let mut python_name = name.map(|name| name.value.0); - let (fn_type, skip_first_arg, fixed_convention) = - Self::parse_fn_type(sig, meth_attrs, &mut python_name)?; + let fn_type = Self::parse_fn_type(sig, meth_attrs, &mut python_name)?; ensure_signatures_on_valid_method(&fn_type, signature.as_ref(), text_signature.as_ref())?; let name = &sig.ident; let ty = get_return_info(&sig.output); let python_name = python_name.as_ref().unwrap_or(name).unraw(); - let arguments: Vec<_> = if skip_first_arg { - sig.inputs - .iter_mut() - .skip(1) - .map(FnArg::parse) - .collect::>()? - } else { - sig.inputs - .iter_mut() - .map(FnArg::parse) - .collect::>()? - }; + let arguments: Vec<_> = sig + .inputs + .iter_mut() + .skip(if fn_type.skip_first_rust_argument_in_python_signature() { + 1 + } else { + 0 + }) + .map(FnArg::parse) + .collect::>()?; let signature = if let Some(signature) = signature { FunctionSignature::from_arguments_and_attribute(arguments, signature)? @@ -291,8 +300,11 @@ impl<'a> FnSpec<'a> { FunctionSignature::from_arguments(arguments)? }; - let convention = - fixed_convention.unwrap_or_else(|| CallingConvention::from_signature(&signature)); + let convention = if matches!(fn_type, FnType::FnNew | FnType::FnNewClass) { + CallingConvention::TpNew + } else { + CallingConvention::from_signature(&signature) + }; Ok(FnSpec { tp: fn_type, @@ -314,7 +326,7 @@ impl<'a> FnSpec<'a> { sig: &syn::Signature, meth_attrs: &mut Vec, python_name: &mut Option, - ) -> Result<(FnType, bool, Option)> { + ) -> Result { let mut method_attributes = parse_method_attributes(meth_attrs)?; let name = &sig.ident; @@ -334,16 +346,12 @@ impl<'a> FnSpec<'a> { .map(|stripped| syn::Ident::new(stripped, name.span())) }; - let (fn_type, skip_first_arg, fixed_convention) = match method_attributes.as_mut_slice() { - [] => ( - FnType::Fn(parse_receiver( - "static method needs #[staticmethod] attribute", - )?), - true, - None, - ), - [MethodTypeAttribute::StaticMethod(_)] => (FnType::FnStatic, false, None), - [MethodTypeAttribute::ClassAttribute(_)] => (FnType::ClassAttribute, false, None), + let fn_type = match method_attributes.as_mut_slice() { + [] => FnType::Fn(parse_receiver( + "static method needs #[staticmethod] attribute", + )?), + [MethodTypeAttribute::StaticMethod(_)] => FnType::FnStatic, + [MethodTypeAttribute::ClassAttribute(_)] => FnType::ClassAttribute, [MethodTypeAttribute::New(_)] | [MethodTypeAttribute::New(_), MethodTypeAttribute::ClassMethod(_)] | [MethodTypeAttribute::ClassMethod(_), MethodTypeAttribute::New(_)] => { @@ -352,12 +360,12 @@ impl<'a> FnSpec<'a> { } *python_name = Some(syn::Ident::new("__new__", Span::call_site())); if matches!(method_attributes.as_slice(), [MethodTypeAttribute::New(_)]) { - (FnType::FnNew, false, Some(CallingConvention::TpNew)) + FnType::FnNew } else { - (FnType::FnNewClass, true, Some(CallingConvention::TpNew)) + FnType::FnNewClass } } - [MethodTypeAttribute::ClassMethod(_)] => (FnType::FnClass, true, None), + [MethodTypeAttribute::ClassMethod(_)] => FnType::FnClass, [MethodTypeAttribute::Getter(_, name)] => { if let Some(name) = name.take() { ensure_spanned!( @@ -369,11 +377,7 @@ impl<'a> FnSpec<'a> { *python_name = strip_fn_name("get_"); } - ( - FnType::Getter(parse_receiver("expected receiver for `#[getter]`")?), - true, - None, - ) + FnType::Getter(parse_receiver("expected receiver for `#[getter]`")?) } [MethodTypeAttribute::Setter(_, name)] => { if let Some(name) = name.take() { @@ -386,11 +390,7 @@ impl<'a> FnSpec<'a> { *python_name = strip_fn_name("set_"); } - ( - FnType::Setter(parse_receiver("expected receiver for `#[setter]`")?), - true, - None, - ) + FnType::Setter(parse_receiver("expected receiver for `#[setter]`")?) } [first, rest @ .., last] => { // Join as many of the spans together as possible @@ -416,7 +416,7 @@ impl<'a> FnSpec<'a> { bail_spanned!(span => msg) } }; - Ok((fn_type, skip_first_arg, fixed_convention)) + Ok(fn_type) } /// Return a C wrapper function for this signature.