refactor(rome_js_formatter): Support arbitrarily deep binary expressi…

…ons (rome#2376)

`format_binarish` used to use recursion to format the binary expressions. While this led to somewhat simpler code it had the downside that it stack overflowed for binary expressions with deep left subtrees. 

This PR rewrites the implementation to use an `Iterator` instead that visits the left-hand sides of binary-like-expressions in post order (first visiting the deepest left-hand side, then traversing upwards). 

This PR further fixes a unicode issue in the diff printing logic and renames `operator` to `operator_token` for a consistent naming.

crates/rome_analyze/src/analyzers/no_double_equals.rs

@@ -13,7 +13,7 @@ pub const ANALYZER: Analyzer = Analyzer {
 fn analyze(ctx: &AnalyzerContext) -> Option<Analysis> {
     ctx.query_nodes::<JsBinaryExpression>()
         .filter_map(|n| {
-            let op = n.operator().ok()?;
+            let op = n.operator_token().ok()?;
 
             if !matches!(op.kind(), EQ2 | NEQ) {
                 return None;

crates/rome_analyze/src/assists/flip_bin_exp.rs

@@ -14,7 +14,7 @@ pub const ASSIST: AssistProvider = AssistProvider {
 fn analyze(ctx: &AssistContext) -> Option<Analysis> {
     let node = ctx.find_node_at_cursor_range::<JsBinaryExpression>()?;
 
-    let op_range = node.operator().ok()?.text_trimmed_range();
+    let op_range = node.operator_token().ok()?.text_trimmed_range();
     if !op_range.contains_range(ctx.cursor_range) {
         return None;
     }

crates/rome_console/src/diff.rs

@@ -30,7 +30,7 @@ pub enum DiffMode {
     /// 11  9 |   elit,
     ///       | @@ -15,7 +13,5 @@
     /// 14 12 |   eiusmod
-    /// 15 13 |   
+    /// 15 13 |
     /// 16 14 |   incididunt
     /// 17    | - function
     /// 18    | - name(
@@ -154,8 +154,11 @@ impl<'a> Display for Diff<'a> {
                         fmt.write_str(" | ")?;
 
                         let line = change.value().trim_end();
-                        let line_length = line.len().min(MAX_LINE_LENGTH);
-                        let line = &line[..line_length];
+                        let line = line
+                            .char_indices()
+                            .nth(MAX_LINE_LENGTH)
+                            .map(|(byte_index, _)| &line[..byte_index])
+                            .unwrap_or_else(|| line);
 
                         match change.tag() {
                             ChangeTag::Delete => {

crates/rome_formatter/src/format_element.rs

@@ -646,7 +646,11 @@ pub fn hard_group_elements<T: Into<FormatElement>>(content: T) -> FormatElement
     if content.is_empty() {
         content
     } else {
-        FormatElement::HardGroup(Group::new(content))
+        let (leading, content, trailing) = content.split_trivia();
+        format_elements![
+            leading,
+            FormatElement::HardGroup(Group::new(format_elements![content, trailing])),
+        ]
     }
 }
 
@@ -1243,8 +1247,8 @@ impl Deref for Token {
 
 impl FormatElement {
     /// Returns true if the element contains no content.
-    pub fn is_empty(&self) -> bool {
-        self == &FormatElement::Empty
+    pub const fn is_empty(&self) -> bool {
+        matches!(self, FormatElement::Empty)
     }
 
     /// Returns true if this [FormatElement] recursively contains any hard line break
@@ -1276,32 +1280,40 @@ impl FormatElement {
     /// content itself.
     pub fn split_trivia(self) -> (FormatElement, FormatElement, FormatElement) {
         match self {
-            FormatElement::List(list) => {
+            FormatElement::List(mut list) => {
                 // Find the index of the first non-comment element in the list
                 let content_start = list
                     .content
                     .iter()
-                    .position(|elem| !matches!(elem, FormatElement::Comment(_)))
-                    .unwrap_or(list.content.len());
-
-                // Split the list at the found index
-                let mut leading = list.content;
-                let mut content = leading.split_off(content_start);
-
-                // Find the index of the last non-comment element in the list
-                let content_end = content
-                    .iter()
-                    .rposition(|elem| !matches!(elem, FormatElement::Comment(_)))
-                    .map_or(0, |index| index + 1);
-
-                // Split the list at the found index, content now holds the inner elements
-                let trailing = content.split_off(content_end);
-
-                (
-                    concat_elements(leading),
-                    concat_elements(content),
-                    concat_elements(trailing),
-                )
+                    .position(|elem| !matches!(elem, FormatElement::Comment(_)));
+
+                // List contains at least one non trivia element.
+                if let Some(content_start) = content_start {
+                    let (leading, mut content) = if content_start > 0 {
+                        let content = list.content.split_off(content_start);
+                        (FormatElement::List(list), content)
+                    } else {
+                        // No leading trivia
+                        (empty_element(), list.content)
+                    };
+
+                    let content_end = content
+                        .iter()
+                        .rposition(|elem| !matches!(elem, FormatElement::Comment(_)))
+                        .expect("List guaranteed to contain at least one non trivia element.");
+                    let trailing_start = content_end + 1;
+
+                    let trailing = if trailing_start < content.len() {
+                        FormatElement::List(List::new(content.split_off(trailing_start)))
+                    } else {
+                        empty_element()
+                    };
+
+                    (leading, FormatElement::List(List::new(content)), trailing)
+                } else {
+                    // All leading trivia
+                    (FormatElement::List(list), empty_element(), empty_element())
+                }
             }
             FormatElement::HardGroup(group) => {
                 let (leading, content, trailing) = group.content.split_trivia();

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

@@ -1,10 +1,12 @@
-use crate::utils::format_binaryish_expression;
+use crate::utils::{format_binary_like_expression, JsAnyBinaryLikeExpression};
 use crate::{FormatElement, FormatResult, Formatter, ToFormatElement};
 use rome_js_syntax::JsBinaryExpression;
-use rome_rowan::AstNode;
 
 impl ToFormatElement for JsBinaryExpression {
     fn to_format_element(&self, formatter: &Formatter) -> FormatResult<FormatElement> {
-        format_binaryish_expression(self.syntax(), formatter)
+        format_binary_like_expression(
+            JsAnyBinaryLikeExpression::JsBinaryExpression(self.clone()),
+            formatter,
+        )
     }
 }

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

@@ -1,10 +1,12 @@
-use crate::utils::format_binaryish_expression;
+use crate::utils::{format_binary_like_expression, JsAnyBinaryLikeExpression};
 use crate::{FormatElement, FormatResult, Formatter, ToFormatElement};
 use rome_js_syntax::JsInExpression;
-use rome_rowan::AstNode;
 
 impl ToFormatElement for JsInExpression {
     fn to_format_element(&self, formatter: &Formatter) -> FormatResult<FormatElement> {
-        format_binaryish_expression(self.syntax(), formatter)
+        format_binary_like_expression(
+            JsAnyBinaryLikeExpression::JsInExpression(self.clone()),
+            formatter,
+        )
     }
 }

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

@@ -1,10 +1,12 @@
-use crate::utils::format_binaryish_expression;
+use crate::utils::{format_binary_like_expression, JsAnyBinaryLikeExpression};
 use crate::{FormatElement, FormatResult, Formatter, ToFormatElement};
 use rome_js_syntax::JsInstanceofExpression;
-use rome_rowan::AstNode;
 
 impl ToFormatElement for JsInstanceofExpression {
     fn to_format_element(&self, formatter: &Formatter) -> FormatResult<FormatElement> {
-        format_binaryish_expression(self.syntax(), formatter)
+        format_binary_like_expression(
+            JsAnyBinaryLikeExpression::JsInstanceofExpression(self.clone()),
+            formatter,
+        )
     }
 }

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

@@ -1,10 +1,12 @@
-use crate::utils::format_binaryish_expression;
+use crate::utils::{format_binary_like_expression, JsAnyBinaryLikeExpression};
 use crate::{FormatElement, FormatResult, Formatter, ToFormatElement};
 use rome_js_syntax::JsLogicalExpression;
-use rome_rowan::AstNode;
 
 impl ToFormatElement for JsLogicalExpression {
     fn to_format_element(&self, formatter: &Formatter) -> FormatResult<FormatElement> {
-        format_binaryish_expression(self.syntax(), formatter)
+        format_binary_like_expression(
+            JsAnyBinaryLikeExpression::JsLogicalExpression(self.clone()),
+            formatter,
+        )
     }
 }

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

@@ -7,11 +7,14 @@ use rome_js_syntax::JsPostUpdateExpressionFields;
 
 impl ToFormatElement for JsPostUpdateExpression {
     fn to_format_element(&self, formatter: &Formatter) -> FormatResult<FormatElement> {
-        let JsPostUpdateExpressionFields { operand, operator } = self.as_fields();
+        let JsPostUpdateExpressionFields {
+            operand,
+            operator_token,
+        } = self.as_fields();
 
         Ok(format_elements![
             operand.format(formatter)?,
-            operator.format(formatter)?,
+            operator_token.format(formatter)?,
         ])
     }
 }

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

@@ -7,10 +7,13 @@ use rome_js_syntax::JsPreUpdateExpressionFields;
 
 impl ToFormatElement for JsPreUpdateExpression {
     fn to_format_element(&self, formatter: &Formatter) -> FormatResult<FormatElement> {
-        let JsPreUpdateExpressionFields { operator, operand } = self.as_fields();
+        let JsPreUpdateExpressionFields {
+            operator_token,
+            operand,
+        } = self.as_fields();
 
         Ok(format_elements![
-            operator.format(formatter)?,
+            operator_token.format(formatter)?,
             operand.format(formatter)?,
         ])
     }

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

@@ -11,13 +11,13 @@ impl ToFormatElement for JsStaticMemberExpression {
     fn to_format_element(&self, formatter: &Formatter) -> FormatResult<FormatElement> {
         let JsStaticMemberExpressionFields {
             object,
-            operator,
+            operator_token,
             member,
         } = self.as_fields();
 
         Ok(group_elements(format_elements![
             object.format(formatter)?,
-            operator.format(formatter)?,
+            operator_token.format(formatter)?,
             member.format(formatter)?,
         ]))
     }

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

@@ -6,27 +6,30 @@ use crate::{
     FormatResult, Formatter, ToFormatElement,
 };
 
-use rome_js_syntax::JsPreUpdateOperation;
+use rome_js_syntax::JsPreUpdateOperator;
 use rome_js_syntax::{JsAnyExpression, JsUnaryExpression};
-use rome_js_syntax::{JsUnaryExpressionFields, JsUnaryOperation};
+use rome_js_syntax::{JsUnaryExpressionFields, JsUnaryOperator};
 
 impl ToFormatElement for JsUnaryExpression {
     fn to_format_element(&self, formatter: &Formatter) -> FormatResult<FormatElement> {
-        let JsUnaryExpressionFields { operator, argument } = self.as_fields();
+        let JsUnaryExpressionFields {
+            operator_token,
+            argument,
+        } = self.as_fields();
 
-        let operation = self.operation()?;
-        let operator = operator?;
+        let operation = self.operator()?;
+        let operator_token = operator_token?;
         let argument = argument?;
 
         // Insert a space between the operator and argument if its a keyword
         let is_keyword_operator = matches!(
             operation,
-            JsUnaryOperation::Delete | JsUnaryOperation::Void | JsUnaryOperation::Typeof
+            JsUnaryOperator::Delete | JsUnaryOperator::Void | JsUnaryOperator::Typeof
         );
 
         if is_keyword_operator {
             return Ok(format_elements![
-                operator.format(formatter)?,
+                operator_token.format(formatter)?,
                 space_token(),
                 argument.format(formatter)?,
             ]);
@@ -36,19 +39,19 @@ impl ToFormatElement for JsUnaryExpression {
         // operation with an ambiguous operator (+ and ++ or - and --)
         let is_ambiguous_expression = match &argument {
             JsAnyExpression::JsUnaryExpression(expr) => {
-                let inner_op = expr.operation()?;
+                let inner_op = expr.operator()?;
                 matches!(
                     (operation, inner_op),
-                    (JsUnaryOperation::Plus, JsUnaryOperation::Plus)
-                        | (JsUnaryOperation::Minus, JsUnaryOperation::Minus)
+                    (JsUnaryOperator::Plus, JsUnaryOperator::Plus)
+                        | (JsUnaryOperator::Minus, JsUnaryOperator::Minus)
                 )
             }
             JsAnyExpression::JsPreUpdateExpression(expr) => {
-                let inner_op = expr.operation()?;
+                let inner_op = expr.operator()?;
                 matches!(
                     (operation, inner_op),
-                    (JsUnaryOperation::Plus, JsPreUpdateOperation::Increment)
-                        | (JsUnaryOperation::Minus, JsPreUpdateOperation::Decrement)
+                    (JsUnaryOperator::Plus, JsPreUpdateOperator::Increment)
+                        | (JsUnaryOperator::Minus, JsPreUpdateOperator::Decrement)
                 )
             }
             _ => false,
@@ -57,14 +60,14 @@ impl ToFormatElement for JsUnaryExpression {
         if is_ambiguous_expression {
             let parenthesized = if is_simple_expression(argument.clone())? {
                 format_elements![
-                    operator.format(formatter)?,
+                    operator_token.format(formatter)?,
                     token("("),
                     argument.format(formatter)?,
                     token(")"),
                 ]
             } else {
                 format_elements![
-                    operator.format(formatter)?,
+                    operator_token.format(formatter)?,
                     group_elements(format_elements![
                         token("("),
                         soft_block_indent(argument.format(formatter)?),
@@ -77,7 +80,7 @@ impl ToFormatElement for JsUnaryExpression {
         }
 
         Ok(format_elements![
-            operator.format(formatter)?,
+            operator_token.format(formatter)?,
             argument.format(formatter)?,
         ])
     }

crates/rome_js_formatter/src/js/lists/array_element_list.rs

@@ -33,11 +33,11 @@ impl ToFormatElement for JsArrayElementList {
 fn can_print_fill(list: &JsArrayElementList) -> bool {
     use rome_js_syntax::JsAnyArrayElement::*;
     use rome_js_syntax::JsAnyExpression::*;
-    use rome_js_syntax::JsUnaryOperation::*;
+    use rome_js_syntax::JsUnaryOperator::*;
 
     list.iter().all(|item| match item {
         Ok(JsAnyExpression(JsUnaryExpression(expr))) => {
-            match expr.operation() {
+            match expr.operator() {
                 Ok(Plus | Minus | BitwiseNot | LogicalNot) => {}
                 _ => return false,
             }

crates/rome_js_formatter/src/lib.rs

@@ -405,11 +405,10 @@ mod test {
     use rome_js_parser::{parse, SourceType};
 
     #[test]
-    #[ignore]
     // use this test check if your snippet prints as you wish, without using a snapshot
     fn quick_test() {
         let src = r#"
- const functionName1 = <T,>(arg) => false;
+a + b * c > 65 + 5;
 "#;
         let syntax = SourceType::tsx();
         let tree = parse(src, 0, syntax.clone());
@@ -423,7 +422,7 @@ mod test {
         });
         assert_eq!(
             result.as_code(),
-            r#"let g = [[], [0, 1], [0, 1]];
+            r#"(a + (b * c)) > (65 + 5);
 "#
         );
     }