Auto merge of #9573 - Alexendoo:needless-borrowed-ref-slice, r=dswij

bors · bors · commit 5d837b58f7b5 · 2022-10-02T10:39:04.000Z
lint nested patterns and slice patterns in `needless_borrowed_reference`

Now lints in positions other than top level, e.g. `Some(&amp;ref a)`. Or patterns are excluded as that can cause issues

Slice patterns of `ref`s are now linted, e.g. `&amp;[ref a, ref b]`. An easy one to walk into as you might expect that to match against a slice you should use `&amp;[]`,  then to get around a `cannot move out of type [T]` error you apply a `ref`

changelog: [`needless_borrowed_reference`]: lint nested patterns and slice patterns
diff --git a/clippy_lints/src/manual_clamp.rs b/clippy_lints/src/manual_clamp.rs
@@ -324,7 +324,7 @@ fn is_call_max_min_pattern<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>)
         outer_arg: &'tcx Expr<'tcx>,
         span: Span,
     ) -> Option<ClampSuggestion<'tcx>> {
-        if let ExprKind::Call(inner_fn, &[ref first, ref second]) = &inner_call.kind
+        if let ExprKind::Call(inner_fn, [first, second]) = &inner_call.kind
             && let Some(inner_seg) = segment(cx, inner_fn)
             && let Some(outer_seg) = segment(cx, outer_fn)
         {
@@ -377,9 +377,7 @@ fn is_call_max_min_pattern<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>)
 /// # ;
 /// ```
 fn is_match_pattern<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) -> Option<ClampSuggestion<'tcx>> {
