Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

macros: support #[pyo3(name = "...")] in pyfunction #1567

Merged
merged 3 commits into from May 11, 2021

Conversation

davidhewitt
Copy link
Member

This PR is the first in a series of PRs which I plan to do to tidy up the macro attribute APIs, parsing, and documentation. This should make it easier for maintainers as well as users. I've spoken about this kind of idea briefly at #1507 (comment)

At the moment we have the #[name = "..."] attribute which can be placed on #[pyfunction] and in #[pymethods]. It's not documented, and it's hard to document because it's not consistent how names are set. #[pymodule] and #[pyfn] take a name directly as an argument e.g. #[pymodule("foo")] and #[pyfn(m, "foo")], for example.

As a first step I want to unify to an #[attribute(name = "...")] syntax, where attribute can be:

  • #[pyo3(name = "foo")]
  • #[pyfunction(name = "foo")]
  • #[pymodule(name = "foo")]
    etc.

#[pyo3(name = "...")] can appear on individual #[pymethods], as well as alongside #[pyfunction]. For example:

#[pymethods]
impl Foo {
    #[pyo3(name = "foo")]
    fn pymethod_with_name(&self) { }
}

#[pyfunction(a, b, c)]
#[pyo3(name = "foo", pass_module)]
fn pyfunction_form_1(_: &PyModule, a: i32, b: i32, c: i32) { } 

// this has a shorthand:

#[pyfunction(name = "foo", pass_module, a, b, c)]
fn pyfunction_form_2(_: &PyModule, a: i32, b: i32, c: i32) { }

Because pyfunction_form_2 has options mixed in with the signature, I've made it possible to separate the signature out into a new (undocumented, for now) option called signature:

#[pyfunction(name = "foo", pass_module, signature = (a, b, c))]
fn pyfunction_form_2(_: &PyModule, a: i32, b: i32, c: i32) { }

// is just shorthand for

#[pyfunction]
#[pyo3(name = "foo", pass_module, signature = (a, b, c))]
fn pyfunction_form_2(_: &PyModule, a: i32, b: i32, c: i32) { }

