overhaul mut_mut

clippy_lints/src/lib.rs

@@ -482,7 +482,7 @@ pub fn register_lint_passes(store: &mut rustc_lint::LintStore, conf: &'static Co
     store.register_late_pass(|_| Box::new(needless_for_each::NeedlessForEach));
     store.register_late_pass(|_| Box::new(misc::LintPass));
     store.register_late_pass(|_| Box::new(eta_reduction::EtaReduction));
-    store.register_late_pass(|_| Box::new(mut_mut::MutMut));
+    store.register_late_pass(|_| Box::new(mut_mut::MutMut::default()));
     store.register_late_pass(|_| Box::new(unnecessary_mut_passed::UnnecessaryMutPassed));
     store.register_late_pass(|_| Box::<significant_drop_tightening::SignificantDropTightening<'_>>::default());
     store.register_late_pass(|_| Box::new(len_zero::LenZero));

clippy_lints/src/mut_mut.rs

@@ -1,49 +1,100 @@
-use clippy_utils::diagnostics::{span_lint, span_lint_hir};
+use clippy_utils::diagnostics::{span_lint_and_sugg, span_lint_hir_and_then};
 use clippy_utils::higher;
-use rustc_hir::{self as hir, AmbigArg, intravisit};
+use clippy_utils::source::snippet_with_applicability;
+use clippy_utils::sugg::Sugg;
+use rustc_data_structures::fx::FxHashSet;
+use rustc_errors::Applicability;
+use rustc_hir::{self as hir, AmbigArg, BorrowKind, Expr, ExprKind, HirId, Mutability, TyKind, intravisit};
 use rustc_lint::{LateContext, LateLintPass, LintContext};
 use rustc_middle::ty;
-use rustc_session::declare_lint_pass;
+use rustc_session::impl_lint_pass;
 
 declare_clippy_lint! {
     /// ### What it does
     /// Checks for instances of `mut mut` references.
     ///
     /// ### Why is this bad?
-    /// Multiple `mut`s don't add anything meaningful to the
-    /// source. This is either a copy'n'paste error, or it shows a fundamental
-    /// misunderstanding of references.
+    /// This is usually just a typo or a misunderstanding of how references work.
     ///
     /// ### Example
     /// ```no_run
-    /// # let mut y = 1;
-    /// let x = &mut &mut y;
+    /// let x = &mut &mut 1;
+    ///
+    /// let mut x = &mut 1;
+    /// let y = &mut x;
+    ///
+    /// fn foo(x: &mut &mut u32) {}
+    /// ```
+    /// Use instead
+    /// ```no_run
+    /// let x = &mut 1;
+    ///
+    /// let mut x = &mut 1;
+    /// let y = &mut *x; // reborrow
+    ///
+    /// fn foo(x: &mut u32) {}
     /// ```
     #[clippy::version = "pre 1.29.0"]
     pub MUT_MUT,
     pedantic,
-    "usage of double-mut refs, e.g., `&mut &mut ...`"
+    "usage of double mut-refs, e.g., `&mut &mut ...`"
 }
 
-declare_lint_pass!(MutMut => [MUT_MUT]);
+impl_lint_pass!(MutMut => [MUT_MUT]);
+
+#[derive(Default)]
+pub(crate) struct MutMut {
+    seen_tys: FxHashSet<HirId>,
+}
 
 impl<'tcx> LateLintPass<'tcx> for MutMut {
     fn check_block(&mut self, cx: &LateContext<'tcx>, block: &'tcx hir::Block<'_>) {
         intravisit::walk_block(&mut MutVisitor { cx }, block);
     }
 
     fn check_ty(&mut self, cx: &LateContext<'tcx>, ty: &'tcx hir::Ty<'_, AmbigArg>) {
-        if let hir::TyKind::Ref(_, mty) = ty.kind
-            && mty.mutbl == hir::Mutability::Mut
-            && let hir::TyKind::Ref(_, mty) = mty.ty.kind
-            && mty.mutbl == hir::Mutability::Mut
+        if let TyKind::Ref(_, mty) = ty.kind
+            && mty.mutbl == Mutability::Mut
+            && let TyKind::Ref(_, mty2) = mty.ty.kind
+            && mty2.mutbl == Mutability::Mut
             && !ty.span.in_external_macro(cx.sess().source_map())
         {
-            span_lint(
+            if self.seen_tys.contains(&ty.hir_id) {
+                // we have 2+ `&mut`s, e.g., `&mut &mut &mut x`
+                // and we have already flagged on the outermost `&mut &mut (&mut x)`,
+                // so don't flag the inner `&mut &mut (x)`
+                return;
+            }
+
+            // if there is an even longer chain, like `&mut &mut &mut x`, suggest peeling off
+            // all extra ones at once
+            let (mut t, mut t2) = (mty.ty, mty2.ty);
+            let mut many_muts = false;
+            loop {
+                // this should allow us to remember all the nested types, so that the `contains`
+                // above fails faster
+                self.seen_tys.insert(t.hir_id);
+                if let TyKind::Ref(_, next) = t2.kind
+                    && next.mutbl == Mutability::Mut
+                {
+                    (t, t2) = (t2, next.ty);
+                    many_muts = true;
+                } else {
+                    break;
+                }
+            }
+
+            let mut applicability = Applicability::MaybeIncorrect;
+            let sugg = snippet_with_applicability(cx.sess(), t.span, "..", &mut applicability);
+            let suffix = if many_muts { "s" } else { "" };
+            span_lint_and_sugg(
                 cx,
                 MUT_MUT,
                 ty.span,
-                "generally you want to avoid `&mut &mut _` if possible",
+                "a type of form `&mut &mut _`",
+                format!("remove the extra `&mut`{suffix}"),
+                sugg.to_string(),
+                applicability,
             );
         }
     }
@@ -54,7 +105,7 @@ pub struct MutVisitor<'a, 'tcx> {
 }
 
 impl<'tcx> intravisit::Visitor<'tcx> for MutVisitor<'_, 'tcx> {
-    fn visit_expr(&mut self, expr: &'tcx hir::Expr<'_>) {
+    fn visit_expr(&mut self, expr: &'tcx Expr<'_>) {
         if expr.span.in_external_macro(self.cx.sess().source_map()) {
             return;
         }
@@ -68,24 +119,60 @@ impl<'tcx> intravisit::Visitor<'tcx> for MutVisitor<'_, 'tcx> {
             // Let's ignore the generated code.
             intravisit::walk_expr(self, arg);
             intravisit::walk_expr(self, body);
-        } else if let hir::ExprKind::AddrOf(hir::BorrowKind::Ref, hir::Mutability::Mut, e) = expr.kind {
-            if let hir::ExprKind::AddrOf(hir::BorrowKind::Ref, hir::Mutability::Mut, _) = e.kind {
-                span_lint_hir(
+        } else if let ExprKind::AddrOf(BorrowKind::Ref, Mutability::Mut, e) = expr.kind {
+            if let ExprKind::AddrOf(BorrowKind::Ref, Mutability::Mut, e2) = e.kind {
+                if !expr.span.eq_ctxt(e.span) {
+                    return;
+                }
+
+                // if there is an even longer chain, like `&mut &mut &mut x`, suggest peeling off
+                // all extra ones at once
+                let (mut e, mut e2) = (e, e2);
+                let mut many_muts = false;
+                loop {
+                    if !e.span.eq_ctxt(e2.span) {
+                        return;
+                    }
+                    if let ExprKind::AddrOf(BorrowKind::Ref, Mutability::Mut, next) = e2.kind {
+                        (e, e2) = (e2, next);
+                        many_muts = true;
+                    } else {
+                        break;
+                    }
+                }
+
+                let mut applicability = Applicability::MaybeIncorrect;
+                let sugg = Sugg::hir_with_applicability(self.cx, e, "..", &mut applicability);
+                let suffix = if many_muts { "s" } else { "" };
+                span_lint_hir_and_then(
                     self.cx,
                     MUT_MUT,
                     expr.hir_id,
                     expr.span,
-                    "generally you want to avoid `&mut &mut _` if possible",
+                    "an expression of form `&mut &mut _`",
+                    |diag| {
+                        diag.span_suggestion(
+                            expr.span,
+                            format!("remove the extra `&mut`{suffix}"),
+                            sugg,
+                            applicability,
+                        );
+                    },
                 );
-            } else if let ty::Ref(_, ty, hir::Mutability::Mut) = self.cx.typeck_results().expr_ty(e).kind()
+            } else if let ty::Ref(_, ty, Mutability::Mut) = self.cx.typeck_results().expr_ty(e).kind()
                 && ty.peel_refs().is_sized(self.cx.tcx, self.cx.typing_env())
             {
-                span_lint_hir(
+                let mut applicability = Applicability::MaybeIncorrect;
+                let sugg = Sugg::hir_with_applicability(self.cx, e, "..", &mut applicability).mut_addr_deref();
+                span_lint_hir_and_then(
                     self.cx,
                     MUT_MUT,
                     expr.hir_id,
                     expr.span,
-                    "this expression mutably borrows a mutable reference. Consider reborrowing",
+                    "this expression mutably borrows a mutable reference",
+                    |diag| {
+                        diag.span_suggestion(expr.span, "reborrow instead", sugg, applicability);
+                    },
                 );
             }
         }

tests/ui/mut_mut.fixed

@@ -0,0 +1,92 @@
+//@aux-build:proc_macros.rs
+
+#![warn(clippy::mut_mut)]
+#![allow(unused)]
+#![allow(
+    clippy::no_effect,
+    clippy::uninlined_format_args,
+    clippy::unnecessary_operation,
+    clippy::needless_pass_by_ref_mut
+)]
+
+extern crate proc_macros;
+use proc_macros::{external, inline_macros};
+
+fn fun(x: &mut u32) {
+    //~^ mut_mut
+}
+
+fn less_fun(x: *mut *mut u32) {
+    let y = x;
+}
+
+macro_rules! mut_ptr {
+    ($p:expr) => {
+        &mut $p
+    };
+}
+
+#[allow(unused_mut, unused_variables)]
+#[inline_macros]
+fn main() {
+    let mut x = &mut 1u32;
+    //~^ mut_mut
+    {
+        let mut y = &mut *x;
+        //~^ mut_mut
+    }
+
+    {
+        let y: &mut u32 = &mut 2;
+        //~^ mut_mut
+        //~| mut_mut
+    }
+
+    {
+        let y: &mut u32 = &mut 2;
+        //~^ mut_mut
+        //~| mut_mut
+    }
+
+    let mut z = inline!(&mut $(&mut 3u32));
+}
+
+fn issue939() {
+    let array = [5, 6, 7, 8, 9];
+    let mut args = array.iter().skip(2);
+    for &arg in &mut args {
+        println!("{}", arg);
+    }
+
+    let args = &mut args;
+    for arg in args {
+        println!(":{}", arg);
+    }
+}
+
+fn issue6922() {
+    // do not lint from an external macro
+    external!(let mut_mut_ty: &mut &mut u32 = &mut &mut 1u32;);
+}
+
+mod issue9035 {
+    use std::fmt::Display;
+
+    struct Foo<'a> {
+        inner: &'a mut dyn Display,
+    }
+
+    impl Foo<'_> {
+        fn foo(&mut self) {
+            let hlp = &mut self.inner;
+            bar(hlp);
+        }
+    }
+
+    fn bar(_: &mut impl Display) {}
+}
+
+fn allow_works() {
+    #[allow(clippy::mut_mut)]
+    let _ = &mut &mut 1;
+}

tests/ui/mut_mut.rs

@@ -12,9 +12,8 @@
 extern crate proc_macros;
 use proc_macros::{external, inline_macros};
 
-fn fun(x: &mut &mut u32) -> bool {
+fn fun(x: &mut &mut u32) {
     //~^ mut_mut
-    **x > 0
 }
 
 fn less_fun(x: *mut *mut u32) {
@@ -37,23 +36,19 @@ fn main() {
         //~^ mut_mut
     }
 
-    if fun(x) {
+    {
         let y: &mut &mut u32 = &mut &mut 2;
         //~^ mut_mut
         //~| mut_mut
-        **y + **x;
     }
 
-    if fun(x) {
+    {
         let y: &mut &mut &mut u32 = &mut &mut &mut 2;
         //~^ mut_mut
         //~| mut_mut
-        //~| mut_mut
-        ***y + **x;
     }
 
     let mut z = inline!(&mut $(&mut 3u32));
-    //~^ mut_mut
 }
 
 fn issue939() {