Remove final reference on fields and method calls in needless_borrow

Author: Jarcho

CHANGELOG.md

@@ -981,7 +981,7 @@ Released 2021-03-25
   [#6532](https://github.com/rust-lang/rust-clippy/pull/6532)
 * [`single_match`] Suggest `if` over `if let` when possible
   [#6574](https://github.com/rust-lang/rust-clippy/pull/6574)
-* [`ref_in_deref`] Use parentheses correctly in suggestion
+* `ref_in_deref` Use parentheses correctly in suggestion
   [#6609](https://github.com/rust-lang/rust-clippy/pull/6609)
 * [`stable_sort_primitive`] Clarify error message
   [#6611](https://github.com/rust-lang/rust-clippy/pull/6611)

clippy_lints/src/dereference.rs

@@ -1,5 +1,6 @@
 use clippy_utils::diagnostics::{span_lint_and_sugg, span_lint_and_then};
 use clippy_utils::source::{snippet_with_applicability, snippet_with_context};
+use clippy_utils::sugg::has_enclosing_paren;
 use clippy_utils::ty::peel_mid_ty_refs;
 use clippy_utils::{get_parent_expr, get_parent_node, is_lint_allowed, path_to_local};
 use rustc_ast::util::parser::{PREC_POSTFIX, PREC_PREFIX};
@@ -130,8 +131,6 @@ pub struct Dereferencing {
 struct StateData {
     /// Span of the top level expression
     span: Span,
-    /// The required mutability
-    target_mut: Mutability,
 }
 
 enum State {
@@ -140,9 +139,13 @@ 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,
+        required_precedence: i8,
+        msg: &'static str,
     },
 }
 
@@ -213,77 +216,98 @@ 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)
-                                    } else {
-                                        (i, mutability.into())
+                        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,
-                                        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,
-                                        },
-                                    ));
-                                }
+                                    deref_count += 1;
+                                },
+                                None => break None,
+                            };
+                        };
+
+                        // Determine the required number of references before any can be removed. In all cases the
+                        // reference made by the current expression will be removed. After that there are four cases to
+                        // handle.
+                        //
+                        // 1. Auto-borrow will trigger in the current position, so no further references are required.
+                        // 2. Auto-deref ends at a reference, or the underlying type, so one extra needs to be left to
+                        //    handle the automatically inserted re-borrow.
+                        // 3. Auto-deref hits a user-defined `Deref` impl, so at least one reference needs to exist to
+                        //    start auto-deref.
+                        // 4. If the chain of non-user-defined derefs ends with a mutable re-borrow, and re-borrow
+                        //    adjustments will not be inserted automatically, then leave one further reference to avoid
+                        //    moving a mutable borrow.
+                        //    e.g.
+                        //        fn foo<T>(x: &mut Option<&mut T>, y: &mut T) {
+                        //            let x = match x {
+                        //                // Removing the borrow will cause `x` to be moved
+                        //                Some(x) => &mut *x,
+                        //                None => y
+                        //            };
+                        //        }
+                        let deref_msg =
+                            "this expression creates a reference which is immediately dereferenced by the compiler";
+                        let borrow_msg = "this expression borrows a value the compiler would automatically borrow";
+
+                        let (required_refs, required_precedence, msg) = if is_auto_borrow_position(parent, expr.hir_id)
+                        {
+                            (1, PREC_POSTFIX, if deref_count == 1 { borrow_msg } else { deref_msg })
+                        } else if let Some(&Adjust::Borrow(AutoBorrow::Ref(_, mutability))) =
+                            next_adjust.map(|a| &a.kind)
+                        {
+                            if matches!(mutability, AutoBorrowMutability::Mut { .. })
+                                && !is_auto_reborrow_position(parent)
+                            {
+                                (3, 0, deref_msg)
+                            } else {
+                                (2, 0, deref_msg)
                             }
+                        } else {
+                            (2, 0, deref_msg)
+                        };
+
+                        if deref_count >= required_refs {
+                            self.state = Some((
+                                State::DerefedBorrow {
+                                    // One of the required refs is for the current borrow expression, the remaining ones
+                                    // can't be removed without breaking the code. See earlier comment.
+                                    count: deref_count - required_refs,
+                                    required_precedence,
+                                    msg,
+                                },
+                                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,12 +316,30 @@ impl<'tcx> LateLintPass<'tcx> for Dereferencing {
                             ty_changed_count + 1
                         },
                         is_final_ufcs: matches!(expr.kind, ExprKind::Call(..)),
+                        target_mut,
                     },
                     data,
                 ));
             },
-            (Some((State::DerefedBorrow { count }, data)), RefOp::AddrOf) if count != 0 => {
-                self.state = Some((State::DerefedBorrow { count: count - 1 }, data));
+            (
+                Some((
+                    State::DerefedBorrow {
+                        count,
+                        required_precedence,
+                        msg,
+                    },
+                    data,
+                )),
+                RefOp::AddrOf,
+            ) if count != 0 => {
+                self.state = Some((
+                    State::DerefedBorrow {
+                        count: count - 1,
+                        required_precedence,
+                        msg,
+                    },
+                    data,
+                ));
             },
 
             (Some((state, data)), _) => report(cx, expr, state, data),
@@ -475,18 +517,28 @@ 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 {
+fn is_auto_reborrow_position(parent: Option<Node<'_>>) -> bool {
     match parent {
-        Some(Node::Expr(parent)) => match parent.kind {
-            ExprKind::MethodCall(..) => true,
-            ExprKind::Call(callee, _) => callee.hir_id != child_id,
-            _ => false,
-        },
+        Some(Node::Expr(parent)) => matches!(parent.kind, ExprKind::MethodCall(..) | ExprKind::Call(..)),
         Some(Node::Local(_)) => true,
         _ => false,
     }
 }
 
+/// Checks if the given expression is a position which can auto-borrow.
+fn is_auto_borrow_position(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,
+            ExprKind::Call(f, _) => f.hir_id == child_id,
+            _ => false,
+        }
+    } else {
+        false
+    }
+}
+
 /// Adjustments are sometimes made in the parent block rather than the expression itself.
 fn find_adjustments<'tcx>(
     tcx: TyCtxt<'tcx>,
