diff --git a/src/expr.rs b/src/expr.rs index 6bba83b520c..92c62b08125 100644 --- a/src/expr.rs +++ b/src/expr.rs @@ -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 { +) -> 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 { + 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 { 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 { - 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 { + 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 { - // 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 { + // 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) -> Option { @@ -742,14 +754,14 @@ fn and_one_line(x: Option) -> Option { fn nop_block_collapse(block_str: Option, budget: usize) -> Option { 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 { + 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> { + 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 { - 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 { + 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 { diff --git a/src/imports.rs b/src/imports.rs index 3b7c47f0fca..e3c06ae7956 100644 --- a/src/imports.rs +++ b/src/imports.rs @@ -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 { 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) } diff --git a/tests/target/issue-1681.rs b/tests/target/issue-1681.rs new file mode 100644 index 00000000000..5734fd5502d --- /dev/null +++ b/tests/target/issue-1681.rs @@ -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) +}