[JSC] Expose TemporalZonedDateTime::toString and a temporalType(JSValue) classifier - #393
Conversation
…ue) classifier TemporalZonedDateTimeToString was file-static in the prototype file, so ZonedDateTime was the only Temporal class without a toString() an embedder could call; move it onto TemporalZonedDateTime as toString(globalObject, precision, roundingMode, showOffset, showTimeZone, showCalendar) plus an all-~auto~ toString(globalObject) overload, and have toString/toJSON call the member. Add JSC::TemporalType and temporalType(JSValue) in TemporalObject.h so callers that need to discriminate the eight Temporal classes share one classifier instead of each writing an inherits<> chain.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 26 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (5)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — mechanical refactor plus a new embedder-only classifier, no JS-observable change.
Verified the toString body is a verbatim move from the file-static in TemporalZonedDateTimePrototype.cpp (only zdt->x() → member/m_* access; the dropped calendarID param was always zdt->calendarID() at both call sites).
Confirmed the one-arg toString overload's defaults match what toJSON previously passed inline.
Checked that all eight Temporal classes use ObjectType structures, so the type() != ObjectType short-circuit in temporalType() is sound.
Checked the new TemporalObject.h include in TemporalZonedDateTime.h — no header cycle; PrecisionData/RoundingMode resolve from existing includes.
Extended reasoning...
Overview
Two additions for embedder (Bun) consumption:
TemporalZonedDateTime::toString— the file-statictemporalZonedDateTimeToStringinTemporalZonedDateTimePrototype.cppis relocated to a public member onTemporalZonedDateTime. The function body is moved verbatim; the only edits arezdt->exactTime()→m_exactTime,zdt->timeZone()→m_timeZone,zdt->timeZoneId()/calendarId()→ member calls, and thecalendarIDparameter is dropped in favor ofm_calendarID(both callers passedzdt->calendarID(), so this is behavior-preserving). A convenience one-arg overload supplies the same all-~auto~defaults thattoJSONused inline.toString/toJSONon the prototype now delegate to the member.TemporalTypeenum +temporalType(JSValue)— a newJS_EXPORT_PRIVATEclassifier that short-circuits on non-ObjectTypecells and then walks aninherits<>chain over the eight Temporal classes. Not wired into any existing JS path; purely additive for FFI consumers.
Supporting changes: two new includes in TemporalObject.cpp (TemporalDuration.h, TemporalInstant.h) for the classifier, and TemporalZonedDateTime.h now includes TemporalObject.h (for PrecisionData) plus Rounding.h/StringBuilder.h in the .cpp.
Security risks
None. No parsing, no untrusted input handling, no allocation-pattern change, no auth/permissions surface. The classifier reads only the JSCell type/classInfo chain; the toString body is byte-for-byte the pre-existing spec algorithm.
Level of scrutiny
Low. This is a code-motion refactor plus an additive, unreferenced-in-JSC helper. The prototype's toString/toJSON are already covered by test262 Temporal tests, so a regression in the move would surface there. I diffed the removed and added function bodies line-by-line and found no semantic drift.
Other factors
- Confirmed via grep that every Temporal instance class (
TemporalInstant,TemporalPlainDate,TemporalPlainDateTime,TemporalPlainTime,TemporalPlainYearMonth,TemporalPlainMonthDay,TemporalZonedDateTime,TemporalDuration) creates its structure withTypeInfo(ObjectType, ...), so the fast-reject ontype() != ObjectTypeintemporalType()cannot produce a false negative. - Verified no circular header include is introduced:
TemporalObject.hdoes not includeTemporalZonedDateTime.h. RoundingMode(fromTemporalEnums.h) andPrecisionData(fromTemporalObject.h) are both reachable from the updatedTemporalZonedDateTime.h.- No outstanding reviewer comments; the only timeline entry is a coderabbit rate-limit notice.
Preview Builds
|
…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()).
1 participant
Two small additions so embedders can format and classify Temporal values without copying engine internals.
TemporalZonedDateTime::toString—TemporalZonedDateTimeToStringwas file-static inTemporalZonedDateTimePrototype.cpp, which made ZonedDateTime the only Temporal class without atoString()reachable from outside the prototype. It is now a member:toString/toJSONon the prototype call the member; the body is moved verbatim (onlyzdt->x()→ member access and the redundantcalendarIDparameter dropped, since every caller passedzdt->calendarID()).JSC::TemporalType/temporalType(JSValue)inTemporalObject.h— auint8_tenum over the eight classes (None,Instant,PlainDateTime,PlainDate,PlainTime,ZonedDateTime,PlainYearMonth,PlainMonthDay,Duration) and a classifier that short-circuits on non-ObjectTypecells then does theinherits<>chain once. Discriminants are explicit because Bun mirrors them across its FFI boundary.No behavior change for JS. Consumer: oven-sh/bun#37043 (Temporal formatting in
console.log/util.inspect/ test snapshots), which currently replicates the ZonedDateTime recipe and the classifier on the Bun side.