From ae5f02b285fd3f0e209220d9cf707ecd30587653 Mon Sep 17 00:00:00 2001
From: Jen Basch <421772+HT154@users.noreply.github.com>
Date: Thu, 21 Aug 2025 06:43:38 -0700
Subject: [PATCH] Allow trailing commas in comma-separated syntax elements
 (#1137)

---
 .idea/codeStyles/Project.xml                  |   1 +
 .../input/parser/constraintsTrailingComma.pkl |   9 +
 .../input/parser/lambdaTrailingCommas.pkl     |  43 +++++
 .../input/parser/methodTrailingCommas.pkl     | 157 ++++++++++++++++++
 .../input/parser/trailingCommas.pkl           |   3 +
 .../parser/constraintsTrailingComma.pcf       |   3 +
 .../output/parser/methodTrailingCommas.pcf    |  36 ++++
 .../output/parser/trailingCommas.pcf          |   3 +
 .../org/pkl/core/parser/TrailingCommasTest.kt |  82 +++++++++
 .../src/main/java/org/pkl/parser/Parser.java  |  46 +++--
 10 files changed, 370 insertions(+), 13 deletions(-)
 create mode 100644 pkl-core/src/test/files/LanguageSnippetTests/input/parser/constraintsTrailingComma.pkl
 create mode 100644 pkl-core/src/test/files/LanguageSnippetTests/input/parser/lambdaTrailingCommas.pkl
 create mode 100644 pkl-core/src/test/files/LanguageSnippetTests/input/parser/methodTrailingCommas.pkl
 create mode 100644 pkl-core/src/test/files/LanguageSnippetTests/input/parser/trailingCommas.pkl
 create mode 100644 pkl-core/src/test/files/LanguageSnippetTests/output/parser/constraintsTrailingComma.pcf
 create mode 100644 pkl-core/src/test/files/LanguageSnippetTests/output/parser/methodTrailingCommas.pcf
 create mode 100644 pkl-core/src/test/files/LanguageSnippetTests/output/parser/trailingCommas.pcf
 create mode 100644 pkl-core/src/test/kotlin/org/pkl/core/parser/TrailingCommasTest.kt

diff --git a/.idea/codeStyles/Project.xml b/.idea/codeStyles/Project.xml
index afb7ce46..90da0a7e 100644
--- a/.idea/codeStyles/Project.xml
+++ b/.idea/codeStyles/Project.xml
@@ -63,6 +63,7 @@
       </option>
       <option name="IMPORT_LAYOUT_TABLE">
         <value>
+          <package name="" withSubpackages="true" static="false" module="true" />
           <package name="" withSubpackages="true" static="true" />
           <emptyLine />
           <package name="" withSubpackages="true" static="false" />
diff --git a/pkl-core/src/test/files/LanguageSnippetTests/input/parser/constraintsTrailingComma.pkl b/pkl-core/src/test/files/LanguageSnippetTests/input/parser/constraintsTrailingComma.pkl
new file mode 100644
index 00000000..f6a7e388
--- /dev/null
+++ b/pkl-core/src/test/files/LanguageSnippetTests/input/parser/constraintsTrailingComma.pkl
@@ -0,0 +1,9 @@
+res1: Int(isEven, isNonZero, isBetween(-10, 10), ) = 6
+res2: Int(
+    isEven, isNonZero, isBetween(-10, 10),
+) = 6
+res3: Int(
+    isEven,
+    isNonZero,
+    isBetween(-10, 10),
+) = 6
diff --git a/pkl-core/src/test/files/LanguageSnippetTests/input/parser/lambdaTrailingCommas.pkl b/pkl-core/src/test/files/LanguageSnippetTests/input/parser/lambdaTrailingCommas.pkl
new file mode 100644
index 00000000..4c5a234b
--- /dev/null
+++ b/pkl-core/src/test/files/LanguageSnippetTests/input/parser/lambdaTrailingCommas.pkl
@@ -0,0 +1,43 @@
+local lA1 = (a, b, c,) -> true
+local lA2 = (
+  a, b, c,
+) -> true
+local lA3 = (
+  a,
+  b,
+  c,
+) -> true
+
+local lB1 = (a: Int, b: Int, c: Int,) -> true
+local lB2 = (
+  a: Int, b: Int, c: Int,
+) -> true
+local lB3 = (
+  a: Int,
+  b: Int,
+  c: Int,
+) -> true
+
+local lC1: (Dynamic,) -> Dynamic = new Mixin { a, -> x = true }
+local lC2: (Dynamic,) -> Dynamic = new Mixin { a, ->
+  x = true
+}
+local lC3: (Dynamic,) -> Dynamic = new Mixin {
+  a, -> x = true
+}
+local lC4: (Dynamic,) -> Dynamic = new Mixin {
+  a, ->
+  x = true
+}
+
+local lD1: (Dynamic,) -> Dynamic = new Mixin { a: Dynamic, -> x = true }
+local lD2: (Dynamic,) -> Dynamic = new Mixin { a: Dynamic, ->
+  x = true
+}
+local lD3: (Dynamic,) -> Dynamic = new Mixin {
+  a: Dynamic, -> x = true
+}
+local lD4: (Dynamic,) -> Dynamic = new Mixin {
+  a: Dynamic, ->
+  x = true
+}
diff --git a/pkl-core/src/test/files/LanguageSnippetTests/input/parser/methodTrailingCommas.pkl b/pkl-core/src/test/files/LanguageSnippetTests/input/parser/methodTrailingCommas.pkl
new file mode 100644
index 00000000..b9eada00
--- /dev/null
+++ b/pkl-core/src/test/files/LanguageSnippetTests/input/parser/methodTrailingCommas.pkl
@@ -0,0 +1,157 @@
+function moduleMethodA1(a, b, c, ) = true
+function moduleMethodA2(
+  a, b, c,
+) = true
+function moduleMethodA3(
+  a,
+  b,
+  c,
+) = true
+function moduleMethodB1(a: Int, b: Int, c: Int, ) = true
+function moduleMethodB2(
+  a: Int, b: Int, c: Int,
+) = true
+function moduleMethodB3(
+  a: Int,
+  b: Int,
+  c: Int,
+) = true
+
+class A {
+  function classMethodA1(a, b, c, ) = true
+  function classMethodA2(
+    a, b, c,
+  ) = true
+  function classMethodA3(
+    a,
+    b,
+    c,
+  ) = true
+  function classMethodB1(a: Int, b: Int, c: Int, ) = true
+  function classMethodB2(
+    a: Int, b: Int, c: Int,
+  ) = true
+  function classMethodB3(
+    a: Int,
+    b: Int,
+    c: Int,
+  ) = true
+}
+
+moduleMethodA1Call1 = moduleMethodA1(1, 2, 3, )
+moduleMethodA1Call2 = moduleMethodA1(
+  1, 2, 3,
+)
+moduleMethodA1Call3 = moduleMethodA1(
+  1,
+  2,
+  3,
+)
+moduleMethodA2Call1 = moduleMethodA2(1, 2, 3, )
+moduleMethodA2Call2 = moduleMethodA2(
+  1, 2, 3,
+)
+moduleMethodA2Call3 = moduleMethodA2(
+  1,
+  2,
+  3,
+)
+
+moduleMethodA3Call1 = moduleMethodA3(1, 2, 3, )
+moduleMethodA3Call2 = moduleMethodA3(
+  1, 2, 3,
+)
+moduleMethodA3Call3 = moduleMethodA3(
+  1,
+  2,
+  3,
+)
+
+moduleMethodB1Call1 = moduleMethodB1(1, 2, 3, )
+moduleMethodB1Call2 = moduleMethodB1(
+  1, 2, 3,
+)
+moduleMethodB1Call3 = moduleMethodB1(
+  1,
+  2,
+  3,
+)
+moduleMethodB2Call1 = moduleMethodB2(1, 2, 3, )
+moduleMethodB2Call2 = moduleMethodB2(
+  1, 2, 3,
+)
+moduleMethodB2Call3 = moduleMethodB2(
+  1,
+  2,
+  3,
+)
+
+moduleMethodB3Call1 = moduleMethodB3(1, 2, 3, )
+moduleMethodB3Call2 = moduleMethodB3(
+  1, 2, 3,
+)
+moduleMethodB3Call3 = moduleMethodB3(
+  1,
+  2,
+  3,
+)
+
+local a: A = new {}
+
+classMethodA1Call1 = a.classMethodA1(1, 2, 3, )
+classMethodA1Call2 = a.classMethodA1(
+  1, 2, 3,
+)
+classMethodA1Call3 = a.classMethodA1(
+  1,
+  2,
+  3,
+)
+classMethodA2Call1 = a.classMethodA2(1, 2, 3, )
+classMethodA2Call2 = a.classMethodA2(
+  1, 2, 3,
+)
+classMethodA2Call3 = a.classMethodA2(
+  1,
+  2,
+  3,
+)
+
+classMethodA3Call1 = a.classMethodA3(1, 2, 3, )
+classMethodA3Call2 = a.classMethodA3(
+  1, 2, 3,
+)
+classMethodA3Call3 = a.classMethodA3(
+  1,
+  2,
+  3,
+)
+
+classMethodB1Call1 = a.classMethodB1(1, 2, 3, )
+classMethodB1Call2 = a.classMethodB1(
+  1, 2, 3,
+)
+classMethodB1Call3 = a.classMethodB1(
+  1,
+  2,
+  3,
+)
+classMethodB2Call1 = a.classMethodB2(1, 2, 3, )
+classMethodB2Call2 = a.classMethodB2(
+  1, 2, 3,
+)
+classMethodB2Call3 = a.classMethodB2(
+  1,
+  2,
+  3,
+)
+
+classMethodB3Call1 = a.classMethodB3(1, 2, 3, )
+classMethodB3Call2 = a.classMethodB3(
+  1, 2, 3,
+)
+classMethodB3Call3 = a.classMethodB3(
+  1,
+  2,
+  3,
+)
diff --git a/pkl-core/src/test/files/LanguageSnippetTests/input/parser/trailingCommas.pkl b/pkl-core/src/test/files/LanguageSnippetTests/input/parser/trailingCommas.pkl
new file mode 100644
index 00000000..3c4b88aa
--- /dev/null
+++ b/pkl-core/src/test/files/LanguageSnippetTests/input/parser/trailingCommas.pkl
@@ -0,0 +1,3 @@
+res1: Mapping<String, String,>(!isEmpty) = new Mapping<String, String,> {
+  ["hello"] = "world"
+}
diff --git a/pkl-core/src/test/files/LanguageSnippetTests/output/parser/constraintsTrailingComma.pcf b/pkl-core/src/test/files/LanguageSnippetTests/output/parser/constraintsTrailingComma.pcf
new file mode 100644
index 00000000..3f7798be
--- /dev/null
+++ b/pkl-core/src/test/files/LanguageSnippetTests/output/parser/constraintsTrailingComma.pcf
@@ -0,0 +1,3 @@
+res1 = 6
+res2 = 6
+res3 = 6
diff --git a/pkl-core/src/test/files/LanguageSnippetTests/output/parser/methodTrailingCommas.pcf b/pkl-core/src/test/files/LanguageSnippetTests/output/parser/methodTrailingCommas.pcf
new file mode 100644
index 00000000..dba4a19c
--- /dev/null
+++ b/pkl-core/src/test/files/LanguageSnippetTests/output/parser/methodTrailingCommas.pcf
@@ -0,0 +1,36 @@
+moduleMethodA1Call1 = true
+moduleMethodA1Call2 = true
+moduleMethodA1Call3 = true
+moduleMethodA2Call1 = true
+moduleMethodA2Call2 = true
+moduleMethodA2Call3 = true
+moduleMethodA3Call1 = true
+moduleMethodA3Call2 = true
+moduleMethodA3Call3 = true
+moduleMethodB1Call1 = true
+moduleMethodB1Call2 = true
+moduleMethodB1Call3 = true
+moduleMethodB2Call1 = true
+moduleMethodB2Call2 = true
+moduleMethodB2Call3 = true
+moduleMethodB3Call1 = true
+moduleMethodB3Call2 = true
+moduleMethodB3Call3 = true
+classMethodA1Call1 = true
+classMethodA1Call2 = true
+classMethodA1Call3 = true
+classMethodA2Call1 = true
+classMethodA2Call2 = true
+classMethodA2Call3 = true
+classMethodA3Call1 = true
+classMethodA3Call2 = true
+classMethodA3Call3 = true
+classMethodB1Call1 = true
+classMethodB1Call2 = true
+classMethodB1Call3 = true
+classMethodB2Call1 = true
+classMethodB2Call2 = true
+classMethodB2Call3 = true
+classMethodB3Call1 = true
+classMethodB3Call2 = true
+classMethodB3Call3 = true
diff --git a/pkl-core/src/test/files/LanguageSnippetTests/output/parser/trailingCommas.pcf b/pkl-core/src/test/files/LanguageSnippetTests/output/parser/trailingCommas.pcf
new file mode 100644
index 00000000..5ca6f00c
--- /dev/null
+++ b/pkl-core/src/test/files/LanguageSnippetTests/output/parser/trailingCommas.pcf
@@ -0,0 +1,3 @@
+res1 {
+  ["hello"] = "world"
+}
diff --git a/pkl-core/src/test/kotlin/org/pkl/core/parser/TrailingCommasTest.kt b/pkl-core/src/test/kotlin/org/pkl/core/parser/TrailingCommasTest.kt
new file mode 100644
index 00000000..81d53e63
--- /dev/null
+++ b/pkl-core/src/test/kotlin/org/pkl/core/parser/TrailingCommasTest.kt
@@ -0,0 +1,82 @@
+/*
+ * Copyright © 2025 Apple Inc. and the Pkl project authors. All rights reserved.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.pkl.core.parser
+
+import org.assertj.core.api.Assertions.assertThat
+import org.junit.jupiter.api.Test
+import org.pkl.core.Evaluator
+import org.pkl.parser.Parser
+
+// tests type argument and parameter parsing with trailing commas that cannot be tested with
+// snippets because these constructs are currently only allowed in the stdlib
+class TrailingCommasTest {
+  private val evaluator = Evaluator.preconfigured()
+
+  @Test
+  fun `class type parameter lists parse correctly`() {
+    val module =
+      Parser()
+        .parseModule(
+          """
+        class Foo<
+          Key,
+          Value,
+        >
+        
+        class Bar<
+          Key,
+          Value,
+        > {
+          baz: Key
+          buzz: Value
+        }
+      """
+            .trimIndent()
+        )
+
+    val fooClass = module.classes.find { it.name.value == "Foo" }
+    assertThat(fooClass).isNotNull
+    assertThat(fooClass!!.typeParameterList?.parameters?.first()?.identifier?.value)
+      .isEqualTo("Key")
+    assertThat(fooClass.typeParameterList?.parameters?.last()?.identifier?.value).isEqualTo("Value")
+
+    val barClass = module.classes.find { it.name.value == "Bar" }
+    assertThat(barClass).isNotNull
+    assertThat(barClass!!.typeParameterList?.parameters?.first()?.identifier?.value)
+      .isEqualTo("Key")
+    assertThat(barClass.typeParameterList?.parameters?.last()?.identifier?.value).isEqualTo("Value")
+  }
+
+  @Test
+  fun `method type parameter lists parse correctly`() {
+    val module =
+      Parser()
+        .parseModule(
+          """
+        function foo<
+          A,
+          B,
+        >(a: A, b: B,): Value? = "\(a):\(b)"
+      """
+            .trimIndent()
+        )
+
+    val fooMethod = module.methods.find { it.name.value == "foo" }
+    assertThat(fooMethod).isNotNull
+    assertThat(fooMethod!!.typeParameterList?.parameters?.first()?.identifier?.value).isEqualTo("A")
+    assertThat(fooMethod.typeParameterList?.parameters?.last()?.identifier?.value).isEqualTo("B")
+  }
+}
diff --git a/pkl-parser/src/main/java/org/pkl/parser/Parser.java b/pkl-parser/src/main/java/org/pkl/parser/Parser.java
index 52e4154a..b6c1cc1e 100644
--- a/pkl-parser/src/main/java/org/pkl/parser/Parser.java
+++ b/pkl-parser/src/main/java/org/pkl/parser/Parser.java
@@ -534,7 +534,7 @@ public class Parser {
       return new ObjectBody(List.of(), 0, start.endWith(next().span));
     } else if (lookahead == Token.UNDERSCORE) {
       // it's a parameter
-      nodes.addAll(parseListOfParameter(Token.COMMA));
+      nodes.addAll(parseListOfParameter(Token.COMMA, Token.ARROW));
       expect(Token.ARROW, "unexpectedToken2", ",", "->");
     } else if (lookahead == Token.IDENTIFIER) {
       // not sure what it is yet
@@ -546,7 +546,7 @@ public class Parser {
       } else if (lookahead == Token.COMMA) {
         // it's a parameter
         backtrack();
-        nodes.addAll(parseListOfParameter(Token.COMMA));
+        nodes.addAll(parseListOfParameter(Token.COMMA, Token.ARROW));
         expect(Token.ARROW, "unexpectedToken2", ",", "->");
       } else if (lookahead == Token.COLON) {
         // still not sure
@@ -559,7 +559,7 @@ public class Parser {
           nodes.add(
               new TypedIdentifier(
                   identifier, typeAnnotation, identifier.span().endWith(type.span())));
-          nodes.addAll(parseListOfParameter(Token.COMMA));
+          nodes.addAll(parseListOfParameter(Token.COMMA, Token.ARROW));
           expect(Token.ARROW, "unexpectedToken2", ",", "->");
         } else if (lookahead == Token.ARROW) {
           // it's a parameter
@@ -1270,7 +1270,7 @@ public class Parser {
         next();
         var params = new ArrayList<Parameter>();
         params.add(new TypedIdentifier(identifier, null, identifier.span()));
-        params.addAll(parseListOfParameter(Token.COMMA));
+        params.addAll(parseListOfParameter(Token.COMMA, Token.RPAREN));
         var endParen = expect(Token.RPAREN, "unexpectedToken2", ",", ")").span;
         var paramList = new ParameterList(params, start.endWith(endParen));
         expect(Token.ARROW, "unexpectedToken", "->");
@@ -1285,7 +1285,7 @@ public class Parser {
                 identifier, typeAnnotation, identifier.span().endWith(typeAnnotation.span())));
         if (lookahead == Token.COMMA) {
           next();
-          params.addAll(parseListOfParameter(Token.COMMA));
+          params.addAll(parseListOfParameter(Token.COMMA, Token.RPAREN));
         }
         var endParen = expect(Token.RPAREN, "unexpectedToken2", ",", ")").span;
         var paramList = new ParameterList(params, start.endWith(endParen));
@@ -1320,7 +1320,7 @@ public class Parser {
 
   private FunctionLiteralExpr parseFunctionLiteral(Span start) {
     // the open parens is already parsed
-    var params = parseListOfParameter(Token.COMMA);
+    var params = parseListOfParameter(Token.COMMA, Token.RPAREN);
     var endParen = expect(Token.RPAREN, "unexpectedToken2", ",", ")").span;
     var paramList = new ParameterList(params, start.endWith(endParen));
     expect(Token.ARROW, "unexpectedToken", "->");
@@ -1385,7 +1385,7 @@ public class Parser {
         if (lookahead == Token.RPAREN) {
           end = next().span;
         } else {
-          children.addAll(parseListOf(Token.COMMA, () -> parseType(")")));
+          children.addAll(parseListOf(Token.COMMA, Token.RPAREN, () -> parseType(")")));
           end = expect(Token.RPAREN, "unexpectedToken2", ",", ")").span;
         }
         if (lookahead == Token.ARROW || children.size() > 1) {
@@ -1436,7 +1436,7 @@ public class Parser {
     // constrained types: have to start in the same line as the type
     if (lookahead == Token.LPAREN && !precededBySemicolon && _lookahead.newLinesBetween == 0) {
       next();
-      var constraints = parseListOf(Token.COMMA, () -> parseExpr(")"));
+      var constraints = parseListOf(Token.COMMA, Token.RPAREN, () -> parseExpr(")"));
       var end = expect(Token.RPAREN, "unexpectedToken2", ",", ")").span;
       var children = new ArrayList<Node>(constraints.size() + 1);
       children.add(type);
@@ -1498,7 +1498,7 @@ public class Parser {
     if (lookahead == Token.RPAREN) {
       end = next().span;
     } else {
-      args = parseListOfParameter(Token.COMMA);
+      args = parseListOfParameter(Token.COMMA, Token.RPAREN);
       end = expect(Token.RPAREN, "unexpectedToken2", ",", ")").span;
     }
     return new ParameterList(args, start.endWith(end));
@@ -1517,14 +1517,14 @@ public class Parser {
 
   private TypeParameterList parseTypeParameterList() {
     var start = expect(Token.LT, "unexpectedToken", "<").span;
-    var pars = parseListOf(Token.COMMA, this::parseTypeParameter);
+    var pars = parseListOf(Token.COMMA, Token.GT, this::parseTypeParameter);
     var end = expect(Token.GT, "unexpectedToken2", ",", ">").span;
     return new TypeParameterList(pars, start.endWith(end));
   }
 
   private TypeArgumentList parseTypeArgumentList() {
     var start = expect(Token.LT, "unexpectedToken", "<").span;
-    var pars = parseListOf(Token.COMMA, this::parseType);
+    var pars = parseListOf(Token.COMMA, Token.GT, this::parseType);
     var end = expect(Token.GT, "unexpectedToken2", ",", ">").span;
     return new TypeArgumentList(pars, start.endWith(end));
   }
@@ -1534,7 +1534,7 @@ public class Parser {
     if (lookahead == Token.RPAREN) {
       return new ArgumentList(new ArrayList<>(), start.endWith(next().span));
     }
-    var exprs = parseListOf(Token.COMMA, this::parseExpr);
+    var exprs = parseListOf(Token.COMMA, Token.RPAREN, this::parseExpr);
     var end = expect(Token.RPAREN, "unexpectedToken2", ",", ")").span;
     return new ArgumentList(exprs, start.endWith(end));
   }
@@ -1725,11 +1725,31 @@ public class Parser {
     return res;
   }
 
-  private List<Parameter> parseListOfParameter(Token separator) {
+  private <T> List<T> parseListOf(Token separator, Token terminator, Supplier<T> parser) {
+    var res = new ArrayList<T>();
+    res.add(parser.get());
+    while (lookahead == separator) {
+      next();
+      if (lookahead == terminator) {
+        break;
+      }
+      res.add(parser.get());
+    }
+    return res;
+  }
+
+  private List<Parameter> parseListOfParameter(Token separator, Token terminator) {
     var res = new ArrayList<Parameter>();
+    if (lookahead == terminator) {
+      return res;
+    }
+
     res.add(parseParameter());
     while (lookahead == separator) {
       next();
+      if (lookahead == terminator) {
+        break;
+      }
       res.add(parseParameter());
     }
     return res;