feat(rome_js_formatter): Binary expression formatting (#3079)

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

@@ -1,7 +1,5 @@
 use crate::prelude::*;
-use crate::utils::{
-    format_binary_like_expression, needs_binary_like_parentheses, JsAnyBinaryLikeExpression,
-};
+use crate::utils::{needs_binary_like_parentheses, JsAnyBinaryLikeExpression};
 
 use crate::parentheses::{ExpressionNode, NeedsParentheses};
 use rome_js_syntax::{JsAnyExpression, JsBinaryExpression, JsSyntaxNode};
@@ -15,10 +13,11 @@ impl FormatNodeRule<JsBinaryExpression> for FormatJsBinaryExpression {
         node: &JsBinaryExpression,
         formatter: &mut JsFormatter,
     ) -> FormatResult<()> {
-        format_binary_like_expression(
-            JsAnyBinaryLikeExpression::JsBinaryExpression(node.clone()),
-            formatter,
-        )
+        JsAnyBinaryLikeExpression::JsBinaryExpression(node.clone()).fmt(formatter)
+    }
+
+    fn needs_parentheses(&self, item: &JsBinaryExpression) -> bool {
+        item.needs_parentheses()
     }
 }
 

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

@@ -1,7 +1,5 @@
 use crate::prelude::*;
-use crate::utils::{
-    format_binary_like_expression, needs_binary_like_parentheses, JsAnyBinaryLikeExpression,
-};
+use crate::utils::{needs_binary_like_parentheses, JsAnyBinaryLikeExpression};
 
 use crate::parentheses::{ExpressionNode, NeedsParentheses};
 
@@ -15,10 +13,11 @@ pub struct FormatJsInExpression;
 
 impl FormatNodeRule<JsInExpression> for FormatJsInExpression {
     fn fmt_fields(&self, node: &JsInExpression, formatter: &mut JsFormatter) -> FormatResult<()> {
-        format_binary_like_expression(
-            JsAnyBinaryLikeExpression::JsInExpression(node.clone()),
-            formatter,
-        )
+        JsAnyBinaryLikeExpression::JsInExpression(node.clone()).fmt(formatter)
+    }
+
+    fn needs_parentheses(&self, item: &JsInExpression) -> bool {
+        item.needs_parentheses()
     }
 }
 

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

@@ -1,7 +1,5 @@
 use crate::prelude::*;
-use crate::utils::{
-    format_binary_like_expression, needs_binary_like_parentheses, JsAnyBinaryLikeExpression,
-};
+use crate::utils::{needs_binary_like_parentheses, JsAnyBinaryLikeExpression};
 
 use crate::parentheses::{ExpressionNode, NeedsParentheses};
 use rome_js_syntax::{JsAnyExpression, JsInstanceofExpression, JsSyntaxNode};
@@ -15,10 +13,11 @@ impl FormatNodeRule<JsInstanceofExpression> for FormatJsInstanceofExpression {
         node: &JsInstanceofExpression,
         formatter: &mut JsFormatter,
     ) -> FormatResult<()> {
-        format_binary_like_expression(
-            JsAnyBinaryLikeExpression::JsInstanceofExpression(node.clone()),
-            formatter,
-        )
+        JsAnyBinaryLikeExpression::JsInstanceofExpression(node.clone()).fmt(formatter)
+    }
+
+    fn needs_parentheses(&self, item: &JsInstanceofExpression) -> bool {
+        item.needs_parentheses()
     }
 }
 

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

@@ -1,10 +1,9 @@
 use crate::prelude::*;
-use crate::utils::{
-    format_binary_like_expression, needs_binary_like_parentheses, JsAnyBinaryLikeExpression,
-};
+use crate::utils::{needs_binary_like_parentheses, JsAnyBinaryLikeExpression};
 
 use crate::parentheses::{ExpressionNode, NeedsParentheses};
 use rome_js_syntax::{JsAnyExpression, JsLogicalExpression, JsSyntaxNode};
+use rome_rowan::AstNode;
 
 #[derive(Debug, Clone, Default)]
 pub struct FormatJsLogicalExpression;
