Add lint rule for #[deprecated] on re-exports (rebase) by GrigorenkoPV · Pull Request #142731 · rust-lang/rust (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation15 Commits3 Checks10 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
Rebase of #132038
Original description:
As reported in #30827 and #84584, marking a re-export (
pub use) with#[deprecated]does not produce a warning for consumers. In fact, there are instances of this in thecoreandstdcrates (see past issue #82080 where this caused some confusion).This change modifies the stability annotation visitor to mark
#[deprecated]annotations onusestatements withAnnotationKind::DeprecationProhibitedso that library developers are aware that the annotation is not warning their users as expected.#[deprecated] pub use a_module::ActiveType;
error: this `#[deprecated]` annotation has no effect --> $DIR/deprecated_use.rs:6:1 | LL | #[deprecated] | ^^^^^^^^^^^^^ help: remove the unnecessary deprecation attribute | = note: `#[deny(useless_deprecated)]` on by default error: aborting due to 1 previous error
Closes #142436 (I think)
rustbot has assigned @WaffleLapkin.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.
Use r? to explicitly pick a reviewer
Status: Awaiting review from the assignee but also interested parties.
Relevant to the compiler team, which will review and decide on the PR/issue.
Relevant to the library team, which will review and decide on the PR/issue.
labels
I strongly believe that this should just work. You should be able to deprecate reexports. I've talked with @jdonszelmann and we came up with a "plan" on how to implement that.
I'm marking this as blocked. Either until this is properly supported and the lint is not needed, or until we decide that we can't support it and the list is needed.
Status: Blocked on something else such as an RFC or other implementation work.
and removed S-waiting-on-review
Status: Awaiting review from the assignee but also interested parties.
labels
@WaffleLapkin if there is enough of a gap before a solution, would it make sense to go ahead and report that it has no effect until then so people know they are writing deprecated messages that will be ignored? The lint is literally describing the current behavior and it seems like it would be fine to later remove that lint when there is an effect.
@epage I'm not sure. I feel like adding a lint adds too much friction -- the warning will make people either remove the deprecation, or add an #[allow], both of which will need to be reverted eventually...
At the same time it will be some time before Jana and I can work on this and it will take some time to make a fix... So maybe a warning makes sense. 🤷🏻
In case this is relevant, the reason I thought it might be worthwhile is as a library author, I assume my deprecation messages show up and write my library and my changelog assuming that. As a recent example (but with #47237), I had someone complaining about unspecified breaking changes in a library of mine because I had thought my umbrella statement "deprecated items were removed" was sufficient when in reality, they existed only in the code and users never saw them. If I had known about #47237, I would have worked around it by writing wrapper functions where possible like I do for #30827 which I mostly know of because of the number of times it has bitten me.
I see your point. I agree, for users who don't know about this footgun (I assume most users don't), it would be nice to warn them. After I thought about it for a couple days, I think I'd be in favor of adding this warning.
As a recent example (but with #47237), I had someone complaining about unspecified breaking changes in a library of mine because I had thought my umbrella statement "deprecated items were removed" was sufficient when in reality, they existed only in the code and users never saw them. I would have worked around it by writing wrapper functions where possible like I do for #30827 which I mostly know of because of the number of times it has bitten me.
This exact sequence of events just happenned to me, except it was thankfully caught in code review: https://github.com/PyO3/pyo3/pull/5642/changes#r2614462301. If there was a lint I would have noticed the issue and written a wrapper function instead.
@WaffleLapkin maybe remove the blocked label, given the conclusion you came to in #142731 (comment), that way this can be rebased and merged.
Comment on lines -13 to -17
| #[deprecated( |
|---|
| since = "1.52.0", |
| note = "Name does not follow std convention, use LayoutError", |
| suggestion = "LayoutError" |
| )] |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Leave a fixme here
Comment on lines +46 to +49
| // Deprecation does not work on re-exports, so let's be clear about that. |
|---|
| // https://github.com/rust-lang/rust/issues/142436 |
| #[deprecated] //~ ERROR this `#[deprecated]` annotation has no effect |
| pub use inner::Y; |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This really shouldn't be an error. We want to support this in the future, so this should at most be a lint saying "... this is a long-standing bug"
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Possibly also with suggestions on how to workaround this
| @@ -0,0 +1,12 @@ |
|---|
| //@ run-rustfix |
| #![deny(unused_imports)] |
| #![allow(useless_deprecated)] |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| #![allow(useless_deprecated)] |
|---|
| #![expect(useless_deprecated)] |
Status: This is awaiting some action (such as code changes or more information) from the author.
label
Reminder, once the PR becomes ready for a review, use @rustbot ready.
Status: Blocked on something else such as an RFC or other implementation work.
label
Labels
Status: This is awaiting some action (such as code changes or more information) from the author.
Relevant to the compiler team, which will review and decide on the PR/issue.
Relevant to the library team, which will review and decide on the PR/issue.