Always Place Spaces after Commas in Macro Match Arms

[InsertCreativityHere]

This PR fixes #5573 by ensuring a space is always placed after commas in macro match arms,
and updates a test that exhibited the mistake.

[ytmimi]

overall I think the change looks good. I'm curios how the following is formatted:

macro_rules! x {
    ($a:ident, $b:ident,) => {};
}

Do we add a space after the trailing comma? Does it make sense to do so?

[InsertCreativityHere]

Does it make sense to do so?

Personally, I think it looks better without a space between the trailing comma and parenthesis.
So with your example, I'd prefer it remains unchanged.

Do we add a space after the trailing comma?

Nope. In fact we remove it if it's present, although this seems like it was already true before my PR.

macro_rules! x {
    ($a:ident, $b:ident,) => {};     // Unchanged
-   ($a:ident, $b:ident, ) => {};   // Removes the space
+   ($a:ident, $b:ident,) => {};
}

The macro-args-parser parses the stuff between the parenthesis separately (with a new parser):

rustfmt/src/macros.rs

Lines 913 to 915 in e041c20

	// Parse the stuff inside delimiters.
	let parser = MacroArgParser::new();
	let delimited_arg = parser.parse(tts.clone())?;

And it trims any whitespace off the end.

[ytmimi]

I appreciate you looking into this for me. I just wanted to double check that was the case since it would have been unfortunate if we added a space after the trailing comma. I'm not sure if there is a test case with a trailing comma, but if there isn't would you mind adding one, thanks!

[InsertCreativityHere]

I just wanted to double check

Absolutely! Honestly, I hadn't even considered that edge case, so it's good you thought it up!

but if there isn't would you mind adding one

Just added one! I think you're right, we should totally have that anyways, regardless of this PR.

Since the commit for the extra test is unrelated to these changes, it feels cleaner to keep the commits separate.
So maybe better to rebase this instead of squash merging, but only if it's not a hassle : v)

[ytmimi]

Thanks for including the additional test case. Before I forget, I also want to say thanks for your first contribution to rustfmt 🎉

Since the commit for the extra test is unrelated to these changes, it feels cleaner to keep the commits separate.
So maybe better to rebase this instead of squash merging, but only if it's not a hassle : v)

Sure, we'll figure that out once upstream stuff has been sorted out and we're ready to merge.

Just as a side note, rustfmt's goal is to be an implementation of The Rust Style Guide. The section on macro_rules! is quite sparse, and doesn't go into any detail on what to do about the macro arms.

The rustfmt team tries not to make style determinations ourselves so we may also need to add some additional detail to the guide before moving forward. @calebcartwright what are your thoughts? Can we move forward without an addition to the guide?

The only place in the guide where I found some discussion on commas between metavariables were the last few comments on rust-lang/style-team#121, but that discussion hasn't been active for a very long time.

Barring a potential addition to the style guide I think we're good to go here!

[InsertCreativityHere]

Before I forget, I also want to say thanks for your first contribution to rustfmt

Thanks! And right back at you for putting so much effort into maintaining it!

Sure, we'll figure that out once upstream stuff has been sorted out and we're ready to merge.

Totally cool. Just let me know if there's anything else you want me to do!

The section on macro_rules! is quite sparse

"quite sparse", you really aren't kidding!!
I can propose an addition to the style guideline, but honestly, I think it's better if macro formatting is always gated behind an unstable option (currently the case). They're just too weird and flexible IMO.

I only opened this PR because the macro formatting logic already exists, and my coworkers opted-into it
(with format_macro_matchers) and these small formatting oddities bother me alot more than they probably should : v)

[InsertCreativityHere]

Alsooo, while I have your attention ^-^ do you have any opinion on: #3354 (comment)?

I pro-actively offered to fix the problem he was describing, but after thinking about it more, now I'm not completely sure if it's sane to always remove spaces after . characters... Maybe that's reaching too far for rustfmt.

[ytmimi]

I can propose an addition to the style guideline, but honestly, I think it's better if macro formatting is always gated behind an unstable option (currently the case). They're just too weird and flexible IMO.

@calebcartwright would know more about whether or not a style guide amendment is needed before we can move forward here. Yeah macros are tricky since they can contain any valid tokens that might not necessarily be valid rust syntax, while rustfmt is primarily designed to format valid rust syntax. When macros introduce invalid syntax that's when rustfmt struggles to format them.

only opened this PR because the macro formatting logic already exists, and my coworkers opted-into it
(with #3354) and these small formatting oddities bother me alot more than they probably should : v)

I think it's great that your coworkers are playing around with unstable options, and feedback on their behavior is always appreciated! If I've learned anything from working on the project it's that developers are very passionate about their formatting preferences. I don't think we'll ever get to a point where everyone is 100% satisfied by rustfmts output 😅

[ytmimi]

Alsooo, while I have your attention ^-^ do you have any opinion on: #3354 (comment)?

I pro-actively offered to fix the problem he was describing, but after thinking about it more, now I'm not completely sure if it's sane to always remove spaces after . characters... Maybe that's reaching too far for rustfmt.

I can't say I've spent a lot of time thinking about wether or not there should be whitespace around . characters in macro matchers. It sounds like you might have some thoughts on it though. Are there scenarios where you think we should maintain whitespace? Maybe it's best to continue this discussion over on the linked issue so we don't go too off topic here.

[tgross35]

@InsertCreativityHere I think this may fix the single-line issue I mentioned in #5984, but do you know if it may also fix the multiline issue?

[tgross35]

@ytmimi can this merge as-is? I think a change for . can come separately if needed.

[ytmimi]

@tgross35 I looked into this and this seems to resolve the single-line issue you brought up in #5984, but this doesn't resolve the multi-line case. When I've got some time I'll try to dig into what's going on there.

input:

macro_rules! foo {
    (foo: $foo:ident,bar: $bar:ident,baz: $baz:ident,qux: $qux:ident,) => {};
}

output:

macro_rules! foo {
    (foo: $foo:ident, bar: $bar:ident, baz: $baz:ident, qux: $qux:ident,) => {};
}

@ytmimi can this merge as-is?

I think so. I'd still like to discuss this with Caleb and see if he has any reservations, but I'm in favor of moving forward.

[InsertCreativityHere]

I think this may fix the single-line issue I mentioned in #5984, but do you know if it may also fix the multiline issue?

As @ytmimi points out, my fix was pretty specific, only for commas!
But, your problem seems pretty related. I'll (probably) take a look at it over the weekend,
although I don't know if I'll be able to fix it. I'm definitely not a rustfmt expert : vP

I think so. I'd still like to discuss this with Caleb and see if he has any reservations, but I'm in favor of moving forward.

I'll update my PR to be up-to-date with main over the weekend too.
And let me know if there's anything else you think I should do to get this PR back into an acceptable state!
Since I'd also appreciate the fix being merged in! : v)

My coworkers religiously remove rustfmt::skip's, so I'm still stuck looking at the broken formatting, 1.5 years later ;-;

[ytmimi]

@InsertCreativityHere we try to avoid merge commits if possible. When you have a second can you rebase your changes instead?

[InsertCreativityHere]

@ytmimi For sure! I just rebased my commits over master.
I'm just too used to squash merging my PRs : v)