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

fix(rome_js_formatter): change parenthesized expression formatting #2636

Closed
wants to merge 2 commits into from
Closed

fix(rome_js_formatter): change parenthesized expression formatting #2636

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
9153b68
fix(rome_js_formatter): change parenthesized expression indentation
yassere May 29, 2022
be702ce
Update snapshots
yassere May 31, 2022
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
47 changes: 25 additions & 22 deletions crates/rome_js_formatter/src/js/expressions/parenthesized_expression.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ use crate::utils::{is_simple_expression, FormatPrecedence};
use crate::FormatNodeFields;
use rome_js_syntax::{
JsAnyExpression, JsAnyLiteralExpression, JsParenthesizedExpression,
JsParenthesizedExpressionFields, JsStringLiteralExpression, JsSyntaxKind, JsSyntaxNode,
JsParenthesizedExpressionFields, JsSyntaxKind, JsSyntaxNode,
};
use rome_rowan::{AstNode, SyntaxResult};

Expand Down Expand Up @@ -50,27 +50,7 @@ impl FormatNodeFields<JsParenthesizedExpression> for FormatNodeRule<JsParenthesi
formatter.format_replaced(&r_paren_token?, empty_element()),
]
]
}
// if the expression inside the parenthesis is a stringLiteralExpression, we should leave it as is rather than
// add extra soft_block_indent, for example:
// ```js
// ("escaped carriage return \
// ");
// ```
// if we add soft_block_indent, we will get:
// ```js
// (
// "escaped carriage return \
// "
// );
// ```
// which will not match prettier's formatting behavior, if we add this extra branch to handle this case, it become:
// ```js
// ("escaped carriage return \
// ");
// ```
// this is what we want
else if JsStringLiteralExpression::can_cast(expression.syntax().kind()) {
} else if !requires_soft_block_indent(node)? {
formatted![
formatter,
[
Expand Down Expand Up @@ -174,3 +154,26 @@ fn not_binaryish_expression(node: &JsSyntaxNode) -> bool {
JsSyntaxKind::JS_BINARY_EXPRESSION | JsSyntaxKind::JS_LOGICAL_EXPRESSION
)
}

fn requires_soft_block_indent(node: &JsParenthesizedExpression) -> SyntaxResult<bool> {
let expression_kind = node.expression()?.syntax().kind();

let requires_indent = match expression_kind {
// Never block indent a parenthesized multiline string literal
JsSyntaxKind::JS_STRING_LITERAL_EXPRESSION => false,
// Only soft block ident a parenthesized sequence expression when it's
// the child of specific node types
JsSyntaxKind::JS_SEQUENCE_EXPRESSION => {
let parent_kind = node.syntax().parent().map(|p| p.kind());

matches!(
parent_kind,
Some(JsSyntaxKind::JS_RETURN_STATEMENT)
| Some(JsSyntaxKind::JS_UNARY_EXPRESSION)
| Some(JsSyntaxKind::JS_ARROW_FUNCTION_EXPRESSION)
)
}
_ => true,
};
Ok(requires_indent)
Comment on lines +159 to +178
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: It should be possible to match directly on the node rather than going over syntax

match node.expression() {
  JsAnyExpression::JsStringLiteralExpression(_) => false,
  ...
}

}
23 changes: 4 additions & 19 deletions crates/rome_js_formatter/src/js/expressions/sequence_expression.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,25 +16,10 @@ impl FormatNodeFields<JsSequenceExpression> for FormatNodeRule<JsSequenceExpress
// Return statement already does the indentation for us
// Arrow function body can't have a sequence expression unless it's parenthesized, otherwise
// would be a syntax error
if matches!(parent.kind(), JsSyntaxKind::JS_RETURN_STATEMENT) {
true
} else if matches!(parent.kind(), JsSyntaxKind::JS_PARENTHESIZED_EXPRESSION) {
// In case we are inside a sequence expression, we have to go up a level and see the great parent.
// Arrow function body and return statements applying indentation for us, so we signal the
// sequence expression to not add other indentation levels
let great_parent = parent.parent().map(|gp| gp.kind());

matches!(
great_parent,
Some(
JsSyntaxKind::JS_ARROW_FUNCTION_EXPRESSION
| JsSyntaxKind::JS_RETURN_STATEMENT
| JsSyntaxKind::JS_PROPERTY_OBJECT_MEMBER
)
)
} else {
false
}
matches!(
parent.kind(),
JsSyntaxKind::JS_RETURN_STATEMENT | JsSyntaxKind::JS_PARENTHESIZED_EXPRESSION
)
Comment on lines -19 to +22
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I’m uncertain about this change, but it seems to increase Prettier alignment in our existing snapshots.
@ematipico What sort of cases was this condition intended to handle?

Copy link
Contributor

@ematipico ematipico Jun 3, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Cases like this, but as you're changing the logic of the parenthesis, maybe it's not needed anymore.

});

