introduce implied_by in #[unstable] attribute

[davidtwco]

Requested by the library team on Zulip.

If part of a feature is stabilized and a new feature is added for the remaining parts, then the implied_by meta-item can be added to #[unstable] to indicate which now-stable feature was used previously.

error: the feature `foo` has been partially stabilized since 1.62.0 and is succeeded by the feature `foobar`
--> $DIR/stability-attribute-implies-using-unstable.rs:3:12
|
LL | #![feature(foo)]
| ^^^
|
note: the lint level is defined here
--> $DIR/stability-attribute-implies-using-stable.rs:2:9
|
LL | #![deny(stable_features)]
| ^^^^^^^^^^^^^^^
help: if you are using features which are still unstable, change to using `foobar`
|
LL | #![feature(foobar)]
| ~~~~~~
help: if you are using features which are now stable, remove this line
|
LL - #![feature(foo)]
|

When a #![feature(..)] attribute still exists for the now-stable attribute, then there this has two effects:

• There will not be an stability error for uses of items from the implied feature which are still unstable (until the #![feature(..)] is removed or updated to the new feature).
• There will be an improved diagnostic for the remaining use of the feature attribute for the now-stable feature.

/// If part of a feature is stabilized and a new feature is added for the remaining parts,/// then the `implied_by` attribute is used to indicate which now-stable feature previously/// contained a item.////// ```pseudo-Rust/// #[unstable(feature = "foo", issue = "...")]/// fn foo() {}/// #[unstable(feature = "foo", issue = "...")]/// fn foobar() {}/// ```////// ...becomes...////// ```pseudo-Rust/// #[stable(feature = "foo", since = "1.XX.X")]/// fn foo() {}/// #[unstable(feature = "foobar", issue = "...", implied_by = "foo")]/// fn foobar() {}/// ```

In the Zulip discussion, this was envisioned as implies on #[stable] but I went with implied_by on #[unstable] because it means that only the unstable attribute needs to be changed in future, not the new stable attribute, which seems less error-prone. It also isn't particularly feasible for me to detect whether items from the implied feature are used and then only suggest updating or removing the #![feature(..)] as appropriate, so I always do both.

There's some new information in the cross-crate metadata as a result of this change, that's a little unfortunate, but without requiring that the #[unstable] and #[stable] attributes both contain the implication information, it's necessary:

/// This mapping is necessary unless both the `#[stable]` and `#[unstable]` attributes should/// specify their implications (both `implies` and `implied_by`). If only one of the two/// attributes do (as in the current implementation, `implied_by` in `#[unstable]`), then this/// mapping is necessary for diagnostics. When a "unnecessary feature attribute" error is/// reported, only the `#[stable]` attribute information is available, so the map is necessary/// to know that the feature implies another feature. If it were reversed, and the `#[stable]`/// attribute had an `implies` meta item, then a map would be necessary when avoiding a "use of/// unstable feature" error for a feature that was implied.

I also change some comments to documentation comments in the compiler, add a helper for going from a Span to a Span for the entire line, and fix a incorrect part of the pre-existing stability attribute diagnostics.

cc @yaahc

[rust-highfive]

r? @michaelwoerister

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

[davidtwco]

I would have made these translatable given that's what I'm doing everywhere else but I have a patch coming for rustc_passes in the next few days so it can wait for that. :)

[CAD97]

What's the behavior of implied_by = "still_unstable" and implied_by = "not_a_feature"?

The former is useful for feature subsetting while still unstable, e.g. min_specialization. The latter could be useful for feature renaming, but should probably be prevented as likely an unintended typo.

[davidtwco]

What's the behavior of implied_by = "still_unstable" and implied_by = "not_a_feature"?

The former is useful for feature subsetting while still unstable, e.g. min_specialization. The latter could be useful for feature renaming, but should probably be prevented as likely an unintended typo.

