From 4110c7b8c5d2ff671799236350e8a8ffdad34ba7 Mon Sep 17 00:00:00 2001 From: Seiichi Uchida Date: Sun, 10 Dec 2017 21:19:51 +0900 Subject: [PATCH 1/7] Add a test for #2262 --- tests/source/closure.rs | 7 +++++++ tests/target/closure.rs | 9 +++++++++ tests/target/configs-fn_call_indent-block.rs | 8 +++----- 3 files changed, 19 insertions(+), 5 deletions(-) diff --git a/tests/source/closure.rs b/tests/source/closure.rs index cac3b493bfc..e93cc3fb40f 100644 --- a/tests/source/closure.rs +++ b/tests/source/closure.rs @@ -204,3 +204,10 @@ fn issue2207() { a_very_very_very_very_very_very_very_long_function_name_or_anything_else() }.to_string()) } + +fn issue2262() { + result.init(&mut result.slave.borrow_mut(), &mut (result.strategy)()).map_err(|factory| Error { + factory, + slave: None, + })?; +} diff --git a/tests/target/closure.rs b/tests/target/closure.rs index 0a45ad841c6..1912c16ef6e 100644 --- a/tests/target/closure.rs +++ b/tests/target/closure.rs @@ -240,3 +240,12 @@ fn issue2207() { .to_string() }) } + +fn issue2262() { + result + .init(&mut result.slave.borrow_mut(), &mut (result.strategy)()) + .map_err(|factory| Error { + factory, + slave: None, + })?; +} diff --git a/tests/target/configs-fn_call_indent-block.rs b/tests/target/configs-fn_call_indent-block.rs index 303d6419a65..d3522214c2e 100644 --- a/tests/target/configs-fn_call_indent-block.rs +++ b/tests/target/configs-fn_call_indent-block.rs @@ -72,11 +72,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), }, )?; From 5871967312dffd7df20c692ffe233719ac41b10c Mon Sep 17 00:00:00 2001 From: Seiichi Uchida Date: Sun, 10 Dec 2017 21:27:28 +0900 Subject: [PATCH 2/7] Verify whether adding block is safe in rewrite_closure_with_block() Also ensure that the expression is nested to avoid false-positive. --- src/closures.rs | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/src/closures.rs b/src/closures.rs index 73ef2064295..3a80c7adc93 100644 --- a/src/closures.rs +++ b/src/closures.rs @@ -118,6 +118,12 @@ fn rewrite_closure_with_block( context: &RewriteContext, shape: Shape, ) -> Option { + let left_most = left_most_sub_expr(body); + let veto_block = left_most != body && !classify::expr_requires_semi_to_be_stmt(left_most); + if veto_block { + return None; + } + let block = ast::Block { stmts: vec![ ast::Stmt { @@ -142,9 +148,6 @@ fn rewrite_closure_expr( shape: Shape, ) -> Option { let mut rewrite = expr.rewrite(context, shape); - if classify::expr_requires_semi_to_be_stmt(left_most_sub_expr(expr)) { - rewrite = and_one_line(rewrite); - } rewrite = rewrite.and_then(|rw| { if context.config.force_multiline_blocks() && rw.contains('\n') { None From 812fc4ca560261c76c9b988be8c2aa9242c50431 Mon Sep 17 00:00:00 2001 From: Seiichi Uchida Date: Sun, 10 Dec 2017 21:30:12 +0900 Subject: [PATCH 3/7] Remove and_one_line() --- src/closures.rs | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/closures.rs b/src/closures.rs index 3a80c7adc93..bf110bf8002 100644 --- a/src/closures.rs +++ b/src/closures.rs @@ -348,7 +348,3 @@ fn is_block_closure_forced(expr: &ast::Expr) -> bool { _ => false, } } - -fn and_one_line(x: Option) -> Option { - x.and_then(|x| if x.contains('\n') { None } else { Some(x) }) -} From bd6bef8cfa851728a300f77066de3542ccea9362 Mon Sep 17 00:00:00 2001 From: Seiichi Uchida Date: Sun, 10 Dec 2017 21:49:59 +0900 Subject: [PATCH 4/7] Move macro check to is_block_closure_forced() --- src/closures.rs | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-) diff --git a/src/closures.rs b/src/closures.rs index bf110bf8002..05e121560f9 100644 --- a/src/closures.rs +++ b/src/closures.rs @@ -81,7 +81,7 @@ fn try_rewrite_without_block( ) -> Option { let expr = get_inner_expr(expr, prefix, context); - if is_block_closure_forced(expr) { + if is_block_closure_forced(context, expr) { rewrite_closure_with_block(expr, prefix, context, shape) } else { rewrite_closure_expr(expr, prefix, context, body_shape) @@ -107,7 +107,7 @@ fn get_inner_expr<'a>( // Figure out if a block is necessary. fn needs_block(block: &ast::Block, prefix: &str, context: &RewriteContext) -> bool { - is_unsafe_block(block) || block.stmts.len() > 1 || context.inside_macro + is_unsafe_block(block) || block.stmts.len() > 1 || block_contains_comment(block, context.codemap) || prefix.contains('\n') } @@ -272,15 +272,11 @@ pub fn rewrite_last_closure( if prefix.contains('\n') { return None; } - // If we are inside macro, we do not want to add or remove block from closure body. - if context.inside_macro { - return expr.rewrite(context, shape); - } let body_shape = shape.offset_left(extra_offset)?; // We force to use block for the body of the closure for certain kinds of expressions. - if is_block_closure_forced(body) { + if is_block_closure_forced(context, body) { return rewrite_closure_with_block(body, &prefix, context, body_shape).and_then( |body_str| { // If the expression can fit in a single line, we need not force block closure. @@ -332,7 +328,16 @@ where .count() > 1 } -fn is_block_closure_forced(expr: &ast::Expr) -> bool { +fn is_block_closure_forced(context: &RewriteContext, expr: &ast::Expr) -> bool { + // If we are inside macro, we do not want to add or remove block from closure body. + if context.inside_macro { + false + } else { + is_block_closure_forced_inner(expr) + } +} + +fn is_block_closure_forced_inner(expr: &ast::Expr) -> bool { match expr.node { ast::ExprKind::If(..) | ast::ExprKind::IfLet(..) @@ -344,7 +349,7 @@ fn is_block_closure_forced(expr: &ast::Expr) -> bool { | ast::ExprKind::Box(ref expr) | ast::ExprKind::Try(ref expr) | ast::ExprKind::Unary(_, ref expr) - | ast::ExprKind::Cast(ref expr, _) => is_block_closure_forced(expr), + | ast::ExprKind::Cast(ref expr, _) => is_block_closure_forced_inner(expr), _ => false, } } From 42726906f7e1d08ce7b3ef78cf3cf2150a8cb1be Mon Sep 17 00:00:00 2001 From: Seiichi Uchida Date: Sun, 10 Dec 2017 21:52:23 +0900 Subject: [PATCH 5/7] Allow struct to be multi-lined in closure's body without block --- src/closures.rs | 39 ++++++++++++++++++++++++++++++--------- 1 file changed, 30 insertions(+), 9 deletions(-) diff --git a/src/closures.rs b/src/closures.rs index 05e121560f9..f169a3f4d22 100644 --- a/src/closures.rs +++ b/src/closures.rs @@ -147,15 +147,37 @@ fn rewrite_closure_expr( context: &RewriteContext, shape: Shape, ) -> Option { - let mut rewrite = expr.rewrite(context, shape); - rewrite = rewrite.and_then(|rw| { - if context.config.force_multiline_blocks() && rw.contains('\n') { - None - } else { - Some(rw) + fn allow_multi_line(expr: &ast::Expr) -> bool { + match expr.node { + ast::ExprKind::Match(..) + | ast::ExprKind::Block(..) + | ast::ExprKind::Catch(..) + | ast::ExprKind::Loop(..) + | ast::ExprKind::Struct(..) => true, + + ast::ExprKind::AddrOf(_, ref expr) + | ast::ExprKind::Box(ref expr) + | ast::ExprKind::Try(ref expr) + | ast::ExprKind::Unary(_, ref expr) + | ast::ExprKind::Cast(ref expr, _) => allow_multi_line(expr), + + _ => false, } - }); - rewrite.map(|rw| format!("{} {}", prefix, rw)) + } + + // When rewriting closure's body without block, we require it to fit in a single line + // unless it is a block-like expression or we are inside macro call. + let veto_multiline = (!allow_multi_line(expr) && !context.inside_macro) + || context.config.force_multiline_blocks(); + expr.rewrite(context, shape) + .and_then(|rw| { + if veto_multiline && rw.contains('\n') { + None + } else { + Some(rw) + } + }) + .map(|rw| format!("{} {}", prefix, rw)) } // Rewrite closure whose body is block. @@ -341,7 +363,6 @@ fn is_block_closure_forced_inner(expr: &ast::Expr) -> bool { match expr.node { ast::ExprKind::If(..) | ast::ExprKind::IfLet(..) - | ast::ExprKind::Loop(..) | ast::ExprKind::While(..) | ast::ExprKind::WhileLet(..) | ast::ExprKind::ForLoop(..) => true, From 90383d7426c2a71bd7b6b904f81a185dc553d128 Mon Sep 17 00:00:00 2001 From: Seiichi Uchida Date: Sun, 10 Dec 2017 21:53:01 +0900 Subject: [PATCH 6/7] Do not set inside_macro flag when converting try!() to '?' This will keep rustfmt idempotent when using 'use_try_shorthand' config option. --- src/macros.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/macros.rs b/src/macros.rs index 3c5db3c977a..731ebd5adfc 100644 --- a/src/macros.rs +++ b/src/macros.rs @@ -118,6 +118,7 @@ pub fn rewrite_macro( context.inside_macro = true; if context.config.use_try_shorthand() { if let Some(expr) = convert_try_mac(mac, context) { + context.inside_macro = false; return expr.rewrite(context, shape); } } From e3d2f2c2b1a33265375fde45e5152c9250f81b19 Mon Sep 17 00:00:00 2001 From: Seiichi Uchida Date: Sun, 10 Dec 2017 21:54:26 +0900 Subject: [PATCH 7/7] Cargo fmt --- src/chains.rs | 10 ++++++---- src/expr.rs | 7 ++++--- src/items.rs | 20 ++++++++++++-------- 3 files changed, 22 insertions(+), 15 deletions(-) diff --git a/src/chains.rs b/src/chains.rs index 816ce60ad4d..7512ef72e27 100644 --- a/src/chains.rs +++ b/src/chains.rs @@ -419,10 +419,12 @@ fn rewrite_chain_subexpr( context: &RewriteContext, shape: Shape, ) -> Option { - let rewrite_element = |expr_str: String| if expr_str.len() <= shape.width { - Some(expr_str) - } else { - None + let rewrite_element = |expr_str: String| { + if expr_str.len() <= shape.width { + Some(expr_str) + } else { + None + } }; match expr.node { diff --git a/src/expr.rs b/src/expr.rs index 259ac0007ed..ed4adffb9f5 100644 --- a/src/expr.rs +++ b/src/expr.rs @@ -2316,12 +2316,13 @@ fn rewrite_paren(context: &RewriteContext, subexpr: &ast::Expr, shape: Shape) -> .offset_left(paren_overhead) .and_then(|s| s.sub_width(paren_overhead))?; - let paren_wrapper = - |s: &str| if context.config.spaces_within_parens_and_brackets() && !s.is_empty() { + let paren_wrapper = |s: &str| { + if context.config.spaces_within_parens_and_brackets() && !s.is_empty() { format!("( {} )", s) } else { format!("({})", s) - }; + } + }; let subexpr_str = subexpr.rewrite(context, sub_shape)?; debug!("rewrite_paren, subexpr_str: `{:?}`", subexpr_str); diff --git a/src/items.rs b/src/items.rs index b3647f26f53..496c66971d7 100644 --- a/src/items.rs +++ b/src/items.rs @@ -482,10 +482,12 @@ impl<'a> FmtVisitor<'a> { enum_def.variants.iter(), "}", ",", - |f| if !f.node.attrs.is_empty() { - f.node.attrs[0].span.lo() - } else { - f.span.lo() + |f| { + if !f.node.attrs.is_empty() { + f.node.attrs[0].span.lo() + } else { + f.span.lo() + } }, |f| f.span.hi(), |f| self.format_variant(f, one_line_width), @@ -2549,10 +2551,12 @@ fn rewrite_where_clause_rfc_style( }; let preds_str = write_list(&items.collect::>(), &fmt)?; - let comment_separator = |comment: &str, shape: Shape| if comment.is_empty() { - String::new() - } else { - format!("\n{}", shape.indent.to_string(context.config)) + let comment_separator = |comment: &str, shape: Shape| { + if comment.is_empty() { + String::new() + } else { + format!("\n{}", shape.indent.to_string(context.config)) + } }; let newline_before_where = comment_separator(&comment_before, shape); let newline_after_where = comment_separator(&comment_after, clause_shape);