Fix needless_borrow causing mutable borrows to be moved

Author: Jarcho

clippy_lints/src/dereference.rs

@@ -10,11 +10,10 @@ use rustc_hir::{
     Pat, PatKind, UnOp,
 };
 use rustc_lint::{LateContext, LateLintPass};
-use rustc_middle::ty::adjustment::{Adjust, Adjustment, AutoBorrow};
+use rustc_middle::ty::adjustment::{Adjust, Adjustment, AutoBorrow, AutoBorrowMutability};
 use rustc_middle::ty::{self, Ty, TyCtxt, TyS, TypeckResults};
 use rustc_session::{declare_tool_lint, impl_lint_pass};
 use rustc_span::{symbol::sym, Span};
-use std::iter;
 
 declare_clippy_lint! {
     /// ### What it does
@@ -226,40 +225,58 @@ impl<'tcx> LateLintPass<'tcx> for Dereferencing {
                         let mut iter = find_adjustments(cx.tcx, typeck, expr).iter();
                         if let Some((i, adjust)) = iter.by_ref().enumerate().find_map(|(i, adjust)| {
                             if !matches!(adjust.kind, Adjust::Deref(_)) {
-                                Some((i, adjust))
+                                Some((i, Some(adjust)))
                             } else if !adjust.target.is_ref() {
-                                // Add one to the number of references found.
-                                Some((i + 1, adjust))
+                                // Include the current deref.
+                                Some((i + 1, None))
                             } else {
                                 None
                             }
                         }) {
-                            // Found two consecutive derefs. At least one can be removed.
                             if i > 1 {
-                                let target_mut = iter::once(adjust)
-                                    .chain(iter)
-                                    .find_map(|adjust| match adjust.kind {
-                                        Adjust::Borrow(AutoBorrow::Ref(_, m)) => Some(m.into()),
-                                        _ => None,
-                                    })
-                                    // This default should never happen. Auto-deref always reborrows.
-                                    .unwrap_or(Mutability::Not);
-                                self.state = Some((
-                                    // Subtract one for the current borrow expression, and one to cover the last
-                                    // reference which can't be removed (it's either reborrowed, or needed for
-                                    // auto-deref to happen).
-                                    State::DerefedBorrow {
+                                // If the next adjustment is a mutable borrow, then check to see if the compiler will
+                                // insert a re-borrow here. If not, leave an extra borrow here to avoid attempting to
+                                // move the a mutable reference.
+                                let (i, target_mut) = if let Some(&Adjust::Borrow(AutoBorrow::Ref(_, mutability))) =
+                                    adjust.or_else(|| iter.next()).map(|a| &a.kind)
+                                {
+                                    if matches!(mutability, AutoBorrowMutability::Mut { .. })
+                                        && !is_auto_reborrow_position(parent, expr.hir_id)
+                                    {
+                                        (i - 1, Mutability::Mut)
+                                    } else {
+                                        (i, mutability.into())
+                                    }
+                                } else {
+                                    (
+                                        i,
+                                        iter.find_map(|adjust| match adjust.kind {
+                                            Adjust::Borrow(AutoBorrow::Ref(_, m)) => Some(m.into()),
+                                            _ => None,
+                                        })
+                                        // This default should never happen. Auto-deref always reborrows.
+                                        .unwrap_or(Mutability::Not),
+                                    )
+                                };
+
+                                if i > 1 {
+                                    self.state = Some((
+                                        // Subtract one for the current borrow expression, and one to cover the last
+                                        // reference which can't be removed (it's either reborrowed, or needed for
+                                        // auto-deref to happen).
+                                        State::DerefedBorrow {
                                         count:
                                             // Truncation here would require more than a `u32::MAX` level reference. The compiler
                                             // does not support this.
                                             #[allow(clippy::cast_possible_truncation)]
                                             { i as u32 - 2 }
                                     },
-                                    StateData {
-                                        span: expr.span,
-                                        target_mut,
-                                    },
-                                ));
+                                        StateData {
+                                            span: expr.span,
+                                            target_mut,
+                                        },
+                                    ));
+                                }
                             }
                         }
                     },
@@ -456,6 +473,20 @@ fn is_linted_explicit_deref_position(parent: Option<Node<'_>>, child_id: HirId,
     }
 }
 
+/// Checks if the given expression is in a position which can be auto-reborrowed.
+/// Note: This is only correct assuming auto-deref is already occurring.
+fn is_auto_reborrow_position(parent: Option<Node<'_>>, child_id: HirId) -> bool {
+    match parent {
+        Some(Node::Expr(parent)) => match parent.kind {
+            ExprKind::MethodCall(..) => true,
+            ExprKind::Call(callee, _) => callee.hir_id != child_id,
+            _ => false,
+        },
+        Some(Node::Local(_)) => true,
+        _ => false,
+    }
+}
+
 /// Adjustments are sometimes made in the parent block rather than the expression itself.
 fn find_adjustments<'tcx>(
     tcx: TyCtxt<'tcx>,

tests/ui/needless_borrow.fixed

@@ -45,6 +45,23 @@ fn main() {
         let b = &mut b;
         x(b);
     }
+
+    // Issue #8191
+    let mut x = 5;
+    let mut x = &mut x;
+
+    mut_ref(x);
+    mut_ref(x);
+    let y: &mut i32 = x;
+    let y: &mut i32 = x;
+
+    let y = match 0 {
+        // Don't lint. Removing the borrow would move 'x'
+        0 => &mut x,
+        _ => &mut *x,
+    };
+
+    *x = 5;
 }
 
 #[allow(clippy::needless_borrowed_reference)]