Note that mixing of pass_module and signature already exists (#1566), so adding a new explicit signature option helps solve the existing problem too.

To review this PR:

This PR's diff is bigger than just the feature change because I needed to refactor a fair bit of the code to make this possible. At the same time this refactoring fixes #1566, and will make future PRs in this series much easier to compare.

  1. Do you like the migration of #[name = "foo"] to #[pyo3(name = "foo")]? I've used a bit of macro hackery to emit deprecation warnings when the old form is used.
  2. Do you like the shorthand form #[pyfunction(name = "foo")] as well as #[pyfunction] #[pyo3(name = "foo")]?
    • My motivation is that the shorthand form is what most users will want to write, and the second form is what macros could expand to (Allow #[name] with #[getter] and #[setter] #1507 (comment)).
    • Also I think it makes it easy to document - we can document all the #[pyo3()] options and say that #[pyfunction(...)] can take all the same options as a shorthand.

Future steps:

If you like the direction this is going, these are the next steps:

  • Update #[pymodule("name")], #[getter(name)] and #[pyfn(m, name)] to use the same name = "..." form.
  • Do a similar improvement for #[text_signature = "..."] -> #[pyo3(text_signature = "...")] / #[pyfunction(text_signature = "...")].
  • Officially support signature option to pyfunction (and implement Support positional-only arguments in #[pyfunction] #1439 now that the parsing is refactored).
  • Write a reference in the guide with all the attributes

@davidhewitt
Copy link
Member Author

... I'm beginning to consider that this PR could first be merged without deprecating the existing attribute syntax. We can get all the improvements which we like completed, and then finalise the process with documentation and deprecate the current syntax.

Copy link
Member

@kngwyu kngwyu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Almost good, but isn't it OK to use lots of codes for deprecation warnings? I'm not sure this is the best way

pyo3-macros-backend/src/pyfunction.rs Outdated Show resolved Hide resolved
pyo3-macros-backend/src/attributes.rs Outdated Show resolved Hide resolved
pyo3-macros-backend/src/attributes.rs Outdated Show resolved Hide resolved
pyo3-macros-backend/src/attributes.rs Show resolved Hide resolved
pyo3-macros-backend/src/from_pyobject.rs Outdated Show resolved Hide resolved
@davidhewitt
Copy link
Member Author

Thanks for the review! I pushed a commit to address your feedback.

So I assume that you overall are happy with the design proposal here? i.e. the new form of #[name] attribute (and making the rest of the attributes such as #[text_signature] like this too).

Almost good, but isn't it OK to use lots of codes for deprecation warnings? I'm not sure this is the best way

Can you clarify what you mean by this?

@kngwyu
Copy link
Member

kngwyu commented Apr 30, 2021

So I assume that you overall are happy with the design proposal here?

I'm sorry but I recently haven't used these macros in real so have no opinion. Hearing from users might be better.

Can you clarify what you mean by this?

Almost the same as you wrote

I've used a bit of macro hackery to emit deprecation warnings when the old form is used

@mejrs
Copy link
Member

mejrs commented May 1, 2021

Do you like the migration of #[name = "foo"] to #[pyo3(name = "foo")]

Yes, because with #[pyo3(name = "foo")] it's immediately obvious the annotation is related to pyo3.

#[pyfunction(name = "foo")] as well as #[pyfunction] #[pyo3(name = "foo")]?

IMO it is much better when there is only one "proper" way to do things. When figuring out a crate, what do you prefer to read:

  • "You can rename your struct by using #[pyfunction(name = "foo")] or #[pyo3(name = "foo")]."
  • "You can rename your struct by using #[pyo3(name = "foo")]"

Consider also how confusing it is to read examples where either may be used.

As for simplification, I would probably prefer syntax where everything uses #[pyo3(...)] and they can be stacked or combined freely, similar to Serde.

These two could be equivalent:

#[pyo3(function)]
#[pyo3(rename = "foo")]
#[pyo3(pass_module)]
#[pyo3(signature = (a, b, c))]
fn pyfunction_form_2(_: &PyModule, a: i32, b: i32, c: i32) { }

or

#[pyo3(function, pass_module, rename = "foo", signature = (a, b, c))]
fn pyfunction_form_2(_: &PyModule, a: i32, b: i32, c: i32) { }

That would apply to everything, e.g. #[pyclass] -> #[pyo3(class)], #[pymodule] -> #[pyo3(module)].

This is also much nicer to write docs for, because there is less repetition.

@davidhewitt
Copy link
Member Author

TLDR; I'm thinking that having both forms is conveneient for now for backwards compatibility, however we should prefer #[pyo3(name = "...")] in documentation. Over several releases we can make deprecations to migrate users towards what we want for the final design.


Thanks for the feedback! Your #[pyo3(function)] proposal is very similar to #211. Though it seems a bit like repetition of fn / function. I would instead follow what #[wasm_bindgen] does and just use #[pyo3] - e.g.

#[pyo3]
fn foo() { }

would be equivalent to today's #[pyfunction].

Regardless of which form we take, this seems like a good argument to allow #[pyfunction(name = "...", ...)] for now, as migrating would be as simple as changing pyfunction( with either pyo3(function or just pyo3(.

Similarly if we eventually want to just have #[pyo3(name = "...")], then allowing #[pyfunction] #[pyo3(name = "...")] for now is also easy to migrate (just delete #[pyfunction]).


If the above is what we're aiming for, then we need to consider the migration story for users from the current syntax. Over several releases we can make deprecations to gently nudge users towards what we want them to write.

At the moment we have

#[pyfunction(pass_module, a, b, c)]
#[name = "..."]
#[text_signature = "..."]

After this first round of deprecations we might have led users to rewrite this as

#[pyfunction(pass_module, a, b, c)]
#[pyo3(name = "...", text_signature = "...")]

I think also we would not document it as

You can rename your struct by using #[pyfunction(name = "foo")] or #[pyo3(name = "foo")].

but instead only document the #[pyo3] form. We can make a small note somewhere saying that for backwards compatibility we also allow the #[pyfunction] form. In all documentation and examples we would have this form:

#[pyfunction]
#[pyo3(name = "...", text_signature = "...", pass_module, signature = (a, b, c))]

And then in a final step later we could deprecate #[pyfunction] in favour of #[pyo3(function)] or #[pyo3], as per above.

@mejrs
Copy link
Member

mejrs commented May 2, 2021

Though it seems a bit like repetition of fn / function. I would instead follow what #[wasm_bindgen] does and just use #[pyo3]

That is even nicer 👍. I agree with everything you said.

@cmyr
Copy link
Contributor

cmyr commented May 3, 2021

I think the consistency is an improvement, and I think that avoiding bare identifiers like #[name(XXX)] is good practice because it's possible for multiple derives to be present, and some sort of namespace keeps intention clear.

There's one thing I haven't seen mentioned in this issue, though, which is the possibility of using #[py(XXX)] as the name space for these attributes, instead of #[pyo3(XXX)]? This is a very small change, but I think personally I would prefer it, unless there's some clear issue I'm not considering. I wouldn't worry particularly about the fact that py is the name of another crate; pyo3's position in the ecosystem is clear at this point, I think.

@davidhewitt
Copy link
Member Author

I think #[py(XXX)] would be nice, but there's already several #[pyo3(XXX)] attributes around and it's more important imo to stick with that.

Once we've got all these attributes consistent, if we wanted to rename from #[pyo3(XXX)] to #[py(XXX)] we could do one big bulk deprecation across the whole lot. But I think that it's only two characters so not hugely necessary for such a large ecosystem churn.

@davidhewitt
Copy link
Member Author

As all the feedback I've recieved on this has been neutral or positive, I'm going to merge this now and start making progress with the other items in the list in the OP.

We can always make further changes to this ahead of 0.14 if we decide against some of the design. Thanks all for commenting!

@davidhewitt davidhewitt merged commit cc68f4a into PyO3:main May 11, 2021
@davidhewitt davidhewitt mentioned this pull request May 11, 2021
7 tasks
@davidhewitt davidhewitt deleted the attribute-tidy branch December 24, 2021 02:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

pass_module interacts badly with pyfunction arguments
4 participants