Merge pull request #404 from marcusklaas/rewrite-list-fmt

List formatting refactoring

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"
+version = "0.1.7"
-source = "git+https://github.com/utkarshkukreti/diff.rs.git#1921576a73e1b50a0ecb26c8ce62eefb26d273b4"
+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]]

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"),
+        assert_eq!(&uncommented("abc \" /* */\" qsdf"), "abc \" /* */\" qsdf");
-                   "abc \" /* */\" qsdf");
     }
 
     #[test]

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";

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(),
+                wrap_str("return".to_owned(), context.config.max_width, width, offset)
-                         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
+                             |item| item.rewrite(context, max_item_width, offset + 1),
-                             // 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))
-                             },
                              span_after(span, "[", context.codemap),
                              span.hi)
                     .collect::<Vec<_>>();
 
-    let tactic = if items.iter().any(|li| li.item.len() > 10 || li.is_multiline()) {
+    let has_long_item = try_opt!(items.iter()
-        ListTactic::HorizontalVertical
+                                      .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,
+        width: max_item_width,
-        v_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| {
+                                 |arg| arg.rewrite(context, budget, argument_offset),
-                                     // 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)))
-                                        })
-                                 },
                                  span_after(span, "|", context.codemap),
                                  body.span.lo);
+    let item_vec = arg_items.collect::<Vec<_>>();
+    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,
+        width: budget,
-        v_width: budget,
         ends_with_newline: false,
         config: context.config,
     };
-    let list_str = try_opt!(write_list(&arg_items.collect::<Vec<_>>(), &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]),
+    let last_span = mk_sp(arm_end_pos(&arms[arms.len() - 1]), span.hi - BytePos(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,
+            let offset = Indent::new(offset.block_indent, line_start + 4 - 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<R>(context: &RewriteContext,
                              ")",
                              |item| item.span.lo,
                              |item| item.span.hi,
-                             // Take old span when rewrite fails.
+                             |item| item.rewrite(&inner_context, remaining_width, offset),
-                             |item| {
-                                 item.rewrite(&inner_context, remaining_width, offset)
-                                     .unwrap_or(context.snippet(item.span))
-                             },
                              span.lo,
                              span.hi);
 
-    let fmt = ListFormatting::for_fn(remaining_width, offset, context.config);
+    let list_str = match format_fn_args(items, remaining_width, offset, context.config) {
-    let list_str = match write_list(&items.collect::<Vec<_>>(), &fmt) {
         Some(str) => str,
         None => return Err(Ordering::Less),
     };
@@ -1178,9 +1164,7 @@ fn rewrite_paren(context: &RewriteContext,
                  width: usize,
                  offset: Indent)
                  -> Option<String> {
-    debug!("rewrite_paren, width: {}, offset: {:?}",
+    debug!("rewrite_paren, width: {}, offset: {:?}", 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<String> {
-    debug!("rewrite_struct_lit: width {}, offset {:?}",
+    debug!("rewrite_struct_lit: width {}, offset {:?}", 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,
+                                                      try_opt!(v_budget.checked_sub(2)),
                                                       indent + 2)
-                                                         })
+                                             .map(|s| format!("..{}", s))
-                                                         .unwrap_or(context.snippet(expr.span)))
                                      }
                                  }
                              },
                              span_after(span, "{", context.codemap),
                              span.hi);
+    let item_vec = items.collect::<Vec<_>>();
 
-    let mut tactic = match (context.config.struct_lit_style, fields.len()) {
+    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 tactic == ListTactic::HorizontalVertical && fields.len() > 1 {
+        if prelim_tactic == ListTactic::HorizontalVertical && fields.len() > 1 {
-        tactic = ListTactic::LimitedHorizontalVertical(context.config.struct_lit_width);
+            prelim_tactic = ListTactic::LimitedHorizontalVertical(context.config.struct_lit_width);
-    }
+        };
+
+        definitive_tactic(&item_vec, prelim_tactic, h_budget)
+    };
+
+    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,
+        width: budget,
-        v_width: v_budget,
         ends_with_newline: false,
         config: context.config,
     };
-    let fields_str = try_opt!(write_list(&items.collect::<Vec<_>>(), &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<String> {
-    debug!("rewrite_tuple_lit: width: {}, offset: {:?}",
+    debug!("rewrite_tuple_lit: width: {}, offset: {:?}", 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!(format_fn_args(items, budget, indent, context.config));
-    let list_str = try_opt!(write_list(&items.collect::<Vec<_>>(), &fmt));
 
     Some(format!("({})", list_str))
 }

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,
+            print_diff(diff, |line_num| format!("\nDiff at line {}:", line_num));
-                       |line_num| format!("\nDiff at line {}:", line_num));
         }
         WriteMode::Return => {
             // io::Write is not implemented for String, working around with

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<String> {
     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<ListItem>) -> 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

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,
+        assert_eq!(None, text.chars().position(|c| seeker.inspect(c).is_some()));
-                   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",
+    assert!(!is_bad_issue("bad FIXME\n", ReportTactic::Always, ReportTactic::Never));
-                          ReportTactic::Always,
-                          ReportTactic::Never));
 }
 
 #[test]

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,
+                        .rewrite(&context, self.config.max_width - indent.width(), indent)
-                                 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,
+            width: budget,
-            v_width: multi_line_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
+                                                 arg.ty.rewrite(&self.get_context(),
-                                                    .rewrite(&self.get_context(),
                                                                 1000,
                                                                 Indent::empty())
-                                                    .unwrap()
                                              },
                                              span_after(field.span, "(", self.codemap),
                                              next_span_start);
