Rewrite the new attribute argument parser #144689

JonathanBrouwer · 2025-07-30T16:34:30Z

Fixes #143940

This rewrites the parser, should improve performance and maintainability.
This can be reviewed commit by commit

jdonszelmann · 2025-07-30T18:47:19Z

@bors try @rust-timer queue

rust-bors · 2025-07-30T18:47:23Z

⌛ Trying commit 7b3bb12 with merge acc46aa…

To cancel the try build, run the command @bors try cancel.

Rewrite the new attribute parser

compiler/rustc_attr_parsing/src/parser.rs

tests/ui/attributes/key-value-expansion-on-mac.rs

jdonszelmann · 2025-07-30T19:09:24Z

tests/ui/attributes/malformed-fn-align.stderr

@@ -51,12 +37,26 @@ error[E0589]: invalid alignment value: not a power of two
 LL | #[rustc_align(0)]
   |               ^

+error: expected unsuffixed literal, found `-`


it would be pretty cool if we could explain here what an unsuffixed literal is. Maybe give an example of one with a short explanation. It's not required, but does teach the language better through diagnostics which is always cool

I improved the error a lot :)

error: expected a literal (`1u8`, `1.0f32`, `"string"`, etc.) here, found `-` --> $DIR/malformed-fn-align.rs:29:15 | LL | #[rustc_align(-1)] | ^ | help: negative numbers are not literals, try removing the `-` sign | LL - #[rustc_align(-1)] LL + #[rustc_align(1)] |

tests/ui/attributes/malformed-fn-align.stderr

tests/ui/attributes/nonterminal-expansion.rs

tests/ui/parser/bad-lit-suffixes.stderr

rust-bors · 2025-07-30T21:03:08Z

☀️ Try build successful (CI)
Build commit: acc46aa (acc46aa5d053362bb20ae7f352cba2635ef61120, parent: e5e79f8bd428d0b8d26e8240d718b134ef297459)

rust-timer · 2025-07-30T22:13:56Z

Finished benchmarking commit (acc46aa): comparison URL.

Overall result: ❌✅ regressions and improvements - please read the text below

Benchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please do so in sufficient writing along with @rustbot label: +perf-regression-triaged. If not, please fix the regressions and do another perf run. If its results are neutral or positive, the label will be automatically removed.

@bors rollup=never
@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.2%	[0.2%, 0.2%]	1
Regressions ❌ (secondary)	0.8%	[0.0%, 1.0%]	7
Improvements ✅ (primary)	-0.3%	[-0.5%, -0.1%]	12
Improvements ✅ (secondary)	-0.2%	[-0.2%, -0.1%]	12
All ❌✅ (primary)	-0.2%	[-0.5%, 0.2%]	13

Max RSS (memory usage)

Results (secondary -1.3%)

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.0%	[2.0%, 2.0%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.4%	[-2.9%, -2.0%]	3
All ❌✅ (primary)	-	-	0

Cycles

Results (secondary -0.5%)

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)	4.7%	[4.7%, 4.7%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-3.1%	[-3.7%, -2.5%]	2
All ❌✅ (primary)	-	-	0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 469.342s -> 468.873s (-0.10%)
Artifact size: 376.86 MiB -> 376.85 MiB (-0.00%)

lqd · 2025-07-31T11:42:24Z

(match-stress looks currently noisy to me)

jdonszelmann · 2025-07-31T12:37:56Z

Mhm, we came to the same conclusion (@JonathanBrouwer )

JonathanBrouwer · 2025-07-31T13:41:24Z

tests/ui/issues/issue-32782.rs

@@ -4,7 +4,7 @@ macro_rules! bar (

 macro_rules! foo (
    () => (
-        #[allow_internal_unstable] //~ ERROR allow_internal_unstable side-steps
+        #[allow_internal_unstable()] //~ ERROR allow_internal_unstable side-steps


BREAKING CHANGE:
Previously, attributes on macro calls were partially checked for correct syntax (only what check_builtin_meta_item can check).
After this PR, the attributes are fully parsed.

In any case, these attributes are always completely ignored, except for #[cfg], which was always completely parsed and still is and is not affected by this breaking change.

JonathanBrouwer · 2025-07-31T13:42:24Z

compiler/rustc_parse/src/parser/mod.rs

@@ -1095,7 +1095,7 @@ impl<'a> Parser<'a> {
    /// Parses a comma-separated sequence delimited by parentheses (e.g. `(x, y)`).
    /// The function `f` must consume tokens until reaching the next separator or
    /// closing bracket.
-    fn parse_paren_comma_seq<T>(
+    pub fn parse_paren_comma_seq<T>(


I've had to make quite a few of these functions public, how problematic is that?
I don't think it matters right, or at least it's better than the alternative of copying them over

JonathanBrouwer · 2025-07-31T13:47:17Z

@rustbot ready
r? @jdonszelmann

rustbot · 2025-07-31T13:47:24Z

r? @SparrowLii

rustbot has assigned @SparrowLii.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

rustbot · 2025-07-31T13:47:25Z

jdonszelmann is currently at their maximum review capacity.
They may take a while to respond.

jdonszelmann · 2025-07-31T15:41:41Z

@rust-lang/lang

rust-bors · 2025-07-31T17:43:14Z

☀️ Try build successful (CI)
Build commit: c228194 (c2281943234ec635f9ee34a268a0011c3e897826, parent: 3fb1b53a9dbfcdf37a4b67d35cde373316829930)

jdonszelmann · 2025-07-31T18:18:40Z

@craterbot check

craterbot · 2025-07-31T18:18:53Z

👌 Experiment pr-144689 created and queued.
🤖 Automatically detected try build c228194
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

bors · 2025-07-31T21:03:18Z

☔ The latest upstream changes (presumably #144740) made this pull request unmergeable. Please resolve the merge conflicts.

craterbot · 2025-08-01T08:40:39Z

🚧 Experiment pr-144689 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

craterbot · 2025-08-02T13:35:01Z

🎉 Experiment pr-144689 is completed!
📊 4 regressed and 7 fixed (674726 total)
📰 Open the summary report.

⚠️ If you notice any spurious failure please add them to the denylist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

JonathanBrouwer · 2025-08-02T14:02:39Z

All regressions are spurious.

The crater run did find a bug in my new implementation (in the "fixed" results), which is easy to fix. Apparently my new parser accepts #[cfg(target-os = "windows")] (with a dash instead of an underscore) even though it shouldn't, and the current implementation doesn't.

JonathanBrouwer · 2025-08-02T14:31:53Z

^ Fixed the bug and added a regression test

jdonszelmann · 2025-08-02T19:42:44Z

Well, that's nice, that should make the decision for lang easier. Still would like their opinion though. This is a by far superior way to handle errors in attribute parsing, and we emit more errors when things are wrong. That's good, as long as it doesn't break anyone and it seems the impact will be small.

traviscross · 2025-08-02T21:30:56Z

Previously, attributes on macro calls were partially checked for correct syntax (only what check_builtin_meta_item can check). After this PR, the attributes are fully parsed.

If you could please describe further what it meant for the calls to be partially checked, that would help. I.e., what were we catching and what were the limits of that?

bors · 2025-08-03T04:47:55Z

☔ The latest upstream changes (presumably #144814) made this pull request unmergeable. Please resolve the merge conflicts.

JonathanBrouwer · 2025-08-03T06:31:14Z

@traviscross

Previously for attributes on macro calls we were doing the following:

Parsing the attribute arguments to a MetaItem, as per the spec here: https://doc.rust-lang.org/reference/attributes.html#r-attributes.meta . This would report errors for any attributes that are not valid meta items, such as:

#[inline(1 2 3)]
#[inline(1=2)]

Checking that the attribute arguments match the template provided here

rust/compiler/rustc_feature/src/builtin_attrs.rs

Line 534 in 5b9564a

ungated!(inline, Normal, template!(Word, List: "always|never"), FutureWarnFollowing, EncodeCrossCrate::No),

So inline must have Word or List arguments (so not NameValue), so this is invalid

#[inline = "string"]
#[inline = 5]

Note that this only looks at the Word and List parts, the "always|never" is only provided as a hint to the user and is not something that we check against.

What this PR introduces, is fully validating that the arguments to the attribute actually make sense, using the attributes' individual parsers.
So this is rejected, while it is previously accepted:

#[inline(this, is, not, valid)]
#[inline(this(is="not", valid="either"))]

As a reminder, these attributes on macro calls already have a warning that they are unused and will continue to have that warning.

rustbot added A-attributes Area: Attributes (`#[…]`, `#![…]`) 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. labels Jul 30, 2025

JonathanBrouwer force-pushed the share_parse_path branch from 799d605 to 7b3bb12 Compare July 30, 2025 16:42

This comment has been minimized.

Sign in to view

rust-bors bot added a commit that referenced this pull request Jul 30, 2025

Auto merge of #144689 - JonathanBrouwer:share_parse_path, r=<try>

acc46aa

Rewrite the new attribute parser

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jul 30, 2025

jdonszelmann reviewed Jul 30, 2025

View reviewed changes

This comment has been minimized.

Sign in to view

rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Jul 30, 2025

JonathanBrouwer force-pushed the share_parse_path branch 2 times, most recently from 4c9a573 to 244cefa Compare July 31, 2025 13:39

JonathanBrouwer commented Jul 31, 2025

View reviewed changes

JonathanBrouwer force-pushed the share_parse_path branch from 244cefa to c0f407d Compare July 31, 2025 13:46

JonathanBrouwer marked this pull request as ready for review July 31, 2025 13:47

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 Jul 31, 2025

rustbot assigned SparrowLii and jdonszelmann Jul 31, 2025

craterbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels Jul 31, 2025

craterbot added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 31, 2025

traviscross added P-lang-drag-1 Lang team prioritization drag level 1. https://rust-lang.zulipchat.com/#narrow/channel/410516-t-lang T-lang Relevant to the language team needs-fcp This change is insta-stable, or significant enough to need a team FCP to proceed. labels Jul 31, 2025

craterbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels Aug 2, 2025

JonathanBrouwer added 2 commits August 2, 2025 16:06

Move validate_attr to rustc_attr_parsing

17c5895

Improve should_emit & add parse_limited_all

b13efa6

JonathanBrouwer force-pushed the share_parse_path branch from 01f29bf to f2087c4 Compare August 2, 2025 14:31

This comment has been minimized.

Sign in to view

JonathanBrouwer added 3 commits August 2, 2025 18:18

Rewrite the new attribute parser

732d220

Use the new attribute parser throughout the codebase

d912399

Updated uitests for new parser

ffc2116

JonathanBrouwer force-pushed the share_parse_path branch from f2087c4 to ffc2116 Compare August 2, 2025 16:26

Rewrite the new attribute argument parser #144689

Are you sure you want to change the base?

Rewrite the new attribute argument parser #144689

Uh oh!

Conversation

JonathanBrouwer commented Jul 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jdonszelmann commented Jul 30, 2025

Uh oh!

This comment has been minimized.

rust-bors bot commented Jul 30, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rust-bors bot commented Jul 30, 2025

Uh oh!

This comment has been minimized.

rust-timer commented Jul 30, 2025

Overall result: ❌✅ regressions and improvements - please read the text below

Uh oh!

lqd commented Jul 31, 2025

Uh oh!

jdonszelmann commented Jul 31, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JonathanBrouwer commented Jul 31, 2025

Uh oh!

rustbot commented Jul 31, 2025

Uh oh!

rustbot commented Jul 31, 2025

Uh oh!

jdonszelmann commented Jul 31, 2025

Uh oh!

rust-bors bot commented Jul 31, 2025

Uh oh!

jdonszelmann commented Jul 31, 2025

Uh oh!

craterbot commented Jul 31, 2025

Uh oh!

bors commented Jul 31, 2025

Uh oh!

craterbot commented Aug 1, 2025

Uh oh!

craterbot commented Aug 2, 2025

Uh oh!

JonathanBrouwer commented Aug 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JonathanBrouwer commented Aug 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment has been minimized.

jdonszelmann commented Aug 2, 2025

Uh oh!

traviscross commented Aug 2, 2025

Uh oh!

bors commented Aug 3, 2025

Uh oh!

JonathanBrouwer commented Aug 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

JonathanBrouwer commented Jul 30, 2025 •

edited

Loading

JonathanBrouwer commented Aug 2, 2025 •

edited

Loading

JonathanBrouwer commented Aug 2, 2025 •

edited

Loading

JonathanBrouwer commented Aug 3, 2025 •

edited

Loading