Format Temporal values in console.log, util.inspect, and test pretty-format - #37043
Format Temporal values in console.log, util.inspect, and test pretty-format#37043robobun wants to merge 16 commits into
Conversation
…format
Temporal cells are plain ObjectType, so every formatter treated them as
generic objects: console.log/Bun.inspect dumped prototype methods and
slot getters as if they were own properties (33 lines for a PlainDate),
util.inspect printed "PlainDate [Temporal.PlainDate] {}" with the value
invisible, and the test runner's pretty-format printed "PlainDate {}",
so every instance of a class snapshotted identically and toEqual
failures diffed two identical sides.
All three now print the value as a single token in the form
"Temporal.<Class> <toString()>", e.g.:
Temporal.PlainTime 10:20:23
Temporal.ZonedDateTime 2020-01-01T00:00:00-10:00[Pacific/Honolulu]
Temporal.Duration PT1H30M
The text is the spec toString() with default options (lossless,
round-trips through Temporal.X.from()), computed from internal slots via
ClassInfo so inspection never calls user-observable methods. A shared
native helper covers all 8 types: a classifier
(Bun__JSValue__temporalObjectType) and a formatter
(Bun__Temporal__toDisplayString / Bun::temporalDisplayString) backed by
each class's internal toString; ZonedDateTime replicates the spec
recipe since JSC keeps that one file-static. console.log/Bun.inspect
and pretty-format print the label uncolored and the value in Date's
magenta, ignoring own properties and subclass names like Date does.
util.inspect follows Node's behavior for nodejs/node#63154: getPrefix
subclass/null-prototype handling, the 'date' style, and extra own
properties appended.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughChangesTemporal formatting
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I didn't find any bugs, but this adds a new C++ file that replicates JSC's file-static temporalZonedDateTimeToString recipe (offset rounding, calendar annotation) and picks a user-facing output format for Bun.inspect on Temporal values — worth a human glance at those two choices.
What was reviewed:
- ZonedDateTime offset rounding in
Temporal.cpp— half-expand tie handling checks out against the Monrovia (-00:44:30 → -00:45) and Kolkata (+05:53:28 → +05:53) test cases. - Exception scopes in
temporalDisplayString/Bun__Temporal__toDisplayString/jsFunctionTemporalToDisplayString—RETURN_IF_EXCEPTIONafter each fallible call. inspect.jsplacement — the Temporal branch sits afterbraces = ["{", "}"]and after theconstructor === "Object"fast path, so plain objects don't pay for the classifier and the with-own-props case falls through correctly.- Perf of the classifier on the
T::Objecthot path —FinalObject(plain{}) short-circuits before theinherits<>chain.
Extended reasoning...
Overview
Adds Temporal.<Class> <toString()> formatting for all 8 Temporal types across three formatters: console.log/Bun.inspect (ConsoleObject.rs), the test runner's pretty-format (pretty_format.rs), and util.inspect (inspect.js). New C++ bindings in src/jsc/bindings/Temporal.{h,cpp} provide a shared classifier (Bun__JSValue__temporalObjectType, 0–8 discriminant) and a display-string helper (Bun::temporalDisplayString) that reads internal slots directly, never calling user-observable toString. ~500 lines including tests across 12 files.
Security risks
None. The classifier uses JSCell::inherits<> (ClassInfo walk), and the display string is built from JSC-internal slot accessors — no user code runs, no untrusted parsing, no allocation sized from external input.
Level of scrutiny
Medium. This is user-visible output formatting, not a hot correctness path, but it:
- Hand-replicates the spec's
temporalZonedDateTimeToString(JSC's own is file-static inTemporalZonedDateTimePrototype.cpp), including theFormatDateTimeUTCOffsetRoundedhalf-expand rounding. I traced the arithmetic against the test fixtures and it matches, and the tests cross-check against the real.toString()for the sub-minute-offset zones — but a maintainer may prefer patching JSC to expose the real function rather than duplicating it. - Picks an output format for
Bun.inspect(Temporal.PlainDate 2020-01-02, label uncolored + magenta value) that differs from howBun.inspectshowsDate(bare ISO string, no label). It follows Node's open PR #63154 forutil.inspect, which is the right call there; whetherBun.inspectshould carry the label too is a product choice.
Other factors
- All prior automated feedback (comment-cop on comment length, CodeRabbit on clippy lints — SAFETY comment,
&raw mut, redundant&*) is resolved in d20fcc7 and d3613cf. - Test coverage is thorough: all 8 types, calendar/zone annotations, non-ISO YearMonth/MonthDay reference dates, sub-minute historic offsets, nesting, depth, subclasses, null-prototype, colors, prototypes,
Temporal.Now, and a 48-case port of Node's own test. - The PR description flags a known merge conflict with #37018 (same classifier symbol).
- The
uncheckedDowncast<>calls intemporalDisplayStringare safe because every caller passes atemporalTypefreshly returned byBun__JSValue__temporalObjectTypeon the same value; the JS host function re-classifies and returnsundefinedon 0 rather than trusting its argument.
|
On the two judgment calls flagged above, both are deliberate: The label (where Date prints a bare ISO string) is part of the decided format. Bare Temporal text is ambiguous or cryptic without the type: an Instant would print byte-identical to a Date (Deno had exactly this bug, deno#27585), and fragments like PT1H30M, 01-15, or 2020-01 say nothing about what they are. The labeled form also pastes straight back into Temporal.X.from(). Node's util.inspect PR (nodejs/node#63154), Firefox DevTools, and the TC39 reference polyfill all label for the same reason. Replicating the ZonedDateTime recipe: exporting JSC's file-static temporalZonedDateTimeToString would be a WebKit-side change and version bump, so it cannot land in this PR. The replica is small (offset rounding plus two annotations) and the tests pin it to the engine: the Monrovia and Kolkata cases assert Bun.inspect(v) is byte-identical to v.toString(), including the half-minute tie, so any drift between the copy and JSC fails the suite. If the helper gets exported upstream later, the replica reduces to one call. |
… wrapper Move Bun__JSValue__temporalObjectType / Bun__Temporal__toDisplayString into Temporal.cpp next to the formatter, and have the C++ side own the "Temporal.<Class>" label so the discriminant->label table isn't repeated in Rust and JS. Bun__Temporal__toDisplayString now classifies internally and writes (label, text) instead of trusting a caller-supplied discriminant into uncheckedDowncast. Rust callers go through JSValue::is_temporal() / temporal_display_string() instead of each carrying an unsafe out-pointer block; drop the temporal_class_label re-export. deepEquals' private isTemporalObject reuses the exported classifier. No-Verification-Needed: refactor of an unmerged PR; CI runs the existing Temporal inspect suites
…l-inspect # Conflicts: # src/js/internal/util/inspect.js
![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 `@src/js/internal/util/inspect.js`:
- Around line 1641-1647: Update formatRaw to call getTemporalLabel(value) before
reading value[SymbolToStringTag], and branch on that result first; for Temporal
values, keep tag empty and use the native temporal label through the existing
effectiveConstructor/prefix flow. Preserve the non-Temporal tag handling, and
add a regression test covering a Temporal instance with a throwing own
Symbol.toStringTag getter.
🪄 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: fd34065e-3457-437a-b82f-fa9bccdb4f51
📒 Files selected for processing (8)
src/js/internal/util/inspect.jssrc/jsc/ConsoleObject.rssrc/jsc/JSValue.rssrc/jsc/bindings/Temporal.cppsrc/jsc/bindings/Temporal.hsrc/jsc/bindings/bindings.cppsrc/runtime/test_runner/pretty_format.rstest/js/bun/util/inspect.test.js
|
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. |
No-Verification-Needed: comment-only change
|
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. |
…forwarder No-Verification-Needed: mechanical extraction; CI runs the Temporal inspect suites
Bun::TemporalType (enum class : uint8_t) replaces the bare 0-8 discriminant; cppbind maps it to bun_jsc::TemporalType so JSValue::temporal_type() returns the enum directly. No-Verification-Needed: mechanical enum translation; CI runs the Temporal inspect suites
The shared formatter Tag now classifies Temporal cells as their own tag,
so a macro returning one fell into Macro.rs's wildcard, which errors.
That replaces the old useless {} coercion and matches Date, whose JSON
coercion also fails; make it an explicit arm, and since Temporal
ClassInfo names are the generic "Object", name the type via its label:
"cannot coerce Temporal.PlainDate to Bun's AST" instead of
"cannot coerce Object (JSType(33))".
|
Updated 6:09 AM PT - Aug 8th, 2026
❌ @dylan-conway, your commit 6f38c05 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 37043That installs a local version of the PR into your bun-37043 --bun |
…String Bump WebKit to 78d45d318434 (oven-sh/WebKit#393), which exposes TemporalZonedDateTime::toString(JSGlobalObject*) and a JSC::TemporalType / temporalType(JSValue) classifier. Drop Bun's copies: the ZonedDateTime toString recipe, Bun::TemporalType, and Bun::temporalObjectType. cppbind maps JSC::TemporalType to bun_jsc::TemporalType. Also picks up oven-sh/WebKit#391 (module loader: propagate TerminationException from resolve()).
…l-inspect # Conflicts: # test/js/bun/util/inspect.test.js
No-Verification-Needed: comment + test-only change
…acro] call) No-Verification-Needed: test-only change
There was a problem hiding this comment.
I reviewed this PR and the bug-hunting pass found no issues; all three findings from earlier runs (Macro.rs T::Temporal arm, stdout drain, debug-lane stdout assertion) are addressed in edf96b1. Because this bumps WEBKIT_VERSION and adds new C++ FFI (uncheckedDowncast over 8 JSC Temporal types, JSC::temporalType), a human look is still worthwhile.
What was reviewed:
Temporal.cppexception handling —RETURN_IF_EXCEPTIONafter both throwingtoStringpaths (ZonedDateTime/Duration), host functions guardTemporalType::NonebeforeasCell().- Shared
formatter::Tagconsumers —Macro.rsandpretty_format.rsboth handle the newTemporalvariant; no other unguarded matches found. inspect.js—getTemporalLabel/getTemporalDisplayStringbound via$newCppFunctionat module load, not through user-overridable machinery; ordering vs.Symbol.toStringTagmatches Node.- Tests pin
Bun.inspect(zdt)byte-equal tozdt.toString()for sub-minute-offset zones, so drift between the WebKit-side helper and inspection would fail the suite.
Extended reasoning...
Overview
This PR adds atomic Temporal.<Class> <toString()> formatting to all three of Bun's formatters (console.log/Bun.inspect via ConsoleObject.rs, test-runner pretty-format, and Node-compat util.inspect). It touches 17 files: a new C++ binding pair (Temporal.cpp/h) exposing JSC::temporalType classification and per-type toString() dispatch; a Rust TemporalType enum + JSValue::temporal_display_string wrapper; a new Tag::Temporal variant threaded through the shared classifier and both formatters; a T::Temporal arm in Macro.rs (added after an earlier review caught the wildcard fallthrough); inspect.js following nodejs/node#63154; a one-line cppbind.ts type mapping; a WEBKIT_VERSION bump; and test coverage across inspect, snapshot, deep-equals failure messages, node-inspect parity, and macro coercion.
Security risks
None identified. The classifier reads ClassInfo (never user-observable toString/Symbol.toStringTag), uncheckedDowncast is guarded by the temporalType() result on the same cell, and the two host functions exposed to internal JS return undefined on non-Temporal input before touching asCell(). No untrusted-input parsing, no allocation sized from external data.
Level of scrutiny
Medium-high. The formatting logic itself is well-contained and mirrors the existing JSDate path, but two aspects push this beyond auto-approval: (1) the WEBKIT_VERSION bump — even a small fork change to export JSC::temporalType/TemporalZonedDateTime::toString is a JS-engine dependency change that a maintainer should confirm is the intended scope; (2) the shared formatter::Tag::get() classifier is consumed outside the console (Macro.rs), and while that consumer is now handled, adding a variant to a shared discriminant is exactly the class of change REVIEW.md flags for careful auditing. The PR also embeds a user-visible format decision (labeled output, own-properties ignored in Bun.inspect) that the author justified against Deno/Firefox/Node precedent but which is still a UX call.
Other factors
Test coverage is thorough (all 8 types, annotations, historic sub-minute offsets pinned to engine toString(), nesting, colors, subclass/null-prototype via the Node parity suite, macro coercion). All prior review threads — CodeRabbit clippy nits, comment-cop, my three findings on Macro.rs and the macro test — are resolved and reflected in the current diff. The PR description is slightly stale (still describes replicating the ZonedDateTime recipe, which commit 867820a replaced with a direct toString(globalObject) call after the WebKit bump), but the code is consistent.
…l-inspect # Conflicts: # scripts/build/deps/webkit.ts
There was a problem hiding this comment.
All three of my earlier findings (the Macro.rs T::Temporal consumer, the undrained stdout pipe, and the debug-lane expect(stdout).toBe("") assertion) are now addressed and this pass found nothing new. Given the WebKit version bump plus new C++/Rust FFI surface and the user-facing output-format choice, a human sign-off still makes sense.
Checked: JSC::temporalType classification rejects prototypes/Temporal.Now so uncheckedDowncast in temporalDisplayString is guarded; Bun__Temporal__toDisplayString has RETURN_IF_EXCEPTION before writing out-params and the Rust side zero-inits both BunStrings. The new Tag::Temporal variant is now handled at every match site (ConsoleObject, pretty_format Tag::get/print_as/dispatch/ConsoleFormatter bridge, Macro.rs). The macro-test spawn now drains stdout without asserting on it, so the debug [macro] call when line no longer breaks the debug lane.
Extended reasoning...
Overview
This PR adds one-token formatting for the eight Temporal classes across console.log/Bun.inspect (src/jsc/ConsoleObject.rs), the test runner's pretty-format (src/runtime/test_runner/pretty_format.rs), and util.inspect (src/js/internal/util/inspect.js). The shared native helper lives in new src/jsc/bindings/Temporal.{h,cpp} with two [[ZIG_EXPORT]] externs and two host functions, wired through src/jsc/JSValue.rs (temporal_type/temporal_display_string) and a one-line cppbind.ts type-map entry. bindings.cpp drops its local isTemporalObject in favor of JSC::temporalType. The shared formatter Tag enum grows a Temporal variant, and src/js_parser_jsc/Macro.rs handles it explicitly. scripts/build/deps/webkit.ts bumps the pinned WebKit hash (presumably to pick up the exported TemporalZonedDateTime::toString(global) and JSC::temporalType used by Temporal.cpp). Tests span inspect.test.js, a ported Node parallel test, snapshot tests, deep-equals-temporal.test.ts, and a macro coercion spawn test.
Security risks
None identified. The classifier reads ClassInfo (no user-observable getters/toString), the display-string helpers call internal toString() implementations on already-type-checked cells, and both throwing paths (ZonedDateTime/Duration) are covered by RETURN_IF_EXCEPTION under a ThrowScope. The Rust wrapper zero-inits both out-BunStrings before the FFI call, so an exception path leaves them empty rather than uninitialized. util/inspect.js uses `` host functions captured at module load, so userland cannot tamper with the classification.
Level of scrutiny
High. This change bumps the vendored WebKit commit (a cross-cutting dependency change), adds new C++↔Rust FFI surface, extends a shared value-classifier enum consumed by multiple subsystems (one consumer was missed in the first revision and caught in review), and fixes a user-visible output format that is effectively an API decision (labeled vs. bare, matching Node's pending PR #63154). None of that is mechanical.
Other factors
Three prior automated findings on this PR were all fixed (Macro.rs consumer, subprocess pipe drain, debug-lane stdout assertion), and a maintainer (dylan-conway) has been actively iterating. Test coverage is thorough — all eight types, annotations, sub-minute historic offsets pinned to the engine's own toString(), nesting/depth/colors, subclass/own-property handling, Node-parity byte-for-byte, snapshot serialization, and toEqual failure messages. The WebKit bump and the output-format design are the pieces that most benefit from a human maintainer's explicit sign-off; the code itself looks correct.
2 participants
What
Temporal values now print as a single token,
Temporal.<Class> <toString()>, in all three formatters:Before this change, Temporal cells were classified as generic objects everywhere:
console.log/Bun.inspectdumped prototype methods and slot getters as if they were own properties (33 lines for aPlainDate, 50 for aZonedDateTime)util.inspectprintedPlainDate [Temporal.PlainDate] {}, with the value invisiblePlainDate {}, so every instance of a class produced the same snapshot text, and a failingtoEqualdiffed two identical sidesHow
The text is the spec
toString()with default options: lossless, round-trips throughTemporal.X.from(), keeps[Zone]/[u-ca=]annotations. It is computed from internal slots (ClassInfo detection, internaltoStringimplementations), so inspection never calls user-observabletoStringorSymbol.toStringTag.src/jsc/bindings/bindings.cpp+ newsrc/jsc/bindings/Temporal.{h,cpp}: one shared native helper for all 8 types.Bun__JSValue__temporalObjectTypeclassifies a value (0 = not Temporal, 1-8 = type),Bun__Temporal__toDisplayString/Bun::temporalDisplayStringproduce the text. Seven types use the classes' internaltoString();ZonedDateTimereplicates the spec recipe (temporalZonedDateTimeToStringis file-static in JSC) fromgetLocalDateTime, the minute-rounded offset, the zone id, and the calendar annotation. Two host functions expose the helper to internal JS.src/jsc/ConsoleObject.rs: aTemporaltag next to theJSDate -> JSONpath. Atomic at any depth, own properties and subclass names ignored (the existing Date/Map behavior), label uncolored, value in Date's magenta.src/runtime/test_runner/pretty_format.rs: same tag and the same text, so expect diffs and snapshots show the value. The format arm is hoisted to#[inline(never)]like the console arms so deep recursion frames stay small.src/js/internal/util/inspect.js: follows inspect: add Temporal support nodejs/node#63154 (the open Node PR for the same feature):getPrefixhandling for subclasses and null prototypes,'date'style, extra own properties appended. One adaptation: JSC names the constructorsPlainDatewhere V8 usesTemporal.PlainDate, so direct instances map onto the full label to produce the same output as Node (Temporal.PlainDate 2020-01-02, subclassX [Temporal.PlainDate] 2020-01-02).Temporal.Now, the prototypes, and Intl objects are unaffected (the classifier rejects them).Verification
test/js/bun/util/inspect.test.js: all 8 types, annotations, nesting, depth, own-property/subclass behavior, colors,console.logvia spawntest/js/node/util/node-inspect-tests/parallel/util-inspect-temporal.test.js: port of the test from inspect: add Temporal support nodejs/node#63154; all 48 cases produce byte-identical output to Node's expectationstest/js/bun/bun-object/deep-equals-temporal.test.ts:toEqualfailure messages now showExpected: Temporal.PlainDate 1999-12-31/Received: Temporal.PlainDate 2020-01-02test/js/bun/test/snapshot-tests/bun-snapshots.test.ts: snapshot serialization for all 8 types (committed.snap)All new tests fail on the released bun and pass with this change. Also verified against the engine itself: for historic sub-minute offsets (
Africa/Monrovia1950), extreme years, and non-ISO calendars, the inspected text is byte-identical to the realtoString(). Existing suites (inspect.test.js,expect.test.js417 tests, console, snapshot, util,deep-equals-temporal) pass.Note for #37018
#37018 (TOML Temporal) adds the same
Bun__JSValue__temporalObjectTypeclassifier (same name, same discriminants, same location in bindings.cpp). Whichever lands second will hit a small merge conflict there; resolve by keeping one copy of the classifier. The TOML-specific formatter in that PR stays separate since TOML needs annotation-free output.no test proof · iteration 0 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/bun/util/inspect.test.js