Skip to content

Refactor deprecation attributes #82432

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

Closed
wants to merge 1 commit into from
Closed

Conversation

bstrie
Copy link
Contributor

@bstrie bstrie commented Feb 23, 2021

The existing handling of deprecation attributes has grown organically to encompass three different meanings over two distinct attributes, and has become pretty messy as a result. This PR cleans up the handling of deprecated attributes:

  • Inspired by the existing rustc_attr::StabilityLevel, replace the Deprecated struct with a DeprKind enum for the two deprecated attributes: the stable deprecated and the unstable rustc_deprecated. This allows us to eliminate some unnecessary defensive checks regarding the fields present in the stable attribute.
  • The RustcDeprecated variant contains a boolean field to indicate whether or not this will trigger the deprecated lint or the deprecated_in_future lint. The logic for determining this is moved from rustc_middle to rustc_attr. This also allows us to use the pre-existing parse_version logic that is private to rustc_attr.
  • While Deprecated still stores an opaque Symbol as its since field, RustcDeprecated now stores a Version. Because the StabilityLevel enum is closely related to DeprKind, the StabilityLevel::Stable variant now also stores a Version. Storing Version allows us to remove the custom comparison logic for a sanity check in rustc_passes (a test for this sanity check is also added). As a bonus, errors related to passing in invalid version strings now point to the offending attribute rather than to the decorated item.
  • Because Version is now being widely-used, it is exported as a public item. The sizes of its fields are also reduced from u16 to u8. Yes, I am aware of the irony.

@bstrie bstrie added C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Feb 23, 2021
@rust-highfive
Copy link
Contributor

r? @estebank

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 23, 2021
@rust-log-analyzer

This comment has been minimized.

@petrochenkov
Copy link
Contributor

cc @Mark-Simulacrum, IIRC you was the last person who refactored this previously.

@Mark-Simulacrum
Copy link
Member

So IIRC the last refactor here explicitly aimed to unify the two, rather than splitting them apart - most handling after parsing/validation should be able to treat them without needing to know which one it is. One of the reasons that I personally wanted to unify them is that some of the features currently available on the unstable attribute I'd like to see integrated into the stable attribute, so that other libraries can make use of it, and I think splitting the two into enum variants largely harms that.

It seems like parts of this PR could be kept regardless, but as you have everything in one commit that might be painful.

Other reviewers might also feel differently on whether this is a sufficient improvement to merit splitting rather than unifying.

@est31
Copy link
Member

est31 commented Feb 23, 2021

Because Version is now being widely-used, it is exported as a public item. The sizes of its fields are also reduced from u16 to u8. Yes, I am aware of the irony.

FTR at the current publishing scheme, 1.256.0 will be reached on Oct 21, 2044. But it doesn't appear in any public/stable API that can't be updated in time so it's not really a problem currently.

@bstrie
Copy link
Contributor Author

bstrie commented Feb 23, 2021

@Mark-Simulacrum

So IIRC the last refactor here explicitly aimed to unify the two, rather than splitting them apart - most handling after parsing/validation should be able to treat them without needing to know which one it is. One of the reasons that I personally wanted to unify them is that some of the features currently available on the unstable attribute I'd like to see integrated into the stable attribute, so that other libraries can make use of it, and I think splitting the two into enum variants largely harms that.

I disagree that this PR either splits apart or makes it harder to unify the two attributes. Rather, I'd say this PR makes it explicit how divided they already are, whereas before the divisions were more implicit. I'd argue that exposing these divisions makes it easier, not harder, to unify the two in the future where possible. That said, I also don't think we should consider it likely that these attributes will ever be fully (or even mostly!) unified, for the following reasons:

  1. deprecated's since and note fields are optional, whereas they are mandatory for rustc_deprecated. Unifying this would need a breaking change.
  2. deprecated's since field is totally unchecked, whereas rustc_deprecated's field must be validated. Unifying this would need a breaking change.
  3. Because deprecated's since field is opaque, giving it deprecated_in_future-style behavior can't be done without a breaking change.
  4. Because deprecated_in_future can't be done for deprecated, all the post-parsing/validation steps need to keep track of and understand the distinction between deprecated and rustc_deprecated.
  5. The rustc_deprecated attribute requires the presence of a stable attribute. This check has to happen after both attributes have already been parsed and validated. (This same late step is also when the validity of the since field matters.) I don't foresee any sort of stable staged API support ever landing, and even then unifying this would need a breaking change.
  6. deprecated only applies to the item it decorates, whereas rustc_deprecated applies recursively. This also necessitates differing logic post-parsing/validation. Expanding the behavior of deprecated here wouldn't necessarily be a breaking change, but it would be surprising and potentially unwelcome.

One thing worth emphasizing is that saying that there are just two attributes here kind of simplifies the situation; arguably there are actually three, thanks to how much unique handling deprecated_in_future requires. A prior version of this patch actually had a third enum variant dedicated to deprecated_in_future, but I eventually folded that into a bool on rustc_deprecated.

While we're on the topic, I think that the presence of deprecated_in_future creates a conspicuous absence here. To wit, rustc_deprecated gives the stdlib a warn-by-default lint, and deprecated_in_future gives the stdlib an allow-by-default lint. I think there's additional room for the stdlib to someday provide a deny-by-default lint, though of course it would have to rely on the edition system in order to be backwards-compatible; since editions are a rustc-only feature, that would imply another thing that is fundamentally unique to only one of the two attributes.

To reiterate, I think this PR will make it easier to further unify the two where possible. Any unified behavior can be implemented via inherent accessors, as this PR does for fn note() and fn suggestion(); with the previous implementation, keeping track of which attributes were allowed to do which things was IMO more ad-hoc. Readers seeing the explicit enum variants will automatically be put on alert that the behavior they're seeing isn't unified.

It seems like parts of this PR could be kept regardless, but as you have everything in one commit that might be painful.

Ah, I thought I was supposed to be squashing all my commits before submitting. If wouldn't mind splitting things out if there's demand.

@bstrie
Copy link
Contributor Author

bstrie commented Feb 23, 2021

FTR at the current publishing scheme, 1.256.0 will be reached on Oct 21, 2044. But it doesn't appear in any public/stable API that can't be updated in time so it's not really a problem currently.

Right, there's no need to future-proof this since it's neither a stable API nor a part of any stable ABI. There's no harm in only paying for what we'll need for the next 30 years.

@bors
Copy link
Collaborator

bors commented Feb 27, 2021

☔ The latest upstream changes (presumably #82448) made this pull request unmergeable. Please resolve the merge conflicts.

@JohnCSimon
Copy link
Member

@rustbot label: -S-waiting-on-review +S-waiting-on-author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 16, 2021
@Dylan-DPC-zz
Copy link

@bstrie can you resolve the conflicts? thanks

@bstrie
Copy link
Contributor Author

bstrie commented Mar 16, 2021

I'm happy to resolve the conflicts if people think this is a patch that they'd like to accept, but it does not seem as though reception has been overly enthusiastic so far. :) I originally split this patch off of the work I was doing for the proof-of-concept of rust-lang/rfcs#3088 , so unless people think this is likely to be accepted, then I'm more inclined to close this, and reopen only if the aforementioned RFC eventually gets accepted.

@bstrie bstrie closed this Mar 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-cleanup Category: PRs that clean code up or issues documenting cleanup. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.