+                    let item_vec = items.collect::<Vec<_>>();
 
                     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,
+                        width: budget,
-                        v_width: budget,
                         ends_with_newline: true,
                         config: self.config,
                     };
-                    let list_str = match write_list(&items.collect::<Vec<_>>(), &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,
+            width: budget,
-            v_width: budget,
             ends_with_newline: true,
             config: self.config,
         };
 
-        let list_str = try_opt!(write_list(&items.collect::<Vec<_>>(), &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<String> {
         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
+        let mut attr_str = try_opt!(field.node
                                          .attrs
                                          .rewrite(&self.get_context(),
                                                   self.config.max_width - indent.width(),
-                                         indent)
+                                                  indent));
-                                .unwrap();
         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));
-        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));
-        let ty_strs = tys.iter()
-                         .map(|ty_param| ty_param.rewrite(&context, h_budget, offset).unwrap());
 
         // 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.lo,
-                                 |sp| sp.hi,
+                                 |&(sp, _)| sp.hi,
-                                 |_| String::new(),
+                                 // FIXME: don't clone
+                                 |&(_, ref str)| str.clone(),
                                  span_after(span, "<", self.codemap),
                                  span.hi);
-        let mut items = items.collect::<Vec<_>>();
+        let list_str = try_opt!(::lists::format_item_list(items, h_budget, offset, self.config));
-
-        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));
 
         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
+                                 |pred| pred.rewrite(&context, budget, offset),
-                                 // this will be taken care of when write_list
-                                 // takes Rewrite object: see issue #133
-                                 |pred| pred.rewrite(&context, budget, offset).unwrap(),
                                  span_start,
                                  span_end);
+        let item_vec = items.collect::<Vec<_>>();
+        // 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,
+            width: budget,
-            v_width: budget,
             ends_with_newline: true,
             config: self.config,
         };
