Add unstable format_brace_macros option

[trevyn]

This PR adds an unstable format_brace_macros option for formatting brace {} delimited macro invocations that parse as valid Rust code, with fallback to existing behavior.

Motivation

Users consistently expect some degree of formatting in brace delimited macro invocations. (See links below for examples.)

Discussion

It is true that the problem of formatting macro invocations is not solvable in the general case, but skipping formatting entirely is contrary to user expectations. rustfmt already uses heuristics for parentheses () delimited and bracket [] delimited macros, but does not currently attempt to format brace delimited macro invocations.

By putting this behind an unstable option, we can:

• Do better for many common cases.
• Formally collect a test suite of user-reported expectations.
• Develop an understanding of which non-standard macro syntaxes are common.
• Collect specific cases where formatting produces undesirable results.
• Encourage macro authors to create syntaxes that "play nice" with the formatting rules, or at least don't fail catastrophically.

Options are available for skipping formatting for individual macros, including #[rustfmt::skip] and the newer skip_macro_invocations option, which can skip formatting for all invocations of a specified macro.

Approach

Explicitly, this PR simply attempts to format brace-delimited fn-like macro calls

foo! {"bar"}

as they would be formatted if enclosed in parentheses, which is a common workaround:

foo!({ "bar" })

Yielding the final formatting result:

foo! { "bar" }

This formatting is only applied if the resulting TokenStream is identical to the original.

Future work

• Add additional heuristics for further formatting attempts, such as interpreting contents:
  • as body of a match expression
  • as body of a struct instantiation
  • where body of interior delimiters parses as Rust code
  • etc.

See also

• Does rustfmt format macro calls? #5437
• format macros #8
• [Feature Request] Attempt to format brace macros as Block #3445
• Indentation in macros is handled oddly #4611
• Fmt not working? servo/servo#24696
• format usages of async-stream MaterializeInc/materialize#12562
• cargo fmt broken within proptest macro? proptest-rs/proptest#370
• Closes Option: format named macro usage with standard module/function/... formatter #5254
• Closes Rustfmt cannot format stream! blocks tokio-rs/async-stream#68

[dhardy]

I'd like something like this, but turning on/off formatting for all "block macro" usages per project doesn't seem the way to go.

[trevyn]

My personal opinion is that there should be default formatting applied when it makes sense (as with all other Rust code), but as long as there is an option for a global opt-in, I’m not going to spend too much effort on that battle. :)

[dhardy]

when it makes sense

And when is this? Macros literally invent new syntax. How is rustfmt supposed to know which of those use syntax close enough to Rust to be able to use the same styles?

[trevyn]

Can you provide an example of a macro invocation body that parses as valid Rust code that would not make sense to format as Rust code?

[dhardy]

that parses as valid Rust code

Aha, so this is the key

[dhardy]

I tested:

adds an unstable format_brace_macros option

Shouldn't new options be off by default? This appears to be on by default.

This does not respect inline_attribute_width (#3343). There is another report of that not working within extern blocks, so may be a bug in that however.

Result: kas-gui/kas@b90f126

This formats within calls to impl_scope! (macro used in item position) but not within singleton! (macro used in expression position).

[ytmimi]

@dhardy thanks for taking the time to look into this. Could you provide some code snippets that can be used as test cases?

Shouldn't new options be off by default? This appears to be on by default.

Yes we would want this to be opt-in if we were to move forward with this.

[dhardy]

@ytmimi sure. Here's an example that is formatted correctly by this PR:

impl_scope! {
    /// A frame around content
    ///
    /// This widget provides a simple abstraction: drawing a frame around its
    /// contents.
    #[autoimpl(Deref, DerefMut using self.inner)]
    #[autoimpl(class_traits using self.inner where W: trait)]
    #[derive(Clone, Debug, Default,)]
    #[widget{
        layout = frame(kas::theme::FrameStyle::Frame): self.inner;
    }]
pub struct Frame<W: Widget> {
    core: widget_core!(),
    #[widget] pub inner: W,
}

    impl Self {
        /// Construct a frame
        #[inline]
        pub fn new(
            inner: W
        ) -> Self {
            Frame { core: Default::default(), inner }
        }
    }
}

Here is an example which is not:

