Implement magic trailing comma for collections

This gives users a bit more control over whether things get formatted
wide or tall. This matters in practice, as Black learned the hard way,
and I did as well in the GitHub Actions example.

examples/github_actions.rcl

@@ -1,13 +1,19 @@
 {
   name = "Build",
 
-  on = { push = { branches = ["master"] }, workflow_dispatch = {} },
+  on = {
+    push = { branches = ["master"] },
+    workflow_dispatch = {},
+  },
 
   jobs = {
     "Build": {
       runs-on = "ubuntu-22.04",
       steps = [
-        { name = "Checkout", uses = "actions/checkout@v3.5.3" },
+        {
+          name = "Checkout",
+          uses = "actions/checkout@v3.5.3",
+        },
         {
           name = "Install Nix",
           uses = "cachix/install-nix-action@v18",

golden/fmt/call_multiline.test

@@ -1,4 +1,5 @@
-let x = f( a, b , c,);
+let x1 = f( a, b , c);
+let x2 = f( a, b , c,);
 let y = f(
   // The first argument.
   a,
@@ -7,7 +8,12 @@ let y = f(
 x + y
 
 # output:
-let x = f(a, b, c);
+let x1 = f(a, b, c);
+let x2 = f(
+  a,
+  b,
+  c,
+);
 let y = f(
   // The first argument.
   a,

golden/fmt/function.test

@@ -2,7 +2,9 @@ let f =
  (
  ) => "short value";
 
-let g1 = (arg1, arg2,) => [very_long_name_1, very_long_name_2, very_long_name_3, very_long_name_4, very_long_name_5];
+let g1 = (
+arg1,
+arg2) => [very_long_name_1, very_long_name_2, very_long_name_3, very_long_name_4, very_long_name_5];
 let g2 = (rec_self, k) =>
   if {0, 1}.contains(k):
     1
@@ -21,7 +23,7 @@ let doc2 = (
   arg1,
 
   // Argument 2.
-  arg2,
+  arg2
 ) => 42;
 
 let with_lets = (widgets, frobnicators) =>

golden/fmt/magic_trailing_comma.test

@@ -0,0 +1,150 @@
+// RCL implements Black's "magic trailing comma" behavior for lists of length >= 2.
+// https://black.readthedocs.io/en/stable/the_black_code_style/current_style.html#the-magic-trailing-comma
+// If there is a trailing comma originally, and there are at least two elements,
+// then the output is tall regardless of whether it would fit wide. If there is
+// no trailing comma, the output is whatever fits.
+
+// Length 1, trailing comma is not relevant.
+let a = [1,];
+let b = [1];
+let c = [
+  1
+];
+let d = [
+  1,
+];
+
+// Length 2, trailing comma forces tall.
+let a = [1, 2];
+let b = [
+  1, 2
+];
+let c = [1, 2,];
+let d = [
+  1, 2,
+];
+
+// Sets.
+let z = {1,};
+let a = {1, 2};
+let b = {
+  1, 2
+};
+let c = {1, 2,};
+let d = {
+  1, 2,
+};
+
+// Dicts.
+let z = {a=1,};
+let a = {a=1, b=2};
+let b = {
+  a=1, b=2
+};
+let c = {a=1, b=2,};
+let d = {
+  a=1, b=2,
+};
+
+// Function calls.
+let z = f(1,);
+let a = f(1, 2);
+let b = f(
+  1, 2
+);
+let c = f(1, 2,);
+let d = f(
+  1, 2,
+);
+
+// Function definitions.
+let z = (x,) => 1;
+let a = (x, y) => 1;
+let b = (
+  x, y
+) => 1;
+let c = (x, y,) => 1;
+let d = (
+  x, y,
+) => 1;
+
+null
+
+# output:
+// RCL implements Black's "magic trailing comma" behavior for lists of length >= 2.
+// https://black.readthedocs.io/en/stable/the_black_code_style/current_style.html#the-magic-trailing-comma
+// If there is a trailing comma originally, and there are at least two elements,
+// then the output is tall regardless of whether it would fit wide. If there is
+// no trailing comma, the output is whatever fits.
+
+// Length 1, trailing comma is not relevant.
+let a = [1];
+let b = [1];
+let c = [1];
+let d = [1];
+
+// Length 2, trailing comma forces tall.
+let a = [1, 2];
+let b = [1, 2];
+let c = [
+  1,
+  2,
+];
+let d = [
+  1,
+  2,
+];
+
+// Sets.
+let z = {1};
+let a = {1, 2};
+let b = {1, 2};
+let c = {
+  1,
+  2,
+};
+let d = {
+  1,
+  2,
+};
+
+// Dicts.
+let z = { a = 1 };
+let a = { a = 1, b = 2 };
+let b = { a = 1, b = 2 };
+let c = {
+  a = 1,
+  b = 2,
+};
+let d = {
+  a = 1,
+  b = 2,
+};
+
+// Function calls.
+let z = f(1);
+let a = f(1, 2);
+let b = f(1, 2);
+let c = f(
+  1,
+  2,
+);
+let d = f(
+  1,
+  2,
+);
+
+// Function definitions.
+let z = x => 1;
+let a = (x, y) => 1;
+let b = (x, y) => 1;
+let c = (
+  x,
+  y,
+) => 1;
+let d = (
+  x,
+  y,
+) => 1;
+
+null

golden/fmt/number_literals.test

@@ -5,4 +5,8 @@
 ]
 
 # output:
-[0xffff_ffff, 0b0101_0101, 1e10]
+[
+  0xffff_ffff,
+  0b0101_0101,
+  1e10,
+]

src/fmt_cst.rs

@@ -64,17 +64,23 @@ impl<'a> Formatter<'a> {
         Doc::Concat(result)
     }
 
-    /// A soft break if the collection is not empty.
+    /// The separator to add after a collection's opening punctuation.
     ///
     /// This is used in collection literals. If there are elements, then we have
-    /// a soft break between the opening delimiter and content, and between the
+    /// a hard or soft break between the opening delimiter and content, and
-    /// content and closing delimiter. But if we have no content, then we need
+    /// between the content and closing delimiter. But if we have no content
-    /// only one soft break.
+    /// (but possibly a suffix) then we need only one soft break.
-    pub fn soft_break_if_not_empty<T>(&self, elems: &[T]) -> Option<Doc<'a>> {
+    pub fn collection_opening_sep<T>(&self, list: &List<T>) -> Option<Doc<'a>> {
-        if elems.is_empty() {
+        // When there is a list of at least two elements, and there is a trailing
-            None
+        // comma, then regardless of whether the list would fit in wide mode, we
-        } else {
+        // force it to be tall, to give the user some control over wide/tall.
-            Some(Doc::SoftBreak)
+        // This is inspired by Black's "magic trailing comma":
+        // https://black.readthedocs.io/en/stable/the_black_code_style/current_style.html#the-magic-trailing-comma
+        match list.elements.len() {
+            0 => None,
+            1 => Some(Doc::SoftBreak),
+            _ if list.trailing_comma => Some(Doc::HardBreak),
+            _ => Some(Doc::SoftBreak),
         }
     }
 
@@ -289,9 +295,10 @@ impl<'a> Formatter<'a> {
                 if elements.elements.is_empty() && elements.suffix.is_empty() {
                     Doc::str("{}")
                 } else {
-                    let sep = self
+                    let sep = match self.collection_opening_sep(&elements) {
-                        .sep_key_value(&elements.elements)
+                        Some(Doc::HardBreak) => Some(Doc::HardBreak),
-                        .or(self.soft_break_if_not_empty(&elements.elements));
+                        other => self.sep_key_value(&elements.elements).or(other),
+                    };
                     group! {
                         "{"
                         sep
@@ -307,7 +314,7 @@ impl<'a> Formatter<'a> {
                 } else {
                     group! {
                         "["
-                        self.soft_break_if_not_empty(&elements.elements)
+                        self.collection_opening_sep(elements)
                         indent! { self.seqs(elements) }
                         "]"
                     }
@@ -396,7 +403,7 @@ impl<'a> Formatter<'a> {
                     }
                     _ => group! {
                         "("
-                        Doc::SoftBreak
+                        self.collection_opening_sep(args)
                         indent! {
                             Doc::join(
                                 args.elements.iter().map(|arg| concat! {
@@ -473,7 +480,7 @@ impl<'a> Formatter<'a> {
                 Chain::Call { args, .. } => {
                     let call_doc = group! {
                         "("
-                        Doc::SoftBreak
+                        self.collection_opening_sep(args)
                         indent! {
                             Doc::join(
                                 args.elements.iter().map(|(_span, arg)| self.prefixed_expr(arg)),
@@ -530,19 +537,17 @@ impl<'a> Formatter<'a> {
 
             let is_last = i + 1 == seqs.elements.len();
             let sep_doc = match i {
+                // If there is suffix noncode, then we need the separator before
+                // it, otherwise we would output a syntax error.
+                _ if !seqs.suffix.is_empty() => Doc::str(","),
                 // For collections that contain a single comprehension, do not
                 // add a separator, even when they are multi-line. It makes
                 // comprehensions look weird, which are regularly multi-line
                 // but only rarely are there multiple seqs in the collection.
-                // If there is suffix noncode, then we need the separator before
+                _ if seqs.elements.len() == 1 => match seqs.elements[0].inner.is_comprehension() {
-                // it, otherwise we would output a syntax error.
+                    true => Doc::Empty,
-                _ if seqs.elements.len() == 1 && seqs.suffix.is_empty() => {
+                    false => Doc::tall(","),
-                    if seqs.elements[0].inner.is_comprehension() {
+                },
-                        Doc::Empty
-                    } else {
-                        Doc::tall(",")
-                    }
-                }
                 _ if is_last => Doc::tall(","),
                 _ => Doc::str(","),
             };