bjoernager/rust
bjoernager/rust
1
Fork 0
Code Activity

Rollup merge of #99212 - davidtwco:partial-stability-implies, r=michaelwoerister

Browse source 
introduce `implied_by` in `#[unstable]` attribute

Requested by the library team [on Zulip](https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/topic/better.20support.20for.20partial.20stabilizations/near/285581519).

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.

```diagnostic
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.

```rust
        /// 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:

```rust
    /// 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`
This commit is contained in:
Matthias Krüger 2022-07-20 18:58:14 +02:00 • committed by GitHub
commit 857afc75e6
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
24 changed files with 357 additions and 62 deletions
Download patch file Download diff file Expand all files Collapse all files

14
compiler/rustc_middle/src/middle/mod.rs
View file

@ -3,14 +3,14 @@ pub mod dependency_format;
pub mod exported_symbols;
pub mod lang_items;
pub mod lib_features {
use rustc_data_structures::fx::{FxHashMap, FxHashSet};
use rustc_span::symbol::Symbol;
use rustc_data_structures::fx::FxHashMap;
use rustc_span::{symbol::Symbol, Span};
#[derive(HashStable, Debug)]
pub struct LibFeatures {
// A map from feature to stabilisation version.
pub stable: FxHashMap<Symbol, Symbol>,
pub unstable: FxHashSet<Symbol>,
/// A map from feature to stabilisation version.
pub stable: FxHashMap<Symbol, (Symbol, Span)>,
pub unstable: FxHashMap<Symbol, Span>,
}
impl LibFeatures {
@ -18,8 +18,8 @@ pub mod lib_features {
let mut all_features: Vec<_> = self
.stable
.iter()
.map(|(f, s)| (*f, Some(*s)))
.chain(self.unstable.iter().map(|f| (*f, None)))
.map(|(f, (s, _))| (*f, Some(*s)))
.chain(self.unstable.iter().map(|(f, _)| (*f, None)))
.collect();
all_features.sort_unstable_by(|a, b| a.0.as_str().partial_cmp(b.0.as_str()).unwrap());
all_features

24
compiler/rustc_middle/src/middle/stability.rs
View file

@ -62,6 +62,19 @@ pub struct Index {
pub stab_map: FxHashMap<LocalDefId, Stability>,
pub const_stab_map: FxHashMap<LocalDefId, ConstStability>,
pub depr_map: FxHashMap<LocalDefId, DeprecationEntry>,
/// Mapping from feature name to feature name based on the `implied_by` field of `#[unstable]`
/// attributes. If a `#[unstable(feature = "implier", implied_by = "impliee")]` attribute
/// exists, then this map will have a `impliee -> implier` entry.
///
/// 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.
pub implications: FxHashMap<Symbol, Symbol>,
}
impl Index {
@ -423,7 +436,9 @@ impl<'tcx> TyCtxt<'tcx> {
match stability {
Some(Stability {
level: attr::Unstable { reason, issue, is_soft }, feature, ..
level: attr::Unstable { reason, issue, is_soft, implied_by },
feature,
..
}) => {
if span.allows_unstable(feature) {
debug!("stability: skipping span={:?} since it is internal", span);
@ -433,6 +448,13 @@ impl<'tcx> TyCtxt<'tcx> {
return EvalResult::Allow;
}
// If this item was previously part of a now-stabilized feature which is still
// active (i.e. the user hasn't removed the attribute for the stabilized feature
// yet) then allow use of this item.
if let Some(implied_by) = implied_by && self.features().active(implied_by) {
return EvalResult::Allow;
}
// When we're compiling the compiler itself we may pull in
// crates from crates.io, but those crates may depend on other
// crates also pulled in from crates.io. We want to ideally be

8
compiler/rustc_middle/src/query/mod.rs
View file

@ -1634,11 +1634,15 @@ rustc_queries! {
storage(ArenaCacheSelector<'tcx>)
desc { "calculating the lib features map" }
}
query defined_lib_features(_: CrateNum)
-> &'tcx [(Symbol, Option<Symbol>)] {
query defined_lib_features(_: CrateNum) -> &'tcx [(Symbol, Option<Symbol>)] {
desc { "calculating the lib features defined in a crate" }
separate_provide_extern
}
query stability_implications(_: CrateNum) -> FxHashMap<Symbol, Symbol> {
storage(ArenaCacheSelector<'tcx>)
desc { "calculating the implications between `#[unstable]` features defined in a crate" }
separate_provide_extern
}
/// Whether the function is an intrinsic
query is_intrinsic(def_id: DefId) -> bool {
desc { |tcx| "is_intrinsic({})", tcx.def_path_str(def_id) }