fn main() -> kas::shell::Result<()> {
    env_logger::init();

    let window = kas::macros::singleton! {
        #[widget{
            layout = grid: {
                1, 0: "Layout demo";
                2, 0: self.check;
                0..3, 1: ScrollLabel::new(LIPSUM);
                0, 2: align(center): "abc אבג def";
                1..3, 3: align(stretch): ScrollLabel::new(CRASIT);
                0, 3: self.edit;
            };
        }]
        #[derive(Debug)]
        struct {
                core: widget_core!(),
            #[widget] edit = EditBox::new("A small\nsample\nof text")
                .with_multi_line(true),
            #[widget] check: CheckBox,
        }
        impl kas::Window for Self { fn title(&self) -> &str { "Layout demo" }
        }
    };

    let theme = kas::theme::FlatTheme::new();
    kas::shell::Toolkit::new(theme)?.with(window)?.run()
}

(Note: I don't expect rustfmt to do anything with the attributes to the #[widget] macro, at least not at this time. I didn't see any prior examples of this type of syntax.)

[jrose-signal]

I would love if rustfmt could treat brace-macros (opt-in or opt-out) as trying to parse braced-items. That way even if there are things like the unparseable widget attribute, or the yield in async-stream::stream!, you can still say "this is some kind of attribute" or "this is some kind of statement" and format everything around it.

[RReverser]

I'd love to have this functionality. I see it got stuck in review some months ago - is there anything left to move it forward?

[calebcartwright]

Can you provide an example of a macro invocation body that parses as valid Rust code that would not make sense to format as Rust code?

The point @dhardy raised is spot on, and there's various examples where this is problematic.

A simple one is the arg to matches! which is in essence a pattern, but which can also be validly parsed as a binary expression (rustfmt currently does this and formats it accordingly, and the result is subpar). There's also cases where macro args can be parsed as valid Rust and the AST level representation is the same as what a user intended/would expect, but applying the formatting rules is problematic because tokens are necessarily dropped or added as part of the formatting rules (rustfmt knows the addition/removal of certain tokens in valid Rust will be semantics preserving, but it can't in macro args because doing so arbitrarily to any macro cannot be guaranteed to be semantics preserving)

To be clear, I'm not saying that rustfmt can't do better here, nor am I saying that we won't consider something like this. However, my initial reaction is that this is likely too imprecise an approach, and I think there are some more practical (and precise) approaches that are discussed in other issues. Additionally for transparency, this is not something we'll have bandwidth to fully review and evaluate any time soon due to prioritization and limited bandwidth (especially since this has a big question around "should we", on top of the "how"/implementation if so)

[tklajnscek]

Any updates or this or similar functionality? Can we help somehow?
As @RReverser said above this would be a very welcome feature.

We have lots of macros that are just wrappers around regular rust code and the inability to write them using the brace! { ...code... } syntax is unfortunate. We have to fall back to function!({ ...code. }) syntax which IMO is quite ugly :)

[tklajnscek]

@dhardy @ytmimi seems like @trevyn addressed all the outstanding comments with his commits yesterday. Thank you @trevyn! Much appreciated :)

Anything else that needs doing to get this merged?

[dhardy]

I tested with the latest changes and it does nothing to either of the samples above (fails to format).

[trevyn]

