outer macro skip attribute on out-of-line module ignored; can't use inner attribute

[XrXr]

I have a macro the invocation of which I want rustfmt to skip. Since the macro expands to top level items, I tried to use #![rustfmt::skip::macros(items)] at the module level to do this. That unfortunately breaks compilation because custom inner attributes are unstable. The next thing I tried is to put the attribute on top of the mod my_mod; item, that doesn't work either, maybe because the macro is private to the module. Putting a plain #[rustfmt::skip] on top of the module item does work, but it skips the entire file when I only want to skip a specific invocation.

To illustrate, here is a patch against an empty Git repo you could apply with git am. cargo fmt does not skip the invocation:

From 592ea7bd5114c3208105aebe89929eb4978f2cf3 Mon Sep 17 00:00:00 2001
From: Alan Wu <XrXr@users.noreply.github.com>
Date: Tue, 26 Apr 2022 14:25:37 -0400
Subject: [PATCH] Example project for rustfmt bug report

The `rustfmt::skip::macros` attribute does not seem to work.
---
 Cargo.toml     |  6 ++++++
 src/lib.rs     |  3 +++
 src/private.rs | 16 ++++++++++++++++
 3 files changed, 25 insertions(+)
 create mode 100644 Cargo.toml
 create mode 100644 src/lib.rs
 create mode 100644 src/private.rs

diff --git a/Cargo.toml b/Cargo.toml
new file mode 100644
index 0000000..c0b487c
--- /dev/null
+++ b/Cargo.toml
@@ -0,0 +1,6 @@
+[package]
+name = "rustfmt-skip-macro"
+version = "0.1.0"
+edition = "2021"
+
+[dependencies]
diff --git a/src/lib.rs b/src/lib.rs
new file mode 100644
index 0000000..d1a6314
--- /dev/null
+++ b/src/lib.rs
@@ -0,0 +1,3 @@
+// #[rustfmt::skip]
+#[rustfmt::skip::macros(items)]
+mod private;
diff --git a/src/private.rs b/src/private.rs
new file mode 100644
index 0000000..d484423
--- /dev/null
+++ b/src/private.rs
@@ -0,0 +1,16 @@
+// Custom inner attributes are unstable so I can't use them
+// as suggested by the README.
+// https://github.com/rust-lang/rust/issues/54726
+// #![rustfmt::skip::macros(items)]
+
+// A macro private to this module
+macro_rules! items {
+    ($($arg:item)*) => { $($arg)* };
+}
+
+// I would like rustfmt to skip this invocation
+items! (
+        const _: u8 = 0;
+
+
+);
-- 
2.32.0 (Apple Git-132)

Maybe the example in the tips section of the README should be updated to stop suggesting to use custom inner attributes, but I don't know what would be a better alternative.

I tried on rustfmt 1.4.38-stable (7737e0b5 2022-04-04) and rustfmt 1.4.38-nightly (18f314e7 2022-04-24).

[calebcartwright]

Thanks for reaching out. You've ventured into a lot of topics, most of which are not particularly actionable (for us anyway), but I've tried to respond to them all anyway.

I have a macro the invocation of which I want rustfmt to skip

Have you considered simply using brace delimiters? by design, rustfmt is nearly entirely hands-off by default on calls that use those, and it seems like the path of least resistance for you (e.g. items! { ... })

Maybe the example in the tips section of the README should be updated to stop suggesting to use custom inner attributes, but I don't know what would be a better alternative

I would be fine with augmenting that text to highlight the fact that they're unstable, but I'm adamantly opposed to removing the text altogether just because it can't be used on stable (especially since it was a relatively recent change upstream that caused the tool attributes to no longer work on stable). We've myriad existing features/documentation/etc. that's a nightly/unstable context only, and this is no different.

The next thing I tried is to put the attribute on top of the mod my_mod; item, that doesn't work either, maybe because the macro is private to the module

This is the only item I may want to circle back to on our end. Even for out-of-line mods the outer attributes are available for rustfmt in the context of formatting the associated file. It could be intentional behavior, however, as outer attributes on imports can drive inconsistent behavior, such as formatting the file directly (e.g. editor format on save), or if the module is imported from multiple places with divergent sets of outer attributes.

---

Additionally/alternatively, there have been discussions elsewhere about alternative mechanisms, such as a new config option that allows users to enumerate maccalls to be skipped globally, which I'm certainly still open to as well.

[XrXr]

Thanks for the response and thanks for parsing my confusing story! Using braces works for me. I've changed the title to hopefully make it more searchable and more focused on the actionable part of the issue.

[calebcartwright]

Thanks for the response and thanks for parsing my confusing story! Using braces works for me. I've changed the title to hopefully make it more searchable and more focused on the actionable part of the issue.

Thanks, glad the brace delims are working for you! I made a minor adjustment to the title as well to add more clarity.

