Skip to content

fix(formatter): merge the right side of LogicalExpression if it's a LogicalExpression and both have the same operator #14097

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
graphite-app merged 1 commit into from
Sep 29, 2025
Merged

fix(formatter): merge the right side of LogicalExpression if it's a LogicalExpression and both have the same operator #14097

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
1 change: 1 addition & 0 deletions crates/oxc_formatter/src/utils/assignment_like.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
110 changes: 95 additions & 15 deletions crates/oxc_formatter/src/write/binary_like_expression.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
};
Expand All @@ -32,14 +32,18 @@ impl From<LogicalOperator> 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(),
Expand All @@ -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>>),
Expand Down Expand Up @@ -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.
///
Expand All @@ -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:
// <https://github.com/prettier/prettier/blob/7584432401a47a26943dd7a9ca9a8e032ead7285/src/language-js/parse/postprocess/index.js#L64-L69>
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()])?;
Expand All @@ -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,
Expand All @@ -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());

Expand Down Expand Up @@ -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);
Expand All @@ -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 {
Expand Down
4 changes: 1 addition & 3 deletions tasks/coverage/snapshots/formatter_babel.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
18 changes: 1 addition & 17 deletions tasks/coverage/snapshots/formatter_typescript.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
5 changes: 2 additions & 3 deletions tasks/prettier_conformance/snapshots/prettier.js.snap.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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% |
Expand All @@ -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% |
Expand Down
Loading