Remove final reference on fields and method calls in needless_borrow

Author: Jarcho

clippy_lints/src/dereference.rs

@@ -130,8 +130,6 @@ pub struct Dereferencing {
 struct StateData {
     /// Span of the top level expression
     span: Span,
-    /// The required mutability
-    target_mut: Mutability,
 }
 
 enum State {
@@ -140,9 +138,11 @@ enum State {
         // The number of calls in a sequence which changed the referenced type
         ty_changed_count: usize,
         is_final_ufcs: bool,
+        /// The required mutability
+        target_mut: Mutability,
     },
     DerefedBorrow {
-        count: u32,
+        count: usize,
     },
 }
 
@@ -213,77 +213,76 @@ impl<'tcx> LateLintPass<'tcx> for Dereferencing {
                                     1
                                 },
                                 is_final_ufcs: matches!(expr.kind, ExprKind::Call(..)),
-                            },
-                            StateData {
-                                span: expr.span,
                                 target_mut,
                             },
+                            StateData { span: expr.span },
                         ));
                     },
                     RefOp::AddrOf => {
                         // Find the number of times the borrow is auto-derefed.
                         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, Some(adjust)))
-                            } else if !adjust.target.is_ref() {
-                                // Include the current deref.
-                                Some((i + 1, None))
-                            } else {
-                                None
-                            }
-                        }) {
-                            if i > 1 {
-                                // 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)
+                        let mut deref_count = 0usize;
+                        let next_adjust = loop {
+                            match iter.next() {
+                                Some(adjust) => {
+                                    if !matches!(adjust.kind, Adjust::Deref(_)) {
+                                        break Some(adjust);
+                                    } else if !adjust.target.is_ref() {
+                                        deref_count += 1;
+                                        break iter.next();
                                     } else {
-                                        (i, mutability.into())
+                                        deref_count += 1;
                                     }
-                                } 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,
-                                        },
-                                    ));
-                                }
+                                },
+                                None => break None,
+                            };
+                        };
+
+                        let required_derefs: usize = if is_member_access(parent, expr.hir_id) {
+                            // Neither field accesses nor method calls require a borrow to trigger auto-deref.
+                            1
+                        } else if let Some(&Adjust::Borrow(AutoBorrow::Ref(_, mutability))) =
+                            next_adjust.map(|a| &a.kind)
+                        {
+                            // 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 mutable reference.
+                            if matches!(mutability, AutoBorrowMutability::Mut { .. })
+                                && !is_auto_reborrow_position(parent, expr.hir_id)
+                            {
+                                deref_count = deref_count.saturating_sub(1);
                             }
+                            2
+                        } else {
+                            2
+                        };
+
+                        if deref_count >= required_derefs {
+                            self.state = Some((
+                                // Subtract one for the current borrow expression, and, optionally, 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: deref_count - required_derefs,
+                                },
+                                StateData { span: expr.span },
+                            ));
                         }
                     },
                     _ => (),
                 }
             },
