Skip to content

Commit a044f67

Browse files
committed
fix(formatter): merge the right side of LogicalExpression if it's a LogicalExpression and both have the same operator
1 parent 4e5d480 commit a044f67

File tree

5 files changed

+100
-38
lines changed

5 files changed

+100
-38
lines changed

crates/oxc_formatter/src/utils/assignment_like.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -510,6 +510,7 @@ impl<'a> AssignmentLike<'a, '_> {
510510

511511
is_generic(&conditional_type.check_type)
512512
|| is_generic(&conditional_type.extends_type)
513+
|| comments.has_comment_before(decl.type_annotation.span().start)
513514
}
514515
_ => {
515516
// Check for leading comments on any other type

crates/oxc_formatter/src/write/binary_like_expression.rs

Lines changed: 95 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@ use oxc_syntax::precedence::{GetPrecedence, Precedence};
77

88
use crate::{
99
Format,
10-
formatter::{FormatResult, Formatter},
10+
formatter::{FormatResult, Formatter, trivia::FormatTrailingComments},
1111
generated::ast_nodes::{AstNode, AstNodes},
1212
utils::format_node_without_trailing_comments::FormatNodeWithoutTrailingComments,
1313
};
@@ -32,14 +32,18 @@ impl From<LogicalOperator> for BinaryLikeOperator {
3232
}
3333
}
3434

35-
impl BinaryLikeOperator {
36-
fn as_str(self) -> &'static str {
37-
match self {
35+
impl Format<'_> for BinaryLikeOperator {
36+
fn fmt(&self, f: &mut Formatter<'_, '_>) -> FormatResult<()> {
37+
let operator = match self {
3838
Self::BinaryOperator(op) => op.as_str(),
3939
Self::LogicalOperator(op) => op.as_str(),
40-
}
40+
};
41+
42+
write!(f, operator)
4143
}
44+
}
4245

