Skip to content

Commit c7b60b3

Browse files
committed
fix: reject duplicate function parameters with correct error in all contexts
Motivation: go-jsonnet issue #873 reports that (function(x, x, x) x)(null, 3, 2) silently returns 2 instead of rejecting duplicate parameter names. sjsonnet already rejected duplicates in function expressions, but the error message was misleading in local bindings and object methods. Modification: The params parser correctly detects duplicates via flatMapX + Fail. However, in bind (local f(x,x)=...) and field ({f(x,x):...}), the optional group .? around params swallowed the "no duplicate parameter" error and backtracked, producing misleading errors like "Expected )". Replaced .? with ./ (committed-or): "(" ~/ params ~ ")" ./ | Pass(null). The cut ~/ after "(" commits to the params path, and ./ prevents the | alternative from trying the fallback when the committed path fails. Result: All duplicate parameter contexts now produce the correct error: "Expected no duplicate parameter: x" with accurate position info. - (function(x, x, x) x) -> ParseError (already worked) - local f(x, x) = x -> ParseError (was "Expected )") - { f(x, x): x } -> ParseError (was "Expected :") - { local f(x, x) = x } -> ParseError (was "Expected )") Tests pass on Scala 3.3.7, 2.13.18, 2.12.21. References: - google/go-jsonnet#873
1 parent b861ee8 commit c7b60b3

3 files changed

Lines changed: 30 additions & 11 deletions

File tree

sjsonnet/src/sjsonnet/Parser.scala

Lines changed: 18 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -951,7 +951,7 @@ class Parser(
951951

952952
def field[$: P](currentDepth: Int): P[Expr.Member.Field] = {
953953
P(
954-
(Pos ~~ fieldname(currentDepth + 1) ~/ "+".!.? ~ ("(" ~ params(
954+
(Pos ~~ fieldname(currentDepth + 1) ~/ "+".!.? ~ ("(" ~/ params(
955955
currentDepth + 1
956956
) ~ ")").? ~ fieldKeySep ~/ expr(currentDepth + 1)).map { case (pos, name, plus, p, h2, e) =>
957957
Expr.Member.Field(pos, name, plus.nonEmpty, p.orNull, h2, e)
@@ -1045,18 +1045,27 @@ class Parser(
10451045
def params[$: P]: P[Expr.Params] = params(0)
10461046

10471047
def params[$: P](currentDepth: Int): P[Expr.Params] = {
1048-
P((id ~ ("=" ~ expr(currentDepth + 1)).?).rep(sep = ",") ~ ",".?).flatMapX { x =>
1048+
val ctx = implicitly[P[?]]
1049+
P(
1050+
(Pos ~~ id ~ ("=" ~ expr(currentDepth + 1)).?).rep(sep = ",") ~ ",".?
1051+
).flatMapX { x =>
10491052
val seen = collection.mutable.Set.empty[String]
1050-
var overlap: String = null
1051-
for ((k, _) <- x) {
1052-
if (seen(k)) overlap = k
1053-
else seen.add(k)
1053+
var overlap: (Position, String) = null
1054+
for ((pos, k, _) <- x) {
1055+
if (overlap == null) {
1056+
if (seen(k)) overlap = (pos, k)
1057+
else seen.add(k)
1058+
}
10541059
}
10551060
if (overlap == null) {
1056-
val names = x.map(_._1).toArray[String]
1057-
val exprs = x.map(_._2.orNull).toArray[Expr]
1061+
val names = x.map(_._2).toArray[String]
1062+
val exprs = x.map(_._3.orNull).toArray[Expr]
10581063
Pass(Expr.Params(names, exprs))
1059-
} else Fail.opaque("no duplicate parameter: " + overlap)
1064+
} else {
1065+
ctx.cut = true
1066+
ctx.index = overlap._1.offset
1067+
Fail.opaque("no duplicate parameter: " + overlap._2)
1068+
}
10601069
}
10611070
}
10621071

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,3 @@
1-
sjsonnet.ParseError: Expected no duplicate parameter: x:17:14, found ") x\n"
2-
at [<root>].(error.function_duplicate_param.jsonnet:17:14)
1+
sjsonnet.ParseError: Expected no duplicate parameter: x:17:13, found "x) x\n"
2+
at [<root>].(error.function_duplicate_param.jsonnet:17:13)
33

sjsonnet/test/src/sjsonnet/ParserTests.scala

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -57,6 +57,16 @@ object ParserTests extends TestSuite {
5757
test("duplicateFields") {
5858
parseErr("{ a: 1, a: 2 }") ==> """Expected no duplicate field: a:1:14, found "}""""
5959
}
60+
test("duplicateParams") {
61+
parseErr("(function(x, x, x) x)(null, 3, 2)") ==>
62+
"""Expected no duplicate parameter: x:1:14, found "x, x) x)(n""""
63+
parseErr("local f(x, x) = x; f(1, 2)") ==>
64+
"Expected no duplicate parameter: x:1:12, found \"x) = x; f(\""
65+
parseErr("{ f(x, x): x }.f(1, 2)") ==>
66+
"Expected no duplicate parameter: x:1:8, found \"x): x }.f(\""
67+
parseErr("{ local f(x, x) = x, a: f(1, 2) }") ==>
68+
"Expected no duplicate parameter: x:1:14, found \"x) = x, a:\""
69+
}
6070
test("localInObj") {
6171
parse("""{
6272
|local x = 1,

0 commit comments

Comments
 (0)