Auto merge of rust-lang#8726 - Serial-ATA:issue-8723, r=dswij,xFrednet

Fix `match_single_binding` suggestion for assign expressions

changelog: Fix suggestion for assign expressions in [`match_single_binding`]
closes: rust-lang#8723

Author: bors

clippy_lints/src/matches/match_single_binding.rs

@@ -1,15 +1,22 @@
 use clippy_utils::diagnostics::span_lint_and_sugg;
-use clippy_utils::source::{indent_of, snippet_block, snippet_with_applicability};
+use clippy_utils::macros::HirNode;
+use clippy_utils::source::{indent_of, snippet, snippet_block, snippet_with_applicability};
 use clippy_utils::sugg::Sugg;
 use clippy_utils::{get_parent_expr, is_refutable, peel_blocks};
 use rustc_errors::Applicability;
-use rustc_hir::{Arm, Expr, ExprKind, Local, Node, PatKind};
+use rustc_hir::{Arm, Expr, ExprKind, Node, PatKind};
 use rustc_lint::LateContext;
+use rustc_span::Span;
 
 use super::MATCH_SINGLE_BINDING;
 
+enum AssignmentExpr {
+    Assign { span: Span, match_span: Span },
+    Local { span: Span, pat_span: Span },
+}
+
 #[expect(clippy::too_many_lines)]
-pub(crate) fn check<'a>(cx: &LateContext<'a>, ex: &Expr<'a>, arms: &[Arm<'_>], expr: &Expr<'_>) {
+pub(crate) fn check<'a>(cx: &LateContext<'a>, ex: &Expr<'a>, arms: &[Arm<'_>], expr: &Expr<'a>) {
     if expr.span.from_expansion() || arms.len() != 1 || is_refutable(cx, arms[0].pat) {
         return;
     }
@@ -42,61 +49,59 @@ pub(crate) fn check<'a>(cx: &LateContext<'a>, ex: &Expr<'a>, arms: &[Arm<'_>], e
     let mut applicability = Applicability::MaybeIncorrect;
     match arms[0].pat.kind {
         PatKind::Binding(..) | PatKind::Tuple(_, _) | PatKind::Struct(..) => {
-            // If this match is in a local (`let`) stmt
-            let (target_span, sugg) = if let Some(parent_let_node) = opt_parent_let(cx, ex) {
-                (
-                    parent_let_node.span,
+            let (target_span, sugg) = match opt_parent_assign_span(cx, ex) {
+                Some(AssignmentExpr::Assign { span, match_span }) => {
+                    let sugg = sugg_with_curlies(
+                        cx,
+                        (ex, expr),
+                        (bind_names, matched_vars),
+                        &*snippet_body,
+                        &mut applicability,
+                        Some(span),
+                    );
+
+                    span_lint_and_sugg(
+                        cx,
+                        MATCH_SINGLE_BINDING,
+                        span.to(match_span),
+                        "this assignment could be simplified",
+                        "consider removing the `match` expression",
+                        sugg,
+                        applicability,
+                    );
+
+                    return;
+                },
+                Some(AssignmentExpr::Local { span, pat_span }) => (
+                    span,
                     format!(
                         "let {} = {};\n{}let {} = {};",
                         snippet_with_applicability(cx, bind_names, "..", &mut applicability),
                         snippet_with_applicability(cx, matched_vars, "..", &mut applicability),
                         " ".repeat(indent_of(cx, expr.span).unwrap_or(0)),
-                        snippet_with_applicability(cx, parent_let_node.pat.span, "..", &mut applicability),
+                        snippet_with_applicability(cx, pat_span, "..", &mut applicability),
                         snippet_body
                     ),
-                )
-            } else {
-                // If we are in closure, we need curly braces around suggestion
-                let mut indent = " ".repeat(indent_of(cx, ex.span).unwrap_or(0));
-                let (mut cbrace_start, mut cbrace_end) = ("".to_string(), "".to_string());
-                if let Some(parent_expr) = get_parent_expr(cx, expr) {
-                    if let ExprKind::Closure(..) = parent_expr.kind {
-                        cbrace_end = format!("\n{}}}", indent);
-                        // Fix body indent due to the closure
-                        indent = " ".repeat(indent_of(cx, bind_names).unwrap_or(0));
-                        cbrace_start = format!("{{\n{}", indent);
-                    }
-                }
-                // If the parent is already an arm, and the body is another match statement,
-                // we need curly braces around suggestion
-                let parent_node_id = cx.tcx.hir().get_parent_node(expr.hir_id);
-                if let Node::Arm(arm) = &cx.tcx.hir().get(parent_node_id) {
-                    if let ExprKind::Match(..) = arm.body.kind {
-                        cbrace_end = format!("\n{}}}", indent);
-                        // Fix body indent due to the match
-                        indent = " ".repeat(indent_of(cx, bind_names).unwrap_or(0));
-                        cbrace_start = format!("{{\n{}", indent);
-                    }
-                }
-                (
-                    expr.span,
-                    format!(
-                        "{}let {} = {};\n{}{}{}",
-                        cbrace_start,
-                        snippet_with_applicability(cx, bind_names, "..", &mut applicability),
-                        snippet_with_applicability(cx, matched_vars, "..", &mut applicability),
-                        indent,
-                        snippet_body,
-                        cbrace_end
-                    ),
-                )
+                ),
+                None => {
+                    let sugg = sugg_with_curlies(
+                        cx,
+                        (ex, expr),
+                        (bind_names, matched_vars),
+                        &*snippet_body,
+                        &mut applicability,
+                        None,
+                    );
+                    (expr.span, sugg)
+                },
             };
+
             span_lint_and_sugg(
                 cx,
                 MATCH_SINGLE_BINDING,
                 target_span,
                 "this match could be written as a `let` statement",
-                "consider using `let` statement",
+                "consider using a `let` statement",
                 sugg,
                 applicability,
             );
@@ -110,6 +115,7 @@ pub(crate) fn check<'a>(cx: &LateContext<'a>, ex: &Expr<'a>, arms: &[Arm<'_>], e
                     indent,
                     snippet_body
                 );
+
                 span_lint_and_sugg(
                     cx,
                     MATCH_SINGLE_BINDING,
@@ -135,15 +141,76 @@ pub(crate) fn check<'a>(cx: &LateContext<'a>, ex: &Expr<'a>, arms: &[Arm<'_>], e
     }
 }
 
