X-Git-Url: https://git.lizzy.rs/?a=blobdiff_plain;f=clippy_lints%2Fsrc%2Fmatches.rs;h=6d7af45a47224d54a99e6949c50185e792acf9bc;hb=7654125d27d95d5c329e554115b510efc1ab1e60;hp=a124bf8b8db99743ff6d3534757abed6274224f6;hpb=563da5248d867e7147b084161bee040a241a7419;p=rust.git diff --git a/clippy_lints/src/matches.rs b/clippy_lints/src/matches.rs index a124bf8b8db..6d7af45a472 100644 --- a/clippy_lints/src/matches.rs +++ b/clippy_lints/src/matches.rs @@ -3,10 +3,10 @@ use crate::utils::sugg::Sugg; use crate::utils::usage::is_unused; use crate::utils::{ - expr_block, get_arg_name, get_parent_expr, in_macro, indent_of, 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, + expr_block, get_arg_name, get_parent_expr, in_macro, indent_of, is_allowed, is_expn_of, is_refutable, + is_type_diagnostic_item, 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_ast::ast::LitKind; @@ -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", ); } @@ -447,6 +520,11 @@ fn check_pat(&mut self, cx: &LateContext<'a, 'tcx>, pat: &'tcx Pat<'_>) { #[rustfmt::skip] fn check_single_match(cx: &LateContext<'_, '_>, ex: &Expr<'_>, arms: &[Arm<'_>], expr: &Expr<'_>) { if arms.len() == 2 && arms[0].guard.is_none() && arms[1].guard.is_none() { + if in_macro(expr.span) { + // Don't lint match expressions present in + // macro_rules! block + return; + } if let PatKind::Or(..) = arms[0].pat.kind { // don't lint for or patterns for now, this makes // the lint noisy in unnecessary situations @@ -560,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 { @@ -602,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, @@ -627,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", ); } @@ -637,7 +715,7 @@ fn check_overlapping_arms<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, ex: &'tcx Expr<' fn check_wild_err_arm(cx: &LateContext<'_, '_>, ex: &Expr<'_>, arms: &[Arm<'_>]) { let ex_ty = walk_ptrs_ty(cx.tables.expr_ty(ex)); - if match_type(cx, ex_ty, &paths::RESULT) { + if is_type_diagnostic_item(cx, ex_ty, sym!(result_type)) { for arm in arms { if let PatKind::TupleStruct(ref path, ref inner, _) = arm.pat.kind { let path_str = rustc_hir_pretty::to_string(rustc_hir_pretty::NO_ANN, |s| s.print_qpath(path, false)); @@ -665,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", ); } } @@ -719,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())); + } } } } @@ -756,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, @@ -763,7 +866,7 @@ fn check_wild_enum_match(cx: &LateContext<'_, '_>, ex: &Expr<'_>, arms: &[Arm<'_ message, "try this", suggestion.join(" | "), - Applicability::MachineApplicable, + Applicability::MaybeIncorrect, ) } } @@ -808,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); } }); } @@ -878,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.", ); } @@ -1192,3 +1296,40 @@ fn cmp(&self, other: &Self) -> Ordering { None } + +#[test] +fn test_overlapping() { + use rustc_span::source_map::DUMMY_SP; + + let sp = |s, e| SpannedRange { + span: DUMMY_SP, + node: (s, e), + }; + + assert_eq!(None, overlapping::(&[])); + assert_eq!(None, overlapping(&[sp(1, Bound::Included(4))])); + assert_eq!( + None, + overlapping(&[sp(1, Bound::Included(4)), sp(5, Bound::Included(6))]) + ); + assert_eq!( + None, + overlapping(&[ + sp(1, Bound::Included(4)), + sp(5, Bound::Included(6)), + sp(10, Bound::Included(11)) + ],) + ); + assert_eq!( + Some((&sp(1, Bound::Included(4)), &sp(3, Bound::Included(6)))), + overlapping(&[sp(1, Bound::Included(4)), sp(3, Bound::Included(6))]) + ); + assert_eq!( + Some((&sp(5, Bound::Included(6)), &sp(6, Bound::Included(11)))), + overlapping(&[ + sp(1, Bound::Included(4)), + sp(5, Bound::Included(6)), + sp(6, Bound::Included(11)) + ],) + ); +}