From ec12a891ef0dd4829611bfc56de72a2ccd9373ce Mon Sep 17 00:00:00 2001 From: topecongiro Date: Mon, 17 Jun 2019 00:50:08 +0900 Subject: [PATCH 1/3] Add tests for #3614 --- .../version_one.rs} | 1 + tests/source/fn-single-line/version_two.rs | 80 +++++++++++++++++++ .../version_one.rs} | 1 + tests/target/fn-single-line/version_two.rs | 67 ++++++++++++++++ tests/target/issue-3614/version_one.rs | 15 ++++ tests/target/issue-3614/version_two.rs | 8 ++ 6 files changed, 172 insertions(+) rename tests/source/{fn-single-line.rs => fn-single-line/version_one.rs} (97%) create mode 100644 tests/source/fn-single-line/version_two.rs rename tests/target/{fn-single-line.rs => fn-single-line/version_one.rs} (97%) create mode 100644 tests/target/fn-single-line/version_two.rs create mode 100644 tests/target/issue-3614/version_one.rs create mode 100644 tests/target/issue-3614/version_two.rs diff --git a/tests/source/fn-single-line.rs b/tests/source/fn-single-line/version_one.rs similarity index 97% rename from tests/source/fn-single-line.rs rename to tests/source/fn-single-line/version_one.rs index 6d7f764d280..469ab621567 100644 --- a/tests/source/fn-single-line.rs +++ b/tests/source/fn-single-line/version_one.rs @@ -1,4 +1,5 @@ // rustfmt-fn_single_line: true +// rustfmt-version: One // Test single-line functions. fn foo_expr() { diff --git a/tests/source/fn-single-line/version_two.rs b/tests/source/fn-single-line/version_two.rs new file mode 100644 index 00000000000..bf381ff1065 --- /dev/null +++ b/tests/source/fn-single-line/version_two.rs @@ -0,0 +1,80 @@ +// rustfmt-fn_single_line: true +// rustfmt-version: Two +// Test single-line functions. + +fn foo_expr() { + 1 +} + +fn foo_stmt() { + foo(); +} + +fn foo_decl_local() { + let z = 5; + } + +fn foo_decl_item(x: &mut i32) { + x = 3; +} + + fn empty() { + +} + +fn foo_return() -> String { + "yay" +} + +fn foo_where() -> T where T: Sync { + let x = 2; +} + +fn fooblock() { + { + "inner-block" + } +} + +fn fooblock2(x: i32) { + let z = match x { + _ => 2, + }; +} + +fn comment() { + // this is a test comment + 1 +} + +fn comment2() { + // multi-line comment + let z = 2; + 1 +} + +fn only_comment() { + // Keep this here +} + +fn aaaaaaaaaaaaaaaaa_looooooooooooooooooooooong_name() { + let z = "aaaaaaawwwwwwwwwwwwwwwwwwwwwwwwwwww"; +} + +fn lots_of_space () { + 1 +} + +fn mac() -> Vec { vec![] } + +trait CoolTypes { + fn dummy(&self) { + } +} + +trait CoolerTypes { fn dummy(&self) { +} +} + +fn Foo() where T: Bar { +} diff --git a/tests/target/fn-single-line.rs b/tests/target/fn-single-line/version_one.rs similarity index 97% rename from tests/target/fn-single-line.rs rename to tests/target/fn-single-line/version_one.rs index 3617506d89a..013b2cd7216 100644 --- a/tests/target/fn-single-line.rs +++ b/tests/target/fn-single-line/version_one.rs @@ -1,4 +1,5 @@ // rustfmt-fn_single_line: true +// rustfmt-version: One // Test single-line functions. fn foo_expr() { 1 } diff --git a/tests/target/fn-single-line/version_two.rs b/tests/target/fn-single-line/version_two.rs new file mode 100644 index 00000000000..b8053d4c2f5 --- /dev/null +++ b/tests/target/fn-single-line/version_two.rs @@ -0,0 +1,67 @@ +// rustfmt-fn_single_line: true +// rustfmt-version: Two +// Test single-line functions. + +fn foo_expr() { 1 } + +fn foo_stmt() { foo(); } + +fn foo_decl_local() { let z = 5; } + +fn foo_decl_item(x: &mut i32) { x = 3; } + +fn empty() {} + +fn foo_return() -> String { "yay" } + +fn foo_where() -> T +where + T: Sync, +{ + let x = 2; +} + +fn fooblock() { { "inner-block" } } + +fn fooblock2(x: i32) { + let z = match x { + _ => 2, + }; +} + +fn comment() { + // this is a test comment + 1 +} + +fn comment2() { + // multi-line comment + let z = 2; + 1 +} + +fn only_comment() { + // Keep this here +} + +fn aaaaaaaaaaaaaaaaa_looooooooooooooooooooooong_name() { + let z = "aaaaaaawwwwwwwwwwwwwwwwwwwwwwwwwwww"; +} + +fn lots_of_space() { 1 } + +fn mac() -> Vec { vec![] } + +trait CoolTypes { + fn dummy(&self) {} +} + +trait CoolerTypes { + fn dummy(&self) {} +} + +fn Foo() +where + T: Bar, +{ +} diff --git a/tests/target/issue-3614/version_one.rs b/tests/target/issue-3614/version_one.rs new file mode 100644 index 00000000000..8ab28304732 --- /dev/null +++ b/tests/target/issue-3614/version_one.rs @@ -0,0 +1,15 @@ +// rustfmt-version: One + +fn main() { + let toto = || { + if true { + 42 + } else { + 24 + } + }; + + { + T + } +} diff --git a/tests/target/issue-3614/version_two.rs b/tests/target/issue-3614/version_two.rs new file mode 100644 index 00000000000..5d6f8e7a313 --- /dev/null +++ b/tests/target/issue-3614/version_two.rs @@ -0,0 +1,8 @@ +// rustfmt-version: Two + +fn main() { + let toto = || { + if true { 42 } else { 24 } + }; + { T } +} From ef99c742e691100cfe9c70ec04b242f63261727c Mon Sep 17 00:00:00 2001 From: topecongiro Date: Mon, 17 Jun 2019 00:50:22 +0900 Subject: [PATCH 2/3] Format the last expression-statement as expression --- src/expr.rs | 34 +------------- src/items.rs | 20 ++------ src/lib.rs | 1 + src/stmt.rs | 122 +++++++++++++++++++++++++++++++++++++++++++++++++ src/visitor.rs | 53 +++++++++++---------- 5 files changed, 158 insertions(+), 72 deletions(-) create mode 100644 src/stmt.rs diff --git a/src/expr.rs b/src/expr.rs index b1ba99ba6e5..0ecc9e28b63 100644 --- a/src/expr.rs +++ b/src/expr.rs @@ -33,7 +33,7 @@ use crate::types::{rewrite_path, PathContext}; use crate::utils::{ colon_spaces, contains_skip, count_newlines, first_line_ends_with, inner_attributes, last_line_extendable, last_line_width, mk_sp, outer_attributes, ptr_vec_to_ref_vec, - semicolon_for_expr, semicolon_for_stmt, unicode_str_width, wrap_str, + semicolon_for_expr, unicode_str_width, wrap_str, }; use crate::vertical::rewrite_with_alignment; use crate::visitor::FmtVisitor; @@ -568,28 +568,6 @@ fn rewrite_block( result } -impl Rewrite for ast::Stmt { - fn rewrite(&self, context: &RewriteContext<'_>, shape: Shape) -> Option { - skip_out_of_file_lines_range!(context, self.span()); - - let result = match self.node { - ast::StmtKind::Local(ref local) => local.rewrite(context, shape), - ast::StmtKind::Expr(ref ex) | ast::StmtKind::Semi(ref ex) => { - let suffix = if semicolon_for_stmt(context, self) { - ";" - } else { - "" - }; - - let shape = shape.sub_width(suffix.len())?; - format_expr(ex, ExprType::Statement, context, shape).map(|s| s + suffix) - } - ast::StmtKind::Mac(..) | ast::StmtKind::Item(..) => None, - }; - result.and_then(|res| recover_comment_removed(res, self.span(), context)) - } -} - // Rewrite condition if the given expression has one. pub(crate) fn rewrite_cond( context: &RewriteContext<'_>, @@ -1189,16 +1167,6 @@ pub(crate) fn stmt_is_expr(stmt: &ast::Stmt) -> bool { } } -pub(crate) fn stmt_is_if(stmt: &ast::Stmt) -> bool { - match stmt.node { - ast::StmtKind::Expr(ref e) => match e.node { - ast::ExprKind::If(..) => true, - _ => false, - }, - _ => false, - } -} - pub(crate) fn is_unsafe_block(block: &ast::Block) -> bool { if let ast::BlockCheckMode::Unsafe(..) = block.rules { true diff --git a/src/items.rs b/src/items.rs index 14d386047f0..1b7ac780129 100644 --- a/src/items.rs +++ b/src/items.rs @@ -18,8 +18,7 @@ use crate::comment::{ use crate::config::lists::*; use crate::config::{BraceStyle, Config, IndentStyle, Version}; use crate::expr::{ - format_expr, is_empty_block, is_simple_block_stmt, rewrite_assign_rhs, rewrite_assign_rhs_with, - ExprType, RhsTactics, + is_empty_block, is_simple_block_stmt, rewrite_assign_rhs, rewrite_assign_rhs_with, RhsTactics, }; use crate::lists::{definitive_tactic, itemize_list, write_list, ListFormatting, Separator}; use crate::macros::{rewrite_macro, MacroPosition}; @@ -28,6 +27,7 @@ use crate::rewrite::{Rewrite, RewriteContext}; use crate::shape::{Indent, Shape}; use crate::source_map::{LineRangeUtils, SpanUtils}; use crate::spanned::Spanned; +use crate::stmt::Stmt; use crate::utils::*; use crate::vertical::rewrite_with_alignment; use crate::visitor::FmtVisitor; @@ -394,20 +394,8 @@ impl<'a> FmtVisitor<'a> { return None; } - let stmt = block.stmts.first()?; - let res = match stmt_expr(stmt) { - Some(e) => { - let suffix = if semicolon_for_expr(&self.get_context(), e) { - ";" - } else { - "" - }; - - format_expr(e, ExprType::Statement, &self.get_context(), self.shape()) - .map(|s| s + suffix)? - } - None => stmt.rewrite(&self.get_context(), self.shape())?, - }; + let res = Stmt::from_ast_node(block.stmts.first()?, true) + .rewrite(&self.get_context(), self.shape())?; let width = self.block_indent.width() + fn_str.len() + res.len() + 5; if !res.contains('\n') && width <= self.config.max_width() { diff --git a/src/lib.rs b/src/lib.rs index 14e2a6fe6e5..1b4ce663313 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -72,6 +72,7 @@ mod shape; pub(crate) mod source_file; pub(crate) mod source_map; mod spanned; +mod stmt; mod string; #[cfg(test)] mod test; diff --git a/src/stmt.rs b/src/stmt.rs new file mode 100644 index 00000000000..6e78d3b20da --- /dev/null +++ b/src/stmt.rs @@ -0,0 +1,122 @@ +use syntax::ast; +use syntax_pos::Span; + +use crate::comment::recover_comment_removed; +use crate::config::Version; +use crate::expr::{format_expr, ExprType}; +use crate::rewrite::{Rewrite, RewriteContext}; +use crate::shape::Shape; +use crate::source_map::LineRangeUtils; +use crate::spanned::Spanned; +use crate::utils::semicolon_for_stmt; + +pub(crate) struct Stmt<'a> { + inner: &'a ast::Stmt, + is_last: bool, +} + +impl<'a> Spanned for Stmt<'a> { + fn span(&self) -> Span { + self.inner.span() + } +} + +impl<'a> Stmt<'a> { + pub(crate) fn as_ast_node(&self) -> &ast::Stmt { + self.inner + } + + pub(crate) fn is_if(&self) -> bool { + match self.inner.node { + ast::StmtKind::Expr(ref e) => match e.node { + ast::ExprKind::If(..) => true, + _ => false, + }, + _ => false, + } + } + + pub(crate) fn to_item(&self) -> Option<&ast::Item> { + match self.inner.node { + ast::StmtKind::Item(ref item) => Some(&**item), + _ => None, + } + } + + pub(crate) fn from_ast_node(inner: &'a ast::Stmt, is_last: bool) -> Self { + Stmt { inner, is_last } + } + + pub(crate) fn from_ast_nodes(iter: I) -> Vec + where + I: Iterator, + { + let mut result = vec![]; + let mut iter = iter.peekable(); + while iter.peek().is_some() { + result.push(Stmt { + inner: iter.next().unwrap(), + is_last: iter.peek().is_none(), + }) + } + result + } + + fn is_last_expr(&self) -> bool { + if !self.is_last { + return false; + } + + match self.as_ast_node().node { + ast::StmtKind::Expr(ref expr) => match expr.node { + ast::ExprKind::Ret(..) | ast::ExprKind::Continue(..) | ast::ExprKind::Break(..) => { + false + } + _ => true, + }, + _ => false, + } + } +} + +impl<'a> Rewrite for Stmt<'a> { + fn rewrite(&self, context: &RewriteContext<'_>, shape: Shape) -> Option { + let expr_type = if context.config.version() == Version::Two && self.is_last_expr() { + ExprType::SubExpression + } else { + ExprType::Statement + }; + format_stmt(context, shape, self.as_ast_node(), expr_type) + } +} + +impl Rewrite for ast::Stmt { + fn rewrite(&self, context: &RewriteContext<'_>, shape: Shape) -> Option { + format_stmt(context, shape, self, ExprType::Statement) + } +} + +fn format_stmt( + context: &RewriteContext<'_>, + shape: Shape, + stmt: &ast::Stmt, + expr_type: ExprType, +) -> Option { + skip_out_of_file_lines_range!(context, stmt.span()); + + let result = match stmt.node { + ast::StmtKind::Local(ref local) => local.rewrite(context, shape), + ast::StmtKind::Expr(ref ex) | ast::StmtKind::Semi(ref ex) => { + let suffix = if semicolon_for_stmt(context, stmt) { + ";" + } else { + "" + }; + + let shape = shape.sub_width(suffix.len())?; + format_expr(ex, expr_type, context, shape).map(|s| s + suffix) + } + ast::StmtKind::Mac(..) | ast::StmtKind::Item(..) => None, + }; + result.and_then(|res| recover_comment_removed(res, stmt.span(), context)) +} diff --git a/src/visitor.rs b/src/visitor.rs index 0560cd2ff09..a084a93a79f 100644 --- a/src/visitor.rs +++ b/src/visitor.rs @@ -20,6 +20,7 @@ use crate::rewrite::{Rewrite, RewriteContext}; use crate::shape::{Indent, Shape}; use crate::source_map::{LineRangeUtils, SpanUtils}; use crate::spanned::Spanned; +use crate::stmt::Stmt; use crate::utils::{ self, contains_skip, count_newlines, depr_skip_annotation, get_skip_macro_names, inner_attributes, mk_sp, ptr_vec_to_ref_vec, rewrite_ident, stmt_expr, @@ -89,23 +90,27 @@ impl<'b, 'a: 'b> FmtVisitor<'a> { Shape::indented(self.block_indent, self.config) } - fn visit_stmt(&mut self, stmt: &ast::Stmt) { + fn visit_stmt(&mut self, stmt: &Stmt<'_>) { debug!( "visit_stmt: {:?} {:?}", - self.source_map.lookup_char_pos(stmt.span.lo()), - self.source_map.lookup_char_pos(stmt.span.hi()) + self.source_map.lookup_char_pos(stmt.span().lo()), + self.source_map.lookup_char_pos(stmt.span().hi()) ); - match stmt.node { + match stmt.as_ast_node().node { ast::StmtKind::Item(ref item) => { self.visit_item(item); // Handle potential `;` after the item. - self.format_missing(stmt.span.hi()); + self.format_missing(stmt.span().hi()); } ast::StmtKind::Local(..) | ast::StmtKind::Expr(..) | ast::StmtKind::Semi(..) => { - let attrs = get_attrs_from_stmt(stmt); + let attrs = get_attrs_from_stmt(stmt.as_ast_node()); if contains_skip(attrs) { - self.push_skipped_with_span(attrs, stmt.span(), get_span_without_attrs(stmt)); + self.push_skipped_with_span( + attrs, + stmt.span(), + get_span_without_attrs(stmt.as_ast_node()), + ); } else { let shape = self.shape(); let rewrite = self.with_context(|ctx| stmt.rewrite(&ctx, shape)); @@ -115,11 +120,15 @@ impl<'b, 'a: 'b> FmtVisitor<'a> { ast::StmtKind::Mac(ref mac) => { let (ref mac, _macro_style, ref attrs) = **mac; if self.visit_attrs(attrs, ast::AttrStyle::Outer) { - self.push_skipped_with_span(attrs, stmt.span(), get_span_without_attrs(stmt)); + self.push_skipped_with_span( + attrs, + stmt.span(), + get_span_without_attrs(stmt.as_ast_node()), + ); } else { self.visit_mac(mac, None, MacroPosition::Statement); } - self.format_missing(stmt.span.hi()); + self.format_missing(stmt.span().hi()); } } } @@ -717,14 +726,7 @@ impl<'b, 'a: 'b> FmtVisitor<'a> { self.visit_items_with_reordering(&ptr_vec_to_ref_vec(&m.items)); } - fn walk_stmts(&mut self, stmts: &[ast::Stmt]) { - fn to_stmt_item(stmt: &ast::Stmt) -> Option<&ast::Item> { - match stmt.node { - ast::StmtKind::Item(ref item) => Some(&**item), - _ => None, - } - } - + fn walk_stmts(&mut self, stmts: &[Stmt<'_>]) { if stmts.is_empty() { return; } @@ -732,8 +734,8 @@ impl<'b, 'a: 'b> FmtVisitor<'a> { // Extract leading `use ...;`. let items: Vec<_> = stmts .iter() - .take_while(|stmt| to_stmt_item(stmt).map_or(false, is_use_item)) - .filter_map(|stmt| to_stmt_item(stmt)) + .take_while(|stmt| stmt.to_item().map_or(false, is_use_item)) + .filter_map(|stmt| stmt.to_item()) .collect(); if items.is_empty() { @@ -741,12 +743,17 @@ impl<'b, 'a: 'b> FmtVisitor<'a> { // line if possible. if self.config.version() == Version::Two && stmts.len() == 1 - && crate::expr::stmt_is_if(&stmts[0]) - && !contains_skip(get_attrs_from_stmt(&stmts[0])) + && stmts[0].is_if() + && !contains_skip(get_attrs_from_stmt(stmts[0].as_ast_node())) { let shape = self.shape(); let rewrite = self.with_context(|ctx| { - format_expr(stmt_expr(&stmts[0])?, ExprType::SubExpression, ctx, shape) + format_expr( + stmt_expr(stmts[0].as_ast_node())?, + ExprType::SubExpression, + ctx, + shape, + ) }); self.push_rewrite(stmts[0].span(), rewrite); } else { @@ -760,7 +767,7 @@ impl<'b, 'a: 'b> FmtVisitor<'a> { } fn walk_block_stmts(&mut self, b: &ast::Block) { - self.walk_stmts(&b.stmts) + self.walk_stmts(&Stmt::from_ast_nodes(b.stmts.iter())) } fn format_mod( From 39bcc0ac204d2331884c2d1d5fa941677019baaa Mon Sep 17 00:00:00 2001 From: topecongiro Date: Mon, 17 Jun 2019 00:54:45 +0900 Subject: [PATCH 3/3] Remove code that is specific for handling the last if --- src/stmt.rs | 10 ---------- src/visitor.rs | 26 +++----------------------- 2 files changed, 3 insertions(+), 33 deletions(-) diff --git a/src/stmt.rs b/src/stmt.rs index 6e78d3b20da..dbda00f00ce 100644 --- a/src/stmt.rs +++ b/src/stmt.rs @@ -26,16 +26,6 @@ impl<'a> Stmt<'a> { self.inner } - pub(crate) fn is_if(&self) -> bool { - match self.inner.node { - ast::StmtKind::Expr(ref e) => match e.node { - ast::ExprKind::If(..) => true, - _ => false, - }, - _ => false, - } - } - pub(crate) fn to_item(&self) -> Option<&ast::Item> { match self.inner.node { ast::StmtKind::Item(ref item) => Some(&**item), diff --git a/src/visitor.rs b/src/visitor.rs index a084a93a79f..19200f9592c 100644 --- a/src/visitor.rs +++ b/src/visitor.rs @@ -7,8 +7,7 @@ use syntax::{ast, visit}; use crate::attr::*; use crate::comment::{CodeCharKind, CommentCodeSlices}; use crate::config::file_lines::FileName; -use crate::config::{BraceStyle, Config, Version}; -use crate::expr::{format_expr, ExprType}; +use crate::config::{BraceStyle, Config}; use crate::items::{ format_impl, format_trait, format_trait_alias, is_mod_decl, is_use_item, rewrite_associated_impl_type, rewrite_associated_type, rewrite_existential_impl_type, @@ -739,27 +738,8 @@ impl<'b, 'a: 'b> FmtVisitor<'a> { .collect(); if items.is_empty() { - // The `if` expression at the end of the block should be formatted in a single - // line if possible. - if self.config.version() == Version::Two - && stmts.len() == 1 - && stmts[0].is_if() - && !contains_skip(get_attrs_from_stmt(stmts[0].as_ast_node())) - { - let shape = self.shape(); - let rewrite = self.with_context(|ctx| { - format_expr( - stmt_expr(stmts[0].as_ast_node())?, - ExprType::SubExpression, - ctx, - shape, - ) - }); - self.push_rewrite(stmts[0].span(), rewrite); - } else { - self.visit_stmt(&stmts[0]); - self.walk_stmts(&stmts[1..]); - } + self.visit_stmt(&stmts[0]); + self.walk_stmts(&stmts[1..]); } else { self.visit_items_with_reordering(&items); self.walk_stmts(&stmts[items.len()..]);