Auto merge of #11492 - GuillaumeGomez:async-fn-returned-closure, r=Centri3

Fix mutaby used async function argument in closure for `needless_pass_by_ref_mut`

Fixes https://github.com/rust-lang/rust-clippy/issues/11380.

The problem was that it needed to go through closures as well in async functions to correctly find the mutable usage of async function arguments.

changelog: Correctly handle mutable usage of async function arguments in closures.

r? `@Centri3`

3 files changed, 98 insertions, 20 deletions

diff --git a/clippy_lints/src/needless_pass_by_ref_mut.rs b/clippy_lints/src/needless_pass_by_ref_mut.rs
index 7b00eabf97b..36ca61c0b44 100644
--- a/clippy_lints/src/needless_pass_by_ref_mut.rs
+++ b/clippy_lints/src/needless_pass_by_ref_mut.rs
@@ -1,6 +1,7 @@
 use super::needless_pass_by_value::requires_exact_signature;
 use clippy_utils::diagnostics::span_lint_hir_and_then;
 use clippy_utils::source::snippet;
+use clippy_utils::visitors::for_each_expr_with_closures;
 use clippy_utils::{get_parent_node, inherits_cfg, is_from_proc_macro, is_self};
 use rustc_data_structures::fx::{FxHashSet, FxIndexMap};
 use rustc_errors::Applicability;
@@ -9,7 +10,7 @@ use rustc_hir::{
     Body, Closure, Expr, ExprKind, FnDecl, HirId, HirIdMap, HirIdSet, Impl, ItemKind, Mutability, Node, PatKind, QPath,
 };
 use rustc_hir_typeck::expr_use_visitor as euv;
-use rustc_infer::infer::TyCtxtInferExt;
+use rustc_infer::infer::{InferCtxt, TyCtxtInferExt};
 use rustc_lint::{LateContext, LateLintPass};
 use rustc_middle::hir::map::associated_body;
 use rustc_middle::hir::nested_filter::OnlyBodies;
@@ -21,6 +22,8 @@ use rustc_span::symbol::kw;
 use rustc_span::Span;
 use rustc_target::spec::abi::Abi;
 
+use core::ops::ControlFlow;
+
 declare_clippy_lint! {
     /// ### What it does
     /// Check if a `&mut` function argument is actually used mutably.
@@ -95,6 +98,30 @@ fn should_skip<'tcx>(
     is_from_proc_macro(cx, &input)
 }
 
+fn check_closures<'tcx>(
+    ctx: &mut MutablyUsedVariablesCtxt<'tcx>,
+    cx: &LateContext<'tcx>,
+    infcx: &InferCtxt<'tcx>,
+    checked_closures: &mut FxHashSet<LocalDefId>,
+    closures: FxHashSet<LocalDefId>,
+) {
+    let hir = cx.tcx.hir();
+    for closure in closures {
+        if !checked_closures.insert(closure) {
+            continue;
+        }
+        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(ctx, infcx, closure, cx.param_env, cx.typeck_results()).consume_body(body);
+        }
+    }
+}
+
 impl<'tcx> LateLintPass<'tcx> for NeedlessPassByRefMut<'tcx> {
     fn check_fn(
         &mut self,
@@ -161,25 +188,22 @@ impl<'tcx> LateLintPass<'tcx> for NeedlessPassByRefMut<'tcx> {
             euv::ExprUseVisitor::new(&mut ctx, &infcx, fn_def_id, cx.param_env, cx.typeck_results()).consume_body(body);
             if is_async {
                 let mut checked_closures = FxHashSet::default();
+
+                // We retrieve all the closures declared in the async function because they will
+                // not be found by `euv::Delegate`.
+                let mut closures: FxHashSet<LocalDefId> = FxHashSet::default();
+                for_each_expr_with_closures(cx, body, |expr| {
+                    if let ExprKind::Closure(closure) = expr.kind {
+                        closures.insert(closure.def_id);
+                    }
+                    ControlFlow::<()>::Continue(())
+                });
+                check_closures(&mut ctx, cx, &infcx, &mut checked_closures, closures);
+
                 while !ctx.async_closures.is_empty() {
-                    let closures = ctx.async_closures.clone();
+                    let async_closures = ctx.async_closures.clone();
                     ctx.async_closures.clear();
-                    let hir = cx.tcx.hir();
-                    for closure in closures {
-                        if !checked_closures.insert(closure) {
-                            continue;
-                        }
-                        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);
-                        }
-                    }
+                    check_closures(&mut ctx, cx, &infcx, &mut checked_closures, async_closures);
                 }
             }
             ctx
