Format stable macro_rules

[RReverser]

I took an existing logic for Macros 2.0 and adapted for "legacy" (stable) macro_rules syntax. At this point it seems to produce quite consistent and expected results.

Admittedly the code is quite hacky, but I guess that's what one should expect when attempting to format macros. If not, I'm open to suggestions :)

Issue: #8

[RReverser]

Also there are various edge cases still not covered in either macro (such as inserting $item:item or macro params wrapped in [ / { - not sure why this one is deliberately skipped for now), but I guess such incremental improvements can be added in further PRs / commits.

[RReverser]

@topecongiro Any feedback on this please? Looks like it already started getting conflicts, I'd like to resolve issues if any before it becomes unmergeable.

[topecongiro]

Thank you for your contribution, and sorry for the late review.

The code LGTM, but I am a bit afraid that we may lose comments between macro definitions. Would you make sure that your code works with comments please?
Also would you add more tests please? I would like to see tests for macro_rules with multiple matchers and macro_rules with comments.

For example,

macro_rules! write_html {
    // 1
    ($w:expr, ) => (());
    // 2
    ($w:expr, $e:tt) => (write!($w, "{}", $e));
    // 3
    ($w:expr, $tag:ident [ $($inner:tt)* ] $($rest:tt)*) => {{
        write!($w, "<{}>", stringify!($tag));
        write_html!($w, $($inner)*);
        write!($w, "</{}>", stringify!($tag));
        write_html!($w, $($rest)*);
    }};
}

[RReverser]

I would like to see tests for macro_rules with multiple matchers and macro_rules with comments.

Sure, I'll add some!

[RReverser]

I would like to see tests for macro_rules with multiple matchers

Actually current macro_rules test already has multiple matchers, so I guess I should just add comments?

[RReverser]

You're right, comments outside of branches do get lost. I'll try to find how to fix that.

[RReverser]

@topecongiro Ok, honestly, after looking at examples for other syntax, I still don't fully understand how rustfmt deals with comments and what I should do here to preserve them. Any tips on where to start?

[topecongiro]

Since AST does not include comments, rustfmt needs a data structure called Span, which represents source positions, to recover comments. There are some useful functions in comment.rs (e.g. combine_str_with_missing_comments).

Currently MacroParser returns a Span for macro body, but does not return a Span for macro arguments. You could modify MacroParser to return a Span for macro args as well, and use it to recover comments.

[RReverser]

@topecongiro Okay... this was somewhat hard to figure out due to various issues along the way, but it seems to work correctly and preserve comments now.

The only issue I still have is this spurious Box<Any> panic during self_tests. Here is the meaningful part of the stacktrace:

   5: std::panicking::begin_panic
             at /Users/travis/build/rust-lang/rust/src/libstd/panicking.rs:537
   6: syntax::parse::file_to_filemap
             at /Users/rreverser/.cargo/registry/src/github.com-1ecc6299db9ec823/rustc-ap-syntax-12.0.0/parse/mod.rs:228
   7: syntax::parse::lexer::tokentrees::<impl syntax::parse::lexer::StringReader<'a>>::parse_token_tree::{{closure}}
             at /Users/rreverser/.cargo/registry/src/github.com-1ecc6299db9ec823/rustc-ap-syntax-12.0.0/parse/mod.rs:188
   8: syntax::parse::lexer::tokentrees::<impl syntax::parse::lexer::StringReader<'a>>::parse_token_tree::{{closure}}
             at /Users/rreverser/.cargo/registry/src/github.com-1ecc6299db9ec823/rustc-ap-syntax-12.0.0/parse/mod.rs:160
   9: rustfmt_nightly::should_report_error
             at src/lib.rs:513
  10: rustfmt_nightly::format_input
             at src/lib.rs:620
  11: rustfmt_nightly::should_report_error
             at src/lib.rs:544
  12: rustfmt_nightly::macros::MacroBranch::rewrite
             at src/macros.rs:783
  13: rustfmt_nightly::macros::rewrite_macro_def::{{closure}}
             at src/macros.rs:328

Adding println! shows that it occurs during formatting of this snippet in config.rs:

macro_rules! configuration_option_enum{
    ($e:ident: $( $x:ident ),+ $(,)*) => {
        #[derive(Copy, Clone, Eq, PartialEq, Debug)]
        pub enum $e {
            $( $x ),+
        }

        impl_enum_serialize_and_deserialize!($e, $( $x ),+);
    }
}

which was expanded into

#[derive(Copy, Clone, Eq, PartialEq, Debug)]
        pub enum ze {
            $zx ),+
        }

        impl_enum_serialize_and_deserialize!(ze, $zx ),+);

I guess the reason for failure is missing repetition support from your other branch so Rust can't parse $zx but I don't understand why this doesn't fail on master?

UPD Oh wait, it doesn't fail on master because it doesn't attempt to parse macro body... I still don't get why it panics though instead of just returning Err(diagnostic).

[topecongiro]

@RReverser AFAIK rustc calls panic! or panictry! when it encounters unrecoverable error while lexing or parsing, then somehow catches it and exits gracefully. rustfmt does not have this mechanism, so it panics against certain invalid code (cc #753).

[topecongiro]

I think using write_list is a gread idea here. Thanks for the update!

[RReverser]

Thank you!

FWIW one hazard I ran into was that write_list has special handling for , and : when cleaning up post comment chunk, but not for ; so it took a while to figure out why ; was being inserted in the output twice - turns out, it treated original ; as part of the post comment information, and so wrote it out after actual list item plus separator.

I could add ; to that condition in write_list but decided to go with a less invasive approach and just include it into the branch span.

However, in future, I think, it might be worth to change write_list to skip given separator argument just like it currently skips hardcoded list of chars.

[RReverser]

Anyway, the panic aside, do you think this PR is acceptable in the current form?

[RReverser]

@nrc @topecongiro Ping?

[topecongiro]

I am sorry for the late review.

I think the code looks good overall. It would be great if you could add more tests to make sure that nothing is going wrong.

[RReverser]

Sure. What would you like to see covered? So far I covered multiple branches including empty match, empty return, comments in various locations and some repetition matching. Can't think of some other special cases to cover off the top of my head (apart from bunch of those that are just not yet implemented for neither version of macro syntax)

[topecongiro]

Yeah it will be great if those cases are covered in tests 😉

[topecongiro]

Oh, never mind, you already added tests for those cases in macro_rules.rs. I somehow missed those files... I am sorry for bothering you.

[topecongiro]

Would you please rebase agsinst master, and remove or comment out tests for macro def with repeat and add FIXME so that we can merge this PR?

[RReverser]

Ok rebased and ran cargo run cargo-fmt to format all the macros in the codebase, now tests pass fine.

[topecongiro]

Thank you!