match_same_arms, ifs_same_cond: lint once per same arm/condition

Author: Alexendoo

clippy_lints/src/copies.rs

@@ -1,5 +1,5 @@
 use clippy_config::Conf;
-use clippy_utils::diagnostics::{span_lint_and_note, span_lint_and_then};
+use clippy_utils::diagnostics::{span_lint, span_lint_and_note, span_lint_and_then};
 use clippy_utils::source::{IntoSpan, SpanRangeExt, first_line_of_span, indent_of, reindent_multiline, snippet};
 use clippy_utils::ty::{InteriorMut, needs_ordered_drop};
 use clippy_utils::visitors::for_each_expr_without_closures;
@@ -567,7 +567,7 @@ fn method_caller_is_mutable<'tcx>(
 
 /// Implementation of `IFS_SAME_COND`.
 fn lint_same_cond<'tcx>(cx: &LateContext<'tcx>, conds: &[&Expr<'_>], interior_mut: &mut InteriorMut<'tcx>) {
-    for (i, j) in search_same(
+    for group in search_same(
         conds,
         |e| hash_expr(cx, e),
         |lhs, rhs| {
@@ -584,14 +584,8 @@ fn lint_same_cond<'tcx>(cx: &LateContext<'tcx>, conds: &[&Expr<'_>], interior_mu
             }
         },
     ) {
-        span_lint_and_note(
-            cx,
-            IFS_SAME_COND,
-            j.span,
-            "this `if` has the same condition as a previous `if`",
-            Some(i.span),
-            "same as this",
-        );
+        let spans: Vec<_> = group.into_iter().map(|expr| expr.span).collect();
+        span_lint(cx, IFS_SAME_COND, spans, "these `if` branches have the same condition");
     }
 }
 
@@ -609,14 +603,13 @@ fn lint_same_fns_in_if_cond(cx: &LateContext<'_>, conds: &[&Expr<'_>]) {
         SpanlessEq::new(cx).eq_expr(lhs, rhs)
     };
 
-    for (i, j) in search_same(conds, |e| hash_expr(cx, e), eq) {
-        span_lint_and_note(
+    for group in search_same(conds, |e| hash_expr(cx, e), eq) {
+        let spans: Vec<_> = group.into_iter().map(|expr| expr.span).collect();
+        span_lint(
             cx,
             SAME_FUNCTIONS_IN_IF_CONDITION,
-            j.span,
-            "this `if` has the same function call as a previous `if`",
-            Some(i.span),
-            "same as this",
+            spans,
+            "these `if` branches have the same function call",
         );
     }
 }

clippy_lints/src/matches/match_same_arms.rs

@@ -1,8 +1,9 @@
-use clippy_utils::diagnostics::span_lint_hir_and_then;
-use clippy_utils::source::snippet_with_applicability;
-use clippy_utils::{SpanlessEq, SpanlessHash, is_lint_allowed, path_to_local, search_same};
+use clippy_utils::diagnostics::span_lint_and_then;
+use clippy_utils::source::SpanRangeExt;
+use clippy_utils::{SpanlessEq, SpanlessHash, fulfill_or_allowed, is_lint_allowed, path_to_local, search_same};
 use core::cmp::Ordering;
 use core::{iter, slice};
+use itertools::Itertools;
 use rustc_arena::DroplessArena;
 use rustc_ast::ast::LitKind;
 use rustc_errors::Applicability;
@@ -110,57 +111,68 @@ pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, arms: &'tcx [Arm<'_>]) {
             && check_same_body()
     };
 
