Merge pull request #2068 from topecongiro/issue-2067

Fix a subtle bug in rewriting chain

Author: nrc

src/chains.rs

@@ -64,7 +64,8 @@ use config::IndentStyle;
 use expr::rewrite_call;
 use macros::convert_try_mac;
 use rewrite::{Rewrite, RewriteContext};
-use utils::{first_line_width, last_line_extendable, last_line_width, mk_sp, wrap_str};
+use utils::{first_line_width, last_line_extendable, last_line_width, mk_sp,
+            trimmed_last_line_width, wrap_str};
 
 use std::cmp::min;
 use std::iter;
@@ -125,7 +126,7 @@ pub fn rewrite_chain(expr: &ast::Expr, context: &RewriteContext, shape: Shape) -
 
     let first_child_shape = if extend {
         let overhead = last_line_width(&parent_rewrite);
-        let offset = parent_rewrite.lines().rev().next().unwrap().trim().len();
+        let offset = trimmed_last_line_width(&parent_rewrite);
         match context.config.chain_indent() {
             IndentStyle::Visual => parent_shape.offset_left(overhead)?,
             IndentStyle::Block => parent_shape.block().offset_left(offset)?,
@@ -156,7 +157,7 @@ pub fn rewrite_chain(expr: &ast::Expr, context: &RewriteContext, shape: Shape) -
         last_line_width(&parent_rewrite)
     } else {
         rewrites.iter().fold(0, |a, b| a + b.len()) + parent_rewrite.len()
-    };
+    } + suffix_try_num;
     let one_line_budget = if rewrites.is_empty() && !context.config.chain_split_single_child() {
         shape.width
     } else {
@@ -165,25 +166,64 @@ pub fn rewrite_chain(expr: &ast::Expr, context: &RewriteContext, shape: Shape) -
     let all_in_one_line = !parent_rewrite_contains_newline
         && rewrites.iter().all(|s| !s.contains('\n'))
         && almost_total < one_line_budget;
-    let last_shape = if rewrites.is_empty() {
-        // We only have a single child.
-        first_child_shape
-    } else {
-        match context.config.chain_indent() {
-            IndentStyle::Visual => other_child_shape.sub_width(shape.rhs_overhead(context.config))?,
-            IndentStyle::Block => other_child_shape,
-        }
+    let last_shape = match context.config.chain_indent() {
+        IndentStyle::Visual => other_child_shape.sub_width(shape.rhs_overhead(context.config))?,
+        IndentStyle::Block => other_child_shape,
     };
     let last_shape = last_shape.sub_width(suffix_try_num)?;
+
+    // Rewrite the last child. The last child of a chain requires special treatment. We need to
+    // know whether 'overflowing' the last child make a better formatting:
+    //
+    // A chain with overflowing the last child:
+    // ```
+    // parent.child1.child2.last_child(
+    //     a,
+    //     b,
+    //     c,
+    // )
+    // ```
+    //
+    // A chain without overflowing the last child (in vertical layout):
+    // ```
+    // parent
+    //     .child1
+    //     .child2
+    //     .last_child(a, b, c)
+    // ```
+    //
+    // In particular, overflowing is effective when the last child is a method with a multi-lined
+    // block-like argument (e.g. closure):
+    // ```
+    // parent.child1.chlid2.last_child(|a, b, c| {
+    //     let x = foo(a, b, c);
+    //     let y = bar(a, b, c);
+    //
+    //     // ...
+    //
+    //     result
+    // })
+    // ```
+
+    // `rewrite_last` rewrites the last child on its own line. We use a closure here instead of
+    // directly calling `rewrite_chain_subexpr()` to avoid exponential blowup.
     let rewrite_last = || rewrite_chain_subexpr(last_subexpr, total_span, context, last_shape);
     let (last_subexpr_str, fits_single_line) = if all_in_one_line || extend_last_subexr {
+        // First we try to 'overflow' the last child and see if it looks better than using
+        // vertical layout.
         parent_shape.offset_left(almost_total).map(|shape| {
             if let Some(rw) = rewrite_chain_subexpr(last_subexpr, total_span, context, shape) {
+                // We allow overflowing here only if both of the following conditions match:
+                // 1. The entire chain fits in a single line expect the last child.
+                // 2. `last_chlid_str.lines().count() >= 5`.
                 let line_count = rw.lines().count();
                 let fits_single_line = almost_total + first_line_width(&rw) <= one_line_budget;
-                if fits_single_line && (line_count >= 5 || extend_last_subexr) {
+                if fits_single_line && line_count >= 5 {
                     (Some(rw), true)
                 } else {
+                    // We could not know whether overflowing is better than using vertical layout,
+                    // just by looking at the overflowed rewrite. Now we rewrite the last child
+                    // on its own line, and compare two rewrites to choose which is better.
                     match rewrite_last() {
                         Some(ref new_rw) if !fits_single_line => (Some(new_rw.clone()), false),
                         Some(ref new_rw) if new_rw.lines().count() >= line_count => {

src/expr.rs

@@ -1871,7 +1871,9 @@ fn rewrite_pat_expr(
         } else {
             format!("{} ", matcher)
         };
-        let pat_shape = shape.offset_left(matcher.len())?.sub_width(connector.len())?;
+        let pat_shape = shape
+            .offset_left(matcher.len())?
+            .sub_width(connector.len())?;
         let pat_string = pat.rewrite(context, pat_shape)?;
         let result = format!("{}{}{}", matcher, pat_string, connector);
         return rewrite_assign_rhs(context, result, expr, shape);

src/items.rs

@@ -2037,9 +2037,7 @@ fn rewrite_args(
     generics_str_contains_newline: bool,
 ) -> Option<String> {
     let mut arg_item_strs = args.iter()
-        .map(|arg| {
-            arg.rewrite(context, Shape::legacy(multi_line_budget, arg_indent))
-        })
+        .map(|arg| arg.rewrite(context, Shape::legacy(multi_line_budget, arg_indent)))
         .collect::<Option<Vec<_>>>()?;
 
     // Account for sugary self.
@@ -2713,17 +2711,15 @@ impl Rewrite for ast::ForeignItem {
         let span = mk_sp(self.span.lo(), self.span.hi() - BytePos(1));
 
         let item_str = match self.node {
-            ast::ForeignItemKind::Fn(ref fn_decl, ref generics) => {
-                rewrite_fn_base(
-                    context,
-                    shape.indent,
-                    self.ident,
-                    &FnSig::new(fn_decl, generics, self.vis.clone()),
-                    span,
-                    false,
-                    false,
-                ).map(|(s, _)| format!("{};", s))
-            }
+            ast::ForeignItemKind::Fn(ref fn_decl, ref generics) => rewrite_fn_base(
+                context,
+                shape.indent,
+                self.ident,
+                &FnSig::new(fn_decl, generics, self.vis.clone()),
+                span,
+                false,
+                false,
+            ).map(|(s, _)| format!("{};", s)),
             ast::ForeignItemKind::Static(ref ty, is_mutable) => {
                 // FIXME(#21): we're dropping potential comments in between the
                 // function keywords here.

src/string.rs

@@ -67,7 +67,9 @@ pub fn rewrite_string<'a>(orig: &str, fmt: &StringFormat<'a>) -> Option<String>
     let ender_length = fmt.line_end.len();
     // If we cannot put at least a single character per line, the rewrite won't
     // succeed.
-    let max_chars = shape.width.checked_sub(fmt.opener.len() + ender_length + 1)? + 1;
+    let max_chars = shape
+        .width
+        .checked_sub(fmt.opener.len() + ender_length + 1)? + 1;
 
     // Snip a line at a time from `orig` until it is used up. Push the snippet
     // onto result.

tests/source/chains.rs

@@ -163,3 +163,10 @@ fn issue1392() {
         }
         "#.trim());
 }
+
+// #2067
+impl Settings {
+    fn save(&self) -> Result<()> {
+        let mut file = File::create(&settings_path).chain_err(|| ErrorKind::WriteError(settings_path.clone()))?;
+    }
+}

tests/target/chains.rs

@@ -183,3 +183,11 @@ fn issue1392() {
         "#.trim(),
     );
 }
+
+// #2067
+impl Settings {
+    fn save(&self) -> Result<()> {
+        let mut file = File::create(&settings_path)
+            .chain_err(|| ErrorKind::WriteError(settings_path.clone()))?;
+    }
+}