From 1498918b5aed5a42c75fa4cd284f2e914ace2abc 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] error on passing arguments to `#[new]` and similar attributes --- newsfragments/3483.fixed.md | 1 + pyo3-macros-backend/src/method.rs | 273 +++++++++++++------------ tests/ui/invalid_pymethod_names.stderr | 6 +- tests/ui/invalid_pymethods.rs | 30 +++ tests/ui/invalid_pymethods.stderr | 76 +++++-- 5 files changed, 233 insertions(+), 153 deletions(-) create mode 100644 newsfragments/3483.fixed.md diff --git a/newsfragments/3483.fixed.md b/newsfragments/3483.fixed.md new file mode 100644 index 00000000000..c1f27412acf --- /dev/null +++ b/newsfragments/3483.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 9562c6ebbcb..9c517449d4c 100644 --- a/pyo3-macros-backend/src/method.rs +++ b/pyo3-macros-backend/src/method.rs @@ -9,7 +9,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 +69,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 +260,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 +310,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 +332,70 @@ 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) => { + 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!(fn_type_attr, Some(MethodTypeAttribute::New)) { + if matches!(method_attributes.as_slice(), [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() { + [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]")?), + FnType::Getter(parse_receiver("expected receiver for `#[getter]`")?), true, None, ) } - Some(MethodTypeAttribute::Setter) => { - // Strip off "set_" prefix if needed - if python_name.is_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 "get_" prefix if needed *python_name = strip_fn_name("set_"); } ( - FnType::Setter(parse_receiver("expected receiver for #[setter]")?), + FnType::Setter(parse_receiver("expected receiver for `#[setter]`")?), true, None, ) } - None => ( - FnType::Fn(parse_receiver( - "static method needs #[staticmethod] attribute", - )?), - true, - None, - ), + [first, second, ..] => { + bail_spanned!( + second.span() => + format!("`#[{}]` may not be combined with `#[{}]`", first.ident(), second.ident()) + ) + } }; Ok((fn_type, skip_first_arg, fixed_convention)) } @@ -604,107 +603,123 @@ 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 { - 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`"), - }; - }; +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, + } } - macro_rules! set_ty { - ($new_ty:expr, $ident:expr) => { - ensure_spanned!( - ty.replace($new_ty).is_none(), - $ident.span() => "cannot combine these method types" - ); - }; + fn ident(&self) -> &'static str { + match self { + MethodTypeAttribute::New(_) => "new", + MethodTypeAttribute::ClassMethod(_) => "classmethod", + MethodTypeAttribute::StaticMethod(_) => "staticmethod", + MethodTypeAttribute::Getter(_, _) => "getter", + MethodTypeAttribute::Setter(_, _) => "setter", + MethodTypeAttribute::ClassAttribute(_) => "classattr", + } } - 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) + /// 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", ident)) } } - 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()?); + 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(ml) => { + if let Ok(name) = ml.parse_args::() { + Ok(Some(name)) + } else if let Ok(name) = ml.parse_args::() { + name.parse().map(Some) } else { - return Err(syn::Error::new_spanned( - ml, - "expected ident or string literal for property name", - )); + bail_spanned!(ml.tokens.span() => "expected ident or string literal for property name"); } - } else { - new_attrs.push(attr) } } - syn::Meta::NameValue(_) => new_attrs.push(attr), + } + + 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) + } + } +} + +fn parse_method_attributes(attrs: &mut Vec) -> Result> { + let mut new_attrs = Vec::new(); + let mut found_attrs = Vec::new(); + + for attr in attrs.drain(..) { + 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.stderr b/tests/ui/invalid_pymethod_names.stderr index c99c692c459..a1b4e1b5dcc 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 diff --git a/tests/ui/invalid_pymethods.rs b/tests/ui/invalid_pymethods.rs index b2a183a2f7e..0b32ebf59d0 100644 --- a/tests/ui/invalid_pymethods.rs +++ b/tests/ui/invalid_pymethods.rs @@ -113,6 +113,36 @@ impl MyClass { 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 29bd4cc96e1..8a36960b5fb 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,55 +88,89 @@ error: `signature` not allowed with `classattr` 105 | #[pyo3(signature = ())] | ^^^^^^^^^ -error: cannot combine these method types +error: `#[classattr]` may not be combined with `#[staticmethod]` --> tests/ui/invalid_pymethods.rs:112:7 | 112 | #[staticmethod] | ^^^^^^^^^^^^ +error: `#[new]` does not take any arguments + = help: did you mean `#[new] #[pyo3(signature = ())]`? + --> tests/ui/invalid_pymethods.rs:118:7 + | +118 | #[new(signature = ())] + | ^^^ + +error: `#[new]` does not take any arguments + --> tests/ui/invalid_pymethods.rs:124:11 + | +124 | #[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:130:7 + | +130 | #[classmethod(signature = ())] + | ^^^^^^^^^^^ + +error: `#[staticmethod]` does not take any arguments + = help: did you mean `#[staticmethod] #[pyo3(signature = ())]`? + --> tests/ui/invalid_pymethods.rs:136:7 + | +136 | #[staticmethod(signature = ())] + | ^^^^^^^^^^^^ + +error: `#[classattr]` does not take any arguments + = help: did you mean `#[classattr] #[pyo3(signature = ())]`? + --> tests/ui/invalid_pymethods.rs:142:7 + | +142 | #[classattr(signature = ())] + | ^^^^^^^^^ + error: Python functions cannot have generic type parameters - --> tests/ui/invalid_pymethods.rs:118:23 + --> tests/ui/invalid_pymethods.rs:148:23 | -118 | fn generic_method(value: T) {} +148 | 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:153:48 | -123 | fn impl_trait_method_first_arg(impl_trait: impl AsRef) {} +153 | 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:158:56 | -128 | fn impl_trait_method_second_arg(&self, impl_trait: impl AsRef) {} +158 | 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:163:5 | -133 | async fn async_method(&self) {} +163 | 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:168:12 | -138 | #[pyo3(pass_module)] +168 | #[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:174:29 | -144 | fn method_self_by_value(self) {} +174 | fn method_self_by_value(self) {} | ^^^^ 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:179:1 | -149 | #[pymethods] +179 | #[pymethods] | ^^^^^^^^^^^^ | | | first implementation here @@ -145,9 +179,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:179:1 | -149 | #[pymethods] +179 | #[pymethods] | ^^^^^^^^^^^^ | | | duplicate definitions for `__pymethod___new____` @@ -156,9 +190,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:194:1 | -164 | #[pymethods] +194 | #[pymethods] | ^^^^^^^^^^^^ | | | duplicate definitions for `__pymethod_func__`