Auto merge of rust-lang#11184 - GuillaumeGomez:needless_pass_by_ref_mut-async, r=llogiq

bors · bors · commit 43577d58f96d · 2023-07-23T07:21:05.000Z
Fix async functions handling for `needless_pass_by_ref_mut` lint Fixes rust-lang/rust-clippy#11179. The problem with async is that "internals" are actually inside a closure from the `ExprUseVisitor` point of view, meaning we need to actually run the check on the closures' body as well. changelog: none r? `@llogiq`
diff --git a/clippy_lints/src/needless_pass_by_ref_mut.rs b/clippy_lints/src/needless_pass_by_ref_mut.rs
@@ -3,14 +3,16 @@ use clippy_utils::diagnostics::span_lint_and_then;
 use clippy_utils::source::snippet;
 use clippy_utils::{is_from_proc_macro, is_self};
 use if_chain::if_chain;
+use rustc_data_structures::fx::FxHashSet;
 use rustc_errors::Applicability;
 use rustc_hir::intravisit::FnKind;
 use rustc_hir::{Body, FnDecl, HirId, HirIdMap, HirIdSet, Impl, ItemKind, Mutability, Node, PatKind};
 use rustc_hir_typeck::expr_use_visitor as euv;
 use rustc_infer::infer::TyCtxtInferExt;
 use rustc_lint::{LateContext, LateLintPass};
+use rustc_middle::hir::map::associated_body;
 use rustc_middle::mir::FakeReadCause;
-use rustc_middle::ty::{self, Ty};
+use rustc_middle::ty::{self, Ty, UpvarId, UpvarPath};
 use rustc_session::{declare_tool_lint, impl_lint_pass};
 use rustc_span::def_id::LocalDefId;
 use rustc_span::symbol::kw;
@@ -102,17 +104,17 @@ impl<'tcx> LateLintPass<'tcx> for NeedlessPassByRefMut {
         }
 
         let hir_id = cx.tcx.hir().local_def_id_to_hir_id(fn_def_id);
