X-Git-Url: https://git.lizzy.rs/?a=blobdiff_plain;f=clippy_lints%2Fsrc%2Fcopies.rs;h=38654c753bf80a80eddde76885ee3c655bc5a28d;hb=52408f5b7d0392b008d647c5414e45dad4c4f5fd;hp=26669d8c4c2a9b32335b3bc419e710ae103f50ae;hpb=a72e786c5d8866d554effd246c30fb553b63fa06;p=rust.git diff --git a/clippy_lints/src/copies.rs b/clippy_lints/src/copies.rs index 26669d8c4c2..38654c753bf 100644 --- a/clippy_lints/src/copies.rs +++ b/clippy_lints/src/copies.rs @@ -1,128 +1,119 @@ -use crate::rustc::lint::{LateContext, LateLintPass, LintArray, LintPass}; -use crate::rustc::{declare_tool_lint, lint_array}; -use crate::rustc::ty::Ty; -use crate::rustc::hir::*; -use crate::rustc_data_structures::fx::FxHashMap; +use crate::utils::{get_parent_expr, higher, if_sequence, same_tys, snippet, span_lint_and_then, span_note_and_lint}; +use crate::utils::{SpanlessEq, SpanlessHash}; +use rustc::hir::*; +use rustc::lint::{LateContext, LateLintPass, LintArray, LintPass}; +use rustc::ty::Ty; +use rustc::{declare_lint_pass, declare_tool_lint}; +use rustc_data_structures::fx::FxHashMap; +use std::cmp::Ordering; use std::collections::hash_map::Entry; use std::hash::BuildHasherDefault; -use crate::syntax::symbol::LocalInternedString; -use smallvec::SmallVec; -use crate::utils::{SpanlessEq, SpanlessHash}; -use crate::utils::{get_parent_expr, in_macro, snippet, span_lint_and_then, span_note_and_lint}; +use syntax::symbol::Symbol; -/// **What it does:** Checks for consecutive `if`s with the same condition. -/// -/// **Why is this bad?** This is probably a copy & paste error. -/// -/// **Known problems:** Hopefully none. -/// -/// **Example:** -/// ```rust -/// if a == b { -/// … -/// } else if a == b { -/// … -/// } -/// ``` -/// -/// Note that this lint ignores all conditions with a function call as it could -/// have side effects: -/// -/// ```rust -/// if foo() { -/// … -/// } else if foo() { // not linted -/// … -/// } -/// ``` declare_clippy_lint! { + /// **What it does:** Checks for consecutive `if`s with the same condition. + /// + /// **Why is this bad?** This is probably a copy & paste error. + /// + /// **Known problems:** Hopefully none. + /// + /// **Example:** + /// ```ignore + /// if a == b { + /// … + /// } else if a == b { + /// … + /// } + /// ``` + /// + /// Note that this lint ignores all conditions with a function call as it could + /// have side effects: + /// + /// ```ignore + /// if foo() { + /// … + /// } else if foo() { // not linted + /// … + /// } + /// ``` pub IFS_SAME_COND, correctness, "consecutive `ifs` with the same condition" } -/// **What it does:** Checks for `if/else` with the same body as the *then* part -/// and the *else* part. -/// -/// **Why is this bad?** This is probably a copy & paste error. -/// -/// **Known problems:** Hopefully none. -/// -/// **Example:** -/// ```rust -/// let foo = if … { -/// 42 -/// } else { -/// 42 -/// }; -/// ``` declare_clippy_lint! { + /// **What it does:** Checks for `if/else` with the same body as the *then* part + /// and the *else* part. + /// + /// **Why is this bad?** This is probably a copy & paste error. + /// + /// **Known problems:** Hopefully none. + /// + /// **Example:** + /// ```ignore + /// let foo = if … { + /// 42 + /// } else { + /// 42 + /// }; + /// ``` pub IF_SAME_THEN_ELSE, correctness, "if with the same *then* and *else* blocks" } -/// **What it does:** Checks for `match` with identical arm bodies. -/// -/// **Why is this bad?** This is probably a copy & paste error. If arm bodies -/// are the same on purpose, you can factor them -/// [using `|`](https://doc.rust-lang.org/book/patterns.html#multiple-patterns). -/// -/// **Known problems:** False positive possible with order dependent `match` -/// (see issue -/// [#860](https://github.com/rust-lang-nursery/rust-clippy/issues/860)). -/// -/// **Example:** -/// ```rust,ignore -/// match foo { -/// Bar => bar(), -/// Quz => quz(), -/// Baz => bar(), // <= oops -/// } -/// ``` -/// -/// This should probably be -/// ```rust,ignore -/// match foo { -/// Bar => bar(), -/// Quz => quz(), -/// Baz => baz(), // <= fixed -/// } -/// ``` -/// -/// or if the original code was not a typo: -/// ```rust,ignore -/// match foo { -/// Bar | Baz => bar(), // <= shows the intent better -/// Quz => quz(), -/// } -/// ``` declare_clippy_lint! { + /// **What it does:** Checks for `match` with identical arm bodies. + /// + /// **Why is this bad?** This is probably a copy & paste error. If arm bodies + /// are the same on purpose, you can factor them + /// [using `|`](https://doc.rust-lang.org/book/patterns.html#multiple-patterns). + /// + /// **Known problems:** False positive possible with order dependent `match` + /// (see issue + /// [#860](https://github.com/rust-lang/rust-clippy/issues/860)). + /// + /// **Example:** + /// ```rust,ignore + /// match foo { + /// Bar => bar(), + /// Quz => quz(), + /// Baz => bar(), // <= oops + /// } + /// ``` + /// + /// This should probably be + /// ```rust,ignore + /// match foo { + /// Bar => bar(), + /// Quz => quz(), + /// Baz => baz(), // <= fixed + /// } + /// ``` + /// + /// or if the original code was not a typo: + /// ```rust,ignore + /// match foo { + /// Bar | Baz => bar(), // <= shows the intent better + /// Quz => quz(), + /// } + /// ``` pub MATCH_SAME_ARMS, pedantic, "`match` with identical arm bodies" } -#[derive(Copy, Clone, Debug)] -pub struct CopyAndPaste; - -impl LintPass for CopyAndPaste { - fn get_lints(&self) -> LintArray { - lint_array![IFS_SAME_COND, IF_SAME_THEN_ELSE, MATCH_SAME_ARMS] - } -} +declare_lint_pass!(CopyAndPaste => [IFS_SAME_COND, IF_SAME_THEN_ELSE, MATCH_SAME_ARMS]); impl<'a, 'tcx> LateLintPass<'a, 'tcx> for CopyAndPaste { fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr) { - if !in_macro(expr.span) { + if !expr.span.from_expansion() { // skip ifs directly in else, it will be checked in the parent if - if let Some(&Expr { - node: ExprKind::If(_, _, Some(ref else_expr)), - .. - }) = get_parent_expr(cx, expr) - { - if else_expr.id == expr.id { - return; + if let Some(expr) = get_parent_expr(cx, expr) { + if let Some((_, _, Some(ref else_expr))) = higher::if_block(&expr) { + if else_expr.hir_id == expr.hir_id { + return; + } } } @@ -158,9 +149,10 @@ fn lint_same_cond(cx: &LateContext<'_, '_>, conds: &[&Expr]) { h.finish() }; - let eq: &dyn Fn(&&Expr, &&Expr) -> bool = &|&lhs, &rhs| -> bool { SpanlessEq::new(cx).ignore_fn().eq_expr(lhs, rhs) }; + let eq: &dyn Fn(&&Expr, &&Expr) -> bool = + &|&lhs, &rhs| -> bool { SpanlessEq::new(cx).ignore_fn().eq_expr(lhs, rhs) }; - if let Some((i, j)) = search_same(conds, hash, eq) { + for (i, j) in search_same(conds, hash, eq) { span_note_and_lint( cx, IFS_SAME_COND, @@ -173,7 +165,18 @@ fn lint_same_cond(cx: &LateContext<'_, '_>, conds: &[&Expr]) { } /// Implementation of `MATCH_SAME_ARMS`. -fn lint_match_arms(cx: &LateContext<'_, '_>, expr: &Expr) { +fn lint_match_arms<'tcx>(cx: &LateContext<'_, 'tcx>, expr: &Expr) { + fn same_bindings<'tcx>( + cx: &LateContext<'_, 'tcx>, + lhs: &FxHashMap>, + rhs: &FxHashMap>, + ) -> bool { + lhs.len() == rhs.len() + && lhs + .iter() + .all(|(name, l_ty)| rhs.get(name).map_or(false, |r_ty| same_tys(cx, l_ty, r_ty))) + } + if let ExprKind::Match(_, ref arms, MatchSource::Normal) = expr.node { let hash = |&(_, arm): &(usize, &Arm)| -> u64 { let mut h = SpanlessHash::new(cx, cx.tables); @@ -184,16 +187,17 @@ fn lint_match_arms(cx: &LateContext<'_, '_>, expr: &Expr) { let eq = |&(lindex, lhs): &(usize, &Arm), &(rindex, rhs): &(usize, &Arm)| -> bool { let min_index = usize::min(lindex, rindex); let max_index = usize::max(lindex, rindex); + // Arms with a guard are ignored, those can’t always be merged together // This is also the case for arms in-between each there is an arm with a guard (min_index..=max_index).all(|index| arms[index].guard.is_none()) && SpanlessEq::new(cx).eq_expr(&lhs.body, &rhs.body) && // all patterns should have the same bindings - bindings(cx, &lhs.pats[0]) == bindings(cx, &rhs.pats[0]) + same_bindings(cx, &bindings(cx, &lhs.pats[0]), &bindings(cx, &rhs.pats[0])) }; let indexed_arms: Vec<(usize, &Arm)> = arms.iter().enumerate().collect(); - if let Some((&(_, i), &(_, j))) = search_same(&indexed_arms, hash, eq) { + for (&(_, i), &(_, j)) in search_same(&indexed_arms, hash, eq) { span_lint_and_then( cx, MATCH_SAME_ARMS, @@ -202,7 +206,7 @@ fn lint_match_arms(cx: &LateContext<'_, '_>, expr: &Expr) { |db| { db.span_note(i.body.span, "same as this"); - // Note: this does not use `span_suggestion_with_applicability` on purpose: + // Note: this does not use `span_suggestion` on purpose: // there is no clean way // to remove the other arm. Building a span and suggest to replace it to "" // makes an even more confusing error message. Also in order not to make up a @@ -219,10 +223,16 @@ fn lint_match_arms(cx: &LateContext<'_, '_>, expr: &Expr) { // hiding all the subsequent arms, and rust won't compile db.span_note( i.body.span, - &format!("`{}` has the same arm body as the `_` wildcard, consider removing it`", lhs), + &format!( + "`{}` has the same arm body as the `_` wildcard, consider removing it`", + lhs + ), ); } else { - db.span_note(i.body.span, &format!("consider refactoring into `{} | {}`", lhs, rhs)); + db.span_help( + i.pats[0].span, + &format!("consider refactoring into `{} | {}`", lhs, rhs), + ); } } }, @@ -231,60 +241,33 @@ fn lint_match_arms(cx: &LateContext<'_, '_>, expr: &Expr) { } } -/// Return the list of condition expressions and the list of blocks in a -/// sequence of `if/else`. -/// Eg. would return `([a, b], [c, d, e])` for the expression -/// `if a { c } else if b { d } else { e }`. -fn if_sequence(mut expr: &Expr) -> (SmallVec<[&Expr; 1]>, SmallVec<[&Block; 1]>) { - let mut conds = SmallVec::new(); - let mut blocks: SmallVec<[&Block; 1]> = SmallVec::new(); - - while let ExprKind::If(ref cond, ref then_expr, ref else_expr) = expr.node { - conds.push(&**cond); - if let ExprKind::Block(ref block, _) = then_expr.node { - blocks.push(block); - } else { - panic!("ExprKind::If node is not an ExprKind::Block"); - } - - if let Some(ref else_expr) = *else_expr { - expr = else_expr; - } else { - break; - } - } - - // final `else {..}` - if !blocks.is_empty() { - if let ExprKind::Block(ref block, _) = expr.node { - blocks.push(&**block); - } - } - - (conds, blocks) -} - -/// Return the list of bindings in a pattern. -fn bindings<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, pat: &Pat) -> FxHashMap> { - fn bindings_impl<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, pat: &Pat, map: &mut FxHashMap>) { +/// Returns the list of bindings in a pattern. +fn bindings<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, pat: &Pat) -> FxHashMap> { + fn bindings_impl<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, pat: &Pat, map: &mut FxHashMap>) { match pat.node { PatKind::Box(ref pat) | PatKind::Ref(ref pat, _) => bindings_impl(cx, pat, map), - PatKind::TupleStruct(_, ref pats, _) => for pat in pats { - bindings_impl(cx, pat, map); + PatKind::TupleStruct(_, ref pats, _) => { + for pat in pats { + bindings_impl(cx, pat, map); + } }, - PatKind::Binding(_, _, ident, ref as_pat) => { - if let Entry::Vacant(v) = map.entry(ident.as_str()) { + PatKind::Binding(.., ident, ref as_pat) => { + if let Entry::Vacant(v) = map.entry(ident.name) { v.insert(cx.tables.pat_ty(pat)); } if let Some(ref as_pat) = *as_pat { bindings_impl(cx, as_pat, map); } }, - PatKind::Struct(_, ref fields, _) => for pat in fields { - bindings_impl(cx, &pat.node.pat, map); + PatKind::Or(ref fields) | PatKind::Tuple(ref fields, _) => { + for pat in fields { + bindings_impl(cx, pat, map); + } }, - PatKind::Tuple(ref fields, _) => for pat in fields { - bindings_impl(cx, pat, map); + PatKind::Struct(_, ref fields, _) => { + for pat in fields { + bindings_impl(cx, &pat.pat, map); + } }, PatKind::Slice(ref lhs, ref mid, ref rhs) => { for pat in lhs { @@ -306,7 +289,6 @@ fn bindings_impl<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, pat: &Pat, map: &mut FxHa result } - fn search_same_sequenced(exprs: &[T], eq: Eq) -> Option<(&T, &T)> where Eq: Fn(&T, &T) -> bool, @@ -319,33 +301,42 @@ fn search_same_sequenced(exprs: &[T], eq: Eq) -> Option<(&T, &T)> None } -fn search_same(exprs: &[T], hash: Hash, eq: Eq) -> Option<(&T, &T)> +fn search_common_cases<'a, T, Eq>(exprs: &'a [T], eq: &Eq) -> Option<(&'a T, &'a T)> +where + Eq: Fn(&T, &T) -> bool, +{ + match exprs.len().cmp(&2) { + Ordering::Greater | Ordering::Less => None, + Ordering::Equal => { + if eq(&exprs[0], &exprs[1]) { + Some((&exprs[0], &exprs[1])) + } else { + None + } + }, + } +} + +fn search_same(exprs: &[T], hash: Hash, eq: Eq) -> Vec<(&T, &T)> where Hash: Fn(&T) -> u64, Eq: Fn(&T, &T) -> bool, { - // common cases - if exprs.len() < 2 { - return None; - } else if exprs.len() == 2 { - return if eq(&exprs[0], &exprs[1]) { - Some((&exprs[0], &exprs[1])) - } else { - None - }; + if let Some(expr) = search_common_cases(&exprs, &eq) { + return vec![expr]; } - let mut map: FxHashMap<_, Vec<&_>> = FxHashMap::with_capacity_and_hasher( - exprs.len(), - BuildHasherDefault::default() - ); + let mut match_expr_list: Vec<(&T, &T)> = Vec::new(); + + let mut map: FxHashMap<_, Vec<&_>> = + FxHashMap::with_capacity_and_hasher(exprs.len(), BuildHasherDefault::default()); for expr in exprs { match map.entry(hash(expr)) { Entry::Occupied(mut o) => { for o in o.get() { if eq(o, expr) { - return Some((o, expr)); + match_expr_list.push((o, expr)); } } o.get_mut().push(expr); @@ -356,5 +347,5 @@ fn search_same(exprs: &[T], hash: Hash, eq: Eq) -> Option<(&T, &T)> } } - None + match_expr_list }