@@ -15,17 +14,25 @@ impl FormatNodeRule<JsLogicalExpression> for FormatJsLogicalExpression {
         node: &JsLogicalExpression,
         formatter: &mut JsFormatter,
     ) -> FormatResult<()> {
-        format_binary_like_expression(
-            JsAnyBinaryLikeExpression::JsLogicalExpression(node.clone()),
-            formatter,
-        )
+        JsAnyBinaryLikeExpression::JsLogicalExpression(node.clone()).fmt(formatter)
+    }
+
+    fn needs_parentheses(&self, item: &JsLogicalExpression) -> bool {
+        item.needs_parentheses()
     }
 }
 
 impl NeedsParentheses for JsLogicalExpression {
     fn needs_parentheses_with_parent(&self, parent: &JsSyntaxNode) -> bool {
         if let Some(parent) = JsLogicalExpression::cast(parent.clone()) {
-            return parent.operator() != self.operator();
+            return if parent.operator() != self.operator() {
+                true
+            } else {
+                // TODO: Parentheses should never be needed for the same operators BUT this is causing a re-formatting
+                // issue if a logical expression has an in-balanced tree. See issue-7024.js for a test case..
+                // The way prettier solves this is by re-balancing the tree before formatting, something, Rome' doesn't yet support.
+                Ok(self.syntax()) != parent.left().map(AstNode::into_syntax).as_ref()
+            };
         }
 
         needs_binary_like_parentheses(&JsAnyBinaryLikeExpression::from(self.clone()), parent)

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

@@ -1,18 +1,10 @@
 use crate::prelude::*;
-use crate::utils::{
-    binary_argument_needs_parens, is_simple_expression, FormatPrecedence,
-    JsAnyBinaryLikeLeftExpression,
-};
 use rome_formatter::write;
 
-use crate::utils::JsAnyBinaryLikeExpression;
-
 use crate::parentheses::{ExpressionNode, NeedsParentheses};
 use rome_js_syntax::{
-    JsAnyExpression, JsParenthesizedExpression, JsParenthesizedExpressionFields, JsSyntaxKind,
-    JsSyntaxNode,
+    JsAnyExpression, JsParenthesizedExpression, JsParenthesizedExpressionFields, JsSyntaxNode,
 };
-use rome_rowan::{AstNode, SyntaxResult};
 
 #[derive(Debug, Clone, Default)]
 pub struct FormatJsParenthesizedExpression;
@@ -31,130 +23,21 @@ impl FormatNodeRule<JsParenthesizedExpression> for FormatJsParenthesizedExpressi
 
         let expression = expression?;
 
-        if is_expression_handling_parens(&expression) {
-            return write!(
-                f,
-                [
-                    format_removed(&l_paren_token?),
-                    expression.format(),
-                    format_removed(&r_paren_token?)
-                ]
-            );
-        }
-
-        let parenthesis_can_be_omitted = parenthesis_can_be_omitted(node, &expression)?;
-
-        if parenthesis_can_be_omitted {
-            write![
-                f,
-                [
-                    format_removed(&l_paren_token?),
-                    expression.format(),
-                    format_removed(&r_paren_token?),
-                ]
+        write!(
+            f,
+            [
+                format_removed(&l_paren_token?),
+                expression.format(),
+                format_removed(&r_paren_token?)
             ]
-        } else if is_simple_parenthesized_expression(node)? {
-            write![
-                f,
-                [
-                    l_paren_token.format(),
-                    expression.format(),
-                    r_paren_token.format()
-                ]
-            ]
-        } else {
-            write![
-                f,
-                [
-                    format_delimited(&l_paren_token?, &expression.format(), &r_paren_token?,)
-                        .soft_block_indent()
-                ]
-            ]
-        }
+        )
     }
 
     fn needs_parentheses(&self, item: &JsParenthesizedExpression) -> bool {
         item.needs_parentheses()
     }
 }
 
-fn is_simple_parenthesized_expression(node: &JsParenthesizedExpression) -> SyntaxResult<bool> {
-    let JsParenthesizedExpressionFields {
-        l_paren_token,
-        expression,
-        r_paren_token,
-    } = node.as_fields();
-
-    if l_paren_token?.has_trailing_comments() || r_paren_token?.has_leading_comments() {
-        return Ok(false);
-    }
-
-    if !is_simple_expression(&expression?)? {
-        return Ok(false);
-    }
-
-    Ok(true)
-}
-
-// Allow list of nodes that use the new `need_parens` formatting to determine if parentheses are necessary or not.
-pub(crate) fn is_expression_handling_parens(expression: &JsAnyExpression) -> bool {
-    use JsAnyExpression::*;
-
-    if let JsAnyExpression::JsParenthesizedExpression(inner) = expression {
-        if let Ok(inner) = inner.expression() {
-            is_expression_handling_parens(&inner)
-        } else {
-            false
-        }
-    } else {
-        !matches!(
-            expression,
-            JsInstanceofExpression(_)
-                | JsBinaryExpression(_)
-                | JsInExpression(_)
-                | JsLogicalExpression(_)
-        )
-    }
-}
-
-fn parenthesis_can_be_omitted(
-    node: &JsParenthesizedExpression,
-    expression: &JsAnyExpression,
-) -> SyntaxResult<bool> {
-    let parent = node.syntax().parent();
-
-    if let Some(parent) = &parent {
-        match parent.kind() {
-            // The formatting of the return or throw argument takes care of adding parentheses if necessary
-            JsSyntaxKind::JS_RETURN_STATEMENT | JsSyntaxKind::JS_THROW_STATEMENT => {
-                return Ok(true)
-            }
-            JsSyntaxKind::JS_PARENTHESIZED_EXPRESSION => return Ok(true),
-            _ => {
-                // fall through
-            }
-        }
-    }
-
-    let expression = expression.resolve();
-
-    let parent_precedence = FormatPrecedence::with_precedence_for_parenthesis(parent.as_ref());
-
-    if parent_precedence > FormatPrecedence::Low {
-        return Ok(false);
-    }
-
-    if let Some(parent) = parent {
-        if JsAnyBinaryLikeExpression::can_cast(parent.kind())
-            && !binary_argument_needs_parens(&JsAnyBinaryLikeLeftExpression::from(expression))
-        {
-            return Ok(true);
-        }
-    }
-
-    Ok(false)
-}
-
 impl NeedsParentheses for JsParenthesizedExpression {
     #[inline(always)]
     fn needs_parentheses(&self) -> bool {

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

@@ -4,6 +4,7 @@ use rome_formatter::{format_args, write, FormatContext, FormatRuleWithOptions, V
 
 use crate::context::TabWidth;
 use crate::js::lists::template_element_list::{TemplateElementIndention, TemplateElementLayout};
+use crate::parentheses::ExpressionNode;
 use rome_js_syntax::{
     JsAnyExpression, JsSyntaxNode, JsSyntaxToken, JsTemplateElement, TsTemplateElement,
 };
@@ -106,7 +107,10 @@ impl Format<JsFormatContext> for FormatTemplateElement {
             TemplateElementLayout::Fit => {
                 use JsAnyExpression::*;
 
-                let expression = self.element.expression();
+                let expression = self
+                    .element
+                    .expression()
+                    .map(|expression| expression.resolve());
 
                 // It's preferred to break after/before `${` and `}` rather than breaking in the
                 // middle of some expressions.

crates/rome_js_formatter/src/utils/assignment_like.rs

@@ -848,14 +848,16 @@ pub(crate) fn should_break_after_operator(right: &JsAnyExpression) -> SyntaxResu
         right if JsAnyBinaryLikeExpression::can_cast(right.syntax().kind()) => {
             let binary_like = JsAnyBinaryLikeExpression::unwrap_cast(right.syntax().clone());
 
-            !binary_like.should_inline()
+            !binary_like.should_inline_logical_expression()
         }
 
         JsAnyExpression::JsSequenceExpression(_) => true,
 
         JsAnyExpression::JsConditionalExpression(conditional) => {
-            JsAnyBinaryLikeExpression::cast(conditional.test()?.into_syntax())
-                .map_or(false, |expression| !expression.should_inline())
+            JsAnyBinaryLikeExpression::cast(conditional.test()?.into_resolved_syntax())
+                .map_or(false, |expression| {
+                    !expression.should_inline_logical_expression()
+                })
         }
 
         _ => false,