Auto merge of #11468 - mojave2:issue-11465, r=blyxyas

bors · bors · commit ef736489e784 · 2023-09-16T22:02:10.000Z
add extra `byref` checking for the guard's local changelog: [`redundant_guards`]: Now checks if the variable is bound using `ref` before linting. The lint should not be emitted, when the local variable is bind by-ref in the pattern. fixes #11465
diff --git a/clippy_lints/src/matches/redundant_guards.rs b/clippy_lints/src/matches/redundant_guards.rs
@@ -2,11 +2,12 @@ use clippy_utils::diagnostics::span_lint_and_then;
 use clippy_utils::path_to_local;
 use clippy_utils::source::snippet_with_applicability;
 use clippy_utils::visitors::{for_each_expr, is_local_used};
-use rustc_ast::LitKind;
+use rustc_ast::{BorrowKind, LitKind};
 use rustc_errors::Applicability;
 use rustc_hir::def::{DefKind, Res};
 use rustc_hir::{Arm, BinOpKind, Expr, ExprKind, Guard, MatchSource, Node, Pat, PatKind};
 use rustc_lint::LateContext;
+use rustc_span::symbol::Ident;
 use rustc_span::Span;
 use std::ops::ControlFlow;
 
@@ -34,24 +35,37 @@ pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, arms: &'tcx [Arm<'tcx>]) {
                 ],
                 MatchSource::Normal,
             ) = if_expr.kind
+            && let Some(binding) = get_pat_binding(cx, scrutinee, outer_arm)
         {
+            let pat_span = match (arm.pat.kind, binding.byref_ident) {
+                (PatKind::Ref(pat, _), Some(_)) => pat.span,
+                (PatKind::Ref(..), None) | (_, Some(_)) => continue,
+                _ => arm.pat.span,
+            };
             emit_redundant_guards(
                 cx,
                 outer_arm,
                 if_expr.span,
-                scrutinee,
-                arm.pat.span,
+                pat_span,
+                &binding,
                 arm.guard,
             );
         }
         // `Some(x) if let Some(2) = x`
-        else if let Guard::IfLet(let_expr) = guard {
+        else if let Guard::IfLet(let_expr) = guard
+            && let Some(binding) = get_pat_binding(cx, let_expr.init, outer_arm)
+        {
+            let pat_span = match (let_expr.pat.kind, binding.byref_ident) {
+                (PatKind::Ref(pat, _), Some(_)) => pat.span,
+                (PatKind::Ref(..), None) | (_, Some(_)) => continue,
+                _ => let_expr.pat.span,
+            };
             emit_redundant_guards(
                 cx,
                 outer_arm,
                 let_expr.span,
-                let_expr.init,
-                let_expr.pat.span,
+                pat_span,
+                &binding,
                 None,
             );
         }
@@ -67,43 +81,63 @@ pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, arms: &'tcx [Arm<'tcx>]) {
             //
             // This isn't necessary in the other two checks, as they must be a pattern already.
             && cx.typeck_results().expr_ty(local) == cx.typeck_results().expr_ty(pat)
+            && let Some(binding) = get_pat_binding(cx, local, outer_arm)
         {
+            let pat_span = match (pat.kind, binding.byref_ident) {
+                (ExprKind::AddrOf(BorrowKind::Ref, _, expr), Some(_)) => expr.span,
+                (ExprKind::AddrOf(..), None) | (_, Some(_)) => continue,
+                _ => pat.span,
+            };
             emit_redundant_guards(
                 cx,
                 outer_arm,
                 if_expr.span,
-                local,
-                pat.span,
+                pat_span,
+                &binding,
                 None,
             );
         }
     }
 }
 
