feat(rome_js_formatter): Parenthesizing expressions #3057
Conversation
Deploying with
|
| Latest commit: |
9823c4b
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://af726b97.tools-8rn.pages.dev |
| Branch Preview URL: | https://feat-needs-parentheses.tools-8rn.pages.dev |
Parser conformance results on ubuntu-latestjs/262
jsx/babel
symbols/microsoft
ts/babel
ts/microsoft
|
MichaReiser
force-pushed
the
feat/needs-parentheses
branch
from
b9a84c7
to
c5e40b6
Compare
MichaReiser
force-pushed
the
feat/needs-parentheses
branch
from
c5e40b6
to
92a0c31
Compare
MichaReiser
force-pushed
the
feat/needs-parentheses
branch
from
92a0c31
to
fbb4bd8
Compare
MichaReiser
changed the title
MichaReiser
force-pushed
the
feat/needs-parentheses
branch
from
fbb4bd8
to
00a1b22
Compare
| @@ -16,19 +17,30 @@ impl FormatNodeRule<JsNumberLiteralExpression> for FormatJsNumberLiteralExpressi | |||
| let JsNumberLiteralExpressionFields { value_token } = node.as_fields(); | |||
| let value_token = value_token?; | |||
|
|
|||
| if let Some(static_member_expression) = node.parent::<JsStaticMemberExpression>() { | |||
There was a problem hiding this comment.
This is now handled as part of needs_parens
| matches!(arrow.body()?, JsAnyFunctionBody::JsFunctionBody(_)) | ||
| } | ||
| _ => false, | ||
| let is_function_like = match resolve_call_argument_expression(&first) { |
There was a problem hiding this comment.
The changes in this file are that checks testing if an argument ar of a specific expression now skip over parenthesized expressions.
| let formatted = | ||
| FormatLiteralStringToken::new(&value_token, StringLiteralParentKind::Expression); | ||
|
|
||
| // Prevents that a string literal expression becomes a directive | ||
| let needs_parens = |
There was a problem hiding this comment.
Now handled as part of needs_parentheses
| } | ||
|
|
||
| // Parenthesize the inner expression if it's a binary or pre-update | ||
| // operation with an ambiguous operator (+ and ++ or - and --) | ||
| let is_ambiguous_expression = match &argument { |
There was a problem hiding this comment.
This is now handled as part of needs_parentheses
| @@ -17,15 +21,60 @@ impl FormatNodeRule<JsxTagExpression> for FormatJsxTagExpression { | |||
| if_group_breaks(&text(")")) | |||
| ])] | |||
| ], | |||
| WrapState::AlwaysWrap => write![ | |||
There was a problem hiding this comment.
Handled by needs_parens
|
|
||
| -(a?.b).c(); | ||
| -(a?.b[c]).c(); | ||
| +a?.b.c(); |
There was a problem hiding this comment.
Rome always formats optional chains without parentheses for consistency reasons
| +() => ({})``; | ||
| +({})``; | ||
| +a = () => ({}).x; | ||
| +({}) && a, b; |
There was a problem hiding this comment.
Rome parentheses the object expression and not the whole expression if it is the first of the statement.
| + topRankedZoneFit.areaPercentageRemaining - previousZoneFitNow.areaPercentageRemaining | ||
| ).toFixed(2); | ||
| -).toFixed(2); | ||
| +const areaPercentageDiff = |
There was a problem hiding this comment.
This is related to our member chain formatting generating different IR
| @@ -1,38 +1,30 @@ | ||
| -( | ||
| - aaaaaaaaaaaaaaaaaaaaaaaaa && | ||
| +(aaaaaaaaaaaaaaaaaaaaaaaaa && |
There was a problem hiding this comment.
This is due to our binary expression formatting being different. This will be tackled separately
| + .ffffffff.gggggggg2 = class extends ( | ||
| aaaaaaaa.bbbbbbbb.cccccccc.dddddddd.eeeeeeee.ffffffff.gggggggg1 | ||
| ) { | ||
| + .ffffffff.gggggggg2 = class extends aaaaaaaa.bbbbbbbb.cccccccc.dddddddd |
There was a problem hiding this comment.
This is because member assignment aren't formatted the same way as static member expressions (which they should)
|
!bench_formatter |
MichaReiser
force-pushed
the
feat/needs-parentheses
branch
from
00a1b22
to
cfc3531
Compare
Formatter Benchmark Results |
| } | ||
|
|
||
| /// Formats the node's fields. | ||
| fn fmt_fields(&self, item: &N, f: &mut JsFormatter) -> FormatResult<()>; | ||
|
|
||
| /// Returns whether the node requires parens. | ||
| fn needs_parentheses(&self, item: &N) -> bool { |
There was a problem hiding this comment.
Ideally, the default implementation for needs_parentheses would be:
fn needs_parentheses(&self, item: &N) -> bool where N: NeedsParentheses {
item.needs_parentheses()
}
fn needs_parentheses(&self, item: &N) -> bool where N: !NeedsParentheses {
false
}
However, negative type constraints do not exist. If anyone has an idea how we can get a similar behaviour please let me know (CC: @leops, @xunilrj)
There was a problem hiding this comment.
Would it be possible to move the methods of NeedsParentheses into FormatNodeRule with a default implementation (and resolve_parent to an AstNode extension trait I guess) ? This could remove the need for an additional level of indirection
There was a problem hiding this comment.
The reason I prefer having an explicit NeedsParentheses trait is that it feels more ergonomic to call node.needs_parentheses rather than FormatAnyExpressionRule::needs_parentheses(&node).
I also do like the fact that resolve_parent isn't exposed for nodes where it isn't necessary to go through that extra loop. For example, you won't need to call resolve_parent on a statement.
The main reasons for computing whether an expression needs to be parenthesized during the formatting phase are:
The main implication is that it increases complexity as well as the risk for subtle bugs when we forget to call There are a few alternatives to this design, some of which I want to explore after I rewrote binary expression formatting:
|
|
!bench_formatter |
MichaReiser
force-pushed
the
feat/needs-parentheses
branch
from
cfc3531
to
552beab
Compare
MichaReiser
force-pushed
the
feat/needs-parentheses
branch
from
bc64528
to
5747e9a
Compare
MichaReiser
force-pushed
the
feat/needs-parentheses
branch
from
5747e9a
to
18e28ca
Compare
| } | ||
|
|
||
| /// Formats the node's fields. | ||
| fn fmt_fields(&self, item: &N, f: &mut JsFormatter) -> FormatResult<()>; | ||
|
|
||
| /// Returns whether the node requires parens. | ||
| fn needs_parentheses(&self, item: &N) -> bool { |
There was a problem hiding this comment.
Would it be possible to move the methods of NeedsParentheses into FormatNodeRule with a default implementation (and resolve_parent to an AstNode extension trait I guess) ? This could remove the need for an additional level of indirection
|
If the diverging is intentional and we won't go back to that, could you please add a task/reminder for ourselves to update the website with this decision? We have to update the section where we track the difference section |
crates/rome_js_formatter/src/js/expressions/arrow_function_expression.rs
Show resolved
Hide resolved
|
I plan to merge this PR by tomorrow except there are some major conceptual concerns around it. |
MichaReiser
force-pushed
the
feat/needs-parentheses
branch
from
18e28ca
to
1740c40
Compare
There was a problem hiding this comment.
We could have a module comment in the parenthesis file, explaining the new concepts introduced, mainly:
- what “resolving” means, since it has a meaning based on the context and it’s a concept that is now widespread and we will use somewhere else
- what “needs parenthesis” means, mostly to not assume that people already knows
MichaReiser
force-pushed
the
feat/needs-parentheses
branch
from
1740c40
to
6293638
Compare
MichaReiser
force-pushed
the
feat/needs-parentheses
branch
from
9823c4b
to
e896925
Compare
Summary
Part of #3046
This PR adds the capability of adding parentheses to improve readability or removing parentheses if they aren't strictly necessary.
The core architecture is around the
NeedsParenthesestrait that everyJsAnyExpressionnow implements. Its main method isneeds_parentheseswhich must returntrueif parentheses are necessary if removing the parentheses would change the semantics OR result in a syntax error.needs_parenthesesmay also returntruein cases where parentheses aren't strictly necessary but improve readability.The new
NeedsParenthesestrait is used in theFormatNodeRuletrait that gets called when formatting every node. It now adds parentheses ifneeds_parenthesesreturnstrue.FormatJsParenthesizedExpressionhas been changed to always remove parentheses and rely onFormatNodeRuleto re-insert parentheses if necessary. This has the downside that any trailing comments of(and leading comments of)are pushed outside the parentheses. I think that's OK and is something we can address when working on #2768.Note: This PR implements
needs_parenthesesfor all binary like expression but they aren't used yet. I struggled to integrate the logic into the existing binary like formatting and intend to tackle it with anyway needed rewrite to closer match prettier's formatting. I plan to do this next.Performance
This PR significantly reduces Rome performance because it is now necessary to call
expression.resolve()orexpression.resolve_parentto skip over parenthesized expressions when testing if a child or parent is of a certain kind.The only alternative that I see to this approach would be to remove all
ParenthesizedExpressionsbefore formatting but this will have its own performance cost, requires source maps to map back to the original code in case a node must be printed in verbatim (to recover the parentheses), and is something we still have the chance to do in the future.My main concern is more about the added complexity and it's easy to forget a
resolve_expressionorresolve_expression_parentcall. I don't have good answer to this and there are likely still a few places where I missed to add these calls myself. My recommendation is that we can fix these over time.Prettier Difference
I decided to diverge from Prettier's formatting in two cases:
ObjectExpression,FunctionExpression, orClassExpressionat the beginning of a statement.Prettier parenthesizes the whole expression whereas Rome parenthesizes the object/function/class expression only.
The main reason for diverging is that parenthesizing the whole expression requires that the logic is implemented in any expression that starts with another child expression (tagged template, binary expressions, computed member/assignment, sequence, conditional, ... ) and it requires traversing the first expression until it reaches an object expression or any expression that doesn't start with another expression. This is rather expensive and Rome's approach avoids the expensive traversal except for object/function and class expressions, of which there should be fewer.
Parenthesized optional chain
Prettier keeps the parentheses if they were present in the source document but never adds them if they were missing. This is the only case where I found that the fact that parens are present in the source document changed how Prettier parenthesizes formatted expression.
My take is that Rome should apply a consistent formatting regardless if parentheses were or were not present in the source document.
Test Plan
I manually reviewed the snapshot changes.
Average compatibility: 81.82 -> 81.54%
Compatible lines: 78.64 -> 78.34%
This PR is regressing the compatibility. Mainly due to the following reasons:
tagto everyJsParenthesizedExpressionthat has a type cast comment (CC: @leops for a use case for node annotations)JsCallArguments,JsTemplate,JsBinaryLikeExpression, member like expressions and more.I'm in favour of merging this PR regardless of the regressions because these issues should be tackled when working on comments, or when improving the formatting of other syntaxes.