Don't lint if local came from an or pattern

Centri3 · Centri3 · commit edbaca03f524 · 2023-06-30T07:44:01.000-05:00
diff --git a/clippy_lints/src/matches/redundant_guards.rs b/clippy_lints/src/matches/redundant_guards.rs
@@ -34,31 +34,25 @@ pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, arms: &'tcx [Arm<'tcx>]) {
                 ],
                 MatchSource::Normal,
             ) = if_expr.kind
-            && let Some((pat_binding, can_use_shorthand)) = get_pat_binding(cx, scrutinee, outer_arm)
         {
             emit_redundant_guards(
                 cx,
                 outer_arm,
                 if_expr.span,
-                pat_binding,
-                can_use_shorthand,
+                scrutinee,
                 arm.pat.span,
                 arm.guard,
                 &mut app,
             );
         }
         // `Some(x) if let Some(2) = x`
-        else if let Guard::IfLet(let_expr) = guard
-            && let pat = let_expr.pat
-            && let Some((pat_binding, can_use_shorthand)) = get_pat_binding(cx, let_expr.init, outer_arm)
-        {
+        else if let Guard::IfLet(let_expr) = guard {
             emit_redundant_guards(
                 cx,
                 outer_arm,
                 let_expr.span,
-                pat_binding,
-                can_use_shorthand,
-                pat.span,
+                let_expr.init,
+                let_expr.pat.span,
                 None,
                 &mut app,
             );
@@ -67,7 +61,6 @@ pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, arms: &'tcx [Arm<'tcx>]) {
         else if let Guard::If(if_expr) = guard
             && let ExprKind::Binary(bin_op, local, pat) = if_expr.kind
             && matches!(bin_op.node, BinOpKind::Eq)
-            && let Some((pat_binding, can_use_shorthand)) = get_pat_binding(cx, local, outer_arm)
             && expr_can_be_pat(cx, pat)
             // Ensure they have the same type. If they don't, we'd need deref coercion which isn't
             // possible (currently) in a pattern. In some cases, you can use something like
@@ -81,8 +74,7 @@ pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, arms: &'tcx [Arm<'tcx>]) {
                 cx,
                 outer_arm,
                 if_expr.span,
-                pat_binding,
-                can_use_shorthand,
+                local,
                 pat.span,
                 None,
                 &mut app,
@@ -94,12 +86,18 @@ pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, arms: &'tcx [Arm<'tcx>]) {
 fn get_pat_binding<'tcx>(cx: &LateContext<'tcx>, guard_expr: &Expr<'_>, outer_arm: &Arm<'tcx>) -> Option<(Span, bool)> {
     if let Some(local) = path_to_local(guard_expr) && !is_local_used(cx, outer_arm.body, local) {
         let mut span = None;
-        // FIXME: hir_id == local is only true for the first or, this will always be false
         let mut multiple_bindings = false;
-        outer_arm.pat.each_binding(|_, hir_id, binding_span, _| {
-            if hir_id == local && span.replace(binding_span).is_some() {
+        // `each_binding` gives the `HirId` of the `Pat` itself, not the binding
+        outer_arm.pat.walk(|pat| {
+            if let PatKind::Binding(_, hir_id, _, _) = pat.kind
+                && hir_id == local
+                && span.replace(pat.span).is_some()
+            {
                 multiple_bindings = true;
+                return false;
             }
+
+            true
         });
 
         if !multiple_bindings {
@@ -115,28 +113,19 @@ fn get_pat_binding<'tcx>(cx: &LateContext<'tcx>, guard_expr: &Expr<'_>, outer_ar
     None
 }
 
-fn get_guard(cx: &LateContext<'_>, guard: Option<Guard<'_>>, app: &mut Applicability) -> String {
-    guard.map_or_else(String::new, |guard| {
-        let (prefix, span) = match guard {
-            Guard::If(e) => ("if", e.span),
-            Guard::IfLet(l) => ("if let", l.span),
-        };
-
-        format!(" {prefix} {}", snippet_with_applicability(cx, span, "<guard>", app))
-    })
-}
-
-#[expect(clippy::too_many_arguments)]
-fn emit_redundant_guards(
-    cx: &LateContext<'_>,
-    outer_arm: &Arm<'_>,
+fn emit_redundant_guards<'tcx>(
+    cx: &LateContext<'tcx>,
+    outer_arm: &Arm<'tcx>,
     guard_span: Span,
-    pat_binding: Span,
-    can_use_shorthand: bool,
+    local: &Expr<'_>,
     pat_span: Span,
     inner_guard: Option<Guard<'_>>,
     app: &mut Applicability,
 ) {
+    let Some((pat_binding, can_use_shorthand)) = get_pat_binding(cx, local, outer_arm) else {
+        return;
+    };
+
     span_lint_and_then(
         cx,
         REDUNDANT_GUARDS,
@@ -154,7 +143,14 @@ fn emit_redundant_guards(
                     },
                     (
                         guard_span.source_callsite().with_lo(outer_arm.pat.span.hi()),
-                        get_guard(cx, inner_guard, app),
+                        inner_guard.map_or_else(String::new, |guard| {
+                            let (prefix, span) = match guard {
+                                Guard::If(e) => ("if", e.span),
+                                Guard::IfLet(l) => ("if let", l.span),
+                            };
+
+                            format!(" {prefix} {}", snippet_with_applicability(cx, span, "<guard>", app))
+                        }),
                     ),
                 ],
                 *app,
diff --git a/tests/ui/redundant_guards.fixed b/tests/ui/redundant_guards.fixed
@@ -1,5 +1,5 @@
 //@run-rustfix
-//@aux-build:proc_macros.rs
+//@aux-build:proc_macros.rs:proc-macro
 #![feature(if_let_guard)]
 #![allow(clippy::no_effect, unused)]
 #![warn(clippy::redundant_guards)]
@@ -80,22 +80,23 @@ fn main() {
     }
 }
 
+// Do not lint
+
 enum E {
     A(&'static str),
     B(&'static str),
     C(&'static str),
 }
 
-#[allow(clippy::redundant_guards)] // FIXME: hir_id == local is only true for the first or
 fn i() {
     match E::A("") {
         E::A(x) | E::B(x) | E::C(x) if x == "from an or pattern" => {},
+        // Ok well, this one is fine. Lint it
+        E::A("not from an or pattern") => {},
         _ => {},
-    }
+    };
 }
 
-// Do not lint
-
 fn f(s: Option<std::ffi::OsString>) {
     match s {
         Some(x) if x == "a" => {},
diff --git a/tests/ui/redundant_guards.rs b/tests/ui/redundant_guards.rs
@@ -1,5 +1,5 @@
 //@run-rustfix
-//@aux-build:proc_macros.rs
+//@aux-build:proc_macros.rs:proc-macro
 #![feature(if_let_guard)]
 #![allow(clippy::no_effect, unused)]
 #![warn(clippy::redundant_guards)]
@@ -80,22 +80,23 @@ fn main() {
     }
 }
 
+// Do not lint
+
 enum E {
     A(&'static str),
     B(&'static str),
     C(&'static str),
 }
 
-#[allow(clippy::redundant_guards)] // FIXME: hir_id == local is only true for the first or
 fn i() {
     match E::A("") {
         E::A(x) | E::B(x) | E::C(x) if x == "from an or pattern" => {},
+        // Ok well, this one is fine. Lint it
+        E::A(y) if y == "not from an or pattern" => {},
         _ => {},
-    }
+    };
 }
 
-// Do not lint
-
 fn f(s: Option<std::ffi::OsString>) {
     match s {
         Some(x) if x == "a" => {},
diff --git a/tests/ui/redundant_guards.stderr b/tests/ui/redundant_guards.stderr
@@ -71,7 +71,19 @@ LL +         B { e: Some(A(2)) } => ..,
    |
 
 error: redundant guard
-  --> $DIR/redundant_guards.rs:108:14
+  --> $DIR/redundant_guards.rs:95:20
+   |
+LL |         E::A(y) if y == "not from an or pattern" => {},
+   |                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+help: try
+   |
+LL -         E::A(y) if y == "not from an or pattern" => {},
+LL +         E::A("not from an or pattern") => {},
+   |
+
+error: redundant guard
+  --> $DIR/redundant_guards.rs:109:14
    |
 LL |         x if matches!(x, Some(0)) => ..,
    |              ^^^^^^^^^^^^^^^^^^^^
@@ -82,5 +94,5 @@ LL -         x if matches!(x, Some(0)) => ..,
 LL +         Some(0) => ..,
    |
 
-error: aborting due to 7 previous errors
+error: aborting due to 8 previous errors
 
