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

deprecate std::intrinsics::transmute etc, use std::mem::* instead #135003

Merged
merged 4 commits into from
Jan 15, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions compiler/rustc_ast/src/attr/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -723,6 +723,8 @@ impl MetaItemLit {
pub trait AttributeExt: Debug {
fn id(&self) -> AttrId;

/// For a single-segment attribute (i.e., `#[attr]` and not `#[path::atrr]`),
/// return the name of the attribute, else return the empty identifier.
fn name_or_empty(&self) -> Symbol {
self.ident().unwrap_or_else(Ident::empty).name
}
Expand Down
15 changes: 12 additions & 3 deletions compiler/rustc_attr_data_structures/src/stability.rs
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,16 @@ 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)]
Expand Down Expand Up @@ -137,9 +147,8 @@ pub enum StabilityLevel {
Stable {
/// Rust release which stabilized this feature.
since: StableSince,
/// Is this item allowed to be referred to on stable, despite being contained in unstable
/// modules?
allowed_through_unstable_modules: bool,
/// This is `Some` if this item allowed to be referred to on stable via unstable modules.
allowed_through_unstable_modules: Option<AllowedThroughUnstableModules>,
},
}

Expand Down
21 changes: 13 additions & 8 deletions compiler/rustc_attr_parsing/src/attributes/stability.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,8 @@ use rustc_ast::MetaItem;
use rustc_ast::attr::AttributeExt;
use rustc_ast_pretty::pprust;
use rustc_attr_data_structures::{
ConstStability, DefaultBodyStability, Stability, StabilityLevel, StableSince, UnstableReason,
VERSION_PLACEHOLDER,
AllowedThroughUnstableModules, ConstStability, DefaultBodyStability, Stability, StabilityLevel,
StableSince, UnstableReason, VERSION_PLACEHOLDER,
};
use rustc_errors::ErrorGuaranteed;
use rustc_session::Session;
Expand All @@ -24,11 +24,16 @@ pub fn find_stability(
item_sp: Span,
) -> Option<(Stability, Span)> {
let mut stab: Option<(Stability, Span)> = None;
let mut allowed_through_unstable_modules = false;
let mut allowed_through_unstable_modules = None;

for attr in attrs {
match attr.name_or_empty() {
sym::rustc_allowed_through_unstable_modules => allowed_through_unstable_modules = true,
sym::rustc_allowed_through_unstable_modules => {
allowed_through_unstable_modules = Some(match attr.value_str() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(to implement the way I suggested before)

you should use meta_item_list, find one that has name deprecated, then use value_str on that.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And then we'd silently accept arbitrary other meta items, right? Currently the rustc_attr! can describe the template! of the attribute which AFAIK gets us some checks that reject any other shapes; but for custom inner meta item lists I don't think there is a corresponding template!?

I'm not convinced that is worth the effort, but if the reviewer insists I can do it.

Some(msg) => AllowedThroughUnstableModules::WithDeprecation(msg),
None => AllowedThroughUnstableModules::WithoutDeprecation,
})
}
sym::unstable => {
if stab.is_some() {
sess.dcx().emit_err(session_diagnostics::MultipleStabilityLevels {
Expand Down Expand Up @@ -56,15 +61,15 @@ pub fn find_stability(
}
}

if allowed_through_unstable_modules {
if let Some(allowed_through_unstable_modules) = allowed_through_unstable_modules {
match &mut stab {
Some((
Stability {
level: StabilityLevel::Stable { allowed_through_unstable_modules, .. },
level: StabilityLevel::Stable { allowed_through_unstable_modules: in_stab, .. },
..
},
_,
)) => *allowed_through_unstable_modules = true,
)) => *in_stab = Some(allowed_through_unstable_modules),
_ => {
sess.dcx()
.emit_err(session_diagnostics::RustcAllowedUnstablePairing { span: item_sp });
Expand Down Expand Up @@ -283,7 +288,7 @@ fn parse_stability(sess: &Session, attr: &impl AttributeExt) -> Option<(Symbol,

match feature {
Ok(feature) => {
let level = StabilityLevel::Stable { since, allowed_through_unstable_modules: false };
let level = StabilityLevel::Stable { since, allowed_through_unstable_modules: None };
Some((feature, level))
}
Err(ErrorGuaranteed { .. }) => None,
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_feature/src/builtin_attrs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -623,7 +623,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),
rustc_allowed_through_unstable_modules, Normal, template!(Word, NameValueStr: "deprecation message"),
WarnFollowing, EncodeCrossCrate::No,
"rustc_allowed_through_unstable_modules special cases accidental stabilizations of stable items \
through unstable paths"
Expand Down
21 changes: 12 additions & 9 deletions compiler/rustc_middle/src/middle/stability.rs
Original file line number Diff line number Diff line change
Expand Up @@ -232,9 +232,18 @@ fn late_report_deprecation(
return;
}

let is_in_effect = depr.is_in_effect();
let lint = deprecation_lint(is_in_effect);

// 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 tcx.lint_level_at_node(lint, hir_id).0 == Level::Allow {
return;
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This first commit is unrelated, it just moves some logic to what I think is a more sensible location. I thought I'd use that logic for this PR but in the end I did not. I can make this a separate PR if you prefer.


let def_path = with_no_trimmed_paths!(tcx.def_path_str(def_id));
let def_kind = tcx.def_descr(def_id);
let is_in_effect = depr.is_in_effect();

let method_span = method_span.unwrap_or(span);
let suggestion =
Expand All @@ -250,7 +259,7 @@ fn late_report_deprecation(
note: depr.note,
since_kind: deprecated_since_kind(is_in_effect, depr.since),
};
tcx.emit_node_span_lint(deprecation_lint(is_in_effect), hir_id, method_span, diag);
tcx.emit_node_span_lint(lint, hir_id, method_span, diag);
}

/// Result of `TyCtxt::eval_stability`.
Expand Down Expand Up @@ -360,13 +369,7 @@ impl<'tcx> TyCtxt<'tcx> {
// hierarchy.
let depr_attr = &depr_entry.attr;
if !skip || depr_attr.is_since_rustc_version() {
// Calculating message for lint involves calling `self.def_path_str`.
// Which by default to calculate visible path will invoke expensive `visible_parent_map` query.
// So we skip message calculation altogether, if lint is allowed.
let lint = deprecation_lint(depr_attr.is_in_effect());
if self.lint_level_at_node(lint, id).0 != Level::Allow {
late_report_deprecation(self, depr_attr, span, method_span, id, def_id);
}
late_report_deprecation(self, depr_attr, span, method_span, id, def_id);
}
};
}
Expand Down
136 changes: 97 additions & 39 deletions compiler/rustc_passes/src/stability.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,8 @@ use std::mem::replace;
use std::num::NonZero;

use rustc_attr_parsing::{
self as attr, ConstStability, DeprecatedSince, Stability, StabilityLevel, StableSince,
UnstableReason, VERSION_PLACEHOLDER,
self as attr, AllowedThroughUnstableModules, ConstStability, DeprecatedSince, Stability,
StabilityLevel, StableSince, UnstableReason, VERSION_PLACEHOLDER,
};
use rustc_data_structures::fx::FxIndexMap;
use rustc_data_structures::unord::{ExtendUnord, UnordMap, UnordSet};
Expand All @@ -20,11 +20,16 @@ use rustc_hir::{FieldDef, Item, ItemKind, TraitRef, Ty, TyKind, Variant};
use rustc_middle::hir::nested_filter;
use rustc_middle::middle::lib_features::{FeatureStability, LibFeatures};
use rustc_middle::middle::privacy::EffectiveVisibilities;
use rustc_middle::middle::stability::{AllowUnstable, DeprecationEntry, Index};
use rustc_middle::middle::stability::{
AllowUnstable, Deprecated, DeprecationEntry, EvalResult, Index,
};
use rustc_middle::query::Providers;
use rustc_middle::ty::TyCtxt;
use rustc_middle::ty::print::with_no_trimmed_paths;
use rustc_session::lint;
use rustc_session::lint::builtin::{INEFFECTIVE_UNSTABLE_TRAIT_IMPL, USELESS_DEPRECATED};
use rustc_session::lint::builtin::{
DEPRECATED, INEFFECTIVE_UNSTABLE_TRAIT_IMPL, USELESS_DEPRECATED,
};
use rustc_span::{Span, Symbol, sym};
use tracing::{debug, info};

Expand Down Expand Up @@ -844,42 +849,95 @@ impl<'tcx> Visitor<'tcx> for Checker<'tcx> {
},
);

let is_allowed_through_unstable_modules = |def_id| {
self.tcx.lookup_stability(def_id).is_some_and(|stab| match stab.level {
StabilityLevel::Stable { allowed_through_unstable_modules, .. } => {
allowed_through_unstable_modules
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> =
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
},
);
}
}
_ => false,
})
};

if item_is_allowed && !is_allowed_through_unstable_modules(def_id) {
// 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() {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an unfortunate duplication of the loop above, but I didn't find a good way to deduplicate that code.

// 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,
);
}
}
}
}
}
Expand Down
24 changes: 20 additions & 4 deletions library/core/src/intrinsics/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1897,7 +1897,11 @@ pub const fn forget<T: ?Sized>(_: T) {
/// }
/// ```
#[stable(feature = "rust1", since = "1.0.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::mem` instead"
)]
#[rustc_const_stable(feature = "const_transmute", since = "1.56.0")]
#[rustc_diagnostic_item = "transmute"]
#[rustc_nounwind]
Expand Down Expand Up @@ -4325,7 +4329,11 @@ pub const fn ptr_metadata<P: ptr::Pointee<Metadata = M> + ?Sized, M>(_ptr: *cons
/// [`Vec::append`]: ../../std/vec/struct.Vec.html#method.append
#[doc(alias = "memcpy")]
#[stable(feature = "rust1", since = "1.0.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::mem` instead"
)]
#[rustc_const_stable(feature = "const_intrinsic_copy", since = "1.83.0")]
#[inline(always)]
#[cfg_attr(miri, track_caller)] // even without panics, this helps for Miri backtraces
Expand Down Expand Up @@ -4429,7 +4437,11 @@ pub const unsafe fn copy_nonoverlapping<T>(src: *const T, dst: *mut T, count: us
/// ```
#[doc(alias = "memmove")]
#[stable(feature = "rust1", since = "1.0.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::mem` instead"
)]
Comment on lines +4441 to +4444
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
#[cfg_attr(
not(bootstrap),
rustc_allowed_through_unstable_modules = "import this function via `std::mem` instead"
)]
#[cfg_attr(
not(bootstrap),
rustc_allowed_through_unstable_modules(deprecated = "import this function via `std::mem` instead")
)]

