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

RFC: scheduled_removal Parameter for deprecated Attribute #3471

Open
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

HTGAzureX1212
Copy link

@HTGAzureX1212 HTGAzureX1212 commented Aug 13, 2023

Rendered

This RFC proposes the addition of a scheduled_removal parameter to the deprecated attribute to optionally specify when the deprecated item will be removed from the public API, making it easier for users of libraries to prepare for migration beforehand.

Prior Discussion on Rust Internals

Copy link

@illicitonion illicitonion left a comment

Choose a reason for hiding this comment

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

In general I think this is a really reasonable feature.

What should happen when the target version is reached, if the code with the #[deprecated] attr hasn't been removed? Do we want to allow this situation at all? Do we want to make it an error? Do we want the error to be ignorable?

I think there are two perspectives to consider:

The library maintainer

This should maybe be an error-by-default lint. So if you have this code:

#[deprecated(since = "0.2.1", scheduled_removal = "0.3.0")]
struct ThisItemIsDeprecated;

and the Cargo.toml gets bumped to 0.3.0, this should probably error, something like:

error: deprecated item not removed after its scheduled removal:
[code snippet]
note: this struct was scheduled for deletion in 0.3.0

The onus being on the crate maintainer to either delete the code, or bump its scheduled removal. This has two purposes:

  1. To raise the inconsistency with the crate maintainer and help them detect work they still need to do
  2. To avoid giving a confusing error message to a consumer after release ("What do you mean this is scheduled for removal in 0.3.0, I'm using 0.3.0!")

The consumer

(This only applies if we don't make it a hard error to have a scheduled_removal version in the past)

If the code isn't removed by the scheduled version, and the scheduled_removal isn't updated, what should the experience be for the consumer of the crate?

The Pants project (which is written in Python) has a really nice deprecation system in place whereby there is a similar scheduled_removal attribute, and if the current version is newer than the scheduled removal version, a runtime exception is throw telling you that you're using an API which was scheduled for removal, and telling you how its usage was meant to be replaced. This is a runtime error, because it's Python, but in a Rust world I can imagine something like upgrading the default warn to a default error if you try to use a deprecated API which is past its scheduled removal?

So if you're still using ThisItemIsDeprecated when you've bumped your dep to 0.3.0, you'll get the same lint triggering, but at error level rather than warning level. This is more forceful (because the library told you you really should've acted by now), but allows a hook to still show the deprecation message which may suggest useful action to the maintainer of the consuming crate. Concretely, this is the difference between the error message "No such type ThisItemIsDeprecated" and the error message "ThisItemIsDeprecated has been removed from this library, consider using ThisShinyNewReplacement instead`".

@HTGAzureX1212
Copy link
Author

HTGAzureX1212 commented Aug 13, 2023

What should happen when the target version is reached, if the code with the #[deprecated] attr hasn't been removed? Do we want to allow this situation at all? Do we want to make it an error? Do we want the error to be ignorable?

I consider this to be an error and should not be allowed. The error would not be ignorable. The library maintainer has to either remove the deprecated code or bump the scheduled removal version.

@ehuss ehuss added the T-lang Relevant to the language team, which will review and decide on the RFC. label Aug 13, 2023
@jhpratt
Copy link
Member

jhpratt commented Aug 13, 2023

Honestly, I don't see the need for this. Why wouldn't a deprecated item be expected to be removed in the next breaking change?

With regard to enforcing the version, I will note that the since version is not enforced in any way, and can even be an arbitrary string.

@VitWW That's #![feature(deprecated_suggestion)].

@mathstuf
Copy link

Why wouldn't a deprecated item be expected to be removed in the next breaking change?

If it has sufficient history or is particularly gnarly to port from, it may make sense to keep it for more than one deprecation cycle. Holding up other removals for some other large deprecation doesn't sound like a great requirement either.

Alternatively, it may be that it is made API-inaccessible but continues to be provided in the binary for ABI compatibility (e.g., a C API to a Rust crate).

@jhpratt
Copy link
Member

jhpratt commented Aug 14, 2023

@mathstuf I'm referring to it from a user's perspective. The library maintainer may want to keep it around for that reason, but a downstream user should always expect it to be removed imo.

@mathstuf
Copy link

