From ed33fad48b3b2d84fb1375294c7736cf7da4667f Mon Sep 17 00:00:00 2001 From: Dunqing <29533304+Dunqing@users.noreply.github.com> Date: Mon, 29 Sep 2025 13:37:39 +0000 Subject: [PATCH] fix(formatter): merge the right side of `LogicalExpression` if it's a `LogicalExpression` and both have the same `operator` (#14097) I wrote the implementation, and it isn't a port from anywhere. This is to align with [Prettier's unbalancing LogicalExpression](https://github.com/prettier/prettier/blob/7584432401a47a26943dd7a9ca9a8e032ead7285/src/language-js/parse/postprocess/index.js#L64-L69) behavior. We can't use the same approach that Prettier does because our AST isn't mutable, so we have to do the same thing in the formatting phase, which makes the code a little hard to understand. However, our own is more performant. --- .../src/utils/assignment_like.rs | 1 + .../src/write/binary_like_expression.rs | 110 +++++++++++++++--- tasks/coverage/snapshots/formatter_babel.snap | 4 +- .../snapshots/formatter_typescript.snap | 18 +-- .../snapshots/prettier.js.snap.md | 5 +- 5 files changed, 100 insertions(+), 38 deletions(-) diff --git a/crates/oxc_formatter/src/utils/assignment_like.rs b/crates/oxc_formatter/src/utils/assignment_like.rs index b6a5826c8b51b..0bb1ae3a82312 100644 --- a/crates/oxc_formatter/src/utils/assignment_like.rs +++ b/crates/oxc_formatter/src/utils/assignment_like.rs @@ -510,6 +510,7 @@ impl<'a> AssignmentLike<'a, '_> { is_generic(&conditional_type.check_type) || is_generic(&conditional_type.extends_type) + || comments.has_comment_before(decl.type_annotation.span().start) } _ => { // Check for leading comments on any other type diff --git a/crates/oxc_formatter/src/write/binary_like_expression.rs b/crates/oxc_formatter/src/write/binary_like_expression.rs index da3f8da0ccba1..8ce81033b47de 100644 --- a/crates/oxc_formatter/src/write/binary_like_expression.rs +++ b/crates/oxc_formatter/src/write/binary_like_expression.rs @@ -7,7 +7,7 @@ use oxc_syntax::precedence::{GetPrecedence, Precedence}; use crate::{ Format, - formatter::{FormatResult, Formatter}, + formatter::{FormatResult, Formatter, trivia::FormatTrailingComments}, generated::ast_nodes::{AstNode, AstNodes}, utils::format_node_without_trailing_comments::FormatNodeWithoutTrailingComments, }; @@ -32,14 +32,18 @@ impl From for BinaryLikeOperator { } } -impl BinaryLikeOperator { - fn as_str(self) -> &'static str { - match self { +impl Format<'_> for BinaryLikeOperator { + fn fmt(&self, f: &mut Formatter<'_, '_>) -> FormatResult<()> { + let operator = match self { Self::BinaryOperator(op) => op.as_str(), Self::LogicalOperator(op) => op.as_str(), - } + }; + + write!(f, operator) } +} +impl BinaryLikeOperator { pub fn precedence(self) -> Precedence { match self { Self::BinaryOperator(op) => op.precedence(), @@ -52,7 +56,7 @@ impl BinaryLikeOperator { } } -#[derive(Clone, Copy)] +#[derive(Debug, Clone, Copy)] pub enum BinaryLikeExpression<'a, 'b> { LogicalExpression(&'b AstNode<'a, LogicalExpression<'a>>), BinaryExpression(&'b AstNode<'a, BinaryExpression<'a>>), @@ -280,6 +284,7 @@ impl<'a> Format<'a> for BinaryLikeExpression<'a, '_> { } /// Represents the right or left hand side of a binary expression. +#[derive(Debug)] enum BinaryLeftOrRightSide<'a, 'b> { /// A terminal left hand side of a binary expression. /// @@ -306,14 +311,92 @@ impl<'a> Format<'a> for BinaryLeftOrRightSide<'a, '_> { inside_condition: inside_parenthesis, root, } => { + let mut binary_like_expression = *binary_like_expression; // // It's only possible to suppress the formatting of the whole binary expression formatting OR // // the formatting of the right hand side value but not of a nested binary expression. // // This aligns with Prettier's behaviour. // f.context().comments().mark_suppression_checked(binary_like_expression.syntax()); + + let logical_operator = if let BinaryLikeExpression::LogicalExpression(logical) = + binary_like_expression + { + Some(logical.operator()) + } else { + None + }; + + // `(longVariable === "long-string") && ((1 <= longVariable) && (longVariable <= 100000000));` + // ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + // is a LogicalExpression with the `&&` operator + // + // ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + // the right side of the parent `LogicalExpression` is + // also a `LogicalExpression` with the `&&` operator + // + // In this case, the both are `LogicalExpression`s and have the same operator, then we have to + // flatten them into the same group, so these three parts, respectively, + // `longVariable === "long-string"`, `1 <= longVariable` and `longVariable <= 100000000` should + // be formatted in the same group. + // + // In the following logic, we will recursively find the right side of the LogicalExpression to + // ensure that all parts are in the same group. + // + // Example output: + // ```js + // longVariable === "long-string" && + // 1 <= longVariable && + // longVariable <= 100000000; + // ``` + // + // Based on Prettier's rebalancing logic for LogicalExpressions: + // + loop { + if let AstNodes::LogicalExpression(right_logical) = + binary_like_expression.right().as_ast_nodes() + && let Some(operator) = logical_operator + && operator == right_logical.operator() + { + write!( + f, + [ + space(), + operator.as_str(), + soft_line_break_or_space(), + format_once(|f| { + // If the left side of the right logical expression is still a logical expression with + // the same operator, we need to recursively split it into left and right sides. + // This way, we can ensure that all parts are in the same group. + let left_child = right_logical.left(); + if let AstNodes::LogicalExpression(left_logical_child) = + left_child.as_ast_nodes() + && operator == left_logical_child.operator() + { + let left_parts = split_into_left_and_right_sides( + BinaryLikeExpression::LogicalExpression( + left_logical_child, + ), + *inside_parenthesis, + ); + + f.join().entries(left_parts).finish() + } else { + left_child.fmt(f) + } + }) + ] + )?; + + binary_like_expression = + BinaryLikeExpression::LogicalExpression(right_logical); + } else { + break; + } + } + let right = binary_like_expression.right(); - let operator = binary_like_expression.operator(); + let operator_and_right_expression = format_with(|f| { - write!(f, [space(), operator.as_str()])?; + write!(f, [space(), binary_like_expression.operator()])?; if binary_like_expression.should_inline_logical_expression() { write!(f, [space()])?; @@ -338,11 +421,7 @@ impl<'a> Format<'a> for BinaryLeftOrRightSide<'a, '_> { ) || is_same_binary_expression_kind( binary_like_expression, right.as_ast_nodes(), - ) || (*inside_parenthesis - && matches!( - binary_like_expression, - BinaryLikeExpression::LogicalExpression(_) - ))); + ) || (*inside_parenthesis && logical_operator.is_some())); if should_group { // `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, '_> { .rev() .take_while(|comment| { binary_like_expression.left().span().end < comment.span.start + && right.span().start > comment.span.end }) .any(|comment| comment.is_line()); @@ -437,7 +517,7 @@ fn split_into_left_and_right_sides<'a, 'b>( // Stores the left and right parts of the binary expression in sequence (rather than nested as they // appear in the tree). - // `with_capacity(2)` because we expect at most 2 items (left and right). + // `with_capacity(2)` because we expect at least 2 items (left and right). let mut items = Vec::with_capacity(2); 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 { } fn is_same_binary_expression_kind( - binary: &BinaryLikeExpression<'_, '_>, + binary: BinaryLikeExpression<'_, '_>, other: &AstNodes<'_>, ) -> bool { match binary { diff --git a/tasks/coverage/snapshots/formatter_babel.snap b/tasks/coverage/snapshots/formatter_babel.snap index b8abb4cdeb533..817cd6f11b51e 100644 --- a/tasks/coverage/snapshots/formatter_babel.snap +++ b/tasks/coverage/snapshots/formatter_babel.snap @@ -2,11 +2,9 @@ commit: 41d96516 formatter_babel Summary: AST Parsed : 2423/2423 (100.00%) -Positive Passed: 2419/2423 (99.83%) +Positive Passed: 2420/2423 (99.88%) Mismatch: tasks/coverage/babel/packages/babel-parser/test/fixtures/comments/basic/try-statement/input.js -Mismatch: tasks/coverage/babel/packages/babel-parser/test/fixtures/comments/regression/13750/input.js - Mismatch: tasks/coverage/babel/packages/babel-parser/test/fixtures/es2015/class/division/input.js Expect to Parse: tasks/coverage/babel/packages/babel-parser/test/fixtures/es2022/top-level-await-unambiguous/module/input.js diff --git a/tasks/coverage/snapshots/formatter_typescript.snap b/tasks/coverage/snapshots/formatter_typescript.snap index cb3e9c57d91b6..fb7256a9ac44d 100644 --- a/tasks/coverage/snapshots/formatter_typescript.snap +++ b/tasks/coverage/snapshots/formatter_typescript.snap @@ -2,41 +2,25 @@ commit: 261630d6 formatter_typescript Summary: AST Parsed : 8816/8816 (100.00%) -Positive Passed: 8797/8816 (99.78%) +Positive Passed: 8805/8816 (99.88%) Mismatch: tasks/coverage/typescript/tests/cases/compiler/amdLikeInputDeclarationEmit.ts Expect to Parse: tasks/coverage/typescript/tests/cases/compiler/arrayFromAsync.ts `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 -Mismatch: tasks/coverage/typescript/tests/cases/compiler/coAndContraVariantInferences3.ts - -Mismatch: tasks/coverage/typescript/tests/cases/compiler/complexNarrowingWithAny.ts - Mismatch: tasks/coverage/typescript/tests/cases/compiler/declarationEmitCastReusesTypeNode4.ts Expect to Parse: tasks/coverage/typescript/tests/cases/compiler/genericTypeAssertions3.ts Unexpected token -Mismatch: tasks/coverage/typescript/tests/cases/compiler/jsxNamespaceGlobalReexport.tsx - -Mismatch: tasks/coverage/typescript/tests/cases/compiler/jsxNamespaceGlobalReexportMissingAliasTarget.tsx - -Mismatch: tasks/coverage/typescript/tests/cases/compiler/jsxNamespaceImplicitImportJSXNamespace.tsx - Mismatch: tasks/coverage/typescript/tests/cases/compiler/propertyAccessExpressionInnerComments.ts Mismatch: tasks/coverage/typescript/tests/cases/compiler/sourceMapValidationClasses.ts -Mismatch: tasks/coverage/typescript/tests/cases/compiler/styledComponentsInstantiaionLimitNotReached.ts - Mismatch: tasks/coverage/typescript/tests/cases/compiler/tryStatementInternalComments.ts Expect to Parse: tasks/coverage/typescript/tests/cases/conformance/classes/propertyMemberDeclarations/staticPropertyNameConflicts.ts 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 Expect to Parse: tasks/coverage/typescript/tests/cases/conformance/expressions/elementAccess/letIdentifierInElementAccess01.ts Unexpected token -Mismatch: tasks/coverage/typescript/tests/cases/conformance/expressions/typeGuards/typeGuardsInRightOperandOfAndAndOperator.ts - -Mismatch: tasks/coverage/typescript/tests/cases/conformance/expressions/typeGuards/typeGuardsInRightOperandOfOrOrOperator.ts - Mismatch: tasks/coverage/typescript/tests/cases/conformance/generators/yieldStatementNoAsiAfterTransform.ts Mismatch: tasks/coverage/typescript/tests/cases/conformance/statements/returnStatements/returnStatementNoAsiAfterTransform.ts diff --git a/tasks/prettier_conformance/snapshots/prettier.js.snap.md b/tasks/prettier_conformance/snapshots/prettier.js.snap.md index bec95ae8a95ac..fc4fdbaa1dc00 100644 --- a/tasks/prettier_conformance/snapshots/prettier.js.snap.md +++ b/tasks/prettier_conformance/snapshots/prettier.js.snap.md @@ -1,10 +1,10 @@ -js compatibility: 662/699 (94.71%) +js compatibility: 663/699 (94.85%) # Failed | Spec path | Failed or Passed | Match ratio | | :-------- | :--------------: | :---------: | -| js/comments/15661.js | 💥💥 | 55.81% | +| js/comments/15661.js | 💥💥 | 55.17% | | js/comments/empty-statements.js | 💥💥 | 90.91% | | js/comments/function-declaration.js | 💥💥 | 92.80% | | js/comments/return-statement.js | 💥💥 | 98.85% | @@ -20,7 +20,6 @@ js compatibility: 662/699 (94.71%) | js/identifier/for-of/let.js | 💥 | 92.31% | | js/identifier/parentheses/let.js | 💥💥 | 82.27% | | js/last-argument-expansion/dangling-comment-in-arrow-function.js | 💥 | 22.22% | -| js/logical_expressions/issue-7024.js | 💥 | 66.67% | | js/object-multiline/multiline.js | 💥✨ | 22.22% | | js/quote-props/classes.js | 💥💥✨✨ | 47.06% | | js/quote-props/objects.js | 💥💥✨✨ | 45.10% |