-
Notifications
You must be signed in to change notification settings - Fork 13.7k
Support multiple stability attributes on items #131824
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
Conversation
r? @wesleywiser
rustbot has assigned @wesleywiser.
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
Some changes occurred in src/librustdoc/clean/types.rs
cc @camelid
These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.
If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.
This comment has been minimized.
This comment has been minimized.
ebf7852 to
14b8556
Compare
☔ The latest upstream changes (presumably #132020) made this pull request unmergeable. Please resolve the merge conflicts.
14b8556 to
6561424
Compare
☔ The latest upstream changes (presumably #132027) made this pull request unmergeable. Please resolve the merge conflicts.
6561424 to
0a730d9
Compare
☔ The latest upstream changes (presumably #131985) made this pull request unmergeable. Please resolve the merge conflicts.
0a730d9 to
86fac38
Compare
☔ The latest upstream changes (presumably #131349) made this pull request unmergeable. Please resolve the merge conflicts.
86fac38 to
88e33bf
Compare
Some changes occurred in src/tools/clippy
cc @rust-lang/clippy
cc @RalfJung @compiler-errors This rebase was pretty substantial and required some minor refactors. Would either of you mind giving the const-stability parts a look to make sure they make sense and don't go against planned future changes?
cc @rust-lang/libs-api I realize I should probably ping here as well. Does the design supporting a mix of unstable/stable attributes sound good? I'll try documenting it in the dev guide and std dev guide once that's finalized.
☔ The latest upstream changes (presumably #131284) made this pull request unmergeable. Please resolve the merge conflicts.
The PR should explain what the semantics are of having multiple unstable attributes. Do all gates need to be active to use the feature, or only some?
This supports multiple #[stable] attributes, and a mix of #[stable] and #[unstable], in case that's helpful for tracking which features library items depended on as they stabilize (and because it's less difficult to reword E0544 this way).
That sounds really strange. What is the semantics of this? And what is the point?
The PR should explain what the semantics are of having multiple
unstableattributes. Do all gates need to be active to use the feature, or only some?This supports multiple #[stable] attributes, and a mix of #[stable] and #[unstable], in case that's helpful for tracking which features library items depended on as they stabilize (and because it's less difficult to reword E0544 this way).
That sounds really strange. What is the semantics of this? And what is the point?
I've updated the PR description to address this. To your second point, I'm working off of this specification. Treating it as a matter of preference, I decided to implement it both with and without support for multiple #[stable] attributes in separate commits so that I'd have an implementation to point to before asking which approach is nicer. I'll work on addressing your review comments shortly. Thank you for giving this a look!
88e33bf to
bc6c3b0
Compare
The updated PR description is great, thanks. :)
Personally I don't think multiple stable attributes, or both stable and unstable, are worth it. But let's see what t-libs-api thinks.
☔ The latest upstream changes (presumably #132145) made this pull request unmergeable. Please resolve the merge conflicts.
bc6c3b0 to
5a1fd55
Compare
☔ The latest upstream changes (presumably #132435) made this pull request unmergeable. Please resolve the merge conflicts.
☔ The latest upstream changes (presumably #132831) made this pull request unmergeable. Please resolve the merge conflicts.
b9af36e to
4bbcf10
Compare
I did some cleanup to the diagnostic formatting and commit history. Now only the tests for stability attribute sanity (and the tests I added) have diffs. Hopefully it will be more digestible now, and easier to split apart if needed. Once changes to attribute handling (削除) and/or const-stability checks (削除ここまで) land it should hopefully be able to be cleaned up some more.
☔ The latest upstream changes (presumably #132954) made this pull request unmergeable. Please resolve the merge conflicts.
4bbcf10 to
814ab38
Compare
The updated PR description is great, thanks. :)
Personally I don't think multiple stable attributes, or both stable and unstable, are worth it. But let's see what t-libs-api thinks.
I agree that the cost/benefit ratio here is too high, from the compiler maintenance point of view.
It's not even a user facing feature, just a minor convenience for the library people.
☔ The latest upstream changes (presumably #132910) made this pull request unmergeable. Please resolve the merge conflicts.
6f28bda to
9138a57
Compare
☔ The latest upstream changes (presumably #133179) made this pull request unmergeable. Please resolve the merge conflicts.
This moves stability structs' `feature` fields into `StabilityLevel::Unstable` and `ConstStabilityLevel::Unstable`, in preparation to support multiple unstable attributes on items. Seemingly, the `feature` field isn't used with the `StabilityLevel::Stable` variant, so I haven't included it. `rustc_passes::lib_features` uses the 'feature' meta-item for 'stable' attributes, but it extracts them itself, rather than relying on `rustc_attr`.
...ions the logic for adding unstable attrs gets a bit messier when supporting multiple instances thereof. this keeps that from being duplicated in 3 places.
Translation is in an awkward position, but this provides a similar interface for both errors and soft-unstable lints, and enables the use of `DiagSymbolList` for simplifying error message construction.
This changes the text for E0544. Unfortunately, the example doesn't make much sense anymore. The way this merges stability levels together is made to work for stability-checking and rustdoc; as far as I can tell, only `rustc_passes::lib_features` needs them separate, and it extracts them itself. This includes the clarified/fixed const-unstable semantics that were previously in a different commit.
`reason` must appear at most once: it's the reason for the item being unstable, rather than a particular feature. This simplifies diagnostic formatting. `soft` must either be on all or no unstable attributes: it doesn't make sense for something to be partially soft, and allowing inconsistent softness markers would risk an item accidentally becoming properly unstable as its features stabilize.
9138a57 to
b23f28d
Compare
@dianne i know this is waiting for review, but this has gathered conflicts so it would be better to rebase it and we can get it reviewed after that. Thanks
I'm terribly sorry but I'm afraid the conflicts are rather complicated because the way attributes work itself was partially changed. Let me know if I need to help out with info about the new system.
This was really conflict-prone anyway, so I've enjoyed the break from maintaining it. ^^ When I rebase/rewrite it, I'll see how easy it'd be to split the const-stability changes into a separate PR; the const-eval parts were the most conflict-heavy, and that that should hopefully make this a bit more digestible too. I'll also see about once again starting off with only supporting multiple unstable attributes.
@jdonszelmann ty! I'll let you know if I have any specific questions. Do you expect any more reworks on your end to how stability attributes are handled? No worries either way! I'm not in a rush at least, so I'm happy to wait for the dust to settle if that means fewer future conflicts.
@dianne
Thanks for your contribution
From wg-triage. Any updates on this PR?
None yet. I'm planning on getting back to this in a week or two and paring it down as much as I can.
I've been looking at reviving this. As part of cleaning it up and lessening its perf/stdlib-binary-size impact, I think it'd help to arena-allocate the lists of unstable features that definitions are gated by. StabilityLevel::Unstable would be able to contain a slice reference1 , rather than a heap-allocated collection, so Stability and ConstStability still be Copy. Giving the attribute parser access to the dropless arena seems easy enough, but giving hir::Attribute a lifetime parameter is a pretty invasive change, so I figure I should ask: @jdonszelmann, does this sound reasonable? I haven't considered too much whether it would be useful for other attributes, but I've noticed stability attributes are the largest AttributeKind variants, so there's some potential for reducing the size of Attribute (and thus rlib size) by putting the whole Stability/etc. in the arena. Potentially they could be de-duplicated at some point too to further reduce the size of the standard library2 a bit, since most(?) stability attributes are shared between multiple definitions, but I'm not sure whether that's worth the effort.
Relatedly, I've been wondering: is there a reason AttributeKind::Stability and friends are EncodeCrossCrate::Yes? I think this would only affect the size of the standard library2 , but those are encoded in their own tables too, corresponding to the lookup_stability family of queries. I might have missed something, but I couldn't find any use of them via get_attr, attrs_for_def, or anything else defined in terms of those.
Footnotes
-
I checked, and
StabilityLevel::Stablewould be big enough that using a slice reference inStabilityLevel::Unstable(i.e. storing the length in the variant) doesn't makeStabilityLevelany larger. It may not be the ideal representation, but it seems like hopefully a decent compromise between efficiency/simplicity/convenience? ↩ -
And anything built with
-Zforce-unstable-if-unmarkedwhile building the compiler. ↩ ↩2
Uh oh!
There was an error while loading. Please reload this page.
Motivation
Many unstable library items require the stabilization of multiple features before they can be stabilized. By allowing them to be annotated with multiple
#[unstable]attributes, this prevents their accidental stabilization when some of those features are stabilized, and helps mitigate upgrade pains for unstable toolchain users.New stability attribute semantics
#[unstable]attribute is present on an item, it is unstable. Likewise, any#[rustc_const_unstable]marks an item as const-unstable, and any#[rustc_default_body_unstable]marks it as default body-unstable.#[unstable]attributes must be enabled. Conceptually, this is because it depends on multiple unstable features. Practically, this also means nightly toolchain users are less likely to need to replace one unstable feature gate with another in their crate-level attributes.#[stable]attributes may be present. The stabilization version of an item is the most recent stabilization version in present#[stable]attributes. Likewise,#[stable]attributes may be present on an unstable item. This simplifies macros that can apply stability attributes to items.This is documented in rust-lang/rustc-dev-guide#2128.
New syntactic constraints
#[unstable]attribute on an item may have areasonprovided (and likewise for#[rustc_const_unstable]and#[rustc_default_body_unstable]). At a glance, thereasonmeta-item was used to describe the reason for an item being unstable, rather than an individual feature it depends on. Enforcing this makes the diagnostic formatting much cleaner.#[unstable]attribute on an item has asoftmarker, it must be present on all#[unstable]attributes on that item. Items can't be partially soft-unstable, and this helps prevent accidentally making a soft-unstable item unusable on stable when stabilizing one of the features it requires.Fixes #94770
Based on #94988
(削除) PR for a diagnostic formatting change this uses: #132544 (削除ここまで)Some notes on assumptions I've made and how I've handled them:
rustc_passes::lib_featuresalso reads stability attributes, but it parses them itself. I considered it out of the scope of this PR refactor that too, but once attribute handling is reworked to be more unified in the future, this may need further adjustment (but hopefully not too much).#[unstable]attribute, this usesSmallVec<[_; 1]>for storage. Is this assumption reasonable? I haven't done any performance testing.StabilityLevelno longer hasCopy, stability structs are now arena-allocated; that was the simplest fix. I'm not sure what the performance implications are. If it's an issue, I can try try making it only allocate for multiple unstable features (which would also letStabilityLevelregainCopy).(削除) As a stopgap until Proper support for cross-crate recursive const stability checks #132541 lands, this is using aConstStabilityLevelenum for const-stability levels. Once that's merged, I should be able to move const-stability levels back to usingStabilityLevel. (削除ここまで)This doesn't update any libraries to add additional unstable attributes to items that should have them.