improve MATCH_LIKE_MATCHES_MACRO lint

- add tests
- refactor match_same_arms lint
- prioritize match_expr_like_matches_macro over match_same_arms

Author: alex-700

clippy_lints/src/copies.rs

@@ -1,13 +1,8 @@
-use crate::utils::{eq_expr_value, in_macro, SpanlessEq, SpanlessHash};
-use crate::utils::{get_parent_expr, higher, if_sequence, snippet, span_lint_and_note, span_lint_and_then};
-use rustc_data_structures::fx::FxHashMap;
-use rustc_hir::{Arm, Block, Expr, ExprKind, MatchSource, Pat, PatKind};
+use crate::utils::{eq_expr_value, in_macro, search_same, SpanlessEq, SpanlessHash};
+use crate::utils::{get_parent_expr, higher, if_sequence, span_lint_and_note};
+use rustc_hir::{Block, Expr};
 use rustc_lint::{LateContext, LateLintPass};
-use rustc_middle::ty::{Ty, TyS};
 use rustc_session::{declare_lint_pass, declare_tool_lint};
-use rustc_span::symbol::Symbol;
-use std::collections::hash_map::Entry;
-use std::hash::BuildHasherDefault;
 
 declare_clippy_lint! {
     /// **What it does:** Checks for consecutive `if`s with the same condition.
@@ -108,48 +103,7 @@ declare_clippy_lint! {
     "`if` with the same `then` and `else` blocks"
 }
 
-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"
-}
-
-declare_lint_pass!(CopyAndPaste => [IFS_SAME_COND, SAME_FUNCTIONS_IN_IF_CONDITION, IF_SAME_THEN_ELSE, MATCH_SAME_ARMS]);
+declare_lint_pass!(CopyAndPaste => [IFS_SAME_COND, SAME_FUNCTIONS_IN_IF_CONDITION, IF_SAME_THEN_ELSE]);
 
 impl<'tcx> LateLintPass<'tcx> for CopyAndPaste {
     fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
@@ -167,7 +121,6 @@ impl<'tcx> LateLintPass<'tcx> for CopyAndPaste {
             lint_same_then_else(cx, &blocks);
             lint_same_cond(cx, &conds);
             lint_same_fns_in_if_cond(cx, &conds);
-            lint_match_arms(cx, expr);
         }
     }
 }
@@ -243,122 +196,6 @@ fn lint_same_fns_in_if_cond(cx: &LateContext<'_>, conds: &[&Expr<'_>]) {
     }
 }
 
-/// Implementation of `MATCH_SAME_ARMS`.
-fn lint_match_arms<'tcx>(cx: &LateContext<'tcx>, expr: &Expr<'_>) {
-    fn same_bindings<'tcx>(lhs: &FxHashMap<Symbol, Ty<'tcx>>, rhs: &FxHashMap<Symbol, Ty<'tcx>>) -> bool {
-        lhs.len() == rhs.len()
-            && lhs
-                .iter()
-                .all(|(name, l_ty)| rhs.get(name).map_or(false, |r_ty| TyS::same_type(l_ty, r_ty)))
-    }
-
-    if let ExprKind::Match(_, ref arms, MatchSource::Normal) = expr.kind {
-        let hash = |&(_, arm): &(usize, &Arm<'_>)| -> u64 {
-            let mut h = SpanlessHash::new(cx);
-            h.hash_expr(&arm.body);
-            h.finish()
-        };
-
-        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
-                same_bindings(&bindings(cx, &lhs.pat), &bindings(cx, &rhs.pat))
-        };
-
-        let indexed_arms: Vec<(usize, &Arm<'_>)> = arms.iter().enumerate().collect();
-        for (&(_, i), &(_, j)) in search_same(&indexed_arms, hash, eq) {
-            span_lint_and_then(
-                cx,
-                MATCH_SAME_ARMS,
-                j.body.span,
-                "this `match` has identical arm bodies",
-                |diag| {
-                    diag.span_note(i.body.span, "same as this");
-
-                    // 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
-                    // span for the whole pattern, the suggestion is only shown when there is only
-                    // one pattern. The user should know about `|` if they are already using it…
-
-                    let lhs = snippet(cx, i.pat.span, "<pat1>");
-                    let rhs = snippet(cx, j.pat.span, "<pat2>");
-
-                    if let PatKind::Wild = j.pat.kind {
-                        // if the last arm is _, then i could be integrated into _
-                        // note that i.pat cannot be _, because that would mean that we're
-                        // hiding all the subsequent arms, and rust won't compile
-                        diag.span_note(
-                            i.body.span,
-                            &format!(
-                                "`{}` has the same arm body as the `_` wildcard, consider removing it",
-                                lhs
-                            ),
-                        );
-                    } else {
-                        diag.span_help(i.pat.span, &format!("consider refactoring into `{} | {}`", lhs, rhs));
-                    }
-                },
-            );
-        }
-    }
-}
-
-/// Returns the list of bindings in a pattern.
-fn bindings<'tcx>(cx: &LateContext<'tcx>, pat: &Pat<'_>) -> FxHashMap<Symbol, Ty<'tcx>> {
-    fn bindings_impl<'tcx>(cx: &LateContext<'tcx>, pat: &Pat<'_>, map: &mut FxHashMap<Symbol, Ty<'tcx>>) {
-        match pat.kind {
-            PatKind::Box(ref pat) | PatKind::Ref(ref pat, _) => bindings_impl(cx, pat, map),
-            PatKind::TupleStruct(_, pats, _) => {
-                for pat in pats {
-                    bindings_impl(cx, pat, map);
-                }
-            },
-            PatKind::Binding(.., ident, ref as_pat) => {
-                if let Entry::Vacant(v) = map.entry(ident.name) {
-                    v.insert(cx.typeck_results().pat_ty(pat));
-                }
-                if let Some(ref as_pat) = *as_pat {
-                    bindings_impl(cx, as_pat, map);
-                }
-            },
-            PatKind::Or(fields) | PatKind::Tuple(fields, _) => {
-                for pat in fields {
-                    bindings_impl(cx, pat, map);
-                }
-            },
-            PatKind::Struct(_, fields, _) => {
-                for pat in fields {
-                    bindings_impl(cx, &pat.pat, map);
-                }
-            },
-            PatKind::Slice(lhs, ref mid, rhs) => {
-                for pat in lhs {
-                    bindings_impl(cx, pat, map);
-                }
-                if let Some(ref mid) = *mid {
-                    bindings_impl(cx, mid, map);
-                }
-                for pat in rhs {
-                    bindings_impl(cx, pat, map);
-                }
-            },
-            PatKind::Lit(..) | PatKind::Range(..) | PatKind::Wild | PatKind::Path(..) => (),
-        }
-    }
-
-    let mut result = FxHashMap::default();
-    bindings_impl(cx, pat, &mut result);
-    result
-}
-
 fn search_same_sequenced<T, Eq>(exprs: &[T], eq: Eq) -> Option<(&T, &T)>
 where
     Eq: Fn(&T, &T) -> bool,