-    let mut appl = Applicability::MaybeIncorrect;
     let indexed_arms: Vec<(usize, &Arm<'_>)> = arms.iter().enumerate().collect();
-    for (&(i, arm1), &(j, arm2)) in search_same(&indexed_arms, hash, eq) {
-        if matches!(arm2.pat.kind, PatKind::Wild) {
-            if !cx.tcx.features().non_exhaustive_omitted_patterns_lint()
-                || is_lint_allowed(cx, NON_EXHAUSTIVE_OMITTED_PATTERNS, arm2.hir_id)
-            {
-                let arm_span = adjusted_arm_span(cx, arm1.span);
-                span_lint_hir_and_then(
-                    cx,
-                    MATCH_SAME_ARMS,
-                    arm1.hir_id,
-                    arm_span,
-                    "this match arm has an identical body to the `_` wildcard arm",
-                    |diag| {
-                        diag.span_suggestion(arm_span, "try removing the arm", "", appl)
-                            .help("or try changing either arm body")
-                            .span_note(arm2.span, "`_` wildcard arm here");
-                    },
-                );
-            }
-        } else {
-            let back_block = backwards_blocking_idxs[j];
-            let (keep_arm, move_arm) = if back_block < i || (back_block == 0 && forwards_blocking_idxs[i] <= j) {
-                (arm1, arm2)
-            } else {
-                (arm2, arm1)
-            };
-
-            span_lint_hir_and_then(
-                cx,
-                MATCH_SAME_ARMS,
-                keep_arm.hir_id,
-                keep_arm.span,
-                "this match arm has an identical body to another arm",
-                |diag| {
-                    let move_pat_snip = snippet_with_applicability(cx, move_arm.pat.span, "<pat2>", &mut appl);
-                    let keep_pat_snip = snippet_with_applicability(cx, keep_arm.pat.span, "<pat1>", &mut appl);
+    for mut group in search_same(&indexed_arms, hash, eq) {
+        // Filter out (and fulfill) `#[allow]`ed and `#[expect]`ed arms
+        group.retain(|(_, arm)| !fulfill_or_allowed(cx, MATCH_SAME_ARMS, [arm.hir_id]));
 
-                    diag.multipart_suggestion(
-                        "or try merging the arm patterns and removing the obsolete arm",
-                        vec![
-                            (keep_arm.pat.span, format!("{keep_pat_snip} | {move_pat_snip}")),
-                            (adjusted_arm_span(cx, move_arm.span), String::new()),
-                        ],
-                        appl,
-                    )
-                    .help("try changing either arm body");
-                },
-            );
+        if group.len() < 2 {
+            continue;
         }
+
+        span_lint_and_then(
+            cx,
+            MATCH_SAME_ARMS,
+            group.iter().map(|(_, arm)| arm.span).collect_vec(),
+            "these match arms have identical bodies",
+            |diag| {
+                diag.help("if this is unintentional make the arms return different values");
+
+                if let [prev @ .., (_, last)] = group.as_slice()
+                    && is_wildcard_arm(last.pat)
+                    && is_lint_allowed(cx, NON_EXHAUSTIVE_OMITTED_PATTERNS, last.hir_id)
+                {
+                    diag.span_label(last.span, "the wildcard arm");
+
+                    let s = if prev.len() > 1 { "s" } else { "" };
+                    diag.multipart_suggestion_verbose(
+                        format!("otherwise remove the non-wildcard arm{s}"),
+                        prev.iter()
+                            .map(|(_, arm)| (adjusted_arm_span(cx, arm.span), String::new()))
+                            .collect(),
+                        Applicability::MaybeIncorrect,
+                    );
+                } else if let &[&(first_idx, _), .., &(last_idx, _)] = group.as_slice() {
+                    let back_block = backwards_blocking_idxs[last_idx];
+                    let split = if back_block < first_idx
+                        || (back_block == 0 && forwards_blocking_idxs[first_idx] <= last_idx)
+                    {
+                        group.split_first()
+                    } else {
+                        group.split_last()
+                    };
+
+                    if let Some(((_, dest), src)) = split
+                        && let Some(pat_snippets) = group
+                            .iter()
+                            .map(|(_, arm)| arm.pat.span.get_source_text(cx))
+                            .collect::<Option<Vec<_>>>()
+                    {
+                        let mut suggs = src
+                            .iter()
+                            .map(|(_, arm)| (adjusted_arm_span(cx, arm.span), String::new()))
+                            .collect_vec();
+
+                        suggs.push((dest.pat.span, pat_snippets.iter().join(" | ")));
+                        diag.multipart_suggestion_verbose(
+                            "otherwise merge the patterns into a single arm",
+                            suggs,
+                            Applicability::MaybeIncorrect,
+                        );
+                    }
+                }
+            },
+        );
     }
 }
 
@@ -449,3 +461,11 @@ fn bindings_eq(pat: &Pat<'_>, mut ids: HirIdSet) -> bool {
     pat.each_binding_or_first(&mut |_, id, _, _| result &= ids.swap_remove(&id));
     result && ids.is_empty()
 }
+
+fn is_wildcard_arm(pat: &Pat<'_>) -> bool {
+    match pat.kind {
+        PatKind::Wild => true,
+        PatKind::Or([.., last]) => matches!(last.kind, PatKind::Wild),
+        _ => false,
+    }
+}

clippy_utils/src/lib.rs

@@ -27,6 +27,7 @@
 
 // FIXME: switch to something more ergonomic here, once available.
 // (Currently there is no way to opt into sysroot crates without `extern crate`.)
+extern crate indexmap;
 extern crate rustc_abi;
 extern crate rustc_ast;
 extern crate rustc_attr_parsing;
@@ -85,7 +86,6 @@ pub use self::hir_utils::{
 use core::mem;
 use core::ops::ControlFlow;
 use std::collections::hash_map::Entry;
-use std::hash::BuildHasherDefault;
 use std::iter::{once, repeat_n};
 use std::sync::{Mutex, MutexGuard, OnceLock};
 
@@ -95,7 +95,7 @@ use rustc_ast::ast::{self, LitKind, RangeLimits};
 use rustc_attr_parsing::{AttributeKind, find_attr};
 use rustc_data_structures::fx::FxHashMap;
 use rustc_data_structures::packed::Pu128;
-use rustc_data_structures::unhash::UnhashMap;
+use rustc_data_structures::unhash::UnindexMap;
 use rustc_hir::LangItem::{OptionNone, OptionSome, ResultErr, ResultOk};
 use rustc_hir::def::{DefKind, Res};
 use rustc_hir::def_id::{CrateNum, DefId, LOCAL_CRATE, LocalDefId, LocalModDefId};
@@ -2486,45 +2486,46 @@ pub fn is_slice_of_primitives(cx: &LateContext<'_>, expr: &Expr<'_>) -> Option<S
     None
 }
 
-/// Returns list of all pairs `(a, b)` where `eq(a, b) == true`
-/// and `a` is before `b` in `exprs` for all `a` and `b` in
-/// `exprs`
+/// Returns a list of groups where elements in each group are equal according to `eq`
+///
+/// - Within each group the elements are sorted by the order they appear in `exprs`
+/// - The groups themselves are sorted by their first element's appearence in `exprs`
 ///
 /// Given functions `eq` and `hash` such that `eq(a, b) == true`
 /// implies `hash(a) == hash(b)`
-pub fn search_same<T, Hash, Eq>(exprs: &[T], mut hash: Hash, mut eq: Eq) -> Vec<(&T, &T)>
+pub fn search_same<T, Hash, Eq>(exprs: &[T], mut hash: Hash, mut eq: Eq) -> Vec<Vec<&T>>
 where
     Hash: FnMut(&T) -> u64,
     Eq: FnMut(&T, &T) -> bool,
 {
     match exprs {
-        [a, b] if eq(a, b) => return vec![(a, b)],
+        [a, b] if eq(a, b) => return vec![vec![a, b]],
         _ if exprs.len() <= 2 => return vec![],
         _ => {},
     }
 
-    let mut match_expr_list: Vec<(&T, &T)> = Vec::new();
-
-    let mut map: UnhashMap<u64, Vec<&_>> =
-        UnhashMap::with_capacity_and_hasher(exprs.len(), BuildHasherDefault::default());
+    let mut buckets: UnindexMap<u64, Vec<Vec<&T>>> = UnindexMap::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));
-                    }
+        match buckets.entry(hash(expr)) {
+            indexmap::map::Entry::Occupied(mut o) => {
+                let bucket = o.get_mut();
+                match bucket.iter_mut().find(|group| eq(expr, group[0])) {
+                    Some(group) => group.push(expr),
+                    None => bucket.push(vec![expr]),
                 }
-                o.get_mut().push(expr);
             },
-            Entry::Vacant(v) => {
-                v.insert(vec![expr]);
+            indexmap::map::Entry::Vacant(v) => {
+                v.insert(vec![vec![expr]]);
             },
         }
     }
 
