Move #[naked] attribute check to attribute parsing stage

[RichardTjokroutomo]

View all comments

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

LLM disclosure: I wrote the code by hand & took inspiration from #161482. However, I did use LLM to learn how attribute checking is done in Rustc.

[rustbot]

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]

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

[JonathanBrouwer]

This should never fail right? So please make this panic

View changes since the review

[RichardTjokroutomo]

After implementing panic, through the failing CI I just found out that methods defined under traits & impls are represented as associated item instead of normal item.... lucky...

[JonathanBrouwer]

same with this

View changes since the review

[JonathanBrouwer]

Can we avoid duplicating this logic?
Perhaps by parsing the abi into a ExternAbi instead?

View changes since the review

[RichardTjokroutomo]

I don't think we can. I originally tried to convert it to ExternAbi, but I couldn't compile as it creates circular dependency (haven't checked deeper as to why, though).

Then I saw similar check for CanonAbi, and I thought since the logic is already duplicated elsewhere, perhaps I can also add similar method to Extern.

[mejrs]

I'll doubly insist that this logic should not be duplicated.

[RichardTjokroutomo]

OK; I added a helper method to convert extern to externAbi

[rustbot]

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

[RichardTjokroutomo]

hi @JonathanBrouwer @mejrs , do you mind taking another look?

[mejrs]

Sorry we've let this stall a bit. I was already getting around to reviewing again 😅

View changes since this review

[mejrs]

Can this be a doc comment?

[mejrs]

Suggested change

	"`#[naked]` is currently unstable on `extern \"{}\"` functions",
	abi.as_str()
	"`#[naked]` is currently unstable on `extern {abi}` functions",

[mejrs]

The downside of having Item instead of ItemKind etc here is that lets you access the AttrVec, NodeId etc, which I'm not sure I like. It seems a bit unnecessary (you'll probably end up accessing item.kind 99.9% of the time anyway, and it's a bit dangerous in that someone could accidentally smuggle these (and unlowered Spans) into later compilation stages.

Suggested change

	pub enum AstTarget<'a> {
	AssocItem(&'a Item<AssocItemKind>),
	ForeignItem(&'a Item<ForeignItemKind>),
	Item(&'a Item),
	pub enum AstTarget<'a> {
	AssocItem(&'a AssocItemKind),
	ForeignItem(&'a ForeignItemKind),
	Item(&'a ItemKind),

[mejrs]

Here we have:

if cx.target != Target::Struct {
return;
}

        match cx.ast_target {
            rustc_attr_ir::target::AstTarget::Item(ast_item) => {
                let ItemKind::Struct(_, _, data) = &ast_item.kind else {
                    panic!("expected struct AST target item for Target::Struct");
                };
                if let VariantData::Struct { fields, .. } = data
                    && fields.iter().any(|f| f.default_value().is_some())
                {
                    cx.emit_err(NonExhaustiveWithDefaultFieldValues {
                        attr_span,
                        defn_span: cx.target_span,
                    });
                }
            }
            _ => {}
}

Is this equivalent to this?

        match cx.ast_target {
            rustc_attr_ir::target::AstTarget::Item(ast_item) if let ItemKind::Struct(_, _, data) = &ast_item.kind => {
                if let VariantData::Struct { fields, .. } = data
                    && fields.iter().any(|f| f.default_value().is_some())
                {
                    cx.emit_err(NonExhaustiveWithDefaultFieldValues {
                        attr_span,
                        defn_span: cx.target_span,
                    });
                }
            }
            _ => {}

(Same for some other matches)

[RichardTjokroutomo]

done. hopefully I didn't miss any...

[RichardTjokroutomo]

Thanks for the review! PTAL again when you have time.

@rustbot ready

[mejrs]

I'm happy to approve this as is, will wait a moment for @JonathanBrouwer to take a look as well.

View changes since this review

[JonathanBrouwer]

I was taking a look at this earlier today but wasn't entirely confident on adding AstTarget as a duplicate of Target.
I think it would be cool if we could remove Target and completely replace it by AstTarget, but we shouldn't do this in this PR as that would be a lot of work.
I'll further review this PR wednesday

[mejrs]

I was taking a look at this earlier today but wasn't entirely confident on adding AstTarget as a duplicate of Target. I think it would be cool if we could remove Target and completely replace it by AstTarget, but we shouldn't do this in this PR as that would be a lot of work. I'll further review this PR wednesday

What I've been considering is to just not give attribute parsers themselves access to Target, we'd probably want to still use it for AllowedTargets though as I don't want to do "clever" things there.

[RichardTjokroutomo]

but we shouldn't do this in this PR as that would be a lot of work.

In that case, I'll just do a rebase for this PR. Would be happy to work on it next though :)

[rustbot]

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.