Skip to content

fix: reject duplicate function parameters with correct error in all contexts#1011

Merged
stephenamar-db merged 1 commit into
databricks:masterfrom
He-Pin:fix-reject-duplicate-params
Jun 22, 2026
Merged

fix: reject duplicate function parameters with correct error in all contexts#1011
stephenamar-db merged 1 commit into
databricks:masterfrom
He-Pin:fix-reject-duplicate-params

Conversation

@He-Pin

@He-Pin He-Pin commented Jun 20, 2026

Copy link
Copy Markdown
Contributor

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
(e.g. "Expected ")"" or "Expected ":"").

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.

Two changes fix this:

  1. In field(): added ~/ (cut) after "(" to commit to the params
    path once "(" is matched: "(" ~/ params(...) ~ ")".
  2. In params(): set ctx.cut = true programmatically when a
    duplicate is detected, preventing outer .? operators from
    swallowing the error. Also captures Pos for each parameter to
    position the error at the duplicate parameter name (not at the
    closing paren).

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 )")
  • Nested bindings also fixed as a bonus.

Tests pass on Scala 3.3.7, 2.13.18, 2.12.21.

References

Cross-implementation comparison: Duplicate function parameters

Implementation Detection Phase All Contexts Covered Error Message Example
sjsonnet (this fix) Parse time ✅ All 4 contexts Expected no duplicate parameter: x
cpp-jsonnet Static analysis (post-parse) ✅ All 4 (via desugaring) Duplicate function parameter: x
go-jsonnet (master) ❌ None ❌ Bug — silently accepts N/A — last argument wins silently
go-jsonnet (PR #876, unmerged) Parse time ✅ All 4 Duplicate function parameter: a / Duplicate method parameter: x
jrsonnet Static analysis (post-parse) ✅ All 4 (via desugaring) local is already defined in the current frame: x

Behavior by test case

Test Case sjsonnet (fix) cpp-jsonnet go-jsonnet jrsonnet
(function(x, x, x) x)(null, 3, 2) ✅ ParseError ✅ StaticError ❌ Returns 2 ✅ StaticError
local f(x, x) = x; f(1, 2) ✅ ParseError ✅ StaticError ❌ Accepted ✅ StaticError
{ f(x, x): x }.f(1, 2) ✅ ParseError ✅ StaticError ❌ Accepted ✅ StaticError
{ local f(x, x) = x, a: f(1, 2) } ✅ ParseError ✅ StaticError ❌ Accepted ✅ StaticError

Key differences

  • Detection timing: sjsonnet and go-jsonnet (PR perf: capture parse Position without boxing the offset Int #876) detect at parse time; cpp-jsonnet and jrsonnet detect during a separate static analysis pass after parsing.
  • Error message specificity: go-jsonnet PR perf: capture parse Position without boxing the offset Int #876 distinguishes "function parameter" vs "method parameter"; cpp-jsonnet uses "function parameter" uniformly (methods desugar to functions before the check).
  • Position accuracy: sjsonnet points to the duplicate parameter name; cpp-jsonnet points to the entire function AST node.
  • go-jsonnet master has a known bug (issue #873) where duplicate parameters are silently accepted with "last argument wins" semantics.

@He-Pin He-Pin force-pushed the fix-reject-duplicate-params branch from 32e3062 to 83d39d9 Compare June 20, 2026 11:15
@He-Pin He-Pin marked this pull request as draft June 20, 2026 11:18
@He-Pin He-Pin force-pushed the fix-reject-duplicate-params branch from 83d39d9 to b7288e6 Compare June 20, 2026 11:29
…ontexts

Motivation:
go-jsonnet issue databricks#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
@He-Pin He-Pin force-pushed the fix-reject-duplicate-params branch from b7288e6 to c7b60b3 Compare June 20, 2026 11:43
@He-Pin He-Pin marked this pull request as ready for review June 20, 2026 11:47
@He-Pin He-Pin closed this Jun 20, 2026
@He-Pin He-Pin reopened this Jun 20, 2026
@stephenamar-db stephenamar-db merged commit 4f820c6 into databricks:master Jun 22, 2026
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants