close
Skip to content

Rewrite the #[repr] attribute parser#157125

Merged
rust-bors[bot] merged 4 commits into
rust-lang:mainfrom
JonathanBrouwer:repr-target-checking2
May 30, 2026
Merged

Rewrite the #[repr] attribute parser#157125
rust-bors[bot] merged 4 commits into
rust-lang:mainfrom
JonathanBrouwer:repr-target-checking2

Conversation

@JonathanBrouwer
Copy link
Copy Markdown
Contributor

@JonathanBrouwer JonathanBrouwer commented May 29, 2026

The #[repr] attribute parser was one of the first attribute parsers we made, and didn't use a lot of the awesome infra we have nowadays yet, so in preparation for a new approach I'm trying for #156569 I decided to clean it up.

This PR should have no observable effect other than improved diagnostic messages.

r? @mejrs

@rustbot
Copy link
Copy Markdown
Collaborator

rustbot commented May 29, 2026

Some changes occurred in compiler/rustc_attr_parsing

cc @jdonszelmann

@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 May 29, 2026
cx.adcx().expected_specific_argument(
param.span(),
&[
sym::align,
Copy link
Copy Markdown
Contributor Author

@JonathanBrouwer JonathanBrouwer May 29, 2026

Choose a reason for hiding this comment

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

One thing that still annoys me is that in a lot of attribute parsers have these match statements, and then we need to list all branches of the match statement again for the expected_specific_argument error

View changes since the review

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'd like a solution where these symbols are only listed once.

Maybe make an array of [(Symbol, constructor); _] (or maybe [(Symbol, checker, constructor); _]) and if you don't find the symbol in the array, do something like array.iter().map(|x|x.0).collect() and pass that to expected_specific_argument.

See

const DIAGNOSTIC_ATTRIBUTES: &[(Symbol, Option<Symbol>)] = &[
for inspiration.

Copy link
Copy Markdown
Contributor Author

@JonathanBrouwer JonathanBrouwer May 30, 2026

Choose a reason for hiding this comment

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

(let's leave this for a separate PR tho, since this affects more places that I'd like to fix at the same time)

Copy link
Copy Markdown
Contributor

@mejrs mejrs left a comment

Choose a reason for hiding this comment

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

I'll continue reviewing tomorrow, just some comments so far.

The error code for the cfg_select errors are (and were) unfortunate. I will file an issue for that later.

View changes since this review

cx.adcx().expected_specific_argument(
param.span(),
&[
sym::align,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'd like a solution where these symbols are only listed once.

Maybe make an array of [(Symbol, constructor); _] (or maybe [(Symbol, checker, constructor); _]) and if you don't find the symbol in the array, do something like array.iter().map(|x|x.0).collect() and pass that to expected_specific_argument.

See

const DIAGNOSTIC_ATTRIBUTES: &[(Symbol, Option<Symbol>)] = &[
for inspiration.

Comment thread compiler/rustc_attr_parsing/src/attributes/repr.rs Outdated
Comment thread compiler/rustc_attr_parsing/src/attributes/repr.rs Outdated
Comment thread compiler/rustc_error_codes/src/error_codes/E0552.md Outdated
@theemathas
Copy link
Copy Markdown
Contributor

Does this conflict with #157036 ?

@JonathanBrouwer
Copy link
Copy Markdown
Contributor Author

JonathanBrouwer commented May 30, 2026

Does this conflict with #157036 ?

Not really. The code will have merge conflicts, but that shouldn't be too severe. This PR doesn't have any observable changes other than changes in diagnostics, I'm leaving those for #157036

@JonathanBrouwer JonathanBrouwer force-pushed the repr-target-checking2 branch from f4e215b to 7aef596 Compare May 30, 2026 09:39
Copy link
Copy Markdown
Contributor

@mejrs mejrs left a comment

Choose a reason for hiding this comment

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

@rust-bors
Copy link
Copy Markdown
Contributor

rust-bors Bot commented May 30, 2026

📌 Commit 7aef596 has been approved by mejrs

It is now in the queue for this repository.

@rust-bors rust-bors Bot added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 30, 2026
rust-bors Bot pushed a commit that referenced this pull request May 30, 2026
…uwer

Rollup of 8 pull requests

Successful merges:

 - #156863 (Make hint::cold_path #[cold] so that it works even if the MIR inliner can't inline it)
 - #156875 (Correct and document semantics of `yield` terminator)
 - #157115 ([rustdoc] Fix foreign items macro expansion)
 - #157150 (Revert "drop derive helpers during ast lowering" )
 - #156887 (Rename `-Zdebuginfo-for-profiling` switch)
 - #157039 (rustdoc: correctly propagate cfgs for glob reexports)
 - #157125 (Rewrite the `#[repr]` attribute parser)
 - #157154 (Revert workaround used to select the gcc codegen in the coretests CI)
@rust-bors rust-bors Bot merged commit 737cb2d into rust-lang:main May 30, 2026
12 checks passed
@rustbot rustbot added this to the 1.98.0 milestone May 30, 2026
rust-timer added a commit that referenced this pull request May 30, 2026
Rollup merge of #157125 - JonathanBrouwer:repr-target-checking2, r=mejrs

Rewrite the `#[repr]` attribute parser

The `#[repr]` attribute parser was one of the first attribute parsers we made, and didn't use a lot of the awesome infra we have nowadays yet, so in preparation for a new approach I'm trying for #156569 I decided to clean it up.

This PR should have no observable effect other than improved diagnostic messages.

r? @mejrs
@Kobzol
Copy link
Copy Markdown
Member

Kobzol commented May 30, 2026

@rust-timer build e5f1fe2

Testing for #157161.

@rust-timer
Copy link
Copy Markdown
Collaborator

Missing artifact for sha e5f1fe2c4cab1d4f23eb53ed8755fc15913c8d30 (https://ci-artifacts.rust-lang.org/rustc-builds/e5f1fe2c4cab1d4f23eb53ed8755fc15913c8d30/rustc-nightly-x86_64-unknown-linux-gnu.tar.xz); not built yet, try again later.

@JonathanBrouwer
Copy link
Copy Markdown
Contributor Author

@rust-timer build e5f1fe2

@rust-timer
Copy link
Copy Markdown
Collaborator

Missing artifact for sha e5f1fe2 (https://ci-artifacts.rust-lang.org/rustc-builds/e5f1fe2/rustc-nightly-x86_64-unknown-linux-gnu.tar.xz); not built yet, try again later.

@JonathanBrouwer
Copy link
Copy Markdown
Contributor Author

@rust-timer build e5f1fe2

@rust-timer

This comment has been minimized.

@rust-timer
Copy link
Copy Markdown
Collaborator

Finished benchmarking commit (e5f1fe2): comparison URL.

Overall result: ✅ improvements - 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

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-0.0% [-0.0%, -0.0%] 1
All ❌✅ (primary) - - 0

Max RSS (memory usage)

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

Cycles

Results (secondary 3.4%)

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)
3.4% [3.4%, 3.4%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

Binary size

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

Bootstrap: 515.367s -> 514.529s (-0.16%)
Artifact size: 400.82 MiB -> 400.79 MiB (-0.01%)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-attributes Area: Attributes (`#[…]`, `#![…]`) S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. 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.

6 participants