Fix budget-related bugs when rewriting chain

src/bin/rustfmt.rs

@@ -87,9 +87,10 @@ impl CliOptions {
             if let Ok(write_mode) = WriteMode::from_str(write_mode) {
                 options.write_mode = Some(write_mode);
             } else {
-                return Err(FmtError::from(
-                    format!("Invalid write-mode: {}", write_mode),
-                ));
+                return Err(FmtError::from(format!(
+                    "Invalid write-mode: {}",
+                    write_mode
+                )));
             }
         }
 

src/chains.rs

@@ -147,9 +147,8 @@ pub fn rewrite_chain(expr: &ast::Expr, context: &RewriteContext, shape: Shape) -
     let last_subexpr = &subexpr_list[suffix_try_num];
     let subexpr_list = &subexpr_list[suffix_try_num..subexpr_num - prefix_try_num];
     let iter = subexpr_list.iter().skip(1).rev().zip(child_shape_iter);
-    let mut rewrites = iter.map(|(e, shape)| {
-        rewrite_chain_subexpr(e, total_span, context, shape)
-    }).collect::<Option<Vec<_>>>()?;
+    let mut rewrites = iter.map(|(e, shape)| rewrite_chain_subexpr(e, total_span, context, shape))
+        .collect::<Option<Vec<_>>>()?;
 
     // Total of all items excluding the last.
     let extend_last_subexpr = last_line_extendable(&parent_rewrite) && rewrites.is_empty();
@@ -166,18 +165,11 @@ 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 = {
-        let last_shape = if rewrites.len() == 0 {
-            first_child_shape
-        } else {
-            other_child_shape
-        };
-        match context.config.indent_style() {
-            IndentStyle::Visual => last_shape.sub_width(shape.rhs_overhead(context.config))?,
-            IndentStyle::Block => last_shape,
-        }
-    };
-    let last_shape = last_shape.sub_width(suffix_try_num)?;
+    let last_shape = if rewrites.len() == 0 {
+        first_child_shape
+    } else {
+        other_child_shape
+    }.sub_width(shape.rhs_overhead(context.config) + 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:

src/comment.rs

@@ -224,9 +224,7 @@ pub fn rewrite_comment(
     // we should stop now.
     let num_bare_lines = orig.lines()
         .map(|line| line.trim())
-        .filter(|l| {
-            !(l.starts_with('*') || l.starts_with("//") || l.starts_with("/*"))
-        })
+        .filter(|l| !(l.starts_with('*') || l.starts_with("//") || l.starts_with("/*")))
         .count();
     if num_bare_lines > 0 && !config.normalize_comments() {
         return Some(orig.to_owned());

src/expr.rs

@@ -292,9 +292,7 @@ pub fn format_expr(
     };
 
     expr_rw
-        .and_then(|expr_str| {
-            recover_comment_removed(expr_str, expr.span, context)
-        })
+        .and_then(|expr_str| recover_comment_removed(expr_str, expr.span, context))
         .and_then(|expr_str| {
             let attrs = outer_attributes(&expr.attrs);
             let attrs_str = attrs.rewrite(context, shape)?;
@@ -1814,6 +1812,10 @@ where
     let used_width = extra_offset(callee_str, shape);
     let one_line_width = shape.width.checked_sub(used_width + 2 * paren_overhead)?;
 
+    // 1 = "(" or ")"
+    let one_line_shape = shape
+        .offset_left(last_line_width(callee_str) + 1)?
+        .sub_width(1)?;
     let nested_shape = shape_from_indent_style(
         context,
         shape,
@@ -1828,6 +1830,7 @@ where
         context,
         args,
         args_span,
+        one_line_shape,
         nested_shape,
         one_line_width,
         args_max_width,
@@ -1867,7 +1870,8 @@ fn rewrite_call_args<'a, T>(
     context: &RewriteContext,
     args: &[&T],
     span: Span,
-    shape: Shape,
+    one_line_shape: Shape,
+    nested_shape: Shape,
     one_line_width: usize,
     args_max_width: usize,
     force_trailing_comma: bool,
@@ -1882,7 +1886,7 @@ where
         ",",
         |item| item.span().lo(),
         |item| item.span().hi(),
-        |item| item.rewrite(context, shape),
+        |item| item.rewrite(context, nested_shape),
         span.lo(),
         span.hi(),
         true,
@@ -1896,7 +1900,8 @@ where
         context,
         &mut item_vec,
         &args[..],
-        shape,
+        one_line_shape,
+        nested_shape,
         one_line_width,
         args_max_width,
     );
@@ -1912,22 +1917,21 @@ where
             context.config.trailing_comma()
         },
         separator_place: SeparatorPlace::Back,
-        shape: shape,
+        shape: nested_shape,
         ends_with_newline: context.use_block_indent() && tactic == DefinitiveListTactic::Vertical,
         preserve_newline: false,
         config: context.config,
     };
 
-    write_list(&item_vec, &fmt).map(|args_str| {
-        (tactic != DefinitiveListTactic::Vertical, args_str)
-    })
+    write_list(&item_vec, &fmt).map(|args_str| (tactic != DefinitiveListTactic::Vertical, args_str))
 }
 
 fn try_overflow_last_arg<'a, T>(
     context: &RewriteContext,
     item_vec: &mut Vec<ListItem>,
     args: &[&T],
-    shape: Shape,
+    one_line_shape: Shape,
+    nested_shape: Shape,
     one_line_width: usize,
     args_max_width: usize,
 ) -> DefinitiveListTactic
@@ -1945,7 +1949,7 @@ where
                 context.force_one_line_chain = true;
             }
         }
