From 349708655ea924bb34861025e3ee6d99d58e3f8d Mon Sep 17 00:00:00 2001 From: "Samuel \"Sam\" Tardieu" Date: Sun, 12 Mar 2023 14:06:13 +0100 Subject: [PATCH] Do not propose to remove `async move` if variables are captured by ref --- clippy_lints/src/redundant_async_block.rs | 37 +++++++++++++++++- tests/ui/redundant_async_block.fixed | 47 +++++++++++++++++++++++ tests/ui/redundant_async_block.rs | 47 +++++++++++++++++++++++ tests/ui/redundant_async_block.stderr | 22 ++++++++--- 4 files changed, 147 insertions(+), 6 deletions(-) diff --git a/clippy_lints/src/redundant_async_block.rs b/clippy_lints/src/redundant_async_block.rs index 27ad43086..24dcd52b9 100644 --- a/clippy_lints/src/redundant_async_block.rs +++ b/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), + _ => (), + } + } +} diff --git a/tests/ui/redundant_async_block.fixed b/tests/ui/redundant_async_block.fixed index 5f9931df4..d26b7a332 100644 --- a/tests/ui/redundant_async_block.fixed +++ b/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 { + // Lint + let fut = async { 17 }; + fut +} + +fn capture_local_closure(s: &str) -> impl Future { + 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 { + // 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) {} + +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 }); +} diff --git a/tests/ui/redundant_async_block.rs b/tests/ui/redundant_async_block.rs index de3c9970c..04726e628 100644 --- a/tests/ui/redundant_async_block.rs +++ b/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 { + // Lint + let fut = async { 17 }; + async move { fut.await } +} + +fn capture_local_closure(s: &str) -> impl Future { + 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 { + // 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) {} + +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 }); +} diff --git a/tests/ui/redundant_async_block.stderr b/tests/ui/redundant_async_block.stderr index b16d96dce..1a1c1603e 100644 --- a/tests/ui/redundant_async_block.stderr +++ b/tests/ui/redundant_async_block.stderr @@ -1,5 +1,5 @@ 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` @@ -7,22 +7,34 @@ LL | let x = async { f.await }; = 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