diff --git a/compiler/rustc_lint/messages.ftl b/compiler/rustc_lint/messages.ftl index 7a394a6d6c1..28aef9055ef 100644 --- a/compiler/rustc_lint/messages.ftl +++ b/compiler/rustc_lint/messages.ftl @@ -758,6 +758,9 @@ 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 4f3933d461b..1828b6ea93c 100644 --- a/compiler/rustc_lint/src/lib.rs +++ b/compiler/rustc_lint/src/lib.rs @@ -78,6 +78,7 @@ mod ptr_nulls; mod redundant_semicolon; mod reference_casting; mod shadowed_into_iter; +mod tail_expr_drop_order; mod traits; mod types; mod unit_bindings; @@ -115,6 +116,7 @@ use rustc_middle::query::Providers; use rustc_middle::ty::TyCtxt; use shadowed_into_iter::ShadowedIntoIter; pub use shadowed_into_iter::{ARRAY_INTO_ITER, BOXED_SLICE_INTO_ITER}; +use tail_expr_drop_order::TailExprDropOrder; use traits::*; use types::*; use unit_bindings::*; @@ -238,6 +240,7 @@ late_lint_methods!( AsyncFnInTrait: AsyncFnInTrait, NonLocalDefinitions: NonLocalDefinitions::default(), ImplTraitOvercaptures: ImplTraitOvercaptures, + TailExprDropOrder: TailExprDropOrder, ] ] ); diff --git a/compiler/rustc_lint/src/tail_expr_drop_order.rs b/compiler/rustc_lint/src/tail_expr_drop_order.rs new file mode 100644 index 00000000000..f9ecc8c9806 --- /dev/null +++ b/compiler/rustc_lint/src/tail_expr_drop_order.rs @@ -0,0 +1,306 @@ +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::edition::Edition; +use rustc_span::Span; + +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<'tcx, 'a> { + cx: &'a LateContext<'tcx>, + // We only record locals that have significant drops + locals: Vec, +} + +struct LocalCollector<'tcx, 'a> { + cx: &'a LateContext<'tcx>, + locals: &'a mut Vec, +} + +impl<'tcx, 'a> Visitor<'tcx> for LocalCollector<'tcx, 'a> { + 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<'tcx, 'a> Visitor<'tcx> for LintVisitor<'tcx, 'a> { + 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<'tcx, 'a> LintVisitor<'tcx, 'a> { + 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<'tcx, 'a> { + cx: &'a LateContext<'tcx>, + is_root_tail_expr: bool, + locals: &'a [Span], +} + +impl<'tcx, 'a> LintTailExpr<'tcx, 'a> { + 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<'tcx, 'a> Visitor<'tcx> for LintTailExpr<'tcx, 'a> { + 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/src/tools/lint-docs/src/lib.rs b/src/tools/lint-docs/src/lib.rs index e0aef13ca79..72bb9db7e74 100644 --- a/src/tools/lint-docs/src/lib.rs +++ b/src/tools/lint-docs/src/lib.rs @@ -444,6 +444,7 @@ impl<'a> LintExtractor<'a> { let mut cmd = Command::new(self.rustc_path); if options.contains(&"edition2024") { cmd.arg("--edition=2024"); + cmd.arg("-Zunstable-options"); } else if options.contains(&"edition2021") { cmd.arg("--edition=2021"); } else if options.contains(&"edition2018") { diff --git a/tests/ui/drop/lint-tail-expr-drop-order-gated.rs b/tests/ui/drop/lint-tail-expr-drop-order-gated.rs new file mode 100644 index 00000000000..b22e72bcfad --- /dev/null +++ b/tests/ui/drop/lint-tail-expr-drop-order-gated.rs @@ -0,0 +1,35 @@ +// 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. + +//@ revisions: neither no_feature_gate edition_less_than_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 + +#![deny(tail_expr_drop_order)] +#![cfg_attr(edition_less_than_2024, feature(shorter_tail_lifetimes))] + +struct LoudDropper; +impl Drop for LoudDropper { + fn drop(&mut self) { + // This destructor should be considered significant because it is a custom destructor + // and we will assume that the destructor can generate side effects arbitrarily so that + // a change in drop order is visible. + println!("loud drop"); + } +} +impl LoudDropper { + fn get(&self) -> i32 { + 0 + } +} + +fn should_not_lint() -> i32 { + let x = LoudDropper; + x.get() + LoudDropper.get() + // Lint should not action +} + +fn main() {} diff --git a/tests/ui/drop/lint-tail-expr-drop-order.rs b/tests/ui/drop/lint-tail-expr-drop-order.rs new file mode 100644 index 00000000000..0aa0ef02610 --- /dev/null +++ b/tests/ui/drop/lint-tail-expr-drop-order.rs @@ -0,0 +1,71 @@ +//@ 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 + +#![deny(tail_expr_drop_order)] +#![feature(shorter_tail_lifetimes)] + +struct LoudDropper; +impl Drop for LoudDropper { + fn drop(&mut self) { + // This destructor should be considered significant because it is a custom destructor + // and we will assume that the destructor can generate side effects arbitrarily so that + // a change in drop order is visible. + println!("loud drop"); + } +} +impl LoudDropper { + fn get(&self) -> i32 { + 0 + } +} + +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 + //~| 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 + //~| WARN: this changes meaning in Rust 2024 +} + +fn should_not_lint() -> i32 { + let x = LoudDropper; + // Should not lint + x.get() +} + +fn should_not_lint_in_nested_block() -> i32 { + let x = LoudDropper; + // Should not lint because Edition 2021 drops temporaries in blocks earlier already + { LoudDropper.get() } +} + +fn should_not_lint_in_match_arm() -> i32 { + let x = LoudDropper; + // Should not lint because Edition 2021 drops temporaries in blocks earlier already + match &x { + _ => LoudDropper.get(), + } +} + +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 main() {} diff --git a/tests/ui/drop/lint-tail-expr-drop-order.stderr b/tests/ui/drop/lint-tail-expr-drop-order.stderr new file mode 100644 index 00000000000..630f0a80f09 --- /dev/null +++ b/tests/ui/drop/lint-tail-expr-drop-order.stderr @@ -0,0 +1,42 @@ +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 + | +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() + | ^^^^^^^^^^^ + | + = 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 + | +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 + | +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() + | ^^^^^^^^^^^ + | + = 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 + | +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() + | ^^^^^^^^^^^ + | + = warning: this changes meaning in Rust 2024 + = note: for more information, see issue #123739 + +error: aborting due to 3 previous errors +