fix(rome_js_formatter): use parenthesis #2538

ematipico · 2022-05-05T11:47:00Z

Summary

This is part of #2449

It's a long list so it won't be fixed in one single PR. Given this file: https://github.com/prettier/prettier/blob/a9de2a128cc8eea84ddd90efdc210378a894ab6b/src/language-js/needs-parens.js

One thing to note about the above file, is that it contains logic of syntax that is not official yet. Also, it contains some weird cases that are syntactically wrong, probably because they needed to cover some new syntax case. Given that, the progression on this task takes longer then expected

I fixed cases for:

classes
parenthesized expressions (removal of unnecessary parenthesis)
await expressions
some edge case with sequence expressions

I also did some work for the conditional expressions, although it can't be completed because at the moment we don't correctly format them.

Once we do, can add parenthesis to this case

Test Plan

I progressively added new test cases every time I covered a new case.

crates/rome_js_formatter/tests/specs/js/module/class/class.js.snap

github-actions · 2022-05-05T11:49:42Z

Parser conformance results on ubuntu-latest

js/262

Test result	`main` count	This PR count	Difference
Total	45878	45878	0
Passed	44938	44938	0
Failed	940	940	0
Panics	0	0	0
Coverage	97.95%	97.95%	0.00%

jsx/babel

Test result	`main` count	This PR count	Difference
Total	39	39	0
Passed	36	36	0
Failed	3	3	0
Panics	0	0	0
Coverage	92.31%	92.31%	0.00%

ts/babel

Test result	`main` count	This PR count	Difference
Total	588	588	0
Passed	519	519	0
Failed	69	69	0
Panics	0	0	0
Coverage	88.27%	88.27%	0.00%

ts/microsoft

Test result	`main` count	This PR count	Difference
Total	16257	16257	0
Passed	12391	12391	0
Failed	3866	3866	0
Panics	0	0	0
Coverage	76.22%	76.22%	0.00%

crates/rome_js_formatter/tests/specs/js/module/expression/unary_expression.js.snap

github-actions · 2022-05-05T11:51:24Z

Playground for commit 9bc842f

cloudflare-workers-and-pages · 2022-05-05T12:31:11Z

Deploying with Cloudflare Pages

Latest commit:	`7c5cdf2`
Status:	✅ Deploy successful!
Preview URL:	https://58c3fb5f.tools-8rn.pages.dev

View logs

ematipico · 2022-05-05T14:53:31Z

crates/rome_js_formatter/src/utils/mod.rs

+                let parent_kind = node.parent().map(|p| p.kind());
+                if matches!(
+                    parent_kind,
+                    Some(JsSyntaxKind::JS_FOR_STATEMENT | JsSyntaxKind::JS_RETURN_STATEMENT)
+                ) {
+                    FormatPrecedence::Low
+                } else {
+                    FormatPrecedence::High
+                }


This piece of logic covers:

