Fix formatting of argument lists (#1283)

This fixes several issues:

1. Leading/trailing line comments surrounding a lambda should make that
   lambda not "trailing", because the formatting otherwise looks bad and
   also isn't stable
2. Fix incorrect algorithm for detecting trailing lambda (currently,
   any number of lambdas makes the alg return `true`)

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

@@ -614,7 +614,7 @@ internal class Builder(sourceText: String, private val grammarVersion: GrammarVe
       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 trailingNode = if (endsWithClosingCurlyBrace(lastParam.last())) Empty else line()
       val lastNodes = formatGenericWithGen(lastParam, sep, null)
       if (normalParams.isEmpty()) {
         group(Group(newId(), lastNodes), trailingNode)
@@ -639,42 +639,43 @@ internal class Builder(sourceText: String, private val grammarVersion: GrammarVe
     return false
   }
 
-  private tailrec fun endsWithClosingBracket(node: Node): Boolean {
+  private tailrec fun endsWithClosingCurlyBrace(node: Node): Boolean {
     return if (node.children.isNotEmpty()) {
-      endsWithClosingBracket(node.children.last())
+      endsWithClosingCurlyBrace(node.children.last())
     } else {
-      node.isTerminal("}") || node.type.isAffix
+      node.isTerminal("}")
     }
   }
 
   /**
-   * Only considered trailing lamdba if there is only one lambda/new expr/amends expr in the list.
+   * Tells if an argument list has a trailing lambda, new expr, or amends expr.
    *
-   * E.g. avoid formatting `toMap()` weirdly:
+   * Only considered trailing lamdba if:
-   * ```
+   * 1. There is only one lambda/new expr/amends expr in the list. E.g. avoid formatting `toMap()`
-   * foo.toMap(
+   *    weirdly: ``` foo.toMap( (it) -> makeSomeKey(it), (it) -> makeSomeValue(it), ) ```
-   *   (it) -> makeSomeKey(it),
+   * 2. The lambda does not have leading or trailing line comment.
-   *   (it) -> makeSomeValue(it),
-   * )
-   * ```
    */
   private fun hasTrailingLambda(argList: Node): Boolean {
     val children = argList.firstProperChild()?.children ?: return false
-    var seenArg = false
+    var seenLambda = false
-    var ret = false
+    if (children.last().type == NodeType.LINE_COMMENT) {
+      return false
+    }
     for (i in children.lastIndex downTo 0) {
       val child = children[i]
       if (!child.isProper()) continue
-      if (child.type in SAME_LINE_EXPRS) {
+      if (!seenLambda) {
-        if (seenArg) {
+        if (child.type !in SAME_LINE_EXPRS) {
           return false
-        } else {
-          seenArg = true
-          ret = true
         }
+        // preceded by line comment
+        if (children.getOrNull(i - 1)?.type == NodeType.LINE_COMMENT) return false
+        seenLambda = true
+      } else if (child.type in SAME_LINE_EXPRS) {
+        return false
       }
     }
-    return ret
+    return true
   }
 
   private fun pairArguments(nodes: List<Node>): List<Node> {

pkl-formatter/src/test/files/FormatterSnippetTests/input/method-call-trailing-lambdas.pkl

@@ -19,3 +19,27 @@ res4 = foo.foldLeft(
 res5 = ifNonNull((it) -> someFunctionCall(it as SomeReaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaalyLongTypeName))
 
 res6 = ifNonNull((it) -> someFunctionCall(it as SomeReaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaalyLongTypeNameLonger))
+
+res7 =
+  qux(bar, (a) ->
+    hello // some comment
+  )
+
+res8 = qux(
+  // some comment
+  (a) -> bar
+)
+
+res9 =
+  qux(bar, new {
+    bar = 1
+  } // some comment
+  )
+
+res10 = (a) -> hello // some comment
+
+res11 =
+  qux(bar, (a) ->
+    hello
+    // some comment
+  )

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

@@ -76,8 +76,9 @@ local function render(currentIndent: String) =
 
 items: List<Item> =
   allItems
-    .filter((item) ->
+    .filter(
-      badItems.containsKey(item) // some line comment 
+      (item) ->
-        || item.tags.toList().findOrNull((it) -> it.type == "bookmark") != null // some other line comment
+        badItems.containsKey(item) // some line comment 
+          || item.tags.toList().findOrNull((it) -> it.type == "bookmark") != null // some other line comment
     )
     .sortBy((item) -> item.name)

pkl-formatter/src/test/files/FormatterSnippetTests/output/method-call-trailing-lambdas.pkl

@@ -25,3 +25,32 @@ res6 =
   ifNonNull((it) ->
     someFunctionCall(it as SomeReaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaalyLongTypeNameLonger)
   )
+
+res7 =
+  qux(
+    bar,
+    (a) -> hello // some comment
+  )
+
+res8 =
+  qux(
+    // some comment
+    (a) -> bar,
+  )
+
+res9 =
+  qux(
+    bar,
+    new {
+      bar = 1
+    } // some comment
+  )
+
+res10 = (a) -> hello // some comment
+
+res11 =
+  qux(
+    bar,
+    (a) -> hello
+    // some comment
+  )

pkl-parser/src/main/java/org/pkl/parser/GenericParser.java

@@ -77,8 +77,8 @@ public class GenericParser {
       }
       var lastImport = parseImportDecl();
       imports.add(lastImport);
-      // keep trailling affixes as part of the import
+      // keep trailing affixes as part of the import
-      while (lookahead.isAffix() && lastImport.span.sameLine(spanLookahead)) {
+      while (lookahead.isAffix() && lastImport.span.isSameLine(spanLookahead)) {
         imports.add(makeAffix(next()));
       }
       if (!isImport()) break;
@@ -662,7 +662,7 @@ public class GenericParser {
       if (operator.getPrec() < minPrecedence) break;
       // `-` and `[]` must be in the same line as the left operand and have no semicolons inbetween
       if ((operator == Operator.MINUS || operator == Operator.SUBSCRIPT)
-          && (fullOpToken.hasSemicolon || !expr.span.sameLine(fullOpToken.tk.span))) break;
+          && (fullOpToken.hasSemicolon || !expr.span.isSameLine(fullOpToken.tk.span))) break;
       var children = new ArrayList<Node>();
       children.add(expr);
       ff(children);

pkl-parser/src/main/java/org/pkl/parser/syntax/generic/FullSpan.java

@@ -15,8 +15,6 @@
  */
 package org.pkl.parser.syntax.generic;
 
-import org.pkl.parser.Span;
-
 public record FullSpan(
     int charIndex, int length, int lineBegin, int colBegin, int lineEnd, int colEnd) {
 
@@ -30,11 +28,7 @@ public record FullSpan(
         end.colEnd);
   }
 
-  public Span toSpan() {
+  public boolean isSameLine(FullSpan other) {
-    return new Span(charIndex, length);
-  }
-
-  public boolean sameLine(FullSpan other) {
     return lineEnd == other.lineBegin;
   }