From 7033468a67c0e39b2e34de2b43f78a29d101861d Mon Sep 17 00:00:00 2001 From: Michael Goulet Date: Thu, 26 Sep 2024 22:10:04 -0400 Subject: [PATCH 1/9] Mark some more types as having insignificant dtor --- library/alloc/src/ffi/c_str.rs | 1 + library/std/src/io/error/repr_bitpacked.rs | 1 + 2 files changed, 2 insertions(+) diff --git a/library/alloc/src/ffi/c_str.rs b/library/alloc/src/ffi/c_str.rs index 797d591a8b51a..d496899e72bcd 100644 --- a/library/alloc/src/ffi/c_str.rs +++ b/library/alloc/src/ffi/c_str.rs @@ -696,6 +696,7 @@ impl CString { // memory-unsafe code from working by accident. Inline // to prevent LLVM from optimizing it away in debug builds. #[stable(feature = "cstring_drop", since = "1.13.0")] +#[rustc_insignificant_dtor] impl Drop for CString { #[inline] fn drop(&mut self) { diff --git a/library/std/src/io/error/repr_bitpacked.rs b/library/std/src/io/error/repr_bitpacked.rs index 80ba8455df347..a839a2fbac117 100644 --- a/library/std/src/io/error/repr_bitpacked.rs +++ b/library/std/src/io/error/repr_bitpacked.rs @@ -124,6 +124,7 @@ const TAG_SIMPLE: usize = 0b11; /// is_unwind_safe::(); /// ``` #[repr(transparent)] +#[rustc_insignificant_dtor] pub(super) struct Repr(NonNull<()>, PhantomData>>); // All the types `Repr` stores internally are Send + Sync, and so is it. From 24290efafffb83514da4e37f5f42a962d0e9bb1f Mon Sep 17 00:00:00 2001 From: Michael Goulet Date: Fri, 27 Sep 2024 11:59:35 -0400 Subject: [PATCH 2/9] Mark some more smart pointers as insignificant --- library/alloc/src/boxed.rs | 1 + library/alloc/src/sync.rs | 1 + 2 files changed, 2 insertions(+) diff --git a/library/alloc/src/boxed.rs b/library/alloc/src/boxed.rs index 6421504b89640..5f20729568352 100644 --- a/library/alloc/src/boxed.rs +++ b/library/alloc/src/boxed.rs @@ -228,6 +228,7 @@ mod thin; #[lang = "owned_box"] #[fundamental] #[stable(feature = "rust1", since = "1.0.0")] +#[rustc_insignificant_dtor] // The declaration of the `Box` struct must be kept in sync with the // compiler or ICEs will happen. pub struct Box< diff --git a/library/alloc/src/sync.rs b/library/alloc/src/sync.rs index ced789c4f9248..5d099a49854af 100644 --- a/library/alloc/src/sync.rs +++ b/library/alloc/src/sync.rs @@ -237,6 +237,7 @@ macro_rules! acquire { /// [rc_examples]: crate::rc#examples #[cfg_attr(not(test), rustc_diagnostic_item = "Arc")] #[stable(feature = "rust1", since = "1.0.0")] +#[rustc_insignificant_dtor] pub struct Arc< T: ?Sized, #[unstable(feature = "allocator_api", issue = "32838")] A: Allocator = Global, From 1b9e5752bed7293206e70d485d80619b46fc8f9c Mon Sep 17 00:00:00 2001 From: Ding Xiang Fei Date: Mon, 2 Sep 2024 01:13:07 +0800 Subject: [PATCH 3/9] reduce false positives of tail-expr-drop-order from consumed values --- Cargo.lock | 1 + compiler/rustc_borrowck/src/lib.rs | 2 +- .../src/polonius/loan_invalidations.rs | 2 +- compiler/rustc_codegen_cranelift/src/base.rs | 2 +- compiler/rustc_codegen_ssa/src/mir/block.rs | 2 +- .../rustc_const_eval/src/interpret/step.rs | 2 +- .../rustc_hir_typeck/src/expr_use_visitor.rs | 44 ++ compiler/rustc_lint/messages.ftl | 3 - compiler/rustc_lint/src/lib.rs | 3 - .../rustc_lint/src/tail_expr_drop_order.rs | 306 ------------- compiler/rustc_lint_defs/src/builtin.rs | 79 ++++ compiler/rustc_middle/src/arena.rs | 1 + compiler/rustc_middle/src/mir/syntax.rs | 12 +- compiler/rustc_middle/src/mir/terminator.rs | 2 +- compiler/rustc_middle/src/mir/visit.rs | 1 + .../rustc_middle/src/ty/structural_impls.rs | 1 + .../src/build/custom/parse/instruction.rs | 1 + .../src/build/expr/as_rvalue.rs | 4 + compiler/rustc_mir_build/src/build/scope.rs | 49 ++- .../rustc_mir_dataflow/src/elaborate_drops.rs | 14 +- .../src/impls/initialized.rs | 3 +- compiler/rustc_mir_transform/Cargo.toml | 1 + compiler/rustc_mir_transform/messages.ftl | 3 + .../src/add_moves_for_packed_drops.rs | 4 +- compiler/rustc_mir_transform/src/coroutine.rs | 8 +- .../src/elaborate_drops.rs | 14 +- compiler/rustc_mir_transform/src/inline.rs | 4 +- compiler/rustc_mir_transform/src/lib.rs | 2 + .../src/lint_tail_expr_drop_order.rs | 401 ++++++++++++++++++ compiler/rustc_mir_transform/src/shim.rs | 4 + .../src/shim/async_destructor_ctor.rs | 1 + .../rustc_smir/src/rustc_smir/convert/mir.rs | 2 +- tests/ui/drop/lint-tail-expr-drop-order.rs | 62 ++- .../ui/drop/lint-tail-expr-drop-order.stderr | 61 ++- 34 files changed, 728 insertions(+), 373 deletions(-) delete mode 100644 compiler/rustc_lint/src/tail_expr_drop_order.rs create mode 100644 compiler/rustc_mir_transform/src/lint_tail_expr_drop_order.rs diff --git a/Cargo.lock b/Cargo.lock index 0d2ae4ee15fa1..64071edf19724 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4112,6 +4112,7 @@ dependencies = [ "rustc_hir", "rustc_index", "rustc_infer", + "rustc_lint", "rustc_macros", "rustc_middle", "rustc_mir_build", diff --git a/compiler/rustc_borrowck/src/lib.rs b/compiler/rustc_borrowck/src/lib.rs index a11eca0b9c746..ab3dffb534024 100644 --- a/compiler/rustc_borrowck/src/lib.rs +++ b/compiler/rustc_borrowck/src/lib.rs @@ -691,7 +691,7 @@ impl<'a, 'tcx, R> rustc_mir_dataflow::ResultsVisitor<'a, 'tcx, R> TerminatorKind::SwitchInt { discr, targets: _ } => { self.consume_operand(loc, (discr, span), state); } - TerminatorKind::Drop { place, target: _, unwind: _, replace } => { + TerminatorKind::Drop { place, target: _, scope: _, unwind: _, replace } => { debug!( "visit_terminator_drop \ loc: {:?} term: {:?} place: {:?} span: {:?}", diff --git a/compiler/rustc_borrowck/src/polonius/loan_invalidations.rs b/compiler/rustc_borrowck/src/polonius/loan_invalidations.rs index afd811a0efb43..35178fed160a5 100644 --- a/compiler/rustc_borrowck/src/polonius/loan_invalidations.rs +++ b/compiler/rustc_borrowck/src/polonius/loan_invalidations.rs @@ -103,7 +103,7 @@ impl<'a, 'tcx> Visitor<'tcx> for LoanInvalidationsGenerator<'a, 'tcx> { TerminatorKind::SwitchInt { discr, targets: _ } => { self.consume_operand(location, discr); } - TerminatorKind::Drop { place: drop_place, target: _, unwind: _, replace } => { + TerminatorKind::Drop { place: drop_place, target: _, scope: _, unwind: _, replace } => { let write_kind = if *replace { WriteKind::Replace } else { WriteKind::StorageDeadOrDrop }; self.access_place( diff --git a/compiler/rustc_codegen_cranelift/src/base.rs b/compiler/rustc_codegen_cranelift/src/base.rs index 1ce0aacab4998..9d4a71826a4e5 100644 --- a/compiler/rustc_codegen_cranelift/src/base.rs +++ b/compiler/rustc_codegen_cranelift/src/base.rs @@ -549,7 +549,7 @@ fn codegen_fn_body(fx: &mut FunctionCx<'_, '_, '_>, start_block: Block) { | TerminatorKind::CoroutineDrop => { bug!("shouldn't exist at codegen {:?}", bb_data.terminator()); } - TerminatorKind::Drop { place, target, unwind: _, replace: _ } => { + TerminatorKind::Drop { place, target, unwind: _, scope: _, replace: _ } => { let drop_place = codegen_place(fx, *place); crate::abi::codegen_drop(fx, source_info, drop_place, *target); } diff --git a/compiler/rustc_codegen_ssa/src/mir/block.rs b/compiler/rustc_codegen_ssa/src/mir/block.rs index 125d3b908c733..fbd7083686229 100644 --- a/compiler/rustc_codegen_ssa/src/mir/block.rs +++ b/compiler/rustc_codegen_ssa/src/mir/block.rs @@ -1322,7 +1322,7 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> { MergingSucc::False } - mir::TerminatorKind::Drop { place, target, unwind, replace: _ } => self + mir::TerminatorKind::Drop { place, target, unwind, scope: _, replace: _ } => self .codegen_drop_terminator( helper, bx, diff --git a/compiler/rustc_const_eval/src/interpret/step.rs b/compiler/rustc_const_eval/src/interpret/step.rs index 8e2367e0d214c..2175eb84b9ad0 100644 --- a/compiler/rustc_const_eval/src/interpret/step.rs +++ b/compiler/rustc_const_eval/src/interpret/step.rs @@ -528,7 +528,7 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> { } } - Drop { place, target, unwind, replace: _ } => { + Drop { place, target, unwind, scope: _, replace: _ } => { let place = self.eval_place(place)?; let instance = Instance::resolve_drop_in_place(*self.tcx, place.layout.ty); if let ty::InstanceKind::DropGlue(_, None) = instance.def { diff --git a/compiler/rustc_hir_typeck/src/expr_use_visitor.rs b/compiler/rustc_hir_typeck/src/expr_use_visitor.rs index bb5f351137305..8206e79b3e3c8 100644 --- a/compiler/rustc_hir_typeck/src/expr_use_visitor.rs +++ b/compiler/rustc_hir_typeck/src/expr_use_visitor.rs @@ -150,6 +150,50 @@ pub trait TypeInformationCtxt<'tcx> { fn tcx(&self) -> TyCtxt<'tcx>; } +impl<'tcx> TypeInformationCtxt<'tcx> for (TyCtxt<'tcx>, LocalDefId) { + type TypeckResults<'a> = &'tcx ty::TypeckResults<'tcx> + where + Self: 'a; + + type Error = !; + + fn typeck_results(&self) -> Self::TypeckResults<'_> { + self.0.typeck(self.1) + } + + fn resolve_vars_if_possible>>(&self, t: T) -> T { + t + } + + fn try_structurally_resolve_type(&self, _span: Span, ty: Ty<'tcx>) -> Ty<'tcx> { + ty + } + + fn report_error(&self, span: Span, msg: impl ToString) -> Self::Error { + span_bug!(span, "{}", msg.to_string()) + } + + fn error_reported_in_ty(&self, _ty: Ty<'tcx>) -> Result<(), Self::Error> { + Ok(()) + } + + fn tainted_by_errors(&self) -> Result<(), Self::Error> { + Ok(()) + } + + fn type_is_copy_modulo_regions(&self, ty: Ty<'tcx>) -> bool { + ty.is_copy_modulo_regions(self.0, self.0.param_env(self.1)) + } + + fn body_owner_def_id(&self) -> LocalDefId { + self.1 + } + + fn tcx(&self) -> TyCtxt<'tcx> { + self.0 + } +} + impl<'tcx> TypeInformationCtxt<'tcx> for &FnCtxt<'_, 'tcx> { type TypeckResults<'a> = Ref<'a, ty::TypeckResults<'tcx>> diff --git a/compiler/rustc_lint/messages.ftl b/compiler/rustc_lint/messages.ftl index a286ccb22c7a6..cbde615f1eda5 100644 --- a/compiler/rustc_lint/messages.ftl +++ b/compiler/rustc_lint/messages.ftl @@ -778,9 +778,6 @@ lint_suspicious_double_ref_clone = lint_suspicious_double_ref_deref = using `.deref()` on a double reference, which returns `{$ty}` instead of dereferencing the inner type -lint_tail_expr_drop_order = these values and local bindings have significant drop implementation that will have a different drop order from that of Edition 2021 - .label = these values have significant drop implementation and will observe changes in drop order under Edition 2024 - lint_trailing_semi_macro = trailing semicolon in macro used in expression position .note1 = macro invocations at the end of a block are treated as expressions .note2 = to ignore the value produced by the macro, add a semicolon after the invocation of `{$name}` diff --git a/compiler/rustc_lint/src/lib.rs b/compiler/rustc_lint/src/lib.rs index c74cb866f21fa..9b145535959ce 100644 --- a/compiler/rustc_lint/src/lib.rs +++ b/compiler/rustc_lint/src/lib.rs @@ -82,7 +82,6 @@ mod redundant_semicolon; mod reference_casting; mod shadowed_into_iter; mod static_mut_refs; -mod tail_expr_drop_order; mod traits; mod types; mod unit_bindings; @@ -123,7 +122,6 @@ use rustc_middle::ty::TyCtxt; use shadowed_into_iter::ShadowedIntoIter; pub use shadowed_into_iter::{ARRAY_INTO_ITER, BOXED_SLICE_INTO_ITER}; use static_mut_refs::*; -use tail_expr_drop_order::TailExprDropOrder; use traits::*; use types::*; use unit_bindings::*; @@ -248,7 +246,6 @@ late_lint_methods!( AsyncFnInTrait: AsyncFnInTrait, NonLocalDefinitions: NonLocalDefinitions::default(), ImplTraitOvercaptures: ImplTraitOvercaptures, - TailExprDropOrder: TailExprDropOrder, IfLetRescope: IfLetRescope::default(), StaticMutRefs: StaticMutRefs, UnqualifiedLocalImports: UnqualifiedLocalImports, diff --git a/compiler/rustc_lint/src/tail_expr_drop_order.rs b/compiler/rustc_lint/src/tail_expr_drop_order.rs deleted file mode 100644 index 44a36142ed480..0000000000000 --- a/compiler/rustc_lint/src/tail_expr_drop_order.rs +++ /dev/null @@ -1,306 +0,0 @@ -use std::mem::swap; - -use rustc_ast::UnOp; -use rustc_hir::def::Res; -use rustc_hir::intravisit::{self, Visitor}; -use rustc_hir::{self as hir, Block, Expr, ExprKind, LetStmt, Pat, PatKind, QPath, StmtKind}; -use rustc_macros::LintDiagnostic; -use rustc_middle::ty; -use rustc_session::lint::FutureIncompatibilityReason; -use rustc_session::{declare_lint, declare_lint_pass}; -use rustc_span::Span; -use rustc_span::edition::Edition; - -use crate::{LateContext, LateLintPass}; - -declare_lint! { - /// The `tail_expr_drop_order` lint looks for those values generated at the tail expression location, that of type - /// with a significant `Drop` implementation, such as locks. - /// In case there are also local variables of type with significant `Drop` implementation as well, - /// this lint warns you of a potential transposition in the drop order. - /// Your discretion on the new drop order introduced by Edition 2024 is required. - /// - /// ### Example - /// ```rust,edition2024 - /// #![feature(shorter_tail_lifetimes)] - /// #![warn(tail_expr_drop_order)] - /// struct Droppy(i32); - /// impl Droppy { - /// fn get(&self) -> i32 { - /// self.0 - /// } - /// } - /// impl Drop for Droppy { - /// fn drop(&mut self) { - /// // This is a custom destructor and it induces side-effects that is observable - /// // especially when the drop order at a tail expression changes. - /// println!("loud drop {}", self.0); - /// } - /// } - /// fn edition_2024() -> i32 { - /// let another_droppy = Droppy(0); - /// Droppy(1).get() - /// } - /// fn main() { - /// edition_2024(); - /// } - /// ``` - /// - /// {{produces}} - /// - /// ### Explanation - /// - /// In tail expression of blocks or function bodies, - /// values of type with significant `Drop` implementation has an ill-specified drop order - /// before Edition 2024 so that they are dropped only after dropping local variables. - /// Edition 2024 introduces a new rule with drop orders for them, - /// so that they are dropped first before dropping local variables. - /// - /// A significant `Drop::drop` destructor here refers to an explicit, arbitrary - /// implementation of the `Drop` trait on the type, with exceptions including `Vec`, - /// `Box`, `Rc`, `BTreeMap` and `HashMap` that are marked by the compiler otherwise - /// so long that the generic types have no significant destructor recursively. - /// In other words, a type has a significant drop destructor when it has a `Drop` implementation - /// or its destructor invokes a significant destructor on a type. - /// Since we cannot completely reason about the change by just inspecting the existence of - /// a significant destructor, this lint remains only a suggestion and is set to `allow` by default. - /// - /// This lint only points out the issue with `Droppy`, which will be dropped before `another_droppy` - /// does in Edition 2024. - /// No fix will be proposed by this lint. - /// However, the most probable fix is to hoist `Droppy` into its own local variable binding. - /// ```rust - /// struct Droppy(i32); - /// impl Droppy { - /// fn get(&self) -> i32 { - /// self.0 - /// } - /// } - /// fn edition_2024() -> i32 { - /// let value = Droppy(0); - /// let another_droppy = Droppy(1); - /// value.get() - /// } - /// ``` - pub TAIL_EXPR_DROP_ORDER, - Allow, - "Detect and warn on significant change in drop order in tail expression location", - @future_incompatible = FutureIncompatibleInfo { - reason: FutureIncompatibilityReason::EditionSemanticsChange(Edition::Edition2024), - reference: "issue #123739 ", - }; -} - -declare_lint_pass!(TailExprDropOrder => [TAIL_EXPR_DROP_ORDER]); - -impl TailExprDropOrder { - fn check_fn_or_closure<'tcx>( - cx: &LateContext<'tcx>, - fn_kind: hir::intravisit::FnKind<'tcx>, - body: &'tcx hir::Body<'tcx>, - def_id: rustc_span::def_id::LocalDefId, - ) { - let mut locals = vec![]; - if matches!(fn_kind, hir::intravisit::FnKind::Closure) { - for &capture in cx.tcx.closure_captures(def_id) { - if matches!(capture.info.capture_kind, ty::UpvarCapture::ByValue) - && capture.place.ty().has_significant_drop(cx.tcx, cx.param_env) - { - locals.push(capture.var_ident.span); - } - } - } - for param in body.params { - if cx - .typeck_results() - .node_type(param.hir_id) - .has_significant_drop(cx.tcx, cx.param_env) - { - locals.push(param.span); - } - } - if let hir::ExprKind::Block(block, _) = body.value.kind { - LintVisitor { cx, locals }.check_block_inner(block); - } else { - LintTailExpr { cx, locals: &locals, is_root_tail_expr: true }.visit_expr(body.value); - } - } -} - -impl<'tcx> LateLintPass<'tcx> for TailExprDropOrder { - fn check_fn( - &mut self, - cx: &LateContext<'tcx>, - fn_kind: hir::intravisit::FnKind<'tcx>, - _: &'tcx hir::FnDecl<'tcx>, - body: &'tcx hir::Body<'tcx>, - _: Span, - def_id: rustc_span::def_id::LocalDefId, - ) { - if cx.tcx.sess.at_least_rust_2024() && cx.tcx.features().shorter_tail_lifetimes { - Self::check_fn_or_closure(cx, fn_kind, body, def_id); - } - } -} - -struct LintVisitor<'a, 'tcx> { - cx: &'a LateContext<'tcx>, - // We only record locals that have significant drops - locals: Vec, -} - -struct LocalCollector<'a, 'tcx> { - cx: &'a LateContext<'tcx>, - locals: &'a mut Vec, -} - -impl<'a, 'tcx> Visitor<'tcx> for LocalCollector<'a, 'tcx> { - type Result = (); - fn visit_pat(&mut self, pat: &'tcx Pat<'tcx>) { - if let PatKind::Binding(_binding_mode, id, ident, pat) = pat.kind { - let ty = self.cx.typeck_results().node_type(id); - if ty.has_significant_drop(self.cx.tcx, self.cx.param_env) { - self.locals.push(ident.span); - } - if let Some(pat) = pat { - self.visit_pat(pat); - } - } else { - intravisit::walk_pat(self, pat); - } - } -} - -impl<'a, 'tcx> Visitor<'tcx> for LintVisitor<'a, 'tcx> { - fn visit_block(&mut self, block: &'tcx Block<'tcx>) { - let mut locals = <_>::default(); - swap(&mut locals, &mut self.locals); - self.check_block_inner(block); - swap(&mut locals, &mut self.locals); - } - fn visit_local(&mut self, local: &'tcx LetStmt<'tcx>) { - LocalCollector { cx: self.cx, locals: &mut self.locals }.visit_local(local); - } -} - -impl<'a, 'tcx> LintVisitor<'a, 'tcx> { - fn check_block_inner(&mut self, block: &Block<'tcx>) { - if !block.span.at_least_rust_2024() { - // We only lint for Edition 2024 onwards - return; - } - let Some(tail_expr) = block.expr else { return }; - for stmt in block.stmts { - match stmt.kind { - StmtKind::Let(let_stmt) => self.visit_local(let_stmt), - StmtKind::Item(_) => {} - StmtKind::Expr(e) | StmtKind::Semi(e) => self.visit_expr(e), - } - } - if self.locals.is_empty() { - return; - } - LintTailExpr { cx: self.cx, locals: &self.locals, is_root_tail_expr: true } - .visit_expr(tail_expr); - } -} - -struct LintTailExpr<'a, 'tcx> { - cx: &'a LateContext<'tcx>, - is_root_tail_expr: bool, - locals: &'a [Span], -} - -impl<'a, 'tcx> LintTailExpr<'a, 'tcx> { - fn expr_eventually_point_into_local(mut expr: &Expr<'tcx>) -> bool { - loop { - match expr.kind { - ExprKind::Index(access, _, _) | ExprKind::Field(access, _) => expr = access, - ExprKind::AddrOf(_, _, referee) | ExprKind::Unary(UnOp::Deref, referee) => { - expr = referee - } - ExprKind::Path(_) - if let ExprKind::Path(QPath::Resolved(_, path)) = expr.kind - && let [local, ..] = path.segments - && let Res::Local(_) = local.res => - { - return true; - } - _ => return false, - } - } - } - - fn expr_generates_nonlocal_droppy_value(&self, expr: &Expr<'tcx>) -> bool { - if Self::expr_eventually_point_into_local(expr) { - return false; - } - self.cx.typeck_results().expr_ty(expr).has_significant_drop(self.cx.tcx, self.cx.param_env) - } -} - -impl<'a, 'tcx> Visitor<'tcx> for LintTailExpr<'a, 'tcx> { - fn visit_expr(&mut self, expr: &'tcx Expr<'tcx>) { - if self.is_root_tail_expr { - self.is_root_tail_expr = false; - } else if self.expr_generates_nonlocal_droppy_value(expr) { - self.cx.tcx.emit_node_span_lint( - TAIL_EXPR_DROP_ORDER, - expr.hir_id, - expr.span, - TailExprDropOrderLint { spans: self.locals.to_vec() }, - ); - return; - } - match expr.kind { - ExprKind::Match(scrutinee, _, _) => self.visit_expr(scrutinee), - - ExprKind::ConstBlock(_) - | ExprKind::Array(_) - | ExprKind::Break(_, _) - | ExprKind::Continue(_) - | ExprKind::Ret(_) - | ExprKind::Become(_) - | ExprKind::Yield(_, _) - | ExprKind::InlineAsm(_) - | ExprKind::If(_, _, _) - | ExprKind::Loop(_, _, _, _) - | ExprKind::Closure(_) - | ExprKind::DropTemps(_) - | ExprKind::OffsetOf(_, _) - | ExprKind::Assign(_, _, _) - | ExprKind::AssignOp(_, _, _) - | ExprKind::Lit(_) - | ExprKind::Err(_) => {} - - ExprKind::MethodCall(_, _, _, _) - | ExprKind::Call(_, _) - | ExprKind::Type(_, _) - | ExprKind::Tup(_) - | ExprKind::Binary(_, _, _) - | ExprKind::Unary(_, _) - | ExprKind::Path(_) - | ExprKind::Let(_) - | ExprKind::Cast(_, _) - | ExprKind::Field(_, _) - | ExprKind::Index(_, _, _) - | ExprKind::AddrOf(_, _, _) - | ExprKind::Struct(_, _, _) - | ExprKind::Repeat(_, _) => intravisit::walk_expr(self, expr), - - ExprKind::Block(_, _) => { - // We do not lint further because the drop order stays the same inside the block - } - } - } - fn visit_block(&mut self, block: &'tcx Block<'tcx>) { - LintVisitor { cx: self.cx, locals: <_>::default() }.check_block_inner(block); - } -} - -#[derive(LintDiagnostic)] -#[diag(lint_tail_expr_drop_order)] -struct TailExprDropOrderLint { - #[label] - pub spans: Vec, -} diff --git a/compiler/rustc_lint_defs/src/builtin.rs b/compiler/rustc_lint_defs/src/builtin.rs index 549dc64a562b2..a4f9a4dd3730e 100644 --- a/compiler/rustc_lint_defs/src/builtin.rs +++ b/compiler/rustc_lint_defs/src/builtin.rs @@ -99,6 +99,7 @@ declare_lint_pass! { SINGLE_USE_LIFETIMES, SOFT_UNSTABLE, STABLE_FEATURES, + TAIL_EXPR_DROP_ORDER, TEST_UNSTABLE_LINT, TEXT_DIRECTION_CODEPOINT_IN_COMMENT, TRIVIAL_CASTS, @@ -4998,3 +4999,81 @@ declare_lint! { reference: "issue #124535 ", }; } + +declare_lint! { + /// The `tail_expr_drop_order` lint looks for those values generated at the tail expression location, that of type + /// with a significant `Drop` implementation, such as locks. + /// In case there are also local variables of type with significant `Drop` implementation as well, + /// this lint warns you of a potential transposition in the drop order. + /// Your discretion on the new drop order introduced by Edition 2024 is required. + /// + /// ### Example + /// ```rust,edition2024 + /// #![cfg_attr(not(bootstrap), feature(shorter_tail_lifetimes))] // Simplify this in bootstrap bump. + /// #![warn(tail_expr_drop_order)] + /// struct Droppy(i32); + /// impl Droppy { + /// fn get(&self) -> i32 { + /// self.0 + /// } + /// } + /// impl Drop for Droppy { + /// fn drop(&mut self) { + /// // This is a custom destructor and it induces side-effects that is observable + /// // especially when the drop order at a tail expression changes. + /// println!("loud drop {}", self.0); + /// } + /// } + /// fn edition_2024() -> i32 { + /// let another_droppy = Droppy(0); + /// Droppy(1).get() + /// } + /// fn main() { + /// edition_2024(); + /// } + /// ``` + /// + /// {{produces}} + /// + /// ### Explanation + /// + /// In tail expression of blocks or function bodies, + /// values of type with significant `Drop` implementation has an ill-specified drop order + /// before Edition 2024 so that they are dropped only after dropping local variables. + /// Edition 2024 introduces a new rule with drop orders for them, + /// so that they are dropped first before dropping local variables. + /// + /// A significant `Drop::drop` destructor here refers to an explicit, arbitrary + /// implementation of the `Drop` trait on the type, with exceptions including `Vec`, + /// `Box`, `Rc`, `BTreeMap` and `HashMap` that are marked by the compiler otherwise + /// so long that the generic types have no significant destructor recursively. + /// In other words, a type has a significant drop destructor when it has a `Drop` implementation + /// or its destructor invokes a significant destructor on a type. + /// Since we cannot completely reason about the change by just inspecting the existence of + /// a significant destructor, this lint remains only a suggestion and is set to `allow` by default. + /// + /// This lint only points out the issue with `Droppy`, which will be dropped before `another_droppy` + /// does in Edition 2024. + /// No fix will be proposed by this lint. + /// However, the most probable fix is to hoist `Droppy` into its own local variable binding. + /// ```rust + /// struct Droppy(i32); + /// impl Droppy { + /// fn get(&self) -> i32 { + /// self.0 + /// } + /// } + /// fn edition_2024() -> i32 { + /// let value = Droppy(0); + /// let another_droppy = Droppy(1); + /// value.get() + /// } + /// ``` + pub TAIL_EXPR_DROP_ORDER, + Allow, + "Detect and warn on significant change in drop order in tail expression location", + @future_incompatible = FutureIncompatibleInfo { + reason: FutureIncompatibilityReason::EditionSemanticsChange(Edition::Edition2024), + reference: "issue #123739 ", + }; +} diff --git a/compiler/rustc_middle/src/arena.rs b/compiler/rustc_middle/src/arena.rs index 7050a06b8dc22..c0461266de27e 100644 --- a/compiler/rustc_middle/src/arena.rs +++ b/compiler/rustc_middle/src/arena.rs @@ -114,6 +114,7 @@ macro_rules! arena_types { [decode] specialization_graph: rustc_middle::traits::specialization_graph::Graph, [] crate_inherent_impls: rustc_middle::ty::CrateInherentImpls, [] hir_owner_nodes: rustc_hir::OwnerNodes<'tcx>, + [] hir_id_set: rustc_hir::HirIdSet, ]); ) } diff --git a/compiler/rustc_middle/src/mir/syntax.rs b/compiler/rustc_middle/src/mir/syntax.rs index ae75f2d4187ba..dc081c0825ffd 100644 --- a/compiler/rustc_middle/src/mir/syntax.rs +++ b/compiler/rustc_middle/src/mir/syntax.rs @@ -5,14 +5,14 @@ use rustc_ast::{InlineAsmOptions, InlineAsmTemplatePiece, Mutability}; use rustc_data_structures::packed::Pu128; -use rustc_hir::CoroutineKind; use rustc_hir::def_id::DefId; +use rustc_hir::{CoroutineKind, ItemLocalId}; use rustc_index::IndexVec; use rustc_macros::{HashStable, TyDecodable, TyEncodable, TypeFoldable, TypeVisitable}; -use rustc_span::Span; use rustc_span::def_id::LocalDefId; use rustc_span::source_map::Spanned; use rustc_span::symbol::Symbol; +use rustc_span::Span; use rustc_target::abi::{FieldIdx, VariantIdx}; use rustc_target::asm::InlineAsmRegOrRegClass; use smallvec::SmallVec; @@ -705,7 +705,13 @@ pub enum TerminatorKind<'tcx> { /// The `replace` flag indicates whether this terminator was created as part of an assignment. /// This should only be used for diagnostic purposes, and does not have any operational /// meaning. - Drop { place: Place<'tcx>, target: BasicBlock, unwind: UnwindAction, replace: bool }, + Drop { + place: Place<'tcx>, + target: BasicBlock, + unwind: UnwindAction, + scope: Option<(DefId, ItemLocalId)>, + replace: bool, + }, /// Roughly speaking, evaluates the `func` operand and the arguments, and starts execution of /// the referred to function. The operand types must match the argument types of the function. diff --git a/compiler/rustc_middle/src/mir/terminator.rs b/compiler/rustc_middle/src/mir/terminator.rs index 783952fb9cb8a..7711d1d7b8c05 100644 --- a/compiler/rustc_middle/src/mir/terminator.rs +++ b/compiler/rustc_middle/src/mir/terminator.rs @@ -631,7 +631,7 @@ impl<'tcx> TerminatorKind<'tcx> { Goto { target } => TerminatorEdges::Single(target), Assert { target, unwind, expected: _, msg: _, cond: _ } - | Drop { target, unwind, place: _, replace: _ } + | Drop { target, unwind, place: _, scope: _, replace: _ } | FalseUnwind { real_target: target, unwind } => match unwind { UnwindAction::Cleanup(unwind) => TerminatorEdges::Double(target, unwind), UnwindAction::Continue | UnwindAction::Terminate(_) | UnwindAction::Unreachable => { diff --git a/compiler/rustc_middle/src/mir/visit.rs b/compiler/rustc_middle/src/mir/visit.rs index 64898a8495e26..7d65d8a0a98cc 100644 --- a/compiler/rustc_middle/src/mir/visit.rs +++ b/compiler/rustc_middle/src/mir/visit.rs @@ -511,6 +511,7 @@ macro_rules! make_mir_visitor { target: _, unwind: _, replace: _, + scope: _, } => { self.visit_place( place, diff --git a/compiler/rustc_middle/src/ty/structural_impls.rs b/compiler/rustc_middle/src/ty/structural_impls.rs index cd9ff9b60d859..e017b71c7944c 100644 --- a/compiler/rustc_middle/src/ty/structural_impls.rs +++ b/compiler/rustc_middle/src/ty/structural_impls.rs @@ -264,6 +264,7 @@ TrivialTypeTraversalImpls! { // interners). TrivialTypeTraversalAndLiftImpls! { ::rustc_hir::def_id::DefId, + ::rustc_hir::hir_id::ItemLocalId, ::rustc_hir::Safety, ::rustc_target::spec::abi::Abi, crate::ty::ClosureKind, diff --git a/compiler/rustc_mir_build/src/build/custom/parse/instruction.rs b/compiler/rustc_mir_build/src/build/custom/parse/instruction.rs index 9e3af89105294..9cd1e3f1574b3 100644 --- a/compiler/rustc_mir_build/src/build/custom/parse/instruction.rs +++ b/compiler/rustc_mir_build/src/build/custom/parse/instruction.rs @@ -70,6 +70,7 @@ impl<'a, 'tcx> ParseCtxt<'a, 'tcx> { target: self.parse_return_to(args[1])?, unwind: self.parse_unwind_action(args[2])?, replace: false, + scope: None, }) }, @call(mir_call, args) => { diff --git a/compiler/rustc_mir_build/src/build/expr/as_rvalue.rs b/compiler/rustc_mir_build/src/build/expr/as_rvalue.rs index fd949a533845e..dd7a3f0ec177b 100644 --- a/compiler/rustc_mir_build/src/build/expr/as_rvalue.rs +++ b/compiler/rustc_mir_build/src/build/expr/as_rvalue.rs @@ -716,11 +716,15 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { ); if let Operand::Move(to_drop) = value_operand { let success = this.cfg.start_new_block(); + let scope = scope + .and_then(|scope| scope.hir_id(this.region_scope_tree)) + .map(|scope| (scope.owner.to_def_id(), scope.local_id)); this.cfg.terminate(block, outer_source_info, TerminatorKind::Drop { place: to_drop, target: success, unwind: UnwindAction::Continue, replace: false, + scope, }); this.diverge_from(block); block = success; diff --git a/compiler/rustc_mir_build/src/build/scope.rs b/compiler/rustc_mir_build/src/build/scope.rs index dfc82f705a81b..755e4845816b5 100644 --- a/compiler/rustc_mir_build/src/build/scope.rs +++ b/compiler/rustc_mir_build/src/build/scope.rs @@ -151,6 +151,9 @@ struct DropData { /// Whether this is a value Drop or a StorageDead. kind: DropKind, + + /// Scope for which the data is obliged to drop + scope: Option, } #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] @@ -274,8 +277,12 @@ impl DropTree { // represents the block in the tree that should be jumped to once all // of the required drops have been performed. let fake_source_info = SourceInfo::outermost(DUMMY_SP); - let fake_data = - DropData { source_info: fake_source_info, local: Local::MAX, kind: DropKind::Storage }; + let fake_data = DropData { + source_info: fake_source_info, + local: Local::MAX, + kind: DropKind::Storage, + scope: None, + }; let drops = IndexVec::from_raw(vec![DropNode { data: fake_data, next: DropIdx::MAX }]); Self { drops, entry_points: Vec::new(), existing_drops_map: FxHashMap::default() } } @@ -311,12 +318,13 @@ impl DropTree { &mut self, cfg: &mut CFG<'tcx>, blocks: &mut IndexVec>, + scope_tree: ®ion::ScopeTree, ) { debug!("DropTree::build_mir(drops = {:#?})", self); assert_eq!(blocks.len(), self.drops.len()); self.assign_blocks::(cfg, blocks); - self.link_blocks(cfg, blocks) + self.link_blocks(cfg, blocks, scope_tree) } /// Assign blocks for all of the drops in the drop tree that need them. @@ -390,17 +398,24 @@ impl DropTree { &self, cfg: &mut CFG<'tcx>, blocks: &IndexSlice>, + scope_tree: ®ion::ScopeTree, ) { for (drop_idx, drop_node) in self.drops.iter_enumerated().rev() { let Some(block) = blocks[drop_idx] else { continue }; match drop_node.data.kind { DropKind::Value => { + let scope = drop_node + .data + .scope + .and_then(|scope| scope.hir_id(scope_tree)) + .map(|scope| (scope.owner.to_def_id(), scope.local_id)); let terminator = TerminatorKind::Drop { target: blocks[drop_node.next].unwrap(), // The caller will handle this if needed. unwind: UnwindAction::Terminate(UnwindTerminateReason::InCleanup), place: drop_node.data.local.into(), replace: false, + scope, }; cfg.terminate(block, drop_node.data.source_info, terminator); } @@ -763,7 +778,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { let local = place.as_local().unwrap_or_else(|| bug!("projection in tail call args")); - Some(DropData { source_info, local, kind: DropKind::Value }) + Some(DropData { source_info, local, kind: DropKind::Value, scope: None }) } Operand::Constant(_) => None, }) @@ -802,11 +817,16 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { unwind_drops.add_entry_point(block, unwind_entry_point); let next = self.cfg.start_new_block(); + let scope = scope + .region_scope + .hir_id(self.region_scope_tree) + .map(|scope| (scope.owner.to_def_id(), scope.local_id)); self.cfg.terminate(block, source_info, TerminatorKind::Drop { place: local.into(), target: next, unwind: UnwindAction::Continue, replace: false, + scope, }); block = next; } @@ -841,6 +861,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { unwind_to, is_coroutine && needs_cleanup, self.arg_count, + self.region_scope_tree, ) .into_block() } @@ -1089,6 +1110,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { source_info: SourceInfo { span: scope_end, scope: scope.source_scope }, local, kind: drop_kind, + scope: Some(region_scope), }); return; @@ -1280,6 +1302,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { target: assign, unwind: UnwindAction::Cleanup(assign_unwind), replace: true, + scope: None, }); self.diverge_from(block); @@ -1335,6 +1358,7 @@ fn build_scope_drops<'tcx>( mut unwind_to: DropIdx, storage_dead_on_unwind: bool, arg_count: usize, + scope_tree: ®ion::ScopeTree, ) -> BlockAnd<()> { debug!("build_scope_drops({:?} -> {:?})", block, scope); @@ -1381,11 +1405,16 @@ fn build_scope_drops<'tcx>( unwind_drops.add_entry_point(block, unwind_to); let next = cfg.start_new_block(); + let scope = drop_data + .scope + .and_then(|scope| scope.hir_id(scope_tree)) + .map(|scope| (scope.owner.to_def_id(), scope.local_id)); cfg.terminate(block, source_info, TerminatorKind::Drop { place: local.into(), target: next, unwind: UnwindAction::Continue, replace: false, + scope, }); block = next; } @@ -1419,7 +1448,7 @@ impl<'a, 'tcx: 'a> Builder<'a, 'tcx> { let mut blocks = IndexVec::from_elem(None, &drops.drops); blocks[ROOT_NODE] = continue_block; - drops.build_mir::(&mut self.cfg, &mut blocks); + drops.build_mir::(&mut self.cfg, &mut blocks, self.region_scope_tree); let is_coroutine = self.coroutine.is_some(); // Link the exit drop tree to unwind drop tree. @@ -1466,6 +1495,7 @@ impl<'a, 'tcx: 'a> Builder<'a, 'tcx> { &mut self.scopes.unwind_drops, self.fn_span, &mut None, + self.region_scope_tree, ); } } @@ -1476,7 +1506,7 @@ impl<'a, 'tcx: 'a> Builder<'a, 'tcx> { let cfg = &mut self.cfg; let fn_span = self.fn_span; let mut blocks = IndexVec::from_elem(None, &drops.drops); - drops.build_mir::(cfg, &mut blocks); + drops.build_mir::(cfg, &mut blocks, self.region_scope_tree); if let Some(root_block) = blocks[ROOT_NODE] { cfg.terminate( root_block, @@ -1488,7 +1518,7 @@ impl<'a, 'tcx: 'a> Builder<'a, 'tcx> { // Build the drop tree for unwinding in the normal control flow paths. let resume_block = &mut None; let unwind_drops = &mut self.scopes.unwind_drops; - Self::build_unwind_tree(cfg, unwind_drops, fn_span, resume_block); + Self::build_unwind_tree(cfg, unwind_drops, fn_span, resume_block, self.region_scope_tree); // Build the drop tree for unwinding when dropping a suspended // coroutine. @@ -1503,7 +1533,7 @@ impl<'a, 'tcx: 'a> Builder<'a, 'tcx> { drops.entry_points.push((drop_node.next, blocks[drop_idx].unwrap())); } } - Self::build_unwind_tree(cfg, drops, fn_span, resume_block); + Self::build_unwind_tree(cfg, drops, fn_span, resume_block, self.region_scope_tree); } fn build_unwind_tree( @@ -1511,10 +1541,11 @@ impl<'a, 'tcx: 'a> Builder<'a, 'tcx> { drops: &mut DropTree, fn_span: Span, resume_block: &mut Option, + scope_tree: ®ion::ScopeTree, ) { let mut blocks = IndexVec::from_elem(None, &drops.drops); blocks[ROOT_NODE] = *resume_block; - drops.build_mir::(cfg, &mut blocks); + drops.build_mir::(cfg, &mut blocks, scope_tree); if let (None, Some(resume)) = (*resume_block, blocks[ROOT_NODE]) { cfg.terminate(resume, SourceInfo::outermost(fn_span), TerminatorKind::UnwindResume); diff --git a/compiler/rustc_mir_dataflow/src/elaborate_drops.rs b/compiler/rustc_mir_dataflow/src/elaborate_drops.rs index f2541c37167f0..3e15d91dedca8 100644 --- a/compiler/rustc_mir_dataflow/src/elaborate_drops.rs +++ b/compiler/rustc_mir_dataflow/src/elaborate_drops.rs @@ -1,5 +1,7 @@ use std::{fmt, iter}; +use rustc_hir::ItemLocalId; +use rustc_hir::def_id::DefId; use rustc_hir::lang_items::LangItem; use rustc_index::Idx; use rustc_middle::mir::patch::MirPatch; @@ -167,6 +169,10 @@ where path: D::Path, succ: BasicBlock, unwind: Unwind, + + /// Scope from which the drop is obliged + /// which is the HIR node for the "scope" + scope: Option<(DefId, ItemLocalId)>, } /// "Elaborates" a drop of `place`/`path` and patches `bb`'s terminator to execute it. @@ -185,11 +191,12 @@ pub fn elaborate_drop<'b, 'tcx, D>( succ: BasicBlock, unwind: Unwind, bb: BasicBlock, + scope: Option<(DefId, ItemLocalId)>, ) where D: DropElaborator<'b, 'tcx>, 'tcx: 'b, { - DropCtxt { elaborator, source_info, place, path, succ, unwind }.elaborate_drop(bb) + DropCtxt { elaborator, source_info, place, scope, path, succ, unwind }.elaborate_drop(bb) } impl<'a, 'b, 'tcx, D> DropCtxt<'a, 'b, 'tcx, D> @@ -238,6 +245,7 @@ where target: self.succ, unwind: self.unwind.into_action(), replace: false, + scope: self.scope, }); } DropStyle::Conditional => { @@ -299,6 +307,7 @@ where place, succ, unwind, + scope: self.scope, } .elaborated_drop_block() } else { @@ -313,6 +322,7 @@ where // Using `self.path` here to condition the drop on // our own drop flag. path: self.path, + scope: self.scope, } .complete_drop(succ, unwind) } @@ -734,6 +744,7 @@ where target: loop_block, unwind: unwind.into_action(), replace: false, + scope: self.scope, }); loop_block @@ -914,6 +925,7 @@ where target, unwind: unwind.into_action(), replace: false, + scope: self.scope, }; self.new_block(unwind, block) } diff --git a/compiler/rustc_mir_dataflow/src/impls/initialized.rs b/compiler/rustc_mir_dataflow/src/impls/initialized.rs index 25e8726cf6162..6e1d5bff30331 100644 --- a/compiler/rustc_mir_dataflow/src/impls/initialized.rs +++ b/compiler/rustc_mir_dataflow/src/impls/initialized.rs @@ -368,7 +368,8 @@ impl<'tcx> GenKillAnalysis<'tcx> for MaybeInitializedPlaces<'_, 'tcx> { ) -> TerminatorEdges<'mir, 'tcx> { let mut edges = terminator.edges(); if self.skip_unreachable_unwind - && let mir::TerminatorKind::Drop { target, unwind, place, replace: _ } = terminator.kind + && let mir::TerminatorKind::Drop { target, unwind, place, scope: _, replace: _ } = + terminator.kind && matches!(unwind, mir::UnwindAction::Cleanup(_)) && self.is_unwind_dead(place, state) { diff --git a/compiler/rustc_mir_transform/Cargo.toml b/compiler/rustc_mir_transform/Cargo.toml index 07ca51a67aefb..f911d738810cb 100644 --- a/compiler/rustc_mir_transform/Cargo.toml +++ b/compiler/rustc_mir_transform/Cargo.toml @@ -17,6 +17,7 @@ rustc_fluent_macro = { path = "../rustc_fluent_macro" } rustc_hir = { path = "../rustc_hir" } rustc_index = { path = "../rustc_index" } rustc_infer = { path = "../rustc_infer" } +rustc_lint = { path = "../rustc_lint" } rustc_macros = { path = "../rustc_macros" } rustc_middle = { path = "../rustc_middle" } rustc_mir_build = { path = "../rustc_mir_build" } diff --git a/compiler/rustc_mir_transform/messages.ftl b/compiler/rustc_mir_transform/messages.ftl index f9b79d72b0504..78e8062275d90 100644 --- a/compiler/rustc_mir_transform/messages.ftl +++ b/compiler/rustc_mir_transform/messages.ftl @@ -23,6 +23,9 @@ mir_transform_must_not_suspend = {$pre}`{$def_path}`{$post} held across a suspen .help = consider using a block (`{"{ ... }"}`) to shrink the value's scope, ending before the suspend point mir_transform_operation_will_panic = this operation will panic at runtime +mir_transform_tail_expr_drop_order = this value of type `{$ty}` has significant drop implementation that will have a different drop order from that of Edition 2021 + .label = these local bindings with significant drop implementation may observe changes in drop order under Edition 2024 + mir_transform_unaligned_packed_ref = reference to packed field is unaligned .note = packed structs are only aligned by one byte, and many modern architectures penalize unaligned field accesses .note_ub = creating a misaligned reference is undefined behavior (even if that reference is never dereferenced) diff --git a/compiler/rustc_mir_transform/src/add_moves_for_packed_drops.rs b/compiler/rustc_mir_transform/src/add_moves_for_packed_drops.rs index 559df222a5040..fe87b807d6bb6 100644 --- a/compiler/rustc_mir_transform/src/add_moves_for_packed_drops.rs +++ b/compiler/rustc_mir_transform/src/add_moves_for_packed_drops.rs @@ -79,7 +79,8 @@ fn add_move_for_packed_drop<'tcx>( is_cleanup: bool, ) { debug!("add_move_for_packed_drop({:?} @ {:?})", terminator, loc); - let TerminatorKind::Drop { ref place, target, unwind, replace } = terminator.kind else { + let TerminatorKind::Drop { ref place, target, unwind, scope: _, replace } = terminator.kind + else { unreachable!(); }; @@ -100,5 +101,6 @@ fn add_move_for_packed_drop<'tcx>( target: storage_dead_block, unwind, replace, + scope: None, }); } diff --git a/compiler/rustc_mir_transform/src/coroutine.rs b/compiler/rustc_mir_transform/src/coroutine.rs index 8f032728f6be0..3696147395cdd 100644 --- a/compiler/rustc_mir_transform/src/coroutine.rs +++ b/compiler/rustc_mir_transform/src/coroutine.rs @@ -1083,15 +1083,15 @@ fn elaborate_coroutine_drops<'tcx>(tcx: TyCtxt<'tcx>, body: &mut Body<'tcx>) { let mut elaborator = DropShimElaborator { body, patch: MirPatch::new(body), tcx, param_env }; for (block, block_data) in body.basic_blocks.iter_enumerated() { - let (target, unwind, source_info) = match block_data.terminator() { + let (target, unwind, source_info, &scope) = match block_data.terminator() { Terminator { source_info, - kind: TerminatorKind::Drop { place, target, unwind, replace: _ }, + kind: TerminatorKind::Drop { place, target, unwind, scope, replace: _ }, } => { if let Some(local) = place.as_local() && local == SELF_ARG { - (target, unwind, source_info) + (target, unwind, source_info, scope) } else { continue; } @@ -1116,6 +1116,7 @@ fn elaborate_coroutine_drops<'tcx>(tcx: TyCtxt<'tcx>, body: &mut Body<'tcx>) { *target, unwind, block, + scope, ); } elaborator.patch.apply(body); @@ -1374,6 +1375,7 @@ fn insert_clean_drop(body: &mut Body<'_>) -> BasicBlock { target: return_block, unwind: UnwindAction::Continue, replace: false, + scope: None, }; let source_info = SourceInfo::outermost(body.span); diff --git a/compiler/rustc_mir_transform/src/elaborate_drops.rs b/compiler/rustc_mir_transform/src/elaborate_drops.rs index c35aec42408ca..ce55c93c4701d 100644 --- a/compiler/rustc_mir_transform/src/elaborate_drops.rs +++ b/compiler/rustc_mir_transform/src/elaborate_drops.rs @@ -330,7 +330,8 @@ impl<'a, 'tcx> ElaborateDropsCtxt<'a, 'tcx> { // This function should mirror what `collect_drop_flags` does. for (bb, data) in self.body.basic_blocks.iter_enumerated() { let terminator = data.terminator(); - let TerminatorKind::Drop { place, target, unwind, replace } = terminator.kind else { + let TerminatorKind::Drop { place, target, unwind, scope, replace } = terminator.kind + else { continue; }; @@ -366,7 +367,16 @@ impl<'a, 'tcx> ElaborateDropsCtxt<'a, 'tcx> { } }; self.init_data.seek_before(self.body.terminator_loc(bb)); - elaborate_drop(self, terminator.source_info, place, path, target, unwind, bb) + elaborate_drop( + self, + terminator.source_info, + place, + path, + target, + unwind, + bb, + scope, + ) } LookupResult::Parent(None) => {} LookupResult::Parent(Some(_)) => { diff --git a/compiler/rustc_mir_transform/src/inline.rs b/compiler/rustc_mir_transform/src/inline.rs index c9f24764cc2a1..a9718fabbf868 100644 --- a/compiler/rustc_mir_transform/src/inline.rs +++ b/compiler/rustc_mir_transform/src/inline.rs @@ -535,7 +535,9 @@ impl<'tcx> Inliner<'tcx> { checker.visit_basic_block_data(bb, blk); let term = blk.terminator(); - if let TerminatorKind::Drop { ref place, target, unwind, replace: _ } = term.kind { + if let TerminatorKind::Drop { ref place, target, unwind, scope: _, replace: _ } = + term.kind + { work_list.push(target); // If the place doesn't actually need dropping, treat it like a regular goto. diff --git a/compiler/rustc_mir_transform/src/lib.rs b/compiler/rustc_mir_transform/src/lib.rs index 4c090665992bd..5572ce3af2f2a 100644 --- a/compiler/rustc_mir_transform/src/lib.rs +++ b/compiler/rustc_mir_transform/src/lib.rs @@ -103,6 +103,7 @@ mod sanity_check; mod shim; mod ssa; // This pass is public to allow external drivers to perform MIR cleanup +mod lint_tail_expr_drop_order; pub mod simplify; mod simplify_branches; mod simplify_comparison_integral; @@ -356,6 +357,7 @@ fn mir_promoted( &[&promote_pass, &simplify::SimplifyCfg::PromoteConsts, &coverage::InstrumentCoverage], Some(MirPhase::Analysis(AnalysisPhase::Initial)), ); + lint_tail_expr_drop_order::run_lint(tcx, def, &body); let promoted = promote_pass.promoted_fragments.into_inner(); (tcx.alloc_steal_mir(body), tcx.alloc_steal_promoted(promoted)) diff --git a/compiler/rustc_mir_transform/src/lint_tail_expr_drop_order.rs b/compiler/rustc_mir_transform/src/lint_tail_expr_drop_order.rs new file mode 100644 index 0000000000000..274315a3e51d8 --- /dev/null +++ b/compiler/rustc_mir_transform/src/lint_tail_expr_drop_order.rs @@ -0,0 +1,401 @@ +use rustc_hir::def_id::LocalDefId; +use rustc_hir::{ExprKind, HirId, HirIdSet, OwnerId}; +use rustc_index::bit_set::BitSet; +use rustc_index::IndexVec; +use rustc_lint::{self as lint, Level}; +use rustc_macros::LintDiagnostic; +use rustc_middle::mir::visit::{ + MutatingUseContext, NonMutatingUseContext, NonUseContext, PlaceContext, Visitor, +}; +use rustc_middle::mir::{ + dump_mir, BasicBlock, Body, CallReturnPlaces, HasLocalDecls, Local, Location, Place, + ProjectionElem, Statement, Terminator, TerminatorEdges, TerminatorKind, +}; +use rustc_middle::ty::{self, Ty, TyCtxt}; +use rustc_mir_dataflow::{Analysis, AnalysisDomain, GenKill, GenKillAnalysis}; +use rustc_span::Span; +use rustc_type_ir::data_structures::IndexMap; +use tracing::debug; + +fn blocks_reachable_from_tail_expr_rev(body: &Body<'_>, blocks: &mut BitSet) { + let mut new_blocks = blocks.clone(); + let predecessors = body.basic_blocks.predecessors(); + while !new_blocks.is_empty() { + let mut next_front = BitSet::new_empty(new_blocks.domain_size()); + for bb in new_blocks.iter() { + for &pred in &predecessors[bb] { + if body.basic_blocks[pred].is_cleanup { + continue; + } + if blocks.insert(pred) { + next_front.insert(pred); + } + } + } + new_blocks = next_front; + } +} + +fn blocks_reachable_from_tail_expr_fwd(body: &Body<'_>, blocks: &mut BitSet) { + let mut new_blocks = blocks.clone(); + while !new_blocks.is_empty() { + let mut next_front = BitSet::new_empty(new_blocks.domain_size()); + for bb in new_blocks.iter() { + for succ in body.basic_blocks[bb].terminator.iter().flat_map(|term| term.successors()) { + if body.basic_blocks[succ].is_cleanup { + continue; + } + if blocks.insert(succ) { + next_front.insert(succ); + } + } + } + new_blocks = next_front; + } +} + +fn is_descendent_of_hir_id( + tcx: TyCtxt<'_>, + id: HirId, + root: HirId, + descendants: &mut HirIdSet, + filter: &mut HirIdSet, +) -> bool { + let mut path = vec![]; + for id in [id].into_iter().chain(tcx.hir().parent_id_iter(id)) { + if filter.contains(&id) { + filter.extend(path); + return false; + } else if root == id || descendants.contains(&id) { + descendants.extend(path); + return true; + } + path.push(id); + } + filter.extend(path); + false +} + +pub(crate) fn run_lint<'tcx>(tcx: TyCtxt<'tcx>, def_id: LocalDefId, body: &Body<'tcx>) { + let hir_node = tcx.hir_node_by_def_id(def_id); + let Some(body_id) = hir_node.body_id() else { return }; + let (tail_expr_hir_id, tail_expr_span) = { + let expr = tcx.hir().body(body_id).value; + match expr.kind { + ExprKind::Block(block, _) => { + if let Some(expr) = block.expr { + (expr.hir_id, expr.span) + } else { + // There is no tail expression + return; + } + } + _ => (expr.hir_id, expr.span), + } + }; + if tail_expr_span.edition().at_least_rust_2024() { + // We should stop linting since Edition 2024 + return; + } + if let (Level::Allow, _) = + tcx.lint_level_at_node(lint::builtin::TAIL_EXPR_DROP_ORDER, tail_expr_hir_id) + { + // Analysis is expensive, so let's skip if the lint is suppressed anyway + return; + } + debug!(?def_id, ?tail_expr_span); + dump_mir(tcx, false, "lint_drop_order", &0 as _, body, |pass_where, writer| match pass_where { + rustc_middle::mir::PassWhere::AfterLocation(loc) => { + if loc.statement_index < body.basic_blocks[loc.block].statements.len() { + writeln!( + writer, + "@ {:?}", + body.basic_blocks[loc.block].statements[loc.statement_index].source_info.span + ) + } else { + Ok(()) + } + } + rustc_middle::mir::PassWhere::AfterTerminator(term) => { + writeln!(writer, "@ {:?}", body.basic_blocks[term].terminator().source_info.span) + } + _ => Ok(()), + }); + let mut non_tail_expr_parent: HirIdSet = tcx.hir().parent_id_iter(tail_expr_hir_id).collect(); + let mut tail_expr_descendants: HirIdSet = [tail_expr_hir_id].into_iter().collect(); + + let param_env = tcx.param_env(def_id); + let mut droppy_locals = IndexMap::default(); + let (nr_upvars, nr_locals, mut droppy_local_mask) = if tcx.is_closure_like(def_id.to_def_id()) { + let captures = tcx.closure_captures(def_id); + let nr_upvars = captures.len(); + let nr_body_locals = body.local_decls.len(); + let mut mask = BitSet::new_empty(nr_body_locals + nr_upvars); + for (idx, &capture) in captures.iter().enumerate() { + let ty = capture.place.ty(); + if ty.has_significant_drop(tcx, param_env) { + let upvar = Local::from_usize(nr_body_locals + idx); + mask.insert(upvar); + droppy_locals.insert(upvar, (capture.var_ident.span, ty)); + } + } + (nr_upvars, nr_upvars + nr_body_locals, mask) + } else { + let nr_locals = body.local_decls.len(); + (0, nr_locals, BitSet::new_empty(nr_locals)) + }; + for (local, decl) in body.local_decls().iter_enumerated() { + if local == Local::ZERO { + // return place is ignored + continue; + } + if decl.ty.has_significant_drop(tcx, param_env) { + droppy_local_mask.insert(local); + droppy_locals.insert(local, (decl.source_info.span, decl.ty)); + } + } + + let nr_blocks = body.basic_blocks.len(); + let mut tail_expr_blocks = BitSet::new_empty(nr_blocks); + let mut blocks_term_in_tail_expr = BitSet::new_empty(nr_blocks); + let mut tail_expr_stmts = IndexVec::from_elem_n(0, nr_blocks); + for (bb, data) in body.basic_blocks.iter_enumerated() { + if data.is_cleanup { + continue; + } + if let Some(terminator) = &data.terminator + && let TerminatorKind::Drop { scope: Some((def_id, local_id)), .. } = terminator.kind + && let Some(def_id) = def_id.as_local() + { + let hir_id = HirId { owner: OwnerId { def_id }, local_id }; + if is_descendent_of_hir_id( + tcx, + hir_id, + tail_expr_hir_id, + &mut tail_expr_descendants, + &mut non_tail_expr_parent, + ) { + tail_expr_blocks.insert(bb); + blocks_term_in_tail_expr.insert(bb); + continue; + } + } + for (idx, stmt) in data.statements.iter().enumerate().rev() { + if tail_expr_span.contains(stmt.source_info.span) { + tail_expr_blocks.insert(bb); + tail_expr_stmts[bb] = idx; + break; + } + } + } + let mut exit_tail_expr_blocks = tail_expr_blocks.clone(); + debug!(?tail_expr_blocks, "before reachability"); + blocks_reachable_from_tail_expr_rev(body, &mut tail_expr_blocks); + debug!(?tail_expr_blocks, "reachable bb to tail expr"); + blocks_reachable_from_tail_expr_fwd(body, &mut exit_tail_expr_blocks); + debug!(?exit_tail_expr_blocks, "exit reachability"); + exit_tail_expr_blocks.subtract(&tail_expr_blocks); + debug!(?exit_tail_expr_blocks, "exit - rev"); + + let mut tail_expr_exit_boundary_blocks = BitSet::new_empty(nr_blocks); + let mut boundary_lies_on_statement = BitSet::new_empty(nr_blocks); + 'boundary_block: for (bb, block) in + tail_expr_blocks.iter().map(|bb| (bb, &body.basic_blocks[bb])) + { + for succ in block.terminator.iter().flat_map(|term| term.successors()) { + if exit_tail_expr_blocks.contains(succ) { + if blocks_term_in_tail_expr.contains(bb) { + tail_expr_exit_boundary_blocks.insert(succ); + } else { + boundary_lies_on_statement.insert(bb); + tail_expr_exit_boundary_blocks.insert(bb); + continue 'boundary_block; + } + } + } + } + debug!(?tail_expr_exit_boundary_blocks); + + let mut maybe_live = LiveLocals { nr_upvars, nr_body_locals: body.local_decls.len() } + .into_engine(tcx, body) + .iterate_to_fixpoint() + .into_results_cursor(body); + let mut observables = BitSet::new_empty(nr_locals); + for block in tail_expr_exit_boundary_blocks.iter() { + debug!(?observables); + if boundary_lies_on_statement.contains(block) { + let target = Location { block, statement_index: tail_expr_stmts[block] }; + debug!(?target, "in block"); + maybe_live.seek_after_primary_effect(target); + } else { + maybe_live.seek_to_block_start(block); + } + let mut mask = droppy_local_mask.clone(); + let alive = maybe_live.get(); + debug!(?block, ?alive); + mask.intersect(alive); + observables.union(&mask); + } + if observables.is_empty() { + debug!("no observable, bail"); + return; + } + debug!(?observables, ?droppy_locals); + // A linted local has a span contained in the tail expression span + let Some((linted_local, (linted_local_span, linted_local_ty))) = observables + .iter() + .filter_map(|local| droppy_locals.get(&local).map(|&info| (local, info))) + .filter(|(_, (span, _))| tail_expr_span.contains(*span)) + .nth(0) + else { + debug!("nothing to lint"); + return; + }; + debug!(?linted_local); + let body_observables: Vec<_> = observables + .iter() + .filter(|&local| local != linted_local) + .filter_map(|local| droppy_locals.get(&local)) + .map(|&(span, _)| span) + .collect(); + if body_observables.is_empty() { + debug!("nothing observable from body"); + return; + } + debug!(?linted_local, ?body_observables); + let decorator = TailExprDropOrderLint { spans: body_observables, ty: linted_local_ty }; + tcx.emit_node_span_lint( + lint::builtin::TAIL_EXPR_DROP_ORDER, + tail_expr_hir_id, + linted_local_span, + decorator, + ); +} + +#[derive(LintDiagnostic)] +#[diag(mir_transform_tail_expr_drop_order)] +struct TailExprDropOrderLint<'a> { + #[label] + pub spans: Vec, + pub ty: Ty<'a>, +} + +struct LiveLocals { + nr_upvars: usize, + nr_body_locals: usize, +} + +impl<'tcx> AnalysisDomain<'tcx> for LiveLocals { + type Domain = BitSet; + const NAME: &'static str = "liveness-by-use"; + + fn bottom_value(&self, body: &Body<'tcx>) -> Self::Domain { + debug_assert_eq!(self.nr_body_locals, body.local_decls.len()); + BitSet::new_empty(self.nr_body_locals + self.nr_upvars) + } + + fn initialize_start_block(&self, body: &Body<'tcx>, state: &mut Self::Domain) { + for arg in body.args_iter() { + state.insert(arg); + } + debug_assert_eq!(self.nr_body_locals, body.local_decls.len()); + for upvar in 0..self.nr_upvars { + state.insert(Local::from_usize(self.nr_body_locals + upvar)); + } + } +} + +impl<'tcx> GenKillAnalysis<'tcx> for LiveLocals { + type Idx = Local; + + fn domain_size(&self, body: &Body<'tcx>) -> usize { + body.local_decls.len() + } + + fn statement_effect( + &mut self, + transfer: &mut impl GenKill, + statement: &Statement<'tcx>, + location: Location, + ) { + TransferFunction { + nr_upvars: self.nr_upvars, + nr_body_locals: self.nr_body_locals, + transfer, + } + .visit_statement(statement, location); + } + + fn terminator_effect<'mir>( + &mut self, + transfer: &mut Self::Domain, + terminator: &'mir Terminator<'tcx>, + location: Location, + ) -> TerminatorEdges<'mir, 'tcx> { + TransferFunction { + nr_upvars: self.nr_upvars, + nr_body_locals: self.nr_body_locals, + transfer, + } + .visit_terminator(terminator, location); + terminator.edges() + } + + fn call_return_effect( + &mut self, + trans: &mut Self::Domain, + _block: BasicBlock, + return_places: CallReturnPlaces<'_, 'tcx>, + ) { + return_places.for_each(|place| { + if let Some(local) = place.as_local() { + trans.gen_(local); + } + }) + } +} + +struct TransferFunction<'a, T> { + nr_upvars: usize, + nr_body_locals: usize, + transfer: &'a mut T, +} + +impl<'tcx, T> Visitor<'tcx> for TransferFunction<'_, T> +where + T: GenKill, +{ + fn visit_place(&mut self, place: &Place<'tcx>, context: PlaceContext, _location: Location) { + let local = if let Some(local) = place.as_local() { + local + } else if let Place { local: ty::CAPTURE_STRUCT_LOCAL, projection } = *place + && let [ProjectionElem::Field(idx, _)] = **projection + && idx.as_usize() < self.nr_upvars + { + Local::from_usize(self.nr_body_locals + idx.as_usize()) + } else { + return; + }; + + match context { + PlaceContext::NonUse(NonUseContext::StorageDead) + | PlaceContext::MutatingUse(MutatingUseContext::Drop | MutatingUseContext::Deinit) + | PlaceContext::NonMutatingUse(NonMutatingUseContext::Move) => { + self.transfer.kill(local); + if matches!(local, ty::CAPTURE_STRUCT_LOCAL) && self.nr_upvars > 0 { + for upvar in 0..self.nr_upvars { + self.transfer.kill(Local::from_usize(self.nr_body_locals + upvar)); + } + } + } + PlaceContext::MutatingUse( + MutatingUseContext::Store + | MutatingUseContext::AsmOutput + | MutatingUseContext::Call, + ) => { + self.transfer.gen_(local); + } + _ => {} + } + } +} diff --git a/compiler/rustc_mir_transform/src/shim.rs b/compiler/rustc_mir_transform/src/shim.rs index e872878a751ed..88bbeabf8a4a9 100644 --- a/compiler/rustc_mir_transform/src/shim.rs +++ b/compiler/rustc_mir_transform/src/shim.rs @@ -288,6 +288,7 @@ fn build_drop_shim<'tcx>(tcx: TyCtxt<'tcx>, def_id: DefId, ty: Option>) return_block, elaborate_drops::Unwind::To(resume_block), START_BLOCK, + None, ); elaborator.patch }; @@ -616,6 +617,7 @@ impl<'tcx> CloneShimBuilder<'tcx> { target: unwind, unwind: UnwindAction::Terminate(UnwindTerminateReason::InCleanup), replace: false, + scope: None, }, /* is_cleanup */ true, ); @@ -878,6 +880,7 @@ fn build_call_shim<'tcx>( target: BasicBlock::new(2), unwind: UnwindAction::Continue, replace: false, + scope: None, }, false, ); @@ -894,6 +897,7 @@ fn build_call_shim<'tcx>( target: BasicBlock::new(4), unwind: UnwindAction::Terminate(UnwindTerminateReason::InCleanup), replace: false, + scope: None, }, /* is_cleanup */ true, ); diff --git a/compiler/rustc_mir_transform/src/shim/async_destructor_ctor.rs b/compiler/rustc_mir_transform/src/shim/async_destructor_ctor.rs index 71723f040b3b6..79a773c192cc1 100644 --- a/compiler/rustc_mir_transform/src/shim/async_destructor_ctor.rs +++ b/compiler/rustc_mir_transform/src/shim/async_destructor_ctor.rs @@ -430,6 +430,7 @@ impl<'tcx> AsyncDestructorCtorShimBuilder<'tcx> { UnwindTerminateReason::InCleanup, ), replace: false, + scope: None, } } else { TerminatorKind::Goto { target: *top_cleanup_bb } diff --git a/compiler/rustc_smir/src/rustc_smir/convert/mir.rs b/compiler/rustc_smir/src/rustc_smir/convert/mir.rs index 0dbbc338e738b..fe0a701107ecf 100644 --- a/compiler/rustc_smir/src/rustc_smir/convert/mir.rs +++ b/compiler/rustc_smir/src/rustc_smir/convert/mir.rs @@ -622,7 +622,7 @@ impl<'tcx> Stable<'tcx> for mir::TerminatorKind<'tcx> { mir::TerminatorKind::UnwindTerminate(_) => TerminatorKind::Abort, mir::TerminatorKind::Return => TerminatorKind::Return, mir::TerminatorKind::Unreachable => TerminatorKind::Unreachable, - mir::TerminatorKind::Drop { place, target, unwind, replace: _ } => { + mir::TerminatorKind::Drop { place, target, unwind, scope: _, replace: _ } => { TerminatorKind::Drop { place: place.stable(tables), target: target.as_usize(), diff --git a/tests/ui/drop/lint-tail-expr-drop-order.rs b/tests/ui/drop/lint-tail-expr-drop-order.rs index 0aa0ef026101a..2234d52691237 100644 --- a/tests/ui/drop/lint-tail-expr-drop-order.rs +++ b/tests/ui/drop/lint-tail-expr-drop-order.rs @@ -1,6 +1,3 @@ -//@ compile-flags: -Z unstable-options -//@ edition: 2024 - // Edition 2024 lint for change in drop order at tail expression // This lint is to capture potential change in program semantics // due to implementation of RFC 3606 @@ -27,14 +24,32 @@ fn should_lint() -> i32 { let x = LoudDropper; // Should lint x.get() + LoudDropper.get() - //~^ ERROR: these values and local bindings have significant drop implementation that will have a different drop order from that of Edition 2021 + //~^ ERROR: this value of type `LoudDropper` has significant drop implementation that will have a different drop order from that of Edition 2021 //~| WARN: this changes meaning in Rust 2024 } fn should_lint_closure() -> impl FnOnce() -> i32 { let x = LoudDropper; - move || x.get() + LoudDropper.get() - //~^ ERROR: these values and local bindings have significant drop implementation that will have a different drop order from that of Edition 2021 + move || { + x.get() + LoudDropper.get() + //~^ ERROR: this value of type `LoudDropper` has significant drop implementation that will have a different drop order from that of Edition 2021 + //~| WARN: this changes meaning in Rust 2024 + } +} + +fn should_lint_in_nested_items() { + fn should_lint_me() -> i32 { + let x = LoudDropper; + // Should lint + x.get() + LoudDropper.get() + //~^ ERROR: this value of type `LoudDropper` has significant drop implementation that will have a different drop order from that of Edition 2021 + //~| WARN: this changes meaning in Rust 2024 + } +} + +fn should_lint_params(x: LoudDropper) -> i32 { + x.get() + LoudDropper.get() + //~^ ERROR: this value of type `LoudDropper` has significant drop implementation that will have a different drop order from that of Edition 2021 //~| WARN: this changes meaning in Rust 2024 } @@ -44,10 +59,11 @@ fn should_not_lint() -> i32 { x.get() } -fn should_not_lint_in_nested_block() -> i32 { +fn should_lint_in_nested_block() -> i32 { let x = LoudDropper; - // Should not lint because Edition 2021 drops temporaries in blocks earlier already { LoudDropper.get() } + //~^ ERROR: this value of type `LoudDropper` has significant drop implementation that will have a different drop order from that of Edition 2021 + //~| WARN: this changes meaning in Rust 2024 } fn should_not_lint_in_match_arm() -> i32 { @@ -58,14 +74,28 @@ fn should_not_lint_in_match_arm() -> i32 { } } -fn should_lint_in_nested_items() { - fn should_lint_me() -> i32 { - let x = LoudDropper; - // Should lint - x.get() + LoudDropper.get() - //~^ ERROR: these values and local bindings have significant drop implementation that will have a different drop order from that of Edition 2021 - //~| WARN: this changes meaning in Rust 2024 - } +fn should_not_lint_when_consumed() -> (LoudDropper, i32) { + let x = LoudDropper; + // Should not lint + (LoudDropper, x.get()) +} + +struct MyAdt { + a: LoudDropper, + b: LoudDropper, +} + +fn should_not_lint_when_consumed_in_ctor() -> MyAdt { + let a = LoudDropper; + // Should not lint + MyAdt { a, b: LoudDropper } +} + +fn should_not_lint_when_moved() -> i32 { + let x = LoudDropper; + drop(x); + // Should not lint + LoudDropper.get() } fn main() {} diff --git a/tests/ui/drop/lint-tail-expr-drop-order.stderr b/tests/ui/drop/lint-tail-expr-drop-order.stderr index 630f0a80f092b..0f20c4982ec2f 100644 --- a/tests/ui/drop/lint-tail-expr-drop-order.stderr +++ b/tests/ui/drop/lint-tail-expr-drop-order.stderr @@ -1,36 +1,41 @@ -error: these values and local bindings have significant drop implementation that will have a different drop order from that of Edition 2021 - --> $DIR/lint-tail-expr-drop-order.rs:29:15 +error: this value of type `LoudDropper` has significant drop implementation that will have a different drop order from that of Edition 2021 + --> $DIR/lint-tail-expr-drop-order.rs:34:19 | -LL | let x = LoudDropper; - | - these values have significant drop implementation and will observe changes in drop order under Edition 2024 -LL | // Should lint -LL | x.get() + LoudDropper.get() - | ^^^^^^^^^^^ +LL | let x = LoudDropper; + | - these local bindings with significant drop implementation may observe changes in drop order under Edition 2024 +LL | / move || { +LL | | x.get() + LoudDropper.get() + | | ^^^^^^^^^^^ +LL | | +LL | | +LL | | } + | |_____- these local bindings with significant drop implementation may observe changes in drop order under Edition 2024 | = warning: this changes meaning in Rust 2024 = note: for more information, see issue #123739 note: the lint level is defined here - --> $DIR/lint-tail-expr-drop-order.rs:8:9 + --> $DIR/lint-tail-expr-drop-order.rs:5:9 | LL | #![deny(tail_expr_drop_order)] | ^^^^^^^^^^^^^^^^^^^^ -error: these values and local bindings have significant drop implementation that will have a different drop order from that of Edition 2021 - --> $DIR/lint-tail-expr-drop-order.rs:36:23 +error: this value of type `LoudDropper` has significant drop implementation that will have a different drop order from that of Edition 2021 + --> $DIR/lint-tail-expr-drop-order.rs:26:15 | LL | let x = LoudDropper; - | - these values have significant drop implementation and will observe changes in drop order under Edition 2024 -LL | move || x.get() + LoudDropper.get() - | ^^^^^^^^^^^ + | - these local bindings with significant drop implementation may observe changes in drop order under Edition 2024 +LL | // Should lint +LL | x.get() + LoudDropper.get() + | ^^^^^^^^^^^ | = warning: this changes meaning in Rust 2024 = note: for more information, see issue #123739 -error: these values and local bindings have significant drop implementation that will have a different drop order from that of Edition 2021 - --> $DIR/lint-tail-expr-drop-order.rs:65:19 +error: this value of type `LoudDropper` has significant drop implementation that will have a different drop order from that of Edition 2021 + --> $DIR/lint-tail-expr-drop-order.rs:44:19 | LL | let x = LoudDropper; - | - these values have significant drop implementation and will observe changes in drop order under Edition 2024 + | - these local bindings with significant drop implementation may observe changes in drop order under Edition 2024 LL | // Should lint LL | x.get() + LoudDropper.get() | ^^^^^^^^^^^ @@ -38,5 +43,27 @@ LL | x.get() + LoudDropper.get() = warning: this changes meaning in Rust 2024 = note: for more information, see issue #123739 -error: aborting due to 3 previous errors +error: this value of type `LoudDropper` has significant drop implementation that will have a different drop order from that of Edition 2021 + --> $DIR/lint-tail-expr-drop-order.rs:51:15 + | +LL | fn should_lint_params(x: LoudDropper) -> i32 { + | - these local bindings with significant drop implementation may observe changes in drop order under Edition 2024 +LL | x.get() + LoudDropper.get() + | ^^^^^^^^^^^ + | + = warning: this changes meaning in Rust 2024 + = note: for more information, see issue #123739 + +error: this value of type `LoudDropper` has significant drop implementation that will have a different drop order from that of Edition 2021 + --> $DIR/lint-tail-expr-drop-order.rs:64:7 + | +LL | let x = LoudDropper; + | - these local bindings with significant drop implementation may observe changes in drop order under Edition 2024 +LL | { LoudDropper.get() } + | ^^^^^^^^^^^ + | + = warning: this changes meaning in Rust 2024 + = note: for more information, see issue #123739 + +error: aborting due to 5 previous errors From 7e7613f1a37b5d74204b011618ec98ccb825da1e Mon Sep 17 00:00:00 2001 From: Ding Xiang Fei Date: Mon, 23 Sep 2024 04:50:39 +0800 Subject: [PATCH 4/9] add explanation for the new scope info on drop terminators --- compiler/rustc_middle/src/mir/syntax.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/compiler/rustc_middle/src/mir/syntax.rs b/compiler/rustc_middle/src/mir/syntax.rs index dc081c0825ffd..cc8e66a797b53 100644 --- a/compiler/rustc_middle/src/mir/syntax.rs +++ b/compiler/rustc_middle/src/mir/syntax.rs @@ -709,6 +709,8 @@ pub enum TerminatorKind<'tcx> { place: Place<'tcx>, target: BasicBlock, unwind: UnwindAction, + /// The HIR region scope that this drop is issued for when the evaluation exits the said scope. + /// It is provided at best effort and may come from foreign sources due to inlining. scope: Option<(DefId, ItemLocalId)>, replace: bool, }, From 777af6e5a1546c16710f8c0f76c96772167920e7 Mon Sep 17 00:00:00 2001 From: Ding Xiang Fei Date: Mon, 23 Sep 2024 06:19:46 +0800 Subject: [PATCH 5/9] handle synthetic defs --- .../rustc_mir_transform/src/lint_tail_expr_drop_order.rs | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/compiler/rustc_mir_transform/src/lint_tail_expr_drop_order.rs b/compiler/rustc_mir_transform/src/lint_tail_expr_drop_order.rs index 274315a3e51d8..44ef6340c5ed4 100644 --- a/compiler/rustc_mir_transform/src/lint_tail_expr_drop_order.rs +++ b/compiler/rustc_mir_transform/src/lint_tail_expr_drop_order.rs @@ -77,10 +77,12 @@ fn is_descendent_of_hir_id( } pub(crate) fn run_lint<'tcx>(tcx: TyCtxt<'tcx>, def_id: LocalDefId, body: &Body<'tcx>) { - let hir_node = tcx.hir_node_by_def_id(def_id); - let Some(body_id) = hir_node.body_id() else { return }; + if matches!(tcx.def_kind(def_id), rustc_hir::def::DefKind::SyntheticCoroutineBody) { + // Synthetic coroutine has no HIR body and it is enough to just analyse the original body + return; + } let (tail_expr_hir_id, tail_expr_span) = { - let expr = tcx.hir().body(body_id).value; + let expr = tcx.hir().body_owned_by(def_id).value; match expr.kind { ExprKind::Block(block, _) => { if let Some(expr) = block.expr { From 51fe6358429630bacea8a43c51bc8bd3522c7d41 Mon Sep 17 00:00:00 2001 From: Ding Xiang Fei Date: Tue, 24 Sep 2024 06:55:48 +0800 Subject: [PATCH 6/9] switch to MaybeInitializedPlaces --- compiler/rustc_index/src/bit_set.rs | 60 ++++- .../src/lint_tail_expr_drop_order.rs | 250 +++++------------- tests/ui/drop/lint-tail-expr-drop-order.rs | 5 +- .../ui/drop/lint-tail-expr-drop-order.stderr | 34 +-- 4 files changed, 132 insertions(+), 217 deletions(-) diff --git a/compiler/rustc_index/src/bit_set.rs b/compiler/rustc_index/src/bit_set.rs index c2b9cae680bf1..9edbeee605c0a 100644 --- a/compiler/rustc_index/src/bit_set.rs +++ b/compiler/rustc_index/src/bit_set.rs @@ -460,6 +460,10 @@ impl ChunkedBitSet { self.chunks.iter().map(|chunk| chunk.count()).sum() } + pub fn is_empty(&self) -> bool { + self.chunks.iter().all(|chunk| matches!(chunk, Chunk::Zeros(..) | Chunk::Ones(0))) + } + /// Returns `true` if `self` contains `elem`. #[inline] pub fn contains(&self, elem: T) -> bool { @@ -672,8 +676,60 @@ impl BitRelations> for ChunkedBitSet { unimplemented!("implement if/when necessary"); } - fn intersect(&mut self, _other: &ChunkedBitSet) -> bool { - unimplemented!("implement if/when necessary"); + fn intersect(&mut self, other: &ChunkedBitSet) -> bool { + assert_eq!(self.domain_size, other.domain_size); + debug_assert_eq!(self.chunks.len(), other.chunks.len()); + + let mut changed = false; + for (mut self_chunk, other_chunk) in self.chunks.iter_mut().zip(other.chunks.iter()) { + match (&mut self_chunk, &other_chunk) { + (Zeros(..), _) | (_, Ones(..)) => {} + ( + Ones(self_chunk_domain_size), + Zeros(other_chunk_domain_size) | Mixed(other_chunk_domain_size, ..), + ) + | (Mixed(self_chunk_domain_size, ..), Zeros(other_chunk_domain_size)) => { + debug_assert_eq!(self_chunk_domain_size, other_chunk_domain_size); + changed = true; + *self_chunk = other_chunk.clone(); + } + ( + Mixed( + self_chunk_domain_size, + ref mut self_chunk_count, + ref mut self_chunk_words, + ), + Mixed(_other_chunk_domain_size, _other_chunk_count, other_chunk_words), + ) => { + // See [`>>::union`] for the explanation + let op = |a, b| a & b; + let num_words = num_words(*self_chunk_domain_size as usize); + if bitwise_changes( + &self_chunk_words[0..num_words], + &other_chunk_words[0..num_words], + op, + ) { + let self_chunk_words = Rc::make_mut(self_chunk_words); + let has_changed = bitwise( + &mut self_chunk_words[0..num_words], + &other_chunk_words[0..num_words], + op, + ); + debug_assert!(has_changed); + *self_chunk_count = self_chunk_words[0..num_words] + .iter() + .map(|w| w.count_ones() as ChunkSize) + .sum(); + if *self_chunk_count == 0 { + *self_chunk = Zeros(*self_chunk_domain_size); + } + changed = true; + } + } + } + } + + changed } } diff --git a/compiler/rustc_mir_transform/src/lint_tail_expr_drop_order.rs b/compiler/rustc_mir_transform/src/lint_tail_expr_drop_order.rs index 44ef6340c5ed4..029b2b0f02958 100644 --- a/compiler/rustc_mir_transform/src/lint_tail_expr_drop_order.rs +++ b/compiler/rustc_mir_transform/src/lint_tail_expr_drop_order.rs @@ -1,20 +1,15 @@ use rustc_hir::def_id::LocalDefId; use rustc_hir::{ExprKind, HirId, HirIdSet, OwnerId}; -use rustc_index::bit_set::BitSet; +use rustc_index::bit_set::{BitSet, ChunkedBitSet}; use rustc_index::IndexVec; use rustc_lint::{self as lint, Level}; use rustc_macros::LintDiagnostic; -use rustc_middle::mir::visit::{ - MutatingUseContext, NonMutatingUseContext, NonUseContext, PlaceContext, Visitor, -}; -use rustc_middle::mir::{ - dump_mir, BasicBlock, Body, CallReturnPlaces, HasLocalDecls, Local, Location, Place, - ProjectionElem, Statement, Terminator, TerminatorEdges, TerminatorKind, -}; +use rustc_middle::mir::{dump_mir, BasicBlock, Body, Local, Location, TerminatorKind}; use rustc_middle::ty::{self, Ty, TyCtxt}; -use rustc_mir_dataflow::{Analysis, AnalysisDomain, GenKill, GenKillAnalysis}; +use rustc_mir_dataflow::impls::MaybeInitializedPlaces; +use rustc_mir_dataflow::move_paths::MoveData; +use rustc_mir_dataflow::{Analysis, MaybeReachable}; use rustc_span::Span; -use rustc_type_ir::data_structures::IndexMap; use tracing::debug; fn blocks_reachable_from_tail_expr_rev(body: &Body<'_>, blocks: &mut BitSet) { @@ -78,7 +73,7 @@ fn is_descendent_of_hir_id( pub(crate) fn run_lint<'tcx>(tcx: TyCtxt<'tcx>, def_id: LocalDefId, body: &Body<'tcx>) { if matches!(tcx.def_kind(def_id), rustc_hir::def::DefKind::SyntheticCoroutineBody) { - // Synthetic coroutine has no HIR body and it is enough to just analyse the original body + // A synthetic coroutine has no HIR body and it is enough to just analyse the original body return; } let (tail_expr_hir_id, tail_expr_span) = { @@ -127,35 +122,7 @@ pub(crate) fn run_lint<'tcx>(tcx: TyCtxt<'tcx>, def_id: LocalDefId, body: &Body< let mut tail_expr_descendants: HirIdSet = [tail_expr_hir_id].into_iter().collect(); let param_env = tcx.param_env(def_id); - let mut droppy_locals = IndexMap::default(); - let (nr_upvars, nr_locals, mut droppy_local_mask) = if tcx.is_closure_like(def_id.to_def_id()) { - let captures = tcx.closure_captures(def_id); - let nr_upvars = captures.len(); - let nr_body_locals = body.local_decls.len(); - let mut mask = BitSet::new_empty(nr_body_locals + nr_upvars); - for (idx, &capture) in captures.iter().enumerate() { - let ty = capture.place.ty(); - if ty.has_significant_drop(tcx, param_env) { - let upvar = Local::from_usize(nr_body_locals + idx); - mask.insert(upvar); - droppy_locals.insert(upvar, (capture.var_ident.span, ty)); - } - } - (nr_upvars, nr_upvars + nr_body_locals, mask) - } else { - let nr_locals = body.local_decls.len(); - (0, nr_locals, BitSet::new_empty(nr_locals)) - }; - for (local, decl) in body.local_decls().iter_enumerated() { - if local == Local::ZERO { - // return place is ignored - continue; - } - if decl.ty.has_significant_drop(tcx, param_env) { - droppy_local_mask.insert(local); - droppy_locals.insert(local, (decl.source_info.span, decl.ty)); - } - } + let is_closure_like = tcx.is_closure_like(def_id.to_def_id()); let nr_blocks = body.basic_blocks.len(); let mut tail_expr_blocks = BitSet::new_empty(nr_blocks); @@ -218,58 +185,87 @@ pub(crate) fn run_lint<'tcx>(tcx: TyCtxt<'tcx>, def_id: LocalDefId, body: &Body< } debug!(?tail_expr_exit_boundary_blocks); - let mut maybe_live = LiveLocals { nr_upvars, nr_body_locals: body.local_decls.len() } - .into_engine(tcx, body) - .iterate_to_fixpoint() - .into_results_cursor(body); - let mut observables = BitSet::new_empty(nr_locals); + let move_data = MoveData::gather_moves(body, tcx, param_env, |_| true); + let mut droppy_paths = ChunkedBitSet::new_empty(move_data.move_paths.len()); + let mut droppy_paths_not_in_tail = droppy_paths.clone(); + for (idx, path) in move_data.move_paths.iter_enumerated() { + if path.place.ty(&body.local_decls, tcx).ty.has_significant_drop(tcx, param_env) { + droppy_paths.insert(idx); + if !tail_expr_span.contains(body.local_decls[path.place.local].source_info.span) { + droppy_paths_not_in_tail.insert(idx); + } + } + } + let maybe_init = MaybeInitializedPlaces::new(tcx, body, &move_data); + let mut maybe_init = + maybe_init.into_engine(tcx, body).iterate_to_fixpoint().into_results_cursor(body); + let mut observables = ChunkedBitSet::new_empty(move_data.move_paths.len()); for block in tail_expr_exit_boundary_blocks.iter() { debug!(?observables); if boundary_lies_on_statement.contains(block) { let target = Location { block, statement_index: tail_expr_stmts[block] }; debug!(?target, "in block"); - maybe_live.seek_after_primary_effect(target); + maybe_init.seek_after_primary_effect(target); } else { - maybe_live.seek_to_block_start(block); + maybe_init.seek_to_block_start(block); + } + if let MaybeReachable::Reachable(alive) = maybe_init.get() { + debug!(?block, ?alive); + let mut mask = droppy_paths.clone(); + mask.intersect(alive); + observables.union(&mask); } - let mut mask = droppy_local_mask.clone(); - let alive = maybe_live.get(); - debug!(?block, ?alive); - mask.intersect(alive); - observables.union(&mask); - } - if observables.is_empty() { - debug!("no observable, bail"); - return; } - debug!(?observables, ?droppy_locals); + debug!(?observables); // A linted local has a span contained in the tail expression span - let Some((linted_local, (linted_local_span, linted_local_ty))) = observables + let Some(linted_move_path) = observables .iter() - .filter_map(|local| droppy_locals.get(&local).map(|&info| (local, info))) - .filter(|(_, (span, _))| tail_expr_span.contains(*span)) + .map(|path| &move_data.move_paths[path]) + .filter(|move_path| { + if move_path.place.local == Local::ZERO { + return false; + } + if is_closure_like && matches!(move_path.place.local, ty::CAPTURE_STRUCT_LOCAL) { + return false; + } + tail_expr_span.contains(body.local_decls[move_path.place.local].source_info.span) + }) .nth(0) else { debug!("nothing to lint"); return; }; - debug!(?linted_local); + debug!(?linted_move_path); let body_observables: Vec<_> = observables .iter() - .filter(|&local| local != linted_local) - .filter_map(|local| droppy_locals.get(&local)) - .map(|&(span, _)| span) + .filter_map(|idx| { + // We want to lint on place/local in body, not another tail expression temporary. + // Also, closures always drops their upvars last, so we don't have to lint about them. + let base_local = move_data.base_local(idx); + if base_local == linted_move_path.place.local || base_local == Local::ZERO { + debug!(?base_local, "skip"); + return None; + } + if is_closure_like && matches!(base_local, ty::CAPTURE_STRUCT_LOCAL) { + debug!(?base_local, "skip in closure"); + return None; + } + droppy_paths_not_in_tail + .contains(idx) + .then_some(body.local_decls[base_local].source_info.span) + }) .collect(); if body_observables.is_empty() { debug!("nothing observable from body"); return; } - debug!(?linted_local, ?body_observables); - let decorator = TailExprDropOrderLint { spans: body_observables, ty: linted_local_ty }; + debug!(?body_observables); + let linted_local_decl = &body.local_decls[linted_move_path.place.local]; + let decorator = TailExprDropOrderLint { spans: body_observables, ty: linted_local_decl.ty }; tcx.emit_node_span_lint( lint::builtin::TAIL_EXPR_DROP_ORDER, tail_expr_hir_id, - linted_local_span, + linted_local_decl.source_info.span, decorator, ); } @@ -281,123 +277,3 @@ struct TailExprDropOrderLint<'a> { pub spans: Vec, pub ty: Ty<'a>, } - -struct LiveLocals { - nr_upvars: usize, - nr_body_locals: usize, -} - -impl<'tcx> AnalysisDomain<'tcx> for LiveLocals { - type Domain = BitSet; - const NAME: &'static str = "liveness-by-use"; - - fn bottom_value(&self, body: &Body<'tcx>) -> Self::Domain { - debug_assert_eq!(self.nr_body_locals, body.local_decls.len()); - BitSet::new_empty(self.nr_body_locals + self.nr_upvars) - } - - fn initialize_start_block(&self, body: &Body<'tcx>, state: &mut Self::Domain) { - for arg in body.args_iter() { - state.insert(arg); - } - debug_assert_eq!(self.nr_body_locals, body.local_decls.len()); - for upvar in 0..self.nr_upvars { - state.insert(Local::from_usize(self.nr_body_locals + upvar)); - } - } -} - -impl<'tcx> GenKillAnalysis<'tcx> for LiveLocals { - type Idx = Local; - - fn domain_size(&self, body: &Body<'tcx>) -> usize { - body.local_decls.len() - } - - fn statement_effect( - &mut self, - transfer: &mut impl GenKill, - statement: &Statement<'tcx>, - location: Location, - ) { - TransferFunction { - nr_upvars: self.nr_upvars, - nr_body_locals: self.nr_body_locals, - transfer, - } - .visit_statement(statement, location); - } - - fn terminator_effect<'mir>( - &mut self, - transfer: &mut Self::Domain, - terminator: &'mir Terminator<'tcx>, - location: Location, - ) -> TerminatorEdges<'mir, 'tcx> { - TransferFunction { - nr_upvars: self.nr_upvars, - nr_body_locals: self.nr_body_locals, - transfer, - } - .visit_terminator(terminator, location); - terminator.edges() - } - - fn call_return_effect( - &mut self, - trans: &mut Self::Domain, - _block: BasicBlock, - return_places: CallReturnPlaces<'_, 'tcx>, - ) { - return_places.for_each(|place| { - if let Some(local) = place.as_local() { - trans.gen_(local); - } - }) - } -} - -struct TransferFunction<'a, T> { - nr_upvars: usize, - nr_body_locals: usize, - transfer: &'a mut T, -} - -impl<'tcx, T> Visitor<'tcx> for TransferFunction<'_, T> -where - T: GenKill, -{ - fn visit_place(&mut self, place: &Place<'tcx>, context: PlaceContext, _location: Location) { - let local = if let Some(local) = place.as_local() { - local - } else if let Place { local: ty::CAPTURE_STRUCT_LOCAL, projection } = *place - && let [ProjectionElem::Field(idx, _)] = **projection - && idx.as_usize() < self.nr_upvars - { - Local::from_usize(self.nr_body_locals + idx.as_usize()) - } else { - return; - }; - - match context { - PlaceContext::NonUse(NonUseContext::StorageDead) - | PlaceContext::MutatingUse(MutatingUseContext::Drop | MutatingUseContext::Deinit) - | PlaceContext::NonMutatingUse(NonMutatingUseContext::Move) => { - self.transfer.kill(local); - if matches!(local, ty::CAPTURE_STRUCT_LOCAL) && self.nr_upvars > 0 { - for upvar in 0..self.nr_upvars { - self.transfer.kill(Local::from_usize(self.nr_body_locals + upvar)); - } - } - } - PlaceContext::MutatingUse( - MutatingUseContext::Store - | MutatingUseContext::AsmOutput - | MutatingUseContext::Call, - ) => { - self.transfer.gen_(local); - } - _ => {} - } - } -} diff --git a/tests/ui/drop/lint-tail-expr-drop-order.rs b/tests/ui/drop/lint-tail-expr-drop-order.rs index 2234d52691237..2478b5c1f099c 100644 --- a/tests/ui/drop/lint-tail-expr-drop-order.rs +++ b/tests/ui/drop/lint-tail-expr-drop-order.rs @@ -28,12 +28,11 @@ fn should_lint() -> i32 { //~| WARN: this changes meaning in Rust 2024 } -fn should_lint_closure() -> impl FnOnce() -> i32 { +fn should_not_lint_closure() -> impl FnOnce() -> i32 { let x = LoudDropper; move || { + // Should not lint x.get() + LoudDropper.get() - //~^ ERROR: this value of type `LoudDropper` has significant drop implementation that will have a different drop order from that of Edition 2021 - //~| WARN: this changes meaning in Rust 2024 } } diff --git a/tests/ui/drop/lint-tail-expr-drop-order.stderr b/tests/ui/drop/lint-tail-expr-drop-order.stderr index 0f20c4982ec2f..01286e372d147 100644 --- a/tests/ui/drop/lint-tail-expr-drop-order.stderr +++ b/tests/ui/drop/lint-tail-expr-drop-order.stderr @@ -1,24 +1,3 @@ -error: this value of type `LoudDropper` has significant drop implementation that will have a different drop order from that of Edition 2021 - --> $DIR/lint-tail-expr-drop-order.rs:34:19 - | -LL | let x = LoudDropper; - | - these local bindings with significant drop implementation may observe changes in drop order under Edition 2024 -LL | / move || { -LL | | x.get() + LoudDropper.get() - | | ^^^^^^^^^^^ -LL | | -LL | | -LL | | } - | |_____- these local bindings with significant drop implementation may observe changes in drop order under Edition 2024 - | - = warning: this changes meaning in Rust 2024 - = note: for more information, see issue #123739 -note: the lint level is defined here - --> $DIR/lint-tail-expr-drop-order.rs:5:9 - | -LL | #![deny(tail_expr_drop_order)] - | ^^^^^^^^^^^^^^^^^^^^ - error: this value of type `LoudDropper` has significant drop implementation that will have a different drop order from that of Edition 2021 --> $DIR/lint-tail-expr-drop-order.rs:26:15 | @@ -30,9 +9,14 @@ LL | x.get() + LoudDropper.get() | = warning: this changes meaning in Rust 2024 = note: for more information, see issue #123739 +note: the lint level is defined here + --> $DIR/lint-tail-expr-drop-order.rs:5:9 + | +LL | #![deny(tail_expr_drop_order)] + | ^^^^^^^^^^^^^^^^^^^^ error: this value of type `LoudDropper` has significant drop implementation that will have a different drop order from that of Edition 2021 - --> $DIR/lint-tail-expr-drop-order.rs:44:19 + --> $DIR/lint-tail-expr-drop-order.rs:43:19 | LL | let x = LoudDropper; | - these local bindings with significant drop implementation may observe changes in drop order under Edition 2024 @@ -44,7 +28,7 @@ LL | x.get() + LoudDropper.get() = note: for more information, see issue #123739 error: this value of type `LoudDropper` has significant drop implementation that will have a different drop order from that of Edition 2021 - --> $DIR/lint-tail-expr-drop-order.rs:51:15 + --> $DIR/lint-tail-expr-drop-order.rs:50:15 | LL | fn should_lint_params(x: LoudDropper) -> i32 { | - these local bindings with significant drop implementation may observe changes in drop order under Edition 2024 @@ -55,7 +39,7 @@ LL | x.get() + LoudDropper.get() = note: for more information, see issue #123739 error: this value of type `LoudDropper` has significant drop implementation that will have a different drop order from that of Edition 2021 - --> $DIR/lint-tail-expr-drop-order.rs:64:7 + --> $DIR/lint-tail-expr-drop-order.rs:63:7 | LL | let x = LoudDropper; | - these local bindings with significant drop implementation may observe changes in drop order under Edition 2024 @@ -65,5 +49,5 @@ LL | { LoudDropper.get() } = warning: this changes meaning in Rust 2024 = note: for more information, see issue #123739 -error: aborting due to 5 previous errors +error: aborting due to 4 previous errors From aad27d2826acc1407786c12c6d80657f1672a47f Mon Sep 17 00:00:00 2001 From: Ding Xiang Fei Date: Tue, 24 Sep 2024 07:16:57 +0800 Subject: [PATCH 7/9] revert gate test to work only when migration is triggered --- tests/ui/drop/lint-tail-expr-drop-order-gated.rs | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/tests/ui/drop/lint-tail-expr-drop-order-gated.rs b/tests/ui/drop/lint-tail-expr-drop-order-gated.rs index b22e72bcfad22..493ff11b6648d 100644 --- a/tests/ui/drop/lint-tail-expr-drop-order-gated.rs +++ b/tests/ui/drop/lint-tail-expr-drop-order-gated.rs @@ -1,15 +1,13 @@ -// This test ensures that `tail_expr_drop_order` does not activate in case Edition 2024 is not used -// or the feature gate `shorter_tail_lifetimes` is disabled. +// This test ensures that `tail_expr_drop_order` does not activate in case Edition 2024 is used +// because this is a migration lint. +// Only `cargo fix --edition 2024` shall activate this lint. -//@ revisions: neither no_feature_gate edition_less_than_2024 +//@ revisions: neither no_feature_gate edition_at_least_2024 //@ check-pass -//@ [neither] edition: 2021 -//@ [no_feature_gate] compile-flags: -Z unstable-options -//@ [no_feature_gate] edition: 2024 -//@ [edition_less_than_2024] edition: 2021 +//@ compile-flags: -Z unstable-options +//@ edition: 2024 #![deny(tail_expr_drop_order)] -#![cfg_attr(edition_less_than_2024, feature(shorter_tail_lifetimes))] struct LoudDropper; impl Drop for LoudDropper { From 4caa710332d4c15b7f206ce14110a6d8b83a0eb3 Mon Sep 17 00:00:00 2001 From: Ding Xiang Fei Date: Tue, 24 Sep 2024 07:18:01 +0800 Subject: [PATCH 8/9] fmt --- compiler/rustc_hir_typeck/src/expr_use_visitor.rs | 3 ++- compiler/rustc_middle/src/mir/syntax.rs | 2 +- compiler/rustc_mir_transform/src/lint_tail_expr_drop_order.rs | 4 ++-- 3 files changed, 5 insertions(+), 4 deletions(-) diff --git a/compiler/rustc_hir_typeck/src/expr_use_visitor.rs b/compiler/rustc_hir_typeck/src/expr_use_visitor.rs index 8206e79b3e3c8..372f52ba4128c 100644 --- a/compiler/rustc_hir_typeck/src/expr_use_visitor.rs +++ b/compiler/rustc_hir_typeck/src/expr_use_visitor.rs @@ -151,7 +151,8 @@ pub trait TypeInformationCtxt<'tcx> { } impl<'tcx> TypeInformationCtxt<'tcx> for (TyCtxt<'tcx>, LocalDefId) { - type TypeckResults<'a> = &'tcx ty::TypeckResults<'tcx> + type TypeckResults<'a> + = &'tcx ty::TypeckResults<'tcx> where Self: 'a; diff --git a/compiler/rustc_middle/src/mir/syntax.rs b/compiler/rustc_middle/src/mir/syntax.rs index cc8e66a797b53..e44fbbe19835c 100644 --- a/compiler/rustc_middle/src/mir/syntax.rs +++ b/compiler/rustc_middle/src/mir/syntax.rs @@ -9,10 +9,10 @@ use rustc_hir::def_id::DefId; use rustc_hir::{CoroutineKind, ItemLocalId}; use rustc_index::IndexVec; use rustc_macros::{HashStable, TyDecodable, TyEncodable, TypeFoldable, TypeVisitable}; +use rustc_span::Span; use rustc_span::def_id::LocalDefId; use rustc_span::source_map::Spanned; use rustc_span::symbol::Symbol; -use rustc_span::Span; use rustc_target::abi::{FieldIdx, VariantIdx}; use rustc_target::asm::InlineAsmRegOrRegClass; use smallvec::SmallVec; diff --git a/compiler/rustc_mir_transform/src/lint_tail_expr_drop_order.rs b/compiler/rustc_mir_transform/src/lint_tail_expr_drop_order.rs index 029b2b0f02958..bd4dfbe70b38f 100644 --- a/compiler/rustc_mir_transform/src/lint_tail_expr_drop_order.rs +++ b/compiler/rustc_mir_transform/src/lint_tail_expr_drop_order.rs @@ -1,10 +1,10 @@ use rustc_hir::def_id::LocalDefId; use rustc_hir::{ExprKind, HirId, HirIdSet, OwnerId}; -use rustc_index::bit_set::{BitSet, ChunkedBitSet}; use rustc_index::IndexVec; +use rustc_index::bit_set::{BitSet, ChunkedBitSet}; use rustc_lint::{self as lint, Level}; use rustc_macros::LintDiagnostic; -use rustc_middle::mir::{dump_mir, BasicBlock, Body, Local, Location, TerminatorKind}; +use rustc_middle::mir::{BasicBlock, Body, Local, Location, TerminatorKind, dump_mir}; use rustc_middle::ty::{self, Ty, TyCtxt}; use rustc_mir_dataflow::impls::MaybeInitializedPlaces; use rustc_mir_dataflow::move_paths::MoveData; From b46b6bf51847fca7978c5b11b07e6149a1d201f7 Mon Sep 17 00:00:00 2001 From: Ding Xiang Fei Date: Wed, 25 Sep 2024 03:23:54 +0800 Subject: [PATCH 9/9] clean up the previous implementation --- .../rustc_hir_typeck/src/expr_use_visitor.rs | 45 ------------------- 1 file changed, 45 deletions(-) diff --git a/compiler/rustc_hir_typeck/src/expr_use_visitor.rs b/compiler/rustc_hir_typeck/src/expr_use_visitor.rs index 372f52ba4128c..bb5f351137305 100644 --- a/compiler/rustc_hir_typeck/src/expr_use_visitor.rs +++ b/compiler/rustc_hir_typeck/src/expr_use_visitor.rs @@ -150,51 +150,6 @@ pub trait TypeInformationCtxt<'tcx> { fn tcx(&self) -> TyCtxt<'tcx>; } -impl<'tcx> TypeInformationCtxt<'tcx> for (TyCtxt<'tcx>, LocalDefId) { - type TypeckResults<'a> - = &'tcx ty::TypeckResults<'tcx> - where - Self: 'a; - - type Error = !; - - fn typeck_results(&self) -> Self::TypeckResults<'_> { - self.0.typeck(self.1) - } - - fn resolve_vars_if_possible>>(&self, t: T) -> T { - t - } - - fn try_structurally_resolve_type(&self, _span: Span, ty: Ty<'tcx>) -> Ty<'tcx> { - ty - } - - fn report_error(&self, span: Span, msg: impl ToString) -> Self::Error { - span_bug!(span, "{}", msg.to_string()) - } - - fn error_reported_in_ty(&self, _ty: Ty<'tcx>) -> Result<(), Self::Error> { - Ok(()) - } - - fn tainted_by_errors(&self) -> Result<(), Self::Error> { - Ok(()) - } - - fn type_is_copy_modulo_regions(&self, ty: Ty<'tcx>) -> bool { - ty.is_copy_modulo_regions(self.0, self.0.param_env(self.1)) - } - - fn body_owner_def_id(&self) -> LocalDefId { - self.1 - } - - fn tcx(&self) -> TyCtxt<'tcx> { - self.0 - } -} - impl<'tcx> TypeInformationCtxt<'tcx> for &FnCtxt<'_, 'tcx> { type TypeckResults<'a> = Ref<'a, ty::TypeckResults<'tcx>>