Move #[naked] attribute check to attribute parsing stage by RichardTjokroutomo · Pull Request #162530 · rust-lang/rust

RichardTjokroutomo · 2026-09-09T12:24:57Z

Following #161482's idea to add target_item field to FinalizeCheckContext, replace target_item with ast_target, which is an enum that contains all possible types of target Item (obtained by grepping all functions that call lower_attrs()).

This change is needed as methods defined under traits & impls are represented as ast::AssocItem. Lastly, move check_naked to the callback returned by NakedParser::deferred_finalize_check().

Part of #153101. r?@JonathanBrouwer

rustbot · 2026-09-09T12:25:00Z

Some changes occurred in compiler/rustc_passes/src/check_attr.rs

cc @jdonszelmann, @JonathanBrouwer

Some changes occurred in compiler/rustc_attr_parsing

cc @jdonszelmann, @JonathanBrouwer

rustbot · 2026-09-09T12:25:04Z

Thanks for the pull request, and welcome! The Rust Project has assigned @nnethercote (or someone else) to review your changes, you should hear from them (or someone else) within the next two weeks.

Please see the contribution instructions and our LLM policy for more information.

Why was this reviewer chosen?

The reviewer was selected based on:

Owners of files modified in this PR: compiler
compiler expanded to 76 candidates
Random selection from 22 candidates

rustbot · 2026-09-11T09:15:31Z

Reminder, once the PR becomes ready for a review, use @rustbot ready.

rustbot · 2026-10-06T09:07:25Z

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

RichardTjokroutomo · 2026-10-06T09:12:48Z

I just read the LLM guidelines and found out disclosure is needed only if a part of the PR (code, PR desc, etc) is generated by LLM.

Since I only use it to learn the codebase, I'll remove the disclosure.

JonathanBrouwer · 2026-10-07T09:48:06Z

+    WherePredicate(&'a WherePredicate),
+
+    /// Used when it is not possible to get detailed information about the target.
+    None,


Yeah that makes sense, we can have a 'AstTarget::None` for this case then to keep things simple

JonathanBrouwer · 2026-10-07T09:55:18Z

I just read the LLM guidelines and found out disclosure is needed only if a part of the PR (code, PR desc, etc) is generated by LLM.

Yeah as long as reviewers are not seeing any AI generated output you don't need to disclose.
Disclosure of that is still allowed tho ofc :)

Signed-off-by: Richard Tjokroutomo <richard.tjokro2@gmail.com>

JonathanBrouwer · 2026-10-07T13:11:47Z

+            &l.attrs,
+            l.span,
+            Target::Statement,
+            rustc_attr_ir::target::AstTarget::None,


Why can't this be AstTarget::Statement?

View changes since the review

Sorry... should've checked the caller function

JonathanBrouwer · 2026-10-07T13:11:52Z

+            &attrs,
+            span,
+            Target::Expression,
+            rustc_attr_ir::target::AstTarget::None,


Why can't this be AstTarget::Expression?

View changes since the review

I didn't check caller functions, sorry....

JonathanBrouwer · 2026-10-07T13:12:56Z

+                    &param.attrs,
+                    param.span,
+                    Target::Param,
+                    AstTarget::None,


Why can't this be AstTarget::Pram?

View changes since the review

Sorry... I was being careless..

JonathanBrouwer · 2026-10-07T13:13:42Z

+                        &f.attrs,
+                        f.span,
+                        Target::ExprField,
+                        AstTarget::Expression(expr),


I don't think this is correct

View changes since the review

JonathanBrouwer · 2026-10-07T13:14:43Z

