lint on tail expr drop order change in Edition 2024

dingxiangfei2009 · dingxiangfei2009 · commit 7bf79219ef27 · 2024-08-05T03:58:43.000+08:00
diff --git 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
@@ -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
@@ -0,0 +1,217 @@
+use std::mem::swap;
+
+use rustc_ast::UnOp;
+use rustc_hir::def::Res;
+use rustc_hir::intravisit::{self, Visitor};
+use rustc_hir::{Block, Expr, ExprKind, LetStmt, Pat, PatKind, QPath, StmtKind};
+use rustc_macros::LintDiagnostic;
+use rustc_session::{declare_lint, declare_lint_pass};
+use rustc_span::Span;
+use tracing::debug;
+
+use crate::{LateContext, LateLintPass};
+
+declare_lint! {
+    /// ### What it does
+    /// Edition 2024 introduces a new rule with drop orders for values generated in tail expressions
+    /// of blocks.
+    /// Now the values will be dropped first, before the local variable bindings were dropped.
+    ///
+    /// This 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.
+    ///
+    /// ### Why is this bad?
+    /// Values of type with significant `Drop` implementation has an ill-specified drop order that
+    /// come after those of local variables.
+    /// Edition 2024 moves them, so that they are dropped first before dropping local variables.
+    ///
+    /// ### Example
+    /// ```compile_fail,E0597
+    /// fn edition_2024() -> i32 {
+    ///     let mutex = std::sync::Mutex::new(vec![0]);
+    ///     mutex.lock().unwrap()[0]
+    /// }
+    /// ```
+    /// This lint only points out the issue with `mutex.lock()`, which will be dropped before `mutex` does.
+    /// No fix will be proposed.
+    /// However, the most probable fix is to hoist `mutex.lock()` into its own local variable binding.
+    /// ```no_run
+    /// fn edition_2024() -> i32 {
+    ///     let mutex = std::sync::Mutex::new(vec![0]);
+    ///     let guard = mutex.lock().unwrap();
+    ///     guard[0]
+    /// }
+    /// ```
+    pub TAIL_EXPR_DROP_ORDER,
+    Allow,
+    "Detect and warn on significant change in drop order in tail expression location",
+    @feature_gate = shorter_tail_lifetimes;
+}
+
+declare_lint_pass!(TailExprDropOrder => [TAIL_EXPR_DROP_ORDER]);
+
+impl<'tcx> LateLintPass<'tcx> for TailExprDropOrder {
+    fn check_block(&mut self, cx: &LateContext<'tcx>, block: &'tcx Block<'tcx>) {
+        LintVisitor { cx, locals: <_>::default() }.check_block_inner(block);
+    }
+}
+
+struct LintVisitor<'tcx, 'a> {
+    cx: &'a LateContext<'tcx>,
+    locals: Vec<Span>,
+}
+
+struct LocalCollector<'tcx, 'a> {
+    cx: &'a LateContext<'tcx>,
+    locals: &'a mut Vec<Span>,
+}
+
+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;
+        }
+        debug!(?self.locals, "dxf");
+        LintTailExpr { cx: self.cx, locals: &self.locals }.visit_expr(tail_expr);
+    }
+}
+
+struct LintTailExpr<'tcx, 'a> {
+    cx: &'a LateContext<'tcx>,
+    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.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::ConstBlock(_)
+            | ExprKind::Array(_)
+            | ExprKind::Break(_, _)
+            | ExprKind::Continue(_)
+            | ExprKind::Ret(_)
+            | ExprKind::Become(_)
+            | ExprKind::Yield(_, _)
+            | ExprKind::InlineAsm(_)
+            | ExprKind::If(_, _, _)
+            | ExprKind::Loop(_, _, _, _)
+            | ExprKind::Match(_, _, _)
+            | 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(block, _) => {
+                LintVisitor { cx: self.cx, locals: <_>::default() }.check_block_inner(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<Span>,
+}
diff --git a/tests/ui/drop/lint-tail-expr-drop-order.rs b/tests/ui/drop/lint-tail-expr-drop-order.rs
@@ -0,0 +1,32 @@
+//@compile-flags: -Z unstable-options
+//@edition:2024
+
+#![deny(tail_expr_drop_order)]
+#![feature(shorter_tail_lifetimes)]
+
+struct LoudDropper;
+impl Drop for LoudDropper {
+    fn drop(&mut self) {
+        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
+}
+
+fn should_not_lint() -> i32 {
+    let x = LoudDropper;
+    // Should not lint
+    x.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
@@ -0,0 +1,17 @@
+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:22: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()
+   |               ^^^^^^^^^^^
+   |
+note: the lint level is defined here
+  --> $DIR/lint-tail-expr-drop-order.rs:4:9
+   |
+LL | #![deny(tail_expr_drop_order)]
+   |         ^^^^^^^^^^^^^^^^^^^^
+
+error: aborting due to 1 previous error
+
