Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement empty lines surrounding blocks #5969

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions Configurations.md
Original file line number Diff line number Diff line change
Expand Up @@ -1105,6 +1105,19 @@ macro_rules! foo {

See also [`format_macro_bodies`](#format_macro_bodies).

## `surround_blocks_with_empty_lines`

#### `false` (default):

```rust
// todo
```

#### `true`:

```rust
// todo
```

## `format_macro_bodies`

Expand Down
4 changes: 4 additions & 0 deletions src/config/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,9 @@ create_config! {
format_generated_files: bool, true, false, "Format generated files";
generated_marker_line_search_limit: usize, 5, false, "Number of lines to check for a \
`@generated` marker when `format_generated_files` is enabled";
surround_blocks_with_empty_lines: bool, false, false,
"Surround all block expressions with \
empty lines. This option is not stable and could be removed at any time.";

// Options that can change the source code beyond whitespace/blocks (somewhat linty things)
merge_derives: bool, true, true, "Merge multiple `#[derive(...)]` into a single one";
Expand Down Expand Up @@ -683,6 +686,7 @@ version = "One"
inline_attribute_width = 0
format_generated_files = true
generated_marker_line_search_limit = 5
surround_blocks_with_empty_lines = false
merge_derives = true
use_try_shorthand = false
use_field_init_shorthand = false
Expand Down
58 changes: 58 additions & 0 deletions src/expr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1203,6 +1203,64 @@ pub(crate) fn is_simple_block(
&& attrs.map_or(true, |a| a.is_empty())
}

pub(crate) fn contains_curly_block(expr: &ast::Expr) -> bool {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know if contains_curly_block is the right name for this function. Are you trying to add space around anything with curly braces or just all statements?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm open to suggestions for names.

Here's the "problem" that I've run into: I've been reviewing code that will have 50 lines without a single line between them. If one looks at any popular open-source repo (including rustfmt), one can see many more empty lines, breaking code up into logical chunks like paragraphs in prose. So the question is, how can a formatter that doesn't have logic context of the code add some of this spacing? Naturally, new scopes (demarcated by curly braces) provide an opportunity to add spacing.

So maybe the name should be something like has_new_scope? Or something along those lines...

match expr.kind {
ast::ExprKind::If(..)
| ast::ExprKind::While(..)
| ast::ExprKind::ForLoop { .. }
| ast::ExprKind::Loop(..)
| ast::ExprKind::Match(..)
| ast::ExprKind::Let(..)
| ast::ExprKind::Block(..)
| ast::ExprKind::TryBlock(..)
| ast::ExprKind::Try(..)
| ast::ExprKind::Gen(..) => true,
ast::ExprKind::Unary(_, ref expr)
| ast::ExprKind::Cast(ref expr, _)
| ast::ExprKind::Type(ref expr, _)
| ast::ExprKind::Await(ref expr, _)
| ast::ExprKind::Field(ref expr, _)
| ast::ExprKind::Become(ref expr)
| ast::ExprKind::Repeat(ref expr, _)
| ast::ExprKind::Paren(ref expr)
| ast::ExprKind::AddrOf(_, _, ref expr)
| ast::ExprKind::AssignOp(_, ref expr, ..) => contains_curly_block(expr),
ast::ExprKind::Closure(ref closure) => contains_curly_block(&closure.body),
ast::ExprKind::Binary(_, ref a, ref b)
| ast::ExprKind::Assign(ref a, ref b, _)
| ast::ExprKind::Index(ref a, ref b, _) => {
contains_curly_block(a) || contains_curly_block(b)
}
ast::ExprKind::Break(_, ref maybe_expr)
| ast::ExprKind::Ret(ref maybe_expr)
| ast::ExprKind::Yield(ref maybe_expr)
| ast::ExprKind::Yeet(ref maybe_expr) => {
maybe_expr.as_deref().map_or(false, contains_curly_block)
}
ast::ExprKind::Range(ref maybe_a, ref maybe_b, _) => maybe_a
.as_deref()
.or(maybe_b.as_deref())
.map_or(false, contains_curly_block),
ast::ExprKind::InlineAsm(..)
| ast::ExprKind::OffsetOf(..)
| ast::ExprKind::MacCall(..)
| ast::ExprKind::Struct(..)
| ast::ExprKind::Continue(..)
| ast::ExprKind::IncludedBytes(..)
| ast::ExprKind::FormatArgs(..)
| ast::ExprKind::Path(..)
| ast::ExprKind::Array(..)
| ast::ExprKind::ConstBlock(..)
| ast::ExprKind::Call(..)
| ast::ExprKind::MethodCall(..)
| ast::ExprKind::Tup(..)
| ast::ExprKind::Lit(..)
| ast::ExprKind::Underscore
| ast::ExprKind::Err(_)
| ast::ExprKind::Dummy => false,
}
}

/// Checks whether a block contains at most one statement or expression, and no
/// comments or attributes.
pub(crate) fn is_simple_block_stmt(
Expand Down
2 changes: 1 addition & 1 deletion src/items.rs
Original file line number Diff line number Diff line change
Expand Up @@ -489,7 +489,7 @@ impl<'a> FmtVisitor<'a> {
return None;
}

let res = Stmt::from_ast_node(block.stmts.first()?, true)
let res = Stmt::from_ast_node(block.stmts.first()?, true, true)
.rewrite(&self.get_context(), self.shape())?;

let width = self.block_indent.width() + fn_str.len() + res.len() + 5;
Expand Down
54 changes: 48 additions & 6 deletions src/stmt.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ use rustc_span::Span;

use crate::comment::recover_comment_removed;
use crate::config::Version;
use crate::expr::{format_expr, is_simple_block, ExprType};
use crate::expr::{contains_curly_block, format_expr, is_simple_block, ExprType};
use crate::rewrite::{Rewrite, RewriteContext};
use crate::shape::Shape;
use crate::source_map::LineRangeUtils;
Expand All @@ -12,6 +12,7 @@ use crate::utils::semicolon_for_stmt;

pub(crate) struct Stmt<'a> {
inner: &'a ast::Stmt,
is_first: bool,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to add is_first to get this to work?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my current implementation, yes. I'm not sure if there's a better way to do this, though. Open to suggestions.

is_last: bool,
}

Expand Down Expand Up @@ -41,15 +42,24 @@ impl<'a> Stmt<'a> {
if is_simple_block(context, block, attrs) {
let inner = &block.stmts[0];
// Simple blocks only contain one expr and no stmts
let is_first = true;
let is_last = true;
Some(Stmt { inner, is_last })
Some(Stmt {
inner,
is_first,
is_last,
})
} else {
None
}
}

pub(crate) fn from_ast_node(inner: &'a ast::Stmt, is_last: bool) -> Self {
Stmt { inner, is_last }
pub(crate) fn from_ast_node(inner: &'a ast::Stmt, is_first: bool, is_last: bool) -> Self {
Stmt {
inner,
is_first,
is_last,
}
}

pub(crate) fn from_ast_nodes<I>(iter: I) -> Vec<Self>
Expand All @@ -58,9 +68,19 @@ impl<'a> Stmt<'a> {
{
let mut result = vec![];
let mut iter = iter.peekable();
while iter.peek().is_some() {

if let Some(inner) = iter.next() {
result.push(Stmt {
inner: iter.next().unwrap(),
inner,
is_first: true,
is_last: iter.peek().is_none(),
})
}

while let Some(inner) = iter.next() {
result.push(Stmt {
inner,
is_first: false,
is_last: iter.peek().is_none(),
})
}
Expand All @@ -71,6 +91,14 @@ impl<'a> Stmt<'a> {
matches!(self.inner.kind, ast::StmtKind::Empty)
}

pub(crate) fn is_first(&self) -> bool {
self.is_first
}

pub(crate) fn is_last(&self) -> bool {
self.is_last
}

fn is_last_expr(&self) -> bool {
if !self.is_last {
return false;
Expand All @@ -86,6 +114,20 @@ impl<'a> Stmt<'a> {
_ => false,
}
}

pub(crate) fn is_block_with_curly_braces(&self) -> bool {
match self.as_ast_node().kind {
ast::StmtKind::Let(ref local) => match local.kind {
ast::LocalKind::Decl => false,
ast::LocalKind::Init(ref expr) => contains_curly_block(expr),
ast::LocalKind::InitElse(..) => true,
},
ast::StmtKind::Expr(ref expr) | ast::StmtKind::Semi(ref expr) => {
contains_curly_block(expr)
}
_ => false,
}
}
}

impl<'a> Rewrite for Stmt<'a> {
Expand Down
19 changes: 19 additions & 0 deletions src/visitor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -876,8 +876,27 @@ impl<'b, 'a: 'b> FmtVisitor<'a> {
.collect();

if items.is_empty() {
let stmt = &stmts[0];

let should_preced_with_empty_line = self.config.surround_blocks_with_empty_lines()
&& !self.buffer.ends_with('\n')
&& !stmt.is_first()
&& stmt.is_block_with_curly_braces();

if should_preced_with_empty_line {
self.push_str("\n");
}

self.visit_stmt(&stmts[0], include_current_empty_semi);

let should_follow_with_empty_line = self.config.surround_blocks_with_empty_lines()
&& !stmt.is_last()
&& stmt.is_block_with_curly_braces();

if should_follow_with_empty_line {
self.push_str("\n");
}

// FIXME(calebcartwright 2021-01-03) - This exists strictly to maintain legacy
// formatting where rustfmt would preserve redundant semicolons on Items in a
// statement position.
Expand Down
Loading
Loading