Split MacArgs in two.

[nnethercote]

MacArgs is an enum with three variants: Empty, Delimited, and Eq. It's used in two ways:

• For representing attribute macro arguments (e.g. in AttrItem), where all three variants are used.
• For representing function-like macros (e.g. in MacCall and MacroDef), where only the Delimited variant is used.

In other words, MacArgs is used in two quite different places due to them having partial overlap. I find this makes the code hard to read. It also leads to various unreachable code paths, and allows invalid values (such as accidentally using MacArgs::Empty in a MacCall).

This commit splits MacArgs in two:

• DelimArgs is a new struct just for the "delimited arguments" case. It is now used in MacCall and MacroDef.
• AttrArgs is a renaming of the old MacArgs enum for the attribute macro case. Its Delimited variant now contains a DelimArgs.

Various other related things are renamed as well.

These changes make the code clearer, avoids several unreachable paths, and disallows the invalid values.

r? @petrochenkov

[rustbot]

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

Some changes occurred in src/tools/rustfmt

cc @rust-lang/rustfmt

[nnethercote]

This shouldn't affect perf, but let's check.

@bors try @rust-timer queue

[bors]

⌛ Trying commit d12ca878a74b0a5115f6d2437aee163d8536ed3a with merge aeff0eacdd9049f2167cbec2bc220d77283e20f2...

[nnethercote]

That's weird... the commit was fine when applied to the earlier revision I was working against, and only broke when applied to a new revision. I thought the tests would have run against the earlier revision? Huh.

@bors try @rust-timer queue

[bors]

⌛ Trying commit 539fe25b517ed1a10b8a3f7e321bf5ec3678abf2 with merge 5c1b781397ab288bb9524bba03b0f8844b4b636c...

[bors]

☀️ Try build successful - checks-actions
Build commit: 5c1b781397ab288bb9524bba03b0f8844b4b636c (5c1b781397ab288bb9524bba03b0f8844b4b636c)

[rust-timer]

Finished benchmarking commit (5c1b781397ab288bb9524bba03b0f8844b4b636c): comparison URL.

Overall result: ❌ regressions - ACTION NEEDED

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf +perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

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

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	5.3%	[2.3%, 8.2%]	2
Improvements ✅ (primary)	-1.1%	[-1.1%, -1.1%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-1.1%	[-1.1%, -1.1%]	1

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	2.4%	[2.1%, 2.9%]	3
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-3.3%	[-3.7%, -2.9%]	2
All ❌✅ (primary)	2.4%	[2.1%, 2.9%]	3

[nnethercote]

The regressions are small enough to be ignored.

@rustbot label: +perf-regression-triaged

[petrochenkov]

From the description it looks similar to closed #96209, I'll check what are the differences.

[petrochenkov]

Could you keep the single-line style for imports, here and in other files?

[nnethercote]

The great thing about rustfmt is that it mostly avoids the need to make arbitrary formatting decisions. But here:

use crate::ast::{AttrId, AttrItem, AttrKind, AttrStyle, Attribute};
use crate::ast::{Lit, LitKind};
use crate::ast::{MacArgs, MacArgsEq, MacDelimiter, MetaItem, MetaItemKind, NestedMetaItem};

I have multiple choices: add to the end of the first line, add to the start of the second line, or even combine the first and second line. And this is a relatively clean example where everything is in alphabetical order. We have lots of examples like this one:

use rustc_ast::{attr, BindingAnnotation, ByRef, Term};
use rustc_ast::{GenericArg, MacArgs, MacArgsEq};
use rustc_ast::{GenericBound, SelfKind, TraitBoundModifier};

where alphabetical order is not maintained across lines. Where should I insert an identifier like DelimArgs here? It could be on any of the lines. That's exactly the kind of arbitrary choice I don't like having to make. The multi-line style avoids this, because rustfmt does it all for you. I think that's why it gets used for cases like this.

Having said that, I'll revert if it's necessary to get r+.

[petrochenkov]

Where should I insert an identifier like DelimArgs here?

Same as with multi-line imports, into its alphabetic order position.

I think that's why it gets used for cases like this.

For cases like that I'd personally use a glob import.

[petrochenkov]

Single-line imports are easily greppable, and that's something I do quite regularly, that's why I keep this style in crates that I review and that already use this style.

[petrochenkov]

Ok, it indeed looks like #96209 light, with MacDelimiter renaming removed and DelimArgs shared.

I'm still somewhat skeptical about this and suspect that we'll need to revert the change at some point, but let's land this for now if you are insisting.
@rustbot author

[nnethercote]

Thanks for being flexible. I still find the AttrArgs vs. MetaItem split confusing, but this helps.

And I think this PR is better than #96209 because the DelimSpan/MacDelimiter/TokenStream trio isn't repeated across both types, thanks to the fact that AttrArgs::Delimited now contains a DelimArgs.

[bors]

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

[petrochenkov]

r=me after fixing the last multi-line import and squashing commits.
@rustbot author

[nnethercote]

@bors r=petrochenkov rollup

[bors]

📌 Commit 3e3a419 has been approved by petrochenkov

It is now in the queue for this repository.