From 078fff068a3d0e5401bd46214c112cb9276a6240 Mon Sep 17 00:00:00 2001 From: Marcus Klaas Date: Sat, 26 Sep 2015 18:16:07 +0200 Subject: Improve heuristics for match arm body placement --- src/expr.rs | 68 ++++++++++++++++++++++++++++++++++++++++++------------------ src/items.rs | 4 ++-- 2 files changed, 50 insertions(+), 22 deletions(-) (limited to 'src') diff --git a/src/expr.rs b/src/expr.rs index 79e03e644e4..29966d09da9 100644 --- a/src/expr.rs +++ b/src/expr.rs @@ -15,7 +15,7 @@ use Indent; use rewrite::{Rewrite, RewriteContext}; use lists::{write_list, itemize_list, ListFormatting, SeparatorTactic, ListTactic}; use string::{StringFormat, rewrite_string}; -use utils::{span_after, extra_offset, first_line_width, last_line_width, wrap_str, binary_search}; +use utils::{span_after, extra_offset, last_line_width, wrap_str, binary_search}; use visitor::FmtVisitor; use config::{StructLitStyle, MultilineStyle}; use comment::{FindUncommented, rewrite_comment, contains_comment}; @@ -352,9 +352,9 @@ fn rewrite_closure(capture: ast::CaptureClause, Some(format!("{} {}", prefix, try_opt!(body_rewrite))) } -fn nop_block_collapse(block_str: Option) -> Option { +fn nop_block_collapse(block_str: Option, budget: usize) -> Option { block_str.map(|block_str| { - if block_str.starts_with("{") && + if block_str.starts_with("{") && budget >= 2 && (block_str[1..].find(|c: char| !c.is_whitespace()).unwrap() == block_str.len() - 2) { "{}".to_owned() } else { @@ -889,36 +889,64 @@ 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 same_line_body = if context.config.max_width > line_start + comma.len() + 4 { let budget = context.config.max_width - line_start - comma.len() - 4; - if let Some(ref body_str) = nop_block_collapse(body.rewrite(context, - budget, - line_indent + 4)) { - if first_line_width(body_str) <= budget { + let rewrite = nop_block_collapse(body.rewrite(context, budget, line_indent + 4), + budget); + + match rewrite { + Some(ref body_str) if body_str.len() <= budget || comma.is_empty() => return Some(format!("{}{} => {}{}", attr_str.trim_left(), pats_str, body_str, - comma)); - } + comma)), + _ => rewrite, } - } + } else { + None + }; // We have to push the body to the next line. - if comma.is_empty() { + if let ast::ExprBlock(_) = body.node { // We're trying to fit a block in, but it still failed, give up. return None; } let body_budget = try_opt!(width.checked_sub(context.config.tab_spaces)); - let body_str = try_opt!(nop_block_collapse(body.rewrite(context, - body_budget, - context.block_indent))); - Some(format!("{}{} =>\n{}{},", - attr_str.trim_left(), - pats_str, - offset.block_indent(context.config).to_string(context.config), - body_str)) + let next_line_body = nop_block_collapse(body.rewrite(context, + body_budget, + context.block_indent + .block_indent(context.config)), + body_budget); + + let (body_str, break_line) = try_opt!(match_arm_heuristic(same_line_body.as_ref() + .map(|x| &x[..]), + next_line_body.as_ref() + .map(|x| &x[..]))); + + let spacer = if break_line { + format!("\n{}", offset.block_indent(context.config).to_string(context.config)) + } else { + " ".to_owned() + }; + + Some(format!("{}{} =>{}{},", attr_str.trim_left(), pats_str, spacer, body_str)) + } +} + +// Takes two possible rewrites for the match arm body and chooses the "nicest". +// Bool marks break line or no. +fn match_arm_heuristic<'a>(former: Option<&'a str>, + latter: Option<&'a str>) + -> Option<(&'a str, bool)> { + match (former, latter) { + (Some(f), None) => Some((f, false)), + (Some(f), Some(l)) if f.chars().filter(|&c| c == '\n').count() <= + l.chars().filter(|&c| c == '\n').count() => { + Some((f, false)) + } + (_, l) => l.map(|s| (s, true)), } } diff --git a/src/items.rs b/src/items.rs index efd2c852c3e..ec79040f59c 100644 --- a/src/items.rs +++ b/src/items.rs @@ -969,8 +969,8 @@ impl<'a> FmtVisitor<'a> { let extra_indent = match self.config.where_indent { BlockIndentStyle::Inherit => Indent::empty(), - BlockIndentStyle::Tabbed | BlockIndentStyle::Visual => Indent::new(config.tab_spaces, - 0), + BlockIndentStyle::Tabbed | BlockIndentStyle::Visual => + Indent::new(config.tab_spaces, 0), }; let context = self.get_context(); -- cgit 1.4.1-3-g733a5 From 2d4a0cbe3b7a4c8f94bb1d422993ff856d714dae Mon Sep 17 00:00:00 2001 From: Marcus Klaas Date: Sat, 26 Sep 2015 23:16:11 +0200 Subject: Fix match arm indentation bug --- src/expr.rs | 39 +++++++++++++++++++++------------------ src/lib.rs | 4 ++-- tests/target/match.rs | 17 ++++++++++++----- 3 files changed, 35 insertions(+), 25 deletions(-) (limited to 'src') diff --git a/src/expr.rs b/src/expr.rs index 29966d09da9..b8a52a02cc7 100644 --- a/src/expr.rs +++ b/src/expr.rs @@ -155,11 +155,14 @@ impl Rewrite for ast::Expr { rewrite_chain(self, context, width, offset) } ast::Expr_::ExprMac(ref mac) => { - // Failure to rewrite a marco should not imply failure to rewrite the Expr - rewrite_macro(mac, context, width, offset).or(wrap_str(context.snippet(self.span), - context.config.max_width, - width, - offset)) + // Failure to rewrite a marco should not imply failure to + // rewrite the expression. + rewrite_macro(mac, context, width, offset).or_else(|| { + wrap_str(context.snippet(self.span), + context.config.max_width, + width, + offset) + }) } ast::Expr_::ExprRet(None) => { wrap_str("return".to_owned(), @@ -168,10 +171,10 @@ impl Rewrite for ast::Expr { offset) } ast::Expr_::ExprRet(Some(ref expr)) => { - rewrite_unary_prefix(context, "return ", &expr, width, offset) + rewrite_unary_prefix(context, "return ", expr, width, offset) } ast::Expr_::ExprBox(ref expr) => { - rewrite_unary_prefix(context, "box ", &expr, width, offset) + rewrite_unary_prefix(context, "box ", expr, width, offset) } ast::Expr_::ExprAddrOf(mutability, ref expr) => { rewrite_expr_addrof(context, mutability, &expr, width, offset) @@ -872,15 +875,10 @@ impl Rewrite for ast::Arm { let pats_str = format!("{}{}", pats_str, guard_str); // Where the next text can start. let mut line_start = last_line_width(&pats_str); - if pats_str.find('\n').is_none() { + if !pats_str.contains('\n') { line_start += offset.width(); } - let mut line_indent = offset + pats_width; - if vertical { - line_indent = line_indent.block_indent(context.config); - } - let comma = if let ast::ExprBlock(_) = body.node { "" } else { @@ -891,8 +889,9 @@ impl Rewrite for ast::Arm { // 4 = ` => `.len() let same_line_body = if context.config.max_width > line_start + comma.len() + 4 { let budget = context.config.max_width - line_start - comma.len() - 4; - let rewrite = nop_block_collapse(body.rewrite(context, budget, line_indent + 4), - budget); + let offset = Indent::new(offset.block_indent, + line_start + 4 - offset.block_indent); + let rewrite = nop_block_collapse(body.rewrite(context, budget, offset), budget); match rewrite { Some(ref body_str) if body_str.len() <= budget || comma.is_empty() => @@ -907,7 +906,6 @@ impl Rewrite for ast::Arm { None }; - // We have to push the body to the next line. if let ast::ExprBlock(_) = body.node { // We're trying to fit a block in, but it still failed, give up. return None; @@ -926,12 +924,17 @@ impl Rewrite for ast::Arm { .map(|x| &x[..]))); let spacer = if break_line { - format!("\n{}", offset.block_indent(context.config).to_string(context.config)) + format!("\n{}", + offset.block_indent(context.config).to_string(context.config)) } else { " ".to_owned() }; - Some(format!("{}{} =>{}{},", attr_str.trim_left(), pats_str, spacer, body_str)) + Some(format!("{}{} =>{}{},", + attr_str.trim_left(), + pats_str, + spacer, + body_str)) } } diff --git a/src/lib.rs b/src/lib.rs index 73184a0bdc1..b483cae60d4 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -85,9 +85,9 @@ const SKIP_ANNOTATION: &'static str = "rustfmt_skip"; pub struct Indent { // Width of the block indent, in characters. Must be a multiple of // Config::tab_spaces. - block_indent: usize, + pub block_indent: usize, // Alignment in characters. - alignment: usize, + pub alignment: usize, } impl Indent { diff --git a/tests/target/match.rs b/tests/target/match.rs index 3a8bee35aac..d7227a0da59 100644 --- a/tests/target/match.rs +++ b/tests/target/match.rs @@ -65,12 +65,19 @@ fn main() { // Test that one-line bodies align. fn main() { match r { - Variableeeeeeeeeeeeeeeeee => - ("variable", vec!("id", "name", "qualname", "value", "type", "scopeid"), true, true), - Enummmmmmmmmmmmmmmmmmmmm => - ("enum", vec!("id", "qualname", "scopeid", "value"), true, true), + Variableeeeeeeeeeeeeeeeee => ("variable", + vec!("id", "name", "qualname", "value", "type", "scopeid"), + true, + true), + Enummmmmmmmmmmmmmmmmmmmm => ("enum", + vec!("id", "qualname", "scopeid", "value"), + true, + true), Variantttttttttttttttttttttttt => - ("variant", vec!("id", "name", "qualname", "type", "value", "scopeid"), true, true), + ("variant", + vec!("id", "name", "qualname", "type", "value", "scopeid"), + true, + true), } } -- cgit 1.4.1-3-g733a5 From 2eb67827a7a34c9bb221710763f6d16fb4555a24 Mon Sep 17 00:00:00 2001 From: Marcus Klaas Date: Sun, 27 Sep 2015 11:58:26 +0200 Subject: Add extra tests for match arm placement --- src/expr.rs | 28 +++++++++++----------------- tests/source/match.rs | 12 ++++++++++++ tests/target/match.rs | 11 +++++++++++ 3 files changed, 34 insertions(+), 17 deletions(-) (limited to 'src') diff --git a/src/expr.rs b/src/expr.rs index b8a52a02cc7..38305fc7d62 100644 --- a/src/expr.rs +++ b/src/expr.rs @@ -918,16 +918,13 @@ impl Rewrite for ast::Arm { .block_indent(context.config)), body_budget); - let (body_str, break_line) = try_opt!(match_arm_heuristic(same_line_body.as_ref() - .map(|x| &x[..]), - next_line_body.as_ref() - .map(|x| &x[..]))); - - let spacer = if break_line { - format!("\n{}", - offset.block_indent(context.config).to_string(context.config)) - } else { - " ".to_owned() + let body_str = try_opt!(match_arm_heuristic(same_line_body.as_ref().map(|x| &x[..]), + next_line_body.as_ref().map(|x| &x[..]))); + + let spacer = match same_line_body { + Some(ref body) if body == body_str => " ".to_owned(), + _ => format!("\n{}", + offset.block_indent(context.config).to_string(context.config)), }; Some(format!("{}{} =>{}{},", @@ -939,17 +936,14 @@ impl Rewrite for ast::Arm { } // Takes two possible rewrites for the match arm body and chooses the "nicest". -// Bool marks break line or no. -fn match_arm_heuristic<'a>(former: Option<&'a str>, - latter: Option<&'a str>) - -> Option<(&'a str, bool)> { +fn match_arm_heuristic<'a>(former: Option<&'a str>, latter: Option<&'a str>) -> Option<&'a str> { match (former, latter) { - (Some(f), None) => Some((f, false)), + (f @ Some(..), None) => f, (Some(f), Some(l)) if f.chars().filter(|&c| c == '\n').count() <= l.chars().filter(|&c| c == '\n').count() => { - Some((f, false)) + Some(f) } - (_, l) => l.map(|s| (s, true)), + (_, l) => l, } } diff --git a/tests/source/match.rs b/tests/source/match.rs index 9fc32ef1904..49e66e4c0df 100644 --- a/tests/source/match.rs +++ b/tests/source/match.rs @@ -209,3 +209,15 @@ fn issue355() { dddddddddd), } } + +fn issue280() { + { + match x { + CompressionMode::DiscardNewline | CompressionMode::CompressWhitespaceNewline => ch == + '\n', + ast::ItemConst(ref typ, ref expr) => self.process_static_or_const_item(item, + &typ, + &expr), + } + } +} diff --git a/tests/target/match.rs b/tests/target/match.rs index d7227a0da59..6e330505a9b 100644 --- a/tests/target/match.rs +++ b/tests/target/match.rs @@ -199,3 +199,14 @@ fn issue355() { dddddddddd), } } + +fn issue280() { + { + match x { + CompressionMode::DiscardNewline | CompressionMode::CompressWhitespaceNewline => + ch == '\n', + ast::ItemConst(ref typ, ref expr) => + self.process_static_or_const_item(item, &typ, &expr), + } + } +} -- cgit 1.4.1-3-g733a5