passes: improved partial stabilization diagnostic

Improves the diagnostic when a feature attribute is specified
unnecessarily but the feature implies another (i.e. it was partially
stabilized) to refer to the implied feature.

Signed-off-by: David Wood <[email protected]>

Author: davidtwco

compiler/rustc_metadata/src/rmeta/decoder.rs

@@ -951,6 +951,13 @@ impl<'a, 'tcx> CrateMetadataRef<'a> {
         tcx.arena.alloc_from_iter(self.root.lib_features.decode(self))
     }
 
+    /// Iterates over the stability implications in the given crate (when a `#[unstable]` attribute
+    /// has an `implied_by` meta item, then the mapping from the implied feature to the actual
+    /// feature is a stability implication).
+    fn get_stability_implications(self, tcx: TyCtxt<'tcx>) -> &'tcx [(Symbol, Symbol)] {
+        tcx.arena.alloc_from_iter(self.root.stability_implications.decode(self))
+    }
+
     /// Iterates over the language items in the given crate.
     fn get_lang_items(self, tcx: TyCtxt<'tcx>) -> &'tcx [(DefId, usize)] {
         tcx.arena.alloc_from_iter(

compiler/rustc_metadata/src/rmeta/decoder/cstore_impl.rs

@@ -291,6 +291,9 @@ provide! { <'tcx> tcx, def_id, other, cdata,
         tcx.arena.alloc_slice(&result)
     }
     defined_lib_features => { cdata.get_lib_features(tcx) }
+    stability_implications => {
+        cdata.get_stability_implications(tcx).iter().copied().collect()
+    }
     is_intrinsic => { cdata.get_is_intrinsic(def_id.index) }
     defined_lang_items => { cdata.get_lang_items(tcx) }
     diagnostic_items => { cdata.get_diagnostic_items() }

compiler/rustc_metadata/src/rmeta/encoder.rs

@@ -538,6 +538,11 @@ impl<'a, 'tcx> EncodeContext<'a, 'tcx> {
         let lib_features = self.encode_lib_features();
         let lib_feature_bytes = self.position() - i;
 
+        // Encode the stability implications.
+        i = self.position();
+        let stability_implications = self.encode_stability_implications();
+        let stability_implications_bytes = self.position() - i;
+
         // Encode the language items.
         i = self.position();
         let lang_items = self.encode_lang_items();
@@ -686,6 +691,7 @@ impl<'a, 'tcx> EncodeContext<'a, 'tcx> {
             crate_deps,
             dylib_dependency_formats,
             lib_features,
+            stability_implications,
             lang_items,
             diagnostic_items,
             lang_items_missing,
@@ -710,6 +716,7 @@ impl<'a, 'tcx> EncodeContext<'a, 'tcx> {
         let computed_total_bytes = preamble_bytes
             + dep_bytes
             + lib_feature_bytes
+            + stability_implications_bytes
             + lang_item_bytes
             + diagnostic_item_bytes
             + native_lib_bytes
@@ -761,6 +768,7 @@ impl<'a, 'tcx> EncodeContext<'a, 'tcx> {
             p("preamble", preamble_bytes);
             p("dep", dep_bytes);
             p("lib feature", lib_feature_bytes);
+            p("stability_implications", stability_implications_bytes);
             p("lang item", lang_item_bytes);
             p("diagnostic item", diagnostic_item_bytes);
             p("native lib", native_lib_bytes);
@@ -1777,6 +1785,13 @@ impl<'a, 'tcx> EncodeContext<'a, 'tcx> {
         self.lazy_array(lib_features.to_vec())
     }
 
+    fn encode_stability_implications(&mut self) -> LazyArray<(Symbol, Symbol)> {
+        empty_proc_macro!(self);
+        let tcx = self.tcx;
+        let implications = tcx.stability_implications(LOCAL_CRATE);
+        self.lazy_array(implications.iter().map(|(k, v)| (*k, *v)))
+    }
+
     fn encode_diagnostic_items(&mut self) -> LazyArray<(Symbol, DefIndex)> {
         empty_proc_macro!(self);
         let tcx = self.tcx;

compiler/rustc_metadata/src/rmeta/mod.rs

@@ -226,6 +226,7 @@ pub(crate) struct CrateRoot {
     crate_deps: LazyArray<CrateDep>,
     dylib_dependency_formats: LazyArray<Option<LinkagePreference>>,
     lib_features: LazyArray<(Symbol, Option<Symbol>)>,
+    stability_implications: LazyArray<(Symbol, Symbol)>,
     lang_items: LazyArray<(DefIndex, usize)>,
     lang_items_missing: LazyArray<lang_items::LangItem>,
     diagnostic_items: LazyArray<(Symbol, DefIndex)>,

compiler/rustc_middle/src/middle/stability.rs

@@ -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 {

compiler/rustc_middle/src/query/mod.rs

@@ -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) }

compiler/rustc_passes/src/stability.rs

@@ -2,9 +2,9 @@
 //! propagating default levels lexically from parent to children ast nodes.
 
 use attr::StabilityLevel;
-use rustc_attr::{self as attr, ConstStability, Stability};
+use rustc_attr::{self as attr, ConstStability, Stability, Unstable};
 use rustc_data_structures::fx::{FxHashSet, FxIndexMap};
-use rustc_errors::struct_span_err;
+use rustc_errors::{struct_span_err, Applicability};
 use rustc_hir as hir;
 use rustc_hir::def::{DefKind, Res};
 use rustc_hir::def_id::{LocalDefId, CRATE_DEF_ID};
@@ -265,6 +265,10 @@ impl<'a, 'tcx> Annotator<'a, 'tcx> {
                 }
             }
 
+            if let Stability { level: Unstable { implied_by: Some(implied_by), .. }, feature } = stab {
+                self.index.implications.insert(implied_by, feature);
+            }
+
             self.index.stab_map.insert(def_id, stab);
             stab
         });
@@ -610,6 +614,7 @@ fn stability_index(tcx: TyCtxt<'_>, (): ()) -> Index {
         stab_map: Default::default(),
         const_stab_map: Default::default(),
         depr_map: Default::default(),
+        implications: Default::default(),
     };
 
     {
@@ -668,6 +673,7 @@ pub(crate) fn provide(providers: &mut Providers) {
     *providers = Providers {
         check_mod_unstable_api_usage,
         stability_index,
+        stability_implications: |tcx, _| tcx.stability().implications.clone(),
         lookup_stability: |tcx, id| tcx.stability().local_stability(id.expect_local()),
         lookup_const_stability: |tcx, id| tcx.stability().local_const_stability(id.expect_local()),
         lookup_deprecation_entry: |tcx, id| {
@@ -946,11 +952,18 @@ pub fn check_unused_or_stable_features(tcx: TyCtxt<'_>) {
     remaining_lib_features.remove(&sym::libc);
     remaining_lib_features.remove(&sym::test);
 
+    let mut implications = tcx.stability_implications(rustc_hir::def_id::LOCAL_CRATE).clone();
+    for &cnum in tcx.crates(()) {
+        implications.extend(tcx.stability_implications(cnum));
+    }
+
     let check_features = |remaining_lib_features: &mut FxIndexMap<_, _>, defined_features: &[_]| {
         for &(feature, since) in defined_features {
-            if let Some(since) = since {
-                if let Some(span) = remaining_lib_features.get(&feature) {
-                    // Warn if the user has enabled an already-stable lib feature.
+            if let Some(since) = since && let Some(span) = remaining_lib_features.get(&feature) {
+                // Warn if the user has enabled an already-stable lib feature.
+                if let Some(implies) = implications.get(&feature) {
+                    unnecessary_partially_stable_feature_lint(tcx, *span, feature, *implies, since);
+                } else {
                     unnecessary_stable_feature_lint(tcx, *span, feature, since);
                 }
             }
@@ -983,12 +996,41 @@ pub fn check_unused_or_stable_features(tcx: TyCtxt<'_>) {
     // don't lint about unused features. We should re-enable this one day!
 }
 
+fn unnecessary_partially_stable_feature_lint(
+    tcx: TyCtxt<'_>,
+    span: Span,
+    feature: Symbol,
+    implies: Symbol,
+    since: Symbol,
+) {
+    tcx.struct_span_lint_hir(lint::builtin::STABLE_FEATURES, hir::CRATE_HIR_ID, span, |lint| {
+        lint.build(&format!(
+            "the feature `{feature}` has been partially stabilized since {since} and is succeeded \
+             by the feature `{implies}`"
+        ))
+        .span_suggestion(
+            span,
+            &format!(
+                "if you are using features which are still unstable, change to using `{implies}`"
+            ),
+            implies,
+            Applicability::MaybeIncorrect,
+        )
+        .span_suggestion(
+            tcx.sess.source_map().span_extend_to_line(span),
+            "if you are using features which are now stable, remove this line",
+            "",
+            Applicability::MaybeIncorrect,
+        )
+        .emit();
+    });
+}
+
 fn unnecessary_stable_feature_lint(tcx: TyCtxt<'_>, span: Span, feature: Symbol, since: Symbol) {
     tcx.struct_span_lint_hir(lint::builtin::STABLE_FEATURES, hir::CRATE_HIR_ID, span, |lint| {
         lint.build(&format!(
-            "the feature `{}` has been stable since {} and no longer requires \
-                      an attribute to enable",
-            feature, since
+            "the feature `{feature}` has been stable since {since} and no longer requires an \
+             attribute to enable",
         ))
         .emit();
     });

src/test/ui/stability-attribute/stability-attribute-implies-using-stable.rs

@@ -1,7 +1,7 @@
 // aux-build:stability-attribute-implies.rs
 #![deny(stable_features)]
 #![feature(foo)]
-//~^ ERROR the feature `foo` has been stable since 1.62.0 and no longer requires an attribute to enable
+//~^ ERROR the feature `foo` has been partially stabilized since 1.62.0 and is succeeded by the feature `foobar`
 
 // Tests that the use of `implied_by` in the `#[unstable]` attribute results in a diagnostic
 // mentioning partial stabilization, and that given the implied unstable feature is unused (there

src/test/ui/stability-attribute/stability-attribute-implies-using-stable.stderr

@@ -1,4 +1,4 @@
-error: the feature `foo` has been stable since 1.62.0 and no longer requires an attribute to enable
+error: the feature `foo` has been partially stabilized since 1.62.0 and is succeeded by the feature `foobar`
   --> $DIR/stability-attribute-implies-using-stable.rs:3:12
    |
 LL | #![feature(foo)]
@@ -9,6 +9,14 @@ note: the lint level is defined here
    |
 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)]
+   |
 
 error: aborting due to previous error
 

src/test/ui/stability-attribute/stability-attribute-implies-using-unstable.rs

@@ -1,7 +1,7 @@
 // aux-build:stability-attribute-implies.rs
 #![deny(stable_features)]
 #![feature(foo)]
-//~^ ERROR the feature `foo` has been stable since 1.62.0 and no longer requires an attribute to enable
+//~^ ERROR the feature `foo` has been partially stabilized since 1.62.0 and is succeeded by the feature `foobar`
 
 // Tests that the use of `implied_by` in the `#[unstable]` attribute results in a diagnostic
 // mentioning partial stabilization and that given the implied unstable feature is used (there is a

src/test/ui/stability-attribute/stability-attribute-implies-using-unstable.stderr

@@ -1,4 +1,4 @@
-error: the feature `foo` has been stable since 1.62.0 and no longer requires an attribute to enable
+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)]
@@ -9,6 +9,14 @@ note: the lint level is defined here
    |
 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)]
+   |
 
 error: aborting due to previous error