From c680bb4030784ca15cc7d6c39caf297c9fb010e4 Mon Sep 17 00:00:00 2001 From: Marcus Klaas Date: Fri, 11 Sep 2015 00:52:16 +0200 Subject: [PATCH 01/11] Implement basic chain formatting --- src/chains.rs | 133 ++++++++++++++++++++++++++++++++++++++++++++ src/comment.rs | 45 +++++++++++---- src/expr.rs | 109 ++++++++++++++++++++---------------- src/items.rs | 59 ++++++++++---------- src/lib.rs | 1 + src/lists.rs | 28 +++++----- src/missed_spans.rs | 25 ++++----- src/string.rs | 4 +- src/types.rs | 56 +++++++++---------- src/utils.rs | 22 ++++++-- src/visitor.rs | 24 ++------ tests/system.rs | 30 +++++----- 12 files changed, 352 insertions(+), 184 deletions(-) create mode 100644 src/chains.rs diff --git a/src/chains.rs b/src/chains.rs new file mode 100644 index 00000000000..094cd358696 --- /dev/null +++ b/src/chains.rs @@ -0,0 +1,133 @@ +// Copyright 2015 The Rust Project Developers. See the COPYRIGHT +// file at the top-level directory of this distribution and at +// http://rust-lang.org/COPYRIGHT. +// +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +use rewrite::{Rewrite, RewriteContext}; +use utils::{span_after, make_indent, extra_offset}; +use expr::rewrite_call; + +use syntax::{ast, ptr}; +use syntax::codemap::{mk_sp, Span}; +use syntax::print::pprust; + +pub fn rewrite_chain(orig_expr: &ast::Expr, + context: &RewriteContext, + width: usize, + offset: usize) + -> Option { + let mut expr = orig_expr; + let mut rewrites = Vec::new(); + let indent = context.block_indent + context.config.tab_spaces; + let max_width = context.config.max_width - context.config.tab_spaces; + + loop { + match expr.node { + ast::Expr_::ExprMethodCall(ref method_name, ref types, ref expressions) => { + // FIXME: a lot of duplication between this and the + // rewrite_method_call in expr.rs. + let new_span = mk_sp(expressions[0].span.hi, expr.span.hi); + let lo = span_after(new_span, "(", context.codemap); + let new_span = mk_sp(lo, expr.span.hi); + + let rewrite = rewrite_method_call(method_name.node, + types, + &expressions[1..], + new_span, + context, + max_width, + indent); + rewrites.push(try_opt!(rewrite)); + expr = &expressions[0]; + } + ast::Expr_::ExprField(ref subexpr, ref field) => { + expr = subexpr; + rewrites.push(format!(".{}", field.node)); + } + ast::Expr_::ExprTupField(ref subexpr, ref field) => { + expr = subexpr; + rewrites.push(format!(".{}", field.node)); + } + _ => break, + } + } + + let parent_rewrite = try_opt!(expr.rewrite(context, width, offset)); + + // TODO: add exception for when rewrites.len() == 1 + if rewrites.len() == 1 { + let extra_offset = extra_offset(&parent_rewrite, offset); + let max_width = try_opt!(width.checked_sub(extra_offset)); + // FIXME: massive duplication + let rerewrite = match orig_expr.node { + ast::Expr_::ExprMethodCall(ref method_name, ref types, ref expressions) => { + let new_span = mk_sp(expressions[0].span.hi, orig_expr.span.hi); + let lo = span_after(new_span, "(", context.codemap); + let new_span = mk_sp(lo, orig_expr.span.hi); + + rewrite_method_call(method_name.node, + types, + &expressions[1..], + new_span, + context, + max_width, + offset + extra_offset) + } + ast::Expr_::ExprField(_, ref field) => { + Some(format!(".{}", field.node)) + } + ast::Expr_::ExprTupField(_, ref field) => { + Some(format!(".{}", field.node)) + } + _ => unreachable!(), + }; + + return Some(format!("{}{}", parent_rewrite, try_opt!(rerewrite))); + } + + let total_width = rewrites.iter().fold(0, |a, b| a + b.len()) + parent_rewrite.len(); + + let connector = if total_width <= width && rewrites.iter().all(|s| !s.contains('\n')) { + String::new() + } else { + format!("\n{}", make_indent(indent)) + }; + + // FIXME: don't do this. There's a more efficient way. VecDeque? + rewrites.reverse(); + + // Put the first link on the same line as parent, if it fits. + let first_connector = if parent_rewrite.len() + rewrites[0].len() <= width && + !rewrites[0].contains('\n') { + "" + } else { + &connector[..] + }; + + Some(format!("{}{}{}", parent_rewrite, first_connector, rewrites.join(&connector))) +} + +fn rewrite_method_call(method_name: ast::Ident, + types: &[ptr::P], + args: &[ptr::P], + span: Span, + context: &RewriteContext, + width: usize, + offset: usize) + -> Option { + let type_str = if types.is_empty() { + String::new() + } else { + let type_list = types.iter().map(|ty| pprust::ty_to_string(ty)).collect::>(); + format!("::<{}>", type_list.join(", ")) + }; + + let callee_str = format!(".{}{}", method_name, type_str); + + rewrite_call(context, &callee_str, args, span, width, offset) +} diff --git a/src/comment.rs b/src/comment.rs index f1cecd7318b..5cec1678ac2 100644 --- a/src/comment.rs +++ b/src/comment.rs @@ -25,8 +25,7 @@ pub fn rewrite_comment(orig: &str, block_style: bool, width: usize, offset: usiz ("// ", "", "// ") }; - let max_chars = width.checked_sub(closer.len()).unwrap_or(1) - .checked_sub(opener.len()).unwrap_or(1); + let max_chars = width.checked_sub(closer.len() + opener.len()).unwrap_or(1); let fmt = StringFormat { opener: "", @@ -41,17 +40,18 @@ pub fn rewrite_comment(orig: &str, block_style: bool, width: usize, offset: usiz let indent_str = make_indent(offset); let line_breaks = s.chars().filter(|&c| c == '\n').count(); - let (_, mut s) = s.lines().enumerate() + let (_, mut s) = s.lines() + .enumerate() .map(|(i, mut line)| { - line = line.trim(); + line = line.trim(); // Drop old closer. - if i == line_breaks && line.ends_with("*/") && !line.starts_with("//") { - line = &line[..(line.len() - 2)]; - } + if i == line_breaks && line.ends_with("*/") && !line.starts_with("//") { + line = &line[..(line.len() - 2)]; + } - line.trim_right() - }) + line.trim_right() + }) .map(left_trim_comment_line) .map(|line| { if line_breaks == 0 { @@ -160,9 +160,34 @@ fn comment_end() { /// Returns true if text contains any comment. pub fn contains_comment(text: &str) -> bool { - CharClasses::new(text.chars()).any(|(kind, _)| kind == CodeCharKind::Comment ) + CharClasses::new(text.chars()).any(|(kind, _)| kind == CodeCharKind::Comment) } +pub fn uncommented(text: &str) -> String { + CharClasses::new(text.chars()) + .filter_map(|(s, c)| { + match s { + CodeCharKind::Normal => Some(c), + CodeCharKind::Comment => None, + } + }) + .collect() +} + +#[test] +fn test_uncommented() { + assert_eq!(&uncommented("abc/*...*/"), "abc"); + assert_eq!(&uncommented("// .... /* \n../* /* *** / */ */a/* // */c\n"), "..ac\n"); + assert_eq!(&uncommented("abc \" /* */\" qsdf"), "abc \" /* */\" qsdf"); +} + +#[test] +fn test_contains_comment() { + assert_eq!(contains_comment("abc"), false); + assert_eq!(contains_comment("abc // qsdf"), true); + assert_eq!(contains_comment("abc /* kqsdf"), true); + assert_eq!(contains_comment("abc \" /* */\" qsdf"), false); +} struct CharClasses where T: Iterator, diff --git a/src/expr.rs b/src/expr.rs index e641d7c6762..2c77cdc9e78 100644 --- a/src/expr.rs +++ b/src/expr.rs @@ -9,6 +9,7 @@ // except according to those terms. use std::cmp::Ordering; +use std::borrow::Borrow; use rewrite::{Rewrite, RewriteContext}; use lists::{write_list, itemize_list, ListFormatting, SeparatorTactic, ListTactic}; @@ -21,6 +22,7 @@ use config::{BlockIndentStyle, MultilineStyle}; use comment::{FindUncommented, rewrite_comment, contains_comment}; use types::rewrite_path; use items::{span_lo_for_arg, span_hi_for_arg, rewrite_fn_input}; +use chains::rewrite_chain; use syntax::{ast, ptr}; use syntax::codemap::{CodeMap, Span, BytePos, mk_sp}; @@ -38,7 +40,16 @@ impl Rewrite for ast::Expr { } } ast::Expr_::ExprCall(ref callee, ref args) => { - rewrite_call(context, callee, args, self.span, width, offset) + // // FIXME using byte lens instead of char lens (and probably all over the place too) + // // 2 is for parens + // let max_callee_width = try_opt!(width.checked_sub(2)); + // let callee_str = try_opt!(callee.rewrite(context, max_callee_width, offset)); + + // let new_span = mk_sp(callee.span.hi, self.span.hi); + // let lo = span_after(new_span, "(", context.codemap); + // let new_span = mk_sp(lo, self.span.hi); + + rewrite_call(context, &**callee, args, self.span, width, offset) } ast::Expr_::ExprParen(ref subexpr) => { rewrite_paren(context, subexpr, width, offset) @@ -137,6 +148,11 @@ impl Rewrite for ast::Expr { ast::Expr_::ExprClosure(capture, ref fn_decl, ref body) => { rewrite_closure(capture, fn_decl, body, self.span, context, width, offset) } + ast::Expr_::ExprField(..) | + ast::Expr_::ExprTupField(..) | + ast::Expr_::ExprMethodCall(..) => { + rewrite_chain(self, context, width, offset) + } // We do not format these expressions yet, but they should still // satisfy our width restrictions. _ => wrap_str(context.snippet(self.span), context.config.max_width, width, offset), @@ -393,9 +409,9 @@ impl<'a> Rewrite for Loop<'a> { }; // FIXME: this drops any comment between "loop" and the block. - self.block.rewrite(context, width, offset).map(|result| { - format!("{}{}{} {}", label_string, self.keyword, pat_expr_string, result) - }) + self.block + .rewrite(context, width, offset) + .map(|result| format!("{}{}{} {}", label_string, self.keyword, pat_expr_string, result)) } } @@ -762,9 +778,7 @@ fn rewrite_guard(context: &RewriteContext, // 4 = ` if `, 5 = ` => {` let overhead = pattern_width + 4 + 5; if overhead < width { - let cond_str = guard.rewrite(context, - width - overhead, - offset + pattern_width + 4); + let cond_str = guard.rewrite(context, width - overhead, offset + pattern_width + 4); if let Some(cond_str) = cond_str { return Some(format!(" if {}", cond_str)); } @@ -866,36 +880,39 @@ fn rewrite_string_lit(context: &RewriteContext, Some(rewrite_string(str_lit, &fmt)) } -fn rewrite_call(context: &RewriteContext, - callee: &ast::Expr, - args: &[ptr::P], - span: Span, - width: usize, - offset: usize) - -> Option { - let callback = |callee_max_width| { - rewrite_call_inner(context, - callee, - callee_max_width, - args, - span, - width, - offset) - }; - +pub fn rewrite_call(context: &RewriteContext, + callee: &R, + args: &[ptr::P], + span: Span, + width: usize, + offset: usize) + -> Option + where R: Rewrite +{ // 2 is for parens let max_width = try_opt!(width.checked_sub(2)); - binary_search(1, max_width, callback) + binary_search(1, max_width, |callee_max_width| { + rewrite_call_inner(context, + callee, + callee_max_width, + args, + span, + width, + offset) + }) } -fn rewrite_call_inner(context: &RewriteContext, - callee: &ast::Expr, - max_callee_width: usize, - args: &[ptr::P], - span: Span, - width: usize, - offset: usize) - -> Result { +fn rewrite_call_inner(context: &RewriteContext, + callee: &R, + max_callee_width: usize, + args: &[ptr::P], + span: Span, + width: usize, + offset: usize) + -> Result + where R: Rewrite +{ + let callee = callee.borrow(); // FIXME using byte lens instead of char lens (and probably all over the // place too) let callee_str = match callee.rewrite(context, max_callee_width, offset) { @@ -929,7 +946,7 @@ fn rewrite_call_inner(context: &RewriteContext, item.rewrite(&inner_context, remaining_width, offset) .unwrap_or(context.snippet(item.span)) }, - callee.span.hi + BytePos(1), + span.lo, span.hi); let fmt = ListFormatting::for_fn(remaining_width, offset); @@ -1004,8 +1021,9 @@ fn rewrite_struct_lit<'a>(context: &RewriteContext, } }; - let field_iter = fields.into_iter().map(StructLitField::Regular) - .chain(base.into_iter().map(StructLitField::Base)); + let field_iter = fields.into_iter() + .map(StructLitField::Regular) + .chain(base.into_iter().map(StructLitField::Base)); let inner_context = &RewriteContext { block_indent: indent, ..*context }; @@ -1016,10 +1034,10 @@ fn rewrite_struct_lit<'a>(context: &RewriteContext, match *item { StructLitField::Regular(ref field) => field.span.lo, StructLitField::Base(ref expr) => { - let last_field_hi = - fields.last().map_or(span.lo, |field| field.span.hi); - let snippet = - context.snippet(mk_sp(last_field_hi, expr.span.lo)); + let last_field_hi = fields.last() + .map_or(span.lo, |field| field.span.hi); + let snippet = context.snippet(mk_sp(last_field_hi, + expr.span.lo)); let pos = snippet.find_uncommented("..").unwrap(); last_field_hi + BytePos(pos as u32) } @@ -1035,7 +1053,7 @@ fn rewrite_struct_lit<'a>(context: &RewriteContext, match *item { StructLitField::Regular(ref field) => { rewrite_field(inner_context, &field, h_budget, indent) - .unwrap_or(context.snippet(field.span)) + .unwrap_or(context.snippet(field.span)) } StructLitField::Base(ref expr) => { // 2 = .. @@ -1152,10 +1170,7 @@ fn rewrite_binary_op(context: &RewriteContext, // worth trying to put everything on one line. if rhs_result.len() + 2 + operator_str.len() < width && !rhs_result.contains('\n') { // 1 = space between lhs expr and operator - if let Some(mut result) = lhs.rewrite(context, - width - 1 - operator_str.len(), - offset) { - + if let Some(mut result) = lhs.rewrite(context, width - 1 - operator_str.len(), offset) { result.push(' '); result.push_str(&operator_str); result.push(' '); @@ -1167,9 +1182,7 @@ fn rewrite_binary_op(context: &RewriteContext, return Some(result); } - if let Some(rhs_result) = rhs.rewrite(context, - remaining_width, - offset + result.len()) { + if let Some(rhs_result) = rhs.rewrite(context, remaining_width, offset + result.len()) { if rhs_result.len() <= remaining_width { result.push_str(&rhs_result); return Some(result); diff --git a/src/items.rs b/src/items.rs index 7555ea01199..c7a8c4eb0d7 100644 --- a/src/items.rs +++ b/src/items.rs @@ -211,8 +211,10 @@ impl<'a> FmtVisitor<'a> { let ret_str = fd.output.rewrite(&context, self.config.max_width - indent, indent).unwrap(); // Args. - let (one_line_budget, multi_line_budget, mut arg_indent) = - self.compute_budgets_for_args(&result, indent, ret_str.len(), newline_brace); + let (one_line_budget, multi_line_budget, mut arg_indent) = self.compute_budgets_for_args(&result, + indent, + ret_str.len(), + newline_brace); debug!("rewrite_fn: one_line_budget: {}, multi_line_budget: {}, arg_indent: {}", one_line_budget, multi_line_budget, arg_indent); @@ -237,10 +239,7 @@ impl<'a> FmtVisitor<'a> { } // A conservative estimation, to goal is to be over all parens in generics - let args_start = generics.ty_params - .last() - .map(|tp| end_typaram(tp)) - .unwrap_or(span.lo); + let args_start = generics.ty_params.last().map(|tp| end_typaram(tp)).unwrap_or(span.lo); let args_span = codemap::mk_sp(span_after(codemap::mk_sp(args_start, span.hi), "(", self.codemap), @@ -333,12 +332,13 @@ impl<'a> FmtVisitor<'a> { // Account for sugary self. // FIXME: the comment for the self argument is dropped. This is blocked // on rust issue #27522. - let min_args = explicit_self.and_then(|explicit_self| { - rewrite_explicit_self(explicit_self, args) - }).map(|self_str| { - arg_item_strs[0] = self_str; - 2 - }).unwrap_or(1); + let min_args = explicit_self + .and_then(|explicit_self| rewrite_explicit_self(explicit_self, args)) + .map(|self_str| { + arg_item_strs[0] = self_str; + 2 + }) + .unwrap_or(1); // Comments between args let mut arg_items = Vec::new(); @@ -760,11 +760,10 @@ impl<'a> FmtVisitor<'a> { let typ = field.node.ty.rewrite(&self.get_context(), 1000, 0).unwrap(); let indent = self.block_indent + self.config.tab_spaces; - let mut attr_str = field.node.attrs - .rewrite(&self.get_context(), - self.config.max_width - indent, - indent) - .unwrap(); + let mut attr_str = field.node + .attrs + .rewrite(&self.get_context(), self.config.max_width - indent, indent) + .unwrap(); if !attr_str.is_empty() { attr_str.push('\n'); attr_str.push_str(&make_indent(indent)); @@ -803,22 +802,20 @@ 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).unwrap()); + 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| { - let hi = if l.bounds.is_empty() { - l.lifetime.span.hi - } else { - l.bounds[l.bounds.len() - 1].span.hi - }; - codemap::mk_sp(l.lifetime.span.lo, hi) - }); + let lt_spans = lifetimes.iter() + .map(|l| { + let hi = if l.bounds.is_empty() { + l.lifetime.span.hi + } else { + l.bounds[l.bounds.len() - 1].span.hi + }; + codemap::mk_sp(l.lifetime.span.lo, hi) + }); let ty_spans = tys.iter().map(span_for_ty_param); let items = itemize_list(self.codemap, diff --git a/src/lib.rs b/src/lib.rs index 7975c77e06d..6993d3044d3 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -70,6 +70,7 @@ mod string; mod comment; mod modules; pub mod rustfmt_diff; +mod chains; const MIN_STRING: usize = 10; // When we get scoped annotations, we should have rustfmt::skip. diff --git a/src/lists.rs b/src/lists.rs index cf68d86dd2b..a8cf0830f97 100644 --- a/src/lists.rs +++ b/src/lists.rs @@ -283,22 +283,22 @@ impl<'a, T, I, F1, F2, F3> Iterator for ListItems<'a, I, F1, F2, F3> }; // Post-comment - let next_start = match self.inner.peek() { - Some(ref next_item) => (self.get_lo)(next_item), - None => self.next_span_start - }; - let post_snippet = self.codemap.span_to_snippet(codemap::mk_sp((self.get_hi)(&item), - next_start)) - .unwrap(); + let next_start = match self.inner.peek() { + Some(ref next_item) => (self.get_lo)(next_item), + None => self.next_span_start, + }; + let post_snippet = self.codemap + .span_to_snippet(codemap::mk_sp((self.get_hi)(&item), next_start)) + .unwrap(); - let comment_end = match self.inner.peek() { - Some(..) => { - let block_open_index = post_snippet.find("/*"); - let newline_index = post_snippet.find('\n'); - let separator_index = post_snippet.find_uncommented(",").unwrap(); + let comment_end = match self.inner.peek() { + Some(..) => { + let block_open_index = post_snippet.find("/*"); + let newline_index = post_snippet.find('\n'); + let separator_index = post_snippet.find_uncommented(",").unwrap(); - match (block_open_index, newline_index) { - // Separator before comment, with the next item on same line. + match (block_open_index, newline_index) { + // Separator before comment, with the next item on same line. // Comment belongs to next item. (Some(i), None) if i > separator_index => { separator_index + 1 diff --git a/src/missed_spans.rs b/src/missed_spans.rs index 33d096c3ba0..535fc4c8ad9 100644 --- a/src/missed_spans.rs +++ b/src/missed_spans.rs @@ -17,21 +17,20 @@ impl<'a> FmtVisitor<'a> { // TODO these format_missing methods are ugly. Refactor and add unit tests // for the central whitespace stripping loop. pub fn format_missing(&mut self, end: BytePos) { - self.format_missing_inner(end, |this, last_snippet, _| { - this.buffer.push_str(last_snippet) - }) + self.format_missing_inner(end, |this, last_snippet, _| this.buffer.push_str(last_snippet)) } pub fn format_missing_with_indent(&mut self, end: BytePos) { - self.format_missing_inner(end, |this, last_snippet, snippet| { - this.buffer.push_str(last_snippet.trim_right()); - if last_snippet == snippet { + self.format_missing_inner(end, + |this, last_snippet, snippet| { + this.buffer.push_str(last_snippet.trim_right()); + if last_snippet == snippet { // No new lines in the snippet. - this.buffer.push_str("\n"); - } - let indent = make_indent(this.block_indent); - this.buffer.push_str(&indent); - }) + this.buffer.push_str("\n"); + } + let indent = make_indent(this.block_indent); + this.buffer.push_str(&indent); + }) } fn format_missing_inner(&mut self, @@ -60,9 +59,7 @@ impl<'a> FmtVisitor<'a> { let span = codemap::mk_sp(start, end); let snippet = self.snippet(span); - self.write_snippet(&snippet, - true, - &process_last_snippet); + self.write_snippet(&snippet, true, &process_last_snippet); } fn write_snippet(&mut self, diff --git a/src/string.rs b/src/string.rs index aba6a7f48da..5e5af61faf5 100644 --- a/src/string.rs +++ b/src/string.rs @@ -44,8 +44,8 @@ pub fn rewrite_string<'a>(s: &str, fmt: &StringFormat<'a>) -> String { result.push_str(fmt.opener); let ender_length = fmt.line_end.len(); - let max_chars = fmt.width.checked_sub(fmt.opener.len()).unwrap_or(0) - .checked_sub(ender_length).unwrap_or(1); + let max_chars = fmt.width.checked_sub(fmt.opener.len() + ender_length).unwrap_or(1); + loop { let mut cur_end = cur_start + max_chars; diff --git a/src/types.rs b/src/types.rs index dace1d84bca..1e0da1aba4d 100644 --- a/src/types.rs +++ b/src/types.rs @@ -196,13 +196,12 @@ fn rewrite_segment(segment: &ast::PathSegment, ast::PathParameters::AngleBracketedParameters(ref data) if !data.lifetimes.is_empty() || !data.types.is_empty() || !data.bindings.is_empty() => { - let param_list = data.lifetimes.iter() - .map(SegmentParam::LifeTime) - .chain(data.types.iter() - .map(|x| SegmentParam::Type(&*x))) - .chain(data.bindings.iter() - .map(|x| SegmentParam::Binding(&*x))) - .collect::>(); + let param_list = data.lifetimes + .iter() + .map(SegmentParam::LifeTime) + .chain(data.types.iter().map(|x| SegmentParam::Type(&*x))) + .chain(data.bindings.iter().map(|x| SegmentParam::Binding(&*x))) + .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); @@ -279,30 +278,27 @@ impl Rewrite for ast::WherePredicate { ref bounds, ..}) => { if !bound_lifetimes.is_empty() { - let lifetime_str = bound_lifetimes.iter().map(|lt| { - lt.rewrite(context, width, offset).unwrap() - }).collect::>().join(", "); + let lifetime_str = bound_lifetimes.iter() + .map(|lt| lt.rewrite(context, width, offset).unwrap()) + .collect::>() + .join(", "); let type_str = pprust::ty_to_string(bounded_ty); // 8 = "for<> : ".len() let used_width = lifetime_str.len() + type_str.len() + 8; - let bounds_str = bounds.iter().map(|ty_bound| { - ty_bound.rewrite(context, - width - used_width, - offset + used_width) - .unwrap() - }).collect::>().join(" + "); + let bounds_str = bounds.iter() + .map(|ty_bound| ty_bound.rewrite(context, width - used_width, offset + used_width).unwrap()) + .collect::>() + .join(" + "); format!("for<{}> {}: {}", lifetime_str, type_str, bounds_str) } else { let type_str = pprust::ty_to_string(bounded_ty); // 2 = ": ".len() let used_width = type_str.len() + 2; - let bounds_str = bounds.iter().map(|ty_bound| { - ty_bound.rewrite(context, - width - used_width, - offset + used_width) - .unwrap() - }).collect::>().join(" + "); + let bounds_str = bounds.iter() + .map(|ty_bound| ty_bound.rewrite(context, width - used_width, offset + used_width).unwrap()) + .collect::>() + .join(" + "); format!("{}: {}", type_str, bounds_str) } @@ -373,9 +369,11 @@ 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::>().join(" + "); + let bounds = self.bounds + .iter() + .map(|ty_bound| ty_bound.rewrite(context, width, offset).unwrap()) + .collect::>() + .join(" + "); result.push_str(&bounds); } @@ -392,9 +390,11 @@ impl Rewrite for ast::TyParam { impl Rewrite for ast::PolyTraitRef { fn rewrite(&self, context: &RewriteContext, width: usize, offset: usize) -> Option { if !self.bound_lifetimes.is_empty() { - let lifetime_str = self.bound_lifetimes.iter().map(|lt| { - lt.rewrite(context, width, offset).unwrap() - }).collect::>().join(", "); + let lifetime_str = self.bound_lifetimes + .iter() + .map(|lt| lt.rewrite(context, width, offset).unwrap()) + .collect::>() + .join(", "); // 6 is "for<> ".len() let extra_offset = lifetime_str.len() + 6; let max_path_width = try_opt!(width.checked_sub(extra_offset)); diff --git a/src/utils.rs b/src/utils.rs index 94f8fec2b0d..d8acea40d2a 100644 --- a/src/utils.rs +++ b/src/utils.rs @@ -14,6 +14,7 @@ use syntax::ast::{self, Visibility, Attribute, MetaItem, MetaItem_}; use syntax::codemap::{CodeMap, Span, BytePos}; use comment::FindUncommented; +use rewrite::{Rewrite, RewriteContext}; use SKIP_ANNOTATION; @@ -93,10 +94,16 @@ pub fn contains_skip(attrs: &[Attribute]) -> bool { // Find the end of a TyParam #[inline] pub fn end_typaram(typaram: &ast::TyParam) -> BytePos { - typaram.bounds.last().map(|bound| match *bound { - ast::RegionTyParamBound(ref lt) => lt.span, - ast::TraitTyParamBound(ref prt, _) => prt.span, - }).unwrap_or(typaram.span).hi + typaram.bounds + .last() + .map(|bound| { + match *bound { + ast::RegionTyParamBound(ref lt) => lt.span, + ast::TraitTyParamBound(ref prt, _) => prt.span, + } + }) + .unwrap_or(typaram.span) + .hi } #[inline] @@ -202,6 +209,13 @@ pub fn wrap_str>(s: S, max_width: usize, width: usize, offset: usi Some(s) } +impl Rewrite for String { + fn rewrite(&self, context: &RewriteContext, width: usize, offset: usize) -> Option { + // FIXME: unnecessary clone + wrap_str(self.clone(), context.config.max_width, width, offset) + } +} + // Binary search in integer range. Returns the first Ok value returned by the // callback. // The callback takes an integer and returns either an Ok, or an Err indicating diff --git a/src/visitor.rs b/src/visitor.rs index c1c1f2fcbbd..7e02d0f1d1f 100644 --- a/src/visitor.rs +++ b/src/visitor.rs @@ -202,19 +202,11 @@ impl<'a, 'v> visit::Visitor<'v> for FmtVisitor<'a> { } ast::Item_::ItemStruct(ref def, ref generics) => { self.format_missing_with_indent(item.span.lo); - self.visit_struct(item.ident, - item.vis, - def, - generics, - item.span); + self.visit_struct(item.ident, item.vis, def, generics, item.span); } ast::Item_::ItemEnum(ref def, ref generics) => { self.format_missing_with_indent(item.span.lo); - self.visit_enum(item.ident, - item.vis, - def, - generics, - item.span); + self.visit_enum(item.ident, item.vis, def, generics, item.span); self.last_pos = item.span.hi; } ast::Item_::ItemMod(ref module) => { @@ -236,10 +228,7 @@ impl<'a, 'v> visit::Visitor<'v> for FmtVisitor<'a> { self.format_missing_with_indent(ti.span.lo); let indent = self.block_indent; - let new_fn = self.rewrite_required_fn(indent, - ti.ident, - sig, - ti.span); + let new_fn = self.rewrite_required_fn(indent, ti.ident, sig, ti.span); if let Some(fn_str) = new_fn { @@ -299,10 +288,9 @@ impl<'a> FmtVisitor<'a> { if utils::contains_skip(attrs) { true } else { - let rewrite = attrs.rewrite(&self.get_context(), - self.config.max_width - self.block_indent, - self.block_indent) - .unwrap(); + let rewrite = attrs + .rewrite(&self.get_context(), self.config.max_width - self.block_indent, self.block_indent) + .unwrap(); self.buffer.push_str(&rewrite); let last = attrs.last().unwrap(); self.last_pos = last.span.hi; diff --git a/tests/system.rs b/tests/system.rs index b0d96468eca..10dc629c805 100644 --- a/tests/system.rs +++ b/tests/system.rs @@ -120,10 +120,8 @@ pub fn idempotent_check(filename: String) -> Result<(), HashMap HashMap { let regex = regex::Regex::new(&pattern).ok().expect("Failed creating pattern 1."); // Matches lines containing significant comments or whitespace. - let line_regex = regex::Regex::new(r"(^\s*$)|(^\s*//\s*rustfmt-[^:]+:\s*\S+)") - .ok().expect("Failed creating pattern 2."); + let line_regex = regex::Regex::new(r"(^\s*$)|(^\s*//\s*rustfmt-[^:]+:\s*\S+)").ok() + .expect("Failed creating pattern 2."); reader.lines() - .map(|line| line.ok().expect("Failed getting line.")) - .take_while(|line| line_regex.is_match(&line)) - .filter_map(|line| { - regex.captures_iter(&line).next().map(|capture| { - (capture.at(1).expect("Couldn't unwrap capture.").to_owned(), - capture.at(2).expect("Couldn't unwrap capture.").to_owned()) - }) - }) - .collect() + .map(|line| line.ok().expect("Failed getting line.")) + .take_while(|line| line_regex.is_match(&line)) + .filter_map(|line| { + regex.captures_iter(&line) + .next() + .map(|capture| { + (capture.at(1).expect("Couldn't unwrap capture.").to_owned(), + capture.at(2).expect("Couldn't unwrap capture.").to_owned()) + }) + }) + .collect() } // Compare output to input. From 659c9b90373183620eeb6118d41b1e94b8ad70a2 Mon Sep 17 00:00:00 2001 From: Marcus Klaas Date: Wed, 9 Sep 2015 23:09:39 +0200 Subject: [PATCH 02/11] Update indentation heuristics for single arg functions --- src/chains.rs | 13 ++-- src/comment.rs | 16 ++--- src/expr.rs | 7 +- src/imports.rs | 8 +-- src/items.rs | 36 +++++----- src/lists.rs | 29 ++++---- src/types.rs | 112 ++++++++++++++++------------- src/utils.rs | 10 +-- src/visitor.rs | 6 +- tests/system.rs | 20 +++--- tests/target/closure.rs | 47 ++++++------ tests/target/expr-visual-indent.rs | 4 +- tests/target/expr.rs | 12 ++-- 13 files changed, 170 insertions(+), 150 deletions(-) diff --git a/src/chains.rs b/src/chains.rs index 094cd358696..23e669d08af 100644 --- a/src/chains.rs +++ b/src/chains.rs @@ -23,8 +23,8 @@ pub fn rewrite_chain(orig_expr: &ast::Expr, -> Option { let mut expr = orig_expr; let mut rewrites = Vec::new(); - let indent = context.block_indent + context.config.tab_spaces; - let max_width = context.config.max_width - context.config.tab_spaces; + let indent = offset + context.config.tab_spaces; + let max_width = try_opt!(context.config.max_width.checked_sub(indent)); loop { match expr.node { @@ -103,7 +103,8 @@ pub fn rewrite_chain(orig_expr: &ast::Expr, // Put the first link on the same line as parent, if it fits. let first_connector = if parent_rewrite.len() + rewrites[0].len() <= width && - !rewrites[0].contains('\n') { + !rewrites[0].contains('\n') || + parent_rewrite.len() <= context.config.tab_spaces { "" } else { &connector[..] @@ -128,6 +129,10 @@ fn rewrite_method_call(method_name: ast::Ident, }; let callee_str = format!(".{}{}", method_name, type_str); + let inner_context = &RewriteContext { + block_indent: offset, + ..*context + }; - rewrite_call(context, &callee_str, args, span, width, offset) + rewrite_call(inner_context, &callee_str, args, span, width, offset) } diff --git a/src/comment.rs b/src/comment.rs index 5cec1678ac2..ed06ff7e5b6 100644 --- a/src/comment.rs +++ b/src/comment.rs @@ -41,9 +41,9 @@ pub fn rewrite_comment(orig: &str, block_style: bool, width: usize, offset: usiz let line_breaks = s.chars().filter(|&c| c == '\n').count(); let (_, mut s) = s.lines() - .enumerate() - .map(|(i, mut line)| { - line = line.trim(); + .enumerate() + .map(|(i, mut line)| { + line = line.trim(); // Drop old closer. if i == line_breaks && line.ends_with("*/") && !line.starts_with("//") { @@ -166,11 +166,11 @@ pub fn contains_comment(text: &str) -> bool { pub fn uncommented(text: &str) -> String { CharClasses::new(text.chars()) .filter_map(|(s, c)| { - match s { - CodeCharKind::Normal => Some(c), - CodeCharKind::Comment => None, - } - }) + match s { + CodeCharKind::Normal => Some(c), + CodeCharKind::Comment => None, + } + }) .collect() } diff --git a/src/expr.rs b/src/expr.rs index 2c77cdc9e78..710fb6f8703 100644 --- a/src/expr.rs +++ b/src/expr.rs @@ -1022,8 +1022,8 @@ fn rewrite_struct_lit<'a>(context: &RewriteContext, }; let field_iter = fields.into_iter() - .map(StructLitField::Regular) - .chain(base.into_iter().map(StructLitField::Base)); + .map(StructLitField::Regular) + .chain(base.into_iter().map(StructLitField::Base)); let inner_context = &RewriteContext { block_indent: indent, ..*context }; @@ -1035,7 +1035,8 @@ fn rewrite_struct_lit<'a>(context: &RewriteContext, StructLitField::Regular(ref field) => field.span.lo, StructLitField::Base(ref expr) => { let last_field_hi = fields.last() - .map_or(span.lo, |field| field.span.hi); + .map_or(span.lo, + |field| field.span.hi); let snippet = context.snippet(mk_sp(last_field_hi, expr.span.lo)); let pos = snippet.find_uncommented("..").unwrap(); diff --git a/src/imports.rs b/src/imports.rs index 8411b3208df..4b08a8b35e1 100644 --- a/src/imports.rs +++ b/src/imports.rs @@ -38,10 +38,10 @@ impl Rewrite for ast::ViewPath { let path_str = try_opt!(path.rewrite(context, width - ident_str.len() - 4, offset)); Some(if path.segments.last().unwrap().identifier == ident { - path_str - } else { - format!("{} as {}", path_str, ident_str) - }) + path_str + } else { + format!("{} as {}", path_str, ident_str) + }) } } } diff --git a/src/items.rs b/src/items.rs index c7a8c4eb0d7..ef8ad37b126 100644 --- a/src/items.rs +++ b/src/items.rs @@ -333,12 +333,12 @@ impl<'a> FmtVisitor<'a> { // FIXME: the comment for the self argument is dropped. This is blocked // on rust issue #27522. let min_args = explicit_self - .and_then(|explicit_self| rewrite_explicit_self(explicit_self, args)) - .map(|self_str| { - arg_item_strs[0] = self_str; - 2 - }) - .unwrap_or(1); + .and_then(|explicit_self| rewrite_explicit_self(explicit_self, args)) + .map(|self_str| { + arg_item_strs[0] = self_str; + 2 + }) + .unwrap_or(1); // Comments between args let mut arg_items = Vec::new(); @@ -761,9 +761,9 @@ impl<'a> FmtVisitor<'a> { let indent = self.block_indent + self.config.tab_spaces; let mut attr_str = field.node - .attrs - .rewrite(&self.get_context(), self.config.max_width - indent, indent) - .unwrap(); + .attrs + .rewrite(&self.get_context(), self.config.max_width - indent, indent) + .unwrap(); if !attr_str.is_empty() { attr_str.push('\n'); attr_str.push_str(&make_indent(indent)); @@ -804,18 +804,18 @@ impl<'a> FmtVisitor<'a> { // 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()); + .map(|ty_param| ty_param.rewrite(&context, h_budget, offset).unwrap()); // Extract comments between generics. let lt_spans = lifetimes.iter() - .map(|l| { - let hi = if l.bounds.is_empty() { - l.lifetime.span.hi - } else { - l.bounds[l.bounds.len() - 1].span.hi - }; - codemap::mk_sp(l.lifetime.span.lo, hi) - }); + .map(|l| { + let hi = if l.bounds.is_empty() { + l.lifetime.span.hi + } else { + l.bounds[l.bounds.len() - 1].span.hi + }; + codemap::mk_sp(l.lifetime.span.lo, hi) + }); let ty_spans = tys.iter().map(span_for_ty_param); let items = itemize_list(self.codemap, diff --git a/src/lists.rs b/src/lists.rs index a8cf0830f97..b07b81bd672 100644 --- a/src/lists.rs +++ b/src/lists.rs @@ -283,22 +283,23 @@ impl<'a, T, I, F1, F2, F3> Iterator for ListItems<'a, I, F1, F2, F3> }; // Post-comment - let next_start = match self.inner.peek() { - Some(ref next_item) => (self.get_lo)(next_item), - None => self.next_span_start, - }; - let post_snippet = self.codemap - .span_to_snippet(codemap::mk_sp((self.get_hi)(&item), next_start)) - .unwrap(); + let next_start = match self.inner.peek() { + Some(ref next_item) => (self.get_lo)(next_item), + None => self.next_span_start, + }; + let post_snippet = self.codemap + .span_to_snippet(codemap::mk_sp((self.get_hi)(&item), + next_start)) + .unwrap(); - let comment_end = match self.inner.peek() { - Some(..) => { - let block_open_index = post_snippet.find("/*"); - let newline_index = post_snippet.find('\n'); - let separator_index = post_snippet.find_uncommented(",").unwrap(); + let comment_end = match self.inner.peek() { + Some(..) => { + let block_open_index = post_snippet.find("/*"); + let newline_index = post_snippet.find('\n'); + let separator_index = post_snippet.find_uncommented(",").unwrap(); - match (block_open_index, newline_index) { - // Separator before comment, with the next item on same line. + match (block_open_index, newline_index) { + // Separator before comment, with the next item on same line. // Comment belongs to next item. (Some(i), None) if i > separator_index => { separator_index + 1 diff --git a/src/types.rs b/src/types.rs index 1e0da1aba4d..9f3cc17e4c7 100644 --- a/src/types.rs +++ b/src/types.rs @@ -197,11 +197,11 @@ fn rewrite_segment(segment: &ast::PathSegment, !data.types.is_empty() || !data.bindings.is_empty() => { let param_list = data.lifetimes - .iter() - .map(SegmentParam::LifeTime) - .chain(data.types.iter().map(|x| SegmentParam::Type(&*x))) - .chain(data.bindings.iter().map(|x| SegmentParam::Binding(&*x))) - .collect::>(); + .iter() + .map(SegmentParam::LifeTime) + .chain(data.types.iter().map(|x| SegmentParam::Type(&*x))) + .chain(data.bindings.iter().map(|x| SegmentParam::Binding(&*x))) + .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); @@ -273,54 +273,66 @@ impl Rewrite for ast::WherePredicate { // TODO dead spans? // TODO assumes we'll always fit on one line... Some(match *self { - ast::WherePredicate::BoundPredicate(ast::WhereBoundPredicate{ref bound_lifetimes, - ref bounded_ty, - ref bounds, - ..}) => { - if !bound_lifetimes.is_empty() { - let lifetime_str = bound_lifetimes.iter() - .map(|lt| lt.rewrite(context, width, offset).unwrap()) - .collect::>() - .join(", "); - let type_str = pprust::ty_to_string(bounded_ty); + ast::WherePredicate::BoundPredicate(ast::WhereBoundPredicate { ref bound_lifetimes, + ref bounded_ty, + ref bounds, + .. }) => { + if !bound_lifetimes.is_empty() { + let lifetime_str = bound_lifetimes.iter() + .map(|lt| lt.rewrite(context, width, offset).unwrap()) + .collect::>() + .join(", "); + let type_str = pprust::ty_to_string(bounded_ty); // 8 = "for<> : ".len() - let used_width = lifetime_str.len() + type_str.len() + 8; - let bounds_str = bounds.iter() - .map(|ty_bound| ty_bound.rewrite(context, width - used_width, offset + used_width).unwrap()) - .collect::>() - .join(" + "); + let used_width = lifetime_str.len() + type_str.len() + 8; + let bounds_str = bounds.iter() + .map(|ty_bound| { + ty_bound + .rewrite(context, + width - used_width, + offset + used_width) + .unwrap() + }) + .collect::>() + .join(" + "); - format!("for<{}> {}: {}", lifetime_str, type_str, bounds_str) - } else { - let type_str = pprust::ty_to_string(bounded_ty); + format!("for<{}> {}: {}", lifetime_str, type_str, bounds_str) + } else { + let type_str = pprust::ty_to_string(bounded_ty); // 2 = ": ".len() - let used_width = type_str.len() + 2; - let bounds_str = bounds.iter() - .map(|ty_bound| ty_bound.rewrite(context, width - used_width, offset + used_width).unwrap()) - .collect::>() - .join(" + "); + let used_width = type_str.len() + 2; + let bounds_str = bounds.iter() + .map(|ty_bound| { + ty_bound + .rewrite(context, + width - used_width, + offset + used_width) + .unwrap() + }) + .collect::>() + .join(" + "); - format!("{}: {}", type_str, bounds_str) - } + format!("{}: {}", type_str, bounds_str) } - ast::WherePredicate::RegionPredicate(ast::WhereRegionPredicate{ref lifetime, - ref bounds, - ..}) => { - format!("{}: {}", + } + ast::WherePredicate::RegionPredicate(ast::WhereRegionPredicate { ref lifetime, + ref bounds, + .. }) => { + format!("{}: {}", pprust::lifetime_to_string(lifetime), bounds.iter().map(pprust::lifetime_to_string) .collect::>().join(" + ")) - } - ast::WherePredicate::EqPredicate(ast::WhereEqPredicate{ref path, ref ty, ..}) => { - let ty_str = pprust::ty_to_string(ty); + } + ast::WherePredicate::EqPredicate(ast::WhereEqPredicate { ref path, ref ty, .. }) => { + let ty_str = pprust::ty_to_string(ty); // 3 = " = ".len() - let used_width = 3 + ty_str.len(); - let path_str = try_opt!(path.rewrite(context, + let used_width = 3 + ty_str.len(); + let path_str = try_opt!(path.rewrite(context, width - used_width, offset + used_width)); - format!("{} = {}", path_str, ty_str) - } - }) + format!("{} = {}", path_str, ty_str) + } + }) } } @@ -370,10 +382,10 @@ impl Rewrite for ast::TyParam { result.push_str(": "); let bounds = self.bounds - .iter() - .map(|ty_bound| ty_bound.rewrite(context, width, offset).unwrap()) - .collect::>() - .join(" + "); + .iter() + .map(|ty_bound| ty_bound.rewrite(context, width, offset).unwrap()) + .collect::>() + .join(" + "); result.push_str(&bounds); } @@ -391,10 +403,10 @@ impl Rewrite for ast::PolyTraitRef { fn rewrite(&self, context: &RewriteContext, width: usize, offset: usize) -> Option { if !self.bound_lifetimes.is_empty() { let lifetime_str = self.bound_lifetimes - .iter() - .map(|lt| lt.rewrite(context, width, offset).unwrap()) - .collect::>() - .join(", "); + .iter() + .map(|lt| lt.rewrite(context, width, offset).unwrap()) + .collect::>() + .join(", "); // 6 is "for<> ".len() let extra_offset = lifetime_str.len() + 6; let max_path_width = try_opt!(width.checked_sub(extra_offset)); diff --git a/src/utils.rs b/src/utils.rs index d8acea40d2a..faa953ebea3 100644 --- a/src/utils.rs +++ b/src/utils.rs @@ -97,11 +97,11 @@ pub fn end_typaram(typaram: &ast::TyParam) -> BytePos { typaram.bounds .last() .map(|bound| { - match *bound { - ast::RegionTyParamBound(ref lt) => lt.span, - ast::TraitTyParamBound(ref prt, _) => prt.span, - } - }) + match *bound { + ast::RegionTyParamBound(ref lt) => lt.span, + ast::TraitTyParamBound(ref prt, _) => prt.span, + } + }) .unwrap_or(typaram.span) .hi } diff --git a/src/visitor.rs b/src/visitor.rs index 7e02d0f1d1f..1159116b010 100644 --- a/src/visitor.rs +++ b/src/visitor.rs @@ -289,8 +289,10 @@ impl<'a> FmtVisitor<'a> { true } else { let rewrite = attrs - .rewrite(&self.get_context(), self.config.max_width - self.block_indent, self.block_indent) - .unwrap(); + .rewrite(&self.get_context(), + self.config.max_width - self.block_indent, + self.block_indent) + .unwrap(); self.buffer.push_str(&rewrite); let last = attrs.last().unwrap(); self.last_pos = last.span.hi; diff --git a/tests/system.rs b/tests/system.rs index 10dc629c805..e77257663db 100644 --- a/tests/system.rs +++ b/tests/system.rs @@ -120,8 +120,8 @@ pub fn idempotent_check(filename: String) -> Result<(), HashMap HashMap { // Matches lines containing significant comments or whitespace. let line_regex = regex::Regex::new(r"(^\s*$)|(^\s*//\s*rustfmt-[^:]+:\s*\S+)").ok() - .expect("Failed creating pattern 2."); + .expect("Failed creating pattern 2."); reader.lines() .map(|line| line.ok().expect("Failed getting line.")) .take_while(|line| line_regex.is_match(&line)) .filter_map(|line| { - regex.captures_iter(&line) - .next() - .map(|capture| { - (capture.at(1).expect("Couldn't unwrap capture.").to_owned(), - capture.at(2).expect("Couldn't unwrap capture.").to_owned()) - }) - }) + regex.captures_iter(&line) + .next() + .map(|capture| { + (capture.at(1).expect("Couldn't unwrap capture.").to_owned(), + capture.at(2).expect("Couldn't unwrap capture.").to_owned()) + }) + }) .collect() } diff --git a/tests/target/closure.rs b/tests/target/closure.rs index a0565167093..ec095ad8fcc 100644 --- a/tests/target/closure.rs +++ b/tests/target/closure.rs @@ -9,31 +9,30 @@ fn main() { b: WithType, // argument // ignored _| { - (aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa, - bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb) - }; + (aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa, bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb) + }; let block_body = move |xxxxxxxxxxxxxxxxxxxxxxxxxxxxx, ref yyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyy| { - xxxxxxxxxxxxxxxxxxxxxxxxxxxxx + yyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyy - }; + xxxxxxxxxxxxxxxxxxxxxxxxxxxxx + yyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyy + }; let loooooooooooooong_name = |field| { // TODO(#27): format comments. - if field.node.attrs.len() > 0 { - field.node.attrs[0].span.lo - } else { - field.span.lo - } - }; + if field.node.attrs.len() > 0 { + field.node.attrs[0].span.lo + } else { + field.span.lo + } + }; let block_me = |field| { - if true_story() { - 1 - } else { - 2 - } - }; + if true_story() { + 1 + } else { + 2 + } + }; let unblock_me = |trivial| closure(); @@ -44,17 +43,17 @@ fn main() { }; let test = || { - do_something(); - do_something_else(); - }; + do_something(); + do_something_else(); + }; let arg_test = |big_argument_name, test123| { - looooooooooooooooooong_function_naaaaaaaaaaaaaaaaame() - }; + looooooooooooooooooong_function_naaaaaaaaaaaaaaaaame() + }; let arg_test = |big_argument_name, test123| { - looooooooooooooooooong_function_naaaaaaaaaaaaaaaaame() - }; + looooooooooooooooooong_function_naaaaaaaaaaaaaaaaame() + }; let simple_closure = move || -> () {}; diff --git a/tests/target/expr-visual-indent.rs b/tests/target/expr-visual-indent.rs index d74b5aae93c..f5b79d74fcb 100644 --- a/tests/target/expr-visual-indent.rs +++ b/tests/target/expr-visual-indent.rs @@ -4,6 +4,6 @@ fn matcher() { Some(while true { - test(); - }) + test(); + }) } diff --git a/tests/target/expr.rs b/tests/target/expr.rs index 9bfe51e0b26..d24d463c2c2 100644 --- a/tests/target/expr.rs +++ b/tests/target/expr.rs @@ -89,13 +89,13 @@ fn bar() { } syntactically_correct(loop { - sup('?'); - }, + sup('?'); + }, if cond { - 0 - } else { - 1 - }); + 0 + } else { + 1 + }); let third = ..10; let infi_range = ..; From a53be867105394f09c33d929535d37bc6522ff8b Mon Sep 17 00:00:00 2001 From: Marcus Klaas Date: Wed, 9 Sep 2015 23:10:51 +0200 Subject: [PATCH 03/11] address mini offset bug in rewrite_match --- src/expr.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/expr.rs b/src/expr.rs index 710fb6f8703..353150197b1 100644 --- a/src/expr.rs +++ b/src/expr.rs @@ -732,6 +732,7 @@ impl Rewrite for ast::Arm { // Let's try and get the arm body on the same line as the condition. // 4 = ` => `.len() if context.config.max_width > line_start + comma.len() + 4 { + let inner_offset = line_start + 4; let budget = context.config.max_width - line_start - comma.len() - 4; if let Some(ref body_str) = body.rewrite(context, budget, From 749a9689be022f2737820a87fe09d930907435a5 Mon Sep 17 00:00:00 2001 From: Marcus Klaas Date: Fri, 11 Sep 2015 00:52:57 +0200 Subject: [PATCH 04/11] Break chains that don't start with path expressions --- src/bin/rustfmt.rs | 5 ++- src/chains.rs | 99 ++++++++++++++++++++++------------------------ src/comment.rs | 1 - src/expr.rs | 12 ++---- src/lists.rs | 2 +- tests/system.rs | 3 +- 6 files changed, 56 insertions(+), 66 deletions(-) diff --git a/src/bin/rustfmt.rs b/src/bin/rustfmt.rs index 9f2387f5c2c..9913c68cdfa 100644 --- a/src/bin/rustfmt.rs +++ b/src/bin/rustfmt.rs @@ -82,7 +82,9 @@ fn main() { } fn print_usage>(reason: S) { - println!("{}\n\r usage: rustfmt [-h Help] [--write-mode=[replace|overwrite|display|diff]] ", reason.into()); + println!("{}\n\r usage: rustfmt [-h Help] [--write-mode=[replace|overwrite|display|diff]] \ + ", + reason.into()); } fn determine_params(args: I) -> Option<(Vec, WriteMode)> @@ -123,6 +125,5 @@ fn determine_params(args: I) -> Option<(Vec, WriteMode)> return None; } - Some((rustc_args, write_mode)) } diff --git a/src/chains.rs b/src/chains.rs index 23e669d08af..4f395df0487 100644 --- a/src/chains.rs +++ b/src/chains.rs @@ -9,7 +9,7 @@ // except according to those terms. use rewrite::{Rewrite, RewriteContext}; -use utils::{span_after, make_indent, extra_offset}; +use utils::{make_indent, extra_offset}; use expr::rewrite_call; use syntax::{ast, ptr}; @@ -26,66 +26,23 @@ pub fn rewrite_chain(orig_expr: &ast::Expr, let indent = offset + context.config.tab_spaces; let max_width = try_opt!(context.config.max_width.checked_sub(indent)); - loop { - match expr.node { - ast::Expr_::ExprMethodCall(ref method_name, ref types, ref expressions) => { - // FIXME: a lot of duplication between this and the - // rewrite_method_call in expr.rs. - let new_span = mk_sp(expressions[0].span.hi, expr.span.hi); - let lo = span_after(new_span, "(", context.codemap); - let new_span = mk_sp(lo, expr.span.hi); + while let Some(pair) = pop_expr_chain(expr, orig_expr.span, context, max_width, indent) { + let (rewrite, parent_expr) = pair; - let rewrite = rewrite_method_call(method_name.node, - types, - &expressions[1..], - new_span, - context, - max_width, - indent); - rewrites.push(try_opt!(rewrite)); - expr = &expressions[0]; - } - ast::Expr_::ExprField(ref subexpr, ref field) => { - expr = subexpr; - rewrites.push(format!(".{}", field.node)); - } - ast::Expr_::ExprTupField(ref subexpr, ref field) => { - expr = subexpr; - rewrites.push(format!(".{}", field.node)); - } - _ => break, - } + rewrites.push(try_opt!(rewrite)); + expr = parent_expr; } let parent_rewrite = try_opt!(expr.rewrite(context, width, offset)); - // TODO: add exception for when rewrites.len() == 1 if rewrites.len() == 1 { let extra_offset = extra_offset(&parent_rewrite, offset); + let offset = offset + extra_offset; let max_width = try_opt!(width.checked_sub(extra_offset)); - // FIXME: massive duplication - let rerewrite = match orig_expr.node { - ast::Expr_::ExprMethodCall(ref method_name, ref types, ref expressions) => { - let new_span = mk_sp(expressions[0].span.hi, orig_expr.span.hi); - let lo = span_after(new_span, "(", context.codemap); - let new_span = mk_sp(lo, orig_expr.span.hi); - rewrite_method_call(method_name.node, - types, - &expressions[1..], - new_span, - context, - max_width, - offset + extra_offset) - } - ast::Expr_::ExprField(_, ref field) => { - Some(format!(".{}", field.node)) - } - ast::Expr_::ExprTupField(_, ref field) => { - Some(format!(".{}", field.node)) - } - _ => unreachable!(), - }; + let rerewrite = pop_expr_chain(orig_expr, orig_expr.span, context, max_width, offset) + .unwrap() + .0; return Some(format!("{}{}", parent_rewrite, try_opt!(rerewrite))); } @@ -103,6 +60,7 @@ pub fn rewrite_chain(orig_expr: &ast::Expr, // Put the first link on the same line as parent, if it fits. let first_connector = if parent_rewrite.len() + rewrites[0].len() <= width && + is_continuable(expr) && !rewrites[0].contains('\n') || parent_rewrite.len() <= context.config.tab_spaces { "" @@ -113,6 +71,43 @@ pub fn rewrite_chain(orig_expr: &ast::Expr, Some(format!("{}{}{}", parent_rewrite, first_connector, rewrites.join(&connector))) } +// Returns None when the expression is not a chainable. Otherwise, rewrites the +// outermost chain element and returns the remaining chain. +fn pop_expr_chain<'a>(expr: &'a ast::Expr, + span: Span, + context: &RewriteContext, + width: usize, + offset: usize) + -> Option<(Option, &'a ast::Expr)> { + match expr.node { + ast::Expr_::ExprMethodCall(ref method_name, ref types, ref expressions) => { + Some((rewrite_method_call(method_name.node, + types, + expressions, + span, + context, + width, + offset), + &expressions[0])) + } + ast::Expr_::ExprField(ref subexpr, ref field) => { + Some((Some(format!(".{}", field.node)), subexpr)) + } + ast::Expr_::ExprTupField(ref subexpr, ref field) => { + Some((Some(format!(".{}", field.node)), subexpr)) + } + _ => None, + } +} + +// Determines we can continue formatting a given expression on the same line. +fn is_continuable(expr: &ast::Expr) -> bool { + match expr.node { + ast::Expr_::ExprPath(..) => true, + _ => false, + } +} + fn rewrite_method_call(method_name: ast::Ident, types: &[ptr::P], args: &[ptr::P], diff --git a/src/comment.rs b/src/comment.rs index ed06ff7e5b6..313510f02b7 100644 --- a/src/comment.rs +++ b/src/comment.rs @@ -44,7 +44,6 @@ pub fn rewrite_comment(orig: &str, block_style: bool, width: usize, offset: usiz .enumerate() .map(|(i, mut line)| { line = line.trim(); - // Drop old closer. if i == line_breaks && line.ends_with("*/") && !line.starts_with("//") { line = &line[..(line.len() - 2)]; diff --git a/src/expr.rs b/src/expr.rs index 353150197b1..69fea0e5927 100644 --- a/src/expr.rs +++ b/src/expr.rs @@ -40,15 +40,6 @@ impl Rewrite for ast::Expr { } } ast::Expr_::ExprCall(ref callee, ref args) => { - // // FIXME using byte lens instead of char lens (and probably all over the place too) - // // 2 is for parens - // let max_callee_width = try_opt!(width.checked_sub(2)); - // let callee_str = try_opt!(callee.rewrite(context, max_callee_width, offset)); - - // let new_span = mk_sp(callee.span.hi, self.span.hi); - // let lo = span_after(new_span, "(", context.codemap); - // let new_span = mk_sp(lo, self.span.hi); - rewrite_call(context, &**callee, args, self.span, width, offset) } ast::Expr_::ExprParen(ref subexpr) => { @@ -927,6 +918,9 @@ fn rewrite_call_inner(context: &RewriteContext, None => return Err(Ordering::Greater), }; + let span_lo = span_after(span, "(", context.codemap); + let span = mk_sp(span_lo, span.hi); + let extra_offset = extra_offset(&callee_str, offset); // 2 is for parens. let remaining_width = match width.checked_sub(extra_offset + 2) { diff --git a/src/lists.rs b/src/lists.rs index b07b81bd672..0afee8f79ac 100644 --- a/src/lists.rs +++ b/src/lists.rs @@ -282,7 +282,7 @@ impl<'a, T, I, F1, F2, F3> Iterator for ListItems<'a, I, F1, F2, F3> None }; - // Post-comment + // Post-comment let next_start = match self.inner.peek() { Some(ref next_item) => (self.get_lo)(next_item), None => self.next_span_start, diff --git a/tests/system.rs b/tests/system.rs index e77257663db..9cfe9101986 100644 --- a/tests/system.rs +++ b/tests/system.rs @@ -152,7 +152,8 @@ fn read_significant_comments(file_name: &str) -> HashMap { let regex = regex::Regex::new(&pattern).ok().expect("Failed creating pattern 1."); // Matches lines containing significant comments or whitespace. - let line_regex = regex::Regex::new(r"(^\s*$)|(^\s*//\s*rustfmt-[^:]+:\s*\S+)").ok() + let line_regex = regex::Regex::new(r"(^\s*$)|(^\s*//\s*rustfmt-[^:]+:\s*\S+)") + .ok() .expect("Failed creating pattern 2."); reader.lines() From abe8e7de990b88e2b3d7349bc1fc240b4eb49bad Mon Sep 17 00:00:00 2001 From: Marcus Klaas Date: Wed, 9 Sep 2015 23:13:37 +0200 Subject: [PATCH 05/11] Add tests for chain expressions --- src/chains.rs | 9 +++++---- src/comment.rs | 27 --------------------------- tests/source/chains.rs | 33 +++++++++++++++++++++++++++++++++ tests/system.rs | 3 ++- tests/target/chains.rs | 38 ++++++++++++++++++++++++++++++++++++++ 5 files changed, 78 insertions(+), 32 deletions(-) create mode 100644 tests/source/chains.rs create mode 100644 tests/target/chains.rs diff --git a/src/chains.rs b/src/chains.rs index 4f395df0487..e5d2a43840b 100644 --- a/src/chains.rs +++ b/src/chains.rs @@ -34,8 +34,11 @@ pub fn rewrite_chain(orig_expr: &ast::Expr, } let parent_rewrite = try_opt!(expr.rewrite(context, width, offset)); + let total_width = rewrites.iter().fold(0, |a, b| a + b.len()) + parent_rewrite.len(); + let fits_single_line = total_width <= width && rewrites.iter().all(|s| !s.contains('\n')); - if rewrites.len() == 1 { + if rewrites.len() == 1 && !fits_single_line && + (is_continuable(expr) || parent_rewrite.len() <= context.config.tab_spaces) { let extra_offset = extra_offset(&parent_rewrite, offset); let offset = offset + extra_offset; let max_width = try_opt!(width.checked_sub(extra_offset)); @@ -47,9 +50,7 @@ pub fn rewrite_chain(orig_expr: &ast::Expr, return Some(format!("{}{}", parent_rewrite, try_opt!(rerewrite))); } - let total_width = rewrites.iter().fold(0, |a, b| a + b.len()) + parent_rewrite.len(); - - let connector = if total_width <= width && rewrites.iter().all(|s| !s.contains('\n')) { + let connector = if fits_single_line { String::new() } else { format!("\n{}", make_indent(indent)) diff --git a/src/comment.rs b/src/comment.rs index 313510f02b7..290fc62e15a 100644 --- a/src/comment.rs +++ b/src/comment.rs @@ -145,18 +145,6 @@ pub fn find_comment_end(s: &str) -> Option { } } -#[test] -fn comment_end() { - assert_eq!(Some(6), find_comment_end("// hi\n")); - assert_eq!(Some(9), find_comment_end("/* sup */ ")); - assert_eq!(Some(9), find_comment_end("/*/**/ */ ")); - assert_eq!(Some(6), find_comment_end("/*/ */ weird!")); - assert_eq!(None, find_comment_end("/* hi /* test */")); - assert_eq!(None, find_comment_end("// hi /* test */")); - assert_eq!(Some(9), find_comment_end("// hi /*\n.")); -} - - /// Returns true if text contains any comment. pub fn contains_comment(text: &str) -> bool { CharClasses::new(text.chars()).any(|(kind, _)| kind == CodeCharKind::Comment) @@ -173,21 +161,6 @@ pub fn uncommented(text: &str) -> String { .collect() } -#[test] -fn test_uncommented() { - assert_eq!(&uncommented("abc/*...*/"), "abc"); - assert_eq!(&uncommented("// .... /* \n../* /* *** / */ */a/* // */c\n"), "..ac\n"); - assert_eq!(&uncommented("abc \" /* */\" qsdf"), "abc \" /* */\" qsdf"); -} - -#[test] -fn test_contains_comment() { - assert_eq!(contains_comment("abc"), false); - assert_eq!(contains_comment("abc // qsdf"), true); - assert_eq!(contains_comment("abc /* kqsdf"), true); - assert_eq!(contains_comment("abc \" /* */\" qsdf"), false); -} - struct CharClasses where T: Iterator, T::Item: RichChar diff --git a/tests/source/chains.rs b/tests/source/chains.rs new file mode 100644 index 00000000000..7e4bda0671e --- /dev/null +++ b/tests/source/chains.rs @@ -0,0 +1,33 @@ +// Test chain formatting. + +fn main() { + let a = b.c + .d + .1 + .foo(|x| x + 1); + + bbbbbbbbbbbbbbbbbbb.ccccccccccccccccccccccccccccccccccccc + .ddddddddddddddddddddddddddd(); + + bbbbbbbbbbbbbbbbbbb.ccccccccccccccccccccccccccccccccccccc.ddddddddddddddddddddddddddd.eeeeeeee(); + + x() + .y(|| match cond() { true => (), false => () }); + + loong_func() + .quux(move || if true { + 1 + } else { + 2 + }); + + let suuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuum = xxxxxxx + .map(|x| x + 5) + .map(|x| x / 2) + .fold(0, |acc, x| acc + x); + + aaaaaaaaaaaaaaaa.map(|x| { + x += 1; + x + }).filter(some_mod::some_filter) +} diff --git a/tests/system.rs b/tests/system.rs index 9cfe9101986..fb877993daf 100644 --- a/tests/system.rs +++ b/tests/system.rs @@ -121,7 +121,8 @@ pub fn idempotent_check(filename: String) -> Result<(), HashMap (), + false => (), + } + }); + + loong_func() + .quux(move || { + if true { + 1 + } else { + 2 + } + }); + + let suuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuum = xxxxxxx.map(|x| x + 5) + .map(|x| x / 2) + .fold(0, |acc, x| acc + x); + + aaaaaaaaaaaaaaaa + .map(|x| { + x += 1; + x + }) + .filter(some_mod::some_filter) +} From 8e471ece31abb7e329bffef977e1bd0c7d876b8c Mon Sep 17 00:00:00 2001 From: Marcus Klaas Date: Wed, 9 Sep 2015 23:14:09 +0200 Subject: [PATCH 06/11] Add some tests for match blocks --- src/chains.rs | 11 +++++++++++ src/comment.rs | 23 ++++++++--------------- tests/source/chains.rs | 9 +++++++++ tests/target/chains.rs | 8 ++++++++ 4 files changed, 36 insertions(+), 15 deletions(-) diff --git a/src/chains.rs b/src/chains.rs index e5d2a43840b..4eaf0f5acb5 100644 --- a/src/chains.rs +++ b/src/chains.rs @@ -8,6 +8,17 @@ // option. This file may not be copied, modified, or distributed // except according to those terms. +// Formatting of chained expressions, i.e. expressions which are chained by +// dots: struct and enum field access and method calls. +// +// Instead of walking these subexpressions one-by-one, as is our usual strategy +// for expression formatting, we collect maximal sequences of these expressions +// and handle them simultaneously. +// +// Whenever possible, the entire chain is put on a single line. If that fails, +// we put each subexpression on a separate, much like the (default) function +// argument function argument strategy. + use rewrite::{Rewrite, RewriteContext}; use utils::{make_indent, extra_offset}; use expr::rewrite_call; diff --git a/src/comment.rs b/src/comment.rs index 290fc62e15a..d0eeba9a7c1 100644 --- a/src/comment.rs +++ b/src/comment.rs @@ -150,17 +150,6 @@ pub fn contains_comment(text: &str) -> bool { CharClasses::new(text.chars()).any(|(kind, _)| kind == CodeCharKind::Comment) } -pub fn uncommented(text: &str) -> String { - CharClasses::new(text.chars()) - .filter_map(|(s, c)| { - match s { - CodeCharKind::Normal => Some(c), - CodeCharKind::Comment => None, - } - }) - .collect() -} - struct CharClasses where T: Iterator, T::Item: RichChar @@ -321,10 +310,14 @@ mod test { // This is probably intended to be a non-test fn, but it is not used. I'm // keeping it around unless it helps us test stuff. fn uncommented(text: &str) -> String { - CharClasses::new(text.chars()).filter_map(|(s, c)| match s { - CodeCharKind::Normal => Some(c), - CodeCharKind::Comment => None - }).collect() + CharClasses::new(text.chars()) + .filter_map(|(s, c)| { + match s { + CodeCharKind::Normal => Some(c), + CodeCharKind::Comment => None, + } + }) + .collect() } #[test] diff --git a/tests/source/chains.rs b/tests/source/chains.rs index 7e4bda0671e..8282c3baa6b 100644 --- a/tests/source/chains.rs +++ b/tests/source/chains.rs @@ -21,6 +21,15 @@ fn main() { 2 }); + fffffffffffffffffffffffffffffffffff(a, + { + SCRIPT_TASK_ROOT + .with(|root| { + // Another case of write_list failing us. + *root.borrow_mut() = Some(&script_task); + }); + }); + let suuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuum = xxxxxxx .map(|x| x + 5) .map(|x| x / 2) diff --git a/tests/target/chains.rs b/tests/target/chains.rs index 2ed91a2eb8e..daa60d09855 100644 --- a/tests/target/chains.rs +++ b/tests/target/chains.rs @@ -25,6 +25,14 @@ fn main() { } }); + fffffffffffffffffffffffffffffffffff(a, + { + SCRIPT_TASK_ROOT.with(|root| { + // Another case of write_list failing us. + *root.borrow_mut() = Some(&script_task); + }); + }); + let suuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuum = xxxxxxx.map(|x| x + 5) .map(|x| x / 2) .fold(0, |acc, x| acc + x); From 95ef9dedb48061617dd41f8f70b00bf636d0ca99 Mon Sep 17 00:00:00 2001 From: Marcus Klaas Date: Tue, 1 Sep 2015 23:51:57 +0200 Subject: [PATCH 07/11] Escape quotes in string literals --- tests/system.rs | 28 ++++++++++++++++------------ 1 file changed, 16 insertions(+), 12 deletions(-) diff --git a/tests/system.rs b/tests/system.rs index fb877993daf..ea98d94f412 100644 --- a/tests/system.rs +++ b/tests/system.rs @@ -26,9 +26,9 @@ use rustfmt::rustfmt_diff::*; static DIFF_CONTEXT_SIZE: usize = 3; fn get_path_string(dir_entry: io::Result) -> String { - let path = dir_entry.ok().expect("Couldn't get DirEntry.").path(); + let path = dir_entry.ok().expect("Couldn\'t get DirEntry.").path(); - path.to_str().expect("Couldn't stringify path.").to_owned() + path.to_str().expect("Couldn\'t stringify path.").to_owned() } // Integration tests. The files in the tests/source are formatted and compared @@ -40,7 +40,7 @@ fn get_path_string(dir_entry: io::Result) -> String { #[test] fn system_tests() { // Get all files in the tests/source directory - let files = fs::read_dir("tests/source").ok().expect("Couldn't read source dir."); + let files = fs::read_dir("tests/source").ok().expect("Couldn\'t read source dir."); // turn a DirEntry into a String that represents the relative path to the file let files = files.map(get_path_string); @@ -56,9 +56,9 @@ fn system_tests() { #[test] fn idempotence_tests() { // Get all files in the tests/target directory - let files = fs::read_dir("tests/target").ok().expect("Couldn't read target dir."); - let files = files.chain(fs::read_dir("tests").ok().expect("Couldn't read tests dir.")); - let files = files.chain(fs::read_dir("src/bin").ok().expect("Couldn't read src dir.")); + let files = fs::read_dir("tests/target").ok().expect("Couldn\'t read target dir."); + let files = files.chain(fs::read_dir("tests").ok().expect("Couldn\'t read tests dir.")); + let files = files.chain(fs::read_dir("src/bin").ok().expect("Couldn\'t read src dir.")); // turn a DirEntry into a String that represents the relative path to the file let files = files.map(get_path_string); // hack because there's no `IntoIterator` impl for `[T; N]` @@ -137,9 +137,11 @@ fn get_config(config_file: Option<&str>) -> Box { } }; - let mut def_config_file = fs::File::open(config_file_name).ok().expect("Couldn't open config."); + let mut def_config_file = fs::File::open(config_file_name) + .ok() + .expect("Couldn\'t open config."); let mut def_config = String::new(); - def_config_file.read_to_string(&mut def_config).ok().expect("Couldn't read config."); + def_config_file.read_to_string(&mut def_config).ok().expect("Couldn\'t read config."); Box::new(Config::from_toml(&def_config)) } @@ -147,7 +149,9 @@ fn get_config(config_file: Option<&str>) -> Box { // Reads significant comments of the form: // rustfmt-key: value // into a hash map. fn read_significant_comments(file_name: &str) -> HashMap { - let file = fs::File::open(file_name).ok().expect(&format!("Couldn't read file {}.", file_name)); + let file = fs::File::open(file_name) + .ok() + .expect(&format!("Couldn\'t read file {}.", file_name)); let reader = BufReader::new(file); let pattern = r"^\s*//\s*rustfmt-([^:]+):\s*(\S+)"; let regex = regex::Regex::new(&pattern).ok().expect("Failed creating pattern 1."); @@ -164,8 +168,8 @@ fn read_significant_comments(file_name: &str) -> HashMap { regex.captures_iter(&line) .next() .map(|capture| { - (capture.at(1).expect("Couldn't unwrap capture.").to_owned(), - capture.at(2).expect("Couldn't unwrap capture.").to_owned()) + (capture.at(1).expect("Couldn\'t unwrap capture.").to_owned(), + capture.at(2).expect("Couldn\'t unwrap capture.").to_owned()) }) }) .collect() @@ -183,7 +187,7 @@ fn handle_result(result: HashMap) { // If file is in tests/source, compare to file with same name in tests/target. let target = get_target(&file_name, sig_comments.get("target").map(|x| &(*x)[..])); - let mut f = fs::File::open(&target).ok().expect("Couldn't open target."); + let mut f = fs::File::open(&target).ok().expect("Couldn\'t open target."); let mut text = String::new(); // TODO: speedup by running through bytes iterator From a9814149c93ddec27d38028667f67f75d469c3cc Mon Sep 17 00:00:00 2001 From: Marcus Klaas Date: Wed, 9 Sep 2015 23:14:54 +0200 Subject: [PATCH 08/11] Align dots in chained expressions --- src/chains.rs | 99 +++++++++++++++++++++--------------------- src/expr.rs | 12 ++--- src/items.rs | 41 +++++++++-------- src/types.rs | 43 +++++++++--------- src/utils.rs | 18 ++++---- src/visitor.rs | 9 ++-- tests/system.rs | 22 +++++----- tests/target/chains.rs | 20 ++++----- 8 files changed, 134 insertions(+), 130 deletions(-) diff --git a/src/chains.rs b/src/chains.rs index 4eaf0f5acb5..4556946649a 100644 --- a/src/chains.rs +++ b/src/chains.rs @@ -20,61 +20,58 @@ // argument function argument strategy. use rewrite::{Rewrite, RewriteContext}; -use utils::{make_indent, extra_offset}; +use utils::make_indent; use expr::rewrite_call; use syntax::{ast, ptr}; use syntax::codemap::{mk_sp, Span}; use syntax::print::pprust; -pub fn rewrite_chain(orig_expr: &ast::Expr, +pub fn rewrite_chain(mut expr: &ast::Expr, context: &RewriteContext, width: usize, offset: usize) -> Option { - let mut expr = orig_expr; - let mut rewrites = Vec::new(); - let indent = offset + context.config.tab_spaces; - let max_width = try_opt!(context.config.max_width.checked_sub(indent)); + let total_span = expr.span; + let mut subexpr_list = vec![expr]; - while let Some(pair) = pop_expr_chain(expr, orig_expr.span, context, max_width, indent) { - let (rewrite, parent_expr) = pair; - - rewrites.push(try_opt!(rewrite)); - expr = parent_expr; + while let Some(subexpr) = pop_expr_chain(expr) { + subexpr_list.push(subexpr); + expr = subexpr; } + let parent = subexpr_list.pop().unwrap(); let parent_rewrite = try_opt!(expr.rewrite(context, width, offset)); + let (extra_indent, extend) = if !parent_rewrite.contains('\n') && is_continuable(parent) || + parent_rewrite.len() <= context.config.tab_spaces { + (parent_rewrite.len(), true) + } else { + (context.config.tab_spaces, false) + }; + let indent = offset + extra_indent; + + let max_width = try_opt!(width.checked_sub(extra_indent)); + let rewrites = try_opt!(subexpr_list.into_iter() + .rev() + .map(|e| { + rewrite_chain_expr(e, + total_span, + context, + max_width, + indent) + }) + .collect::>>()); + let total_width = rewrites.iter().fold(0, |a, b| a + b.len()) + parent_rewrite.len(); let fits_single_line = total_width <= width && rewrites.iter().all(|s| !s.contains('\n')); - if rewrites.len() == 1 && !fits_single_line && - (is_continuable(expr) || parent_rewrite.len() <= context.config.tab_spaces) { - let extra_offset = extra_offset(&parent_rewrite, offset); - let offset = offset + extra_offset; - let max_width = try_opt!(width.checked_sub(extra_offset)); - - let rerewrite = pop_expr_chain(orig_expr, orig_expr.span, context, max_width, offset) - .unwrap() - .0; - - return Some(format!("{}{}", parent_rewrite, try_opt!(rerewrite))); - } - let connector = if fits_single_line { String::new() } else { format!("\n{}", make_indent(indent)) }; - // FIXME: don't do this. There's a more efficient way. VecDeque? - rewrites.reverse(); - - // Put the first link on the same line as parent, if it fits. - let first_connector = if parent_rewrite.len() + rewrites[0].len() <= width && - is_continuable(expr) && - !rewrites[0].contains('\n') || - parent_rewrite.len() <= context.config.tab_spaces { + let first_connector = if extend { "" } else { &connector[..] @@ -83,32 +80,36 @@ pub fn rewrite_chain(orig_expr: &ast::Expr, Some(format!("{}{}{}", parent_rewrite, first_connector, rewrites.join(&connector))) } -// Returns None when the expression is not a chainable. Otherwise, rewrites the -// outermost chain element and returns the remaining chain. -fn pop_expr_chain<'a>(expr: &'a ast::Expr, +fn pop_expr_chain<'a>(expr: &'a ast::Expr) -> Option<&'a ast::Expr> { + match expr.node { + ast::Expr_::ExprMethodCall(_, _, ref expressions) => { + Some(&expressions[0]) + } + ast::Expr_::ExprTupField(ref subexpr, _) | + ast::Expr_::ExprField(ref subexpr, _) => { + Some(subexpr) + } + _ => None, + } +} + +fn rewrite_chain_expr(expr: &ast::Expr, span: Span, context: &RewriteContext, width: usize, offset: usize) - -> Option<(Option, &'a ast::Expr)> { + -> Option { match expr.node { ast::Expr_::ExprMethodCall(ref method_name, ref types, ref expressions) => { - Some((rewrite_method_call(method_name.node, - types, - expressions, - span, - context, - width, - offset), - &expressions[0])) + rewrite_method_call(method_name.node, types, expressions, span, context, width, offset) } - ast::Expr_::ExprField(ref subexpr, ref field) => { - Some((Some(format!(".{}", field.node)), subexpr)) + ast::Expr_::ExprField(_, ref field) => { + Some(format!(".{}", field.node)) } - ast::Expr_::ExprTupField(ref subexpr, ref field) => { - Some((Some(format!(".{}", field.node)), subexpr)) + ast::Expr_::ExprTupField(_, ref field) => { + Some(format!(".{}", field.node)) } - _ => None, + _ => unreachable!(), } } diff --git a/src/expr.rs b/src/expr.rs index 69fea0e5927..986b5d50a1f 100644 --- a/src/expr.rs +++ b/src/expr.rs @@ -1017,8 +1017,8 @@ fn rewrite_struct_lit<'a>(context: &RewriteContext, }; let field_iter = fields.into_iter() - .map(StructLitField::Regular) - .chain(base.into_iter().map(StructLitField::Base)); + .map(StructLitField::Regular) + .chain(base.into_iter().map(StructLitField::Base)); let inner_context = &RewriteContext { block_indent: indent, ..*context }; @@ -1030,8 +1030,8 @@ fn rewrite_struct_lit<'a>(context: &RewriteContext, StructLitField::Regular(ref field) => field.span.lo, StructLitField::Base(ref expr) => { let last_field_hi = fields.last() - .map_or(span.lo, - |field| field.span.hi); + .map_or(span.lo, + |field| field.span.hi); let snippet = context.snippet(mk_sp(last_field_hi, expr.span.lo)); let pos = snippet.find_uncommented("..").unwrap(); @@ -1104,8 +1104,8 @@ fn rewrite_field(context: &RewriteContext, -> Option { let name = &field.ident.node.to_string(); let overhead = name.len() + 2; - let expr = - field.expr.rewrite(context, try_opt!(width.checked_sub(overhead)), offset + overhead); + let expr = field.expr + .rewrite(context, try_opt!(width.checked_sub(overhead)), offset + overhead); expr.map(|s| format!("{}: {}", name, s)) } diff --git a/src/items.rs b/src/items.rs index ef8ad37b126..a2e562d9d26 100644 --- a/src/items.rs +++ b/src/items.rs @@ -332,13 +332,14 @@ impl<'a> FmtVisitor<'a> { // Account for sugary self. // FIXME: the comment for the self argument is dropped. This is blocked // on rust issue #27522. - let min_args = explicit_self - .and_then(|explicit_self| rewrite_explicit_self(explicit_self, args)) - .map(|self_str| { - arg_item_strs[0] = self_str; - 2 - }) - .unwrap_or(1); + let min_args = explicit_self.and_then(|explicit_self| { + rewrite_explicit_self(explicit_self, args) + }) + .map(|self_str| { + arg_item_strs[0] = self_str; + 2 + }) + .unwrap_or(1); // Comments between args let mut arg_items = Vec::new(); @@ -761,9 +762,11 @@ impl<'a> FmtVisitor<'a> { let indent = self.block_indent + self.config.tab_spaces; let mut attr_str = field.node - .attrs - .rewrite(&self.get_context(), self.config.max_width - indent, indent) - .unwrap(); + .attrs + .rewrite(&self.get_context(), + self.config.max_width - indent, + indent) + .unwrap(); if !attr_str.is_empty() { attr_str.push('\n'); attr_str.push_str(&make_indent(indent)); @@ -804,18 +807,18 @@ impl<'a> FmtVisitor<'a> { // 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()); + .map(|ty_param| ty_param.rewrite(&context, h_budget, offset).unwrap()); // Extract comments between generics. let lt_spans = lifetimes.iter() - .map(|l| { - let hi = if l.bounds.is_empty() { - l.lifetime.span.hi - } else { - l.bounds[l.bounds.len() - 1].span.hi - }; - codemap::mk_sp(l.lifetime.span.lo, hi) - }); + .map(|l| { + let hi = if l.bounds.is_empty() { + l.lifetime.span.hi + } else { + l.bounds[l.bounds.len() - 1].span.hi + }; + codemap::mk_sp(l.lifetime.span.lo, hi) + }); let ty_spans = tys.iter().map(span_for_ty_param); let items = itemize_list(self.codemap, diff --git a/src/types.rs b/src/types.rs index 9f3cc17e4c7..83e92ff314b 100644 --- a/src/types.rs +++ b/src/types.rs @@ -279,22 +279,24 @@ impl Rewrite for ast::WherePredicate { .. }) => { if !bound_lifetimes.is_empty() { let lifetime_str = bound_lifetimes.iter() - .map(|lt| lt.rewrite(context, width, offset).unwrap()) - .collect::>() - .join(", "); + .map(|lt| { + lt.rewrite(context, width, offset) + .unwrap() + }) + .collect::>() + .join(", "); let type_str = pprust::ty_to_string(bounded_ty); // 8 = "for<> : ".len() let used_width = lifetime_str.len() + type_str.len() + 8; let bounds_str = bounds.iter() - .map(|ty_bound| { - ty_bound - .rewrite(context, - width - used_width, - offset + used_width) - .unwrap() - }) - .collect::>() - .join(" + "); + .map(|ty_bound| { + ty_bound.rewrite(context, + width - used_width, + offset + used_width) + .unwrap() + }) + .collect::>() + .join(" + "); format!("for<{}> {}: {}", lifetime_str, type_str, bounds_str) } else { @@ -302,15 +304,14 @@ impl Rewrite for ast::WherePredicate { // 2 = ": ".len() let used_width = type_str.len() + 2; let bounds_str = bounds.iter() - .map(|ty_bound| { - ty_bound - .rewrite(context, - width - used_width, - offset + used_width) - .unwrap() - }) - .collect::>() - .join(" + "); + .map(|ty_bound| { + ty_bound.rewrite(context, + width - used_width, + offset + used_width) + .unwrap() + }) + .collect::>() + .join(" + "); format!("{}: {}", type_str, bounds_str) } diff --git a/src/utils.rs b/src/utils.rs index faa953ebea3..cb1dd718375 100644 --- a/src/utils.rs +++ b/src/utils.rs @@ -95,15 +95,15 @@ pub fn contains_skip(attrs: &[Attribute]) -> bool { #[inline] pub fn end_typaram(typaram: &ast::TyParam) -> BytePos { typaram.bounds - .last() - .map(|bound| { - match *bound { - ast::RegionTyParamBound(ref lt) => lt.span, - ast::TraitTyParamBound(ref prt, _) => prt.span, - } - }) - .unwrap_or(typaram.span) - .hi + .last() + .map(|bound| { + match *bound { + ast::RegionTyParamBound(ref lt) => lt.span, + ast::TraitTyParamBound(ref prt, _) => prt.span, + } + }) + .unwrap_or(typaram.span) + .hi } #[inline] diff --git a/src/visitor.rs b/src/visitor.rs index 1159116b010..54ada1b7455 100644 --- a/src/visitor.rs +++ b/src/visitor.rs @@ -288,11 +288,10 @@ impl<'a> FmtVisitor<'a> { if utils::contains_skip(attrs) { true } else { - let rewrite = attrs - .rewrite(&self.get_context(), - self.config.max_width - self.block_indent, - self.block_indent) - .unwrap(); + let rewrite = attrs.rewrite(&self.get_context(), + self.config.max_width - self.block_indent, + self.block_indent) + .unwrap(); self.buffer.push_str(&rewrite); let last = attrs.last().unwrap(); self.last_pos = last.span.hi; diff --git a/tests/system.rs b/tests/system.rs index ea98d94f412..4db15dbcfd1 100644 --- a/tests/system.rs +++ b/tests/system.rs @@ -162,17 +162,17 @@ fn read_significant_comments(file_name: &str) -> HashMap { .expect("Failed creating pattern 2."); reader.lines() - .map(|line| line.ok().expect("Failed getting line.")) - .take_while(|line| line_regex.is_match(&line)) - .filter_map(|line| { - regex.captures_iter(&line) - .next() - .map(|capture| { - (capture.at(1).expect("Couldn\'t unwrap capture.").to_owned(), - capture.at(2).expect("Couldn\'t unwrap capture.").to_owned()) - }) - }) - .collect() + .map(|line| line.ok().expect("Failed getting line.")) + .take_while(|line| line_regex.is_match(&line)) + .filter_map(|line| { + regex.captures_iter(&line) + .next() + .map(|capture| { + (capture.at(1).expect("Couldn\'t unwrap capture.").to_owned(), + capture.at(2).expect("Couldn\'t unwrap capture.").to_owned()) + }) + }) + .collect() } // Compare output to input. diff --git a/tests/target/chains.rs b/tests/target/chains.rs index daa60d09855..282dc58b492 100644 --- a/tests/target/chains.rs +++ b/tests/target/chains.rs @@ -6,8 +6,8 @@ fn main() { bbbbbbbbbbbbbbbbbbb.ccccccccccccccccccccccccccccccccccccc.ddddddddddddddddddddddddddd(); bbbbbbbbbbbbbbbbbbb.ccccccccccccccccccccccccccccccccccccc - .ddddddddddddddddddddddddddd - .eeeeeeee(); + .ddddddddddddddddddddddddddd + .eeeeeeee(); x().y(|| { match cond() { @@ -34,13 +34,13 @@ fn main() { }); let suuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuum = xxxxxxx.map(|x| x + 5) - .map(|x| x / 2) - .fold(0, |acc, x| acc + x); + .map(|x| x / 2) + .fold(0, + |acc, x| acc + x); - aaaaaaaaaaaaaaaa - .map(|x| { - x += 1; - x - }) - .filter(some_mod::some_filter) + aaaaaaaaaaaaaaaa.map(|x| { + x += 1; + x + }) + .filter(some_mod::some_filter) } From 48d17f54d34c5457e88ef7ffa234601c6e342a70 Mon Sep 17 00:00:00 2001 From: Marcus Klaas Date: Wed, 9 Sep 2015 23:15:37 +0200 Subject: [PATCH 09/11] Rebase onto master --- src/expr.rs | 28 ++------ src/lists.rs | 152 +++++++++++++++++++++--------------------- tests/target/match.rs | 2 +- 3 files changed, 85 insertions(+), 97 deletions(-) diff --git a/src/expr.rs b/src/expr.rs index 986b5d50a1f..4da191e8bf8 100644 --- a/src/expr.rs +++ b/src/expr.rs @@ -723,11 +723,8 @@ impl Rewrite for ast::Arm { // Let's try and get the arm body on the same line as the condition. // 4 = ` => `.len() if context.config.max_width > line_start + comma.len() + 4 { - let inner_offset = line_start + 4; let budget = context.config.max_width - line_start - comma.len() - 4; - if let Some(ref body_str) = body.rewrite(context, - budget, - line_start + 4) { + if let Some(ref body_str) = body.rewrite(context, budget, line_start + 4) { if first_line_width(body_str) <= budget { return Some(format!("{}{} => {}{}", attr_str.trim_left(), @@ -928,8 +925,12 @@ fn rewrite_call_inner(context: &RewriteContext, None => return Err(Ordering::Greater), }; let offset = offset + extra_offset + 1; - let inner_indent = expr_indent(context, offset); - let inner_context = context.overflow_context(inner_indent - context.block_indent); + let block_indent = if args.len() == 1 { + context.block_indent + } else { + offset + }; + let inner_context = &RewriteContext { block_indent: block_indent, ..*context }; let items = itemize_list(context.codemap, args.iter(), @@ -953,21 +954,6 @@ fn rewrite_call_inner(context: &RewriteContext, Ok(format!("{}({})", callee_str, list_str)) } -macro_rules! block_indent_helper { - ($name:ident, $option:ident) => ( - fn $name(context: &RewriteContext, offset: usize) -> usize { - match context.config.$option { - BlockIndentStyle::Inherit => context.block_indent, - BlockIndentStyle::Tabbed => context.block_indent + context.config.tab_spaces, - BlockIndentStyle::Visual => offset, - } - } - ); -} - -block_indent_helper!(expr_indent, expr_indent_style); -block_indent_helper!(closure_indent, closure_indent_style); - fn rewrite_paren(context: &RewriteContext, subexpr: &ast::Expr, width: usize, diff --git a/src/lists.rs b/src/lists.rs index 0afee8f79ac..3435bd8170b 100644 --- a/src/lists.rs +++ b/src/lists.rs @@ -269,18 +269,21 @@ impl<'a, T, I, F1, F2, F3> Iterator for ListItems<'a, I, F1, F2, F3> fn next(&mut self) -> Option { let white_space: &[_] = &[' ', '\t']; - self.inner.next().map(|item| { - let mut new_lines = false; - // Pre-comment - let pre_snippet = self.codemap.span_to_snippet(codemap::mk_sp(self.prev_span_end, - (self.get_lo)(&item))) - .unwrap(); - let trimmed_pre_snippet = pre_snippet.trim(); - let pre_comment = if !trimmed_pre_snippet.is_empty() { - Some(trimmed_pre_snippet.to_owned()) - } else { - None - }; + self.inner + .next() + .map(|item| { + let mut new_lines = false; + // Pre-comment + let pre_snippet = self.codemap + .span_to_snippet(codemap::mk_sp(self.prev_span_end, + (self.get_lo)(&item))) + .unwrap(); + let trimmed_pre_snippet = pre_snippet.trim(); + let pre_comment = if !trimmed_pre_snippet.is_empty() { + Some(trimmed_pre_snippet.to_owned()) + } else { + None + }; // Post-comment let next_start = match self.inner.peek() { @@ -300,76 +303,75 @@ impl<'a, T, I, F1, F2, F3> Iterator for ListItems<'a, I, F1, F2, F3> match (block_open_index, newline_index) { // Separator before comment, with the next item on same line. - // Comment belongs to next item. - (Some(i), None) if i > separator_index => { - separator_index + 1 + // Comment belongs to next item. + (Some(i), None) if i > separator_index => { + separator_index + 1 + } + // Block-style post-comment before the separator. + (Some(i), None) => { + cmp::max(find_comment_end(&post_snippet[i..]).unwrap() + i, + separator_index + 1) + } + // Block-style post-comment. Either before or after the separator. + (Some(i), Some(j)) if i < j => { + cmp::max(find_comment_end(&post_snippet[i..]).unwrap() + i, + separator_index + 1) + } + // Potential *single* line comment. + (_, Some(j)) => j + 1, + _ => post_snippet.len(), } - // Block-style post-comment before the separator. - (Some(i), None) => { - cmp::max(find_comment_end(&post_snippet[i..]).unwrap() + i, - separator_index + 1) - } - // Block-style post-comment. Either before or after the separator. - (Some(i), Some(j)) if i < j => { - cmp::max(find_comment_end(&post_snippet[i..]).unwrap() + i, - separator_index + 1) - } - // Potential *single* line comment. - (_, Some(j)) => j + 1, - _ => post_snippet.len() } - }, - None => { - post_snippet.find_uncommented(self.terminator) - .unwrap_or(post_snippet.len()) + None => { + post_snippet.find_uncommented(self.terminator).unwrap_or(post_snippet.len()) + } + }; + + if !post_snippet.is_empty() && comment_end > 0 { + // Account for extra whitespace between items. This is fiddly + // because of the way we divide pre- and post- comments. + + // Everything from the separator to the next item. + let test_snippet = &post_snippet[comment_end-1..]; + let first_newline = test_snippet.find('\n').unwrap_or(test_snippet.len()); + // From the end of the first line of comments. + let test_snippet = &test_snippet[first_newline..]; + let first = test_snippet.find(|c: char| !c.is_whitespace()) + .unwrap_or(test_snippet.len()); + // From the end of the first line of comments to the next non-whitespace char. + let test_snippet = &test_snippet[..first]; + + if test_snippet.chars().filter(|c| c == &'\n').count() > 1 { + // There were multiple line breaks which got trimmed to nothing. + new_lines = true; + } } - }; - if !post_snippet.is_empty() && comment_end > 0 { - // Account for extra whitespace between items. This is fiddly - // because of the way we divide pre- and post- comments. + // Cleanup post-comment: strip separators and whitespace. + self.prev_span_end = (self.get_hi)(&item) + BytePos(comment_end as u32); + let post_snippet = post_snippet[..comment_end].trim(); - // Everything from the separator to the next item. - let test_snippet = &post_snippet[comment_end-1..]; - let first_newline = test_snippet.find('\n').unwrap_or(test_snippet.len()); - // From the end of the first line of comments. - let test_snippet = &test_snippet[first_newline..]; - let first = test_snippet.find(|c: char| !c.is_whitespace()) - .unwrap_or(test_snippet.len()); - // From the end of the first line of comments to the next non-whitespace char. - let test_snippet = &test_snippet[..first]; + let post_snippet_trimmed = if post_snippet.starts_with(',') { + post_snippet[1..].trim_matches(white_space) + } else if post_snippet.ends_with(",") { + post_snippet[..(post_snippet.len() - 1)].trim_matches(white_space) + } else { + post_snippet + }; - if test_snippet.chars().filter(|c| c == &'\n').count() > 1 { - // There were multiple line breaks which got trimmed to nothing. - new_lines = true; + let post_comment = if !post_snippet_trimmed.is_empty() { + Some(post_snippet_trimmed.to_owned()) + } else { + None + }; + + ListItem { + pre_comment: pre_comment, + item: (self.get_item_string)(&item), + post_comment: post_comment, + new_lines: new_lines, } - } - - // Cleanup post-comment: strip separators and whitespace. - self.prev_span_end = (self.get_hi)(&item) + BytePos(comment_end as u32); - let post_snippet = post_snippet[..comment_end].trim(); - - let post_snippet_trimmed = if post_snippet.starts_with(',') { - post_snippet[1..].trim_matches(white_space) - } else if post_snippet.ends_with(",") { - post_snippet[..(post_snippet.len() - 1)].trim_matches(white_space) - } else { - post_snippet - }; - - let post_comment = if !post_snippet_trimmed.is_empty() { - Some(post_snippet_trimmed.to_owned()) - } else { - None - }; - - ListItem { - pre_comment: pre_comment, - item: (self.get_item_string)(&item), - post_comment: post_comment, - new_lines: new_lines, - } - }) + }) } } diff --git a/tests/target/match.rs b/tests/target/match.rs index eab8c604084..5cfbb5a7072 100644 --- a/tests/target/match.rs +++ b/tests/target/match.rs @@ -57,7 +57,7 @@ fn foo() { // Test that a match on an overflow line is laid out properly. fn main() { let sub_span = - match self.span.sub_span_after_keywooooooooooooooooooooord(use_item.span, keywords::As) { + match xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx { Some(sub_span) => Some(sub_span), None => sub_span, }; From 03c660633fbac0492f891aed6c47731be90f23ca Mon Sep 17 00:00:00 2001 From: Marcus Klaas Date: Wed, 9 Sep 2015 23:17:31 +0200 Subject: [PATCH 10/11] Refine chain breaking heuristics Don't make a single line chain when it is was multi line in the source; allow overflow of the last chain element onto the next lines without breaking the chain. --- src/chains.rs | 87 ++++++++++--- src/config.rs | 15 +-- src/expr.rs | 23 ++-- src/items.rs | 22 ++-- src/lib.rs | 1 + src/lists.rs | 190 ++++++++++++++-------------- src/types.rs | 25 ++-- tests/config/small_tabs.toml | 4 +- tests/source/chains-no-overflow.rs | 38 ++++++ tests/source/chains.rs | 9 +- tests/source/expr-no-hints.rs | 8 ++ tests/source/expr-visual-indent.rs | 9 -- tests/system.rs | 10 +- tests/target/chains-no-overflow.rs | 45 +++++++ tests/target/chains-no-overlow-2.rs | 16 +++ tests/target/chains.rs | 36 +++--- tests/target/expr-no-hints.rs | 6 + tests/target/expr-visual-indent.rs | 9 -- 18 files changed, 351 insertions(+), 202 deletions(-) create mode 100644 tests/source/chains-no-overflow.rs create mode 100644 tests/source/expr-no-hints.rs delete mode 100644 tests/source/expr-visual-indent.rs create mode 100644 tests/target/chains-no-overflow.rs create mode 100644 tests/target/chains-no-overlow-2.rs create mode 100644 tests/target/expr-no-hints.rs delete mode 100644 tests/target/expr-visual-indent.rs diff --git a/src/chains.rs b/src/chains.rs index 4556946649a..bd9f302824b 100644 --- a/src/chains.rs +++ b/src/chains.rs @@ -20,7 +20,7 @@ // argument function argument strategy. use rewrite::{Rewrite, RewriteContext}; -use utils::make_indent; +use utils::{first_line_width, make_indent}; use expr::rewrite_call; use syntax::{ast, ptr}; @@ -51,19 +51,67 @@ pub fn rewrite_chain(mut expr: &ast::Expr, let indent = offset + extra_indent; let max_width = try_opt!(width.checked_sub(extra_indent)); - let rewrites = try_opt!(subexpr_list.into_iter() - .rev() - .map(|e| { - rewrite_chain_expr(e, - total_span, - context, - max_width, - indent) - }) - .collect::>>()); + let mut rewrites = try_opt!(subexpr_list.iter() + .rev() + .map(|e| { + rewrite_chain_expr(e, + total_span, + context, + max_width, + indent) + }) + .collect::>>()); - let total_width = rewrites.iter().fold(0, |a, b| a + b.len()) + parent_rewrite.len(); - let fits_single_line = total_width <= width && rewrites.iter().all(|s| !s.contains('\n')); + // Total of all items excluding the last. + let almost_total = rewrites.split_last() + .unwrap() + .1 + .iter() + .fold(0, |a, b| a + first_line_width(b)) + + parent_rewrite.len(); + let total_width = almost_total + first_line_width(rewrites.last().unwrap()); + let veto_single_line = if context.config.take_source_hints && subexpr_list.len() > 1 { + // Look at the source code. Unless all chain elements start on the same + // line, we won't consider putting them on a single line either. + let first_line_no = context.codemap.lookup_char_pos(subexpr_list[0].span.lo).line; + + subexpr_list[1..] + .iter() + .any(|ex| context.codemap.lookup_char_pos(ex.span.hi).line != first_line_no) + } else { + false + }; + + let fits_single_line = !veto_single_line && + match subexpr_list[0].node { + ast::Expr_::ExprMethodCall(ref method_name, ref types, ref expressions) + if context.config.chains_overflow_last => { + let (last, init) = rewrites.split_last_mut().unwrap(); + + if init.iter().all(|s| !s.contains('\n')) && total_width <= width { + let last_rewrite = width.checked_sub(almost_total) + .and_then(|inner_width| { + rewrite_method_call(method_name.node, + types, + expressions, + total_span, + context, + inner_width, + offset + almost_total) + }); + match last_rewrite { + Some(mut string) => { + ::std::mem::swap(&mut string, last); + true + } + None => false, + } + } else { + false + } + } + _ => total_width <= width && rewrites.iter().all(|s| !s.contains('\n')), + }; let connector = if fits_single_line { String::new() @@ -101,7 +149,11 @@ fn rewrite_chain_expr(expr: &ast::Expr, -> Option { match expr.node { ast::Expr_::ExprMethodCall(ref method_name, ref types, ref expressions) => { - rewrite_method_call(method_name.node, types, expressions, span, context, width, offset) + let inner = &RewriteContext { + block_indent: offset, + ..*context + }; + rewrite_method_call(method_name.node, types, expressions, span, inner, width, offset) } ast::Expr_::ExprField(_, ref field) => { Some(format!(".{}", field.node)) @@ -137,10 +189,7 @@ fn rewrite_method_call(method_name: ast::Ident, }; let callee_str = format!(".{}{}", method_name, type_str); - let inner_context = &RewriteContext { - block_indent: offset, - ..*context - }; + let span = mk_sp(args[0].span.hi, span.hi); - rewrite_call(inner_context, &callee_str, args, span, width, offset) + rewrite_call(context, &callee_str, &args[1..], span, width, offset) } diff --git a/src/config.rs b/src/config.rs index 95994531950..d45f184051f 100644 --- a/src/config.rs +++ b/src/config.rs @@ -133,8 +133,8 @@ create_config! { fn_args_density: Density, fn_args_layout: StructLitStyle, fn_arg_indent: BlockIndentStyle, - where_density: Density, // Should we at least try to put the where clause on the same line as - // the rest of the function decl? + where_density: Density, // Should we at least try to put the where clause on + // the same line as the rest of the function decl? where_indent: BlockIndentStyle, // Visual will be treated like Tabbed where_layout: ListTactic, where_pred_indent: BlockIndentStyle, @@ -147,14 +147,14 @@ create_config! { report_todo: ReportTactic, report_fixme: ReportTactic, reorder_imports: bool, // Alphabetically, case sensitive. - expr_indent_style: BlockIndentStyle, - closure_indent_style: BlockIndentStyle, single_line_if_else: bool, format_strings: bool, + chains_overflow_last: bool, + take_source_hints: bool, // Retain some formatting characteristics from + // the source code. } impl Default for Config { - fn default() -> Config { Config { max_width: 100, @@ -181,11 +181,10 @@ impl Default for Config { report_todo: ReportTactic::Always, report_fixme: ReportTactic::Never, reorder_imports: false, - expr_indent_style: BlockIndentStyle::Tabbed, - closure_indent_style: BlockIndentStyle::Visual, single_line_if_else: false, format_strings: true, + chains_overflow_last: true, + take_source_hints: true, } } - } diff --git a/src/expr.rs b/src/expr.rs index 4da191e8bf8..98b93a5faa7 100644 --- a/src/expr.rs +++ b/src/expr.rs @@ -40,6 +40,12 @@ impl Rewrite for ast::Expr { } } ast::Expr_::ExprCall(ref callee, ref args) => { + // FIXME using byte lens instead of char lens (and probably all over the place too) + // 2 is for parens + let max_callee_width = try_opt!(width.checked_sub(2)); + let callee_str = try_opt!(callee.rewrite(context, max_callee_width, offset)); + let span = mk_sp(callee.span.hi, self.span.hi); + rewrite_call(context, &**callee, args, self.span, width, offset) } ast::Expr_::ExprParen(ref subexpr) => { @@ -284,8 +290,10 @@ impl Rewrite for ast::Block { }; if is_simple_block(self, context.codemap) && prefix.len() < width { - let body = - self.expr.as_ref().unwrap().rewrite(context, width - prefix.len(), offset); + let body = self.expr + .as_ref() + .unwrap() + .rewrite(context, width - prefix.len(), offset); if let Some(ref expr_str) = body { let result = format!("{}{{ {} }}", prefix, expr_str); if result.len() <= width && !result.contains('\n') { @@ -677,15 +685,13 @@ impl Rewrite for ast::Arm { total_width += (pat_strs.len() - 1) * 3; let mut vertical = total_width > pat_budget || pat_strs.iter().any(|p| p.contains('\n')); - if !vertical { + if !vertical && context.config.take_source_hints { // If the patterns were previously stacked, keep them stacked. - // FIXME should be an option. let pat_span = mk_sp(pats[0].span.lo, pats[pats.len() - 1].span.hi); let pat_str = context.snippet(pat_span); vertical = pat_str.find('\n').is_some(); } - let pats_width = if vertical { pat_strs[pat_strs.len() - 1].len() } else { @@ -1015,9 +1021,10 @@ fn rewrite_struct_lit<'a>(context: &RewriteContext, match *item { StructLitField::Regular(ref field) => field.span.lo, StructLitField::Base(ref expr) => { - let last_field_hi = fields.last() - .map_or(span.lo, - |field| field.span.hi); + let last_field_hi = fields.last().map_or(span.lo, + |field| { + field.span.hi + }); let snippet = context.snippet(mk_sp(last_field_hi, expr.span.lo)); let pos = snippet.find_uncommented("..").unwrap(); diff --git a/src/items.rs b/src/items.rs index a2e562d9d26..2547e8f5dd5 100644 --- a/src/items.rs +++ b/src/items.rs @@ -512,8 +512,9 @@ impl<'a> FmtVisitor<'a> { |arg| arg.ty.span.hi, |arg| { // FIXME silly width, indent - arg.ty.rewrite(&self.get_context(), 1000, 0) - .unwrap() + arg.ty + .rewrite(&self.get_context(), 1000, 0) + .unwrap() }, span_after(field.span, "(", self.codemap), next_span_start); @@ -810,15 +811,14 @@ impl<'a> FmtVisitor<'a> { .map(|ty_param| ty_param.rewrite(&context, h_budget, offset).unwrap()); // Extract comments between generics. - let lt_spans = lifetimes.iter() - .map(|l| { - let hi = if l.bounds.is_empty() { - l.lifetime.span.hi - } else { - l.bounds[l.bounds.len() - 1].span.hi - }; - codemap::mk_sp(l.lifetime.span.lo, hi) - }); + let lt_spans = lifetimes.iter().map(|l| { + let hi = if l.bounds.is_empty() { + l.lifetime.span.hi + } else { + l.bounds[l.bounds.len() - 1].span.hi + }; + codemap::mk_sp(l.lifetime.span.lo, hi) + }); let ty_spans = tys.iter().map(span_for_ty_param); let items = itemize_list(self.codemap, diff --git a/src/lib.rs b/src/lib.rs index 6993d3044d3..3f8900d6713 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -10,6 +10,7 @@ #![feature(rustc_private)] #![feature(custom_attribute)] +#![feature(slice_splits)] #![allow(unused_attributes)] // TODO we're going to allocate a whole bunch of temp Strings, is it worth diff --git a/src/lists.rs b/src/lists.rs index 3435bd8170b..678c84c199a 100644 --- a/src/lists.rs +++ b/src/lists.rs @@ -269,109 +269,107 @@ impl<'a, T, I, F1, F2, F3> Iterator for ListItems<'a, I, F1, F2, F3> fn next(&mut self) -> Option { let white_space: &[_] = &[' ', '\t']; - self.inner - .next() - .map(|item| { - let mut new_lines = false; - // Pre-comment - let pre_snippet = self.codemap - .span_to_snippet(codemap::mk_sp(self.prev_span_end, - (self.get_lo)(&item))) - .unwrap(); - let trimmed_pre_snippet = pre_snippet.trim(); - let pre_comment = if !trimmed_pre_snippet.is_empty() { - Some(trimmed_pre_snippet.to_owned()) - } else { - None - }; + self.inner.next().map(|item| { + let mut new_lines = false; + // Pre-comment + let pre_snippet = self.codemap + .span_to_snippet(codemap::mk_sp(self.prev_span_end, + (self.get_lo)(&item))) + .unwrap(); + let trimmed_pre_snippet = pre_snippet.trim(); + let pre_comment = if !trimmed_pre_snippet.is_empty() { + Some(trimmed_pre_snippet.to_owned()) + } else { + None + }; - // Post-comment - let next_start = match self.inner.peek() { - Some(ref next_item) => (self.get_lo)(next_item), - None => self.next_span_start, - }; - let post_snippet = self.codemap - .span_to_snippet(codemap::mk_sp((self.get_hi)(&item), - next_start)) - .unwrap(); + // Post-comment + let next_start = match self.inner.peek() { + Some(ref next_item) => (self.get_lo)(next_item), + None => self.next_span_start, + }; + let post_snippet = self.codemap + .span_to_snippet(codemap::mk_sp((self.get_hi)(&item), + next_start)) + .unwrap(); - let comment_end = match self.inner.peek() { - Some(..) => { - let block_open_index = post_snippet.find("/*"); - let newline_index = post_snippet.find('\n'); - let separator_index = post_snippet.find_uncommented(",").unwrap(); + let comment_end = match self.inner.peek() { + Some(..) => { + let block_open_index = post_snippet.find("/*"); + let newline_index = post_snippet.find('\n'); + let separator_index = post_snippet.find_uncommented(",").unwrap(); - match (block_open_index, newline_index) { - // Separator before comment, with the next item on same line. - // Comment belongs to next item. - (Some(i), None) if i > separator_index => { - separator_index + 1 - } - // Block-style post-comment before the separator. - (Some(i), None) => { - cmp::max(find_comment_end(&post_snippet[i..]).unwrap() + i, - separator_index + 1) - } - // Block-style post-comment. Either before or after the separator. - (Some(i), Some(j)) if i < j => { - cmp::max(find_comment_end(&post_snippet[i..]).unwrap() + i, - separator_index + 1) - } - // Potential *single* line comment. - (_, Some(j)) => j + 1, - _ => post_snippet.len(), + match (block_open_index, newline_index) { + // Separator before comment, with the next item on same line. + // Comment belongs to next item. + (Some(i), None) if i > separator_index => { + separator_index + 1 } - } - None => { - post_snippet.find_uncommented(self.terminator).unwrap_or(post_snippet.len()) - } - }; - - if !post_snippet.is_empty() && comment_end > 0 { - // Account for extra whitespace between items. This is fiddly - // because of the way we divide pre- and post- comments. - - // Everything from the separator to the next item. - let test_snippet = &post_snippet[comment_end-1..]; - let first_newline = test_snippet.find('\n').unwrap_or(test_snippet.len()); - // From the end of the first line of comments. - let test_snippet = &test_snippet[first_newline..]; - let first = test_snippet.find(|c: char| !c.is_whitespace()) - .unwrap_or(test_snippet.len()); - // From the end of the first line of comments to the next non-whitespace char. - let test_snippet = &test_snippet[..first]; - - if test_snippet.chars().filter(|c| c == &'\n').count() > 1 { - // There were multiple line breaks which got trimmed to nothing. - new_lines = true; + // Block-style post-comment before the separator. + (Some(i), None) => { + cmp::max(find_comment_end(&post_snippet[i..]).unwrap() + i, + separator_index + 1) + } + // Block-style post-comment. Either before or after the separator. + (Some(i), Some(j)) if i < j => { + cmp::max(find_comment_end(&post_snippet[i..]).unwrap() + i, + separator_index + 1) + } + // Potential *single* line comment. + (_, Some(j)) => j + 1, + _ => post_snippet.len(), } } - - // Cleanup post-comment: strip separators and whitespace. - self.prev_span_end = (self.get_hi)(&item) + BytePos(comment_end as u32); - let post_snippet = post_snippet[..comment_end].trim(); - - let post_snippet_trimmed = if post_snippet.starts_with(',') { - post_snippet[1..].trim_matches(white_space) - } else if post_snippet.ends_with(",") { - post_snippet[..(post_snippet.len() - 1)].trim_matches(white_space) - } else { - post_snippet - }; - - let post_comment = if !post_snippet_trimmed.is_empty() { - Some(post_snippet_trimmed.to_owned()) - } else { - None - }; - - ListItem { - pre_comment: pre_comment, - item: (self.get_item_string)(&item), - post_comment: post_comment, - new_lines: new_lines, + None => { + post_snippet.find_uncommented(self.terminator).unwrap_or(post_snippet.len()) } - }) + }; + + if !post_snippet.is_empty() && comment_end > 0 { + // Account for extra whitespace between items. This is fiddly + // because of the way we divide pre- and post- comments. + + // Everything from the separator to the next item. + let test_snippet = &post_snippet[comment_end-1..]; + let first_newline = test_snippet.find('\n').unwrap_or(test_snippet.len()); + // From the end of the first line of comments. + let test_snippet = &test_snippet[first_newline..]; + let first = test_snippet.find(|c: char| !c.is_whitespace()) + .unwrap_or(test_snippet.len()); + // From the end of the first line of comments to the next non-whitespace char. + let test_snippet = &test_snippet[..first]; + + if test_snippet.chars().filter(|c| c == &'\n').count() > 1 { + // There were multiple line breaks which got trimmed to nothing. + new_lines = true; + } + } + + // Cleanup post-comment: strip separators and whitespace. + self.prev_span_end = (self.get_hi)(&item) + BytePos(comment_end as u32); + let post_snippet = post_snippet[..comment_end].trim(); + + let post_snippet_trimmed = if post_snippet.starts_with(',') { + post_snippet[1..].trim_matches(white_space) + } else if post_snippet.ends_with(",") { + post_snippet[..(post_snippet.len() - 1)].trim_matches(white_space) + } else { + post_snippet + }; + + let post_comment = if !post_snippet_trimmed.is_empty() { + Some(post_snippet_trimmed.to_owned()) + } else { + None + }; + + ListItem { + pre_comment: pre_comment, + item: (self.get_item_string)(&item), + post_comment: post_comment, + new_lines: new_lines, + } + }) } } diff --git a/src/types.rs b/src/types.rs index 83e92ff314b..e0f3140d4d9 100644 --- a/src/types.rs +++ b/src/types.rs @@ -130,16 +130,16 @@ impl<'a> Rewrite for SegmentParam<'a> { // FIXME doesn't always use width, offset fn rewrite(&self, context: &RewriteContext, width: usize, offset: usize) -> Option { Some(match *self { - SegmentParam::LifeTime(ref lt) => { - pprust::lifetime_to_string(lt) - } - SegmentParam::Type(ref ty) => { - try_opt!(ty.rewrite(context, width, offset)) - } - SegmentParam::Binding(ref binding) => { - format!("{} = {}", binding.ident, try_opt!(binding.ty.rewrite(context, width, offset))) - } - }) + SegmentParam::LifeTime(ref lt) => { + pprust::lifetime_to_string(lt) + } + SegmentParam::Type(ref ty) => { + try_opt!(ty.rewrite(context, width, offset)) + } + SegmentParam::Binding(ref binding) => { + format!("{} = {}", binding.ident, try_opt!(binding.ty.rewrite(context, width, offset))) + } + }) } } @@ -368,8 +368,9 @@ impl Rewrite for ast::TyParamBound { impl Rewrite for ast::TyParamBounds { fn rewrite(&self, context: &RewriteContext, width: usize, offset: usize) -> Option { - let strs: Vec<_> = - self.iter().map(|b| b.rewrite(context, width, offset).unwrap()).collect(); + let strs: Vec<_> = self.iter() + .map(|b| b.rewrite(context, width, offset).unwrap()) + .collect(); Some(strs.join(" + ")) } } diff --git a/tests/config/small_tabs.toml b/tests/config/small_tabs.toml index c5122b760a0..754cde57cd9 100644 --- a/tests/config/small_tabs.toml +++ b/tests/config/small_tabs.toml @@ -21,7 +21,7 @@ enum_trailing_comma = true report_todo = "Always" report_fixme = "Never" reorder_imports = false -expr_indent_style = "Tabbed" -closure_indent_style = "Visual" single_line_if_else = false format_strings = true +chains_overflow_last = true +take_source_hints = true diff --git a/tests/source/chains-no-overflow.rs b/tests/source/chains-no-overflow.rs new file mode 100644 index 00000000000..fa2c66524fc --- /dev/null +++ b/tests/source/chains-no-overflow.rs @@ -0,0 +1,38 @@ +// rustfmt-chains_overflow_last: false +// Test chain formatting without overflowing the last item. + +fn main() { + bbbbbbbbbbbbbbbbbbb.ccccccccccccccccccccccccccccccccccccc + .ddddddddddddddddddddddddddd(); + + bbbbbbbbbbbbbbbbbbb.ccccccccccccccccccccccccccccccccccccc.ddddddddddddddddddddddddddd.eeeeeeee(); + + x() + .y(|| match cond() { true => (), false => () }); + + loong_func() + .quux(move || if true { + 1 + } else { + 2 + }); + + fffffffffffffffffffffffffffffffffff(a, + { + SCRIPT_TASK_ROOT + .with(|root| { + // Another case of write_list failing us. + *root.borrow_mut() = Some(&script_task); + }); + }); + + let suuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuum = xxxxxxx + .map(|x| x + 5) + .map(|x| x / 2) + .fold(0, |acc, x| acc + x); + + aaaaaaaaaaaaaaaa.map(|x| { + x += 1; + x + }).filter(some_mod::some_filter) +} diff --git a/tests/source/chains.rs b/tests/source/chains.rs index 8282c3baa6b..d2f11f1dcbe 100644 --- a/tests/source/chains.rs +++ b/tests/source/chains.rs @@ -1,10 +1,10 @@ // Test chain formatting. fn main() { - let a = b.c - .d - .1 - .foo(|x| x + 1); + // Don't put chains on a single list if it wasn't so in source. + let a = b .c + .d.1 + .foo(|x| x + 1); bbbbbbbbbbbbbbbbbbb.ccccccccccccccccccccccccccccccccccccc .ddddddddddddddddddddddddddd(); @@ -25,7 +25,6 @@ fn main() { { SCRIPT_TASK_ROOT .with(|root| { - // Another case of write_list failing us. *root.borrow_mut() = Some(&script_task); }); }); diff --git a/tests/source/expr-no-hints.rs b/tests/source/expr-no-hints.rs new file mode 100644 index 00000000000..7329a742bac --- /dev/null +++ b/tests/source/expr-no-hints.rs @@ -0,0 +1,8 @@ +// rustfmt-take_source_hints: false +// We know best! + +fn main() { + a.b + .c + .d(); +} diff --git a/tests/source/expr-visual-indent.rs b/tests/source/expr-visual-indent.rs deleted file mode 100644 index 3d7c1b92be8..00000000000 --- a/tests/source/expr-visual-indent.rs +++ /dev/null @@ -1,9 +0,0 @@ -// rustfmt-expr_indent_style: Visual - -// Visual level block indentation. - -fn matcher() { - Some(while true { - test(); - }) -} \ No newline at end of file diff --git a/tests/system.rs b/tests/system.rs index 4db15dbcfd1..d0b6ae78faa 100644 --- a/tests/system.rs +++ b/tests/system.rs @@ -165,12 +165,10 @@ fn read_significant_comments(file_name: &str) -> HashMap { .map(|line| line.ok().expect("Failed getting line.")) .take_while(|line| line_regex.is_match(&line)) .filter_map(|line| { - regex.captures_iter(&line) - .next() - .map(|capture| { - (capture.at(1).expect("Couldn\'t unwrap capture.").to_owned(), - capture.at(2).expect("Couldn\'t unwrap capture.").to_owned()) - }) + regex.captures_iter(&line).next().map(|capture| { + (capture.at(1).expect("Couldn\'t unwrap capture.").to_owned(), + capture.at(2).expect("Couldn\'t unwrap capture.").to_owned()) + }) }) .collect() } diff --git a/tests/target/chains-no-overflow.rs b/tests/target/chains-no-overflow.rs new file mode 100644 index 00000000000..b4fdb9f99ce --- /dev/null +++ b/tests/target/chains-no-overflow.rs @@ -0,0 +1,45 @@ +// rustfmt-chains_overflow_last: false +// Test chain formatting without overflowing the last item. + +fn main() { + bbbbbbbbbbbbbbbbbbb.ccccccccccccccccccccccccccccccccccccc.ddddddddddddddddddddddddddd(); + + bbbbbbbbbbbbbbbbbbb.ccccccccccccccccccccccccccccccccccccc + .ddddddddddddddddddddddddddd + .eeeeeeee(); + + x().y(|| { + match cond() { + true => (), + false => (), + } + }); + + loong_func() + .quux(move || { + if true { + 1 + } else { + 2 + } + }); + + fffffffffffffffffffffffffffffffffff(a, + { + SCRIPT_TASK_ROOT.with(|root| { + // Another case of write_list failing us. + *root.borrow_mut() = Some(&script_task); + }); + }); + + let suuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuum = xxxxxxx.map(|x| x + 5) + .map(|x| x / 2) + .fold(0, + |acc, x| acc + x); + + aaaaaaaaaaaaaaaa.map(|x| { + x += 1; + x + }) + .filter(some_mod::some_filter) +} diff --git a/tests/target/chains-no-overlow-2.rs b/tests/target/chains-no-overlow-2.rs new file mode 100644 index 00000000000..d795fa2f961 --- /dev/null +++ b/tests/target/chains-no-overlow-2.rs @@ -0,0 +1,16 @@ +// rustfmt-chains_overflow_last: false + +fn main() { + reader.lines() + .map(|line| line.ok().expect("Failed getting line.")) + .take_while(|line| line_regex.is_match(&line)) + .filter_map(|line| { + regex.captures_iter(&line) + .next() + .map(|capture| { + (capture.at(1).expect("Couldn\'t unwrap capture.").to_owned(), + capture.at(2).expect("Couldn\'t unwrap capture.").to_owned()) + }) + }) + .collect(); +} diff --git a/tests/target/chains.rs b/tests/target/chains.rs index 282dc58b492..e49233e22a2 100644 --- a/tests/target/chains.rs +++ b/tests/target/chains.rs @@ -1,7 +1,11 @@ // Test chain formatting. fn main() { - let a = b.c.d.1.foo(|x| x + 1); + // Don't put chains on a single list if it wasn't so in source. + let a = b.c + .d + .1 + .foo(|x| x + 1); bbbbbbbbbbbbbbbbbbb.ccccccccccccccccccccccccccccccccccccc.ddddddddddddddddddddddddddd(); @@ -10,27 +14,25 @@ fn main() { .eeeeeeee(); x().y(|| { - match cond() { - true => (), - false => (), - } - }); + match cond() { + true => (), + false => (), + } + }); - loong_func() - .quux(move || { - if true { - 1 - } else { - 2 - } - }); + loong_func().quux(move || { + if true { + 1 + } else { + 2 + } + }); fffffffffffffffffffffffffffffffffff(a, { SCRIPT_TASK_ROOT.with(|root| { - // Another case of write_list failing us. - *root.borrow_mut() = Some(&script_task); - }); + *root.borrow_mut() = Some(&script_task); + }); }); let suuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuum = xxxxxxx.map(|x| x + 5) diff --git a/tests/target/expr-no-hints.rs b/tests/target/expr-no-hints.rs new file mode 100644 index 00000000000..62dd64c28a6 --- /dev/null +++ b/tests/target/expr-no-hints.rs @@ -0,0 +1,6 @@ +// rustfmt-take_source_hints: false +// We know best! + +fn main() { + a.b.c.d(); +} diff --git a/tests/target/expr-visual-indent.rs b/tests/target/expr-visual-indent.rs deleted file mode 100644 index f5b79d74fcb..00000000000 --- a/tests/target/expr-visual-indent.rs +++ /dev/null @@ -1,9 +0,0 @@ -// rustfmt-expr_indent_style: Visual - -// Visual level block indentation. - -fn matcher() { - Some(while true { - test(); - }) -} From 7f576b06022357672eb24d6103673f982a6ba817 Mon Sep 17 00:00:00 2001 From: Marcus Klaas Date: Fri, 11 Sep 2015 00:53:21 +0200 Subject: [PATCH 11/11] General cleanup after rebase --- src/comment.rs | 69 +++++++++++++++--------------- src/expr.rs | 57 +++++++++--------------- src/imports.rs | 8 ++-- src/items.rs | 11 ++--- src/utils.rs | 17 ++++---- tests/source/chains-no-overflow.rs | 5 +-- tests/source/chains.rs | 4 +- tests/source/string-lit.rs | 2 +- tests/system.rs | 24 +++++------ tests/target/chains-no-overflow.rs | 4 +- tests/target/chains.rs | 4 +- tests/target/closure.rs | 8 ++-- tests/target/string-lit.rs | 2 +- 13 files changed, 102 insertions(+), 113 deletions(-) diff --git a/src/comment.rs b/src/comment.rs index d0eeba9a7c1..ff7558131d2 100644 --- a/src/comment.rs +++ b/src/comment.rs @@ -41,43 +41,44 @@ pub fn rewrite_comment(orig: &str, block_style: bool, width: usize, offset: usiz let line_breaks = s.chars().filter(|&c| c == '\n').count(); let (_, mut s) = s.lines() - .enumerate() - .map(|(i, mut line)| { - line = line.trim(); - // Drop old closer. - if i == line_breaks && line.ends_with("*/") && !line.starts_with("//") { - line = &line[..(line.len() - 2)]; - } + .enumerate() + .map(|(i, mut line)| { + line = line.trim(); + // Drop old closer. + if i == line_breaks && line.ends_with("*/") && !line.starts_with("//") { + line = &line[..(line.len() - 2)]; + } - line.trim_right() - }) - .map(left_trim_comment_line) - .map(|line| { - if line_breaks == 0 { - line.trim_left() - } else { - line - } - }) - .fold((true, opener.to_owned()), |(first, mut acc), line| { - if !first { - acc.push('\n'); - acc.push_str(&indent_str); - acc.push_str(line_start); - } + line.trim_right() + }) + .map(left_trim_comment_line) + .map(|line| { + if line_breaks == 0 { + line.trim_left() + } else { + line + } + }) + .fold((true, opener.to_owned()), + |(first, mut acc), line| { + if !first { + acc.push('\n'); + acc.push_str(&indent_str); + acc.push_str(line_start); + } - if line.len() > max_chars { - acc.push_str(&rewrite_string(line, &fmt)); - } else { - if line.len() == 0 { - acc.pop(); // Remove space if this is an empty comment. - } else { - acc.push_str(line); - } - } + if line.len() > max_chars { + acc.push_str(&rewrite_string(line, &fmt)); + } else { + if line.len() == 0 { + acc.pop(); // Remove space if this is an empty comment. + } else { + acc.push_str(line); + } + } - (false, acc) - }); + (false, acc) + }); s.push_str(closer); diff --git a/src/expr.rs b/src/expr.rs index 98b93a5faa7..c7dad3d2ccf 100644 --- a/src/expr.rs +++ b/src/expr.rs @@ -18,7 +18,7 @@ use StructLitStyle; use utils::{span_after, make_indent, extra_offset, first_line_width, last_line_width, wrap_str, binary_search}; use visitor::FmtVisitor; -use config::{BlockIndentStyle, MultilineStyle}; +use config::MultilineStyle; use comment::{FindUncommented, rewrite_comment, contains_comment}; use types::rewrite_path; use items::{span_lo_for_arg, span_hi_for_arg, rewrite_fn_input}; @@ -40,12 +40,6 @@ impl Rewrite for ast::Expr { } } ast::Expr_::ExprCall(ref callee, ref args) => { - // FIXME using byte lens instead of char lens (and probably all over the place too) - // 2 is for parens - let max_callee_width = try_opt!(width.checked_sub(2)); - let callee_str = try_opt!(callee.rewrite(context, max_callee_width, offset)); - let span = mk_sp(callee.span.hi, self.span.hi); - rewrite_call(context, &**callee, args, self.span, width, offset) } ast::Expr_::ExprParen(ref subexpr) => { @@ -214,8 +208,6 @@ fn rewrite_closure(capture: ast::CaptureClause, prefix.push_str(&ret_str); } - let closure_indent = closure_indent(context, offset); - // Try to format closure body as a single line expression without braces. if is_simple_block(body, context.codemap) && !prefix.contains('\n') { let (spacer, closer) = if ret_str.is_empty() { @@ -246,17 +238,16 @@ fn rewrite_closure(capture: ast::CaptureClause, // We couldn't format the closure body as a single line expression; fall // back to block formatting. - let inner_context = context.overflow_context(closure_indent - context.block_indent); let body_rewrite = body.expr .as_ref() .and_then(|body_expr| { if let ast::Expr_::ExprBlock(ref inner) = body_expr.node { - Some(inner.rewrite(&inner_context, 2, 0)) + Some(inner.rewrite(&context, 2, 0)) } else { None } }) - .unwrap_or_else(|| body.rewrite(&inner_context, 2, 0)); + .unwrap_or_else(|| body.rewrite(&context, 2, 0)); Some(format!("{} {}", prefix, try_opt!(body_rewrite))) } @@ -876,25 +867,21 @@ fn rewrite_string_lit(context: &RewriteContext, } pub fn rewrite_call(context: &RewriteContext, - callee: &R, - args: &[ptr::P], - span: Span, - width: usize, - offset: usize) - -> Option + callee: &R, + args: &[ptr::P], + span: Span, + width: usize, + offset: usize) + -> Option where R: Rewrite { + let closure = |callee_max_width| { + rewrite_call_inner(context, callee, callee_max_width, args, span, width, offset) + }; + // 2 is for parens let max_width = try_opt!(width.checked_sub(2)); - binary_search(1, max_width, |callee_max_width| { - rewrite_call_inner(context, - callee, - callee_max_width, - args, - span, - width, - offset) - }) + binary_search(1, max_width, closure) } fn rewrite_call_inner(context: &RewriteContext, @@ -1021,10 +1008,9 @@ fn rewrite_struct_lit<'a>(context: &RewriteContext, match *item { StructLitField::Regular(ref field) => field.span.lo, StructLitField::Base(ref expr) => { - let last_field_hi = fields.last().map_or(span.lo, - |field| { - field.span.hi - }); + let last_field_hi = fields.last() + .map_or(span.lo, + |field| field.span.hi); let snippet = context.snippet(mk_sp(last_field_hi, expr.span.lo)); let pos = snippet.find_uncommented("..").unwrap(); @@ -1074,11 +1060,10 @@ fn rewrite_struct_lit<'a>(context: &RewriteContext, let fields_str = try_opt!(write_list(&items.collect::>(), &fmt)); let format_on_newline = || { - let inner_indent = make_indent(context.block_indent + - context.config.tab_spaces); - let outer_indent = make_indent(context.block_indent); - Some(format!("{} {{\n{}{}\n{}}}", path_str, inner_indent, fields_str, outer_indent)) - }; + let inner_indent = make_indent(context.block_indent + context.config.tab_spaces); + let outer_indent = make_indent(context.block_indent); + Some(format!("{} {{\n{}{}\n{}}}", path_str, inner_indent, fields_str, outer_indent)) + }; match (context.config.struct_lit_style, context.config.struct_lit_multiline_style) { (StructLitStyle::Block, _) if fields_str.contains('\n') => format_on_newline(), diff --git a/src/imports.rs b/src/imports.rs index 4b08a8b35e1..1d215d022d1 100644 --- a/src/imports.rs +++ b/src/imports.rs @@ -150,10 +150,10 @@ pub fn rewrite_use_list(width: usize, let list_str = try_opt!(write_list(&items[first_index..], &fmt)); Some(if path_str.is_empty() { - format!("{{{}}}", list_str) - } else { - format!("{}::{{{}}}", path_str, list_str) - }) + format!("{{{}}}", list_str) + } else { + format!("{}::{{{}}}", path_str, list_str) + }) } // Returns true when self item was found. diff --git a/src/items.rs b/src/items.rs index 2547e8f5dd5..958dbb20df0 100644 --- a/src/items.rs +++ b/src/items.rs @@ -211,10 +211,8 @@ impl<'a> FmtVisitor<'a> { let ret_str = fd.output.rewrite(&context, self.config.max_width - indent, indent).unwrap(); // Args. - let (one_line_budget, multi_line_budget, mut arg_indent) = self.compute_budgets_for_args(&result, - indent, - ret_str.len(), - newline_brace); + let (one_line_budget, multi_line_budget, mut arg_indent) = + self.compute_budgets_for_args(&result, indent, ret_str.len(), newline_brace); debug!("rewrite_fn: one_line_budget: {}, multi_line_budget: {}, arg_indent: {}", one_line_budget, multi_line_budget, arg_indent); @@ -239,7 +237,10 @@ impl<'a> FmtVisitor<'a> { } // A conservative estimation, to goal is to be over all parens in generics - let args_start = generics.ty_params.last().map(|tp| end_typaram(tp)).unwrap_or(span.lo); + let args_start = generics.ty_params + .last() + .map(|tp| end_typaram(tp)) + .unwrap_or(span.lo); let args_span = codemap::mk_sp(span_after(codemap::mk_sp(args_start, span.hi), "(", self.codemap), diff --git a/src/utils.rs b/src/utils.rs index cb1dd718375..973f0fa56ab 100644 --- a/src/utils.rs +++ b/src/utils.rs @@ -211,8 +211,7 @@ pub fn wrap_str>(s: S, max_width: usize, width: usize, offset: usi impl Rewrite for String { fn rewrite(&self, context: &RewriteContext, width: usize, offset: usize) -> Option { - // FIXME: unnecessary clone - wrap_str(self.clone(), context.config.max_width, width, offset) + wrap_str(self, context.config.max_width, width, offset).map(ToOwned::to_owned) } } @@ -245,13 +244,13 @@ pub fn binary_search(mut lo: usize, mut hi: usize, callback: C) -> Option< #[test] fn bin_search_test() { let closure = |i| { - match i { - 4 => Ok(()), - j if j > 4 => Err(Ordering::Less), - j if j < 4 => Err(Ordering::Greater), - _ => unreachable!(), - } - }; + match i { + 4 => Ok(()), + j if j > 4 => Err(Ordering::Less), + j if j < 4 => Err(Ordering::Greater), + _ => unreachable!(), + } + }; assert_eq!(Some(()), binary_search(1, 10, &closure)); assert_eq!(None, binary_search(1, 3, &closure)); diff --git a/tests/source/chains-no-overflow.rs b/tests/source/chains-no-overflow.rs index fa2c66524fc..b94ca21b258 100644 --- a/tests/source/chains-no-overflow.rs +++ b/tests/source/chains-no-overflow.rs @@ -21,9 +21,8 @@ fn main() { { SCRIPT_TASK_ROOT .with(|root| { - // Another case of write_list failing us. - *root.borrow_mut() = Some(&script_task); - }); + *root.borrow_mut() = Some(&script_task); + }); }); let suuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuum = xxxxxxx diff --git a/tests/source/chains.rs b/tests/source/chains.rs index d2f11f1dcbe..f53afaa0c28 100644 --- a/tests/source/chains.rs +++ b/tests/source/chains.rs @@ -1,7 +1,7 @@ // Test chain formatting. fn main() { - // Don't put chains on a single list if it wasn't so in source. + // Don't put chains on a single line if it wasn't so in source. let a = b .c .d.1 .foo(|x| x + 1); @@ -11,6 +11,8 @@ fn main() { bbbbbbbbbbbbbbbbbbb.ccccccccccccccccccccccccccccccccccccc.ddddddddddddddddddddddddddd.eeeeeeee(); + // Test case where first chain element isn't a path, but is shorter than + // the size of a tab. x() .y(|| match cond() { true => (), false => () }); diff --git a/tests/source/string-lit.rs b/tests/source/string-lit.rs index e95aaae75e0..35d8ec072c2 100644 --- a/tests/source/string-lit.rs +++ b/tests/source/string-lit.rs @@ -30,5 +30,5 @@ formatting"#; let unicode3 = "中华Việt Nam"; let unicode4 = "☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃"; - "stuff" + "stuffin'" } diff --git a/tests/system.rs b/tests/system.rs index d0b6ae78faa..9086697978e 100644 --- a/tests/system.rs +++ b/tests/system.rs @@ -26,9 +26,9 @@ use rustfmt::rustfmt_diff::*; static DIFF_CONTEXT_SIZE: usize = 3; fn get_path_string(dir_entry: io::Result) -> String { - let path = dir_entry.ok().expect("Couldn\'t get DirEntry.").path(); + let path = dir_entry.ok().expect("Couldn't get DirEntry.").path(); - path.to_str().expect("Couldn\'t stringify path.").to_owned() + path.to_str().expect("Couldn't stringify path.").to_owned() } // Integration tests. The files in the tests/source are formatted and compared @@ -40,7 +40,7 @@ fn get_path_string(dir_entry: io::Result) -> String { #[test] fn system_tests() { // Get all files in the tests/source directory - let files = fs::read_dir("tests/source").ok().expect("Couldn\'t read source dir."); + let files = fs::read_dir("tests/source").ok().expect("Couldn't read source dir."); // turn a DirEntry into a String that represents the relative path to the file let files = files.map(get_path_string); @@ -56,9 +56,9 @@ fn system_tests() { #[test] fn idempotence_tests() { // Get all files in the tests/target directory - let files = fs::read_dir("tests/target").ok().expect("Couldn\'t read target dir."); - let files = files.chain(fs::read_dir("tests").ok().expect("Couldn\'t read tests dir.")); - let files = files.chain(fs::read_dir("src/bin").ok().expect("Couldn\'t read src dir.")); + let files = fs::read_dir("tests/target").ok().expect("Couldn't read target dir."); + let files = files.chain(fs::read_dir("tests").ok().expect("Couldn't read tests dir.")); + let files = files.chain(fs::read_dir("src/bin").ok().expect("Couldn't read src dir.")); // turn a DirEntry into a String that represents the relative path to the file let files = files.map(get_path_string); // hack because there's no `IntoIterator` impl for `[T; N]` @@ -139,9 +139,9 @@ fn get_config(config_file: Option<&str>) -> Box { let mut def_config_file = fs::File::open(config_file_name) .ok() - .expect("Couldn\'t open config."); + .expect("Couldn't open config."); let mut def_config = String::new(); - def_config_file.read_to_string(&mut def_config).ok().expect("Couldn\'t read config."); + def_config_file.read_to_string(&mut def_config).ok().expect("Couldn't read config."); Box::new(Config::from_toml(&def_config)) } @@ -151,7 +151,7 @@ fn get_config(config_file: Option<&str>) -> Box { fn read_significant_comments(file_name: &str) -> HashMap { let file = fs::File::open(file_name) .ok() - .expect(&format!("Couldn\'t read file {}.", file_name)); + .expect(&format!("Couldn't read file {}.", file_name)); let reader = BufReader::new(file); let pattern = r"^\s*//\s*rustfmt-([^:]+):\s*(\S+)"; let regex = regex::Regex::new(&pattern).ok().expect("Failed creating pattern 1."); @@ -166,8 +166,8 @@ fn read_significant_comments(file_name: &str) -> HashMap { .take_while(|line| line_regex.is_match(&line)) .filter_map(|line| { regex.captures_iter(&line).next().map(|capture| { - (capture.at(1).expect("Couldn\'t unwrap capture.").to_owned(), - capture.at(2).expect("Couldn\'t unwrap capture.").to_owned()) + (capture.at(1).expect("Couldn't unwrap capture.").to_owned(), + capture.at(2).expect("Couldn't unwrap capture.").to_owned()) }) }) .collect() @@ -185,7 +185,7 @@ fn handle_result(result: HashMap) { // If file is in tests/source, compare to file with same name in tests/target. let target = get_target(&file_name, sig_comments.get("target").map(|x| &(*x)[..])); - let mut f = fs::File::open(&target).ok().expect("Couldn\'t open target."); + let mut f = fs::File::open(&target).ok().expect("Couldn't open target."); let mut text = String::new(); // TODO: speedup by running through bytes iterator diff --git a/tests/target/chains-no-overflow.rs b/tests/target/chains-no-overflow.rs index b4fdb9f99ce..9b1f244637d 100644 --- a/tests/target/chains-no-overflow.rs +++ b/tests/target/chains-no-overflow.rs @@ -27,8 +27,8 @@ fn main() { fffffffffffffffffffffffffffffffffff(a, { SCRIPT_TASK_ROOT.with(|root| { - // Another case of write_list failing us. - *root.borrow_mut() = Some(&script_task); + *root.borrow_mut() = + Some(&script_task); }); }); diff --git a/tests/target/chains.rs b/tests/target/chains.rs index e49233e22a2..590f0a5b156 100644 --- a/tests/target/chains.rs +++ b/tests/target/chains.rs @@ -1,7 +1,7 @@ // Test chain formatting. fn main() { - // Don't put chains on a single list if it wasn't so in source. + // Don't put chains on a single line if it wasn't so in source. let a = b.c .d .1 @@ -13,6 +13,8 @@ fn main() { .ddddddddddddddddddddddddddd .eeeeeeee(); + // Test case where first chain element isn't a path, but is shorter than + // the size of a tab. x().y(|| { match cond() { true => (), diff --git a/tests/target/closure.rs b/tests/target/closure.rs index ec095ad8fcc..b42296487be 100644 --- a/tests/target/closure.rs +++ b/tests/target/closure.rs @@ -39,8 +39,8 @@ fn main() { let empty = |arg| {}; let simple = |arg| { /* TODO(#27): comment formatting */ - foo(arg) - }; + foo(arg) + }; let test = || { do_something(); @@ -62,8 +62,8 @@ fn main() { let closure_with_return_type = |aaaaaaaaaaaaaaaaaaaaaaarg1, aaaaaaaaaaaaaaaaaaaaaaarg2| -> Strong { - "sup".to_owned() - }; + "sup".to_owned() + }; |arg1, arg2, _, _, arg3, arg4| { let temp = arg4 + arg3; diff --git a/tests/target/string-lit.rs b/tests/target/string-lit.rs index 21cdc199d6e..36f8f6a48f9 100644 --- a/tests/target/string-lit.rs +++ b/tests/target/string-lit.rs @@ -36,5 +36,5 @@ formatting"#; let unicode4 = "☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃\ ☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃☃"; - "stuff" + "stuffin'" }