From d29ae07e1412ff18dec4b0ff87e03f5178f2b84c Mon Sep 17 00:00:00 2001 From: Daniel Chao Date: Mon, 3 Nov 2025 09:15:58 -0800 Subject: [PATCH] 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`) --- .../main/kotlin/org/pkl/formatter/Builder.kt | 41 ++++++++++--------- .../input/method-call-trailing-lambdas.pkl | 24 +++++++++++ .../output/line-breaks.pkl | 7 ++-- .../output/method-call-trailing-lambdas.pkl | 29 +++++++++++++ .../java/org/pkl/parser/GenericParser.java | 6 +-- .../pkl/parser/syntax/generic/FullSpan.java | 8 +--- 6 files changed, 82 insertions(+), 33 deletions(-) diff --git a/pkl-formatter/src/main/kotlin/org/pkl/formatter/Builder.kt b/pkl-formatter/src/main/kotlin/org/pkl/formatter/Builder.kt index bbf3cd26..2a26adb1 100644 --- a/pkl-formatter/src/main/kotlin/org/pkl/formatter/Builder.kt +++ b/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: - * ``` - * foo.toMap( - * (it) -> makeSomeKey(it), - * (it) -> makeSomeValue(it), - * ) - * ``` + * Only considered trailing lamdba if: + * 1. There is only one lambda/new expr/amends expr in the list. E.g. avoid formatting `toMap()` + * weirdly: ``` foo.toMap( (it) -> makeSomeKey(it), (it) -> makeSomeValue(it), ) ``` + * 2. The lambda does not have leading or trailing line comment. */ private fun hasTrailingLambda(argList: Node): Boolean { val children = argList.firstProperChild()?.children ?: return false - var seenArg = false - var ret = false + var seenLambda = 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 (seenArg) { + if (!seenLambda) { + 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): List { diff --git a/pkl-formatter/src/test/files/FormatterSnippetTests/input/method-call-trailing-lambdas.pkl b/pkl-formatter/src/test/files/FormatterSnippetTests/input/method-call-trailing-lambdas.pkl index 8da81f2a..b2ba01f4 100644 --- a/pkl-formatter/src/test/files/FormatterSnippetTests/input/method-call-trailing-lambdas.pkl +++ b/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 + ) diff --git a/pkl-formatter/src/test/files/FormatterSnippetTests/output/line-breaks.pkl b/pkl-formatter/src/test/files/FormatterSnippetTests/output/line-breaks.pkl index cfce478f..656f9776 100644 --- a/pkl-formatter/src/test/files/FormatterSnippetTests/output/line-breaks.pkl +++ b/pkl-formatter/src/test/files/FormatterSnippetTests/output/line-breaks.pkl @@ -76,8 +76,9 @@ local function render(currentIndent: String) = items: List = allItems - .filter((item) -> - badItems.containsKey(item) // some line comment - || item.tags.toList().findOrNull((it) -> it.type == "bookmark") != null // some other line comment + .filter( + (item) -> + badItems.containsKey(item) // some line comment + || item.tags.toList().findOrNull((it) -> it.type == "bookmark") != null // some other line comment ) .sortBy((item) -> item.name) diff --git a/pkl-formatter/src/test/files/FormatterSnippetTests/output/method-call-trailing-lambdas.pkl b/pkl-formatter/src/test/files/FormatterSnippetTests/output/method-call-trailing-lambdas.pkl index 376c97d3..91a283c0 100644 --- a/pkl-formatter/src/test/files/FormatterSnippetTests/output/method-call-trailing-lambdas.pkl +++ b/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 + ) diff --git a/pkl-parser/src/main/java/org/pkl/parser/GenericParser.java b/pkl-parser/src/main/java/org/pkl/parser/GenericParser.java index c8457cda..ff6c74f2 100644 --- a/pkl-parser/src/main/java/org/pkl/parser/GenericParser.java +++ b/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 - while (lookahead.isAffix() && lastImport.span.sameLine(spanLookahead)) { + // keep trailing affixes as part of the import + 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(); children.add(expr); ff(children); diff --git a/pkl-parser/src/main/java/org/pkl/parser/syntax/generic/FullSpan.java b/pkl-parser/src/main/java/org/pkl/parser/syntax/generic/FullSpan.java index 319ba1f2..f53f1774 100644 --- a/pkl-parser/src/main/java/org/pkl/parser/syntax/generic/FullSpan.java +++ b/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() { - return new Span(charIndex, length); - } - - public boolean sameLine(FullSpan other) { + public boolean isSameLine(FullSpan other) { return lineEnd == other.lineBegin; }