@tommilligan - noticed you've sent a few PRs of late, and while I'm not sure if you have the bandwidth or interest in working on anything else, was curious if this is something you'd be willing to try to tackle? 👇

Additionally/alternatively, there have been discussions elsewhere about alternative mechanisms, such as a new config option that allows users to enumerate maccalls to be skipped globally, which I'm certainly still open to as well.

[tommilligan]

@tommilligan - noticed you've sent a few PRs of late, and while I'm not sure if you have the bandwidth or interest in working on anything else, was curious if this is something you'd be willing to try to tackle? point_down

Additionally/alternatively, there have been discussions elsewhere about alternative mechanisms, such as a new config option that allows users to enumerate maccalls to be skipped globally, which I'm certainly still open to as well.

I'd be open to writing up an initial implementation of something, it might take a little while though. To clarify the requirements:

• As a rustfmt user, I would like to specify a list of macros that are not formatted by rustfmt. i.e. these macros are skipped in all code.
  • Skipping formatting means that anything within the enclosing braces/brackets/parentheses of the macro invocation is not formatted.
• These macros are specified in the standard configuration locations (--config, rustfmt.toml etc.) as a flat list of idents.
• A macro is specified by the ident is is invoked with, such as println or panic
  • Macros with this name originally, that are aliased to a new name, will still be formatted (e.g. use core::panic as renamed_panic - renamed_panic would be formatted, even if panic was ignored)

This seems like the most reasonable first pass?

[ytmimi]

Hey @tommilligan, I think your outline of the requirements for a new configuration option are spot on. I also agree that we probably shouldn't need to worry about macro's that have been aliased. At the very least not for a first pass at this. Here is some of what I've found and thoughts for an implementation to hopefully help you out.

When calling rewrite_macro the first thing we do is check if we can bail early, by checking the RewriteContext's SkipContext

rustfmt/src/macros.rs

Lines 151 to 163 in 5ff7b63

	pub(crate) fn rewrite_macro(
	mac: &ast::MacCall,
	extra_ident: Option<symbol::Ident>,
	context: &RewriteContext<'_>,
	shape: Shape,
	position: MacroPosition,
	) -> Option<String> {
	let should_skip = context
	.skip_context
	.skip_macro(context.snippet(mac.path.span));
	if should_skip {
	None
	} else {

(SkipContext API linked for reference)

rustfmt/src/skip.rs

Lines 6 to 33 in a37d3ab

	/// Take care of skip name stack. You can update it by attributes slice or
	/// by other context. Query this context to know if you need skip a block.
	#[derive(Default, Clone)]
	pub(crate) struct SkipContext {
	macros: Vec<String>,
	attributes: Vec<String>,
	}
	impl SkipContext {
	pub(crate) fn update_with_attrs(&mut self, attrs: &[ast::Attribute]) {
	self.macros.append(&mut get_skip_names("macros", attrs));
	self.attributes
	.append(&mut get_skip_names("attributes", attrs));
	}
	pub(crate) fn update(&mut self, mut other: SkipContext) {
	self.macros.append(&mut other.macros);
	self.attributes.append(&mut other.attributes);
	}
	pub(crate) fn skip_macro(&self, name: &str) -> bool {
	self.macros.iter().any(|n| n == name)
	}
	pub(crate) fn skip_attribute(&self, name: &str) -> bool {
	self.attributes.iter().any(|n| n == name)
	}
	}

The main API for adding new items to the skip context is update_with_attrs, which seems to get called in two places:

1. At the start of format_file to capture any top level skips that might be defined on crate,
2. When calling visit_item.

You'll likely need to add a new method to update the skip context from the config.

I think we could update FmtVisitor::from_parse_sess to populate the skip_context from the config. If the users specifies any globally skippable macros. That way the skip_context is populated right away when calling format_file

rustfmt/src/formatting.rs

Lines 181 to 195 in a37d3ab

	// Formats a single file/module.
	fn format_file(
	&mut self,
	path: FileName,
	module: &Module<'_>,
	is_macro_def: bool,
	) -> Result<(), ErrorKind> {
	let snippet_provider = self.parse_session.snippet_provider(module.span);
	let mut visitor = FmtVisitor::from_parse_sess(
	&self.parse_session,
	self.config,
	&snippet_provider,
	self.report.clone(),
	Rc::default(),
	);

[calebcartwright]

I'd be open to writing up an initial implementation of something, it might take a little while though. To clarify the requirements:

Fantastic and certainly no rush. Agreed that the outline looks good, and @ytmimi has also provided some good code hints.

[tommilligan]

Opened an initial implementation of skip_macro_names here: #5347

[ytmimi]

@calebcartwright, I'm wondering if we should close this now that #5347 has been merged or if we should try to come up with a solution that can be used on stable?

[calebcartwright]

The new option (skip_macro_invocations) will be the vehicle through which we'll support this on stable, just a matter of getting that stabilized.

Agreed that we can close this though, folks can subscribe #5346 to track stabilization progress