Rollup merge of rust-lang#136434 - RalfJung:rustc_allowed_through_unstable_modules-deprecation-required, r=compiler-errors

rustc_allowed_through_unstable_modules: require deprecation message

This changes the `#[rustc_allowed_through_unstable_modules]` attribute so that a deprecation message (ideally directing people towards the stable path) is required.

Author: matthiaskrgr

compiler/rustc_attr_data_structures/src/stability.rs

@@ -101,16 +101,6 @@ impl PartialConstStability {
     }
 }
 
-#[derive(Encodable, Decodable, PartialEq, Copy, Clone, Debug, Eq, Hash)]
-#[derive(HashStable_Generic)]
-pub enum AllowedThroughUnstableModules {
-    /// This does not get a deprecation warning. We still generally would prefer people to use the
-    /// fully stable path, and a warning will likely be emitted in the future.
-    WithoutDeprecation,
-    /// Emit the given deprecation warning.
-    WithDeprecation(Symbol),
-}
-
 /// The available stability levels.
 #[derive(Encodable, Decodable, PartialEq, Copy, Clone, Debug, Eq, Hash)]
 #[derive(HashStable_Generic)]
@@ -147,8 +137,9 @@ pub enum StabilityLevel {
     Stable {
         /// Rust release which stabilized this feature.
         since: StableSince,
-        /// This is `Some` if this item allowed to be referred to on stable via unstable modules.
-        allowed_through_unstable_modules: Option<AllowedThroughUnstableModules>,
+        /// This is `Some` if this item allowed to be referred to on stable via unstable modules;
+        /// the `Symbol` is the deprecation message printed in that case.
+        allowed_through_unstable_modules: Option<Symbol>,
     },
 }
 

compiler/rustc_attr_parsing/src/attributes/stability.rs

@@ -6,12 +6,12 @@ use rustc_ast::MetaItem;
 use rustc_ast::attr::AttributeExt;
 use rustc_ast_pretty::pprust;
 use rustc_attr_data_structures::{
-    AllowedThroughUnstableModules, ConstStability, DefaultBodyStability, Stability, StabilityLevel,
-    StableSince, UnstableReason, VERSION_PLACEHOLDER,
+    ConstStability, DefaultBodyStability, Stability, StabilityLevel, StableSince, UnstableReason,
+    VERSION_PLACEHOLDER,
 };
 use rustc_errors::ErrorGuaranteed;
 use rustc_session::Session;
-use rustc_span::{Span, Symbol, sym};
+use rustc_span::{Span, Symbol, kw, sym};
 
 use crate::attributes::util::UnsupportedLiteralReason;
 use crate::{parse_version, session_diagnostics};
