Auto merge of #12582 - kpreid:stacksize, r=Manishearth

bors · bors · commit 97ba291d5aa0 · 2024-03-29T15:07:03.000Z
`large_stack_frames`: print total size and largest component.

Instead of just saying “this function's stack frame is big”, report:

* the (presumed) size of the frame
* the size and type of the largest local contributing to that size
* the configurable limit that was exceeded (once)

Known issues:

* The lint may report an over-estimate because codegen may be able to overlap some of these locals. However, that already affected whether the lint fired at all; I believe this change is still an improvement because it gives the user much more actionable information about _why_ the lint fired.
* Please tell me a better way to determine whether a local has a variable name.

changelog: [`large_stack_frames`]: print total size and largest component.
diff --git a/clippy_lints/src/large_stack_frames.rs b/clippy_lints/src/large_stack_frames.rs
@@ -1,10 +1,12 @@
-use std::ops::AddAssign;
+use std::{fmt, ops};
 
-use clippy_utils::diagnostics::span_lint_and_note;
+use clippy_utils::diagnostics::span_lint_and_then;
 use clippy_utils::fn_has_unsatisfiable_preds;
+use clippy_utils::source::snippet_opt;
 use rustc_hir::def_id::LocalDefId;
 use rustc_hir::intravisit::FnKind;
 use rustc_hir::{Body, FnDecl};
+use rustc_lexer::is_ident;
 use rustc_lint::{LateContext, LateLintPass};
 use rustc_session::impl_lint_pass;
 use rustc_span::Span;
@@ -108,13 +110,25 @@ impl Space {
     }
 }
 
-impl AddAssign<u64> for Space {
-    fn add_assign(&mut self, rhs: u64) {
-        if let Self::Used(lhs) = self {
-            match lhs.checked_add(rhs) {
-                Some(sum) => *self = Self::Used(sum),
-                None => *self = Self::Overflow,
-            }
+impl fmt::Display for Space {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        match self {
+            Space::Used(1) => write!(f, "1 byte"),
+            Space::Used(n) => write!(f, "{n} bytes"),
+            Space::Overflow => write!(f, "over 2⁶⁴-1 bytes"),
+        }
+    }
+}
+
+impl ops::Add<u64> for Space {
+    type Output = Self;
+    fn add(self, rhs: u64) -> Self {
+        match self {
+            Self::Used(lhs) => match lhs.checked_add(rhs) {
+                Some(sum) => Self::Used(sum),
+                None => Self::Overflow,
+            },
+            Self::Overflow => self,
         }
     }
 }
@@ -123,10 +137,10 @@ impl<'tcx> LateLintPass<'tcx> for LargeStackFrames {
     fn check_fn(
         &mut self,
         cx: &LateContext<'tcx>,
-        _: FnKind<'tcx>,
+        fn_kind: FnKind<'tcx>,
         _: &'tcx FnDecl<'tcx>,
         _: &'tcx Body<'tcx>,
-        span: Span,
+        entire_fn_span: Span,
         local_def_id: LocalDefId,
     ) {
         let def_id = local_def_id.to_def_id();
@@ -138,22 +152,68 @@ impl<'tcx> LateLintPass<'tcx> for LargeStackFrames {
         let mir = cx.tcx.optimized_mir(def_id);
         let param_env = cx.tcx.param_env(def_id);
 
-        let mut frame_size = Space::Used(0);
+        let sizes_of_locals = || {
+            mir.local_decls.iter().filter_map(|local| {
+                let layout = cx.tcx.layout_of(param_env.and(local.ty)).ok()?;
+                Some((local, layout.size.bytes()))
+            })
+        };
 
-        for local in &mir.local_decls {
-            if let Ok(layout) = cx.tcx.layout_of(param_env.and(local.ty)) {
-                frame_size += layout.size.bytes();
-            }
-        }
+        let frame_size = sizes_of_locals().fold(Space::Used(0), |sum, (_, size)| sum + size);
+
+        let limit = self.maximum_allowed_size;
+        if frame_size.exceeds_limit(limit) {
+            // Point at just the function name if possible, because lints that span
+            // the entire body and don't have to are less legible.
+            let fn_span = match fn_kind {
+                FnKind::ItemFn(ident, _, _) | FnKind::Method(ident, _) => ident.span,
+                FnKind::Closure => entire_fn_span,
+            };
 
-        if frame_size.exceeds_limit(self.maximum_allowed_size) {
-            span_lint_and_note(
+            span_lint_and_then(
                 cx,
                 LARGE_STACK_FRAMES,
-                span,
-                "this function allocates a large amount of stack space",
-                None,
-                "allocating large amounts of stack space can overflow the stack",
+                fn_span,
+                &format!("this function may allocate {frame_size} on the stack"),
+                |diag| {
+                    // Point out the largest individual contribution to this size, because
+                    // it is the most likely to be unintentionally large.
+                    if let Some((local, size)) = sizes_of_locals().max_by_key(|&(_, size)| size) {
+                        let local_span: Span = local.source_info.span;
+                        let size = Space::Used(size); // pluralizes for us
+                        let ty = local.ty;
+
+                        // TODO: Is there a cleaner, robust way to ask this question?
+                        // The obvious `LocalDecl::is_user_variable()` panics on "unwrapping cross-crate data",
+                        // and that doesn't get us the true name in scope rather than the span text either.
+                        if let Some(name) = snippet_opt(cx, local_span)
+                            && is_ident(&name)
+                        {
+                            // If the local is an ordinary named variable,
+                            // print its name rather than relying solely on the span.
+                            diag.span_label(
+                                local_span,
+                                format!("`{name}` is the largest part, at {size} for type `{ty}`"),
+                            );
+                        } else {
+                            diag.span_label(
+                                local_span,
+                                format!("this is the largest part, at {size} for type `{ty}`"),
+                            );
+                        }
+                    }
+
+                    // Explain why we are linting this and not other functions.
+                    diag.note(format!(
+                        "{frame_size} is larger than Clippy's configured `stack-size-threshold` of {limit}"
+                    ));
+
+                    // Explain why the user should care, briefly.
+                    diag.note_once(
+                        "allocating large amounts of stack space can overflow the stack \
+                        and cause the program to abort",
+                    );
+                },
             );
         }
     }
diff --git a/tests/ui-toml/large_stack_frames/large_stack_frames.rs b/tests/ui-toml/large_stack_frames/large_stack_frames.rs
@@ -10,7 +10,7 @@ fn f() {
     let _x = create_array::<1000>();
 }
 fn f2() {
-    //~^ ERROR: this function allocates a large amount of stack space
+    //~^ ERROR: this function may allocate 1001 bytes on the stack
     let _x = create_array::<1001>();
 }
 
diff --git a/tests/ui-toml/large_stack_frames/large_stack_frames.stderr b/tests/ui-toml/large_stack_frames/large_stack_frames.stderr
@@ -1,13 +1,14 @@
-error: this function allocates a large amount of stack space
-  --> tests/ui-toml/large_stack_frames/large_stack_frames.rs:12:1
+error: this function may allocate 1001 bytes on the stack
+  --> tests/ui-toml/large_stack_frames/large_stack_frames.rs:12:4
    |
-LL | / fn f2() {
-LL | |
-LL | |     let _x = create_array::<1001>();
-LL | | }
-   | |_^
+LL | fn f2() {
+   |    ^^
+LL |
+LL |     let _x = create_array::<1001>();
+   |         -- `_x` is the largest part, at 1001 bytes for type `[u8; 1001]`
    |
-   = note: allocating large amounts of stack space can overflow the stack
+   = note: 1001 bytes is larger than Clippy's configured `stack-size-threshold` of 1000
+   = note: allocating large amounts of stack space can overflow the stack and cause the program to abort
    = note: `-D clippy::large-stack-frames` implied by `-D warnings`
    = help: to override `-D warnings` add `#[allow(clippy::large_stack_frames)]`
 
diff --git a/tests/ui/large_stack_frames.rs b/tests/ui/large_stack_frames.rs
@@ -1,3 +1,5 @@
+//@ normalize-stderr-test: "\b10000(08|16|32)\b" -> "100$$PTR"
+//@ normalize-stderr-test: "\b2500(060|120)\b" -> "250$$PTR"
 #![allow(unused, incomplete_features)]
 #![warn(clippy::large_stack_frames)]
 #![feature(unsized_locals)]
@@ -23,8 +25,7 @@ impl<const N: usize> Default for ArrayDefault<N> {
 }
 
 fn many_small_arrays() {
-    //~^ ERROR: this function allocates a large amount of stack space
-    //~| NOTE: allocating large amounts of stack space can overflow the stack
+    //~^ ERROR: this function may allocate
     let x = [0u8; 500_000];
     let x2 = [0u8; 500_000];
     let x3 = [0u8; 500_000];
@@ -34,17 +35,21 @@ fn many_small_arrays() {
 }
 
 fn large_return_value() -> ArrayDefault<1_000_000> {
-    //~^ ERROR: this function allocates a large amount of stack space
-    //~| NOTE: allocating large amounts of stack space can overflow the stack
+    //~^ ERROR: this function may allocate 1000000 bytes on the stack
     Default::default()
 }
 
 fn large_fn_arg(x: ArrayDefault<1_000_000>) {
-    //~^ ERROR: this function allocates a large amount of stack space
-    //~| NOTE: allocating large amounts of stack space can overflow the stack
+    //~^ ERROR: this function may allocate
     black_box(&x);
 }
 
+fn has_large_closure() {
+    let f = || black_box(&[0u8; 1_000_000]);
+    //~^ ERROR: this function may allocate
+    f();
+}
+
 fn main() {
     generic::<ArrayDefault<1_000_000>>();
 }
diff --git a/tests/ui/large_stack_frames.stderr b/tests/ui/large_stack_frames.stderr
@@ -1,42 +1,42 @@
-error: this function allocates a large amount of stack space
-  --> tests/ui/large_stack_frames.rs:25:1
-   |
-LL | / fn many_small_arrays() {
-LL | |
-LL | |
-LL | |     let x = [0u8; 500_000];
-...  |
-LL | |     black_box((&x, &x2, &x3, &x4, &x5));
-LL | | }
-   | |_^
-   |
-   = note: allocating large amounts of stack space can overflow the stack
+error: this function may allocate 250$PTR bytes on the stack
+  --> tests/ui/large_stack_frames.rs:27:4
+   |
+LL | fn many_small_arrays() {
+   |    ^^^^^^^^^^^^^^^^^
+...
+LL |     let x5 = [0u8; 500_000];
+   |         -- `x5` is the largest part, at 500000 bytes for type `[u8; 500000]`
+   |
+   = note: 250$PTR bytes is larger than Clippy's configured `stack-size-threshold` of 512000
+   = note: allocating large amounts of stack space can overflow the stack and cause the program to abort
    = note: `-D clippy::large-stack-frames` implied by `-D warnings`
    = help: to override `-D warnings` add `#[allow(clippy::large_stack_frames)]`
 
-error: this function allocates a large amount of stack space
-  --> tests/ui/large_stack_frames.rs:36:1
+error: this function may allocate 1000000 bytes on the stack
+  --> tests/ui/large_stack_frames.rs:37:4
+   |
+LL | fn large_return_value() -> ArrayDefault<1_000_000> {
+   |    ^^^^^^^^^^^^^^^^^^      ----------------------- this is the largest part, at 1000000 bytes for type `ArrayDefault<1000000>`
+   |
+   = note: 1000000 bytes is larger than Clippy's configured `stack-size-threshold` of 512000
+
+error: this function may allocate 100$PTR bytes on the stack
+  --> tests/ui/large_stack_frames.rs:42:4
    |
-LL | / fn large_return_value() -> ArrayDefault<1_000_000> {
-LL | |
-LL | |
-LL | |     Default::default()
-LL | | }
-   | |_^
+LL | fn large_fn_arg(x: ArrayDefault<1_000_000>) {
+   |    ^^^^^^^^^^^^ - `x` is the largest part, at 1000000 bytes for type `ArrayDefault<1000000>`
    |
-   = note: allocating large amounts of stack space can overflow the stack
+   = note: 100$PTR bytes is larger than Clippy's configured `stack-size-threshold` of 512000
 
-error: this function allocates a large amount of stack space
-  --> tests/ui/large_stack_frames.rs:42:1
+error: this function may allocate 100$PTR bytes on the stack
+  --> tests/ui/large_stack_frames.rs:48:13
    |
-LL | / fn large_fn_arg(x: ArrayDefault<1_000_000>) {
-LL | |
-LL | |
-LL | |     black_box(&x);
-LL | | }
-   | |_^
+LL |     let f = || black_box(&[0u8; 1_000_000]);
+   |             ^^^^^^^^^^^^^^----------------^
+   |                           |
+   |                           this is the largest part, at 1000000 bytes for type `[u8; 1000000]`
    |
-   = note: allocating large amounts of stack space can overflow the stack
+   = note: 100$PTR bytes is larger than Clippy's configured `stack-size-threshold` of 512000
 
-error: aborting due to 3 previous errors
+error: aborting due to 4 previous errors
 

Original file line number	Diff line number	Diff line change
`@@ -10,7 +10,7 @@ fn f() {`
`10`	`10`	`let _x = create_array::<1000>();`
`11`	`11`	`}`
`12`	`12`	`fn f2() {`
`13`		`- //~^ ERROR: this function allocates a large amount of stack space`
	`13`	`+ //~^ ERROR: this function may allocate 1001 bytes on the stack`
`14`	`14`	`let _x = create_array::<1001>();`
`15`	`15`	`}`
`16`	`16`