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.

Sign up for GitHub

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

Jump to bottom

filter empty statements when checking block emptiness #3878

Merged
merged 2 commits into from
Oct 26, 2019
Merged

filter empty statements when checking block emptiness #3878

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9c84d17
fix: checking block emptiness
calebcartwright Oct 21, 2019
d87f961
chore: resovle merge conflicts
calebcartwright Oct 25, 2019
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
17 changes: 15 additions & 2 deletions src/expr.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -421,7 +421,7 @@ fn rewrite_empty_block(
prefix: &str,
shape: Shape,
) -> Option<String> {
if !block.stmts.is_empty() {
if block_has_statements(&block) {
return None;
}

Expand Down Expand Up @@ -1165,14 +1165,27 @@ pub(crate) fn is_simple_block_stmt(
&& attrs.map_or(true, |a| a.is_empty())
}

fn block_has_statements(block: &ast::Block) -> bool {
block.stmts.iter().any(|stmt| {
if let ast::StmtKind::Semi(ref expr) = stmt.kind {
if let ast::ExprKind::Tup(ref tup_exprs) = expr.kind {
if tup_exprs.is_empty() {
return false;
}
}
}
true
})
}

/// Checks whether a block contains no statements, expressions, comments, or
/// inner attributes.
pub(crate) fn is_empty_block(
context: &RewriteContext<'_>,
block: &ast::Block,
attrs: Option<&[ast::Attribute]>,
) -> bool {
block.stmts.is_empty()
!block_has_statements(&block)
&& !block_contains_comment(context, block)
&& attrs.map_or(true, |a| inner_attributes(a).is_empty())
}
Expand Down
14 changes: 7 additions & 7 deletions tests/source/control-brace-style-always-next-line.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,33 +2,33 @@

fn main() {
loop {
();
();
let foo = ();
Copy link
Contributor

Choose a reason for hiding this comment

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

@calebcartwright Why do we need to update all the tests?

Copy link
Member Author

@calebcartwright calebcartwright Oct 21, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Sorry i meant to leave a note on that. Apparently both ; and (); are parsed as empty tuple exprs in the block statements, so with the updated logic filtering out empty statements, these blocks were now being detected as empty.

    loop {
        ();
        ();
        }

would be flattened to

    loop {}

which was making these tests fail since they're trying to validate brace placement. the updates were just to convert to non-empty statements in those blocks to prevent the flattening.

Is this the correct behavior? Should we treat a block that only has a (); statement the same as one that only has a ; (both as empty blocks)?

If not I can do some minor refactoring to get the SnippetProvider available when checking the block statements, and in the event of the empty tuple grab the associated snippet and check for ; vs ();

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for your explanation!

Is this the correct behavior? Should we treat a block that only has a (); statement the same as one that only has a ; (both as empty blocks)?

Removing (); seems fine to me, though I am afraid that we may need to version-gate this as we didn't use to remove it. As we will be preparing for 2.0 release in the near future, I would like to merge this PR after we released 1.4.10 so that we don't need to add an extra version-gate which will be removed soon.

Copy link
Member Author

Choose a reason for hiding this comment

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

SGTM!

let bar = ();
}


'label: loop // loop comment
'label: loop // loop comment
{
();
let foo = ();
}


cond = true;
while cond {
();
let foo = ();
}


'while_label: while cond { // while comment
();
let foo = ();
}


for obj in iter {
for sub_obj in obj
{
'nested_while_label: while cond {
();
let foo = ();
}
}
}
Expand Down
14 changes: 7 additions & 7 deletions tests/source/control-brace-style-always-same-line.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,32 +1,32 @@
fn main() {
loop {
();
();
let foo = ();
let bar = ();
}


'label: loop // loop comment
'label: loop // loop comment
{
();
let foo = ();
}


cond = true;
while cond {
();
let foo = ();
}


'while_label: while cond { // while comment
();
let foo = ();
}


for obj in iter {
for sub_obj in obj
{
'nested_while_label: while cond {
();
let foo = ();
}
}
}
Expand Down
32 changes: 16 additions & 16 deletions tests/source/else-if-brace-style-always-next-line.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,14 @@
fn main() {
if false
{
();
();
let foo = ();
let bar = ();
}

if false // lone if comment
{
();
();
let foo = ();
let bar = ();
}


Expand All @@ -26,29 +26,29 @@ fn main() {

if true
{
();
let foo = ();
} else if false {
();
();
let foo = ();
let bar = ();
}
else {
();
();
();
let foo = ();
let bar = ();
let baz = ();
}

if true // else-if-chain if comment
{
();
let foo = ();
}
else if false // else-if-chain else-if comment
{
();
();
let foo = ();
let bar = ();
} else // else-if-chain else comment
{
();
();
();
let foo = ();
let bar = ();
let baz = ();
}
}
32 changes: 16 additions & 16 deletions tests/source/else-if-brace-style-always-same-line.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,14 @@
fn main() {
if false
{
();
();
let foo = ();
let bar = ();
}

if false // lone if comment
{
();
();
let foo = ();
let bar = ();
}


Expand All @@ -24,29 +24,29 @@ fn main() {

if true
{
();
let foo = ();
} else if false {
();
();
let foo = ();
let bar = ();
}
else {
();
();
();
let foo = ();
let bar = ();
let baz = ();
}

if true // else-if-chain if comment
{
();
let foo = ();
}
else if false // else-if-chain else-if comment
{
();
();
let foo = ();
let bar = ();
} else // else-if-chain else comment
{
();
();
();
let foo = ();
let bar = ();
let baz = ();
}
}
32 changes: 16 additions & 16 deletions tests/source/else-if-brace-style-closing-next-line.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,14 @@
fn main() {
if false
{
();
();
let foo = ();
let bar = ();
}

if false // lone if comment
{
();
();
let foo = ();
let bar = ();
}


Expand All @@ -26,29 +26,29 @@ fn main() {

if true
{
();
let foo = ();
} else if false {
();
();
let foo = ();
let bar = ();
}
else {
();
();
();
let foo = ();
let bar = ();
let baz = ();
}

if true // else-if-chain if comment
{
();
let foo = ();
}
else if false // else-if-chain else-if comment
{
();
();
let foo = ();
let bar = ();
} else // else-if-chain else comment
{
();
();
();
let foo = ();
let bar = ();
let baz = ();
}
}
13 changes: 13 additions & 0 deletions tests/source/issue_3868.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
fn foo() {
;
}

fn bar() {
for _ in 0..1 {
;
}
}

fn baz() {
();
}
12 changes: 6 additions & 6 deletions tests/target/control-brace-style-always-next-line.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,26 +3,26 @@
fn main() {
loop
{
();
();
let foo = ();
let bar = ();
}

'label: loop
// loop comment
{
();
let foo = ();
}

cond = true;
while cond
{
();
let foo = ();
}

'while_label: while cond
{
// while comment
();
let foo = ();
}

for obj in iter
Expand All @@ -31,7 +31,7 @@ fn main() {
{
'nested_while_label: while cond
{
();
let foo = ();
}
}
}
Expand Down
12 changes: 6 additions & 6 deletions tests/target/control-brace-style-always-same-line.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,29 +1,29 @@
fn main() {
loop {
();
();
let foo = ();
let bar = ();
}

'label: loop
// loop comment
{
();
let foo = ();
}

cond = true;
while cond {
();
let foo = ();
}

'while_label: while cond {
// while comment
();
let foo = ();
}

for obj in iter {
for sub_obj in obj {
'nested_while_label: while cond {
();
let foo = ();
}
}
}
Expand Down
Loading