-    match_expr_list
+    buckets
+        .into_values()
+        .flatten()
+        .filter(|group| group.len() > 1)
+        .collect()
 }
 
 /// Peels off all references on the pattern. Returns the underlying pattern and the number of

tests/ui-toml/ifs_same_cond/ifs_same_cond.rs

@@ -11,9 +11,9 @@ fn issue10272() {
     // should trigger warning
     let x = Cell::new(true);
     if x.get() {
+        //~^ ifs_same_cond
     } else if !x.take() {
     } else if x.get() {
-        //~^ ifs_same_cond
     } else {
     }
 }

tests/ui-toml/ifs_same_cond/ifs_same_cond.stderr

@@ -1,14 +1,12 @@
-error: this `if` has the same condition as a previous `if`
-  --> tests/ui-toml/ifs_same_cond/ifs_same_cond.rs:15:15
-   |
-LL |     } else if x.get() {
-   |               ^^^^^^^
-   |
-note: same as this
+error: these `if` branches have the same condition
   --> tests/ui-toml/ifs_same_cond/ifs_same_cond.rs:13:8
    |
 LL |     if x.get() {
    |        ^^^^^^^
+...
+LL |     } else if x.get() {
+   |               ^^^^^^^
+   |
    = note: `-D clippy::ifs-same-cond` implied by `-D warnings`
    = help: to override `-D warnings` add `#[allow(clippy::ifs_same_cond)]`
 

tests/ui/ifs_same_cond.rs

@@ -6,19 +6,25 @@ fn ifs_same_cond() {
     let b = false;
 
     if b {
+        //~^ ifs_same_cond
     } else if b {
+    }
+
+    if b {
         //~^ ifs_same_cond
+    } else if b {
+    } else if b {
     }
 
     if a == 1 {
-    } else if a == 1 {
         //~^ ifs_same_cond
+    } else if a == 1 {
     }
 
     if 2 * a == 1 {
+        //~^ ifs_same_cond
     } else if 2 * a == 2 {
     } else if 2 * a == 1 {
-        //~^ ifs_same_cond
     } else if a == 1 {
     }
 
@@ -50,8 +56,8 @@ fn ifs_same_cond() {
 fn issue10272() {
     let a = String::from("ha");
     if a.contains("ah") {
-    } else if a.contains("ah") {
         //~^ ifs_same_cond
+    } else if a.contains("ah") {
 
         // Trigger this lint
     } else if a.contains("ha") {