From: Serial <69764315+Serial-ATA@users.noreply.github.com> Date: Wed, 20 Apr 2022 14:14:11 +0000 (-0400) Subject: Fix `match_single_binding` for assign expressions X-Git-Url: https://git.lizzy.rs/?a=commitdiff_plain;h=554dc41ceac0f4a625fc344e9248e7e1dae11378;p=rust.git Fix `match_single_binding` for assign expressions --- diff --git a/clippy_lints/src/matches/match_single_binding.rs b/clippy_lints/src/matches/match_single_binding.rs index 028e8c297fb..a59711d4cac 100644 --- a/clippy_lints/src/matches/match_single_binding.rs +++ b/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 { 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, +) -> 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 + ) +} diff --git a/tests/ui/match_single_binding.fixed b/tests/ui/match_single_binding.fixed index b8dc8179f7d..de46e6cff55 100644 --- a/tests/ui/match_single_binding.fixed +++ b/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; +} diff --git a/tests/ui/match_single_binding.rs b/tests/ui/match_single_binding.rs index fe63dcd63f2..eea64fcb292 100644 --- a/tests/ui/match_single_binding.rs +++ b/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; +} diff --git a/tests/ui/match_single_binding.stderr b/tests/ui/match_single_binding.stderr index d939291f53c..5d4e7314b21 100644 --- a/tests/ui/match_single_binding.stderr +++ b/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 diff --git a/tests/ui/match_single_binding2.stderr b/tests/ui/match_single_binding2.stderr index d3493319466..22bf7d8be4a 100644 --- a/tests/ui/match_single_binding2.stderr +++ b/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);