-        last_arg_shape(&context, item_vec, shape, args_max_width).and_then(|arg_shape| {
+        last_arg_shape(args, item_vec, one_line_shape, args_max_width).and_then(|arg_shape| {
             rewrite_last_arg_with_overflow(&context, args, &mut item_vec[args.len() - 1], arg_shape)
         })
     } else {
@@ -1992,26 +1996,32 @@ where
     tactic
 }
 
-fn last_arg_shape(
-    context: &RewriteContext,
+/// Returns a shape for the last argument which is going to be overflowed.
+fn last_arg_shape<T>(
+    lists: &[&T],
     items: &[ListItem],
     shape: Shape,
     args_max_width: usize,
-) -> Option<Shape> {
-    let overhead = items.iter().rev().skip(1).fold(0, |acc, i| {
-        acc + i.item.as_ref().map_or(0, |s| first_line_width(s))
+) -> Option<Shape>
+where
+    T: Rewrite + Spanned + ToExpr,
+{
+    let is_nested_call = lists
+        .iter()
+        .next()
+        .and_then(|item| item.to_expr())
+        .map_or(false, is_nested_call);
+    if items.len() == 1 && !is_nested_call {
+        return Some(shape);
+    }
+    let offset = items.iter().rev().skip(1).fold(0, |acc, i| {
+        // 2 = ", "
+        acc + 2 + i.inner_as_ref().len()
     });
-    let max_width = min(args_max_width, shape.width);
-    let arg_indent = if context.use_block_indent() {
-        shape.block().indent.block_unindent(context.config)
-    } else {
-        shape.block().indent
-    };
-    Some(Shape {
-        width: max_width.checked_sub(overhead)?,
-        indent: arg_indent,
-        offset: 0,
-    })
+    Shape {
+        width: min(args_max_width, shape.width),
+        ..shape
+    }.offset_left(offset)
 }
 
 fn rewrite_last_arg_with_overflow<'a, T>(
@@ -2093,6 +2103,18 @@ pub fn can_be_overflowed_expr(context: &RewriteContext, expr: &ast::Expr, args_l
     }
 }
 
+fn is_nested_call(expr: &ast::Expr) -> bool {
+    match expr.node {
+        ast::ExprKind::Call(..) | ast::ExprKind::Mac(..) => true,
+        ast::ExprKind::AddrOf(_, ref expr)
+        | ast::ExprKind::Box(ref expr)
+        | ast::ExprKind::Try(ref expr)
+        | ast::ExprKind::Unary(_, ref expr)
+        | ast::ExprKind::Cast(ref expr, _) => is_nested_call(expr),
+        _ => false,
+    }
+}
+
 pub fn wrap_args_with_parens(
     context: &RewriteContext,
     args_str: &str,

src/items.rs

@@ -817,8 +817,7 @@ fn format_impl_ref_and_type(
             IndentStyle::Visual => new_line_offset + trait_ref_overhead,
             IndentStyle::Block => new_line_offset,
         };
-        result.push_str(&*self_ty
-            .rewrite(context, Shape::legacy(budget, type_offset))?);
+        result.push_str(&*self_ty.rewrite(context, Shape::legacy(budget, type_offset))?);
         Some(result)
     } else {
         unreachable!();
@@ -1578,9 +1577,7 @@ fn rewrite_static(
             lhs,
             &**expr,
             Shape::legacy(remaining_width, offset.block_only()),
-        ).and_then(|res| {
-            recover_comment_removed(res, static_parts.span, context)
-        })
+        ).and_then(|res| recover_comment_removed(res, static_parts.span, context))
             .map(|s| if s.ends_with(';') { s } else { s + ";" })
     } else {
         Some(format!("{}{};", prefix, ty_str))
@@ -2096,18 +2093,14 @@ 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.
     // FIXME: the comment for the self argument is dropped. This is blocked
     // on rust issue #27522.
     let min_args = explicit_self
-        .and_then(|explicit_self| {
-            rewrite_explicit_self(explicit_self, args, context)
-        })
+        .and_then(|explicit_self| rewrite_explicit_self(explicit_self, args, context))
         .map_or(1, |self_str| {
             arg_item_strs[0] = self_str;
             2
@@ -2326,9 +2319,8 @@ fn rewrite_generics(
 ) -> Option<String> {
     let g_shape = generics_shape_from_config(context.config, shape, 0)?;
     let one_line_width = shape.width.checked_sub(2).unwrap_or(0);
-    rewrite_generics_inner(context, generics, g_shape, one_line_width, span).or_else(|| {
-        rewrite_generics_inner(context, generics, g_shape, 0, span)
-    })
+    rewrite_generics_inner(context, generics, g_shape, one_line_width, span)
+        .or_else(|| rewrite_generics_inner(context, generics, g_shape, 0, span))
 }
 
 fn rewrite_generics_inner(

src/visitor.rs

@@ -109,9 +109,7 @@ impl<'a> FmtVisitor<'a> {
         if self.config.remove_blank_lines_at_start_or_end_of_block() {
             if let Some(first_stmt) = b.stmts.first() {
                 let attr_lo = inner_attrs
-                    .and_then(|attrs| {
-                        inner_attributes(attrs).first().map(|attr| attr.span.lo())
-                    })
+                    .and_then(|attrs| inner_attributes(attrs).first().map(|attr| attr.span.lo()))
                     .or_else(|| {
                         // Attributes for an item in a statement position
                         // do not belong to the statement. (rust-lang/rust#34459)
@@ -872,10 +870,7 @@ fn rewrite_first_group_attrs(
             for derive in derives {
                 derive_args.append(&mut get_derive_args(context, derive)?);
             }
-            return Some((
-                derives.len(),
-                format_derive(context, &derive_args, shape)?,
-            ));
+            return Some((derives.len(), format_derive(context, &derive_args, shape)?));
         }
     }
     // Rewrite the first attribute.

tests/source/chains.rs

@@ -185,3 +185,24 @@ fn issue2126() {
         }
     }
 }
+
+// #2200
+impl Foo {
+    pub fn from_ast(diagnostic: &::errors::Handler,
+                    attrs: &[ast::Attribute]) -> Attributes {
+        let other_attrs = attrs.iter().filter_map(|attr| {
+            attr.with_desugared_doc(|attr| {
+                if attr.check_name("doc") {
+                    if let Some(mi) = attr.meta() {
+                        if let Some(value) = mi.value_str() {
+                            doc_strings.push(DocFragment::Include(line,
+                                                                  attr.span,
+                                                                  filename,
+                                                                  contents));
+                        }
+                    }
+                }
+            })
+        }).collect();
+    }
+}

tests/target/chains-visual.rs

@@ -28,24 +28,24 @@ fn main() {
     });
 
     some_fuuuuuuuuunction().method_call_a(aaaaa, bbbbb, |c| {
-        let x = c;
-        x
-    })
+                               let x = c;
+                               x
+                           })
                            .method_call_b(aaaaa, bbbbb, |c| {
-        let x = c;
-        x
-    });
+                               let x = c;
+                               x
+                           });
 
     fffffffffffffffffffffffffffffffffff(a, {
         SCRIPT_TASK_ROOT.with(|root| {
                                   *root.borrow_mut() = Some(&script_task);
                               });
     });
 
-    let suuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuum = xxxxxxx.map(|x| x + 5)
-                                                                          .map(|x| x / 2)
-                                                                          .fold(0,
-                                                                                |acc, x| acc + x);
+    let suuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuum =
+        xxxxxxx.map(|x| x + 5)
+               .map(|x| x / 2)
+               .fold(0, |acc, x| acc + x);
 
     aaaaaaaaaaaaaaaa.map(|x| {
                              x += 1;

tests/target/chains.rs

@@ -213,3 +213,28 @@ fn issue2126() {
         }
     }
 }
+
+// #2200
+impl Foo {
+    pub fn from_ast(diagnostic: &::errors::Handler, attrs: &[ast::Attribute]) -> Attributes {
+        let other_attrs = attrs
+            .iter()
+            .filter_map(|attr| {
+                attr.with_desugared_doc(|attr| {
+                    if attr.check_name("doc") {
+                        if let Some(mi) = attr.meta() {
+                            if let Some(value) = mi.value_str() {
+                                doc_strings.push(DocFragment::Include(
+                                    line,
+                                    attr.span,
+                                    filename,
+                                    contents,
+                                ));
+                            }
+                        }
+                    }
+                })
+            })
+            .collect();
+    }
+}