Add `#[pyo3(deprecated(...))]` to `#[pyfunction]` (#4316)

[FlickerSoul]

This PR implements #[pyo3(deprecated(message = "...", category = ...))] attribute for #[pyfunction] and #[pymethods]. The new attribute works like the following

#[pyfunction]
#[pyo3(deprecated(message = "this is a deprecated function"))]
fn deprecated_function() {
    // the execution of this function will allow user to see 
    // DeprecationWarning: this is a deprecated function
}

#[pyfunction]
#[pyo3(deprecated(message = "this is a deprecated function", category = PyFutureWarning))]
fn deprecated_function() {
    // and with a specified category
    // PyFutureWarning: This is a warning message
}

Note that this function does not deprecate rust function. Programmers need to add #[deprecated] attribute explicitly.

Ref: (#4316)

[FlickerSoul]

Thanks for the detailed feedback! I'll work on these points!! :)

Regarding documenting this attribute for #[pymethods], I remember the document refers back to #[pyfunction]'s options, so maybe putting it there would be enough?

I'll add hygiene test for coverage a bit later. :)

[FlickerSoul]

• Reading this, it feels like this is really just a general mechanism for raising warnings, not necessarily deprecation warnings. Should we maybe just call it pyo3(warn)? 🤔

The name pyo3(warn) seems to make more sense! I also suggested this in the original issue but I probably didn't make that clear :)

• Should the message be optional? It seems like https://docs.python.org/3/library/warnings.html#warnings.warn allows this.

The signature of warn is warnings.warn(message, category=None, stacklevel=1, source=None, *, skip_file_prefixes=None) so I think message is required.

If we are making the attribute named pyo3(warn), should we also parse for stacklevel in the attribute, because PyErr::warn_bound (internally PyErr_WarnEx) accepts it optionally?

I'll work on additional testing and the compile error, and wait for more opinions on what this attribute should be called :)

[Icxolu]

I like the idea of #[pyo3(warn), but in that case I think we should match Python and default to UserWarning. Maybe we want to add a shortcut like #[pyo3(deprecated = "msg")] that would match #[pyo3(warn(message = "msg", category = "DeprecationWarning"))]?

[davidhewitt]

Reading this, I also think the general mechanism #[pyo3(warn)] seems like the right choice.

Probably the category should be a path to a Rust type, i.e. #[pyo3(warn(message = "msg", category = PyDeprecationWarning))]

Having been away from the discussion for a while, did we decide strongly that we don't want to support the Rust #[deprecated] message? My intuition is that it's still extremely convenient and intuitive to have #[deprecated(note = "foo")] to automatically set #[pyo3(warn(message = "foo", category = PyDeprecationWarning)].

In the (I think rare) case where users want a Rust deprecation but not a Python one, we could always have #[pyo3(warn(false))].

Finally, a style question. I think that #[pyo3(warn(message = "..."))] is the first time that we've had parenthesised pyo3 options. Should we consider e.g. #[pyo3(warn = (message = "..."))] or another syntax instead? I think warn(...) is probably the right choice as we see it elsewhere in the Rust ecosystem, but I'd at least like to make sure we all are happy before we add a new style of attribute to the library.

[davidhewitt]

Maybe we want to add a shortcut like #[pyo3(deprecated = "msg")] that would match #[pyo3(warn(message = "msg", category = "DeprecationWarning"))]?

Personally I'd want this shorthand to just be the Rust #[deprecated(note = "msg")] as per my comment above.

[FlickerSoul]

That sounds good! I will refactor the current one to warn and make a new deprecated to be a shortcut of DeprecationWarning :)

[Icxolu]

Having been away from the discussion for a while, did we decide strongly that we don't want to support the Rust #[deprecated] message? My intuition is that it's still extremely convenient and intuitive to have #[deprecated(note = "foo")] to automatically set #[pyo3(warn(message = "foo", category = PyDeprecationWarning)].

There was some discussion about that in #4316. I kind of agree with the points made there, that reusing the #[deprecated] attribute is not such a good idea.

• we loose control about syntax and options
• deprecating only Rust / only Python get harder
• deprecating both, but using a different category for Python gets harder
• using different messages for Rust and Python would not be possible
• it starts to blur the line of what only effects Rust, and what does have an effect on Python as well. #[must_use] would be an example that does not have any effect on Python, but could still occur on a #[pyfunction] or #[pymethod].
• It would also be the only option that's not configured through #[pyo3(...)]

I think the most straight forward solution without additional complexity or "magic" is to stick to our options system, possibly with an option, opt-in or opt-out, to also emit the #[deprecated]. attribute.

Finally, a style question. I think that #[pyo3(warn(message = "..."))] is the first time that we've had parenthesised pyo3 options. Should we consider e.g. #[pyo3(warn = (message = "..."))] or another syntax instead? I think warn(...) is probably the right choice as we see it elsewhere in the Rust ecosystem, but I'd at least like to make sure we all are happy before we add a new style of attribute to the library.

I think this (#[pyo3(warn(message = "..."))]) is fairly common syntax. The #[pyo3] attribute itself uses that syntax, so having it for nested options as well, makes sense to me.

[davidhewitt]

That makes sense. I still think that most commonly users will want to deprecate both sides at the same time and so tying both together with Rust's #[deprecated] is convenient for the average case. However I am happy to cede my opinion as the minority here.

Let's go with #[pyo3(deprecated = "...")] shorthand in that case, and keep it Python only.

[FlickerSoul]

I still think that most commonly users will want to deprecate both sides at the same time and so tying both together with Rust's #[deprecated] is convenient for the average case.

I think the most straight forward solution without additional complexity or "magic" is to stick to our options system, possibly with an option, opt-in or opt-out, to also emit the #[deprecated]. attribute.

I agree with both. I proposed in the issue that we could use something like #[pyo3(deprecated = "...", python_only)], if we want to make #[pyo3(deprecated = "...")] deprecate both sides. Though, I can't think of a good name other than python_only.

In this way, users don't need to duplicate their deprecation note twice if they want to deprecate both sides. They also have the control of which side of deprecation they'd like to apply on. This seems to be a win-win for both sides.

[FlickerSoul]

Hi all!

The #[pyo3(warn(message = "...", category = ...))] and #[pyo3(deprecated = "...")] should have been implemented properly.

The warn attribute now behaves like the warnings.warn and emits UserWarning when category is absent.

The deprecated attribute currently only deprecates Python side. If we want to allow deprecated to deprecate both sides and add an opt-out option, like the #[pyo3(deprecated = "...", python_only)], I can add it later this week.

Please let me know if I missed anything. Thanks!

Best regards, L

[FlickerSoul]

I saw that you opened this to support multiple warnings. I currently don't see a big problem with that. It does however interact with automatic Rust deprecation since we currently could have multiple #[pyo3(deprecated)], but only one #[deprecated] attribute is allowed per item, so we should be aware of the consequences before committing to anything here. Let's see what the others think of this.

I agree we should be cautious on the multiplicity of the same attribute. I'm also curious what others think of this. :)

[FlickerSoul]

Clippy fails on x86_64 linux target surprisingly, because of

  error: struct `PyFunction` is never constructed
     --> src/types/function.rs:184:12
      |
  184 | pub struct PyFunction(PyAny);
      |            ^^^^^^^^^^
      |
      = note: `-D dead-code` implied by `-D warnings`
      = help: to override `-D warnings` add `#[allow(dead_code)]`

It seems that PyFunction is used with ffi and is not intended to be constructed manually. Should I mark it with #[allow(dead_code)]?

[birkenfeld]

Clippy fails on x86_64 linux target surprisingly

Only on beta though. I wonder if that's a newly introduced rustc bug?

[FlickerSoul]

Only on beta though. I wonder if that's a newly introduced rustc bug?

Looks like so?

The check had been passing until this one. Could you maybe rerun the check to see if the error goes away?

[Icxolu]

This is just dead code detection getting better 🎉. It also happens on main. The reason is our reexport of PyFunction https://github.com/PyO3/pyo3/blob/b2b5203ca4fb103e84f1ae8a267ea2d2912d5307/src/types/mod.rs#L25-L26 has a more restrictive gate than its type definition https://github.com/PyO3/pyo3/blob/b2b5203ca4fb103e84f1ae8a267ea2d2912d5307/src/types/function.rs#L183-L184 So that type is actually not nameable on PyPy 3.8+ but it was actually intended to be.

You don't have to worry about it here, we can fix that separately. I'll file a PR shortly, #4375.

[FlickerSoul]

Awesome!! Thanks!

[miikkas]

Thanks, this looks great! One question: Would adding the __deprecated__ attribute to the Python object (as per PEP 702) be out of scope? It's going to be available (implemented here) in Python 3.13, so technically it's usefulness would be quite limited until October this year.

[FlickerSoul]

@miikkas That's a great suggestion! If I understand the PEP 702 correctly, __deprecated__ is mainly type checkers. I would open a separate PR for that as the changes in this PR are getting bigger.

@Icxolu All requested changes have been made and thanks for the detailed feedback! I also rebased on main to resolve the clippy error. It looks like nightly compiler is also getting better and emits new errors. I can open a separate PR to fix it. :)

I also see that coverage is not hitting the target. They are mainly caused by functions in the ToTokens trait of the attribute struct and a couple helper functions to get Span, required by others parts of code. The functions are only called when errors are raised and it looks like test_compile_error won't help with the coverage. Could you suggest a place to add tests for those?

Would it be helpful if I squash the commits now, because this thread is getting long?

[Icxolu]

All requested changes have been made and thanks for the detailed feedback! I also rebased on main to resolve the clippy error.

@FlickerSoul Thanks, implementation and tests look good to me. I don't think we need to worry too much about CodeCov not hitting these specific paths.

It looks like nightly compiler is also getting better and emits new errors. I can open a separate PR to fix it. :)

