Extend branches_sharing_code to match arms with a shared tail

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,

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);
         }
     }
 }

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<u32, ()> {
+    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>) -> 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() {}