@@ -535,6 +587,7 @@ fn report(cx: &LateContext<'_>, expr: &Expr<'_>, state: State, data: StateData)
         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 +602,12 @@ fn report(cx: &LateContext<'_>, expr: &Expr<'_>, state: State, data: StateData)
             };
             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 +623,7 @@ fn report(cx: &LateContext<'_>, expr: &Expr<'_>, state: State, data: StateData)
                 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",
                 },
@@ -579,19 +632,24 @@ fn report(cx: &LateContext<'_>, expr: &Expr<'_>, state: State, data: StateData)
                 app,
             );
         },
-        State::DerefedBorrow { .. } => {
+        State::DerefedBorrow {
+            required_precedence,
+            msg,
+            ..
+        } => {
             let mut app = Applicability::MachineApplicable;
             let snip = snippet_with_context(cx, expr.span, data.span.ctxt(), "..", &mut app).0;
             span_lint_and_sugg(
                 cx,
                 NEEDLESS_BORROW,
                 data.span,
-                &format!(
-                    "this expression borrows a reference (`{}`) that is immediately dereferenced by the compiler",
-                    cx.typeck_results().expr_ty(expr),
-                ),
+                msg,
                 "change this to",
-                snip.into(),
+                if required_precedence > expr.precedence().order() && !has_enclosing_paren(&snip) {
+                    format!("({})", snip)
+                } else {
+                    snip.into()
+                },
                 app,
             );
         },

clippy_lints/src/implicit_hasher.rs

@@ -179,7 +179,7 @@ impl<'tcx> LateLintPass<'tcx> for ImplicitHasher {
                             )
                             .and_then(|snip| {
                                 let i = snip.find("fn")?;
-                                Some(item.span.lo() + BytePos((i + (&snip[i..]).find('(')?) as u32))
+                                Some(item.span.lo() + BytePos((i + snip[i..].find('(')?) as u32))
                             })
                             .expect("failed to create span for type parameters");
                             Span::new(pos, pos, item.span.ctxt(), item.span.parent())

clippy_lints/src/octal_escapes.rs

@@ -101,7 +101,7 @@ fn check_lit(cx: &EarlyContext<'_>, lit: &Lit, span: Span, is_string: bool) {
         // construct a replacement escape
         // the maximum value is \077, or \x3f, so u8 is sufficient here
         if let Ok(n) = u8::from_str_radix(&contents[from + 1..to], 8) {
-            write!(&mut suggest_1, "\\x{:02x}", n).unwrap();
+            write!(suggest_1, "\\x{:02x}", n).unwrap();
         }
 
         // append the null byte as \x00 and the following digits literally

clippy_utils/src/sugg.rs

@@ -388,7 +388,7 @@ fn binop_to_string(op: AssocOp, lhs: &str, rhs: &str) -> String {
 }
 
 /// Return `true` if `sugg` is enclosed in parenthesis.
-fn has_enclosing_paren(sugg: impl AsRef<str>) -> bool {
+pub fn has_enclosing_paren(sugg: impl AsRef<str>) -> bool {
     let mut chars = sugg.as_ref().chars();
     if chars.next() == Some('(') {
         let mut depth = 1;

tests/ui/bytecount.rs

@@ -1,3 +1,5 @@
+#![allow(clippy::needless_borrow)]
+
 #[deny(clippy::naive_bytecount)]
 fn main() {
     let x = vec![0_u8; 16];

tests/ui/bytecount.stderr

@@ -1,23 +1,23 @@
 error: you appear to be counting bytes the naive way
-  --> $DIR/bytecount.rs:5:13
+  --> $DIR/bytecount.rs:7:13
    |
 LL |     let _ = x.iter().filter(|&&a| a == 0).count(); // naive byte count
    |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using the bytecount crate: `bytecount::count(x, 0)`
    |
 note: the lint level is defined here
-  --> $DIR/bytecount.rs:1:8
+  --> $DIR/bytecount.rs:3:8
    |
 LL | #[deny(clippy::naive_bytecount)]
    |        ^^^^^^^^^^^^^^^^^^^^^^^
 
 error: you appear to be counting bytes the naive way
-  --> $DIR/bytecount.rs:7:13
+  --> $DIR/bytecount.rs:9:13
    |
 LL |     let _ = (&x[..]).iter().filter(|&a| *a == 0).count(); // naive byte count
    |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using the bytecount crate: `bytecount::count((&x[..]), 0)`
 
 error: you appear to be counting bytes the naive way
-  --> $DIR/bytecount.rs:19:13
+  --> $DIR/bytecount.rs:21:13
    |
 LL |     let _ = x.iter().filter(|a| b + 1 == **a).count(); // naive byte count
    |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using the bytecount crate: `bytecount::count(x, b + 1)`

tests/ui/clone_on_copy.fixed

@@ -7,7 +7,8 @@
     clippy::no_effect,
     clippy::unnecessary_operation,
     clippy::vec_init_then_push,
-    clippy::toplevel_ref_arg
+    clippy::toplevel_ref_arg,
+    clippy::needless_borrow
 )]
 
 use std::cell::RefCell;