Skip to content

Spaces before colons in macro_rules? #2534

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
ghost opened this issue Mar 16, 2018 · 16 comments · Fixed by #5452
Closed

Spaces before colons in macro_rules? #2534

ghost opened this issue Mar 16, 2018 · 16 comments · Fixed by #5452

Comments

@ghost
Copy link

ghost commented Mar 16, 2018

Before formatting:

macro_rules! foo {
    ($a:ident : $b:ty) => {};
    ($a:ident $b:ident $c:ident) => {};
}

After formatting:

macro_rules! foo {
    ($a: ident: $b: ty) => {};
    ($a: ident $b: ident $c: ident) => {};
}

This formatting makes the macro patterns difficult to read.

I understand why rustfmt adds a space after every :, and it makes a lot of sense when arguments are separated by commas or semicolons. But if arguments are separated by spaces or other kinds of delimiters, the end result is not great. :(

cc @RReverser

@RReverser
Copy link
Contributor

I also agree it's not perfect when it's not a list of comma-separated arguments but that's how it was implemented before me in initial version f86f6dc by @nrc , so I decided not to change that bit when extending support for macros 1.1/1.0.

@topecongiro
Copy link
Contributor

As much as I prefer to keep formatting consistent between function definitions and macro_rules, IMHO we should not add a space after : by default, because it makes parsing arguments by human eyes harder. Unlike function definitions where arguments are separated by commas, arguments for macros can be separated by anything, or even there could be no separators.
Also I rarely find a code where a space is added after comma in the wild.

topecongiro added a commit to topecongiro/rustfmt that referenced this issue Mar 18, 2018
@nrc nrc closed this as completed in adc257f Mar 19, 2018
@ghost
Copy link
Author

ghost commented Mar 19, 2018

This was fixed by adding a space before the colon. But I wonder: should we add spaces before some other characters, too?

Here it sort of looks like ident is a function name because paratheses are sticked to it:

($name: ident($($dol: tt $var: ident)*) $($body: tt)*) => {};

But if we add a space, it's clearer that ident is actually the type of $name:

($name: ident ($($dol: tt $var: ident)*) $($body: tt)*) => {};

I'd probably prefer to always add a space after the token type, unless it is followed by one of ,;)]}.

@RReverser
Copy link
Contributor

I'd probably rather not add spaces neither before nor after column (which seems what @topecongiro agreed with above but implemented differently?).

That way rustfmt would be consistent with most of the existing macro code in the wild, including the formatting in the Rust Book that probably inspired everyone else. https://doc.rust-lang.org/book/first-edition/macros.html

@dtolnay dtolnay reopened this Mar 24, 2018
@dtolnay
Copy link
Member

dtolnay commented Mar 24, 2018

Reopening because the commit that closed this did not address where the discussion seemed to be heading -- that as much as it may be motivated by the formatting of function definitions, macro syntax has different requirements and we should not be inserting whitespace after the metavariable colon. I agree with @topecongiro's and @RReverser reasoning.

macro_rules! foo {
    ($a:ident : $b:ty) => {};
    ($a:ident $b:ident $c:ident) => {};
}

@petrochenkov
Copy link
Contributor

I always reformat macros as functions when I touch them in code (and use appropriate lower/upper-case for macro parameters), it makes them looking much more like normal code and therefore more readable for a non-macro-expert.

I agree with @stjepang though that colons used as separators and not as a part of $param: type should have an extra space, i.e.

macro_rules! foo {
    ($a: ident : $T: ty) => {};
    ($a: ident $b: ident $c: ident) => {};
}

including the formatting in the Rust Book that probably inspired everyone else

We should fix the book, IMO.

@topecongiro
Copy link
Contributor

@dtolnay The first PR adds a space before non-metavariable colon which comes after metavariable. The following PR (#2549) removes a space after metavariable colon. So the issue is fixed on the master.

We have not settled on how we should format macro arguments. I think that we have three options to choose from:

  1. Never put a space after metavariable colon ($a:ident).
  2. Always put a space after metavariable colon ($a: ident).
  3. Heuristically choose whether to put a space after metavariable colon (e.g. put a space if metavariables are separated by commas).

Currently rustfmt follows 1.

@dtolnay
Copy link
Member

dtolnay commented Mar 25, 2018

Thank you! That PR had not been linked to this issue before. Looking forward to a release.

@nrc nrc changed the title Suboptimal formatting in macro_rules Spaces before colons in macro_rules? Jun 24, 2018
@nrc nrc added this to the 1.0 milestone Jul 10, 2018
@aturon
Copy link
Member

aturon commented Jul 10, 2018

Related to #2753, in that we may want to punt on this formatting via a flag for now.

@nrc
Copy link
Member

nrc commented Jul 18, 2018

Removing from milestone since we no longer format macro matchers by default (90c5792)

@nrc nrc removed this from the 1.0 (preview 2) milestone Jul 18, 2018
@RReverser
Copy link
Contributor

we no longer format macro matchers by default

@nrc Why not? The attached commit just mentions #2543 but that seems unrelated to macro matchers.

@nrc
Copy link
Member

nrc commented Jul 18, 2018

So we can decide on the formatting later - there is no clear answer here and we're approaching the cut-off for 1.0, so I think we need to punt.

@RReverser
Copy link
Contributor

I see. Too bad, I really wanted macros to be autoformatted (which is why I PRd it in the first place) but understandable that exact rules need more care and time. I will still be able to opt-in to their formatting with rustfmt.toml in the meanwhile, right?

@RReverser
Copy link
Contributor

Oh wait, if I'm reading the diff correctly, this also affects just the matching definitions, but bodies will be still formatted?

@nrc
Copy link
Member

nrc commented Jul 18, 2018

this also affects just the matching definitions, but bodies will be still formatted?

That's the defaults, yes. You'll be able to change both in rustfmt.toml

@RReverser
Copy link
Contributor

Sounds perfect and reasonable, thanks.

ytmimi added a commit to ytmimi/rustfmt that referenced this issue Jul 19, 2022
Closes rust-lang#2534

The behavior described in the original issue can no longer be
reproduced. The tests show that to be the case regardless of if
`format_macro_matchers` is `true` or `false`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants