Skip to content

Move the foreign module #[link] ABI check to attribute parsing - #162373

Open
hizyyo wants to merge 1 commit into
rust-lang:mainfrom
hizyyo:check-link-153101
Open

Move the foreign module #[link] ABI check to attribute parsing#162373
hizyyo wants to merge 1 commit into
rust-lang:mainfrom
hizyyo:check-link-153101

Conversation

@hizyyo

@hizyyo hizyyo commented Sep 6, 2026

Copy link
Copy Markdown

Addresses #153101.

This moves the #[link] check for extern "Rust" blocks from the late HIR pass in rustc_passes to attribute finalization.

The check belongs to attribute validation, but it previously had to wait until HIR because the attribute parser did not know the foreign module's ABI. Instead of storing additional state in the parser or HIR, this change passes the ABI as local context for the current item. The authoritative ABI lowering remains unchanged, including invalid-ABI diagnostics, target validation, and feature gates.

This preserves the existing lint behavior while removing the separate HIR-side check. Regression tests cover the relevant ABI cases, lint levels, multiple #[link] attributes, and invalid-ABI recovery.

LLM disclosure

I proposed the initial approach and developed the main solution collaboratively with an LLM. The LLM helped evaluate the approach against the compiler architecture and review my understanding of the completed changes and their test coverage.

I wrote the implementation and tests myself and independently reviewed the final diff.

@rustbot

rustbot commented Sep 6, 2026

Copy link
Copy Markdown
Collaborator

Some changes occurred in compiler/rustc_attr_parsing

cc @jdonszelmann, @JonathanBrouwer

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

cc @jdonszelmann, @JonathanBrouwer

@rustbot 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 6, 2026
@rustbot

rustbot commented Sep 6, 2026

Copy link
Copy Markdown
Collaborator

Thanks for the pull request, and welcome! The Rust Project has assigned @jieyouxu (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 75 candidates
  • Random selection from 21 candidates

@JonathanBrouwer

Copy link
Copy Markdown
Member

r? me

@rustbot rustbot assigned JonathanBrouwer and unassigned jieyouxu Sep 6, 2026
@JonathanBrouwer

Copy link
Copy Markdown
Member

Hi there, thanks for your contributions and thanks for the LLM disclosure!
Could you take a look at the LLM policy (https://forge.rust-lang.org/policies/llm-usage.html) and verify that you are following its rules?
Specifically, the policy disallows PR descriptions by LLMs from being submitted for review, and the PR description is a bit verbose for my liking

attrs: &[Attribute],
target_span: Span,
target: Target,
foreign_mod_abi: Option<ExternAbi>,

@JonathanBrouwer JonathanBrouwer Sep 6, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm not sure how I like the approach of this approach of passing such specific information into this function. Specifically, I think that approach that this PR takes might also work for this attribute and this doesn't require that. It might make more sense to wait to see for what happens to that PR first

View changes since the review

@JonathanBrouwer

Copy link
Copy Markdown
Member

@rustbot author

@rustbot 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 6, 2026
@rustbot

rustbot commented Sep 6, 2026

Copy link
Copy Markdown
Collaborator

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

@hizyyo

hizyyo commented Sep 6, 2026

Copy link
Copy Markdown
Author

I wrote the PR description myself. I made it so detailed because I take this work very seriously, and I wanted to accurately document the behavior. The llm was used only to the extent specified in the disclosure.
I’m also willing to shorten the description myself if you prefer a shorter version.
I’ve reviewed #161482 and understand the approach using target_item. I’m willing to adapt the PR to this approach and have no objection to having this PR marked as pending. Would you like me to adapt the implementation to target_item right now, or should I leave the current PR on hold pending resolution of #161482?

@JonathanBrouwer

Copy link
Copy Markdown
Member

I'd prefer if you shortened the PR description a bit, for example the "Validation" is not necessary as we have CI, and it's not really relevant to me how you tested your PR locally. I asked about your AI usage since this is a section that AI models tend to include.

More generally, try to limit PR descriptions to a few sentences of "what are you changing", and focus mostly on "why did you take this approach". Anything that I could easily see from reading the code does not need to be in the PR description.

I just did some review on #161482 in order to get it moving again, it's your choice whether you change the implementation of this PR now or wait for #161482 to be merged, but I will wait with reviewing this further until #161482 is merged regardless.

@rustbot label +S-blocked

@rustbot rustbot added the S-blocked Status: Blocked on something else such as an RFC or other implementation work. label Sep 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-attributes Area: Attributes (`#[…]`, `#![…]`) S-blocked Status: Blocked on something else such as an RFC or other implementation work. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants