Change binary operation formatting (#2473)

Co-authored-by: Louis Pilfold <[email protected]>

Author: giacomocavalieri

CHANGELOG.md

@@ -2,11 +2,18 @@
 
 ## Unreleased
 
+### Formatter
+
+- The formatter now tries to split long chains of binary operations around the
+  operator itself, rather than around other elements like lists or function
+  calls.
+
 ### Bug fixes
 
 - Fixed a bug where string prefix aliases defined in alternative case branches
   would all be bound to the same constant.
 
+
 ## v0.33.0-rc2 - 2023-12-07
 
 ### Language changes

compiler-core/src/format.rs

@@ -958,14 +958,50 @@ impl<'comments> Formatter<'comments> {
         let precedence = name.precedence();
         let left_precedence = left.binop_precedence();
         let right_precedence = right.binop_precedence();
-        let left = self.expr(left);
-        let right = self.expr(right);
-        self.operator_side(left, precedence, left_precedence)
-            .append(name)
-            .append(self.operator_side(right, precedence, right_precedence - 1))
+        let left_doc = self.expr(left);
+        let right_doc = self.expr(right);
+
+        let mut doc = docvec!();
+        doc = match Self::binop_name(left) {
+            // In case the left side is a binary operation as well and it can
+            // be grouped together with the current binary operation, the two
+            // docs are simply concatenated, so that they will end up in the
+            // same group and the formatter will try to keep those on a single
+            // line.
+            Some(left_name) if Self::should_be_grouped_together(left_name, name) => {
+                doc.append(left_doc)
+            }
+            // In case the binary operations cannot be grouped together the left
+            // side is treated as a group on its own so that this operation can
+            // be broken independently.
+            _ => doc.append(self.operator_side(left_doc.group(), precedence, left_precedence)),
+        };
+        doc = doc.append(break_("", " ")).append(name).append(" ");
+        doc = match Self::binop_name(right) {
+            // This works the same as the left side
+            Some(right_name) if Self::should_be_grouped_together(right_name, name) => {
+                doc.append(right_doc)
+            }
+            _ => doc.append(self.operator_side(right_doc.group(), precedence, right_precedence)),
+        };
+        doc
+    }
+
+    fn binop_name(expr: &UntypedExpr) -> Option<&BinOp> {
+        match expr {
+            UntypedExpr::BinOp { name, .. } => Some(name),
+            _ => None,
+        }
+    }
+
+    fn should_be_grouped_together(one: &BinOp, other: &BinOp) -> bool {
+        // Try to keep on a single line groups of binary operations with the
+        // same name, or binary operations on booleans.
+        let is_boolean_binop = |binop| matches!(binop, BinOp::And | BinOp::Or);
+        one == other || (is_boolean_binop(*one) && is_boolean_binop(*other))
     }
 
-    pub fn operator_side<'a>(&mut self, doc: Document<'a>, op: u8, side: u8) -> Document<'a> {
+    pub fn operator_side<'a>(&self, doc: Document<'a>, op: u8, side: u8) -> Document<'a> {
         if op > side {
             wrap_block(doc).group()
         } else {
@@ -1676,28 +1712,28 @@ impl<'a> Documentable<'a> for &'a UnqualifiedImport {
 impl<'a> Documentable<'a> for &'a BinOp {
     fn to_doc(self) -> Document<'a> {
         match self {
-            BinOp::And => " && ",
-            BinOp::Or => " || ",
-            BinOp::LtInt => " < ",
-            BinOp::LtEqInt => " <= ",
-            BinOp::LtFloat => " <. ",
-            BinOp::LtEqFloat => " <=. ",
-            BinOp::Eq => " == ",
-            BinOp::NotEq => " != ",
-            BinOp::GtEqInt => " >= ",
-            BinOp::GtInt => " > ",
-            BinOp::GtEqFloat => " >=. ",
-            BinOp::GtFloat => " >. ",
-            BinOp::AddInt => " + ",
-            BinOp::AddFloat => " +. ",
-            BinOp::SubInt => " - ",
-            BinOp::SubFloat => " -. ",
-            BinOp::MultInt => " * ",
-            BinOp::MultFloat => " *. ",
-            BinOp::DivInt => " / ",
-            BinOp::DivFloat => " /. ",
-            BinOp::RemainderInt => " % ",
-            BinOp::Concatenate => " <> ",
+            BinOp::And => "&&",
+            BinOp::Or => "||",
+            BinOp::LtInt => "<",
+            BinOp::LtEqInt => "<=",
+            BinOp::LtFloat => "<.",
+            BinOp::LtEqFloat => "<=.",
+            BinOp::Eq => "==",
+            BinOp::NotEq => "!=",
+            BinOp::GtEqInt => ">=",
+            BinOp::GtInt => ">",
+            BinOp::GtEqFloat => ">=.",
+            BinOp::GtFloat => ">.",
+            BinOp::AddInt => "+",
+            BinOp::AddFloat => "+.",
+            BinOp::SubInt => "-",
+            BinOp::SubFloat => "-.",
+            BinOp::MultInt => "*",
+            BinOp::MultFloat => "*.",
+            BinOp::DivInt => "/",
+            BinOp::DivFloat => "/.",
+            BinOp::RemainderInt => "%",
+            BinOp::Concatenate => "<>",
         }
         .to_doc()
     }

