overhaul mut_mut (#15417)

This PR:
- adds structured suggestions to all 3 lint scenarios
- adds more examples of linted patterns, and recommended fixes
- moves some test cases to `_unfixable.rs`, to allow running rustfix on
the main file
- stops the lint from firing multiple times on types like `&mut &mut
&mut T`

Open questions:
- I'd like to add a note explaining why chained `&mut` are useless.. but
can't really come up with something consice. I very much don't like the
one in the docs -- it's a bit too condescending imo.
- see comments in the diff for more

I do realize that the PR ended up being quite wide-scoped, but that's
primarily because once I added structured suggestions to one of the lint
cases, `ui_test` started complaining about warnings remaining after the
rustfix run, which of course had to with the fact that the other two
lint cases didn't actually fix the code they linted, only warned. I
_guess_ `ui_test` could be smarter about this, by understanding that if
a warning was created without a suggestion, then that warning will still
remain in the fixed file. But oh well.

changelog: [`mut_mut`]: add structured suggestions, improve docs

fixes #13021

Author: blyxyas

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() {