feat: don't insert semi in macro_rules arm body

Author: calebcartwright

Diff for: src/comment.rs

@@ -659,7 +659,7 @@ impl<'a> CommentRewrite<'a> {
                         config.set().wrap_comments(false);
                         if config.format_code_in_doc_comments() {
                             if let Some(s) =
-                                crate::format_code_block(&self.code_block_buffer, &config)
+                                crate::format_code_block(&self.code_block_buffer, &config, false)
                             {
                                 trim_custom_comment_prefix(&s.snippet)
                             } else {

Diff for: src/formatting.rs

@@ -25,7 +25,11 @@ pub(crate) type SourceFile = Vec<FileRecord>;
 pub(crate) type FileRecord = (FileName, String);
 
 impl<'b, T: Write + 'b> Session<'b, T> {
-    pub(crate) fn format_input_inner(&mut self, input: Input) -> Result<FormatReport, ErrorKind> {
+    pub(crate) fn format_input_inner(
+        &mut self,
+        input: Input,
+        is_macro_def: bool,
+    ) -> Result<FormatReport, ErrorKind> {
         if !self.config.version_meets_requirement() {
             return Err(ErrorKind::VersionMismatch);
         }
@@ -42,7 +46,7 @@ impl<'b, T: Write + 'b> Session<'b, T> {
             }
 
             let config = &self.config.clone();
-            let format_result = format_project(input, config, self);
+            let format_result = format_project(input, config, self, is_macro_def);
 
             format_result.map(|report| {
                 self.errors.add(&report.internal.borrow().1);
@@ -57,6 +61,7 @@ fn format_project<T: FormatHandler>(
     input: Input,
     config: &Config,
     handler: &mut T,
+    is_macro_def: bool,
 ) -> Result<FormatReport, ErrorKind> {
     let mut timer = Timer::start();
 
@@ -103,7 +108,7 @@ fn format_project<T: FormatHandler>(
             continue;
         }
         should_emit_verbose(input_is_stdin, config, || println!("Formatting {}", path));
-        context.format_file(path, &module)?;
+        context.format_file(path, &module, is_macro_def)?;
     }
     timer = timer.done_formatting();
 
@@ -134,7 +139,12 @@ impl<'a, T: FormatHandler + 'a> FormatContext<'a, T> {
     }
 
     // Formats a single file/module.
-    fn format_file(&mut self, path: FileName, module: &Module<'_>) -> Result<(), ErrorKind> {
+    fn format_file(
+        &mut self,
+        path: FileName,
+        module: &Module<'_>,
+        is_macro_def: bool,
+    ) -> Result<(), ErrorKind> {
         let snippet_provider = self.parse_session.snippet_provider(module.as_ref().inner);
         let mut visitor = FmtVisitor::from_parse_sess(
             &self.parse_session,
@@ -143,7 +153,7 @@ impl<'a, T: FormatHandler + 'a> FormatContext<'a, T> {
             self.report.clone(),
         );
         visitor.skip_context.update_with_attrs(&self.krate.attrs);
-
+        visitor.is_macro_def = is_macro_def;
         visitor.last_pos = snippet_provider.start_pos();
         visitor.skip_empty_lines(snippet_provider.end_pos());
         visitor.format_separate_mod(module, snippet_provider.end_pos());

Diff for: src/lib.rs

@@ -286,7 +286,7 @@ impl fmt::Display for FormatReport {
 
 /// Format the given snippet. The snippet is expected to be *complete* code.
 /// When we cannot parse the given snippet, this function returns `None`.
-fn format_snippet(snippet: &str, config: &Config) -> Option<FormattedSnippet> {
+fn format_snippet(snippet: &str, config: &Config, is_macro_def: bool) -> Option<FormattedSnippet> {
     let mut config = config.clone();
     panic::catch_unwind(|| {
         let mut out: Vec<u8> = Vec::with_capacity(snippet.len() * 2);
@@ -297,7 +297,7 @@ fn format_snippet(snippet: &str, config: &Config) -> Option<FormattedSnippet> {
         let (formatting_error, result) = {
             let input = Input::Text(snippet.into());
             let mut session = Session::new(config, Some(&mut out));
-            let result = session.format(input);
+            let result = session.format_input_inner(input, is_macro_def);
             (
                 session.errors.has_macro_format_failure
                     || session.out.as_ref().unwrap().is_empty() && !snippet.is_empty()
@@ -323,7 +323,11 @@ fn format_snippet(snippet: &str, config: &Config) -> Option<FormattedSnippet> {
 /// 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.
-fn format_code_block(code_snippet: &str, config: &Config) -> Option<FormattedSnippet> {
+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 {
@@ -356,7 +360,7 @@ fn format_code_block(code_snippet: &str, config: &Config) -> Option<FormattedSni
     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();
 
@@ -435,7 +439,7 @@ impl<'b, T: Write + 'b> Session<'b, T> {
     /// The main entry point for Rustfmt. Formats the given input according to the
     /// given config. `out` is only necessary if required by the configuration.
     pub fn format(&mut self, input: Input) -> Result<FormatReport, ErrorKind> {
-        self.format_input_inner(input)
+        self.format_input_inner(input, false)
     }
 
     pub fn override_config<F, U>(&mut self, mut config: Config, f: F) -> U
@@ -550,15 +554,15 @@ mod unit_tests {
         // `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) -> Option<FormattedSnippet>,
     {
-        let output = formatter(input, &Config::default());
+        let output = formatter(input, &Config::default(), false);
         output.is_some() && output.unwrap().snippet == expected
     }
 
@@ -580,7 +584,7 @@ mod unit_tests {
     fn test_format_code_block_fail() {
         #[rustfmt::skip]
         let code_block = "this_line_is_100_characters_long_xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx(x, y, z);";
-        assert!(format_code_block(code_block, &Config::default()).is_none());
+        assert!(format_code_block(code_block, &Config::default(), false).is_none());
     }
 
     #[test]

Diff for: src/macros.rs

@@ -1363,12 +1363,12 @@ impl MacroBranch {
         config.set().max_width(new_width);
 
         // First try to format as items, then as statements.
-        let new_body_snippet = match crate::format_snippet(&body_str, &config) {
+        let new_body_snippet = match crate::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 crate::format_code_block(&body_str, &config) {
+                match crate::format_code_block(&body_str, &config, true) {
                     Some(new_body) => new_body,
                     None => return None,
                 }

Diff for: src/rewrite.rs

@@ -39,6 +39,7 @@ pub(crate) struct RewriteContext<'a> {
     pub(crate) snippet_provider: &'a SnippetProvider,
     // Used for `format_snippet`
     pub(crate) macro_rewrite_failure: Cell<bool>,
+    pub(crate) is_macro_def: bool,
     pub(crate) report: FormatReport,
     pub(crate) skip_context: SkipContext,
     pub(crate) skipped_range: Rc<RefCell<Vec<(usize, usize)>>>,

Diff for: src/utils.rs

@@ -279,6 +279,13 @@ 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()

Diff for: src/visitor.rs

@@ -86,6 +86,7 @@ pub(crate) struct FmtVisitor<'a> {
     pub(crate) macro_rewrite_failure: bool,
     pub(crate) report: FormatReport,
     pub(crate) skip_context: SkipContext,
+    pub(crate) is_macro_def: bool,
 }
 
 impl<'a> Drop for FmtVisitor<'a> {
@@ -811,6 +812,7 @@ impl<'b, 'a: 'b> FmtVisitor<'a> {
             snippet_provider,
             line_number: 0,
             skipped_range: Rc::new(RefCell::new(vec![])),
+            is_macro_def: false,
             macro_rewrite_failure: false,
             report,
             skip_context: Default::default(),
@@ -1003,6 +1005,7 @@ impl<'b, 'a: 'b> FmtVisitor<'a> {
             force_one_line_chain: Cell::new(false),
             snippet_provider: self.snippet_provider,
             macro_rewrite_failure: Cell::new(false),
+            is_macro_def: self.is_macro_def,
             report: self.report.clone(),
             skip_context: self.skip_context.clone(),
             skipped_range: self.skipped_range.clone(),

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() {}