From 3445d41f07aba83d83b2093381512a24b9fa974c Mon Sep 17 00:00:00 2001 From: ThibsG Date: Wed, 15 Jan 2020 07:56:56 +0100 Subject: [PATCH 1/5] Add new lint: match with a single binding statement - Lint name: MATCH_SINGLE_BINDING --- clippy_lints/src/lib.rs | 3 ++ clippy_lints/src/matches.rs | 60 ++++++++++++++++++++++++++-- tests/ui/match_single_binding.rs | 25 ++++++++++++ tests/ui/match_single_binding.stderr | 14 +++++++ 4 files changed, 98 insertions(+), 4 deletions(-) create mode 100644 tests/ui/match_single_binding.rs create mode 100644 tests/ui/match_single_binding.stderr diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index fcf01ae48..2f4e6a0fd 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -605,6 +605,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: &matches::MATCH_OVERLAPPING_ARM, &matches::MATCH_REF_PATS, &matches::MATCH_WILD_ERR_ARM, + &matches::MATCH_SINGLE_BINDING, &matches::SINGLE_MATCH, &matches::SINGLE_MATCH_ELSE, &matches::WILDCARD_ENUM_MATCH_ARM, @@ -1206,6 +1207,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(&matches::MATCH_OVERLAPPING_ARM), LintId::of(&matches::MATCH_REF_PATS), LintId::of(&matches::MATCH_WILD_ERR_ARM), + LintId::of(&matches::MATCH_SINGLE_BINDING), LintId::of(&matches::SINGLE_MATCH), LintId::of(&matches::WILDCARD_IN_OR_PATTERNS), LintId::of(&mem_discriminant::MEM_DISCRIMINANT_NON_ENUM), @@ -1483,6 +1485,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(&map_unit_fn::OPTION_MAP_UNIT_FN), LintId::of(&map_unit_fn::RESULT_MAP_UNIT_FN), LintId::of(&matches::MATCH_AS_REF), + LintId::of(&matches::MATCH_SINGLE_BINDING), LintId::of(&matches::WILDCARD_IN_OR_PATTERNS), LintId::of(&methods::CHARS_NEXT_CMP), LintId::of(&methods::CLONE_ON_COPY), diff --git a/clippy_lints/src/matches.rs b/clippy_lints/src/matches.rs index 0fc7799c9..b5cf12b09 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::{ - expr_block, is_allowed, is_expn_of, is_wild, match_qpath, match_type, multispan_sugg, remove_blocks, snippet, - snippet_with_applicability, span_lint_and_help, span_lint_and_note, span_lint_and_sugg, span_lint_and_then, - walk_ptrs_ty, + 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_with_applicability, span_lint_and_sugg, span_lint_and_then, }; use if_chain::if_chain; use rustc::lint::in_external_macro; @@ -245,6 +245,33 @@ declare_clippy_lint! { "a wildcard pattern used with others patterns in same match arm" } +declare_clippy_lint! { + /// **What it does:** Checks for useless match that binds to only one value. + /// + /// **Why is this bad?** Readability and needless complexity. + /// + /// **Known problems:** This situation frequently happen in macros, so can't lint there. + /// + /// **Example:** + /// ```rust + /// # let a = 1; + /// # let b = 2; + /// + /// // Bad + /// match (a, b) { + /// (c, d) => { + /// // useless match + /// } + /// } + /// + /// // Good + /// let (c, d) = (a, b); + /// ``` + pub MATCH_SINGLE_BINDING, + complexity, + "a match with a single binding instead of using `let` statement" +} + declare_lint_pass!(Matches => [ SINGLE_MATCH, MATCH_REF_PATS, @@ -254,7 +281,8 @@ declare_lint_pass!(Matches => [ MATCH_WILD_ERR_ARM, MATCH_AS_REF, WILDCARD_ENUM_MATCH_ARM, - WILDCARD_IN_OR_PATTERNS + WILDCARD_IN_OR_PATTERNS, + MATCH_SINGLE_BINDING, ]); impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Matches { @@ -270,6 +298,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Matches { check_wild_enum_match(cx, ex, arms); check_match_as_ref(cx, ex, arms, expr); check_wild_in_or_pats(cx, arms); + check_match_single_binding(cx, ex, arms, expr); } if let ExprKind::Match(ref ex, ref arms, _) = expr.kind { check_match_ref_pats(cx, ex, arms, expr); @@ -712,6 +741,29 @@ 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) { + return; + } + if arms.len() == 1 { + let bind_names = arms[0].pat.span; + let matched_vars = ex.span; + span_lint_and_sugg( + cx, + MATCH_SINGLE_BINDING, + expr.span, + "this match could be written as a `let` statement", + "try this", + format!( + "let {} = {};", + snippet(cx, bind_names, ".."), + snippet(cx, matched_vars, "..") + ), + Applicability::HasPlaceholders, + ); + } +} + /// Gets all arms that are unbounded `PatRange`s. fn all_ranges<'a, 'tcx>( cx: &LateContext<'a, 'tcx>, diff --git a/tests/ui/match_single_binding.rs b/tests/ui/match_single_binding.rs new file mode 100644 index 000000000..816a7e719 --- /dev/null +++ b/tests/ui/match_single_binding.rs @@ -0,0 +1,25 @@ +#![warn(clippy::match_single_binding)] +#[allow(clippy::many_single_char_names)] + +fn main() { + let a = 1; + let b = 2; + let c = 3; + // Lint + match (a, b, c) { + (x, y, z) => { + println!("{} {} {}", x, y, z); + }, + } + // Ok + match a { + 2 => println!("2"), + _ => println!("Not 2"), + } + // Ok + let d = Some(5); + match d { + Some(d) => println!("5"), + _ => println!("None"), + } +} diff --git a/tests/ui/match_single_binding.stderr b/tests/ui/match_single_binding.stderr new file mode 100644 index 000000000..2fad6aab3 --- /dev/null +++ b/tests/ui/match_single_binding.stderr @@ -0,0 +1,14 @@ +error: this match could be written as a `let` statement + --> $DIR/match_single_binding.rs:9:5 + | +LL | / match (a, b, c) { +LL | | (x, y, z) => { +LL | | println!("{} {} {}", x, y, z); +LL | | }, +LL | | } + | |_____^ help: try this: `let (x, y, z) = (a, b, c);` + | + = note: `-D clippy::match-single-binding` implied by `-D warnings` + +error: aborting due to previous error + From 6afd7ea147cf34eb2ce505d513664b5f4fadfb58 Mon Sep 17 00:00:00 2001 From: ThibsG Date: Sun, 19 Jan 2020 11:07:13 +0100 Subject: [PATCH 2/5] Use span_lint_and_sugg + move infaillible lint - moving infaillible lint to prevent collisions --- CHANGELOG.md | 1 + .../src/infallible_destructuring_match.rs | 77 ----------- clippy_lints/src/lib.rs | 14 +- clippy_lints/src/matches.rs | 130 +++++++++++++++--- src/lintlist/mod.rs | 9 +- tests/ui/escape_analysis.rs | 2 +- tests/ui/escape_analysis.stderr | 4 +- tests/ui/match_single_binding.fixed | 26 ++++ tests/ui/match_single_binding.rs | 4 +- tests/ui/match_single_binding.stderr | 11 +- 10 files changed, 166 insertions(+), 112 deletions(-) delete mode 100644 clippy_lints/src/infallible_destructuring_match.rs create mode 100644 tests/ui/match_single_binding.fixed diff --git a/CHANGELOG.md b/CHANGELOG.md index eb3b518cd..49de9df64 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1217,6 +1217,7 @@ Released 2018-09-13 [`match_overlapping_arm`]: https://rust-lang.github.io/rust-clippy/master/index.html#match_overlapping_arm [`match_ref_pats`]: https://rust-lang.github.io/rust-clippy/master/index.html#match_ref_pats [`match_same_arms`]: https://rust-lang.github.io/rust-clippy/master/index.html#match_same_arms +[`match_single_binding`]: https://rust-lang.github.io/rust-clippy/master/index.html#match_single_binding [`match_wild_err_arm`]: https://rust-lang.github.io/rust-clippy/master/index.html#match_wild_err_arm [`maybe_infinite_iter`]: https://rust-lang.github.io/rust-clippy/master/index.html#maybe_infinite_iter [`mem_discriminant_non_enum`]: https://rust-lang.github.io/rust-clippy/master/index.html#mem_discriminant_non_enum diff --git a/clippy_lints/src/infallible_destructuring_match.rs b/clippy_lints/src/infallible_destructuring_match.rs deleted file mode 100644 index 1d23dd115..000000000 --- a/clippy_lints/src/infallible_destructuring_match.rs +++ /dev/null @@ -1,77 +0,0 @@ -use super::utils::{get_arg_name, match_var, remove_blocks, snippet_with_applicability, span_lint_and_sugg}; -use if_chain::if_chain; -use rustc_errors::Applicability; -use rustc_hir::*; -use rustc_lint::{LateContext, LateLintPass}; -use rustc_session::{declare_lint_pass, declare_tool_lint}; - -declare_clippy_lint! { - /// **What it does:** Checks for matches being used to destructure a single-variant enum - /// or tuple struct where a `let` will suffice. - /// - /// **Why is this bad?** Just readability – `let` doesn't nest, whereas a `match` does. - /// - /// **Known problems:** None. - /// - /// **Example:** - /// ```rust - /// enum Wrapper { - /// Data(i32), - /// } - /// - /// let wrapper = Wrapper::Data(42); - /// - /// let data = match wrapper { - /// Wrapper::Data(i) => i, - /// }; - /// ``` - /// - /// The correct use would be: - /// ```rust - /// enum Wrapper { - /// Data(i32), - /// } - /// - /// let wrapper = Wrapper::Data(42); - /// let Wrapper::Data(data) = wrapper; - /// ``` - pub INFALLIBLE_DESTRUCTURING_MATCH, - style, - "a `match` statement with a single infallible arm instead of a `let`" -} - -declare_lint_pass!(InfallibleDestructingMatch => [INFALLIBLE_DESTRUCTURING_MATCH]); - -impl<'a, 'tcx> LateLintPass<'a, 'tcx> for InfallibleDestructingMatch { - fn check_local(&mut self, cx: &LateContext<'a, 'tcx>, local: &'tcx Local<'_>) { - if_chain! { - if let Some(ref expr) = local.init; - if let ExprKind::Match(ref target, ref arms, MatchSource::Normal) = expr.kind; - if arms.len() == 1 && arms[0].guard.is_none(); - if let PatKind::TupleStruct(QPath::Resolved(None, ref variant_name), ref args, _) = arms[0].pat.kind; - if args.len() == 1; - if let Some(arg) = get_arg_name(&args[0]); - let body = remove_blocks(&arms[0].body); - if match_var(body, arg); - - then { - let mut applicability = Applicability::MachineApplicable; - span_lint_and_sugg( - cx, - INFALLIBLE_DESTRUCTURING_MATCH, - local.span, - "you seem to be trying to use `match` to destructure a single infallible pattern. \ - Consider using `let`", - "try this", - format!( - "let {}({}) = {};", - snippet_with_applicability(cx, variant_name.span, "..", &mut applicability), - snippet_with_applicability(cx, local.pat.span, "..", &mut applicability), - snippet_with_applicability(cx, target.span, "..", &mut applicability), - ), - applicability, - ); - } - } - } -} diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 2f4e6a0fd..45342cc7e 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -218,7 +218,6 @@ pub mod if_let_some_result; pub mod if_not_else; pub mod implicit_return; pub mod indexing_slicing; -pub mod infallible_destructuring_match; pub mod infinite_iter; pub mod inherent_impl; pub mod inherent_to_string; @@ -555,7 +554,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: &implicit_return::IMPLICIT_RETURN, &indexing_slicing::INDEXING_SLICING, &indexing_slicing::OUT_OF_BOUNDS_INDEXING, - &infallible_destructuring_match::INFALLIBLE_DESTRUCTURING_MATCH, &infinite_iter::INFINITE_ITER, &infinite_iter::MAYBE_INFINITE_ITER, &inherent_impl::MULTIPLE_INHERENT_IMPL, @@ -600,12 +598,13 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: &map_clone::MAP_CLONE, &map_unit_fn::OPTION_MAP_UNIT_FN, &map_unit_fn::RESULT_MAP_UNIT_FN, + &matches::INFALLIBLE_DESTRUCTURING_MATCH, &matches::MATCH_AS_REF, &matches::MATCH_BOOL, &matches::MATCH_OVERLAPPING_ARM, &matches::MATCH_REF_PATS, - &matches::MATCH_WILD_ERR_ARM, &matches::MATCH_SINGLE_BINDING, + &matches::MATCH_WILD_ERR_ARM, &matches::SINGLE_MATCH, &matches::SINGLE_MATCH_ELSE, &matches::WILDCARD_ENUM_MATCH_ARM, @@ -865,7 +864,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: store.register_late_pass(|| box types::Casts); let type_complexity_threshold = conf.type_complexity_threshold; store.register_late_pass(move || box types::TypeComplexity::new(type_complexity_threshold)); - store.register_late_pass(|| box matches::Matches); + store.register_late_pass(|| box matches::Matches::default()); store.register_late_pass(|| box minmax::MinMaxPass); store.register_late_pass(|| box open_options::OpenOptions); store.register_late_pass(|| box zero_div_zero::ZeroDiv); @@ -942,7 +941,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: store.register_late_pass(|| box question_mark::QuestionMark); store.register_late_pass(|| box suspicious_trait_impl::SuspiciousImpl); store.register_late_pass(|| box map_unit_fn::MapUnit); - store.register_late_pass(|| box infallible_destructuring_match::InfallibleDestructingMatch); store.register_late_pass(|| box inherent_impl::MultipleInherentImpl::default()); store.register_late_pass(|| box neg_cmp_op_on_partial_ord::NoNegCompOpForPartialOrd); store.register_late_pass(|| box unwrap::Unwrap); @@ -1167,7 +1165,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(&identity_op::IDENTITY_OP), LintId::of(&if_let_some_result::IF_LET_SOME_RESULT), LintId::of(&indexing_slicing::OUT_OF_BOUNDS_INDEXING), - LintId::of(&infallible_destructuring_match::INFALLIBLE_DESTRUCTURING_MATCH), LintId::of(&infinite_iter::INFINITE_ITER), LintId::of(&inherent_to_string::INHERENT_TO_STRING), LintId::of(&inherent_to_string::INHERENT_TO_STRING_SHADOW_DISPLAY), @@ -1202,12 +1199,13 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(&map_clone::MAP_CLONE), LintId::of(&map_unit_fn::OPTION_MAP_UNIT_FN), LintId::of(&map_unit_fn::RESULT_MAP_UNIT_FN), + LintId::of(&matches::INFALLIBLE_DESTRUCTURING_MATCH), LintId::of(&matches::MATCH_AS_REF), LintId::of(&matches::MATCH_BOOL), LintId::of(&matches::MATCH_OVERLAPPING_ARM), LintId::of(&matches::MATCH_REF_PATS), - LintId::of(&matches::MATCH_WILD_ERR_ARM), LintId::of(&matches::MATCH_SINGLE_BINDING), + LintId::of(&matches::MATCH_WILD_ERR_ARM), LintId::of(&matches::SINGLE_MATCH), LintId::of(&matches::WILDCARD_IN_OR_PATTERNS), LintId::of(&mem_discriminant::MEM_DISCRIMINANT_NON_ENUM), @@ -1384,7 +1382,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(&functions::DOUBLE_MUST_USE), LintId::of(&functions::MUST_USE_UNIT), LintId::of(&if_let_some_result::IF_LET_SOME_RESULT), - LintId::of(&infallible_destructuring_match::INFALLIBLE_DESTRUCTURING_MATCH), LintId::of(&inherent_to_string::INHERENT_TO_STRING), LintId::of(&len_zero::LEN_WITHOUT_IS_EMPTY), LintId::of(&len_zero::LEN_ZERO), @@ -1397,6 +1394,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(&loops::WHILE_LET_ON_ITERATOR), LintId::of(&main_recursion::MAIN_RECURSION), LintId::of(&map_clone::MAP_CLONE), + LintId::of(&matches::INFALLIBLE_DESTRUCTURING_MATCH), LintId::of(&matches::MATCH_BOOL), LintId::of(&matches::MATCH_OVERLAPPING_ARM), LintId::of(&matches::MATCH_REF_PATS), diff --git a/clippy_lints/src/matches.rs b/clippy_lints/src/matches.rs index b5cf12b09..e4335abce 100644 --- a/clippy_lints/src/matches.rs +++ b/clippy_lints/src/matches.rs @@ -5,7 +5,7 @@ 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_with_applicability, span_lint_and_sugg, span_lint_and_then, + snippet, snippet_block, snippet_with_applicability, span_lint_and_sugg, span_lint_and_then, }; use if_chain::if_chain; use rustc::lint::in_external_macro; @@ -14,7 +14,7 @@ use rustc_errors::Applicability; use rustc_hir::def::CtorKind; use rustc_hir::*; use rustc_lint::{LateContext, LateLintPass, LintContext}; -use rustc_session::{declare_lint_pass, declare_tool_lint}; +use rustc_session::{declare_tool_lint, impl_lint_pass}; use rustc_span::source_map::Span; use std::cmp::Ordering; use std::collections::Bound; @@ -245,12 +245,47 @@ declare_clippy_lint! { "a wildcard pattern used with others patterns in same match arm" } +declare_clippy_lint! { + /// **What it does:** Checks for matches being used to destructure a single-variant enum + /// or tuple struct where a `let` will suffice. + /// + /// **Why is this bad?** Just readability – `let` doesn't nest, whereas a `match` does. + /// + /// **Known problems:** None. + /// + /// **Example:** + /// ```rust + /// enum Wrapper { + /// Data(i32), + /// } + /// + /// let wrapper = Wrapper::Data(42); + /// + /// let data = match wrapper { + /// Wrapper::Data(i) => i, + /// }; + /// ``` + /// + /// The correct use would be: + /// ```rust + /// enum Wrapper { + /// Data(i32), + /// } + /// + /// let wrapper = Wrapper::Data(42); + /// let Wrapper::Data(data) = wrapper; + /// ``` + pub INFALLIBLE_DESTRUCTURING_MATCH, + style, + "a `match` statement with a single infallible arm instead of a `let`" +} + declare_clippy_lint! { /// **What it does:** Checks for useless match that binds to only one value. /// /// **Why is this bad?** Readability and needless complexity. /// - /// **Known problems:** This situation frequently happen in macros, so can't lint there. + /// **Known problems:** None. /// /// **Example:** /// ```rust @@ -272,7 +307,12 @@ declare_clippy_lint! { "a match with a single binding instead of using `let` statement" } -declare_lint_pass!(Matches => [ +#[derive(Default)] +pub struct Matches { + infallible_destructuring_match_linted: bool, +} + +impl_lint_pass!(Matches => [ SINGLE_MATCH, MATCH_REF_PATS, MATCH_BOOL, @@ -283,6 +323,7 @@ declare_lint_pass!(Matches => [ WILDCARD_ENUM_MATCH_ARM, WILDCARD_IN_OR_PATTERNS, MATCH_SINGLE_BINDING, + INFALLIBLE_DESTRUCTURING_MATCH ]); impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Matches { @@ -298,12 +339,51 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Matches { check_wild_enum_match(cx, ex, arms); check_match_as_ref(cx, ex, arms, expr); check_wild_in_or_pats(cx, arms); - check_match_single_binding(cx, ex, arms, expr); + + if self.infallible_destructuring_match_linted { + self.infallible_destructuring_match_linted = false; + } else { + check_match_single_binding(cx, ex, arms, expr); + } } if let ExprKind::Match(ref ex, ref arms, _) = expr.kind { check_match_ref_pats(cx, ex, arms, expr); } } + + fn check_local(&mut self, cx: &LateContext<'a, 'tcx>, local: &'tcx Local<'_>) { + if_chain! { + if let Some(ref expr) = local.init; + if let ExprKind::Match(ref target, ref arms, MatchSource::Normal) = expr.kind; + if arms.len() == 1 && arms[0].guard.is_none(); + if let PatKind::TupleStruct( + QPath::Resolved(None, ref variant_name), ref args, _) = arms[0].pat.kind; + if args.len() == 1; + if let Some(arg) = get_arg_name(&args[0]); + let body = remove_blocks(&arms[0].body); + if match_var(body, arg); + + then { + let mut applicability = Applicability::MachineApplicable; + self.infallible_destructuring_match_linted = true; + span_lint_and_sugg( + cx, + INFALLIBLE_DESTRUCTURING_MATCH, + local.span, + "you seem to be trying to use `match` to destructure a single infallible pattern. \ + Consider using `let`", + "try this", + format!( + "let {}({}) = {};", + snippet_with_applicability(cx, variant_name.span, "..", &mut applicability), + snippet_with_applicability(cx, local.pat.span, "..", &mut applicability), + snippet_with_applicability(cx, target.span, "..", &mut applicability), + ), + applicability, + ); + } + } + } } #[rustfmt::skip] @@ -746,21 +826,31 @@ fn check_match_single_binding(cx: &LateContext<'_, '_>, ex: &Expr<'_>, arms: &[A return; } if arms.len() == 1 { - let bind_names = arms[0].pat.span; - let matched_vars = ex.span; - span_lint_and_sugg( - cx, - MATCH_SINGLE_BINDING, - expr.span, - "this match could be written as a `let` statement", - "try this", - format!( - "let {} = {};", - snippet(cx, bind_names, ".."), - snippet(cx, matched_vars, "..") - ), - Applicability::HasPlaceholders, - ); + 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, + ); + }, + _ => (), + } } } diff --git a/src/lintlist/mod.rs b/src/lintlist/mod.rs index eaada9961..b28a0917a 100644 --- a/src/lintlist/mod.rs +++ b/src/lintlist/mod.rs @@ -775,7 +775,7 @@ pub const ALL_LINTS: [Lint; 351] = [ group: "style", desc: "a `match` statement with a single infallible arm instead of a `let`", deprecation: None, - module: "infallible_destructuring_match", + module: "matches", }, Lint { name: "infinite_iter", @@ -1092,6 +1092,13 @@ pub const ALL_LINTS: [Lint; 351] = [ deprecation: None, module: "copies", }, + Lint { + name: "match_single_binding", + group: "complexity", + desc: "a match with a single binding instead of using `let` statement", + deprecation: None, + module: "matches", + }, Lint { name: "match_wild_err_arm", group: "style", diff --git a/tests/ui/escape_analysis.rs b/tests/ui/escape_analysis.rs index d435484d3..7d4b71d09 100644 --- a/tests/ui/escape_analysis.rs +++ b/tests/ui/escape_analysis.rs @@ -3,7 +3,7 @@ clippy::borrowed_box, clippy::needless_pass_by_value, clippy::unused_unit, - clippy::redundant_clone + clippy::redundant_clone, )] #![warn(clippy::boxed_local)] diff --git a/tests/ui/escape_analysis.stderr b/tests/ui/escape_analysis.stderr index 19342fe1b..c86a769a3 100644 --- a/tests/ui/escape_analysis.stderr +++ b/tests/ui/escape_analysis.stderr @@ -1,5 +1,5 @@ error: local variable doesn't need to be boxed here - --> $DIR/escape_analysis.rs:39:13 + --> $DIR/escape_analysis.rs:40:13 | LL | fn warn_arg(x: Box) { | ^ @@ -7,7 +7,7 @@ LL | fn warn_arg(x: Box) { = note: `-D clippy::boxed-local` implied by `-D warnings` error: local variable doesn't need to be boxed here - --> $DIR/escape_analysis.rs:130:12 + --> $DIR/escape_analysis.rs:131:12 | LL | pub fn new(_needs_name: Box>) -> () {} | ^^^^^^^^^^^ diff --git a/tests/ui/match_single_binding.fixed b/tests/ui/match_single_binding.fixed new file mode 100644 index 000000000..41faa1e1c --- /dev/null +++ b/tests/ui/match_single_binding.fixed @@ -0,0 +1,26 @@ +// run-rustfix + +#![warn(clippy::match_single_binding)] +#[allow(clippy::many_single_char_names)] + +fn main() { + let a = 1; + let b = 2; + let c = 3; + // Lint + let (x, y, z) = (a, b, c); +{ + println!("{} {} {}", x, y, z); +} + // Ok + match a { + 2 => println!("2"), + _ => println!("Not 2"), + } + // Ok + let d = Some(5); + match d { + Some(d) => println!("{}", d), + _ => println!("None"), + } +} diff --git a/tests/ui/match_single_binding.rs b/tests/ui/match_single_binding.rs index 816a7e719..06b924d04 100644 --- a/tests/ui/match_single_binding.rs +++ b/tests/ui/match_single_binding.rs @@ -1,3 +1,5 @@ +// run-rustfix + #![warn(clippy::match_single_binding)] #[allow(clippy::many_single_char_names)] @@ -19,7 +21,7 @@ fn main() { // Ok let d = Some(5); match d { - Some(d) => println!("5"), + Some(d) => println!("{}", d), _ => println!("None"), } } diff --git a/tests/ui/match_single_binding.stderr b/tests/ui/match_single_binding.stderr index 2fad6aab3..64216a72e 100644 --- a/tests/ui/match_single_binding.stderr +++ b/tests/ui/match_single_binding.stderr @@ -1,14 +1,21 @@ error: this match could be written as a `let` statement - --> $DIR/match_single_binding.rs:9:5 + --> $DIR/match_single_binding.rs:11:5 | LL | / match (a, b, c) { LL | | (x, y, z) => { LL | | println!("{} {} {}", x, y, z); LL | | }, LL | | } - | |_____^ help: try this: `let (x, y, z) = (a, b, c);` + | |_____^ | = note: `-D clippy::match-single-binding` implied by `-D warnings` +help: consider using `let` statement + | +LL | let (x, y, z) = (a, b, c); +LL | { +LL | println!("{} {} {}", x, y, z); +LL | } + | error: aborting due to previous error From b29aacfec8b45aad12288b011a23fc2dea38d0fc Mon Sep 17 00:00:00 2001 From: ThibsG Date: Sun, 26 Jan 2020 17:03:39 +0100 Subject: [PATCH 3/5] Add wild and struct handling --- clippy_lints/src/matches.rs | 88 ++++++++++++------- tests/ui/escape_analysis.rs | 1 + tests/ui/match_ref_pats.rs | 1 + tests/ui/match_ref_pats.stderr | 6 +- tests/ui/match_single_binding.fixed | 39 ++++++++- tests/ui/match_single_binding.rs | 50 ++++++++++- tests/ui/match_single_binding.stderr | 123 ++++++++++++++++++++++++++- 7 files changed, 272 insertions(+), 36 deletions(-) diff --git a/clippy_lints/src/matches.rs b/clippy_lints/src/matches.rs index e4335abce..2323df1f0 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 7d4b71d09..c0a52d832 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 d26b59db9..5de43733a 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 492f4b9ba..52cb4a14b 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 41faa1e1c..8fb8dc323 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 06b924d04..55b0b09a0 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 64216a72e..d76e229ad 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 From 53094de08efea5f4f4ff2d5e8e7381bf8aede625 Mon Sep 17 00:00:00 2001 From: ThibsG Date: Tue, 4 Feb 2020 08:20:49 +0100 Subject: [PATCH 4/5] Merge fixes --- README.md | 2 +- src/lintlist/mod.rs | 2 +- tests/ui/println_empty_string.fixed | 1 + tests/ui/println_empty_string.rs | 1 + tests/ui/println_empty_string.stderr | 4 ++-- 5 files changed, 6 insertions(+), 4 deletions(-) diff --git a/README.md b/README.md index b68eb3ed7..da02591f6 100644 --- a/README.md +++ b/README.md @@ -6,7 +6,7 @@ A collection of lints to catch common mistakes and improve your [Rust](https://github.com/rust-lang/rust) code. -[There are 351 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html) +[There are 352 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html) We have a bunch of lint categories to allow you to choose how much Clippy is supposed to ~~annoy~~ help you: diff --git a/src/lintlist/mod.rs b/src/lintlist/mod.rs index b28a0917a..7d2aedd66 100644 --- a/src/lintlist/mod.rs +++ b/src/lintlist/mod.rs @@ -6,7 +6,7 @@ pub use lint::Lint; pub use lint::LINT_LEVELS; // begin lint list, do not remove this comment, it’s used in `update_lints` -pub const ALL_LINTS: [Lint; 351] = [ +pub const ALL_LINTS: [Lint; 352] = [ Lint { name: "absurd_extreme_comparisons", group: "correctness", diff --git a/tests/ui/println_empty_string.fixed b/tests/ui/println_empty_string.fixed index 4e84511d7..2b889b62e 100644 --- a/tests/ui/println_empty_string.fixed +++ b/tests/ui/println_empty_string.fixed @@ -1,4 +1,5 @@ // run-rustfix +#![allow(clippy::match_single_binding)] fn main() { println!(); diff --git a/tests/ui/println_empty_string.rs b/tests/ui/println_empty_string.rs index 9fdfb03a3..890f5f684 100644 --- a/tests/ui/println_empty_string.rs +++ b/tests/ui/println_empty_string.rs @@ -1,4 +1,5 @@ // run-rustfix +#![allow(clippy::match_single_binding)] fn main() { println!(); diff --git a/tests/ui/println_empty_string.stderr b/tests/ui/println_empty_string.stderr index 689624a0f..23112b881 100644 --- a/tests/ui/println_empty_string.stderr +++ b/tests/ui/println_empty_string.stderr @@ -1,5 +1,5 @@ error: using `println!("")` - --> $DIR/println_empty_string.rs:5:5 + --> $DIR/println_empty_string.rs:6:5 | LL | println!(""); | ^^^^^^^^^^^^ help: replace it with: `println!()` @@ -7,7 +7,7 @@ LL | println!(""); = note: `-D clippy::println-empty-string` implied by `-D warnings` error: using `println!("")` - --> $DIR/println_empty_string.rs:8:14 + --> $DIR/println_empty_string.rs:9:14 | LL | _ => println!(""), | ^^^^^^^^^^^^ help: replace it with: `println!()` From 00904cb100f9f9d3291f98a3434f7aba9301b8de Mon Sep 17 00:00:00 2001 From: ThibsG Date: Tue, 4 Feb 2020 20:19:20 +0100 Subject: [PATCH 5/5] Manage macros case + move to MaybeIncorrect when binding values --- clippy_lints/src/matches.rs | 36 ++++++++++++++++++------------------ 1 file changed, 18 insertions(+), 18 deletions(-) diff --git a/clippy_lints/src/matches.rs b/clippy_lints/src/matches.rs index 2323df1f0..1d8c5ec90 100644 --- a/clippy_lints/src/matches.rs +++ b/clippy_lints/src/matches.rs @@ -285,7 +285,8 @@ declare_clippy_lint! { /// /// **Why is this bad?** Readability and needless complexity. /// - /// **Known problems:** None. + /// **Known problems:** Suggested replacements may be incorrect when `match` + /// is actually binding temporary value, bringing a 'dropped while borrowed' error. /// /// **Example:** /// ```rust @@ -835,23 +836,22 @@ fn check_match_single_binding(cx: &LateContext<'_, '_>, ex: &Expr<'_>, arms: &[A }; // 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 match_body.kind { + ExprKind::Block(block, _) => { + // macro + expr_ty(body) == () + if block.span.from_expansion() && cx.tables.expr_ty(&match_body).is_unit() { + snippet_body.push(';'); } - } + }, + _ => { + // expr_ty(body) == () + if cx.tables.expr_ty(&match_body).is_unit() { + snippet_body.push(';'); + } + }, } + let mut applicability = Applicability::MaybeIncorrect; match arms[0].pat.kind { PatKind::Binding(..) | PatKind::Tuple(_, _) | PatKind::Struct(..) => { span_lint_and_sugg( @@ -862,11 +862,11 @@ fn check_match_single_binding(cx: &LateContext<'_, '_>, ex: &Expr<'_>, arms: &[A "consider using `let` statement", format!( "let {} = {};\n{}", - snippet(cx, bind_names, ".."), - snippet(cx, matched_vars, ".."), + snippet_with_applicability(cx, bind_names, "..", &mut applicability), + snippet_with_applicability(cx, matched_vars, "..", &mut applicability), snippet_body ), - Applicability::MachineApplicable, + applicability, ); }, PatKind::Wild => {