implied_by = "still_unstable" works right now, and the behaviour is the exact same as implied_by = "stable". This could be changed but it sounds like it might be useful to t-libs, so I don't mind leaving it as is.

As for implied_by = "not_a_feature", I don't really know what would happen if you did this, I suspect nothing would happen (but we might crash, I haven't checked). If it is feasible, I'll add a check somewhere to prevent this.

[michaelwoerister]

The implementation looks good to me. Thanks, @davidtwco!

Maybe someone from @rust-lang/libs wants to take another look to make sure that this PR actually implements the desired behavior? Otherwise, feel free to r=me.

[michaelwoerister]

As for implied_by = "not_a_feature", I don't really know what would happen if you did this, I suspect nothing would happen (but we might crash, I haven't checked). If it is feasible, I'll add a check somewhere to prevent this.

Adding a check like this sounds like a good idea. Here maybe?: https://github.com/rust-lang/rust/pull/99212/files#diff-5f57ad10e1bdde3d046b258d390fd2ecc6f1511158aa130cebb72093da16ef29R955-R958

[m-ou-se]

cc @rust-lang/libs-api

[davidtwco]

As for implied_by = "not_a_feature", I don't really know what would happen if you did this, I suspect nothing would happen (but we might crash, I haven't checked). If it is feasible, I'll add a check somewhere to prevent this.

Adding a check like this sounds like a good idea. Here maybe?: #99212 (files)

Added!

[m-ou-se]

I think this is a useful improvement, but there's one thing that concerns me. It's important that #[unstable] things are considered unstable, such that we can always change them without feeling pressure from expectations to keep the unstable features somewhat 'stable'. So, while this implied_by feature definitely makes unstable features more ergonomic, I'm worried that the more ergonomic we make them, the more we (accidentally) grow expectations around the stability and ergonomics of unstable features. For example, while it's nice that nightly users will not have to change their #![feature(..)] lines as often, there should not be an end goal of making that happen in all cases, as that would suggest some kind of stability of unstable features.

I don't think that's a blocker here. Improving ergonomics is good. But something to keep in mind for #[unstable] related changes in general.

[michaelwoerister]

OK, r=me with the test fixed.

[davidtwco]

@bors r=michaelwoerister

[bors]

📌 Commit e587299 has been approved by michaelwoerister

It is now in the queue for this repository.

[pnkfelix]

Hey @davidtwco , is there any chance that this PR was the cause of the performance regression reported in rollup PR #99520? You can see my line of reasoning about why I thought it might be, over in zulip.

[davidtwco]

Hey @davidtwco , is there any chance that this PR was the cause of the performance regression reported in rollup PR #99520? You can see my line of reasoning about why I thought it might be, over in zulip.

It's possible because of some additions to the cross-crate metadata. I've had another look at the changes and can't think of any ways to do less work that it currently does (it should just be writing a HashMap if there are elements in it, which there might only be one or two of, depends how much the library team are already using this).

[yaahc]

I don't think we've landed any PRs yet that need this, only #99573 which is still open and has another path to landing that we can use instead, so if there are perf issues with the impl as is and y'all feel like pumping the breaks that should be okay on our side for now at least.

[steffahn]

Hi, out of curiosity, since I just found out this had actually been implemented, I wanted to try this on the only occurrence currently in std… and it doesn’t work at all?

#![feature(const_btree_new)]use std::collections::BTreeMap;constfnf() -> BTreeMap<(),()>{let x = BTreeMap::new();let l = x.len();
x
}

error: `BTreeMap::<K, V, A>::len` is not yet stable as a const fn
--> src/lib.rs:7:13
|
7 | let l = x.len();
| ^^^^^^^
|
= help: add `#![feature(const_btree_len)]` to the crate attributes to enable

(playground)

I’m not doing anything wrong, am I? I guess I should open an issue?

[steffahn]

Oh… maybe it’s a problem because it isn’t using unstable/stable but instead rustc_const_unstable/rustc_const_stable!?