From eeb20e20333127add88aa712091a958a0539a604 Mon Sep 17 00:00:00 2001 From: topecongiro Date: Tue, 20 Jun 2017 21:34:19 +0900 Subject: [PATCH 1/4] Refactor rewrite for ast::Block --- src/expr.rs | 195 ++++++++++++++++++++++++++++++---------------------- 1 file changed, 113 insertions(+), 82 deletions(-) diff --git a/src/expr.rs b/src/expr.rs index 2be7987d7c6..ebf9d5c1baa 100644 --- a/src/expr.rs +++ b/src/expr.rs @@ -290,7 +290,9 @@ fn format_expr( ) } ast::ExprKind::Catch(ref block) => { - if let rewrite @ Some(_) = try_one_line_block(context, shape, "do catch ", block) { + if let rewrite @ Some(_) = + rewrite_single_line_block(context, "do catch ", block, shape) + { return rewrite; } // 9 = `do catch ` @@ -315,23 +317,6 @@ fn format_expr( } } -fn try_one_line_block( - context: &RewriteContext, - shape: Shape, - prefix: &str, - block: &ast::Block, -) -> Option { - if is_simple_block(block, context.codemap) { - let expr_shape = Shape::legacy(shape.width - prefix.len(), shape.indent); - let expr_str = try_opt!(block.stmts[0].rewrite(context, expr_shape)); - let result = format!("{}{{ {} }}", prefix, expr_str); - if result.len() <= shape.width && !result.contains('\n') { - return Some(result); - } - } - None -} - pub fn rewrite_pair( lhs: &LHS, rhs: &RHS, @@ -763,78 +748,124 @@ fn nop_block_collapse(block_str: Option, budget: usize) -> Option Option { + if block.stmts.is_empty() && !block_contains_comment(block, context.codemap) && + shape.width >= 2 + { + return Some("{}".to_owned()); + } + + // If a block contains only a single-line comment, then leave it on one line. + let user_str = context.snippet(block.span); + let user_str = user_str.trim(); + if user_str.starts_with('{') && user_str.ends_with('}') { + let comment_str = user_str[1..user_str.len() - 1].trim(); + if block.stmts.is_empty() && !comment_str.contains('\n') && + !comment_str.starts_with("//") && comment_str.len() + 4 <= shape.width + { + return Some(format!("{{ {} }}", comment_str)); + } + } + + None +} + +fn block_prefix(context: &RewriteContext, block: &ast::Block, shape: Shape) -> Option { + Some(match block.rules { + ast::BlockCheckMode::Unsafe(..) => { + let snippet = context.snippet(block.span); + let open_pos = try_opt!(snippet.find_uncommented("{")); + // Extract comment between unsafe and block start. + let trimmed = &snippet[6..open_pos].trim(); + + if !trimmed.is_empty() { + // 9 = "unsafe {".len(), 7 = "unsafe ".len() + let budget = try_opt!(shape.width.checked_sub(9)); + format!( + "unsafe {} ", + try_opt!(rewrite_comment( + trimmed, + true, + Shape::legacy(budget, shape.indent + 7), + context.config, + )) + ) + } else { + "unsafe ".to_owned() + } + } + ast::BlockCheckMode::Default => String::new(), + }) +} + +fn rewrite_single_line_block( + context: &RewriteContext, + prefix: &str, + block: &ast::Block, + shape: Shape, +) -> Option { + if is_simple_block(block, context.codemap) { + let expr_shape = Shape::legacy(shape.width - prefix.len(), shape.indent); + let expr_str = try_opt!(block.stmts[0].rewrite(context, expr_shape)); + let result = format!("{}{{ {} }}", prefix, expr_str); + if result.len() <= shape.width && !result.contains('\n') { + return Some(result); + } + } + None +} + +fn rewrite_block_with_visitor( + context: &RewriteContext, + prefix: &str, + block: &ast::Block, + shape: Shape, +) -> Option { + if let rw @ Some(_) = rewrite_empty_block(context, block, shape) { + return rw; + } + + let mut visitor = FmtVisitor::from_codemap(context.parse_session, context.config); + visitor.block_indent = shape.indent; + visitor.is_if_else_block = context.is_if_else_block; + match block.rules { + ast::BlockCheckMode::Unsafe(..) => { + let snippet = context.snippet(block.span); + let open_pos = try_opt!(snippet.find_uncommented("{")); + visitor.last_pos = block.span.lo + BytePos(open_pos as u32) + } + ast::BlockCheckMode::Default => visitor.last_pos = block.span.lo, + } + + visitor.visit_block(block); + if visitor.failed && shape.indent.alignment != 0 { + block.rewrite( + context, + Shape::indented(shape.indent.block_only(), context.config), + ) + } else { + Some(format!("{}{}", prefix, visitor.buffer)) + } +} + impl Rewrite for ast::Block { fn rewrite(&self, context: &RewriteContext, shape: Shape) -> Option { // shape.width is used only for the single line case: either the empty block `{}`, // or an unsafe expression `unsafe { e }`. - - if self.stmts.is_empty() && !block_contains_comment(self, context.codemap) && - shape.width >= 2 - { - return Some("{}".to_owned()); + if let rw @ Some(_) = rewrite_empty_block(context, self, shape) { + return rw; } - // If a block contains only a single-line comment, then leave it on one line. - let user_str = context.snippet(self.span); - let user_str = user_str.trim(); - if user_str.starts_with('{') && user_str.ends_with('}') { - let comment_str = user_str[1..user_str.len() - 1].trim(); - if self.stmts.is_empty() && !comment_str.contains('\n') && - !comment_str.starts_with("//") && - comment_str.len() + 4 <= shape.width - { - return Some(format!("{{ {} }}", comment_str)); - } + let prefix = try_opt!(block_prefix(context, self, shape)); + if let rw @ Some(_) = rewrite_single_line_block(context, &prefix, self, shape) { + return rw; } - let mut visitor = FmtVisitor::from_codemap(context.parse_session, context.config); - visitor.block_indent = shape.indent; - visitor.is_if_else_block = context.is_if_else_block; - - let prefix = match self.rules { - ast::BlockCheckMode::Unsafe(..) => { - let snippet = context.snippet(self.span); - let open_pos = try_opt!(snippet.find_uncommented("{")); - visitor.last_pos = self.span.lo + BytePos(open_pos as u32); - - // Extract comment between unsafe and block start. - let trimmed = &snippet[6..open_pos].trim(); - - let prefix = if !trimmed.is_empty() { - // 9 = "unsafe {".len(), 7 = "unsafe ".len() - let budget = try_opt!(shape.width.checked_sub(9)); - format!( - "unsafe {} ", - try_opt!(rewrite_comment( - trimmed, - true, - Shape::legacy(budget, shape.indent + 7), - context.config, - )) - ) - } else { - "unsafe ".to_owned() - }; - if let result @ Some(_) = try_one_line_block(context, shape, &prefix, self) { - return result; - } - prefix - } - ast::BlockCheckMode::Default => { - visitor.last_pos = self.span.lo; - String::new() - } - }; - - visitor.visit_block(self); - if visitor.failed && shape.indent.alignment != 0 { - self.rewrite( - context, - Shape::indented(shape.indent.block_only(), context.config), - ) - } else { - Some(format!("{}{}", prefix, visitor.buffer)) - } + rewrite_block_with_visitor(context, &prefix, self, shape) } } From fb1225a8af14567eaf21d5978f7e7a6498867381 Mon Sep 17 00:00:00 2001 From: topecongiro Date: Tue, 20 Jun 2017 21:35:52 +0900 Subject: [PATCH 2/4] Use format_expr wherever single-lined block is not allowed --- src/expr.rs | 38 +++++++++++++++++++++++++++++++------- src/items.rs | 6 ++++-- src/visitor.rs | 16 +++++++++++++++- 3 files changed, 50 insertions(+), 10 deletions(-) diff --git a/src/expr.rs b/src/expr.rs index ebf9d5c1baa..e3c80cd95ca 100644 --- a/src/expr.rs +++ b/src/expr.rs @@ -42,7 +42,7 @@ impl Rewrite for ast::Expr { } #[derive(PartialEq)] -enum ExprType { +pub enum ExprType { Statement, SubExpression, } @@ -67,7 +67,7 @@ fn combine_attr_and_expr( format!("{}{}{}", attr_str, separator, expr_str) } -fn format_expr( +pub fn format_expr( expr: &ast::Expr, expr_type: ExprType, context: &RewriteContext, @@ -160,7 +160,23 @@ fn format_expr( 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::Block(ref block) => { + match expr_type { + ExprType::Statement => { + if is_unsafe_block(block) { + block.rewrite(context, shape) + } else { + // Rewrite block without trying to put it in a single line. + if let rw @ Some(_) = rewrite_empty_block(context, block, shape) { + return rw; + } + let prefix = try_opt!(block_prefix(context, block, shape)); + rewrite_block_with_visitor(context, &prefix, block, shape) + } + } + ExprType::SubExpression => block.rewrite(context, shape), + } + } ast::ExprKind::Match(ref cond, ref arms) => { rewrite_match(context, cond, arms, shape, expr.span) } @@ -1280,7 +1296,12 @@ impl<'a> Rewrite for ControlFlow<'a> { }; 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 block_str = try_opt!(rewrite_block_with_visitor( + &block_context, + "", + self.block, + block_shape, + )); let mut result = format!("{}{}", cond_str, block_str); @@ -1322,7 +1343,7 @@ impl<'a> Rewrite for ControlFlow<'a> { width: min(1, shape.width), ..shape }; - else_block.rewrite(context, else_shape) + format_expr(else_block, ExprType::Statement, context, else_shape) } }; @@ -1689,7 +1710,10 @@ impl Rewrite for ast::Arm { .unwrap() .sub_width(comma.len()) .unwrap(); - let rewrite = nop_block_collapse(body.rewrite(context, arm_shape), arm_shape.width); + let rewrite = nop_block_collapse( + format_expr(body, ExprType::Statement, context, arm_shape), + arm_shape.width, + ); let is_block = if let ast::ExprKind::Block(..) = body.node { true } else { @@ -1724,7 +1748,7 @@ impl Rewrite for ast::Arm { // necessary. let body_shape = try_opt!(shape.block_left(context.config.tab_spaces())); let next_line_body = try_opt!(nop_block_collapse( - body.rewrite(context, body_shape), + format_expr(body, ExprType::Statement, context, body_shape), body_shape.width, )); let indent_str = shape diff --git a/src/items.rs b/src/items.rs index 9adecb9be94..cb68231733e 100644 --- a/src/items.rs +++ b/src/items.rs @@ -17,7 +17,7 @@ use utils::{format_mutability, format_visibility, contains_skip, end_typaram, wr trimmed_last_line_width, colon_spaces, mk_sp}; use lists::{write_list, itemize_list, ListItem, ListFormatting, SeparatorTactic, list_helper, DefinitiveListTactic, ListTactic, definitive_tactic}; -use expr::{is_empty_block, is_simple_block_stmt, rewrite_assign_rhs}; +use expr::{format_expr, is_empty_block, is_simple_block_stmt, rewrite_assign_rhs, ExprType}; use comment::{FindUncommented, contains_comment, rewrite_comment, recover_comment_removed}; use visitor::FmtVisitor; use rewrite::{Rewrite, RewriteContext}; @@ -352,7 +352,9 @@ impl<'a> FmtVisitor<'a> { Some(e) => { let suffix = if semicolon_for_expr(e) { ";" } else { "" }; - e.rewrite( + format_expr( + &e, + ExprType::Statement, &self.get_context(), Shape::indented(self.block_indent, self.config), ).map(|s| s + suffix) diff --git a/src/visitor.rs b/src/visitor.rs index b34108800e7..0f574d433fd 100644 --- a/src/visitor.rs +++ b/src/visitor.rs @@ -17,6 +17,7 @@ use syntax::parse::ParseSess; use strings::string_buffer::StringBuffer; use {Indent, Shape}; +use expr::{format_expr, ExprType}; use utils::{self, mk_sp}; use codemap::{LineRangeUtils, SpanUtils}; use comment::{contains_comment, FindUncommented}; @@ -87,7 +88,20 @@ impl<'a> FmtVisitor<'a> { ); self.push_rewrite(stmt.span, rewrite); } - ast::StmtKind::Expr(ref expr) | + ast::StmtKind::Expr(ref expr) => { + let rewrite = format_expr( + expr, + ExprType::Statement, + &self.get_context(), + Shape::indented(self.block_indent, self.config), + ); + let span = if expr.attrs.is_empty() { + stmt.span + } else { + mk_sp(expr.attrs[0].span.lo, stmt.span.hi) + }; + self.push_rewrite(span, rewrite) + } ast::StmtKind::Semi(ref expr) => { let rewrite = stmt.rewrite( &self.get_context(), From b5a13602d96b39fbaae46c44c1805f42c55a99be Mon Sep 17 00:00:00 2001 From: topecongiro Date: Tue, 20 Jun 2017 21:36:28 +0900 Subject: [PATCH 3/4] Update tests --- tests/source/expr.rs | 9 +++++++++ tests/target/expr.rs | 9 +++++++++ 2 files changed, 18 insertions(+) diff --git a/tests/source/expr.rs b/tests/source/expr.rs index 11d3fa98f9d..80e4f2b623f 100644 --- a/tests/source/expr.rs +++ b/tests/source/expr.rs @@ -299,3 +299,12 @@ fn issue1106() { .filter_entry(|entry| exclusions.filter_entry(entry)) { } } + +fn issue1570() { + a_very_long_function_name({some_func(1, {1})}) +} + +fn issue1714() { + v = &mut {v}[mid..]; + let (left, right) = {v}.split_at_mut(mid); +} diff --git a/tests/target/expr.rs b/tests/target/expr.rs index 55d023cb55d..b6509040714 100644 --- a/tests/target/expr.rs +++ b/tests/target/expr.rs @@ -358,3 +358,12 @@ fn issue1106() { .filter_entry(|entry| exclusions.filter_entry(entry)) {} } + +fn issue1570() { + a_very_long_function_name({ some_func(1, { 1 }) }) +} + +fn issue1714() { + v = &mut { v }[mid..]; + let (left, right) = { v }.split_at_mut(mid); +} From 64fc9e31e7e102e15d3d6570ce855b8738f6ace9 Mon Sep 17 00:00:00 2001 From: topecongiro Date: Tue, 20 Jun 2017 22:35:56 +0900 Subject: [PATCH 4/4] Fix a typo --- src/bin/rustfmt.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/bin/rustfmt.rs b/src/bin/rustfmt.rs index ba00f123d03..4d2f1cdc041 100644 --- a/src/bin/rustfmt.rs +++ b/src/bin/rustfmt.rs @@ -79,7 +79,10 @@ impl CliOptions { )); } } else { - println!("Warning: the default write-mode for Rustfmt will soon change to overwrite - this will not leave backups of changed files."); + println!( + "Warning: the default write-mode for Rustfmt will soon change to overwrite \ + - this will not leave backups of changed files." + ); } if let Some(ref file_lines) = matches.opt_str("file-lines") {