Auto merge of #10554 - samueltardieu:redundant-async-block, r=Jarcho

Make redundant_async_block a more complete late pass

This lets us detect more complex situations: `async { x.await }` is simplified into `x` if:

- `x` is an expression without side-effect
- or `x` is an `async` block itself

In both cases, no part of the `async` expression can be part of a macro expansion.

Fixes #10509.
Fixes #10525.

changelog: [`redundant_async_block`] Do not lint expressions with side effects.

Author: bors

clippy_lints/src/lib.rs

@@ -945,7 +945,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
     store.register_late_pass(|_| Box::new(no_mangle_with_rust_abi::NoMangleWithRustAbi));
     store.register_late_pass(|_| Box::new(collection_is_never_read::CollectionIsNeverRead));
     store.register_late_pass(|_| Box::new(missing_assert_message::MissingAssertMessage));
-    store.register_early_pass(|| Box::new(redundant_async_block::RedundantAsyncBlock));
+    store.register_late_pass(|_| Box::new(redundant_async_block::RedundantAsyncBlock));
     store.register_late_pass(|_| Box::new(let_with_type_underscore::UnderscoreTyped));
     store.register_late_pass(|_| Box::new(allow_attributes::AllowAttribute));
     store.register_late_pass(move |_| Box::new(manual_main_separator_str::ManualMainSeparatorStr::new(msrv())));

clippy_lints/src/redundant_async_block.rs

@@ -1,8 +1,15 @@
-use clippy_utils::{diagnostics::span_lint_and_sugg, source::snippet};
-use rustc_ast::ast::{Expr, ExprKind, Stmt, StmtKind};
-use rustc_ast::visit::Visitor as AstVisitor;
+use std::ops::ControlFlow;
+
+use clippy_utils::{
+    diagnostics::span_lint_and_sugg,
+    peel_blocks,
+    source::{snippet, walk_span_to_context},
+    visitors::for_each_expr,
+};
 use rustc_errors::Applicability;
