Skip to content
This repository has been archived by the owner on Aug 31, 2023. It is now read-only.

fix(rome_js_formatter): print empty statements as empty blocks #2504

Closed
wants to merge 2 commits into from
Closed
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
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use crate::format_traits::FormatOptional;
use crate::format_traits::{FormatOptional, FormatWith};
use rome_formatter::FormatResult;

use crate::{
Expand All @@ -21,7 +21,7 @@ impl FormatNode for JsDoWhileStatement {
semicolon_token,
} = self.as_fields();

let head = format_elements![do_token.format(formatter)?, space_token(),];
let head = do_token.format_with(formatter, |e| format_elements![e, space_token()])?;

let tail = format_elements![
space_token(),
Expand All @@ -42,6 +42,11 @@ impl FormatNode for JsDoWhileStatement {
body.format(formatter)?,
tail,
]))
} else if matches!(body, JsAnyStatement::JsEmptyStatement(_)) {
Ok(format_elements![
hard_group_elements(format_elements![head, body.format(formatter)?,]),
tail,
])
} else {
Ok(format_elements![
hard_group_elements(head),
Expand Down
20 changes: 15 additions & 5 deletions crates/rome_js_formatter/src/js/statements/empty_statement.rs
Original file line number Diff line number Diff line change
@@ -1,13 +1,23 @@
use crate::{empty_element, FormatElement, FormatNode, Formatter};
use rome_formatter::FormatResult;

use rome_js_syntax::JsEmptyStatement;
use rome_formatter::{format_elements, hard_line_break, token, FormatResult};
use rome_js_syntax::JsEmptyStatementFields;
use rome_js_syntax::{JsEmptyStatement, JsSyntaxKind};
use rome_rowan::AstNode;

impl FormatNode for JsEmptyStatement {
fn format_fields(&self, formatter: &Formatter) -> FormatResult<FormatElement> {
let JsEmptyStatementFields { semicolon_token } = self.as_fields();

Ok(formatter.format_replaced(&semicolon_token?, empty_element()))
let parent_kind = self.syntax().parent().map(|p| p.kind());
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: this pattern shows up a couple times. Should we have a function that gets the parent kind directly?

if matches!(
parent_kind,
Some(JsSyntaxKind::JS_DO_WHILE_STATEMENT)
| Some(JsSyntaxKind::JS_IF_STATEMENT)
| Some(JsSyntaxKind::JS_ELSE_CLAUSE)
) {
let new_body = format_elements![token("{"), hard_line_break(), token("}")];
Ok(formatter.format_replaced(&semicolon_token?, new_body))
} else {
Ok(formatter.format_replaced(&semicolon_token?, empty_element()))
}
}
}
16 changes: 3 additions & 13 deletions crates/rome_js_formatter/src/js/statements/if_statement.rs
Original file line number Diff line number Diff line change
@@ -1,12 +1,7 @@
use crate::format_traits::FormatOptional;
use crate::{
block_indent, concat_elements, group_elements, hard_group_elements, hard_line_break, token,
Format,
};
use rome_formatter::FormatResult;

use crate::{block_indent, concat_elements, group_elements, hard_group_elements, token, Format};
use crate::{format_elements, space_token, FormatElement, FormatNode, Formatter};

use rome_formatter::FormatResult;
use rome_js_syntax::JsSyntaxToken;
use rome_js_syntax::{JsAnyStatement, JsElseClauseFields, JsIfStatement};
use rome_js_syntax::{JsElseClause, JsIfStatementFields};
Expand Down Expand Up @@ -88,12 +83,7 @@ fn into_block(formatter: &Formatter, stmt: JsAnyStatement) -> FormatResult<Forma
// If the body is an empty statement, force a line break to ensure behavior
// is coherent with `is_non_collapsable_empty_block`
if matches!(stmt, JsAnyStatement::JsEmptyStatement(_)) {
return Ok(format_elements![
token("{"),
stmt.format(formatter)?,
hard_line_break(),
token("}")
]);
return Ok(format_elements![stmt.format(formatter)?]);
}

Ok(group_elements(format_elements![
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,4 +9,7 @@ do { // trailing
var foo = 4
}

while (something)
while (something)


do; while(true);
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
---
source: crates/rome_js_formatter/tests/spec_test.rs
assertion_line: 242
expression: do_while.js

---
# Input
do {
Expand All @@ -15,6 +17,9 @@ do { // trailing
}

while (something)


do; while(true);
=============================
# Outputs
## Output 1
Expand All @@ -32,3 +37,6 @@ do {
var foo = 4;
} while (something);

do;
while (true);

Original file line number Diff line number Diff line change
@@ -1,3 +1,6 @@
if(a);
if(a); else ;

if (Math.random() > 0.5) {
console.log(1)
} else if (Math.random() > 0.5) {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,8 +1,13 @@
---
source: crates/rome_js_formatter/tests/spec_test.rs
assertion_line: 242
expression: if_else.js

---
# Input
if(a);
if(a); else ;

if (Math.random() > 0.5) {
console.log(1)
} else if (Math.random() > 0.5) {
Expand Down Expand Up @@ -71,6 +76,12 @@ Indent style: Tab
Line width: 80
Quote style: Double Quotes
-----
if (a) {
}
if (a) {
} else {
}

if (Math.random() > 0.5) {
console.log(1);
} else if (Math.random() > 0.5) {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
---
source: crates/rome_js_formatter/tests/prettier_tests.rs
assertion_line: 144
assertion_line: 175
expression: body.js

---
Expand All @@ -27,7 +27,8 @@ for (;;);
while (1);
for (var i in o);
for (var i of o);
do while (1);
do {
} while (1);

```

Expand Down