Merge pull request #912 from rust-lang-nursery/pat-simple-mixed

Change the logic around breaking multiple patterns in match arms

src/config.rs

@@ -98,7 +98,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

@@ -21,7 +21,7 @@ use lists::{write_list, itemize_list, ListFormatting, SeparatorTactic, ListTacti
             DefinitiveListTactic, definitive_tactic, ListItem, format_item_list};
 use string::{StringFormat, rewrite_string};
 use utils::{CodeMapSpanUtils, extra_offset, last_line_width, wrap_str, binary_search,
-            first_line_width, semicolon_for_stmt};
+            first_line_width, semicolon_for_stmt, trimmed_last_line_width};
 use visitor::FmtVisitor;
 use config::{Config, StructLitStyle, MultilineStyle};
 use comment::{FindUncommented, rewrite_comment, contains_comment, recover_comment_removed};
@@ -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 - offset.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,
+                                               trimmed_last_line_width(&pats_str)));
 
         let pats_str = format!("{}{}", pats_str, guard_str);
         // Where the next text can start.
@@ -1023,7 +1019,7 @@ impl Rewrite for ast::Arm {
         let body = match **body {
             ast::Expr { node: ast::ExprKind::Block(ref block), .. }
                 if !is_unsafe_block(block) && is_simple_block(block, context.codemap) &&
-                context.config.wrap_match_arms => block.expr.as_ref().map(|e| &**e).unwrap(),
+                   context.config.wrap_match_arms => block.expr.as_ref().map(|e| &**e).unwrap(),
             ref x => x,
         };
 
@@ -1085,6 +1081,13 @@ impl Rewrite for ast::Arm {
     }
 }
 
+// A pattern is simple if it is very short or it is short-ish and just a path.
+// E.g. `Foo::Bar` is simple, but `Foo(..)` is not.
+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>>,
@@ -1106,11 +1109,12 @@ fn rewrite_guard(context: &RewriteContext,
         }
 
         // Not enough space to put the guard after the pattern, try a newline.
-        let overhead = context.config.tab_spaces + 4 + 5;
+        let overhead = offset.block_indent(context.config).width() + 4 + 5;
         if overhead < width {
             let cond_str = guard.rewrite(context,
                                          width - overhead,
-                                         offset.block_indent(context.config));
+                                         // 3 == `if `
+                                         offset.block_indent(context.config) + 3);
             if let Some(cond_str) = cond_str {
                 return Some(format!("\n{}if {}",
                                     offset.block_indent(context.config).to_string(context.config),

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/utils.rs

@@ -113,6 +113,13 @@ pub fn last_line_width(s: &str) -> usize {
         None => s.len(),
     }
 }
+#[inline]
+pub fn trimmed_last_line_width(s: &str) -> usize {
+    match s.rfind('\n') {
+        Some(n) => s[(n + 1)..].trim().len(),
+        None => s.trim().len(),
+    }
+}
 
 #[inline]
 fn is_skip(meta_item: &MetaItem) -> bool {

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,23 @@ 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,
+    }
+}
+
+fn guards() {
+    match foo {
+        aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa if foooooooooooooo && barrrrrrrrrrrr => {}
+        aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa | aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa if foooooooooooooo && barrrrrrrrrrrr => {}
+        aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
+            if fooooooooooooooooooooo &&
+               (bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb || cccccccccccccccccccccccccccccccccccccccc) => {}
+    }
+}

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,25 @@ 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,
+    }
+}
+
+fn guards() {
+    match foo {
+        aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa if foooooooooooooo &&
+                                                                      barrrrrrrrrrrr => {}
+        aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa |
+        aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa if foooooooooooooo &&
+                                                                      barrrrrrrrrrrr => {}
+        aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
+            if fooooooooooooooooooooo &&
+               (bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb ||
+                cccccccccccccccccccccccccccccccccccccccc) => {}
+    }
+}