for ((i = 0), (len = arr.length); i < len; i++) { // ^^^^^^^^^^^^^^^^^^ console.log(arr[i]) }

MichaReiser · 2022-05-06T06:33:55Z

crates/rome_js_formatter/src/js/classes/extends_clause.rs

+        let super_class = super_class?;
+        let needs_parens = matches!(
+            super_class.syntax().kind(),
+            JsSyntaxKind::JS_NEW_EXPRESSION
+                | JsSyntaxKind::JS_YIELD_EXPRESSION
+                | JsSyntaxKind::JS_OBJECT_EXPRESSION
+                | JsSyntaxKind::TS_NON_NULL_ASSERTION_EXPRESSION
+        );
+        let super_class = super_class.format_with(formatter, |super_class| {
+            if needs_parens {
+                format_elements![token("("), super_class, token(")")]
+            } else {
+                super_class
+            }
+        })?;


Adding parentheses to a sub-expression seems to be a common pattern. Could we introduce a format_sub_expression(expression: JsAnyExpression, formatter; &Formatter) -> FormatResult<FormatElement> method instead that internally uses a match similar to prettier's to determine if parentheses for this expression are needed.

This would reduce the boilerplate code that now had to be added to every place where we format a sub expression.

I did try this approach at first, but it didn't work for some particular cases.

prettier's approach orbits around their architecture of formatting things, which is different from ours; by default their approach is elide parenthesis as much as they can, and then they use that monster switch to refrain the algorithm from doing so; (I didn't study it in detail, but I saw some issue where prettier was removing parenthesis where it should not, and they added some cases there... we don't have this problem here)

their AST is different from ours, and sometimes it didn't work inside a big match; I found myself moving code from the generic function and put it inside the specific node;

adding parenthesis requires great context, which would lead to the same issue I introduced in the sequence expressions, where I added a context but it wouldn't work for formatting range;

NicholasLYang

Unrealistic situation, but any reason this doesn't match prettier?

NicholasLYang · 2022-05-26T22:40:27Z

crates/rome_js_formatter/src/js/expressions/unary_expression.rs

@@ -27,10 +29,18 @@ impl FormatNode for JsUnaryExpression {
        );

        if is_keyword_operator {
+            let needs_parenthesis = JsAwaitExpression::can_cast(argument.syntax().kind());


nit: we can do a match on argument directly:

Suggested change

let needs_parenthesis = JsAwaitExpression::can_cast(argument.syntax().kind());

let needs_parenthesis = matches!(argument, JsAnyExpression::JsAwaitExpression(_));

NicholasLYang · 2022-05-26T22:41:15Z

crates/rome_js_formatter/src/js/expressions/unary_expression.rs

            _ => false,
        };

+        dbg!(&is_ambiguous_expression);


Any reason to keep this?

MichaReiser · 2022-05-27T06:58:51Z

I think we need a more general approach to handling parentheses rather than doing it on a per-node level as discussed on discord. Do you want to explore if it would be feasible to handle parentheses inside the FormatNodeRule?

ematipico · 2022-05-27T07:44:03Z

I think we need a more general approach to handling parentheses rather than doing it on a per-node level as discussed on discord. Do you want to explore if it would be feasible to handle parentheses inside the FormatNodeRule?

If you feel strong about this, then I am happy to delegate the implementation to someone else. As I explained in #2538 (comment), I already explored this approach and it didn't work as intended.

MichaReiser · 2022-05-27T08:05:18Z

I think we need a more general approach to handling parentheses rather than doing it on a per-node level as discussed on discord. Do you want to explore if it would be feasible to handle parentheses inside the FormatNodeRule?

If you feel strong about this, then I am happy to delegate the implementation to someone else. As I explained in #2538 (comment), I already explored this approach and it didn't work as intended.

What I'm looking for in an implementation is that it is possible to keep the removal of parentheses in the parenthesized expression formatting rule and the addition of parentheses in the case where they are needed in sync.

ematipico added 3 commits May 4, 2022 14:10

fix(rome_js_formatter): parens, classes

2db2a38

fix(rome_js_formatter): fix precedence inside parenthesized expressions

a7f9fbb

fix(rome_js_formatter): more parenthesis formatting

dc95c1c

ematipico temporarily deployed to aws May 5, 2022 11:47 Inactive

ematipico commented May 5, 2022

View reviewed changes

crates/rome_js_formatter/tests/specs/js/module/class/class.js.snap Outdated Show resolved Hide resolved

ematipico commented May 5, 2022

View reviewed changes

crates/rome_js_formatter/tests/specs/js/module/expression/unary_expression.js.snap Outdated Show resolved Hide resolved

fix(rome_js_formatter): fix unary expression parenthesis

b49b2df

ematipico temporarily deployed to aws May 5, 2022 12:31 Inactive

ematipico requested review from leops, NicholasLYang and yassere May 5, 2022 12:31

chore: correct the snapshot, man

7c5cdf2

ematipico temporarily deployed to aws May 5, 2022 14:52 Inactive

ematipico requested a review from MichaReiser May 5, 2022 14:52

ematipico commented May 5, 2022

View reviewed changes

MichaReiser reviewed May 6, 2022

View reviewed changes

ematipico requested a review from MichaReiser May 6, 2022 12:09

NicholasLYang reviewed May 26, 2022

View reviewed changes

ematipico closed this Jul 16, 2022

sebmck deleted the feature/needs-parenthesis branch November 4, 2022 20:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(rome_js_formatter): use parenthesis #2538

fix(rome_js_formatter): use parenthesis #2538

ematipico commented May 5, 2022

github-actions bot commented May 5, 2022

github-actions bot commented May 5, 2022 •

edited

Loading

cloudflare-workers-and-pages bot commented May 5, 2022 •

edited

Loading

ematipico May 5, 2022 •

edited

Loading

MichaReiser May 6, 2022

ematipico May 6, 2022 •

edited

Loading

NicholasLYang left a comment

NicholasLYang May 26, 2022

NicholasLYang May 26, 2022

MichaReiser commented May 27, 2022

ematipico commented May 27, 2022 •

edited

Loading

MichaReiser commented May 27, 2022

	let needs_parenthesis = JsAwaitExpression::can_cast(argument.syntax().kind());
	let needs_parenthesis = matches!(argument, JsAnyExpression::JsAwaitExpression(_));

fix(rome_js_formatter): use parenthesis #2538

fix(rome_js_formatter): use parenthesis #2538

Conversation

ematipico commented May 5, 2022

Summary

Test Plan

github-actions bot commented May 5, 2022

Parser conformance results on ubuntu-latest

js/262

jsx/babel

ts/babel

ts/microsoft

github-actions bot commented May 5, 2022 • edited Loading

cloudflare-workers-and-pages bot commented May 5, 2022 • edited Loading

Deploying with Cloudflare Pages

ematipico May 5, 2022 • edited Loading

Choose a reason for hiding this comment

MichaReiser May 6, 2022

Choose a reason for hiding this comment

ematipico May 6, 2022 • edited Loading

Choose a reason for hiding this comment

NicholasLYang left a comment

Choose a reason for hiding this comment

NicholasLYang May 26, 2022

Choose a reason for hiding this comment

NicholasLYang May 26, 2022

Choose a reason for hiding this comment

MichaReiser commented May 27, 2022

ematipico commented May 27, 2022 • edited Loading

MichaReiser commented May 27, 2022

github-actions bot commented May 5, 2022 •

edited

Loading

cloudflare-workers-and-pages bot commented May 5, 2022 •

edited

Loading

ematipico May 5, 2022 •

edited

Loading

ematipico May 6, 2022 •

edited

Loading

ematipico commented May 27, 2022 •

edited

Loading