From 4dd09d6ce5b49eb7ea2d55718d742a638adc1f10 Mon Sep 17 00:00:00 2001 From: Sylvestre Ledru Date: Sun, 28 Jun 2026 10:43:06 +0200 Subject: [PATCH] Extend branches_sharing_code to match arms with a shared tail Detect `match` expressions where every arm is a block ending in the same trailing expression and the `match` is in tail position, suggesting the expression be hoisted out below the `match`. changelog: [`branches_sharing_code`]: also lint `match` expressions whose arms end with the same expression --- clippy_lints/src/ifs/branches_sharing_code.rs | 110 +++++++++++- clippy_lints/src/ifs/mod.rs | 29 +++- .../shared_match_tail.fixed | 153 +++++++++++++++++ .../shared_match_tail.rs | 162 ++++++++++++++++++ .../shared_match_tail.stderr | 115 +++++++++++++ 5 files changed, 565 insertions(+), 4 deletions(-) create mode 100644 tests/ui/branches_sharing_code/shared_match_tail.fixed create mode 100644 tests/ui/branches_sharing_code/shared_match_tail.rs create mode 100644 tests/ui/branches_sharing_code/shared_match_tail.stderr diff --git a/clippy_lints/src/ifs/branches_sharing_code.rs b/clippy_lints/src/ifs/branches_sharing_code.rs index 2701cdaa394d..010ac361085b 100644 --- a/clippy_lints/src/ifs/branches_sharing_code.rs +++ b/clippy_lints/src/ifs/branches_sharing_code.rs @@ -5,11 +5,14 @@ use clippy_utils::ty::needs_ordered_drop; use clippy_utils::visitors::for_each_expr_without_closures; use clippy_utils::{ ContainsName, HirEqInterExpr, SpanlessEq, capture_local_usage, get_enclosing_block, hash_expr, hash_stmt, + is_expr_final_block_expr, }; use core::iter; use core::ops::ControlFlow; use rustc_errors::Applicability; -use rustc_hir::{Block, Expr, ExprKind, HirId, HirIdSet, ItemKind, LetStmt, Node, Stmt, StmtKind, UseKind, intravisit}; +use rustc_hir::{ + Arm, Block, Expr, ExprKind, HirId, HirIdSet, ItemKind, LetStmt, Node, Stmt, StmtKind, UseKind, intravisit, +}; use rustc_lint::LateContext; use rustc_span::hygiene::walk_chain; use rustc_span::source_map::SourceMap; @@ -100,6 +103,111 @@ pub(super) fn check<'tcx>( }); } +/// Detects `match` expressions where every arm's body is a block ending in the same trailing +/// expression, so that expression can be hoisted out below the `match`. +/// +/// ```ignore +/// match mode { +/// Mode::A => { a(); Ok(()) } +/// Mode::B => { b(); Ok(()) } +/// } +/// ``` +/// can become +/// ```ignore +/// match mode { +/// Mode::A => a(), +/// Mode::B => b(), +/// } +/// Ok(()) +/// ``` +pub(super) fn check_match<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, arms: &'tcx [Arm<'tcx>]) { + if arms.len() < 2 { + return; + } + + // We only move the shared expression after the `match`, so the `match` must be in tail position + // of its block. Cheapest, most selective check, so do it first. + if !is_expr_final_block_expr(cx.tcx, expr) { + return; + } + + // Each arm body must be a block with a trailing expression. + let mut blocks = Vec::with_capacity(arms.len()); + for arm in arms { + let ExprKind::Block(block, None) = arm.body.kind else { + return; + }; + let Some(tail) = block.expr else { + return; + }; + if block.span.from_expansion() || tail.span.from_expansion() { + return; + } + // Same drop-order concern for arm-locals dropped at the end of the arm. + let arm_local_needs_ordered_drop = block.stmts.iter().any(|stmt| { + matches!(stmt.kind, StmtKind::Let(l) if needs_ordered_drop(cx, cx.typeck_results().node_type(l.hir_id))) + }); + if arm_local_needs_ordered_drop { + return; + } + blocks.push((block, tail)); + } + + // All tails must be equal. This also rules out tails referencing an arm-local: those have a + // distinct `HirId` per arm and so never compare equal. + let first = blocks[0].1; + let mut eq = SpanlessEq::new(cx); + if !blocks[1..] + .iter() + .all(|&(_, t)| eq.eq_expr(SyntaxContext::root(), first, t)) + { + return; + } + + // Don't bother for a `()` tail: nothing worth moving. + if matches!(first.kind, ExprKind::Tup([])) { + return; + } + + // Require some code before the tail, else hoisting just leaves empty arms. + if blocks.iter().all(|(block, _)| block.stmts.is_empty()) { + return; + } + + span_lint_and_then( + cx, + BRANCHES_SHARING_CODE, + expr.span, + "all match arms end with the same expression", + |diag| { + let indent = " ".repeat(indent_of(cx, expr.span).unwrap_or(0)); + let tail_snippet = snippet(cx, first.span, "..").into_owned(); + let mut suggestions: Vec<(Span, String)> = blocks + .iter() + .map(|&(block, tail)| { + // Drop the tail (and the whitespace before it) from each arm. + let span = match block.stmts.last() { + Some(last) => last.span.shrink_to_hi().to(tail.span), + None => tail.span, + }; + (span, String::new()) + }) + .collect(); + suggestions.push((expr.span.shrink_to_hi(), format!("\n{indent}{tail_snippet}"))); + // `Unspecified`, like the if/else end-suggestion: may need manual adjustment (e.g. a + // significant-drop temporary in the scrutinee), so don't auto-apply via `--fix`. + diag.multipart_suggestion( + "consider moving the shared expression after the `match`", + suggestions, + Applicability::Unspecified, + ); + diag.note( + "the suggestion may need adjustments to preserve drop order or to use the expression result correctly", + ); + }, + ); +} + struct BlockEq { /// The end of the range of equal stmts at the start. start_end_eq: usize, diff --git a/clippy_lints/src/ifs/mod.rs b/clippy_lints/src/ifs/mod.rs index 8a22d8935368..8e693c3e0e70 100644 --- a/clippy_lints/src/ifs/mod.rs +++ b/clippy_lints/src/ifs/mod.rs @@ -13,8 +13,8 @@ mod same_functions_in_if_cond; declare_clippy_lint! { /// ### What it does - /// Checks if the `if` and `else` block contain shared code that can be - /// moved out of the blocks. + /// Checks if the blocks of an `if`/`else`, or the arms of a `match`, contain shared code that + /// can be moved out of the branches. /// /// ### Why is this bad? /// Duplicate code is less maintainable. @@ -39,6 +39,24 @@ declare_clippy_lint! { /// 42 /// }; /// ``` + /// + /// For a `match`, when every arm ends with the same expression it can be moved after the + /// `match`: + /// ```ignore + /// match mode { + /// Mode::A => { a(); Ok(()) } + /// Mode::B => { b(); Ok(()) } + /// } + /// ``` + /// + /// Use instead: + /// ```ignore + /// match mode { + /// Mode::A => { a(); } + /// Mode::B => { b(); } + /// } + /// Ok(()) + /// ``` #[clippy::version = "1.53.0"] pub BRANCHES_SHARING_CODE, nursery, @@ -168,7 +186,10 @@ impl<'tcx> CopyAndPaste<'tcx> { impl<'tcx> LateLintPass<'tcx> for CopyAndPaste<'tcx> { fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) { - if !expr.span.from_expansion() && matches!(expr.kind, ExprKind::If(..)) && !is_else_clause(cx.tcx, expr) { + if expr.span.from_expansion() { + return; + } + if matches!(expr.kind, ExprKind::If(..)) && !is_else_clause(cx.tcx, expr) { let (conds, blocks) = if_sequence(expr); ifs_same_cond::check(cx, &conds, &mut self.interior_mut); same_functions_in_if_cond::check(cx, &conds); @@ -177,6 +198,8 @@ impl<'tcx> LateLintPass<'tcx> for CopyAndPaste<'tcx> { if !all_same && conds.len() != blocks.len() { branches_sharing_code::check(cx, &conds, &blocks, expr); } + } else if let ExprKind::Match(_, arms, _) = expr.kind { + branches_sharing_code::check_match(cx, expr, arms); } } } diff --git a/tests/ui/branches_sharing_code/shared_match_tail.fixed b/tests/ui/branches_sharing_code/shared_match_tail.fixed new file mode 100644 index 000000000000..bacecea1a876 --- /dev/null +++ b/tests/ui/branches_sharing_code/shared_match_tail.fixed @@ -0,0 +1,153 @@ +#![deny(clippy::branches_sharing_code)] +#![allow(clippy::let_and_return)] + +enum Mode { + Kill, + Table, + List, +} + +fn kill() {} +fn table() {} +fn list() {} + +// Every arm ends in `Ok(())`: the shared tail can be hoisted out below the `match`. +fn shared_tail(mode: Mode) -> Result<(), ()> { + //~v branches_sharing_code + match mode { + Mode::Kill => { + kill(); + }, + Mode::Table => { + table(); + }, + Mode::List => { + list(); + }, + } + Ok(()) +} + +// Shared tail even though one arm has an early `return`. +fn shared_tail_with_return(mode: Mode, skip: bool) -> Result<(), ()> { + //~v branches_sharing_code + match mode { + Mode::Kill => { + if skip { + return Ok(()); + } + kill(); + }, + _ => { + table(); + }, + } + Ok(()) +} + +// No lint: the trailing expressions differ. +fn different_tail(mode: Mode) -> Result { + match mode { + Mode::Kill => { + kill(); + Ok(1) + }, + Mode::Table => { + table(); + Ok(2) + }, + Mode::List => Ok(3), + } +} + +// No lint: the tail references a binding introduced inside the arm. +fn refs_local(x: Option) -> i32 { + match x { + Some(v) => { + kill(); + v + }, + None => { + table(); + 0 + }, + } +} + +// No lint: the `match` value is bound, so the tail cannot simply be moved after it. +fn value_used(mode: Mode) -> Result<(), ()> { + let res = match mode { + Mode::Kill => { + kill(); + Ok(()) + }, + _ => { + table(); + Ok(()) + }, + }; + res +} + +struct Guard; + +impl Drop for Guard { + fn drop(&mut self) {} +} + +// No lint: an arm-local binding has a non-trivial `Drop`. Hoisting the tail would drop the guard +// before the moved expression runs, changing observable drop order. +fn arm_local_drop(mode: Mode) -> Result<(), ()> { + match mode { + Mode::Kill => { + let _g = Guard; + kill(); + Ok(()) + }, + _ => { + table(); + Ok(()) + }, + } +} + +// Comments inside the arm bodies, including right before the shared tail. +fn comments_in_arm_body(mode: Mode) -> Result<(), ()> { + //~v branches_sharing_code + match mode { + Mode::Kill => { + // kill it + kill(); + }, + Mode::Table => { + // table it + table(); + }, + Mode::List => { + list(); + }, + } + Ok(()) +} + +// Comments before the `=>` of each arm. +fn comments_before_fat_arrow(mode: Mode) -> Result<(), ()> { + //~v branches_sharing_code + match mode { + // killing time + Mode::Kill => { + kill(); + }, + // table time + Mode::Table => { + table(); + }, + // list time + Mode::List => { + list(); + }, + } + Ok(()) +} + +fn main() {} diff --git a/tests/ui/branches_sharing_code/shared_match_tail.rs b/tests/ui/branches_sharing_code/shared_match_tail.rs new file mode 100644 index 000000000000..38160e2fc000 --- /dev/null +++ b/tests/ui/branches_sharing_code/shared_match_tail.rs @@ -0,0 +1,162 @@ +#![deny(clippy::branches_sharing_code)] +#![allow(clippy::let_and_return)] + +enum Mode { + Kill, + Table, + List, +} + +fn kill() {} +fn table() {} +fn list() {} + +// Every arm ends in `Ok(())`: the shared tail can be hoisted out below the `match`. +fn shared_tail(mode: Mode) -> Result<(), ()> { + //~v branches_sharing_code + match mode { + Mode::Kill => { + kill(); + Ok(()) + }, + Mode::Table => { + table(); + Ok(()) + }, + Mode::List => { + list(); + Ok(()) + }, + } +} + +// Shared tail even though one arm has an early `return`. +fn shared_tail_with_return(mode: Mode, skip: bool) -> Result<(), ()> { + //~v branches_sharing_code + match mode { + Mode::Kill => { + if skip { + return Ok(()); + } + kill(); + Ok(()) + }, + _ => { + table(); + Ok(()) + }, + } +} + +// No lint: the trailing expressions differ. +fn different_tail(mode: Mode) -> Result { + match mode { + Mode::Kill => { + kill(); + Ok(1) + }, + Mode::Table => { + table(); + Ok(2) + }, + Mode::List => Ok(3), + } +} + +// No lint: the tail references a binding introduced inside the arm. +fn refs_local(x: Option) -> i32 { + match x { + Some(v) => { + kill(); + v + }, + None => { + table(); + 0 + }, + } +} + +// No lint: the `match` value is bound, so the tail cannot simply be moved after it. +fn value_used(mode: Mode) -> Result<(), ()> { + let res = match mode { + Mode::Kill => { + kill(); + Ok(()) + }, + _ => { + table(); + Ok(()) + }, + }; + res +} + +struct Guard; + +impl Drop for Guard { + fn drop(&mut self) {} +} + +// No lint: an arm-local binding has a non-trivial `Drop`. Hoisting the tail would drop the guard +// before the moved expression runs, changing observable drop order. +fn arm_local_drop(mode: Mode) -> Result<(), ()> { + match mode { + Mode::Kill => { + let _g = Guard; + kill(); + Ok(()) + }, + _ => { + table(); + Ok(()) + }, + } +} + +// Comments inside the arm bodies, including right before the shared tail. +fn comments_in_arm_body(mode: Mode) -> Result<(), ()> { + //~v branches_sharing_code + match mode { + Mode::Kill => { + // kill it + kill(); + // done killing + Ok(()) + }, + Mode::Table => { + // table it + table(); + Ok(()) + }, + Mode::List => { + list(); + // list it, then return + Ok(()) + }, + } +} + +// Comments before the `=>` of each arm. +fn comments_before_fat_arrow(mode: Mode) -> Result<(), ()> { + //~v branches_sharing_code + match mode { + // killing time + Mode::Kill => { + kill(); + Ok(()) + }, + // table time + Mode::Table => { + table(); + Ok(()) + }, + // list time + Mode::List => { + list(); + Ok(()) + }, + } +} + +fn main() {} diff --git a/tests/ui/branches_sharing_code/shared_match_tail.stderr b/tests/ui/branches_sharing_code/shared_match_tail.stderr new file mode 100644 index 000000000000..e7fa9f1b0d65 --- /dev/null +++ b/tests/ui/branches_sharing_code/shared_match_tail.stderr @@ -0,0 +1,115 @@ +error: all match arms end with the same expression + --> tests/ui/branches_sharing_code/shared_match_tail.rs:17:5 + | +LL | / match mode { +LL | | Mode::Kill => { +LL | | kill(); +LL | | Ok(()) +... | +LL | | }, +LL | | } + | |_____^ + | + = note: the suggestion may need adjustments to preserve drop order or to use the expression result correctly +note: the lint level is defined here + --> tests/ui/branches_sharing_code/shared_match_tail.rs:1:9 + | +LL | #![deny(clippy::branches_sharing_code)] + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +help: consider moving the shared expression after the `match` + | +LL ~ kill(); +LL | }, +LL | Mode::Table => { +LL ~ table(); +LL | }, +LL | Mode::List => { +LL ~ list(); +LL | }, +LL ~ } +LL + Ok(()) + | + +error: all match arms end with the same expression + --> tests/ui/branches_sharing_code/shared_match_tail.rs:36:5 + | +LL | / match mode { +LL | | Mode::Kill => { +LL | | if skip { +LL | | return Ok(()); +... | +LL | | }, +LL | | } + | |_____^ + | + = note: the suggestion may need adjustments to preserve drop order or to use the expression result correctly +help: consider moving the shared expression after the `match` + | +LL ~ kill(); +LL | }, +LL | _ => { +LL ~ table(); +LL | }, +LL ~ } +LL + Ok(()) + | + +error: all match arms end with the same expression + --> tests/ui/branches_sharing_code/shared_match_tail.rs:120:5 + | +LL | / match mode { +LL | | Mode::Kill => { +LL | | // kill it +LL | | kill(); +... | +LL | | }, +LL | | } + | |_____^ + | + = note: the suggestion may need adjustments to preserve drop order or to use the expression result correctly +help: consider moving the shared expression after the `match` + | +LL ~ kill(); +LL | }, +LL | Mode::Table => { +LL | // table it +LL ~ table(); +LL | }, +LL | Mode::List => { +LL ~ list(); +LL | }, +LL ~ } +LL + Ok(()) + | + +error: all match arms end with the same expression + --> tests/ui/branches_sharing_code/shared_match_tail.rs:143:5 + | +LL | / match mode { +LL | | // killing time +LL | | Mode::Kill => { +LL | | kill(); +... | +LL | | }, +LL | | } + | |_____^ + | + = note: the suggestion may need adjustments to preserve drop order or to use the expression result correctly +help: consider moving the shared expression after the `match` + | +LL ~ kill(); +LL | }, +LL | // table time +LL | Mode::Table => { +LL ~ table(); +LL | }, +LL | // list time +LL | Mode::List => { +LL ~ list(); +LL | }, +LL ~ } +LL + Ok(()) + | + +error: aborting due to 4 previous errors +