mikros/rust
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Merge pull request #1688 from topecongiro/overflow-closure

Browse Source 
Wrap closure in block with a single control flow expr with multi line condition
This commit is contained in:
Nick Cameron 2017-06-16 13:05:57 +12:00 committed by GitHub
commit 3c36cfbeea
3 changed files with 312 additions and 151 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

437
src/expr.rs
View File

@ -151,41 +151,17 @@ fn format_expr(
shape,
)
}
ast::ExprKind::While(ref cond, ref block, label) => {
ControlFlow::new_while(None, cond, block, label, expr.span).rewrite(context, shape)
}
ast::ExprKind::WhileLet(ref pat, ref cond, ref block, label) => {
ControlFlow::new_while(Some(pat), cond, block, label, expr.span).rewrite(context, shape)
}
ast::ExprKind::ForLoop(ref pat, ref cond, ref block, label) => {
ControlFlow::new_for(pat, cond, block, label, expr.span).rewrite(context, shape)
}
ast::ExprKind::Loop(ref block, label) => {
ControlFlow::new_loop(block, label, expr.span).rewrite(context, shape)
ast::ExprKind::If(..) |
ast::ExprKind::IfLet(..) |
ast::ExprKind::ForLoop(..) |
ast::ExprKind::Loop(..) |
ast::ExprKind::While(..) |
ast::ExprKind::WhileLet(..) => {
to_control_flow(expr, expr_type).and_then(|control_flow| {
control_flow.rewrite(context, shape)
})
}
ast::ExprKind::Block(ref block) => block.rewrite(context, shape),
ast::ExprKind::If(ref cond, ref if_block, ref else_block) => {
ControlFlow::new_if(
cond,
None,
if_block,
else_block.as_ref().map(|e| &**e),
expr_type == ExprType::SubExpression,
false,
expr.span,
).rewrite(context, shape)
}
ast::ExprKind::IfLet(ref pat, ref cond, ref if_block, ref else_block) => {
ControlFlow::new_if(
cond,
Some(pat),
if_block,
else_block.as_ref().map(|e| &**e),
expr_type == ExprType::SubExpression,
false,
expr.span,
).rewrite(context, shape)
}
ast::ExprKind::Match(ref cond, ref arms) => {
rewrite_match(context, cond, arms, shape, expr.span)
}
@ -559,23 +535,15 @@ where
Some(result)
}
// This functions is pretty messy because of the rules around closures and blocks:
// FIXME - the below is probably no longer true in full.
// * if there is a return type, then there must be braces,
// * given a closure with braces, whether that is parsed to give an inner block
// or not depends on if there is a return type and if there are statements
// in that block,
// * if the first expression in the body ends with a block (i.e., is a
// statement without needing a semi-colon), then adding or removing braces
// can change whether it is treated as an expression or statement.
fn rewrite_closure(
// Return type is (prefix, extra_offset)
fn rewrite_closure_fn_decl(
capture: ast::CaptureBy,
fn_decl: &ast::FnDecl,
body: &ast::Expr,
span: Span,
context: &RewriteContext,
shape: Shape,
) -> Option<String> {
) -> Option<(String, usize)> {
let mover = if capture == ast::CaptureBy::Value {
"move "
} else {
@ -622,6 +590,8 @@ fn rewrite_closure(
};
let list_str = try_opt!(write_list(&item_vec, &fmt));
let mut prefix = format!("{}|{}|", mover, list_str);
// 1 = space between `|...|` and body.
let extra_offset = extra_offset(&prefix, shape) + 1;
if !ret_str.is_empty() {
if prefix.contains('\n') {
@ -633,8 +603,35 @@ fn rewrite_closure(
prefix.push_str(&ret_str);
}
Some((prefix, extra_offset))
}
// This functions is pretty messy because of the rules around closures and blocks:
// FIXME - the below is probably no longer true in full.
// * if there is a return type, then there must be braces,
// * given a closure with braces, whether that is parsed to give an inner block
// or not depends on if there is a return type and if there are statements
// in that block,
// * if the first expression in the body ends with a block (i.e., is a
// statement without needing a semi-colon), then adding or removing braces
// can change whether it is treated as an expression or statement.
fn rewrite_closure(
capture: ast::CaptureBy,
fn_decl: &ast::FnDecl,
body: &ast::Expr,
span: Span,
context: &RewriteContext,
shape: Shape,
) -> Option<String> {
let (prefix, extra_offset) = try_opt!(rewrite_closure_fn_decl(
capture,
fn_decl,
body,
span,
context,
shape,
));
// 1 = space between `|...|` and body.
let extra_offset = extra_offset(&prefix, shape) + 1;
let body_shape = try_opt!(shape.offset_left(extra_offset));
if let ast::ExprKind::Block(ref block) = body.node {
@ -649,7 +646,12 @@ fn rewrite_closure(
block_contains_comment(block, context.codemap) ||
prefix.contains('\n');
if ret_str.is_empty() && !needs_block {
let no_return_type = if let ast::FunctionRetTy::Default(_) = fn_decl.output {
true
} else {
false
};
if no_return_type && !needs_block {
// lock.stmts.len() == 1
if let Some(ref expr) = stmt_expr(&block.stmts[0]) {
if let Some(rw) = rewrite_closure_expr(expr, &prefix, context, body_shape) {
@ -671,15 +673,23 @@ fn rewrite_closure(
}
// Either we require a block, or tried without and failed.
return rewrite_closure_block(&block, prefix, context, body_shape);
rewrite_closure_block(&block, &prefix, context, body_shape)
} else {
rewrite_closure_expr(body, &prefix, context, body_shape).or_else(|| {
// The closure originally had a non-block expression, but we can't fit on
// one line, so we'll insert a block.
rewrite_closure_with_block(context, body_shape, &prefix, body)
})
}
}
if let Some(rw) = rewrite_closure_expr(body, &prefix, context, body_shape) {
return Some(rw);
}
// The closure originally had a non-block expression, but we can't fit on
// one line, so we'll insert a block.
// Rewrite closure with a single expression wrapping its body with block.
fn rewrite_closure_with_block(
context: &RewriteContext,
shape: Shape,
prefix: &str,
body: &ast::Expr,
) -> Option<String> {
let block = ast::Block {
stmts: vec![
ast::Stmt {
@ -692,48 +702,50 @@ fn rewrite_closure(
rules: ast::BlockCheckMode::Default,
span: body.span,
};
return rewrite_closure_block(&block, prefix, context, body_shape);
rewrite_closure_block(&block, prefix, context, shape)
}
fn rewrite_closure_expr(
expr: &ast::Expr,
prefix: &str,
context: &RewriteContext,
shape: Shape,
) -> Option<String> {
let mut rewrite = expr.rewrite(context, shape);
if classify::expr_requires_semi_to_be_stmt(left_most_sub_expr(expr)) {
rewrite = and_one_line(rewrite);
}
rewrite.map(|rw| format!("{} {}", prefix, rw))
// Rewrite closure with a single expression without wrapping its body with block.
fn rewrite_closure_expr(
expr: &ast::Expr,
prefix: &str,
context: &RewriteContext,
shape: Shape,
) -> Option<String> {
let mut rewrite = expr.rewrite(context, shape);
if classify::expr_requires_semi_to_be_stmt(left_most_sub_expr(expr)) {
rewrite = and_one_line(rewrite);
}
rewrite.map(|rw| format!("{} {}", prefix, rw))
}
fn rewrite_closure_block(
block: &ast::Block,
prefix: String,
context: &RewriteContext,
shape: Shape,
) -> Option<String> {
// Start with visual indent, then fall back to block indent if the
// closure is large.
let block_threshold = context.config.closure_block_indent_threshold();
if block_threshold >= 0 {
if let Some(block_str) = block.rewrite(&context, shape) {
if block_str.matches('\n').count() <= block_threshold as usize &&
!need_block_indent(&block_str, shape)
{
if let Some(block_str) = block_str.rewrite(context, shape) {
return Some(format!("{} {}", prefix, block_str));
}
// Rewrite closure whose body is block.
fn rewrite_closure_block(
block: &ast::Block,
prefix: &str,
context: &RewriteContext,
shape: Shape,
) -> Option<String> {
// Start with visual indent, then fall back to block indent if the
// closure is large.
let block_threshold = context.config.closure_block_indent_threshold();
if block_threshold >= 0 {
if let Some(block_str) = block.rewrite(&context, shape) {
if block_str.matches('\n').count() <= block_threshold as usize &&
!need_block_indent(&block_str, shape)
{
if let Some(block_str) = block_str.rewrite(context, shape) {
return Some(format!("{} {}", prefix, block_str));
}
}
}
// The body of the closure is big enough to be block indented, that
// means we must re-format.
let block_shape = shape.block().with_max_width(context.config);
let block_str = try_opt!(block.rewrite(&context, block_shape));
Some(format!("{} {}", prefix, block_str))
}
// The body of the closure is big enough to be block indented, that
// means we must re-format.
let block_shape = shape.block().with_max_width(context.config);
let block_str = try_opt!(block.rewrite(&context, block_shape));
Some(format!("{} {}", prefix, block_str))
}
fn and_one_line(x: Option<String>) -> Option<String> {
@ -742,14 +754,14 @@ fn and_one_line(x: Option<String>) -> Option<String> {
fn nop_block_collapse(block_str: Option<String>, budget: usize) -> Option<String> {
debug!("nop_block_collapse {:?} {}", block_str, budget);
block_str.map(|block_str| if block_str.starts_with('{') && budget >= 2 &&
(block_str[1..]
.find(|c: char| !c.is_whitespace())
.unwrap() == block_str.len() - 2)
{
"{}".to_owned()
} else {
block_str.to_owned()
block_str.map(|block_str| {
if block_str.starts_with('{') && budget >= 2 &&
(block_str[1..].find(|c: char| !c.is_whitespace()).unwrap() == block_str.len() - 2)
{
"{}".to_owned()
} else {
block_str.to_owned()
}
})
}
@ -856,6 +868,32 @@ impl Rewrite for ast::Stmt {
}
}
// Rewrite condition if the given expression has one.
fn rewrite_cond(context: &RewriteContext, expr: &ast::Expr, shape: Shape) -> Option<String> {
match expr.node {
ast::ExprKind::Match(ref cond, _) => {
// `match `cond` {`
let cond_shape = match context.config.control_style() {
Style::Legacy => try_opt!(shape.shrink_left(6).and_then(|s| s.sub_width(2))),
Style::Rfc => try_opt!(shape.offset_left(8)),
};
cond.rewrite(context, cond_shape)
}
ast::ExprKind::Block(ref block) if block.stmts.len() == 1 => {
stmt_expr(&block.stmts[0]).and_then(|e| rewrite_cond(context, e, shape))
}
_ => {
to_control_flow(expr, ExprType::SubExpression).and_then(|control_flow| {
let alt_block_sep = String::from("\n") +
&shape.indent.block_only().to_string(context.config);
control_flow
.rewrite_cond(context, shape, &alt_block_sep)
.and_then(|rw| Some(rw.0))
})
}
}
}
// Abstraction over control flow expressions
#[derive(Debug)]
struct ControlFlow<'a> {
@ -873,6 +911,56 @@ struct ControlFlow<'a> {
span: Span,
}
fn to_control_flow<'a>(expr: &'a ast::Expr, expr_type: ExprType) -> Option<ControlFlow<'a>> {
match expr.node {
ast::ExprKind::If(ref cond, ref if_block, ref else_block) => {
Some(ControlFlow::new_if(
cond,
None,
if_block,
else_block.as_ref().map(|e| &**e),
expr_type == ExprType::SubExpression,
false,
expr.span,
))
}
ast::ExprKind::IfLet(ref pat, ref cond, ref if_block, ref else_block) => {
Some(ControlFlow::new_if(
cond,
Some(pat),
if_block,
else_block.as_ref().map(|e| &**e),
expr_type == ExprType::SubExpression,
false,
expr.span,
))
}
ast::ExprKind::ForLoop(ref pat, ref cond, ref block, label) => {
Some(ControlFlow::new_for(pat, cond, block, label, expr.span))
}
ast::ExprKind::Loop(ref block, label) => Some(
ControlFlow::new_loop(block, label, expr.span),
),
ast::ExprKind::While(ref cond, ref block, label) => Some(ControlFlow::new_while(
None,
cond,
block,
label,
expr.span,
)),
ast::ExprKind::WhileLet(ref pat, ref cond, ref block, label) => {
Some(ControlFlow::new_while(
Some(pat),
cond,
block,
label,
expr.span,
))
}
_ => None,
}
}
impl<'a> ControlFlow<'a> {
fn new_if(
cond: &'a ast::Expr,
@ -1021,9 +1109,13 @@ impl<'a> ControlFlow<'a> {
}
}
impl<'a> Rewrite for ControlFlow<'a> {
fn rewrite(&self, context: &RewriteContext, shape: Shape) -> Option<String> {
debug!("ControlFlow::rewrite {:?} {:?}", self, shape);
impl<'a> ControlFlow<'a> {
fn rewrite_cond(
&self,
context: &RewriteContext,
shape: Shape,
alt_block_sep: &str,
) -> Option<(String, usize)> {
let constr_shape = if self.nested_if {
// We are part of an if-elseif-else chain. Our constraints are tightened.
// 7 = "} else " .len()
@ -1067,38 +1159,13 @@ impl<'a> Rewrite for ControlFlow<'a> {
if self.allow_single_line && context.config.single_line_if_else_max_width() > 0 {
let trial = self.rewrite_single_line(&pat_expr_string, context, shape.width);
if trial.is_some() &&
trial.as_ref().unwrap().len() <= context.config.single_line_if_else_max_width()
{
return trial;
if let Some(cond_str) = trial {
if cond_str.len() <= context.config.single_line_if_else_max_width() {
return Some((cond_str, 0));
}
}
}
let used_width = if pat_expr_string.contains('\n') {
last_line_width(&pat_expr_string)
} else {
// 2 = spaces after keyword and condition.
label_string.len() + self.keyword.len() + pat_expr_string.len() + 2
};
let block_width = shape.width.checked_sub(used_width).unwrap_or(0);
// This is used only for the empty block case: `{}`. So, we use 1 if we know
// we should avoid the single line case.
let block_width = if self.else_block.is_some() || self.nested_if {
min(1, block_width)
} else {
block_width
};
let block_shape = Shape {
width: block_width,
..shape
};
let mut block_context = context.clone();
block_context.is_if_else_block = self.else_block.is_some();
let block_str = try_opt!(self.block.rewrite(&block_context, block_shape));
let cond_span = if let Some(cond) = self.cond {
cond.span
} else {
@ -1123,34 +1190,75 @@ impl<'a> Rewrite for ControlFlow<'a> {
let after_cond_comment =
extract_comment(mk_sp(cond_span.hi, self.block.span.lo), context, shape);
let alt_block_sep = String::from("\n") +
&shape.indent.block_only().to_string(context.config);
let block_sep = if self.cond.is_none() && between_kwd_cond_comment.is_some() {
""
} else if context.config.control_brace_style() == ControlBraceStyle::AlwaysNextLine ||
force_newline_brace
{
alt_block_sep.as_str()
alt_block_sep
} else {
" "
};
let mut result =
format!("{}{}{}{}{}{}",
label_string,
self.keyword,
between_kwd_cond_comment
.as_ref()
.map_or(if pat_expr_string.is_empty() ||
pat_expr_string.starts_with('\n') {
""
} else {
" "
},
|s| &**s),
pat_expr_string,
after_cond_comment.as_ref().map_or(block_sep, |s| &**s),
block_str);
let used_width = if pat_expr_string.contains('\n') {
last_line_width(&pat_expr_string)
} else {
// 2 = spaces after keyword and condition.
label_string.len() + self.keyword.len() + pat_expr_string.len() + 2
};
Some((
format!(
"{}{}{}{}{}",
label_string,
self.keyword,
between_kwd_cond_comment.as_ref().map_or(
if pat_expr_string.is_empty() ||
pat_expr_string.starts_with('\n')
{
""
} else {
" "
},
|s| &**s,
),
pat_expr_string,
after_cond_comment.as_ref().map_or(block_sep, |s| &**s)
),
used_width,
))
}
}
impl<'a> Rewrite for ControlFlow<'a> {
fn rewrite(&self, context: &RewriteContext, shape: Shape) -> Option<String> {
debug!("ControlFlow::rewrite {:?} {:?}", self, shape);
let alt_block_sep = String::from("\n") +
&shape.indent.block_only().to_string(context.config);
let (cond_str, used_width) = try_opt!(self.rewrite_cond(context, shape, &alt_block_sep));
// If `used_width` is 0, it indicates that whole control flow is written in a single line.
if used_width == 0 {
return Some(cond_str);
}
let block_width = shape.width.checked_sub(used_width).unwrap_or(0);
// This is used only for the empty block case: `{}`. So, we use 1 if we know
// we should avoid the single line case.
let block_width = if self.else_block.is_some() || self.nested_if {
min(1, block_width)
} else {
block_width
};
let block_shape = Shape {
width: block_width,
..shape
};
let mut block_context = context.clone();
block_context.is_if_else_block = self.else_block.is_some();
let block_str = try_opt!(self.block.rewrite(&block_context, block_shape));
let mut result = format!("{}{}", cond_str, block_str);
if let Some(else_block) = self.else_block {
let shape = Shape::indented(shape.indent, context.config);
@ -2108,7 +2216,40 @@ fn rewrite_last_arg_with_overflow<'a, T>(
where
T: Rewrite + Spanned + ToExpr + 'a,
{
let rewrite = last_arg.rewrite(context, shape);
let rewrite = if let Some(expr) = last_arg.to_expr() {
match expr.node {
// When overflowing the closure which consists of a single control flow expression,
// force to use block if its condition uses multi line.
ast::ExprKind::Closure(capture, ref fn_decl, ref body, _) => {
let try_closure_with_block = || {
let body = match body.node {
ast::ExprKind::Block(ref block) if block.stmts.len() == 1 => {
try_opt!(stmt_expr(&block.stmts[0]))
}
_ => body,
};
let (prefix, extra_offset) = try_opt!(rewrite_closure_fn_decl(
capture,
fn_decl,
body,
expr.span,
context,
shape,
));
let shape = try_opt!(shape.offset_left(extra_offset));
rewrite_cond(context, body, shape).map_or(None, |cond| if cond.contains('\n') {
rewrite_closure_with_block(context, shape, &prefix, body)
} else {
None
})
};
try_closure_with_block().or_else(|| expr.rewrite(context, shape))
}
_ => expr.rewrite(context, shape),
}
} else {
last_arg.rewrite(context, shape)
};
let orig_last = last_item.item.clone();
if let Some(rewrite) = rewrite {

6
src/imports.rs
View File

@ -159,9 +159,9 @@ impl Rewrite for ast::ViewPath {
// Returns an empty string when the ViewPath is empty (like foo::bar::{})
fn rewrite(&self, context: &RewriteContext, shape: Shape) -> Option<String> {
match self.node {
ast::ViewPath_::ViewPathList(_, ref path_list) if path_list.is_empty() => {
Some(String::new())
}
ast::ViewPath_::ViewPathList(_, ref path_list) if path_list.is_empty() => Some(
String::new(),
),
ast::ViewPath_::ViewPathList(ref path, ref path_list) => {
rewrite_use_list(shape, path, path_list, self.span, context)
}

20
tests/target/issue-1681.rs Normal file
View File

@ -0,0 +1,20 @@
// rustfmt-max_width: 80
// We would like to surround closure body with block when overflowing the last
// argument of function call if the last argument has condition and without
// block it may go multi lines.
fn foo() {
refmut_map_result(self.cache.borrow_mut(), |cache| {
match cache.entry(cache_key) {
Occupied(entry) => Ok(entry.into_mut()),
Vacant(entry) => {
let statement = {
let sql = try!(entry.key().sql(source));
prepare_fn(&sql)
};
Ok(entry.insert(try!(statement)))
}
}
}).map(MaybeCached::Cached)
}