Fix `large_stack_frames` FP on compiler generated targets #15101

profetia · 2025-06-22T07:44:08Z

The compiler will generate a special main function for the tests, which caused this FP.

changelog: [large_stack_frames] fix FP on compiler generated targets

rustbot · 2025-06-22T07:44:13Z

rustbot has assigned @samueltardieu.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

clippy_lints/src/large_stack_frames.rs

samueltardieu · 2025-06-22T08:49:00Z

clippy_lints/src/large_stack_frames.rs

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


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?

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

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.

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.

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.

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

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!"); } }

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)

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

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

LebedevRI · 2025-06-22T15:12:51Z

@profetia @samueltardieu @y21 thank you!

Reassigned

y21 · 2025-06-26T19:49:57Z

tests/ui-toml/large_stack_frames_for_generated_fn/large_stack_frames.rs

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

y21 · 2025-06-26T20:04:57Z

tests/ui-toml/large_stack_frames_for_generated_fn/large_stack_frames.stderr

@@ -0,0 +1,16 @@
+error: this function may allocate 88 bytes on the stack


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.

y21 · 2025-06-26T20:11:06Z

tests/ui-toml/large_stack_frames_for_generated_fn/large_stack_frames.rs

@@ -0,0 +1,15 @@
+//@compile-flags: --test
+#![allow(unused)]


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

y21 · 2025-06-26T20:14:14Z

clippy_lints/src/large_stack_frames.rs

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


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

rustbot assigned samueltardieu Jun 22, 2025

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Jun 22, 2025

samueltardieu requested changes Jun 22, 2025

View reviewed changes

clippy_lints/src/large_stack_frames.rs Show resolved Hide resolved

rustbot added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Jun 22, 2025

fix: large_stack_frames FP on compiler generated targets

1369367

profetia force-pushed the issue15099 branch from ad9d9e5 to 1369367 Compare June 22, 2025 08:36

profetia requested a review from samueltardieu June 22, 2025 08:37

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties and removed S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Jun 22, 2025

This comment was marked as duplicate.

Sign in to view

samueltardieu previously requested changes Jun 22, 2025

View reviewed changes

rustbot added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Jun 22, 2025

y21 mentioned this pull request Jun 22, 2025

large_stack_frames: unsilenceable diagnostic about compiler-generated functions #15099

Open

rustbot assigned y21 and unassigned samueltardieu Jun 26, 2025

y21 reviewed Jun 26, 2025

View reviewed changes

		@@ -0,0 +1,16 @@
		error: this function may allocate 88 bytes on the stack

Fix large_stack_frames FP on compiler generated targets #15101

Are you sure you want to change the base?

Fix large_stack_frames FP on compiler generated targets #15101

Conversation

profetia commented Jun 22, 2025 • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented Jun 22, 2025

Uh oh!

Uh oh!

This comment was marked as duplicate.

Uh oh!

samueltardieu Jun 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

y21 Jun 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LebedevRI commented Jun 22, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Fix `large_stack_frames` FP on compiler generated targets #15101

Fix `large_stack_frames` FP on compiler generated targets #15101

profetia commented Jun 22, 2025 •

edited by rustbot

Loading

samueltardieu Jun 22, 2025 •

edited

Loading

y21 Jun 22, 2025 •

edited

Loading