fix [`large_stack_arrays`] linting in `vec` macro #12624

J-ZhengLi · 2024-04-03T03:44:25Z

this PR also adds a wrapper function matching_root_macro_call to clippy_utils::macros, considering how often that same pattern appears in the codebase.

(I'm always very indecisive towards naming, so, if anyone have better idea of how that function should be named, feel free to suggest it)

changelog: fix [large_stack_arrays] linting in vec macro; add matching_root_macro_call to clippy_utils

rustbot · 2024-04-03T03:44:30Z

r? @xFrednet

rustbot has assigned @xFrednet.
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

J-ZhengLi · 2024-04-03T06:51:41Z

Actually... on a second thought, this could still has FP when the vec! is not the root macro call, such as:

some_macro!(vec![x, x, x, x, x]);

But then if I check the first expanded macro call it will have FP with:

vec![some_macro_that_reduces_x_frequency!(x;5)];

So, what will be the best approach here, do I need to check all the macro calls?

xFrednet · 2024-04-04T09:03:40Z

But then if I check the first expanded macro call it will have FP with:
vec![some_macro_that_reduces_x_frequency!(x;5)];
So, what will be the best approach here, do I need to check all the macro calls?

We could check if the span of the x expressions comes from a different macro and abort if it does. That sounds like the best solution for me

J-ZhengLi · 2024-04-04T09:13:50Z

We could check if the span of the x expressions comes from a different macro and abort if it does. That sounds like the best solution for me

Ok I see~ should I keep that newly added utils function tho? Since this solution won't need it anymore.

oh wait... you meant check the root macro to see if it's vec, then iterates macro calls again to find any non-vec macro call, is that correct?

xFrednet · 2024-04-04T09:21:04Z

should I keep that newly added utils function tho?

I like the new function, it makes the code cleaner, so please keep it.

oh wait... you meant check the root macro to see if it's vec, then iterates macro calls again to find any non-vec macro call, is that correct?

I meant to check the span of x to see if it's used directly in the vec![] macro or if it's created and copied from some other macro. I'm always a bit uncertain, how rustc models spans but this information should be available somewhere 🤔

J-ZhengLi · 2024-04-10T10:49:10Z

vec![some_macro_that_reduces_x_frequency!(x;5)];

well... I though I can write such function-like macro to only produce some tokens, in this case x, x, x, x, x. But seems like I was wrong, it doesn't allows me 🤦‍♂️ , I've tried both using proc_macro and macro_rules!...

So I'm guessing the xs can only be created and copied from some other macro? If true then checking if there's a vec call in macro backtrace should be good right?

xFrednet · 2024-04-11T19:37:09Z

There is the quote::quote_spanned macro which allows you to modify the span of tokens. But Clippy usually doesn't handle these cases since they're quite messy. Maybe we have a test macro in Clippy with this?

If true then checking if there's a vec call in macro backtrace should be good right?

I'd say so. We can just run lintcheck and see what it spits out :D

…t_macro_call` function in `clippy_utils`

…r false help messages if in one.

J-ZhengLi · 2024-04-17T10:29:34Z

tests/ui/large_stack_arrays.rs

+    let y = proc_macros::make_it_big!([x; 1]);
+    //~^ ERROR: allocating a local array larger than 512000 bytes


Well... in this test case, the expr being checked is [x; 1], but I guess since the span of 1 was used in quote_spanned(?), for some reason even is_from_proc_macro(cx, expr) fails.

oh, runing lintcheck has no differences unfortunately, both gives 4 occurrences lol

I would guess that those occurrences are the result of incorrect quote_spanned usage. Let's get this version merged for now.

J-ZhengLi · 2024-04-17T10:32:13Z

clippy_lints/src/large_stack_arrays.rs

+    /// Check if the span of `ArrayLen` of a repeat expression is within the expr's span,
+    /// if not, meaning this repeat expr is definitely from some proc-macro.
+    ///
+    /// This is a fail-safe to a case where even the `is_from_proc_macro` is unable to determain the
+    /// correct result.
+    fn repeat_expr_might_be_expanded<'tcx>(cx: &LateContext<'tcx>, expr: &Expr<'tcx>) -> bool {
+        let ExprKind::Repeat(_, ArrayLen::Body(anon_const)) = expr.kind else {
+            return false;
+        };
+        let len_span = cx.tcx.def_span(anon_const.def_id);
+        !expr.span.contains(len_span)
+    }


therefore I have to use this hacky way to check, but I don't know if this can always work or just coincidentally succeed in my specific test case.

I think this hack is good enough. quote_spanned is sadly pretty bad for rust tools, we just have to hope that users of quote_spanned understand what they're doing

xFrednet · 2024-04-27T09:28:57Z

Thank you for the update :D

@bors r+

bors · 2024-04-27T09:29:00Z

📌 Commit 2861729 has been approved by xFrednet

It is now in the queue for this repository.

bors · 2024-04-27T09:30:23Z

⌛ Testing commit 2861729 with merge c6bf954...

bors · 2024-04-27T09:38:13Z

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: xFrednet
Pushing c6bf954 to master...

rustbot assigned xFrednet Apr 3, 2024

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Apr 3, 2024

J-ZhengLi force-pushed the issue12586 branch from 7a94fb2 to f3aedd1 Compare April 8, 2024 11:22

J-ZhengLi marked this pull request as draft April 9, 2024 12:15

J-ZhengLi force-pushed the issue12586 branch 2 times, most recently from 2d1d7b2 to 5824c4c Compare April 10, 2024 07:53

J-ZhengLi marked this pull request as ready for review April 10, 2024 10:18

J-ZhengLi marked this pull request as draft April 15, 2024 10:04

J-ZhengLi marked this pull request as ready for review April 16, 2024 07:36

J-ZhengLi force-pushed the issue12586 branch 3 times, most recently from 6630ead to efef920 Compare April 16, 2024 09:38

fix [large_stack_arrays] linting in vec macro & add `matching_roo…

666e2f2

…t_macro_call` function in `clippy_utils`

J-ZhengLi force-pushed the issue12586 branch 3 times, most recently from 027b1b3 to e70c892 Compare April 17, 2024 10:09

test [large_stack_arrays] with proc-macro and make sure not to offe…

2861729

…r false help messages if in one.

J-ZhengLi force-pushed the issue12586 branch from e70c892 to 2861729 Compare April 17, 2024 10:11

J-ZhengLi commented Apr 17, 2024

View reviewed changes

xFrednet approved these changes Apr 27, 2024

View reviewed changes

bors merged commit c6bf954 into rust-lang:master Apr 27, 2024
5 checks passed

This was referenced Apr 27, 2024

fix suggestion error for [manual_is_ascii_check] with missing type #11988

Merged

Fix FormatArgs storage when -Zthreads > 1 #12567

Merged

		let y = proc_macros::make_it_big!([x; 1]);
		//~^ ERROR: allocating a local array larger than 512000 bytes

fix [large_stack_arrays] linting in vec macro #12624

fix [large_stack_arrays] linting in vec macro #12624

Uh oh!

Conversation

J-ZhengLi commented Apr 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented Apr 3, 2024

Uh oh!

J-ZhengLi commented Apr 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xFrednet commented Apr 4, 2024

Uh oh!

J-ZhengLi commented Apr 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xFrednet commented Apr 4, 2024

Uh oh!

J-ZhengLi commented Apr 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xFrednet commented Apr 11, 2024

Uh oh!

J-ZhengLi Apr 17, 2024

Choose a reason for hiding this comment

Uh oh!

J-ZhengLi Apr 17, 2024

Choose a reason for hiding this comment

Uh oh!

xFrednet Apr 27, 2024

Choose a reason for hiding this comment

Uh oh!

J-ZhengLi Apr 17, 2024

Choose a reason for hiding this comment

Uh oh!

xFrednet Apr 27, 2024

Choose a reason for hiding this comment

Uh oh!

xFrednet commented Apr 27, 2024

Uh oh!

bors commented Apr 27, 2024

Uh oh!

bors commented Apr 27, 2024

Uh oh!

bors commented Apr 27, 2024

Uh oh!

Uh oh!

Uh oh!

fix [`large_stack_arrays`] linting in `vec` macro #12624

fix [`large_stack_arrays`] linting in `vec` macro #12624

J-ZhengLi commented Apr 3, 2024 •

edited

Loading

J-ZhengLi commented Apr 3, 2024 •

edited

Loading

J-ZhengLi commented Apr 4, 2024 •

edited

Loading

J-ZhengLi commented Apr 10, 2024 •

edited

Loading