-        let preds_str = try_opt!(write_list(&items.collect::<Vec<_>>(), &fmt));
+        let preds_str = try_opt!(write_list(&item_vec, &fmt));
 
         // 9 = " where ".len() + " {".len()
         if density == Density::Tall || preds_str.contains('\n') ||

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.block_indent / config.tab_spaces, self.alignment)
-             self.alignment)
         } else {
             (0, self.block_indent + self.alignment)
         };

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 width: usize,
-    pub h_width: usize,
-    // Available width if we layout vertically
-    pub v_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 format_fn_args<I>(items: I, width: usize, offset: Indent, config: &Config) -> Option<String>
-    pub fn for_fn(width: usize, offset: Indent, config: &'a Config) -> ListFormatting<'a> {
+    where I: Iterator<Item = ListItem>
-        ListFormatting {
+{
-            tactic: ListTactic::LimitedHorizontalVertical(config.fn_call_width),
+    list_helper(items,
-            separator: ",",
+                width,
-            trailing_separator: SeparatorTactic::Never,
+                offset,
-            indent: offset,
+                config,
-            h_width: width,
+                ListTactic::LimitedHorizontalVertical(config.fn_call_width))
-            v_width: width,
-            ends_with_newline: false,
-            config: config,
-        }
 }
 
-    pub fn for_item(width: usize, offset: Indent, config: &'a Config) -> ListFormatting<'a> {
+pub fn format_item_list<I>(items: I,
-        ListFormatting {
+                           width: usize,
-            tactic: ListTactic::HorizontalVertical,
+                           offset: Indent,
+                           config: &Config)
+                           -> Option<String>
+    where I: Iterator<Item = ListItem>
+{
+    list_helper(items, width, offset, config, ListTactic::HorizontalVertical)
+}
+
+fn list_helper<I>(items: I,
+                  width: usize,
+                  offset: Indent,
+                  config: &Config,
+                  tactic: ListTactic)
+                  -> Option<String>
+    where I: Iterator<Item = ListItem>
+{
+    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,
-            h_width: width,
+        width: width,
-            v_width: width,
         ends_with_newline: false,
         config: config,
+    };
+
+    write_list(&item_vec, &fmt)
 }
+
+impl AsRef<ListItem> for ListItem {
+    fn as_ref(&self) -> &ListItem {
+        self
     }
 }
 
 pub struct ListItem {
+    // None for comments mean that they are not present.
     pub pre_comment: Option<String>,
-    // Item should include attributes and doc comments.
+    // Item should include attributes and doc comments. None indicates a failed
-    pub item: String,
+    // rewrite.
+    pub item: Option<String>,
     pub post_comment: Option<String>,
     // 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: Into<String>>(s: S) -> ListItem {
         ListItem {
             pre_comment: None,
-            item: s.into(),
+            item: Some(s.into()),
             post_comment: None,
             new_lines: false,
         }
     }
 }
 
+#[derive(Eq, PartialEq, Debug, Copy, Clone)]
+/// The definitive formatting tactic for lists.
+pub enum DefinitiveListTactic {
+    Vertical,
+    Horizontal,
+    Mixed,
+}
+
+pub fn definitive_tactic<'t, I, T>(items: I,
+                                   tactic: ListTactic,
+                                   width: usize)
+                                   -> DefinitiveListTactic
+    where I: IntoIterator<Item = T> + Clone,
+          T: AsRef<ListItem>
+{
+    let pre_line_comments = items.clone()
+                                 .into_iter()
+                                 .any(|item| item.as_ref().has_line_pre_comment());
+
+    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.
-// FIXME: this has grown into a monstrosity
 // TODO: add unit tests
-pub fn write_list<'b>(items: &[ListItem], formatting: &ListFormatting<'b>) -> Option<String> {
+pub fn write_list<'b, I, T>(items: I, formatting: &ListFormatting<'b>) -> Option<String>
-    if items.is_empty() {
+    where I: IntoIterator<Item = T>,
-        return Some(String::new());
+          T: AsRef<ListItem>
-    }
+{
-
+    let tactic = formatting.tactic;
-    let mut tactic = formatting.tactic;
-
-    // Conservatively overestimates because of the changing separator tactic.
-    let sep_count = if formatting.trailing_separator == SeparatorTactic::Always {
-        items.len()
-    } else {
-        items.len() - 1
-    };
     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);
-
+    let mut result = String::new();
-    // Create a buffer for the result.
+    let mut iter = items.into_iter().enumerate().peekable();
-    // 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 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<Item = T>,
           F1: Fn(&T) -> BytePos,
           F2: Fn(&T) -> BytePos,
-          F3: Fn(&T) -> String
+          F3: Fn(&T) -> Option<String>
 {
     type Item = ListItem;
 
@@ -435,7 +452,7 @@ pub fn itemize_list<'a, T, I, F1, F2, F3>(codemap: &'a CodeMap,
     where I: Iterator<Item = T>,
           F1: Fn(&T) -> BytePos,
           F2: Fn(&T) -> BytePos,
-          F3: Fn(&T) -> String
+          F3: Fn(&T) -> Option<String>
 {
     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 {
+/// Returns the count and total width of the list items.
-    items.iter().map(total_item_width).fold(0, |a, l| a + l)
+fn calculate_width<'li, I, T>(items: I) -> (usize, usize)
+    where I: IntoIterator<Item = T>,
+          T: AsRef<ListItem>
+{
+    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<String>) -> usize {
+fn comment_len(comment: Option<&str>) -> usize {
-    match *comment {
+    match comment {
-        Some(ref s) => {
+        Some(s) => {
             let text_len = s.trim().len();
             if text_len > 0 {
                 // We'll put " /*" before and " */" after inline comments.

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::<Vec<_>>();
 
             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),
+            let list_lo = span_after(codemap::mk_sp(*span_lo, span_hi), "<", context.codemap);
-                                     "<",
-                                     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 list_str = try_opt!(format_item_list(items,
-            let fmt = ListFormatting::for_item(list_width, offset + extra_offset, context.config);
+                                                     list_width,
-            let list_str = try_opt!(write_list(&items.collect::<Vec<_>>(), &fmt));
+                                                     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 list_str = try_opt!(format_fn_args(items, budget, offset, context.config));
-            let fmt = ListFormatting::for_fn(budget, offset, context.config);
-            let list_str = try_opt!(write_list(&items.collect::<Vec<_>>(), &fmt));
 
             format!("({}){}", list_str, output)
         }
@@ -287,24 +280,24 @@ 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()
+                    let lifetime_str = try_opt!(bound_lifetimes.iter()
                                                                .map(|lt| {
-                                                          lt.rewrite(context, width, offset)
+                                                                   lt.rewrite(context,
-                                                            .unwrap()
+                                                                              width,
+                                                                              offset)
                                                                })
-                                                      .collect::<Vec<_>>()
+                                                               .collect::<Option<Vec<_>>>())
                                            .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()
+                    let bounds_str = try_opt!(bounds.iter()
                                                     .map(|ty_bound| {
                                                         ty_bound.rewrite(context,
                                                                          budget,
                                                                          offset + used_width)
-                                                       .unwrap()
                                                     })
-                                           .collect::<Vec<_>>()
+                                                    .collect::<Option<Vec<_>>>())
                                          .join(" + ");
 
                     format!("for<{}> {}: {}", lifetime_str, type_str, bounds_str)
@@ -312,14 +305,13 @@ impl Rewrite for ast::WherePredicate {
                     // 2 = ": ".len()
                     let used_width = type_str.len() + 2;
                     let budget = try_opt!(width.checked_sub(used_width));
-                    let bounds_str = bounds.iter()
+                    let bounds_str = try_opt!(bounds.iter()
                                                     .map(|ty_bound| {
                                                         ty_bound.rewrite(context,
                                                                          budget,
                                                                          offset + used_width)
-                                                       .unwrap()
                                                     })
-                                           .collect::<Vec<_>>()
+                                                    .collect::<Option<Vec<_>>>())
                                          .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!("?{}",
+                Some(format!("?{}", try_opt!(tref.rewrite(context, budget, offset + 1))))
-                             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<String> {
-        let strs: Vec<_> = self.iter()
+        let strs: Vec<_> = try_opt!(self.iter()
-                               .map(|b| b.rewrite(context, width, offset).unwrap())
+                                        .map(|b| b.rewrite(context, width, offset))
-                               .collect();
+                                        .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
+            let bounds = try_opt!(self.bounds
                                       .iter()
-                             .map(|ty_bound| ty_bound.rewrite(context, width, offset).unwrap())
+                                      .map(|ty_bound| ty_bound.rewrite(context, width, offset))
-                             .collect::<Vec<_>>()
+                                      .collect::<Option<Vec<_>>>())
                              .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<String> {
         if !self.bound_lifetimes.is_empty() {
-            let lifetime_str = self.bound_lifetimes
+            let lifetime_str = try_opt!(self.bound_lifetimes
                                             .iter()
-                                   .map(|lt| lt.rewrite(context, width, offset).unwrap())
+                                            .map(|lt| lt.rewrite(context, width, offset))
-                                   .collect::<Vec<_>>()
+                                            .collect::<Option<Vec<_>>>())
                                    .join(", ");
             // 6 is "for<> ".len()
             let extra_offset = lifetime_str.len() + 6;

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: {:?}",
+        debug!("FmtVisitor::format_mod: ident: {:?}, span: {:?}", ident, s);
-               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,
+        match vp.rewrite(&context, self.config.max_width - offset.width() - 1, offset) {
-                         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);