Don't insert semicolons inside of a macro_rules! arm body

Currently, rustfmt inserts semicolons for certain trailing expressions
(`return`, `continue`, and `break`). However, this should not be done in
the body of a `macro_rules!` arm, since the macro might be used in
expression position (where a trailing semicolon will be invalid).

Currently, this rewriting has no effect due to rust-lang/rust#33953
If this issue is fixed, then this rewriting will prevent some macros
from being used in expression position.

This commit prevents rustfmt from inserting semicolons for trailing
expressions in `macro_rules!` arms. The user is free to either include
or omit the semicolon - rustfmt will not modify either case.

Author: Aaron1011

Diff for: src/formatting.rs

@@ -149,6 +149,7 @@ fn format_project(
             &format_report,
             &files,
             original_snippet.clone(),
+            operation_setting.is_macro_def,
         )?;
     }
     timer = timer.done_formatting();
@@ -173,6 +174,7 @@ fn format_file(
     report: &FormatReport,
     file_mod_map: &FileModMap<'_>,
     original_snippet: Option<String>,
+    is_macro_def: bool,
 ) -> Result<(), OperationError> {
     let snippet_provider = parse_session.snippet_provider(module.as_ref().inner);
     let mut visitor = FmtVisitor::from_parse_sess(
@@ -182,6 +184,7 @@ fn format_file(
         file_mod_map,
         report.clone(),
     );
+    visitor.is_macro_def = is_macro_def;
     visitor.skip_context.update_with_attrs(&krate.attrs);
     visitor.last_pos = snippet_provider.start_pos();
     visitor.skip_empty_lines(snippet_provider.end_pos());

Diff for: src/formatting/comment.rs

@@ -684,7 +684,9 @@ impl<'a> CommentRewrite<'a> {
                         let mut config = self.fmt.config.clone();
                         config.set().wrap_comments(false);
                         if config.format_code_in_doc_comments() {
-                            if let Some(s) = format_code_block(&self.code_block_buffer, &config) {
+                            if let Some(s) =
+                                format_code_block(&self.code_block_buffer, &config, false)
+                            {
                                 trim_custom_comment_prefix(s.as_ref())
                             } else {
                                 trim_custom_comment_prefix(&self.code_block_buffer)

Diff for: src/formatting/macros.rs

@@ -1379,12 +1379,12 @@ impl MacroBranch {
         config.set().max_width(new_width);
 
         // First try to format as items, then as statements.
-        let new_body_snippet = match format_snippet(&body_str, &config) {
+        let new_body_snippet = match format_snippet(&body_str, &config, true) {
             Some(new_body) => new_body,
             None => {
                 let new_width = new_width + config.tab_spaces();
                 config.set().max_width(new_width);
-                match format_code_block(&body_str, &config) {
+                match format_code_block(&body_str, &config, true) {
                     Some(new_body) => new_body,
                     None => return None,
                 }

Diff for: src/formatting/rewrite.rs

@@ -33,6 +33,7 @@ pub(crate) struct RewriteContext<'a> {
     pub(crate) file_mod_map: &'a FileModMap<'a>,
     pub(crate) config: &'a Config,
     pub(crate) inside_macro: Rc<Cell<bool>>,
+    pub(crate) is_macro_def: bool,
     // Force block indent style even if we are using visual indent style.
     pub(crate) use_block: Cell<bool>,
     // When `is_if_else_block` is true, unindent the comment on top

Diff for: src/formatting/utils.rs

@@ -300,6 +300,12 @@ pub(crate) fn contains_skip(attrs: &[Attribute]) -> bool {
 
 #[inline]
 pub(crate) fn semicolon_for_expr(context: &RewriteContext<'_>, expr: &ast::Expr) -> bool {
+    // Never try to insert semicolons on expressions when we're inside
+    // a macro definition - this can prevent the macro from compiling
+    // when used in expression position
+    if context.is_macro_def {
+        return false;
+    }
     match expr.kind {
         ast::ExprKind::Ret(..) | ast::ExprKind::Continue(..) | ast::ExprKind::Break(..) => {
             context.config.trailing_semicolon()
@@ -700,7 +706,11 @@ pub(crate) fn unicode_str_width(s: &str) -> usize {
 
 /// Format the given snippet. The snippet is expected to be *complete* code.
 /// When we cannot parse the given snippet, this function returns `None`.
-pub(crate) fn format_snippet(snippet: &str, config: &Config) -> Option<FormattedSnippet> {
+pub(crate) fn format_snippet(
+    snippet: &str,
+    config: &Config,
+    is_macro_def: bool,
+) -> Option<FormattedSnippet> {
     let mut config = config.clone();
     std::panic::catch_unwind(move || {
         config.set().hide_parse_errors(true);
@@ -712,6 +722,7 @@ pub(crate) fn format_snippet(snippet: &str, config: &Config) -> Option<Formatted
                 &config,
                 OperationSetting {
                     verbosity: Verbosity::Quiet,
+                    is_macro_def,
                     ..OperationSetting::default()
                 },
             )
@@ -740,7 +751,11 @@ pub(crate) fn format_snippet(snippet: &str, config: &Config) -> Option<Formatted
 /// The code block may be incomplete (i.e., parser may be unable to parse it).
 /// To avoid panic in parser, we wrap the code block with a dummy function.
 /// The returned code block does **not** end with newline.
-pub(crate) fn format_code_block(code_snippet: &str, config: &Config) -> Option<FormattedSnippet> {
+pub(crate) fn format_code_block(
+    code_snippet: &str,
+    config: &Config,
+    is_macro_def: bool,
+) -> Option<FormattedSnippet> {
     const FN_MAIN_PREFIX: &str = "fn main() {\n";
 
     fn enclose_in_main_block(s: &str, config: &Config) -> String {
@@ -773,7 +788,7 @@ pub(crate) fn format_code_block(code_snippet: &str, config: &Config) -> Option<F
     config_with_unix_newline
         .set()
         .newline_style(NewlineStyle::Unix);
-    let mut formatted = format_snippet(&snippet, &config_with_unix_newline)?;
+    let mut formatted = format_snippet(&snippet, &config_with_unix_newline, is_macro_def)?;
     // Remove wrapping main block
     formatted.unwrap_code_block();
 
@@ -854,15 +869,15 @@ mod test {
         // `format_snippet()` and `format_code_block()` should not panic
         // even when we cannot parse the given snippet.
         let snippet = "let";
-        assert!(format_snippet(snippet, &Config::default()).is_none());
-        assert!(format_code_block(snippet, &Config::default()).is_none());
+        assert!(format_snippet(snippet, &Config::default(), false).is_none());
+        assert!(format_code_block(snippet, &Config::default(), false).is_none());
     }
 
     fn test_format_inner<F>(formatter: F, input: &str, expected: &str) -> bool
     where
-        F: Fn(&str, &Config) -> Option<FormattedSnippet>,
+        F: Fn(&str, &Config, bool /* is_code_block */) -> Option<FormattedSnippet>,
     {
-        let output = formatter(input, &Config::default());
+        let output = formatter(input, &Config::default(), false);
         output.is_some() && output.unwrap().snippet == expected
     }
 

Diff for: src/formatting/visitor.rs

@@ -90,6 +90,8 @@ pub(crate) struct FmtVisitor<'a> {
     pub(crate) skip_context: SkipContext,
     /// If set to `true`, normalize number of vertical spaces on formatting missing snippets.
     pub(crate) normalize_vertical_spaces: bool,
+    /// If set to `true`, we are formatting a macro definition
+    pub(crate) is_macro_def: bool,
 }
 
 impl<'a> Drop for FmtVisitor<'a> {
@@ -888,6 +890,7 @@ impl<'b, 'a: 'b> FmtVisitor<'a> {
             report,
             skip_context: Default::default(),
             normalize_vertical_spaces: false,
+            is_macro_def: false,
         }
     }
 
@@ -1091,6 +1094,7 @@ impl<'b, 'a: 'b> FmtVisitor<'a> {
             report: self.report.clone(),
             skip_context: self.skip_context.clone(),
             skipped_range: self.skipped_range.clone(),
+            is_macro_def: self.is_macro_def,
         }
     }
 }

Diff for: src/lib.rs

@@ -39,6 +39,8 @@ mod test;
 pub struct OperationSetting {
     /// If set to `true`, format sub-modules which are defined in the given input.
     pub recursive: bool,
+    /// If set to `true`, we are formatting a macro definition
+    pub is_macro_def: bool,
     pub verbosity: Verbosity,
 }
 

Diff for: src/rustfmt/main.rs

@@ -438,6 +438,7 @@ fn format_string(input: String, opt: Opt) -> Result<i32> {
     let setting = OperationSetting {
         recursive: opt.recursive,
         verbosity: Verbosity::Quiet,
+        is_macro_def: false,
     };
     let report = rustfmt_nightly::format(Input::Text(input), &config, setting)?;
 
@@ -538,6 +539,7 @@ fn format(opt: Opt) -> Result<i32> {
     let setting = OperationSetting {
         recursive: opt.recursive,
         verbosity: opt.verbosity(),
+        is_macro_def: false,
     };
 
     let inputs = FileConfigPairIter::new(&opt, config_paths.is_some()).collect::<Vec<_>>();

Diff for: tests/target/macro_rules_semi.rs

@@ -0,0 +1,22 @@
+macro_rules! expr {
+    (no_semi) => {
+        return true
+    };
+    (semi) => {
+        return true;
+    };
+}
+
+fn foo() -> bool {
+    match true {
+        true => expr!(no_semi),
+        false if false => {
+            expr!(semi)
+        }
+        false => {
+            expr!(semi);
+        }
+    }
+}
+
+fn main() {}