X-Git-Url: https://git.lizzy.rs/?a=blobdiff_plain;f=clippy_lints%2Fsrc%2Fmatches.rs;h=6d7af45a47224d54a99e6949c50185e792acf9bc;hb=7654125d27d95d5c329e554115b510efc1ab1e60;hp=206a842d21ca9f82a29c45da71dda075ba5d0b9b;hpb=a524be6df505598dabb27902eb9d9fc31b61cab0;p=rust.git diff --git a/clippy_lints/src/matches.rs b/clippy_lints/src/matches.rs index 206a842d21c..6d7af45a472 100644 --- a/clippy_lints/src/matches.rs +++ b/clippy_lints/src/matches.rs @@ -36,10 +36,17 @@ /// ```rust /// # fn bar(stool: &str) {} /// # let x = Some("abc"); + /// + /// // Bad /// match x { /// Some(ref foo) => bar(foo), /// _ => (), /// } + /// + /// // Good + /// if let Some(ref foo) = x { + /// bar(foo); + /// } /// ``` pub SINGLE_MATCH, style, @@ -97,11 +104,19 @@ /// /// **Example:** /// ```rust,ignore + /// // Bad /// match x { /// &A(ref y) => foo(y), /// &B => bar(), /// _ => frob(&x), /// } + /// + /// // Good + /// match *x { + /// A(ref y) => foo(y), + /// B => bar(), + /// _ => frob(x), + /// } /// ``` pub MATCH_REF_PATS, style, @@ -138,7 +153,7 @@ /// } /// ``` pub MATCH_BOOL, - style, + pedantic, "a `match` on a boolean expression instead of an `if..else` block" } @@ -168,7 +183,7 @@ /// **What it does:** Checks for arm which matches all errors with `Err(_)` /// and take drastic actions like `panic!`. /// - /// **Why is this bad?** It is generally a bad practice, just like + /// **Why is this bad?** It is generally a bad practice, similar to /// catching all exceptions in java with `catch(Exception)` /// /// **Known problems:** None. @@ -182,7 +197,7 @@ /// } /// ``` pub MATCH_WILD_ERR_ARM, - style, + pedantic, "a `match` with `Err(_)` arm and take drastic actions" } @@ -197,10 +212,15 @@ /// **Example:** /// ```rust /// let x: Option<()> = None; + /// + /// // Bad /// let r: Option<&()> = match x { /// None => None, /// Some(ref v) => Some(v), /// }; + /// + /// // Good + /// let r: Option<&()> = x.as_ref(); /// ``` pub MATCH_AS_REF, complexity, @@ -219,16 +239,56 @@ /// ```rust /// # enum Foo { A(usize), B(usize) } /// # let x = Foo::B(1); + /// + /// // Bad /// match x { - /// A => {}, + /// Foo::A(_) => {}, /// _ => {}, /// } + /// + /// // Good + /// match x { + /// Foo::A(_) => {}, + /// Foo::B(_) => {}, + /// } /// ``` pub WILDCARD_ENUM_MATCH_ARM, restriction, "a wildcard enum match arm using `_`" } +declare_clippy_lint! { + /// **What it does:** Checks for wildcard enum matches for a single variant. + /// + /// **Why is this bad?** New enum variants added by library updates can be missed. + /// + /// **Known problems:** Suggested replacements may not use correct path to enum + /// if it's not present in the current scope. + /// + /// **Example:** + /// + /// ```rust + /// # enum Foo { A, B, C } + /// # let x = Foo::B; + /// // Bad + /// match x { + /// Foo::A => {}, + /// Foo::B => {}, + /// _ => {}, + /// } + /// + /// // Good + /// match x { + /// Foo::A => {}, + /// Foo::B => {}, + /// Foo::C => {}, + /// } + /// ``` + pub MATCH_WILDCARD_FOR_SINGLE_VARIANTS, + pedantic, + "a wildcard enum match for a single variant" +} + declare_clippy_lint! { /// **What it does:** Checks for wildcard pattern used with others patterns in same match arm. /// @@ -239,10 +299,17 @@ /// /// **Example:** /// ```rust + /// // Bad /// match "foo" { /// "a" => {}, /// "bar" | _ => {}, /// } + /// + /// // Good + /// match "foo" { + /// "a" => {}, + /// _ => {}, + /// } /// ``` pub WILDCARD_IN_OR_PATTERNS, complexity, @@ -356,6 +423,7 @@ pub struct Matches { MATCH_WILD_ERR_ARM, MATCH_AS_REF, WILDCARD_ENUM_MATCH_ARM, + MATCH_WILDCARD_FOR_SINGLE_VARIANTS, WILDCARD_IN_OR_PATTERNS, MATCH_SINGLE_BINDING, INFALLIBLE_DESTRUCTURING_MATCH, @@ -389,6 +457,8 @@ fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr<'_>) { fn check_local(&mut self, cx: &LateContext<'a, 'tcx>, local: &'tcx Local<'_>) { if_chain! { + if !in_external_macro(cx.sess(), local.span); + if !in_macro(local.span); 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(); @@ -423,6 +493,8 @@ fn check_local(&mut self, cx: &LateContext<'a, 'tcx>, local: &'tcx Local<'_>) { fn check_pat(&mut self, cx: &LateContext<'a, 'tcx>, pat: &'tcx Pat<'_>) { if_chain! { + if !in_external_macro(cx.sess(), pat.span); + if !in_macro(pat.span); if let PatKind::Struct(ref qpath, fields, true) = pat.kind; if let QPath::Resolved(_, ref path) = qpath; if let Some(def_id) = path.res.opt_def_id(); @@ -437,6 +509,7 @@ fn check_pat(&mut self, cx: &LateContext<'a, 'tcx>, pat: &'tcx Pat<'_>) { REST_PAT_IN_FULLY_BOUND_STRUCTS, pat.span, "unnecessary use of `..` pattern in struct binding. All fields were already bound", + None, "consider removing `..` from this binding", ); } @@ -565,7 +638,7 @@ fn check_match_bool(cx: &LateContext<'_, '_>, ex: &Expr<'_>, arms: &[Arm<'_>], e MATCH_BOOL, expr.span, "you seem to be trying to match on a boolean expression", - move |db| { + move |diag| { if arms.len() == 2 { // no guards let exprs = if let PatKind::Lit(ref arm_bool) = arms[0].pat.kind { @@ -607,7 +680,7 @@ fn check_match_bool(cx: &LateContext<'_, '_>, ex: &Expr<'_>, arms: &[Arm<'_>], e }; if let Some(sugg) = sugg { - db.span_suggestion( + diag.span_suggestion( expr.span, "consider using an `if`/`else` expression", sugg, @@ -632,7 +705,7 @@ fn check_overlapping_arms<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, ex: &'tcx Expr<' MATCH_OVERLAPPING_ARM, start.span, "some ranges overlap", - end.span, + Some(end.span), "overlaps with this", ); } @@ -670,8 +743,8 @@ fn check_wild_err_arm(cx: &LateContext<'_, '_>, ex: &Expr<'_>, arms: &[Arm<'_>]) MATCH_WILD_ERR_ARM, arm.pat.span, &format!("`Err({})` matches all errors", &ident_bind_name), - arm.pat.span, - "match each error separately or use the error output", + None, + "match each error separately or use the error output, or use `.except(msg)` if the error case is unreachable", ); } } @@ -724,9 +797,21 @@ fn check_wild_enum_match(cx: &LateContext<'_, '_>, ex: &Expr<'_>, arms: &[Arm<'_ if let QPath::Resolved(_, p) = path { missing_variants.retain(|e| e.ctor_def_id != Some(p.res.def_id())); } - } else if let PatKind::TupleStruct(ref path, ..) = arm.pat.kind { + } else if let PatKind::TupleStruct(ref path, ref patterns, ..) = arm.pat.kind { if let QPath::Resolved(_, p) = path { - missing_variants.retain(|e| e.ctor_def_id != Some(p.res.def_id())); + // Some simple checks for exhaustive patterns. + // There is a room for improvements to detect more cases, + // but it can be more expensive to do so. + let is_pattern_exhaustive = |pat: &&Pat<'_>| { + if let PatKind::Wild | PatKind::Binding(.., None) = pat.kind { + true + } else { + false + } + }; + if patterns.iter().all(is_pattern_exhaustive) { + missing_variants.retain(|e| e.ctor_def_id != Some(p.res.def_id())); + } } } } @@ -761,6 +846,19 @@ fn check_wild_enum_match(cx: &LateContext<'_, '_>, ex: &Expr<'_>, arms: &[Arm<'_ } } + if suggestion.len() == 1 { + // No need to check for non-exhaustive enum as in that case len would be greater than 1 + span_lint_and_sugg( + cx, + MATCH_WILDCARD_FOR_SINGLE_VARIANTS, + wildcard_span, + message, + "try this", + suggestion[0].clone(), + Applicability::MaybeIncorrect, + ) + }; + span_lint_and_sugg( cx, WILDCARD_ENUM_MATCH_ARM, @@ -768,7 +866,7 @@ fn check_wild_enum_match(cx: &LateContext<'_, '_>, ex: &Expr<'_>, arms: &[Arm<'_ message, "try this", suggestion.join(" | "), - Applicability::MachineApplicable, + Applicability::MaybeIncorrect, ) } } @@ -813,9 +911,9 @@ fn check_match_ref_pats(cx: &LateContext<'_, '_>, ex: &Expr<'_>, arms: &[Arm<'_> } })); - span_lint_and_then(cx, MATCH_REF_PATS, expr.span, title, |db| { + span_lint_and_then(cx, MATCH_REF_PATS, expr.span, title, |diag| { if !expr.span.from_expansion() { - multispan_sugg(db, msg.to_owned(), suggs); + multispan_sugg(diag, msg, suggs); } }); } @@ -883,6 +981,7 @@ fn check_wild_in_or_pats(cx: &LateContext<'_, '_>, arms: &[Arm<'_>]) { WILDCARD_IN_OR_PATTERNS, arm.pat.span, "wildcard pattern covers any other pattern as it will match anyway.", + None, "Consider handling `_` separately.", ); }