Merge pull request #2775 from topecongiro/macro-def-with-complex-macro

Change format_snippet to return None when it has failed to format macro call

Author: nrc

src/config/summary.rs

@@ -23,6 +23,9 @@ pub struct Summary {
     // Code is valid, but it is impossible to format it properly.
     has_formatting_errors: bool,
 
+    // Code contains macro call that was unable to format.
+    pub(crate) has_macro_format_failure: bool,
+
     // Failed a check, such as the license check or other opt-in checking.
     has_check_errors: bool,
 
@@ -80,6 +83,10 @@ impl Summary {
         self.has_check_errors
     }
 
+    pub(crate) fn has_macro_formatting_failure(&self) -> bool {
+        self.has_macro_format_failure
+    }
+
     pub fn add_operational_error(&mut self) {
         self.has_operational_errors = true;
     }
@@ -100,6 +107,10 @@ impl Summary {
         self.has_diff = true;
     }
 
+    pub(crate) fn add_macro_foramt_failure(&mut self) {
+        self.has_macro_format_failure = true;
+    }
+
     pub fn has_no_errors(&self) -> bool {
         !(self.has_operational_errors
             || self.has_parsing_errors

src/lib.rs

@@ -422,13 +422,14 @@ fn format_ast<F>(
     config: &Config,
     report: FormatReport,
     mut after_file: F,
-) -> Result<(FileMap, bool), io::Error>
+) -> Result<(FileMap, bool, bool), io::Error>
 where
     F: FnMut(&FileName, &mut String, &[(usize, usize)], &FormatReport) -> Result<bool, io::Error>,
 {
     let mut result = FileMap::new();
     // diff mode: check if any files are differing
     let mut has_diff = false;
+    let mut has_macro_rewrite_failure = false;
 
     let skip_children = config.skip_children();
     for (path, module) in modules::list_files(krate, parse_session.codemap())? {
@@ -472,10 +473,12 @@ where
             }
         };
 
+        has_macro_rewrite_failure |= visitor.macro_rewrite_failure;
+
         result.push((path.clone(), visitor.buffer));
     }
 
-    Ok((result, has_diff))
+    Ok((result, has_diff, has_macro_rewrite_failure))
 }
 
 /// Returns true if the line with the given line number was skipped by `#[rustfmt::skip]`.
@@ -686,6 +689,7 @@ fn format_snippet(snippet: &str, config: &Config) -> Option<String> {
     config.set().hide_parse_errors(true);
     match format_input(input, &config, Some(&mut out)) {
         // `format_input()` returns an empty string on parsing error.
+        Ok((summary, _)) if summary.has_macro_formatting_failure() => None,
         Ok(..) if out.is_empty() && !snippet.is_empty() => None,
         Ok(..) => String::from_utf8(out).ok(),
         Err(..) => None,
@@ -902,7 +906,7 @@ fn format_input_inner<T: Write>(
     }
 
     match format_result {
-        Ok((file_map, has_diff)) => {
+        Ok((file_map, has_diff, has_macro_rewrite_failure)) => {
             if report.has_warnings() {
                 summary.add_formatting_error();
             }
@@ -911,6 +915,10 @@ fn format_input_inner<T: Write>(
                 summary.add_diff();
             }
 
+            if has_macro_rewrite_failure {
+                summary.add_macro_foramt_failure();
+            }
+
             Ok((summary, file_map, report))
         }
         Err(e) => Err((From::from(e), summary)),

src/macros.rs

@@ -129,6 +129,15 @@ fn rewrite_macro_name(path: &ast::Path, extra_ident: Option<ast::Ident>) -> Stri
     }
 }
 
+// Use this on failing to format the macro call.
+fn return_original_snippet_with_failure_marked(
+    context: &RewriteContext,
+    span: Span,
+) -> Option<String> {
+    context.macro_rewrite_failure.replace(true);
+    Some(context.snippet(span).to_owned())
+}
+
 pub fn rewrite_macro(
     mac: &ast::Mac,
     extra_ident: Option<ast::Ident>,
@@ -138,6 +147,9 @@ pub fn rewrite_macro(
 ) -> Option<String> {
     context.inside_macro.replace(true);
     let result = rewrite_macro_inner(mac, extra_ident, context, shape, position);
+    if result.is_none() {
+        context.macro_rewrite_failure.replace(true);
+    }
     context.inside_macro.replace(false);
     result
 }
@@ -196,7 +208,7 @@ pub fn rewrite_macro_inner(
         loop {
             match parse_macro_arg(&mut parser) {
                 Some(arg) => arg_vec.push(arg),
-                None => return Some(context.snippet(mac.span).to_owned()),
+                None => return return_original_snippet_with_failure_marked(context, mac.span),
             }
 
             match parser.token {
@@ -216,13 +228,17 @@ pub fn rewrite_macro_inner(
                                         break;
                                     }
                                 }
-                                None => return Some(context.snippet(mac.span).to_owned()),
+                                None => {
+                                    return return_original_snippet_with_failure_marked(
+                                        context, mac.span,
+                                    )
+                                }
                             }
                         }
                     }
-                    return Some(context.snippet(mac.span).to_owned());
+                    return return_original_snippet_with_failure_marked(context, mac.span);
                 }
-                _ => return Some(context.snippet(mac.span).to_owned()),
+                _ => return return_original_snippet_with_failure_marked(context, mac.span),
             }
 
             parser.bump();

