Consider nested lifetimes in mut_from_ref (#14471)

fixes #7749.
This issue proposes searching for `DerefMut` impls, which is not done
here: every lifetime parameter (aka `<'a>`) is the input types is
considered to be potentially mutable, and thus deactivates the lint.

changelog: [`mut_from_ref`]: Fixes false positive, where lifetimes
nested in the type (e.g. `Box<&'a mut T>`) were not considered.

Author: blyxyas

clippy_lints/src/ptr.rs

@@ -498,29 +498,33 @@ fn check_fn_args<'cx, 'tcx: 'cx>(
 }
 
 fn check_mut_from_ref<'tcx>(cx: &LateContext<'tcx>, sig: &FnSig<'_>, body: Option<&Body<'tcx>>) {
-    if let FnRetTy::Return(ty) = sig.decl.output
-        && let Some((out, Mutability::Mut, _)) = get_ref_lm(ty)
-    {
+    let FnRetTy::Return(ty) = sig.decl.output else { return };
+    for (out, mutability, out_span) in get_lifetimes(ty) {
+        if mutability != Some(Mutability::Mut) {
+            continue;
+        }
         let out_region = cx.tcx.named_bound_var(out.hir_id);
-        let args: Option<Vec<_>> = sig
+        // `None` if one of the types contains `&'a mut T` or `T<'a>`.
+        // Else, contains all the locations of `&'a T` types.
+        let args_immut_refs: Option<Vec<Span>> = sig
             .decl
             .inputs
             .iter()
-            .filter_map(get_ref_lm)
+            .flat_map(get_lifetimes)
             .filter(|&(lt, _, _)| cx.tcx.named_bound_var(lt.hir_id) == out_region)
-            .map(|(_, mutability, span)| (mutability == Mutability::Not).then_some(span))
+            .map(|(_, mutability, span)| (mutability == Some(Mutability::Not)).then_some(span))
             .collect();
-        if let Some(args) = args
-            && !args.is_empty()
+        if let Some(args_immut_refs) = args_immut_refs
+            && !args_immut_refs.is_empty()
             && body.is_none_or(|body| sig.header.is_unsafe() || contains_unsafe_block(cx, body.value))
         {
             span_lint_and_then(
                 cx,
                 MUT_FROM_REF,
-                ty.span,
+                out_span,
                 "mutable borrow from immutable input(s)",
                 |diag| {
-                    let ms = MultiSpan::from_spans(args);
+                    let ms = MultiSpan::from_spans(args_immut_refs);
                     diag.span_note(ms, "immutable borrow here");
                 },
             );
@@ -686,12 +690,36 @@ fn matches_preds<'tcx>(
         })
 }
 
-fn get_ref_lm<'tcx>(ty: &'tcx hir::Ty<'tcx>) -> Option<(&'tcx Lifetime, Mutability, Span)> {
-    if let TyKind::Ref(lt, ref m) = ty.kind {
-        Some((lt, m.mutbl, ty.span))
-    } else {
-        None
+struct LifetimeVisitor<'tcx> {
+    result: Vec<(&'tcx Lifetime, Option<Mutability>, Span)>,
+}
+
+impl<'tcx> Visitor<'tcx> for LifetimeVisitor<'tcx> {
+    fn visit_ty(&mut self, ty: &'tcx hir::Ty<'tcx, hir::AmbigArg>) {
+        if let TyKind::Ref(lt, ref m) = ty.kind {
+            self.result.push((lt, Some(m.mutbl), ty.span));
+        }
+        hir::intravisit::walk_ty(self, ty);
     }
+
+    fn visit_generic_arg(&mut self, generic_arg: &'tcx GenericArg<'tcx>) {
+        if let GenericArg::Lifetime(lt) = generic_arg {
+            self.result.push((lt, None, generic_arg.span()));
+        }
+        hir::intravisit::walk_generic_arg(self, generic_arg);
+    }
+}
+
+/// Visit `ty` and collect the all the lifetimes appearing in it, implicit or not.
+///
+/// The second field of the vector's elements indicate if the lifetime is attached to a
+/// shared reference, a mutable reference, or neither.
+fn get_lifetimes<'tcx>(ty: &'tcx hir::Ty<'tcx>) -> Vec<(&'tcx Lifetime, Option<Mutability>, Span)> {
+    use hir::intravisit::VisitorExt as _;
+
+    let mut visitor = LifetimeVisitor { result: Vec::new() };
+    visitor.visit_ty_unambig(ty);
+    visitor.result
 }
 
 fn is_null_path(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool {

tests/ui/mut_from_ref.rs

@@ -1,4 +1,10 @@
-#![allow(unused, clippy::needless_lifetimes, clippy::needless_pass_by_ref_mut)]
+#![allow(
+    unused,
+    clippy::needless_lifetimes,
+    clippy::needless_pass_by_ref_mut,
+    clippy::redundant_allocation,
+    clippy::boxed_local
+)]
 #![warn(clippy::mut_from_ref)]
 
 struct Foo;
