fix(formatter): Stable member chain printing (#2582)

This PR simplifies the member printing logic to make sure its output is stable.
This results in a small regression

```
Before:
**File Based Average Prettier Similarity**: 69.29%
**Line Based Average Prettier Similarity**: 64.28%

After:
**File Based Average Prettier Similarity**: 69.13%
**Line Based Average Prettier Similarity**: 64.06%
```

But unblocks my work on refactoring the printer behaviour (which seems to trigger this now a lot), and #2458

crates/rome_js_formatter/src/utils/member_chain/flatten_item.rs

@@ -6,7 +6,6 @@ use rome_js_syntax::{
 };
 use rome_rowan::{AstNode, SyntaxResult};
 use std::fmt::Debug;
-use std::slice;
 
 #[derive(Clone)]
 /// Data structure that holds the node with its formatted version
@@ -35,15 +34,6 @@ impl FlattenItem {
         }
     }
 
-    pub(crate) fn as_format_elements(&self) -> &[FormatElement] {
-        match self {
-            FlattenItem::StaticMember(_, elements) => elements,
-            FlattenItem::CallExpression(_, elements) => elements,
-            FlattenItem::ComputedExpression(_, elements) => elements,
-            FlattenItem::Node(_, element) => slice::from_ref(element),
-        }
-    }
-
     pub(crate) fn as_syntax(&self) -> &JsSyntaxNode {
         match self {
             FlattenItem::StaticMember(node, _) => node.syntax(),
@@ -53,15 +43,6 @@ impl FlattenItem {
         }
     }
 
-    pub(crate) fn has_leading_comments(&self) -> bool {
-        match self {
-            FlattenItem::StaticMember(node, _) => node.syntax().has_leading_comments(),
-            FlattenItem::CallExpression(node, _) => node.syntax().has_leading_comments(),
-            FlattenItem::ComputedExpression(node, _) => node.syntax().has_leading_comments(),
-            FlattenItem::Node(node, _) => node.has_leading_comments(),
-        }
-    }
-
     pub(crate) fn has_trailing_comments(&self) -> bool {
         match self {
             FlattenItem::StaticMember(node, _) => node.syntax().has_trailing_comments(),

crates/rome_js_formatter/src/utils/member_chain/groups.rs

@@ -2,7 +2,7 @@ use crate::prelude::*;
 use crate::utils::member_chain::flatten_item::FlattenItem;
 use crate::utils::member_chain::simple_argument::SimpleArgument;
 
-use rome_js_syntax::{JsAnyCallArgument, JsAnyExpression, JsCallExpression};
+use rome_js_syntax::JsCallExpression;
 use rome_rowan::{AstSeparatedList, SyntaxResult};
 use std::mem;
 
@@ -78,48 +78,19 @@ impl<'f> Groups<'f> {
     }
 
     /// It tells if the groups should be break on multiple lines
-    pub fn groups_should_break(
-        &self,
-        calls_count: usize,
-        head_group: &HeadGroup,
-    ) -> SyntaxResult<bool> {
+    pub fn groups_should_break(&self, calls_count: usize) -> SyntaxResult<bool> {
         // Do not allow the group to break if it only contains a single call expression
         if calls_count <= 1 {
             return Ok(false);
         }
 
-        let node_has_comments = self.has_comments() || head_group.has_comments();
         // we want to check the simplicity of the call expressions only if we have at least
         // two of them
         // Check prettier: https://github.com/prettier/prettier/blob/main/src/language-js/print/member-chain.js#L389
-        let call_expressions_are_not_simple = if calls_count > 2 {
-            self.call_expressions_are_not_simple()?
-        } else {
-            false
-        };
-        let last_group_will_break_and_other_calls_have_function_arguments =
-            self.last_group_will_break_and_other_calls_have_function_arguments()?;
-
-        // This emulates a simplified version of the similar logic found in the
-        // printer to force groups to break if they contain any "hard line
-        // break" (these not only include hard_line_break elements but also
-        // empty_line or tokens containing the "\n" character): The idea is
-        // that since any of these will force the group to break when it gets
-        // printed, the formatter needs to emit a group element for the call
-        // chain in the first place or it will not be printed correctly
-        let has_line_breaks = self
-            .groups
-            .iter()
-            .flat_map(|group| group.iter())
-            .flat_map(|item| item.as_format_elements())
-            .any(|element| element.will_break());
-
-        let should_break = has_line_breaks
-            || node_has_comments
-            || call_expressions_are_not_simple
-            || last_group_will_break_and_other_calls_have_function_arguments;
+        let call_expressions_are_not_simple =
+            calls_count > 2 && self.call_expressions_are_not_simple()?;
 
-        Ok(should_break)
+        Ok(call_expressions_are_not_simple)
     }
 
     fn into_formatted_groups(self) -> Vec<FormatElement> {
@@ -139,46 +110,9 @@ impl<'f> Groups<'f> {
     /// and the other one that goes on multiple lines.
     ///
     /// It's up to the printer to decide which one to use.
-    pub fn into_format_elements(self) -> (FormatElement, FormatElement) {
+    pub fn into_format_elements(self) -> FormatElement {
         let formatted_groups = self.into_formatted_groups();
-        (
-            concat_elements(formatted_groups.clone()),
-            join_elements(soft_line_break(), formatted_groups),
-        )
-    }
-
-    /// Checks if the groups contain comments.
-    pub fn has_comments(&self) -> bool {
-        let has_leading_comments = if self.groups.len() > 1 {
-            // SAFETY: access guarded by the previous check
-            self.groups
-                .iter()
-                .flat_map(|item| item.iter())
-                .skip(1)
-                .any(|item| item.has_leading_comments())
-        } else {
-            false
-        };
-        let has_trailing_comments = self
-            .groups
-            .iter()
-            .flat_map(|item| item.iter())
-            .any(|item| item.has_trailing_comments());
-
-        // This check might not be needed... trying to understand why Prettier has it
-        let cutoff_has_leading_comments = if self.groups.len() >= self.cutoff as usize {
-            self.groups
-                .get(self.cutoff as usize)
-                .map_or(false, |group| {
-                    group
-                        .first()
-                        .map_or(false, |group| group.has_leading_comments())
-                })
-        } else {
-            false
-        };
-
-        has_leading_comments || has_trailing_comments || cutoff_has_leading_comments
+        concat_elements(formatted_groups)
     }
 
     /// Filters the stack of [FlattenItem] and return only the ones that
@@ -210,48 +144,6 @@ impl<'f> Groups<'f> {
         }))
     }
 
-    /// Checks if the last group will break - by emulating the behaviour of the printer,
-    /// or if there's a call expression that contain a function/arrow function as argument
-    pub fn last_group_will_break_and_other_calls_have_function_arguments(
-        &self,
-    ) -> SyntaxResult<bool> {
-        let last_group = self.groups.iter().flat_map(|group| group.iter()).last();
-
-        if let Some(last_group) = last_group {
-            let element = last_group.as_format_elements().last();
-            let group_will_break = element.map_or(false, |element| element.will_break());
-
-            let is_call_expression = last_group.is_loose_call_expression();
-
-            Ok(group_will_break
-                && is_call_expression
-                && self.call_expressions_have_function_or_arrow_func_as_argument()?)
-        } else {
-            Ok(false)
-        }
-    }
-
-    /// Checks if any of the call expressions contains arguments that are functions or arrow
-    /// functions.
-    pub fn call_expressions_have_function_or_arrow_func_as_argument(&self) -> SyntaxResult<bool> {
-        for call_expression in self.get_call_expressions() {
-            let arguments = call_expression.arguments()?;
-            for argument in arguments.args() {
-                if matches!(
-                    argument?,
-                    JsAnyCallArgument::JsAnyExpression(JsAnyExpression::JsFunctionExpression(_))
-                        | JsAnyCallArgument::JsAnyExpression(
-                            JsAnyExpression::JsArrowFunctionExpression(_)
-                        )
-                ) {
-                    return Ok(true);
-                }
-            }
-        }
-
-        Ok(false)
-    }
-
     /// This is an heuristic needed to check when the first element of the group
     /// Should be part of the "head" or the "tail".
     fn should_not_wrap(&self, first_group: &HeadGroup) -> SyntaxResult<bool> {
@@ -332,8 +224,4 @@ impl HeadGroup {
     pub fn expand_group(&mut self, group: Vec<FlattenItem>) {
         self.items.extend(group)
     }
-
-    fn has_comments(&self) -> bool {
-        self.items.iter().any(|item| item.has_trailing_comments())
-    }
 }

crates/rome_js_formatter/src/utils/member_chain/mod.rs

@@ -289,7 +289,8 @@ fn format_groups(
     head_group: HeadGroup,
     groups: Groups,
 ) -> FormatResult<FormatElement> {
-    if groups.groups_should_break(calls_count, &head_group)? {
+    // TODO use Alternatives once available
+    if groups.groups_should_break(calls_count)? {
         Ok(format_elements![
             head_group.into_format_element(),
             group_elements(indent(format_elements![
@@ -298,13 +299,10 @@ fn format_groups(
             ]),)
         ])
     } else {
-        let head_formatted = head_group.into_format_element();
-        let (one_line, _) = groups.into_format_elements();
-
-        // TODO: this is not the definitive solution, as there are few restrictions due to how the printer works:
-        // - groups that contains other groups with hard lines break all the groups
-        // - conditionally print one single line is subject to how the printer works (by default, multiline)
-        Ok(format_elements![head_formatted, one_line])
+        Ok(format_elements![
+            head_group.into_format_element(),
+            groups.into_format_elements()
+        ])
     }
 }
 

crates/rome_js_formatter/tests/specs/js/module/expression/member-chain/complex_arguments.js.snap

@@ -1,8 +1,6 @@
 ---
 source: crates/rome_js_formatter/tests/spec_test.rs
-assertion_line: 242
 expression: complex_arguments.js
-
 ---
 # Input
 client.execute(

crates/rome_js_formatter/tests/specs/js/module/expression/member-chain/computed.js.snap

@@ -1,6 +1,5 @@
 ---
 source: crates/rome_js_formatter/tests/spec_test.rs
-assertion_line: 242
 expression: computed.js
 ---
 # Input

crates/rome_js_formatter/tests/specs/js/module/expression/member-chain/inline-merge.js.snap

@@ -1,8 +1,6 @@
 ---
 source: crates/rome_js_formatter/tests/spec_test.rs
-assertion_line: 242
 expression: inline-merge.js
-
 ---
 # Input
 _.flatMap(this.visibilityHandlers, fn => fn())
@@ -30,10 +28,9 @@ _.flatMap(this.visibilityHandlers, (fn) => fn())
 	.concat(this.record.resolved_legacy_visrules)
 	.filter(Boolean);
 
-Object.keys(availableLocales({ test: true }))
-	.forEach(
-		(locale) => {
-			// ...
-		},
-	);
+Object.keys(availableLocales({ test: true })).forEach(
+	(locale) => {
+		// ...
+	},
+);
 

crates/rome_js_formatter/tests/specs/prettier/js/arrays/holes-in-args.js.snap

@@ -1,6 +1,5 @@
 ---
 source: crates/rome_js_formatter/tests/prettier_tests.rs
-assertion_line: 175
 expression: holes-in-args.js
 ---
 # Input

crates/rome_js_formatter/tests/specs/prettier/js/async/inline-await.js.snap

@@ -1,8 +1,6 @@
 ---
 source: crates/rome_js_formatter/tests/prettier_tests.rs
-assertion_line: 125
 expression: inline-await.js
-
 ---
 # Input
 ```js

crates/rome_js_formatter/tests/specs/prettier/js/comments/dangling_array.js.snap

@@ -1,8 +1,6 @@
 ---
 source: crates/rome_js_formatter/tests/prettier_tests.rs
-assertion_line: 125
 expression: dangling_array.js
-
 ---
 # Input
 ```js
@@ -16,10 +14,9 @@ expect(() => {}).toTriggerReadyStateChanges([
 
 # Output
 ```js
-expect(() => {})
-  .toTriggerReadyStateChanges([
-    // Nothing.
-  ]);
+expect(() => {}).toTriggerReadyStateChanges([
+  // Nothing.
+]);
 
 [1 /* first comment */ , 2 /* second comment */ , 3];
 

crates/rome_js_formatter/tests/specs/prettier/js/function-first-param/function_expression.js.snap

@@ -1,6 +1,5 @@
 ---
 source: crates/rome_js_formatter/tests/prettier_tests.rs
-assertion_line: 175
 expression: function_expression.js
 ---
 # Input
@@ -35,29 +34,25 @@ db.collection('indexOptionDefault').createIndex({ a: 1 }, {
 # Output
 ```js
 //https://github.com/prettier/prettier/issues/3002
-beep
-  .boop()
-  .baz(
-    "foo",
-    { some: { thing: { nested: true } } },
-    { another: { thing: true } },
-    () => {},
-  );
+beep.boop().baz(
+  "foo",
+  { some: { thing: { nested: true } } },
+  { another: { thing: true } },
+  () => {},
+);
 
 //https://github.com/prettier/prettier/issues/2984
-db
-  .collection("indexOptionDefault")
-  .createIndex(
-    { a: 1 },
-    { indexOptionDefaults: true, w: 2, wtimeout: 1000 },
-    function (err) {
-      test.equal(null, err);
-      test.deepEqual({ w: 2, wtimeout: 1000 }, commandResult.writeConcern);
-
-      client.close();
-      done();
-    },
-  );
+db.collection("indexOptionDefault").createIndex(
+  { a: 1 },
+  { indexOptionDefaults: true, w: 2, wtimeout: 1000 },
+  function (err) {
+    test.equal(null, err);
+    test.deepEqual({ w: 2, wtimeout: 1000 }, commandResult.writeConcern);
+
+    client.close();
+    done();
+  },
+);
 
 ```