diff --git a/clippy_lints/src/matches.rs b/clippy_lints/src/matches.rs index e4335abcea2..2323df1f0fc 100644 --- a/clippy_lints/src/matches.rs +++ b/clippy_lints/src/matches.rs @@ -3,9 +3,9 @@ use crate::utils::paths; use crate::utils::sugg::Sugg; use crate::utils::usage::is_unused; use crate::utils::{ - span_lint_and_help, span_lint_and_note, - expr_block, in_macro, is_allowed, is_expn_of, is_wild, match_qpath, match_type, multispan_sugg, remove_blocks, - snippet, snippet_block, snippet_with_applicability, span_lint_and_sugg, span_lint_and_then, + expr_block, get_arg_name, in_macro, is_allowed, is_expn_of, is_refutable, is_wild, match_qpath, match_type, + match_var, multispan_sugg, remove_blocks, snippet, snippet_block, snippet_with_applicability, span_lint_and_help, + span_lint_and_note, span_lint_and_sugg, span_lint_and_then, walk_ptrs_ty, }; use if_chain::if_chain; use rustc::lint::in_external_macro; @@ -822,36 +822,66 @@ fn check_wild_in_or_pats(cx: &LateContext<'_, '_>, arms: &[Arm<'_>]) { } fn check_match_single_binding(cx: &LateContext<'_, '_>, ex: &Expr<'_>, arms: &[Arm<'_>], expr: &Expr<'_>) { - if in_macro(expr.span) { + if in_macro(expr.span) || arms.len() != 1 || is_refutable(cx, arms[0].pat) { return; } - if arms.len() == 1 { - if is_refutable(cx, arms[0].pat) { - return; - } - match arms[0].pat.kind { - PatKind::Binding(..) | PatKind::Tuple(_, _) => { - let bind_names = arms[0].pat.span; - let matched_vars = ex.span; - let match_body = remove_blocks(&arms[0].body); - span_lint_and_sugg( - cx, - MATCH_SINGLE_BINDING, - expr.span, - "this match could be written as a `let` statement", - "consider using `let` statement", - format!( - "let {} = {};\n{}", - snippet(cx, bind_names, ".."), - snippet(cx, matched_vars, ".."), - snippet_block(cx, match_body.span, "..") - ), - Applicability::MachineApplicable, - ); - }, - _ => (), + let matched_vars = ex.span; + let bind_names = arms[0].pat.span; + let match_body = remove_blocks(&arms[0].body); + let mut snippet_body = if match_body.span.from_expansion() { + Sugg::hir_with_macro_callsite(cx, match_body, "..").to_string() + } else { + snippet_block(cx, match_body.span, "..").to_owned().to_string() + }; + + // Do we need to add ';' to suggestion ? + if_chain! { + if let ExprKind::Block(block, _) = &arms[0].body.kind; + if block.stmts.len() == 1; + if let StmtKind::Semi(s) = block.stmts.get(0).unwrap().kind; + then { + match s.kind { + ExprKind::Block(_, _) => (), + _ => { + // expr_ty(body) == () + if cx.tables.expr_ty(&arms[0].body).is_unit() { + snippet_body.push(';'); + } + } + } } } + + match arms[0].pat.kind { + PatKind::Binding(..) | PatKind::Tuple(_, _) | PatKind::Struct(..) => { + span_lint_and_sugg( + cx, + MATCH_SINGLE_BINDING, + expr.span, + "this match could be written as a `let` statement", + "consider using `let` statement", + format!( + "let {} = {};\n{}", + snippet(cx, bind_names, ".."), + snippet(cx, matched_vars, ".."), + snippet_body + ), + Applicability::MachineApplicable, + ); + }, + PatKind::Wild => { + span_lint_and_sugg( + cx, + MATCH_SINGLE_BINDING, + expr.span, + "this match could be replaced by its body itself", + "consider using the match body instead", + snippet_body, + Applicability::MachineApplicable, + ); + }, + _ => (), + } } /// Gets all arms that are unbounded `PatRange`s. diff --git a/tests/ui/escape_analysis.rs b/tests/ui/escape_analysis.rs index 7d4b71d09dc..c0a52d832c0 100644 --- a/tests/ui/escape_analysis.rs +++ b/tests/ui/escape_analysis.rs @@ -4,6 +4,7 @@ clippy::needless_pass_by_value, clippy::unused_unit, clippy::redundant_clone, + clippy::match_single_binding )] #![warn(clippy::boxed_local)] diff --git a/tests/ui/match_ref_pats.rs b/tests/ui/match_ref_pats.rs index d26b59db91b..5de43733ad3 100644 --- a/tests/ui/match_ref_pats.rs +++ b/tests/ui/match_ref_pats.rs @@ -26,6 +26,7 @@ fn ref_pats() { } // False positive: only wildcard pattern. let w = Some(0); + #[allow(clippy::match_single_binding)] match w { _ => println!("none"), } diff --git a/tests/ui/match_ref_pats.stderr b/tests/ui/match_ref_pats.stderr index 492f4b9ba20..52cb4a14b72 100644 --- a/tests/ui/match_ref_pats.stderr +++ b/tests/ui/match_ref_pats.stderr @@ -47,7 +47,7 @@ LL | None => println!("none"), | error: you don't need to add `&` to all patterns - --> $DIR/match_ref_pats.rs:34:5 + --> $DIR/match_ref_pats.rs:35:5 | LL | / if let &None = a { LL | | println!("none"); @@ -60,7 +60,7 @@ LL | if let None = *a { | ^^^^ ^^ error: you don't need to add `&` to both the expression and the patterns - --> $DIR/match_ref_pats.rs:39:5 + --> $DIR/match_ref_pats.rs:40:5 | LL | / if let &None = &b { LL | | println!("none"); @@ -73,7 +73,7 @@ LL | if let None = b { | ^^^^ ^ error: you don't need to add `&` to all patterns - --> $DIR/match_ref_pats.rs:66:9 + --> $DIR/match_ref_pats.rs:67:9 | LL | / match foo_variant!(0) { LL | | &Foo::A => println!("A"), diff --git a/tests/ui/match_single_binding.fixed b/tests/ui/match_single_binding.fixed index 41faa1e1c21..8fb8dc323e4 100644 --- a/tests/ui/match_single_binding.fixed +++ b/tests/ui/match_single_binding.fixed @@ -1,7 +1,12 @@ // run-rustfix #![warn(clippy::match_single_binding)] -#[allow(clippy::many_single_char_names)] +#![allow(clippy::many_single_char_names, clippy::toplevel_ref_arg)] + +struct Point { + x: i32, + y: i32, +} fn main() { let a = 1; @@ -12,6 +17,9 @@ fn main() { { println!("{} {} {}", x, y, z); } + // Lint + let (x, y, z) = (a, b, c); +println!("{} {} {}", x, y, z); // Ok match a { 2 => println!("2"), @@ -23,4 +31,33 @@ fn main() { Some(d) => println!("{}", d), _ => println!("None"), } + // Lint + println!("whatever"); + // Lint + { + let x = 29; + println!("x has a value of {}", x); +} + // Lint + { + let e = 5 * a; + if e >= 5 { + println!("e is superior to 5"); + } +} + // Lint + let p = Point { x: 0, y: 7 }; + let Point { x, y } = p; +println!("Coords: ({}, {})", x, y); + // Lint + let Point { x: x1, y: y1 } = p; +println!("Coords: ({}, {})", x1, y1); + // Lint + let x = 5; + let ref r = x; +println!("Got a reference to {}", r); + // Lint + let mut x = 5; + let ref mut mr = x; +println!("Got a mutable reference to {}", mr); } diff --git a/tests/ui/match_single_binding.rs b/tests/ui/match_single_binding.rs index 06b924d0471..55b0b09a008 100644 --- a/tests/ui/match_single_binding.rs +++ b/tests/ui/match_single_binding.rs @@ -1,7 +1,12 @@ // run-rustfix #![warn(clippy::match_single_binding)] -#[allow(clippy::many_single_char_names)] +#![allow(clippy::many_single_char_names, clippy::toplevel_ref_arg)] + +struct Point { + x: i32, + y: i32, +} fn main() { let a = 1; @@ -13,6 +18,10 @@ fn main() { println!("{} {} {}", x, y, z); }, } + // Lint + match (a, b, c) { + (x, y, z) => println!("{} {} {}", x, y, z), + } // Ok match a { 2 => println!("2"), @@ -24,4 +33,43 @@ fn main() { Some(d) => println!("{}", d), _ => println!("None"), } + // Lint + match a { + _ => println!("whatever"), + } + // Lint + match a { + _ => { + let x = 29; + println!("x has a value of {}", x); + }, + } + // Lint + match a { + _ => { + let e = 5 * a; + if e >= 5 { + println!("e is superior to 5"); + } + }, + } + // Lint + let p = Point { x: 0, y: 7 }; + match p { + Point { x, y } => println!("Coords: ({}, {})", x, y), + } + // Lint + match p { + Point { x: x1, y: y1 } => println!("Coords: ({}, {})", x1, y1), + } + // Lint + let x = 5; + match x { + ref r => println!("Got a reference to {}", r), + } + // Lint + let mut x = 5; + match x { + ref mut mr => println!("Got a mutable reference to {}", mr), + } } diff --git a/tests/ui/match_single_binding.stderr b/tests/ui/match_single_binding.stderr index 64216a72ef7..d76e229adff 100644 --- a/tests/ui/match_single_binding.stderr +++ b/tests/ui/match_single_binding.stderr @@ -1,5 +1,5 @@ error: this match could be written as a `let` statement - --> $DIR/match_single_binding.rs:11:5 + --> $DIR/match_single_binding.rs:16:5 | LL | / match (a, b, c) { LL | | (x, y, z) => { @@ -17,5 +17,124 @@ LL | println!("{} {} {}", x, y, z); LL | } | -error: aborting due to previous error +error: this match could be written as a `let` statement + --> $DIR/match_single_binding.rs:22:5 + | +LL | / match (a, b, c) { +LL | | (x, y, z) => println!("{} {} {}", x, y, z), +LL | | } + | |_____^ + | +help: consider using `let` statement + | +LL | let (x, y, z) = (a, b, c); +LL | println!("{} {} {}", x, y, z); + | + +error: this match could be replaced by its body itself + --> $DIR/match_single_binding.rs:37:5 + | +LL | / match a { +LL | | _ => println!("whatever"), +LL | | } + | |_____^ help: consider using the match body instead: `println!("whatever");` + +error: this match could be replaced by its body itself + --> $DIR/match_single_binding.rs:41:5 + | +LL | / match a { +LL | | _ => { +LL | | let x = 29; +LL | | println!("x has a value of {}", x); +LL | | }, +LL | | } + | |_____^ + | +help: consider using the match body instead + | +LL | { +LL | let x = 29; +LL | println!("x has a value of {}", x); +LL | } + | + +error: this match could be replaced by its body itself + --> $DIR/match_single_binding.rs:48:5 + | +LL | / match a { +LL | | _ => { +LL | | let e = 5 * a; +LL | | if e >= 5 { +... | +LL | | }, +LL | | } + | |_____^ + | +help: consider using the match body instead + | +LL | { +LL | let e = 5 * a; +LL | if e >= 5 { +LL | println!("e is superior to 5"); +LL | } +LL | } + | + +error: this match could be written as a `let` statement + --> $DIR/match_single_binding.rs:58:5 + | +LL | / match p { +LL | | Point { x, y } => println!("Coords: ({}, {})", x, y), +LL | | } + | |_____^ + | +help: consider using `let` statement + | +LL | let Point { x, y } = p; +LL | println!("Coords: ({}, {})", x, y); + | + +error: this match could be written as a `let` statement + --> $DIR/match_single_binding.rs:62:5 + | +LL | / match p { +LL | | Point { x: x1, y: y1 } => println!("Coords: ({}, {})", x1, y1), +LL | | } + | |_____^ + | +help: consider using `let` statement + | +LL | let Point { x: x1, y: y1 } = p; +LL | println!("Coords: ({}, {})", x1, y1); + | + +error: this match could be written as a `let` statement + --> $DIR/match_single_binding.rs:67:5 + | +LL | / match x { +LL | | ref r => println!("Got a reference to {}", r), +LL | | } + | |_____^ + | +help: consider using `let` statement + | +LL | let ref r = x; +LL | println!("Got a reference to {}", r); + | + +error: this match could be written as a `let` statement + --> $DIR/match_single_binding.rs:72:5 + | +LL | / match x { +LL | | ref mut mr => println!("Got a mutable reference to {}", mr), +LL | | } + | |_____^ + | +help: consider using `let` statement + | +LL | let ref mut mr = x; +LL | println!("Got a mutable reference to {}", mr); + | + +error: aborting due to 9 previous errors