@@ -370,47 +207,3 @@ where
     }
     None
 }
-
-fn search_common_cases<'a, T, Eq>(exprs: &'a [T], eq: &Eq) -> Option<(&'a T, &'a T)>
-where
-    Eq: Fn(&T, &T) -> bool,
-{
-    if exprs.len() == 2 && eq(&exprs[0], &exprs[1]) {
-        Some((&exprs[0], &exprs[1]))
-    } else {
-        None
-    }
-}
-
-fn search_same<T, Hash, Eq>(exprs: &[T], hash: Hash, eq: Eq) -> Vec<(&T, &T)>
-where
-    Hash: Fn(&T) -> u64,
-    Eq: Fn(&T, &T) -> bool,
-{
-    if let Some(expr) = search_common_cases(&exprs, &eq) {
-        return vec![expr];
-    }
-
-    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) {
-                        match_expr_list.push((o, expr));
-                    }
-                }
-                o.get_mut().push(expr);
-            },
-            Entry::Vacant(v) => {
-                v.insert(vec![expr]);
-            },
-        }
-    }
-
-    match_expr_list
-}

clippy_lints/src/lib.rs

@@ -528,7 +528,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         &comparison_chain::COMPARISON_CHAIN,
         &copies::IFS_SAME_COND,
         &copies::IF_SAME_THEN_ELSE,
-        &copies::MATCH_SAME_ARMS,
         &copies::SAME_FUNCTIONS_IN_IF_CONDITION,
         &copy_iterator::COPY_ITERATOR,
         &create_dir::CREATE_DIR,
@@ -659,6 +658,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         &matches::MATCH_LIKE_MATCHES_MACRO,
         &matches::MATCH_OVERLAPPING_ARM,
         &matches::MATCH_REF_PATS,
+        &matches::MATCH_SAME_ARMS,
         &matches::MATCH_SINGLE_BINDING,
         &matches::MATCH_WILDCARD_FOR_SINGLE_VARIANTS,
         &matches::MATCH_WILD_ERR_ARM,
@@ -1204,7 +1204,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         LintId::of(&attrs::INLINE_ALWAYS),
         LintId::of(&bit_mask::VERBOSE_BIT_MASK),
         LintId::of(&checked_conversions::CHECKED_CONVERSIONS),
-        LintId::of(&copies::MATCH_SAME_ARMS),
         LintId::of(&copies::SAME_FUNCTIONS_IN_IF_CONDITION),
         LintId::of(&copy_iterator::COPY_ITERATOR),
         LintId::of(&default_trait_access::DEFAULT_TRAIT_ACCESS),
@@ -1234,6 +1233,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         LintId::of(&map_err_ignore::MAP_ERR_IGNORE),
         LintId::of(&match_on_vec_items::MATCH_ON_VEC_ITEMS),
         LintId::of(&matches::MATCH_BOOL),
+        LintId::of(&matches::MATCH_SAME_ARMS),
         LintId::of(&matches::MATCH_WILDCARD_FOR_SINGLE_VARIANTS),
         LintId::of(&matches::MATCH_WILD_ERR_ARM),
         LintId::of(&matches::SINGLE_MATCH_ELSE),