From 1ee5a73aa762000341a3743017186ff051b15418 Mon Sep 17 00:00:00 2001 From: David Bar-On Date: Thu, 28 Jan 2021 18:06:39 +0200 Subject: [PATCH] Fix for issues 4603 and 4609 - unlimitted indentation of macro code --- src/formatting.rs | 39 ++++++++++++++++++++- src/formatting/comment.rs | 11 ++++-- src/formatting/expr.rs | 5 +++ src/formatting/items.rs | 13 +++++++ src/formatting/macros.rs | 71 ++++++++++++++++++++++++++++--------- src/formatting/rewrite.rs | 1 + src/formatting/utils.rs | 43 ++++++++++++++++++----- src/formatting/visitor.rs | 14 +++++++- src/lib.rs | 2 ++ tests/source/issue-4603.rs | 47 +++++++++++++++++++++++++ tests/source/issue-4609.rs | 72 ++++++++++++++++++++++++++++++++++++++ tests/target/issue-4603.rs | 47 +++++++++++++++++++++++++ tests/target/issue-4609.rs | 71 +++++++++++++++++++++++++++++++++++++ 13 files changed, 406 insertions(+), 30 deletions(-) create mode 100644 tests/source/issue-4603.rs create mode 100644 tests/source/issue-4609.rs create mode 100644 tests/target/issue-4603.rs create mode 100644 tests/target/issue-4609.rs diff --git a/src/formatting.rs b/src/formatting.rs index 0a78dc4a930..5f4166ae600 100644 --- a/src/formatting.rs +++ b/src/formatting.rs @@ -58,18 +58,45 @@ pub(crate) mod visitor; pub(crate) mod report; +// As `catch_unwind` in `format_snippet()` does not allow the the of a `mut bool`, +// this function is used to `format_input_inner()` using a reference to a new +// `bool` variable, and return its value as part of the function return value. +pub(crate) fn format_input_inner_for_catch_unwind( + input: Input, + config: &Config, + operation_setting: OperationSetting, + is_macro_def: bool, +) -> (Result, bool) { + let mut macro_original_code_was_used = false; + let result = crate::format_input_inner( + input, + &config, + operation_setting, + is_macro_def, + &mut macro_original_code_was_used, + ); + return (result, macro_original_code_was_used); +} + pub(crate) fn format_input_inner( input: Input, config: &Config, operation_setting: OperationSetting, is_macro_def: bool, + macro_original_code_was_used: &mut bool, ) -> Result { if !config.version_meets_requirement() { return Err(OperationError::VersionMismatch); } rustc_span::with_session_globals(config.edition().into(), || { - format_project(input, config, operation_setting, is_macro_def) + format_project( + input, + config, + operation_setting, + is_macro_def, + macro_original_code_was_used, + ) }) } @@ -78,6 +105,7 @@ fn format_project( config: &Config, operation_setting: OperationSetting, is_macro_def: bool, + macro_original_code_was_used: &mut bool, ) -> Result { let mut timer = Timer::start(); @@ -87,6 +115,7 @@ fn format_project( let input_is_stdin = main_file == FileName::Stdin; let mut parse_session = ParseSess::new(config)?; + if !operation_setting.recursive && parse_session.ignore_file(&main_file) { format_report.add_ignored_file(main_file); return Ok(format_report); @@ -152,6 +181,7 @@ fn format_project( &files, original_snippet.clone(), is_macro_def, + macro_original_code_was_used, )?; } timer = timer.done_formatting(); @@ -177,8 +207,10 @@ fn format_file( file_mod_map: &FileModMap<'_>, original_snippet: Option, is_macro_def: bool, + macro_original_code_was_used: &mut bool, ) -> Result<(), OperationError> { let snippet_provider = parse_session.snippet_provider(module.as_ref().inner); + let mut visitor = FmtVisitor::from_parse_sess( &parse_session, config, @@ -186,6 +218,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(); @@ -237,6 +270,10 @@ fn format_file( ); report.add_format_result(path.clone(), format_result); + if visitor.macro_original_code_was_used.get() { + *macro_original_code_was_used = true; + } + Ok(()) } diff --git a/src/formatting/comment.rs b/src/formatting/comment.rs index 23d7700056c..bd6c04762d2 100644 --- a/src/formatting/comment.rs +++ b/src/formatting/comment.rs @@ -687,9 +687,13 @@ 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, false) - { + let mut macro_original_code_was_used = false; + if let Some(s) = format_code_block( + &self.code_block_buffer, + &config, + false, + &mut macro_original_code_was_used, + ) { trim_custom_comment_prefix(s.as_ref()) } else { trim_custom_comment_prefix(&self.code_block_buffer) @@ -1604,6 +1608,7 @@ pub(crate) fn recover_comment_removed( let snippet = context.snippet(span); let includes_comment = contains_comment(snippet); if snippet != new && includes_comment && changed_comment_content(snippet, &new) { + context.macro_original_code_was_used.replace(true); snippet.to_owned() } else { new diff --git a/src/formatting/expr.rs b/src/formatting/expr.rs index 75c165d15b5..c2a9d5db7b8 100644 --- a/src/formatting/expr.rs +++ b/src/formatting/expr.rs @@ -606,6 +606,11 @@ pub(crate) fn rewrite_block_with_visitor( .skipped_range .borrow_mut() .append(&mut visitor_context.skipped_range.borrow_mut()); + + if visitor.macro_original_code_was_used.get() { + context.macro_original_code_was_used.replace(true); + } + Some(format!("{}{}{}", prefix, label_str, visitor.buffer)) } diff --git a/src/formatting/items.rs b/src/formatting/items.rs index 5a8abd8312c..3b9edb58b75 100644 --- a/src/formatting/items.rs +++ b/src/formatting/items.rs @@ -890,6 +890,10 @@ pub(crate) fn format_impl( result.push_str(&inner_indent_str); result.push_str(visitor.buffer.trim()); result.push_str(&outer_indent_str); + + if visitor.macro_original_code_was_used.get() { + context.macro_original_code_was_used.replace(true); + } } else if need_newline || !context.config.empty_item_single_line() { result.push_str(&sep); } @@ -1266,6 +1270,10 @@ pub(crate) fn format_trait( result.push_str(&inner_indent_str); result.push_str(visitor.buffer.trim()); result.push_str(&outer_indent_str); + + if visitor.macro_original_code_was_used.get() { + context.macro_original_code_was_used.replace(true); + } } else if result.contains('\n') { result.push_str(&outer_indent_str); } @@ -3264,6 +3272,11 @@ impl Rewrite for ast::ForeignItem { defaultness, Some(&inner_attrs), ); + + if visitor.macro_original_code_was_used.get() { + context.macro_original_code_was_used.replace(true); + } + Some(visitor.buffer.to_owned()) } ast::ForeignItemKind::Fn(_, ref fn_sig, ref generics, None) => rewrite_fn_base( diff --git a/src/formatting/macros.rs b/src/formatting/macros.rs index cff47329266..c7874420d27 100644 --- a/src/formatting/macros.rs +++ b/src/formatting/macros.rs @@ -170,7 +170,7 @@ fn return_macro_parse_failure_fallback( ) -> Option { // Mark this as a failure however we format it context.macro_rewrite_failure.replace(true); - + context.macro_original_code_was_used.replace(true); // Heuristically determine whether the last line of the macro uses "Block" style // rather than using "Visual" style, or another indentation style. let is_like_block_indent_style = context @@ -229,6 +229,7 @@ pub(crate) fn rewrite_macro( guard.is_nested(), ) })); + match result { Err(..) | Ok(None) => { context.macro_rewrite_failure.replace(true); @@ -510,13 +511,17 @@ pub(crate) fn rewrite_macro_def( ) -> Option { let snippet = Some(remove_trailing_white_spaces(context.snippet(span))); if snippet.as_ref().map_or(true, |s| s.ends_with(';')) { + context.macro_original_code_was_used.replace(true); return snippet; } let ts = def.body.inner_tokens(); let mut parser = MacroParser::new(ts.into_trees()); let parsed_def = match parser.parse(def.macro_rules) { Some(def) => def, - None => return snippet, + None => { + context.macro_original_code_was_used.replace(true); + return snippet; + } }; let mut result = if def.macro_rules { @@ -527,7 +532,6 @@ pub(crate) fn rewrite_macro_def( result += " "; result += rewrite_ident(context, ident); - let multi_branch_style = def.macro_rules || parsed_def.branches.len() != 1; let arm_shape = if multi_branch_style { @@ -550,6 +554,7 @@ pub(crate) fn rewrite_macro_def( // if the rewrite returned None because a macro could not be rewritten, then return the // original body None if context.macro_rewrite_failure.get() => { + context.macro_original_code_was_used.replace(true); Some(context.snippet(branch.body).trim().to_string()) } None => None, @@ -576,14 +581,16 @@ pub(crate) fn rewrite_macro_def( match write_list(&branch_items, &fmt) { Some(ref s) => result += s, - None => return snippet, + None => { + context.macro_original_code_was_used.replace(true); + return snippet; + } } if multi_branch_style { result += &indent.to_string_with_newline(context.config); result += "}"; } - Some(result) } @@ -1274,7 +1281,8 @@ impl MacroParser { branches.push(self.parse_branch(is_macro_rules)?); } - Some(Macro { branches }) + let ret = Some(Macro { branches }); + ret } // `(` ... `)` `=>` `{` ... `}` @@ -1356,6 +1364,10 @@ impl MacroBranch { result += " =>"; } + // Prepare for restoring original macro body if original code was use during formatting + // (since in this case indentation done by this function doesn't work properly). + let result_without_body = result.clone(); + if !context.config.format_macro_bodies() { result += " "; result += context.snippet(self.whole_body); @@ -1386,23 +1398,44 @@ 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, 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, true) { - Some(new_body) => new_body, - None => return None, + + let mut macro_original_code_was_used = false; + + let new_body_snippet = + match format_snippet(&body_str, &config, true, &mut macro_original_code_was_used) { + 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, + true, + &mut macro_original_code_was_used, + ) { + Some(new_body) => new_body, + None => { + return None; + } + } } - } - }; + }; + let new_body = wrap_str( new_body_snippet.as_ref().to_owned(), config.max_width(), shape, )?; + // If original code was used during the macro body formatting - + // use whole original body without formatting + if macro_original_code_was_used { + result = result_without_body; + result += " "; + result += context.snippet(self.whole_body); + return Some(result); + } + // Indent the body since it is in a block. let indent_str = body_indent.to_string(&config); let mut new_body = LineClasses::new(new_body.trim_end()) @@ -1425,7 +1458,6 @@ impl MacroBranch { // FIXME: this could be *much* more efficient. for (old, new) in &substs { if old_body.contains(new) { - debug!("rewrite_macro_def: bailing matching variable: `{}`", new); return None; } new_body = new_body.replace(new, old); @@ -1576,6 +1608,11 @@ fn rewrite_macro_with_items( result.push_str(&shape.indent.to_string_with_newline(context.config)); result.push_str(closer); result.push_str(trailing_semicolon); + + if visitor.macro_original_code_was_used.get() { + context.macro_original_code_was_used.replace(true); + } + Some(result) } diff --git a/src/formatting/rewrite.rs b/src/formatting/rewrite.rs index 048245ddf71..2d7d7ac5bfe 100644 --- a/src/formatting/rewrite.rs +++ b/src/formatting/rewrite.rs @@ -44,6 +44,7 @@ pub(crate) struct RewriteContext<'a> { pub(crate) snippet_provider: &'a SnippetProvider, // Used for `format_snippet` pub(crate) macro_rewrite_failure: Cell, + pub(crate) macro_original_code_was_used: Cell, pub(crate) report: FormatReport, pub(crate) skip_context: SkipContext, pub(crate) skipped_range: Rc>>, diff --git a/src/formatting/utils.rs b/src/formatting/utils.rs index 70bd42e03c0..ed9282c3805 100644 --- a/src/formatting/utils.rs +++ b/src/formatting/utils.rs @@ -13,6 +13,7 @@ use crate::config::Config; use crate::emitter::Verbosity; use crate::formatting::{ comment::{filter_normal_code, CharClasses, FullCodeCharKind, LineClasses}, + format_input_inner_for_catch_unwind, rewrite::RewriteContext, shape::{Indent, Shape}, FormattedSnippet, @@ -711,14 +712,15 @@ pub(crate) fn format_snippet( snippet: &str, config: &Config, is_macro_def: bool, + macro_original_code_was_used: &mut bool, ) -> Option { let mut config = config.clone(); std::panic::catch_unwind(move || { config.set().hide_parse_errors(true); - let result = { + let (result, orig_code_was_used) = { let input = Input::Text(snippet.into()); - crate::format_input_inner( + format_input_inner_for_catch_unwind( input, &config, OperationSetting { @@ -728,7 +730,8 @@ pub(crate) fn format_snippet( is_macro_def, ) }; - match result { + + let out = match result { Ok(report) if !report.has_errors() => { match report.format_result_as_rc().borrow().iter().next() { Some((_, format_result)) @@ -741,7 +744,15 @@ pub(crate) fn format_snippet( } } _ => None, + }; + (out, orig_code_was_used) + }) + // Update `macro_original_code_was_used` as it could not be done inside the `catch_unwind` + .map(|(out, orig_code_was_used)| { + if orig_code_was_used { + *macro_original_code_was_used = true; } + out }) // Discard panics encountered while formatting the snippet // The ? operator is needed to remove the extra Option @@ -756,6 +767,7 @@ pub(crate) fn format_code_block( code_snippet: &str, config: &Config, is_macro_def: bool, + macro_original_code_was_used: &mut bool, ) -> Option { const FN_MAIN_PREFIX: &str = "fn main() {\n"; @@ -789,7 +801,13 @@ pub(crate) fn format_code_block( config_with_unix_newline .set() .newline_style(NewlineStyle::Unix); - let mut formatted = format_snippet(&snippet, &config_with_unix_newline, is_macro_def)?; + let mut formatted = format_snippet( + &snippet, + &config_with_unix_newline, + is_macro_def, + macro_original_code_was_used, + )?; + // Remove wrapping main block formatted.unwrap_code_block(); @@ -827,6 +845,7 @@ pub(crate) fn format_code_block( result.push_str(trimmed_line); is_indented = indent_next_line(kind); } + Some(FormattedSnippet { snippet: result, non_formatted_ranges: formatted.non_formatted_ranges, @@ -870,15 +889,23 @@ 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(), false).is_none()); - assert!(format_code_block(snippet, &Config::default(), false).is_none()); + let mut b = false; + assert!(format_snippet(snippet, &Config::default(), false, &mut b).is_none()); + let mut b = false; + assert!(format_code_block(snippet, &Config::default(), false, &mut b).is_none()); } fn test_format_inner(formatter: F, input: &str, expected: &str) -> bool where - F: Fn(&str, &Config, bool /* is_code_block */) -> Option, + F: Fn( + &str, + &Config, + bool, /* is_code_block */ + &mut bool, /* Macro Orig Code Used */ + ) -> Option, { - let output = formatter(input, &Config::default(), false); + let mut b = false; + let output = formatter(input, &Config::default(), false, &mut b); output.is_some() && output.unwrap().snippet == expected } diff --git a/src/formatting/visitor.rs b/src/formatting/visitor.rs index 40ecfd5b865..8d71dc726a8 100644 --- a/src/formatting/visitor.rs +++ b/src/formatting/visitor.rs @@ -92,6 +92,7 @@ pub(crate) struct FmtVisitor<'a> { pub(crate) normalize_vertical_spaces: bool, /// If set to `true`, we are formatting a macro definition pub(crate) is_macro_def: bool, + pub(crate) macro_original_code_was_used: Cell, } impl<'a> Drop for FmtVisitor<'a> { @@ -644,8 +645,9 @@ impl<'b, 'a: 'b> FmtVisitor<'a> { self.push_rewrite(item.span, snippet); } ast::ItemKind::MacroDef(ref def) => { + let self_context = self.get_context(); let rewrite = rewrite_macro_def( - &self.get_context(), + &self_context, self.shape(), self.block_indent, def, @@ -653,6 +655,9 @@ impl<'b, 'a: 'b> FmtVisitor<'a> { &item.vis, item.span, ); + if self_context.macro_original_code_was_used.get() { + self.macro_original_code_was_used.replace(true); + } self.push_rewrite(item.span, rewrite); } }; @@ -891,6 +896,7 @@ impl<'b, 'a: 'b> FmtVisitor<'a> { skip_context: Default::default(), normalize_vertical_spaces: false, is_macro_def: false, + macro_original_code_was_used: Cell::new(false), } } @@ -1076,7 +1082,12 @@ impl<'b, 'a: 'b> FmtVisitor<'a> { let context = self.get_context(); let result = f(&context); + if context.macro_original_code_was_used.get() { + self.macro_original_code_was_used.replace(true); + } + self.macro_rewrite_failure |= context.macro_rewrite_failure.get(); + result } @@ -1095,6 +1106,7 @@ impl<'b, 'a: 'b> FmtVisitor<'a> { skip_context: self.skip_context.clone(), skipped_range: self.skipped_range.clone(), is_macro_def: self.is_macro_def, + macro_original_code_was_used: Cell::new(self.macro_original_code_was_used.get()), } } } diff --git a/src/lib.rs b/src/lib.rs index af4e0a6093f..b212aec9d3b 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -49,11 +49,13 @@ pub fn format( config: &Config, operation_setting: OperationSetting, ) -> Result { + let mut macro_original_code_was_used = false; format_input_inner( input, config, operation_setting, /* is_macro_def */ false, + &mut macro_original_code_was_used, ) } diff --git a/tests/source/issue-4603.rs b/tests/source/issue-4603.rs new file mode 100644 index 00000000000..ba0803e0eca --- /dev/null +++ b/tests/source/issue-4603.rs @@ -0,0 +1,47 @@ +// Formatting when original macro snippet is used + +// Original issue #4603 code +#![feature(or_patterns)] +macro_rules! t_or_f { + () => { + (true // some comment + | false) + }; +} + +// Other test cases variations +macro_rules! RULES { + () => { + ( + xxxxxxx // COMMENT + | yyyyyyy + ) + }; +} +macro_rules! RULES { + () => { + (xxxxxxx // COMMENT + | yyyyyyy) + }; +} + +fn main() { + macro_rules! RULES { + () => { + (xxxxxxx // COMMENT + | yyyyyyy) + }; + } +} + +macro_rules! RULES { + () => { + (xxxxxxx /* COMMENT */ | yyyyyyy) + }; +} +macro_rules! RULES { + () => { + (xxxxxxx /* COMMENT */ + | yyyyyyy) + }; +} diff --git a/tests/source/issue-4609.rs b/tests/source/issue-4609.rs new file mode 100644 index 00000000000..e6dedc134c8 --- /dev/null +++ b/tests/source/issue-4609.rs @@ -0,0 +1,72 @@ +// Original issue +macro_rules! outer { + ($d:tt) => { + macro_rules! inner { + ($d s:expr) => { + println!("{}", $d s); + } + } + }; +} + +// Other tests with illegal code +fn main() { + let s = 7; + + macro_rules! outer { + ($d:tt) => { + macro_rules! inner { + ($d s:something) => { + println!("{}", $d s); + } + } + }; + } + + outer!(5); + inner!(5 s:something); +} +macro_rules! outer { + () => { + macro_rules! inner { + () => { + println!("", $dddddd sssssss); + } + } + }; +} + +// Tests with legal code +macro_rules! outer { + () => { + macro_rules! inner { + () => { + println!("", zddddd, ssssss, vvvvvvv); + } + } + }; +} +macro_rules! outer { + ($d:tt) => { + macro_rules! inner { + ($d s:expr) => { + } + } + }; +} +fn main() { + let s = 7; + + macro_rules! outer { + ($d:tt) => { + macro_rules! inner { + ($d s:something) => { + println!("{}{}", $d, s); + } + } + }; + } + + outer!(5); + inner!(5 s:something); +} diff --git a/tests/target/issue-4603.rs b/tests/target/issue-4603.rs new file mode 100644 index 00000000000..e3ad9cc38cb --- /dev/null +++ b/tests/target/issue-4603.rs @@ -0,0 +1,47 @@ +// Formatting when original macro snippet is used + +// Original issue #4603 code +#![feature(or_patterns)] +macro_rules! t_or_f { + () => { + (true // some comment + | false) + }; +} + +// Other test cases variations +macro_rules! RULES { + () => { + ( + xxxxxxx // COMMENT + | yyyyyyy + ) + }; +} +macro_rules! RULES { + () => { + (xxxxxxx // COMMENT + | yyyyyyy) + }; +} + +fn main() { + macro_rules! RULES { + () => { + (xxxxxxx // COMMENT + | yyyyyyy) + }; + } +} + +macro_rules! RULES { + () => { + (xxxxxxx /* COMMENT */ | yyyyyyy) + }; +} +macro_rules! RULES { + () => { + (xxxxxxx /* COMMENT */ + | yyyyyyy) + }; +} diff --git a/tests/target/issue-4609.rs b/tests/target/issue-4609.rs new file mode 100644 index 00000000000..01baee85cf2 --- /dev/null +++ b/tests/target/issue-4609.rs @@ -0,0 +1,71 @@ +// Original issue +macro_rules! outer { + ($d:tt) => { + macro_rules! inner { + ($d s:expr) => { + println!("{}", $d s); + } + } + }; +} + +// Other tests with illegal code +fn main() { + let s = 7; + + macro_rules! outer { + ($d:tt) => { + macro_rules! inner { + ($d s:something) => { + println!("{}", $d s); + } + } + }; + } + + outer!(5); + inner!(5 s:something); +} +macro_rules! outer { + () => { + macro_rules! inner { + () => { + println!("", $dddddd sssssss); + } + } + }; +} + +// Tests with legal code +macro_rules! outer { + () => { + macro_rules! inner { + () => { + println!("", zddddd, ssssss, vvvvvvv); + }; + } + }; +} +macro_rules! outer { + ($d:tt) => { + macro_rules! inner { + ($d s:expr) => {}; + } + }; +} +fn main() { + let s = 7; + + macro_rules! outer { + ($d:tt) => { + macro_rules! inner { + ($d s:something) => { + println!("{}{}", $d, s); + }; + } + }; + } + + outer!(5); + inner!(5 s:something); +}