-
-        match kind {
+        let is_async = match kind {
             FnKind::ItemFn(.., header) => {
                 let attrs = cx.tcx.hir().attrs(hir_id);
                 if header.abi != Abi::Rust || requires_exact_signature(attrs) {
                     return;
                 }
+                header.is_async()
             },
-            FnKind::Method(..) => (),
+            FnKind::Method(.., sig) => sig.header.is_async(),
             FnKind::Closure => return,
-        }
+        };
 
         // Exclude non-inherent impls
         if let Some(Node::Item(item)) = cx.tcx.hir().find_parent(hir_id) {
@@ -128,13 +130,14 @@ impl<'tcx> LateLintPass<'tcx> for NeedlessPassByRefMut {
         let fn_sig = cx.tcx.liberate_late_bound_regions(fn_def_id.to_def_id(), fn_sig);
 
         // If there are no `&mut` argument, no need to go any further.
-        if !decl
+        let mut it = decl
             .inputs
             .iter()
             .zip(fn_sig.inputs())
             .zip(body.params)
-            .any(|((&input, &ty), arg)| !should_skip(cx, input, ty, arg))
-        {
+            .filter(|((&input, &ty), arg)| !should_skip(cx, input, ty, arg))
+            .peekable();
+        if it.peek().is_none() {
             return;
         }
 
@@ -144,19 +147,25 @@ impl<'tcx> LateLintPass<'tcx> for NeedlessPassByRefMut {
             let mut ctx = MutablyUsedVariablesCtxt::default();
             let infcx = cx.tcx.infer_ctxt().build();
             euv::ExprUseVisitor::new(&mut ctx, &infcx, fn_def_id, cx.param_env, cx.typeck_results()).consume_body(body);
+            if is_async {
+                let closures = ctx.async_closures.clone();
+                let hir = cx.tcx.hir();
+                for closure in closures {
+                    ctx.prev_bind = None;
+                    ctx.prev_move_to_closure.clear();
+                    if let Some(body) = hir
+                        .find_by_def_id(closure)
+                        .and_then(associated_body)
+                        .map(|(_, body_id)| hir.body(body_id))
+                    {
+                        euv::ExprUseVisitor::new(&mut ctx, &infcx, closure, cx.param_env, cx.typeck_results())
+                            .consume_body(body);
+                    }
+                }
+            }
             ctx
         };
 
-        let mut it = decl
-            .inputs
-            .iter()
-            .zip(fn_sig.inputs())
-            .zip(body.params)
-            .filter(|((&input, &ty), arg)| !should_skip(cx, input, ty, arg))
-            .peekable();
-        if it.peek().is_none() {
-            return;
-        }
         let show_semver_warning = self.avoid_breaking_exported_api && cx.effective_visibilities.is_exported(fn_def_id);
         for ((&input, &_), arg) in it {
             // Only take `&mut` arguments.
@@ -197,7 +206,9 @@ impl<'tcx> LateLintPass<'tcx> for NeedlessPassByRefMut {
 struct MutablyUsedVariablesCtxt {
     mutably_used_vars: HirIdSet,
     prev_bind: Option<HirId>,
+    prev_move_to_closure: HirIdSet,
     aliases: HirIdMap<HirId>,
+    async_closures: FxHashSet<LocalDefId>,
 }
 
 impl MutablyUsedVariablesCtxt {
@@ -213,16 +224,27 @@ impl MutablyUsedVariablesCtxt {
 impl<'tcx> euv::Delegate<'tcx> for MutablyUsedVariablesCtxt {
     fn consume(&mut self, cmt: &euv::PlaceWithHirId<'tcx>, _id: HirId) {
         if let euv::Place {
-            base: euv::PlaceBase::Local(vid),
+            base:
+                euv::PlaceBase::Local(vid)
+                | euv::PlaceBase::Upvar(UpvarId {
+                    var_path: UpvarPath { hir_id: vid },
+                    ..
+                }),
             base_ty,
             ..
         } = &cmt.place
         {
             if let Some(bind_id) = self.prev_bind.take() {
-                self.aliases.insert(bind_id, *vid);
-            } else if matches!(base_ty.ref_mutability(), Some(Mutability::Mut)) {
+                if bind_id != *vid {
+                    self.aliases.insert(bind_id, *vid);
+                }
+            } else if !self.prev_move_to_closure.contains(vid)
+                && matches!(base_ty.ref_mutability(), Some(Mutability::Mut))
+            {
                 self.add_mutably_used_var(*vid);
             }
+            self.prev_bind = None;
+            self.prev_move_to_closure.remove(vid);
         }
     }
 
@@ -265,7 +287,30 @@ impl<'tcx> euv::Delegate<'tcx> for MutablyUsedVariablesCtxt {
         self.prev_bind = None;
     }
 
-    fn fake_read(&mut self, _: &rustc_hir_typeck::expr_use_visitor::PlaceWithHirId<'tcx>, _: FakeReadCause, _: HirId) {}
+    fn fake_read(
+        &mut self,
+        cmt: &rustc_hir_typeck::expr_use_visitor::PlaceWithHirId<'tcx>,
+        cause: FakeReadCause,
+        _id: HirId,
+    ) {
+        if let euv::Place {
+            base:
+                euv::PlaceBase::Upvar(UpvarId {
+                    var_path: UpvarPath { hir_id: vid },
+                    ..
+                }),
+            ..
+        } = &cmt.place
+        {
+            if let FakeReadCause::ForLet(Some(inner)) = cause {
+                // Seems like we are inside an async function. We need to store the closure `DefId`
+                // to go through it afterwards.
+                self.async_closures.insert(inner);
+                self.aliases.insert(cmt.hir_id, *vid);
+                self.prev_move_to_closure.insert(*vid);
+            }
+        }
+    }
 
     fn bind(&mut self, _cmt: &euv::PlaceWithHirId<'tcx>, id: HirId) {
         self.prev_bind = Some(id);
diff --git a/tests/ui/needless_pass_by_ref_mut.rs b/tests/ui/needless_pass_by_ref_mut.rs
@@ -91,15 +91,79 @@ impl<T> Mut<T> {
 // Should not warn.
 fn unused(_: &mut u32, _b: &mut u8) {}
 
+// Should not warn.
+async fn f1(x: &mut i32) {
+    *x += 1;
+}
+// Should not warn.
+async fn f2(x: &mut i32, y: String) {
+    *x += 1;
+}
+// Should not warn.
+async fn f3(x: &mut i32, y: String, z: String) {
+    *x += 1;
+}
+// Should not warn.
+async fn f4(x: &mut i32, y: i32) {
+    *x += 1;
+}
+// Should not warn.
+async fn f5(x: i32, y: &mut i32) {
+    *y += 1;
+}
+// Should not warn.
+async fn f6(x: i32, y: &mut i32, z: &mut i32) {
+    *y += 1;
+    *z += 1;
+}
+// Should not warn.
+async fn f7(x: &mut i32, y: i32, z: &mut i32, a: i32) {
+    *x += 1;
+    *z += 1;
+}
+
+// Should warn.
+async fn a1(x: &mut i32) {
+    println!("{:?}", x);
+}
+// Should warn.
+async fn a2(x: &mut i32, y: String) {
+    println!("{:?}", x);
+}
+// Should warn.
+async fn a3(x: &mut i32, y: String, z: String) {
+    println!("{:?}", x);
+}
+// Should warn.
+async fn a4(x: &mut i32, y: i32) {
+    println!("{:?}", x);
+}
+// Should warn.
+async fn a5(x: i32, y: &mut i32) {
+    println!("{:?}", x);
+}
+// Should warn.
+async fn a6(x: i32, y: &mut i32) {
+    println!("{:?}", x);
+}
+// Should warn.
+async fn a7(x: i32, y: i32, z: &mut i32) {
+    println!("{:?}", z);
+}
+// Should warn.
+async fn a8(x: i32, a: &mut i32, y: i32, z: &mut i32) {
+    println!("{:?}", z);
+}
+
 fn main() {
-    let mut u = 0;
-    let mut v = vec![0];
-    foo(&mut v, &0, &mut u);
-    foo2(&mut v);
-    foo3(&mut v);
-    foo4(&mut v);
-    foo5(&mut v);
-    alias_check(&mut v);
-    alias_check2(&mut v);
-    println!("{u}");
+    // let mut u = 0;
+    // let mut v = vec![0];
+    // foo(&mut v, &0, &mut u);
+    // foo2(&mut v);
+    // foo3(&mut v);
+    // foo4(&mut v);
+    // foo5(&mut v);
+    // alias_check(&mut v);
+    // alias_check2(&mut v);
+    // println!("{u}");
 }
diff --git a/tests/ui/needless_pass_by_ref_mut.stderr b/tests/ui/needless_pass_by_ref_mut.stderr
@@ -24,5 +24,59 @@ error: this argument is a mutable reference, but not used mutably
 LL |     fn badger(&mut self, vec: &mut Vec<i32>) -> usize {
    |                               ^^^^^^^^^^^^^ help: consider changing to: `&Vec<i32>`
 
-error: aborting due to 4 previous errors
+error: this argument is a mutable reference, but not used mutably
+  --> $DIR/needless_pass_by_ref_mut.rs:126:16
+   |
+LL | async fn a1(x: &mut i32) {
+   |                ^^^^^^^^ help: consider changing to: `&i32`
+
+error: this argument is a mutable reference, but not used mutably
+  --> $DIR/needless_pass_by_ref_mut.rs:130:16
+   |
+LL | async fn a2(x: &mut i32, y: String) {
+   |                ^^^^^^^^ help: consider changing to: `&i32`
+
+error: this argument is a mutable reference, but not used mutably
+  --> $DIR/needless_pass_by_ref_mut.rs:134:16
+   |
+LL | async fn a3(x: &mut i32, y: String, z: String) {
+   |                ^^^^^^^^ help: consider changing to: `&i32`
+
+error: this argument is a mutable reference, but not used mutably
+  --> $DIR/needless_pass_by_ref_mut.rs:138:16
+   |
+LL | async fn a4(x: &mut i32, y: i32) {
+   |                ^^^^^^^^ help: consider changing to: `&i32`
+
+error: this argument is a mutable reference, but not used mutably
+  --> $DIR/needless_pass_by_ref_mut.rs:142:24
+   |
+LL | async fn a5(x: i32, y: &mut i32) {
+   |                        ^^^^^^^^ help: consider changing to: `&i32`
+
+error: this argument is a mutable reference, but not used mutably
+  --> $DIR/needless_pass_by_ref_mut.rs:146:24
+   |
+LL | async fn a6(x: i32, y: &mut i32) {
+   |                        ^^^^^^^^ help: consider changing to: `&i32`
+
+error: this argument is a mutable reference, but not used mutably
+  --> $DIR/needless_pass_by_ref_mut.rs:150:32
+   |
+LL | async fn a7(x: i32, y: i32, z: &mut i32) {
+   |                                ^^^^^^^^ help: consider changing to: `&i32`
+
+error: this argument is a mutable reference, but not used mutably
+  --> $DIR/needless_pass_by_ref_mut.rs:154:24
+   |
+LL | async fn a8(x: i32, a: &mut i32, y: i32, z: &mut i32) {
+   |                        ^^^^^^^^ help: consider changing to: `&i32`
+
+error: this argument is a mutable reference, but not used mutably
+  --> $DIR/needless_pass_by_ref_mut.rs:154:45
+   |
+LL | async fn a8(x: i32, a: &mut i32, y: i32, z: &mut i32) {
+   |                                             ^^^^^^^^ help: consider changing to: `&i32`
+
+error: aborting due to 13 previous errors
 
