feat: add skip_macro_invocations option

[tommilligan]

Tracking issue: #5346
Original feature request and design: #5324 (comment)

Globally skippable macro names are defined in the config, and added to the existing SkipContext. Thanks to @ytmimi for the pointers on where to implement this, it made the changes super painless!

[ytmimi]

@tommilligan Thanks so much for getting this PR in! The approach you took looks great to me! Just pointed out a few minor things. Let me know if anything is unclear.

I'm still wondering what the best way to expose this new option to users is. As I mentioned in a previous comment, the Ignore list can only be set via the rustfmt.toml and I wonder if we should take a similar approach with this new option.

[calebcartwright]

Thanks so much @tommilligan!

I wanted to echo @ytmimi's question about definitions vs. bodies. The latter was what I understood the original scope to be, and which I believe to be the more important of two. I feel like definitions can be more easily handled via the outer skip attribute (since definition occurs only once), whereas calls can be scattered throughout the codebase.

Additionally, while pondering the above and the implications for the config name, I was also wondering if we should provide a mechanism to disable all calls in a simple manner, e.g. ["*"]?

[tommilligan]

Additionally, while pondering the above and the implications for the config name, I was also wondering if we should provide a mechanism to disable all calls in a simple manner, e.g. ["*"]?

@calebcartwright this is now implemented! Nice idea.

definitions vs. bodies

ref: this thread, I think this may be a question of renaming the option to clarify, if you have any opinion: #5347 (comment)

[tommilligan]

I don't really like this structure, I think it should be refactored into something like

enum SkipMacroContext {
    All,
    Named(HashSet<MacroName>),
}

pub(crate) struct SkipContext {
    macros: SkipMacroContext,
    attributes: HashSet<String>,
}

but thought that was probably out of scope for this PR. Happy to open a followup afterwards, or include as an additional commit if you think it's okay to add here.

[ytmimi]

I think the PR is in a pretty good state and I'm leaning towards making this change in a follow up PR. If you feel strongly about it though and don't think it will add a lot of extra work to the current PR, then I'll leave that call up to you

[tommilligan]

@ytmimi thanks for your comments, they were super helpful. I've implemented or commented on all your points, so ready for another review.

[ytmimi]

@tommilligan Thanks for taking the time to address all my comments. I've been away on a trip for the last two weeks so I haven't had much time for the project, but I'm back now and I plan on going through this again early next week (likely tomorrow!)

[ytmimi]

@tommilligan Thanks for sticking with this. I feel pretty good about the proposed changes, and I'm happy to move forward after we update the name of the option (will also need to update the name on the tracking issue), and add a few more tests. Here are some test cases I have in mind:

• the "*" case -> Expect all macros to be ignored
• explicitly passing an empty list "[]" -> Expect all macros to be formatted
• passing more than one macro name to the ignore list -> Expect all named macro invocations to be formatted.
• calling a macro with a fully qualified path (e.g. self::items), where items is in the ignore list -> Expect that self::items would be ignored
• passing the name of a macro that isn't defined in the source text -> Expect all macros to be formatted
  • This next idea is definitely out of the scope of this PR, but an idea just popped into my head so I figured I'd share. What if the user set items in the ignore list but item! is the name of the macro. We should probably still format it, but maybe there's an opportunity for us to warn the user that the value in their ignore list might have a typo.
• aliasing a macro so that it matches up with the name in the ignore list (e.g. self::items as renamed_items), where renamed_items is in the ignore list. -> Expect the aliased macro to be ignored

[ytmimi]

Also wondering if we could update the help text here to be a little more descriptive. Something like:

rustfmt will not format any macro invocation for macros who's names are set in this list. Setting this value to "*" will prevent any macro invocations from being formatted.

Note: This option does not have any impact on how rustfmt formats macro definitions.

[calebcartwright]

Perhaps this for the opening line?

Suggested change

	Skip formatting the bodies of macro invocations with the following names.
	Specify the names of macro invocations that rustfmt should always skip, regardless of which delimiters are used.

[calebcartwright]

Will just add that I agree with skip_macro_invocations as the name (and have updated the PR title to that effect), agree with the proposed approach in #5347 (comment) but doing so in a follow up, and also agree with the additional test cases outlined in #5347 (review)

Everything else looks to be in good order to me 👍

[tommilligan]

Updated to:

• Expand the documentation
• Rename the option to skip_macro_invocations
• Add test cases as suggested

@ytmimi thanks for your test case list - there were some that I disagreed with:

