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

Fix(rome_js_formatter):🐛 fix parenthesis expression bad case #2564

Merged
merged 5 commits into from
May 11, 2022
Merged
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
Expand Up @@ -5,8 +5,8 @@ use crate::{
};
use rome_formatter::FormatResult;
use rome_js_syntax::{
JsAnyExpression, JsParenthesizedExpression, JsParenthesizedExpressionFields,
JsStringLiteralExpression, JsSyntaxKind, JsSyntaxNode,
JsAnyExpression, JsAnyLiteralExpression, JsParenthesizedExpression,
JsParenthesizedExpressionFields, JsStringLiteralExpression, JsSyntaxKind, JsSyntaxNode,
};
use rome_rowan::{AstNode, SyntaxResult};

Expand Down Expand Up @@ -99,13 +99,37 @@ fn is_simple_parenthesized_expression(node: &JsParenthesizedExpression) -> Synta
fn parenthesis_can_be_omitted(node: &JsParenthesizedExpression) -> SyntaxResult<bool> {
let expression = node.expression()?;
let parent = node.syntax().parent();

// if expression is a StringLiteralExpression, we need to check it before precedence comparison, here is an example:
// ```js
// a[("test")]
// ```
// if we use precedence comparison, we will get:
// parent_precedence should be `High` due to the parenthesized_expression's parent is ComputedMemberExpression,
// and node_precedence should be `Low` due to expression is StringLiteralExpression. `parent_precedence > node_precedence` will return false,
// the parenthesis will not be omitted.
// But the expected behavior is that the parenthesis will be omitted. The code above should be formatted as:
// ```js
// a["test"]
// ```
// So we need to add extra branch to handle this case.
if matches!(
expression,
JsAnyExpression::JsAnyLiteralExpression(JsAnyLiteralExpression::JsStringLiteralExpression(
_
))
) {
return Ok(!matches!(
parent.map(|p| p.kind()),
Some(JsSyntaxKind::JS_EXPRESSION_STATEMENT)
));
}
let parent_precedence = FormatPrecedence::with_precedence_for_parenthesis(parent.as_ref());
let node_precedence = FormatPrecedence::with_precedence_for_parenthesis(Some(node.syntax()));

if parent_precedence > node_precedence {
return Ok(false);
}

// Here we handle cases where we have binary/logical expressions.
// We want to remove the parenthesis only in cases where `left` and `right` are not other
// binary/logical expressions.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,4 @@ nock(/test/)
[httpMethodNock(method)]('/foo')
.reply(200, {
foo: 'bar',
});

a[("test")]
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 remove this, due to this case has been added in another test file.

});
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,6 @@ nock(/test/)
.reply(200, {
foo: 'bar',
});

a[("test")]
=============================
# Outputs
## Output 1
Expand All @@ -24,5 +22,3 @@ nock(/test/)
.matchHeader("Accept", "application/json")[httpMethodNock(method)]("/foo")
.reply(200, { foo: "bar" });

a[("test")];

Original file line number Diff line number Diff line change
Expand Up @@ -14,3 +14,6 @@ x.very.long.chain.of.static.members.that.goes.on.for.ever.I.mean.it.for.ever["an
x.very.long.chain.of.static.members.that.goes.on.for.ever.I.mean.it.for.ever[and()].even.longer.than.that.and["rofefefeefefme"].doesnt.supportit

x[b["test"]]


a[("test")]
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
---
source: crates/rome_js_formatter/tests/spec_test.rs
assertion_line: 242
expression: computed_member.js
---
# Input
Expand All @@ -20,6 +21,9 @@ x.very.long.chain.of.static.members.that.goes.on.for.ever.I.mean.it.for.ever[and

x[b["test"]]


a[("test")]

=============================
# Outputs
## Output 1
Expand Down Expand Up @@ -53,3 +57,5 @@ x.very.long.chain.of.static.members.that.goes.on.for.ever.I.mean.it.for.ever[

x[b["test"]];

a["test"];