error on passing arguments to #[new] and similar attributes
#3484
Conversation
| 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) | ||
| } |
There was a problem hiding this comment.
This is essentially the new logic for these #[pymethods] attributes.
| } else if path.is_ident("__new__") { | ||
| // TODO deprecate this form? | ||
| ensure_no_arguments(meta, "__new__")?; | ||
| Ok(Some(MethodTypeAttribute::New(path.span()))) |
There was a problem hiding this comment.
I discovered #[__new__] is a valid attribute in our code, I think as this is undocumented and redundant with #[new] we should deprecate it.
| 11 | #[getter(number)] | ||
| | ^^^^^^ |
There was a problem hiding this comment.
This test changed from erroring on the first of two attributes which specify name (#[pyo3(name = "num")] to erroring on the second site (#[getter(number)]`). Functionally no different as the code is rejected either way.
davidhewitt
force-pushed
the
new-no-arguments
branch
from
1498918
to
3c5b752
Compare
| /// `#[new]` | ||
| New, | ||
| /// `#[new]` && `#[classmethod]` | ||
| NewClassMethod, |
There was a problem hiding this comment.
What is the thought behind yeeting the NewClassMethod variant? I have no particular opinion - just curious.
There was a problem hiding this comment.
A lot of the complexity from the old implementation came from the need to work out when this variant was being built out of the two attributes. By removing it and instead pattern matching this combination I think the code reads better.
pyo3-macros-backend/src/method.rs
Outdated
| 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() { |
There was a problem hiding this comment.
IMO skip_first_arg should really be a boolean-like enum, there are too many trues and falses in here.
There was a problem hiding this comment.
I agree but I'll deal with that in a follow-up PR.
There was a problem hiding this comment.
Actually I pushed a second commit to address this, which moves this to be a method on the FnType enum.
davidhewitt
force-pushed
the
new-no-arguments
branch
from
79b712d
to
87ddaa8
Compare
|
Thanks for the review, force-pushed to apply all suggestions. |
davidhewitt
force-pushed
the
new-no-arguments
branch
from
87ddaa8
to
6c3c505
Compare
|
@mejrs anything further you would like to see in this PR? |
|
I'm going to assume this is now ok to merge; if there is more you wanted please forgive me and I'll happily follow up 🙏 |
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
davidhewitt
force-pushed
the
new-no-arguments
branch
from
6c3c505
to
f05ffb5
Compare
davidhewitt
force-pushed
the
new-no-arguments
branch
from
f05ffb5
to
b3ee70d
Compare
This PR reworks
#[new]to reject additional arguments rather than ignore them, as noted in #3475 (comment)We could change in the future to allow
#[new(signature = ())]to be equivalent to#[new] #[pyo3(signature = ())]if desired, as a backwards-compatible change.The bulk of the diff is because I threw out the old implementation of
parse_method_attributesto make it compose better for the error messages I wanted to emit.