Change the logic around breaking multiple patterns in match arms

Refactor to use the list code, don't preserve original stacking-ness, base vertical vs mixed formatting on complexity of the patterns.

Closes #386

src/config.rs

@@ -87,7 +87,8 @@ impl Density {
     pub fn to_list_tactic(self) -> ListTactic {
         match self {
             Density::Compressed => ListTactic::Mixed,
-            Density::Tall | Density::CompressedIfEmpty => ListTactic::HorizontalVertical,
+            Density::Tall |
+            Density::CompressedIfEmpty => ListTactic::HorizontalVertical,
             Density::Vertical => ListTactic::Vertical,
         }
     }

src/expr.rs

@@ -497,7 +497,8 @@ impl Rewrite for ast::Stmt {
                     None
                 }
             }
-            ast::StmtKind::Expr(ref ex, _) | ast::StmtKind::Semi(ref ex, _) => {
+            ast::StmtKind::Expr(ref ex, _) |
+            ast::StmtKind::Semi(ref ex, _) => {
                 let suffix = if semicolon_for_stmt(self) {
                     ";"
                 } else {
@@ -953,7 +954,6 @@ fn arm_comma(config: &Config, arm: &ast::Arm, body: &ast::Expr) -> &'static str
 impl Rewrite for ast::Arm {
     fn rewrite(&self, context: &RewriteContext, width: usize, offset: Indent) -> Option<String> {
         let &ast::Arm { ref attrs, ref pats, ref guard, ref body } = self;
-        let indent_str = offset.to_string(context.config);
 
         // FIXME this is all a bit grotty, would be nice to abstract out the
         // treatment of attributes.
@@ -980,38 +980,34 @@ impl Rewrite for ast::Arm {
                                     .map(|p| p.rewrite(context, pat_budget, offset))
                                     .collect::<Option<Vec<_>>>());
 
-        let mut total_width = pat_strs.iter().fold(0, |a, p| a + p.len());
-        // Add ` | `.len().
-        total_width += (pat_strs.len() - 1) * 3;
+        let all_simple = pat_strs.iter().all(|p| pat_is_simple(&p));
+        let items: Vec<_> = pat_strs.into_iter().map(|s| ListItem::from_str(s)).collect();
+        let fmt = ListFormatting {
+            tactic: if all_simple {
+                DefinitiveListTactic::Mixed
+            } else {
+                DefinitiveListTactic::Vertical
+            },
+            separator: " |",
+            trailing_separator: SeparatorTactic::Never,
+            indent: offset,
+            width: pat_budget,
+            ends_with_newline: false,
+            config: context.config,
+        };
+        let pats_str = try_opt!(write_list(items, &fmt));
 
-        let mut vertical = total_width > pat_budget || pat_strs.iter().any(|p| p.contains('\n'));
-        if !vertical && context.config.take_source_hints {
-            // If the patterns were previously stacked, keep them stacked.
-            let pat_span = mk_sp(pats[0].span.lo, pats[pats.len() - 1].span.hi);
-            let pat_str = context.snippet(pat_span);
-            vertical = pat_str.contains('\n');
-        }
-
-        let pats_width = if vertical {
-            pat_strs.last().unwrap().len()
+        let budget = if pats_str.contains('\n') {
+            context.config.max_width
         } else {
-            total_width
+            width
         };
 
-        let mut pats_str = String::new();
-        for p in pat_strs {
-            if !pats_str.is_empty() {
-                if vertical {
-                    pats_str.push_str(" |\n");
-                    pats_str.push_str(&indent_str);
-                } else {
-                    pats_str.push_str(" | ");
-                }
-            }
-            pats_str.push_str(&p);
-        }
-
-        let guard_str = try_opt!(rewrite_guard(context, guard, width, offset, pats_width));
+        let guard_str = try_opt!(rewrite_guard(context,
+                                               guard,
+                                               budget,
+                                               offset,
+                                               last_line_width(&pats_str)));
 
         let pats_str = format!("{}{}", pats_str, guard_str);
         // Where the next text can start.
@@ -1085,6 +1081,11 @@ impl Rewrite for ast::Arm {
     }
 }
 
+fn pat_is_simple(pat_str: &str) -> bool {
+    pat_str.len() <= 16 ||
+    (pat_str.len() <= 24 && pat_str.chars().all(|c| c.is_alphabetic() || c == ':'))
+}
+
 // The `if ...` guard on a match arm.
 fn rewrite_guard(context: &RewriteContext,
                  guard: &Option<ptr::P<ast::Expr>>,

src/macros.rs

@@ -58,7 +58,8 @@ pub fn rewrite_macro(mac: &ast::Mac,
                      -> Option<String> {
     let original_style = macro_style(mac, context);
     let macro_name = match extra_ident {
-        None | Some(ast::Ident { name: ast::Name(0), .. }) => format!("{}!", mac.node.path),
+        None |
+        Some(ast::Ident { name: ast::Name(0), .. }) => format!("{}!", mac.node.path),
         Some(ident) => format!("{}! {}", mac.node.path, ident),
     };
     let style = if FORCED_BRACKET_MACROS.contains(&&macro_name[..]) {

src/types.rs

@@ -552,7 +552,8 @@ impl Rewrite for ast::Ty {
             ast::TyKind::BareFn(ref bare_fn) => {
                 rewrite_bare_fn(bare_fn, self.span, context, width, offset)
             }
-            ast::TyKind::Mac(..) | ast::TyKind::Typeof(..) => unreachable!(),
+            ast::TyKind::Mac(..) |
+            ast::TyKind::Typeof(..) => unreachable!(),
         }
     }
 }

src/visitor.rs

@@ -46,7 +46,8 @@ impl<'a> FmtVisitor<'a> {
                     self.push_rewrite(stmt.span, rewrite);
                 }
             }
-            ast::StmtKind::Expr(..) | ast::StmtKind::Semi(..) => {
+            ast::StmtKind::Expr(..) |
+            ast::StmtKind::Semi(..) => {
                 let rewrite = stmt.rewrite(&self.get_context(),
                                            self.config.max_width - self.block_indent.width(),
                                            self.block_indent);

tests/source/match.rs

@@ -274,3 +274,13 @@ fn issue494() {
         }
     }
 }
+
+fn issue386() {
+    match foo {
+        BiEq | BiLt | BiLe | BiNe | BiGt | BiGe =>
+                    true,
+        BiAnd | BiOr | BiAdd | BiSub | BiMul | BiDiv | BiRem |
+        BiBitXor | BiBitAnd | BiBitOr | BiShl | BiShr =>
+            false,
+    }
+}

tests/target/match.rs

@@ -221,9 +221,8 @@ fn issue355() {
 fn issue280() {
     {
         match x {
-            CompressionMode::DiscardNewline | CompressionMode::CompressWhitespaceNewline => {
-                ch == '\n'
-            }
+            CompressionMode::DiscardNewline |
+            CompressionMode::CompressWhitespaceNewline => ch == '\n',
             ast::ItemConst(ref typ, ref expr) => {
                 self.process_static_or_const_item(item, &typ, &expr)
             }
@@ -260,7 +259,8 @@ fn issue496() {
         {
             {
                 match def {
-                    def::DefConst(def_id) | def::DefAssociatedConst(def_id) => {
+                    def::DefConst(def_id) |
+                    def::DefAssociatedConst(def_id) => {
                         match const_eval::lookup_const_by_id(cx.tcx, def_id, Some(self.pat.id)) {
                             Some(const_expr) => x,
                         }
@@ -274,7 +274,8 @@ fn issue496() {
 fn issue494() {
     {
         match stmt.node {
-            hir::StmtExpr(ref expr, id) | hir::StmtSemi(ref expr, id) => {
+            hir::StmtExpr(ref expr, id) |
+            hir::StmtSemi(ref expr, id) => {
                 result.push(StmtRef::Mirror(Box::new(Stmt {
                     span: stmt.span,
                     kind: StmtKind::Expr {
@@ -286,3 +287,11 @@ fn issue494() {
         }
     }
 }
+
+fn issue386() {
+    match foo {
+        BiEq | BiLt | BiLe | BiNe | BiGt | BiGe => true,
+        BiAnd | BiOr | BiAdd | BiSub | BiMul | BiDiv | BiRem | BiBitXor | BiBitAnd | BiBitOr |
+        BiShl | BiShr => false,
+    }
+}