@@ -40,6 +46,18 @@ fn fail_double<'a, 'b>(x: &'a u32, y: &'a u32, z: &'b mut u32) -> &'a mut u32 {
     unsafe { unimplemented!() }
 }
 
+fn fail_tuples<'a>(x: (&'a u32, &'a u32)) -> &'a mut u32 {
+    //~^ mut_from_ref
+
+    unsafe { unimplemented!() }
+}
+
+fn fail_box<'a>(x: Box<&'a u32>) -> &'a mut u32 {
+    //~^ mut_from_ref
+
+    unsafe { unimplemented!() }
+}
+
 // this is OK, because the result borrows y
 fn works<'a>(x: &u32, y: &'a mut u32) -> &'a mut u32 {
     unsafe { unimplemented!() }
@@ -50,6 +68,20 @@ fn also_works<'a>(x: &'a u32, y: &'a mut u32) -> &'a mut u32 {
     unsafe { unimplemented!() }
 }
 
+fn works_tuples<'a>(x: (&'a u32, &'a mut u32)) -> &'a mut u32 {
+    unsafe { unimplemented!() }
+}
+
+fn works_box<'a>(x: &'a u32, y: Box<&'a mut u32>) -> &'a mut u32 {
+    unsafe { unimplemented!() }
+}
+
+struct RefMut<'a>(&'a mut u32);
+
+fn works_parameter<'a>(x: &'a u32, y: RefMut<'a>) -> &'a mut u32 {
+    unsafe { unimplemented!() }
+}
+
 unsafe fn also_broken(x: &u32) -> &mut u32 {
     //~^ mut_from_ref
 

tests/ui/mut_from_ref.stderr

@@ -1,76 +1,100 @@
 error: mutable borrow from immutable input(s)
-  --> tests/ui/mut_from_ref.rs:7:39
+  --> tests/ui/mut_from_ref.rs:13:39
    |
 LL |     fn this_wont_hurt_a_bit(&self) -> &mut Foo {
    |                                       ^^^^^^^^
    |
 note: immutable borrow here
-  --> tests/ui/mut_from_ref.rs:7:29
+  --> tests/ui/mut_from_ref.rs:13:29
    |
 LL |     fn this_wont_hurt_a_bit(&self) -> &mut Foo {
    |                             ^^^^^
    = note: `-D clippy::mut-from-ref` implied by `-D warnings`
    = help: to override `-D warnings` add `#[allow(clippy::mut_from_ref)]`
 
 error: mutable borrow from immutable input(s)
-  --> tests/ui/mut_from_ref.rs:15:25
+  --> tests/ui/mut_from_ref.rs:21:25
    |
 LL |     fn ouch(x: &Foo) -> &mut Foo;
    |                         ^^^^^^^^
    |
 note: immutable borrow here
-  --> tests/ui/mut_from_ref.rs:15:16
+  --> tests/ui/mut_from_ref.rs:21:16
    |
 LL |     fn ouch(x: &Foo) -> &mut Foo;
    |                ^^^^
 
 error: mutable borrow from immutable input(s)
-  --> tests/ui/mut_from_ref.rs:25:21
+  --> tests/ui/mut_from_ref.rs:31:21
    |
 LL | fn fail(x: &u32) -> &mut u16 {
    |                     ^^^^^^^^
    |
 note: immutable borrow here
-  --> tests/ui/mut_from_ref.rs:25:12
+  --> tests/ui/mut_from_ref.rs:31:12
    |
 LL | fn fail(x: &u32) -> &mut u16 {
    |            ^^^^
 
 error: mutable borrow from immutable input(s)
-  --> tests/ui/mut_from_ref.rs:31:50
+  --> tests/ui/mut_from_ref.rs:37:50
    |
 LL | fn fail_lifetime<'a>(x: &'a u32, y: &mut u32) -> &'a mut u32 {
    |                                                  ^^^^^^^^^^^
    |
 note: immutable borrow here
-  --> tests/ui/mut_from_ref.rs:31:25
+  --> tests/ui/mut_from_ref.rs:37:25
    |
 LL | fn fail_lifetime<'a>(x: &'a u32, y: &mut u32) -> &'a mut u32 {
    |                         ^^^^^^^
 
 error: mutable borrow from immutable input(s)
-  --> tests/ui/mut_from_ref.rs:37:67
+  --> tests/ui/mut_from_ref.rs:43:67
    |
 LL | fn fail_double<'a, 'b>(x: &'a u32, y: &'a u32, z: &'b mut u32) -> &'a mut u32 {
    |                                                                   ^^^^^^^^^^^
    |
 note: immutable borrow here
-  --> tests/ui/mut_from_ref.rs:37:27
+  --> tests/ui/mut_from_ref.rs:43:27
    |
 LL | fn fail_double<'a, 'b>(x: &'a u32, y: &'a u32, z: &'b mut u32) -> &'a mut u32 {
    |                           ^^^^^^^     ^^^^^^^
 
 error: mutable borrow from immutable input(s)
-  --> tests/ui/mut_from_ref.rs:53:35
+  --> tests/ui/mut_from_ref.rs:49:46
+   |
+LL | fn fail_tuples<'a>(x: (&'a u32, &'a u32)) -> &'a mut u32 {
+   |                                              ^^^^^^^^^^^
+   |
+note: immutable borrow here
+  --> tests/ui/mut_from_ref.rs:49:24
+   |
+LL | fn fail_tuples<'a>(x: (&'a u32, &'a u32)) -> &'a mut u32 {
+   |                        ^^^^^^^  ^^^^^^^
+
+error: mutable borrow from immutable input(s)
+  --> tests/ui/mut_from_ref.rs:55:37
+   |
+LL | fn fail_box<'a>(x: Box<&'a u32>) -> &'a mut u32 {
+   |                                     ^^^^^^^^^^^
+   |
+note: immutable borrow here
+  --> tests/ui/mut_from_ref.rs:55:24
+   |
+LL | fn fail_box<'a>(x: Box<&'a u32>) -> &'a mut u32 {
+   |                        ^^^^^^^
+
+error: mutable borrow from immutable input(s)
+  --> tests/ui/mut_from_ref.rs:85:35
    |
 LL | unsafe fn also_broken(x: &u32) -> &mut u32 {
    |                                   ^^^^^^^^
    |
 note: immutable borrow here
-  --> tests/ui/mut_from_ref.rs:53:26
+  --> tests/ui/mut_from_ref.rs:85:26
    |
 LL | unsafe fn also_broken(x: &u32) -> &mut u32 {
    |                          ^^^^
 
-error: aborting due to 6 previous errors
+error: aborting due to 8 previous errors