Discussion on Discourse seemed to prefer (though there wasn't much discussion) wording in terms of the author of the attribute. The generated diagnostic messages can be worded in a way that is more targeting the consumer directly.

@HTGAzureX1212
Copy link
Author

HTGAzureX1212 commented Aug 14, 2023

I'm referring to it from a user's perspective. The library maintainer may want to keep it around for that reason, but a downstream user should always expect it to be removed imo.

@jhpratt Yes, I agree with the part that it will be removed, but being specific on which version would it be removed is still a good idea both for library authors and users. Deprecated items are not always removed in the next major version - it's just that it may be removed in a future version, and it is optional whether a version is given, as outlined in this RFC.

@epage
Copy link
Contributor

epage commented Aug 14, 2023

I'll go a step further: I think a user-provided fully-specified version for scheduled removal may give developers the wrong impression that it is fine to remove API items in the middle of a release which would be damaging to the culture of semver within the Rust community which could break the technical reliance on semver.

@mathstuf
Copy link

Can't there be a diagnostic for "planned removal within a semver compatibility bucket"?

@epage
Copy link
Contributor

epage commented Aug 14, 2023

Can't there be a diagnostic for "planned removal within a semver compatibility bucket"?

The main use case I can think of to remove an item within a semver-compatible release is if its a #[doc(hidden)] item used for macros exported by the package and the deprecation is for a preponderance of caution in case someone is using the internal-but-public API.

In my mind, that is minor enough of a use case to not justify supporting it.

@epage
Copy link
Contributor

epage commented Aug 14, 2023

@jhpratt Yes, I agree with the part that it will be removed, but being specific on which version would it be removed is still a good idea both for library authors and users. Deprecated items are not always removed in the next major version - it's just that it may be removed in a future version, and it is optional whether a version is given, as outlined in this RFC.

imo the "remove in next major breaking" is the most likely case and so should be the easiest.

Personally, I would do absence of the attribute to mean "no removal" (like for the stdlib) and attribute-without-value to opt-in with a default that is Inferred from since. Users would then need to explicitly set a value if they are wanting something later.

@veber-alex
Copy link

veber-alex commented Aug 14, 2023

I am not sure I see the added value of this over just doing #[deprecated(since = "3.8", note = "will be removed in version 4.0")]

@HTGAzureX1212
Copy link
Author

I am not sure I see the added value of this over just doing #[deprecated(since = "3.8", note = "will be removed in version 4.0")]

I interpret the "note" field for providing an alternative API to use and/or why this item is deprecated, rather than specifying when would this be removed.

The method as proposed in this RFC, albeit a longer way to express a scheduled removal in the future, is clearer in a sense that it is written explicitly.

@dlight
Copy link

dlight commented Aug 15, 2023

@jhpratt it's still useful to communicate clearly when and if something deprecated is expected to be removed, rather than let the users guess.

And here I say "if" because some deprecated items are expected to never ever be removed. For example, deprecated items in Rust's stdlib, and maybe in other libraries. In those cases, deprecation just means that people should stop using that API, but no breakage would ensue. Maybe this should be communicated with #[deprecated(since = "0.2.1", scheduled_removal = "never")]

So I think that the user can't infer anything from the absence of a scheduled_removal attribute.

@jhpratt
Copy link
Member

jhpratt commented Aug 15, 2023

@dlight Why does it need to be separate from the note?

@HTGAzureX1212 As previously mentioned, there is #![feature(deprecated_suggestion)] to indicate the replacement.

@HTGAzureX1212
Copy link
Author

@jhpratt If we have something to indicate the replacement, why can't we have something to indicate when a deprecated item is to be removed?

@jhpratt
Copy link
Member

jhpratt commented Aug 15, 2023

The replacement is machine applicable, which requires compiler integration. Indicating when something will be removed does not require that.

@HTGAzureX1212
Copy link
Author

The replacement is machine applicable, which requires compiler integration. Indicating when something will be removed does not require that.

Using the note parameter IMO is not clear enough. Additionally having an extra parameter would allow for better diagnostics and a much better way to handle deprecation as mentioned above. Simply putting that in the note parameter does not "force" a library author to remove a deprecated item on that version and is not ideal IMO.

@jhpratt
Copy link
Member

jhpratt commented Aug 15, 2023

Can you demonstrate how the diagnostics would be improved from the existing situation? I find the existing diagnostic extremely clear. The only difference I see is that the note is on a separate line. This could be accomplished by simply turning the existing "note" field into a diagnostic note. That would be trivial to implement and would not require an RFC.

does not "force" a library author to remove a deprecated item on that version

Nor does your proposal. It is impossible to force a removal. While you may think that the new field can be enforced (of which there is no detail in the RFC), the author can trivially change or remove the version with no consequence.

@HTGAzureX1212
Copy link
Author

HTGAzureX1212 commented Aug 15, 2023

Can you demonstrate how the diagnostics would be improved from the existing situation? I find the existing diagnostic extremely clear. The only difference I see is that the note is on a separate line. This could be accomplished by simply turning the existing "note" field into a diagnostic note. That would be trivial to implement and would not require an RFC.

does not "force" a library author to remove a deprecated item on that version

Nor does your proposal. It is impossible to force a removal. While you may think that the new field can be enforced (of which there is no detail in the RFC), the author can trivially change or remove the version with no consequence.

At least it is a hard error not removing a deprecated item scheduled to be removed in a version (will update RFC reflecting this), rather than just a simple warning.

Edit: the RFC has been updated.

@ssokolow
Copy link

ssokolow commented Aug 17, 2023

Nothing the compiler does can "force" the developer to do anything. That's why things like .clone() and Rc<T> exist, and abusing unsafe to produce code the compiler doesn't notice is invoking Undefined Behaviour is always a possibility.

The point is that having an explicit scheduled_removal would enable a "Please either bump the removal milestone or remove the code" error message without the spooky action of having deprecated(since = "0.2.1" silently introduce a potentially surprising build failure at 0.3.0.

Not to mention, there are already enough people who are rubbed the wrong way with how it's rustc, not Clippy, that's opinionated about stylistic things like variable and constant naming conventions. Baking an opinion on deprecation policies into the toolchain wouldn't go over well, so scheduled_removal would be necessary if you want the build process to report stuff that's overdue for removal.

Heck, I could easily imagine a project which has some kind of LTS-esque policy that something that's deprecated at 0.2.x gets removed in 0.4.0, with 0.3.0 being a breaking change because it's removing things that got deprecated in 0.1.x and possibly changing some function signatures without removing them.

@obi1kenobi
Copy link
Member

In general, I think having more structured data available when possible. Even if rustc diagnostics don't change at all, having a dedicated field for this information makes it possible and easier to build custom tools that support maintainers' workloads. Such tools could have parsed the note field to extract the scheduled removal date, but that's a high barrier to entry — and we all know that lowering the barrier to entry makes good things happen.

To my naive eyes, this seems like a change with limited scope, high upside, little downside, and sound precedent in other ecosystems. That makes me in favor of it.

Just my 2 cents. Of course, it's entirely possible I've missed some glaring issue.

@tgross35
Copy link
Contributor

I am not sure I see the added value of this over just doing #[deprecated(since = "3.8", note = "will be removed in version 4.0")]

I interpret the "note" field for providing an alternative API to use and/or why this item is deprecated, rather than specifying when would this be removed.

I believe there is an open (or approved but unstable?) RFC for an attribute to suggest an alternative


To design one wall of the bikeshed, if this is accepted I would prefer the attribute be called removal. I don't think the scheduled_ prefix adds much to the understanding of removal = "1.2.3" and it just feels a bit clunkier than the simple since and note attributes.

@ssokolow
Copy link

To design one wall of the bikeshed, if this is accepted I would prefer the attribute be called removal. I don't think the scheduled_ prefix adds much to the understanding of removal = "1.2.3" and it just feels a bit clunkier than the simple since and note attributes.

As long as a removal version <= to the current version causes a compile-time error, that makes sense.

Otherwise, you could have it be "in the past" and the "scheduled" clarifies that it was the intended removal but not necessarily the actual one.

@HTGAzureX1212
Copy link
Author

To design one wall of the bikeshed, if this is accepted I would prefer the attribute be called removal. I don't think the scheduled_ prefix adds much to the understanding of removal = "1.2.3" and it just feels a bit clunkier than the simple since and note attributes.

As long as a removal version <= to the current version causes a compile-time error, that makes sense.

Otherwise, you could have it be "in the past" and the "scheduled" clarifies that it was the intended removal but not necessarily the actual one.

Yes, it is indeed a hard-error as suggested by the RFC as of its latest revision, I would believe that also makes sense to remove the scheduled_ prefix.

Indeed, that name does feel a bit more chunkier than the other shorter attribute parameters, and perhaps that may not be ideal over the general "scene" of naming things.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-lang Relevant to the language team, which will review and decide on the RFC.
Projects
None yet
Development

Successfully merging this pull request may close these issues.