-use rustc_lint::{EarlyContext, EarlyLintPass};
+use rustc_hir::{AsyncGeneratorKind, Closure, Expr, ExprKind, GeneratorKind, MatchSource};
+use rustc_lint::{LateContext, LateLintPass};
+use rustc_middle::{lint::in_external_macro, ty::UpvarCapture};
 use rustc_session::{declare_lint_pass, declare_tool_lint};
 
 declare_clippy_lint! {
@@ -14,106 +21,88 @@ declare_clippy_lint! {
     ///
     /// ### Example
     /// ```rust
-    /// async fn f() -> i32 {
-    ///     1 + 2
-    /// }
-    ///
+    /// let f = async {
+    ///    1 + 2
+    /// };
     /// let fut = async {
-    ///     f().await
+    ///     f.await
     /// };
     /// ```
     /// Use instead:
     /// ```rust
-    /// async fn f() -> i32 {
-    ///     1 + 2
-    /// }
-    ///
-    /// let fut = f();
+    /// let f = async {
+    ///    1 + 2
+    /// };
+    /// let fut = f;
     /// ```
     #[clippy::version = "1.69.0"]
     pub REDUNDANT_ASYNC_BLOCK,
-    nursery,
+    complexity,
     "`async { future.await }` can be replaced by `future`"
 }
 declare_lint_pass!(RedundantAsyncBlock => [REDUNDANT_ASYNC_BLOCK]);
 
-impl EarlyLintPass for RedundantAsyncBlock {
-    fn check_expr(&mut self, cx: &EarlyContext<'_>, expr: &Expr) {
-        if expr.span.from_expansion() {
-            return;
-        }
-        if let ExprKind::Async(_, _, block) = &expr.kind && block.stmts.len() == 1 &&
-            let Some(Stmt { kind: StmtKind::Expr(last), .. }) = block.stmts.last() &&
-            let ExprKind::Await(future) = &last.kind &&
-            !future.span.from_expansion() &&
-            !await_in_expr(future)
+impl<'tcx> LateLintPass<'tcx> for RedundantAsyncBlock {
+    fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
+        let span = expr.span;
+        if !in_external_macro(cx.tcx.sess, span) &&
+            let Some(body_expr) = desugar_async_block(cx, expr) &&
+            let Some(expr) = desugar_await(peel_blocks(body_expr)) &&
+            // The await prefix must not come from a macro as its content could change in the future.
+            expr.span.ctxt() == body_expr.span.ctxt() &&
+            // An async block does not have immediate side-effects from a `.await` point-of-view.
+            (!expr.can_have_side_effects() || desugar_async_block(cx, expr).is_some()) &&
+            let Some(shortened_span) = walk_span_to_context(expr.span, span.ctxt())
         {
-            if captures_value(last) {
-                // If the async block captures variables then there is no equivalence.
-                return;
-            }
-
             span_lint_and_sugg(
                 cx,
                 REDUNDANT_ASYNC_BLOCK,
-                expr.span,
+                span,
                 "this async expression only awaits a single future",
                 "you can reduce it to",
-                snippet(cx, future.span, "..").into_owned(),
+                snippet(cx, shortened_span, "..").into_owned(),
                 Applicability::MachineApplicable,
             );
         }
     }
 }
 
-/// Check whether an expression contains `.await`
-fn await_in_expr(expr: &Expr) -> bool {
-    let mut detector = AwaitDetector::default();
-    detector.visit_expr(expr);
-    detector.await_found
-}
-
-#[derive(Default)]
-struct AwaitDetector {
-    await_found: bool,
-}
-
-impl<'ast> AstVisitor<'ast> for AwaitDetector {
-    fn visit_expr(&mut self, ex: &'ast Expr) {
-        match (&ex.kind, self.await_found) {
-            (ExprKind::Await(_), _) => self.await_found = true,
-            (_, false) => rustc_ast::visit::walk_expr(self, ex),
-            _ => (),
-        }
+/// If `expr` is a desugared `async` block, return the original expression if it does not capture
+/// any variable by ref.
+fn desugar_async_block<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) -> Option<&'tcx Expr<'tcx>> {
+    if let ExprKind::Closure(Closure { body, def_id, .. }) = expr.kind &&
+        let body = cx.tcx.hir().body(*body) &&
+        matches!(body.generator_kind, Some(GeneratorKind::Async(AsyncGeneratorKind::Block)))
+    {
+        cx
+            .typeck_results()
+            .closure_min_captures
+            .get(def_id)
+            .map_or(true, |m| {
+                m.values().all(|places| {
+                    places
+                        .iter()
+                        .all(|place| matches!(place.info.capture_kind, UpvarCapture::ByValue))
+                })
+            })
+            .then_some(body.value)
+    } else {
+        None
     }
 }
 
-/// 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),
-            _ => (),
-        }
+/// If `expr` is a desugared `.await`, return the original expression if it does not come from a
+/// macro expansion.
+fn desugar_await<'tcx>(expr: &'tcx Expr<'_>) -> Option<&'tcx Expr<'tcx>> {
+    if let ExprKind::Match(match_value, _, MatchSource::AwaitDesugar) = expr.kind &&
+        let ExprKind::Call(_, [into_future_arg]) = match_value.kind &&
+        let ctxt = expr.span.ctxt() &&
+        for_each_expr(into_future_arg, |e|
+            walk_span_to_context(e.span, ctxt)
+                .map_or(ControlFlow::Break(()), |_| ControlFlow::Continue(()))).is_none()
+    {
+        Some(into_future_arg)
+    } else {
+        None
     }
 }

tests/ui/redundant_async_block.fixed

@@ -1,6 +1,6 @@
 // run-rustfix
 
-#![allow(unused)]
+#![allow(unused, clippy::manual_async_fn)]
 #![warn(clippy::redundant_async_block)]
 
 use std::future::Future;
@@ -16,40 +16,16 @@ async fn func2() -> String {
     x.await
 }
 
-macro_rules! await_in_macro {
-    ($e:expr) => {
-        std::convert::identity($e).await
-    };
-}
-
-async fn func3(n: usize) -> usize {
-    // Do not lint (suggestion would be `std::convert::identity(func1(n))`
-    // which copies code from inside the macro)
-    async move { await_in_macro!(func1(n)) }.await
-}
-
-// This macro should never be linted as `$e` might contain `.await`
-macro_rules! async_await_parameter_in_macro {
-    ($e:expr) => {
-        async { $e.await }
-    };
-}
-
-// MISSED OPPORTUNITY: this macro could be linted as the `async` block does not
-// contain code coming from the parameters
-macro_rules! async_await_in_macro {
-    ($f:expr) => {
-        ($f)(async { func2().await })
-    };
-}
-
 fn main() {
     let fut1 = async { 17 };
+    // Lint
     let fut2 = fut1;
 
     let fut1 = async { 25 };
+    // Lint
     let fut2 = fut1;
 
+    // Lint
     let fut = async { 42 };
 
     // Do not lint: not a single expression
@@ -60,15 +36,12 @@ fn main() {
 
     // Do not lint: expression contains `.await`
     let fut = async { func1(func2().await.len()).await };
-
-    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 };
+    // Lint
     fut
 }
 
@@ -80,11 +53,39 @@ fn capture_local_closure(s: &str) -> impl Future<Output = &str> {
 
 #[allow(clippy::let_and_return)]
 fn capture_arg(s: &str) -> impl Future<Output = &str> {
-    // Lint
     let fut = async move { s };
+    // Lint
     fut
 }
 
+fn capture_future_arg<T>(f: impl Future<Output = T>) -> impl Future<Output = T> {
+    // Lint
+    f
+}
+
+fn capture_func_result<FN, F, T>(f: FN) -> impl Future<Output = T>
+where
+    F: Future<Output = T>,
+    FN: FnOnce() -> F,
+{
+    // Do not lint, as f() would be evaluated prematurely
+    async { f().await }
+}
+
+fn double_future(f: impl Future<Output = impl Future<Output = u32>>) -> impl Future<Output = u32> {
+    // Do not lint, we will get a `.await` outside a `.async`
+    async { f.await.await }
+}
+
+fn await_in_async<F, R>(f: F) -> impl Future<Output = u32>
+where
+    F: FnOnce() -> R,
+    R: Future<Output = u32>,
+{
+    // Lint
+    async { f().await + 1 }
+}
+
 #[derive(Debug, Clone)]
 struct F {}
 
@@ -109,3 +110,84 @@ fn capture() {
     // Do not lint: `val` would not live long enough
     spawn(async { work(&{ val }).await });
 }
+
+fn await_from_macro() -> impl Future<Output = u32> {
+    macro_rules! mac {
+        ($e:expr) => {
+            $e.await
+        };
+    }
+    // Do not lint: the macro may change in the future
+    // or return different things depending on its argument
+    async { mac!(async { 42 }) }
+}
+
+fn async_expr_from_macro() -> impl Future<Output = u32> {
+    macro_rules! mac {
+        () => {
+            async { 42 }
+        };
+    }
+    // Do not lint: the macro may change in the future
+    async { mac!().await }
+}
+
+fn async_expr_from_macro_deep() -> impl Future<Output = u32> {
+    macro_rules! mac {
+        () => {
+            async { 42 }
+        };
+    }
+    // Do not lint: the macro may change in the future
+    async { ({ mac!() }).await }
+}
+
+fn all_from_macro() -> impl Future<Output = u32> {
+    macro_rules! mac {
+        () => {
+            // Lint
+            async { 42 }
+        };
+    }
+    mac!()
+}
+
+fn parts_from_macro() -> impl Future<Output = u32> {
+    macro_rules! mac {
+        ($e: expr) => {
+            // Do not lint: `$e` might not always be side-effect free
+            async { $e.await }
+        };
+    }
+    mac!(async { 42 })
+}
+
+fn safe_parts_from_macro() -> impl Future<Output = u32> {
+    macro_rules! mac {
+        ($e: expr) => {
+            // Lint
+            async { $e }
+        };
+    }
+    mac!(42)
+}
+
+fn parts_from_macro_deep() -> impl Future<Output = u32> {
+    macro_rules! mac {
+        ($e: expr) => {
+            // Do not lint: `$e` might not always be side-effect free
+            async { ($e,).0.await }
+        };
+    }
+    let f = std::future::ready(42);
+    mac!(f)
+}
+
+fn await_from_macro_deep() -> impl Future<Output = u32> {
+    macro_rules! mac {
+        ($e:expr) => {{ $e }.await};
+    }
+    // Do not lint: the macro may change in the future
+    // or return different things depending on its argument
+    async { mac!(async { 42 }) }
+}