-fn get_pat_binding<'tcx>(cx: &LateContext<'tcx>, guard_expr: &Expr<'_>, outer_arm: &Arm<'tcx>) -> Option<(Span, bool)> {
+struct PatBindingInfo {
+    span: Span,
+    byref_ident: Option<Ident>,
+    is_field: bool,
+}
+
+fn get_pat_binding<'tcx>(
+    cx: &LateContext<'tcx>,
+    guard_expr: &Expr<'_>,
+    outer_arm: &Arm<'tcx>,
+) -> Option<PatBindingInfo> {
     if let Some(local) = path_to_local(guard_expr) && !is_local_used(cx, outer_arm.body, local) {
         let mut span = None;
+        let mut byref_ident = None;
         let mut multiple_bindings = false;
         // `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
+            if let PatKind::Binding(bind_annot, hir_id, ident, _) = pat.kind
                 && hir_id == local
-                && span.replace(pat.span).is_some()
             {
-                multiple_bindings = true;
-                return false;
+                if matches!(bind_annot.0, rustc_ast::ByRef::Yes) {
+                    let _ = byref_ident.insert(ident);
+                }
+                // the second call of `replace()` returns a `Some(span)`, meaning a multi-binding pattern
+                if span.replace(pat.span).is_some() {
+                    multiple_bindings = true;
+                    return false;
+                }
             }
-
             true
         });
 
         // Ignore bindings from or patterns, like `First(x) | Second(x, _) | Third(x, _, _)`
         if !multiple_bindings {
-            return span.map(|span| {
-                (
-                    span,
-                    !matches!(cx.tcx.hir().get_parent(local), Node::PatField(_)),
-                )
+            return span.map(|span| PatBindingInfo {
+                span,
+                byref_ident,
+                is_field: matches!(cx.tcx.hir().get_parent(local), Node::PatField(_)),
             });
         }
     }
@@ -115,14 +149,11 @@ fn emit_redundant_guards<'tcx>(
     cx: &LateContext<'tcx>,
     outer_arm: &Arm<'tcx>,
     guard_span: Span,
-    local: &Expr<'_>,
     pat_span: Span,
+    pat_binding: &PatBindingInfo,
     inner_guard: Option<Guard<'_>>,
 ) {
     let mut app = Applicability::MaybeIncorrect;
-    let Some((pat_binding, can_use_shorthand)) = get_pat_binding(cx, local, outer_arm) else {
-        return;
-    };
 
     span_lint_and_then(
         cx,
@@ -131,14 +162,21 @@ fn emit_redundant_guards<'tcx>(
         "redundant guard",
         |diag| {
             let binding_replacement = snippet_with_applicability(cx, pat_span, "<binding_repl>", &mut app);
+            let suggestion_span = match *pat_binding {
+                PatBindingInfo {
+                    span,
+                    byref_ident: Some(ident),
+                    is_field: true,
+                } => (span, format!("{ident}: {binding_replacement}")),
+                PatBindingInfo {
+                    span, is_field: true, ..
+                } => (span.shrink_to_hi(), format!(": {binding_replacement}")),
+                PatBindingInfo { span, .. } => (span, binding_replacement.into_owned()),
+            };
             diag.multipart_suggestion_verbose(
                 "try",
                 vec![
-                    if can_use_shorthand {
-                        (pat_binding, binding_replacement.into_owned())
-                    } else {
-                        (pat_binding.shrink_to_hi(), format!(": {binding_replacement}"))
-                    },
+                    suggestion_span,
                     (
                         guard_span.source_callsite().with_lo(outer_arm.pat.span.hi()),
                         inner_guard.map_or_else(String::new, |guard| {
diff --git a/tests/ui/redundant_guards.fixed b/tests/ui/redundant_guards.fixed
@@ -143,3 +143,44 @@ fn g(opt_s: Option<S>) {
         _ => {},
     }
 }
+
+mod issue11465 {
+    enum A {
+        Foo([u8; 3]),
+    }
+
+    struct B {
+        b: String,
+        c: i32,
+    }
+
+    fn issue11465() {
+        let c = Some(1);
+        match c {
+            Some(1) => {},
+            Some(2) => {},
+            Some(3) => {},
+            _ => {},
+        };
+
+        let enum_a = A::Foo([98, 97, 114]);
+        match enum_a {
+            A::Foo(ref arr) if arr == b"foo" => {},
+            A::Foo(ref arr) if let b"bar" = arr => {},
+            A::Foo(ref arr) if matches!(arr, b"baz") => {},
+            _ => {},
+        };
+
+        let struct_b = B {
+            b: "bar".to_string(),
+            c: 42,
+        };
+        match struct_b {
+            B { ref b, .. } if b == "bar" => {},
+            B { c: 1, .. } => {},
+            B { c: 1, .. } => {},
+            B { c: 1, .. } => {},
+            _ => {},
+        }
+    }
+}
diff --git a/tests/ui/redundant_guards.rs b/tests/ui/redundant_guards.rs
@@ -143,3 +143,44 @@ fn g(opt_s: Option<S>) {
         _ => {},
     }
 }
+
+mod issue11465 {
+    enum A {
+        Foo([u8; 3]),
+    }
+
+    struct B {
+        b: String,
+        c: i32,
+    }
+
+    fn issue11465() {
+        let c = Some(1);
+        match c {
+            Some(ref x) if x == &1 => {},
+            Some(ref x) if let &2 = x => {},
+            Some(ref x) if matches!(x, &3) => {},
+            _ => {},
+        };
+
+        let enum_a = A::Foo([98, 97, 114]);
+        match enum_a {
+            A::Foo(ref arr) if arr == b"foo" => {},
+            A::Foo(ref arr) if let b"bar" = arr => {},
+            A::Foo(ref arr) if matches!(arr, b"baz") => {},
+            _ => {},
+        };
+
+        let struct_b = B {
+            b: "bar".to_string(),
+            c: 42,
+        };
+        match struct_b {
+            B { ref b, .. } if b == "bar" => {},
+            B { ref c, .. } if c == &1 => {},
+            B { ref c, .. } if let &1 = c => {},
+            B { ref c, .. } if matches!(c, &1) => {},
+            _ => {},
+        }
+    }
+}
diff --git a/tests/ui/redundant_guards.stderr b/tests/ui/redundant_guards.stderr
@@ -95,5 +95,77 @@ LL -         x if matches!(x, Some(0)) => ..,
 LL +         Some(0) => ..,
    |
 
-error: aborting due to 8 previous errors
+error: redundant guard
+  --> $DIR/redundant_guards.rs:160:28
+   |
+LL |             Some(ref x) if x == &1 => {},
+   |                            ^^^^^^^
+   |
+help: try
+   |
+LL -             Some(ref x) if x == &1 => {},
+LL +             Some(1) => {},
+   |
+
+error: redundant guard
+  --> $DIR/redundant_guards.rs:161:28
+   |
+LL |             Some(ref x) if let &2 = x => {},
+   |                            ^^^^^^^^^^
+   |
+help: try
+   |
+LL -             Some(ref x) if let &2 = x => {},
+LL +             Some(2) => {},
+   |
+
+error: redundant guard
+  --> $DIR/redundant_guards.rs:162:28
+   |
+LL |             Some(ref x) if matches!(x, &3) => {},
+   |                            ^^^^^^^^^^^^^^^
+   |
+help: try
+   |
+LL -             Some(ref x) if matches!(x, &3) => {},
+LL +             Some(3) => {},
+   |
+
+error: redundant guard
+  --> $DIR/redundant_guards.rs:180:32
+   |
+LL |             B { ref c, .. } if c == &1 => {},
+   |                                ^^^^^^^
+   |
+help: try
+   |
+LL -             B { ref c, .. } if c == &1 => {},
+LL +             B { c: 1, .. } => {},
+   |
+
+error: redundant guard
+  --> $DIR/redundant_guards.rs:181:32
+   |
+LL |             B { ref c, .. } if let &1 = c => {},
+   |                                ^^^^^^^^^^
+   |
+help: try
+   |
+LL -             B { ref c, .. } if let &1 = c => {},
+LL +             B { c: 1, .. } => {},
+   |
+
+error: redundant guard
+  --> $DIR/redundant_guards.rs:182:32
+   |
+LL |             B { ref c, .. } if matches!(c, &1) => {},
+   |                                ^^^^^^^^^^^^^^^
+   |
+help: try
+   |
+LL -             B { ref c, .. } if matches!(c, &1) => {},
+LL +             B { c: 1, .. } => {},
+   |
+
+error: aborting due to 14 previous errors
 