@@ -244,6 +268,10 @@ impl<'tcx> LateLintPass<'tcx> for NeedlessPassByRefMut<'tcx> {
 struct MutablyUsedVariablesCtxt<'tcx> {
     mutably_used_vars: HirIdSet,
     prev_bind: Option<HirId>,
+    /// In async functions, the inner AST is composed of multiple layers until we reach the code
+    /// defined by the user. Because of that, some variables are marked as mutably borrowed even
+    /// though they're not. This field lists the `HirId` that should not be considered as mutable
+    /// use of a variable.
     prev_move_to_closure: HirIdSet,
     aliases: HirIdMap<HirId>,
     async_closures: FxHashSet<LocalDefId>,
diff --git a/tests/ui/needless_pass_by_ref_mut.rs b/tests/ui/needless_pass_by_ref_mut.rs
index e1e5e8fd220..da2a72caacb 100644
--- a/tests/ui/needless_pass_by_ref_mut.rs
+++ b/tests/ui/needless_pass_by_ref_mut.rs
@@ -1,4 +1,4 @@
-#![allow(clippy::if_same_then_else, clippy::no_effect)]
+#![allow(clippy::if_same_then_else, clippy::no_effect, clippy::redundant_closure_call)]
 #![feature(lint_reasons)]
 //@no-rustfix
 use std::ptr::NonNull;
@@ -231,6 +231,32 @@ async fn async_vec2(b: &mut Vec<bool>) {
     b.push(true);
 }
 
+// Should not warn.
+pub async fn closure(n: &mut usize) -> impl '_ + FnMut() {
+    || {
+        *n += 1;
+    }
+}
+
+// Should warn.
+pub fn closure2(n: &mut usize) -> impl '_ + FnMut() -> usize {
+    //~^ ERROR: this argument is a mutable reference, but not used mutably
+    || *n + 1
+}
+
+// Should not warn.
+pub async fn closure3(n: &mut usize) {
+    (|| *n += 1)();
+}
+
+// Should warn.
+pub async fn closure4(n: &mut usize) {
+    //~^ ERROR: this argument is a mutable reference, but not used mutably
+    (|| {
+        let _x = *n + 1;
+    })();
+}
+
 fn main() {
     let mut u = 0;
     let mut v = vec![0];
diff --git a/tests/ui/needless_pass_by_ref_mut.stderr b/tests/ui/needless_pass_by_ref_mut.stderr
index df3df045776..0fb9458d794 100644
--- a/tests/ui/needless_pass_by_ref_mut.stderr
+++ b/tests/ui/needless_pass_by_ref_mut.stderr
@@ -107,5 +107,29 @@ error: this argument is a mutable reference, but not used mutably
 LL | async fn inner_async3(x: &mut i32, y: &mut u32) {
    |                          ^^^^^^^^ help: consider changing to: `&i32`
 
-error: aborting due to 17 previous errors
+error: this argument is a mutable reference, but not used mutably
+  --> $DIR/needless_pass_by_ref_mut.rs:235:25
+   |
+LL | pub async fn closure(n: &mut usize) -> impl '_ + FnMut() {
+   |                         ^^^^^^^^^^ help: consider changing to: `&usize`
+   |
+   = warning: changing this function will impact semver compatibility
+
+error: this argument is a mutable reference, but not used mutably
+  --> $DIR/needless_pass_by_ref_mut.rs:242:20
+   |
+LL | pub fn closure2(n: &mut usize) -> impl '_ + FnMut() -> usize {
+   |                    ^^^^^^^^^^ help: consider changing to: `&usize`
+   |
+   = warning: changing this function will impact semver compatibility
+
+error: this argument is a mutable reference, but not used mutably
+  --> $DIR/needless_pass_by_ref_mut.rs:253:26
+   |
+LL | pub async fn closure4(n: &mut usize) {
+   |                          ^^^^^^^^^^ help: consider changing to: `&usize`
+   |
+   = warning: changing this function will impact semver compatibility
+
+error: aborting due to 20 previous errors