Skip to content

Remove skip_arg attribute from Diagnostic and Subdiagnostic proc-macros#157070

Open
GuillaumeGomez wants to merge 2 commits into
rust-lang:mainfrom
GuillaumeGomez:rm-skip_arg
Open

Remove skip_arg attribute from Diagnostic and Subdiagnostic proc-macros#157070
GuillaumeGomez wants to merge 2 commits into
rust-lang:mainfrom
GuillaumeGomez:rm-skip_arg

Conversation

@GuillaumeGomez
Copy link
Copy Markdown
Member

@GuillaumeGomez GuillaumeGomez commented May 28, 2026

Instead of having users to manually add #[skip_arg] for each field that is not used in fluent messages, I think it's better to instead let the proc-macro only call diag.arg("name", field) on the fields actually used.

r? @JonathanBrouwer

@GuillaumeGomez
Remove skip_arg attribute from Diagnostic and Subdiagnostic pro…
8f53ae3 
…c-macros, skipping fields based on whether or not they are used in error messages
@GuillaumeGomez
Remove #[skip_arg] references from rustc-dev-guide
14b45f0
@rustbot
Copy link
Copy Markdown
Collaborator

rustbot commented May 28, 2026

The rustc-dev-guide subtree was changed. If this PR only touches the dev guide consider submitting a PR directly to rust-lang/rustc-dev-guide otherwise thank you for updating the dev guide with your changes.

cc @BoxyUwU, @tshepang

Some changes occurred to the CTFE machinery

cc @RalfJung, @oli-obk, @lcnr

rustc_macros::diagnostics was changed

cc @TaKO8Ki, @JonathanBrouwer

The parser was modified, potentially altering the grammar of (stable) Rust
which would be a breaking change.

cc @fmease

Some changes occurred in match checking

cc @Nadrieril

@rustbot rustbot added A-rustc-dev-guide Area: rustc-dev-guide A-translation Area: Translation infrastructure, and migrating existing diagnostics to SessionDiagnostic 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 28, 2026
@GuillaumeGomez
Copy link
Copy Markdown
Member Author

GuillaumeGomez commented May 28, 2026

I didn't expect to ping half the compiler team though. O.o

@JonathanBrouwer
Copy link
Copy Markdown
Contributor

JonathanBrouwer commented May 28, 2026 •
edited
Loading

@bors try @rust-timer queue
Just in case

@rust-timer

This comment has been minimized.

Sign in to view
@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label May 28, 2026
@rust-bors

This comment has been minimized.

Sign in to view
rust-bors Bot pushed a commit that referenced this pull request May 28, 2026
@bors
Auto merge of #157070 - GuillaumeGomez:rm-skip_arg, r=<try>
8f93c9d 
Remove `skip_arg` attribute from `Diagnostic` and `Subdiagnostic` proc-macros
@rust-bors
Copy link
Copy Markdown
Contributor

rust-bors Bot commented May 28, 2026

☀️ Try build successful (CI)
Build commit: 8f93c9d (8f93c9d17d1ef0821b932d7ec77d1f88c1687baa, parent: b5e038d7158c1af55a646027fdacf5ecd7c783c7)

@rust-timer

This comment has been minimized.

Sign in to view
@rust-timer
Copy link
Copy Markdown
Collaborator

rust-timer commented May 28, 2026

Finished benchmarking commit (8f93c9d): 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)

Results (primary 15.5%, secondary -1.7%)

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

mean range count
Regressions ❌
(primary)		 15.5% [15.5%, 15.5%] 1
Regressions ❌
(secondary)		 - - 0
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 -1.7% [-2.5%, -0.8%] 2
All ❌✅ (primary) 15.5% [15.5%, 15.5%] 1

Cycles

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

Binary size

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

Bootstrap: 512.125s -> 509.812s (-0.45%)
Artifact size: 400.67 MiB -> 400.68 MiB (0.00%)

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label May 28, 2026
JonathanBrouwer
JonathanBrouwer approved these changes May 29, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@JonathanBrouwer JonathanBrouwer left a comment •
edited by rustbot
Loading

Choose a reason for hiding this comment

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

@bors r+ rollup

While this doesn't have the expected perf effect (or it does but it's not measurable), I still think getting rid of skip_arg is worthwhile as it simplifies the usage and implementation of the macro

View changes since this review

@rust-bors
Copy link
Copy Markdown
Contributor

rust-bors Bot commented May 29, 2026

