Skip to content

Fix large_stack_frames FP on compiler generated targets #15101

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix large_stack_frames FP on compiler generated targets #15101

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions clippy_lints/src/large_stack_frames.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,6 +170,11 @@ impl<'tcx> LateLintPass<'tcx> for LargeStackFrames {
FnKind::Closure => entire_fn_span,
};

// Don't lint compiler-generated targets
if fn_span.is_dummy() {
return;
}

Comment on lines +173 to +177
Copy link
Contributor

@samueltardieu samueltardieu Jun 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comparing against the dummy span feels too specific. Why not return from the beginning of the function if entire_fn_span.from_expansion() is true?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it is a good idea to ignore macros in this lint

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you produce a realistic test case in which a macro should not be ignored and would get a sensible message? Right now, checking for entire_fn_span.from_expansion() or using your proposed change makes no difference in tests results.

And if the goal is to lint all functions to ensure that a given stack size is not exceeded, maybe compiler generated functions should be checked after all instead of being silently ignored, and the message fixed. One could also add a configuration option to exclude test methods from the lint, that would be my preferred option.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe compiler generated functions should be checked after all instead of being silently ignored, and the message fixed. One could also add a configuration option to exclude test methods from the lint, that would be my preferred option.

Adding @y21 to the discussion as they are currently working on this lint.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree for adding configuration option for test methods. As for compiler generated functions, I'm not sure whether we should ignore it or not. I doubt if we can give a sensible error message, and even if we can, it would be hard for the user to fix it.

Copy link
Member

@y21 y21 Jun 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, I'm a bit torn on this one. When I initially implemented the lint, the idea was to catch functions that allocate so much stack space that they almost certainly always instantly overflow the stack when called, or are seriously at risk for it. So like most other correctness lints, I think it'd still make sense to lint some macro generated code, since you'd probably still want to know about those functions in your program.

Here's a real world example where a function is generated by a macro that this lint would catch: https://docs.rs/hex/latest/src/hex/lib.rs.html#206
The invocation from_hex_array_impl! { 4294967296 } generates a function that creates a [u8; 4294967296].
So I'd prefer to still lint macro generated functions.

I am honestly a bit surprised about the linked issue. Surely they don't literally run with stack-size-threshold = 0 in a real project. I'd expect these compiler generated functions to use mostly constant or very little stack space.
So I think it's fine to ignore compiler generated functions with a dummy span like this. Any other span, like a span from a macro, at least has enough context for the user that they know where to put an #[expect] to silence the lint

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case, should external macros be filtered or not? For example, in the current test case

#[derive(serde::Deserialize, serde::Serialize)]
struct S {
    a: [u128; 31],
}

will lint, and also this one:

proc_macros::external! {
    fn foo() {
        println!("Hello, world!");
    }
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would personally say it still makes sense to lint external macros. It seems useful to get a warning if there's any function that could accidentally overflow the stack regardless of where it comes from (to the best of its abilities ofc, it will always have some FNs)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

r? @y21 given their ongoing work on this topic.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thought a bit more about it and yeah I think I'm fine with this specifically only ignoring dummy spans. I'd ideally like this lint to warn any function regardless of where it comes from, but if there's nowhere where people can put lint level attrs for a test harness' main function, it means they'd need to have a global #![cfg_attr(test, allow(...))] which seems unfortunate (and as I've mentioned before, it seems rather unlikely that triggering in such a function would actually represent a real problem anyway?), so lgtm

span_lint_and_then(
cx,
LARGE_STACK_FRAMES,
Expand Down
1 change: 1 addition & 0 deletions tests/ui-toml/large_stack_frames_for_generated_fn/clippy.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
stack-size-threshold = 0
15 changes: 15 additions & 0 deletions tests/ui-toml/large_stack_frames_for_generated_fn/large_stack_frames.rs
View file
Open in desktop
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add a short comment at the top of the file that this is specifically testing the test harness' compiler-generated main function which has a dummy span? Might not be immediately obvious to someone looking at this

Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
//@compile-flags: --test
#![allow(unused)]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is needed. -Aunused is always passed to uitest

#![warn(clippy::large_stack_frames)]

fn main() {
//~^ large_stack_frames
println!("Hello, world!");
}

#[cfg(test)]
#[allow(clippy::large_stack_frames)]
mod test {
#[test]
fn main_test() {}
}
16 changes: 16 additions & 0 deletions tests/ui-toml/large_stack_frames_for_generated_fn/large_stack_frames.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
error: this function may allocate 88 bytes on the stack
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this might lead to spurious failures when test suite is run on a 32 bit machine since fmt::Arguments is gonna have a different size (I believe this happens at least in r-l/r if not also here in clippy when trying to merge).

The existing test file already normalizes away differing digits for pointer sizes in the stderr like this:

rust-clippy/tests/ui/large_stack_frames.rs

Line 1 in c5dbd1d

//@ normalize-stderr-test: "\b10000(08|16|32)\b" -> "100$$PTR"

But, given that this is (I assume) mainly just testing that we don't emit a warning, we can just use //@check-pass and remove the main fn so that there's just no output.

--> tests/ui-toml/large_stack_frames_for_generated_fn/large_stack_frames.rs:5:4
|
LL | fn main() {
| ^^^^
LL |
LL | println!("Hello, world!");
| ------------------------- this is the largest part, at 48 bytes for type `std::fmt::Arguments<'_>`
|
= note: 88 bytes is larger than Clippy's configured `stack-size-threshold` of 0
= 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: aborting due to 1 previous error