From c5168405b0a730cd101b04a4892c14808487e6c2 Mon Sep 17 00:00:00 2001 From: Matthew McAllister Date: Fri, 20 Oct 2017 22:09:45 -0700 Subject: [PATCH] Format attributes on block expressions --- src/closures.rs | 8 +- src/expr.rs | 131 +++++++++++++++++++++--------- src/items.rs | 17 ++-- src/visitor.rs | 10 ++- tests/target/attrib-block-expr.rs | 58 +++++++++++++ 5 files changed, 175 insertions(+), 49 deletions(-) create mode 100644 tests/target/attrib-block-expr.rs diff --git a/src/closures.rs b/src/closures.rs index 6097f6339edb..65052d766ebb 100644 --- a/src/closures.rs +++ b/src/closures.rs @@ -50,7 +50,8 @@ pub fn rewrite_closure( if let ast::ExprKind::Block(ref block) = body.node { // The body of the closure is an empty block. if block.stmts.is_empty() && !block_contains_comment(block, context.codemap) { - return Some(format!("{} {{}}", prefix)); + return body.rewrite(context, shape) + .map(|s| format!("{} {}", prefix, s)); } let result = match fn_decl.output { @@ -138,7 +139,7 @@ fn rewrite_closure_with_block( span: body.span, recovered: false, }; - let block = ::expr::rewrite_block_with_visitor(context, "", &block, shape, false)?; + let block = ::expr::rewrite_block_with_visitor(context, "", &block, None, shape, false)?; Some(format!("{} {}", prefix, block)) } @@ -291,7 +292,8 @@ pub fn rewrite_last_closure( if let ast::ExprKind::Closure(capture, movability, ref fn_decl, ref body, _) = expr.node { let body = match body.node { ast::ExprKind::Block(ref block) - if !is_unsafe_block(block) && is_simple_block(block, context.codemap) => + if !is_unsafe_block(block) + && is_simple_block(block, Some(&body.attrs), context.codemap) => { stmt_expr(&block.stmts[0]).unwrap_or(body) } diff --git a/src/expr.rs b/src/expr.rs index 0e6b8f1bc1e3..c959edd2fd72 100644 --- a/src/expr.rs +++ b/src/expr.rs @@ -115,16 +115,25 @@ pub fn format_expr( match expr_type { ExprType::Statement => { if is_unsafe_block(block) { - block.rewrite(context, shape) - } else if let rw @ Some(_) = rewrite_empty_block(context, block, shape) { + rewrite_block(block, Some(&expr.attrs), context, shape) + } else if let rw @ Some(_) = + rewrite_empty_block(context, block, Some(&expr.attrs), "", shape) + { // Rewrite block without trying to put it in a single line. rw } else { let prefix = block_prefix(context, block, shape)?; - rewrite_block_with_visitor(context, &prefix, block, shape, true) + rewrite_block_with_visitor( + context, + &prefix, + block, + Some(&expr.attrs), + shape, + true, + ) } } - ExprType::SubExpression => block.rewrite(context, shape), + ExprType::SubExpression => rewrite_block(block, Some(&expr.attrs), context, shape), } } ast::ExprKind::Match(ref cond, ref arms) => { @@ -290,7 +299,9 @@ pub fn format_expr( Some(context.snippet(expr.span).to_owned()) } ast::ExprKind::Catch(ref block) => { - if let rw @ Some(_) = rewrite_single_line_block(context, "do catch ", block, shape) { + if let rw @ Some(_) = + rewrite_single_line_block(context, "do catch ", block, Some(&expr.attrs), shape) + { rw } else { // 9 = `do catch ` @@ -298,7 +309,12 @@ pub fn format_expr( Some(format!( "{}{}", "do catch ", - block.rewrite(context, Shape::legacy(budget, shape.indent))? + rewrite_block( + block, + Some(&expr.attrs), + context, + Shape::legacy(budget, shape.indent) + )? )) } } @@ -347,7 +363,7 @@ where let lhs_result = lhs.rewrite(context, lhs_shape) .map(|lhs_str| format!("{}{}", pp.prefix, lhs_str))?; - // Try to the both lhs and rhs on the same line. + // Try to put both lhs and rhs on the same line. let rhs_orig_result = shape .offset_left(last_line_width(&lhs_result) + pp.infix.len()) .and_then(|s| s.sub_width(pp.suffix.len())) @@ -564,11 +580,17 @@ fn nop_block_collapse(block_str: Option, budget: usize) -> Option, + prefix: &str, shape: Shape, ) -> Option { + if attrs.map_or(false, |a| !inner_attributes(a).is_empty()) { + return None; + } + if block.stmts.is_empty() && !block_contains_comment(block, context.codemap) && shape.width >= 2 { - return Some("{}".to_owned()); + return Some(format!("{}{{}}", prefix)); } // If a block contains only a single-line comment, then leave it on one line. @@ -579,7 +601,7 @@ fn rewrite_empty_block( if block.stmts.is_empty() && !comment_str.contains('\n') && !comment_str.starts_with("//") && comment_str.len() + 4 <= shape.width { - return Some(format!("{{ {} }}", comment_str)); + return Some(format!("{}{{ {} }}", prefix, comment_str)); } } @@ -618,9 +640,10 @@ fn rewrite_single_line_block( context: &RewriteContext, prefix: &str, block: &ast::Block, + attrs: Option<&[ast::Attribute]>, shape: Shape, ) -> Option { - if is_simple_block(block, context.codemap) { + if is_simple_block(block, attrs, context.codemap) { let expr_shape = shape.offset_left(last_line_width(prefix))?; let expr_str = block.stmts[0].rewrite(context, expr_shape)?; let result = format!("{}{{ {} }}", prefix, expr_str); @@ -635,10 +658,11 @@ pub fn rewrite_block_with_visitor( context: &RewriteContext, prefix: &str, block: &ast::Block, + attrs: Option<&[ast::Attribute]>, shape: Shape, has_braces: bool, ) -> Option { - if let rw @ Some(_) = rewrite_empty_block(context, block, shape) { + if let rw @ Some(_) = rewrite_empty_block(context, block, attrs, prefix, shape) { return rw; } @@ -654,31 +678,41 @@ pub fn rewrite_block_with_visitor( ast::BlockCheckMode::Default => visitor.last_pos = block.span.lo(), } - visitor.visit_block(block, None, has_braces); + let inner_attrs = attrs.map(inner_attributes); + visitor.visit_block(block, inner_attrs.as_ref().map(|a| &**a), has_braces); 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 let rw @ Some(_) = rewrite_empty_block(context, self, shape) { - return rw; - } + rewrite_block(self, None, context, shape) + } +} - let prefix = block_prefix(context, self, shape)?; +fn rewrite_block( + block: &ast::Block, + attrs: Option<&[ast::Attribute]>, + context: &RewriteContext, + shape: Shape, +) -> Option { + let prefix = block_prefix(context, block, shape)?; - let result = rewrite_block_with_visitor(context, &prefix, self, shape, true); - if let Some(ref result_str) = result { - if result_str.lines().count() <= 3 { - if let rw @ Some(_) = rewrite_single_line_block(context, &prefix, self, shape) { - return rw; - } + // shape.width is used only for the single line case: either the empty block `{}`, + // or an unsafe expression `unsafe { e }`. + if let rw @ Some(_) = rewrite_empty_block(context, block, attrs, &prefix, shape) { + return rw; + } + + let result = rewrite_block_with_visitor(context, &prefix, block, attrs, shape, true); + if let Some(ref result_str) = result { + if result_str.lines().count() <= 3 { + if let rw @ Some(_) = rewrite_single_line_block(context, &prefix, block, attrs, shape) { + return rw; } } - - result } + + result } impl Rewrite for ast::Stmt { @@ -889,8 +923,8 @@ impl<'a> ControlFlow<'a> { let fixed_cost = self.keyword.len() + " { } else { }".len(); if let ast::ExprKind::Block(ref else_node) = else_block.node { - if !is_simple_block(self.block, context.codemap) - || !is_simple_block(else_node, context.codemap) + if !is_simple_block(self.block, None, context.codemap) + || !is_simple_block(else_node, None, context.codemap) || pat_expr_str.contains('\n') { return None; @@ -1123,7 +1157,7 @@ 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 = - rewrite_block_with_visitor(&block_context, "", self.block, block_shape, true)?; + rewrite_block_with_visitor(&block_context, "", self.block, None, block_shape, true)?; let mut result = format!("{}{}", cond_str, block_str); @@ -1234,22 +1268,39 @@ pub fn block_contains_comment(block: &ast::Block, codemap: &CodeMap) -> bool { contains_comment(&snippet) } -// Checks that a block contains no statements, an expression and no comments. +// Checks that a block contains no statements, an expression and no comments or +// attributes. // FIXME: incorrectly returns false when comment is contained completely within // the expression. -pub fn is_simple_block(block: &ast::Block, codemap: &CodeMap) -> bool { +pub fn is_simple_block( + block: &ast::Block, + attrs: Option<&[ast::Attribute]>, + codemap: &CodeMap, +) -> bool { (block.stmts.len() == 1 && stmt_is_expr(&block.stmts[0]) - && !block_contains_comment(block, codemap)) + && !block_contains_comment(block, codemap) && attrs.map_or(true, |a| a.is_empty())) } -/// Checks whether a block contains at most one statement or expression, and no comments. -pub fn is_simple_block_stmt(block: &ast::Block, codemap: &CodeMap) -> bool { +/// Checks whether a block contains at most one statement or expression, and no +/// comments or attributes. +pub fn is_simple_block_stmt( + block: &ast::Block, + attrs: Option<&[ast::Attribute]>, + codemap: &CodeMap, +) -> bool { block.stmts.len() <= 1 && !block_contains_comment(block, codemap) + && attrs.map_or(true, |a| a.is_empty()) } -/// Checks whether a block contains no statements, expressions, or comments. -pub fn is_empty_block(block: &ast::Block, codemap: &CodeMap) -> bool { +/// Checks whether a block contains no statements, expressions, comments, or +/// inner attributes. +pub fn is_empty_block( + block: &ast::Block, + attrs: Option<&[ast::Attribute]>, + codemap: &CodeMap, +) -> bool { block.stmts.is_empty() && !block_contains_comment(block, codemap) + && attrs.map_or(true, |a| inner_attributes(a).is_empty()) } pub fn stmt_is_expr(stmt: &ast::Stmt) -> bool { @@ -1577,7 +1628,8 @@ fn rewrite_match_pattern( fn flatten_arm_body<'a>(context: &'a RewriteContext, body: &'a ast::Expr) -> (bool, &'a ast::Expr) { match body.node { ast::ExprKind::Block(ref block) - if !is_unsafe_block(block) && is_simple_block(block, context.codemap) => + if !is_unsafe_block(block) + && is_simple_block(block, Some(&body.attrs), context.codemap) => { if let ast::StmtKind::Expr(ref expr) = block.stmts[0].node { ( @@ -1605,7 +1657,10 @@ fn rewrite_match_body( ) -> Option { let (extend, body) = flatten_arm_body(context, body); let (is_block, is_empty_block) = if let ast::ExprKind::Block(ref block) = body.node { - (true, is_empty_block(block, context.codemap)) + ( + true, + is_empty_block(block, Some(&body.attrs), context.codemap), + ) } else { (false, false) }; diff --git a/src/items.rs b/src/items.rs index b639697194b7..6306767f00c2 100644 --- a/src/items.rs +++ b/src/items.rs @@ -301,6 +301,7 @@ impl<'a> FmtVisitor<'a> { fn_sig: &FnSig, span: Span, block: &ast::Block, + inner_attrs: Option<&[ast::Attribute]>, ) -> Option { let context = self.get_context(); @@ -329,7 +330,8 @@ impl<'a> FmtVisitor<'a> { result.push(' '); } - self.single_line_fn(&result, block).or_else(|| Some(result)) + self.single_line_fn(&result, block, inner_attrs) + .or_else(|| Some(result)) } pub fn rewrite_required_fn( @@ -360,20 +362,25 @@ impl<'a> FmtVisitor<'a> { Some(result) } - fn single_line_fn(&self, fn_str: &str, block: &ast::Block) -> Option { - if fn_str.contains('\n') { + fn single_line_fn( + &self, + fn_str: &str, + block: &ast::Block, + inner_attrs: Option<&[ast::Attribute]>, + ) -> Option { + if fn_str.contains('\n') || inner_attrs.map_or(false, |a| !a.is_empty()) { return None; } let codemap = self.get_context().codemap; - if self.config.empty_item_single_line() && is_empty_block(block, codemap) + if self.config.empty_item_single_line() && is_empty_block(block, None, codemap) && self.block_indent.width() + fn_str.len() + 2 <= self.config.max_width() { return Some(format!("{}{{}}", fn_str)); } - if self.config.fn_single_line() && is_simple_block_stmt(block, codemap) { + if self.config.fn_single_line() && is_simple_block_stmt(block, None, codemap) { let rewrite = { if let Some(stmt) = block.stmts.first() { match stmt_expr(stmt) { diff --git a/src/visitor.rs b/src/visitor.rs index f1b50e19b610..81f35d787ab8 100644 --- a/src/visitor.rs +++ b/src/visitor.rs @@ -268,6 +268,7 @@ impl<'b, 'a: 'b> FmtVisitor<'a> { &FnSig::from_fn_kind(&fk, generics, fd, defaultness), mk_sp(s.lo(), b.span.lo()), b, + inner_attrs, ) } visit::FnKind::Closure(_) => unreachable!(), @@ -381,13 +382,14 @@ impl<'b, 'a: 'b> FmtVisitor<'a> { self.visit_static(&StaticParts::from_item(item)); } ast::ItemKind::Fn(ref decl, unsafety, constness, abi, ref generics, ref body) => { + let inner_attrs = inner_attributes(&item.attrs); self.visit_fn( visit::FnKind::ItemFn(item.ident, unsafety, constness, abi, &item.vis, body), generics, decl, item.span, ast::Defaultness::Final, - Some(&item.attrs), + Some(&inner_attrs), ) } ast::ItemKind::Ty(ref ty, ref generics) => { @@ -438,13 +440,14 @@ impl<'b, 'a: 'b> FmtVisitor<'a> { self.push_rewrite(ti.span, rewrite); } ast::TraitItemKind::Method(ref sig, Some(ref body)) => { + let inner_attrs = inner_attributes(&ti.attrs); self.visit_fn( visit::FnKind::Method(ti.ident, sig, None, body), &ti.generics, &sig.decl, ti.span, ast::Defaultness::Final, - Some(&ti.attrs), + Some(&inner_attrs), ); } ast::TraitItemKind::Type(ref type_param_bounds, ref type_default) => { @@ -473,13 +476,14 @@ impl<'b, 'a: 'b> FmtVisitor<'a> { match ii.node { ast::ImplItemKind::Method(ref sig, ref body) => { + let inner_attrs = inner_attributes(&ii.attrs); self.visit_fn( visit::FnKind::Method(ii.ident, sig, Some(&ii.vis), body), &ii.generics, &sig.decl, ii.span, ii.defaultness, - Some(&ii.attrs), + Some(&inner_attrs), ); } ast::ImplItemKind::Const(..) => self.visit_static(&StaticParts::from_impl_item(ii)), diff --git a/tests/target/attrib-block-expr.rs b/tests/target/attrib-block-expr.rs new file mode 100644 index 000000000000..1e9557dc039e --- /dev/null +++ b/tests/target/attrib-block-expr.rs @@ -0,0 +1,58 @@ +fn issue_2073() { + let x = { + #![my_attr] + do_something() + }; + + let x = #[my_attr] + { + do_something() + }; + + let x = #[my_attr] + {}; + + { + #![just_an_attribute] + }; + + let z = #[attr1] + #[attr2] + { + body() + }; + + x = |y| { + #![inner] + }; + + x = |y| #[outer] + {}; + + x = |y| { + //! ynot + }; + + x = |y| #[outer] + unsafe {}; + + let x = unsafe { + #![my_attr] + do_something() + }; + + let x = #[my_attr] + unsafe { + do_something() + }; + + // This is a dumb but possible case + let x = #[my_attr] + unsafe {}; + + x = |y| #[outer] + #[outer2] + unsafe { + //! Comment + }; +}