Fix needless_borrow causing mutable borrows to be moved

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)
@@ -3227,7 +3227,6 @@ Released 2018-09-13
 [`redundant_slicing`]: https://rust-lang.github.io/rust-clippy/master/index.html#redundant_slicing
 [`redundant_static_lifetimes`]: https://rust-lang.github.io/rust-clippy/master/index.html#redundant_static_lifetimes
 [`ref_binding_to_reference`]: https://rust-lang.github.io/rust-clippy/master/index.html#ref_binding_to_reference
-[`ref_in_deref`]: https://rust-lang.github.io/rust-clippy/master/index.html#ref_in_deref
 [`ref_option_ref`]: https://rust-lang.github.io/rust-clippy/master/index.html#ref_option_ref
 [`regex_macro`]: https://rust-lang.github.io/rust-clippy/master/index.html#regex_macro
 [`repeat_once`]: https://rust-lang.github.io/rust-clippy/master/index.html#repeat_once

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};
@@ -10,11 +11,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
@@ -131,8 +131,6 @@ pub struct Dereferencing {
 struct StateData {
     /// Span of the top level expression
     span: Span,
-    /// The required mutability
-    target_mut: Mutability,
 }
 
 enum State {
@@ -141,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,
     },
 }
 
@@ -214,59 +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, adjust))
-                            } else if !adjust.target.is_ref() {
-                                // Add one to the number of references found.
-                                Some((i + 1, adjust))
+                        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();
+                                    }
+                                    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 {
-                                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 {
-                                        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,
-                                    },
-                                ));
+                                (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)) {
@@ -275,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),
@@ -456,6 +515,30 @@ 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<'_>>) -> bool {
+    match parent {
+        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>,
@@ -504,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);
@@ -518,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 {
                 "&"
@@ -539,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",
                 },
@@ -548,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/lib.register_all.rs

@@ -247,7 +247,6 @@ store.register_group(true, "clippy::all", Some("clippy_all"), vec![
     LintId::of(redundant_slicing::REDUNDANT_SLICING),
     LintId::of(redundant_static_lifetimes::REDUNDANT_STATIC_LIFETIMES),
     LintId::of(reference::DEREF_ADDROF),
-    LintId::of(reference::REF_IN_DEREF),
     LintId::of(regex::INVALID_REGEX),
     LintId::of(repeat_once::REPEAT_ONCE),
     LintId::of(returns::LET_AND_RETURN),

clippy_lints/src/lib.register_complexity.rs

@@ -71,7 +71,6 @@ store.register_group(true, "clippy::complexity", Some("clippy_complexity"), vec!
     LintId::of(redundant_closure_call::REDUNDANT_CLOSURE_CALL),
     LintId::of(redundant_slicing::REDUNDANT_SLICING),
     LintId::of(reference::DEREF_ADDROF),
-    LintId::of(reference::REF_IN_DEREF),
     LintId::of(repeat_once::REPEAT_ONCE),
     LintId::of(strings::STRING_FROM_UTF8_AS_BYTES),
     LintId::of(strlen_on_c_strings::STRLEN_ON_C_STRINGS),

clippy_lints/src/lib.register_lints.rs

@@ -423,7 +423,6 @@ store.register_lints(&[
     redundant_static_lifetimes::REDUNDANT_STATIC_LIFETIMES,
     ref_option_ref::REF_OPTION_REF,
     reference::DEREF_ADDROF,
-    reference::REF_IN_DEREF,
     regex::INVALID_REGEX,
     regex::TRIVIAL_REGEX,
     repeat_once::REPEAT_ONCE,

clippy_lints/src/lib.rs

@@ -703,7 +703,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
     store.register_late_pass(|| Box::new(mut_key::MutableKeyType));
     store.register_late_pass(|| Box::new(modulo_arithmetic::ModuloArithmetic));
     store.register_early_pass(|| Box::new(reference::DerefAddrOf));
-    store.register_early_pass(|| Box::new(reference::RefInDeref));
     store.register_early_pass(|| Box::new(double_parens::DoubleParens));
     store.register_late_pass(|| Box::new(to_string_in_display::ToStringInDisplay::new()));
     store.register_early_pass(|| Box::new(unsafe_removed_from_name::UnsafeNameRemoval));
@@ -935,6 +934,7 @@ pub fn register_renamed(ls: &mut rustc_lint::LintStore) {
     ls.register_renamed("clippy::if_let_some_result", "clippy::match_result_ok");
     ls.register_renamed("clippy::disallowed_type", "clippy::disallowed_types");
     ls.register_renamed("clippy::disallowed_method", "clippy::disallowed_methods");
+    ls.register_renamed("clippy::ref_in_deref", "clippy::needless_borrow");
 
     // uplifted lints
     ls.register_renamed("clippy::invalid_ref", "invalid_value");

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