46+
impl BinaryLikeOperator {
4347
pub fn precedence(self) -> Precedence {
4448
match self {
4549
Self::BinaryOperator(op) => op.precedence(),
@@ -52,7 +56,7 @@ impl BinaryLikeOperator {
5256
}
5357
}
5458

55-
#[derive(Clone, Copy)]
59+
#[derive(Debug, Clone, Copy)]
5660
pub enum BinaryLikeExpression<'a, 'b> {
5761
LogicalExpression(&'b AstNode<'a, LogicalExpression<'a>>),
5862
BinaryExpression(&'b AstNode<'a, BinaryExpression<'a>>),
@@ -280,6 +284,7 @@ impl<'a> Format<'a> for BinaryLikeExpression<'a, '_> {
280284
}
281285

282286
/// Represents the right or left hand side of a binary expression.
287+
#[derive(Debug)]
283288
enum BinaryLeftOrRightSide<'a, 'b> {
284289
/// A terminal left hand side of a binary expression.
285290
///
@@ -306,14 +311,92 @@ impl<'a> Format<'a> for BinaryLeftOrRightSide<'a, '_> {
306311
inside_condition: inside_parenthesis,
307312
root,
308313
} => {
314+
let mut binary_like_expression = *binary_like_expression;
309315
// // It's only possible to suppress the formatting of the whole binary expression formatting OR
310316
// // the formatting of the right hand side value but not of a nested binary expression.
311317
// // This aligns with Prettier's behaviour.
312318
// f.context().comments().mark_suppression_checked(binary_like_expression.syntax());
319+
320+
let logical_operator = if let BinaryLikeExpression::LogicalExpression(logical) =
321+
binary_like_expression
322+
{
323+
Some(logical.operator())
324+
} else {
325+
None
326+
};
327+
328+
// `(longVariable === "long-string") && ((1 <= longVariable) && (longVariable <= 100000000));`
329+
// ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
330+
// is a LogicalExpression with the `&&` operator
331+
//
332+
// ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
333+
// the right side of the parent `LogicalExpression` is
334+
// also a `LogicalExpression` with the `&&` operator
335+
//
336+
// In this case, the both are `LogicalExpression`s and have the same operator, then we have to
337+
// flatten them into the same group, so these three parts, respectively,
338+
// `longVariable === "long-string"`, `1 <= longVariable` and `longVariable <= 100000000` should
339+
// be formatted in the same group.
340+
//
341+
// In the following logic, we will recursively find the right side of the LogicalExpression to
342+
// ensure that all parts are in the same group.
343+
//
344+
// Example output:
345+
// ```js
346+
// longVariable === "long-string" &&
347+
// 1 <= longVariable &&
348+
// longVariable <= 100000000;
349+
// ```
350+
//
351+
// Based on Prettier's rebalancing logic for LogicalExpressions:
352+
// <https://github.com/prettier/prettier/blob/7584432401a47a26943dd7a9ca9a8e032ead7285/src/language-js/parse/postprocess/index.js#L64-L69>
353+
loop {
354+
if let AstNodes::LogicalExpression(right_logical) =
355+
binary_like_expression.right().as_ast_nodes()
356+
&& let Some(operator) = logical_operator
357+
&& operator == right_logical.operator()
358+
{
359+
write!(
360+
f,
361+
[
362+
space(),
363+
operator.as_str(),
364+
soft_line_break_or_space(),
365+
format_once(|f| {
366+
// If the left side of the right logical expression is still a logical expression with
367+
// the same operator, we need to recursively split it into left and right sides.
368+
// This way, we can ensure that all parts are in the same group.
369+
let left_child = right_logical.left();
370+
if let AstNodes::LogicalExpression(left_logical_child) =
371+
left_child.as_ast_nodes()
372+
&& operator == left_logical_child.operator()
373+
{
374+
let left_parts = split_into_left_and_right_sides(
375+
BinaryLikeExpression::LogicalExpression(
376+
left_logical_child,
377+
),
378+
*inside_parenthesis,
379+
);
380+
381+
f.join().entries(left_parts).finish()
382+
} else {
383+
left_child.fmt(f)
384+
}
385+
})
386+
]
387+
)?;
388+
389+
binary_like_expression =
390+
BinaryLikeExpression::LogicalExpression(right_logical);
391+
} else {
392+
break;
393+
}
394+
}
395+
313396
let right = binary_like_expression.right();
314-
let operator = binary_like_expression.operator();
397+
315398
let operator_and_right_expression = format_with(|f| {
316-
write!(f, [space(), operator.as_str()])?;
399+
write!(f, [space(), binary_like_expression.operator()])?;
317400

318401
if binary_like_expression.should_inline_logical_expression() {
319402
write!(f, [space()])?;
@@ -338,11 +421,7 @@ impl<'a> Format<'a> for BinaryLeftOrRightSide<'a, '_> {
338421
) || is_same_binary_expression_kind(
339422
binary_like_expression,
340423
right.as_ast_nodes(),
341-
) || (*inside_parenthesis
342-
&& matches!(
343-
binary_like_expression,
344-
BinaryLikeExpression::LogicalExpression(_)
345-
)));
424+
) || (*inside_parenthesis && logical_operator.is_some()));
346425

347426
if should_group {
348427
// `left` side has printed before `right` side, so that trailing comments of `left` side has been printed,
@@ -364,6 +443,7 @@ impl<'a> Format<'a> for BinaryLeftOrRightSide<'a, '_> {
364443
.rev()
365444
.take_while(|comment| {
366445
binary_like_expression.left().span().end < comment.span.start
446+
&& right.span().start > comment.span.end
367447
})
368448
.any(|comment| comment.is_line());
369449

@@ -437,7 +517,7 @@ fn split_into_left_and_right_sides<'a, 'b>(
437517

438518
// Stores the left and right parts of the binary expression in sequence (rather than nested as they
439519
// appear in the tree).
440-
// `with_capacity(2)` because we expect at most 2 items (left and right).
520+
// `with_capacity(2)` because we expect at least 2 items (left and right).
441521
let mut items = Vec::with_capacity(2);
442522

443523
split_into_left_and_right_sides_inner(true, binary, inside_condition, &mut items);
@@ -460,7 +540,7 @@ fn should_indent_if_parent_inlines(parent: &AstNodes<'_>) -> bool {
460540
}
461541

462542
fn is_same_binary_expression_kind(
463-
binary: &BinaryLikeExpression<'_, '_>,
543+
binary: BinaryLikeExpression<'_, '_>,
464544
other: &AstNodes<'_>,
465545
) -> bool {
466546
match binary {

tasks/coverage/snapshots/formatter_babel.snap

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2,11 +2,9 @@ commit: 41d96516
22

33
formatter_babel Summary:
44
AST Parsed : 2423/2423 (100.00%)
5-
Positive Passed: 2419/2423 (99.83%)
5+
Positive Passed: 2420/2423 (99.88%)
66
Mismatch: tasks/coverage/babel/packages/babel-parser/test/fixtures/comments/basic/try-statement/input.js
77

8-
Mismatch: tasks/coverage/babel/packages/babel-parser/test/fixtures/comments/regression/13750/input.js
9-
108
Mismatch: tasks/coverage/babel/packages/babel-parser/test/fixtures/es2015/class/division/input.js
119

1210
Expect to Parse: tasks/coverage/babel/packages/babel-parser/test/fixtures/es2022/top-level-await-unambiguous/module/input.js

tasks/coverage/snapshots/formatter_typescript.snap

Lines changed: 1 addition & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -2,41 +2,25 @@ commit: 261630d6
22

33
formatter_typescript Summary:
44
AST Parsed : 8816/8816 (100.00%)
5-
Positive Passed: 8797/8816 (99.78%)
5+
Positive Passed: 8805/8816 (99.88%)
66
Mismatch: tasks/coverage/typescript/tests/cases/compiler/amdLikeInputDeclarationEmit.ts
77

88
Expect to Parse: tasks/coverage/typescript/tests/cases/compiler/arrayFromAsync.ts
99
`await` is only allowed within async functions and at the top levels of modules`await` is only allowed within async functions and at the top levels of modules`await` is only allowed within async functions and at the top levels of modules`await` is only allowed within async functions and at the top levels of modules`await` is only allowed within async functions and at the top levels of modules`await` is only allowed within async functions and at the top levels of modules`await` is only allowed within async functions and at the top levels of modules`await` is only allowed within async functions and at the top levels of modules`await` is only allowed within async functions and at the top levels of modules`await` is only allowed within async functions and at the top levels of modules`await` is only allowed within async functions and at the top levels of modules
10-
Mismatch: tasks/coverage/typescript/tests/cases/compiler/coAndContraVariantInferences3.ts
11-
12-
Mismatch: tasks/coverage/typescript/tests/cases/compiler/complexNarrowingWithAny.ts
13-
1410
Mismatch: tasks/coverage/typescript/tests/cases/compiler/declarationEmitCastReusesTypeNode4.ts
1511

1612
Expect to Parse: tasks/coverage/typescript/tests/cases/compiler/genericTypeAssertions3.ts
1713
Unexpected token
18-
Mismatch: tasks/coverage/typescript/tests/cases/compiler/jsxNamespaceGlobalReexport.tsx
19-
20-
Mismatch: tasks/coverage/typescript/tests/cases/compiler/jsxNamespaceGlobalReexportMissingAliasTarget.tsx
21-
22-
Mismatch: tasks/coverage/typescript/tests/cases/compiler/jsxNamespaceImplicitImportJSXNamespace.tsx
23-
2414
Mismatch: tasks/coverage/typescript/tests/cases/compiler/propertyAccessExpressionInnerComments.ts
2515

2616
Mismatch: tasks/coverage/typescript/tests/cases/compiler/sourceMapValidationClasses.ts
2717

28-
Mismatch: tasks/coverage/typescript/tests/cases/compiler/styledComponentsInstantiaionLimitNotReached.ts
29-
3018
Mismatch: tasks/coverage/typescript/tests/cases/compiler/tryStatementInternalComments.ts
3119

3220
Expect to Parse: tasks/coverage/typescript/tests/cases/conformance/classes/propertyMemberDeclarations/staticPropertyNameConflicts.ts
3321
Classes may not have a static property named prototypeClasses may not have a static property named prototypeClasses may not have a static property named prototypeClasses may not have a static property named prototypeClasses may not have a static property named prototypeClasses may not have a static property named prototype
3422
Expect to Parse: tasks/coverage/typescript/tests/cases/conformance/expressions/elementAccess/letIdentifierInElementAccess01.ts
3523
Unexpected token
36-
Mismatch: tasks/coverage/typescript/tests/cases/conformance/expressions/typeGuards/typeGuardsInRightOperandOfAndAndOperator.ts
37-
38-
Mismatch: tasks/coverage/typescript/tests/cases/conformance/expressions/typeGuards/typeGuardsInRightOperandOfOrOrOperator.ts
39-
4024
Mismatch: tasks/coverage/typescript/tests/cases/conformance/generators/yieldStatementNoAsiAfterTransform.ts
4125

4226
Mismatch: tasks/coverage/typescript/tests/cases/conformance/statements/returnStatements/returnStatementNoAsiAfterTransform.ts

tasks/prettier_conformance/snapshots/prettier.js.snap.md

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,10 @@
1-
js compatibility: 662/699 (94.71%)
1+
js compatibility: 663/699 (94.85%)
22

33
# Failed
44

55
| Spec path | Failed or Passed | Match ratio |
66
| :-------- | :--------------: | :---------: |
7-
| js/comments/15661.js | 💥💥 | 55.81% |
7+
| js/comments/15661.js | 💥💥 | 55.17% |
88
| js/comments/empty-statements.js | 💥💥 | 90.91% |
99
| js/comments/function-declaration.js | 💥💥 | 92.80% |
1010
| js/comments/return-statement.js | 💥💥 | 98.85% |
@@ -20,7 +20,6 @@ js compatibility: 662/699 (94.71%)
2020
| js/identifier/for-of/let.js | 💥 | 92.31% |
2121
| js/identifier/parentheses/let.js | 💥💥 | 82.27% |
2222
| js/last-argument-expansion/dangling-comment-in-arrow-function.js | 💥 | 22.22% |
23-
| js/logical_expressions/issue-7024.js | 💥 | 66.67% |
2423
| js/object-multiline/multiline.js | 💥✨ | 22.22% |
2524
| js/quote-props/classes.js | 💥💥✨✨ | 47.06% |
2625
| js/quote-props/objects.js | 💥💥✨✨ | 45.10% |

0 commit comments

Comments
 (0)