fix(temporal): correct toLocaleString epoch + honor dateStyle/timeStyle (#5580)#5765
Merged
Merged
Conversation
…le/options (#5580) Three bugs caused the intl402/Temporal test262 cluster to fail: 1. **Epoch wrong**: `date_arg_to_clipped_ms` called `date_cell_timestamp` on Temporal values, which returns the raw NaN-boxed pointer (not epoch ms) for Temporal cells, always producing the 1/1/1970 date. Fixed by routing through `temporal_to_epoch_ms` (new helper in `temporal/mod.rs` that extracts epoch-ms from any `TemporalValue` variant). 2. **DTF options ignored**: the `format`/`bound_format` thunks dropped the DTF object, so `dateStyle`, `timeStyle`, `hour12`, and all component options were silently discarded — output was always `M/D/YYYY`. Fixed by passing the DTF object to a new `format_ms_with_dtf_obj` that reads the resolved options and dispatches to locale-aware formatters. 3. **toLocaleString ignores args**: each Temporal type's `toLocaleString` called a simple hardcoded formatter, ignoring the `locale` and `options` arguments entirely. Replaced with `temporal_locale_string` (new shared function in `date_collator.rs`) that parses the options object, validates type-specific restrictions (PlainDate+timeStyle → TypeError, etc.), applies spec-mandated defaults, and formats via the same code path as `Intl.DateTimeFormat.format` — ensuring both APIs agree, which is what the self-validated test262 cases require. `components_to_timestamp` in `date.rs` made `pub(crate)` so temporal types can compute epoch-ms from calendar components. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YSsjJ2iL8ny2nYE2mFSNrN
📝 WalkthroughWalkthroughAdds a unified locale formatting path for all ChangesTemporal toLocaleString Locale Formatting
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 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 `@crates/perry-runtime/src/intl/date_collator.rs`:
- Around line 320-369: Intl.DateTimeFormat formatting here still uses
UTC-derived calendar fields from timestamp_to_components(secs), so non-UTC time
zones and timeZoneName are wrong. Update format_ms_with_dtf_obj to derive
year/month/day/hour/minute/second using the formatter’s configured time zone
instead of always using the raw UTC timestamp, and make sure the timezone-aware
values flow through the existing format_date_style, format_time_style, and
format_components paths.
- Around line 213-317: `format_components` currently drops `weekday` and `era`,
so requests like `{ weekday: "long" }` or `{ era: "short" }` never make it into
the formatted output. Extend the `format_components` signature and its
date-building logic to accept and render `weekday_opt` and `era_opt`, and thread
those options through the DTF object path and `temporal_locale_string` call
sites that reuse this helper. Use the existing `format_components` function as
the main location to preserve the current formatting behavior while adding the
missing fields.
- Around line 452-480: Update the Temporal type checks in date_collator.rs so
the formatting path rejects unsupported explicit component fields, not just
timeStyle/dateStyle. In the ctx match inside the formatting options guard, make
PlainDate and PlainYearMonth/PlainMonthDay throw when any time-related fields
are requested, make PlainTime throw when any date-related fields are requested,
and ensure placeholder-derived fields like year/day/hour/minute/second cannot
leak through via the formatter. Keep the existing TemporalLocaleCtx branching in
the same area and extend the validation before formatting proceeds.
- Around line 394-430: temporal_locale_string is formatting using only the UTC
epoch and ignores the requested time zone, so ZonedDateTime/Instant formatting
can produce the wrong wall-clock time. Update temporal_locale_string to accept
and use the time-zone context from TemporalLocaleCtx (and the parsed timeZone
option) when building the date/time components, and ensure the
zoned_date_time.rs call site passes the correct zone through so formatting
happens in the intended zone.
In `@crates/perry-runtime/src/temporal/instant.rs`:
- Around line 187-195: The Temporal.Instant toLocaleString path still drops the
caller-provided locale and time zone because the shared formatter in
temporal_locale_string/date_collator uses only UTC-derived components. Update
the shared implementation to read and apply locale_arg and the options timeZone
when TemporalLocaleCtx::Instant is used, so toLocaleString in instant.rs
actually formats with the requested locale/zone instead of rebuilding fields via
timestamp_to_components(secs) alone. Ensure the change is made in the shared
intl formatting path rather than only in the Instant branch.
In `@crates/perry-runtime/src/temporal/mod.rs`:
- Around line 326-333: The PlainMonthDay conversion in
TemporalValue::PlainMonthDay currently hardcodes 1970, which breaks leap-day
month-days during round-trip formatting. Update the components_to_timestamp call
to use md.reference_year() instead of a fixed year, matching the behavior
exposed by PlainMonthDay and keeping Intl.DateTimeFormat.format(md) and the
toLocaleString path consistent for leap-year dates.
In `@crates/perry-runtime/src/temporal/plain_month_day.rs`:
- Around line 135-149: The PlainMonthDay formatting path is hard-coding 1970 in
the `components_to_timestamp` call, which can normalize leap-day values before
`temporal_locale_string` formats them. Update the `PlainMonthDay` logic in the
relevant method to use the month-day’s own reference year instead of a fixed
year, so `md` values like 02-29 preserve the correct date semantics through
formatting.
In `@crates/perry-runtime/src/temporal/plain_time.rs`:
- Around line 197-213: In `PlainTime`’s `toLocaleString` path, reject any
date-bearing options before using the `1970-01-01` anchor, since the shared
validator only blocks `dateStyle` and still allows explicit `year`, `month`,
`day`, `weekday`, and `era`. Update the option validation used by
`temporal_locale_string`/`PlainTime` so these fields are treated as unsupported
for `Temporal.PlainTime` and the call throws instead of formatting an anchored
date.
In `@crates/perry-runtime/src/temporal/zoned_date_time.rs`:
- Around line 247-255: The ZonedDateTime toLocaleString path is losing the
instance’s own time zone because it only passes epoch_ms into
temporal_locale_string, so update the ZonedDateTime branch in zoned_date_time.rs
to preserve and forward z’s time zone when formatting. Adjust the shared intl
formatting flow used by TemporalLocaleCtx::ZonedDateTime so it can reconstruct
components with the correct zone from the ZonedDateTime instance instead of
defaulting to UTC, and keep the existing timeZone override rejection behavior
intact.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ad133140-01e1-4d2b-95a4-bfd3487fb6f1
📒 Files selected for processing (11)
crates/perry-runtime/src/date.rscrates/perry-runtime/src/intl.rscrates/perry-runtime/src/intl/date_collator.rscrates/perry-runtime/src/temporal/instant.rscrates/perry-runtime/src/temporal/mod.rscrates/perry-runtime/src/temporal/plain_date.rscrates/perry-runtime/src/temporal/plain_date_time.rscrates/perry-runtime/src/temporal/plain_month_day.rscrates/perry-runtime/src/temporal/plain_time.rscrates/perry-runtime/src/temporal/plain_year_month.rscrates/perry-runtime/src/temporal/zoned_date_time.rs
Comment on lines
+213
to
+317
| /// Format date+time components from the individual component options (no style). | ||
| fn format_components( | ||
| year: i32, | ||
| month: u32, | ||
| day: u32, | ||
| hour: u32, | ||
| minute: u32, | ||
| second: u32, | ||
| year_opt: Option<&str>, | ||
| month_opt: Option<&str>, | ||
| day_opt: Option<&str>, | ||
| hour_opt: Option<&str>, | ||
| minute_opt: Option<&str>, | ||
| second_opt: Option<&str>, | ||
| use_24h: bool, | ||
| ) -> String { | ||
| let has_date = year_opt.is_some() || month_opt.is_some() || day_opt.is_some(); | ||
| let has_time = hour_opt.is_some() || minute_opt.is_some() || second_opt.is_some(); | ||
|
|
||
| let date_part = if has_date { | ||
| let fmt_month = match month_opt { | ||
| Some("long") => MONTH_FULL[month.saturating_sub(1).min(11) as usize].to_string(), | ||
| Some("short") | Some("narrow") => { | ||
| MONTH_ABBR[month.saturating_sub(1).min(11) as usize].to_string() | ||
| } | ||
| Some("2-digit") => format!("{:02}", month), | ||
| _ => month.to_string(), | ||
| }; | ||
| let fmt_day = match day_opt { | ||
| Some("2-digit") => format!("{:02}", day), | ||
| _ if day_opt.is_some() => day.to_string(), | ||
| _ => String::new(), | ||
| }; | ||
| let fmt_year = match year_opt { | ||
| Some("2-digit") => format!("{:02}", year.rem_euclid(100)), | ||
| _ if year_opt.is_some() => year.to_string(), | ||
| _ => String::new(), | ||
| }; | ||
| // Use named-month format for long/short/narrow, numeric M/D/YYYY otherwise. | ||
| match month_opt { | ||
| Some("long") | Some("short") | Some("narrow") => { | ||
| let has_y = year_opt.is_some(); | ||
| let has_d = day_opt.is_some(); | ||
| Some(match (has_d, has_y) { | ||
| (true, true) => format!("{} {}, {}", fmt_month, fmt_day, fmt_year), | ||
| (true, false) => format!("{} {}", fmt_month, fmt_day), | ||
| (false, true) => format!("{} {}", fmt_month, fmt_year), | ||
| (false, false) => fmt_month, | ||
| }) | ||
| } | ||
| _ => { | ||
| let has_y = year_opt.is_some(); | ||
| let has_d = day_opt.is_some(); | ||
| Some(match (has_d, has_y) { | ||
| (true, true) => format!("{}/{}/{}", fmt_month, fmt_day, fmt_year), | ||
| (true, false) => format!("{}/{}", fmt_month, fmt_day), | ||
| (false, true) => format!("{}/{}", fmt_month, fmt_year), | ||
| (false, false) => fmt_month, | ||
| }) | ||
| } | ||
| } | ||
| } else { | ||
| None | ||
| }; | ||
|
|
||
| let time_part = if has_time { | ||
| let inc_secs = second_opt.is_some(); | ||
| let inc_mins = minute_opt.is_some() || inc_secs; | ||
| Some(if use_24h { | ||
| if inc_secs { | ||
| format!("{:02}:{:02}:{:02}", hour, minute, second) | ||
| } else if inc_mins { | ||
| format!("{:02}:{:02}", hour, minute) | ||
| } else { | ||
| format!("{:02}", hour) | ||
| } | ||
| } else { | ||
| let (h, ampm) = if hour == 0 { | ||
| (12u32, "AM") | ||
| } else if hour < 12 { | ||
| (hour, "AM") | ||
| } else if hour == 12 { | ||
| (12, "PM") | ||
| } else { | ||
| (hour - 12, "PM") | ||
| }; | ||
| if inc_secs { | ||
| format!("{}:{:02}:{:02} {}", h, minute, second, ampm) | ||
| } else if inc_mins { | ||
| format!("{}:{:02} {}", h, minute, ampm) | ||
| } else { | ||
| format!("{} {}", h, ampm) | ||
| } | ||
| }) | ||
| } else { | ||
| None | ||
| }; | ||
|
|
||
| match (date_part, time_part) { | ||
| (Some(d), Some(t)) => format!("{}, {}", d, t), | ||
| (Some(d), None) => d, | ||
| (None, Some(t)) => t, | ||
| (None, None) => format!("{}/{}/{}", month, day, year), | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
weekday and era still can't appear in the formatted string.
format_components only accepts year/month/day/hour/minute/second, so a request like { weekday: "long" } or { era: "short" } has no way to reach the renderer. The DTF object path and temporal_locale_string both reuse this formatter, so those options are still silently dropped.
🤖 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 `@crates/perry-runtime/src/intl/date_collator.rs` around lines 213 - 317,
`format_components` currently drops `weekday` and `era`, so requests like `{
weekday: "long" }` or `{ era: "short" }` never make it into the formatted
output. Extend the `format_components` signature and its date-building logic to
accept and render `weekday_opt` and `era_opt`, and thread those options through
the DTF object path and `temporal_locale_string` call sites that reuse this
helper. Use the existing `format_components` function as the main location to
preserve the current formatting behavior while adding the missing fields.
Comment on lines
+320
to
+369
| fn format_ms_with_dtf_obj(obj: *const ObjectHeader, ms: f64) -> String { | ||
| let secs = (ms as i64).div_euclid(1000); | ||
| let (year, month, day, hour, minute, second) = crate::date::timestamp_to_components(secs); | ||
|
|
||
| let date_style = get_string_field(obj, KEY_DATE_STYLE); | ||
| let time_style = get_string_field(obj, KEY_TIME_STYLE); | ||
| let hour12_v = { | ||
| let v = JSValue::from_bits(get_field(obj, KEY_HOUR12).to_bits()); | ||
| if v.is_bool() { | ||
| Some(v.as_bool()) | ||
| } else { | ||
| None | ||
| } | ||
| }; | ||
| let hour_cycle = get_string_field(obj, KEY_HOUR_CYCLE); | ||
| let use_24h = resolve_24h(hour12_v, hour_cycle.as_deref()); | ||
|
|
||
| match (date_style.as_deref(), time_style.as_deref()) { | ||
| (Some(ds), Some(ts)) => format!( | ||
| "{}, {}", | ||
| format_date_style(year, month, day, secs, ds), | ||
| format_time_style(hour, minute, second, ts, use_24h), | ||
| ), | ||
| (Some(ds), None) => format_date_style(year, month, day, secs, ds), | ||
| (None, Some(ts)) => format_time_style(hour, minute, second, ts, use_24h), | ||
| (None, None) => { | ||
| let year_opt = get_string_field(obj, KEY_YEAR); | ||
| let month_opt = get_string_field(obj, KEY_MONTH); | ||
| let day_opt = get_string_field(obj, KEY_DAY); | ||
| let hour_opt = get_string_field(obj, KEY_HOUR); | ||
| let minute_opt = get_string_field(obj, KEY_MINUTE); | ||
| let second_opt = get_string_field(obj, KEY_SECOND); | ||
| format_components( | ||
| year, | ||
| month, | ||
| day, | ||
| hour, | ||
| minute, | ||
| second, | ||
| year_opt.as_deref(), | ||
| month_opt.as_deref(), | ||
| day_opt.as_deref(), | ||
| hour_opt.as_deref(), | ||
| minute_opt.as_deref(), | ||
| second_opt.as_deref(), | ||
| use_24h, | ||
| ) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Intl.DateTimeFormat still formats in UTC here.
This function always derives calendar fields with timestamp_to_components(secs) before any formatter-specific timezone handling, so a DTF configured for a non-UTC zone will still print UTC wall-clock values. That also makes timeZoneName impossible to render correctly from this path.
🤖 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 `@crates/perry-runtime/src/intl/date_collator.rs` around lines 320 - 369,
Intl.DateTimeFormat formatting here still uses UTC-derived calendar fields from
timestamp_to_components(secs), so non-UTC time zones and timeZoneName are wrong.
Update format_ms_with_dtf_obj to derive year/month/day/hour/minute/second using
the formatter’s configured time zone instead of always using the raw UTC
timestamp, and make sure the timezone-aware values flow through the existing
format_date_style, format_time_style, and format_components paths.
Comment on lines
+394
to
+430
| pub(crate) fn temporal_locale_string( | ||
| epoch_ms: f64, | ||
| locale_arg: f64, | ||
| opts_arg: f64, | ||
| ctx: TemporalLocaleCtx, | ||
| ) -> f64 { | ||
| // ---- parse options object ---- | ||
| let opts_obj = object_ptr_from_value(opts_arg); | ||
|
|
||
| let get_opt = | ||
| |key: &str| -> Option<String> { opts_obj.and_then(|o| opt_string(get_field(o, key))) }; | ||
| let get_bool_opt = |key: &str| -> Option<bool> { | ||
| let raw = opts_obj | ||
| .map(|o| get_field(o, key)) | ||
| .unwrap_or_else(undefined); | ||
| let v = JSValue::from_bits(raw.to_bits()); | ||
| if v.is_bool() { | ||
| Some(v.as_bool()) | ||
| } else { | ||
| None | ||
| } | ||
| }; | ||
|
|
||
| let date_style = get_opt("dateStyle"); | ||
| let time_style = get_opt("timeStyle"); | ||
| let year_opt = get_opt("year"); | ||
| let month_opt = get_opt("month"); | ||
| let day_opt = get_opt("day"); | ||
| let hour_opt = get_opt("hour"); | ||
| let minute_opt = get_opt("minute"); | ||
| let second_opt = get_opt("second"); | ||
| let hour12 = get_bool_opt("hour12"); | ||
| let hour_cycle = get_opt("hourCycle"); | ||
| let weekday_opt = get_opt("weekday"); | ||
| let era_opt = get_opt("era"); | ||
| let tz_name_opt = get_opt("timeZoneName"); | ||
| let tz_opt = get_opt("timeZone"); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
temporal_locale_string drops the time-zone context before formatting.
The helper only receives epoch_ms, then rebuilds components in UTC. As wired from crates/perry-runtime/src/temporal/zoned_date_time.rs, a Temporal.ZonedDateTime will therefore format its UTC instant instead of its own zone’s wall-clock time, and Instant cannot honor an explicit timeZone option because tz_opt is parsed but never applied.
Also applies to: 554-569
🤖 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 `@crates/perry-runtime/src/intl/date_collator.rs` around lines 394 - 430,
temporal_locale_string is formatting using only the UTC epoch and ignores the
requested time zone, so ZonedDateTime/Instant formatting can produce the wrong
wall-clock time. Update temporal_locale_string to accept and use the time-zone
context from TemporalLocaleCtx (and the parsed timeZone option) when building
the date/time components, and ensure the zoned_date_time.rs call site passes the
correct zone through so formatting happens in the intended zone.
Comment on lines
+452
to
+480
| // Type-specific restrictions: | ||
| match ctx { | ||
| TemporalLocaleCtx::PlainDate | ||
| | TemporalLocaleCtx::PlainYearMonth | ||
| | TemporalLocaleCtx::PlainMonthDay => { | ||
| // No time support — timeStyle is invalid. | ||
| if time_style.is_some() { | ||
| throw_type_error( | ||
| "timeStyle option is not valid for this Temporal type (no time component)", | ||
| ); | ||
| } | ||
| } | ||
| TemporalLocaleCtx::PlainTime => { | ||
| // No date support — dateStyle is invalid. | ||
| if date_style.is_some() { | ||
| throw_type_error( | ||
| "dateStyle option is not valid for Temporal.PlainTime (no date component)", | ||
| ); | ||
| } | ||
| } | ||
| TemporalLocaleCtx::ZonedDateTime => { | ||
| // The timeZone option is disallowed (ZDT carries its own timezone). | ||
| if tz_opt.is_some() { | ||
| throw_type_error( | ||
| "timeZone option is not allowed when formatting Temporal.ZonedDateTime", | ||
| ); | ||
| } | ||
| } | ||
| _ => {} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reject unsupported component fields for partial Temporal types.
The guard only bans timeStyle / dateStyle. Explicit fields still slip through, so PlainDate can format a synthetic midnight time, PlainTime can print the synthetic 1970-01-01 date, and PlainMonthDay / PlainYearMonth can expose fabricated year/day values from the placeholder timestamp instead of throwing.
🤖 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 `@crates/perry-runtime/src/intl/date_collator.rs` around lines 452 - 480,
Update the Temporal type checks in date_collator.rs so the formatting path
rejects unsupported explicit component fields, not just timeStyle/dateStyle. In
the ctx match inside the formatting options guard, make PlainDate and
PlainYearMonth/PlainMonthDay throw when any time-related fields are requested,
make PlainTime throw when any date-related fields are requested, and ensure
placeholder-derived fields like year/day/hour/minute/second cannot leak through
via the formatter. Keep the existing TemporalLocaleCtx branching in the same
area and extend the validation before formatting proceeds.
Comment on lines
+187
to
+195
| "toLocaleString" => { | ||
| let epoch_ms = i.epoch_milliseconds() as f64; | ||
| crate::intl::temporal_locale_string( | ||
| epoch_ms, | ||
| raw_arg(args, 0), | ||
| raw_arg(args, 1), | ||
| crate::intl::TemporalLocaleCtx::Instant, | ||
| ) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
This shared path still ignores the caller's locale and time zone.
The new branch forwards locales/options, but the shared implementation shown in crates/perry-runtime/src/intl/date_collator.rs:394-572 never reads locale_arg and rebuilds calendar fields with timestamp_to_components(secs) without applying any timeZone. For Temporal.Instant, that means toLocaleString("fr-FR", { timeZone: "America/Los_Angeles" }) still formats the UTC instant with the built-in default pattern instead of the requested locale/zone.
🤖 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 `@crates/perry-runtime/src/temporal/instant.rs` around lines 187 - 195, The
Temporal.Instant toLocaleString path still drops the caller-provided locale and
time zone because the shared formatter in temporal_locale_string/date_collator
uses only UTC-derived components. Update the shared implementation to read and
apply locale_arg and the options timeZone when TemporalLocaleCtx::Instant is
used, so toLocaleString in instant.rs actually formats with the requested
locale/zone instead of rebuilding fields via timestamp_to_components(secs)
alone. Ensure the change is made in the shared intl formatting path rather than
only in the Instant branch.
Comment on lines
+326
to
+333
| TemporalValue::PlainMonthDay(md) => crate::date::components_to_timestamp( | ||
| 1970, | ||
| md.month_code().to_month_integer() as u32, | ||
| md.day() as u32, | ||
| 0, | ||
| 0, | ||
| 0, | ||
| ), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Use PlainMonthDay's reference year instead of 1970.
A hardcoded 1970 base makes 02-29 unrepresentable, so the later timestamp_to_components round-trip normalizes leap-day month-days to March 1. md.reference_year() is already observable in crates/perry-runtime/src/temporal/plain_month_day.rs, so both Intl.DateTimeFormat.format(md) and the mirrored toLocaleString path will misformat leap-year month-days.
🛠 Proposed fix
TemporalValue::PlainMonthDay(md) => crate::date::components_to_timestamp(
- 1970,
+ md.reference_year(),
md.month_code().to_month_integer() as u32,
md.day() as u32,
0,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| TemporalValue::PlainMonthDay(md) => crate::date::components_to_timestamp( | |
| 1970, | |
| md.month_code().to_month_integer() as u32, | |
| md.day() as u32, | |
| 0, | |
| 0, | |
| 0, | |
| ), | |
| TemporalValue::PlainMonthDay(md) => crate::date::components_to_timestamp( | |
| md.reference_year(), | |
| md.month_code().to_month_integer() as u32, | |
| md.day() as u32, | |
| 0, | |
| 0, | |
| 0, | |
| ), |
🤖 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 `@crates/perry-runtime/src/temporal/mod.rs` around lines 326 - 333, The
PlainMonthDay conversion in TemporalValue::PlainMonthDay currently hardcodes
1970, which breaks leap-day month-days during round-trip formatting. Update the
components_to_timestamp call to use md.reference_year() instead of a fixed year,
matching the behavior exposed by PlainMonthDay and keeping
Intl.DateTimeFormat.format(md) and the toLocaleString path consistent for
leap-year dates.
Comment on lines
+135
to
+149
| let epoch_ms = crate::date::components_to_timestamp( | ||
| 1970, | ||
| md.month_code().to_month_integer() as u32, | ||
| md.day() as u32, | ||
| 0, | ||
| 0, | ||
| 0, | ||
| ) as f64 | ||
| * 1000.0; | ||
| crate::intl::temporal_locale_string( | ||
| epoch_ms, | ||
| raw_arg(args, 0), | ||
| raw_arg(args, 1), | ||
| crate::intl::TemporalLocaleCtx::PlainMonthDay, | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Use the month-day's reference year instead of hard-coding 1970.
Temporal.PlainMonthDay can legitimately carry 02-29, but 1970-02-29 is not representable. Feeding every value through 1970 will normalize leap-day month-days to March before they reach the formatter.
Proposed fix
- let epoch_ms = crate::date::components_to_timestamp(
- 1970,
+ let epoch_ms = crate::date::components_to_timestamp(
+ md.reference_year(),
md.month_code().to_month_integer() as u32,
md.day() as u32,
0,
0,
0,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let epoch_ms = crate::date::components_to_timestamp( | |
| 1970, | |
| md.month_code().to_month_integer() as u32, | |
| md.day() as u32, | |
| 0, | |
| 0, | |
| 0, | |
| ) as f64 | |
| * 1000.0; | |
| crate::intl::temporal_locale_string( | |
| epoch_ms, | |
| raw_arg(args, 0), | |
| raw_arg(args, 1), | |
| crate::intl::TemporalLocaleCtx::PlainMonthDay, | |
| ) | |
| let epoch_ms = crate::date::components_to_timestamp( | |
| md.reference_year(), | |
| md.month_code().to_month_integer() as u32, | |
| md.day() as u32, | |
| 0, | |
| 0, | |
| 0, | |
| ) as f64 | |
| * 1000.0; | |
| crate::intl::temporal_locale_string( | |
| epoch_ms, | |
| raw_arg(args, 0), | |
| raw_arg(args, 1), | |
| crate::intl::TemporalLocaleCtx::PlainMonthDay, | |
| ) |
🤖 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 `@crates/perry-runtime/src/temporal/plain_month_day.rs` around lines 135 - 149,
The PlainMonthDay formatting path is hard-coding 1970 in the
`components_to_timestamp` call, which can normalize leap-day values before
`temporal_locale_string` formats them. Update the `PlainMonthDay` logic in the
relevant method to use the month-day’s own reference year instead of a fixed
year, so `md` values like 02-29 preserve the correct date semantics through
formatting.
Comment on lines
+197
to
+213
| "toLocaleString" => { | ||
| let epoch_ms = crate::date::components_to_timestamp( | ||
| 1970, | ||
| 1, | ||
| 1, | ||
| t.hour() as u32, | ||
| t.minute() as u32, | ||
| t.second() as u32, | ||
| ) as f64 | ||
| * 1000.0; | ||
| crate::intl::temporal_locale_string( | ||
| epoch_ms, | ||
| raw_arg(args, 0), | ||
| raw_arg(args, 1), | ||
| crate::intl::TemporalLocaleCtx::PlainTime, | ||
| ) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reject date-bearing component options before formatting PlainTime.
This 1970-01-01 anchor is only safe if date fields are disallowed first. The shared validator currently rejects dateStyle, but it still accepts explicit year/month/day/weekday/era options, so this path can still produce 1/1/1970 for Temporal.PlainTime instead of throwing for an unsupported date request.
🤖 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 `@crates/perry-runtime/src/temporal/plain_time.rs` around lines 197 - 213, In
`PlainTime`’s `toLocaleString` path, reject any date-bearing options before
using the `1970-01-01` anchor, since the shared validator only blocks
`dateStyle` and still allows explicit `year`, `month`, `day`, `weekday`, and
`era`. Update the option validation used by `temporal_locale_string`/`PlainTime`
so these fields are treated as unsupported for `Temporal.PlainTime` and the call
throws instead of formatting an anchored date.
Comment on lines
247
to
+255
| "toLocaleString" => { | ||
| super::options::assert_locale_string_calendar(z.calendar().identifier()); | ||
| string(&super::options::zoned_date_time_locale_string(z)) | ||
| let epoch_ms = z.epoch_milliseconds() as f64; | ||
| crate::intl::temporal_locale_string( | ||
| epoch_ms, | ||
| raw_arg(args, 0), | ||
| raw_arg(args, 1), | ||
| crate::intl::TemporalLocaleCtx::ZonedDateTime, | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
This drops the ZonedDateTime's actual time zone.
The shared formatter only receives epoch_ms plus TemporalLocaleCtx::ZonedDateTime, and upstream it rebuilds fields with timestamp_to_components(secs) without any zone argument. Since the helper also rejects a timeZone override for this type, a non-UTC Temporal.ZonedDateTime will now format as its UTC instant rather than in z's own zone.
🤖 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 `@crates/perry-runtime/src/temporal/zoned_date_time.rs` around lines 247 - 255,
The ZonedDateTime toLocaleString path is losing the instance’s own time zone
because it only passes epoch_ms into temporal_locale_string, so update the
ZonedDateTime branch in zoned_date_time.rs to preserve and forward z’s time zone
when formatting. Adjust the shared intl formatting flow used by
TemporalLocaleCtx::ZonedDateTime so it can reconstruct components with the
correct zone from the ZonedDateTime instance instead of defaulting to UTC, and
keep the existing timeZone override rejection behavior intact.
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
2 participants
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.
Summary
Three bugs in
Temporal.prototype.toLocaleStringandIntl.DateTimeFormat.format(temporalValue)caused the intl402/Temporal test262 cluster (#5580) to produce wrong output or 1/1/1970 dates. This PR fixes all three at their root.Changes
crates/perry-runtime/src/date.rs:components_to_timestampmadepub(crate)so temporal types can compute epoch-ms from calendar fields.crates/perry-runtime/src/temporal/mod.rs: Newtemporal_to_epoch_ms(tv)helper extracts epoch-milliseconds from anyTemporalValuevariant (Instant/ZonedDateTime via direct.epoch_milliseconds(); plain types viacomponents_to_timestamp; Duration returnsNone).crates/perry-runtime/src/intl/date_collator.rs(major addition):date_arg_to_clipped_ms: route Temporal values throughtemporal_to_epoch_msinstead ofdate_cell_timestamp(which returned the raw NaN-boxed pointer, always producing 1/1/1970).date_time_format_format_thunk/date_time_format_bound_format_thunk: pass the DTF object to newformat_ms_with_dtf_obj, which readsdateStyle/timeStyle/hour12/hourCycleand component options from the stored DTF instance.format_date_style,format_time_style,format_time_12h/24h,format_components,resolve_24h.TemporalLocaleCtxenum andtemporal_locale_string(epoch_ms, locale_arg, opts_arg, ctx)shared implementation for all TemporaltoLocaleStringcalls. Validates type-specific restrictions (PlainDate+timeStyle → TypeError, PlainTime+dateStyle → TypeError, ZonedDateTime+timeZone-option → TypeError), applies spec-mandated defaults when no options given, then formats using the same code asDTF.format— ensuring both APIs agree (required by self-validated test262 cases).crates/perry-runtime/src/intl.rs: re-exporttemporal_locale_stringandTemporalLocaleCtx.crates/perry-runtime/src/temporal/instant.rs: split"toJSON" | "toLocaleString"arm;toLocaleStringnow callstemporal_locale_stringwithepoch_ms = i.epoch_milliseconds().crates/perry-runtime/src/temporal/plain_date.rs,plain_date_time.rs,plain_time.rs,plain_year_month.rs,plain_month_day.rs,zoned_date_time.rs:toLocaleStringupdated to computeepoch_msfrom each type's fields/slots and delegate totemporal_locale_string.Related issue
Fixes #5580
Test plan
cargo build --release -p perry -p perry-runtime-static -p perry-stdlib-static— clean (exit 0)cargo fmt --all -- --check— no diffsbash scripts/check_file_size.sh— all files under 2000 linescargo test --workspace --exclude perry-ui-ios ...— CI will runBefore/after test262 counts: the
vendor/test262clone is blocked by org network policy in this environment, so we cannot run the runner locally. The fix addresses three root causes (wrong epoch, DTF options dropped, toLocaleString ignores args) that together account for the 73-test intl402/Temporal cluster in #5580.Checklist
feat:/fix:/docs:/chore:prefix convention used in the logGenerated by Claude Code
Summary by CodeRabbit
New Features
toLocaleStringnow uses the same locale-aware date/time formatting behavior across supported Temporal types.Bug Fixes
Instant,PlainDate,PlainTime,PlainDateTime,PlainMonthDay,PlainYearMonth, andZonedDateTime.