Please feel free to do so 😄

Would it be helpful if I squash the commits now, because this thread is getting long?

I leave that up to you, we have squash merging enabled anyway, so from that perspective it does not really matter. It stretches the thread a bit, but depending on your workflow the commits could be useful if we device to take a step back, more on that below.

Thanks, this looks great! One question: Would adding the __deprecated__ attribute to the Python object (as per PEP 702) be out of scope?

@miikkas Thanks for bringing this to our attention. I can definitely see us supporting that. However I agree with @FlickerSoul that we should postpone that to a followup. This means we should carefully evaluate what we can and want to guarantee in this first base implementation.

From my reading PEP 702 talks specifically about deprecations, and not warnings in general. This could force us to diverge #[pyo3(warn)] and #[pyo3(deprecated)] in the future, since matching on item names (in this case the category) from macro code is usually not a good idea. This changes a bit what we want to guarantee here about the equivalence between them.

This together with a possibility of automatic Rust #[deprecated] makes me kind of lean towards only supporting a single warning (or at least only a single #[pyo3(deprecated)] xor multiple #[pyo3(warn)]) for now. (Multiple #[pyo3(deprecated)] would not make a lot of sense anyway IMHO) We can always open it up later once we put more design work into either or both of these directions.

cc @davidhewitt A second opinion here would be appreciated 🙃

