Refine chain breaking heuristics

Don't make a single line chain when it is was multi line in the source; allow overflow of the last chain element onto the next lines without breaking the chain.

src/chains.rs

@@ -20,7 +20,7 @@
 // argument function argument strategy.
 
 use rewrite::{Rewrite, RewriteContext};
-use utils::make_indent;
+use utils::{first_line_width, make_indent};
 use expr::rewrite_call;
 
 use syntax::{ast, ptr};
@@ -51,7 +51,7 @@ pub fn rewrite_chain(mut expr: &ast::Expr,
     let indent = offset + extra_indent;
 
     let max_width = try_opt!(width.checked_sub(extra_indent));
-    let rewrites = try_opt!(subexpr_list.into_iter()
+    let mut rewrites = try_opt!(subexpr_list.iter()
                                             .rev()
                                             .map(|e| {
                                                 rewrite_chain_expr(e,
@@ -62,8 +62,56 @@ pub fn rewrite_chain(mut expr: &ast::Expr,
                                             })
                                             .collect::<Option<Vec<_>>>());
 
-    let total_width = rewrites.iter().fold(0, |a, b| a + b.len()) + parent_rewrite.len();
-    let fits_single_line = total_width <= width && rewrites.iter().all(|s| !s.contains('\n'));
+    // Total of all items excluding the last.
+    let almost_total = rewrites.split_last()
+                               .unwrap()
+                               .1
+                               .iter()
+                               .fold(0, |a, b| a + first_line_width(b)) +
+                       parent_rewrite.len();
+    let total_width = almost_total + first_line_width(rewrites.last().unwrap());
+    let veto_single_line = if context.config.take_source_hints && subexpr_list.len() > 1 {
+        // Look at the source code. Unless all chain elements start on the same
+        // line, we won't consider putting them on a single line either.
+        let first_line_no = context.codemap.lookup_char_pos(subexpr_list[0].span.lo).line;
+
+        subexpr_list[1..]
+            .iter()
+            .any(|ex| context.codemap.lookup_char_pos(ex.span.hi).line != first_line_no)
+    } else {
+        false
+    };
+
+    let fits_single_line = !veto_single_line &&
+                           match subexpr_list[0].node {
+        ast::Expr_::ExprMethodCall(ref method_name, ref types, ref expressions)
+            if context.config.chains_overflow_last => {
+            let (last, init) = rewrites.split_last_mut().unwrap();
+
+            if init.iter().all(|s| !s.contains('\n')) && total_width <= width {
+                let last_rewrite = width.checked_sub(almost_total)
+                                        .and_then(|inner_width| {
+                                            rewrite_method_call(method_name.node,
+                                                                types,
+                                                                expressions,
+                                                                total_span,
+                                                                context,
+                                                                inner_width,
+                                                                offset + almost_total)
+                                        });
+                match last_rewrite {
+                    Some(mut string) => {
+                        ::std::mem::swap(&mut string, last);
+                        true
+                    }
+                    None => false,
+                }
+            } else {
+                false
+            }
+        }
+        _ => total_width <= width && rewrites.iter().all(|s| !s.contains('\n')),
+    };
 
     let connector = if fits_single_line {
         String::new()
@@ -101,7 +149,11 @@ fn rewrite_chain_expr(expr: &ast::Expr,
                       -> Option<String> {
     match expr.node {
         ast::Expr_::ExprMethodCall(ref method_name, ref types, ref expressions) => {
-            rewrite_method_call(method_name.node, types, expressions, span, context, width, offset)
+            let inner = &RewriteContext {
+                block_indent: offset,
+                ..*context
+            };
+            rewrite_method_call(method_name.node, types, expressions, span, inner, width, offset)
         }
         ast::Expr_::ExprField(_, ref field) => {
             Some(format!(".{}", field.node))
@@ -137,10 +189,7 @@ fn rewrite_method_call(method_name: ast::Ident,
     };
 
     let callee_str = format!(".{}{}", method_name, type_str);
-    let inner_context = &RewriteContext {
-        block_indent: offset,
-        ..*context
-    };
+    let span = mk_sp(args[0].span.hi, span.hi);
 
-    rewrite_call(inner_context, &callee_str, args, span, width, offset)
+    rewrite_call(context, &callee_str, &args[1..], span, width, offset)
 }

src/config.rs

@@ -133,8 +133,8 @@ create_config! {
     fn_args_density: Density,
     fn_args_layout: StructLitStyle,
     fn_arg_indent: BlockIndentStyle,
-    where_density: Density, // Should we at least try to put the where clause on the same line as
-                            // the rest of the function decl?
+    where_density: Density, // Should we at least try to put the where clause on
+                            // the same line as the rest of the function decl?
     where_indent: BlockIndentStyle, // Visual will be treated like Tabbed
     where_layout: ListTactic,
     where_pred_indent: BlockIndentStyle,
@@ -147,14 +147,14 @@ create_config! {
     report_todo: ReportTactic,
     report_fixme: ReportTactic,
     reorder_imports: bool, // Alphabetically, case sensitive.
-    expr_indent_style: BlockIndentStyle,
-    closure_indent_style: BlockIndentStyle,
     single_line_if_else: bool,
     format_strings: bool,
+    chains_overflow_last: bool,
+    take_source_hints: bool, // Retain some formatting characteristics from
+                             // the source code.
 }
 
 impl Default for Config {
-
     fn default() -> Config {
         Config {
             max_width: 100,
@@ -181,11 +181,10 @@ impl Default for Config {
             report_todo: ReportTactic::Always,
             report_fixme: ReportTactic::Never,
             reorder_imports: false,
-            expr_indent_style: BlockIndentStyle::Tabbed,
-            closure_indent_style: BlockIndentStyle::Visual,
             single_line_if_else: false,
             format_strings: true,
+            chains_overflow_last: true,
+            take_source_hints: true,
         }
     }
-
 }

src/expr.rs

@@ -40,6 +40,12 @@ impl Rewrite for ast::Expr {
                 }
             }
             ast::Expr_::ExprCall(ref callee, ref args) => {
+                // FIXME using byte lens instead of char lens (and probably all over the place too)
+                // 2 is for parens
+                let max_callee_width = try_opt!(width.checked_sub(2));
+                let callee_str = try_opt!(callee.rewrite(context, max_callee_width, offset));
+                let span = mk_sp(callee.span.hi, self.span.hi);
+
                 rewrite_call(context, &**callee, args, self.span, width, offset)
             }
             ast::Expr_::ExprParen(ref subexpr) => {
@@ -284,8 +290,10 @@ impl Rewrite for ast::Block {
                 };
 
                 if is_simple_block(self, context.codemap) && prefix.len() < width {
-                    let body =
-                        self.expr.as_ref().unwrap().rewrite(context, width - prefix.len(), offset);
+                    let body = self.expr
+                                   .as_ref()
+                                   .unwrap()
+                                   .rewrite(context, width - prefix.len(), offset);
                     if let Some(ref expr_str) = body {
                         let result = format!("{}{{ {} }}", prefix, expr_str);
                         if result.len() <= width && !result.contains('\n') {
@@ -677,15 +685,13 @@ impl Rewrite for ast::Arm {
         total_width += (pat_strs.len() - 1) * 3;
 
         let mut vertical = total_width > pat_budget || pat_strs.iter().any(|p| p.contains('\n'));
-        if !vertical {
+        if !vertical && context.config.take_source_hints {
             // If the patterns were previously stacked, keep them stacked.
-            // FIXME should be an option.
             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.find('\n').is_some();
         }
 
-
         let pats_width = if vertical {
             pat_strs[pat_strs.len() - 1].len()
         } else {
@@ -1015,9 +1021,10 @@ fn rewrite_struct_lit<'a>(context: &RewriteContext,
                                  match *item {
                                      StructLitField::Regular(ref field) => field.span.lo,
                                      StructLitField::Base(ref expr) => {
-                                         let last_field_hi = fields.last()
-                                                                   .map_or(span.lo,
-                                                                           |field| field.span.hi);
+                                         let last_field_hi = fields.last().map_or(span.lo,
+                                                                                  |field| {
+                                                                                      field.span.hi
+                                                                                  });
                                          let snippet = context.snippet(mk_sp(last_field_hi,
                                                                              expr.span.lo));
                                          let pos = snippet.find_uncommented("..").unwrap();

src/items.rs

@@ -512,7 +512,8 @@ impl<'a> FmtVisitor<'a> {
                                              |arg| arg.ty.span.hi,
                                              |arg| {
                                                  // FIXME silly width, indent
-                                                 arg.ty.rewrite(&self.get_context(), 1000, 0)
+                                                 arg.ty
+                                                    .rewrite(&self.get_context(), 1000, 0)
                                                     .unwrap()
                                              },
                                              span_after(field.span, "(", self.codemap),
@@ -810,8 +811,7 @@ impl<'a> FmtVisitor<'a> {
                          .map(|ty_param| ty_param.rewrite(&context, h_budget, offset).unwrap());
 
         // Extract comments between generics.
-        let lt_spans = lifetimes.iter()
-                                .map(|l| {
+        let lt_spans = lifetimes.iter().map(|l| {
             let hi = if l.bounds.is_empty() {
                 l.lifetime.span.hi
             } else {

src/lib.rs

@@ -10,6 +10,7 @@
 
 #![feature(rustc_private)]
 #![feature(custom_attribute)]
+#![feature(slice_splits)]
 #![allow(unused_attributes)]
 
 // TODO we're going to allocate a whole bunch of temp Strings, is it worth

src/lists.rs

@@ -269,9 +269,7 @@ impl<'a, T, I, F1, F2, F3> Iterator for ListItems<'a, I, F1, F2, F3>
     fn next(&mut self) -> Option<Self::Item> {
         let white_space: &[_] = &[' ', '\t'];
 
-        self.inner
-            .next()
-            .map(|item| {
+        self.inner.next().map(|item| {
             let mut new_lines = false;
             // Pre-comment
             let pre_snippet = self.codemap

src/types.rs

@@ -368,8 +368,9 @@ impl Rewrite for ast::TyParamBound {
 
 impl Rewrite for ast::TyParamBounds {
     fn rewrite(&self, context: &RewriteContext, width: usize, offset: usize) -> Option<String> {
-        let strs: Vec<_> =
-            self.iter().map(|b| b.rewrite(context, width, offset).unwrap()).collect();
+        let strs: Vec<_> = self.iter()
+                               .map(|b| b.rewrite(context, width, offset).unwrap())
+                               .collect();
         Some(strs.join(" + "))
     }
 }

tests/config/small_tabs.toml

@@ -21,7 +21,7 @@ enum_trailing_comma = true
 report_todo = "Always"
 report_fixme = "Never"
 reorder_imports = false
-expr_indent_style = "Tabbed"
-closure_indent_style = "Visual"
 single_line_if_else = false
 format_strings = true
+chains_overflow_last = true
+take_source_hints = true

tests/source/chains-no-overflow.rs

@@ -0,0 +1,38 @@
+// rustfmt-chains_overflow_last: false
+// Test chain formatting without overflowing the last item.
+
+fn main() {
+    bbbbbbbbbbbbbbbbbbb.ccccccccccccccccccccccccccccccccccccc
+                       .ddddddddddddddddddddddddddd();
+
+    bbbbbbbbbbbbbbbbbbb.ccccccccccccccccccccccccccccccccccccc.ddddddddddddddddddddddddddd.eeeeeeee();
+
+    x()
+        .y(|| match cond() { true => (), false => () });
+
+    loong_func()
+        .quux(move || if true {
+            1
+        } else {
+            2
+        });
+
+    fffffffffffffffffffffffffffffffffff(a,
+                                        {
+                                            SCRIPT_TASK_ROOT
+                                            .with(|root| {
+                                                                // Another case of write_list failing us.
+                                                *root.borrow_mut()  =   Some(&script_task);
+                                            });
+                                        });                                        
+
+    let suuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuum = xxxxxxx
+        .map(|x| x + 5)
+        .map(|x| x / 2)
+        .fold(0, |acc, x| acc + x);
+
+    aaaaaaaaaaaaaaaa.map(|x| {
+                         x += 1;
+                         x
+                     }).filter(some_mod::some_filter)
+}

tests/source/chains.rs

@@ -1,9 +1,9 @@
 // Test chain formatting.
 
 fn main() {
-    let a = b.c
-             .d
-             .1
+    // Don't put chains on a single list if it wasn't so in source.
+    let a = b .c
+    .d.1
                 .foo(|x| x + 1);
 
     bbbbbbbbbbbbbbbbbbb.ccccccccccccccccccccccccccccccccccccc
@@ -25,7 +25,6 @@ fn main() {
                                         {
                                             SCRIPT_TASK_ROOT
                                             .with(|root| {
-                                                                // Another case of write_list failing us.
                                                 *root.borrow_mut()  =   Some(&script_task);
                                             });
                                         });                                        

tests/source/expr-no-hints.rs

@@ -0,0 +1,8 @@
+// rustfmt-take_source_hints: false
+// We know best!
+
+fn main() {
+    a.b
+     .c
+     .d();
+}

tests/source/expr-visual-indent.rs

@@ -1,9 +0,0 @@
-// rustfmt-expr_indent_style: Visual
-
-// Visual level block indentation.
-
-fn matcher() {
-    Some(while true {
-        test();
-    })
-}

tests/system.rs

@@ -165,9 +165,7 @@ fn read_significant_comments(file_name: &str) -> HashMap<String, String> {
           .map(|line| line.ok().expect("Failed getting line."))
           .take_while(|line| line_regex.is_match(&line))
           .filter_map(|line| {
-              regex.captures_iter(&line)
-                   .next()
-                   .map(|capture| {
+              regex.captures_iter(&line).next().map(|capture| {
                   (capture.at(1).expect("Couldn\'t unwrap capture.").to_owned(),
                    capture.at(2).expect("Couldn\'t unwrap capture.").to_owned())
               })

tests/target/chains-no-overflow.rs

@@ -0,0 +1,45 @@
+// rustfmt-chains_overflow_last: false
+// Test chain formatting without overflowing the last item.
+
+fn main() {
+    bbbbbbbbbbbbbbbbbbb.ccccccccccccccccccccccccccccccccccccc.ddddddddddddddddddddddddddd();
+
+    bbbbbbbbbbbbbbbbbbb.ccccccccccccccccccccccccccccccccccccc
+                       .ddddddddddddddddddddddddddd
+                       .eeeeeeee();
+
+    x().y(|| {
+           match cond() {
+               true => (),
+               false => (),
+           }
+       });
+
+    loong_func()
+        .quux(move || {
+            if true {
+                1
+            } else {
+                2
+            }
+        });
+
+    fffffffffffffffffffffffffffffffffff(a,
+                                        {
+                                            SCRIPT_TASK_ROOT.with(|root| {
+                                                                // Another case of write_list failing us.
+                                                                *root.borrow_mut() = Some(&script_task);
+                                                            });
+                                        });
+
+    let suuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuum = xxxxxxx.map(|x| x + 5)
+                                                                          .map(|x| x / 2)
+                                                                          .fold(0,
+                                                                                |acc, x| acc + x);
+
+    aaaaaaaaaaaaaaaa.map(|x| {
+                        x += 1;
+                        x
+                    })
+                    .filter(some_mod::some_filter)
+}

tests/target/chains-no-overlow-2.rs

@@ -0,0 +1,16 @@
+// rustfmt-chains_overflow_last: false
+
+fn main() {
+    reader.lines()
+          .map(|line| line.ok().expect("Failed getting line."))
+          .take_while(|line| line_regex.is_match(&line))
+          .filter_map(|line| {
+              regex.captures_iter(&line)
+                   .next()
+                   .map(|capture| {
+                       (capture.at(1).expect("Couldn\'t unwrap capture.").to_owned(),
+                        capture.at(2).expect("Couldn\'t unwrap capture.").to_owned())
+                   })
+          })
+          .collect();
+}

tests/target/chains.rs

@@ -1,7 +1,11 @@
 // Test chain formatting.
 
 fn main() {
-    let a = b.c.d.1.foo(|x| x + 1);
+    // Don't put chains on a single list if it wasn't so in source.
+    let a = b.c
+             .d
+             .1
+             .foo(|x| x + 1);
 
     bbbbbbbbbbbbbbbbbbb.ccccccccccccccccccccccccccccccccccccc.ddddddddddddddddddddddddddd();
 
@@ -16,8 +20,7 @@ fn main() {
         }
     });
 
-    loong_func()
-        .quux(move || {
+    loong_func().quux(move || {
         if true {
             1
         } else {
@@ -28,7 +31,6 @@ fn main() {
     fffffffffffffffffffffffffffffffffff(a,
                                         {
                                             SCRIPT_TASK_ROOT.with(|root| {
-                                                                // Another case of write_list failing us.
                                                 *root.borrow_mut() = Some(&script_task);
                                             });
                                         });

tests/target/expr-no-hints.rs

@@ -0,0 +1,6 @@
+// rustfmt-take_source_hints: false
+// We know best!
+
+fn main() {
+    a.b.c.d();
+}

tests/target/expr-visual-indent.rs

@@ -1,9 +0,0 @@
-// rustfmt-expr_indent_style: Visual
-
-// Visual level block indentation.
-
-fn matcher() {
-    Some(while true {
-        test();
-    })
-}