@@ -29,10 +29,14 @@ pub fn find_stability(
     for attr in attrs {
         match attr.name_or_empty() {
             sym::rustc_allowed_through_unstable_modules => {
-                allowed_through_unstable_modules = Some(match attr.value_str() {
-                    Some(msg) => AllowedThroughUnstableModules::WithDeprecation(msg),
-                    None => AllowedThroughUnstableModules::WithoutDeprecation,
-                })
+                // The value is mandatory, but avoid ICEs in case such code reaches this function.
+                allowed_through_unstable_modules = Some(attr.value_str().unwrap_or_else(|| {
+                    sess.dcx().span_delayed_bug(
+                        item_sp,
+                        "`#[rustc_allowed_through_unstable_modules]` without deprecation message",
+                    );
+                    kw::Empty
+                }))
             }
             sym::unstable => {
                 if stab.is_some() {

compiler/rustc_feature/src/builtin_attrs.rs

@@ -616,7 +616,7 @@ pub const BUILTIN_ATTRIBUTES: &[BuiltinAttribute] = &[
         EncodeCrossCrate::No, "allow_internal_unsafe side-steps the unsafe_code lint",
     ),
     rustc_attr!(
-        rustc_allowed_through_unstable_modules, Normal, template!(Word, NameValueStr: "deprecation message"),
+        rustc_allowed_through_unstable_modules, Normal, template!(NameValueStr: "deprecation message"),
         WarnFollowing, EncodeCrossCrate::No,
         "rustc_allowed_through_unstable_modules special cases accidental stabilizations of stable items \
         through unstable paths"

compiler/rustc_passes/src/stability.rs

@@ -5,8 +5,8 @@ use std::mem::replace;
 use std::num::NonZero;
 
 use rustc_attr_parsing::{
-    self as attr, AllowedThroughUnstableModules, ConstStability, DeprecatedSince, Stability,
-    StabilityLevel, StableSince, UnstableReason, VERSION_PLACEHOLDER,
+    self as attr, ConstStability, DeprecatedSince, Stability, StabilityLevel, StableSince,
+    UnstableReason, VERSION_PLACEHOLDER,
 };
 use rustc_data_structures::fx::FxIndexMap;
 use rustc_data_structures::unord::{ExtendUnord, UnordMap, UnordSet};
@@ -880,91 +880,87 @@ impl<'tcx> Visitor<'tcx> for Checker<'tcx> {
 
             if item_is_allowed {
                 // The item itself is allowed; check whether the path there is also allowed.
-                let is_allowed_through_unstable_modules: Option<AllowedThroughUnstableModules> =
+                let is_allowed_through_unstable_modules: Option<Symbol> =
                     self.tcx.lookup_stability(def_id).and_then(|stab| match stab.level {
                         StabilityLevel::Stable { allowed_through_unstable_modules, .. } => {
                             allowed_through_unstable_modules
                         }
                         _ => None,
                     });
 
-                if is_allowed_through_unstable_modules.is_none() {
-                    // Check parent modules stability as well if the item the path refers to is itself
-                    // stable. We only emit warnings for unstable path segments if the item is stable
-                    // or allowed because stability is often inherited, so the most common case is that
-                    // both the segments and the item are unstable behind the same feature flag.
-                    //
-                    // We check here rather than in `visit_path_segment` to prevent visiting the last
-                    // path segment twice
-                    //
-                    // We include special cases via #[rustc_allowed_through_unstable_modules] for items
-                    // that were accidentally stabilized through unstable paths before this check was
-                    // added, such as `core::intrinsics::transmute`
-                    let parents = path.segments.iter().rev().skip(1);
-                    for path_segment in parents {
-                        if let Some(def_id) = path_segment.res.opt_def_id() {
-                            // use `None` for id to prevent deprecation check
-                            self.tcx.check_stability_allow_unstable(
-                                def_id,
-                                None,
-                                path.span,
-                                None,
-                                if is_unstable_reexport(self.tcx, id) {
-                                    AllowUnstable::Yes
-                                } else {
-                                    AllowUnstable::No
-                                },
-                            );
-                        }
-                    }
-                } else if let Some(AllowedThroughUnstableModules::WithDeprecation(deprecation)) =
-                    is_allowed_through_unstable_modules
-                {
-                    // Similar to above, but we cannot use `check_stability_allow_unstable` as that would
-                    // immediately show the stability error. We just want to know the result and disaplay
-                    // our own kind of error.
-                    let parents = path.segments.iter().rev().skip(1);
-                    for path_segment in parents {
-                        if let Some(def_id) = path_segment.res.opt_def_id() {
-                            // use `None` for id to prevent deprecation check
-                            let eval_result = self.tcx.eval_stability_allow_unstable(
-                                def_id,
-                                None,
-                                path.span,
-                                None,
-                                if is_unstable_reexport(self.tcx, id) {
-                                    AllowUnstable::Yes
-                                } else {
-                                    AllowUnstable::No
-                                },
-                            );
-                            let is_allowed = matches!(eval_result, EvalResult::Allow);
-                            if !is_allowed {
-                                // Calculating message for lint involves calling `self.def_path_str`,
-                                // which will by default invoke the expensive `visible_parent_map` query.
-                                // Skip all that work if the lint is allowed anyway.
-                                if self.tcx.lint_level_at_node(DEPRECATED, id).0
-                                    == lint::Level::Allow
-                                {
-                                    return;
-                                }
-                                // Show a deprecation message.
-                                let def_path =
-                                    with_no_trimmed_paths!(self.tcx.def_path_str(def_id));
-                                let def_kind = self.tcx.def_descr(def_id);
-                                let diag = Deprecated {
-                                    sub: None,
-                                    kind: def_kind.to_owned(),
-                                    path: def_path,
-                                    note: Some(deprecation),
-                                    since_kind: lint::DeprecatedSinceKind::InEffect,
-                                };
-                                self.tcx.emit_node_span_lint(
-                                    DEPRECATED,
-                                    id,
-                                    method_span.unwrap_or(path.span),
-                                    diag,
+                // Check parent modules stability as well if the item the path refers to is itself
+                // stable. We only emit errors for unstable path segments if the item is stable
+                // or allowed because stability is often inherited, so the most common case is that
+                // both the segments and the item are unstable behind the same feature flag.
+                //
+                // We check here rather than in `visit_path_segment` to prevent visiting the last
+                // path segment twice
+                //
+                // We include special cases via #[rustc_allowed_through_unstable_modules] for items
+                // that were accidentally stabilized through unstable paths before this check was
+                // added, such as `core::intrinsics::transmute`
+                let parents = path.segments.iter().rev().skip(1);
+                for path_segment in parents {
+                    if let Some(def_id) = path_segment.res.opt_def_id() {
+                        match is_allowed_through_unstable_modules {
+                            None => {
+                                // Emit a hard stability error if this path is not stable.
+
+                                // use `None` for id to prevent deprecation check
+                                self.tcx.check_stability_allow_unstable(
+                                    def_id,
+                                    None,
+                                    path.span,
+                                    None,
+                                    if is_unstable_reexport(self.tcx, id) {
+                                        AllowUnstable::Yes
+                                    } else {
+                                        AllowUnstable::No
+                                    },
+                                );
+                            }
+                            Some(deprecation) => {
+                                // Call the stability check directly so that we can control which
+                                // diagnostic is emitted.
+                                let eval_result = self.tcx.eval_stability_allow_unstable(
+                                    def_id,
+                                    None,
+                                    path.span,
+                                    None,
+                                    if is_unstable_reexport(self.tcx, id) {
+                                        AllowUnstable::Yes
+                                    } else {
+                                        AllowUnstable::No
+                                    },
                                 );
+                                let is_allowed = matches!(eval_result, EvalResult::Allow);
+                                if !is_allowed {
+                                    // Calculating message for lint involves calling `self.def_path_str`,
+                                    // which will by default invoke the expensive `visible_parent_map` query.
+                                    // Skip all that work if the lint is allowed anyway.
+                                    if self.tcx.lint_level_at_node(DEPRECATED, id).0
+                                        == lint::Level::Allow
+                                    {
+                                        return;
+                                    }
+                                    // Show a deprecation message.
+                                    let def_path =
+                                        with_no_trimmed_paths!(self.tcx.def_path_str(def_id));
+                                    let def_kind = self.tcx.def_descr(def_id);
+                                    let diag = Deprecated {
+                                        sub: None,
+                                        kind: def_kind.to_owned(),
+                                        path: def_path,
+                                        note: Some(deprecation),
+                                        since_kind: lint::DeprecatedSinceKind::InEffect,
+                                    };
+                                    self.tcx.emit_node_span_lint(
+                                        DEPRECATED,
+                                        id,
+                                        method_span.unwrap_or(path.span),
+                                        diag,
+                                    );
+                                }
                             }
                         }
                     }

library/core/src/intrinsics/mod.rs

@@ -78,7 +78,11 @@ pub mod simd;
 use crate::sync::atomic::{self, AtomicBool, AtomicI32, AtomicIsize, AtomicU32, Ordering};
 
 #[stable(feature = "drop_in_place", since = "1.8.0")]
-#[rustc_allowed_through_unstable_modules]
+#[cfg_attr(bootstrap, rustc_allowed_through_unstable_modules)]
+#[cfg_attr(
+    not(bootstrap),
+    rustc_allowed_through_unstable_modules = "import this function via `std::ptr` instead"
+)]
 #[deprecated(note = "no longer an intrinsic - use `ptr::drop_in_place` directly", since = "1.52.0")]
 #[inline]
 pub unsafe fn drop_in_place<T: ?Sized>(to_drop: *mut T) {

src/librustdoc/clean/types.rs

@@ -5,9 +5,7 @@ use std::{fmt, iter};
 
 use arrayvec::ArrayVec;
 use rustc_abi::{ExternAbi, VariantIdx};
-use rustc_attr_parsing::{
-    AllowedThroughUnstableModules, ConstStability, Deprecation, Stability, StableSince,
-};
+use rustc_attr_parsing::{ConstStability, Deprecation, Stability, StableSince};
 use rustc_data_structures::fx::{FxHashSet, FxIndexMap, FxIndexSet};
 use rustc_hir::def::{CtorKind, DefKind, Res};
 use rustc_hir::def_id::{CrateNum, DefId, LOCAL_CRATE, LocalDefId};
@@ -411,15 +409,9 @@ impl Item {
                 ..
             } = stab.level
             {
-                let note = match note {
-                    AllowedThroughUnstableModules::WithDeprecation(note) => Some(note),
-                    // FIXME: Would be better to say *something* here about the *path* being
-                    // deprecated rather than the item.
-                    AllowedThroughUnstableModules::WithoutDeprecation => None,
-                };
                 Some(Deprecation {
                     since: rustc_attr_parsing::DeprecatedSince::Unspecified,
-                    note,
+                    note: Some(note),
                     suggestion: None,
                 })
             } else {

tests/rustdoc/inline_local/fully-stable-path-is-better.rs

@@ -16,10 +16,10 @@ pub mod stb1 {
 #[unstable(feature = "uns", issue = "135003")]
 pub mod uns {
     #[stable(since = "1.0", feature = "stb1")]
-    #[rustc_allowed_through_unstable_modules]
+    #[rustc_allowed_through_unstable_modules = "use stable path instead"]
     pub struct Inside1;
     #[stable(since = "1.0", feature = "stb2")]
-    #[rustc_allowed_through_unstable_modules]
+    #[rustc_allowed_through_unstable_modules = "use stable path instead"]
     pub struct Inside2;
 }
 

tests/rustdoc/stability.rs

@@ -85,7 +85,7 @@ pub mod stable_later {
 }
 
 #[stable(feature = "rust1", since = "1.0.0")]
-#[rustc_allowed_through_unstable_modules]
+#[rustc_allowed_through_unstable_modules = "use stable path instead"]
 pub mod stable_earlier1 {
     //@ has stability/stable_earlier1/struct.StableInUnstable.html \
     //      '//div[@class="main-heading"]//span[@class="since"]' '1.0.0'

tests/ui/error-codes/E0789.rs

@@ -4,7 +4,7 @@
 #![feature(staged_api)]
 #![unstable(feature = "foo_module", reason = "...", issue = "123")]
 
-#[rustc_allowed_through_unstable_modules]
+#[rustc_allowed_through_unstable_modules = "use stable path instead"]
 // #[stable(feature = "foo", since = "1.0")]
 struct Foo;
 //~^ ERROR `rustc_allowed_through_unstable_modules` attribute must be paired with a `stable` attribute

tests/ui/stability-attribute/allowed-through-unstable.rs

@@ -5,6 +5,5 @@
 
 extern crate allowed_through_unstable_core;
 
-use allowed_through_unstable_core::unstable_module::OldStableTraitAllowedThoughUnstable;
-use allowed_through_unstable_core::unstable_module::OldStableTraitAllowedThoughUnstableWithDeprecation; //~WARN use of deprecated module `allowed_through_unstable_core::unstable_module`: use the new path instead
+use allowed_through_unstable_core::unstable_module::OldStableTraitAllowedThoughUnstable; //~WARN use of deprecated module `allowed_through_unstable_core::unstable_module`: use the new path instead
 use allowed_through_unstable_core::unstable_module::NewStableTraitNotAllowedThroughUnstable; //~ ERROR use of unstable library feature `unstable_test_feature`