From a2741d8a5ae756a5fd7138df4a58200d8cdbe460 Mon Sep 17 00:00:00 2001 From: David Bar-On Date: Mon, 9 Nov 2020 20:05:21 +0200 Subject: [PATCH 1/5] Changes for #4394 - Support preserving block wrapping of closure bodies --- src/formatting/closures.rs | 6 +++- tests/target/closure_block_style.rs | 28 ++++++++++++++----- tests/target/closure_visual_style.rs | 4 +-- .../configs/force_multiline_block/false.rs | 8 ++++-- .../target/configs/indent_style/block_call.rs | 8 ++++-- tests/target/hard-tabs.rs | 8 ++++-- tests/target/issue-1366.rs | 10 ++++--- tests/target/issue_3844.rs | 2 +- tests/target/match.rs | 12 ++++---- 9 files changed, 57 insertions(+), 29 deletions(-) diff --git a/src/formatting/closures.rs b/src/formatting/closures.rs index fabbf3904eb..67dc0773436 100644 --- a/src/formatting/closures.rs +++ b/src/formatting/closures.rs @@ -56,7 +56,11 @@ pub(crate) fn rewrite_closure( } let result = match fn_decl.output { - ast::FnRetTy::Default(_) if !context.inside_macro() => { + // Try-without-block only if not a macro and original code is not a block (#4394) + ast::FnRetTy::Default(_) + if !context.inside_macro() + && !context.snippet(body.span).trim().starts_with('{') => + { try_rewrite_without_block(body, &prefix, capture, context, shape, body_shape) } _ => None, diff --git a/tests/target/closure_block_style.rs b/tests/target/closure_block_style.rs index 077b8017a4b..6315e3a269c 100644 --- a/tests/target/closure_block_style.rs +++ b/tests/target/closure_block_style.rs @@ -33,7 +33,7 @@ fn main() { } }; - let unblock_me = |trivial| closure(); + let unblock_me = |trivial| { closure() }; let empty = |arg| {}; @@ -51,7 +51,7 @@ fn main() { |big_argument_name, test123| looooooooooooooooooong_function_naaaaaaaaaaaaaaaaame(); let arg_test = - |big_argument_name, test123| looooooooooooooooooong_function_naaaaaaaaaaaaaaaaame(); + |big_argument_name, test123| { looooooooooooooooooong_function_naaaaaaaaaaaaaaaaame() }; let simple_closure = move || -> () {}; @@ -164,7 +164,9 @@ fn issue470() { impl Foo { pub fn bar(&self) { Some(SomeType { - push_closure_out_to_100_chars: iter(otherwise_it_works_ok.into_iter().map(|f| Ok(f))), + push_closure_out_to_100_chars: iter( + otherwise_it_works_ok.into_iter().map(|f| { Ok(f) }), + ), }) } } @@ -219,10 +221,22 @@ fn issue2063() { } fn issue1524() { - let f = |x| x; - let f = |x| x; - let f = |x| x; - let f = |x| x; + let f = |x| { + { + { + { x } + } + } + }; + let f = |x| { + { + { x } + } + }; + let f = |x| { + { x } + }; + let f = |x| { x }; let f = |x| x; } diff --git a/tests/target/closure_visual_style.rs b/tests/target/closure_visual_style.rs index a4593b12cfe..bdd9ff80c5b 100644 --- a/tests/target/closure_visual_style.rs +++ b/tests/target/closure_visual_style.rs @@ -56,7 +56,7 @@ fn main() { } }; - let unblock_me = |trivial| closure(); + let unblock_me = |trivial| { closure() }; let empty = |arg| {}; @@ -74,7 +74,7 @@ fn main() { |big_argument_name, test123| looooooooooooooooooong_function_naaaaaaaaaaaaaaaaame(); let arg_test = - |big_argument_name, test123| looooooooooooooooooong_function_naaaaaaaaaaaaaaaaame(); + |big_argument_name, test123| { looooooooooooooooooong_function_naaaaaaaaaaaaaaaaame() }; let simple_closure = move || -> () {}; diff --git a/tests/target/configs/force_multiline_block/false.rs b/tests/target/configs/force_multiline_block/false.rs index 7cb4cac1d69..b97e348e5da 100644 --- a/tests/target/configs/force_multiline_block/false.rs +++ b/tests/target/configs/force_multiline_block/false.rs @@ -13,8 +13,10 @@ fn main() { } fn main() { - result.and_then(|maybe_value| match maybe_value { - None => Err("oops"), - Some(value) => Ok(1), + result.and_then(|maybe_value| { + match maybe_value { + None => Err("oops"), + Some(value) => Ok(1), + } }); } diff --git a/tests/target/configs/indent_style/block_call.rs b/tests/target/configs/indent_style/block_call.rs index caf7b41e596..4f89c693dbf 100644 --- a/tests/target/configs/indent_style/block_call.rs +++ b/tests/target/configs/indent_style/block_call.rs @@ -74,9 +74,11 @@ fn query(conn: &Connection) -> Result<()> { WHERE DATE(date) = $1 "#, &[], - |row| Post { - title: row.get(0), - date: row.get(1), + |row| { + Post { + title: row.get(0), + date: row.get(1), + } }, )?; diff --git a/tests/target/hard-tabs.rs b/tests/target/hard-tabs.rs index d581b8ae0ca..fe332a4d350 100644 --- a/tests/target/hard-tabs.rs +++ b/tests/target/hard-tabs.rs @@ -80,9 +80,11 @@ fn main() { }); a.b.c.d(); - x().y(|| match cond() { - true => {} - false => {} + x().y(|| { + match cond() { + true => {} + false => {} + } }); } diff --git a/tests/target/issue-1366.rs b/tests/target/issue-1366.rs index eee147baab9..52ef15bc9d4 100644 --- a/tests/target/issue-1366.rs +++ b/tests/target/issue-1366.rs @@ -3,10 +3,12 @@ fn main() { Some("fffffffsssssssssddddssssfffffddddff") .map(|s| s) .map(|s| s.to_string()) - .map(|res| match Some(res) { - Some(ref s) if s == "" => 41, - Some(_) => 42, - _ => 43, + .map(|res| { + match Some(res) { + Some(ref s) if s == "" => 41, + Some(_) => 42, + _ => 43, + } }) } println!("{:?}", f()) diff --git a/tests/target/issue_3844.rs b/tests/target/issue_3844.rs index 81d2083463e..42fcd0728ec 100644 --- a/tests/target/issue_3844.rs +++ b/tests/target/issue_3844.rs @@ -1,3 +1,3 @@ fn main() { - || {}; + || { {} }; } diff --git a/tests/target/match.rs b/tests/target/match.rs index 7f91f4359bc..ad2ee335bb3 100644 --- a/tests/target/match.rs +++ b/tests/target/match.rs @@ -364,11 +364,13 @@ fn issue1395() { let bar = Some(true); let foo = Some(true); let mut x = false; - bar.and_then(|_| match foo { - None => None, - Some(b) => { - x = true; - Some(b) + bar.and_then(|_| { + match foo { + None => None, + Some(b) => { + x = true; + Some(b) + } } }); } From 4736a43ff34421bf2352b6ad20a40b1cb839ee3b Mon Sep 17 00:00:00 2001 From: David Bar-On Date: Thu, 12 Nov 2020 13:02:20 +0200 Subject: [PATCH 2/5] Add preserve_closure_block_wrapping config option support --- Configurations.md | 27 +++++++++++ src/config.rs | 2 + src/formatting/closures.rs | 15 ++++--- .../source/preserve_closure_block_wrapping.rs | 32 +++++++++++++ tests/target/closure_block_style.rs | 28 +++--------- tests/target/closure_visual_style.rs | 4 +- .../configs/force_multiline_block/false.rs | 8 ++-- .../target/configs/indent_style/block_call.rs | 8 ++-- tests/target/hard-tabs.rs | 8 ++-- tests/target/issue-1366.rs | 10 ++--- tests/target/issue_3844.rs | 2 +- tests/target/match.rs | 12 +++-- .../target/preserve_closure_block_wrapping.rs | 45 +++++++++++++++++++ 13 files changed, 143 insertions(+), 58 deletions(-) create mode 100755 tests/source/preserve_closure_block_wrapping.rs create mode 100755 tests/target/preserve_closure_block_wrapping.rs diff --git a/Configurations.md b/Configurations.md index 08e60b1c837..c64383f6b37 100644 --- a/Configurations.md +++ b/Configurations.md @@ -1836,6 +1836,33 @@ fn say_hi() { } ``` +## `preserve_closure_block_wrapping` + +Preserves block wraping arround closures. For example, useful when the closure `||` can be +confused with OR. + +- **Default value**: `false` +- **Possible values**: `true`, `false` +- **Stable**: No + +#### `true`: +Original block wrapping is preserved: +```rust +fn main() { + let explicit_conversion_preserves_semantics = + || { !is_mod || (is_mod && attrs.map_or(true, |a| a.is_empty())) }; +} +``` + +#### `false` (default): +Block is not preserved: +```rust +fn main() { + let explicit_conversion_preserves_semantics = + || !is_mod || (is_mod && attrs.map_or(true, |a| a.is_empty())); +} +``` + ## `overflow_delimited_expr` When structs, slices, arrays, and block/array-like macros are used as the last diff --git a/src/config.rs b/src/config.rs index 737194c8e9b..1c2689d0f4f 100644 --- a/src/config.rs +++ b/src/config.rs @@ -134,6 +134,7 @@ create_config! { format_generated_files: bool, false, false, "Format generated files"; preserve_block_start_blank_lines: bool, false, false, "Preserve blank lines at the start of \ blocks."; + preserve_closure_block_wrapping: bool, false , false, "Preserve block wrapping around closures"; // Options that can change the source code beyond whitespace/blocks (somewhat linty things) merge_derives: bool, true, true, "Merge multiple `#[derive(...)]` into a single one"; @@ -623,6 +624,7 @@ edition = "2018" inline_attribute_width = 0 format_generated_files = false preserve_block_start_blank_lines = false +preserve_closure_block_wrapping = false merge_derives = true use_try_shorthand = false use_field_init_shorthand = false diff --git a/src/formatting/closures.rs b/src/formatting/closures.rs index 67dc0773436..9b230473564 100644 --- a/src/formatting/closures.rs +++ b/src/formatting/closures.rs @@ -56,12 +56,15 @@ pub(crate) fn rewrite_closure( } let result = match fn_decl.output { - // Try-without-block only if not a macro and original code is not a block (#4394) - ast::FnRetTy::Default(_) - if !context.inside_macro() - && !context.snippet(body.span).trim().starts_with('{') => - { - try_rewrite_without_block(body, &prefix, capture, context, shape, body_shape) + ast::FnRetTy::Default(_) if !context.inside_macro() => { + // Try-without-block only if configured and original code is not a block (#4394) + if !context.config.preserve_closure_block_wrapping() + || !context.snippet(body.span).trim().starts_with('{') + { + try_rewrite_without_block(body, &prefix, capture, context, shape, body_shape) + } else { + None + } } _ => None, }; diff --git a/tests/source/preserve_closure_block_wrapping.rs b/tests/source/preserve_closure_block_wrapping.rs new file mode 100755 index 00000000000..b53367a1da0 --- /dev/null +++ b/tests/source/preserve_closure_block_wrapping.rs @@ -0,0 +1,32 @@ +// rustfmt-preserve_closure_block_wrapping: true + +fn main() { +let explicit_conversion_preserves_semantics = + || { !is_mod || (is_mod && attrs.map_or(true, |a| a.is_empty())) }; +} + +fn main() { +|| {{}}; +} + +fn issue1524() { +let f = |x| {{{{x}}}}; +let f = |x| {{{x}}}; +let f = |x| {{x}}; +let f = |x| {x}; +let f = |x| x; +} + +fn main() { +let arg_test2 = |big_argument_name, test123| {looooooooooooooooooong_function_naaaaaaaaaaaaaaaaame()}; +} + +impl Foo { +pub fn bar(&self) { + Some(SomeType { + push_closure_out_to_100_chars: iter(otherwise_it_works_ok.into_iter().map(|f| { + Ok(f) + })), + }) +} +} diff --git a/tests/target/closure_block_style.rs b/tests/target/closure_block_style.rs index 6315e3a269c..077b8017a4b 100644 --- a/tests/target/closure_block_style.rs +++ b/tests/target/closure_block_style.rs @@ -33,7 +33,7 @@ fn main() { } }; - let unblock_me = |trivial| { closure() }; + let unblock_me = |trivial| closure(); let empty = |arg| {}; @@ -51,7 +51,7 @@ fn main() { |big_argument_name, test123| looooooooooooooooooong_function_naaaaaaaaaaaaaaaaame(); let arg_test = - |big_argument_name, test123| { looooooooooooooooooong_function_naaaaaaaaaaaaaaaaame() }; + |big_argument_name, test123| looooooooooooooooooong_function_naaaaaaaaaaaaaaaaame(); let simple_closure = move || -> () {}; @@ -164,9 +164,7 @@ fn issue470() { impl Foo { pub fn bar(&self) { Some(SomeType { - push_closure_out_to_100_chars: iter( - otherwise_it_works_ok.into_iter().map(|f| { Ok(f) }), - ), + push_closure_out_to_100_chars: iter(otherwise_it_works_ok.into_iter().map(|f| Ok(f))), }) } } @@ -221,22 +219,10 @@ fn issue2063() { } fn issue1524() { - let f = |x| { - { - { - { x } - } - } - }; - let f = |x| { - { - { x } - } - }; - let f = |x| { - { x } - }; - let f = |x| { x }; + let f = |x| x; + let f = |x| x; + let f = |x| x; + let f = |x| x; let f = |x| x; } diff --git a/tests/target/closure_visual_style.rs b/tests/target/closure_visual_style.rs index bdd9ff80c5b..a4593b12cfe 100644 --- a/tests/target/closure_visual_style.rs +++ b/tests/target/closure_visual_style.rs @@ -56,7 +56,7 @@ fn main() { } }; - let unblock_me = |trivial| { closure() }; + let unblock_me = |trivial| closure(); let empty = |arg| {}; @@ -74,7 +74,7 @@ fn main() { |big_argument_name, test123| looooooooooooooooooong_function_naaaaaaaaaaaaaaaaame(); let arg_test = - |big_argument_name, test123| { looooooooooooooooooong_function_naaaaaaaaaaaaaaaaame() }; + |big_argument_name, test123| looooooooooooooooooong_function_naaaaaaaaaaaaaaaaame(); let simple_closure = move || -> () {}; diff --git a/tests/target/configs/force_multiline_block/false.rs b/tests/target/configs/force_multiline_block/false.rs index b97e348e5da..7cb4cac1d69 100644 --- a/tests/target/configs/force_multiline_block/false.rs +++ b/tests/target/configs/force_multiline_block/false.rs @@ -13,10 +13,8 @@ fn main() { } fn main() { - result.and_then(|maybe_value| { - match maybe_value { - None => Err("oops"), - Some(value) => Ok(1), - } + result.and_then(|maybe_value| match maybe_value { + None => Err("oops"), + Some(value) => Ok(1), }); } diff --git a/tests/target/configs/indent_style/block_call.rs b/tests/target/configs/indent_style/block_call.rs index 4f89c693dbf..caf7b41e596 100644 --- a/tests/target/configs/indent_style/block_call.rs +++ b/tests/target/configs/indent_style/block_call.rs @@ -74,11 +74,9 @@ fn query(conn: &Connection) -> Result<()> { WHERE DATE(date) = $1 "#, &[], - |row| { - Post { - title: row.get(0), - date: row.get(1), - } + |row| Post { + title: row.get(0), + date: row.get(1), }, )?; diff --git a/tests/target/hard-tabs.rs b/tests/target/hard-tabs.rs index fe332a4d350..d581b8ae0ca 100644 --- a/tests/target/hard-tabs.rs +++ b/tests/target/hard-tabs.rs @@ -80,11 +80,9 @@ fn main() { }); a.b.c.d(); - x().y(|| { - match cond() { - true => {} - false => {} - } + x().y(|| match cond() { + true => {} + false => {} }); } diff --git a/tests/target/issue-1366.rs b/tests/target/issue-1366.rs index 52ef15bc9d4..eee147baab9 100644 --- a/tests/target/issue-1366.rs +++ b/tests/target/issue-1366.rs @@ -3,12 +3,10 @@ fn main() { Some("fffffffsssssssssddddssssfffffddddff") .map(|s| s) .map(|s| s.to_string()) - .map(|res| { - match Some(res) { - Some(ref s) if s == "" => 41, - Some(_) => 42, - _ => 43, - } + .map(|res| match Some(res) { + Some(ref s) if s == "" => 41, + Some(_) => 42, + _ => 43, }) } println!("{:?}", f()) diff --git a/tests/target/issue_3844.rs b/tests/target/issue_3844.rs index 42fcd0728ec..81d2083463e 100644 --- a/tests/target/issue_3844.rs +++ b/tests/target/issue_3844.rs @@ -1,3 +1,3 @@ fn main() { - || { {} }; + || {}; } diff --git a/tests/target/match.rs b/tests/target/match.rs index ad2ee335bb3..7f91f4359bc 100644 --- a/tests/target/match.rs +++ b/tests/target/match.rs @@ -364,13 +364,11 @@ fn issue1395() { let bar = Some(true); let foo = Some(true); let mut x = false; - bar.and_then(|_| { - match foo { - None => None, - Some(b) => { - x = true; - Some(b) - } + bar.and_then(|_| match foo { + None => None, + Some(b) => { + x = true; + Some(b) } }); } diff --git a/tests/target/preserve_closure_block_wrapping.rs b/tests/target/preserve_closure_block_wrapping.rs new file mode 100755 index 00000000000..a25ccdfec8f --- /dev/null +++ b/tests/target/preserve_closure_block_wrapping.rs @@ -0,0 +1,45 @@ +// rustfmt-preserve_closure_block_wrapping: true + +fn main() { + let explicit_conversion_preserves_semantics = + || { !is_mod || (is_mod && attrs.map_or(true, |a| a.is_empty())) }; +} + +fn main() { + || { {} }; +} + +fn issue1524() { + let f = |x| { + { + { + { x } + } + } + }; + let f = |x| { + { + { x } + } + }; + let f = |x| { + { x } + }; + let f = |x| { x }; + let f = |x| x; +} + +fn main() { + let arg_test2 = + |big_argument_name, test123| { looooooooooooooooooong_function_naaaaaaaaaaaaaaaaame() }; +} + +impl Foo { + pub fn bar(&self) { + Some(SomeType { + push_closure_out_to_100_chars: iter( + otherwise_it_works_ok.into_iter().map(|f| { Ok(f) }), + ), + }) + } +} From 2204519657d1ab2ee0ad8f2f78f46204ded1f198 Mon Sep 17 00:00:00 2001 From: David Bar-On Date: Mon, 23 Nov 2020 12:00:29 +0200 Subject: [PATCH 3/5] Code purifying based on the discussion in PR #4519 --- src/formatting/closures.rs | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-) diff --git a/src/formatting/closures.rs b/src/formatting/closures.rs index 9b230473564..c646b151ced 100644 --- a/src/formatting/closures.rs +++ b/src/formatting/closures.rs @@ -56,15 +56,8 @@ pub(crate) fn rewrite_closure( } let result = match fn_decl.output { - ast::FnRetTy::Default(_) if !context.inside_macro() => { - // Try-without-block only if configured and original code is not a block (#4394) - if !context.config.preserve_closure_block_wrapping() - || !context.snippet(body.span).trim().starts_with('{') - { - try_rewrite_without_block(body, &prefix, capture, context, shape, body_shape) - } else { - None - } + ast::FnRetTy::Default(_) if can_try_rewrite_without_block(body, context) => { + try_rewrite_without_block(body, &prefix, capture, context, shape, body_shape) } _ => None, }; @@ -110,6 +103,18 @@ pub(crate) fn rewrite_closure( } } +// Check if closure block wrapping may not be preserved (#4394) +fn can_try_rewrite_without_block(body: &ast::Expr, context: &RewriteContext<'_>) -> bool { + if !context.inside_macro() + && (!context.config.preserve_closure_block_wrapping() + || !context.snippet(body.span).trim().starts_with('{')) + { + true + } else { + false + } +} + fn try_rewrite_without_block( expr: &ast::Expr, prefix: &str, From aa917fc52e9215b6276706923d9030537c22e8f2 Mon Sep 17 00:00:00 2001 From: David Bar-On Date: Thu, 26 Nov 2020 13:08:24 +0200 Subject: [PATCH 4/5] Further purifying code --- src/formatting/closures.rs | 25 ++++++++++++------------- 1 file changed, 12 insertions(+), 13 deletions(-) diff --git a/src/formatting/closures.rs b/src/formatting/closures.rs index c646b151ced..c2edde27e4a 100644 --- a/src/formatting/closures.rs +++ b/src/formatting/closures.rs @@ -55,8 +55,19 @@ pub(crate) fn rewrite_closure( .map(|s| format!("{} {}", prefix, s)); } + // Whether closure block wrapping may not be preserved (#4394). + let can_try_rewrite_without_block = if context.inside_macro() { + false + } else if context.config.preserve_closure_block_wrapping() + && context.snippet(body.span).trim_start().starts_with('{') + { + false + } else { + true + }; + let result = match fn_decl.output { - ast::FnRetTy::Default(_) if can_try_rewrite_without_block(body, context) => { + ast::FnRetTy::Default(_) if can_try_rewrite_without_block => { try_rewrite_without_block(body, &prefix, capture, context, shape, body_shape) } _ => None, @@ -103,18 +114,6 @@ pub(crate) fn rewrite_closure( } } -// Check if closure block wrapping may not be preserved (#4394) -fn can_try_rewrite_without_block(body: &ast::Expr, context: &RewriteContext<'_>) -> bool { - if !context.inside_macro() - && (!context.config.preserve_closure_block_wrapping() - || !context.snippet(body.span).trim().starts_with('{')) - { - true - } else { - false - } -} - fn try_rewrite_without_block( expr: &ast::Expr, prefix: &str, From 5e982ae6619523a07940bed0d6b48f2bc4366a03 Mon Sep 17 00:00:00 2001 From: David Bar-On Date: Tue, 1 Dec 2020 15:44:25 +0200 Subject: [PATCH 5/5] Cosmetic comment change --- src/formatting/closures.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/formatting/closures.rs b/src/formatting/closures.rs index c2edde27e4a..a47ace7107d 100644 --- a/src/formatting/closures.rs +++ b/src/formatting/closures.rs @@ -55,7 +55,7 @@ pub(crate) fn rewrite_closure( .map(|s| format!("{} {}", prefix, s)); } - // Whether closure block wrapping may not be preserved (#4394). + // Whether a closure block wrapping may not be preserved (#4394). let can_try_rewrite_without_block = if context.inside_macro() { false } else if context.config.preserve_closure_block_wrapping()