diff --git a/Cargo.lock b/Cargo.lock index b0f1f478df3..3648e5e5f67 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2,11 +2,11 @@ name = "rustfmt" version = "0.0.1" dependencies = [ - "diff 0.1.5 (git+https://github.com/utkarshkukreti/diff.rs.git)", + "diff 0.1.7 (git+https://github.com/utkarshkukreti/diff.rs.git)", "regex 0.1.41 (registry+https://github.com/rust-lang/crates.io-index)", "rustc-serialize 0.3.16 (registry+https://github.com/rust-lang/crates.io-index)", "strings 0.0.1 (git+https://github.com/nrc/strings.rs.git)", - "term 0.2.11 (registry+https://github.com/rust-lang/crates.io-index)", + "term 0.2.12 (registry+https://github.com/rust-lang/crates.io-index)", "toml 0.1.22 (registry+https://github.com/rust-lang/crates.io-index)", "unicode-segmentation 0.1.2 (registry+https://github.com/rust-lang/crates.io-index)", ] @@ -21,15 +21,15 @@ dependencies = [ [[package]] name = "diff" -version = "0.1.5" -source = "git+https://github.com/utkarshkukreti/diff.rs.git#1921576a73e1b50a0ecb26c8ce62eefb26d273b4" +version = "0.1.7" +source = "git+https://github.com/utkarshkukreti/diff.rs.git#6edb9454bf4127087aced0fe07ab3ea6894083cb" [[package]] name = "kernel32-sys" version = "0.1.4" source = "registry+https://github.com/rust-lang/crates.io-index" dependencies = [ - "winapi 0.2.3 (registry+https://github.com/rust-lang/crates.io-index)", + "winapi 0.2.4 (registry+https://github.com/rust-lang/crates.io-index)", "winapi-build 0.1.1 (registry+https://github.com/rust-lang/crates.io-index)", ] @@ -84,11 +84,11 @@ dependencies = [ [[package]] name = "term" -version = "0.2.11" +version = "0.2.12" source = "registry+https://github.com/rust-lang/crates.io-index" dependencies = [ "kernel32-sys 0.1.4 (registry+https://github.com/rust-lang/crates.io-index)", - "winapi 0.2.3 (registry+https://github.com/rust-lang/crates.io-index)", + "winapi 0.2.4 (registry+https://github.com/rust-lang/crates.io-index)", ] [[package]] @@ -106,7 +106,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" [[package]] name = "winapi" -version = "0.2.3" +version = "0.2.4" source = "registry+https://github.com/rust-lang/crates.io-index" [[package]] diff --git a/src/comment.rs b/src/comment.rs index b08e46681a7..86d1a9aeb14 100644 --- a/src/comment.rs +++ b/src/comment.rs @@ -348,8 +348,7 @@ mod test { assert_eq!(&uncommented("abc/*...*/"), "abc"); assert_eq!(&uncommented("// .... /* \n../* /* *** / */ */a/* // */c\n"), "..ac\n"); - assert_eq!(&uncommented("abc \" /* */\" qsdf"), - "abc \" /* */\" qsdf"); + assert_eq!(&uncommented("abc \" /* */\" qsdf"), "abc \" /* */\" qsdf"); } #[test] diff --git a/src/config.rs b/src/config.rs index 3a73867f873..b237ce4ddfc 100644 --- a/src/config.rs +++ b/src/config.rs @@ -260,9 +260,9 @@ create_config! { max_width: usize, 100, "Maximum width of each line"; ideal_width: usize, 80, "Ideal width of each line"; tab_spaces: usize, 4, "Number of spaces per tab"; - fn_call_width: usize, 50, + fn_call_width: usize, 60, "Maximum width of the args of a function call before faling back to vertical formatting"; - struct_lit_width: usize, 12, + struct_lit_width: usize, 16, "Maximum width in the body of a struct lit before faling back to vertical formatting"; newline_style: NewlineStyle, NewlineStyle::Unix, "Unix or Windows line endings"; fn_brace_style: BraceStyle, BraceStyle::SameLineWhere, "Brace style for functions"; diff --git a/src/expr.rs b/src/expr.rs index fcb2ae1ff05..1d0621539e0 100644 --- a/src/expr.rs +++ b/src/expr.rs @@ -13,7 +13,8 @@ use std::borrow::Borrow; use Indent; use rewrite::{Rewrite, RewriteContext}; -use lists::{write_list, itemize_list, ListFormatting, SeparatorTactic, ListTactic}; +use lists::{write_list, itemize_list, ListFormatting, SeparatorTactic, ListTactic, + DefinitiveListTactic, definitive_tactic, ListItem, format_fn_args}; use string::{StringFormat, rewrite_string}; use utils::{span_after, extra_offset, last_line_width, wrap_str, binary_search}; use visitor::FmtVisitor; @@ -156,10 +157,7 @@ impl Rewrite for ast::Expr { }) } ast::Expr_::ExprRet(None) => { - wrap_str("return".to_owned(), - context.config.max_width, - width, - offset) + wrap_str("return".to_owned(), context.config.max_width, width, offset) } ast::Expr_::ExprRet(Some(ref expr)) => { rewrite_unary_prefix(context, "return ", expr, width, offset) @@ -266,21 +264,20 @@ pub fn rewrite_array<'a, I>(expr_iter: I, |item| item.span.lo, |item| item.span.hi, // 1 = [ - // FIXME(#133): itemize_list doesn't support - // rewrite failure. This may not be its - // responsibility, but that of write_list. - |item| { - item.rewrite(context, max_item_width, offset + 1) - .unwrap_or_else(|| context.snippet(item.span)) - }, + |item| item.rewrite(context, max_item_width, offset + 1), span_after(span, "[", context.codemap), span.hi) .collect::>(); - let tactic = if items.iter().any(|li| li.item.len() > 10 || li.is_multiline()) { - ListTactic::HorizontalVertical + let has_long_item = try_opt!(items.iter() + .map(|li| li.item.as_ref().map(|s| s.len() > 10)) + .fold(Some(false), + |acc, x| acc.and_then(|y| x.map(|x| (x || y))))); + + let tactic = if has_long_item || items.iter().any(ListItem::is_multiline) { + definitive_tactic(&items, ListTactic::HorizontalVertical, max_item_width) } else { - ListTactic::Mixed + DefinitiveListTactic::Mixed }; let fmt = ListFormatting { @@ -288,8 +285,7 @@ pub fn rewrite_array<'a, I>(expr_iter: I, separator: ",", trailing_separator: SeparatorTactic::Never, indent: offset + 1, - h_width: max_item_width, - v_width: max_item_width, + width: max_item_width, ends_with_newline: false, config: context.config, }; @@ -329,29 +325,26 @@ fn rewrite_closure(capture: ast::CaptureClause, "|", |arg| span_lo_for_arg(arg), |arg| span_hi_for_arg(arg), - |arg| { - // FIXME: we should just escalate failure - // here, but itemize_list doesn't allow it. - arg.rewrite(context, budget, argument_offset) - .unwrap_or_else(|| { - context.snippet(mk_sp(span_lo_for_arg(arg), - span_hi_for_arg(arg))) - }) - }, + |arg| arg.rewrite(context, budget, argument_offset), span_after(span, "|", context.codemap), body.span.lo); + let item_vec = arg_items.collect::>(); + let tactic = definitive_tactic(&item_vec, ListTactic::HorizontalVertical, horizontal_budget); + let budget = match tactic { + DefinitiveListTactic::Horizontal => horizontal_budget, + _ => budget, + }; let fmt = ListFormatting { - tactic: ListTactic::HorizontalVertical, + tactic: tactic, separator: ",", trailing_separator: SeparatorTactic::Never, indent: argument_offset, - h_width: horizontal_budget, - v_width: budget, + width: budget, ends_with_newline: false, config: context.config, }; - let list_str = try_opt!(write_list(&arg_items.collect::>(), &fmt)); + let list_str = try_opt!(write_list(&item_vec, &fmt)); let mut prefix = format!("{}|{}|", mover, list_str); if !ret_str.is_empty() { @@ -801,8 +794,7 @@ fn rewrite_match(context: &RewriteContext, } } // BytePos(1) = closing match brace. - let last_span = mk_sp(arm_end_pos(&arms[arms.len() - 1]), - span.hi - BytePos(1)); + let last_span = mk_sp(arm_end_pos(&arms[arms.len() - 1]), span.hi - BytePos(1)); let last_comment = context.snippet(last_span); let comment = try_opt!(rewrite_match_arm_comment(context, &last_comment, @@ -915,8 +907,7 @@ impl Rewrite for ast::Arm { // 4 = ` => `.len() let same_line_body = if context.config.max_width > line_start + comma.len() + 4 { let budget = context.config.max_width - line_start - comma.len() - 4; - let offset = Indent::new(offset.block_indent, - line_start + 4 - offset.block_indent); + let offset = Indent::new(offset.block_indent, line_start + 4 - offset.block_indent); let rewrite = nop_block_collapse(body.rewrite(context, budget, offset), budget); match rewrite { @@ -1156,16 +1147,11 @@ fn rewrite_call_inner(context: &RewriteContext, ")", |item| item.span.lo, |item| item.span.hi, - // Take old span when rewrite fails. - |item| { - item.rewrite(&inner_context, remaining_width, offset) - .unwrap_or(context.snippet(item.span)) - }, + |item| item.rewrite(&inner_context, remaining_width, offset), span.lo, span.hi); - let fmt = ListFormatting::for_fn(remaining_width, offset, context.config); - let list_str = match write_list(&items.collect::>(), &fmt) { + let list_str = match format_fn_args(items, remaining_width, offset, context.config) { Some(str) => str, None => return Err(Ordering::Less), }; @@ -1178,9 +1164,7 @@ fn rewrite_paren(context: &RewriteContext, width: usize, offset: Indent) -> Option { - debug!("rewrite_paren, width: {}, offset: {:?}", - width, - offset); + debug!("rewrite_paren, width: {}, offset: {:?}", width, offset); // 1 is for opening paren, 2 is for opening+closing, we want to keep the closing // paren on the same line as the subexpr. let subexpr_str = subexpr.rewrite(context, try_opt!(width.checked_sub(2)), offset + 1); @@ -1196,9 +1180,7 @@ fn rewrite_struct_lit<'a>(context: &RewriteContext, width: usize, offset: Indent) -> Option { - debug!("rewrite_struct_lit: width {}, offset {:?}", - width, - offset); + debug!("rewrite_struct_lit: width {}, offset {:?}", width, offset); assert!(!fields.is_empty() || base.is_some()); enum StructLitField<'a> { @@ -1259,32 +1241,37 @@ fn rewrite_struct_lit<'a>(context: &RewriteContext, match *item { StructLitField::Regular(ref field) => { rewrite_field(inner_context, &field, v_budget, indent) - .unwrap_or(context.snippet(field.span)) } StructLitField::Base(ref expr) => { // 2 = .. - format!("..{}", - v_budget.checked_sub(2) - .and_then(|v_budget| { - expr.rewrite(inner_context, - v_budget, - indent + 2) - }) - .unwrap_or(context.snippet(expr.span))) + expr.rewrite(inner_context, + try_opt!(v_budget.checked_sub(2)), + indent + 2) + .map(|s| format!("..{}", s)) } } }, span_after(span, "{", context.codemap), span.hi); + let item_vec = items.collect::>(); - let mut tactic = match (context.config.struct_lit_style, fields.len()) { - (StructLitStyle::Visual, 1) => ListTactic::HorizontalVertical, - _ => context.config.struct_lit_multiline_style.to_list_tactic(), + let tactic = { + let mut prelim_tactic = match (context.config.struct_lit_style, fields.len()) { + (StructLitStyle::Visual, 1) => ListTactic::HorizontalVertical, + _ => context.config.struct_lit_multiline_style.to_list_tactic(), + }; + + if prelim_tactic == ListTactic::HorizontalVertical && fields.len() > 1 { + prelim_tactic = ListTactic::LimitedHorizontalVertical(context.config.struct_lit_width); + }; + + definitive_tactic(&item_vec, prelim_tactic, h_budget) }; - if tactic == ListTactic::HorizontalVertical && fields.len() > 1 { - tactic = ListTactic::LimitedHorizontalVertical(context.config.struct_lit_width); - } + let budget = match tactic { + DefinitiveListTactic::Horizontal => h_budget, + _ => v_budget, + }; let fmt = ListFormatting { tactic: tactic, @@ -1295,12 +1282,11 @@ fn rewrite_struct_lit<'a>(context: &RewriteContext, context.config.struct_lit_trailing_comma }, indent: indent, - h_width: h_budget, - v_width: v_budget, + width: budget, ends_with_newline: false, config: context.config, }; - let fields_str = try_opt!(write_list(&items.collect::>(), &fmt)); + let fields_str = try_opt!(write_list(&item_vec, &fmt)); let format_on_newline = || { let inner_indent = context.block_indent @@ -1345,9 +1331,7 @@ fn rewrite_tuple_lit(context: &RewriteContext, width: usize, offset: Indent) -> Option { - debug!("rewrite_tuple_lit: width: {}, offset: {:?}", - width, - offset); + debug!("rewrite_tuple_lit: width: {}, offset: {:?}", width, offset); let indent = offset + 1; // In case of length 1, need a trailing comma if items.len() == 1 { @@ -1364,14 +1348,11 @@ fn rewrite_tuple_lit(context: &RewriteContext, |item| { let inner_width = context.config.max_width - indent.width() - 1; item.rewrite(context, inner_width, indent) - .unwrap_or(context.snippet(item.span)) }, span.lo + BytePos(1), // Remove parens span.hi - BytePos(1)); - let budget = try_opt!(width.checked_sub(2)); - let fmt = ListFormatting::for_fn(budget, indent, context.config); - let list_str = try_opt!(write_list(&items.collect::>(), &fmt)); + let list_str = try_opt!(format_fn_args(items, budget, indent, context.config)); Some(format!("({})", list_str)) } diff --git a/src/filemap.rs b/src/filemap.rs index f3f952bd00c..d94c1501734 100644 --- a/src/filemap.rs +++ b/src/filemap.rs @@ -115,8 +115,7 @@ fn write_file(text: &StringBuffer, try!(write_system_newlines(&mut v, text, config)); let fmt_text = String::from_utf8(v).unwrap(); let diff = make_diff(&ori_text, &fmt_text, 3); - print_diff(diff, - |line_num| format!("\nDiff at line {}:", line_num)); + print_diff(diff, |line_num| format!("\nDiff at line {}:", line_num)); } WriteMode::Return => { // io::Write is not implemented for String, working around with diff --git a/src/imports.rs b/src/imports.rs index 8c3545f2e12..aab5dd76799 100644 --- a/src/imports.rs +++ b/src/imports.rs @@ -9,7 +9,7 @@ // except according to those terms. use Indent; -use lists::{write_list, itemize_list, ListItem, ListFormatting, SeparatorTactic, ListTactic}; +use lists::{write_list, itemize_list, ListItem, ListFormatting, SeparatorTactic, definitive_tactic}; use utils::span_after; use rewrite::{Rewrite, RewriteContext}; @@ -71,7 +71,7 @@ fn rewrite_single_use_list(path_str: String, vpi: &ast::PathListItem) -> String append_alias(path_item_str, vpi) } -fn rewrite_path_item(vpi: &&ast::PathListItem) -> String { +fn rewrite_path_item(vpi: &&ast::PathListItem) -> Option { let path_item_str = match vpi.node { ast::PathListItem_::PathListIdent{ name, .. } => { name.to_string() @@ -81,7 +81,7 @@ fn rewrite_path_item(vpi: &&ast::PathListItem) -> String { } }; - append_alias(path_item_str, vpi) + Some(append_alias(path_item_str, vpi)) } fn append_alias(path_item_str: String, vpi: &ast::PathListItem) -> String { @@ -124,20 +124,6 @@ pub fn rewrite_use_list(width: usize, // 1 = } let remaining_width = width.checked_sub(supp_indent + 1).unwrap_or(0); - let fmt = ListFormatting { - tactic: ListTactic::Mixed, - separator: ",", - trailing_separator: SeparatorTactic::Never, - indent: offset + supp_indent, - h_width: remaining_width, - // FIXME This is too conservative, and will not use all width - // available - // (loose 1 column (";")) - v_width: remaining_width, - ends_with_newline: false, - config: context.config, - }; - let mut items = { // Dummy value, see explanation below. let mut items = vec![ListItem::from_str("")]; @@ -156,8 +142,6 @@ pub fn rewrite_use_list(width: usize, // We prefixed the item list with a dummy value so that we can // potentially move "self" to the front of the vector without touching // the rest of the items. - // FIXME: Make more efficient by using a linked list? That would require - // changes to the signatures of write_list. let has_self = move_self_to_front(&mut items); let first_index = if has_self { 0 @@ -169,6 +153,21 @@ pub fn rewrite_use_list(width: usize, items[1..].sort_by(|a, b| a.item.cmp(&b.item)); } + let tactic = definitive_tactic(&items[first_index..], + ::lists::ListTactic::Mixed, + remaining_width); + let fmt = ListFormatting { + tactic: tactic, + separator: ",", + trailing_separator: SeparatorTactic::Never, + indent: offset + supp_indent, + // FIXME This is too conservative, and will not use all width + // available + // (loose 1 column (";")) + width: remaining_width, + ends_with_newline: false, + config: context.config, + }; let list_str = try_opt!(write_list(&items[first_index..], &fmt)); Some(if path_str.is_empty() { @@ -180,7 +179,7 @@ pub fn rewrite_use_list(width: usize, // Returns true when self item was found. fn move_self_to_front(items: &mut Vec) -> bool { - match items.iter().position(|item| item.item == "self") { + match items.iter().position(|item| item.item.as_ref().map(|x| &x[..]) == Some("self")) { Some(pos) => { items[0] = items.remove(pos); true diff --git a/src/issues.rs b/src/issues.rs index 6f64436bb52..04a61fff358 100644 --- a/src/issues.rs +++ b/src/issues.rs @@ -236,8 +236,7 @@ fn find_unnumbered_issue() { fn check_pass(text: &str) { let mut seeker = BadIssueSeeker::new(ReportTactic::Unnumbered, ReportTactic::Unnumbered); - assert_eq!(None, - text.chars().position(|c| seeker.inspect(c).is_some())); + assert_eq!(None, text.chars().position(|c| seeker.inspect(c).is_some())); } check_fail("TODO\n", 4); @@ -272,9 +271,7 @@ fn find_issue() { ReportTactic::Never, ReportTactic::Always)); - assert!(!is_bad_issue("bad FIXME\n", - ReportTactic::Always, - ReportTactic::Never)); + assert!(!is_bad_issue("bad FIXME\n", ReportTactic::Always, ReportTactic::Never)); } #[test] diff --git a/src/items.rs b/src/items.rs index 827b528771e..3d9a0e56aa4 100644 --- a/src/items.rs +++ b/src/items.rs @@ -12,7 +12,8 @@ use Indent; use utils::{format_mutability, format_visibility, contains_skip, span_after, end_typaram, wrap_str}; -use lists::{write_list, itemize_list, ListItem, ListFormatting, SeparatorTactic, ListTactic}; +use lists::{write_list, itemize_list, ListItem, ListFormatting, SeparatorTactic, ListTactic, + DefinitiveListTactic, definitive_tactic}; use expr::rewrite_assign_rhs; use comment::FindUncommented; use visitor::FmtVisitor; @@ -311,9 +312,7 @@ impl<'a> FmtVisitor<'a> { let context = self.get_context(); let ret_str = fd.output - .rewrite(&context, - self.config.max_width - indent.width(), - indent) + .rewrite(&context, self.config.max_width - indent.width(), indent) .unwrap(); // Args. @@ -480,7 +479,7 @@ impl<'a> FmtVisitor<'a> { ")", |arg| span_lo_for_arg(arg), |arg| arg.ty.span.hi, - |_| String::new(), + |_| None, comment_span_start, span.hi); @@ -490,7 +489,7 @@ impl<'a> FmtVisitor<'a> { assert_eq!(arg_item_strs.len(), arg_items.len()); for (item, arg) in arg_items.iter_mut().zip(arg_item_strs) { - item.item = arg; + item.item = Some(arg); } let indent = match self.config.fn_arg_indent { @@ -499,13 +498,20 @@ impl<'a> FmtVisitor<'a> { BlockIndentStyle::Visual => arg_indent, }; + let tactic = definitive_tactic(&arg_items, + self.config.fn_args_density.to_list_tactic(), + one_line_budget); + let budget = match tactic { + DefinitiveListTactic::Horizontal => one_line_budget, + _ => multi_line_budget, + }; + let fmt = ListFormatting { - tactic: self.config.fn_args_density.to_list_tactic(), + tactic: tactic, separator: ",", trailing_separator: SeparatorTactic::Never, indent: indent, - h_width: one_line_budget, - v_width: multi_line_budget, + width: budget, ends_with_newline: false, config: self.config, }; @@ -622,14 +628,13 @@ impl<'a> FmtVisitor<'a> { |arg| arg.ty.span.hi, |arg| { // FIXME silly width, indent - arg.ty - .rewrite(&self.get_context(), - 1000, - Indent::empty()) - .unwrap() + arg.ty.rewrite(&self.get_context(), + 1000, + Indent::empty()) }, span_after(field.span, "(", self.codemap), next_span_start); + let item_vec = items.collect::>(); result.push('('); @@ -641,18 +646,20 @@ impl<'a> FmtVisitor<'a> { 0 }; let budget = self.config.max_width - indent.width() - comma_cost - 1; // 1 = ) + let tactic = definitive_tactic(&item_vec, + ListTactic::HorizontalVertical, + budget); let fmt = ListFormatting { - tactic: ListTactic::HorizontalVertical, + tactic: tactic, separator: ",", trailing_separator: SeparatorTactic::Never, indent: indent, - h_width: budget, - v_width: budget, + width: budget, ends_with_newline: true, config: self.config, }; - let list_str = match write_list(&items.collect::>(), &fmt) { + let list_str = match write_list(&item_vec, &fmt) { Some(list_str) => list_str, None => return, }; @@ -766,9 +773,9 @@ impl<'a> FmtVisitor<'a> { result.push('\n'); result.push_str(&indentation); - ListTactic::Vertical + DefinitiveListTactic::Vertical } else { - ListTactic::Horizontal + DefinitiveListTactic::Horizontal }; // 1 = , @@ -778,13 +785,12 @@ impl<'a> FmtVisitor<'a> { separator: ",", trailing_separator: self.config.struct_trailing_comma, indent: offset.block_indent(self.config), - h_width: self.config.max_width, - v_width: budget, + width: budget, ends_with_newline: true, config: self.config, }; - let list_str = try_opt!(write_list(&items.collect::>(), &fmt)); + let list_str = try_opt!(write_list(items, &fmt)); result.push_str(&list_str); if break_line { @@ -853,9 +859,14 @@ impl<'a> FmtVisitor<'a> { } // Field of a struct - fn format_field(&self, field: &ast::StructField) -> String { + fn format_field(&self, field: &ast::StructField) -> Option { if contains_skip(&field.node.attrs) { - return self.snippet(codemap::mk_sp(field.node.attrs[0].span.lo, field.span.hi)); + // FIXME: silly width, indent + return wrap_str(self.snippet(codemap::mk_sp(field.node.attrs[0].span.lo, + field.span.hi)), + self.config.max_width, + 1000, + Indent::empty()); } let name = match field.node.kind { @@ -867,24 +878,23 @@ impl<'a> FmtVisitor<'a> { ast::StructFieldKind::UnnamedField(vis) => format_visibility(vis), }; // FIXME silly width, indent - let typ = field.node.ty.rewrite(&self.get_context(), 1000, Indent::empty()).unwrap(); + let typ = try_opt!(field.node.ty.rewrite(&self.get_context(), 1000, Indent::empty())); let indent = self.block_indent.block_indent(self.config); - let mut attr_str = field.node - .attrs - .rewrite(&self.get_context(), - self.config.max_width - indent.width(), - indent) - .unwrap(); + let mut attr_str = try_opt!(field.node + .attrs + .rewrite(&self.get_context(), + self.config.max_width - indent.width(), + indent)); if !attr_str.is_empty() { attr_str.push('\n'); attr_str.push_str(&indent.to_string(self.config)); } - match name { + Some(match name { Some(name) => format!("{}{}{}: {}", attr_str, vis, name, typ), None => format!("{}{}{}", attr_str, vis, typ), - } + }) } fn rewrite_generics(&self, @@ -913,10 +923,8 @@ impl<'a> FmtVisitor<'a> { // Strings for the generics. let context = self.get_context(); - // FIXME: don't unwrap - let lt_strs = lifetimes.iter().map(|lt| lt.rewrite(&context, h_budget, offset).unwrap()); - let ty_strs = tys.iter() - .map(|ty_param| ty_param.rewrite(&context, h_budget, offset).unwrap()); + let lt_strs = lifetimes.iter().map(|lt| lt.rewrite(&context, h_budget, offset)); + let ty_strs = tys.iter().map(|ty_param| ty_param.rewrite(&context, h_budget, offset)); // Extract comments between generics. let lt_spans = lifetimes.iter().map(|l| { @@ -930,21 +938,15 @@ impl<'a> FmtVisitor<'a> { let ty_spans = tys.iter().map(span_for_ty_param); let items = itemize_list(self.codemap, - lt_spans.chain(ty_spans), + lt_spans.chain(ty_spans).zip(lt_strs.chain(ty_strs)), ">", - |sp| sp.lo, - |sp| sp.hi, - |_| String::new(), + |&(sp, _)| sp.lo, + |&(sp, _)| sp.hi, + // FIXME: don't clone + |&(_, ref str)| str.clone(), span_after(span, "<", self.codemap), span.hi); - let mut items = items.collect::>(); - - for (item, ty) in items.iter_mut().zip(lt_strs.chain(ty_strs)) { - item.item = ty; - } - - let fmt = ListFormatting::for_item(h_budget, offset, self.config); - let list_str = try_opt!(write_list(&items, &fmt)); + let list_str = try_opt!(::lists::format_item_list(items, h_budget, offset, self.config)); Some(format!("<{}>", list_str)) } @@ -984,24 +986,23 @@ impl<'a> FmtVisitor<'a> { "{", |pred| span_for_where_pred(pred).lo, |pred| span_for_where_pred(pred).hi, - // FIXME: we should handle failure better - // this will be taken care of when write_list - // takes Rewrite object: see issue #133 - |pred| pred.rewrite(&context, budget, offset).unwrap(), + |pred| pred.rewrite(&context, budget, offset), span_start, span_end); + let item_vec = items.collect::>(); + // FIXME: we don't need to collect here if the where_layout isnt horizontalVertical + let tactic = definitive_tactic(&item_vec, self.config.where_layout, budget); let fmt = ListFormatting { - tactic: self.config.where_layout, + tactic: tactic, separator: ",", trailing_separator: SeparatorTactic::Never, indent: offset, - h_width: budget, - v_width: budget, + width: budget, ends_with_newline: true, config: self.config, }; - let preds_str = try_opt!(write_list(&items.collect::>(), &fmt)); + let preds_str = try_opt!(write_list(&item_vec, &fmt)); // 9 = " where ".len() + " {".len() if density == Density::Tall || preds_str.contains('\n') || diff --git a/src/lib.rs b/src/lib.rs index c075589e99c..1dfb05c88b6 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -19,7 +19,6 @@ // keeping some scratch mem for this and running our own StrPool? // TODO for lint violations of names, emit a refactor script - #[macro_use] extern crate log; @@ -119,8 +118,7 @@ impl Indent { pub fn to_string(&self, config: &Config) -> String { let (num_tabs, num_spaces) = if config.hard_tabs { - (self.block_indent / config.tab_spaces, - self.alignment) + (self.block_indent / config.tab_spaces, self.alignment) } else { (0, self.block_indent + self.alignment) }; diff --git a/src/lists.rs b/src/lists.rs index 9a169c15ce1..6f464805e4b 100644 --- a/src/lists.rs +++ b/src/lists.rs @@ -14,17 +14,20 @@ use std::iter::Peekable; use syntax::codemap::{self, CodeMap, BytePos}; use Indent; -use utils::{round_up_to_power_of_two, wrap_str}; +use utils::wrap_str; use comment::{FindUncommented, rewrite_comment, find_comment_end}; use config::Config; #[derive(Eq, PartialEq, Debug, Copy, Clone)] +/// Formatting tactic for lists. This will be cast down to a +/// DefinitiveListTactic depending on the number and length of the items and +/// their comments. pub enum ListTactic { // One item per row. Vertical, // All items on one row. Horizontal, - // Try Horizontal layout, if that fails then vertical + // Try Horizontal layout, if that fails then vertical. HorizontalVertical, // HorizontalVertical with a soft limit of n characters. LimitedHorizontalVertical(usize), @@ -43,54 +46,73 @@ pub enum SeparatorTactic { impl_enum_decodable!(SeparatorTactic, Always, Never, Vertical); -// TODO having some helpful ctors for ListFormatting would be nice. pub struct ListFormatting<'a> { - pub tactic: ListTactic, + pub tactic: DefinitiveListTactic, pub separator: &'a str, pub trailing_separator: SeparatorTactic, pub indent: Indent, - // Available width if we layout horizontally. - pub h_width: usize, - // Available width if we layout vertically - pub v_width: usize, + pub width: usize, // Non-expressions, e.g. items, will have a new line at the end of the list. // Important for comment styles. pub ends_with_newline: bool, pub config: &'a Config, } -impl<'a> ListFormatting<'a> { - pub fn for_fn(width: usize, offset: Indent, config: &'a Config) -> ListFormatting<'a> { - ListFormatting { - tactic: ListTactic::LimitedHorizontalVertical(config.fn_call_width), - separator: ",", - trailing_separator: SeparatorTactic::Never, - indent: offset, - h_width: width, - v_width: width, - ends_with_newline: false, - config: config, - } - } +pub fn format_fn_args(items: I, width: usize, offset: Indent, config: &Config) -> Option + where I: Iterator +{ + list_helper(items, + width, + offset, + config, + ListTactic::LimitedHorizontalVertical(config.fn_call_width)) +} - pub fn for_item(width: usize, offset: Indent, config: &'a Config) -> ListFormatting<'a> { - ListFormatting { - tactic: ListTactic::HorizontalVertical, - separator: ",", - trailing_separator: SeparatorTactic::Never, - indent: offset, - h_width: width, - v_width: width, - ends_with_newline: false, - config: config, - } +pub fn format_item_list(items: I, + width: usize, + offset: Indent, + config: &Config) + -> Option + where I: Iterator +{ + list_helper(items, width, offset, config, ListTactic::HorizontalVertical) +} + +fn list_helper(items: I, + width: usize, + offset: Indent, + config: &Config, + tactic: ListTactic) + -> Option + where I: Iterator +{ + let item_vec: Vec<_> = items.collect(); + let tactic = definitive_tactic(&item_vec, tactic, width); + let fmt = ListFormatting { + tactic: tactic, + separator: ",", + trailing_separator: SeparatorTactic::Never, + indent: offset, + width: width, + ends_with_newline: false, + config: config, + }; + + write_list(&item_vec, &fmt) +} + +impl AsRef for ListItem { + fn as_ref(&self) -> &ListItem { + self } } pub struct ListItem { + // None for comments mean that they are not present. pub pre_comment: Option, - // Item should include attributes and doc comments. - pub item: String, + // Item should include attributes and doc comments. None indicates a failed + // rewrite. + pub item: Option, pub post_comment: Option, // Whether there is extra whitespace before this item. pub new_lines: bool, @@ -98,7 +120,8 @@ pub struct ListItem { impl ListItem { pub fn is_multiline(&self) -> bool { - self.item.contains('\n') || self.pre_comment.is_some() || + self.item.as_ref().map(|s| s.contains('\n')).unwrap_or(false) || + self.pre_comment.is_some() || self.post_comment.as_ref().map(|s| s.contains('\n')).unwrap_or(false) } @@ -109,103 +132,96 @@ impl ListItem { pub fn from_str>(s: S) -> ListItem { ListItem { pre_comment: None, - item: s.into(), + item: Some(s.into()), post_comment: None, new_lines: false, } } } -// Format a list of commented items into a string. -// FIXME: this has grown into a monstrosity -// TODO: add unit tests -pub fn write_list<'b>(items: &[ListItem], formatting: &ListFormatting<'b>) -> Option { - if items.is_empty() { - return Some(String::new()); - } +#[derive(Eq, PartialEq, Debug, Copy, Clone)] +/// The definitive formatting tactic for lists. +pub enum DefinitiveListTactic { + Vertical, + Horizontal, + Mixed, +} - let mut tactic = formatting.tactic; +pub fn definitive_tactic<'t, I, T>(items: I, + tactic: ListTactic, + width: usize) + -> DefinitiveListTactic + where I: IntoIterator + Clone, + T: AsRef +{ + let pre_line_comments = items.clone() + .into_iter() + .any(|item| item.as_ref().has_line_pre_comment()); - // Conservatively overestimates because of the changing separator tactic. - let sep_count = if formatting.trailing_separator == SeparatorTactic::Always { - items.len() - } else { - items.len() - 1 + let limit = match tactic { + _ if pre_line_comments => return DefinitiveListTactic::Vertical, + ListTactic::Mixed => return DefinitiveListTactic::Mixed, + ListTactic::Horizontal => return DefinitiveListTactic::Horizontal, + ListTactic::Vertical => return DefinitiveListTactic::Vertical, + ListTactic::LimitedHorizontalVertical(limit) => ::std::cmp::min(width, limit), + ListTactic::HorizontalVertical => width, }; + + let (sep_count, total_width) = calculate_width(items.clone()); + let sep_len = ", ".len(); // FIXME: make more generic? + let total_sep_len = sep_len * sep_count.checked_sub(1).unwrap_or(0); + let real_total = total_width + total_sep_len; + + if real_total <= limit && !pre_line_comments && + !items.into_iter().any(|item| item.as_ref().is_multiline()) { + DefinitiveListTactic::Horizontal + } else { + DefinitiveListTactic::Vertical + } +} + +// Format a list of commented items into a string. +// TODO: add unit tests +pub fn write_list<'b, I, T>(items: I, formatting: &ListFormatting<'b>) -> Option + where I: IntoIterator, + T: AsRef +{ + let tactic = formatting.tactic; let sep_len = formatting.separator.len(); - let total_sep_len = (sep_len + 1) * sep_count; - let total_width = calculate_width(items); - let fits_single = total_width + total_sep_len <= formatting.h_width; - - // Check if we need to fallback from horizontal listing, if possible. - if let ListTactic::LimitedHorizontalVertical(limit) = tactic { - if total_width > limit { - tactic = ListTactic::Vertical; - } else { - tactic = ListTactic::HorizontalVertical; - } - } - if tactic == ListTactic::HorizontalVertical { - debug!("write_list: total_width: {}, total_sep_len: {}, h_width: {}", - total_width, - total_sep_len, - formatting.h_width); - tactic = if fits_single && !items.iter().any(ListItem::is_multiline) { - ListTactic::Horizontal - } else { - ListTactic::Vertical - }; - } - - // Check if we can fit everything on a single line in mixed mode. - // The horizontal tactic does not break after v_width columns. - if tactic == ListTactic::Mixed && fits_single { - tactic = ListTactic::Horizontal; - } - - // Switch to vertical mode if we find non-block comments. - if items.iter().any(ListItem::has_line_pre_comment) { - tactic = ListTactic::Vertical; - } // Now that we know how we will layout, we can decide for sure if there // will be a trailing separator. let trailing_separator = needs_trailing_separator(formatting.trailing_separator, tactic); - - // Create a buffer for the result. - // TODO could use a StringBuffer or rope for this - let alloc_width = if tactic == ListTactic::Horizontal { - total_width + total_sep_len - } else { - total_width + items.len() * (formatting.indent.width() + 1) - }; - let mut result = String::with_capacity(round_up_to_power_of_two(alloc_width)); + let mut result = String::new(); + let mut iter = items.into_iter().enumerate().peekable(); let mut line_len = 0; let indent_str = &formatting.indent.to_string(formatting.config); - for (i, item) in items.iter().enumerate() { + while let Some((i, item)) = iter.next() { + let item = item.as_ref(); + let inner_item = try_opt!(item.item.as_ref()); let first = i == 0; - let last = i == items.len() - 1; + let last = iter.peek().is_none(); let separate = !last || trailing_separator; let item_sep_len = if separate { sep_len } else { 0 }; - let item_width = item.item.len() + item_sep_len; + let item_width = inner_item.len() + item_sep_len; match tactic { - ListTactic::Horizontal if !first => { + DefinitiveListTactic::Horizontal if !first => { result.push(' '); } - ListTactic::Vertical if !first => { + DefinitiveListTactic::Vertical if !first => { result.push('\n'); result.push_str(indent_str); } - ListTactic::Mixed => { + DefinitiveListTactic::Mixed => { let total_width = total_item_width(item) + item_sep_len; - if line_len > 0 && line_len + total_width > formatting.v_width { + if line_len > 0 && line_len + total_width > formatting.width { result.push('\n'); result.push_str(indent_str); line_len = 0; @@ -224,9 +240,9 @@ pub fn write_list<'b>(items: &[ListItem], formatting: &ListFormatting<'b>) -> Op // Pre-comments if let Some(ref comment) = item.pre_comment { // Block style in non-vertical mode. - let block_mode = tactic != ListTactic::Vertical; + let block_mode = tactic != DefinitiveListTactic::Vertical; // Width restriction is only relevant in vertical mode. - let max_width = formatting.v_width; + let max_width = formatting.width; let comment = try_opt!(rewrite_comment(comment, block_mode, max_width, @@ -234,7 +250,7 @@ pub fn write_list<'b>(items: &[ListItem], formatting: &ListFormatting<'b>) -> Op formatting.config)); result.push_str(&comment); - if tactic == ListTactic::Vertical { + if tactic == DefinitiveListTactic::Vertical { result.push('\n'); result.push_str(indent_str); } else { @@ -242,18 +258,19 @@ pub fn write_list<'b>(items: &[ListItem], formatting: &ListFormatting<'b>) -> Op } } - let item_str = try_opt!(wrap_str(&item.item[..], + // Make sure that string actually fits. + let item_str = try_opt!(wrap_str(&inner_item[..], formatting.config.max_width, - formatting.v_width, + formatting.width, formatting.indent)); result.push_str(&item_str); // Post-comments - if tactic != ListTactic::Vertical && item.post_comment.is_some() { + if tactic != DefinitiveListTactic::Vertical && item.post_comment.is_some() { let comment = item.post_comment.as_ref().unwrap(); let formatted_comment = try_opt!(rewrite_comment(comment, true, - formatting.v_width, + formatting.width, Indent::empty(), formatting.config)); @@ -265,9 +282,9 @@ pub fn write_list<'b>(items: &[ListItem], formatting: &ListFormatting<'b>) -> Op result.push_str(formatting.separator); } - if tactic == ListTactic::Vertical && item.post_comment.is_some() { + if tactic == DefinitiveListTactic::Vertical && item.post_comment.is_some() { // 1 = space between item and comment. - let width = formatting.v_width.checked_sub(item_width + 1).unwrap_or(1); + let width = formatting.width.checked_sub(item_width + 1).unwrap_or(1); let mut offset = formatting.indent; offset.alignment += item_width + 1; let comment = item.post_comment.as_ref().unwrap(); @@ -286,7 +303,7 @@ pub fn write_list<'b>(items: &[ListItem], formatting: &ListFormatting<'b>) -> Op result.push_str(&formatted_comment); } - if !last && tactic == ListTactic::Vertical && item.new_lines { + if !last && tactic == DefinitiveListTactic::Vertical && item.new_lines { result.push('\n'); } } @@ -311,7 +328,7 @@ impl<'a, T, I, F1, F2, F3> Iterator for ListItems<'a, I, F1, F2, F3> where I: Iterator, F1: Fn(&T) -> BytePos, F2: Fn(&T) -> BytePos, - F3: Fn(&T) -> String + F3: Fn(&T) -> Option { type Item = ListItem; @@ -435,7 +452,7 @@ pub fn itemize_list<'a, T, I, F1, F2, F3>(codemap: &'a CodeMap, where I: Iterator, F1: Fn(&T) -> BytePos, F2: Fn(&T) -> BytePos, - F3: Fn(&T) -> String + F3: Fn(&T) -> Option { ListItems { codemap: codemap, @@ -449,25 +466,35 @@ pub fn itemize_list<'a, T, I, F1, F2, F3>(codemap: &'a CodeMap, } } -fn needs_trailing_separator(separator_tactic: SeparatorTactic, list_tactic: ListTactic) -> bool { +fn needs_trailing_separator(separator_tactic: SeparatorTactic, + list_tactic: DefinitiveListTactic) + -> bool { match separator_tactic { SeparatorTactic::Always => true, - SeparatorTactic::Vertical => list_tactic == ListTactic::Vertical, + SeparatorTactic::Vertical => list_tactic == DefinitiveListTactic::Vertical, SeparatorTactic::Never => false, } } -fn calculate_width(items: &[ListItem]) -> usize { - items.iter().map(total_item_width).fold(0, |a, l| a + l) +/// Returns the count and total width of the list items. +fn calculate_width<'li, I, T>(items: I) -> (usize, usize) + where I: IntoIterator, + T: AsRef +{ + items.into_iter() + .map(|item| total_item_width(item.as_ref())) + .fold((0, 0), |acc, l| (acc.0 + 1, acc.1 + l)) } fn total_item_width(item: &ListItem) -> usize { - comment_len(&item.pre_comment) + comment_len(&item.post_comment) + item.item.len() + comment_len(item.pre_comment.as_ref().map(|x| &(*x)[..])) + + comment_len(item.post_comment.as_ref().map(|x| &(*x)[..])) + + item.item.as_ref().map(|str| str.len()).unwrap_or(0) } -fn comment_len(comment: &Option) -> usize { - match *comment { - Some(ref s) => { +fn comment_len(comment: Option<&str>) -> usize { + match comment { + Some(s) => { let text_len = s.trim().len(); if text_len > 0 { // We'll put " /*" before and " */" after inline comments. diff --git a/src/types.rs b/src/types.rs index 2b757671a13..2cbbed92fa5 100644 --- a/src/types.rs +++ b/src/types.rs @@ -13,7 +13,7 @@ use syntax::print::pprust; use syntax::codemap::{self, Span, BytePos, CodeMap}; use Indent; -use lists::{itemize_list, write_list, ListFormatting}; +use lists::{format_item_list, itemize_list, format_fn_args}; use rewrite::{Rewrite, RewriteContext}; use utils::{extra_offset, span_after, format_mutability, wrap_str}; @@ -206,9 +206,7 @@ fn rewrite_segment(segment: &ast::PathSegment, .collect::>(); let next_span_lo = param_list.last().unwrap().get_span().hi + BytePos(1); - let list_lo = span_after(codemap::mk_sp(*span_lo, span_hi), - "<", - context.codemap); + let list_lo = span_after(codemap::mk_sp(*span_lo, span_hi), "<", context.codemap); let separator = get_path_separator(context.codemap, *span_lo, list_lo); // 1 for < @@ -221,20 +219,17 @@ fn rewrite_segment(segment: &ast::PathSegment, ">", |param| param.get_span().lo, |param| param.get_span().hi, - // FIXME(#133): write_list should call - // rewrite itself, because it has a better - // context. |seg| { seg.rewrite(context, context.config.max_width, offset + extra_offset) - .unwrap() }, list_lo, span_hi); - - let fmt = ListFormatting::for_item(list_width, offset + extra_offset, context.config); - let list_str = try_opt!(write_list(&items.collect::>(), &fmt)); + let list_str = try_opt!(format_item_list(items, + list_width, + offset + extra_offset, + context.config)); // Update position of last bracket. *span_lo = next_span_lo; @@ -260,12 +255,10 @@ fn rewrite_segment(segment: &ast::PathSegment, ")", |ty| ty.span.lo, |ty| ty.span.hi, - |ty| ty.rewrite(context, budget, offset).unwrap(), + |ty| ty.rewrite(context, budget, offset), list_lo, span_hi); - - let fmt = ListFormatting::for_fn(budget, offset, context.config); - let list_str = try_opt!(write_list(&items.collect::>(), &fmt)); + let list_str = try_opt!(format_fn_args(items, budget, offset, context.config)); format!("({}){}", list_str, output) } @@ -287,40 +280,39 @@ impl Rewrite for ast::WherePredicate { let type_str = try_opt!(bounded_ty.rewrite(context, width, offset)); if !bound_lifetimes.is_empty() { - let lifetime_str = bound_lifetimes.iter() - .map(|lt| { - lt.rewrite(context, width, offset) - .unwrap() - }) - .collect::>() - .join(", "); + let lifetime_str = try_opt!(bound_lifetimes.iter() + .map(|lt| { + lt.rewrite(context, + width, + offset) + }) + .collect::>>()) + .join(", "); // 8 = "for<> : ".len() let used_width = lifetime_str.len() + type_str.len() + 8; let budget = try_opt!(width.checked_sub(used_width)); - let bounds_str = bounds.iter() - .map(|ty_bound| { - ty_bound.rewrite(context, - budget, - offset + used_width) - .unwrap() - }) - .collect::>() - .join(" + "); + let bounds_str = try_opt!(bounds.iter() + .map(|ty_bound| { + ty_bound.rewrite(context, + budget, + offset + used_width) + }) + .collect::>>()) + .join(" + "); format!("for<{}> {}: {}", lifetime_str, type_str, bounds_str) } else { // 2 = ": ".len() let used_width = type_str.len() + 2; let budget = try_opt!(width.checked_sub(used_width)); - let bounds_str = bounds.iter() - .map(|ty_bound| { - ty_bound.rewrite(context, - budget, - offset + used_width) - .unwrap() - }) - .collect::>() - .join(" + "); + let bounds_str = try_opt!(bounds.iter() + .map(|ty_bound| { + ty_bound.rewrite(context, + budget, + offset + used_width) + }) + .collect::>>()) + .join(" + "); format!("{}: {}", type_str, bounds_str) } @@ -371,8 +363,7 @@ impl Rewrite for ast::TyParamBound { } ast::TyParamBound::TraitTyParamBound(ref tref, ast::TraitBoundModifier::Maybe) => { let budget = try_opt!(width.checked_sub(1)); - Some(format!("?{}", - try_opt!(tref.rewrite(context, budget, offset + 1)))) + Some(format!("?{}", try_opt!(tref.rewrite(context, budget, offset + 1)))) } ast::TyParamBound::RegionTyParamBound(ref l) => { Some(pprust::lifetime_to_string(l)) @@ -383,9 +374,9 @@ impl Rewrite for ast::TyParamBound { impl Rewrite for ast::TyParamBounds { fn rewrite(&self, context: &RewriteContext, width: usize, offset: Indent) -> Option { - let strs: Vec<_> = self.iter() - .map(|b| b.rewrite(context, width, offset).unwrap()) - .collect(); + let strs: Vec<_> = try_opt!(self.iter() + .map(|b| b.rewrite(context, width, offset)) + .collect()); Some(strs.join(" + ")) } } @@ -398,10 +389,10 @@ impl Rewrite for ast::TyParam { if !self.bounds.is_empty() { result.push_str(": "); - let bounds = self.bounds - .iter() - .map(|ty_bound| ty_bound.rewrite(context, width, offset).unwrap()) - .collect::>() + let bounds = try_opt!(self.bounds + .iter() + .map(|ty_bound| ty_bound.rewrite(context, width, offset)) + .collect::>>()) .join(" + "); result.push_str(&bounds); @@ -421,10 +412,10 @@ impl Rewrite for ast::TyParam { impl Rewrite for ast::PolyTraitRef { fn rewrite(&self, context: &RewriteContext, width: usize, offset: Indent) -> Option { if !self.bound_lifetimes.is_empty() { - let lifetime_str = self.bound_lifetimes - .iter() - .map(|lt| lt.rewrite(context, width, offset).unwrap()) - .collect::>() + let lifetime_str = try_opt!(self.bound_lifetimes + .iter() + .map(|lt| lt.rewrite(context, width, offset)) + .collect::>>()) .join(", "); // 6 is "for<> ".len() let extra_offset = lifetime_str.len() + 6; diff --git a/src/visitor.rs b/src/visitor.rs index 21dc2cde2a5..69217034d00 100644 --- a/src/visitor.rs +++ b/src/visitor.rs @@ -319,9 +319,7 @@ impl<'a> FmtVisitor<'a> { } fn format_mod(&mut self, m: &ast::Mod, s: Span, ident: ast::Ident) { - debug!("FmtVisitor::format_mod: ident: {:?}, span: {:?}", - ident, - s); + debug!("FmtVisitor::format_mod: ident: {:?}, span: {:?}", ident, s); // Decide whether this is an inline mod or an external mod. let local_file_name = self.codemap.span_to_filename(s); @@ -359,9 +357,7 @@ impl<'a> FmtVisitor<'a> { overflow_indent: Indent::empty(), }; // 1 = ";" - match vp.rewrite(&context, - self.config.max_width - offset.width() - 1, - offset) { + match vp.rewrite(&context, self.config.max_width - offset.width() - 1, offset) { Some(ref s) if s.is_empty() => { // Format up to last newline let prev_span = codemap::mk_sp(self.last_pos, span.lo);