From b81b9028215dbb89cd8e16d783ccd146e1d4c162 Mon Sep 17 00:00:00 2001 From: Nick Cameron Date: Mon, 15 Oct 2018 11:48:12 +1300 Subject: [PATCH 1/3] Simplify situations in which the last sub-expr in a bin-op can go multiline without making the whole expr multiline Fixes #3034 --- src/pairs.rs | 39 +++++++++++++-------------------------- 1 file changed, 13 insertions(+), 26 deletions(-) diff --git a/src/pairs.rs b/src/pairs.rs index f078022b9e07..c4451a8e4660 100644 --- a/src/pairs.rs +++ b/src/pairs.rs @@ -43,7 +43,7 @@ pub(crate) fn rewrite_all_pairs( context: &RewriteContext, ) -> Option { // First we try formatting on one line. - if let Some(list) = expr.flatten(context, false) { + if let Some(list) = expr.flatten(false) { if let Some(r) = rewrite_pairs_one_line(&list, shape, context) { return Some(r); } @@ -53,7 +53,7 @@ pub(crate) fn rewrite_all_pairs( // to only flatten pairs with the same operator, that way we don't // necessarily need one line per sub-expression, but we don't do anything // too funny wrt precedence. - expr.flatten(context, true) + expr.flatten(true) .and_then(|list| rewrite_pairs_multiline(list, shape, context)) } @@ -83,33 +83,22 @@ fn rewrite_pairs_one_line( result.push(' '); } + let prefix_len = result.len(); let last = list.list.last().unwrap(); let cur_shape = base_shape.offset_left(last_line_width(&result))?; - let rewrite = last.rewrite(context, cur_shape)?; - result.push_str(&rewrite); + let last_rewrite = last.rewrite(context, cur_shape)?; + result.push_str(&last_rewrite); if first_line_width(&result) > shape.width { return None; } - // Check the last expression in the list. We let this expression go over - // multiple lines, but we check that if this is necessary, then we can't - // do better using multi-line formatting. - if !is_single_line(&result) { - let multiline_shape = shape.offset_left(list.separators.last().unwrap().len() + 1)?; - let multiline_list: PairList = PairList { - list: vec![last], - separators: vec![], - separator_place: list.separator_place, - }; - // Format as if we were multi-line. - if let Some(rewrite) = rewrite_pairs_multiline(multiline_list, multiline_shape, context) { - // Also, don't let expressions surrounded by parens go multi-line, - // this looks really bad. - if rewrite.starts_with('(') || is_single_line(&rewrite) { - return None; - } - } + // Check the last expression in the list. We sometimes let this expression + // go over multiple lines, but we check for some ugly conditions. + if !(is_single_line(&result) || last_rewrite.starts_with('{')) + && (last_rewrite.starts_with('(') || prefix_len > context.config.tab_spaces()) + { + return None; } wrap_str(result, context.config.max_width(), shape) @@ -272,7 +261,7 @@ trait FlattenPair: Rewrite + Sized { // operator into the list. E.g,, if the source is `a * b + c`, if `_same_op` // is true, we make `[(a * b), c]` if `_same_op` is false, we make // `[a, b, c]` - fn flatten(&self, _context: &RewriteContext, _same_op: bool) -> Option> { + fn flatten(&self, _same_op: bool) -> Option> { None } } @@ -280,11 +269,10 @@ trait FlattenPair: Rewrite + Sized { struct PairList<'a, 'b, T: Rewrite + 'b> { list: Vec<&'b T>, separators: Vec<&'a str>, - separator_place: SeparatorPlace, } impl FlattenPair for ast::Expr { - fn flatten(&self, context: &RewriteContext, same_op: bool) -> Option> { + fn flatten(&self, same_op: bool) -> Option> { let top_op = match self.node { ast::ExprKind::Binary(op, _, _) => op.node, _ => return None, @@ -323,7 +311,6 @@ impl FlattenPair for ast::Expr { Some(PairList { list, separators, - separator_place: context.config.binop_separator(), }) } } From cbd568083d87a90dfe5ab0e90f404454946c9f20 Mon Sep 17 00:00:00 2001 From: Nick Cameron Date: Mon, 15 Oct 2018 11:52:27 +1300 Subject: [PATCH 2/3] Fixup formatting --- src/expr.rs | 9 +++++---- src/lists.rs | 10 ++++++---- src/macros.rs | 34 ++++++++++++++++++---------------- src/pairs.rs | 16 +++++++--------- src/string.rs | 11 ++++++----- src/utils.rs | 9 +++++---- 6 files changed, 47 insertions(+), 42 deletions(-) diff --git a/src/expr.rs b/src/expr.rs index 8a5b3de7a147..47777e4c5c48 100644 --- a/src/expr.rs +++ b/src/expr.rs @@ -898,10 +898,11 @@ impl<'a> ControlFlow<'a> { let trial = self.rewrite_single_line(&pat_expr_string, context, shape.width); if let Some(cond_str) = trial { - if cond_str.len() <= context - .config - .width_heuristics() - .single_line_if_else_max_width + if cond_str.len() + <= context + .config + .width_heuristics() + .single_line_if_else_max_width { return Some((cond_str, 0)); } diff --git a/src/lists.rs b/src/lists.rs index 57f778896d30..6005ae6fd259 100644 --- a/src/lists.rs +++ b/src/lists.rs @@ -146,10 +146,12 @@ impl ListItem { } pub fn is_different_group(&self) -> bool { - self.inner_as_ref().contains('\n') || self.pre_comment.is_some() || self - .post_comment - .as_ref() - .map_or(false, |s| s.contains('\n')) + self.inner_as_ref().contains('\n') + || self.pre_comment.is_some() + || self + .post_comment + .as_ref() + .map_or(false, |s| s.contains('\n')) } pub fn is_multiline(&self) -> bool { diff --git a/src/macros.rs b/src/macros.rs index dab27d8a073b..49f4a03ee1db 100644 --- a/src/macros.rs +++ b/src/macros.rs @@ -1168,22 +1168,24 @@ fn indent_macro_snippet( .min()?; Some( - first_line + "\n" + &trimmed_lines - .iter() - .map( - |&(trimmed, ref line, prefix_space_width)| match prefix_space_width { - _ if !trimmed => line.to_owned(), - Some(original_indent_width) => { - let new_indent_width = indent.width() + original_indent_width - .saturating_sub(min_prefix_space_width); - let new_indent = Indent::from_width(context.config, new_indent_width); - format!("{}{}", new_indent.to_string(context.config), line) - } - None => String::new(), - }, - ) - .collect::>() - .join("\n"), + first_line + + "\n" + + &trimmed_lines + .iter() + .map( + |&(trimmed, ref line, prefix_space_width)| match prefix_space_width { + _ if !trimmed => line.to_owned(), + Some(original_indent_width) => { + let new_indent_width = indent.width() + + original_indent_width.saturating_sub(min_prefix_space_width); + let new_indent = Indent::from_width(context.config, new_indent_width); + format!("{}{}", new_indent.to_string(context.config), line) + } + None => String::new(), + }, + ) + .collect::>() + .join("\n"), ) } diff --git a/src/pairs.rs b/src/pairs.rs index c4451a8e4660..79ae3081f1f8 100644 --- a/src/pairs.rs +++ b/src/pairs.rs @@ -204,11 +204,12 @@ where // If the length of the lhs is equal to or shorter than the tab width or // the rhs looks like block expression, we put the rhs on the same // line with the lhs even if the rhs is multi-lined. - let allow_same_line = lhs_result.len() <= tab_spaces || rhs_result - .lines() - .next() - .map(|first_line| first_line.ends_with('{')) - .unwrap_or(false); + let allow_same_line = lhs_result.len() <= tab_spaces + || rhs_result + .lines() + .next() + .map(|first_line| first_line.ends_with('{')) + .unwrap_or(false); if !rhs_result.contains('\n') || allow_same_line { let one_line_width = last_line_width(&lhs_result) + pp.infix.len() @@ -308,10 +309,7 @@ impl FlattenPair for ast::Expr { } assert_eq!(list.len() - 1, separators.len()); - Some(PairList { - list, - separators, - }) + Some(PairList { list, separators }) } } diff --git a/src/string.rs b/src/string.rs index 2fe13db2139c..b3f6daadfac6 100644 --- a/src/string.rs +++ b/src/string.rs @@ -287,11 +287,12 @@ fn break_string(max_chars: usize, trim_end: bool, line_end: &str, input: &[&str] return break_at(max_chars - 1); } if let Some(url_index_end) = detect_url(input, max_chars) { - let index_plus_ws = url_index_end + input[url_index_end..] - .iter() - .skip(1) - .position(|grapheme| not_whitespace_except_line_feed(grapheme)) - .unwrap_or(0); + let index_plus_ws = url_index_end + + input[url_index_end..] + .iter() + .skip(1) + .position(|grapheme| not_whitespace_except_line_feed(grapheme)) + .unwrap_or(0); return if trim_end { SnippetState::LineEnd( input[..=url_index_end].join("").to_string(), diff --git a/src/utils.rs b/src/utils.rs index 63b238a78176..421ab38e3f83 100644 --- a/src/utils.rs +++ b/src/utils.rs @@ -324,10 +324,11 @@ pub fn mk_sp(lo: BytePos, hi: BytePos) -> Span { // Return true if the given span does not intersect with file lines. macro_rules! out_of_file_lines_range { ($self:ident, $span:expr) => { - !$self.config.file_lines().is_all() && !$self - .config - .file_lines() - .intersects(&$self.source_map.lookup_line_range($span)) + !$self.config.file_lines().is_all() + && !$self + .config + .file_lines() + .intersects(&$self.source_map.lookup_line_range($span)) }; } From 7be173eb8c9e043fbe814970c93dc7d98ddc8009 Mon Sep 17 00:00:00 2001 From: Nick Cameron Date: Mon, 15 Oct 2018 12:09:53 +1300 Subject: [PATCH 3/3] add test --- tests/source/chains.rs | 5 +++++ tests/target/chains.rs | 5 +++++ 2 files changed, 10 insertions(+) diff --git a/tests/source/chains.rs b/tests/source/chains.rs index 52a077e435f5..c77f5bac4cb2 100644 --- a/tests/source/chains.rs +++ b/tests/source/chains.rs @@ -259,3 +259,8 @@ fn issue_2773() { None }); } + +fn issue_3034() { + disallowed_headers.iter().any(|header| *header == name) || + disallowed_header_prefixes.iter().any(|prefix| name.starts_with(prefix)) +} diff --git a/tests/target/chains.rs b/tests/target/chains.rs index 2945f836a6b1..292da2981954 100644 --- a/tests/target/chains.rs +++ b/tests/target/chains.rs @@ -299,3 +299,8 @@ fn issue_2773() { None }); } + +fn issue_3034() { + disallowed_headers.iter().any(|header| *header == name) + || disallowed_header_prefixes.iter().any(|prefix| name.starts_with(prefix)) +}