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

Support multiple stability attributes on items #131824

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

Conversation

dianne
Copy link
Contributor

@dianne dianne commented Oct 17, 2024

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

  • If any #[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.
  • In order to use an unstable item, all feature gates specified by its #[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.
  • Multiple #[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

  • As a sanity check, an item is not allowed to have multiple stability attributes for the same feature.
  • At most one #[unstable] attribute on an item may have a reason provided (and likewise for #[rustc_const_unstable] and #[rustc_default_body_unstable]). At a glance, the reason meta-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.
  • If any #[unstable] attribute on an item has a soft marker, 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:

  • Stability structs are specialized to how they're used for stability checking and rustdoc. rustc_passes::lib_features also 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).
  • Under the assumption that most library items will still have at most one #[unstable] attribute, this uses SmallVec<[_; 1]> for storage. Is this assumption reasonable? I haven't done any performance testing.
  • Since StabilityLevel no longer has Copy, 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 let StabilityLevel regain Copy).
  • As a stopgap until Proper support for cross-crate recursive const stability checks #132541 lands, this is using a ConstStabilityLevel enum for const-stability levels. Once that's merged, I should be able to move const-stability levels back to using StabilityLevel.

This doesn't update any libraries to add additional unstable attributes to items that should have them.

@rustbot
Copy link
Collaborator

rustbot commented Oct 17, 2024

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

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Oct 17, 2024
@rustbot
Copy link
Collaborator

rustbot commented Oct 17, 2024

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.

@rust-log-analyzer

This comment has been minimized.

@dianne dianne force-pushed the multiple-unstables branch from ebf7852 to 14b8556 Compare October 17, 2024 10:03
@bors
Copy link
Contributor

bors commented Oct 22, 2024

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

@dianne dianne force-pushed the multiple-unstables branch from 14b8556 to 6561424 Compare October 23, 2024 02:37
@bors
Copy link
Contributor

bors commented Oct 23, 2024

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

@dianne dianne force-pushed the multiple-unstables branch from 6561424 to 0a730d9 Compare October 23, 2024 19:52
@bors
Copy link
Contributor

bors commented Oct 24, 2024

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

@dianne dianne force-pushed the multiple-unstables branch from 0a730d9 to 86fac38 Compare October 25, 2024 01:50
@bors
Copy link
Contributor

bors commented Oct 26, 2024

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

@dianne dianne force-pushed the multiple-unstables branch from 86fac38 to 88e33bf Compare October 26, 2024 23:16
@rustbot
Copy link
Collaborator

rustbot commented Oct 26, 2024

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

@dianne
Copy link
Contributor Author

dianne commented Oct 26, 2024

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.

@bors
Copy link
Contributor

bors commented Oct 27, 2024

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

@RalfJung
Copy link
Member

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?

compiler/rustc_const_eval/src/check_consts/check.rs Outdated Show resolved Hide resolved
compiler/rustc_attr/src/builtin.rs Outdated Show resolved Hide resolved
compiler/rustc_attr/src/builtin.rs Outdated Show resolved Hide resolved
tests/ui/consts/const-unstable-intrinsic.stderr Outdated Show resolved Hide resolved
@dianne
Copy link
Contributor Author

dianne commented Oct 27, 2024

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?

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!

@dianne dianne force-pushed the multiple-unstables branch from 88e33bf to bc6c3b0 Compare October 28, 2024 06:44
@dianne dianne requested a review from RalfJung October 28, 2024 06:56
@RalfJung
Copy link
Member

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.

@RalfJung RalfJung added I-libs-api-nominated Nominated for discussion during a libs-api team meeting. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Oct 28, 2024
@bors
Copy link
Contributor

bors commented Oct 28, 2024

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

@dianne dianne force-pushed the multiple-unstables branch from bc6c3b0 to 5a1fd55 Compare October 28, 2024 23:40
@bors
Copy link
Contributor

bors commented Nov 1, 2024

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

@Amanieu
Copy link
Member

Amanieu commented Nov 5, 2024

We discussed this in the libs-api meeting today and we're happy with this as proposed. Support for multiple stable attribute was somewhat controversial but in the end we felt that it would be useful in macros which apply a mix of stability attributes to many items.

@Amanieu Amanieu removed the I-libs-api-nominated Nominated for discussion during a libs-api team meeting. label Nov 5, 2024
@dianne
Copy link
Contributor Author

dianne commented Nov 5, 2024

Whoever happens to merge their changes first will force the other to retrofit their changes onto it haha, but with some luck for you, that'll be me who has to do the retrofitting :3

I've been wondering about this. My one hope if I do the rebase is that the attribute rework will make this cleaner in the end. 😅

We discussed this in the libs-api meeting today and we're happy with this as proposed.

Nice! I'll get to work on documenting it soon.

@jdonszelmann
Copy link
Contributor

It probably will, the reason it's annoying is that in my local version, builtins.rs is simply gone. It's split up into files for all the different attributes that are processed there. So we'll just have to find the parts that have to move, should be doable.

@dianne dianne force-pushed the multiple-unstables branch from e393929 to b9af36e Compare November 6, 2024 01:26
@rustbot
Copy link
Collaborator

rustbot commented Nov 6, 2024

Some changes occurred to the CTFE machinery

cc @rust-lang/wg-const-eval

@dianne
Copy link
Contributor Author

dianne commented Nov 6, 2024

Is that a new notification? Unless I made a mistake, I think all I did was resolve the merge conflict straightforwardly, so see the above discussion for how this touches the CTFE machinery.

@RalfJung
Copy link
Member

RalfJung commented Nov 6, 2024 via email

@bors
Copy link
Contributor

bors commented Nov 10, 2024

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

@dianne dianne force-pushed the multiple-unstables branch from b9af36e to 4bbcf10 Compare November 11, 2024 03:55
@dianne
Copy link
Contributor Author

dianne commented Nov 11, 2024

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.

@bors
Copy link
Contributor

bors commented Nov 12, 2024

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

@dianne dianne force-pushed the multiple-unstables branch from 4bbcf10 to 814ab38 Compare November 13, 2024 04:59
@petrochenkov
Copy link
Contributor

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.

@bors
Copy link
Contributor

bors commented Nov 15, 2024

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

@dianne dianne force-pushed the multiple-unstables branch 2 times, most recently from 6f28bda to 9138a57 Compare November 16, 2024 04:54
@bors
Copy link
Contributor

bors commented Nov 18, 2024

☔ 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.
@dianne dianne force-pushed the multiple-unstables branch from 9138a57 to b23f28d Compare November 19, 2024 06:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow multiple #[unstable] attributes on one item
9 participants