Change large_futures such that it applies to all expressions of type Future

clippy_lints/src/large_futures.rs

@@ -1,20 +1,28 @@
+use crate::clippy_utils::res::MaybeDef;
 use clippy_config::Conf;
-use clippy_utils::diagnostics::span_lint_and_sugg;
+use clippy_utils::diagnostics::span_lint_hir_and_then;
+use clippy_utils::res::MaybeResPath;
 use clippy_utils::source::snippet;
+use clippy_utils::sym;
 use clippy_utils::ty::implements_trait;
 use rustc_abi::Size;
 use rustc_errors::Applicability;
-use rustc_hir::{Expr, ExprKind, LangItem, MatchSource};
+use rustc_hir::def_id::LocalDefId;
+use rustc_hir::intravisit::{FnKind, Visitor, walk_body, walk_expr};
+use rustc_hir::{Body, Expr, ExprKind, FnDecl, HirIdSet, LangItem, QPath};
 use rustc_lint::{LateContext, LateLintPass};
+use rustc_middle::hir::nested_filter;
+use rustc_middle::ty::Ty;
 use rustc_session::impl_lint_pass;
+use rustc_span::{DesugaringKind, Span};
 
 declare_clippy_lint! {
     /// ### What it does
     /// It checks for the size of a `Future` created by `async fn` or `async {}`.
     ///
     /// ### Why is this bad?
     /// Due to the current [unideal implementation](https://github.com/rust-lang/rust/issues/69826) of `Coroutine`,
-    /// large size of a `Future` may cause stack overflows.
+    /// large size of a `Future` may cause stack overflows or be inefficient.
     ///
     /// ### Example
     /// ```no_run
@@ -25,7 +33,7 @@ declare_clippy_lint! {
     /// }
     /// ```
     ///
-    /// `Box::pin` the big future instead.
+    /// `Box::pin` the big future or the captured elements inside the future instead.
     ///
     /// ```no_run
     /// async fn large_future(_x: [u8; 16 * 1024]) {}
@@ -44,39 +52,136 @@ impl_lint_pass!(LargeFuture => [LARGE_FUTURES]);
 
 pub struct LargeFuture {
     future_size_threshold: u64,
+    visited: HirIdSet,
 }
 
 impl LargeFuture {
     pub fn new(conf: &'static Conf) -> Self {
         Self {
             future_size_threshold: conf.future_size_threshold,
+            visited: HirIdSet::default(),
         }
     }
 }
 
-impl<'tcx> LateLintPass<'tcx> for LargeFuture {
-    fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) {
-        if let ExprKind::Match(scrutinee, _, MatchSource::AwaitDesugar) = expr.kind
-            && let ExprKind::Call(func, [arg]) = scrutinee.kind
-            && let ExprKind::Path(qpath) = func.kind
-            && cx.tcx.qpath_is_lang_item(qpath, LangItem::IntoFutureIntoFuture)
-            && !expr.span.from_expansion()
-            && let ty = cx.typeck_results().expr_ty(arg)
-            && let Some(future_trait_def_id) = cx.tcx.lang_items().future_trait()
-            && implements_trait(cx, ty, future_trait_def_id, &[])
-            && let Ok(layout) = cx.tcx.layout_of(cx.typing_env().as_query_input(ty))
-            && let size = layout.layout.size()
-            && size >= Size::from_bytes(self.future_size_threshold)
+struct AsyncFnVisitor<'a, 'tcx> {
+    parent: &'a mut LargeFuture,
+    cx: &'a LateContext<'tcx>,
+    /// Span of the function decl to be used when emitting a lint concerning the entire decl.
+    top_level_span: Span,
+    /// Depth of the visitor with value `0` denoting the top-level expression.
+    depth: usize,
+    /// Whether a clippy suggestion was emitted for deeper levels of expressions.
+    emitted: bool,
+}
+
+impl<'tcx> Visitor<'tcx> for AsyncFnVisitor<'_, 'tcx> {
+    type NestedFilter = nested_filter::OnlyBodies;
+
+    fn visit_expr(&mut self, expr: &'tcx Expr<'tcx>) {
+        // For each expression, try to find the 'deepest' occurance of a too large a future.
+        // Chances are that subsequent less deep expression contain that too large a future,
+        // and hence it is redudant to also emit a suggestion for these expressions.
+        // Expressions on the same depth level can however emit a suggestion too.
+
+        if !self.parent.visited.insert(expr.hir_id) {
+            // This expression was already visited once.
+            return;
+        }
+
+        if let ExprKind::Call(method, _) = expr.kind
+            && let ExprKind::Path(QPath::TypeRelative(ty, seg)) = method.kind
+            && ty.basic_res().is_lang_item(self.cx, LangItem::OwnedBox)
+            && seg.ident.name == sym::pin
+        {
+            // When the current expression is already a `Box::pin(...)` call,
+            // our suggestion has been applied. Do not recurse deeper.
+            return;
+        }
+
+        self.depth += 1;
+        let old_emitted = self.emitted;
+        self.emitted = false; // Reset emitted for deeper level, but recover it later.
+        walk_expr(self, expr);
+        self.depth -= 1;
+
+        if !self.emitted
+            && !expr.span.is_desugaring(DesugaringKind::Await)
+            && let Some(ty) = self.cx.typeck_results().expr_ty_opt(expr)
+            && let Some(size) = type_is_large_future(self.cx, ty, self.parent.future_size_threshold)
         {
-            span_lint_and_sugg(
-                cx,
-                LARGE_FUTURES,
-                arg.span,
-                format!("large future with a size of {} bytes", size.bytes()),
-                "consider `Box::pin` on it",
-                format!("Box::pin({})", snippet(cx, arg.span, "..")),
-                Applicability::Unspecified,
-            );
+            if self.depth == 0 {
+                // Special lint for top level fn bodies.
+                span_lint_hir_and_then(
+                    self.cx,
+                    LARGE_FUTURES,
+                    expr.hir_id,
+                    self.top_level_span,
+                    format!("definition for a large future with a size of {} bytes", size.bytes()),
+                    |db| {
+                        db.help("consider `Box::pin` when constructing it or reducing direct ownership of the items in scope and use references instead where possible");
+                    },
+                );
+            } else {
+                // Expressions within a fn body.
+                span_lint_hir_and_then(
+                    self.cx,
+                    LARGE_FUTURES,
+                    expr.hir_id,
+                    expr.span,
+                    format!("usage of large future with a size of {} bytes", size.bytes()),
+                    |db| {
+                        db.span_suggestion(
+                            expr.span,
+                            "consider `Box::pin` on it or reducing direct ownership of the items in scope and use references instead where possible",
+                            format!("Box::pin({})", snippet(self.cx, expr.span, "..")),
+                            Applicability::Unspecified,
+                        );
+                    },
+                );
+            }
+
+            self.emitted = true;
         }
+
+        self.emitted |= old_emitted;
+    }
+
+    fn maybe_tcx(&mut self) -> Self::MaybeTyCtxt {
+        self.cx.tcx
+    }
+}
+
+fn type_is_large_future<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>, future_size_threshold: u64) -> Option<Size> {
+    if let Some(future_trait_def_id) = cx.tcx.lang_items().future_trait()
+        && implements_trait(cx, ty, future_trait_def_id, &[])
+        && let Ok(layout) = cx.tcx.layout_of(cx.typing_env().as_query_input(ty))
+        && let size = layout.layout.size()
+        && size >= Size::from_bytes(future_size_threshold)
+    {
+        Some(size)
+    } else {
+        None
+    }
+}
+
+impl<'tcx> LateLintPass<'tcx> for LargeFuture {
+    fn check_fn(
+        &mut self,
+        cx: &LateContext<'tcx>,
+        _: FnKind<'tcx>,
+        _: &'tcx FnDecl<'_>,
+        body: &'tcx Body<'_>,
+        span: Span,
+        _: LocalDefId,
+    ) {
+        let mut visitor = AsyncFnVisitor {
+            parent: self,
+            cx,
+            top_level_span: span,
+            depth: 0,
+            emitted: false,
+        };
+        walk_body(&mut visitor, body);
     }
 }

tests/ui-toml/large_futures/large_futures.fixed

@@ -2,14 +2,21 @@
 
 fn main() {}
 
+// Note: large_futures are allowed here, as rustfix cannot actually fix this case.
+// The reason we still keep it around is that it's used as a helper in other tests.
+// See large_futures_unfixable.rs where this definition is demonstrated to emit a lint as well.
+#[allow(clippy::large_futures)]
 pub async fn should_warn() {
     let x = [0u8; 1024];
     async {}.await;
     dbg!(x);
 }
 
 pub async fn should_not_warn() {
-    let x = [0u8; 1020];
+    // Note: not 1020 bytes as expected, because after being corrected to `Box::pin(should_warn())`,
+    // `bar()` is now the largest future in the tree, and with this being 1020 bytes `bar()` is larger
+    // than 1024 bytes because of the added discriminant overhead.
+    let x = [0u8; 1012];
     async {}.await;
     dbg!(x);
 }

tests/ui-toml/large_futures/large_futures.rs

@@ -2,14 +2,21 @@
 
 fn main() {}
 
+// Note: large_futures are allowed here, as rustfix cannot actually fix this case.
+// The reason we still keep it around is that it's used as a helper in other tests.
+// See large_futures_unfixable.rs where this definition is demonstrated to emit a lint as well.
+#[allow(clippy::large_futures)]
 pub async fn should_warn() {
     let x = [0u8; 1024];
     async {}.await;
     dbg!(x);
 }
 
 pub async fn should_not_warn() {
-    let x = [0u8; 1020];
+    // Note: not 1020 bytes as expected, because after being corrected to `Box::pin(should_warn())`,
+    // `bar()` is now the largest future in the tree, and with this being 1020 bytes `bar()` is larger
+    // than 1024 bytes because of the added discriminant overhead.
+    let x = [0u8; 1012];
     async {}.await;
     dbg!(x);
 }

tests/ui-toml/large_futures/large_futures.stderr

@@ -1,11 +1,16 @@
-error: large future with a size of 1026 bytes
-  --> tests/ui-toml/large_futures/large_futures.rs:18:5
+error: usage of large future with a size of 1026 bytes
+  --> tests/ui-toml/large_futures/large_futures.rs:25:5
    |
 LL |     should_warn().await;
-   |     ^^^^^^^^^^^^^ help: consider `Box::pin` on it: `Box::pin(should_warn())`
+   |     ^^^^^^^^^^^^^
    |
    = note: `-D clippy::large-futures` implied by `-D warnings`
    = help: to override `-D warnings` add `#[allow(clippy::large_futures)]`
+help: consider `Box::pin` on it or reducing direct ownership of the items in scope and use references instead where possible
+   |
+LL -     should_warn().await;
+LL +     Box::pin(should_warn()).await;
+   |
 
 error: aborting due to 1 previous error
 

tests/ui-toml/large_futures/large_futures_unfixable.rs

@@ -0,0 +1,11 @@
+#![warn(clippy::large_futures)]
+//@no-rustfix
+
+fn main() {}
+
+//~v large_futures
+pub async fn should_warn() {
+    let x = [0u8; 1024];
+    async {}.await;
+    dbg!(x);
+}

tests/ui-toml/large_futures/large_futures_unfixable.stderr

@@ -0,0 +1,16 @@
+error: definition for a large future with a size of 1026 bytes
+  --> tests/ui-toml/large_futures/large_futures_unfixable.rs:7:1
+   |
+LL | / pub async fn should_warn() {
+LL | |     let x = [0u8; 1024];
+LL | |     async {}.await;
+LL | |     dbg!(x);
+LL | | }
+   | |_^
+   |
+   = help: consider `Box::pin` when constructing it or reducing direct ownership of the items in scope and use references instead where possible
+   = note: `-D clippy::large-futures` implied by `-D warnings`
+   = help: to override `-D warnings` add `#[allow(clippy::large_futures)]`
+
+error: aborting due to 1 previous error
+

tests/ui/large_futures.fixed

@@ -1,11 +1,10 @@
-#![allow(
-    clippy::future_not_send,
-    clippy::manual_async_fn,
-    clippy::never_loop,
-    clippy::uninlined_format_args
-)]
+#![allow(clippy::manual_async_fn)]
 #![warn(clippy::large_futures)]
 
