Auto merge of rust-lang#10490 - samueltardieu:issue-10482, r=xFrednet

Do not propose to remove `async move` if variables are captured by ref

Fixes rust-lang#10482

changelog: FP [`redundant_async_block`] Do not propose to remove `async move` if variables are captured by ref

Author: bors

clippy_lints/src/redundant_async_block.rs

@@ -32,7 +32,7 @@ declare_clippy_lint! {
     /// ```
     #[clippy::version = "1.69.0"]
     pub REDUNDANT_ASYNC_BLOCK,
-    complexity,
+    nursery,
     "`async { future.await }` can be replaced by `future`"
 }
 declare_lint_pass!(RedundantAsyncBlock => [REDUNDANT_ASYNC_BLOCK]);
@@ -48,6 +48,11 @@ impl EarlyLintPass for RedundantAsyncBlock {
             !future.span.from_expansion() &&
             !await_in_expr(future)
         {
+            if captures_value(last) {
+                // If the async block captures variables then there is no equivalence.
+                return;
+            }
+
             span_lint_and_sugg(
                 cx,
                 REDUNDANT_ASYNC_BLOCK,
@@ -82,3 +87,33 @@ impl<'ast> AstVisitor<'ast> for AwaitDetector {
         }
     }
 }
+
+/// Check whether an expression may have captured a local variable.
+/// This is done by looking for paths with only one segment, except as
+/// a prefix of `.await` since this would be captured by value.
+///
+/// This function will sometimes return `true` even tough there are no
+/// captures happening: at the AST level, it is impossible to
+/// dinstinguish a function call from a call to a closure which comes
+/// from the local environment.
+fn captures_value(expr: &Expr) -> bool {
+    let mut detector = CaptureDetector::default();
+    detector.visit_expr(expr);
+    detector.capture_found
+}
+
+#[derive(Default)]
+struct CaptureDetector {
+    capture_found: bool,
+}
+
+impl<'ast> AstVisitor<'ast> for CaptureDetector {
+    fn visit_expr(&mut self, ex: &'ast Expr) {
+        match (&ex.kind, self.capture_found) {
+            (ExprKind::Await(fut), _) if matches!(fut.kind, ExprKind::Path(..)) => (),
+            (ExprKind::Path(_, path), _) if path.segments.len() == 1 => self.capture_found = true,
+            (_, false) => rustc_ast::visit::walk_expr(self, ex),
+            _ => (),
+        }
+    }
+}

tests/ui/redundant_async_block.fixed

@@ -3,6 +3,8 @@
 #![allow(unused)]
 #![warn(clippy::redundant_async_block)]
 
+use std::future::Future;
+
 async fn func1(n: usize) -> usize {
     n + 1
 }
@@ -62,3 +64,48 @@ fn main() {
     let fut = async_await_parameter_in_macro!(func2());
     let fut = async_await_in_macro!(std::convert::identity);
 }
+
+#[allow(clippy::let_and_return)]
+fn capture_local() -> impl Future<Output = i32> {
+    // Lint
+    let fut = async { 17 };
+    fut
+}
+
+fn capture_local_closure(s: &str) -> impl Future<Output = &str> {
+    let f = move || std::future::ready(s);
+    // Do not lint: `f` would not live long enough
+    async move { f().await }
+}
+
+#[allow(clippy::let_and_return)]
+fn capture_arg(s: &str) -> impl Future<Output = &str> {
+    // Lint
+    let fut = async move { s };
+    fut
+}
+
+#[derive(Debug, Clone)]
+struct F {}
+
+impl F {
+    async fn run(&self) {}
+}
+
+pub async fn run() {
+    let f = F {};
+    let c = f.clone();
+    // Do not lint: `c` would not live long enough
+    spawn(async move { c.run().await });
+    let _f = f;
+}
+
+fn spawn<F: Future + 'static>(_: F) {}
+
+async fn work(_: &str) {}
+
+fn capture() {
+    let val = "Hello World".to_owned();
+    // Do not lint: `val` would not live long enough
+    spawn(async { work(&{ val }).await });
+}

tests/ui/redundant_async_block.rs

@@ -3,6 +3,8 @@
 #![allow(unused)]
 #![warn(clippy::redundant_async_block)]
 
+use std::future::Future;
+
 async fn func1(n: usize) -> usize {
     n + 1
 }
@@ -62,3 +64,48 @@ fn main() {
     let fut = async_await_parameter_in_macro!(func2());
     let fut = async_await_in_macro!(std::convert::identity);
 }
+
+#[allow(clippy::let_and_return)]
+fn capture_local() -> impl Future<Output = i32> {
+    // Lint
+    let fut = async { 17 };
+    async move { fut.await }
+}
+
+fn capture_local_closure(s: &str) -> impl Future<Output = &str> {
+    let f = move || std::future::ready(s);
+    // Do not lint: `f` would not live long enough
+    async move { f().await }
+}
+
+#[allow(clippy::let_and_return)]
+fn capture_arg(s: &str) -> impl Future<Output = &str> {
+    // Lint
+    let fut = async move { s };
+    async move { fut.await }
+}
+
+#[derive(Debug, Clone)]
+struct F {}
+
+impl F {
+    async fn run(&self) {}
+}
+
+pub async fn run() {
+    let f = F {};
+    let c = f.clone();
+    // Do not lint: `c` would not live long enough
+    spawn(async move { c.run().await });
+    let _f = f;
+}
+
+fn spawn<F: Future + 'static>(_: F) {}
+
+async fn work(_: &str) {}
+
+fn capture() {
+    let val = "Hello World".to_owned();
+    // Do not lint: `val` would not live long enough
+    spawn(async { work(&{ val }).await });
+}

tests/ui/redundant_async_block.stderr

@@ -1,28 +1,40 @@
 error: this async expression only awaits a single future
-  --> $DIR/redundant_async_block.rs:13:13
+  --> $DIR/redundant_async_block.rs:15:13
    |
 LL |     let x = async { f.await };
    |             ^^^^^^^^^^^^^^^^^ help: you can reduce it to: `f`
    |
    = note: `-D clippy::redundant-async-block` implied by `-D warnings`
 
 error: this async expression only awaits a single future
-  --> $DIR/redundant_async_block.rs:46:16
+  --> $DIR/redundant_async_block.rs:48:16
    |
 LL |     let fut2 = async { fut1.await };
    |                ^^^^^^^^^^^^^^^^^^^^ help: you can reduce it to: `fut1`
 
 error: this async expression only awaits a single future
-  --> $DIR/redundant_async_block.rs:49:16
+  --> $DIR/redundant_async_block.rs:51:16
    |
 LL |     let fut2 = async move { fut1.await };
    |                ^^^^^^^^^^^^^^^^^^^^^^^^^ help: you can reduce it to: `fut1`
 
 error: this async expression only awaits a single future
-  --> $DIR/redundant_async_block.rs:51:15
+  --> $DIR/redundant_async_block.rs:53:15
    |
 LL |     let fut = async { async { 42 }.await };
    |               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: you can reduce it to: `async { 42 }`
 
-error: aborting due to 4 previous errors
+error: this async expression only awaits a single future
+  --> $DIR/redundant_async_block.rs:72:5
+   |
+LL |     async move { fut.await }
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^ help: you can reduce it to: `fut`
+
+error: this async expression only awaits a single future
+  --> $DIR/redundant_async_block.rs:85:5
+   |
+LL |     async move { fut.await }
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^ help: you can reduce it to: `fut`
+
+error: aborting due to 6 previous errors