compiler-core/src/format/tests.rs

@@ -3696,7 +3696,8 @@ pub fn main(
 fn commented_binop() {
     assert_format!(
         "fn main() {
-  1 + // hello
+  1
+  + // hello
   2
 }
 "
@@ -3705,8 +3706,10 @@ fn commented_binop() {
     assert_format!(
         "fn main() {
   // one
-  1 + // two
-  2 + // three
+  1
+  + // two
+  2
+  + // three
   3
 }
 "
@@ -3971,7 +3974,8 @@ fn binary_operator_precedence() {
 
     assert_format!(
         "fn main() {
-  3 * {
+  3
+  * {
     1
     |> inc
   }
@@ -3984,7 +3988,8 @@ fn binary_operator_precedence() {
   {
     1
     |> inc
-  } * 3
+  }
+  * 3
 }
 "
     );
@@ -4999,13 +5004,35 @@ fn wrap_long_line_with_int_negation() {
         r#"pub fn main() {
   let a = 3
   let b =
-    a * a * a * a * a * a * a * a * a * a * a * a * a * {
-      a * a * a * a * a * a * a * a * a * a
-    }
+    a
+    * a
+    * a
+    * a
+    * a
+    * a
+    * a
+    * a
+    * a
+    * a
+    * a
+    * a
+    * a
+    * { a * a * a * a * a * a * a * a * a * a }
   let c =
-    c * c * c * c * c * c * c * c * c * c * c * c * c * -{
-      c * c * c * c * c * c * c * c * c * c
-    }
+    c
+    * c
+    * c
+    * c
+    * c
+    * c
+    * c
+    * c
+    * c
+    * c
+    * c
+    * c
+    * c
+    * -{ c * c * c * c * c * c * c * c * c * c }
 }
 "#
     );
@@ -5023,13 +5050,35 @@ fn wrap_long_line_with_bool_negation() {
         r#"pub fn main() {
   let a = True
   let b =
-    a || a || a || a || a || a || a || a || a || a || a || a || a || {
-      a || a || a || a || a || a || a || a || a || a
-    }
+    a
+    || a
+    || a
+    || a
+    || a
+    || a
+    || a
+    || a
+    || a
+    || a
+    || a
+    || a
+    || a
+    || { a || a || a || a || a || a || a || a || a || a }
   let c =
-    c || c || c || c || c || c || c || c || c || c || c || c || c || !{
-      c || c || c || c || c || c || c || c || c || c
-    }
+    c
+    || c
+    || c
+    || c
+    || c
+    || c
+    || c
+    || c
+    || c
+    || c
+    || c
+    || c
+    || c
+    || !{ c || c || c || c || c || c || c || c || c || c }
 }
 "#
     );
@@ -5291,3 +5340,38 @@ fn single_argument_call_nested_nested() {
 "#
     );
 }
+
+#[test]
+pub fn long_binary_operation_sequence() {
+    assert_format!(
+        r#"pub fn main() {
+  int.to_string(color.red)
+  <> ", "
+  <> int.to_string(color.green)
+  <> ", "
+  <> int.to_string(color.blue)
+  <> ", "
+  <> float.to_string(color.alpha)
+}
+"#
+    );
+}
+
+#[test]
+pub fn long_comparison_chain() {
+    assert_format!(
+        r#"pub fn main() {
+  trying_a_comparison(this, is, a, function) > with_ints
+  && trying_other_comparisons < with_ints
+  || trying_other_comparisons <= with_ints
+  && trying_other_comparisons >= with_ints
+  || and_now_an_equality_check == with_a_function(foo, bar)
+  && trying_other_comparisons >. with_floats
+  || trying_other_comparisons <. with_floats(baz)
+  && trying_other_comparisons <=. with_floats
+  || trying_other_comparisons(foo, bar) >=. with_floats
+  && foo <> bar
+}
+"#
+    );
+}