MacroArgParser Discards Spaces After Commas Sometimes

[InsertCreativityHere]

Example

This is the minimal example of the issue I was running into, but some of the tests (talked about below),
also do this, despite having 2 meta-variables.

 macro_rules! test_macro {
-    ($child:expr, Module) => {
+    ($child:expr,Module) => {
         let x = 11;
     };
 }

Version

rustfmt 1.5.1-nightly (a24a020e 2022-10-18)
Running on Windows 10 (21H2)

Discussion

There are tests that validate that spaces after commas ARE removed. This line:

rustfmt/tests/source/macro_rules.rs

Line 30 in ef91154

macro_rules! $n { ($va:expr, $vb:expr) => { $n($va, $vb) } }

gets reformatted to:

rustfmt/tests/target/macro_rules.rs

Lines 45 to 49 in ef91154

	macro_rules! $n {
	($va: expr,$vb: expr) => {
	$n($va, $vb)
	};
	}

But this seems suspicious,
the test was added in #2542 "Put spaces around braces", and this is completely unrelated to brace spacing. Additionally, the 'source' does have the space in it.
All the tests affected by this weird formatting were added in the commit.

Either way, not having a space after the comma looks pretty weird to me.
Everywhere else in Rust I'm aware of, commas should be followed by whitespace (or a newline).

Proposed Solution

I wanted to make sure this was an issue before opening a PR, but I've fixed this locally.
The MacroArgParser calls next_space to determine whether to place a space:

rustfmt/src/macros.rs

Lines 1055 to 1079 in ef91154

	fn next_space(tok: &TokenKind) -> SpaceState {
	debug!("next_space: {:?}", tok);
	match tok {
	TokenKind::Not
	| TokenKind::BinOp(BinOpToken::And)
	| TokenKind::Tilde
	| TokenKind::At
	| TokenKind::Comma
	| TokenKind::Dot
	| TokenKind::DotDot
	| TokenKind::DotDotDot
	| TokenKind::DotDotEq
	| TokenKind::Question => SpaceState::Punctuation,
	TokenKind::ModSep
	| TokenKind::Pound
	| TokenKind::Dollar
	| TokenKind::OpenDelim(_)
	| TokenKind::CloseDelim(_) => SpaceState::Never,
	TokenKind::Literal(..) | TokenKind::Ident(..) | TokenKind::Lifetime(_) => SpaceState::Ident,
	_ => SpaceState::Always,
	}

By removing TokenKind::Comma from the match it will fall into the default case.
So it returns Always instead of Punctuation, so a space is always placed after it.

I think this makes sense compared to the other punctuation in the list,
since it's pretty standard to place a space after commas.

Honestly, this fix feels deceptively simple, but all tests pass (except those mentioned above),
and everything I've thrown at my build so far seems fine.

Feel free to point me in the right direction if this fix is totally bogus though!! : v)

[ytmimi]

Thanks for taking the time to outline the issue.

There are tests that validate that commas ARE removed

I don't see any commas being removed. Do you mean space after commas are removed?

The formatting you've described does seem a bit strange 🤔. Are there any rustfmt options that are required to trigger the formatting you've described above? PRs are always welcome!

[InsertCreativityHere]

Thanks for taking the time to outline the issue.

Thanks for taking the time to read through it all : vP

I don't see any commas being removed. Do you mean space after commas are removed?

Yep! Totally my bad... That's exactly what I meant (Edited it to say that instead).

Are there any rustfmt options that are required to trigger the formatting you've described above?

Just format_macro_matchers, since without that it leaves the match arms alone.

Alright, I'll open a PR for this then, just wanted to make sure I wasn't totally off base here!

[ytmimi]

Just format_macro_matchers, since without that it leaves the match arms alone.

Thanks for clarifying. linking the tracking issue for format_macro_matchers #3354

[InsertCreativityHere]

Thanks for all the help @ytmimi!
I opened a PR with my suggested fixes!