X-Git-Url: https://git.lizzy.rs/?a=blobdiff_plain;f=clippy_lints%2Fsrc%2Fmatches.rs;h=6d7af45a47224d54a99e6949c50185e792acf9bc;hb=7654125d27d95d5c329e554115b510efc1ab1e60;hp=6f1efe0fd85c162dab711b933436dcae19f616b5;hpb=4253aa7137cb7378acc96133c787e49a345c2b3c;p=rust.git diff --git a/clippy_lints/src/matches.rs b/clippy_lints/src/matches.rs index 6f1efe0fd85..6d7af45a472 100644 --- a/clippy_lints/src/matches.rs +++ b/clippy_lints/src/matches.rs @@ -3,21 +3,22 @@ use crate::utils::sugg::Sugg; use crate::utils::usage::is_unused; use crate::utils::{ - expr_block, get_arg_name, 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::lint::in_external_macro; -use rustc::ty::{self, Ty}; use rustc_ast::ast::LitKind; use rustc_errors::Applicability; use rustc_hir::def::CtorKind; use rustc_hir::{ - print, Arm, BindingAnnotation, Block, BorrowKind, Expr, ExprKind, Local, MatchSource, Mutability, PatKind, QPath, - RangeEnd, + Arm, BindingAnnotation, Block, BorrowKind, Expr, ExprKind, Local, MatchSource, Mutability, Node, Pat, PatKind, + QPath, RangeEnd, }; use rustc_lint::{LateContext, LateLintPass, LintContext}; +use rustc_middle::lint::in_external_macro; +use rustc_middle::ty::{self, Ty}; use rustc_session::{declare_tool_lint, impl_lint_pass}; use rustc_span::source_map::Span; use std::cmp::Ordering; @@ -35,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, @@ -96,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, @@ -137,7 +153,7 @@ /// } /// ``` pub MATCH_BOOL, - style, + pedantic, "a `match` on a boolean expression instead of an `if..else` block" } @@ -167,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. @@ -181,7 +197,7 @@ /// } /// ``` pub MATCH_WILD_ERR_ARM, - style, + pedantic, "a `match` with `Err(_)` arm and take drastic actions" } @@ -196,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, @@ -218,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. /// @@ -238,10 +299,17 @@ /// /// **Example:** /// ```rust + /// // Bad /// match "foo" { /// "a" => {}, /// "bar" | _ => {}, /// } + /// + /// // Good + /// match "foo" { + /// "a" => {}, + /// _ => {}, + /// } /// ``` pub WILDCARD_IN_OR_PATTERNS, complexity, @@ -311,6 +379,36 @@ "a match with a single binding instead of using `let` statement" } +declare_clippy_lint! { + /// **What it does:** Checks for unnecessary '..' pattern binding on struct when all fields are explicitly matched. + /// + /// **Why is this bad?** Correctness and readability. It's like having a wildcard pattern after + /// matching all enum variants explicitly. + /// + /// **Known problems:** None. + /// + /// **Example:** + /// ```rust + /// # struct A { a: i32 } + /// let a = A { a: 5 }; + /// + /// // Bad + /// match a { + /// A { a: 5, .. } => {}, + /// _ => {}, + /// } + /// + /// // Good + /// match a { + /// A { a: 5 } => {}, + /// _ => {}, + /// } + /// ``` + pub REST_PAT_IN_FULLY_BOUND_STRUCTS, + restriction, + "a match on a struct that binds all fields but still uses the wildcard pattern" +} + #[derive(Default)] pub struct Matches { infallible_destructuring_match_linted: bool, @@ -325,9 +423,11 @@ 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 + INFALLIBLE_DESTRUCTURING_MATCH, + REST_PAT_IN_FULLY_BOUND_STRUCTS ]); impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Matches { @@ -357,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(); @@ -388,11 +490,41 @@ 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(); + let ty = cx.tcx.type_of(def_id); + if let ty::Adt(def, _) = ty.kind; + if def.is_struct() || def.is_union(); + if fields.len() == def.non_enum_variant().fields.len(); + + then { + span_lint_and_help( + cx, + 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", + ); + } + } + } } #[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 @@ -482,10 +614,12 @@ fn check_single_match_opt_like( if !inner.iter().all(is_wild) { return; } - print::to_string(print::NO_ANN, |s| s.print_qpath(path, false)) + rustc_hir_pretty::to_string(rustc_hir_pretty::NO_ANN, |s| s.print_qpath(path, false)) }, PatKind::Binding(BindingAnnotation::Unannotated, .., ident, None) => ident.to_string(), - PatKind::Path(ref path) => print::to_string(print::NO_ANN, |s| s.print_qpath(path, false)), + PatKind::Path(ref path) => { + rustc_hir_pretty::to_string(rustc_hir_pretty::NO_ANN, |s| s.print_qpath(path, false)) + }, _ => return, }; @@ -504,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 { @@ -546,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, @@ -571,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", ); } @@ -581,10 +715,10 @@ 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 = print::to_string(print::NO_ANN, |s| s.print_qpath(path, false)); + let path_str = rustc_hir_pretty::to_string(rustc_hir_pretty::NO_ANN, |s| s.print_qpath(path, false)); if path_str == "Err" { let mut matching_wild = inner.iter().any(is_wild); let mut ident_bind_name = String::from("_"); @@ -609,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", ); } } @@ -663,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())); + } } } } @@ -700,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, @@ -707,7 +866,7 @@ fn check_wild_enum_match(cx: &LateContext<'_, '_>, ex: &Expr<'_>, arms: &[Arm<'_ message, "try this", suggestion.join(" | "), - Applicability::MachineApplicable, + Applicability::MaybeIncorrect, ) } } @@ -752,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); } }); } @@ -822,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.", ); } @@ -829,7 +989,7 @@ fn check_wild_in_or_pats(cx: &LateContext<'_, '_>, arms: &[Arm<'_>]) { } } -fn check_match_single_binding(cx: &LateContext<'_, '_>, ex: &Expr<'_>, arms: &[Arm<'_>], expr: &Expr<'_>) { +fn check_match_single_binding<'a>(cx: &LateContext<'_, 'a>, ex: &Expr<'a>, arms: &[Arm<'_>], expr: &Expr<'_>) { if in_macro(expr.span) || arms.len() != 1 || is_refutable(cx, arms[0].pat) { return; } @@ -861,19 +1021,51 @@ fn check_match_single_binding(cx: &LateContext<'_, '_>, ex: &Expr<'_>, arms: &[A 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, + 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_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); + } + }; + ( + 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 + ), + ) + }; span_lint_and_sugg( cx, MATCH_SINGLE_BINDING, - expr.span, + target_span, "this match could be written as a `let` statement", "consider using `let` statement", - format!( - "let {} = {};\n{}{}", - 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_body, - ), + sugg, applicability, ); }, @@ -892,6 +1084,19 @@ fn check_match_single_binding(cx: &LateContext<'_, '_>, ex: &Expr<'_>, arms: &[A } } +/// 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>> { + if_chain! { + let map = &cx.tcx.hir(); + 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); + } + } + None +} + /// Gets all arms that are unbounded `PatRange`s. fn all_ranges<'a, 'tcx>( cx: &LateContext<'a, 'tcx>, @@ -1091,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)) + ],) + ); +}