extend needless_borrowed_reference to lint mutable ref patterns

clippy_lints/src/needless_borrowed_ref.rs

@@ -1,6 +1,7 @@
 use clippy_utils::diagnostics::span_lint_and_then;
+use clippy_utils::usage::local_used_after_expr;
 use rustc_errors::Applicability;
-use rustc_hir::{BindingMode, Mutability, Node, Pat, PatKind, Pinnedness};
+use rustc_hir::{self as hir, BindingMode, ExprKind, Mutability, Node, Pat, PatKind, Pinnedness, QPath};
 use rustc_lint::{LateContext, LateLintPass, declare_lint_pass};
 
 declare_clippy_lint! {
@@ -104,7 +105,77 @@ impl<'tcx> LateLintPass<'tcx> for NeedlessBorrowedRef {
                 _ => {},
             }
         }
+
+        // Check for `&mut ref mut x` patterns where the `&mut` can be removed
+        if let PatKind::Ref(pat, Pinnedness::Not, Mutability::Mut) = ref_pat.kind
+            && !ref_pat.span.from_expansion()
+            && let PatKind::Binding(BindingMode::REF_MUT, _, ident, None) = pat.kind
+            && cx
+                .tcx
+                .hir_parent_iter(ref_pat.hir_id)
+                .map_while(|(_, parent)| if let Node::Pat(pat) = parent { Some(pat) } else { None })
+                // Do not lint if behind another ref pattern (can't move out of a reference)
+                // or part of an OR `|` pattern
+                .all(|pat| !matches!(pat.kind, PatKind::Or(_) | PatKind::Ref(..)))
+            && can_move_ref(cx, ref_pat)
+        {
+            span_lint_and_then(
+                cx,
+                NEEDLESS_BORROWED_REFERENCE,
+                ref_pat.span,
+                "this pattern takes a mutable reference on something that is being dereferenced",
+                |diag| {
+                    let span = ref_pat.span.until(ident.span);
+                    diag.span_suggestion_verbose(
+                        span,
+                        "try removing the `&mut ref mut` part",
+                        String::new(),
+                        Applicability::MachineApplicable,
+                    );
+                },
+            );
+        }
+    }
+}
+
+/// Checks whether the `&mut` reference matched by `ref_pat` can be moved (rather than reborrowed).
+/// This is only valid when the scrutinee is a simple local variable that is not used after the
+/// enclosing match/if let expression.
+fn can_move_ref(cx: &LateContext<'_>, ref_pat: &Pat<'_>) -> bool {
+    // Walk up past patterns and arms to find the enclosing match/if let expression
+    let mut scrutinee_and_expr = None;
+    for (_, parent) in cx.tcx.hir_parent_iter(ref_pat.hir_id) {
+        if matches!(parent, Node::Pat(_) | Node::PatField(_) | Node::Arm(_)) {
+            // skip intermediate pattern/arm nodes
+        } else if let Node::Expr(expr) = parent {
+            match expr.kind {
+                ExprKind::Match(scrutinee, _, _) => {
+                    scrutinee_and_expr = Some((scrutinee, expr));
+                    break;
+                },
+                // the pattern's direct parent is the `Let` expression;
+                // keep going one level up to the `If` for the right "used after" scope.
+                ExprKind::Let(let_expr) => {
+                    scrutinee_and_expr = Some((let_expr.init, expr));
+                },
+                ExprKind::If(..) if scrutinee_and_expr.is_some() => {
+                    scrutinee_and_expr = scrutinee_and_expr.map(|(s, _)| (s, expr));
+                    break;
+                },
+                _ => break,
+            }
+        } else {
+            break;
+        }
+    }
+
+    if let Some((scrutinee, scope_expr)) = scrutinee_and_expr
+        && let ExprKind::Path(QPath::Resolved(None, path)) = scrutinee.kind
+        && let hir::def::Res::Local(local_id) = path.res
+    {
+        return !local_used_after_expr(cx, local_id, scope_expr);
     }
+    false
 }
 
 fn check_subpatterns<'tcx>(

tests/ui/needless_borrowed_ref.fixed