-            (Some((State::DerefMethod { ty_changed_count, .. }, data)), RefOp::Method(_)) => {
+            (
+                Some((
+                    State::DerefMethod {
+                        target_mut,
+                        ty_changed_count,
+                        ..
+                    },
+                    data,
+                )),
+                RefOp::Method(_),
+            ) => {
                 self.state = Some((
                     State::DerefMethod {
                         ty_changed_count: if deref_method_same_type(typeck.expr_ty(expr), typeck.expr_ty(sub_expr)) {
@@ -292,6 +291,7 @@ impl<'tcx> LateLintPass<'tcx> for Dereferencing {
                             ty_changed_count + 1
                         },
                         is_final_ufcs: matches!(expr.kind, ExprKind::Call(..)),
+                        target_mut,
                     },
                     data,
                 ));
@@ -487,6 +487,19 @@ fn is_auto_reborrow_position(parent: Option<Node<'_>>, child_id: HirId) -> bool
     }
 }
 
+/// Checks if the given expression is a method of field access.
+fn is_member_access(parent: Option<Node<'_>>, child_id: HirId) -> bool {
+    if let Some(Node::Expr(parent)) = parent {
+        match parent.kind {
+            ExprKind::MethodCall(_, _, [self_arg, ..], _) => self_arg.hir_id == child_id,
+            ExprKind::Field(..) => true,
+            _ => false,
+        }
+    } else {
+        false
+    }
+}
+
 /// Adjustments are sometimes made in the parent block rather than the expression itself.
 fn find_adjustments(
     tcx: TyCtxt<'tcx>,
@@ -535,6 +548,7 @@ fn report(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, state: State, data: Stat
         State::DerefMethod {
             ty_changed_count,
             is_final_ufcs,
+            target_mut,
         } => {
             let mut app = Applicability::MachineApplicable;
             let (expr_str, expr_is_macro_call) = snippet_with_context(cx, expr.span, data.span.ctxt(), "..", &mut app);
@@ -549,12 +563,12 @@ fn report(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, state: State, data: Stat
             };
             let addr_of_str = if ty_changed_count < ref_count {
                 // Check if a reborrow from &mut T -> &T is required.
-                if data.target_mut == Mutability::Not && matches!(ty.kind(), ty::Ref(_, _, Mutability::Mut)) {
+                if target_mut == Mutability::Not && matches!(ty.kind(), ty::Ref(_, _, Mutability::Mut)) {
                     "&*"
                 } else {
                     ""
                 }
-            } else if data.target_mut == Mutability::Mut {
+            } else if target_mut == Mutability::Mut {
                 "&mut "
             } else {
                 "&"
@@ -570,7 +584,7 @@ fn report(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, state: State, data: Stat
                 cx,
                 EXPLICIT_DEREF_METHODS,
                 data.span,
-                match data.target_mut {
+                match target_mut {
                     Mutability::Not => "explicit `deref` method call",
                     Mutability::Mut => "explicit `deref_mut` method call",
                 },

tests/ui/needless_borrow.fixed

@@ -62,6 +62,11 @@ fn main() {
     };
 
     *x = 5;
+
+    let s = String::new();
+    let _ = s.len();
+    let _ = s.capacity();
+    let _ = s.capacity();
 }
 
 #[allow(clippy::needless_borrowed_reference)]

tests/ui/needless_borrow.rs

@@ -62,6 +62,11 @@ fn main() {
     };
 
     *x = 5;
+
+    let s = String::new();
+    let _ = (&s).len();
+    let _ = (&s).capacity();
+    let _ = (&&s).capacity();
 }
 
 #[allow(clippy::needless_borrowed_reference)]

tests/ui/needless_borrow.stderr

@@ -84,5 +84,23 @@ error: this expression borrows a reference (`&mut i32`) that is immediately dere
 LL |     let y: &mut i32 = &mut &mut x;
    |                       ^^^^^^^^^^^ help: change this to: `x`
 
-error: aborting due to 14 previous errors
+error: this expression borrows a reference (`std::string::String`) that is immediately dereferenced by the compiler
+  --> $DIR/needless_borrow.rs:67:13
+   |
+LL |     let _ = (&s).len();
+   |             ^^^^ help: change this to: `s`
+
+error: this expression borrows a reference (`std::string::String`) that is immediately dereferenced by the compiler
+  --> $DIR/needless_borrow.rs:68:13
+   |
+LL |     let _ = (&s).capacity();
+   |             ^^^^ help: change this to: `s`
+
+error: this expression borrows a reference (`std::string::String`) that is immediately dereferenced by the compiler
+  --> $DIR/needless_borrow.rs:69:13
+   |
+LL |     let _ = (&&s).capacity();
+   |             ^^^^^ help: change this to: `s`
+
+error: aborting due to 17 previous errors