@dhardy Your second example has an unnamed struct {, so is not valid Rust and therefore intentionally unformatted. A version of your first example is in the test suite; it looks like the #[derive(Clone, Debug, Default,)] would lose the comma and therefore change the TokenStream, therefore it’s also skipped.

[trevyn]

An “aggressive” vs “conservative” setting would make sense to me.🤪

[RReverser]

Your second example has an unnamed struct {, so is not valid Rust and therefore intentionally unformatted.

Also #[widget] edit = .

[dhardy]

Even this fails to format (using the latest commit in this branch):

$ cargo run --bin rustfmt -- sample1.rs 
    Finished dev [unoptimized + debuginfo] target(s) in 0.03s
     Running `target/debug/rustfmt sample1.rs`
$ cat sample1.rs 
impl_scope! {
    /// A frame around content
    ///
    /// This widget provides a simple abstraction: drawing a frame around its
    /// contents.
    #[derive(Clone, Debug, Default)]
pub struct Frame<W: Widget> {
    pub inner: W,
}

    impl<W: Widget> Frame<W> {
        /// Construct a frame
        #[inline]
        pub fn new(
            inner: W
        ) -> Self {
            Frame { inner }
        }
    }
}

In this case the impl_scope! macro isn't in scope, but IIUC that shouldn't matter to rustfmt.

But my other hacks would need a custom (or slightly more permissive) formatter then? Syntax extensions are the whole purpose of this macro.

[trevyn]

It's off by default now. This worked for me:

cargo run --bin rustfmt -- --config format_brace_macros=true sample1.rs

But my other hacks would need a custom (or slightly more permissive) formatter then? Syntax extensions are the whole purpose of this macro.

Yes, if it doesn't parse as valid Rust code, it's out of scope for this PR. I'd love to work on how to do custom formatting of more exotic macros, but it seems rustfmt is in a bit of a purgatory at the moment.

[ytmimi]

but it seems rustfmt is in a bit of a purgatory at the moment.

Could you expand on this statement? If you're implying that rustfmt isn't being maintained, then I 100% disagree.

[trevyn]

but it seems rustfmt is in a bit of a purgatory at the moment.

Could you expand on this statement? If you're implying that rustfmt isn't being maintained, then I 100% disagree.

My experience is of seeing multiple new PRs open for months that have no feedback at all, and Caleb reiterating how little bandwidth he has available.

I absolutely respect and appreciate your work, I understand that this is volunteer work, and I do not mean to imply that rustfmt is "unmaintained", but surely additional review and design capacity would be useful? It is very helpful and motivating to get concrete feedback and/or merged PRs to learn the codebase so that I could contribute more.

I totally understand if this PR is too crazy to be merged. If I developed a different, more rigorous approach, would that idea or PR be reviewed or discussed?

[ytmimi]

@trevyn thanks for the response. I can sort of see where you're coming from, but the PRs don't tell the whole story. There's a lot of work going on outside the rustfmt repo too. We have regular meetings on Zulip, we're triaging issues that come in, we're managing changes made to rustfmt directly in the rust-lang/rust subtree and syncing them back into rustfmt, etc. It's a lot. Furthermore, I'm personally putting a lot of my free time into the project, and reading comments like "rustfmt is in a bit of a purgatory at the moment" doesn't feel great. Please be mindful before you make comments like that.

Yes, I agree with you that having more review and design capacity would be useful. If you're interested in contributing and learning more about the project I would suggest shooting me a message on Zulip. I can direct you towards issues that I think are more meaningful to address, and ones that I can commit to reviewing.

Rewriting brace macros is a very low priority issue for the team and that's why this is unlikely to get much attention from us right now. I'm going to mark this as on hold for now.

[tklajnscek]

@ytmimi let me chime in here with what's probably going to be a controversial opinion so please don't take this personally. I know everyone is under pressure and the rust project as a whole seems to be understaffed, especially given the amount of attention and user growth it's seeing. It's the price of success!

I think this is the wrong approach. It's not about what's important to your team, but what's important to the users of your software. There will always be niche use cases for any software, but those niches turn out to be unexpectedly large some times and enable a whole new set of use cases.

That's why there are tests and unstable features etc. That's the whole point. Do a review, but don't expect to cover all the obscure corner cases etc. Make sure all the test pass and nothing obvious breaks and let it be. It's unstable so it can be fixed. Don't beat yourself up over it. If it breaks it gets fixed and new tests added to cover the changes. That way the feature gets a place in the codebase, it gets a maintainer (@trevyn) and gets you a potential new contributor of more cool stuff in him, and most importantly, you get new users and use cases out of the software.

If you try to keep absolutely everthing under direct control as a small team, you will likely burn out which is the worst possible outcome for the project.

On this specific feature:
We for one sorely miss this feature and have dropped quite a few very cool macro based ideas in our UI project specifically because there's no way to format those nicely and it just makes the user experience very bad...

I'm sure @trevyn would be more than happy to do the legwork to get this merged, but you need to allow it to happen. You can't just go telling him to work on some other features you need. Frankly, that's a bit condescensing. He worked on this because he needs it. I am willing to work on this, because I need it. Maybe I want to work on some other stuff too, but this is at the top of our list, because we have specific things we'd like to do that are worth a lot to us and we can't do them currently unless we maintain a fork of rustfmt.

Anyways, I hope I managed to get my point across without upsetting people too much. Processes need to be put in place that foster contributions better. I believe all the work you're doing is amazing and a huge value and a core pillar that makes rust amazing to use!