fix: reject stray commas in object, array, call and parameter lists - #1113
Open
He-Pin wants to merge 2 commits into
Open
fix: reject stray commas in object, array, call and parameter lists#1113He-Pin wants to merge 2 commits into
He-Pin wants to merge 2 commits into
Conversation
…r lists
Motivation:
`{,}` (and `{ , }`) parsed as an empty object while go-jsonnet and
jrsonnet both report a parse error: the comma-list grammars combined
`rep(sep = ",")` (which accepts zero elements) with an optional trailing
comma, so a single comma was consumed as the "trailing" comma of an
empty list. The same hole accepted `f(,)` and `function(,)` (rejected
only later, or at runtime). The Jsonnet grammar requires at least one
element before a comma.
Modification:
Capture the trailing comma in the three affected grammars (objinside,
args, params) and fail with a descriptive parse error when a comma
appears with zero preceding elements. A straight `rep(1, ...) ~ ... .?`
restructuring was rejected because it triggers a Scala 2.13 lambda-lift
compiler bug ("Could not find proxy for case val x1: ParsingRun") with
fastparse's implicit plumbing; the capture-and-check shape keeps the
original flat structure.
Result:
`{,}`, `{ , }`, `f(,)`, `function(,)` are parse errors; valid trailing
commas are unchanged (`{a: 1,}`, `[1, 2,]`, `f(1, 2,)`,
`function(a,)`, object comprehensions, object locals/asserts).
References:
Found by four-way differential testing (sjsonnet vs go-jsonnet vs
jrsonnet vs spec).
Motivation: The same trailing-comma capture hole existed in arrBody: after the first comma, `expr.rep(0, sep = ",") ~ ",".?` accepted zero elements followed by a comma, so `[1,,]` (and `[1, ,]`) parsed as `[1]` while go-jsonnet, C++ jsonnet and jrsonnet all report a parse error. Modification: Capture the trailing comma in the inner rest-of-array grammar and fail with "at least one array element before ','" when the repetition is empty, mirroring the objinside/args/params fixes. Set cut=true so the error propagates through the enclosing `.?`. Valid forms are unaffected: `[1,]`, `[1, 2,]`, and `[e, for x in xs]`. Result: `[1,,]` is a parse error, matching all three reference implementations. Found by the same four-way differential testing. References: Extends PR databricks#1113 (lone commas in objects, calls, params).
He-Pin
marked this pull request as ready for review
August 7, 2026 06:01
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
{,}(and{ , }) parsed as an empty object, while go-jsonnet and jrsonnet both report a parse error:{,}{ }Unexpected: "," while parsing field definitionexpected field name, got ','{ , }{ }local f(a) = a; f(,)(function(,) 1)()1[1,,][1]Root cause: the comma-list grammars combined
rep(sep = ",")(accepts zero elements) with an optional trailing comma, so a lone comma was consumed as the "trailing" comma of an empty list. The Jsonnet grammar requires at least one element before a comma. The array grammar had a related hole: after the first comma,expr.rep(0, sep = ",") ~ ",".?accepted zero elements followed by a comma, so[1,,]parsed as[1].Modification
sjsonnet/src/sjsonnet/Parser.scala— capture the trailing comma (,.!) in the four affected grammars (objinside,args,params,arrBody`) and fail with a descriptive parse error when a comma appears with zero preceding elements:Expected at least one object member before ','Expected at least one argument before ','Expected at least one parameter before ','Expected at least one array element before ','Implementation note: the obvious
rep(1, sep = ",") ~ ",".?wrapped in an outer.?was tried first and rejected — it triggers a Scala 2.13 lambda-lift compiler bug (Could not find proxy for case val x1: fastparse.ParsingRun) with fastparse's implicit plumbing. The capture-and-check shape keeps the original flat parser structure and compiles on all Scala versions. ForarrBody,cut = trueis set before the failure so the error propagates through the enclosing.?instead of being swallowed.Result
{,},{ , },f(,),function(,),[1,,]are parse errors.{a: 1,},{a: 1, b: 2,},[1, 2,],f(1, 2,),function(a,) e,function(x=1,) e,[1, for x in xs], plus object comprehensions, object locals and asserts with commas.{,a: 1},{a: 1,, b: 2},[,],[1,,2],{,,}).Test plan
new_test_suite/error.parse_object_lone_comma.jsonnet,error.parse_call_lone_comma.jsonnet,error.parse_params_lone_comma.jsonnet,error.parse_array_double_comma.jsonnet(+ goldens)new_test_suite/parse_trailing_commas_valid.jsonnet(+ golden) pinning the preserved valid forms./mill 'sjsonnet.jvm[_].test'— all Scala versions (2.12.21 / 2.13.18 / 3.3.8) pass./mill 'sjsonnet.js[_].compile' 'sjsonnet.native[_].compile' 'sjsonnet.wasm[_].compile'— pass./mill __.checkFormat— cleanFound by four-way differential testing (sjsonnet vs go-jsonnet vs jrsonnet vs spec).