add byref checking for the guard's local

rust-lang · Sep 12, 2023 · 07dd57c · 07dd57c
1 parent 69fcbfd
commit 07dd57c
Show file tree

Hide file tree

Showing 4 changed files with 178 additions and 20 deletions.
diff --git a/clippy_lints/src/matches/redundant_guards.rs b/clippy_lints/src/matches/redundant_guards.rs
@@ -35,23 +35,46 @@ pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, arms: &'tcx [Arm<'tcx>]) {
                 MatchSource::Normal,
             ) = if_expr.kind
         {
+            let Some((binding_span, is_field, is_byref)) = get_pat_binding(cx, scrutinee, outer_arm) else { continue; };
+
+            if is_field && is_byref { return; }
+            let pat_span = if let PatKind::Ref(pat, _) = arm.pat.kind {
+                if is_byref { pat.span } else { continue; }
+            } else {
+                if is_byref { continue; }
+                arm.pat.span
+            };
+
             emit_redundant_guards(
                 cx,
                 outer_arm,
                 if_expr.span,
-                scrutinee,
-                arm.pat.span,
+                pat_span,
+                binding_span,
+                is_field,
                 arm.guard,
             );
         }
         // `Some(x) if let Some(2) = x`
         else if let Guard::IfLet(let_expr) = guard {
+            let Some((binding_span, is_field, is_byref)) = get_pat_binding(cx, let_expr.init, outer_arm)
+                else { continue; };
+
+            if is_field && is_byref { return; }
+            let pat_span = if let PatKind::Ref(pat, _) = let_expr.pat.kind {
+                if is_byref && !is_field { pat.span } else { continue; }
+            } else {
+                if is_byref { continue; }
+                let_expr.pat.span
+            };
+
             emit_redundant_guards(
                 cx,
                 outer_arm,
                 let_expr.span,
-                let_expr.init,
-                let_expr.pat.span,
+                pat_span,
+                binding_span,
+                is_field,
                 None,
             );
         }
@@ -68,30 +91,48 @@ 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_span, is_field, is_byref)) = get_pat_binding(cx, local, outer_arm) else { continue; };
+
+            if is_field && is_byref { return; }
+            let pat_span = if let ExprKind::AddrOf(rustc_ast::BorrowKind::Ref, _, expr) = pat.kind {
+                if is_byref { expr.span } else { continue; }
+            } else {
+                if is_byref { continue; }
+                pat.span
+            };
+
             emit_redundant_guards(
                 cx,
                 outer_arm,
                 if_expr.span,
-                local,
-                pat.span,
+                pat_span,
+                binding_span,
+                is_field,
                 None,
             );
         }
     }
 }
 
-fn get_pat_binding<'tcx>(cx: &LateContext<'tcx>, guard_expr: &Expr<'_>, outer_arm: &Arm<'tcx>) -> Option<(Span, bool)> {
+fn get_pat_binding<'tcx>(
+    cx: &LateContext<'tcx>,
+    guard_expr: &Expr<'_>,
+    outer_arm: &Arm<'tcx>,
+) -> Option<(Span, bool, bool)> {
     if let Some(local) = path_to_local(guard_expr) && !is_local_used(cx, outer_arm.body, local) {
         let mut span = None;
         let mut multiple_bindings = false;
+        let mut is_byref = 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, _, _) = pat.kind
                 && hir_id == local
-                && span.replace(pat.span).is_some()
             {
-                multiple_bindings = true;
-                return false;
+                is_byref = matches!(bind_annot.0, rustc_ast::ByRef::Yes);
+                if span.replace(pat.span).is_some() {
+                    multiple_bindings = true;
+                    return false;
+                }
             }
 
             true
@@ -102,7 +143,8 @@ fn get_pat_binding<'tcx>(cx: &LateContext<'tcx>, guard_expr: &Expr<'_>, outer_ar
             return span.map(|span| {
                 (
                     span,
-                    !matches!(cx.tcx.hir().get_parent(local), Node::PatField(_)),
+                    matches!(cx.tcx.hir().get_parent(local), Node::PatField(_)),
+                    is_byref,
                 )
             });
         }
@@ -115,14 +157,12 @@ fn emit_redundant_guards<'tcx>(
     cx: &LateContext<'tcx>,
     outer_arm: &Arm<'tcx>,
     guard_span: Span,
-    local: &Expr<'_>,
     pat_span: Span,
+    binding_span: Span,
+    field_binding: bool,
     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,
@@ -134,10 +174,10 @@ fn emit_redundant_guards<'tcx>(
             diag.multipart_suggestion_verbose(
                 "try",
                 vec![
-                    if can_use_shorthand {
-                        (pat_binding, binding_replacement.into_owned())
+                    if field_binding {
+                        (binding_span.shrink_to_hi(), format!(": {binding_replacement}"))
                     } else {
-                        (pat_binding.shrink_to_hi(), format!(": {binding_replacement}"))
+                        (binding_span, binding_replacement.into_owned())
                     },
                     (
                         guard_span.source_callsite().with_lo(outer_arm.pat.span.hi()),

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 { 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.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
@@ -94,5 +94,41 @@ 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: aborting due to 11 previous errors