Bun.JSONC.parse: throw SyntaxError instead of BuildMessage on invalid input - #35066
Conversation
WalkthroughChangesBun.JSONC.parse now rejects empty input, preserves explicit resource-error handling, and reports parser-specific syntax messages. Tests update JSONTestSuite classification and cover error types and message selection. JSONC Error Handling
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — the follow-up commit addresses the warning-vs-error message ordering and adds a test for it.
What was reviewed:
- Error path now mirrors
TOMLObject.rs/JSON5Object.rs/YAMLObject.rsexactly (StackOverflow→stack overflow,Alloc→OOM, else→SyntaxErrorfrom log). bun_parsers::Errorvariants — remaining catch-all arm covers only syntax-class errors (SyntaxError,ParserError,UTF8Fail,UnexpectedSyntax,JSONStringsMustUseDoubleQuotes).- Empty-input check is scoped to the host fn, so internal tsconfig/package.json callers keep lenient
{}; whitespace-only inputs still route through the parser (covered byn_single_space/" "test case). .d.tsand docs don't document the old""→{}behavior, so nothing else needs updating.
Extended reasoning...
Overview
Changes the error type thrown by Bun.JSONC.parse on invalid input from BuildMessage (not an Error subclass) to SyntaxError, aligning it with JSON.parse and every sibling Bun parser (Bun.JSON5, Bun.TOML, Bun.YAML, Bun.JSONL). Also makes Bun.JSONC.parse("") throw SyntaxError instead of returning {}, matching JSON.parse(""). Touches one ~30-line host function in src/runtime/api/JSONCObject.rs, moves one JSONTestSuite entry from the "valid JSONC" to the "invalid" bucket, and adds two targeted tests to jsonc.test.ts.
Security risks
None. This is error-type reshaping on an already-rejecting parse path; no new input is accepted, no allocation sizing changes, and the underlying parser is untouched.
Level of scrutiny
Low-to-moderate. It's a user-facing behavior change (error class + empty-string semantics), but a small mechanical one that copies the exact structure already shipped for TOML/JSON5/YAML in the same directory. The empty-string change is scoped to the public host function so internal config-file callers (tsconfig, package.json) retain their lenient behavior. Test coverage is strong: the new SyntaxError test iterates six invalid inputs including "" and " ", the JSONTestSuite n_structure_no_data.json case moved to the throwing bucket, and the whole 318-case corpus continues to pass.
Other factors
My earlier inline comment (a duplicate-key warning could mask the real error message because log.msgs.first() was taken unconditionally) was addressed in 9138cab: the code now prefers the first Kind::Err entry with a fallback to first(), and a regression test asserts the message for '{"a":1,"a":2,' does not mention the duplicate key. I checked bun_parsers::Error — the catch-all Err(_) arm only covers syntax-class variants, so mapping them all to SyntaxError is correct. The LogJsc import removal is clean (no longer used after dropping log.to_js). Grepped packages/bun-types and docs/ — neither documents the old ""→{} behavior, so no doc drift.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/js/bun/jsonc/jsonc.test.ts`:
- Around line 128-153: Strengthen the assertions in the Bun.JSONC.parse error
tests by checking the specific fatal diagnostic text for representative
malformed inputs, rather than only the shared prefix. Update the malformed-input
table to pair inputs with expected messages, and make the
duplicate-key-plus-truncation test assert the expected truncation diagnostic
while still excluding the preceding “Duplicate key” warning.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5830fdae-0d8e-4ea1-bbb9-a6012a3018d8
📒 Files selected for processing (3)
src/runtime/api/JSONCObject.rstest/js/bun/jsonc/json-test-suite.test.tstest/js/bun/jsonc/jsonc.test.ts
There was a problem hiding this comment.
LGTM — brings Bun.JSONC.parse in line with the TOML/JSON5/YAML sibling parsers by throwing SyntaxError instead of BuildMessage.
What was reviewed:
- Error-path shape mirrors
TOMLObject.rs/JSON5Object.rs(StackOverflow→ stack overflow,Alloc→ OOM, else →SyntaxErrorfrom first log msg); theKind::Err-first filter from my earlier comment is in place and covered by the inline-snapshot test. - Empty-input rejection is scoped to the host function so internal tsconfig/package.json callers keep lenient
{};n_structure_no_data.jsonreclassified accordingly and whitespace-only (" ",n_single_space.json) still routes through the parser and throws. - Unused
LogJscimport dropped;bun_parsers::Errorcatch-all covers the remaining variants (SyntaxError/ParserError/UTF8Fail/UnexpectedSyntax).
Extended reasoning...
Overview
Changes the error type thrown by Bun.JSONC.parse on invalid input from BuildMessage (not an Error) to SyntaxError, matching JSON.parse and every other Bun.*.parse sibling. Also makes Bun.JSONC.parse("") throw instead of returning {}. Touches one ~70-line host-function file and two test files (+63/-6 net).
Security risks
None. No new input paths — only the error object constructed on the existing failure path changes. The empty-input check reads source.contents.is_empty() and throws; no allocation or parsing happens on that branch.
Level of scrutiny
Low-medium. The implementation is a near-verbatim copy of the TOMLObject.rs / JSON5Object.rs / YAMLObject.rs error path (verified by grep), plus the Kind::Err-preference tweak from my earlier review. The one behavioral judgment call — rejecting empty input — is well-justified (matches JSON.parse and Bun.JSON5.parse), scoped to the public API only (internal config-file callers still get lenient {} from parse_jsonc itself), and pinned by moving n_structure_no_data.json into N_INVALID in the JSONTestSuite corpus.
Other factors
- All three prior review threads (mine on warning-vs-error message selection, CodeRabbit on assertion strength) are resolved; the fix commits landed and the duplicate-key-plus-truncation case is now pinned via
toMatchInlineSnapshot. - The
bun_parsers::Errormatch handlesStackOverflowandAlloc(_)explicitly and catches the rest (SyntaxError,ParserError,UTF8Fail,UnexpectedSyntax) in the fallback arm — same coverage as the TOML sibling. - New tests cover:
SyntaxErrorinstance/name/message-prefix across six malformed inputs including""and" "; the warning-before-error case; and the JSONTestSuiten_structure_no_datareclassification. PR evidence shows tests fail on main (release) and pass on the PR (ASAN + release). - The now-unused
LogJscimport was removed.
|
Rebased onto current main (includes the SIMD JSON parser rewrite from #33032; no conflicts, all 370 JSONC tests pass locally with the debug build, including the inline snapshot for the diagnostic message). Added Fail-before still holds: the two new tests fail on the released build and pass with this diff. Previous build 77492 was red only on lanes unrelated to this diff (worker_threads stress SIGABRT, already reported for main-break triage, plus retries-passed flakes). Fresh CI run on the rebase: 89444. Review requested. The one behavior callout for reviewers: |
… input
Aligns Bun.JSONC.parse with JSON.parse, Bun.JSON5.parse, Bun.YAML.parse,
Bun.TOML.parse, and Bun.JSONL.parse, all of which throw a real SyntaxError
(which is instanceof Error). Previously JSONC threw a BuildMessage, so a
'catch (e) { if (e instanceof SyntaxError) }' handler that worked for the
rest of the parser family missed JSONC.
Also makes Bun.JSONC.parse('') throw SyntaxError to match JSON.parse and
Bun.JSON5.parse. The internal JSONC parser (tsconfig.json, package.json)
keeps its lenient empty-file -> {} behavior.
…yntaxError message parse_jsonc emits duplicate-key warnings before the fatal error, so log.msgs.first() could surface the warning text instead of the real parse error in the thrown SyntaxError message.
…nostic via snapshot
4809a05 to
71d34a5
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code looks correct and all prior review feedback (warning-vs-error message selection, comment-cop) is addressed — the error path now mirrors TOMLObject.rs / JSON5Object.rs exactly, plus the Kind::Err filter. Deferring to a maintainer only for the flagged public-API behavior change: Bun.JSONC.parse("") now throws where it previously returned {}.
What was reviewed:
- Error-path shape checked against
TOMLObject.rs:31-46andJSON5Object.rs:61-74— sameStackOverflow/Alloc/SyntaxErrorstructure and message format. - Empty-input check is on
source.contents(raw bytes before parsing), so whitespace-only still reaches the parser and errors there — covered by the" "case in the new test. n_structure_no_data.jsonmoved fromN_VALID_JSONCtoN_INVALID; corpus count assertion (318) unchanged since it's a move, not an add.
Extended reasoning...
Overview
The PR changes Bun.JSONC.parse to throw SyntaxError instead of BuildMessage on invalid input, aligning it with the five sibling parsers (JSON.parse, Bun.JSON5.parse, Bun.YAML.parse, Bun.TOML.parse, Bun.JSONL.parse). It also makes Bun.JSONC.parse("") throw rather than return {}. Files touched: src/runtime/api/JSONCObject.rs (~35 lines net), two test files, and one JSDoc line in bun.d.ts.
Security risks
None. This is error-type/message plumbing on an in-process text parser. No new input surface, no allocation driven by untrusted sizes, no privilege boundaries.
Level of scrutiny
Medium. The mechanical change (BuildMessage → SyntaxError) is a straightforward bug fix that copies the exact pattern from TOMLObject.rs and JSON5Object.rs — I verified the match structure, error variants, and message format are byte-for-byte parallel. The Kind::Err filter is a small, well-justified deviation (JSONC's parser emits duplicate-key warnings that TOML/JSON5 don't). Tests are solid: a table of malformed inputs asserting instanceof SyntaxError, an inline snapshot pinning the diagnostic for the warning-precedes-error case, and the JSONTestSuite n_structure_no_data case moved to the must-reject list.
Other factors
Two rounds of my own review feedback were applied and resolved (the Kind::Err filter, and collapsing multi-line comments to satisfy comment-cop). CodeRabbit's assertion-strength concern was also addressed via the inline snapshot. All inline threads are resolved. CI is running on the latest commit.
The reason I'm not approving outright: the author explicitly flagged the Bun.JSONC.parse("") behavior change as "the one behavior callout for reviewers." It's well-justified (JSONTestSuite classifies empty input as must-reject, VS Code's jsonc-parser rejects it, JSON.parse("") and Bun.JSON5.parse("") both throw, and whitespace-only input already threw), and the internal tsconfig/package.json path keeps its lenient behavior. But it is a user-observable breaking change on a public API, and per REVIEW.md's API-design guidance that's a maintainer call, not mine.
… input (oven-sh#35066) ## What `Bun.JSONC.parse` now throws a `SyntaxError` on invalid input, matching `JSON.parse`, `Bun.JSON5.parse`, `Bun.YAML.parse`, `Bun.TOML.parse`, and `Bun.JSONL.parse`. Previously it threw a `BuildMessage`, which is not an `Error`, so `catch (e) { if (e instanceof SyntaxError) }` (or `e instanceof Error`) missed JSONC alone out of the whole parser family. Also: `Bun.JSONC.parse("")` now throws `SyntaxError` to match `JSON.parse("")` and `Bun.JSON5.parse("")`. The old behavior was accidental: `Bun.JSONC.parse("")` returned `{}` while `Bun.JSONC.parse(" ")` threw, because the internal parser's empty-file special case (there so an empty `tsconfig.json` / `package.json` parses as `{}`) leaked through the public API and whitespace-only input missed it. Empty input is invalid in every JSON dialect: the official JSONTestSuite lists it as a must-reject case (`n_structure_no_data`, moved to the invalid list in this PR), and VS Code's `jsonc-parser` reports a parse error on it. The internal tsconfig/package.json path keeps its lenient empty-file -> `{}` behavior (the check is in the `Bun.JSONC.parse` host function, not the shared parser). ## Repro ```js for (const [n, f] of [["JSON.parse", JSON.parse], ["Bun.JSON5.parse", Bun.JSON5.parse], ["Bun.JSONC.parse", Bun.JSONC.parse], ["Bun.YAML.parse", Bun.YAML.parse], ["Bun.TOML.parse", Bun.TOML.parse], ["Bun.JSONL.parse", Bun.JSONL.parse]]) { try { f("{ not valid"); } catch (e) { console.log(n, e.constructor.name, "instanceof Error:", e instanceof Error); } } ``` Before: ``` Bun.JSONC.parse BuildMessage instanceof Error: false ``` (everything else: `SyntaxError` / `true`) After: ``` Bun.JSONC.parse SyntaxError instanceof Error: true ``` ## Cause `src/runtime/api/JSONCObject.rs` threw `log.to_js(global, "Failed to parse JSONC")`, which materializes a `BuildMessage`. The sibling parsers (`TOMLObject.rs`, `JSON5Object.rs`, `YAMLObject.rs`) all build a `SyntaxError` via `global.create_syntax_error_instance(...)` from the first log message. TOML switched to this pattern in the parser rewrite; JSONC never did. ## Fix Mirror the TOML/JSON5 error path: match on `bun_parsers::Error` (`StackOverflow` -> stack overflow, `Alloc` -> OOM, everything else -> `SyntaxError` carrying the first log message text), and reject empty input up front with a `SyntaxError`. The `JSONC.parse` JSDoc in `packages/bun-types/bun.d.ts` now documents the throw (`@throws {SyntaxError}`, same as `TOML.parse`). <!-- robobun:evidence:begin --> --- **[review]** gate passed · iteration 0 · 3 files touched <details><summary>fails on main (without fix)</summary> ```console ASAN without fix: BUILD FAILED (no junit output) $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" test/js/bun/jsonc/json-test-suite.test.ts test/js/bun/jsonc/jsonc.test.ts ninja: Entering directory `/workspace/bun/build/debug' [1/41] cxx obj/src/jsc/bindings/webcore/streams/BunAsyncIterableSource.cpp.o [2/41] cxx obj/src/jsc/bindings/webcore/streams/JSByteLengthQueuingStrategy.cpp.o [3/41] cxx obj/src/jsc/bindings/webcore/streams/JSReadableByteStreamController.cpp.o [4/41] cxx obj/src/jsc/bindings/webcore/streams/JSDirectStreamController.cpp.o [5/41] cxx obj/src/jsc/bindings/webcore/streams/CrossRealmTransform.cpp.o [6/41] cxx obj/unified/UnifiedSource-src_jsc_bindings_webcore-11.cpp.o [7/41] cxx obj/unified/UnifiedSource-src_jsc_bindings_webcore-14.cpp.o [8/41] cxx obj/src/jsc/bindings/webcore/HTTPParsers.cpp.o [9/41] cxx obj/src/jsc/bindings/webcore/streams/JSCountQueuingStrategy.cpp.o [10/41] cxx obj/src/jsc/bindings/bindings.cpp.o [11/41] cxx obj/src/jsc/bindings/webcore/streams/BunStreamConsumers.cpp.o [12/41] cxx obj/src/jsc/bindings/webcore/streams/JSReadableStream.cpp.o [13/41] cxx obj/src/jsc/bindings/webcore/streams/BunSt ... (truncated) release without fix: all passed bun test v1.4.0-canary.1 (9138cab) test/js/bun/jsonc/jsonc.test.ts: (pass) Bun.JSONC exists [0.05ms] (pass) Bun.JSONC.parse handles basic JSON [0.08ms] (pass) Bun.JSONC.parse handles comments [0.03ms] (pass) Bun.JSONC.parse handles a comment after a scalar on the same line [0.04ms] (pass) Bun.JSONC.parse handles trailing commas [0.03ms] (pass) Bun.JSONC.parse handles arrays with trailing commas [0.02ms] (pass) Bun.JSONC.parse handles complex JSONC [0.05ms] (pass) Bun.JSONC.parse handles nested objects [0.03ms] (pass) Bun.JSONC.parse handles boolean and null values [0.03ms] (pass) Bun.JSONC.parse throws on invalid JSON [0.05ms] (pass) Bun.JSONC.parse throws a SyntaxError on invalid input [2.72ms] (pass) Bun.JSONC.parse SyntaxError names the actual error, not a preceding warning [0.17ms] (pass) Bun.JSONC.parse handles empty object [0.07ms] (pass) Bun.JSONC.parse handles empty array [0.03ms] (pass) Bun.JSONC.parse throws on deeply nested arrays instead of crashing [6.46ms] (pass) Bun.JSONC.parse throws on deeply nested objects instead of crashing [5.66ms] (pass) Bun.JSONC.parse handles pathological inputs in linear time [47.35ms] (pass) Bun.JSONC.parse matches JSON. ... (truncated) ``` </details> <details><summary>passes on PR (with fix)</summary> ```console ASAN with fix: all passed $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" test/js/bun/jsonc/json-test-suite.test.ts test/js/bun/jsonc/jsonc.test.ts bun test v1.4.0 (4809a05) test/js/bun/jsonc/jsonc.test.ts: (pass) Bun.JSONC exists [3.12ms] (pass) Bun.JSONC.parse handles basic JSON [2.58ms] (pass) Bun.JSONC.parse handles comments [2.21ms] (pass) Bun.JSONC.parse handles a comment after a scalar on the same line [2.55ms] (pass) Bun.JSONC.parse handles trailing commas [3.20ms] (pass) Bun.JSONC.parse handles arrays with trailing commas [2.09ms] (pass) Bun.JSONC.parse handles complex JSONC [3.27ms] (pass) Bun.JSONC.parse handles nested objects [2.32ms] (pass) Bun.JSONC.parse handles boolean and null values [2.35ms] (pass) Bun.JSONC.parse throws on invalid JSON [4.44ms] (pass) Bun.JSONC.parse throws a SyntaxError on invalid input [17.76ms] (pass) Bun.JSONC.parse SyntaxError names the actual error, not a preceding warning [4.38ms] (pass) Bun.JSONC.parse handles empty object [4.47ms] (pass) Bun.JSONC.parse handles empty array [1.86ms] (pass) Bun.JSONC.parse throws on deeply nested arrays instead of crashing [17.77 ... (truncated) release with fix: all passed $ bun scripts/build.ts --profile=release [configured] bun-profile → bun (stripped) in 748ms (unchanged) ninja: Entering directory `/workspace/bun/build/release' [1/41] cxx obj/src/jsc/bindings/webcore/streams/BunAsyncIterableSource.cpp.o [2/41] cxx obj/src/jsc/bindings/webcore/streams/JSCountQueuingStrategy.cpp.o [3/41] cxx obj/src/jsc/bindings/webcore/streams/CrossRealmTransform.cpp.o [4/41] cxx obj/src/jsc/bindings/webcore/streams/JSReadableStreamAsyncIterator.cpp.o [5/41] cxx obj/src/jsc/bindings/webcore/streams/BunStreamConsumers.cpp.o [6/41] cxx obj/src/jsc/bindings/webcore/streams/BunStreamSource.cpp.o [7/41] cxx obj/src/jsc/bindings/webcore/streams/JSByteLengthQueuingStrategy.cpp.o [8/41] cxx obj/src/jsc/bindings/webcore/streams/JSDirectStreamController.cpp.o [9/41] cxx obj/src/jsc/bindings/webcore/HTTPParsers.cpp.o [10/41] cxx obj/src/jsc/bindings/webcore/streams/JSReadableByteStreamController.cpp.o [11/41] cxx obj/src/jsc/bindings/webcore/streams/JSReadableStream.cpp.o [12/41] cxx obj/unified/UnifiedSource-src_jsc_bindings_webcore-3.cpp.o [13/41] cxx obj/src/jsc/bindings/webcore/streams/JSStreamsRuntime.cpp.o [14/41] cxx obj/src/jsc/bindings/webcore/strea ... (truncated) ``` </details> <details><summary>diff hotspot</summary> ``` src/runtime/api/JSONCObject.rs | 41 +++++++++++++++++++++++++++---- test/js/bun/jsonc/json-test-suite.test.ts | 2 +- test/js/bun/jsonc/jsonc.test.ts | 27 ++++++++++++++++++++ 3 files changed, 64 insertions(+), 6 deletions(-) ``` </details> **gate history** · 3 passed · 0 rejected · iteration 0 <details><summary>evidence per changed file</summary> ``` file reads edits tests src/runtime/api/JSONCObject.rs 3 5 0 test/js/bun/jsonc/json-test-suite.test.ts 2 2 0 test/js/bun/jsonc/jsonc.test.ts 1 3 0 ``` </details> <!-- robobun:evidence:end --> --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
3 participants
What
Bun.JSONC.parsenow throws aSyntaxErroron invalid input, matchingJSON.parse,Bun.JSON5.parse,Bun.YAML.parse,Bun.TOML.parse, andBun.JSONL.parse. Previously it threw aBuildMessage, which is not anError, socatch (e) { if (e instanceof SyntaxError) }(ore instanceof Error) missed JSONC alone out of the whole parser family.Also:
Bun.JSONC.parse("")now throwsSyntaxErrorto matchJSON.parse("")andBun.JSON5.parse(""). The old behavior was accidental:Bun.JSONC.parse("")returned{}whileBun.JSONC.parse(" ")threw, because the internal parser's empty-file special case (there so an emptytsconfig.json/package.jsonparses as{}) leaked through the public API and whitespace-only input missed it. Empty input is invalid in every JSON dialect: the official JSONTestSuite lists it as a must-reject case (n_structure_no_data, moved to the invalid list in this PR), and VS Code'sjsonc-parserreports a parse error on it. The internal tsconfig/package.json path keeps its lenient empty-file ->{}behavior (the check is in theBun.JSONC.parsehost function, not the shared parser).Repro
Before:
(everything else:
SyntaxError/true)After:
Cause
src/runtime/api/JSONCObject.rsthrewlog.to_js(global, "Failed to parse JSONC"), which materializes aBuildMessage. The sibling parsers (TOMLObject.rs,JSON5Object.rs,YAMLObject.rs) all build aSyntaxErrorviaglobal.create_syntax_error_instance(...)from the first log message. TOML switched to this pattern in the parser rewrite; JSONC never did.Fix
Mirror the TOML/JSON5 error path: match on
bun_parsers::Error(StackOverflow-> stack overflow,Alloc-> OOM, everything else ->SyntaxErrorcarrying the first log message text), and reject empty input up front with aSyntaxError.The
JSONC.parseJSDoc inpackages/bun-types/bun.d.tsnow documents the throw (@throws {SyntaxError}, same asTOML.parse).[review] gate passed · iteration 0 · 3 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 3 passed · 0 rejected · iteration 0
evidence per changed file