From a76df6b4d9a67e67137cf0773d96f0027da827c7 Mon Sep 17 00:00:00 2001 From: Marcus Klaas Date: Sun, 4 Oct 2015 20:20:15 +0200 Subject: [PATCH] Make listItem contain option --- src/config.rs | 1 - src/expr.rs | 44 ++++++++--------------------- src/imports.rs | 8 ++---- src/items.rs | 49 +++++++++++++++----------------- src/lists.rs | 29 +++++++++++-------- src/types.rs | 77 +++++++++++++++++++++++--------------------------- 6 files changed, 92 insertions(+), 116 deletions(-) diff --git a/src/config.rs b/src/config.rs index 14d46a76834..1688b4b363e 100644 --- a/src/config.rs +++ b/src/config.rs @@ -90,7 +90,6 @@ pub fn to_list_tactic(self) -> ListTactic { FileLicense, } -// TODO: this is not necessary any more configuration_option_enum! { MultilineStyle: // Use horizontal layout if it fits in one line, fall back to vertical PreferSingle, diff --git a/src/expr.rs b/src/expr.rs index d51bd256caa..53e24b70984 100644 --- a/src/expr.rs +++ b/src/expr.rs @@ -218,18 +218,16 @@ 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()) { + 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(|li| li.is_multiline()) { definitive_tactic(&items, ListTactic::HorizontalVertical, max_item_width) } else { DefinitiveListTactic::Mixed @@ -280,15 +278,7 @@ 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::>(); @@ -1151,11 +1141,7 @@ 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); @@ -1251,18 +1237,13 @@ enum StructLitField<'a> { 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)) } } }, @@ -1365,7 +1346,6 @@ 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)); diff --git a/src/imports.rs b/src/imports.rs index 9c29ece623f..2c518378b36 100644 --- a/src/imports.rs +++ b/src/imports.rs @@ -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 { @@ -142,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 @@ -181,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/items.rs b/src/items.rs index c2f547162be..e2f59f0fb6a 100644 --- a/src/items.rs +++ b/src/items.rs @@ -481,7 +481,7 @@ fn rewrite_args(&self, ")", |arg| span_lo_for_arg(arg), |arg| arg.ty.span.hi, - |_| String::new(), + |_| None, comment_span_start, span.hi); @@ -491,7 +491,7 @@ fn rewrite_args(&self, 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 { @@ -630,11 +630,9 @@ fn visit_variant(&mut self, field: &ast::Variant, last_field: bool, next_span_st |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); @@ -863,9 +861,14 @@ fn format_generics(&self, } // 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 { @@ -877,24 +880,23 @@ fn format_field(&self, field: &ast::StructField) -> String { 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, @@ -923,10 +925,8 @@ fn rewrite_generics(&self, // 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| { @@ -988,10 +988,7 @@ fn rewrite_where_clause(&self, "{", |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::>(); diff --git a/src/lists.rs b/src/lists.rs index 296df76e688..514acd13f53 100644 --- a/src/lists.rs +++ b/src/lists.rs @@ -43,8 +43,6 @@ pub enum SeparatorTactic { impl_enum_decodable!(SeparatorTactic, Always, Never, Vertical); -// TODO having some helpful ctors for ListFormatting would be nice. -// FIXME: this should have only 1 width param pub struct ListFormatting<'a> { pub tactic: DefinitiveListTactic, pub separator: &'a str, @@ -111,9 +109,11 @@ fn as_ref(&self) -> &ListItem { } 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 failed + // rewrite. + pub item: Option, pub post_comment: Option, // Whether there is extra whitespace before this item. pub new_lines: bool, @@ -121,7 +121,9 @@ pub struct ListItem { impl ListItem { pub fn is_multiline(&self) -> bool { - self.item.contains('\n') || self.pre_comment.is_some() || + // FIXME: fail earlier! + 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) } @@ -132,7 +134,7 @@ pub fn has_line_pre_comment(&self) -> bool { pub fn from_str>(s: S) -> ListItem { ListItem { pre_comment: None, - item: s.into(), + item: Some(s.into()), post_comment: None, new_lines: false, } @@ -198,6 +200,7 @@ pub fn write_list<'b, I, T>(items: I, formatting: &ListFormatting<'b>) -> Option let indent_str = &formatting.indent.to_string(formatting.config); 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 = iter.peek().is_none(); let separate = !last || trailing_separator; @@ -206,7 +209,7 @@ pub fn write_list<'b, I, T>(items: I, formatting: &ListFormatting<'b>) -> Option } else { 0 }; - let item_width = item.item.len() + item_sep_len; + let item_width = inner_item.len() + item_sep_len; match tactic { DefinitiveListTactic::Horizontal if !first => { @@ -256,7 +259,8 @@ pub fn write_list<'b, I, T>(items: I, formatting: &ListFormatting<'b>) -> Option } } - 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.width, formatting.indent)); @@ -325,7 +329,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; @@ -449,7 +453,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, @@ -484,8 +488,11 @@ fn calculate_width<'li, I, T>(items: I) -> (usize, usize) } fn total_item_width(item: &ListItem) -> usize { + // FIXME: If the item has a `None` item, it may be better to fail earlier + // rather than later. comment_len(item.pre_comment.as_ref().map(|x| &(*x)[..])) + - comment_len(item.post_comment.as_ref().map(|x| &(*x)[..])) + item.item.len() + 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<&str>) -> usize { diff --git a/src/types.rs b/src/types.rs index 9fe837fced4..6f6afbfc42e 100644 --- a/src/types.rs +++ b/src/types.rs @@ -219,14 +219,10 @@ 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); @@ -259,7 +255,7 @@ 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!(::lists::format_fn_args(items, budget, offset, context.config)); @@ -284,40 +280,39 @@ fn rewrite(&self, context: &RewriteContext, width: usize, offset: Indent) -> Opt 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) } @@ -380,9 +375,9 @@ fn rewrite(&self, context: &RewriteContext, width: usize, offset: Indent) -> Opt 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(" + ")) } } @@ -395,10 +390,10 @@ fn rewrite(&self, context: &RewriteContext, width: usize, offset: Indent) -> Opt 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); @@ -418,10 +413,10 @@ fn rewrite(&self, context: &RewriteContext, width: usize, offset: Indent) -> Opt 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; -- 2.44.0