• calling a macro with a fully qualified path (e.g. self::items), where items is in the ignore list -> Expect that self::items would be ignored
  This isn't the current behaviour - and I don't think it should be. As written, the feature is based purely on the literal invocation name, not on resolving the macro's identity to some more rigorous level - and I think that's fine. I think it's out of scope to handle path resolution.

We could do a really simple pass of 'remove all but the last path segment of the invocation name, but then you couldn't do a more qualified match for e.g. pretty_assertions::assert_eq`. You also wouldn't be able to selectively ignore just some macro invocations with the same name, but qualified by different paths.

• aliasing a macro so that it matches up with the name in the ignore list (e.g. self::items as renamed_items), where renamed_items is in the ignore list. -> Expect the aliased macro to be ignored

This is implicitly tested in the different cases I added, because the use statement doesn't impact this rule, it's just the invocation name.

[ytmimi]

• calling a macro with a fully qualified path (e.g. self::items), where items is in the ignore list -> Expect that self::items would be ignored
  This isn't the current behaviour - and I don't think it should be. As written, the feature is based purely on the literal invocation name, not on resolving the macro's identity to some more rigorous level - and I think that's fine. I think it's out of scope to handle path resolution.

We could do a really simple pass of 'remove all but the last path segment of the invocation name, but then you couldn't do a more qualified match for e.g. pretty_assertions::assert_eq`. You also wouldn't be able to selectively ignore just some macro invocations with the same name, but qualified by different paths.

That's a good point. I'm happy to go along with this behavior, and not ignore fully qualified macros if they aren't explicitly listed. I don't want to expand the scope of the current PR, and maybe this next idea needs to be discussed in more detail, but I wonder if we also need to consider the wild card case for fully qualified paths? Would "self::*" in the ignore list prevent self::item and self::item as renamed from being formatted? Again, I don't want to expand the scope of what we have right now, but I could see users asking for something like this in the future.

• aliasing a macro so that it matches up with the name in the ignore list (e.g. self::items as renamed_items), where renamed_items is in the ignore list. -> Expect the aliased macro to be ignored

This is implicitly tested in the different cases I added, because the use statement doesn't impact this rule, it's just the invocation name.

Yup, I get that it's implicitly tested, but I think having at least one test where we're explicitly testing for it will make it clearer in the future that we've also considered this case.

[tommilligan]

..., I wonder if we also need to consider the wild card case for fully qualified paths? Would "self::*" in the ignore list prevent self::item and self::item as renamed from being formatted? Again, I don't want to expand the scope of what we have right now, but I could see users asking for something like this in the future.

I think that's a reasonable feature request, and one that's compatible with the current design. As those patterns are invalid macro names, I think we're fine to give them special functionality in future without breaking existing behaviour.

I think that functionality would also be reasonable simple to add, but let's keep the scope down for now and wait for someone to ask!

Yup, I get that it's implicitly tested, but I think having at least one test where we're explicitly testing for it will make it clearer in the future that we've also considered this case.

Fair enough - added a new test case file to demonstrate the current behaviour, and ensure use statements don't impact the behaviour (I guess basically a regression test actually)

[ytmimi]

Thanks for adding the additional test! I'm looking to give this another review later this week.

[tommilligan]

Rebased on master to resolve merge conflict.

[calebcartwright]

I think it will probably also be worth mentioning and reinforcing rustfmt's macro formatting behavior anyway. For example, calls that use brace delimiters are essentially already skipped anyway, while calls with paren or bracket delims are attempted to be formatted if some other conditions hold true.

It may not make sense to try to articulate the totality of that behavior here, but if we can come up with a simple way to reference that we can (later) add a link that has more details

[calebcartwright]

I actually went ahead and put up #5437 with a quick and dirty explanation that I think can suffice for now to give us something to point to here. Feel free to wordsmith, but I'd suggest we phrase this something like:

Suggested change

	All macro invocations will be formatted.
	rustfmt will follow its standard approach to formatting macro invocations, and no macro invocations will be skipped based on their name. More information about rustfmt's standard macro invocation formatting behavior can be found in #5437

[tommilligan]

Thanks for the suggestion and the writeup as well, that's nice and clear.

Updated the configurations doc

[ytmimi]

Everything here looks good to me! I think we might consider the rewording proposed in https://github.com/rust-lang/rustfmt/pull/5347/files#r917441100, but I can't think of anything else to add here

[calebcartwright]

Going to go ahead and merge this, thanks for putting this together!

@ytmimi we can make any other changes, like tweaking the config text or adding some tests that use bracket delims in follow ups