Skip to content

Commit

Permalink
Rollup merge of rust-lang#137455 - compiler-errors:drop-lint-dtor, r=…
Browse files Browse the repository at this point in the history
…oli-obk

Reuse machinery from `tail_expr_drop_order` for `if_let_rescope`

Namely, it defines its own `extract_component_with_significant_dtor` which is a bit more accurate than `Ty::has_significant_drop`, since it has a hard-coded list of types from the ecosystem which are opted out of the lint.[^a]

Also, since we extract the dtors themselves, adopt the same *label* we use in `tail_expr_drop_order` to point out the destructor impl. This makes it much clear what's actually being dropped, so it should be clearer to know when it's a false positive.

This conflicts with rust-lang#137444, but I will rebase whichever lands first.

[^a]: Side-note, it's kinda a shame that now there are two functions that presumably do the same thing. But this isn't my circus, nor are these my monkeys.
  • Loading branch information
matthiaskrgr authored Feb 27, 2025
2 parents a334f8b + 864cca8 commit 55073e8
Show file tree
Hide file tree
Showing 11 changed files with 425 additions and 186 deletions.
1 change: 1 addition & 0 deletions Cargo.lock
Original file line number Diff line number Diff line change
Expand Up @@ -3916,6 +3916,7 @@ dependencies = [
"rustc_target",
"rustc_trait_selection",
"rustc_type_ir",
"smallvec",
"tracing",
"unicode-security",
]
Expand Down
1 change: 1 addition & 0 deletions compiler/rustc_lint/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ rustc_span = { path = "../rustc_span" }
rustc_target = { path = "../rustc_target" }
rustc_trait_selection = { path = "../rustc_trait_selection" }
rustc_type_ir = { path = "../rustc_type_ir" }
smallvec = { version = "1.8.1", features = ["union", "may_dangle"] }
tracing = "0.1"
unicode-security = "0.1.0"
# tidy-alphabetical-end
5 changes: 5 additions & 0 deletions compiler/rustc_lint/messages.ftl
Original file line number Diff line number Diff line change
Expand Up @@ -333,6 +333,11 @@ lint_identifier_uncommon_codepoints = identifier contains {$codepoints_len ->
*[other] {" "}{$identifier_type}
} Unicode general security profile
lint_if_let_dtor = {$dtor_kind ->
[dyn] value may invoke a custom destructor because it contains a trait object
*[concrete] value invokes this custom destructor
}
lint_if_let_rescope = `if let` assigns a shorter lifetime since Edition 2024
.label = this value has a significant drop implementation which may observe a major change in drop order and requires your discretion
.help = the value is now dropped here in Edition 2024
Expand Down
77 changes: 59 additions & 18 deletions compiler/rustc_lint/src/if_let_rescope.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,13 +7,17 @@ use rustc_errors::{
Applicability, Diag, EmissionGuarantee, SubdiagMessageOp, Subdiagnostic, SuggestionStyle,
};
use rustc_hir::{self as hir, HirIdSet};
use rustc_macros::LintDiagnostic;
use rustc_middle::ty::TyCtxt;
use rustc_macros::{LintDiagnostic, Subdiagnostic};
use rustc_middle::ty::adjustment::Adjust;
use rustc_middle::ty::significant_drop_order::{
extract_component_with_significant_dtor, ty_dtor_span,
};
use rustc_middle::ty::{self, Ty, TyCtxt};
use rustc_session::lint::{FutureIncompatibilityReason, LintId};
use rustc_session::{declare_lint, impl_lint_pass};
use rustc_span::Span;
use rustc_span::edition::Edition;
use rustc_span::{DUMMY_SP, Span};
use smallvec::SmallVec;

use crate::{LateContext, LateLintPass};

Expand Down Expand Up @@ -130,13 +134,15 @@ impl IfLetRescope {
hir::ExprKind::If(_cond, _conseq, Some(alt)) => alt.span.shrink_to_hi(),
_ => return,
};
let mut seen_dyn = false;
let mut add_bracket_to_match_head = match_head_needs_bracket(tcx, expr);
let mut significant_droppers = vec![];
let mut lifetime_ends = vec![];
let mut closing_brackets = 0;
let mut alt_heads = vec![];
let mut match_heads = vec![];
let mut consequent_heads = vec![];
let mut destructors = vec![];
let mut first_if_to_lint = None;
let mut first_if_to_rewrite = false;
let mut empty_alt = false;
Expand All @@ -160,11 +166,25 @@ impl IfLetRescope {
let before_conseq = conseq.span.shrink_to_lo();
let lifetime_end = source_map.end_point(conseq.span);

if let ControlFlow::Break(significant_dropper) =
if let ControlFlow::Break((drop_span, drop_tys)) =
(FindSignificantDropper { cx }).check_if_let_scrutinee(init)
{
destructors.extend(drop_tys.into_iter().filter_map(|ty| {
if let Some(span) = ty_dtor_span(tcx, ty) {
Some(DestructorLabel { span, dtor_kind: "concrete" })
} else if matches!(ty.kind(), ty::Dynamic(..)) {
if seen_dyn {
None
} else {
seen_dyn = true;
Some(DestructorLabel { span: DUMMY_SP, dtor_kind: "dyn" })
}
} else {
None
}
}));
first_if_to_lint = first_if_to_lint.or_else(|| Some((span, expr.hir_id)));
significant_droppers.push(significant_dropper);
significant_droppers.push(drop_span);
lifetime_ends.push(lifetime_end);
if ty_ascription.is_some()
|| !expr.span.can_be_used_for_suggestions()
Expand Down Expand Up @@ -227,6 +247,7 @@ impl IfLetRescope {
hir_id,
span,
IfLetRescopeLint {
destructors,
significant_droppers,
lifetime_ends,
rewrite: first_if_to_rewrite.then_some(IfLetRescopeRewrite {
Expand Down Expand Up @@ -288,6 +309,8 @@ impl<'tcx> LateLintPass<'tcx> for IfLetRescope {
#[derive(LintDiagnostic)]
#[diag(lint_if_let_rescope)]
struct IfLetRescopeLint {
#[subdiagnostic]
destructors: Vec<DestructorLabel>,
#[label]
significant_droppers: Vec<Span>,
#[help]
Expand Down Expand Up @@ -347,6 +370,14 @@ impl Subdiagnostic for IfLetRescopeRewrite {
}
}

#[derive(Subdiagnostic)]
#[note(lint_if_let_dtor)]
struct DestructorLabel {
#[primary_span]
span: Span,
dtor_kind: &'static str,
}

struct AltHead(Span);

struct ConsequentRewrite {
Expand Down Expand Up @@ -374,7 +405,10 @@ impl<'tcx> FindSignificantDropper<'_, 'tcx> {
/// of the scrutinee itself, and also recurses into the expression to find any ref
/// exprs (or autoref) which would promote temporaries that would be scoped to the
/// end of this `if`.
fn check_if_let_scrutinee(&mut self, init: &'tcx hir::Expr<'tcx>) -> ControlFlow<Span> {
fn check_if_let_scrutinee(
&mut self,
init: &'tcx hir::Expr<'tcx>,
) -> ControlFlow<(Span, SmallVec<[Ty<'tcx>; 4]>)> {
self.check_promoted_temp_with_drop(init)?;
self.visit_expr(init)
}
Expand All @@ -385,28 +419,35 @@ impl<'tcx> FindSignificantDropper<'_, 'tcx> {
/// An expression is a promoted temporary if it has an addr taken (i.e. `&expr` or autoref)
/// or is the scrutinee of the `if let`, *and* the expression is not a place
/// expr, and it has a significant drop.
fn check_promoted_temp_with_drop(&self, expr: &'tcx hir::Expr<'tcx>) -> ControlFlow<Span> {
if !expr.is_place_expr(|base| {
fn check_promoted_temp_with_drop(
&self,
expr: &'tcx hir::Expr<'tcx>,
) -> ControlFlow<(Span, SmallVec<[Ty<'tcx>; 4]>)> {
if expr.is_place_expr(|base| {
self.cx
.typeck_results()
.adjustments()
.get(base.hir_id)
.is_some_and(|x| x.iter().any(|adj| matches!(adj.kind, Adjust::Deref(_))))
}) && self
.cx
.typeck_results()
.expr_ty(expr)
.has_significant_drop(self.cx.tcx, self.cx.typing_env())
{
ControlFlow::Break(expr.span)
} else {
ControlFlow::Continue(())
}) {
return ControlFlow::Continue(());
}

let drop_tys = extract_component_with_significant_dtor(
self.cx.tcx,
self.cx.typing_env(),
self.cx.typeck_results().expr_ty(expr),
);
if drop_tys.is_empty() {
return ControlFlow::Continue(());
}

ControlFlow::Break((expr.span, drop_tys))
}
}

impl<'tcx> Visitor<'tcx> for FindSignificantDropper<'_, 'tcx> {
type Result = ControlFlow<Span>;
type Result = ControlFlow<(Span, SmallVec<[Ty<'tcx>; 4]>)>;

fn visit_block(&mut self, b: &'tcx hir::Block<'tcx>) -> Self::Result {
// Blocks introduce temporary terminating scope for all of its
Expand Down
1 change: 1 addition & 0 deletions compiler/rustc_middle/src/ty/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,7 @@ pub mod normalize_erasing_regions;
pub mod pattern;
pub mod print;
pub mod relate;
pub mod significant_drop_order;
pub mod trait_def;
pub mod util;
pub mod visit;
Expand Down
172 changes: 172 additions & 0 deletions compiler/rustc_middle/src/ty/significant_drop_order.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,172 @@
use rustc_data_structures::fx::FxHashSet;
use rustc_data_structures::unord::UnordSet;
use rustc_hir::def_id::DefId;
use rustc_span::Span;
use smallvec::{SmallVec, smallvec};
use tracing::{debug, instrument};

use crate::ty::{self, Ty, TyCtxt};

/// An additional filter to exclude well-known types from the ecosystem
/// because their drops are trivial.
/// This returns additional types to check if the drops are delegated to those.
/// A typical example is `hashbrown::HashMap<K, V>`, whose drop is delegated to `K` and `V`.
fn true_significant_drop_ty<'tcx>(
tcx: TyCtxt<'tcx>,
ty: Ty<'tcx>,
) -> Option<SmallVec<[Ty<'tcx>; 2]>> {
if let ty::Adt(def, args) = ty.kind() {
let mut did = def.did();
let mut name_rev = vec![];
loop {
let key = tcx.def_key(did);

match key.disambiguated_data.data {
rustc_hir::definitions::DefPathData::CrateRoot => {
name_rev.push(tcx.crate_name(did.krate))
}
rustc_hir::definitions::DefPathData::TypeNs(symbol) => name_rev.push(symbol),
_ => return None,
}
if let Some(parent) = key.parent {
did = DefId { krate: did.krate, index: parent };
} else {
break;
}
}
let name_str: Vec<_> = name_rev.iter().rev().map(|x| x.as_str()).collect();
debug!(?name_str);
match name_str[..] {
// These are the types from Rust core ecosystem
["syn" | "proc_macro2", ..]
| ["core" | "std", "task", "LocalWaker" | "Waker"]
| ["core" | "std", "task", "wake", "LocalWaker" | "Waker"] => Some(smallvec![]),
// These are important types from Rust ecosystem
["tracing", "instrument", "Instrumented"] | ["bytes", "Bytes"] => Some(smallvec![]),
["hashbrown", "raw", "RawTable" | "RawIntoIter"] => {
if let [ty, ..] = &***args
&& let Some(ty) = ty.as_type()
{
Some(smallvec![ty])
} else {
None
}
}
["hashbrown", "raw", "RawDrain"] => {
if let [_, ty, ..] = &***args
&& let Some(ty) = ty.as_type()
{
Some(smallvec![ty])
} else {
None
}
}
_ => None,
}
} else {
None
}
}

/// Returns the list of types with a "potentially sigificant" that may be dropped
/// by dropping a value of type `ty`.
#[instrument(level = "trace", skip(tcx, typing_env))]
pub fn extract_component_raw<'tcx>(
tcx: TyCtxt<'tcx>,
typing_env: ty::TypingEnv<'tcx>,
ty: Ty<'tcx>,
ty_seen: &mut UnordSet<Ty<'tcx>>,
) -> SmallVec<[Ty<'tcx>; 4]> {
// Droppiness does not depend on regions, so let us erase them.
let ty = tcx.try_normalize_erasing_regions(typing_env, ty).unwrap_or(ty);

let tys = tcx.list_significant_drop_tys(typing_env.as_query_input(ty));
debug!(?ty, "components");
let mut out_tys = smallvec![];
for ty in tys {
if let Some(tys) = true_significant_drop_ty(tcx, ty) {
// Some types can be further opened up because the drop is simply delegated
for ty in tys {
if ty_seen.insert(ty) {
out_tys.extend(extract_component_raw(tcx, typing_env, ty, ty_seen));
}
}
} else {
if ty_seen.insert(ty) {
out_tys.push(ty);
}
}
}
out_tys
}

#[instrument(level = "trace", skip(tcx, typing_env))]
pub fn extract_component_with_significant_dtor<'tcx>(
tcx: TyCtxt<'tcx>,
typing_env: ty::TypingEnv<'tcx>,
ty: Ty<'tcx>,
) -> SmallVec<[Ty<'tcx>; 4]> {
let mut tys = extract_component_raw(tcx, typing_env, ty, &mut Default::default());
let mut deduplicate = FxHashSet::default();
tys.retain(|oty| deduplicate.insert(*oty));
tys.into_iter().collect()
}

/// Extract the span of the custom destructor of a type
/// especially the span of the `impl Drop` header or its entire block
/// when we are working with current local crate.
#[instrument(level = "trace", skip(tcx))]
pub fn ty_dtor_span<'tcx>(tcx: TyCtxt<'tcx>, ty: Ty<'tcx>) -> Option<Span> {
match ty.kind() {
ty::Bool
| ty::Char
| ty::Int(_)
| ty::Uint(_)
| ty::Float(_)
| ty::Error(_)
| ty::Str
| ty::Never
| ty::RawPtr(_, _)
| ty::Ref(_, _, _)
| ty::FnPtr(_, _)
| ty::Tuple(_)
| ty::Dynamic(_, _, _)
| ty::Alias(_, _)
| ty::Bound(_, _)
| ty::Pat(_, _)
| ty::Placeholder(_)
| ty::Infer(_)
| ty::Slice(_)
| ty::Array(_, _)
| ty::UnsafeBinder(_) => None,

ty::Adt(adt_def, _) => {
let did = adt_def.did();
let try_local_did_span = |did: DefId| {
if let Some(local) = did.as_local() {
tcx.source_span(local)
} else {
tcx.def_span(did)
}
};
let dtor = if let Some(dtor) = tcx.adt_destructor(did) {
dtor.did
} else if let Some(dtor) = tcx.adt_async_destructor(did) {
dtor.future
} else {
return Some(try_local_did_span(did));
};
let def_key = tcx.def_key(dtor);
let Some(parent_index) = def_key.parent else { return Some(try_local_did_span(dtor)) };
let parent_did = DefId { index: parent_index, krate: dtor.krate };
Some(try_local_did_span(parent_did))
}
ty::Coroutine(did, _)
| ty::CoroutineWitness(did, _)
| ty::CoroutineClosure(did, _)
| ty::Closure(did, _)
| ty::FnDef(did, _)
| ty::Foreign(did) => Some(tcx.def_span(did)),
ty::Param(_) => None,
}
}
Loading

0 comments on commit 55073e8

Please sign in to comment.