[FlickerSoul]

This together with a possibility of automatic Rust #[deprecated] makes me kind of lean towards only supporting a single warning (or at least only a single #[pyo3(deprecated)] xor multiple #[pyo3(warn)]) for now. (Multiple #[pyo3(deprecated)] would not make a lot of sense anyway IMHO) We can always open it up later once we put more design work into either or both of these directions.

I agree multiple #[pyo3(deprecated)] wouldn't make sense and I like the idea of allowing the ability to use multiple #[pyo3(warn)]. Would "at least only a single #[pyo3(deprecated)] (logical) or multiple #[pyo3(warn)]" be a viable option? It seems to me this way we give users more choices without going to trouble.

I'll wait on @davidhewitt's opinion before going for the implementation. :))

This could force us to diverge #[pyo3(warn)] and #[pyo3(deprecated)] in the future, since matching on item names (in this case the category) from macro code is usually not a good idea. This changes a bit what we want to guarantee here about the equivalence between them.

This makes sense. I can't think of a good API for this off the top of my head. Maybe David can shed some light on this as well :))

Again, thanks for the feedback. Much appreciated!

[mejrs]

My apologies for the delay. To me personally the PR looks good and I'd like to merge it. @Icxolu was your review addressed?

Finally, can you please resolve the merge conflicts?

[FlickerSoul]

Hey @mejrs, thanks for getting back to me. I vaguely remember the discussion was not fully addressed. I'll take a look and resolve the merge conflicts in the next couple of days. :)

[Icxolu]

Thanks for bringing this up again @mejrs. I think implementation wise this looked good to me as well. I think the last discussion point was the guarantee about the equivalency of #[pyo3(deprecated(...))] and `#[pyo3(warn(message = "...", category = PyDeprecationWarning))]``. If we want to support PEP 702 in the future, we should remove this guarantee from the docs, and maybe even emphasize that they may not be equivalent. Alternatively we could also split this, land the general warning mechanism first, and add specialized deprecations later.

[mejrs]

Alternatively we could also split this, land the general warning mechanism first, and add specialized deprecations later.

That sounds good to me. I propose we remove #[pyo3(deprecated)] from this pr, and just do #[pyo3(warn)] for now.

[FlickerSoul]

Sounds good. I'll try to get back to you by this week. :)

[FlickerSoul]

Hi @mejrs and @Icxolu,

I removed the #[pyo3(deprecated)] from this PR and updated the code and doc to only have #[pyo3(warn)].

I noticed that the current #[pyo3(deprecated)] only supports the first two arguments of the warn function in warnings, namely message and category. Do you think if we want to add support for the rest three arguments: stacklevel, source and skip_file_prefixes?

Additionally, the UI tests are failing for Ubuntu but pass on my Mac. What should I do about those?

[mejrs]

Can you edit the original post of this PR too? To avoid confusing future readers?

I noticed that the current #[pyo3(deprecated)] only supports the first two arguments of the warn function in warnings, namely message and category. Do you think if we want to add support for the rest three arguments: stacklevel, source and skip_file_prefixes?

I'd err on the side of not having this. It keeps the documentation simple and we can always add it later. I don't think any of them are particularly useful (from native code, anyway) and users can always just raise warnings manually if they really need it.

Additionally, the UI tests are failing for Ubuntu but pass on my Mac. What should I do about those?

It's just error output from the compiler that has changed, you should update to the latest stable compiler. And bless the output, with the environment variable TRYBUILD=overwrite

[davidhewitt]

For sake of merging this before #5107, I'll push a fix for the ui tests now.

[davidhewitt]

... actually I'm not able to do so (looks like permissions have not been given for maintainers to push to the branch).

[FlickerSoul]

Hi all, sorry for the delay. I'll push a fix to the UI tests and resolve the conflicts today :))