From 7bca0467369bcfae4feeb8307374ebf134fe3e71 Mon Sep 17 00:00:00 2001 From: Ding Xiang Fei Date: Mon, 29 Jul 2024 01:44:34 +0800 Subject: [PATCH] lint against possible change in drop order from tail expr --- CHANGELOG.md | 1 + clippy_lints/src/declared_lints.rs | 1 + clippy_lints/src/lib.rs | 2 + clippy_lints/src/tail_expr_drop_order.rs | 229 +++++++++++++++++++++++ tests/ui/tail_expr_drop_order.rs | 30 +++ tests/ui/tail_expr_drop_order.stderr | 16 ++ 6 files changed, 279 insertions(+) create mode 100644 clippy_lints/src/tail_expr_drop_order.rs create mode 100644 tests/ui/tail_expr_drop_order.rs create mode 100644 tests/ui/tail_expr_drop_order.stderr diff --git a/CHANGELOG.md b/CHANGELOG.md index 60c03b03d9be..7c3e61c267a1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5903,6 +5903,7 @@ Released 2018-09-13 [`suspicious_xor_used_as_pow`]: https://rust-lang.github.io/rust-clippy/master/index.html#suspicious_xor_used_as_pow [`swap_ptr_to_ref`]: https://rust-lang.github.io/rust-clippy/master/index.html#swap_ptr_to_ref [`tabs_in_doc_comments`]: https://rust-lang.github.io/rust-clippy/master/index.html#tabs_in_doc_comments +[`tail_expr_drop_order`]: https://rust-lang.github.io/rust-clippy/master/index.html#tail_expr_drop_order [`temporary_assignment`]: https://rust-lang.github.io/rust-clippy/master/index.html#temporary_assignment [`temporary_cstring_as_ptr`]: https://rust-lang.github.io/rust-clippy/master/index.html#temporary_cstring_as_ptr [`test_attr_in_doctest`]: https://rust-lang.github.io/rust-clippy/master/index.html#test_attr_in_doctest diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index 69f9eb6842bc..72e9a2cda6a8 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -681,6 +681,7 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[ crate::swap::MANUAL_SWAP_INFO, crate::swap_ptr_to_ref::SWAP_PTR_TO_REF_INFO, crate::tabs_in_doc_comments::TABS_IN_DOC_COMMENTS_INFO, + crate::tail_expr_drop_order::TAIL_EXPR_DROP_ORDER_INFO, crate::temporary_assignment::TEMPORARY_ASSIGNMENT_INFO, crate::tests_outside_test_module::TESTS_OUTSIDE_TEST_MODULE_INFO, crate::to_digit_is_some::TO_DIGIT_IS_SOME_INFO, diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 9c035bfca39c..cdae06a82586 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -345,6 +345,7 @@ mod suspicious_xor_used_as_pow; mod swap; mod swap_ptr_to_ref; mod tabs_in_doc_comments; +mod tail_expr_drop_order; mod temporary_assignment; mod tests_outside_test_module; mod to_digit_is_some; @@ -911,6 +912,7 @@ pub fn register_lints(store: &mut rustc_lint::LintStore, conf: &'static Conf) { store.register_late_pass(|_| Box::new(set_contains_or_insert::SetContainsOrInsert)); store.register_early_pass(|| Box::new(byte_char_slices::ByteCharSlice)); store.register_early_pass(|| Box::new(cfg_not_test::CfgNotTest)); + store.register_late_pass(|_| Box::new(tail_expr_drop_order::TailExprDropOrder)); // add lints here, do not remove this comment, it's used in `new_lint` } diff --git a/clippy_lints/src/tail_expr_drop_order.rs b/clippy_lints/src/tail_expr_drop_order.rs new file mode 100644 index 000000000000..a32763d4107e --- /dev/null +++ b/clippy_lints/src/tail_expr_drop_order.rs @@ -0,0 +1,229 @@ +use std::mem::swap; + +use clippy_utils::diagnostics::span_lint_and_then; +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_lint::{LateContext, LateLintPass}; +use rustc_session::declare_lint_pass; +use rustc_span::Span; + +declare_clippy_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 + /// ```ignore + /// 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] + /// } + /// ``` + #[clippy::version = "1.82.0"] + pub TAIL_EXPR_DROP_ORDER, + nursery, + "Detect and warn on significant change in drop order in tail expression location" +} + +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, +} + +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, + } + .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) { + span_lint_and_then( + self.cx, + TAIL_EXPR_DROP_ORDER, + expr.span, + "discretion required on this expression which generates a value with a significant drop implementation", + |diag| { + diag.span_help(self.locals.to_vec(), "one or more locals with a significant drop implementation will observe a visible change in drop order"); + }, + ); + 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); + } +} diff --git a/tests/ui/tail_expr_drop_order.rs b/tests/ui/tail_expr_drop_order.rs new file mode 100644 index 000000000000..afeea1c6f30a --- /dev/null +++ b/tests/ui/tail_expr_drop_order.rs @@ -0,0 +1,30 @@ +//@compile-flags: -Z unstable-options +//@edition:2024 + +#![warn(clippy::tail_expr_drop_order)] + +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: discretion required on this expression which generates a value with a significant drop implementation +} + +fn should_not_lint() -> i32 { + let x = LoudDropper; + // Should not lint + x.get() +} + +fn main() {} diff --git a/tests/ui/tail_expr_drop_order.stderr b/tests/ui/tail_expr_drop_order.stderr new file mode 100644 index 000000000000..1b9cad881058 --- /dev/null +++ b/tests/ui/tail_expr_drop_order.stderr @@ -0,0 +1,16 @@ +error: discretion required on this expression which generates a value with a significant drop implementation + --> tests/ui/tail_expr_drop_order.rs:21:15 + | +LL | x.get() + LoudDropper.get() + | ^^^^^^^^^^^ + | +help: one or more locals with a significant drop implementation will observe a visible change in drop order + --> tests/ui/tail_expr_drop_order.rs:19:9 + | +LL | let x = LoudDropper; + | ^ + = note: `-D clippy::tail-expr-drop-order` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::tail_expr_drop_order)]` + +error: aborting due to 1 previous error +