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)

[Icxolu]

Thank you very much! I think this looks really good already. Just left some early thoughts while skimming through the macro code.

[Icxolu]

Implementation wise this looks good to me. I think for testing we still need

• #[pymethods] tests, probably in test_methods.rs
• coverage in the hygiene tests
• I think this also deserves a pytest

For the docs, we might want to say somewhere that this is also available to #[pymethods], but I haven't seen a good place for that yet myself.

[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. :)

[mejrs]

Thanks, this looks good. A couple of thoughts:

• 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)? 🤔
• Should the message be optional? It seems like https://docs.python.org/3/library/warnings.html#warnings.warn allows this.

Fwiw I'm not sure about either. I'd welcome your and anyone else's thoughts

[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

[Icxolu]

Thanks! This mostly looks good to me. Just some minor things here and there.

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. This would currently be the only pyo3 option that would be allowed multiple times (I think), so we should document that behavior somewhere if we decide to keep it.

I think we should add one or two of these attributes to the tests/hygiene tests, to make sure all the emitted code is fully qualified.

Also it should be possible to use a user defined warning category that extends from PyWarning. Maybe we should add a test to make sure that works?

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.

I'm open to this, but I would suggest if we decide to go down that route we should do it as a followup.

[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

pyo3/src/types/mod.rs

Lines 25 to 26 in b2b5203

	#[cfg(all(not(Py_LIMITED_API), not(PyPy), not(GraalPy)))]
	pub use self::function::PyFunction;

has a more restrictive gate than its type definition

pyo3/src/types/function.rs

Lines 183 to 184 in b2b5203

	#[cfg(all(not(Py_LIMITED_API), not(all(PyPy, not(Py_3_8)))))]
	pub struct PyFunction(PyAny);

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 🙃

[Icxolu]

Do we want to guarantee that these are equivalent? Especially with Pep 702 support, which we might only be able to properly provide for #[pyo3(deprecated = "...")]. Maybe we should formulate this a bit more loosely to give us some wiggle room in a future where these might need to diverge a bit? cc @davidhewitt

[FlickerSoul]

Yeah I agree this should be changed. I wrote this before reading the PEP 702. The semantics of deprecated in the PEP doesn't seem to match what I'm promising here. Using the same name thus could cause future confusion.

[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!