From 728b0182c5ba5e6b98981c523868a23cbaf88719 Mon Sep 17 00:00:00 2001 From: Nick Cameron Date: Mon, 9 Jul 2018 09:13:33 +1200 Subject: [PATCH 01/19] Changes to chains with block indent * More often combine a chain item to the previous line (if it is a block) * Don't indent if the parent is a block This is not perfect and doesn't pass tests, but I need to refactor to make more progress --- src/chains.rs | 91 ++++++++++++++++++++++++++++++++++++--------------- 1 file changed, 64 insertions(+), 27 deletions(-) diff --git a/src/chains.rs b/src/chains.rs index 52999635cf12..44edaa9c34dd 100644 --- a/src/chains.rs +++ b/src/chains.rs @@ -78,7 +78,6 @@ use utils::{ use std::borrow::Cow; use std::cmp::min; -use std::iter; use syntax::codemap::Span; use syntax::{ast, ptr}; @@ -99,8 +98,8 @@ pub fn rewrite_chain(expr: &ast::Expr, context: &RewriteContext, shape: Shape) - // Parent is the first item in the chain, e.g., `foo` in `foo.bar.baz()`. let parent_shape = if is_block_expr(context, &parent, "\n") { match context.config.indent_style() { - IndentStyle::Visual => shape.visual_indent(0), IndentStyle::Block => shape, + IndentStyle::Visual => shape.visual_indent(0), } } else { shape @@ -110,18 +109,30 @@ pub fn rewrite_chain(expr: &ast::Expr, context: &RewriteContext, shape: Shape) - .map(|parent_rw| parent_rw + &"?".repeat(prefix_try_num))?; let parent_rewrite_contains_newline = parent_rewrite.contains('\n'); let is_small_parent = shape.offset + parent_rewrite.len() <= context.config.tab_spaces(); + let parent_is_block = is_block_expr(context, &parent, &parent_rewrite); // Decide how to layout the rest of the chain. `extend` is true if we can // put the first non-parent item on the same line as the parent. let (nested_shape, extend) = if !parent_rewrite_contains_newline && is_continuable(&parent) { - ( - chain_indent(context, shape.add_offset(parent_rewrite.len())), - context.config.indent_style() == IndentStyle::Visual || is_small_parent, - ) - } else if is_block_expr(context, &parent, &parent_rewrite) { + match context.config.indent_style() { + IndentStyle::Block => { + // TODO this is naive since if the first child is on the same line + // as the parent, then we should look at that, instead of the parent. + // Can we make the parent the first two things then? + let shape = if parent_is_block { + shape + } else { + chain_indent(context, shape.add_offset(parent_rewrite.len())) + }; + (shape, is_small_parent) + } + // TODO is this right? + IndentStyle::Visual => (chain_indent(context, shape.add_offset(parent_rewrite.len())), true), + } + } else if parent_is_block { match context.config.indent_style() { // Try to put the first child on the same line with parent's last line - IndentStyle::Block => (parent_shape.block_indent(context.config.tab_spaces()), true), + IndentStyle::Block => (parent_shape, true), // The parent is a block, so align the rest of the chain with the closing // brace. IndentStyle::Visual => (parent_shape, false), @@ -136,11 +147,21 @@ pub fn rewrite_chain(expr: &ast::Expr, context: &RewriteContext, shape: Shape) - let other_child_shape = nested_shape.with_max_width(context.config); let first_child_shape = if extend { - let overhead = last_line_width(&parent_rewrite); - let offset = trimmed_last_line_width(&parent_rewrite) + prefix_try_num; match context.config.indent_style() { - IndentStyle::Visual => parent_shape.offset_left(overhead)?, - IndentStyle::Block => parent_shape.offset_left(offset)?, + IndentStyle::Block => { + let offset = trimmed_last_line_width(&parent_rewrite) + prefix_try_num; + if parent_is_block { + parent_shape.offset_left(offset)? + } else { + parent_shape + .block_indent(context.config.tab_spaces()) + .offset_left(offset)? + } + } + IndentStyle::Visual => { + let overhead = last_line_width(&parent_rewrite); + parent_shape.offset_left(overhead)? + } } } else { other_child_shape @@ -150,16 +171,25 @@ pub fn rewrite_chain(expr: &ast::Expr, context: &RewriteContext, shape: Shape) - first_child_shape, other_child_shape ); - let child_shape_iter = Some(first_child_shape) - .into_iter() - .chain(iter::repeat(other_child_shape)); let subexpr_num = subexpr_list.len(); 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::>>()?; + + let mut rewrites: Vec = Vec::with_capacity(subexpr_list.len()); + let mut is_block_like = Vec::with_capacity(subexpr_list.len()); + is_block_like.push(true); + for (i, expr) in subexpr_list.iter().skip(1).rev().enumerate() { + // TODO should only use first_child_shape if expr is block like + let shape = if *is_block_like.last().unwrap() && !(extend && i == 0) { + // TODO change the name of the shapes + first_child_shape + } else { + other_child_shape + }; + let rewrite = rewrite_chain_subexpr(expr, total_span, context, shape)?; + is_block_like.push(is_block_expr(context, expr, &rewrite)); + rewrites.push(rewrite); + } // Total of all items excluding the last. let extend_last_subexpr = if is_small_parent { @@ -180,7 +210,7 @@ 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() { + let last_shape = if is_block_like[rewrites.len()] { first_child_shape } else { other_child_shape @@ -228,7 +258,7 @@ pub fn rewrite_chain(expr: &ast::Expr, context: &RewriteContext, shape: Shape) - 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. + // 1. The entire chain fits in a single line except the last child. // 2. `last_child_str.lines().count() >= 5`. let line_count = rw.lines().count(); let fits_single_line = almost_total + first_line_width(&rw) <= one_line_budget; @@ -255,6 +285,9 @@ pub fn rewrite_chain(expr: &ast::Expr, context: &RewriteContext, shape: Shape) - (rewrite_last(), false) }; rewrites.push(last_subexpr_str?); + // We should never look at this, since we only look at the block-ness of the + // previous item in the chain. + is_block_like.push(false); let connector = if fits_single_line && !parent_rewrite_contains_newline { // Yay, we can put everything on one line. @@ -293,14 +326,14 @@ pub fn rewrite_chain(expr: &ast::Expr, context: &RewriteContext, shape: Shape) - first_connector, rewrites[0], second_connector, - join_rewrites(&rewrites[1..], &connector) + join_rewrites(&rewrites[1..], &is_block_like[2..], &connector), ) } else { format!( "{}{}{}", parent_rewrite, first_connector, - join_rewrites(&rewrites, &connector) + join_rewrites(&rewrites, &is_block_like[1..], &connector), ) }; let result = format!("{}{}", result, "?".repeat(suffix_try_num)); @@ -332,12 +365,12 @@ fn rewrite_try( Some(format!("{}{}", sub_expr, "?".repeat(try_count))) } -fn join_rewrites(rewrites: &[String], connector: &str) -> String { +fn join_rewrites(rewrites: &[String], is_block_like: &[bool], connector: &str) -> String { let mut rewrite_iter = rewrites.iter(); let mut result = rewrite_iter.next().unwrap().clone(); - for rewrite in rewrite_iter { - if rewrite != "?" { + for (rewrite, prev_is_block_like) in rewrite_iter.zip(is_block_like.iter()) { + if rewrite != "?" && !prev_is_block_like { result.push_str(connector); } result.push_str(&rewrite); @@ -365,7 +398,11 @@ fn is_block_expr(context: &RewriteContext, expr: &ast::Expr, repr: &str) -> bool ast::ExprKind::Paren(ref expr) | ast::ExprKind::Binary(_, _, ref expr) | ast::ExprKind::Index(_, ref expr) - | ast::ExprKind::Unary(_, ref expr) => is_block_expr(context, expr, repr), + | ast::ExprKind::Unary(_, ref expr) + | ast::ExprKind::Closure(_, _, _, _, ref expr, _) => is_block_expr(context, expr, repr), + ast::ExprKind::MethodCall(_, ref exprs) => { + is_block_expr(context, exprs.last().unwrap(), repr) + } _ => false, } } From a8d5f2557216d2102a8cbb73e3b10f8d88de44a6 Mon Sep 17 00:00:00 2001 From: Nick Cameron Date: Mon, 9 Jul 2018 13:02:14 +1200 Subject: [PATCH 02/19] chains: split handling of chains in block and visual cases Just refactoring, lots of code dup here, but it should get better... --- src/chains.rs | 280 +++++++++++++++++++++++++++++++++++++++----------- 1 file changed, 221 insertions(+), 59 deletions(-) diff --git a/src/chains.rs b/src/chains.rs index 44edaa9c34dd..e0d31e45caaa 100644 --- a/src/chains.rs +++ b/src/chains.rs @@ -84,6 +84,13 @@ use syntax::{ast, ptr}; pub fn rewrite_chain(expr: &ast::Expr, context: &RewriteContext, shape: Shape) -> Option { debug!("rewrite_chain {:?}", shape); + match context.config.indent_style() { + IndentStyle::Block => rewrite_chain_block(expr, context, shape), + IndentStyle::Visual => rewrite_chain_visual(expr, context, shape), + } +} + +fn rewrite_chain_block(expr: &ast::Expr, context: &RewriteContext, shape: Shape) -> Option { let total_span = expr.span; let (parent, subexpr_list) = make_subexpr_list(expr, context); @@ -96,14 +103,7 @@ pub fn rewrite_chain(expr: &ast::Expr, context: &RewriteContext, shape: Shape) - let prefix_try_num = subexpr_list.iter().rev().take_while(|e| is_try(e)).count(); // Parent is the first item in the chain, e.g., `foo` in `foo.bar.baz()`. - let parent_shape = if is_block_expr(context, &parent, "\n") { - match context.config.indent_style() { - IndentStyle::Block => shape, - IndentStyle::Visual => shape.visual_indent(0), - } - } else { - shape - }; + let parent_shape = shape; let parent_rewrite = parent .rewrite(context, parent_shape) .map(|parent_rw| parent_rw + &"?".repeat(prefix_try_num))?; @@ -114,29 +114,14 @@ pub fn rewrite_chain(expr: &ast::Expr, context: &RewriteContext, shape: Shape) - // Decide how to layout the rest of the chain. `extend` is true if we can // put the first non-parent item on the same line as the parent. let (nested_shape, extend) = if !parent_rewrite_contains_newline && is_continuable(&parent) { - match context.config.indent_style() { - IndentStyle::Block => { - // TODO this is naive since if the first child is on the same line - // as the parent, then we should look at that, instead of the parent. - // Can we make the parent the first two things then? - let shape = if parent_is_block { - shape - } else { - chain_indent(context, shape.add_offset(parent_rewrite.len())) - }; - (shape, is_small_parent) - } - // TODO is this right? - IndentStyle::Visual => (chain_indent(context, shape.add_offset(parent_rewrite.len())), true), - } + let shape = if parent_is_block { + shape + } else { + chain_indent(context, shape.add_offset(parent_rewrite.len())) + }; + (shape, is_small_parent) } else if parent_is_block { - match context.config.indent_style() { - // Try to put the first child on the same line with parent's last line - IndentStyle::Block => (parent_shape, true), - // The parent is a block, so align the rest of the chain with the closing - // brace. - IndentStyle::Visual => (parent_shape, false), - } + (parent_shape, true) } else { ( chain_indent(context, shape.add_offset(parent_rewrite.len())), @@ -147,21 +132,13 @@ pub fn rewrite_chain(expr: &ast::Expr, context: &RewriteContext, shape: Shape) - let other_child_shape = nested_shape.with_max_width(context.config); let first_child_shape = if extend { - match context.config.indent_style() { - IndentStyle::Block => { - let offset = trimmed_last_line_width(&parent_rewrite) + prefix_try_num; - if parent_is_block { - parent_shape.offset_left(offset)? - } else { - parent_shape - .block_indent(context.config.tab_spaces()) - .offset_left(offset)? - } - } - IndentStyle::Visual => { - let overhead = last_line_width(&parent_rewrite); - parent_shape.offset_left(overhead)? - } + let offset = trimmed_last_line_width(&parent_rewrite) + prefix_try_num; + if parent_is_block { + parent_shape.offset_left(offset)? + } else { + parent_shape + .block_indent(context.config.tab_spaces()) + .offset_left(offset)? } } else { other_child_shape @@ -179,9 +156,7 @@ pub fn rewrite_chain(expr: &ast::Expr, context: &RewriteContext, shape: Shape) - let mut is_block_like = Vec::with_capacity(subexpr_list.len()); is_block_like.push(true); for (i, expr) in subexpr_list.iter().skip(1).rev().enumerate() { - // TODO should only use first_child_shape if expr is block like let shape = if *is_block_like.last().unwrap() && !(extend && i == 0) { - // TODO change the name of the shapes first_child_shape } else { other_child_shape @@ -303,7 +278,6 @@ pub fn rewrite_chain(expr: &ast::Expr, context: &RewriteContext, shape: Shape) - let first_connector = if is_small_parent || fits_single_line || last_line_extendable(&parent_rewrite) - || context.config.indent_style() == IndentStyle::Visual { "" } else { @@ -314,7 +288,6 @@ pub fn rewrite_chain(expr: &ast::Expr, context: &RewriteContext, shape: Shape) - let second_connector = if fits_single_line || rewrites[1] == "?" || last_line_extendable(&rewrites[0]) - || context.config.indent_style() == IndentStyle::Visual { "" } else { @@ -337,11 +310,203 @@ pub fn rewrite_chain(expr: &ast::Expr, context: &RewriteContext, shape: Shape) - ) }; let result = format!("{}{}", result, "?".repeat(suffix_try_num)); - if context.config.indent_style() == IndentStyle::Visual { - wrap_str(result, context.config.max_width(), shape) - } else { - Some(result) + Some(result) +} + +fn rewrite_chain_visual(expr: &ast::Expr, context: &RewriteContext, shape: Shape) -> Option { + let total_span = expr.span; + let (parent, subexpr_list) = make_subexpr_list(expr, context); + + // Bail out if the chain is just try sugar, i.e., an expression followed by + // any number of `?`s. + if chain_only_try(&subexpr_list) { + return rewrite_try(&parent, subexpr_list.len(), context, shape); } + let suffix_try_num = subexpr_list.iter().take_while(|e| is_try(e)).count(); + let prefix_try_num = subexpr_list.iter().rev().take_while(|e| is_try(e)).count(); + + // Parent is the first item in the chain, e.g., `foo` in `foo.bar.baz()`. + let parent_shape = if is_block_expr(context, &parent, "\n") { + shape.visual_indent(0) + } else { + shape + }; + let parent_rewrite = parent + .rewrite(context, parent_shape) + .map(|parent_rw| parent_rw + &"?".repeat(prefix_try_num))?; + let parent_rewrite_contains_newline = parent_rewrite.contains('\n'); + let is_small_parent = shape.offset + parent_rewrite.len() <= context.config.tab_spaces(); + let parent_is_block = is_block_expr(context, &parent, &parent_rewrite); + + // Decide how to layout the rest of the chain. `extend` is true if we can + // put the first non-parent item on the same line as the parent. + let (nested_shape, extend) = if !parent_rewrite_contains_newline && is_continuable(&parent) { + (shape.visual_indent(0), true) + } else if parent_is_block { + (parent_shape, false) + } else { + ( + shape.visual_indent(0), + false, + ) + }; + + let other_child_shape = nested_shape.with_max_width(context.config); + + let first_child_shape = if extend { + let overhead = last_line_width(&parent_rewrite); + parent_shape.offset_left(overhead)? + } else { + other_child_shape + }; + debug!( + "child_shapes {:?} {:?}", + first_child_shape, other_child_shape + ); + + let subexpr_num = subexpr_list.len(); + let last_subexpr = &subexpr_list[suffix_try_num]; + let subexpr_list = &subexpr_list[suffix_try_num..subexpr_num - prefix_try_num]; + + let mut rewrites: Vec = Vec::with_capacity(subexpr_list.len()); + let mut is_block_like = Vec::with_capacity(subexpr_list.len()); + is_block_like.push(true); + for (i, expr) in subexpr_list.iter().skip(1).rev().enumerate() { + let shape = if *is_block_like.last().unwrap() && !(extend && i == 0) { + first_child_shape + } else { + other_child_shape + }; + let rewrite = rewrite_chain_subexpr(expr, total_span, context, shape)?; + is_block_like.push(is_block_expr(context, expr, &rewrite)); + rewrites.push(rewrite); + } + + // Total of all items excluding the last. + let extend_last_subexpr = if is_small_parent { + rewrites.len() == 1 && last_line_extendable(&rewrites[0]) + } else { + rewrites.is_empty() && last_line_extendable(&parent_rewrite) + }; + let almost_total = if extend_last_subexpr { + 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() { + shape.width + } else { + min(shape.width, context.config.width_heuristics().chain_width) + }; + 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 is_block_like[rewrites.len()] { + 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: + // + // 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.child2.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_subexpr { + // 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 except the last child. + // 2. `last_child_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 { + (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 => { + (Some(rw), fits_single_line) + } + new_rw @ Some(..) => (new_rw, false), + _ => (Some(rw), fits_single_line), + } + } + } else { + (rewrite_last(), false) + } + })? + } else { + (rewrite_last(), false) + }; + rewrites.push(last_subexpr_str?); + // We should never look at this, since we only look at the block-ness of the + // previous item in the chain. + is_block_like.push(false); + + let connector = if fits_single_line && !parent_rewrite_contains_newline { + // Yay, we can put everything on one line. + Cow::from("") + } else { + // Use new lines. + if *context.force_one_line_chain.borrow() { + return None; + } + nested_shape.indent.to_string_with_newline(context.config) + }; + + let result = if is_small_parent && rewrites.len() > 1 { + format!( + "{}{}{}", + parent_rewrite, + rewrites[0], + join_rewrites(&rewrites[1..], &is_block_like[2..], &connector), + ) + } else { + format!( + "{}{}", + parent_rewrite, + join_rewrites(&rewrites, &is_block_like[1..], &connector), + ) + }; + let result = format!("{}{}", result, "?".repeat(suffix_try_num)); + wrap_str(result, context.config.max_width(), shape) } // True if the chain is only `?`s. @@ -421,12 +586,9 @@ fn make_subexpr_list(expr: &ast::Expr, context: &RewriteContext) -> (ast::Expr, } fn chain_indent(context: &RewriteContext, shape: Shape) -> Shape { - match context.config.indent_style() { - IndentStyle::Visual => shape.visual_indent(0), - IndentStyle::Block => shape - .block_indent(context.config.tab_spaces()) - .with_max_width(context.config), - } + IndentStyle::Block => shape + .block_indent(context.config.tab_spaces()) + .with_max_width(context.config), } // Returns the expression's subexpression, if it exists. When the subexpr From d244234607417e850e264afc9fbf7a4842ebad19 Mon Sep 17 00:00:00 2001 From: Nick Cameron Date: Mon, 9 Jul 2018 13:10:57 +1200 Subject: [PATCH 03/19] factor out treatment of 1-length chains And create `Chain` and `ChainItem` structs --- src/chains.rs | 106 +++++++++++++++++++++++++++----------------------- 1 file changed, 57 insertions(+), 49 deletions(-) diff --git a/src/chains.rs b/src/chains.rs index e0d31e45caaa..3a851899f08a 100644 --- a/src/chains.rs +++ b/src/chains.rs @@ -84,29 +84,56 @@ use syntax::{ast, ptr}; pub fn rewrite_chain(expr: &ast::Expr, context: &RewriteContext, shape: Shape) -> Option { debug!("rewrite_chain {:?}", shape); + let chain = Chain::from_ast(expr, context); + if chain.children.is_empty() { + return rewrite_try(&chain.parent.expr, chain.parent.tries, context, shape); + } match context.config.indent_style() { - IndentStyle::Block => rewrite_chain_block(expr, context, shape), - IndentStyle::Visual => rewrite_chain_visual(expr, context, shape), + IndentStyle::Block => rewrite_chain_block(chain, context, shape), + IndentStyle::Visual => rewrite_chain_visual(chain, context, shape), } } -fn rewrite_chain_block(expr: &ast::Expr, context: &RewriteContext, shape: Shape) -> Option { - let total_span = expr.span; - let (parent, subexpr_list) = make_subexpr_list(expr, context); +// An expression plus trailing `?`s to be formatted together. +struct ChainItem { + expr: ast::Expr, + tries: usize, +} - // Bail out if the chain is just try sugar, i.e., an expression followed by - // any number of `?`s. - if chain_only_try(&subexpr_list) { - return rewrite_try(&parent, subexpr_list.len(), context, shape); +struct Chain { + parent: ChainItem, + // TODO do we need to clone the exprs? + children: Vec, + span: Span, +} + +impl Chain { + fn from_ast(expr: &ast::Expr, context: &RewriteContext) -> Chain { + let (parent, mut subexpr_list) = make_subexpr_list(expr, context); + let tries = subexpr_list.iter().rev().take_while(|e| is_try(e)).count(); + let new_len = subexpr_list.len() - tries; + subexpr_list.truncate(new_len); + Chain { + parent: ChainItem { + expr: parent, + tries, + }, + children: subexpr_list, + span: expr.span, + } } +} + +fn rewrite_chain_block(chain: Chain, context: &RewriteContext, shape: Shape) -> Option { + let (parent, subexpr_list) = (&chain.parent.expr, &chain.children); + let suffix_try_num = subexpr_list.iter().take_while(|e| is_try(e)).count(); - let prefix_try_num = subexpr_list.iter().rev().take_while(|e| is_try(e)).count(); // Parent is the first item in the chain, e.g., `foo` in `foo.bar.baz()`. let parent_shape = shape; let parent_rewrite = parent .rewrite(context, parent_shape) - .map(|parent_rw| parent_rw + &"?".repeat(prefix_try_num))?; + .map(|parent_rw| parent_rw + &"?".repeat(chain.parent.tries))?; let parent_rewrite_contains_newline = parent_rewrite.contains('\n'); let is_small_parent = shape.offset + parent_rewrite.len() <= context.config.tab_spaces(); let parent_is_block = is_block_expr(context, &parent, &parent_rewrite); @@ -132,7 +159,7 @@ fn rewrite_chain_block(expr: &ast::Expr, context: &RewriteContext, shape: Shape) let other_child_shape = nested_shape.with_max_width(context.config); let first_child_shape = if extend { - let offset = trimmed_last_line_width(&parent_rewrite) + prefix_try_num; + let offset = trimmed_last_line_width(&parent_rewrite) + chain.parent.tries; if parent_is_block { parent_shape.offset_left(offset)? } else { @@ -148,9 +175,8 @@ fn rewrite_chain_block(expr: &ast::Expr, context: &RewriteContext, shape: Shape) first_child_shape, other_child_shape ); - let subexpr_num = subexpr_list.len(); let last_subexpr = &subexpr_list[suffix_try_num]; - let subexpr_list = &subexpr_list[suffix_try_num..subexpr_num - prefix_try_num]; + let subexpr_list = &subexpr_list[suffix_try_num..]; let mut rewrites: Vec = Vec::with_capacity(subexpr_list.len()); let mut is_block_like = Vec::with_capacity(subexpr_list.len()); @@ -161,7 +187,7 @@ fn rewrite_chain_block(expr: &ast::Expr, context: &RewriteContext, shape: Shape) } else { other_child_shape }; - let rewrite = rewrite_chain_subexpr(expr, total_span, context, shape)?; + let rewrite = rewrite_chain_subexpr(expr, chain.span, context, shape)?; is_block_like.push(is_block_expr(context, expr, &rewrite)); rewrites.push(rewrite); } @@ -226,12 +252,12 @@ fn rewrite_chain_block(expr: &ast::Expr, context: &RewriteContext, shape: Shape) // `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 rewrite_last = || rewrite_chain_subexpr(last_subexpr, chain.span, context, last_shape); let (last_subexpr_str, fits_single_line) = if all_in_one_line || extend_last_subexpr { // 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) { + if let Some(rw) = rewrite_chain_subexpr(last_subexpr, chain.span, context, shape) { // We allow overflowing here only if both of the following conditions match: // 1. The entire chain fits in a single line except the last child. // 2. `last_child_str.lines().count() >= 5`. @@ -313,17 +339,10 @@ fn rewrite_chain_block(expr: &ast::Expr, context: &RewriteContext, shape: Shape) Some(result) } -fn rewrite_chain_visual(expr: &ast::Expr, context: &RewriteContext, shape: Shape) -> Option { - let total_span = expr.span; - let (parent, subexpr_list) = make_subexpr_list(expr, context); +fn rewrite_chain_visual(chain: Chain, context: &RewriteContext, shape: Shape) -> Option { + let (parent, subexpr_list) = (&chain.parent.expr, &chain.children); - // Bail out if the chain is just try sugar, i.e., an expression followed by - // any number of `?`s. - if chain_only_try(&subexpr_list) { - return rewrite_try(&parent, subexpr_list.len(), context, shape); - } let suffix_try_num = subexpr_list.iter().take_while(|e| is_try(e)).count(); - let prefix_try_num = subexpr_list.iter().rev().take_while(|e| is_try(e)).count(); // Parent is the first item in the chain, e.g., `foo` in `foo.bar.baz()`. let parent_shape = if is_block_expr(context, &parent, "\n") { @@ -333,7 +352,7 @@ fn rewrite_chain_visual(expr: &ast::Expr, context: &RewriteContext, shape: Shape }; let parent_rewrite = parent .rewrite(context, parent_shape) - .map(|parent_rw| parent_rw + &"?".repeat(prefix_try_num))?; + .map(|parent_rw| parent_rw + &"?".repeat(chain.parent.tries))?; let parent_rewrite_contains_newline = parent_rewrite.contains('\n'); let is_small_parent = shape.offset + parent_rewrite.len() <= context.config.tab_spaces(); let parent_is_block = is_block_expr(context, &parent, &parent_rewrite); @@ -364,9 +383,8 @@ fn rewrite_chain_visual(expr: &ast::Expr, context: &RewriteContext, shape: Shape first_child_shape, other_child_shape ); - let subexpr_num = subexpr_list.len(); let last_subexpr = &subexpr_list[suffix_try_num]; - let subexpr_list = &subexpr_list[suffix_try_num..subexpr_num - prefix_try_num]; + let subexpr_list = &subexpr_list[suffix_try_num..]; let mut rewrites: Vec = Vec::with_capacity(subexpr_list.len()); let mut is_block_like = Vec::with_capacity(subexpr_list.len()); @@ -377,7 +395,7 @@ fn rewrite_chain_visual(expr: &ast::Expr, context: &RewriteContext, shape: Shape } else { other_child_shape }; - let rewrite = rewrite_chain_subexpr(expr, total_span, context, shape)?; + let rewrite = rewrite_chain_subexpr(expr, chain.span, context, shape)?; is_block_like.push(is_block_expr(context, expr, &rewrite)); rewrites.push(rewrite); } @@ -442,12 +460,12 @@ fn rewrite_chain_visual(expr: &ast::Expr, context: &RewriteContext, shape: Shape // `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 rewrite_last = || rewrite_chain_subexpr(last_subexpr, chain.span, context, last_shape); let (last_subexpr_str, fits_single_line) = if all_in_one_line || extend_last_subexpr { // 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) { + if let Some(rw) = rewrite_chain_subexpr(last_subexpr, chain.span, context, shape) { // We allow overflowing here only if both of the following conditions match: // 1. The entire chain fits in a single line except the last child. // 2. `last_child_str.lines().count() >= 5`. @@ -509,17 +527,6 @@ fn rewrite_chain_visual(expr: &ast::Expr, context: &RewriteContext, shape: Shape wrap_str(result, context.config.max_width(), shape) } -// True if the chain is only `?`s. -fn chain_only_try(exprs: &[ast::Expr]) -> bool { - exprs.iter().all(|e| { - if let ast::ExprKind::Try(_) = e.node { - true - } else { - false - } - }) -} - fn rewrite_try( expr: &ast::Expr, try_count: usize, @@ -566,12 +573,19 @@ fn is_block_expr(context: &RewriteContext, expr: &ast::Expr, repr: &str) -> bool | ast::ExprKind::Unary(_, ref expr) | ast::ExprKind::Closure(_, _, _, _, ref expr, _) => is_block_expr(context, expr, repr), ast::ExprKind::MethodCall(_, ref exprs) => { + // TODO maybe should be like Call is_block_expr(context, exprs.last().unwrap(), repr) } _ => false, } } +fn chain_indent(context: &RewriteContext, shape: Shape) -> Shape { + shape + .block_indent(context.config.tab_spaces()) + .with_max_width(context.config) +} + // Returns the root of the chain and a Vec of the prefixes of the rest of the chain. // E.g., for input `a.b.c` we return (`a`, [`a.b.c`, `a.b`]) fn make_subexpr_list(expr: &ast::Expr, context: &RewriteContext) -> (ast::Expr, Vec) { @@ -585,12 +599,6 @@ fn make_subexpr_list(expr: &ast::Expr, context: &RewriteContext) -> (ast::Expr, (parent, subexpr_list) } -fn chain_indent(context: &RewriteContext, shape: Shape) -> Shape { - IndentStyle::Block => shape - .block_indent(context.config.tab_spaces()) - .with_max_width(context.config), -} - // Returns the expression's subexpression, if it exists. When the subexpr // is a try! macro, we'll convert it to shorthand when the option is set. fn pop_expr_chain(expr: &ast::Expr, context: &RewriteContext) -> Option { From 9c8222474696d3d833cf850a1d50f1c485b63f09 Mon Sep 17 00:00:00 2001 From: Nick Cameron Date: Mon, 9 Jul 2018 14:18:58 +1200 Subject: [PATCH 04/19] chains: simplify visual formatting a bit --- src/chains.rs | 58 ++++++++++++++++++++++++--------------------------- 1 file changed, 27 insertions(+), 31 deletions(-) diff --git a/src/chains.rs b/src/chains.rs index 3a851899f08a..220cc4b8750e 100644 --- a/src/chains.rs +++ b/src/chains.rs @@ -340,9 +340,9 @@ fn rewrite_chain_block(chain: Chain, context: &RewriteContext, shape: Shape) -> } fn rewrite_chain_visual(chain: Chain, context: &RewriteContext, shape: Shape) -> Option { - let (parent, subexpr_list) = (&chain.parent.expr, &chain.children); + let parent = &chain.parent.expr; - let suffix_try_num = subexpr_list.iter().take_while(|e| is_try(e)).count(); + let suffix_try_num = chain.children.iter().take_while(|e| is_try(e)).count(); // Parent is the first item in the chain, e.g., `foo` in `foo.bar.baz()`. let parent_shape = if is_block_expr(context, &parent, "\n") { @@ -355,22 +355,13 @@ fn rewrite_chain_visual(chain: Chain, context: &RewriteContext, shape: Shape) -> .map(|parent_rw| parent_rw + &"?".repeat(chain.parent.tries))?; let parent_rewrite_contains_newline = parent_rewrite.contains('\n'); let is_small_parent = shape.offset + parent_rewrite.len() <= context.config.tab_spaces(); - let parent_is_block = is_block_expr(context, &parent, &parent_rewrite); + + let nested_shape = shape.visual_indent(0); + let other_child_shape = nested_shape.with_max_width(context.config); // Decide how to layout the rest of the chain. `extend` is true if we can // put the first non-parent item on the same line as the parent. - let (nested_shape, extend) = if !parent_rewrite_contains_newline && is_continuable(&parent) { - (shape.visual_indent(0), true) - } else if parent_is_block { - (parent_shape, false) - } else { - ( - shape.visual_indent(0), - false, - ) - }; - - let other_child_shape = nested_shape.with_max_width(context.config); + let extend = !parent_rewrite_contains_newline && is_continuable(&parent); let first_child_shape = if extend { let overhead = last_line_width(&parent_rewrite); @@ -383,20 +374,17 @@ fn rewrite_chain_visual(chain: Chain, context: &RewriteContext, shape: Shape) -> first_child_shape, other_child_shape ); - let last_subexpr = &subexpr_list[suffix_try_num]; - let subexpr_list = &subexpr_list[suffix_try_num..]; + let last_subexpr = &chain.children[suffix_try_num]; + let subexpr_list = &chain.children[suffix_try_num..]; let mut rewrites: Vec = Vec::with_capacity(subexpr_list.len()); - let mut is_block_like = Vec::with_capacity(subexpr_list.len()); - is_block_like.push(true); for (i, expr) in subexpr_list.iter().skip(1).rev().enumerate() { - let shape = if *is_block_like.last().unwrap() && !(extend && i == 0) { + let shape = if i == 0 { first_child_shape } else { other_child_shape }; let rewrite = rewrite_chain_subexpr(expr, chain.span, context, shape)?; - is_block_like.push(is_block_expr(context, expr, &rewrite)); rewrites.push(rewrite); } @@ -419,11 +407,8 @@ fn rewrite_chain_visual(chain: Chain, 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 is_block_like[rewrites.len()] { - first_child_shape - } else { - other_child_shape - }.sub_width(shape.rhs_overhead(context.config) + suffix_try_num)?; + let last_shape = + 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: @@ -494,9 +479,6 @@ fn rewrite_chain_visual(chain: Chain, context: &RewriteContext, shape: Shape) -> (rewrite_last(), false) }; rewrites.push(last_subexpr_str?); - // We should never look at this, since we only look at the block-ness of the - // previous item in the chain. - is_block_like.push(false); let connector = if fits_single_line && !parent_rewrite_contains_newline { // Yay, we can put everything on one line. @@ -514,13 +496,13 @@ fn rewrite_chain_visual(chain: Chain, context: &RewriteContext, shape: Shape) -> "{}{}{}", parent_rewrite, rewrites[0], - join_rewrites(&rewrites[1..], &is_block_like[2..], &connector), + join_rewrites_vis(&rewrites[1..], &connector), ) } else { format!( "{}{}", parent_rewrite, - join_rewrites(&rewrites, &is_block_like[1..], &connector), + join_rewrites_vis(&rewrites, &connector), ) }; let result = format!("{}{}", result, "?".repeat(suffix_try_num)); @@ -551,6 +533,20 @@ fn join_rewrites(rewrites: &[String], is_block_like: &[bool], connector: &str) - result } +fn join_rewrites_vis(rewrites: &[String], connector: &str) -> String { + let mut rewrite_iter = rewrites.iter(); + let mut result = rewrite_iter.next().unwrap().clone(); + + for rewrite in rewrite_iter { + if rewrite != "?" { + result.push_str(connector); + } + result.push_str(&rewrite); + } + + result +} + // States whether an expression's last line exclusively consists of closing // parens, braces, and brackets in its idiomatic formatting. fn is_block_expr(context: &RewriteContext, expr: &ast::Expr, repr: &str) -> bool { From 38e8b086e8b1b318be3164e89d16e5e19d16cc59 Mon Sep 17 00:00:00 2001 From: Nick Cameron Date: Mon, 9 Jul 2018 20:45:30 +1200 Subject: [PATCH 05/19] chains: refactoring pre-process the expression tree to get a list of chain items. --- src/chains.rs | 203 +++++++++++++++++++++----------------------------- 1 file changed, 84 insertions(+), 119 deletions(-) diff --git a/src/chains.rs b/src/chains.rs index 220cc4b8750e..99fd3d0779b8 100644 --- a/src/chains.rs +++ b/src/chains.rs @@ -85,9 +85,13 @@ use syntax::{ast, ptr}; pub fn rewrite_chain(expr: &ast::Expr, context: &RewriteContext, shape: Shape) -> Option { debug!("rewrite_chain {:?}", shape); let chain = Chain::from_ast(expr, context); + // If this is just an expression with some `?`s, then format it trivially and + // return early. if chain.children.is_empty() { - return rewrite_try(&chain.parent.expr, chain.parent.tries, context, shape); + let rewrite = chain.parent.expr.rewrite(context, shape.sub_width(chain.parent.tries)?)?; + return Some(format!("{}{}", rewrite, "?".repeat(chain.parent.tries))); } + match context.config.indent_style() { IndentStyle::Block => rewrite_chain_block(chain, context, shape), IndentStyle::Visual => rewrite_chain_visual(chain, context, shape), @@ -103,70 +107,69 @@ struct ChainItem { struct Chain { parent: ChainItem, // TODO do we need to clone the exprs? - children: Vec, - span: Span, + children: Vec, } impl Chain { fn from_ast(expr: &ast::Expr, context: &RewriteContext) -> Chain { - let (parent, mut subexpr_list) = make_subexpr_list(expr, context); - let tries = subexpr_list.iter().rev().take_while(|e| is_try(e)).count(); - let new_len = subexpr_list.len() - tries; - subexpr_list.truncate(new_len); + let mut subexpr_list = make_subexpr_list(expr, context); + + // Un-parse the expression tree into ChainItems + let mut children = vec![]; + let mut sub_tries = 0; + loop { + if subexpr_list.is_empty() { + break; + } + + let subexpr = subexpr_list.pop().unwrap(); + match subexpr.node { + ast::ExprKind::Try(_) => sub_tries += 1, + _ => { + children.push(ChainItem { + expr: subexpr.clone(), + tries: sub_tries, + }); + sub_tries = 0; + } + } + } + Chain { - parent: ChainItem { - expr: parent, - tries, - }, - children: subexpr_list, - span: expr.span, + parent: children.pop().unwrap(), + children: children.into_iter().rev().collect(), } } } -fn rewrite_chain_block(chain: Chain, context: &RewriteContext, shape: Shape) -> Option { - let (parent, subexpr_list) = (&chain.parent.expr, &chain.children); - - let suffix_try_num = subexpr_list.iter().take_while(|e| is_try(e)).count(); +fn rewrite_chain_block(mut chain: Chain, context: &RewriteContext, shape: Shape) -> Option { + let last = chain.children.pop().unwrap(); // Parent is the first item in the chain, e.g., `foo` in `foo.bar.baz()`. - let parent_shape = shape; - let parent_rewrite = parent - .rewrite(context, parent_shape) + let parent_rewrite = chain.parent.expr + .rewrite(context, shape) .map(|parent_rw| parent_rw + &"?".repeat(chain.parent.tries))?; let parent_rewrite_contains_newline = parent_rewrite.contains('\n'); let is_small_parent = shape.offset + parent_rewrite.len() <= context.config.tab_spaces(); - let parent_is_block = is_block_expr(context, &parent, &parent_rewrite); + let parent_is_block = is_block_expr(context, &chain.parent.expr, &parent_rewrite); // Decide how to layout the rest of the chain. `extend` is true if we can // put the first non-parent item on the same line as the parent. - let (nested_shape, extend) = if !parent_rewrite_contains_newline && is_continuable(&parent) { - let shape = if parent_is_block { - shape - } else { - chain_indent(context, shape.add_offset(parent_rewrite.len())) - }; - (shape, is_small_parent) - } else if parent_is_block { - (parent_shape, true) + let other_child_shape = if parent_is_block { + shape } else { - ( - chain_indent(context, shape.add_offset(parent_rewrite.len())), - false, - ) - }; + shape.block_indent(context.config.tab_spaces()) + }.with_max_width(context.config); - let other_child_shape = nested_shape.with_max_width(context.config); + let extend = if !parent_rewrite_contains_newline && is_continuable(&chain.parent.expr) { + is_small_parent + } else { + parent_is_block + }; let first_child_shape = if extend { let offset = trimmed_last_line_width(&parent_rewrite) + chain.parent.tries; - if parent_is_block { - parent_shape.offset_left(offset)? - } else { - parent_shape - .block_indent(context.config.tab_spaces()) - .offset_left(offset)? - } + other_child_shape.offset_left(offset)? } else { other_child_shape }; @@ -175,21 +178,18 @@ fn rewrite_chain_block(chain: Chain, context: &RewriteContext, shape: Shape) -> first_child_shape, other_child_shape ); - let last_subexpr = &subexpr_list[suffix_try_num]; - let subexpr_list = &subexpr_list[suffix_try_num..]; - - let mut rewrites: Vec = Vec::with_capacity(subexpr_list.len()); - let mut is_block_like = Vec::with_capacity(subexpr_list.len()); + let mut rewrites: Vec = Vec::with_capacity(chain.children.len()); + let mut is_block_like = Vec::with_capacity(chain.children.len()); is_block_like.push(true); - for (i, expr) in subexpr_list.iter().skip(1).rev().enumerate() { + for (i, item) in chain.children.iter().enumerate() { let shape = if *is_block_like.last().unwrap() && !(extend && i == 0) { first_child_shape } else { other_child_shape }; - let rewrite = rewrite_chain_subexpr(expr, chain.span, context, shape)?; - is_block_like.push(is_block_expr(context, expr, &rewrite)); - rewrites.push(rewrite); + let rewrite = rewrite_chain_subexpr(&item.expr, context, shape)?; + is_block_like.push(is_block_expr(context, &item.expr, &rewrite)); + rewrites.push(format!("{}{}", rewrite, "?".repeat(item.tries))); } // Total of all items excluding the last. @@ -202,7 +202,7 @@ fn rewrite_chain_block(chain: Chain, 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; + } + last.tries; let one_line_budget = if rewrites.is_empty() { shape.width } else { @@ -215,7 +215,7 @@ fn rewrite_chain_block(chain: Chain, context: &RewriteContext, shape: Shape) -> first_child_shape } else { other_child_shape - }.sub_width(shape.rhs_overhead(context.config) + suffix_try_num)?; + }.sub_width(shape.rhs_overhead(context.config) + last.tries)?; // 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: @@ -252,12 +252,11 @@ fn rewrite_chain_block(chain: Chain, context: &RewriteContext, shape: Shape) -> // `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, chain.span, context, last_shape); let (last_subexpr_str, fits_single_line) = if all_in_one_line || extend_last_subexpr { // 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, chain.span, context, shape) { + shape.offset_left(almost_total).map(|shape| { + if let Some(rw) = rewrite_chain_subexpr(&last.expr, context, shape) { // We allow overflowing here only if both of the following conditions match: // 1. The entire chain fits in a single line except the last child. // 2. `last_child_str.lines().count() >= 5`. @@ -269,7 +268,7 @@ fn rewrite_chain_block(chain: Chain, context: &RewriteContext, shape: Shape) -> // 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() { + match rewrite_chain_subexpr(&last.expr, context, last_shape) { Some(ref new_rw) if !fits_single_line => (Some(new_rw.clone()), false), Some(ref new_rw) if new_rw.lines().count() >= line_count => { (Some(rw), fits_single_line) @@ -279,11 +278,11 @@ fn rewrite_chain_block(chain: Chain, context: &RewriteContext, shape: Shape) -> } } } else { - (rewrite_last(), false) + (rewrite_chain_subexpr(&last.expr, context, last_shape), false) } })? } else { - (rewrite_last(), false) + (rewrite_chain_subexpr(&last.expr, context, last_shape), false) }; rewrites.push(last_subexpr_str?); // We should never look at this, since we only look at the block-ness of the @@ -298,7 +297,7 @@ fn rewrite_chain_block(chain: Chain, context: &RewriteContext, shape: Shape) -> if *context.force_one_line_chain.borrow() { return None; } - nested_shape.indent.to_string_with_newline(context.config) + other_child_shape.indent.to_string_with_newline(context.config) }; let first_connector = if is_small_parent @@ -335,33 +334,30 @@ fn rewrite_chain_block(chain: Chain, context: &RewriteContext, shape: Shape) -> join_rewrites(&rewrites, &is_block_like[1..], &connector), ) }; - let result = format!("{}{}", result, "?".repeat(suffix_try_num)); + let result = format!("{}{}", result, "?".repeat(last.tries)); Some(result) } -fn rewrite_chain_visual(chain: Chain, context: &RewriteContext, shape: Shape) -> Option { - let parent = &chain.parent.expr; - - let suffix_try_num = chain.children.iter().take_while(|e| is_try(e)).count(); +fn rewrite_chain_visual(mut chain: Chain, context: &RewriteContext, shape: Shape) -> Option { + let last = chain.children.pop().unwrap(); // Parent is the first item in the chain, e.g., `foo` in `foo.bar.baz()`. - let parent_shape = if is_block_expr(context, &parent, "\n") { + let parent_shape = if is_block_expr(context, &chain.parent.expr, "\n") { shape.visual_indent(0) } else { shape }; - let parent_rewrite = parent + let parent_rewrite = chain.parent.expr .rewrite(context, parent_shape) .map(|parent_rw| parent_rw + &"?".repeat(chain.parent.tries))?; let parent_rewrite_contains_newline = parent_rewrite.contains('\n'); let is_small_parent = shape.offset + parent_rewrite.len() <= context.config.tab_spaces(); - let nested_shape = shape.visual_indent(0); - let other_child_shape = nested_shape.with_max_width(context.config); + let other_child_shape = shape.visual_indent(0).with_max_width(context.config); // Decide how to layout the rest of the chain. `extend` is true if we can // put the first non-parent item on the same line as the parent. - let extend = !parent_rewrite_contains_newline && is_continuable(&parent); + let extend = !parent_rewrite_contains_newline && is_continuable(&chain.parent.expr); let first_child_shape = if extend { let overhead = last_line_width(&parent_rewrite); @@ -374,18 +370,15 @@ fn rewrite_chain_visual(chain: Chain, context: &RewriteContext, shape: Shape) -> first_child_shape, other_child_shape ); - let last_subexpr = &chain.children[suffix_try_num]; - let subexpr_list = &chain.children[suffix_try_num..]; - - let mut rewrites: Vec = Vec::with_capacity(subexpr_list.len()); - for (i, expr) in subexpr_list.iter().skip(1).rev().enumerate() { + let mut rewrites: Vec = Vec::with_capacity(chain.children.len()); + for (i, item) in chain.children.iter().enumerate() { let shape = if i == 0 { first_child_shape } else { other_child_shape }; - let rewrite = rewrite_chain_subexpr(expr, chain.span, context, shape)?; - rewrites.push(rewrite); + let rewrite = rewrite_chain_subexpr(&item.expr, context, shape)?; + rewrites.push(format!("{}{}", rewrite, "?".repeat(item.tries))); } // Total of all items excluding the last. @@ -398,7 +391,7 @@ fn rewrite_chain_visual(chain: Chain, 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; + } + last.tries; let one_line_budget = if rewrites.is_empty() { shape.width } else { @@ -408,7 +401,7 @@ fn rewrite_chain_visual(chain: Chain, context: &RewriteContext, shape: Shape) -> && rewrites.iter().all(|s| !s.contains('\n')) && almost_total < one_line_budget; let last_shape = - other_child_shape.sub_width(shape.rhs_overhead(context.config) + suffix_try_num)?; + other_child_shape.sub_width(shape.rhs_overhead(context.config) + last.tries)?; // 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: @@ -443,14 +436,11 @@ fn rewrite_chain_visual(chain: Chain, context: &RewriteContext, shape: Shape) -> // }) // ``` - // `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, chain.span, context, last_shape); let (last_subexpr_str, fits_single_line) = if all_in_one_line || extend_last_subexpr { // 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, chain.span, context, shape) { + if let Some(rw) = rewrite_chain_subexpr(&last.expr, context, shape) { // We allow overflowing here only if both of the following conditions match: // 1. The entire chain fits in a single line except the last child. // 2. `last_child_str.lines().count() >= 5`. @@ -462,7 +452,7 @@ fn rewrite_chain_visual(chain: Chain, context: &RewriteContext, shape: Shape) -> // 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() { + match rewrite_chain_subexpr(&last.expr, context, last_shape) { Some(ref new_rw) if !fits_single_line => (Some(new_rw.clone()), false), Some(ref new_rw) if new_rw.lines().count() >= line_count => { (Some(rw), fits_single_line) @@ -472,11 +462,11 @@ fn rewrite_chain_visual(chain: Chain, context: &RewriteContext, shape: Shape) -> } } } else { - (rewrite_last(), false) + (rewrite_chain_subexpr(&last.expr, context, last_shape), false) } })? } else { - (rewrite_last(), false) + (rewrite_chain_subexpr(&last.expr, context, last_shape), false) }; rewrites.push(last_subexpr_str?); @@ -488,7 +478,7 @@ fn rewrite_chain_visual(chain: Chain, context: &RewriteContext, shape: Shape) -> if *context.force_one_line_chain.borrow() { return None; } - nested_shape.indent.to_string_with_newline(context.config) + other_child_shape.indent.to_string_with_newline(context.config) }; let result = if is_small_parent && rewrites.len() > 1 { @@ -505,20 +495,10 @@ fn rewrite_chain_visual(chain: Chain, context: &RewriteContext, shape: Shape) -> join_rewrites_vis(&rewrites, &connector), ) }; - let result = format!("{}{}", result, "?".repeat(suffix_try_num)); + let result = format!("{}{}", result, "?".repeat(last.tries)); wrap_str(result, context.config.max_width(), shape) } -fn rewrite_try( - expr: &ast::Expr, - try_count: usize, - context: &RewriteContext, - shape: Shape, -) -> Option { - let sub_expr = expr.rewrite(context, shape.sub_width(try_count)?)?; - Some(format!("{}{}", sub_expr, "?".repeat(try_count))) -} - fn join_rewrites(rewrites: &[String], is_block_like: &[bool], connector: &str) -> String { let mut rewrite_iter = rewrites.iter(); let mut result = rewrite_iter.next().unwrap().clone(); @@ -576,23 +556,16 @@ fn is_block_expr(context: &RewriteContext, expr: &ast::Expr, repr: &str) -> bool } } -fn chain_indent(context: &RewriteContext, shape: Shape) -> Shape { - shape - .block_indent(context.config.tab_spaces()) - .with_max_width(context.config) -} - -// Returns the root of the chain and a Vec of the prefixes of the rest of the chain. -// E.g., for input `a.b.c` we return (`a`, [`a.b.c`, `a.b`]) -fn make_subexpr_list(expr: &ast::Expr, context: &RewriteContext) -> (ast::Expr, Vec) { +// Returns a Vec of the prefixes of the chain. +// E.g., for input `a.b.c` we return [`a.b.c`, `a.b`, 'a'] +fn make_subexpr_list(expr: &ast::Expr, context: &RewriteContext) -> Vec { let mut subexpr_list = vec![expr.clone()]; while let Some(subexpr) = pop_expr_chain(subexpr_list.last().unwrap(), context) { subexpr_list.push(subexpr.clone()); } - let parent = subexpr_list.pop().unwrap(); - (parent, subexpr_list) + subexpr_list } // Returns the expression's subexpression, if it exists. When the subexpr @@ -626,7 +599,6 @@ fn convert_try(expr: &ast::Expr, context: &RewriteContext) -> ast::Expr { // `.c`. fn rewrite_chain_subexpr( expr: &ast::Expr, - span: Span, context: &RewriteContext, shape: Shape, ) -> Option { @@ -647,7 +619,7 @@ fn rewrite_chain_subexpr( }, _ => &[], }; - rewrite_method_call(segment.ident, types, expressions, span, context, shape) + rewrite_method_call(segment.ident, types, expressions, expr.span, context, shape) } ast::ExprKind::Field(ref nested, ref field) => { let space = if is_tup_field_access(expr) && is_tup_field_access(nested) { @@ -679,13 +651,6 @@ fn is_continuable(expr: &ast::Expr) -> bool { } } -fn is_try(expr: &ast::Expr) -> bool { - match expr.node { - ast::ExprKind::Try(..) => true, - _ => false, - } -} - fn rewrite_method_call( method_name: ast::Ident, types: &[ast::GenericArg], From 914e750c9ead9a80e6514c08388cce11380cb949 Mon Sep 17 00:00:00 2001 From: Nick Cameron Date: Mon, 9 Jul 2018 21:09:25 +1200 Subject: [PATCH 06/19] chains: further simplification --- src/chains.rs | 80 +++++++++++++++++++++------------------------------ 1 file changed, 33 insertions(+), 47 deletions(-) diff --git a/src/chains.rs b/src/chains.rs index 99fd3d0779b8..a407fdda112d 100644 --- a/src/chains.rs +++ b/src/chains.rs @@ -161,11 +161,7 @@ fn rewrite_chain_block(mut chain: Chain, context: &RewriteContext, shape: Shape) shape.block_indent(context.config.tab_spaces()) }.with_max_width(context.config); - let extend = if !parent_rewrite_contains_newline && is_continuable(&chain.parent.expr) { - is_small_parent - } else { - parent_is_block - }; + let extend = parent_is_block || (is_small_parent && !parent_rewrite_contains_newline && is_continuable(&chain.parent.expr)); let first_child_shape = if extend { let offset = trimmed_last_line_width(&parent_rewrite) + chain.parent.tries; @@ -351,7 +347,6 @@ fn rewrite_chain_visual(mut chain: Chain, context: &RewriteContext, shape: Shape .rewrite(context, parent_shape) .map(|parent_rw| parent_rw + &"?".repeat(chain.parent.tries))?; let parent_rewrite_contains_newline = parent_rewrite.contains('\n'); - let is_small_parent = shape.offset + parent_rewrite.len() <= context.config.tab_spaces(); let other_child_shape = shape.visual_indent(0).with_max_width(context.config); @@ -382,16 +377,7 @@ fn rewrite_chain_visual(mut chain: Chain, context: &RewriteContext, shape: Shape } // Total of all items excluding the last. - let extend_last_subexpr = if is_small_parent { - rewrites.len() == 1 && last_line_extendable(&rewrites[0]) - } else { - rewrites.is_empty() && last_line_extendable(&parent_rewrite) - }; - let almost_total = if extend_last_subexpr { - last_line_width(&parent_rewrite) - } else { - rewrites.iter().fold(0, |a, b| a + b.len()) + parent_rewrite.len() - } + last.tries; + let almost_total = rewrites.iter().fold(0, |a, b| a + b.len()) + parent_rewrite.len() + last.tries; let one_line_budget = if rewrites.is_empty() { shape.width } else { @@ -436,38 +422,48 @@ fn rewrite_chain_visual(mut chain: Chain, context: &RewriteContext, shape: Shape // }) // ``` - let (last_subexpr_str, fits_single_line) = if all_in_one_line || extend_last_subexpr { + let mut last_subexpr_str = None; + let mut fits_single_line = false; + + if all_in_one_line { // 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(shape) = parent_shape.offset_left(almost_total) { if let Some(rw) = rewrite_chain_subexpr(&last.expr, context, shape) { // We allow overflowing here only if both of the following conditions match: // 1. The entire chain fits in a single line except the last child. // 2. `last_child_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 { - (Some(rw), true) + let could_fit_single_line = almost_total + first_line_width(&rw) <= one_line_budget; + if could_fit_single_line && line_count >= 5 { + last_subexpr_str = Some(rw); + fits_single_line = 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_chain_subexpr(&last.expr, context, last_shape) { - Some(ref new_rw) if !fits_single_line => (Some(new_rw.clone()), false), - Some(ref new_rw) if new_rw.lines().count() >= line_count => { - (Some(rw), fits_single_line) + Some(ref new_rw) if !could_fit_single_line => { + last_subexpr_str = Some(new_rw.clone()); + } + Some(ref new_rw) if new_rw.lines().count() >= line_count => { + last_subexpr_str = Some(rw); + fits_single_line = could_fit_single_line; + } + new_rw @ Some(..) => { + last_subexpr_str = new_rw; + } + _ => { + last_subexpr_str = Some(rw); + fits_single_line = could_fit_single_line; } - new_rw @ Some(..) => (new_rw, false), - _ => (Some(rw), fits_single_line), } } - } else { - (rewrite_chain_subexpr(&last.expr, context, last_shape), false) } - })? - } else { - (rewrite_chain_subexpr(&last.expr, context, last_shape), false) - }; + } + } + + last_subexpr_str = last_subexpr_str.or_else(|| rewrite_chain_subexpr(&last.expr, context, last_shape)); rewrites.push(last_subexpr_str?); let connector = if fits_single_line && !parent_rewrite_contains_newline { @@ -481,21 +477,11 @@ fn rewrite_chain_visual(mut chain: Chain, context: &RewriteContext, shape: Shape other_child_shape.indent.to_string_with_newline(context.config) }; - let result = if is_small_parent && rewrites.len() > 1 { - format!( - "{}{}{}", - parent_rewrite, - rewrites[0], - join_rewrites_vis(&rewrites[1..], &connector), - ) - } else { - format!( - "{}{}", - parent_rewrite, - join_rewrites_vis(&rewrites, &connector), - ) - }; - let result = format!("{}{}", result, "?".repeat(last.tries)); + let result = format!("{}{}{}", + parent_rewrite, + join_rewrites_vis(&rewrites, &connector), + "?".repeat(last.tries), + ); wrap_str(result, context.config.max_width(), shape) } From 92701552bcdcd29dd93f72bd5055f96caf2d25c6 Mon Sep 17 00:00:00 2001 From: Nick Cameron Date: Tue, 10 Jul 2018 12:24:45 +1200 Subject: [PATCH 07/19] chains: refactor block formatting --- src/chains.rs | 209 +++++++++++++++++++++++--------------------------- 1 file changed, 94 insertions(+), 115 deletions(-) diff --git a/src/chains.rs b/src/chains.rs index a407fdda112d..ff3633a8cffd 100644 --- a/src/chains.rs +++ b/src/chains.rs @@ -71,10 +71,7 @@ use macros::convert_try_mac; use rewrite::{Rewrite, RewriteContext}; use shape::Shape; use spanned::Spanned; -use utils::{ - first_line_width, last_line_extendable, last_line_width, mk_sp, trimmed_last_line_width, - wrap_str, -}; +use utils::{first_line_width, last_line_extendable, last_line_width, mk_sp, wrap_str}; use std::borrow::Cow; use std::cmp::min; @@ -99,11 +96,13 @@ pub fn rewrite_chain(expr: &ast::Expr, context: &RewriteContext, shape: Shape) - } // An expression plus trailing `?`s to be formatted together. +#[derive(Debug)] struct ChainItem { expr: ast::Expr, tries: usize, } +#[derive(Debug)] struct Chain { parent: ChainItem, // TODO do we need to clone the exprs? @@ -136,82 +135,89 @@ impl Chain { } Chain { - parent: children.pop().unwrap(), - children: children.into_iter().rev().collect(), + parent: children.remove(0), + children, } } } -fn rewrite_chain_block(mut chain: Chain, context: &RewriteContext, shape: Shape) -> Option { - let last = chain.children.pop().unwrap(); - +fn rewrite_chain_block(chain: Chain, context: &RewriteContext, shape: Shape) -> Option { + debug!("rewrite_chain_block {:?} {:?}", chain, shape); // Parent is the first item in the chain, e.g., `foo` in `foo.bar.baz()`. - let parent_rewrite = chain.parent.expr + // Root is the parent plus any other chain items placed on the first line to + // avoid an orphan. E.g., + // ``` + // foo.bar + // .baz() + // ``` + // If `bar` were not part of the root, then baz would be orphaned and 'float'. + let mut root_rewrite = chain.parent.expr .rewrite(context, shape) .map(|parent_rw| parent_rw + &"?".repeat(chain.parent.tries))?; - let parent_rewrite_contains_newline = parent_rewrite.contains('\n'); - let is_small_parent = shape.offset + parent_rewrite.len() <= context.config.tab_spaces(); - let parent_is_block = is_block_expr(context, &chain.parent.expr, &parent_rewrite); - // Decide how to layout the rest of the chain. `extend` is true if we can - // put the first non-parent item on the same line as the parent. - let other_child_shape = if parent_is_block { + let mut children: &[_] = &chain.children; + let mut root_ends_with_block = is_block_expr(context, &chain.parent.expr, &root_rewrite); + let tab_width = context.config.tab_spaces().saturating_sub(shape.offset); + + while root_rewrite.len() <= tab_width && !root_rewrite.contains('\n') { + let item = &children[0]; + let shape = shape.offset_left(root_rewrite.len())?; + match rewrite_chain_subexpr(&item.expr, context, shape) { + Some(rewrite) => { + root_rewrite.push_str(&rewrite); + root_rewrite.push_str(&"?".repeat(item.tries)); + } + None => break, + } + + root_ends_with_block = is_block_expr(context, &item.expr, &root_rewrite); + + children = &children[1..]; + if children.is_empty() { + return Some(root_rewrite); + } + } + + // Separate out the last item in the chain for special treatment below. + let last = &children[children.len() - 1]; + children = &children[..children.len() - 1]; + + // Decide how to layout the rest of the chain. + let child_shape = if root_ends_with_block { shape } else { shape.block_indent(context.config.tab_spaces()) }.with_max_width(context.config); - let extend = parent_is_block || (is_small_parent && !parent_rewrite_contains_newline && is_continuable(&chain.parent.expr)); - - let first_child_shape = if extend { - let offset = trimmed_last_line_width(&parent_rewrite) + chain.parent.tries; - other_child_shape.offset_left(offset)? - } else { - other_child_shape - }; - debug!( - "child_shapes {:?} {:?}", - first_child_shape, other_child_shape - ); - - let mut rewrites: Vec = Vec::with_capacity(chain.children.len()); - let mut is_block_like = Vec::with_capacity(chain.children.len()); - is_block_like.push(true); - for (i, item) in chain.children.iter().enumerate() { - let shape = if *is_block_like.last().unwrap() && !(extend && i == 0) { - first_child_shape - } else { - other_child_shape - }; - let rewrite = rewrite_chain_subexpr(&item.expr, context, shape)?; + let mut rewrites: Vec = Vec::with_capacity(children.len()); + rewrites.push(root_rewrite); + let mut is_block_like = Vec::with_capacity(children.len()); + is_block_like.push(root_ends_with_block); + for item in children { + let rewrite = rewrite_chain_subexpr(&item.expr, context, child_shape)?; is_block_like.push(is_block_expr(context, &item.expr, &rewrite)); rewrites.push(format!("{}{}", rewrite, "?".repeat(item.tries))); } // Total of all items excluding the last. - let extend_last_subexpr = if is_small_parent { - rewrites.len() == 1 && last_line_extendable(&rewrites[0]) - } else { - rewrites.is_empty() && last_line_extendable(&parent_rewrite) - }; + let extend_last_subexpr = last_line_extendable(&rewrites[rewrites.len() - 1]); let almost_total = if extend_last_subexpr { - last_line_width(&parent_rewrite) + last_line_width(&rewrites[rewrites.len() - 1]) } else { - rewrites.iter().fold(0, |a, b| a + b.len()) + parent_rewrite.len() + rewrites.iter().fold(0, |a, b| a + b.len()) } + last.tries; - let one_line_budget = if rewrites.is_empty() { + let one_line_budget = if rewrites.len() == 1 { shape.width } else { min(shape.width, context.config.width_heuristics().chain_width) }; - let all_in_one_line = !parent_rewrite_contains_newline - && rewrites.iter().all(|s| !s.contains('\n')) + let all_in_one_line = rewrites.iter().all(|s| !s.contains('\n')) && almost_total < one_line_budget; - let last_shape = if is_block_like[rewrites.len()] { - first_child_shape + let last_shape = if all_in_one_line { + shape.sub_width(last.tries)? } else { - other_child_shape - }.sub_width(shape.rhs_overhead(context.config) + last.tries)?; + child_shape.sub_width(shape.rhs_overhead(context.config) + last.tries)? + }; // 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: @@ -246,46 +252,54 @@ fn rewrite_chain_block(mut chain: Chain, context: &RewriteContext, shape: Shape) // }) // ``` - // `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 (last_subexpr_str, fits_single_line) = if all_in_one_line || extend_last_subexpr { + let mut last_subexpr_str = None; + let mut fits_single_line = false; + if all_in_one_line || extend_last_subexpr { // First we try to 'overflow' the last child and see if it looks better than using // vertical layout. - shape.offset_left(almost_total).map(|shape| { + if let Some(shape) = last_shape.offset_left(almost_total) { if let Some(rw) = rewrite_chain_subexpr(&last.expr, context, shape) { // We allow overflowing here only if both of the following conditions match: // 1. The entire chain fits in a single line except the last child. // 2. `last_child_str.lines().count() >= 5`. let line_count = rw.lines().count(); - let fits_single_line = almost_total + first_line_width(&rw) <= one_line_budget; + let could_fit_single_line = almost_total + first_line_width(&rw) <= one_line_budget; if fits_single_line && line_count >= 5 { - (Some(rw), true) + last_subexpr_str = Some(rw); + fits_single_line = 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_chain_subexpr(&last.expr, context, last_shape) { - Some(ref new_rw) if !fits_single_line => (Some(new_rw.clone()), false), - Some(ref new_rw) if new_rw.lines().count() >= line_count => { - (Some(rw), fits_single_line) + Some(ref new_rw) if !could_fit_single_line => { + last_subexpr_str = Some(new_rw.clone()); + } + Some(ref new_rw) if new_rw.lines().count() >= line_count => { + last_subexpr_str = Some(rw); + fits_single_line = could_fit_single_line; + } + new_rw @ Some(..) => { + last_subexpr_str = new_rw; + } + _ => { + last_subexpr_str = Some(rw); + fits_single_line = could_fit_single_line; } - new_rw @ Some(..) => (new_rw, false), - _ => (Some(rw), fits_single_line), } } - } else { - (rewrite_chain_subexpr(&last.expr, context, last_shape), false) } - })? - } else { - (rewrite_chain_subexpr(&last.expr, context, last_shape), false) - }; - rewrites.push(last_subexpr_str?); + } + } + + last_subexpr_str = last_subexpr_str.or_else(|| rewrite_chain_subexpr(&last.expr, context, last_shape)); + rewrites.push(format!("{}{}", last_subexpr_str?, "?".repeat(last.tries))); + // We should never look at this, since we only look at the block-ness of the // previous item in the chain. is_block_like.push(false); - let connector = if fits_single_line && !parent_rewrite_contains_newline { + let connector = if fits_single_line && all_in_one_line { // Yay, we can put everything on one line. Cow::from("") } else { @@ -293,44 +307,10 @@ fn rewrite_chain_block(mut chain: Chain, context: &RewriteContext, shape: Shape) if *context.force_one_line_chain.borrow() { return None; } - other_child_shape.indent.to_string_with_newline(context.config) + child_shape.indent.to_string_with_newline(context.config) }; - let first_connector = if is_small_parent - || fits_single_line - || last_line_extendable(&parent_rewrite) - { - "" - } else { - &connector - }; - - let result = if is_small_parent && rewrites.len() > 1 { - let second_connector = if fits_single_line - || rewrites[1] == "?" - || last_line_extendable(&rewrites[0]) - { - "" - } else { - &connector - }; - format!( - "{}{}{}{}{}", - parent_rewrite, - first_connector, - rewrites[0], - second_connector, - join_rewrites(&rewrites[1..], &is_block_like[2..], &connector), - ) - } else { - format!( - "{}{}{}", - parent_rewrite, - first_connector, - join_rewrites(&rewrites, &is_block_like[1..], &connector), - ) - }; - let result = format!("{}{}", result, "?".repeat(last.tries)); + let result = join_rewrites(&rewrites, &is_block_like, &connector); Some(result) } @@ -424,7 +404,6 @@ fn rewrite_chain_visual(mut chain: Chain, context: &RewriteContext, shape: Shape let mut last_subexpr_str = None; let mut fits_single_line = false; - if all_in_one_line { // First we try to 'overflow' the last child and see if it looks better than using // vertical layout. @@ -517,7 +496,9 @@ fn join_rewrites_vis(rewrites: &[String], connector: &str) -> String { // parens, braces, and brackets in its idiomatic formatting. fn is_block_expr(context: &RewriteContext, expr: &ast::Expr, repr: &str) -> bool { match expr.node { - ast::ExprKind::Mac(..) | ast::ExprKind::Call(..) => { + ast::ExprKind::Mac(..) + | ast::ExprKind::Call(..) + | ast::ExprKind::MethodCall(..) => { context.use_block_indent() && repr.contains('\n') } ast::ExprKind::Struct(..) @@ -533,11 +514,9 @@ fn is_block_expr(context: &RewriteContext, expr: &ast::Expr, repr: &str) -> bool | ast::ExprKind::Binary(_, _, ref expr) | ast::ExprKind::Index(_, ref expr) | ast::ExprKind::Unary(_, ref expr) - | ast::ExprKind::Closure(_, _, _, _, ref expr, _) => is_block_expr(context, expr, repr), - ast::ExprKind::MethodCall(_, ref exprs) => { - // TODO maybe should be like Call - is_block_expr(context, exprs.last().unwrap(), repr) - } + | ast::ExprKind::Closure(_, _, _, _, ref expr, _) + | ast::ExprKind::Try(ref expr) + | ast::ExprKind::Yield(Some(ref expr)) => is_block_expr(context, expr, repr), _ => false, } } From 86314bf09f22341703dfc539d4e0c55478face0b Mon Sep 17 00:00:00 2001 From: Nick Cameron Date: Tue, 10 Jul 2018 15:14:44 +1200 Subject: [PATCH 08/19] chains: more refactoring of visual formatting --- src/chains.rs | 74 +++++++++++++++++++++++---------------------------- 1 file changed, 33 insertions(+), 41 deletions(-) diff --git a/src/chains.rs b/src/chains.rs index ff3633a8cffd..9f06b240c7ce 100644 --- a/src/chains.rs +++ b/src/chains.rs @@ -189,9 +189,9 @@ fn rewrite_chain_block(chain: Chain, context: &RewriteContext, shape: Shape) -> shape.block_indent(context.config.tab_spaces()) }.with_max_width(context.config); - let mut rewrites: Vec = Vec::with_capacity(children.len()); + let mut rewrites: Vec = Vec::with_capacity(children.len() + 2); rewrites.push(root_rewrite); - let mut is_block_like = Vec::with_capacity(children.len()); + let mut is_block_like = Vec::with_capacity(children.len() + 2); is_block_like.push(root_ends_with_block); for item in children { let rewrite = rewrite_chain_subexpr(&item.expr, context, child_shape)?; @@ -314,60 +314,53 @@ fn rewrite_chain_block(chain: Chain, context: &RewriteContext, shape: Shape) -> Some(result) } -fn rewrite_chain_visual(mut chain: Chain, context: &RewriteContext, shape: Shape) -> Option { - let last = chain.children.pop().unwrap(); - +fn rewrite_chain_visual(chain: Chain, context: &RewriteContext, shape: Shape) -> Option { // Parent is the first item in the chain, e.g., `foo` in `foo.bar.baz()`. let parent_shape = if is_block_expr(context, &chain.parent.expr, "\n") { shape.visual_indent(0) } else { shape }; - let parent_rewrite = chain.parent.expr + let mut children: &[_] = &chain.children; + let mut root_rewrite = chain.parent.expr .rewrite(context, parent_shape) .map(|parent_rw| parent_rw + &"?".repeat(chain.parent.tries))?; - let parent_rewrite_contains_newline = parent_rewrite.contains('\n'); - let other_child_shape = shape.visual_indent(0).with_max_width(context.config); - - // Decide how to layout the rest of the chain. `extend` is true if we can - // put the first non-parent item on the same line as the parent. - let extend = !parent_rewrite_contains_newline && is_continuable(&chain.parent.expr); - - let first_child_shape = if extend { - let overhead = last_line_width(&parent_rewrite); - parent_shape.offset_left(overhead)? - } else { - other_child_shape - }; - debug!( - "child_shapes {:?} {:?}", - first_child_shape, other_child_shape - ); - - let mut rewrites: Vec = Vec::with_capacity(chain.children.len()); - for (i, item) in chain.children.iter().enumerate() { - let shape = if i == 0 { - first_child_shape - } else { - other_child_shape - }; + if !root_rewrite.contains('\n') && is_continuable(&chain.parent.expr) { + let item = &children[0]; + let overhead = last_line_width(&root_rewrite); + let shape = parent_shape.offset_left(overhead)?; let rewrite = rewrite_chain_subexpr(&item.expr, context, shape)?; + root_rewrite.push_str(&rewrite); + root_rewrite.push_str(&"?".repeat(item.tries)); + children = &children[1..]; + if children.is_empty() { + return Some(root_rewrite); + } + } + + let last = &children[children.len() - 1]; + children = &children[..children.len() - 1]; + + let child_shape = shape.visual_indent(0).with_max_width(context.config); + + let mut rewrites: Vec = Vec::with_capacity(children.len() + 2); + rewrites.push(root_rewrite); + for item in chain.children.iter() { + let rewrite = rewrite_chain_subexpr(&item.expr, context, child_shape)?; rewrites.push(format!("{}{}", rewrite, "?".repeat(item.tries))); } // Total of all items excluding the last. - let almost_total = rewrites.iter().fold(0, |a, b| a + b.len()) + parent_rewrite.len() + last.tries; - let one_line_budget = if rewrites.is_empty() { + let almost_total = rewrites.iter().fold(0, |a, b| a + b.len()) + last.tries; + let one_line_budget = if rewrites.len() == 1 { shape.width } else { min(shape.width, context.config.width_heuristics().chain_width) }; - let all_in_one_line = !parent_rewrite_contains_newline - && rewrites.iter().all(|s| !s.contains('\n')) + let all_in_one_line = rewrites.iter().all(|s| !s.contains('\n')) && almost_total < one_line_budget; - let last_shape = - other_child_shape.sub_width(shape.rhs_overhead(context.config) + last.tries)?; + let last_shape = child_shape.sub_width(shape.rhs_overhead(context.config) + last.tries)?; // 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: @@ -445,7 +438,7 @@ fn rewrite_chain_visual(mut chain: Chain, context: &RewriteContext, shape: Shape last_subexpr_str = last_subexpr_str.or_else(|| rewrite_chain_subexpr(&last.expr, context, last_shape)); rewrites.push(last_subexpr_str?); - let connector = if fits_single_line && !parent_rewrite_contains_newline { + let connector = if fits_single_line && all_in_one_line { // Yay, we can put everything on one line. Cow::from("") } else { @@ -453,11 +446,10 @@ fn rewrite_chain_visual(mut chain: Chain, context: &RewriteContext, shape: Shape if *context.force_one_line_chain.borrow() { return None; } - other_child_shape.indent.to_string_with_newline(context.config) + child_shape.indent.to_string_with_newline(context.config) }; - let result = format!("{}{}{}", - parent_rewrite, + let result = format!("{}{}", join_rewrites_vis(&rewrites, &connector), "?".repeat(last.tries), ); From a56ff9d02f67a2736a6735014564fe5e496ad148 Mon Sep 17 00:00:00 2001 From: Nick Cameron Date: Wed, 11 Jul 2018 12:01:39 +1200 Subject: [PATCH 09/19] chains: factor into objects --- src/chains.rs | 638 ++++++++++++++++++++++++++------------------------ 1 file changed, 334 insertions(+), 304 deletions(-) diff --git a/src/chains.rs b/src/chains.rs index 9f06b240c7ce..692d1c946fc5 100644 --- a/src/chains.rs +++ b/src/chains.rs @@ -111,7 +111,7 @@ struct Chain { impl Chain { fn from_ast(expr: &ast::Expr, context: &RewriteContext) -> Chain { - let mut subexpr_list = make_subexpr_list(expr, context); + let mut subexpr_list = Self::make_subexpr_list(expr, context); // Un-parse the expression tree into ChainItems let mut children = vec![]; @@ -139,10 +139,67 @@ impl Chain { children, } } + + // Returns a Vec of the prefixes of the chain. + // E.g., for input `a.b.c` we return [`a.b.c`, `a.b`, 'a'] + fn make_subexpr_list(expr: &ast::Expr, context: &RewriteContext) -> Vec { + let mut subexpr_list = vec![expr.clone()]; + + while let Some(subexpr) = Self::pop_expr_chain(subexpr_list.last().unwrap(), context) { + subexpr_list.push(subexpr.clone()); + } + + subexpr_list + } + + // Returns the expression's subexpression, if it exists. When the subexpr + // is a try! macro, we'll convert it to shorthand when the option is set. + fn pop_expr_chain(expr: &ast::Expr, context: &RewriteContext) -> Option { + match expr.node { + ast::ExprKind::MethodCall(_, ref expressions) => { + Some(Self::convert_try(&expressions[0], context)) + } + ast::ExprKind::Field(ref subexpr, _) | ast::ExprKind::Try(ref subexpr) => { + Some(Self::convert_try(subexpr, context)) + } + _ => None, + } + } + + fn convert_try(expr: &ast::Expr, context: &RewriteContext) -> ast::Expr { + match expr.node { + ast::ExprKind::Mac(ref mac) if context.config.use_try_shorthand() => { + if let Some(subexpr) = convert_try_mac(mac, context) { + subexpr + } else { + expr.clone() + } + } + _ => expr.clone(), + } + } } -fn rewrite_chain_block(chain: Chain, context: &RewriteContext, shape: Shape) -> Option { - debug!("rewrite_chain_block {:?} {:?}", chain, shape); +// TODO comments +struct ChainFormatterBlock<'a> { + children: &'a[ChainItem], + rewrites: Vec, + root_ends_with_block: bool, + is_block_like: Vec, + fits_single_line: bool, +} + +impl <'a> ChainFormatterBlock<'a> { + fn new(chain: &'a Chain) -> ChainFormatterBlock<'a> { + ChainFormatterBlock { + children: &chain.children, + root_ends_with_block: false, + rewrites: Vec::with_capacity(chain.children.len() + 1), + is_block_like: Vec::with_capacity(chain.children.len() + 1), + fits_single_line: false, + } + } + // Parent is the first item in the chain, e.g., `foo` in `foo.bar.baz()`. // Root is the parent plus any other chain items placed on the first line to // avoid an orphan. E.g., @@ -151,73 +208,53 @@ fn rewrite_chain_block(chain: Chain, context: &RewriteContext, shape: Shape) -> // .baz() // ``` // If `bar` were not part of the root, then baz would be orphaned and 'float'. - let mut root_rewrite = chain.parent.expr - .rewrite(context, shape) - .map(|parent_rw| parent_rw + &"?".repeat(chain.parent.tries))?; + fn format_root(&mut self, parent: &ChainItem, context: &RewriteContext, shape: Shape) -> Option<()> { + let mut root_rewrite: String = parent.expr + .rewrite(context, shape) + .map(|parent_rw| parent_rw + &"?".repeat(parent.tries))?; - let mut children: &[_] = &chain.children; - let mut root_ends_with_block = is_block_expr(context, &chain.parent.expr, &root_rewrite); - let tab_width = context.config.tab_spaces().saturating_sub(shape.offset); + self.root_ends_with_block = is_block_expr(context, &parent.expr, &root_rewrite); + let tab_width = context.config.tab_spaces().saturating_sub(shape.offset); - while root_rewrite.len() <= tab_width && !root_rewrite.contains('\n') { - let item = &children[0]; - let shape = shape.offset_left(root_rewrite.len())?; - match rewrite_chain_subexpr(&item.expr, context, shape) { - Some(rewrite) => { - root_rewrite.push_str(&rewrite); - root_rewrite.push_str(&"?".repeat(item.tries)); + while root_rewrite.len() <= tab_width && !root_rewrite.contains('\n') { + let item = &self.children[0]; + let shape = shape.offset_left(root_rewrite.len())?; + match rewrite_chain_subexpr(&item.expr, context, shape) { + Some(rewrite) => { + root_rewrite.push_str(&rewrite); + root_rewrite.push_str(&"?".repeat(item.tries)); + } + None => break, } - None => break, - } - root_ends_with_block = is_block_expr(context, &item.expr, &root_rewrite); + self.root_ends_with_block = is_block_expr(context, &item.expr, &root_rewrite); - children = &children[1..]; - if children.is_empty() { - return Some(root_rewrite); + self.children = &self.children[1..]; + if self.children.is_empty() { + break; + } } + self.rewrites.push(root_rewrite); + Some(()) } - // Separate out the last item in the chain for special treatment below. - let last = &children[children.len() - 1]; - children = &children[..children.len() - 1]; - - // Decide how to layout the rest of the chain. - let child_shape = if root_ends_with_block { - shape - } else { - shape.block_indent(context.config.tab_spaces()) - }.with_max_width(context.config); - - let mut rewrites: Vec = Vec::with_capacity(children.len() + 2); - rewrites.push(root_rewrite); - let mut is_block_like = Vec::with_capacity(children.len() + 2); - is_block_like.push(root_ends_with_block); - for item in children { - let rewrite = rewrite_chain_subexpr(&item.expr, context, child_shape)?; - is_block_like.push(is_block_expr(context, &item.expr, &rewrite)); - rewrites.push(format!("{}{}", rewrite, "?".repeat(item.tries))); + fn child_shape(&self, context: &RewriteContext, shape: Shape) -> Shape { + if self.root_ends_with_block { + shape + } else { + shape.block_indent(context.config.tab_spaces()) + }.with_max_width(context.config) } - // Total of all items excluding the last. - let extend_last_subexpr = last_line_extendable(&rewrites[rewrites.len() - 1]); - let almost_total = if extend_last_subexpr { - last_line_width(&rewrites[rewrites.len() - 1]) - } else { - rewrites.iter().fold(0, |a, b| a + b.len()) - } + last.tries; - let one_line_budget = if rewrites.len() == 1 { - shape.width - } else { - min(shape.width, context.config.width_heuristics().chain_width) - }; - let all_in_one_line = rewrites.iter().all(|s| !s.contains('\n')) - && almost_total < one_line_budget; - let last_shape = if all_in_one_line { - shape.sub_width(last.tries)? - } else { - child_shape.sub_width(shape.rhs_overhead(context.config) + last.tries)? - }; + fn format_children(&mut self, context: &RewriteContext, child_shape: Shape) -> Option<()> { + self.is_block_like.push(self.root_ends_with_block); + for item in &self.children[..self.children.len()] { + let rewrite = rewrite_chain_subexpr(&item.expr, context, child_shape)?; + self.is_block_like.push(is_block_expr(context, &item.expr, &rewrite)); + self.rewrites.push(format!("{}{}", rewrite, "?".repeat(item.tries))); + } + Some(()) + } // 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: @@ -251,239 +288,279 @@ fn rewrite_chain_block(chain: Chain, context: &RewriteContext, shape: Shape) -> // result // }) // ``` + fn format_last_child(&mut self, context: &RewriteContext, shape: Shape, child_shape: Shape) -> Option<()> { + let last = &self.children[self.children.len() - 1]; + let extendable = last_line_extendable(&self.rewrites[self.rewrites.len() - 1]); + // Total of all items excluding the last. + let almost_total = if extendable { + last_line_width(&self.rewrites[self.rewrites.len() - 1]) + } else { + self.rewrites.iter().fold(0, |a, b| a + b.len()) + } + last.tries; + let one_line_budget = if self.rewrites.len() == 1 { + shape.width + } else { + min(shape.width, context.config.width_heuristics().chain_width) + }.saturating_sub(almost_total); - let mut last_subexpr_str = None; - let mut fits_single_line = false; - if all_in_one_line || extend_last_subexpr { - // First we try to 'overflow' the last child and see if it looks better than using - // vertical layout. - if let Some(shape) = last_shape.offset_left(almost_total) { - if let Some(rw) = rewrite_chain_subexpr(&last.expr, context, shape) { - // We allow overflowing here only if both of the following conditions match: - // 1. The entire chain fits in a single line except the last child. - // 2. `last_child_str.lines().count() >= 5`. - let line_count = rw.lines().count(); - let could_fit_single_line = almost_total + first_line_width(&rw) <= one_line_budget; - if fits_single_line && line_count >= 5 { - last_subexpr_str = Some(rw); - fits_single_line = 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_chain_subexpr(&last.expr, context, last_shape) { - Some(ref new_rw) if !could_fit_single_line => { - last_subexpr_str = Some(new_rw.clone()); - } - Some(ref new_rw) if new_rw.lines().count() >= line_count => { - last_subexpr_str = Some(rw); - fits_single_line = could_fit_single_line; - } - new_rw @ Some(..) => { - last_subexpr_str = new_rw; - } - _ => { - last_subexpr_str = Some(rw); - fits_single_line = could_fit_single_line; + let all_in_one_line = self.rewrites.iter().all(|s| !s.contains('\n')) && one_line_budget > 0; + let last_shape = if all_in_one_line { + shape.sub_width(last.tries)? + } else { + child_shape.sub_width(shape.rhs_overhead(context.config) + last.tries)? + }; + + let mut last_subexpr_str = None; + if all_in_one_line || extendable { + // First we try to 'overflow' the last child and see if it looks better than using + // vertical layout. + if let Some(shape) = last_shape.offset_left(almost_total) { + if let Some(rw) = rewrite_chain_subexpr(&last.expr, context, shape) { + // We allow overflowing here only if both of the following conditions match: + // 1. The entire chain fits in a single line except the last child. + // 2. `last_child_str.lines().count() >= 5`. + let line_count = rw.lines().count(); + let could_fit_single_line = first_line_width(&rw) <= one_line_budget; + if could_fit_single_line && line_count >= 5 { + last_subexpr_str = Some(rw); + self.fits_single_line = all_in_one_line; + } 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_chain_subexpr(&last.expr, context, last_shape) { + Some(ref new_rw) if !could_fit_single_line => { + last_subexpr_str = Some(new_rw.clone()); + } + Some(ref new_rw) if new_rw.lines().count() >= line_count => { + last_subexpr_str = Some(rw); + self.fits_single_line = could_fit_single_line && all_in_one_line; + } + new_rw @ Some(..) => { + last_subexpr_str = new_rw; + } + _ => { + last_subexpr_str = Some(rw); + self.fits_single_line = could_fit_single_line && all_in_one_line; + } } } } } } + + last_subexpr_str = last_subexpr_str.or_else(|| rewrite_chain_subexpr(&last.expr, context, last_shape)); + self.rewrites.push(format!("{}{}", last_subexpr_str?, "?".repeat(last.tries))); + Some(()) } - last_subexpr_str = last_subexpr_str.or_else(|| rewrite_chain_subexpr(&last.expr, context, last_shape)); - rewrites.push(format!("{}{}", last_subexpr_str?, "?".repeat(last.tries))); - // We should never look at this, since we only look at the block-ness of the - // previous item in the chain. - is_block_like.push(false); + fn join_rewrites(&self, context: &RewriteContext, child_shape: Shape) -> Option { + let connector = if self.fits_single_line { + // Yay, we can put everything on one line. + Cow::from("") + } else { + // Use new lines. + if *context.force_one_line_chain.borrow() { + return None; + } + child_shape.indent.to_string_with_newline(context.config) + }; - let connector = if fits_single_line && all_in_one_line { - // Yay, we can put everything on one line. - Cow::from("") - } else { - // Use new lines. - if *context.force_one_line_chain.borrow() { - return None; + let mut rewrite_iter = self.rewrites.iter(); + let mut result = rewrite_iter.next().unwrap().clone(); + + for (rewrite, prev_is_block_like) in rewrite_iter.zip(self.is_block_like.iter()) { + if rewrite != "?" && !prev_is_block_like { + result.push_str(&connector); + } + result.push_str(&rewrite); } - child_shape.indent.to_string_with_newline(context.config) - }; - let result = join_rewrites(&rewrites, &is_block_like, &connector); + Some(result) + } +} + +fn rewrite_chain_block(chain: Chain, context: &RewriteContext, shape: Shape) -> Option { + debug!("rewrite_chain_block {:?} {:?}", chain, shape); + + let mut formatter = ChainFormatterBlock::new(&chain); + + formatter.format_root(&chain.parent, context, shape)?; + if formatter.children.is_empty() { + assert_eq!(formatter.rewrites.len(), 1); + return Some(formatter.rewrites.pop().unwrap()); + } + + // Decide how to layout the rest of the chain. + let child_shape = formatter.child_shape(context, shape); + formatter.format_children(context, child_shape)?; + + formatter.format_last_child(context, shape, child_shape)?; + + let result = formatter.join_rewrites(context, child_shape)?; Some(result) } -fn rewrite_chain_visual(chain: Chain, context: &RewriteContext, shape: Shape) -> Option { - // Parent is the first item in the chain, e.g., `foo` in `foo.bar.baz()`. - let parent_shape = if is_block_expr(context, &chain.parent.expr, "\n") { - shape.visual_indent(0) - } else { - shape - }; - let mut children: &[_] = &chain.children; - let mut root_rewrite = chain.parent.expr - .rewrite(context, parent_shape) - .map(|parent_rw| parent_rw + &"?".repeat(chain.parent.tries))?; +struct ChainFormatterVisual<'a> { + children: &'a[ChainItem], + rewrites: Vec, + fits_single_line: bool, +} - if !root_rewrite.contains('\n') && is_continuable(&chain.parent.expr) { - let item = &children[0]; - let overhead = last_line_width(&root_rewrite); - let shape = parent_shape.offset_left(overhead)?; - let rewrite = rewrite_chain_subexpr(&item.expr, context, shape)?; - root_rewrite.push_str(&rewrite); - root_rewrite.push_str(&"?".repeat(item.tries)); - children = &children[1..]; - if children.is_empty() { - return Some(root_rewrite); +impl<'a> ChainFormatterVisual<'a> { + fn new(chain: &'a Chain) -> ChainFormatterVisual<'a> { + ChainFormatterVisual { + children: &chain.children, + rewrites: Vec::with_capacity(chain.children.len() + 1), + fits_single_line: false, } } - let last = &children[children.len() - 1]; - children = &children[..children.len() - 1]; + fn format_root(&mut self, parent: &ChainItem, context: &RewriteContext, shape: Shape) -> Option<()> { + // Parent is the first item in the chain, e.g., `foo` in `foo.bar.baz()`. + let parent_shape = if is_block_expr(context, &parent.expr, "\n") { + shape.visual_indent(0) + } else { + shape + }; + let mut root_rewrite = parent.expr + .rewrite(context, parent_shape) + .map(|parent_rw| parent_rw + &"?".repeat(parent.tries))?; - let child_shape = shape.visual_indent(0).with_max_width(context.config); + if !root_rewrite.contains('\n') && Self::is_continuable(&parent.expr) { + let item = &self.children[0]; + let overhead = last_line_width(&root_rewrite); + let shape = parent_shape.offset_left(overhead)?; + let rewrite = rewrite_chain_subexpr(&item.expr, context, shape)?; + root_rewrite.push_str(&rewrite); + root_rewrite.push_str(&"?".repeat(item.tries)); - let mut rewrites: Vec = Vec::with_capacity(children.len() + 2); - rewrites.push(root_rewrite); - for item in chain.children.iter() { - let rewrite = rewrite_chain_subexpr(&item.expr, context, child_shape)?; - rewrites.push(format!("{}{}", rewrite, "?".repeat(item.tries))); + self.children = &self.children[1..]; + } + + self.rewrites.push(root_rewrite); + Some(()) } - // Total of all items excluding the last. - let almost_total = rewrites.iter().fold(0, |a, b| a + b.len()) + last.tries; - let one_line_budget = if rewrites.len() == 1 { - shape.width - } else { - min(shape.width, context.config.width_heuristics().chain_width) - }; - let all_in_one_line = rewrites.iter().all(|s| !s.contains('\n')) - && almost_total < one_line_budget; - let last_shape = child_shape.sub_width(shape.rhs_overhead(context.config) + last.tries)?; + // Determines if we can continue formatting a given expression on the same line. + fn is_continuable(expr: &ast::Expr) -> bool { + match expr.node { + ast::ExprKind::Path(..) => true, + _ => false, + } + } - // 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.child2.last_child(|a, b, c| { - // let x = foo(a, b, c); - // let y = bar(a, b, c); - // - // // ... - // - // result - // }) - // ``` + fn format_children(&mut self, context: &RewriteContext, child_shape: Shape) -> Option<()> { + for item in &self.children[..self.children.len() - 1] { + let rewrite = rewrite_chain_subexpr(&item.expr, context, child_shape)?; + self.rewrites.push(format!("{}{}", rewrite, "?".repeat(item.tries))); + } + Some(()) + } - let mut last_subexpr_str = None; - let mut fits_single_line = false; - if all_in_one_line { - // First we try to 'overflow' the last child and see if it looks better than using - // vertical layout. - if let Some(shape) = parent_shape.offset_left(almost_total) { - if let Some(rw) = rewrite_chain_subexpr(&last.expr, context, shape) { - // We allow overflowing here only if both of the following conditions match: - // 1. The entire chain fits in a single line except the last child. - // 2. `last_child_str.lines().count() >= 5`. - let line_count = rw.lines().count(); - let could_fit_single_line = almost_total + first_line_width(&rw) <= one_line_budget; - if could_fit_single_line && line_count >= 5 { - last_subexpr_str = Some(rw); - fits_single_line = 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_chain_subexpr(&last.expr, context, last_shape) { - Some(ref new_rw) if !could_fit_single_line => { - last_subexpr_str = Some(new_rw.clone()); - } - Some(ref new_rw) if new_rw.lines().count() >= line_count => { - last_subexpr_str = Some(rw); - fits_single_line = could_fit_single_line; - } - new_rw @ Some(..) => { - last_subexpr_str = new_rw; - } - _ => { - last_subexpr_str = Some(rw); - fits_single_line = could_fit_single_line; + fn format_last_child(&mut self, context: &RewriteContext, shape: Shape, child_shape: Shape) -> Option<()> { + let last = &self.children[self.children.len() - 1]; + + // Total of all items excluding the last. + let almost_total = self.rewrites.iter().fold(0, |a, b| a + b.len()) + last.tries; + let one_line_budget = if self.rewrites.len() == 1 { + shape.width + } else { + min(shape.width, context.config.width_heuristics().chain_width) + }; + let all_in_one_line = self.rewrites.iter().all(|s| !s.contains('\n')) + && almost_total < one_line_budget; + let last_shape = child_shape.sub_width(shape.rhs_overhead(context.config) + last.tries)?; + + + let mut last_subexpr_str = None; + if all_in_one_line { + // First we try to 'overflow' the last child and see if it looks better than using + // vertical layout. + if let Some(shape) = shape.offset_left(almost_total) { + if let Some(rw) = rewrite_chain_subexpr(&last.expr, context, shape) { + // We allow overflowing here only if both of the following conditions match: + // 1. The entire chain fits in a single line except the last child. + // 2. `last_child_str.lines().count() >= 5`. + let line_count = rw.lines().count(); + let could_fit_single_line = almost_total + first_line_width(&rw) <= one_line_budget; + if could_fit_single_line && line_count >= 5 { + last_subexpr_str = Some(rw); + self.fits_single_line = all_in_one_line; + } 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_chain_subexpr(&last.expr, context, last_shape) { + Some(ref new_rw) if !could_fit_single_line => { + last_subexpr_str = Some(new_rw.clone()); + } + Some(ref new_rw) if new_rw.lines().count() >= line_count => { + last_subexpr_str = Some(rw); + self.fits_single_line = could_fit_single_line && all_in_one_line; + } + new_rw @ Some(..) => { + last_subexpr_str = new_rw; + } + _ => { + last_subexpr_str = Some(rw); + self.fits_single_line = could_fit_single_line && all_in_one_line; + } } } } } + } + + let last_subexpr_str = last_subexpr_str.or_else(|| rewrite_chain_subexpr(&last.expr, context, last_shape)); + self.rewrites.push(format!("{}{}", last_subexpr_str?, "?".repeat(last.tries))); + Some(()) + } + + fn join_rewrites(&self, context: &RewriteContext, child_shape: Shape) -> Option { + let connector = if self.fits_single_line { + // Yay, we can put everything on one line. + Cow::from("") + } else { + // Use new lines. + if *context.force_one_line_chain.borrow() { + return None; + } + child_shape.indent.to_string_with_newline(context.config) + }; + + let mut rewrite_iter = self.rewrites.iter(); + let mut result = rewrite_iter.next().unwrap().clone(); + + for rewrite in rewrite_iter { + result.push_str(&connector); + result.push_str(&rewrite); } - } - last_subexpr_str = last_subexpr_str.or_else(|| rewrite_chain_subexpr(&last.expr, context, last_shape)); - rewrites.push(last_subexpr_str?); + Some(result) + } +} - let connector = if fits_single_line && all_in_one_line { - // Yay, we can put everything on one line. - Cow::from("") - } else { - // Use new lines. - if *context.force_one_line_chain.borrow() { - return None; - } - child_shape.indent.to_string_with_newline(context.config) - }; +fn rewrite_chain_visual(chain: Chain, context: &RewriteContext, shape: Shape) -> Option { + let mut formatter = ChainFormatterVisual::new(&chain); - let result = format!("{}{}", - join_rewrites_vis(&rewrites, &connector), - "?".repeat(last.tries), - ); + formatter.format_root(&chain.parent, context, shape)?; + + if formatter.children.is_empty() { + assert_eq!(formatter.rewrites.len(), 1); + return Some(formatter.rewrites.pop().unwrap()); + } + + let child_shape = shape.visual_indent(0).with_max_width(context.config); + + formatter.format_children(context, child_shape)?; + formatter.format_last_child(context, shape, child_shape)?; + + let result = formatter.join_rewrites(context, child_shape)?; wrap_str(result, context.config.max_width(), shape) } -fn join_rewrites(rewrites: &[String], is_block_like: &[bool], connector: &str) -> String { - let mut rewrite_iter = rewrites.iter(); - let mut result = rewrite_iter.next().unwrap().clone(); - - for (rewrite, prev_is_block_like) in rewrite_iter.zip(is_block_like.iter()) { - if rewrite != "?" && !prev_is_block_like { - result.push_str(connector); - } - result.push_str(&rewrite); - } - - result -} - -fn join_rewrites_vis(rewrites: &[String], connector: &str) -> String { - let mut rewrite_iter = rewrites.iter(); - let mut result = rewrite_iter.next().unwrap().clone(); - - for rewrite in rewrite_iter { - if rewrite != "?" { - result.push_str(connector); - } - result.push_str(&rewrite); - } - - result -} - // States whether an expression's last line exclusively consists of closing // parens, braces, and brackets in its idiomatic formatting. fn is_block_expr(context: &RewriteContext, expr: &ast::Expr, repr: &str) -> bool { @@ -513,45 +590,6 @@ fn is_block_expr(context: &RewriteContext, expr: &ast::Expr, repr: &str) -> bool } } -// Returns a Vec of the prefixes of the chain. -// E.g., for input `a.b.c` we return [`a.b.c`, `a.b`, 'a'] -fn make_subexpr_list(expr: &ast::Expr, context: &RewriteContext) -> Vec { - let mut subexpr_list = vec![expr.clone()]; - - while let Some(subexpr) = pop_expr_chain(subexpr_list.last().unwrap(), context) { - subexpr_list.push(subexpr.clone()); - } - - subexpr_list -} - -// Returns the expression's subexpression, if it exists. When the subexpr -// is a try! macro, we'll convert it to shorthand when the option is set. -fn pop_expr_chain(expr: &ast::Expr, context: &RewriteContext) -> Option { - match expr.node { - ast::ExprKind::MethodCall(_, ref expressions) => { - Some(convert_try(&expressions[0], context)) - } - ast::ExprKind::Field(ref subexpr, _) | ast::ExprKind::Try(ref subexpr) => { - Some(convert_try(subexpr, context)) - } - _ => None, - } -} - -fn convert_try(expr: &ast::Expr, context: &RewriteContext) -> ast::Expr { - match expr.node { - ast::ExprKind::Mac(ref mac) if context.config.use_try_shorthand() => { - if let Some(subexpr) = convert_try_mac(mac, context) { - subexpr - } else { - expr.clone() - } - } - _ => expr.clone(), - } -} - // Rewrite the last element in the chain `expr`. E.g., given `a.b.c` we rewrite // `.c`. fn rewrite_chain_subexpr( @@ -600,14 +638,6 @@ fn is_tup_field_access(expr: &ast::Expr) -> bool { } } -// Determines if we can continue formatting a given expression on the same line. -fn is_continuable(expr: &ast::Expr) -> bool { - match expr.node { - ast::ExprKind::Path(..) => true, - _ => false, - } -} - fn rewrite_method_call( method_name: ast::Ident, types: &[ast::GenericArg], From f55cadb65aacded4593312e56709fff3388e5f13 Mon Sep 17 00:00:00 2001 From: Nick Cameron Date: Wed, 11 Jul 2018 13:39:35 +1200 Subject: [PATCH 10/19] chains: refactor formatting of chain items --- src/chains.rs | 205 +++++++++++++++++++++++++------------------------- 1 file changed, 102 insertions(+), 103 deletions(-) diff --git a/src/chains.rs b/src/chains.rs index 692d1c946fc5..5e2c3e051f65 100644 --- a/src/chains.rs +++ b/src/chains.rs @@ -85,8 +85,7 @@ pub fn rewrite_chain(expr: &ast::Expr, context: &RewriteContext, shape: Shape) - // If this is just an expression with some `?`s, then format it trivially and // return early. if chain.children.is_empty() { - let rewrite = chain.parent.expr.rewrite(context, shape.sub_width(chain.parent.tries)?)?; - return Some(format!("{}{}", rewrite, "?".repeat(chain.parent.tries))); + return chain.parent.rewrite(context, shape); } match context.config.indent_style() { @@ -102,6 +101,89 @@ struct ChainItem { tries: usize, } +impl Rewrite for ChainItem { + fn rewrite(&self, context: &RewriteContext, shape: Shape) -> Option { + let rewrite = self.expr.rewrite(context, shape.sub_width(self.tries)?)?; + Some(format!("{}{}", rewrite, "?".repeat(self.tries))) + } +} + +impl ChainItem { + // Rewrite the last element in the chain `expr`. E.g., given `a.b.c` we rewrite + // `.c` and any trailing `?`s. + fn rewrite_postfix( + &self, + context: &RewriteContext, + shape: Shape, + ) -> Option { + let shape = shape.sub_width(self.tries)?; + let mut rewrite = match self.expr.node { + ast::ExprKind::MethodCall(ref segment, ref expressions) => { + let types = match segment.args { + Some(ref params) => match **params { + ast::GenericArgs::AngleBracketed(ref data) => &data.args[..], + _ => &[], + }, + _ => &[], + }; + Self::rewrite_method_call(segment.ident, types, expressions, self.expr.span, context, shape)? + } + ast::ExprKind::Field(ref nested, ref field) => { + let space = if Self::is_tup_field_access(&self.expr) && Self::is_tup_field_access(nested) { + " " + } else { + "" + }; + let result = format!("{}.{}", space, field.name); + if result.len() <= shape.width { + result + } else { + return None; + } + } + _ => unreachable!(), + }; + rewrite.push_str(&"?".repeat(self.tries)); + Some(rewrite) + } + + fn is_tup_field_access(expr: &ast::Expr) -> bool { + match expr.node { + ast::ExprKind::Field(_, ref field) => { + field.name.to_string().chars().all(|c| c.is_digit(10)) + } + _ => false, + } + } + + fn rewrite_method_call( + method_name: ast::Ident, + types: &[ast::GenericArg], + args: &[ptr::P], + span: Span, + context: &RewriteContext, + shape: Shape, + ) -> Option { + let (lo, type_str) = if types.is_empty() { + (args[0].span.hi(), String::new()) + } else { + let type_list = types + .iter() + .map(|ty| ty.rewrite(context, shape)) + .collect::>>()?; + + let type_str = format!("::<{}>", type_list.join(", ")); + + (types.last().unwrap().span().hi(), type_str) + }; + + let callee_str = format!(".{}{}", method_name, type_str); + let span = mk_sp(lo, span.hi()); + + rewrite_call(context, &callee_str, &args[1..], span, shape) + } +} + #[derive(Debug)] struct Chain { parent: ChainItem, @@ -209,9 +291,7 @@ impl <'a> ChainFormatterBlock<'a> { // ``` // If `bar` were not part of the root, then baz would be orphaned and 'float'. fn format_root(&mut self, parent: &ChainItem, context: &RewriteContext, shape: Shape) -> Option<()> { - let mut root_rewrite: String = parent.expr - .rewrite(context, shape) - .map(|parent_rw| parent_rw + &"?".repeat(parent.tries))?; + let mut root_rewrite: String = parent.rewrite(context, shape)?; self.root_ends_with_block = is_block_expr(context, &parent.expr, &root_rewrite); let tab_width = context.config.tab_spaces().saturating_sub(shape.offset); @@ -219,11 +299,8 @@ impl <'a> ChainFormatterBlock<'a> { while root_rewrite.len() <= tab_width && !root_rewrite.contains('\n') { let item = &self.children[0]; let shape = shape.offset_left(root_rewrite.len())?; - match rewrite_chain_subexpr(&item.expr, context, shape) { - Some(rewrite) => { - root_rewrite.push_str(&rewrite); - root_rewrite.push_str(&"?".repeat(item.tries)); - } + match &item.rewrite_postfix(context, shape) { + Some(rewrite) => root_rewrite.push_str(rewrite), None => break, } @@ -248,10 +325,10 @@ impl <'a> ChainFormatterBlock<'a> { fn format_children(&mut self, context: &RewriteContext, child_shape: Shape) -> Option<()> { self.is_block_like.push(self.root_ends_with_block); - for item in &self.children[..self.children.len()] { - let rewrite = rewrite_chain_subexpr(&item.expr, context, child_shape)?; + for item in &self.children[..self.children.len() - 1] { + let rewrite = item.rewrite_postfix(context, child_shape)?; self.is_block_like.push(is_block_expr(context, &item.expr, &rewrite)); - self.rewrites.push(format!("{}{}", rewrite, "?".repeat(item.tries))); + self.rewrites.push(rewrite); } Some(()) } @@ -315,7 +392,7 @@ impl <'a> ChainFormatterBlock<'a> { // First we try to 'overflow' the last child and see if it looks better than using // vertical layout. if let Some(shape) = last_shape.offset_left(almost_total) { - if let Some(rw) = rewrite_chain_subexpr(&last.expr, context, shape) { + if let Some(rw) = last.rewrite_postfix(context, shape) { // We allow overflowing here only if both of the following conditions match: // 1. The entire chain fits in a single line except the last child. // 2. `last_child_str.lines().count() >= 5`. @@ -328,7 +405,7 @@ impl <'a> ChainFormatterBlock<'a> { // 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_chain_subexpr(&last.expr, context, last_shape) { + match last.rewrite_postfix(context, last_shape) { Some(ref new_rw) if !could_fit_single_line => { last_subexpr_str = Some(new_rw.clone()); } @@ -349,8 +426,8 @@ impl <'a> ChainFormatterBlock<'a> { } } - last_subexpr_str = last_subexpr_str.or_else(|| rewrite_chain_subexpr(&last.expr, context, last_shape)); - self.rewrites.push(format!("{}{}", last_subexpr_str?, "?".repeat(last.tries))); + last_subexpr_str = last_subexpr_str.or_else(|| last.rewrite_postfix(context, last_shape)); + self.rewrites.push(last_subexpr_str?); Some(()) } @@ -424,17 +501,14 @@ impl<'a> ChainFormatterVisual<'a> { } else { shape }; - let mut root_rewrite = parent.expr - .rewrite(context, parent_shape) - .map(|parent_rw| parent_rw + &"?".repeat(parent.tries))?; + let mut root_rewrite = parent.rewrite(context, parent_shape)?; if !root_rewrite.contains('\n') && Self::is_continuable(&parent.expr) { let item = &self.children[0]; let overhead = last_line_width(&root_rewrite); let shape = parent_shape.offset_left(overhead)?; - let rewrite = rewrite_chain_subexpr(&item.expr, context, shape)?; + let rewrite = item.rewrite_postfix(context, shape)?; root_rewrite.push_str(&rewrite); - root_rewrite.push_str(&"?".repeat(item.tries)); self.children = &self.children[1..]; } @@ -453,8 +527,8 @@ impl<'a> ChainFormatterVisual<'a> { fn format_children(&mut self, context: &RewriteContext, child_shape: Shape) -> Option<()> { for item in &self.children[..self.children.len() - 1] { - let rewrite = rewrite_chain_subexpr(&item.expr, context, child_shape)?; - self.rewrites.push(format!("{}{}", rewrite, "?".repeat(item.tries))); + let rewrite = item.rewrite_postfix(context, child_shape)?; + self.rewrites.push(rewrite); } Some(()) } @@ -479,7 +553,7 @@ impl<'a> ChainFormatterVisual<'a> { // First we try to 'overflow' the last child and see if it looks better than using // vertical layout. if let Some(shape) = shape.offset_left(almost_total) { - if let Some(rw) = rewrite_chain_subexpr(&last.expr, context, shape) { + if let Some(rw) = last.rewrite_postfix(context, shape) { // We allow overflowing here only if both of the following conditions match: // 1. The entire chain fits in a single line except the last child. // 2. `last_child_str.lines().count() >= 5`. @@ -492,7 +566,7 @@ impl<'a> ChainFormatterVisual<'a> { // 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_chain_subexpr(&last.expr, context, last_shape) { + match last.rewrite_postfix(context, last_shape) { Some(ref new_rw) if !could_fit_single_line => { last_subexpr_str = Some(new_rw.clone()); } @@ -513,8 +587,8 @@ impl<'a> ChainFormatterVisual<'a> { } } - let last_subexpr_str = last_subexpr_str.or_else(|| rewrite_chain_subexpr(&last.expr, context, last_shape)); - self.rewrites.push(format!("{}{}", last_subexpr_str?, "?".repeat(last.tries))); + let last_subexpr_str = last_subexpr_str.or_else(|| last.rewrite_postfix(context, last_shape)); + self.rewrites.push(last_subexpr_str?); Some(()) } @@ -589,78 +663,3 @@ fn is_block_expr(context: &RewriteContext, expr: &ast::Expr, repr: &str) -> bool _ => false, } } - -// Rewrite the last element in the chain `expr`. E.g., given `a.b.c` we rewrite -// `.c`. -fn rewrite_chain_subexpr( - expr: &ast::Expr, - context: &RewriteContext, - shape: Shape, -) -> Option { - let rewrite_element = |expr_str: String| { - if expr_str.len() <= shape.width { - Some(expr_str) - } else { - None - } - }; - - match expr.node { - ast::ExprKind::MethodCall(ref segment, ref expressions) => { - let types = match segment.args { - Some(ref params) => match **params { - ast::GenericArgs::AngleBracketed(ref data) => &data.args[..], - _ => &[], - }, - _ => &[], - }; - rewrite_method_call(segment.ident, types, expressions, expr.span, context, shape) - } - ast::ExprKind::Field(ref nested, ref field) => { - let space = if is_tup_field_access(expr) && is_tup_field_access(nested) { - " " - } else { - "" - }; - rewrite_element(format!("{}.{}", space, field.name)) - } - ast::ExprKind::Try(_) => rewrite_element(String::from("?")), - _ => unreachable!(), - } -} - -fn is_tup_field_access(expr: &ast::Expr) -> bool { - match expr.node { - ast::ExprKind::Field(_, ref field) => { - field.name.to_string().chars().all(|c| c.is_digit(10)) - } - _ => false, - } -} - -fn rewrite_method_call( - method_name: ast::Ident, - types: &[ast::GenericArg], - args: &[ptr::P], - span: Span, - context: &RewriteContext, - shape: Shape, -) -> Option { - let (lo, type_str) = if types.is_empty() { - (args[0].span.hi(), String::new()) - } else { - let type_list = types - .iter() - .map(|ty| ty.rewrite(context, shape)) - .collect::>>()?; - - let type_str = format!("::<{}>", type_list.join(", ")); - - (types.last().unwrap().span().hi(), type_str) - }; - - let callee_str = format!(".{}{}", method_name, type_str); - let span = mk_sp(lo, span.hi()); - - rewrite_call(context, &callee_str, &args[1..], span, shape) -} From 467b095d48ebf5f5e057e1a2b4c1c25aa0375ea1 Mon Sep 17 00:00:00 2001 From: Nick Cameron Date: Wed, 11 Jul 2018 14:50:42 +1200 Subject: [PATCH 11/19] chains: share code between block and visual formatters --- src/chains.rs | 356 +++++++++++++++++++++++--------------------------- 1 file changed, 163 insertions(+), 193 deletions(-) diff --git a/src/chains.rs b/src/chains.rs index 5e2c3e051f65..13436106ccb3 100644 --- a/src/chains.rs +++ b/src/chains.rs @@ -75,6 +75,7 @@ use utils::{first_line_width, last_line_extendable, last_line_width, mk_sp, wrap use std::borrow::Cow; use std::cmp::min; +use std::iter; use syntax::codemap::Span; use syntax::{ast, ptr}; @@ -88,10 +89,7 @@ pub fn rewrite_chain(expr: &ast::Expr, context: &RewriteContext, shape: Shape) - return chain.parent.rewrite(context, shape); } - match context.config.indent_style() { - IndentStyle::Block => rewrite_chain_block(chain, context, shape), - IndentStyle::Visual => rewrite_chain_visual(chain, context, shape), - } + chain.rewrite(context, shape) } // An expression plus trailing `?`s to be formatted together. @@ -262,26 +260,36 @@ impl Chain { } } -// TODO comments -struct ChainFormatterBlock<'a> { - children: &'a[ChainItem], - rewrites: Vec, - root_ends_with_block: bool, - is_block_like: Vec, - fits_single_line: bool, +impl Rewrite for Chain { + fn rewrite(&self, context: &RewriteContext, shape: Shape) -> Option { + debug!("rewrite chain {:?} {:?}", self, shape); + + let mut formatter = match context.config.indent_style() { + IndentStyle::Block => Box::new(ChainFormatterBlock::new(self)) as Box, + IndentStyle::Visual => Box::new(ChainFormatterVisual::new(self)) as Box, + }; + + formatter.format_root(&self.parent, context, shape)?; + if let result @ Some(_) = formatter.pure_root() { + return result; + } + + // Decide how to layout the rest of the chain. + let child_shape = formatter.child_shape(context, shape); + + formatter.format_children(context, child_shape)?; + formatter.format_last_child(context, shape, child_shape)?; + + let result = formatter.join_rewrites(context, child_shape)?; + wrap_str(result, context.config.max_width(), shape) + } } -impl <'a> ChainFormatterBlock<'a> { - fn new(chain: &'a Chain) -> ChainFormatterBlock<'a> { - ChainFormatterBlock { - children: &chain.children, - root_ends_with_block: false, - rewrites: Vec::with_capacity(chain.children.len() + 1), - is_block_like: Vec::with_capacity(chain.children.len() + 1), - fits_single_line: false, - } - } - +// There are a few types for formatting chains. This is because there is a lot +// in common between formatting with block vs visual indent, but they are +// different enough that branching on the indent all over the place gets ugly. +// Anything that can format a chain is a ChainFormatter. +trait ChainFormatter { // Parent is the first item in the chain, e.g., `foo` in `foo.bar.baz()`. // Root is the parent plus any other chain items placed on the first line to // avoid an orphan. E.g., @@ -290,47 +298,43 @@ impl <'a> ChainFormatterBlock<'a> { // .baz() // ``` // If `bar` were not part of the root, then baz would be orphaned and 'float'. - fn format_root(&mut self, parent: &ChainItem, context: &RewriteContext, shape: Shape) -> Option<()> { - let mut root_rewrite: String = parent.rewrite(context, shape)?; + fn format_root(&mut self, parent: &ChainItem, context: &RewriteContext, shape: Shape) -> Option<()>; + fn child_shape(&self, context: &RewriteContext, shape: Shape) -> Shape; + fn format_children(&mut self, context: &RewriteContext, child_shape: Shape) -> Option<()>; + fn format_last_child(&mut self, context: &RewriteContext, shape: Shape, child_shape: Shape) -> Option<()>; + fn join_rewrites(&self, context: &RewriteContext, child_shape: Shape) -> Option; + // Returns `Some` if the chain is only a root, None otherwise. + fn pure_root(&mut self) -> Option; +} - self.root_ends_with_block = is_block_expr(context, &parent.expr, &root_rewrite); - let tab_width = context.config.tab_spaces().saturating_sub(shape.offset); +// Data and behaviour that is shared by both chain formatters. The concrete +// formatters can delegate much behaviour to `ChainFormatterShared`. +struct ChainFormatterShared<'a> { + // The current working set of child items. + children: &'a[ChainItem], + // The current rewrites of items (includes trailing `?`s, but not any way to + // connect the rewrites together). + rewrites: Vec, + // Whether the chain can fit on one line. + fits_single_line: bool, +} - while root_rewrite.len() <= tab_width && !root_rewrite.contains('\n') { - let item = &self.children[0]; - let shape = shape.offset_left(root_rewrite.len())?; - match &item.rewrite_postfix(context, shape) { - Some(rewrite) => root_rewrite.push_str(rewrite), - None => break, - } - - self.root_ends_with_block = is_block_expr(context, &item.expr, &root_rewrite); - - self.children = &self.children[1..]; - if self.children.is_empty() { - break; - } +impl <'a> ChainFormatterShared<'a> { + fn new(chain: &'a Chain) -> ChainFormatterShared<'a> { + ChainFormatterShared { + children: &chain.children, + rewrites: Vec::with_capacity(chain.children.len() + 1), + fits_single_line: false, } - self.rewrites.push(root_rewrite); - Some(()) } - fn child_shape(&self, context: &RewriteContext, shape: Shape) -> Shape { - if self.root_ends_with_block { - shape + fn pure_root(&mut self) -> Option { + if self.children.is_empty() { + assert_eq!(self.rewrites.len(), 1); + Some(self.rewrites.pop().unwrap()) } else { - shape.block_indent(context.config.tab_spaces()) - }.with_max_width(context.config) - } - - fn format_children(&mut self, context: &RewriteContext, child_shape: Shape) -> Option<()> { - self.is_block_like.push(self.root_ends_with_block); - for item in &self.children[..self.children.len() - 1] { - let rewrite = item.rewrite_postfix(context, child_shape)?; - self.is_block_like.push(is_block_expr(context, &item.expr, &rewrite)); - self.rewrites.push(rewrite); + None } - Some(()) } // Rewrite the last child. The last child of a chain requires special treatment. We need to @@ -365,9 +369,10 @@ impl <'a> ChainFormatterBlock<'a> { // result // }) // ``` - fn format_last_child(&mut self, context: &RewriteContext, shape: Shape, child_shape: Shape) -> Option<()> { + fn format_last_child(&mut self, may_extend: bool, context: &RewriteContext, shape: Shape, child_shape: Shape) -> Option<()> { let last = &self.children[self.children.len() - 1]; - let extendable = last_line_extendable(&self.rewrites[self.rewrites.len() - 1]); + let extendable = may_extend && last_line_extendable(&self.rewrites[self.rewrites.len() - 1]); + // Total of all items excluding the last. let almost_total = if extendable { last_line_width(&self.rewrites[self.rewrites.len() - 1]) @@ -431,8 +436,7 @@ impl <'a> ChainFormatterBlock<'a> { Some(()) } - - fn join_rewrites(&self, context: &RewriteContext, child_shape: Shape) -> Option { + fn join_rewrites(&self, context: &RewriteContext, child_shape: Shape, block_like_iter: impl Iterator) -> Option { let connector = if self.fits_single_line { // Yay, we can put everything on one line. Cow::from("") @@ -447,8 +451,8 @@ impl <'a> ChainFormatterBlock<'a> { let mut rewrite_iter = self.rewrites.iter(); let mut result = rewrite_iter.next().unwrap().clone(); - for (rewrite, prev_is_block_like) in rewrite_iter.zip(self.is_block_like.iter()) { - if rewrite != "?" && !prev_is_block_like { + for (rewrite, prev_is_block_like) in rewrite_iter.zip(block_like_iter) { + if !prev_is_block_like { result.push_str(&connector); } result.push_str(&rewrite); @@ -458,43 +462,102 @@ impl <'a> ChainFormatterBlock<'a> { } } -fn rewrite_chain_block(chain: Chain, context: &RewriteContext, shape: Shape) -> Option { - debug!("rewrite_chain_block {:?} {:?}", chain, shape); - - let mut formatter = ChainFormatterBlock::new(&chain); - - formatter.format_root(&chain.parent, context, shape)?; - if formatter.children.is_empty() { - assert_eq!(formatter.rewrites.len(), 1); - return Some(formatter.rewrites.pop().unwrap()); - } - - // Decide how to layout the rest of the chain. - let child_shape = formatter.child_shape(context, shape); - formatter.format_children(context, child_shape)?; - - formatter.format_last_child(context, shape, child_shape)?; - - let result = formatter.join_rewrites(context, child_shape)?; - Some(result) +// Formats a chain using block indent. +struct ChainFormatterBlock<'a> { + shared: ChainFormatterShared<'a>, + // For each rewrite, whether the corresponding item is block-like. + is_block_like: Vec, } +impl <'a> ChainFormatterBlock<'a> { + fn new(chain: &'a Chain) -> ChainFormatterBlock<'a> { + ChainFormatterBlock { + shared: ChainFormatterShared::new(chain), + is_block_like: Vec::with_capacity(chain.children.len() + 1), + } + } +} + +impl <'a> ChainFormatter for ChainFormatterBlock<'a> { + fn format_root(&mut self, parent: &ChainItem, context: &RewriteContext, shape: Shape) -> Option<()> { + let mut root_rewrite: String = parent.rewrite(context, shape)?; + + let mut root_ends_with_block = is_block_expr(context, &parent.expr, &root_rewrite); + let tab_width = context.config.tab_spaces().saturating_sub(shape.offset); + + while root_rewrite.len() <= tab_width && !root_rewrite.contains('\n') { + let item = &self.shared.children[0]; + let shape = shape.offset_left(root_rewrite.len())?; + match &item.rewrite_postfix(context, shape) { + Some(rewrite) => root_rewrite.push_str(rewrite), + None => break, + } + + root_ends_with_block = is_block_expr(context, &item.expr, &root_rewrite); + + self.shared.children = &self.shared.children[1..]; + if self.shared.children.is_empty() { + break; + } + } + self.is_block_like.push(root_ends_with_block); + self.shared.rewrites.push(root_rewrite); + Some(()) + } + + fn child_shape(&self, context: &RewriteContext, shape: Shape) -> Shape { + if self.is_block_like[0] { + shape + } else { + shape.block_indent(context.config.tab_spaces()) + }.with_max_width(context.config) + } + + fn format_children(&mut self, context: &RewriteContext, child_shape: Shape) -> Option<()> { + for item in &self.shared.children[..self.shared.children.len() - 1] { + let rewrite = item.rewrite_postfix(context, child_shape)?; + self.is_block_like.push(is_block_expr(context, &item.expr, &rewrite)); + self.shared.rewrites.push(rewrite); + } + Some(()) + } + + fn format_last_child(&mut self, context: &RewriteContext, shape: Shape, child_shape: Shape) -> Option<()> { + self.shared.format_last_child(true, context, shape, child_shape) + } + + fn join_rewrites(&self, context: &RewriteContext, child_shape: Shape) -> Option { + self.shared.join_rewrites(context, child_shape, self.is_block_like.iter().cloned()) + } + + fn pure_root(&mut self) -> Option { + self.shared.pure_root() + } +} + +// Format a chain using visual indent. struct ChainFormatterVisual<'a> { - children: &'a[ChainItem], - rewrites: Vec, - fits_single_line: bool, + shared: ChainFormatterShared<'a>, } impl<'a> ChainFormatterVisual<'a> { fn new(chain: &'a Chain) -> ChainFormatterVisual<'a> { ChainFormatterVisual { - children: &chain.children, - rewrites: Vec::with_capacity(chain.children.len() + 1), - fits_single_line: false, + shared: ChainFormatterShared::new(chain), } } +} +impl <'a> ChainFormatter for ChainFormatterVisual<'a> { fn format_root(&mut self, parent: &ChainItem, context: &RewriteContext, shape: Shape) -> Option<()> { + // Determines if we can continue formatting a given expression on the same line. + fn is_continuable(expr: &ast::Expr) -> bool { + match expr.node { + ast::ExprKind::Path(..) => true, + _ => false, + } + } + // Parent is the first item in the chain, e.g., `foo` in `foo.bar.baz()`. let parent_shape = if is_block_expr(context, &parent.expr, "\n") { shape.visual_indent(0) @@ -503,136 +566,43 @@ impl<'a> ChainFormatterVisual<'a> { }; let mut root_rewrite = parent.rewrite(context, parent_shape)?; - if !root_rewrite.contains('\n') && Self::is_continuable(&parent.expr) { - let item = &self.children[0]; + if !root_rewrite.contains('\n') && is_continuable(&parent.expr) { + let item = &self.shared.children[0]; let overhead = last_line_width(&root_rewrite); let shape = parent_shape.offset_left(overhead)?; let rewrite = item.rewrite_postfix(context, shape)?; root_rewrite.push_str(&rewrite); - self.children = &self.children[1..]; + self.shared.children = &self.shared.children[1..]; } - self.rewrites.push(root_rewrite); + self.shared.rewrites.push(root_rewrite); Some(()) } - // Determines if we can continue formatting a given expression on the same line. - fn is_continuable(expr: &ast::Expr) -> bool { - match expr.node { - ast::ExprKind::Path(..) => true, - _ => false, - } + fn child_shape(&self, context: &RewriteContext, shape: Shape) -> Shape { + shape.visual_indent(0).with_max_width(context.config) } fn format_children(&mut self, context: &RewriteContext, child_shape: Shape) -> Option<()> { - for item in &self.children[..self.children.len() - 1] { + for item in &self.shared.children[..self.shared.children.len() - 1] { let rewrite = item.rewrite_postfix(context, child_shape)?; - self.rewrites.push(rewrite); + self.shared.rewrites.push(rewrite); } Some(()) } fn format_last_child(&mut self, context: &RewriteContext, shape: Shape, child_shape: Shape) -> Option<()> { - let last = &self.children[self.children.len() - 1]; - - // Total of all items excluding the last. - let almost_total = self.rewrites.iter().fold(0, |a, b| a + b.len()) + last.tries; - let one_line_budget = if self.rewrites.len() == 1 { - shape.width - } else { - min(shape.width, context.config.width_heuristics().chain_width) - }; - let all_in_one_line = self.rewrites.iter().all(|s| !s.contains('\n')) - && almost_total < one_line_budget; - let last_shape = child_shape.sub_width(shape.rhs_overhead(context.config) + last.tries)?; - - - let mut last_subexpr_str = None; - if all_in_one_line { - // First we try to 'overflow' the last child and see if it looks better than using - // vertical layout. - if let Some(shape) = shape.offset_left(almost_total) { - if let Some(rw) = last.rewrite_postfix(context, shape) { - // We allow overflowing here only if both of the following conditions match: - // 1. The entire chain fits in a single line except the last child. - // 2. `last_child_str.lines().count() >= 5`. - let line_count = rw.lines().count(); - let could_fit_single_line = almost_total + first_line_width(&rw) <= one_line_budget; - if could_fit_single_line && line_count >= 5 { - last_subexpr_str = Some(rw); - self.fits_single_line = all_in_one_line; - } 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 last.rewrite_postfix(context, last_shape) { - Some(ref new_rw) if !could_fit_single_line => { - last_subexpr_str = Some(new_rw.clone()); - } - Some(ref new_rw) if new_rw.lines().count() >= line_count => { - last_subexpr_str = Some(rw); - self.fits_single_line = could_fit_single_line && all_in_one_line; - } - new_rw @ Some(..) => { - last_subexpr_str = new_rw; - } - _ => { - last_subexpr_str = Some(rw); - self.fits_single_line = could_fit_single_line && all_in_one_line; - } - } - } - } - } - } - - let last_subexpr_str = last_subexpr_str.or_else(|| last.rewrite_postfix(context, last_shape)); - self.rewrites.push(last_subexpr_str?); - Some(()) + self.shared.format_last_child(false, context, shape, child_shape) } fn join_rewrites(&self, context: &RewriteContext, child_shape: Shape) -> Option { - let connector = if self.fits_single_line { - // Yay, we can put everything on one line. - Cow::from("") - } else { - // Use new lines. - if *context.force_one_line_chain.borrow() { - return None; - } - child_shape.indent.to_string_with_newline(context.config) - }; - - let mut rewrite_iter = self.rewrites.iter(); - let mut result = rewrite_iter.next().unwrap().clone(); - - for rewrite in rewrite_iter { - result.push_str(&connector); - result.push_str(&rewrite); - } - - Some(result) - } -} - -fn rewrite_chain_visual(chain: Chain, context: &RewriteContext, shape: Shape) -> Option { - let mut formatter = ChainFormatterVisual::new(&chain); - - formatter.format_root(&chain.parent, context, shape)?; - - if formatter.children.is_empty() { - assert_eq!(formatter.rewrites.len(), 1); - return Some(formatter.rewrites.pop().unwrap()); + self.shared.join_rewrites(context, child_shape, iter::repeat(false)) } - let child_shape = shape.visual_indent(0).with_max_width(context.config); - - formatter.format_children(context, child_shape)?; - formatter.format_last_child(context, shape, child_shape)?; - - let result = formatter.join_rewrites(context, child_shape)?; - wrap_str(result, context.config.max_width(), shape) + fn pure_root(&mut self) -> Option { + self.shared.pure_root() + } } // States whether an expression's last line exclusively consists of closing From 5bc27593f44a7c2108ca1a69eba2d34b3db90935 Mon Sep 17 00:00:00 2001 From: Nick Cameron Date: Wed, 11 Jul 2018 14:56:26 +1200 Subject: [PATCH 12/19] chains: minor fixups * remove unnecessary clone * comment formatting * fix bug with `?` collection * respect the heuristic if the root is more than just the parent --- src/chains.rs | 47 +++++++++++++++++++++++++---------------------- 1 file changed, 25 insertions(+), 22 deletions(-) diff --git a/src/chains.rs b/src/chains.rs index 13436106ccb3..81be8d541287 100644 --- a/src/chains.rs +++ b/src/chains.rs @@ -81,8 +81,9 @@ use syntax::codemap::Span; use syntax::{ast, ptr}; pub fn rewrite_chain(expr: &ast::Expr, context: &RewriteContext, shape: Shape) -> Option { - debug!("rewrite_chain {:?}", shape); let chain = Chain::from_ast(expr, context); + debug!("rewrite_chain {:?} {:?}", chain, shape); + // If this is just an expression with some `?`s, then format it trivially and // return early. if chain.children.is_empty() { @@ -95,6 +96,9 @@ pub fn rewrite_chain(expr: &ast::Expr, context: &RewriteContext, shape: Shape) - // An expression plus trailing `?`s to be formatted together. #[derive(Debug)] struct ChainItem { + // FIXME: we can't use a reference here because to convert `try!` to `?` we + // synthesise the AST node. However, I think we could use `Cow` and that + // would remove a lot of cloning. expr: ast::Expr, tries: usize, } @@ -185,28 +189,22 @@ impl ChainItem { #[derive(Debug)] struct Chain { parent: ChainItem, - // TODO do we need to clone the exprs? children: Vec, } impl Chain { fn from_ast(expr: &ast::Expr, context: &RewriteContext) -> Chain { - let mut subexpr_list = Self::make_subexpr_list(expr, context); + let subexpr_list = Self::make_subexpr_list(expr, context); // Un-parse the expression tree into ChainItems let mut children = vec![]; let mut sub_tries = 0; - loop { - if subexpr_list.is_empty() { - break; - } - - let subexpr = subexpr_list.pop().unwrap(); + for subexpr in subexpr_list { match subexpr.node { ast::ExprKind::Try(_) => sub_tries += 1, _ => { children.push(ChainItem { - expr: subexpr.clone(), + expr: subexpr, tries: sub_tries, }); sub_tries = 0; @@ -215,7 +213,7 @@ impl Chain { } Chain { - parent: children.remove(0), + parent: children.pop().unwrap(), children, } } @@ -317,6 +315,9 @@ struct ChainFormatterShared<'a> { rewrites: Vec, // Whether the chain can fit on one line. fits_single_line: bool, + // The number of children in the chain. This is not equal to `self.children.len()` + // because `self.children` will change size as we process the chain. + child_count: usize, } impl <'a> ChainFormatterShared<'a> { @@ -325,6 +326,7 @@ impl <'a> ChainFormatterShared<'a> { children: &chain.children, rewrites: Vec::with_capacity(chain.children.len() + 1), fits_single_line: false, + child_count: chain.children.len(), } } @@ -370,7 +372,7 @@ impl <'a> ChainFormatterShared<'a> { // }) // ``` fn format_last_child(&mut self, may_extend: bool, context: &RewriteContext, shape: Shape, child_shape: Shape) -> Option<()> { - let last = &self.children[self.children.len() - 1]; + let last = &self.children[0]; let extendable = may_extend && last_line_extendable(&self.rewrites[self.rewrites.len() - 1]); // Total of all items excluding the last. @@ -379,7 +381,7 @@ impl <'a> ChainFormatterShared<'a> { } else { self.rewrites.iter().fold(0, |a, b| a + b.len()) } + last.tries; - let one_line_budget = if self.rewrites.len() == 1 { + let one_line_budget = if self.child_count == 1 { shape.width } else { min(shape.width, context.config.width_heuristics().chain_width) @@ -407,9 +409,10 @@ impl <'a> ChainFormatterShared<'a> { last_subexpr_str = Some(rw); self.fits_single_line = all_in_one_line; } 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. + // 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 last.rewrite_postfix(context, last_shape) { Some(ref new_rw) if !could_fit_single_line => { last_subexpr_str = Some(new_rw.clone()); @@ -486,7 +489,7 @@ impl <'a> ChainFormatter for ChainFormatterBlock<'a> { let tab_width = context.config.tab_spaces().saturating_sub(shape.offset); while root_rewrite.len() <= tab_width && !root_rewrite.contains('\n') { - let item = &self.shared.children[0]; + let item = &self.shared.children[self.shared.children.len() - 1]; let shape = shape.offset_left(root_rewrite.len())?; match &item.rewrite_postfix(context, shape) { Some(rewrite) => root_rewrite.push_str(rewrite), @@ -495,7 +498,7 @@ impl <'a> ChainFormatter for ChainFormatterBlock<'a> { root_ends_with_block = is_block_expr(context, &item.expr, &root_rewrite); - self.shared.children = &self.shared.children[1..]; + self.shared.children = &self.shared.children[..self.shared.children.len() - 1]; if self.shared.children.is_empty() { break; } @@ -514,7 +517,7 @@ impl <'a> ChainFormatter for ChainFormatterBlock<'a> { } fn format_children(&mut self, context: &RewriteContext, child_shape: Shape) -> Option<()> { - for item in &self.shared.children[..self.shared.children.len() - 1] { + for item in self.shared.children[1..].iter().rev() { let rewrite = item.rewrite_postfix(context, child_shape)?; self.is_block_like.push(is_block_expr(context, &item.expr, &rewrite)); self.shared.rewrites.push(rewrite); @@ -567,13 +570,13 @@ impl <'a> ChainFormatter for ChainFormatterVisual<'a> { let mut root_rewrite = parent.rewrite(context, parent_shape)?; if !root_rewrite.contains('\n') && is_continuable(&parent.expr) { - let item = &self.shared.children[0]; + let item = &self.shared.children[self.shared.children.len() - 1]; let overhead = last_line_width(&root_rewrite); let shape = parent_shape.offset_left(overhead)?; let rewrite = item.rewrite_postfix(context, shape)?; root_rewrite.push_str(&rewrite); - self.shared.children = &self.shared.children[1..]; + self.shared.children = &self.shared.children[..self.shared.children.len() - 1]; } self.shared.rewrites.push(root_rewrite); @@ -585,7 +588,7 @@ impl <'a> ChainFormatter for ChainFormatterVisual<'a> { } fn format_children(&mut self, context: &RewriteContext, child_shape: Shape) -> Option<()> { - for item in &self.shared.children[..self.shared.children.len() - 1] { + for item in self.shared.children[1..].iter().rev() { let rewrite = item.rewrite_postfix(context, child_shape)?; self.shared.rewrites.push(rewrite); } From f0fe9c3c4acb4eafba5a552a2823e6997e75a3f4 Mon Sep 17 00:00:00 2001 From: Nick Cameron Date: Wed, 11 Jul 2018 21:01:40 +1200 Subject: [PATCH 13/19] chains: prefer to use the next line for an expression, if using the same line would introduce an open block or similar This problem came to light due to the chains changes, but effects other code too. It only happens rarely, e.g., before this fix: ``` match foo { MacroArgKind::Delimited(ref delim_tok, ref args) => rewrite_delimited_inner( delim_tok, args, ).map(|(lhs, inner, rhs)| format!("{}{}{}", lhs, inner, rhs)), }; ``` after: ``` match foo { MacroArgKind::Delimited(ref delim_tok, ref args) => { rewrite_delimited_inner(delim_tok, args) .map(|(lhs, inner, rhs)| format!("{}{}{}", lhs, inner, rhs)) } } ``` --- src/chains.rs | 5 +++-- src/expr.rs | 9 ++++++--- src/utils.rs | 7 +++++++ 3 files changed, 16 insertions(+), 5 deletions(-) diff --git a/src/chains.rs b/src/chains.rs index 81be8d541287..d7126b2f5e79 100644 --- a/src/chains.rs +++ b/src/chains.rs @@ -398,8 +398,8 @@ impl <'a> ChainFormatterShared<'a> { if all_in_one_line || extendable { // First we try to 'overflow' the last child and see if it looks better than using // vertical layout. - if let Some(shape) = last_shape.offset_left(almost_total) { - if let Some(rw) = last.rewrite_postfix(context, shape) { + if let Some(one_line_shape) = last_shape.offset_left(almost_total) { + if let Some(rw) = last.rewrite_postfix(context, one_line_shape) { // We allow overflowing here only if both of the following conditions match: // 1. The entire chain fits in a single line except the last child. // 2. `last_child_str.lines().count() >= 5`. @@ -413,6 +413,7 @@ impl <'a> ChainFormatterShared<'a> { // 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. + let last_shape = child_shape.sub_width(shape.rhs_overhead(context.config) + last.tries)?; match last.rewrite_postfix(context, last_shape) { Some(ref new_rw) if !could_fit_single_line => { last_subexpr_str = Some(new_rw.clone()); diff --git a/src/expr.rs b/src/expr.rs index 6e1af266e9bf..8aa5d9bd2864 100644 --- a/src/expr.rs +++ b/src/expr.rs @@ -39,9 +39,9 @@ use spanned::Spanned; use string::{rewrite_string, StringFormat}; use types::{can_be_overflowed_type, rewrite_path, PathContext}; use utils::{ - colon_spaces, contains_skip, count_newlines, first_line_width, inner_attributes, - last_line_extendable, last_line_width, mk_sp, outer_attributes, ptr_vec_to_ref_vec, - semicolon_for_stmt, wrap_str, + colon_spaces, contains_skip, count_newlines, first_line_ends_with, first_line_width, + inner_attributes, last_line_extendable, last_line_width, mk_sp, outer_attributes, + ptr_vec_to_ref_vec, semicolon_for_stmt, wrap_str, }; use vertical::rewrite_with_alignment; use visitor::FmtVisitor; @@ -1953,6 +1953,9 @@ pub fn prefer_next_line(orig_rhs: &str, next_line_rhs: &str, rhs_tactics: RhsTac rhs_tactics == RhsTactics::ForceNextLineWithoutIndent || !next_line_rhs.contains('\n') || count_newlines(orig_rhs) > count_newlines(next_line_rhs) + 1 + || first_line_ends_with(orig_rhs, '(') && !first_line_ends_with(next_line_rhs, '(') + || first_line_ends_with(orig_rhs, '{') && !first_line_ends_with(next_line_rhs, '{') + || first_line_ends_with(orig_rhs, '[') && !first_line_ends_with(next_line_rhs, '[') } fn rewrite_expr_addrof( diff --git a/src/utils.rs b/src/utils.rs index f1b0582b1200..b8474792cd30 100644 --- a/src/utils.rs +++ b/src/utils.rs @@ -382,6 +382,7 @@ pub fn colon_spaces(before: bool, after: bool) -> &'static str { } } +#[inline] pub fn left_most_sub_expr(e: &ast::Expr) -> &ast::Expr { match e.node { ast::ExprKind::Call(ref e, _) @@ -398,6 +399,12 @@ pub fn left_most_sub_expr(e: &ast::Expr) -> &ast::Expr { } } +#[inline] pub fn starts_with_newline(s: &str) -> bool { s.starts_with('\n') || s.starts_with("\r\n") } + +#[inline] +pub fn first_line_ends_with(s: &str, c: char) -> bool { + s.lines().next().map_or(false, |l| l.ends_with(c)) +} From 481e85cc5847b02458288cc068816625a21dcfad Mon Sep 17 00:00:00 2001 From: Nick Cameron Date: Wed, 11 Jul 2018 21:35:10 +1200 Subject: [PATCH 14/19] formatting --- src/bin/main.rs | 3 +- src/cargo-fmt/main.rs | 3 +- src/chains.rs | 123 ++++++++++++++++++++++++++++------------ src/closures.rs | 10 +--- src/comment.rs | 12 ++-- src/config/options.rs | 3 +- src/expr.rs | 3 +- src/git-rustfmt/main.rs | 3 +- src/imports.rs | 3 +- src/items.rs | 22 ++++--- src/macros.rs | 38 ++++++------- src/reorder.rs | 19 +++---- src/rustfmt_diff.rs | 16 +++--- src/test/mod.rs | 9 ++- src/types.rs | 6 +- src/vertical.rs | 6 +- 16 files changed, 154 insertions(+), 125 deletions(-) diff --git a/src/bin/main.rs b/src/bin/main.rs index 5b6398f028ef..7767ea847646 100644 --- a/src/bin/main.rs +++ b/src/bin/main.rs @@ -422,8 +422,7 @@ fn determine_operation(matches: &Matches) -> Result { // we will do comparison later, so here tries to canonicalize first // to get the expected behavior. p.canonicalize().unwrap_or(p) - }) - .collect(); + }).collect(); Ok(Operation::Format { files, diff --git a/src/cargo-fmt/main.rs b/src/cargo-fmt/main.rs index 3246908e20c1..2d8234ef41e3 100644 --- a/src/cargo-fmt/main.rs +++ b/src/cargo-fmt/main.rs @@ -163,8 +163,7 @@ fn format_crate( if verbosity == Verbosity::Verbose { println!("[{}] {:?}", t.kind, t.path) } - }) - .map(|t| t.path) + }).map(|t| t.path) .collect(); run_rustfmt(&files, &rustfmt_args, verbosity) diff --git a/src/chains.rs b/src/chains.rs index d7126b2f5e79..439c0d188a26 100644 --- a/src/chains.rs +++ b/src/chains.rs @@ -113,11 +113,7 @@ impl Rewrite for ChainItem { impl ChainItem { // Rewrite the last element in the chain `expr`. E.g., given `a.b.c` we rewrite // `.c` and any trailing `?`s. - fn rewrite_postfix( - &self, - context: &RewriteContext, - shape: Shape, - ) -> Option { + fn rewrite_postfix(&self, context: &RewriteContext, shape: Shape) -> Option { let shape = shape.sub_width(self.tries)?; let mut rewrite = match self.expr.node { ast::ExprKind::MethodCall(ref segment, ref expressions) => { @@ -128,14 +124,22 @@ impl ChainItem { }, _ => &[], }; - Self::rewrite_method_call(segment.ident, types, expressions, self.expr.span, context, shape)? + Self::rewrite_method_call( + segment.ident, + types, + expressions, + self.expr.span, + context, + shape, + )? } ast::ExprKind::Field(ref nested, ref field) => { - let space = if Self::is_tup_field_access(&self.expr) && Self::is_tup_field_access(nested) { - " " - } else { - "" - }; + let space = + if Self::is_tup_field_access(&self.expr) && Self::is_tup_field_access(nested) { + " " + } else { + "" + }; let result = format!("{}.{}", space, field.name); if result.len() <= shape.width { result @@ -296,10 +300,20 @@ trait ChainFormatter { // .baz() // ``` // If `bar` were not part of the root, then baz would be orphaned and 'float'. - fn format_root(&mut self, parent: &ChainItem, context: &RewriteContext, shape: Shape) -> Option<()>; + fn format_root( + &mut self, + parent: &ChainItem, + context: &RewriteContext, + shape: Shape, + ) -> Option<()>; fn child_shape(&self, context: &RewriteContext, shape: Shape) -> Shape; fn format_children(&mut self, context: &RewriteContext, child_shape: Shape) -> Option<()>; - fn format_last_child(&mut self, context: &RewriteContext, shape: Shape, child_shape: Shape) -> Option<()>; + fn format_last_child( + &mut self, + context: &RewriteContext, + shape: Shape, + child_shape: Shape, + ) -> Option<()>; fn join_rewrites(&self, context: &RewriteContext, child_shape: Shape) -> Option; // Returns `Some` if the chain is only a root, None otherwise. fn pure_root(&mut self) -> Option; @@ -309,7 +323,7 @@ trait ChainFormatter { // formatters can delegate much behaviour to `ChainFormatterShared`. struct ChainFormatterShared<'a> { // The current working set of child items. - children: &'a[ChainItem], + children: &'a [ChainItem], // The current rewrites of items (includes trailing `?`s, but not any way to // connect the rewrites together). rewrites: Vec, @@ -320,7 +334,7 @@ struct ChainFormatterShared<'a> { child_count: usize, } -impl <'a> ChainFormatterShared<'a> { +impl<'a> ChainFormatterShared<'a> { fn new(chain: &'a Chain) -> ChainFormatterShared<'a> { ChainFormatterShared { children: &chain.children, @@ -371,9 +385,16 @@ impl <'a> ChainFormatterShared<'a> { // result // }) // ``` - fn format_last_child(&mut self, may_extend: bool, context: &RewriteContext, shape: Shape, child_shape: Shape) -> Option<()> { + fn format_last_child( + &mut self, + may_extend: bool, + context: &RewriteContext, + shape: Shape, + child_shape: Shape, + ) -> Option<()> { let last = &self.children[0]; - let extendable = may_extend && last_line_extendable(&self.rewrites[self.rewrites.len() - 1]); + let extendable = + may_extend && last_line_extendable(&self.rewrites[self.rewrites.len() - 1]); // Total of all items excluding the last. let almost_total = if extendable { @@ -387,7 +408,8 @@ impl <'a> ChainFormatterShared<'a> { min(shape.width, context.config.width_heuristics().chain_width) }.saturating_sub(almost_total); - let all_in_one_line = self.rewrites.iter().all(|s| !s.contains('\n')) && one_line_budget > 0; + let all_in_one_line = + self.rewrites.iter().all(|s| !s.contains('\n')) && one_line_budget > 0; let last_shape = if all_in_one_line { shape.sub_width(last.tries)? } else { @@ -413,7 +435,8 @@ impl <'a> ChainFormatterShared<'a> { // 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. - let last_shape = child_shape.sub_width(shape.rhs_overhead(context.config) + last.tries)?; + let last_shape = child_shape + .sub_width(shape.rhs_overhead(context.config) + last.tries)?; match last.rewrite_postfix(context, last_shape) { Some(ref new_rw) if !could_fit_single_line => { last_subexpr_str = Some(new_rw.clone()); @@ -440,7 +463,12 @@ impl <'a> ChainFormatterShared<'a> { Some(()) } - fn join_rewrites(&self, context: &RewriteContext, child_shape: Shape, block_like_iter: impl Iterator) -> Option { + fn join_rewrites( + &self, + context: &RewriteContext, + child_shape: Shape, + block_like_iter: impl Iterator, + ) -> Option { let connector = if self.fits_single_line { // Yay, we can put everything on one line. Cow::from("") @@ -473,7 +501,7 @@ struct ChainFormatterBlock<'a> { is_block_like: Vec, } -impl <'a> ChainFormatterBlock<'a> { +impl<'a> ChainFormatterBlock<'a> { fn new(chain: &'a Chain) -> ChainFormatterBlock<'a> { ChainFormatterBlock { shared: ChainFormatterShared::new(chain), @@ -482,8 +510,13 @@ impl <'a> ChainFormatterBlock<'a> { } } -impl <'a> ChainFormatter for ChainFormatterBlock<'a> { - fn format_root(&mut self, parent: &ChainItem, context: &RewriteContext, shape: Shape) -> Option<()> { +impl<'a> ChainFormatter for ChainFormatterBlock<'a> { + fn format_root( + &mut self, + parent: &ChainItem, + context: &RewriteContext, + shape: Shape, + ) -> Option<()> { let mut root_rewrite: String = parent.rewrite(context, shape)?; let mut root_ends_with_block = is_block_expr(context, &parent.expr, &root_rewrite); @@ -520,18 +553,26 @@ impl <'a> ChainFormatter for ChainFormatterBlock<'a> { fn format_children(&mut self, context: &RewriteContext, child_shape: Shape) -> Option<()> { for item in self.shared.children[1..].iter().rev() { let rewrite = item.rewrite_postfix(context, child_shape)?; - self.is_block_like.push(is_block_expr(context, &item.expr, &rewrite)); + self.is_block_like + .push(is_block_expr(context, &item.expr, &rewrite)); self.shared.rewrites.push(rewrite); } Some(()) } - fn format_last_child(&mut self, context: &RewriteContext, shape: Shape, child_shape: Shape) -> Option<()> { - self.shared.format_last_child(true, context, shape, child_shape) + fn format_last_child( + &mut self, + context: &RewriteContext, + shape: Shape, + child_shape: Shape, + ) -> Option<()> { + self.shared + .format_last_child(true, context, shape, child_shape) } fn join_rewrites(&self, context: &RewriteContext, child_shape: Shape) -> Option { - self.shared.join_rewrites(context, child_shape, self.is_block_like.iter().cloned()) + self.shared + .join_rewrites(context, child_shape, self.is_block_like.iter().cloned()) } fn pure_root(&mut self) -> Option { @@ -552,8 +593,13 @@ impl<'a> ChainFormatterVisual<'a> { } } -impl <'a> ChainFormatter for ChainFormatterVisual<'a> { - fn format_root(&mut self, parent: &ChainItem, context: &RewriteContext, shape: Shape) -> Option<()> { +impl<'a> ChainFormatter for ChainFormatterVisual<'a> { + fn format_root( + &mut self, + parent: &ChainItem, + context: &RewriteContext, + shape: Shape, + ) -> Option<()> { // Determines if we can continue formatting a given expression on the same line. fn is_continuable(expr: &ast::Expr) -> bool { match expr.node { @@ -596,12 +642,19 @@ impl <'a> ChainFormatter for ChainFormatterVisual<'a> { Some(()) } - fn format_last_child(&mut self, context: &RewriteContext, shape: Shape, child_shape: Shape) -> Option<()> { - self.shared.format_last_child(false, context, shape, child_shape) + fn format_last_child( + &mut self, + context: &RewriteContext, + shape: Shape, + child_shape: Shape, + ) -> Option<()> { + self.shared + .format_last_child(false, context, shape, child_shape) } fn join_rewrites(&self, context: &RewriteContext, child_shape: Shape) -> Option { - self.shared.join_rewrites(context, child_shape, iter::repeat(false)) + self.shared + .join_rewrites(context, child_shape, iter::repeat(false)) } fn pure_root(&mut self) -> Option { @@ -613,9 +666,7 @@ impl <'a> ChainFormatter for ChainFormatterVisual<'a> { // parens, braces, and brackets in its idiomatic formatting. fn is_block_expr(context: &RewriteContext, expr: &ast::Expr, repr: &str) -> bool { match expr.node { - ast::ExprKind::Mac(..) - | ast::ExprKind::Call(..) - | ast::ExprKind::MethodCall(..) => { + ast::ExprKind::Mac(..) | ast::ExprKind::Call(..) | ast::ExprKind::MethodCall(..) => { context.use_block_indent() && repr.contains('\n') } ast::ExprKind::Struct(..) @@ -631,7 +682,7 @@ fn is_block_expr(context: &RewriteContext, expr: &ast::Expr, repr: &str) -> bool | ast::ExprKind::Binary(_, _, ref expr) | ast::ExprKind::Index(_, ref expr) | ast::ExprKind::Unary(_, ref expr) - | ast::ExprKind::Closure(_, _, _, _, ref expr, _) + | ast::ExprKind::Closure(_, _, _, _, ref expr, _) | ast::ExprKind::Try(ref expr) | ast::ExprKind::Yield(Some(ref expr)) => is_block_expr(context, expr, repr), _ => false, diff --git a/src/closures.rs b/src/closures.rs index 783ca1c029b1..128e1dc8e0b6 100644 --- a/src/closures.rs +++ b/src/closures.rs @@ -198,8 +198,7 @@ fn rewrite_closure_expr( } else { Some(rw) } - }) - .map(|rw| format!("{} {}", prefix, rw)) + }).map(|rw| format!("{} {}", prefix, rw)) } // Rewrite closure whose body is block. @@ -376,11 +375,8 @@ where .map(|e| match e.node { ast::ExprKind::Closure(..) => true, _ => false, - }) - .unwrap_or(false) - }) - .count() - > 1 + }).unwrap_or(false) + }).count() > 1 } fn is_block_closure_forced(context: &RewriteContext, expr: &ast::Expr) -> bool { diff --git a/src/comment.rs b/src/comment.rs index 3d55f6560e3e..ed83a3925b00 100644 --- a/src/comment.rs +++ b/src/comment.rs @@ -348,8 +348,7 @@ fn rewrite_comment_inner( } line - }) - .map(|s| left_trim_comment_line(s, &style)) + }).map(|s| left_trim_comment_line(s, &style)) .map(|(line, has_leading_whitespace)| { if orig.starts_with("/*") && line_breaks == 0 { ( @@ -517,8 +516,7 @@ fn trim_custom_comment_prefix(s: &str) -> String { } else { line } - }) - .collect::>() + }).collect::>() .join("\n") } @@ -606,8 +604,7 @@ fn light_rewrite_comment( }; // Preserve markdown's double-space line break syntax in doc comment. trim_right_unless_two_whitespaces(left_trimmed, is_doc_comment) - }) - .collect(); + }).collect(); Some(lines.join(&format!("\n{}", offset.to_string(config)))) } @@ -1341,8 +1338,7 @@ mod test { .filter_map(|(s, c)| match s { FullCodeCharKind::Normal | FullCodeCharKind::InString => Some(c), _ => None, - }) - .collect() + }).collect() } #[test] diff --git a/src/config/options.rs b/src/config/options.rs index a581cdef43df..c48f9a4d7272 100644 --- a/src/config/options.rs +++ b/src/config/options.rs @@ -322,8 +322,7 @@ impl IgnoreList { path.push(s); path } - }) - .collect(); + }).collect(); } fn skip_file_inner(&self, file: &Path) -> bool { diff --git a/src/expr.rs b/src/expr.rs index 8aa5d9bd2864..6814b91e0532 100644 --- a/src/expr.rs +++ b/src/expr.rs @@ -1212,8 +1212,7 @@ fn rewrite_string_lit(context: &RewriteContext, span: Span, shape: Shape) -> Opt new_indent.to_string(context.config), line.trim_left() ) - }) - .collect::>() + }).collect::>() .join("\n") .trim_left(), ); diff --git a/src/git-rustfmt/main.rs b/src/git-rustfmt/main.rs index d6ab36614824..d47827ab55e1 100644 --- a/src/git-rustfmt/main.rs +++ b/src/git-rustfmt/main.rs @@ -46,8 +46,7 @@ fn prune_files(files: Vec<&str>) -> Vec<&str> { return true; } pruned_prefixes.iter().all(|pp| !f.starts_with(pp)) - }) - .collect() + }).collect() } fn git_diff(commits: &str) -> String { diff --git a/src/imports.rs b/src/imports.rs index f087ea5e52a8..93f2ac8867a0 100644 --- a/src/imports.rs +++ b/src/imports.rs @@ -344,8 +344,7 @@ impl UseTree { .zip(items.into_iter()) .map(|(t, list_item)| { Self::from_ast(context, &t.0, Some(list_item), None, None, None) - }) - .collect(), + }).collect(), )); } UseTreeKind::Simple(ref rename, ..) => { diff --git a/src/items.rs b/src/items.rs index 721f02b01878..23338435d9ac 100644 --- a/src/items.rs +++ b/src/items.rs @@ -1669,7 +1669,7 @@ fn rewrite_static( &**expr, Shape::legacy(remaining_width, offset.block_only()), ).and_then(|res| recover_comment_removed(res, static_parts.span, context)) - .map(|s| if s.ends_with(';') { s } else { s + ";" }) + .map(|s| if s.ends_with(';') { s } else { s + ";" }) } else { Some(format!("{}{};", prefix, ty_str)) } @@ -2783,17 +2783,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. diff --git a/src/macros.rs b/src/macros.rs index 4f3891fbf495..91c29958ba16 100644 --- a/src/macros.rs +++ b/src/macros.rs @@ -1115,26 +1115,25 @@ fn indent_macro_snippet( }; trimmed_lines.push((trimmed, line, prefix_space_width)); prefix_space_width - }) - .min()?; + }).min()?; Some( - first_line + "\n" + &trimmed_lines - .iter() - .map( - |&(trimmed, ref line, prefix_space_width)| match prefix_space_width { - _ if !trimmed => line.to_owned(), - Some(original_indent_width) => { - let new_indent_width = indent.width() + original_indent_width - .saturating_sub(min_prefix_space_width); - let new_indent = Indent::from_width(context.config, new_indent_width); - format!("{}{}", new_indent.to_string(context.config), line.trim()) - } - None => String::new(), - }, - ) - .collect::>() - .join("\n"), + first_line + "\n" + + &trimmed_lines + .iter() + .map( + |&(trimmed, ref line, prefix_space_width)| match prefix_space_width { + _ if !trimmed => line.to_owned(), + Some(original_indent_width) => { + let new_indent_width = indent.width() + + original_indent_width.saturating_sub(min_prefix_space_width); + let new_indent = Indent::from_width(context.config, new_indent_width); + format!("{}{}", new_indent.to_string(context.config), line.trim()) + } + None => String::new(), + }, + ).collect::>() + .join("\n"), ) } @@ -1296,8 +1295,7 @@ impl MacroBranch { } (s + l + "\n", !kind.is_string() || l.ends_with('\\')) }, - ) - .0; + ).0; // Undo our replacement of macro variables. // FIXME: this could be *much* more efficient. diff --git a/src/reorder.rs b/src/reorder.rs index 5947b4ae87ff..df32ab47b2fd 100644 --- a/src/reorder.rs +++ b/src/reorder.rs @@ -151,8 +151,7 @@ fn rewrite_reorderable_items( .map(|use_tree| ListItem { item: use_tree.rewrite_top_level(context, nested_shape), ..use_tree.list_item.unwrap_or_else(ListItem::empty) - }) - .collect(); + }).collect(); wrap_reorderable_items(context, &item_vec, nested_shape) } @@ -242,15 +241,13 @@ impl<'b, 'a: 'b> FmtVisitor<'a> { let item_length = items .iter() .take_while(|ppi| { - item_kind.is_same_item_kind(&***ppi) - && (!in_group || { - let current = self.codemap.lookup_line_range(ppi.span()); - let in_same_group = current.lo < last.hi + 2; - last = current; - in_same_group - }) - }) - .count(); + item_kind.is_same_item_kind(&***ppi) && (!in_group || { + let current = self.codemap.lookup_line_range(ppi.span()); + let in_same_group = current.lo < last.hi + 2; + last = current; + in_same_group + }) + }).count(); let items = &items[..item_length]; let at_least_one_in_file_lines = items diff --git a/src/rustfmt_diff.rs b/src/rustfmt_diff.rs index de52b28c08bb..2a331eec19ae 100644 --- a/src/rustfmt_diff.rs +++ b/src/rustfmt_diff.rs @@ -193,13 +193,15 @@ where W: Write, { for mismatch in diff { - let (num_removed, num_added) = mismatch.lines.iter().fold((0, 0), |(rem, add), line| { - match *line { - DiffLine::Context(_) => panic!("No Context expected"), - DiffLine::Expected(_) => (rem, add + 1), - DiffLine::Resulting(_) => (rem + 1, add), - } - }); + let (num_removed, num_added) = + mismatch + .lines + .iter() + .fold((0, 0), |(rem, add), line| match *line { + DiffLine::Context(_) => panic!("No Context expected"), + DiffLine::Expected(_) => (rem, add + 1), + DiffLine::Resulting(_) => (rem + 1, add), + }); // Write a header with enough information to separate the modified lines. writeln!( out, diff --git a/src/test/mod.rs b/src/test/mod.rs index 63b5f244b906..ba4607be37ce 100644 --- a/src/test/mod.rs +++ b/src/test/mod.rs @@ -514,8 +514,7 @@ fn read_significant_comments(file_name: &Path) -> HashMap { .to_owned(), ) }) - }) - .collect() + }).collect() } // Compare output to input. @@ -884,8 +883,8 @@ fn configuration_snippet_tests() { fs::File::open(Path::new(CONFIGURATIONS_FILE_NAME)) .expect(&format!("Couldn't read file {}", CONFIGURATIONS_FILE_NAME)), ).lines() - .map(|l| l.unwrap()) - .enumerate(); + .map(|l| l.unwrap()) + .enumerate(); let mut code_blocks: Vec = Vec::new(); let mut hash_set = Config::hash_set(); @@ -961,5 +960,5 @@ fn verify_check_works() { "--check", temp_file.path.to_str().unwrap(), ]).succeeds() - .unwrap(); + .unwrap(); } diff --git a/src/types.rs b/src/types.rs index 0c978ea28a67..d6e4001eacfc 100644 --- a/src/types.rs +++ b/src/types.rs @@ -548,7 +548,8 @@ impl Rewrite for ast::GenericParam { }; result.push_str(eq_str); let budget = shape.width.checked_sub(result.len())?; - let rewrite = def.rewrite(context, Shape::legacy(budget, shape.indent + result.len()))?; + let rewrite = + def.rewrite(context, Shape::legacy(budget, shape.indent + result.len()))?; result.push_str(&rewrite); } @@ -793,8 +794,7 @@ fn rewrite_lifetime_param( .filter(|p| match p.kind { ast::GenericParamKind::Lifetime => true, _ => false, - }) - .map(|lt| lt.rewrite(context, shape)) + }).map(|lt| lt.rewrite(context, shape)) .collect::>>()? .join(", "); if result.is_empty() { diff --git a/src/vertical.rs b/src/vertical.rs index 1b7fdb9be1d1..ead5719f61f8 100644 --- a/src/vertical.rs +++ b/src/vertical.rs @@ -200,14 +200,12 @@ fn struct_field_prefix_max_min_width( Some(field_str.len()) } }) - }) - .fold(Some((0, ::std::usize::MAX)), |acc, len| match (acc, len) { + }).fold(Some((0, ::std::usize::MAX)), |acc, len| match (acc, len) { (Some((max_len, min_len)), Some(len)) => { Some((cmp::max(max_len, len), cmp::min(min_len, len))) } _ => None, - }) - .unwrap_or((0, 0)) + }).unwrap_or((0, 0)) } fn rewrite_aligned_items_inner( From 8618a558342e98b57f5b2e4697c615b1d0f9b735 Mon Sep 17 00:00:00 2001 From: Nick Cameron Date: Wed, 11 Jul 2018 22:06:30 +1200 Subject: [PATCH 15/19] chains: treat some string lits as blocks --- src/chains.rs | 81 ++++++++++++++++++++++++++------------------------- 1 file changed, 42 insertions(+), 39 deletions(-) diff --git a/src/chains.rs b/src/chains.rs index 439c0d188a26..67390bc6b7cf 100644 --- a/src/chains.rs +++ b/src/chains.rs @@ -71,7 +71,10 @@ use macros::convert_try_mac; use rewrite::{Rewrite, RewriteContext}; use shape::Shape; use spanned::Spanned; -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::borrow::Cow; use std::cmp::min; @@ -395,10 +398,11 @@ impl<'a> ChainFormatterShared<'a> { let last = &self.children[0]; let extendable = may_extend && last_line_extendable(&self.rewrites[self.rewrites.len() - 1]); + let prev_last_line_width = last_line_width(&self.rewrites[self.rewrites.len() - 1]); // Total of all items excluding the last. let almost_total = if extendable { - last_line_width(&self.rewrites[self.rewrites.len() - 1]) + prev_last_line_width } else { self.rewrites.iter().fold(0, |a, b| a + b.len()) } + last.tries; @@ -410,7 +414,7 @@ impl<'a> ChainFormatterShared<'a> { let all_in_one_line = self.rewrites.iter().all(|s| !s.contains('\n')) && one_line_budget > 0; - let last_shape = if all_in_one_line { + let last_shape = if all_in_one_line || extendable { shape.sub_width(last.tries)? } else { child_shape.sub_width(shape.rhs_overhead(context.config) + last.tries)? @@ -508,6 +512,37 @@ impl<'a> ChainFormatterBlock<'a> { is_block_like: Vec::with_capacity(chain.children.len() + 1), } } + + // States whether an expression's last line exclusively consists of closing + // parens, braces, and brackets in its idiomatic formatting. + fn is_block_expr(context: &RewriteContext, expr: &ast::Expr, repr: &str) -> bool { + match expr.node { + ast::ExprKind::Mac(..) + | ast::ExprKind::Call(..) + | ast::ExprKind::MethodCall(..) + | ast::ExprKind::Struct(..) + | ast::ExprKind::While(..) + | ast::ExprKind::WhileLet(..) + | ast::ExprKind::If(..) + | ast::ExprKind::IfLet(..) + | ast::ExprKind::Block(..) + | ast::ExprKind::Loop(..) + | ast::ExprKind::ForLoop(..) + | ast::ExprKind::Match(..) => repr.contains('\n'), + ast::ExprKind::Paren(ref expr) + | ast::ExprKind::Binary(_, _, ref expr) + | ast::ExprKind::Index(_, ref expr) + | ast::ExprKind::Unary(_, ref expr) + | ast::ExprKind::Closure(_, _, _, _, ref expr, _) + | ast::ExprKind::Try(ref expr) + | ast::ExprKind::Yield(Some(ref expr)) => Self::is_block_expr(context, expr, repr), + // This can only be a string lit + ast::ExprKind::Lit(_) => { + repr.contains('\n') && trimmed_last_line_width(repr) <= context.config.tab_spaces() + } + _ => false, + } + } } impl<'a> ChainFormatter for ChainFormatterBlock<'a> { @@ -519,7 +554,7 @@ impl<'a> ChainFormatter for ChainFormatterBlock<'a> { ) -> Option<()> { let mut root_rewrite: String = parent.rewrite(context, shape)?; - let mut root_ends_with_block = is_block_expr(context, &parent.expr, &root_rewrite); + let mut root_ends_with_block = Self::is_block_expr(context, &parent.expr, &root_rewrite); let tab_width = context.config.tab_spaces().saturating_sub(shape.offset); while root_rewrite.len() <= tab_width && !root_rewrite.contains('\n') { @@ -530,7 +565,7 @@ impl<'a> ChainFormatter for ChainFormatterBlock<'a> { None => break, } - root_ends_with_block = is_block_expr(context, &item.expr, &root_rewrite); + root_ends_with_block = Self::is_block_expr(context, &item.expr, &root_rewrite); self.shared.children = &self.shared.children[..self.shared.children.len() - 1]; if self.shared.children.is_empty() { @@ -554,7 +589,7 @@ impl<'a> ChainFormatter for ChainFormatterBlock<'a> { for item in self.shared.children[1..].iter().rev() { let rewrite = item.rewrite_postfix(context, child_shape)?; self.is_block_like - .push(is_block_expr(context, &item.expr, &rewrite)); + .push(Self::is_block_expr(context, &item.expr, &rewrite)); self.shared.rewrites.push(rewrite); } Some(()) @@ -608,12 +643,7 @@ impl<'a> ChainFormatter for ChainFormatterVisual<'a> { } } - // Parent is the first item in the chain, e.g., `foo` in `foo.bar.baz()`. - let parent_shape = if is_block_expr(context, &parent.expr, "\n") { - shape.visual_indent(0) - } else { - shape - }; + let parent_shape = shape.visual_indent(0); let mut root_rewrite = parent.rewrite(context, parent_shape)?; if !root_rewrite.contains('\n') && is_continuable(&parent.expr) { @@ -661,30 +691,3 @@ impl<'a> ChainFormatter for ChainFormatterVisual<'a> { self.shared.pure_root() } } - -// States whether an expression's last line exclusively consists of closing -// parens, braces, and brackets in its idiomatic formatting. -fn is_block_expr(context: &RewriteContext, expr: &ast::Expr, repr: &str) -> bool { - match expr.node { - ast::ExprKind::Mac(..) | ast::ExprKind::Call(..) | ast::ExprKind::MethodCall(..) => { - context.use_block_indent() && repr.contains('\n') - } - ast::ExprKind::Struct(..) - | ast::ExprKind::While(..) - | ast::ExprKind::WhileLet(..) - | ast::ExprKind::If(..) - | ast::ExprKind::IfLet(..) - | ast::ExprKind::Block(..) - | ast::ExprKind::Loop(..) - | ast::ExprKind::ForLoop(..) - | ast::ExprKind::Match(..) => repr.contains('\n'), - ast::ExprKind::Paren(ref expr) - | ast::ExprKind::Binary(_, _, ref expr) - | ast::ExprKind::Index(_, ref expr) - | ast::ExprKind::Unary(_, ref expr) - | ast::ExprKind::Closure(_, _, _, _, ref expr, _) - | ast::ExprKind::Try(ref expr) - | ast::ExprKind::Yield(Some(ref expr)) => is_block_expr(context, expr, repr), - _ => false, - } -} From f9510a55eb952f078764f3caa8a8415bfbf9d352 Mon Sep 17 00:00:00 2001 From: Nick Cameron Date: Thu, 12 Jul 2018 10:44:06 +1200 Subject: [PATCH 16/19] chains: fix visual indent chain layout --- src/chains.rs | 140 ++++++++++++++++++++++++++++---------------------- src/shape.rs | 6 +++ 2 files changed, 85 insertions(+), 61 deletions(-) diff --git a/src/chains.rs b/src/chains.rs index 67390bc6b7cf..45b45b16732a 100644 --- a/src/chains.rs +++ b/src/chains.rs @@ -275,12 +275,12 @@ impl Rewrite for Chain { }; formatter.format_root(&self.parent, context, shape)?; - if let result @ Some(_) = formatter.pure_root() { - return result; + if let Some(result) = formatter.pure_root() { + return wrap_str(result, context.config.max_width(), shape); } // Decide how to layout the rest of the chain. - let child_shape = formatter.child_shape(context, shape); + let child_shape = formatter.child_shape(context, shape)?; formatter.format_children(context, child_shape)?; formatter.format_last_child(context, shape, child_shape)?; @@ -309,7 +309,7 @@ trait ChainFormatter { context: &RewriteContext, shape: Shape, ) -> Option<()>; - fn child_shape(&self, context: &RewriteContext, shape: Shape) -> Shape; + fn child_shape(&self, context: &RewriteContext, shape: Shape) -> Option; fn format_children(&mut self, context: &RewriteContext, child_shape: Shape) -> Option<()>; fn format_last_child( &mut self, @@ -414,8 +414,10 @@ impl<'a> ChainFormatterShared<'a> { let all_in_one_line = self.rewrites.iter().all(|s| !s.contains('\n')) && one_line_budget > 0; - let last_shape = if all_in_one_line || extendable { + let last_shape = if all_in_one_line { shape.sub_width(last.tries)? + } else if extendable { + child_shape.sub_width(last.tries)? } else { child_shape.sub_width(shape.rhs_overhead(context.config) + last.tries)? }; @@ -481,7 +483,7 @@ impl<'a> ChainFormatterShared<'a> { if *context.force_one_line_chain.borrow() { return None; } - child_shape.indent.to_string_with_newline(context.config) + child_shape.to_string_with_newline(context.config) }; let mut rewrite_iter = self.rewrites.iter(); @@ -512,37 +514,6 @@ impl<'a> ChainFormatterBlock<'a> { is_block_like: Vec::with_capacity(chain.children.len() + 1), } } - - // States whether an expression's last line exclusively consists of closing - // parens, braces, and brackets in its idiomatic formatting. - fn is_block_expr(context: &RewriteContext, expr: &ast::Expr, repr: &str) -> bool { - match expr.node { - ast::ExprKind::Mac(..) - | ast::ExprKind::Call(..) - | ast::ExprKind::MethodCall(..) - | ast::ExprKind::Struct(..) - | ast::ExprKind::While(..) - | ast::ExprKind::WhileLet(..) - | ast::ExprKind::If(..) - | ast::ExprKind::IfLet(..) - | ast::ExprKind::Block(..) - | ast::ExprKind::Loop(..) - | ast::ExprKind::ForLoop(..) - | ast::ExprKind::Match(..) => repr.contains('\n'), - ast::ExprKind::Paren(ref expr) - | ast::ExprKind::Binary(_, _, ref expr) - | ast::ExprKind::Index(_, ref expr) - | ast::ExprKind::Unary(_, ref expr) - | ast::ExprKind::Closure(_, _, _, _, ref expr, _) - | ast::ExprKind::Try(ref expr) - | ast::ExprKind::Yield(Some(ref expr)) => Self::is_block_expr(context, expr, repr), - // This can only be a string lit - ast::ExprKind::Lit(_) => { - repr.contains('\n') && trimmed_last_line_width(repr) <= context.config.tab_spaces() - } - _ => false, - } - } } impl<'a> ChainFormatter for ChainFormatterBlock<'a> { @@ -554,7 +525,7 @@ impl<'a> ChainFormatter for ChainFormatterBlock<'a> { ) -> Option<()> { let mut root_rewrite: String = parent.rewrite(context, shape)?; - let mut root_ends_with_block = Self::is_block_expr(context, &parent.expr, &root_rewrite); + let mut root_ends_with_block = is_block_expr(context, &parent.expr, &root_rewrite); let tab_width = context.config.tab_spaces().saturating_sub(shape.offset); while root_rewrite.len() <= tab_width && !root_rewrite.contains('\n') { @@ -565,7 +536,7 @@ impl<'a> ChainFormatter for ChainFormatterBlock<'a> { None => break, } - root_ends_with_block = Self::is_block_expr(context, &item.expr, &root_rewrite); + root_ends_with_block = is_block_expr(context, &item.expr, &root_rewrite); self.shared.children = &self.shared.children[..self.shared.children.len() - 1]; if self.shared.children.is_empty() { @@ -577,19 +548,21 @@ impl<'a> ChainFormatter for ChainFormatterBlock<'a> { Some(()) } - fn child_shape(&self, context: &RewriteContext, shape: Shape) -> Shape { - if self.is_block_like[0] { - shape - } else { - shape.block_indent(context.config.tab_spaces()) - }.with_max_width(context.config) + fn child_shape(&self, context: &RewriteContext, shape: Shape) -> Option { + Some( + if self.is_block_like[0] { + shape.block_indent(0) + } else { + shape.block_indent(context.config.tab_spaces()) + }.with_max_width(context.config), + ) } fn format_children(&mut self, context: &RewriteContext, child_shape: Shape) -> Option<()> { for item in self.shared.children[1..].iter().rev() { let rewrite = item.rewrite_postfix(context, child_shape)?; self.is_block_like - .push(Self::is_block_expr(context, &item.expr, &rewrite)); + .push(is_block_expr(context, &item.expr, &rewrite)); self.shared.rewrites.push(rewrite); } Some(()) @@ -618,12 +591,15 @@ impl<'a> ChainFormatter for ChainFormatterBlock<'a> { // Format a chain using visual indent. struct ChainFormatterVisual<'a> { shared: ChainFormatterShared<'a>, + // The extra offset from the chain's shape to the position of the `.` + offset: usize, } impl<'a> ChainFormatterVisual<'a> { fn new(chain: &'a Chain) -> ChainFormatterVisual<'a> { ChainFormatterVisual { shared: ChainFormatterShared::new(chain), + offset: 0, } } } @@ -635,23 +611,31 @@ impl<'a> ChainFormatter for ChainFormatterVisual<'a> { context: &RewriteContext, shape: Shape, ) -> Option<()> { - // Determines if we can continue formatting a given expression on the same line. - fn is_continuable(expr: &ast::Expr) -> bool { - match expr.node { - ast::ExprKind::Path(..) => true, - _ => false, - } - } - let parent_shape = shape.visual_indent(0); let mut root_rewrite = parent.rewrite(context, parent_shape)?; + let multiline = root_rewrite.contains('\n'); + self.offset = if multiline { + last_line_width(&root_rewrite).saturating_sub(shape.used_width()) + } else { + trimmed_last_line_width(&root_rewrite) + }; - if !root_rewrite.contains('\n') && is_continuable(&parent.expr) { + if !multiline || is_block_expr(context, &parent.expr, &root_rewrite) { let item = &self.shared.children[self.shared.children.len() - 1]; - let overhead = last_line_width(&root_rewrite); - let shape = parent_shape.offset_left(overhead)?; - let rewrite = item.rewrite_postfix(context, shape)?; - root_rewrite.push_str(&rewrite); + let child_shape = parent_shape + .visual_indent(self.offset) + .sub_width(self.offset)?; + let rewrite = item.rewrite_postfix(context, child_shape)?; + match wrap_str(rewrite, context.config.max_width(), shape) { + Some(rewrite) => root_rewrite.push_str(&rewrite), + None => { + // We couldn't fit in at the visual indent, try the last + // indent. + let rewrite = item.rewrite_postfix(context, parent_shape)?; + root_rewrite.push_str(&rewrite); + self.offset = 0; + } + } self.shared.children = &self.shared.children[..self.shared.children.len() - 1]; } @@ -660,8 +644,11 @@ impl<'a> ChainFormatter for ChainFormatterVisual<'a> { Some(()) } - fn child_shape(&self, context: &RewriteContext, shape: Shape) -> Shape { - shape.visual_indent(0).with_max_width(context.config) + fn child_shape(&self, context: &RewriteContext, shape: Shape) -> Option { + shape + .with_max_width(context.config) + .offset_left(self.offset) + .map(|s| s.visual_indent(0)) } fn format_children(&mut self, context: &RewriteContext, child_shape: Shape) -> Option<()> { @@ -691,3 +678,34 @@ impl<'a> ChainFormatter for ChainFormatterVisual<'a> { self.shared.pure_root() } } + +// States whether an expression's last line exclusively consists of closing +// parens, braces, and brackets in its idiomatic formatting. +fn is_block_expr(context: &RewriteContext, expr: &ast::Expr, repr: &str) -> bool { + match expr.node { + ast::ExprKind::Mac(..) + | ast::ExprKind::Call(..) + | ast::ExprKind::MethodCall(..) + | ast::ExprKind::Struct(..) + | ast::ExprKind::While(..) + | ast::ExprKind::WhileLet(..) + | ast::ExprKind::If(..) + | ast::ExprKind::IfLet(..) + | ast::ExprKind::Block(..) + | ast::ExprKind::Loop(..) + | ast::ExprKind::ForLoop(..) + | ast::ExprKind::Match(..) => repr.contains('\n'), + ast::ExprKind::Paren(ref expr) + | ast::ExprKind::Binary(_, _, ref expr) + | ast::ExprKind::Index(_, ref expr) + | ast::ExprKind::Unary(_, ref expr) + | ast::ExprKind::Closure(_, _, _, _, ref expr, _) + | ast::ExprKind::Try(ref expr) + | ast::ExprKind::Yield(Some(ref expr)) => is_block_expr(context, expr, repr), + // This can only be a string lit + ast::ExprKind::Lit(_) => { + repr.contains('\n') && trimmed_last_line_width(repr) <= context.config.tab_spaces() + } + _ => false, + } +} diff --git a/src/shape.rs b/src/shape.rs index ab33826c028f..12a911a19315 100644 --- a/src/shape.rs +++ b/src/shape.rs @@ -274,6 +274,12 @@ impl Shape { ); Shape { width, ..*self } } + + pub fn to_string_with_newline(&self, config: &Config) -> Cow<'static, str> { + let mut offset_indent = self.indent; + offset_indent.alignment = self.offset; + offset_indent.to_string_inner(config, 0) + } } #[cfg(test)] From 4fa2969c39761fccf4f44488ac7d2000549b273a Mon Sep 17 00:00:00 2001 From: Nick Cameron Date: Thu, 12 Jul 2018 14:05:50 +1200 Subject: [PATCH 17/19] fixup tests --- tests/target/chains-visual.rs | 30 +++++++++---------- tests/target/chains.rs | 19 +++++------- tests/target/closure.rs | 29 +++++++++--------- .../target/configs/indent_style/block_call.rs | 3 +- tests/target/expr-block.rs | 3 +- tests/target/file-lines-1.rs | 2 +- tests/target/file-lines-3.rs | 2 +- tests/target/issue-2759.rs | 4 +-- tests/target/macros.rs | 3 +- tests/target/match.rs | 3 +- 10 files changed, 45 insertions(+), 53 deletions(-) diff --git a/tests/target/chains-visual.rs b/tests/target/chains-visual.rs index 3db40053e6b1..ab49096e55ab 100644 --- a/tests/target/chains-visual.rs +++ b/tests/target/chains-visual.rs @@ -15,16 +15,16 @@ fn main() { // Test case where first chain element isn't a path, but is shorter than // the size of a tab. x().y(|| match cond() { - true => (), - false => (), - }); + true => (), + false => (), + }); loong_func().quux(move || if true { 1 } else { 2 }); some_fuuuuuuuuunction().method_call_a(aaaaa, bbbbb, |c| { - let x = c; - x - }); + let x = c; + x + }); some_fuuuuuuuuunction().method_call_a(aaaaa, bbbbb, |c| { let x = c; @@ -59,7 +59,7 @@ fn floaters() { let x = Foo { field1: val1, field2: val2, }.method_call() - .method_call(); + .method_call(); let y = if cond { val1 } else { val2 }.method_call(); @@ -80,7 +80,7 @@ fn floaters() { } else { none(); }.bar() - .baz(); + .baz(); Foo { x: val }.baz(|| { force(); @@ -90,10 +90,10 @@ fn floaters() { Foo { y: i_am_multi_line, z: ok, }.baz(|| { - force(); - multiline(); - }) - .quux(); + force(); + multiline(); + }) + .quux(); a + match x { true => "yay!", @@ -137,9 +137,9 @@ fn issue1434() { for _ in 0..100 { let prototype_id = PrototypeIdData::from_reader::<_, B>(&mut self.file_cursor).chain_err(|| { - format!("could not read prototype ID at offset {:#010x}", - current_offset) - })?; + format!("could not read prototype ID at offset {:#010x}", + current_offset) + })?; } } diff --git a/tests/target/chains.rs b/tests/target/chains.rs index 69c7671503d9..a328ef004bed 100644 --- a/tests/target/chains.rs +++ b/tests/target/chains.rs @@ -38,8 +38,7 @@ fn main() { .method_call_a(aaaaa, bbbbb, |c| { let x = c; x - }) - .method_call_b(aaaaa, bbbbb, |c| { + }).method_call_b(aaaaa, bbbbb, |c| { let x = c; x }); @@ -65,8 +64,7 @@ fn main() { .map(|x| { x += 1; x - }) - .filter(some_mod::some_filter) + }).filter(some_mod::some_filter) } fn floaters() { @@ -79,7 +77,7 @@ fn floaters() { field1: val1, field2: val2, }.method_call() - .method_call(); + .method_call(); let y = if cond { val1 @@ -106,15 +104,14 @@ fn floaters() { } else { none(); }.bar() - .baz(); + .baz(); Foo { x: val, }.baz(|| { force(); multiline(); - }) - .quux(); + }).quux(); Foo { y: i_am_multi_line, @@ -122,8 +119,7 @@ fn floaters() { }.baz(|| { force(); multiline(); - }) - .quux(); + }).quux(); a + match x { true => "yay!", @@ -238,8 +234,7 @@ impl Foo { } } }) - }) - .collect(); + }).collect(); } } diff --git a/tests/target/closure.rs b/tests/target/closure.rs index 5bc89d582229..2360ccfbd2fd 100644 --- a/tests/target/closure.rs +++ b/tests/target/closure.rs @@ -138,18 +138,20 @@ fn issue470() { { { { - let explicit_arg_decls = explicit_arguments.into_iter().enumerate().map( - |(index, (ty, pattern))| { - let lvalue = Lvalue::Arg(index as u32); - block = this.pattern( - block, - argument_extent, - hair::PatternRef::Hair(pattern), - &lvalue, - ); - ArgDecl { ty: ty } - }, - ); + let explicit_arg_decls = + explicit_arguments + .into_iter() + .enumerate() + .map(|(index, (ty, pattern))| { + let lvalue = Lvalue::Arg(index as u32); + block = this.pattern( + block, + argument_extent, + hair::PatternRef::Hair(pattern), + &lvalue, + ); + ArgDecl { ty: ty } + }); } } } @@ -169,8 +171,7 @@ fn issue1329() { .map(|x| { x += 1; x - }) - .filter + }).filter } fn issue325() { diff --git a/tests/target/configs/indent_style/block_call.rs b/tests/target/configs/indent_style/block_call.rs index 77f3c551f0a5..19a45cb9f74e 100644 --- a/tests/target/configs/indent_style/block_call.rs +++ b/tests/target/configs/indent_style/block_call.rs @@ -117,8 +117,7 @@ impl Cursor { debug_assert_eq!(n, -1); None } - }) - .or_else(|| { + }).or_else(|| { let canonical = self.canonical(); if canonical != *self { canonical.num_template_args() diff --git a/tests/target/expr-block.rs b/tests/target/expr-block.rs index 3e23d5ee20d7..501ee3eaea04 100644 --- a/tests/target/expr-block.rs +++ b/tests/target/expr-block.rs @@ -141,8 +141,7 @@ fn issue_1450() { Relaxed, Release, Relaxed, - ) - .is_ok() + ).is_ok() { return; } diff --git a/tests/target/file-lines-1.rs b/tests/target/file-lines-1.rs index 2601ca9fc94a..1c63b105b988 100644 --- a/tests/target/file-lines-1.rs +++ b/tests/target/file-lines-1.rs @@ -5,7 +5,7 @@ fn floaters() { field1: val1, field2: val2, }.method_call() - .method_call(); + .method_call(); let y = if cond { val1 diff --git a/tests/target/file-lines-3.rs b/tests/target/file-lines-3.rs index 4831e5164a80..9aba967e29dd 100644 --- a/tests/target/file-lines-3.rs +++ b/tests/target/file-lines-3.rs @@ -5,7 +5,7 @@ fn floaters() { field1: val1, field2: val2, }.method_call() - .method_call(); + .method_call(); let y = if cond { val1 } else { val2 }.method_call(); diff --git a/tests/target/issue-2759.rs b/tests/target/issue-2759.rs index 3685afbdc0e7..4441b140386e 100644 --- a/tests/target/issue-2759.rs +++ b/tests/target/issue-2759.rs @@ -56,8 +56,8 @@ fn bar() {} /// .boxed(), /// ] /// }).bind("127.0.0.1:8080") -/// .unwrap() -/// .run() +/// .unwrap() +/// .run() /// # }); /// } /// ``` diff --git a/tests/target/macros.rs b/tests/target/macros.rs index 00b91ca81a29..ac076615f99a 100644 --- a/tests/target/macros.rs +++ b/tests/target/macros.rs @@ -183,8 +183,7 @@ fn issue_1885() { chan_select! { rx.recv() => {} } - }) - .collect::>(); + }).collect::>(); } fn issue_1917() { diff --git a/tests/target/match.rs b/tests/target/match.rs index cda0ed2abe57..e0774ef011ca 100644 --- a/tests/target/match.rs +++ b/tests/target/match.rs @@ -381,8 +381,7 @@ fn issue1456() { .iter() .map(|node| { XPathNodeReader::new(node, &context).and_then(|r| ArtistRef::from_xml(&r)) - }) - .collect(); + }).collect(); res? } _ => Vec::new(), From dcf9f61635a02b95d3ac351faac0da43859a9b62 Mon Sep 17 00:00:00 2001 From: Nick Cameron Date: Thu, 12 Jul 2018 14:08:16 +1200 Subject: [PATCH 18/19] Add tests Closes #2773 Closes #2786 --- tests/source/chains.rs | 31 +++++++++++++++++++++++++++++++ tests/target/chains.rs | 29 +++++++++++++++++++++++++++++ 2 files changed, 60 insertions(+) diff --git a/tests/source/chains.rs b/tests/source/chains.rs index 749d18c57d79..52a077e435f5 100644 --- a/tests/source/chains.rs +++ b/tests/source/chains.rs @@ -228,3 +228,34 @@ fn issue2415() { })() .unwrap_or_else(|_: Box<::std::error::Error>| String::from("")); } + +impl issue_2786 { + fn thing(&self) { + foo(|a| { + println!("a"); + println!("b"); + }).bar(|c| { + println!("a"); + println!("b"); + }) + .baz(|c| { + println!("a"); + println!("b"); + }) + } +} + +fn issue_2773() { + let bar = Some(0); + bar.or_else(|| { + // do stuff + None + }).or_else(|| { + // do other stuff + None + }) + .and_then(|val| { + // do this stuff + None + }); +} diff --git a/tests/target/chains.rs b/tests/target/chains.rs index a328ef004bed..2d945e3c390d 100644 --- a/tests/target/chains.rs +++ b/tests/target/chains.rs @@ -250,3 +250,32 @@ fn issue2415() { })().ok_or("")?) })().unwrap_or_else(|_: Box<::std::error::Error>| String::from("")); } + +impl issue_2786 { + fn thing(&self) { + foo(|a| { + println!("a"); + println!("b"); + }).bar(|c| { + println!("a"); + println!("b"); + }).baz(|c| { + println!("a"); + println!("b"); + }) + } +} + +fn issue_2773() { + let bar = Some(0); + bar.or_else(|| { + // do stuff + None + }).or_else(|| { + // do other stuff + None + }).and_then(|val| { + // do this stuff + None + }); +} From df4fb8a05b95f770304dae63c0bd90bb7408a232 Mon Sep 17 00:00:00 2001 From: Nick Cameron Date: Tue, 24 Jul 2018 15:48:23 +1200 Subject: [PATCH 19/19] Reformatting after rebase --- src/chains.rs | 2 +- src/closures.rs | 3 ++- src/formatting.rs | 3 +-- src/macros.rs | 31 +++++++++++++++---------------- src/pairs.rs | 2 +- src/reorder.rs | 13 +++++++------ 6 files changed, 27 insertions(+), 27 deletions(-) diff --git a/src/chains.rs b/src/chains.rs index 45b45b16732a..7acb9b9126f4 100644 --- a/src/chains.rs +++ b/src/chains.rs @@ -302,7 +302,7 @@ trait ChainFormatter { // foo.bar // .baz() // ``` - // If `bar` were not part of the root, then baz would be orphaned and 'float'. + // If `bar` were not part of the root, then foo would be orphaned and 'float'. fn format_root( &mut self, parent: &ChainItem, diff --git a/src/closures.rs b/src/closures.rs index 128e1dc8e0b6..c03c8e6b6065 100644 --- a/src/closures.rs +++ b/src/closures.rs @@ -376,7 +376,8 @@ where ast::ExprKind::Closure(..) => true, _ => false, }).unwrap_or(false) - }).count() > 1 + }).count() + > 1 } fn is_block_closure_forced(context: &RewriteContext, expr: &ast::Expr) -> bool { diff --git a/src/formatting.rs b/src/formatting.rs index ee2539a2fa50..42ab4e89c3bc 100644 --- a/src/formatting.rs +++ b/src/formatting.rs @@ -246,8 +246,7 @@ impl FormattingError { fl.file .get_line(fl.lines[0].line_index) .map(|l| l.into_owned()) - }) - .unwrap_or_else(|| String::new()), + }).unwrap_or_else(|| String::new()), } } diff --git a/src/macros.rs b/src/macros.rs index 91c29958ba16..b55375b949c1 100644 --- a/src/macros.rs +++ b/src/macros.rs @@ -1118,22 +1118,21 @@ fn indent_macro_snippet( }).min()?; Some( - first_line + "\n" - + &trimmed_lines - .iter() - .map( - |&(trimmed, ref line, prefix_space_width)| match prefix_space_width { - _ if !trimmed => line.to_owned(), - Some(original_indent_width) => { - let new_indent_width = indent.width() - + original_indent_width.saturating_sub(min_prefix_space_width); - let new_indent = Indent::from_width(context.config, new_indent_width); - format!("{}{}", new_indent.to_string(context.config), line.trim()) - } - None => String::new(), - }, - ).collect::>() - .join("\n"), + first_line + "\n" + &trimmed_lines + .iter() + .map( + |&(trimmed, ref line, prefix_space_width)| match prefix_space_width { + _ if !trimmed => line.to_owned(), + Some(original_indent_width) => { + let new_indent_width = indent.width() + original_indent_width + .saturating_sub(min_prefix_space_width); + let new_indent = Indent::from_width(context.config, new_indent_width); + format!("{}{}", new_indent.to_string(context.config), line.trim()) + } + None => String::new(), + }, + ).collect::>() + .join("\n"), ) } diff --git a/src/pairs.rs b/src/pairs.rs index 2c4358f7278a..17b869750226 100644 --- a/src/pairs.rs +++ b/src/pairs.rs @@ -125,7 +125,7 @@ fn rewrite_pairs_multiline( IndentStyle::Visual => shape.visual_indent(0), IndentStyle::Block => shape.block_indent(context.config.tab_spaces()), }).with_max_width(&context.config) - .sub_width(rhs_offset)?; + .sub_width(rhs_offset)?; let indent_str = nested_shape.indent.to_string_with_newline(context.config); let mut result = String::new(); diff --git a/src/reorder.rs b/src/reorder.rs index df32ab47b2fd..48ddd1163379 100644 --- a/src/reorder.rs +++ b/src/reorder.rs @@ -241,12 +241,13 @@ impl<'b, 'a: 'b> FmtVisitor<'a> { let item_length = items .iter() .take_while(|ppi| { - item_kind.is_same_item_kind(&***ppi) && (!in_group || { - let current = self.codemap.lookup_line_range(ppi.span()); - let in_same_group = current.lo < last.hi + 2; - last = current; - in_same_group - }) + item_kind.is_same_item_kind(&***ppi) + && (!in_group || { + let current = self.codemap.lookup_line_range(ppi.span()); + let in_same_group = current.lo < last.hi + 2; + last = current; + in_same_group + }) }).count(); let items = &items[..item_length];