Merge pull request #2265 from topecongiro/issue-2262

Fix bugs related to closures

src/chains.rs

@@ -419,10 +419,12 @@ fn rewrite_chain_subexpr(
     context: &RewriteContext,
     shape: Shape,
 ) -> Option<String> {
-    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 {

src/closures.rs

@@ -81,7 +81,7 @@ fn try_rewrite_without_block(
 ) -> Option<String> {
     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')
 }
 
@@ -118,6 +118,12 @@ fn rewrite_closure_with_block(
     context: &RewriteContext,
     shape: Shape,
 ) -> Option<String> {
+    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 {
@@ -141,18 +147,37 @@ fn rewrite_closure_expr(
     context: &RewriteContext,
     shape: Shape,
 ) -> Option<String> {
-    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
-        } 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.
@@ -269,15 +294,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.
@@ -329,11 +350,19 @@ pub fn args_have_many_closure<T>(args: &[&T]) -> bool
         .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(..)
-        | ast::ExprKind::Loop(..)
         | ast::ExprKind::While(..)
         | ast::ExprKind::WhileLet(..)
         | ast::ExprKind::ForLoop(..) => true,
@@ -341,11 +370,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,
     }
 }
-
-fn and_one_line(x: Option<String>) -> Option<String> {
-    x.and_then(|x| if x.contains('\n') { None } else { Some(x) })
-}

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);

src/items.rs

@@ -481,10 +481,12 @@ fn format_variant_list(
                 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),
@@ -2548,10 +2550,12 @@ fn rewrite_where_clause_rfc_style(
     };
     let preds_str = write_list(&items.collect::<Vec<_>>(), &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);

src/macros.rs

@@ -119,6 +119,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);
         }
     }

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,
+    })?;
+}

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,
+        })?;
+}

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),
         },
     )?;