-    if let ExprKind::Match(value, &[ref first_arm, ref second_arm, ref last_arm], rustc_hir::MatchSource::Normal) =
-        &expr.kind
-    {
+    if let ExprKind::Match(value, [first_arm, second_arm, last_arm], rustc_hir::MatchSource::Normal) = &expr.kind {
         // Find possible min/max branches
         let minmax_values = |a: &'tcx Arm<'tcx>| {
             if let PatKind::Binding(_, var_hir_id, _, None) = &a.pat.kind
diff --git a/clippy_lints/src/map_unit_fn.rs b/clippy_lints/src/map_unit_fn.rs
@@ -131,12 +131,12 @@ fn reduce_unit_expression<'a>(cx: &LateContext<'_>, expr: &'a hir::Expr<'_>) ->
         },
         hir::ExprKind::Block(block, _) => {
             match (block.stmts, block.expr.as_ref()) {
-                (&[], Some(inner_expr)) => {
+                ([], Some(inner_expr)) => {
                     // If block only contains an expression,
                     // reduce `{ X }` to `X`
                     reduce_unit_expression(cx, inner_expr)
                 },
-                (&[ref inner_stmt], None) => {
+                ([inner_stmt], None) => {
                     // If block only contains statements,
                     // reduce `{ X; }` to `X` or `X;`
                     match inner_stmt.kind {
diff --git a/clippy_lints/src/methods/manual_ok_or.rs b/clippy_lints/src/methods/manual_ok_or.rs
@@ -55,7 +55,7 @@ fn is_ok_wrapping(cx: &LateContext<'_>, map_expr: &Expr<'_>) -> bool {
         if let ExprKind::Closure(&Closure { body, .. }) = map_expr.kind;
         let body = cx.tcx.hir().body(body);
         if let PatKind::Binding(_, param_id, ..) = body.params[0].pat.kind;
-        if let ExprKind::Call(Expr { kind: ExprKind::Path(ok_path), .. }, &[ref ok_arg]) = body.value.kind;
+        if let ExprKind::Call(Expr { kind: ExprKind::Path(ok_path), .. }, [ok_arg]) = body.value.kind;
         if is_lang_ctor(cx, ok_path, ResultOk);
         then { path_to_local_id(ok_arg, param_id) } else { false }
     }
diff --git a/clippy_lints/src/needless_borrowed_ref.rs b/clippy_lints/src/needless_borrowed_ref.rs
@@ -1,43 +1,31 @@
 use clippy_utils::diagnostics::span_lint_and_then;
-use clippy_utils::source::snippet_with_applicability;
-use if_chain::if_chain;
 use rustc_errors::Applicability;
 use rustc_hir::{BindingAnnotation, Mutability, Node, Pat, PatKind};
 use rustc_lint::{LateContext, LateLintPass};
 use rustc_session::{declare_lint_pass, declare_tool_lint};
 
 declare_clippy_lint! {
     /// ### What it does
-    /// Checks for bindings that destructure a reference and borrow the inner
+    /// Checks for bindings that needlessly destructure a reference and borrow the inner
     /// value with `&ref`.
     ///
     /// ### Why is this bad?
     /// This pattern has no effect in almost all cases.
     ///
-    /// ### Known problems
-    /// In some cases, `&ref` is needed to avoid a lifetime mismatch error.
-    /// Example:
-    /// ```rust
-    /// fn foo(a: &Option<String>, b: &Option<String>) {
-    ///     match (a, b) {
-    ///         (None, &ref c) | (&ref c, None) => (),
-    ///         (&Some(ref c), _) => (),
-    ///     };
-    /// }
-    /// ```
-    ///
     /// ### Example
     /// ```rust
     /// let mut v = Vec::<String>::new();
-    /// # #[allow(unused)]
     /// v.iter_mut().filter(|&ref a| a.is_empty());
+    ///
+    /// if let &[ref first, ref second] = v.as_slice() {}
     /// ```
     ///
     /// Use instead:
     /// ```rust
     /// let mut v = Vec::<String>::new();
-    /// # #[allow(unused)]
     /// v.iter_mut().filter(|a| a.is_empty());
+    ///
+    /// if let [first, second] = v.as_slice() {}
     /// ```
     #[clippy::version = "pre 1.29.0"]
     pub NEEDLESS_BORROWED_REFERENCE,
@@ -54,34 +42,83 @@ impl<'tcx> LateLintPass<'tcx> for NeedlessBorrowedRef {
             return;
         }
 
-        if_chain! {
-            // Only lint immutable refs, because `&mut ref T` may be useful.
-            if let PatKind::Ref(sub_pat, Mutability::Not) = pat.kind;
+        // Do not lint patterns that are part of an OR `|` pattern, the binding mode must match in all arms
+        for (_, node) in cx.tcx.hir().parent_iter(pat.hir_id) {
+            let Node::Pat(pat) = node else { break };
+
+            if matches!(pat.kind, PatKind::Or(_)) {
+                return;
+            }
+        }
+
+        // Only lint immutable refs, because `&mut ref T` may be useful.
+        let PatKind::Ref(sub_pat, Mutability::Not) = pat.kind else { return };
 
+        match sub_pat.kind {
             // Check sub_pat got a `ref` keyword (excluding `ref mut`).
-            if let PatKind::Binding(BindingAnnotation::REF, .., spanned_name, _) = sub_pat.kind;
-            let parent_id = cx.tcx.hir().get_parent_node(pat.hir_id);
-            if let Some(parent_node) = cx.tcx.hir().find(parent_id);
-            then {
-                // do not recurse within patterns, as they may have other references
-                // XXXManishearth we can relax this constraint if we only check patterns
-                // with a single ref pattern inside them
-                if let Node::Pat(_) = parent_node {
-                    return;
+            PatKind::Binding(BindingAnnotation::REF, _, ident, None) => {
+                span_lint_and_then(
+                    cx,
+                    NEEDLESS_BORROWED_REFERENCE,
+                    pat.span,
+                    "this pattern takes a reference on something that is being dereferenced",
+                    |diag| {
+                        // `&ref ident`
+                        //  ^^^^^
+                        let span = pat.span.until(ident.span);
+                        diag.span_suggestion_verbose(
+                            span,
+                            "try removing the `&ref` part",
+                            String::new(),
+                            Applicability::MachineApplicable,
+                        );
+                    },
+                );
+            },
+            // Slices where each element is `ref`: `&[ref a, ref b, ..., ref z]`
+            PatKind::Slice(
+                before,
+                None
+                | Some(Pat {
+                    kind: PatKind::Wild, ..
+                }),
+                after,
+            ) => {
+                let mut suggestions = Vec::new();
+
+                for element_pat in itertools::chain(before, after) {
+                    if let PatKind::Binding(BindingAnnotation::REF, _, ident, None) = element_pat.kind {
+                        // `&[..., ref ident, ...]`
+                        //         ^^^^
+                        let span = element_pat.span.until(ident.span);
+                        suggestions.push((span, String::new()));
+                    } else {
+                        return;
+                    }
                 }
-                let mut applicability = Applicability::MachineApplicable;
-                span_lint_and_then(cx, NEEDLESS_BORROWED_REFERENCE, pat.span,
-                                   "this pattern takes a reference on something that is being de-referenced",
-                                   |diag| {
-                                       let hint = snippet_with_applicability(cx, spanned_name.span, "..", &mut applicability).into_owned();
-                                       diag.span_suggestion(
-                                           pat.span,
-                                           "try removing the `&ref` part and just keep",
-                                           hint,
-                                           applicability,
-                                       );
-                                   });
-            }
+
+                if !suggestions.is_empty() {
+                    span_lint_and_then(
+                        cx,
+                        NEEDLESS_BORROWED_REFERENCE,
+                        pat.span,
+                        "dereferencing a slice pattern where every element takes a reference",
+                        |diag| {
+                            // `&[...]`
+                            //  ^
+                            let span = pat.span.until(sub_pat.span);
+                            suggestions.push((span, String::new()));
+
+                            diag.multipart_suggestion(
+                                "try removing the `&` and `ref` parts",
+                                suggestions,
+                                Applicability::MachineApplicable,
+                            );
+                        },
+                    );
+                }
+            },
+            _ => {},
         }
     }
 }
diff --git a/src/docs/needless_borrowed_reference.txt b/src/docs/needless_borrowed_reference.txt
@@ -1,30 +1,22 @@
 ### What it does
-Checks for bindings that destructure a reference and borrow the inner
+Checks for bindings that needlessly destructure a reference and borrow the inner
 value with `&ref`.
 
 ### Why is this bad?
 This pattern has no effect in almost all cases.
 
-### Known problems
-In some cases, `&ref` is needed to avoid a lifetime mismatch error.
-Example:
-```
-fn foo(a: &Option<String>, b: &Option<String>) {
-    match (a, b) {
-        (None, &ref c) | (&ref c, None) => (),
-        (&Some(ref c), _) => (),
-    };
-}
-```
-
 ### Example
 ```
 let mut v = Vec::<String>::new();
 v.iter_mut().filter(|&ref a| a.is_empty());
+
+if let &[ref first, ref second] = v.as_slice() {}
 ```
 
 Use instead:
 ```
 let mut v = Vec::<String>::new();
 v.iter_mut().filter(|a| a.is_empty());
+
+if let [first, second] = v.as_slice() {}
 ```
diff --git a/tests/ui/needless_borrowed_ref.fixed b/tests/ui/needless_borrowed_ref.fixed
@@ -1,17 +1,38 @@
 // run-rustfix
 
-#[warn(clippy::needless_borrowed_reference)]
-#[allow(unused_variables)]
-fn main() {
+#![warn(clippy::needless_borrowed_reference)]
+#![allow(unused, clippy::needless_borrow)]
+
+fn main() {}
+
+fn should_lint(array: [u8; 4], slice: &[u8], slice_of_refs: &[&u8], vec: Vec<u8>) {
     let mut v = Vec::<String>::new();
     let _ = v.iter_mut().filter(|a| a.is_empty());
-    //                            ^ should be linted
 
     let var = 3;
     let thingy = Some(&var);
-    if let Some(&ref v) = thingy {
-        //          ^ should be linted
-    }
+    if let Some(v) = thingy {}
+
+    if let &[a, ref b] = slice_of_refs {}
+
+    let [a, ..] = &array;
+    let [a, b, ..] = &array;
+
+    if let [a, b] = slice {}
+    if let [a, b] = &vec[..] {}
+
+    if let [a, b, ..] = slice {}
+    if let [a, .., b] = slice {}
+    if let [.., a, b] = slice {}
+}
+
+fn should_not_lint(array: [u8; 4], slice: &[u8], slice_of_refs: &[&u8], vec: Vec<u8>) {
+    if let [ref a] = slice {}
+    if let &[ref a, b] = slice {}
+    if let &[ref a, .., b] = slice {}
+
+    // must not be removed as variables must be bound consistently across | patterns
+    if let (&[ref a], _) | ([], ref a) = (slice_of_refs, &1u8) {}
 
     let mut var2 = 5;
     let thingy2 = Some(&mut var2);
@@ -28,17 +49,15 @@ fn main() {
     }
 }
 
-#[allow(dead_code)]
 enum Animal {
     Cat(u64),
     Dog(u64),
 }
 
-#[allow(unused_variables)]
-#[allow(dead_code)]
 fn foo(a: &Animal, b: &Animal) {
     match (a, b) {
-        (&Animal::Cat(v), &ref k) | (&ref k, &Animal::Cat(v)) => (), // lifetime mismatch error if there is no '&ref'
+        // lifetime mismatch error if there is no '&ref' before `feature(nll)` stabilization in 1.63
+        (&Animal::Cat(v), &ref k) | (&ref k, &Animal::Cat(v)) => (),
         //                  ^    and   ^ should **not** be linted
         (&Animal::Dog(ref a), &Animal::Dog(_)) => (), //              ^ should **not** be linted
     }
diff --git a/tests/ui/needless_borrowed_ref.rs b/tests/ui/needless_borrowed_ref.rs
@@ -1,17 +1,38 @@
 // run-rustfix
 
-#[warn(clippy::needless_borrowed_reference)]
-#[allow(unused_variables)]
-fn main() {
+#![warn(clippy::needless_borrowed_reference)]
+#![allow(unused, clippy::needless_borrow)]
+
+fn main() {}
+
+fn should_lint(array: [u8; 4], slice: &[u8], slice_of_refs: &[&u8], vec: Vec<u8>) {
     let mut v = Vec::<String>::new();
     let _ = v.iter_mut().filter(|&ref a| a.is_empty());
-    //                            ^ should be linted
 
     let var = 3;
     let thingy = Some(&var);
-    if let Some(&ref v) = thingy {
-        //          ^ should be linted
-    }
+    if let Some(&ref v) = thingy {}
+
+    if let &[&ref a, ref b] = slice_of_refs {}
+
+    let &[ref a, ..] = &array;
+    let &[ref a, ref b, ..] = &array;
+
+    if let &[ref a, ref b] = slice {}
+    if let &[ref a, ref b] = &vec[..] {}
+
+    if let &[ref a, ref b, ..] = slice {}
+    if let &[ref a, .., ref b] = slice {}
+    if let &[.., ref a, ref b] = slice {}
+}
+
+fn should_not_lint(array: [u8; 4], slice: &[u8], slice_of_refs: &[&u8], vec: Vec<u8>) {
+    if let [ref a] = slice {}
+    if let &[ref a, b] = slice {}
+    if let &[ref a, .., b] = slice {}
+
+    // must not be removed as variables must be bound consistently across | patterns
+    if let (&[ref a], _) | ([], ref a) = (slice_of_refs, &1u8) {}
 
     let mut var2 = 5;
     let thingy2 = Some(&mut var2);
@@ -28,17 +49,15 @@ fn main() {
     }
 }
 
-#[allow(dead_code)]
 enum Animal {
     Cat(u64),
     Dog(u64),
 }
 
-#[allow(unused_variables)]
-#[allow(dead_code)]
 fn foo(a: &Animal, b: &Animal) {
     match (a, b) {
-        (&Animal::Cat(v), &ref k) | (&ref k, &Animal::Cat(v)) => (), // lifetime mismatch error if there is no '&ref'
+        // lifetime mismatch error if there is no '&ref' before `feature(nll)` stabilization in 1.63
+        (&Animal::Cat(v), &ref k) | (&ref k, &Animal::Cat(v)) => (),
         //                  ^    and   ^ should **not** be linted
         (&Animal::Dog(ref a), &Animal::Dog(_)) => (), //              ^ should **not** be linted
     }
diff --git a/tests/ui/needless_borrowed_ref.stderr b/tests/ui/needless_borrowed_ref.stderr

Original file line number	Diff line number	Diff line change
`@@ -55,7 +55,7 @@ fn is_ok_wrapping(cx: &LateContext<'_>, map_expr: &Expr<'_>) -> bool {`
`55`	`55`	`if let ExprKind::Closure(&Closure { body, .. }) = map_expr.kind;`
`56`	`56`	`let body = cx.tcx.hir().body(body);`
`57`	`57`	`if let PatKind::Binding(_, param_id, ..) = body.params[0].pat.kind;`
`58`		`- if let ExprKind::Call(Expr { kind: ExprKind::Path(ok_path), .. }, &[ref ok_arg]) = body.value.kind;`
	`58`	`+ if let ExprKind::Call(Expr { kind: ExprKind::Path(ok_path), .. }, [ok_arg]) = body.value.kind;`
`59`	`59`	`if is_lang_ctor(cx, ok_path, ResultOk);`
`60`	`60`	`then { path_to_local_id(ok_arg, param_id) } else { false }`
`61`	`61`	`}`