Respect line breaks in operator chains and argument lists (#1268)

If an operator chain or method call is multiline, keep those newlines
in the formatted output.

Help preserve code like:

```
foo
  |> (it) -> it + 2
  |> (it) -> it / 2
```

docs/modules/release-notes/pages/0.30.adoc

@@ -25,7 +25,7 @@ To get started, follow xref:pkl-cli:index.adoc#installation[Installation].#
 [[formatter]]
 === Formatter
 
-Pkl now has a formatter (https://github.com/apple/pkl/pull/1107[#1107], https://github.com/apple/pkl/pull/1208[#1208], https://github.com/apple/pkl/pull/1211[#1211], https://github.com/apple/pkl/pull/1215[#1215], https://github.com/apple/pkl/pull/1217[#1217], https://github.com/apple/pkl/pull/1235[#1235], https://github.com/apple/pkl/pull/1246[#1246], https://github.com/apple/pkl/pull/1247[#1247], https://github.com/apple/pkl/pull/1252[#1252], https://github.com/apple/pkl/pull/1256[#1256], https://github.com/apple/pkl/pull/1259[#1259], https://github.com/apple/pkl/pull/1260[#1260], https://github.com/apple/pkl/pull/1263[#1263], https://github.com/apple/pkl/pull/1265[#1265], https://github.com/apple/pkl/pull/1266[#1266], https://github.com/apple/pkl/pull/1267[#1267], https://github.com/apple/pkl/pull/1270[#1270], https://github.com/apple/pkl/pull/1271[#1271], https://github.com/apple/pkl/pull/1272[#1272], https://github.com/apple/pkl/pull/1273[#1273], https://github.com/apple/pkl/pull/1274[#1274], https://github.com/apple/pkl/pull/1280[#1280])!
+Pkl now has a formatter (https://github.com/apple/pkl/pull/1107[#1107], https://github.com/apple/pkl/pull/1208[#1208], https://github.com/apple/pkl/pull/1211[#1211], https://github.com/apple/pkl/pull/1215[#1215], https://github.com/apple/pkl/pull/1217[#1217], https://github.com/apple/pkl/pull/1235[#1235], https://github.com/apple/pkl/pull/1246[#1246], https://github.com/apple/pkl/pull/1247[#1247], https://github.com/apple/pkl/pull/1252[#1252], https://github.com/apple/pkl/pull/1256[#1256], https://github.com/apple/pkl/pull/1259[#1259], https://github.com/apple/pkl/pull/1260[#1260], https://github.com/apple/pkl/pull/1263[#1263], https://github.com/apple/pkl/pull/1265[#1265], https://github.com/apple/pkl/pull/1266[#1266], https://github.com/apple/pkl/pull/1267[#1267], https://github.com/apple/pkl/pull/1268[#1268], https://github.com/apple/pkl/pull/1270[#1270], https://github.com/apple/pkl/pull/1271[#1271], https://github.com/apple/pkl/pull/1272[#1272], https://github.com/apple/pkl/pull/1273[#1273], https://github.com/apple/pkl/pull/1274[#1274], https://github.com/apple/pkl/pull/1280[#1280])!
 
 Pkl's formatter is _canonical_, which means that it has a single set of formatting rules, with (almost) no configuration options.
 The goal is to eliminate the possibility of formatting debates, which can lead to churn and bike-shedding.

pkl-formatter/src/main/kotlin/org/pkl/formatter/Builder.kt

@@ -299,7 +299,15 @@ internal class Builder(sourceText: String, private val grammarVersion: GrammarVe
 
     gatherFacts(node)
 
-    val separator: (Node, Node) -> FormatNode? = { prev, next ->
+    val leadingSeparator: (Node, Node) -> FormatNode? = { prev, next ->
+      when {
+        prev.type == NodeType.OPERATOR -> null
+        next.type == NodeType.OPERATOR -> line()
+        else -> spaceOrLine()
+      }
+    }
+
+    val trailingSeparator: (Node, Node) -> FormatNode? = { prev, next ->
       when {
         prev.type == NodeType.OPERATOR -> null
         next.type == NodeType.OPERATOR -> if (lambdaCount > 1) forceLine() else line()
@@ -312,8 +320,9 @@ internal class Builder(sourceText: String, private val grammarVersion: GrammarVe
         methodCallCount == 1 && isMethodCall(flat.lastProperNode()!!) -> {
           // lift argument list into its own node
           val (callChain, argsList) = splitFunctionCallNode(flat)
-          val leadingNodes = indentAfterFirstNewline(formatGeneric(callChain, separator), true)
+          val leadingNodes =
-          val trailingNodes = formatGeneric(argsList, separator)
+            indentAfterFirstNewline(formatGeneric(callChain, leadingSeparator), true)
+          val trailingNodes = formatGeneric(argsList, trailingSeparator)
           val sep = getBaseSeparator(callChain.last(), argsList.first())
           if (sep != null) {
             leadingNodes + sep + trailingNodes
@@ -324,11 +333,11 @@ internal class Builder(sourceText: String, private val grammarVersion: GrammarVe
         methodCallCount > 0 && indexBeforeFirstMethodCall > 0 -> {
           val leading = flat.subList(0, indexBeforeFirstMethodCall)
           val trailing = flat.subList(indexBeforeFirstMethodCall, flat.size)
-          val leadingNodes = indentAfterFirstNewline(formatGeneric(leading, separator), true)
+          val leadingNodes = indentAfterFirstNewline(formatGeneric(leading, leadingSeparator), true)
-          val trailingNodes = formatGeneric(trailing, separator)
+          val trailingNodes = formatGeneric(trailing, trailingSeparator)
           leadingNodes + line() + trailingNodes
         }
-        else -> formatGeneric(flat, separator)
+        else -> formatGeneric(flat, trailingSeparator)
       }
 
     val shouldGroup = node.children.size == flat.size
@@ -585,10 +594,14 @@ internal class Builder(sourceText: String, private val grammarVersion: GrammarVe
     twoBy2: Boolean = false,
   ): FormatNode {
     val children = node.children
+    val shouldMultiline = shouldMultlineNodes(node) { it.isTerminal(",") }
+    val sep: (Node, Node) -> FormatNode = { _, _ ->
+      if (shouldMultiline) forceSpaceyLine() else spaceOrLine()
+    }
     return if (twoBy2) {
       val pairs = pairArguments(children)
       val nodes =
-        formatGenericWithGen(pairs, spaceOrLine()) { node, _ ->
+        formatGenericWithGen(pairs, sep) { node, _ ->
           if (node.type == NodeType.ARGUMENT_LIST_ELEMENTS) {
             Group(newId(), formatGeneric(node.children, spaceOrLine()))
           } else {
@@ -597,24 +610,35 @@ internal class Builder(sourceText: String, private val grammarVersion: GrammarVe
         }
       Indent(nodes)
     } else if (hasTrailingLambda) {
-      // if the args have a trailing expression (lambda, new, amends) group them differently
+      // if the args have a trailing lambda, group them differently
       val splitIndex = children.indexOfLast { it.type in SAME_LINE_EXPRS }
       val normalParams = children.subList(0, splitIndex)
       val lastParam = children.subList(splitIndex, children.size)
       val trailingNode = if (endsWithClosingBracket(lastParam.last())) Empty else line()
-      val lastNodes = formatGeneric(lastParam, spaceOrLine())
+      val lastNodes = formatGenericWithGen(lastParam, sep, null)
       if (normalParams.isEmpty()) {
         group(Group(newId(), lastNodes), trailingNode)
       } else {
         val separator = getSeparator(normalParams.last(), lastParam[0], Space)
-        val paramNodes = formatGeneric(normalParams, spaceOrLine())
+        val paramNodes = formatGenericWithGen(normalParams, sep, null)
         group(Group(newId(), paramNodes), separator, Group(newId(), lastNodes), trailingNode)
       }
     } else {
-      Indent(formatGeneric(children, spaceOrLine()))
+      Indent(formatGeneric(children, sep))
     }
   }
 
+  private fun shouldMultlineNodes(node: Node, predicate: (Node) -> Boolean): Boolean {
+    for (idx in 0..<node.children.lastIndex) {
+      val prev = node.children[idx]
+      val next = node.children[idx + 1]
+      if ((predicate(prev) || predicate(next)) && prev.linesBetween(next) > 0) {
+        return true
+      }
+    }
+    return false
+  }
+
   private tailrec fun endsWithClosingBracket(node: Node): Boolean {
     return if (node.children.isNotEmpty()) {
       endsWithClosingBracket(node.children.last())
@@ -998,19 +1022,21 @@ internal class Builder(sourceText: String, private val grammarVersion: GrammarVe
 
   private fun formatBinaryOpExpr(node: Node): FormatNode {
     val flat = flattenBinaryOperatorExprs(node)
+    val shouldMultiline = shouldMultlineNodes(node) { it.type == NodeType.OPERATOR }
     val nodes =
       formatGeneric(flat) { prev, next ->
+        val sep = if (shouldMultiline) forceSpaceyLine() else spaceOrLine()
         if (prev.type == NodeType.OPERATOR) {
           when (prev.text()) {
-            "-" -> spaceOrLine()
+            "-" -> sep
             else -> Space
           }
         } else if (next.type == NodeType.OPERATOR) {
           when (next.text()) {
             "-" -> Space
-            else -> spaceOrLine()
+            else -> sep
           }
-        } else spaceOrLine()
+        } else sep
       }
 
     val shouldGroup = node.children.size == flat.size
@@ -1396,15 +1422,15 @@ internal class Builder(sourceText: String, private val grammarVersion: GrammarVe
   private fun flattenBinaryOperatorExprs(node: Node, prec: Int): List<Node> {
     val actualOp = node.children.first { it.type == NodeType.OPERATOR }.text()
     if (prec != Operator.byName(actualOp).prec) return listOf(node)
-    val res = mutableListOf<Node>()
+    return buildList {
-    for (child in node.children) {
+      for (child in node.children) {
-      if (child.type == NodeType.BINARY_OP_EXPR) {
+        if (child.type == NodeType.BINARY_OP_EXPR) {
-        res += flattenBinaryOperatorExprs(child, prec)
+          addAll(flattenBinaryOperatorExprs(child, prec))
-      } else {
+        } else {
-        res += child
+          add(child)
+        }
       }
     }
-    return res
   }
 
   private fun Node.linesBetween(next: Node): Int = next.span.lineBegin - span.lineEnd

pkl-formatter/src/test/files/FormatterSnippetTests/input/expr-chain-grouping.pkl

@@ -29,3 +29,6 @@ res10 = foo.bar /* some comment */ (5)
 
 res11 = foo.bar /* some comment */.baz(new { foooooooooooooooooooooooooo = 1; baaaaaaaaaaaaaaaaaaaaar = 2 }).buz
 
+res12 = foo.biz
+  .bar((it) -> it + 2)
+  .qux((it) -> it + 1)

pkl-formatter/src/test/files/FormatterSnippetTests/input/line-breaks3.pkl

@@ -0,0 +1,58 @@
+// respect line breaks on:
+// * operator chains
+// * argument lists in method calls
+//
+// only look for line breaks on either side of the operator, or the comma.
+res1 =
+  foo
+    * bar
+    * baz
+
+res2 = foo
+  |> bar + bar
+  |> baz
+
+res3 = foo
+  |> bar
+    + baz
+  |> qux
+
+res4 = foo(
+  (it) -> it + 1,
+  (it) -> it + 2
+)
+
+res5 = foo(
+  1,
+  2, 3, 4
+)
+
+// don't respect newlines right after opening paren and closing paren
+res6 = foo.bar(
+  1, 2, 3
+)
+
+// only respect newlines on either side of a binary operator (we still want to inline `bar(3)` here).
+res7 = foo + bar(
+      3
+    ) + baz
+
+// only the newlines after each "entry" are respected 
+res8 = Map(
+  1, 2,
+  3, 4
+)
+
+res9 = Map(
+  1,
+  2,
+  3, 4
+)
+
+res10 = foo(1
+  ,2
+  ,3)
+
+res11 = foo +
+  bar +
+  baz

pkl-formatter/src/test/files/FormatterSnippetTests/output/expr-chain-grouping.pkl

@@ -63,3 +63,8 @@ res11 =
   foo.bar /* some comment */
     .baz(new { foooooooooooooooooooooooooo = 1; baaaaaaaaaaaaaaaaaaaaar = 2 })
     .buz
+
+res12 =
+  foo.biz
+    .bar((it) -> it + 2)
+    .qux((it) -> it + 1)

pkl-formatter/src/test/files/FormatterSnippetTests/output/line-breaks3.pkl

@@ -0,0 +1,65 @@
+// respect line breaks on:
+// * operator chains
+// * argument lists in method calls
+//
+// only look for line breaks on either side of the operator, or the comma.
+res1 =
+  foo
+    * bar
+    * baz
+
+res2 =
+  foo
+    |> bar + bar
+    |> baz
+
+res3 =
+  foo
+    |> bar
+      + baz
+    |> qux
+
+res4 =
+  foo(
+    (it) -> it + 1,
+    (it) -> it + 2,
+  )
+
+res5 =
+  foo(
+    1,
+    2,
+    3,
+    4,
+  )
+
+// don't respect newlines right after opening paren and closing paren
+res6 = foo.bar(1, 2, 3)
+
+// only respect newlines on either side of a binary operator (we still want to inline `bar(3)` here).
+res7 = foo + bar(3) + baz
+
+// only the newlines after each "entry" are respected 
+res8 =
+  Map(
+    1, 2,
+    3, 4,
+  )
+
+res9 =
+  Map(
+    1, 2,
+    3, 4,
+  )
+
+res10 =
+  foo(
+    1,
+    2,
+    3,
+  )
+
+res11 =
+  foo
+    + bar
+    + baz