// Find the left most sequence expression
Expand Down
6 changes: 5 additions & 1 deletion crates/rome_js_formatter/src/utils/binary_like_expression.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -264,7 +264,11 @@ fn should_not_indent_if_parent_indents(current_node: &JsSyntaxNode) -> bool {

matches!(
parent_kind,
Some(JsSyntaxKind::JS_RETURN_STATEMENT | JsSyntaxKind::JS_ARROW_FUNCTION_EXPRESSION)
Some(
JsSyntaxKind::JS_RETURN_STATEMENT
| JsSyntaxKind::JS_ARROW_FUNCTION_EXPRESSION
| JsSyntaxKind::JS_PARENTHESIZED_EXPRESSION
)
)
}

Expand Down
34 changes: 15 additions & 19 deletions crates/rome_js_formatter/tests/specs/js/module/expression/sequence_expression.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
---
source: crates/rome_js_formatter/tests/spec_test.rs
assertion_line: 244
assertion_line: 257
expression: sequence_expression.js
---
# Input
Expand Down Expand Up @@ -51,15 +51,13 @@ const f = () => (
____________third
);

(
____________first,
____________second,
____________third,
____________third,
____________third,
____________third,
____________third
);
(____________first,
____________second,
Comment on lines +54 to +55
Copy link
Contributor

Choose a reason for hiding this comment

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

Wasn't this case handled by the code that you have now removed (and mentioned ema?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this case, Prettier would just omit the parentheses. But if you force the parentheses to remain like in your original example, Prettier doesn't block indent the sequence expression. The indentation in your example (copied below) came from the binary expression.

(aaaaaaaaaaaaaaaaaaaaaaaaa +
  bbbbbbbbbbbbbbbbbbbbbbbbb +
  ccccccccccccccccccccccccc +
  ddddddddddddddddddddddddd +
  eeeeeeeeeeeeeeeeeeeeeeeee,
"test")();

Another demonstration is when you assign a sequence expression. I'll add a test case for that.

Before this PR:

foo =
	(
		____________first,
			____________second,
			____________third,
			____________third,
			____________third,
			____________third,
			____________third
	);

After this PR and also Prettier:

foo =
	(____________first,
	____________second,
	____________third,
	____________third,
	____________third,
	____________third,
	____________third);

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I don't know. Given what we have and what prettier does, do we really need to match it? Doesn't seem that good honestly

Copy link
Contributor

Choose a reason for hiding this comment

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

I"m always in favor of deleting code :D I don't think it's that terrible, nice and compact. But to phrase this differently. I don't think it's that bad that it's reason enough for us to diverge, especially if it means we can delete code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I expect that this sort of code is uncommon enough in the wild that we don't need to be terribly concerned with how it looks, so I'm also in favor of just simplifying our rules here and matching Prettier. If we ever discover enough of an impact on real-world projects, we can reconsider in the future.

____________third,
____________third,
____________third,
____________third,
____________third);

function a() {
return (
Expand All @@ -74,15 +72,13 @@ function a() {
}

const object = {
something: (
____________first,
____________second,
____________third,
____________third,
____________third,
____________third,
____________third
),
something: (____________first,
____________second,
____________third,
____________third,
____________third,
____________third,
____________third),
};
Comment on lines +75 to 82
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This isn’t really an improvement, but trying to fix it in this PR could conflict with #2627

Our current formatting for this already doesn’t match Prettier’s though.


aLongIdentifierName,
Expand Down
35 changes: 17 additions & 18 deletions crates/rome_js_formatter/tests/specs/prettier/js/binary-expressions/call.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,7 @@
---
source: crates/rome_js_formatter/tests/prettier_tests.rs
assertion_line: 125
assertion_line: 182
expression: call.js

---
# Input
```js
Expand Down Expand Up @@ -78,38 +77,38 @@ expression: call.js
```js
(
aaaaaaaaaaaaaaaaaaaaaaaaa &&
bbbbbbbbbbbbbbbbbbbbbbbbb &&
ccccccccccccccccccccccccc &&
ddddddddddddddddddddddddd &&
eeeeeeeeeeeeeeeeeeeeeeeee
bbbbbbbbbbbbbbbbbbbbbbbbb &&
ccccccccccccccccccccccccc &&
ddddddddddddddddddddddddd &&
eeeeeeeeeeeeeeeeeeeeeeeee
)();

(aa && bb && cc && dd && ee)();

(
aaaaaaaaaaaaaaaaaaaaaaaaa +
bbbbbbbbbbbbbbbbbbbbbbbbb +
ccccccccccccccccccccccccc +
ddddddddddddddddddddddddd +
eeeeeeeeeeeeeeeeeeeeeeeee
bbbbbbbbbbbbbbbbbbbbbbbbb +
ccccccccccccccccccccccccc +
ddddddddddddddddddddddddd +
eeeeeeeeeeeeeeeeeeeeeeeee
)();

(aa + bb + cc + dd + ee)();

(
aaaaaaaaaaaaaaaaaaaaaaaaa &&
bbbbbbbbbbbbbbbbbbbbbbbbb &&
ccccccccccccccccccccccccc &&
ddddddddddddddddddddddddd &&
eeeeeeeeeeeeeeeeeeeeeeeee
bbbbbbbbbbbbbbbbbbbbbbbbb &&
ccccccccccccccccccccccccc &&
ddddddddddddddddddddddddd &&
eeeeeeeeeeeeeeeeeeeeeeeee
)()()();

(
aaaaaaaaaaaaaaaaaaaaaaaaa &&
bbbbbbbbbbbbbbbbbbbbbbbbb &&
ccccccccccccccccccccccccc &&
ddddddddddddddddddddddddd &&
eeeeeeeeeeeeeeeeeeeeeeeee
bbbbbbbbbbbbbbbbbbbbbbbbb &&
ccccccccccccccccccccccccc &&
ddddddddddddddddddddddddd &&
eeeeeeeeeeeeeeeeeeeeeeeee
)(
aaaaaaaaaaaaaaaaaaaaaaaaa &&
bbbbbbbbbbbbbbbbbbbbbbbbb &&
Expand Down
19 changes: 10 additions & 9 deletions crates/rome_js_formatter/tests/specs/prettier/js/binary-expressions/comment.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
---
source: crates/rome_js_formatter/tests/prettier_tests.rs
assertion_line: 182
expression: comment.js
---
# Input
Expand Down Expand Up @@ -110,15 +111,15 @@ foo[

!(
a +
a + // comment
a +
!(
b +
b +
b + // comment
b +
b
)
a + // comment
a +
!(
b +
b +
b + // comment
b +
b
)
);

```
Expand Down
13 changes: 7 additions & 6 deletions .../rome_js_formatter/tests/specs/prettier/js/binary-expressions/inline-object-array.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
---
source: crates/rome_js_formatter/tests/prettier_tests.rs
assertion_line: 182
expression: inline-object-array.js
---
# Input
Expand Down Expand Up @@ -172,12 +173,12 @@ const create = () => {
const result = doSomething();
return (
shouldReturn &&
result.ok &&
{
status: "ok",
createdAt: result.createdAt,
updatedAt: result.updatedAt,
}
result.ok &&
{
status: "ok",
createdAt: result.createdAt,
updatedAt: result.updatedAt,
}
);
};

Expand Down
7 changes: 3 additions & 4 deletions crates/rome_js_formatter/tests/specs/prettier/js/member/logical.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,7 @@
---
source: crates/rome_js_formatter/tests/prettier_tests.rs
assertion_line: 125
assertion_line: 182
expression: logical.js

---
# Input
```js
Expand All @@ -18,8 +17,8 @@ expression: logical.js

(
veryLongVeryLongVeryLong ||
anotherVeryLongVeryLongVeryLong ||
veryVeryVeryLongError
anotherVeryLongVeryLongVeryLong ||
veryVeryVeryLongError
).prop;

```
Expand Down
9 changes: 5 additions & 4 deletions crates/rome_js_formatter/tests/specs/prettier/js/method-chain/logical.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
---
source: crates/rome_js_formatter/tests/prettier_tests.rs
assertion_line: 182
expression: logical.js
---
# Input
Expand Down Expand Up @@ -42,14 +43,14 @@ const someLongVariableName = (

(
veryLongVeryLongVeryLong ||
anotherVeryLongVeryLongVeryLong ||
veryVeryVeryLongError
anotherVeryLongVeryLongVeryLong ||
veryVeryVeryLongError
).map((tickets) => TicketRecord.createFromSomeLongString());

(
veryLongVeryLongVeryLong ||
anotherVeryLongVeryLongVeryLong ||
veryVeryVeryLongError
anotherVeryLongVeryLongVeryLong ||
veryVeryVeryLongError
).map(
(tickets) => TicketRecord.createFromSomeLongString(),
).filter((obj) => !!obj);
Expand Down
15 changes: 6 additions & 9 deletions crates/rome_js_formatter/tests/specs/prettier/js/objects/expression.js.snap
View file
Open in desktop
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: 181
assertion_line: 182
expression: expression.js
---
# Input
Expand Down Expand Up @@ -51,17 +51,14 @@ const a1 = {
};

const a2 = {
someKey: (
longLongLongLongLongLongLongLongLongLongLongLongLongLongName, shortName
),
someKey: (longLongLongLongLongLongLongLongLongLongLongLongLongLongName,
shortName),
};

const a3 = {
someKey: (
longLongLongLongLongLongLongLongLongLongLongLongLongLongName,
longLongLongLongLongLongLongLongLongLongLongLongLongLongName,
longLongLongLongLongLongLongLongLongLongLongLongLongLongName
),
someKey: (longLongLongLongLongLongLongLongLongLongLongLongLongLongName,
longLongLongLongLongLongLongLongLongLongLongLongLongLongName,
longLongLongLongLongLongLongLongLongLongLongLongLongLongName),
};

```
Expand Down
7 changes: 3 additions & 4 deletions crates/rome_js_formatter/tests/specs/prettier/js/optional-chaining/chaining.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,7 @@
---
source: crates/rome_js_formatter/tests/prettier_tests.rs
assertion_line: 144
assertion_line: 182
expression: chaining.js

---
# Input
```js
Expand Down Expand Up @@ -158,8 +157,8 @@ a?.[b ? c : d];
(a?.(x)).x;
(
aaaaaaaaaaaaaaaaaaaaaaaa &&
aaaaaaaaaaaaaaaaaaaaaaaa &&
aaaaaaaaaaaaaaaaaaaaaaaa
aaaaaaaaaaaaaaaaaaaaaaaa &&
aaaaaaaaaaaaaaaaaaaaaaaa
)?.();

let f = () => ({}?.());
Expand Down
Loading