📌 Commit 14b45f0 has been approved by JonathanBrouwer

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 29, 2026
JonathanBrouwer added a commit to JonathanBrouwer/rust that referenced this pull request May 29, 2026
@JonathanBrouwer
Rollup merge of rust-lang#157070 - GuillaumeGomez:rm-skip_arg, r=Jona…
04b6367 
…thanBrouwer

Remove `skip_arg` attribute from `Diagnostic` and `Subdiagnostic` proc-macros

Instead of having users to manually add `#[skip_arg]` for each field that is not used in fluent messages, I think it's better to instead let the proc-macro only call `diag.arg("name", field)` on the fields actually used.

r? @JonathanBrouwer
@JonathanBrouwer JonathanBrouwer mentioned this pull request May 29, 2026
Closed
rust-bors Bot pushed a commit that referenced this pull request May 29, 2026
@bors
Auto merge of #157109 - JonathanBrouwer:rollup-I3kBnQT, r=JonathanBro…
d49de3c 
…uwer

Rollup of 9 pull requests

Successful merges:

 - #156963 (definitions: remove `DefPathTable`, use `LocalDefId` instead of `DefIndex`)
 - #157053 (Eagerly resolve delegations in late resolution)
 - #157068 (NVPTX: Remove the unstable ptx linker flavor)
 - #157076 (Various proc-macro related code cleanups)
 - #157100 (Some more per owner things)
 - #157065 (Stabilize `Path::is_empty`)
 - #157070 (Remove `skip_arg` attribute from `Diagnostic` and `Subdiagnostic` proc-macros)
 - #157088 (Improve suggestions for malformed deprecated attribute)
 - #157103 (Add reproducibly failing tests for parallel frontend)
JonathanBrouwer added a commit to JonathanBrouwer/rust that referenced this pull request May 29, 2026
@JonathanBrouwer
Rollup merge of rust-lang#157070 - GuillaumeGomez:rm-skip_arg, r=Jona…
26e3bc4 
…thanBrouwer

Remove `skip_arg` attribute from `Diagnostic` and `Subdiagnostic` proc-macros

Instead of having users to manually add `#[skip_arg]` for each field that is not used in fluent messages, I think it's better to instead let the proc-macro only call `diag.arg("name", field)` on the fields actually used.

r? @JonathanBrouwer
@JonathanBrouwer JonathanBrouwer mentioned this pull request May 29, 2026
Closed
rust-bors Bot pushed a commit that referenced this pull request May 29, 2026
@bors
Auto merge of #157113 - JonathanBrouwer:rollup-OQIpn3C, r=JonathanBro…
1aa8c32 
…uwer

Rollup of 11 pull requests

Successful merges:

 - #156960 (Some cleanups around passing extra lifetime params from the resolver to ast lowering)
 - #156963 (definitions: remove `DefPathTable`, use `LocalDefId` instead of `DefIndex`)
 - #157053 (Eagerly resolve delegations in late resolution)
 - #157068 (NVPTX: Remove the unstable ptx linker flavor)
 - #157076 (Various proc-macro related code cleanups)
 - #157106 (add ABI check logic for wasm)
 - #154835 (std::offload sharedmem)
 - #157065 (Stabilize `Path::is_empty`)
 - #157070 (Remove `skip_arg` attribute from `Diagnostic` and `Subdiagnostic` proc-macros)
 - #157088 (Improve suggestions for malformed deprecated attribute)
 - #157103 (Add reproducibly failing tests for parallel frontend)

Failed merges:

 - #157100 (Some more per owner things)
@JonathanBrouwer
Copy link
Copy Markdown
Contributor

JonathanBrouwer commented May 29, 2026

@bors r-
#157113 (comment)
(why does this not fail PR ci?)

@rust-bors rust-bors Bot added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label May 29, 2026
@rust-bors rust-bors Bot removed the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label May 29, 2026
@rust-bors
Copy link
Copy Markdown
Contributor

rust-bors Bot commented May 29, 2026

This pull request was unapproved.

This PR was contained in a rollup (#157113), which was unapproved.

View changes since this unapproval

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

Reviewers

@JonathanBrouwer JonathanBrouwer JonathanBrouwer approved these changes

Assignees

@JonathanBrouwer JonathanBrouwer

Labels

A-rustc-dev-guide Area: rustc-dev-guide A-translation Area: Translation infrastructure, and migrating existing diagnostics to SessionDiagnostic 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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@GuillaumeGomez @rustbot @JonathanBrouwer @rust-timer