+#[derive(Clone, Copy, Debug)]
+pub enum AstTarget<'a> {
+    // Target types that may correspond to different kinds of items.
+    Delegation { target: DelegationAstTarget<'a>, mac: bool },


Also here I'm leaning towards that we don't need the fields of this, and it adds a lot of complexity

View changes since the review

JonathanBrouwer · 2026-10-07T13:17:27Z

+    pub fn from_ast_item(kind: &'a ast::ItemKind) -> Self {
+        match kind {
+            ast::ItemKind::ExternCrate(..) => AstTarget::ExternCrate(kind),
+            ast::ItemKind::Use(..) => AstTarget::Use(kind),


I'd prefer if we map each ItemKind to its field where reasonable.
Like have AstTarget::Use take a UseTree rather than a ast::ItemKind.

View changes since the review

JonathanBrouwer · 2026-10-07T13:17:59Z

+    Union(&'a ItemKind),
+    Trait(&'a ItemKind),
+    TraitAlias(&'a ItemKind),
+    Impl { item: &'a ItemKind, of_trait: bool },


Do we need this of_trait field? That should already be stored in the Impl right?

View changes since the review

yeah. It's irrelevant now after the most recent commit

Signed-off-by: Richard Tjokroutomo <richard.tjokro2@gmail.com>

RichardTjokroutomo · 2026-10-07T15:28:36Z

@JonathanBrouwer I think everything is correct now. Let's just wait for CI results...

Previously there were a lot of changes when making AstTarget map 1-1 with Target, so TBH, at some point I stopped giving full attention when passing AstTarget as argument of various functions. I'm really sorry for this, but thankfully you catched them before merging

RichardTjokroutomo · 2026-10-08T00:56:16Z

@rustbot ready

JonathanBrouwer · 2026-10-09T07:36:23Z

@bors try @rust-timer queue

JonathanBrouwer · 2026-10-09T07:40:17Z

+    Closure(&'a Closure),
+    Expression(&'a Expr),
+    ForLoop(&'a ForLoop),
+    Loop,


Are these targets (ForLoop, Loop, While, Break) ever produced?
If not lets remove them, and accept that this is a mismatch with Target

View changes since the review

Could you also check for the other AstTargets if they're used?

If not lets remove them, and accept that this is a mismatch with Target

So we won't be replacing Target with AstTarget?

For now, there's only one case where we absolutely need AstTarget::None, and I think it is possible to replace it since AstTarget::MacroCall no longer carry detailed info of the AST target.

I also think that we can remove the fields of the enum types that we don't need (e.g. ForLoop), so we can still replace Target with AstTarget in the future. Just my thoughts though. I'm fine with removing them.

Could you also check for the other AstTargets if they're used?

Most are unused. They were only added since we wanted to make AstTarget map 1-1 with Target.
Edit: turns out not that many.

@JonathanBrouwer what are your thoughts?

For now, I'm gonna go ahead with your suggestion and remove all unused AstTarget types on a new commit. If you have different thoughts we can always hard reset to previous commit :)

JonathanBrouwer · 2026-10-09T07:43:04Z

+    Field(&'a FieldDef),
+    GenericParam(&'a GenericParam),
+    LifetimeParam(&'a GenericParam),
+    Local(&'a Local),


Is Local still used?

View changes since the review

No. It's only there to make 1-1 matching

JonathanBrouwer · 2026-10-09T07:46:38Z

-                ExternAbi::Rust
-            })
-        });
+                abi.symbol_unescaped.as_str().parse().unwrap_or_else(|_| {


What happened to the indent here?

It would be nice if you could self-review (go through the diff of the PR) your changes to at least catch these kind of things, before submitting it for review

View changes since the review

Oops.

Sorry, I thought the CI tests everything including tidiness, so I assumed if all checks pass then everything is good.

JonathanBrouwer · 2026-10-09T07:48:21Z

            target_span,
            target,
-            None,
+            rustc_attr_ir::target::AstTarget::None,


nit: I'd prefer if everywhere (not just here) we could import AstTarget and make this AstTarget::None
(don't import the variants)

View changes since the review

rust-bors · 2026-10-09T08:41:10Z

☀️ Try build successful (CI)
Build commit: b2e91b7 (b2e91b7f17cefe008d1fa62606cc5049d9b68899)
Base parent: 76c9095 (76c90957b7e422c4b9c45192b0197214d7de5a54)

rust-timer · 2026-10-09T09:25:10Z

Finished benchmarking commit (b2e91b7): comparison URL.

Overall result: no relevant changes - no action needed

Benchmarking means the PR may be perf-sensitive. Consider adding rollup=never if this change is not fit for rolling up.

@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

This perf run didn't have relevant results for this metric.

Max RSS (memory usage)

This perf run didn't have relevant results for this metric.

Cycles

Results (secondary 2.7%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	2.7%	[2.1%, 3.4%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Binary size

This perf run didn't have relevant results for this metric.

Bootstrap: 490.84s -> 489.964s (-0.18%)
Artifact size: 406.48 MiB -> 406.53 MiB (0.01%)

Signed-off-by: Richard Tjokroutomo <richard.tjokro2@gmail.com>

RichardTjokroutomo · 2026-10-10T07:06:41Z

@rustbot ready

rustbot added A-attributes Area: Attributes (`#[…]`, `#![…]`) S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Sep 9, 2026

rustbot assigned nnethercote Sep 9, 2026

RichardTjokroutomo force-pushed the check-attr-naked branch from 57ae8f4 to c65aa6f Compare September 9, 2026 12:26

rustbot assigned JonathanBrouwer and unassigned nnethercote Sep 9, 2026

This comment has been minimized.

Sign in to view

RichardTjokroutomo force-pushed the check-attr-naked branch from c65aa6f to 4c2e629 Compare September 9, 2026 13:43

This comment has been minimized.

Sign in to view

RichardTjokroutomo force-pushed the check-attr-naked branch from 4c2e629 to ee7a87a Compare September 9, 2026 13:48

RichardTjokroutomo changed the title ~~move check_naked() from check_attr.rs to codegen_attrs.rs~~ Move #[naked] attribute check to attribute parsing stage Sep 10, 2026

RichardTjokroutomo force-pushed the check-attr-naked branch from ee7a87a to 9d144c2 Compare September 10, 2026 06:56

This comment has been minimized.

Sign in to view

JonathanBrouwer reviewed Sep 11, 2026

View reviewed changes

Comment thread compiler/rustc_attr_parsing/src/attributes/codegen_attrs.rs

JonathanBrouwer reviewed Sep 11, 2026

View reviewed changes

Comment thread compiler/rustc_attr_parsing/src/attributes/codegen_attrs.rs

JonathanBrouwer reviewed Sep 11, 2026

View reviewed changes

Comment thread compiler/rustc_ast/src/ast.rs Outdated

JonathanBrouwer reviewed Sep 11, 2026

View reviewed changes

Comment thread compiler/rustc_ast_lowering/src/lib.rs Outdated

JonathanBrouwer requested changes Sep 11, 2026

View reviewed changes

rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 11, 2026

RichardTjokroutomo force-pushed the check-attr-naked branch from 9d144c2 to 7e93b9a Compare September 11, 2026 12:00

This comment has been minimized.

Sign in to view

RichardTjokroutomo force-pushed the check-attr-naked branch from 7e93b9a to cdc980c Compare September 11, 2026 13:49

This comment has been minimized.

Sign in to view

RichardTjokroutomo force-pushed the check-attr-naked branch from cdc980c to 4fbadae Compare September 11, 2026 13:57

RichardTjokroutomo force-pushed the check-attr-naked branch from 2d331a2 to 566fa74 Compare October 6, 2026 09:07

JonathanBrouwer requested changes Oct 7, 2026

View reviewed changes

rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 7, 2026

rebase latest changes & addressing latest comments

35b1fab

Signed-off-by: Richard Tjokroutomo <richard.tjokro2@gmail.com>

RichardTjokroutomo force-pushed the check-attr-naked branch from 566fa74 to f12bf6b Compare October 7, 2026 12:49

JonathanBrouwer requested changes Oct 7, 2026

View reviewed changes

RichardTjokroutomo force-pushed the check-attr-naked branch from f12bf6b to 4280fe0 Compare October 7, 2026 13:34

some tidy ups

b567ca0

Signed-off-by: Richard Tjokroutomo <richard.tjokro2@gmail.com>

RichardTjokroutomo force-pushed the check-attr-naked branch from 4280fe0 to b567ca0 Compare October 7, 2026 13:55

remove Item, AsocItem, & ForeignItem

bba59d2

Signed-off-by: Richard Tjokroutomo <richard.tjokro2@gmail.com>

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 (such as code changes or more information) from the author. labels Oct 8, 2026

RichardTjokroutomo requested a review from JonathanBrouwer October 8, 2026 00:56

This comment has been minimized.

Sign in to view

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Oct 9, 2026

This comment has been minimized.

Sign in to view

JonathanBrouwer requested changes Oct 9, 2026

View reviewed changes

This comment has been minimized.

Sign in to view

RichardTjokroutomo added 2 commits October 9, 2026 22:36

pass AstTarget::Expression too

62543f6

Signed-off-by: Richard Tjokroutomo <richard.tjokro2@gmail.com>

remove unused AstTarget types & some tidy ups

2176bb5

Signed-off-by: Richard Tjokroutomo <richard.tjokro2@gmail.com>

Conversation

RichardTjokroutomo commented Sep 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented Sep 9, 2026

Uh oh!

rustbot commented Sep 9, 2026

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rustbot commented Sep 11, 2026

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

rustbot commented Oct 6, 2026

Uh oh!

RichardTjokroutomo commented Oct 6, 2026

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

JonathanBrouwer commented Oct 7, 2026

Uh oh!

JonathanBrouwer Oct 7, 2026 • edited by rustbot 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!

JonathanBrouwer Oct 7, 2026 • edited by rustbot 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!

JonathanBrouwer Oct 7, 2026 • edited by rustbot 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!

JonathanBrouwer Oct 7, 2026 • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JonathanBrouwer Oct 7, 2026 • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JonathanBrouwer Oct 7, 2026 • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JonathanBrouwer Oct 7, 2026 • edited by rustbot 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!

RichardTjokroutomo commented Oct 7, 2026

Uh oh!

RichardTjokroutomo commented Oct 8, 2026

Uh oh!

JonathanBrouwer commented Oct 9, 2026

Uh oh!

This comment has been minimized.

This comment has been minimized.

JonathanBrouwer Oct 9, 2026 • edited by rustbot 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

RichardTjokroutomo commented Sep 9, 2026 •

edited

Loading

JonathanBrouwer Oct 7, 2026 •

edited by rustbot

Loading

JonathanBrouwer Oct 7, 2026 •

edited by rustbot

Loading

JonathanBrouwer Oct 7, 2026 •

edited by rustbot

Loading

JonathanBrouwer Oct 7, 2026 •

edited by rustbot

Loading

JonathanBrouwer Oct 7, 2026 •

edited by rustbot

Loading

JonathanBrouwer Oct 7, 2026 •

edited by rustbot

Loading

JonathanBrouwer Oct 7, 2026 •

edited by rustbot

Loading

JonathanBrouwer Oct 9, 2026 •

edited by rustbot

Loading

RichardTjokroutomo Oct 9, 2026 •

edited

Loading

JonathanBrouwer Oct 9, 2026 •

edited by rustbot

Loading

JonathanBrouwer Oct 9, 2026 •

edited by rustbot

Loading

JonathanBrouwer Oct 9, 2026 •

edited by rustbot

Loading