Auto merge of #6754 - camsteffen:spanlesseq-res, r=flip1995

Teach SpanlessEq binding IDs

changelog: Fix collapsible_match false positive

Fixes #6740

This PR changes the way `SpanlessEq` determines whether two local variables are the same. Instead of checking that the names match, it checks that the `HirId`s match. If local bindings are declared within the expressions that are being compared, `SpanlessEq` will remember bindings that correspond to each other in a `FxHashMap<HirId, HirId>`. This makes `SpanlessEq` more flexible while also fixing false positives.

Example: `{ let x = 1; x + 2 }` is equal to `{ let y = 1; y + 2 }`.

CC `@xFrednet` I think this will resolve some concerns in #6463

Author: bors

clippy_lints/src/matches.rs

@@ -3,19 +3,19 @@ use crate::utils::sugg::Sugg;
 use crate::utils::visitors::LocalUsedVisitor;
 use crate::utils::{
     expr_block, get_parent_expr, implements_trait, in_macro, indent_of, is_allowed, is_expn_of, is_refutable,
-    is_type_diagnostic_item, is_wild, match_qpath, match_type, meets_msrv, multispan_sugg, path_to_local_id,
-    peel_hir_pat_refs, peel_mid_ty_refs, peel_n_hir_expr_refs, remove_blocks, snippet, snippet_block, snippet_opt,
-    snippet_with_applicability, span_lint_and_help, span_lint_and_note, span_lint_and_sugg, span_lint_and_then,
-    strip_pat_refs,
+    is_type_diagnostic_item, is_wild, match_qpath, match_type, meets_msrv, multispan_sugg, path_to_local,
+    path_to_local_id, peel_hir_pat_refs, peel_mid_ty_refs, peel_n_hir_expr_refs, remove_blocks, snippet, snippet_block,
+    snippet_opt, snippet_with_applicability, span_lint_and_help, span_lint_and_note, span_lint_and_sugg,
+    span_lint_and_then, strip_pat_refs,
 };
 use crate::utils::{paths, search_same, SpanlessEq, SpanlessHash};
 use if_chain::if_chain;
 use rustc_ast::ast::LitKind;
-use rustc_data_structures::fx::FxHashMap;
+use rustc_data_structures::fx::{FxHashMap, FxHashSet};
 use rustc_errors::Applicability;
 use rustc_hir::def::CtorKind;
 use rustc_hir::{
-    Arm, BindingAnnotation, Block, BorrowKind, Expr, ExprKind, Guard, Local, MatchSource, Mutability, Node, Pat,
+    Arm, BindingAnnotation, Block, BorrowKind, Expr, ExprKind, Guard, HirId, Local, MatchSource, Mutability, Node, Pat,
     PatKind, QPath, RangeEnd,
 };
 use rustc_lint::{LateContext, LateLintPass, LintContext};
@@ -24,7 +24,7 @@ use rustc_middle::ty::{self, Ty, TyS};
 use rustc_semver::RustcVersion;
 use rustc_session::{declare_tool_lint, impl_lint_pass};
 use rustc_span::source_map::{Span, Spanned};
-use rustc_span::{sym, Symbol};
+use rustc_span::sym;
 use std::cmp::Ordering;
 use std::collections::hash_map::Entry;
 use std::collections::Bound;
@@ -1873,13 +1873,6 @@ fn test_overlapping() {
 
 /// 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);
@@ -1891,12 +1884,38 @@ fn lint_match_arms<'tcx>(cx: &LateContext<'tcx>, expr: &Expr<'_>) {
             let min_index = usize::min(lindex, rindex);
             let max_index = usize::max(lindex, rindex);
 
+            let mut local_map: FxHashMap<HirId, HirId> = FxHashMap::default();
+            let eq_fallback = |a: &Expr<'_>, b: &Expr<'_>| {
+                if_chain! {
+                    if let Some(a_id) = path_to_local(a);
+                    if let Some(b_id) = path_to_local(b);
+                    let entry = match local_map.entry(a_id) {
+                        Entry::Vacant(entry) => entry,
+                        // check if using the same bindings as before
+                        Entry::Occupied(entry) => return *entry.get() == b_id,
+                    };
+                    // the names technically don't have to match; this makes the lint more conservative
+                    if cx.tcx.hir().name(a_id) == cx.tcx.hir().name(b_id);
+                    if TyS::same_type(cx.typeck_results().expr_ty(a), cx.typeck_results().expr_ty(b));
+                    if pat_contains_local(lhs.pat, a_id);
+                    if pat_contains_local(rhs.pat, b_id);
+                    then {
+                        entry.insert(b_id);
+                        true
+                    } else {
+                        false
+                    }
+                }
+            };
             // 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))
+            (min_index..=max_index).all(|index| arms[index].guard.is_none())
+                && SpanlessEq::new(cx)
+                    .expr_fallback(eq_fallback)
+                    .eq_expr(&lhs.body, &rhs.body)
+                // these checks could be removed to allow unused bindings
+                && bindings_eq(lhs.pat, local_map.keys().copied().collect())
+                && bindings_eq(rhs.pat, local_map.values().copied().collect())
         };
 
         let indexed_arms: Vec<(usize, &Arm<'_>)> = arms.iter().enumerate().collect();
@@ -1939,50 +1958,18 @@ fn lint_match_arms<'tcx>(cx: &LateContext<'tcx>, expr: &Expr<'_>) {
     }
 }
 
-/// 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);
+fn pat_contains_local(pat: &Pat<'_>, id: HirId) -> bool {
+    let mut result = false;
+    pat.walk_short(|p| {
+        result |= matches!(p.kind, PatKind::Binding(_, binding_id, ..) if binding_id == id);
+        !result
+    });
     result
 }
+
+/// Returns true if all the bindings in the `Pat` are in `ids` and vice versa
+fn bindings_eq(pat: &Pat<'_>, mut ids: FxHashSet<HirId>) -> bool {
+    let mut result = true;
+    pat.each_binding_or_first(&mut |_, id, _, _| result &= ids.remove(&id));
+    result && ids.is_empty()
+}