TOML: map date/time values to Temporal, round-trip them through stringify - #37018
Conversation
… as date/time literals
TOML.parse previously returned all four TOML date/time types as strings
of their source text. They now map 1:1 onto Temporal:
- offset date-time -> Temporal.Instant (an offset date-time specifies an
instant; the written offset normalizes to UTC)
- local date-time -> Temporal.PlainDateTime (including TOML 1.1's space
separator and omitted seconds)
- local date -> Temporal.PlainDate
- local time -> Temporal.PlainTime
The parser now produces a dedicated E::DateTime AST node carrying the
kind and source text (fractional seconds truncated to Temporal's 9-digit
limit, as the TOML spec directs). All three sinks of the TOML AST stay
consistent: Bun.TOML.parse and import/require construct the Temporal
object through the same JSC code paths Temporal.*.from(string) uses, and
the bundler lowers the node to a Temporal.*.from("...") call over a real
unbound Temporal symbol so chunk renaming protects the global reference
and unused date exports stay tree-shakable.
TOML.stringify now emits Temporal.Instant, PlainDateTime, PlainDate, and
PlainTime as unquoted TOML date/time literals, so stringify(parse(doc))
round-trips date/times instead of re-quoting them as strings.
Temporal.ZonedDateTime emits its offset form (the time-zone annotation
has no TOML representation), non-ISO calendar annotations are dropped
the same way, and values outside TOML's 4-digit years throw like Date
already did. PlainYearMonth, PlainMonthDay, and Duration have no TOML
form and throw. Date is unchanged.
With BUN_JSC_useTemporal=0 a date/time value now throws a TypeError
(and a TOML module import fails with that exception instead of
panicking).
The toml-test conformance suite is regenerated: expectations compare
Temporal class + canonical toString, since Temporal instances have no
own properties for toEqual to see.
WalkthroughTOML date/time literals now retain their specific kind, parse into corresponding Temporal objects, and serialize back to TOML-compatible values. Bundler output, runtime conversion, error propagation, tests, and documentation were updated for the new behavior. ChangesTOML Temporal date/time handling
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
The TOML AST never enters the JS visit/transform passes (SLazyExport is not visited; the runtime import and Bun.TOML.parse convert the raw expr directly), so a dedicated expression variant is not needed: a toml_datetime tag on EString carries the same information, and only the TOML sinks (expr_to_js, data_to_js, to_lazy_export_ast, the printer's EString arm) check it. The bundler still rewrites tagged strings into Temporal.*.from calls over an unbound Temporal symbol for rename safety and tree shaking; behavior is unchanged and the test suite is identical.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
…assified Temporal cells unchecked
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/bundler/ParseTask.rs (1)
768-839: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winHandle the XML loader with the same error conversion.
The
Loader::Xmlbranch at Line 870 still calls.unwrap()onnew_lazy_export_ast(...). The upstream contract insrc/js_parser/parser.rs:1709-1756returnsOk(None)after logging parser failures. An XML parser failure can therefore panic instead of returningParserError. The panic also prevents the temporary log from flushing at Line 873.As per coding guidelines, user-reachable failures must be recoverable errors rather than panics or unreachable assertions.
Proposed fix
- .unwrap() + .ok_or(AnyError::ParserError)?,🤖 Prompt for 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. In `@src/bundler/ParseTask.rs` around lines 768 - 839, Update the Loader::Xml branch to handle new_lazy_export_ast(...) without unwrap: propagate its error and convert Ok(None) to AnyError::ParserError, matching the TOML/YAML/JSON5 branches. Keep the temporary-log closure and ensure all XML failure paths return through the existing temp_log flush before returning.Source: Coding guidelines
🤖 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 `@docs/runtime/toml.mdx`:
- Around line 144-146: The TOML serialization documentation and public
declaration must consistently state that invalid Date values are rejected and
supported Date or Temporal values outside years 0000–9999 are rejected. Update
the relevant serialization descriptions in docs/runtime/toml.mdx lines 144-146
and packages/bun-types/bun.d.ts lines 816-825, preserving the existing
supported-type behavior and wording consistency across both sites.
- Around line 144-146: Document that calendar annotations are normalized to
TOML-supported fields and not preserved alongside the existing time-zone
behavior in docs/runtime/toml.mdx lines 144-146 and packages/bun-types/bun.d.ts
lines 816-820; update both affected descriptions consistently without changing
the surrounding serialization behavior.
- Line 86: Update the date/time type description near the TOML-to-Temporal
mapping statement to remove the claim of universal losslessness. State that
Temporal preserves fractional seconds up to nanosecond precision and that the
TOML scanner truncates fractions beyond nine digits.
- Around line 140-150: Update the TOML runtime documentation around the
top-level object requirement to explicitly state that top-level undefined,
function, and symbol inputs return undefined, while those values remain skipped
when used as properties and throw inside arrays. Preserve the existing behavior
descriptions for other unsupported values.
- Line 54: The TOML date/time documentation must state that parsing fails when
Temporal is disabled. In docs/runtime/toml.mdx lines 54-54, add this condition
beside the Temporal type mappings; in packages/bun-types/bun.d.ts lines 795-800,
include the same failure behavior in the parse error documentation.
In `@src/bundler/transpiler.rs`:
- Around line 1942-1964: Update the candidate collision check in the
mangled_global_this construction to compare candidate against each property key
after applying the same normalization as ensure_valid_identifier. Preserve the
existing grow-loop behavior so normalized collisions append underscores before
allocating the final name.
In `@test/js/bun/toml/toml.test.ts`:
- Around line 341-346: Update the Bun.spawn invocation in the affected test to
pass an absolute entry path by joining or resolving dir with index.ts, matching
the approach used by the sibling test while preserving the existing cwd and
process options.
---
Outside diff comments:
In `@src/bundler/ParseTask.rs`:
- Around line 768-839: Update the Loader::Xml branch to handle
new_lazy_export_ast(...) without unwrap: propagate its error and convert
Ok(None) to AnyError::ParserError, matching the TOML/YAML/JSON5 branches. Keep
the temporary-log closure and ensure all XML failure paths return through the
existing temp_log flush before returning.
🪄 Autofix
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: 24893e72-9e74-420f-abd1-da115dd9f515
📒 Files selected for processing (22)
docs/runtime/toml.mdxpackages/bun-types/bun.d.tssrc/ast/e.rssrc/ast/expr.rssrc/bundler/ParseTask.rssrc/bundler/bundle_v2.rssrc/bundler/transpiler.rssrc/js_parser/parse/parse_entry.rssrc/js_parser_jsc/expr_jsc.rssrc/js_parser_jsc/lib.rssrc/js_printer/lib.rssrc/jsc/bindings/bindings.cppsrc/parsers/toml.rssrc/runtime/api.rssrc/runtime/api/TOMLObject.rstest/bundler/bundler_loader.test.tstest/js/bun/resolve/toml/toml-fixture.tomltest/js/bun/resolve/toml/toml-fixture.toml.txttest/js/bun/resolve/toml/toml.test.jstest/js/bun/toml/generate_toml_test_suite.tstest/js/bun/toml/toml-test-suite.test.tstest/js/bun/toml/toml.test.ts
…ead of throwing An offset date-time at the edge of TOML's four-digit years (e.g. 0000-01-01T00:00:00+01:00) parses to an Instant whose UTC year is -1 or 10000. stringify printed every Instant with Z and so rejected a value that TOML.parse itself produced. Pick the offset closest to Z (or to a ZonedDateTime's own offset) whose local year fits; the instant is unchanged to the nanosecond. Only instants a day or more outside 0000..9999, which TOML cannot spell at all, still throw.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
…hter comments, docs/types wording for truncation, annotations, year bounds
…xport unwrap parse_data_loader hand-builds the module without a symbol table, so a top-level TOML key that shadows a global the printed code needs cannot be handled the way the bundler does. Renaming vars by string comparison was open-ended; a key named globalThis next to a date/time on this path now fails at evaluation with a TypeError instead.
…sc::TemporalType Main's WebKit already provides JSC::TemporalType / JSC::temporalType(), so drop the hand-rolled inherits<> chain and the TOML-private enum in favor of Bun__JSValue__temporalType -> bun_jsc::TemporalType, the same surface the Temporal console formatting work uses.
There was a problem hiding this comment.
I reviewed the latest revision (through 0a4b019) and the automated bug-hunting pass found no issues; my earlier inline nits on ParseTask.rs and the deep-header test have all been addressed. Given the scope — new user-facing API surface (breaking change to TOML.parse return types, Date output format tweak), new C++ Temporal bindings with uncheckedDowncast, and the deliberate --no-bundle limitation for a top-level Temporal key — a maintainer sign-off is still warranted.
What was reviewed:
- Exception handling in
Bun__Temporal__fromDateTimeLiteral/toTOMLDateTime— throw scopes andRETURN_IF_EXCEPTIONare placed correctly;uncheckedDowncastis guarded by the caller'stemporalTypeclassification. - The 64-byte stack buffer vs. worst-case Temporal string length (35 bytes for a 9-frac-digit offset date-time) —
RELEASE_ASSERT(length <= bufLen)holds. tomlOffsetForInstantInt128 arithmetic and the year-edge offset selection round-trip cases.EStringlayout change (newOption<TomlDateTimeKind>field) propagated throughDefault,init, and theexpr.rsclone path.
Extended reasoning...
Overview
This PR changes Bun.TOML.parse to return Temporal objects for TOML's four date/time types instead of source-text strings, and teaches TOML.stringify to emit Temporal (and Date) values as unquoted TOML date/time literals so parse↔stringify round-trips. It touches 25 files across the TOML parser (src/parsers/toml.rs), AST (src/ast/e.rs, expr.rs), three consumption sinks (runtime expr_jsc.rs, bundler parse_entry.rs, printer lib.rs), new C++ JSC bindings (bindings.cpp — Bun__Temporal__fromDateTimeLiteral, Bun__Temporal__toTOMLDateTime, Bun__JSValue__temporalType, tomlOffsetForInstant), the TOML.stringify implementation, docs, types, and ~1000 lines of test changes including a regenerated 708-case conformance suite.
Security risks
None identified. Input is TOML source text validated by the existing scanner before reaching Temporal construction; the new truncate_fractional_seconds bounds its allocation by input length. The C++ toTOMLDateTime writes into a caller-supplied 64-byte stack buffer with a RELEASE_ASSERT(length <= bufLen) guard; the longest possible output (9999-12-31T23:59:59.999999999-23:59, 35 bytes) fits comfortably. uncheckedDowncast on Temporal cells is safe because temporalType was already computed on the same value and has_toml_form gates the switch arms.
Level of scrutiny
High. This is a breaking API change (date/times were strings, now Temporal objects), adds new C++↔Rust FFI surface with manual buffer handling, embeds an API design decision (offset date-time → Instant rather than ZonedDateTime; year-edge instants pick a fitting offset rather than throwing; .000Z → Z for Date), and accepts a known --no-bundle limitation (top-level Temporal key shadows the printed global). These are exactly the kind of decisions REVIEW.md says need maintainer agreement, not automated approval.
Other factors
The PR has been through extensive iteration: earlier rounds fixed iterative-walk stack safety, converted all new_lazy_export_ast .unwrap() sites, dropped an open-ended globalThis mangling scheme, and addressed docs/types wording. All 40+ review threads are marked resolved, and dylan-conway has been actively engaged resolving the latest round. Test coverage is thorough (conformance suite round-trip lap, bundler symbol-collision tests, both useTemporal=0 paths, GC stress, year-edge boundary cases). No bugs were found on this revision. The remaining question is maintainer sign-off on the API shape and the --no-bundle trade-off, which is not something an automated review should decide.
|
CI on 0a4b019 (build 93779): no failing job. The red status is two |
There was a problem hiding this comment.
I reviewed this PR and the bug-hunting pass found no issues. Given it's a user-facing API change (TOML date/times now return Temporal objects instead of strings) that spans the parser, printer, bundler lazy-export path, and new JSC C++ bindings, a maintainer sign-off on the API mapping and the --no-bundle shadowing trade-off would still be worthwhile.
What was reviewed:
- The three sinks (JSC materialization, bundler lowering, printer) stay consistent; the unbound
Temporalsymbol lets the chunk renamer handle collisions. Bun__Temporal__toTOMLDateTimebuffer bound: longest output (9999-12-31T23:59:59.999999999-23:59, 35 bytes) fits the 64-byte stack buffer, guarded byRELEASE_ASSERT.tomlOffsetForInstantInt128 arithmetic and the year-edge offset selection round-trip against the parse-side test cases.- Exception handling in the new C++ bindings uses
RETURN_IF_EXCEPTIONafter every fallible Temporal call; theuseTemporal=0path throws instead of reaching the missing structures.
Extended reasoning...
Overview
This PR changes Bun.TOML.parse to return Temporal objects (Instant/PlainDateTime/PlainDate/PlainTime) for TOML's four date/time types instead of source-text strings, and teaches TOML.stringify to emit all serializable Temporal types as unquoted TOML literals. It touches 25 files across the AST (toml_datetime tag on EString), the TOML parser, the JS printer, the bundler's lazy-export lowering (lower_date_time_literals in parse_entry.rs), the runtime expr_to_js converter, and ~130 lines of new C++ in bindings.cpp (Bun__Temporal__fromDateTimeLiteral, Bun__Temporal__toTOMLDateTime, tomlOffsetForInstant). The net diff is ~+1540/-770, plus a regenerated 708-case conformance suite and extensive hand-written tests.
Security risks
None identified. The only untrusted-input surface is the TOML scanner, which already validated date/time syntax; the new truncate_fractional_seconds operates on that validated ASCII slice. The C++ toTOMLDateTime writes into a caller-provided stack buffer with a RELEASE_ASSERT(length <= bufLen) bound; the longest possible output is well under 64 bytes. No auth, crypto, or filesystem paths are involved.
Level of scrutiny
High. This is a user-facing behavioral change to a documented API — code that previously received strings from TOML.parse will now receive Temporal objects. The PR encodes several API design decisions a maintainer should ratify: offset date-time → Instant (offset normalizes away) rather than preserving the written offset; ZonedDateTime and non-ISO calendars silently drop annotations on stringify; the --no-bundle path deliberately fails at runtime with a TypeError if a top-level TOML key is literally Temporal (the earlier string-mangling approach was removed in c45d52f after three review rounds). It also adds new C++ JSC bindings that use uncheckedDowncast after a Rust-side type discrimination — correct as written, but the kind of code REVIEW.md flags for careful review.
Other factors
The PR has been through many review iterations: eight prior inline findings from this system (all resolved with code changes — iterative worklist instead of recursion, .ok_or(ParserError)? on all 14 new_lazy_export_ast sites, concurrent stderr draining in tests), plus CodeRabbit and comment-cop passes. dylan-conway has been actively pushing fixes and resolving threads, so a human is already engaged, but no independent approval is on the thread. Test coverage is thorough (conformance suite round-trips, boundary years, sub-minute LMT offsets, GC stress, useTemporal=0 on both parse and import paths, bundler symbol-renaming tests). CI is green on all lanes that ran. Given the API-design surface and the C++ bindings, deferring rather than auto-approving.
2 participants
TOML.parse returned all four TOML date/time types as strings of their source text. They now map 1:1 and losslessly onto Temporal, which is enabled by default on main:
1979-05-27T00:32:00-07:00)Temporal.Instant1979-05-27T07:32:00, 1.1's1979-05-27 07:32)Temporal.PlainDateTime1979-05-27)Temporal.PlainDate07:32:00,07:32)Temporal.PlainTimePer the spec an offset date-time "specifies an instant", so it is
Temporal.Instantand the written offset normalizes to UTC. Sub-second digits are preserved (Temporal carries nanoseconds; fractional seconds beyond 9 digits are truncated as the TOML spec directs, since Temporal rejects them). Leap-second:60clamps to:59through Temporal's own ISO parsing.How
E::Stringit produces with the date/time kind (toml_datetimeonEString); the lexer already distinguished the four kinds. The TOML AST never enters the JS visit/transform passes, so only the TOML sinks check the tag.Bun.TOML.parseandimport/requireof.tomlconstruct the object through the same JSC code pathsTemporal.*.from(string)uses (new bindings inbindings.cpp).Temporal.*.from("...")call over a real unboundTemporalsymbol, so the chunk renamer renames a user binding namedTemporalinstead of letting it capture the reference, and the calls are pure-annotated so unused exports tree-shake.bun build --no-bundleof a.tomlfile (which prints the data AST directly, without a symbol table) prints the tagged string as a bareTemporal.*.from("...")call. That path turns each top-level key into a module-scopevar, so a document with both a date/time and a top-level key literally namedTemporalfails at evaluation with aTypeError; renaming without a symbol table was tried and dropped as open-ended.TOML.stringifyemitsTemporal.Instant/PlainDateTime/PlainDate/PlainTimeas unquoted TOML literals, sostringify(parse(doc))round-trips date/times.Temporal.ZonedDateTimeis also accepted and emits the offset at that instant, dropping the[Time/Zone]annotation (TOML has no zone syntax); non-ISO[u-ca=...]calendar annotations are dropped the same way (the stored ISO fields are emitted).PlainYearMonth/PlainMonthDay/Durationhave no TOML form and throw. Values TOML's 4-digit years cannot spell throw likeDatealready did, with one refinement for instants: an offset date-time at a year edge (0000-01-01T00:00:00+01:00is valid TOML but its UTC year is -1) is emitted with the nearest offset whose local year fits, so everythingparseaccepts also stringifies; only instants a day or more outside 0000..9999 throw.Dateoutput now trims trailing fraction zeros (Zrather than.000Z) soDateandInstantspell the same instant identically; otherwiseDatehandling is unchanged.BUN_JSC_useTemporal=0, a date/time value throws aTypeError("Date/time values require Temporal, which is disabled in this process"); the module-import path previously would have panicked on any conversion error and now fails the load with the pending exception.Everything else about parse/stringify (error messages, layout, numbers) is byte-for-byte unchanged.
Tests
toml-test-suite.test.ts(708 cases) from the same pinned toml-test commit: datetime expectations construct the Temporal value and compare with plaintoEqual(deepEquals learned Temporal objects in Compare Temporal objects by value in Bun.deepEquals and toEqual #37024). All pass, including theparse(stringify(parse(input)))lap each valid case asserts.toml.test.ts: the four mappings, instant semantics, TOML spellings Temporal does not print (space separator, lowercaset/z, omitted seconds, leap second), fraction truncation, nesting, bothuseTemporal=0behaviors, stringify of all eight Temporal types, year bounds, calendar/zone annotation dropping, array layout.test/js/bun/resolve/toml) extended with a[dates]table; bundler tests assert the emittedTemporal.*.frommodules run and that a uservar Temporalin the same bundle is renamed instead of captured.Types and docs updated (
bun.d.tsJSDoc,docs/runtime/toml.mdx).[review] gate passed · iteration 3 · 25 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 7 passed · 0 rejected · iteration 3
evidence per changed file