-/// Returns true if the `ex` match expression is in a local (`let`) statement
-fn opt_parent_let<'a>(cx: &LateContext<'a>, ex: &Expr<'a>) -> Option<&'a Local<'a>> {
+/// Returns true if the `ex` match expression is in a local (`let`) or assign expression
+fn opt_parent_assign_span<'a>(cx: &LateContext<'a>, ex: &Expr<'a>) -> Option<AssignmentExpr> {
     let map = &cx.tcx.hir();
-    if_chain! {
-        if let Some(Node::Expr(parent_arm_expr)) = map.find(map.get_parent_node(ex.hir_id));
-        if let Some(Node::Local(parent_let_expr)) = map.find(map.get_parent_node(parent_arm_expr.hir_id));
-        then {
-            return Some(parent_let_expr);
-        }
+
+    if let Some(Node::Expr(parent_arm_expr)) = map.find(map.get_parent_node(ex.hir_id)) {
+        return match map.find(map.get_parent_node(parent_arm_expr.hir_id)) {
+            Some(Node::Local(parent_let_expr)) => Some(AssignmentExpr::Local {
+                span: parent_let_expr.span,
+                pat_span: parent_let_expr.pat.span(),
+            }),
+            Some(Node::Expr(Expr {
+                kind: ExprKind::Assign(parent_assign_expr, match_expr, _),
+                ..
+            })) => Some(AssignmentExpr::Assign {
+                span: parent_assign_expr.span,
+                match_span: match_expr.span,
+            }),
+            _ => None,
+        };
     }
+
     None
 }
+
+fn sugg_with_curlies<'a>(
+    cx: &LateContext<'a>,
+    (ex, match_expr): (&Expr<'a>, &Expr<'a>),
+    (bind_names, matched_vars): (Span, Span),
+    snippet_body: &str,
+    applicability: &mut Applicability,
+    assignment: Option<Span>,
+) -> String {
+    let mut indent = " ".repeat(indent_of(cx, ex.span).unwrap_or(0));
+
+    let (mut cbrace_start, mut cbrace_end) = (String::new(), String::new());
+    if let Some(parent_expr) = get_parent_expr(cx, match_expr) {
+        if let ExprKind::Closure(..) = parent_expr.kind {
+            cbrace_end = format!("\n{}}}", indent);
+            // Fix body indent due to the closure
+            indent = " ".repeat(indent_of(cx, bind_names).unwrap_or(0));
+            cbrace_start = format!("{{\n{}", indent);
+        }
+    }
+
+    // If the parent is already an arm, and the body is another match statement,
+    // we need curly braces around suggestion
+    let parent_node_id = cx.tcx.hir().get_parent_node(match_expr.hir_id);
+    if let Node::Arm(arm) = &cx.tcx.hir().get(parent_node_id) {
+        if let ExprKind::Match(..) = arm.body.kind {
+            cbrace_end = format!("\n{}}}", indent);
+            // Fix body indent due to the match
+            indent = " ".repeat(indent_of(cx, bind_names).unwrap_or(0));
+            cbrace_start = format!("{{\n{}", indent);
+        }
+    }
+
+    let assignment_str = assignment.map_or_else(String::new, |span| {
+        let mut s = snippet(cx, span, "..").to_string();
+        s.push_str(" = ");
+        s
+    });
+
+    format!(
+        "{}let {} = {};\n{}{}{}{}",
+        cbrace_start,
+        snippet_with_applicability(cx, bind_names, "..", applicability),
+        snippet_with_applicability(cx, matched_vars, "..", applicability),
+        indent,
+        assignment_str,
+        snippet_body,
+        cbrace_end
+    )
+}