+// Note: large_futures are allowed here, as rustfix cannot actually fix this case.
+// The reason we still keep it around is that it's used as a helper in other tests.
+// See large_futures_unfixable.rs where this definition is demonstrated to emit a lint as well.
+#[allow(clippy::large_futures)]
 async fn big_fut(_arg: [u8; 1024 * 16]) {}
 
 async fn wait() {
@@ -31,33 +30,37 @@ async fn calls_fut(fut: impl std::future::Future<Output = ()>) {
 }
 
 pub async fn test() {
-    let fut = big_fut([0u8; 1024 * 16]);
+    let fut = Box::pin(big_fut([0u8; 1024 * 16]));
+    //~^ large_futures
+
     Box::pin(foo()).await;
     //~^ large_futures
 
-    Box::pin(calls_fut(fut)).await;
+    calls_fut(Box::pin(fut)).await;
     //~^ large_futures
 }
 
 pub fn foo() -> impl std::future::Future<Output = ()> {
-    async {
+    Box::pin(async {
+        //~^ large_futures
         let x = [0i32; 1024 * 16];
         async {}.await;
         dbg!(x);
-    }
+    })
 }
 
 pub async fn lines() {
     Box::pin(async {
         //~^ large_futures
-
         let x = [0i32; 1024 * 16];
         async {}.await;
+
         println!("{:?}", x);
     })
     .await;
 }
 
+// Note: large_futures are allowed here, as rustfix cannot actually fix this case.
 pub async fn macro_expn() {
     macro_rules! macro_ {
         () => {