From a2ad4ccc642799177f5973a93cfa7717fe9f1fec Mon Sep 17 00:00:00 2001 From: Aaron Hill Date: Tue, 27 Oct 2020 17:35:00 -0400 Subject: [PATCH] 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 https://github.com/rust-lang/rust/issues/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. --- src/formatting.rs | 3 +++ src/formatting/comment.rs | 4 +++- src/formatting/macros.rs | 4 ++-- src/formatting/rewrite.rs | 1 + src/formatting/utils.rs | 29 ++++++++++++++++++++++------- src/formatting/visitor.rs | 4 ++++ src/lib.rs | 2 ++ src/rustfmt/main.rs | 2 ++ tests/target/macro_rules_semi.rs | 22 ++++++++++++++++++++++ 9 files changed, 61 insertions(+), 10 deletions(-) create mode 100644 tests/target/macro_rules_semi.rs diff --git a/src/formatting.rs b/src/formatting.rs index c4bbc76b347..ce8f2f7637a 100644 --- a/src/formatting.rs +++ b/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, + 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 --git a/src/formatting/comment.rs b/src/formatting/comment.rs index b1f28d7fae8..57fb56865c5 100644 --- a/src/formatting/comment.rs +++ b/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 --git a/src/formatting/macros.rs b/src/formatting/macros.rs index 7dc28cc1fd4..999a3a6aaba 100644 --- a/src/formatting/macros.rs +++ b/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 --git a/src/formatting/rewrite.rs b/src/formatting/rewrite.rs index fc11b065a44..048245ddf71 100644 --- a/src/formatting/rewrite.rs +++ b/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>, + pub(crate) is_macro_def: bool, // Force block indent style even if we are using visual indent style. pub(crate) use_block: Cell, // When `is_if_else_block` is true, unindent the comment on top diff --git a/src/formatting/utils.rs b/src/formatting/utils.rs index 01d4c4b202f..71dac7510b2 100644 --- a/src/formatting/utils.rs +++ b/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 { +pub(crate) fn format_snippet( + snippet: &str, + config: &Config, + is_macro_def: bool, +) -> Option { 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 Option Option { +pub(crate) fn format_code_block( + code_snippet: &str, + config: &Config, + is_macro_def: bool, +) -> Option { 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(formatter: F, input: &str, expected: &str) -> bool where - F: Fn(&str, &Config) -> Option, + F: Fn(&str, &Config, bool /* is_code_block */) -> Option, { - let output = formatter(input, &Config::default()); + let output = formatter(input, &Config::default(), false); output.is_some() && output.unwrap().snippet == expected } diff --git a/src/formatting/visitor.rs b/src/formatting/visitor.rs index 06917a704cb..f487f8c5ea9 100644 --- a/src/formatting/visitor.rs +++ b/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 --git a/src/lib.rs b/src/lib.rs index d19f8144da2..cb8b0540adb 100644 --- a/src/lib.rs +++ b/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 --git a/src/rustfmt/main.rs b/src/rustfmt/main.rs index d47a2f62da9..3d3018370aa 100644 --- a/src/rustfmt/main.rs +++ b/src/rustfmt/main.rs @@ -438,6 +438,7 @@ fn format_string(input: String, opt: Opt) -> Result { 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 { let setting = OperationSetting { recursive: opt.recursive, verbosity: opt.verbosity(), + is_macro_def: false, }; let inputs = FileConfigPairIter::new(&opt, config_paths.is_some()).collect::>(); diff --git a/tests/target/macro_rules_semi.rs b/tests/target/macro_rules_semi.rs new file mode 100644 index 00000000000..84e12d16e6e --- /dev/null +++ b/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() {}