Merge pull request #370 from marcusklaas/match-arm-delining

Improve heuristics for match arm body placement

1 files changed, 56 insertions, 31 deletions

diff --git a/src/expr.rs b/src/expr.rs
index f2e6b1e0dfa..4076298cec8 100644
--- a/src/expr.rs
+++ b/src/expr.rs
@@ -15,7 +15,7 @@ use Indent;
 use rewrite::{Rewrite, RewriteContext};
 use lists::{write_list, itemize_list, ListFormatting, SeparatorTactic, ListTactic};
 use string::{StringFormat, rewrite_string};
-use utils::{span_after, extra_offset, first_line_width, last_line_width, wrap_str, binary_search};
+use utils::{span_after, extra_offset, last_line_width, wrap_str, binary_search};
 use visitor::FmtVisitor;
 use config::{StructLitStyle, MultilineStyle};
 use comment::{FindUncommented, rewrite_comment, contains_comment};
@@ -155,11 +155,14 @@ impl Rewrite for ast::Expr {
                 rewrite_chain(self, context, width, offset)
             }
             ast::Expr_::ExprMac(ref mac) => {
-                // Failure to rewrite a marco should not imply failure to rewrite the Expr
-                rewrite_macro(mac, context, width, offset).or(wrap_str(context.snippet(self.span),
-                                                                       context.config.max_width,
-                                                                       width,
-                                                                       offset))
+                // Failure to rewrite a marco should not imply failure to
+                // rewrite the expression.
+                rewrite_macro(mac, context, width, offset).or_else(|| {
+                    wrap_str(context.snippet(self.span),
+                             context.config.max_width,
+                             width,
+                             offset)
+                })
             }
             ast::Expr_::ExprRet(None) => {
                 wrap_str("return".to_owned(),
@@ -168,10 +171,10 @@ impl Rewrite for ast::Expr {
                          offset)
             }
             ast::Expr_::ExprRet(Some(ref expr)) => {
-                rewrite_unary_prefix(context, "return ", &expr, width, offset)
+                rewrite_unary_prefix(context, "return ", expr, width, offset)
             }
             ast::Expr_::ExprBox(ref expr) => {
-                rewrite_unary_prefix(context, "box ", &expr, width, offset)
+                rewrite_unary_prefix(context, "box ", expr, width, offset)
             }
             ast::Expr_::ExprAddrOf(mutability, ref expr) => {
                 rewrite_expr_addrof(context, mutability, &expr, width, offset)
@@ -352,9 +355,9 @@ fn rewrite_closure(capture: ast::CaptureClause,
     Some(format!("{} {}", prefix, try_opt!(body_rewrite)))
 }
 
-fn nop_block_collapse(block_str: Option<String>) -> Option<String> {
+fn nop_block_collapse(block_str: Option<String>, budget: usize) -> Option<String> {
     block_str.map(|block_str| {
-        if block_str.starts_with("{") &&
+        if block_str.starts_with("{") && budget >= 2 &&
            (block_str[1..].find(|c: char| !c.is_whitespace()).unwrap() == block_str.len() - 2) {
             "{}".to_owned()
         } else {
@@ -872,15 +875,10 @@ impl Rewrite for ast::Arm {
         let pats_str = format!("{}{}", pats_str, guard_str);
         // Where the next text can start.
         let mut line_start = last_line_width(&pats_str);
-        if pats_str.find('\n').is_none() {
+        if !pats_str.contains('\n') {
             line_start += offset.width();
         }
 
-        let mut line_indent = offset + pats_width;
-        if vertical {
-            line_indent = line_indent.block_indent(context.config);
-        }
-
         let comma = if let ast::ExprBlock(_) = body.node {
             ""
         } else {
@@ -889,39 +887,66 @@ impl Rewrite for ast::Arm {
 
         // Let's try and get the arm body on the same line as the condition.
         // 4 = ` => `.len()
-        if context.config.max_width > line_start + comma.len() + 4 {
+        let same_line_body = if context.config.max_width > line_start + comma.len() + 4 {
             let budget = context.config.max_width - line_start - comma.len() - 4;
-            if let Some(ref body_str) = nop_block_collapse(body.rewrite(context,
-                                                                        budget,
-                                                                        line_indent + 4)) {
-                if first_line_width(body_str) <= budget {
+            let offset = Indent::new(offset.block_indent,
+                                     line_start + 4 - offset.block_indent);
+            let rewrite = nop_block_collapse(body.rewrite(context, budget, offset), budget);
+
+            match rewrite {
+                Some(ref body_str) if body_str.len() <= budget || comma.is_empty() =>
                     return Some(format!("{}{} => {}{}",
                                         attr_str.trim_left(),
                                         pats_str,
                                         body_str,
-                                        comma));
-                }
+                                        comma)),
+                _ => rewrite,
             }
-        }
+        } else {
+            None
+        };
 
-        // We have to push the body to the next line.
-        if comma.is_empty() {
+        if let ast::ExprBlock(_) = body.node {
             // We're trying to fit a block in, but it still failed, give up.
             return None;
         }
 
         let body_budget = try_opt!(width.checked_sub(context.config.tab_spaces));
-        let body_str = try_opt!(nop_block_collapse(body.rewrite(context,
-                                                                body_budget,
-                                                                context.block_indent)));
-        Some(format!("{}{} =>\n{}{},",
+        let next_line_body = nop_block_collapse(body.rewrite(context,
+                                                             body_budget,
+                                                             context.block_indent
+                                                                    .block_indent(context.config)),
+                                                body_budget);
+
+        let body_str = try_opt!(match_arm_heuristic(same_line_body.as_ref().map(|x| &x[..]),
+                                                    next_line_body.as_ref().map(|x| &x[..])));
+
+        let spacer = match same_line_body {
+            Some(ref body) if body == body_str => " ".to_owned(),
+            _ => format!("\n{}",
+                         offset.block_indent(context.config).to_string(context.config)),
+        };
+
+        Some(format!("{}{} =>{}{},",
                      attr_str.trim_left(),
                      pats_str,
-                     offset.block_indent(context.config).to_string(context.config),
+                     spacer,
                      body_str))
     }
 }
 
+// Takes two possible rewrites for the match arm body and chooses the "nicest".
+fn match_arm_heuristic<'a>(former: Option<&'a str>, latter: Option<&'a str>) -> Option<&'a str> {
+    match (former, latter) {
+        (f @ Some(..), None) => f,
+        (Some(f), Some(l)) if f.chars().filter(|&c| c == '\n').count() <=
+                              l.chars().filter(|&c| c == '\n').count() => {
+            Some(f)
+        }
+        (_, l) => l,
+    }
+}
+
 // The `if ...` guard on a match arm.
 fn rewrite_guard(context: &RewriteContext,
                  guard: &Option<ptr::P<ast::Expr>>,