tests/ui/match_single_binding.fixed

@@ -111,3 +111,16 @@ fn main() {
     let x = 1;
     println!("Not an array index start");
 }
+
+#[allow(dead_code)]
+fn issue_8723() {
+    let (mut val, idx) = ("a b", 1);
+
+    let (pre, suf) = val.split_at(idx);
+    val = {
+        println!("{}", pre);
+        suf
+    };
+
+    let _ = val;
+}

tests/ui/match_single_binding.rs

@@ -126,3 +126,17 @@ fn main() {
         _ => println!("Not an array index start"),
     }
 }
+
+#[allow(dead_code)]
+fn issue_8723() {
+    let (mut val, idx) = ("a b", 1);
+
+    val = match val.split_at(idx) {
+        (pre, suf) => {
+            println!("{}", pre);
+            suf
+        },
+    };
+
+    let _ = val;
+}

tests/ui/match_single_binding.stderr

@@ -9,7 +9,7 @@ LL | |     }
    | |_____^
    |
    = note: `-D clippy::match-single-binding` implied by `-D warnings`
-help: consider using `let` statement
+help: consider using a `let` statement
    |
 LL ~     let (x, y, z) = (a, b, c);
 LL +     {
@@ -25,7 +25,7 @@ LL | |         (x, y, z) => println!("{} {} {}", x, y, z),
 LL | |     }
    | |_____^
    |
-help: consider using `let` statement
+help: consider using a `let` statement
    |
 LL ~     let (x, y, z) = (a, b, c);
 LL +     println!("{} {} {}", x, y, z);
@@ -88,7 +88,7 @@ LL | |         Point { x, y } => println!("Coords: ({}, {})", x, y),
 LL | |     }
    | |_____^
    |
-help: consider using `let` statement
+help: consider using a `let` statement
    |
 LL ~     let Point { x, y } = p;
 LL +     println!("Coords: ({}, {})", x, y);
@@ -102,7 +102,7 @@ LL | |         Point { x: x1, y: y1 } => println!("Coords: ({}, {})", x1, y1),
 LL | |     }
    | |_____^
    |
-help: consider using `let` statement
+help: consider using a `let` statement
    |
 LL ~     let Point { x: x1, y: y1 } = p;
 LL +     println!("Coords: ({}, {})", x1, y1);
@@ -116,7 +116,7 @@ LL | |         ref r => println!("Got a reference to {}", r),
 LL | |     }
    | |_____^
    |
-help: consider using `let` statement
+help: consider using a `let` statement
    |
 LL ~     let ref r = x;
 LL +     println!("Got a reference to {}", r);
@@ -130,7 +130,7 @@ LL | |         ref mut mr => println!("Got a mutable reference to {}", mr),
 LL | |     }
    | |_____^
    |
-help: consider using `let` statement
+help: consider using a `let` statement
    |
 LL ~     let ref mut mr = x;
 LL +     println!("Got a mutable reference to {}", mr);
@@ -144,7 +144,7 @@ LL | |         Point { x, y } => x * y,
 LL | |     };
    | |______^
    |
-help: consider using `let` statement
+help: consider using a `let` statement
    |
 LL ~     let Point { x, y } = coords();
 LL +     let product = x * y;
@@ -159,7 +159,7 @@ LL | |             unwrapped => unwrapped,
 LL | |         })
    | |_________^
    |
-help: consider using `let` statement
+help: consider using a `let` statement
    |
 LL ~         .map(|i| {
 LL +             let unwrapped = i.unwrap();
@@ -176,5 +176,25 @@ LL | |         _ => println!("Not an array index start"),
 LL | |     }
    | |_____^ help: consider using the match body instead: `println!("Not an array index start");`
 
-error: aborting due to 12 previous errors
+error: this assignment could be simplified
+  --> $DIR/match_single_binding.rs:134:5
+   |
+LL | /     val = match val.split_at(idx) {
+LL | |         (pre, suf) => {
+LL | |             println!("{}", pre);
+LL | |             suf
+LL | |         },
+LL | |     };
+   | |_____^
+   |
+help: consider removing the `match` expression
+   |
+LL ~     let (pre, suf) = val.split_at(idx);
+LL +     val = {
+LL +         println!("{}", pre);
+LL +         suf
+LL ~     };
+   |
+
+error: aborting due to 13 previous errors
 

tests/ui/match_single_binding2.stderr

@@ -8,7 +8,7 @@ LL | |             },
    | |_____________^
    |
    = note: `-D clippy::match-single-binding` implied by `-D warnings`
-help: consider using `let` statement
+help: consider using a `let` statement
    |
 LL ~             Some((iter, _item)) => {
 LL +                 let (min, max) = iter.size_hint();
@@ -24,7 +24,7 @@ LL | |                 (a, b) => println!("a {:?} and b {:?}", a, b),
 LL | |             }
    | |_____________^
    |
-help: consider using `let` statement
+help: consider using a `let` statement
    |
 LL ~             let (a, b) = get_tup();
 LL +             println!("a {:?} and b {:?}", a, b);