tests/ui/needless_borrow.rs

@@ -45,6 +45,23 @@ fn main() {
         let b = &mut b;
         x(&b);
     }
+
+    // Issue #8191
+    let mut x = 5;
+    let mut x = &mut x;
+
+    mut_ref(&mut x);
+    mut_ref(&mut &mut x);
+    let y: &mut i32 = &mut x;
+    let y: &mut i32 = &mut &mut x;
+
+    let y = match 0 {
+        // Don't lint. Removing the borrow would move 'x'
+        0 => &mut x,
+        _ => &mut *x,
+    };
+
+    *x = 5;
 }
 
 #[allow(clippy::needless_borrowed_reference)]

tests/ui/needless_borrow.stderr

@@ -60,5 +60,29 @@ error: this expression borrows a reference (`&mut i32`) that is immediately dere
 LL |         x(&b);
    |           ^^ help: change this to: `b`
 
-error: aborting due to 10 previous errors
+error: this expression borrows a reference (`&mut i32`) that is immediately dereferenced by the compiler
+  --> $DIR/needless_borrow.rs:53:13
+   |
+LL |     mut_ref(&mut x);
+   |             ^^^^^^ help: change this to: `x`
+
+error: this expression borrows a reference (`&mut i32`) that is immediately dereferenced by the compiler
+  --> $DIR/needless_borrow.rs:54:13
+   |
+LL |     mut_ref(&mut &mut x);
+   |             ^^^^^^^^^^^ help: change this to: `x`
+
+error: this expression borrows a reference (`&mut i32`) that is immediately dereferenced by the compiler
+  --> $DIR/needless_borrow.rs:55:23
+   |
+LL |     let y: &mut i32 = &mut x;
+   |                       ^^^^^^ help: change this to: `x`
+
+error: this expression borrows a reference (`&mut i32`) that is immediately dereferenced by the compiler
+  --> $DIR/needless_borrow.rs:56:23
+   |
+LL |     let y: &mut i32 = &mut &mut x;
+   |                       ^^^^^^^^^^^ help: change this to: `x`
+
+error: aborting due to 14 previous errors