src/rewrite.rs

@@ -39,6 +39,8 @@ pub struct RewriteContext<'a> {
     // When rewriting chain, veto going multi line except the last element
     pub force_one_line_chain: RefCell<bool>,
     pub snippet_provider: &'a SnippetProvider<'a>,
+    // Used for `format_snippet`
+    pub(crate) macro_rewrite_failure: RefCell<bool>,
     pub(crate) report: FormatReport,
 }
 

src/visitor.rs

@@ -70,6 +70,7 @@ pub struct FmtVisitor<'a> {
     pub snippet_provider: &'a SnippetProvider<'a>,
     pub line_number: usize,
     pub skipped_range: Vec<(usize, usize)>,
+    pub macro_rewrite_failure: bool,
     pub(crate) report: FormatReport,
 }
 
@@ -519,7 +520,7 @@ impl<'b, 'a: 'b> FmtVisitor<'a> {
 
         // 1 = ;
         let shape = self.shape().sub_width(1).unwrap();
-        let rewrite = rewrite_macro(mac, ident, &self.get_context(), shape, pos);
+        let rewrite = self.with_context(|ctx| rewrite_macro(mac, ident, ctx, shape, pos));
         self.push_rewrite(mac.span, rewrite);
     }
 
@@ -578,6 +579,7 @@ impl<'b, 'a: 'b> FmtVisitor<'a> {
             snippet_provider,
             line_number: 0,
             skipped_range: vec![],
+            macro_rewrite_failure: false,
             report,
         }
     }
@@ -736,6 +738,20 @@ impl<'b, 'a: 'b> FmtVisitor<'a> {
         }
     }
 
+    pub fn with_context<F>(&mut self, f: F) -> Option<String>
+    where
+        F: Fn(&RewriteContext) -> Option<String>,
+    {
+        let result;
+        let macro_rewrite_failure = {
+            let context = self.get_context();
+            result = f(&context);
+            unsafe { *context.macro_rewrite_failure.as_ptr() }
+        };
+        self.macro_rewrite_failure |= macro_rewrite_failure;
+        result
+    }
+
     pub fn get_context(&self) -> RewriteContext {
         RewriteContext {
             parse_session: self.parse_session,
@@ -746,6 +762,7 @@ impl<'b, 'a: 'b> FmtVisitor<'a> {
             is_if_else_block: RefCell::new(false),
             force_one_line_chain: RefCell::new(false),
             snippet_provider: self.snippet_provider,
+            macro_rewrite_failure: RefCell::new(false),
             report: self.report.clone(),
         }
     }

tests/source/macro_rules.rs

@@ -204,3 +204,30 @@ macro_rules! foo {
 macro_rules! __wundergraph_expand_sqlite_mutation {
     ( $mutation_name:ident $((context = $($context:tt)*))*{ $( $entity_name:ident( $(insert = $insert:ident,)* $(update = $update:ident,)* $(delete = $($delete:tt)+)* ), )* } ) => {};
 }
+
+// #2607
+macro_rules! bench {
+    ($ty:ident) => {
+        criterion_group!(
+            name = benches;
+            config = ::common_bench::reduced_samples();
+            targets = call, map;
+        );
+    };
+}
+
+// #2770
+macro_rules! save_regs {
+    () => {
+        asm!("push rax
+              push rcx
+              push rdx
+              push rsi
+              push rdi
+              push r8
+              push r9
+              push r10
+              push r11"
+             :::: "intel", "volatile");
+    };
+}

tests/target/issue-2523.rs

@@ -2,7 +2,7 @@
 
 // Do not unindent macro calls in comment with unformattable syntax.
 //! ```rust
-//! let x = 3;
+//! let x = 3  ;
 //! some_macro!(pub fn fn foo() (
 //!     println!("Don't unindent me!");
 //! ));

tests/target/macro_rules.rs

@@ -246,3 +246,30 @@ macro_rules! __wundergraph_expand_sqlite_mutation {
         }
     ) => {};
 }
+
+// #2607
+macro_rules! bench {
+    ($ty:ident) => {
+        criterion_group!(
+            name = benches;
+            config = ::common_bench::reduced_samples();
+            targets = call, map;
+        );
+    };
+}
+
+// #2770
+macro_rules! save_regs {
+    () => {
+        asm!("push rax
+              push rcx
+              push rdx
+              push rsi
+              push rdi
+              push r8
+              push r9
+              push r10
+              push r11"
+             :::: "intel", "volatile");
+    };
+}