X-Git-Url: https://git.lizzy.rs/?a=blobdiff_plain;f=clippy_lints%2Fsrc%2Fmatches.rs;h=4298e62b80375f5c1189a61ca5eceb3717d00124;hb=79d152190cb8ddce0da420929c0a9dcc47727ba9;hp=d753b664045d80b10fe4a7ee9a66b1b9a5cf5a4a;hpb=779b6aeaa62d7d832bb791c7d92ed11d43f4873a;p=rust.git diff --git a/clippy_lints/src/matches.rs b/clippy_lints/src/matches.rs index d753b664045..4298e62b803 100644 --- a/clippy_lints/src/matches.rs +++ b/clippy_lints/src/matches.rs @@ -3,22 +3,26 @@ 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_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::*; +use rustc_hir::{ + 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; use std::collections::Bound; -use syntax::ast::LitKind; declare_clippy_lint! { /// **What it does:** Checks for matches with a single arm where an `if let` @@ -308,6 +312,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, @@ -324,7 +358,8 @@ pub struct Matches { WILDCARD_ENUM_MATCH_ARM, 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 { @@ -385,11 +420,38 @@ 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 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", + "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 @@ -479,10 +541,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, }; @@ -581,7 +645,7 @@ fn check_wild_err_arm(cx: &LateContext<'_, '_>, ex: &Expr<'_>, arms: &[Arm<'_>]) if match_type(cx, ex_ty, &paths::RESULT) { 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("_"); @@ -724,7 +788,7 @@ fn is_panic_block(block: &Block<'_>) -> bool { fn check_match_ref_pats(cx: &LateContext<'_, '_>, ex: &Expr<'_>, arms: &[Arm<'_>], expr: &Expr<'_>) { if has_only_ref_pats(arms) { - let mut suggs = Vec::new(); + let mut suggs = Vec::with_capacity(arms.len() + 1); let (title, msg) = if let ExprKind::AddrOf(BorrowKind::Ref, Mutability::Not, ref inner) = ex.kind { let span = ex.span.source_callsite(); suggs.push((span, Sugg::hir_with_macro_callsite(cx, inner, "..").to_string())); @@ -826,7 +890,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; } @@ -858,19 +922,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, ); }, @@ -889,6 +985,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>, @@ -1088,3 +1197,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)) + ],) + ); +}