@@ -123,6 +123,50 @@ fn should_not_lint(
     }
 }
 
+fn should_lint_mut_ref(x: Option<&mut i32>) {
+    if let Some(x) = x {
+        //~^ needless_borrowed_reference
+        *x = 0;
+    }
+}
+
+fn should_not_lint_mut_ref_nested(x: Option<&mut &mut i32>) {
+    if let Some(&mut &mut ref mut x) = x {
+        *x = 0;
+    }
+}
+
+fn should_not_lint_mut_ref_used_after(x: Option<&mut i32>) -> Option<&mut i32> {
+    if let Some(&mut ref mut x) = x {
+        *x = 0;
+    }
+    x
+}
+
+struct WithDrop<'a>(Option<&'a mut i32>);
+impl Drop for WithDrop<'_> {
+    fn drop(&mut self) {}
+}
+
+fn should_not_lint_mut_ref_drop(x: WithDrop<'_>) {
+    if let Some(&mut ref mut x) = x.0 {
+        *x = 0;
+    }
+}
+
+fn identity<T>(value: T) -> T {
+    value
+}
+
+fn inferred_scrutinee(reference: &mut i32) {
+    let val = identity(Some(reference));
+
+    if let Some(x) = val {
+        //~^ needless_borrowed_reference
+        *x = 0;
+    }
+}
+
 enum Animal {
     Cat(u64),
     Dog(u64),

tests/ui/needless_borrowed_ref.rs

@@ -123,6 +123,50 @@ fn should_not_lint(
     }
 }
 
+fn should_lint_mut_ref(x: Option<&mut i32>) {
+    if let Some(&mut ref mut x) = x {
+        //~^ needless_borrowed_reference
+        *x = 0;
+    }
+}
+
+fn should_not_lint_mut_ref_nested(x: Option<&mut &mut i32>) {
+    if let Some(&mut &mut ref mut x) = x {
+        *x = 0;
+    }
+}
+
+fn should_not_lint_mut_ref_used_after(x: Option<&mut i32>) -> Option<&mut i32> {
+    if let Some(&mut ref mut x) = x {
+        *x = 0;
+    }
+    x
+}
+
+struct WithDrop<'a>(Option<&'a mut i32>);
+impl Drop for WithDrop<'_> {
+    fn drop(&mut self) {}
+}
+
+fn should_not_lint_mut_ref_drop(x: WithDrop<'_>) {
+    if let Some(&mut ref mut x) = x.0 {
+        *x = 0;
+    }
+}
+
+fn identity<T>(value: T) -> T {
+    value
+}
+
+fn inferred_scrutinee(reference: &mut i32) {
+    let val = identity(Some(reference));
+
+    if let Some(&mut ref mut x) = val {
+        //~^ needless_borrowed_reference
+        *x = 0;
+    }
+}
+
 enum Animal {
     Cat(u64),
     Dog(u64),

tests/ui/needless_borrowed_ref.stderr

@@ -213,5 +213,29 @@ LL -     if let &Struct { ref a, b: _, .. } = &s {}
 LL +     if let Struct { a, b: _, .. } = &s {}
    |
 
-error: aborting due to 17 previous errors
+error: this pattern takes a mutable reference on something that is being dereferenced
+  --> tests/ui/needless_borrowed_ref.rs:127:17
+   |
+LL |     if let Some(&mut ref mut x) = x {
+   |                 ^^^^^^^^^^^^^^
+   |
+help: try removing the `&mut ref mut` part
+   |
+LL -     if let Some(&mut ref mut x) = x {
+LL +     if let Some(x) = x {
+   |
+
+error: this pattern takes a mutable reference on something that is being dereferenced
+  --> tests/ui/needless_borrowed_ref.rs:164:17
+   |
+LL |     if let Some(&mut ref mut x) = val {
+   |                 ^^^^^^^^^^^^^^
+   |
+help: try removing the `&mut ref mut` part
+   |
+LL -     if let Some(&mut ref mut x) = val {
+LL +     if let Some(x) = val {
+   |
+
+error: aborting due to 19 previous errors