Copy link
Member Author

@RalfJung RalfJung Jan 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no idea how to implement that with reasonable effort, sorry. This is a rustc-internal attribute so it doesn't have to be super pretty.

#[rustc_const_stable(feature = "const_intrinsic_copy", since = "1.83.0")]
#[inline(always)]
#[cfg_attr(miri, track_caller)] // even without panics, this helps for Miri backtraces
Expand Down Expand Up @@ -4512,7 +4524,11 @@ pub const unsafe fn copy<T>(src: *const T, dst: *mut T, count: usize) {
/// ```
#[doc(alias = "memset")]
#[stable(feature = "rust1", since = "1.0.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::mem` instead"
)]
#[rustc_const_stable(feature = "const_ptr_write", since = "1.83.0")]
#[inline(always)]
#[cfg_attr(miri, track_caller)] // even without panics, this helps for Miri backtraces
Expand Down
16 changes: 11 additions & 5 deletions src/librustdoc/clean/types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,9 @@ use std::{fmt, iter};

use arrayvec::ArrayVec;
use rustc_abi::{ExternAbi, VariantIdx};
use rustc_attr_parsing::{ConstStability, Deprecation, Stability, StableSince};
use rustc_attr_parsing::{
AllowedThroughUnstableModules, 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};
Expand Down Expand Up @@ -406,15 +408,19 @@ impl Item {
// were never supposed to work at all.
let stab = self.stability(tcx)?;
if let rustc_attr_parsing::StabilityLevel::Stable {
allowed_through_unstable_modules: true,
allowed_through_unstable_modules: Some(note),
..
} = stab.level
{
Copy link
Member Author

@RalfJung RalfJung Jan 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@notriddle I am not sure what you mean by "translate" in your comment below. rustc itself also does not currently consider all of these paths deprecated (only the ones where a deprecation message has been set), but it seems like a good idea to have rustdoc de-emphasize all of them as a form of soft deprecation.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant to set note: Some(deprecation_message), because, if note were set, it would show up here.

image

Instead of just saying 👎 Deprecated, I'd like to show the same message that's shown in the compiler, since that points you at mem::transmute.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not all of these will have a message though, so things will still look odd for those that don't. It seems nice to have rustdoc de-emphasize these items even before we have a proper deprecation message for them, but I am not sure how to do that. I can easily change the code here to only show "deprecated" when there is a message, but then the "wrong" path would show up in search results again. Would be nice to have a sort of soft deprecation where rustc doesn't warn but also rustdoc doesn't point people there.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you give an example where (1) no deprecation message is being added (2) the "bad" path is shorter than, or the same length as, the stable one? If there aren't any, then nothing needs to be done and we can just remove the deprecation tag whenever the message is None, because intra-doc links and search results both already prefer shorter paths.

Copy link
Member Author

@RalfJung RalfJung Jan 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

intra-doc links and search results both already prefer shorter paths.

That doesn't seem to be true, given that intrinsics::transmute is preferred over mem::transmute (and even with your patch, the longer path is preferred for intra-doc links).

Copy link
Contributor

@notriddle notriddle Jan 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah. I haven't yet checked the other uses of allowed_through_unstable_modules; I plan to do that once this PR lands.

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 {
// FIXME(#131676, #135003): when a note is added to this stability tag,
// translate it here
since: rustc_attr_parsing::DeprecatedSince::Unspecified,
note: None,
note,
suggestion: None,
})
} else {
Expand Down
Loading
Loading