Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Improve heuristics for match arm body placement #370

Merged
merged 3 commits into from
Sep 27, 2015
Merged

Improve heuristics for match arm body placement #370

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
078fff0
Improve heuristics for match arm body placement
marcusklaas Sep 26, 2015
2d4a0cb
Fix match arm indentation bug
marcusklaas Sep 26, 2015
2eb6782
Add extra tests for match arm placement
marcusklaas Sep 27, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
93 changes: 62 additions & 31 deletions src/expr.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand Down Expand Up @@ -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(),
Expand All @@ -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)
Expand Down Expand Up @@ -352,9 +355,9 @@ fn rewrite_closure(capture: ast::CaptureClause,
Some(format!("{} {}", prefix, try_opt!(body_rewrite)))
}

fn nop_block_collapse(block_str: Option<String>) -> Option<String> {
fn nop_block_collapse(block_str: Option<String>, budget: usize) -> Option<String> {
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 {
Expand Down Expand Up @@ -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 {
Expand All @@ -889,39 +887,72 @@ 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 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() =>
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{}{},",
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,
offset.block_indent(context.config).to_string(context.config),
spacer,
body_str))
}
}

// Takes two possible rewrites for the match arm body and chooses the "nicest".
// Bool marks break line or no.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The bool seems only to indicate if we took the second option, rather than actually checking for line breaks. It would be nice if we didn't need it tbh.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a very vague comment. What I meant to say is that it indicates whether the result should be placed on a new line.

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)),
}
}

// The `if ...` guard on a match arm.
fn rewrite_guard(context: &RewriteContext,
guard: &Option<ptr::P<ast::Expr>>,
Expand Down
4 changes: 2 additions & 2 deletions src/items.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

\o/

};

let context = self.get_context();
Expand Down
4 changes: 2 additions & 2 deletions src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
119 changes: 0 additions & 119 deletions tests/source/expr.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,125 +130,6 @@ fn issue184(source: &str) {
}
}

fn matches() {
match 1 {
1 => 1, // foo
2 => 2,
// bar
3 => 3,
_ => 0 // baz
}
}

fn issue339() {
match a {
b => {}
c => { }
d => {
}
e => {



}
// collapsing here is safe
ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff => {
}
// collapsing here exceeds line length
ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffg => {
}
h => { // comment above block
}
i => {
} // comment below block
j => {
// comment inside block
}
j2 => {
// comments inside...
} // ... and after
// TODO uncomment when vertical whitespace is handled better
// k => {
//
// // comment with WS above
// }
// l => {
// // comment with ws below
//
// }
m => {
} n => { } o =>
{

}
p => { // Dont collapse me
} q => { } r =>
{

}
s => 0, // s comment
// t comment
t => 1,
u => 2,
// TODO uncomment when block-support exists
// v => {
// } /* funky block
// * comment */
// final comment
}
}

fn issue355() {
match mac {
a => println!("a", b),
b => vec!(1, 2),
c => vec!(3; 4),
d => {
println!("a", b)
}
e => {
vec!(1, 2)
}
f => {
vec!(3; 4)
}
h => println!("a", b), // h comment
i => vec!(1, 2), // i comment
j => vec!(3; 4), // j comment
// k comment
k => println!("a", b),
// l comment
l => vec!(1, 2),
// m comment
m => vec!(3; 4),
// Rewrite splits macro
nnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnn => println!("a", b),
// Rewrite splits macro
oooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooo => vec!(1, 2),
// Macro support fails to recognise this macro as splitable
// We push the whole expr to a new line, TODO split this macro as well
pppppppppppppppppppppppppppppppppppppppppppppppppppppppppppppppppppppppppppppp => vec!(3; 4),
// q, r and s: Rewrite splits match arm
qqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqq => println!("a", b),
rrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrr => vec!(1, 2),
ssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssss => vec!(3; 4),
// Funky bracketing styles
t => println!{"a", b},
u => vec!{1, 2},
v => vec!{3; 4},
w => println!["a", b],
x => vec![1, 2],
y =>vec![3; 4],
// Brackets with comments
tc => println!{"a", b}, // comment
uc => vec!{1, 2}, // comment
vc =>vec!{3; 4}, // comment
wc =>println!["a", b], // comment
xc => vec![1,2], // comment
yc => vec![3; 4], // comment
}
}

fn arrays() {
let x = [0,
1,
Expand Down
Loading