fix(runtime/hir/intl): #5800 — fix 29 Temporal/Intl regressions from #5789/#5793/#5783 - #5806
Merged
Merged
Conversation
…5789/#5793/#5783 Cluster A (12 tests): `temporal_subclass_cell` crashed on Linux when processing Proxy ids because the old guard (`obj >= GC_HEADER_SIZE + 0x1000`) passes proxy ids (0xF0000–0xFFFFF) on Linux (HEAP_MIN=0x1000). Switch to `is_plausible_heap_addr` which unconditionally rejects the entire handle band [0, 0x100000). Cluster B (16 tests): `temporal_locale_string` defaults regression from #5789. Restore per-type ECMA-402 defaults: PlainDate→{y,m,d}, PlainDateTime/Instant/ZDT→{y,m,d,h,min,sec}, PlainTime→{h,min,sec}, PlainYearMonth→{y,m}, PlainMonthDay→{m,d}. Tests comparing `Date.toLocaleString(locale, opts)` with a Temporal type newly diverged because #5789 fixed Temporal args-dropping but left Date on the fold path. Install a real `Date.prototype.toLocaleString` thunk (`date_to_locale_string_opts`) that delegates to `temporal_locale_string` with `PlainDateTime` defaults. Remove the `recv_class == Some("Date")` shortcut from the HIR fold so Date calls with args also use the thunk. Cluster C (1 test): `NumberFormat({style:"currency",unit:"test"})` threw RangeError (from the stricter `is_well_formed_unit_identifier` added in #5783) before the TypeError for the missing `currency` field. Move the currency TypeError check ahead of the unit reads to restore spec order. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a Date.prototype.toLocaleString runtime thunk and installer, updates Temporal locale default field selection, tightens Date.toLocaleString HIR folding, moves currency validation earlier, and changes temporal subclass receiver validation. ChangesDate/Temporal locale and Intl fixes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/perry-runtime/src/object/field_get_set.rs (1)
75-79: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winUse the validated GC-header reader here.
is_plausible_heap_addrfixes proxy-id rejection, but this still manually dereferencesobj - GC_HEADER_SIZE. The canonicaltry_read_gc_headeralso rejects small-buffer slab addresses before reading the header, avoiding fake-header routing on heap-plausible non-GC allocations.Proposed fix
- if !crate::value::addr_class::is_plausible_heap_addr(obj) { + let Some(gc_header) = (unsafe { crate::value::addr_class::try_read_gc_header(obj) }) else { return None; - } - let gc_header = (obj as *const u8).sub(crate::gc::GC_HEADER_SIZE) as *const crate::gc::GcHeader; - if (*gc_header).obj_type != crate::gc::GC_TYPE_OBJECT { + }; + if gc_header.obj_type != crate::gc::GC_TYPE_OBJECT { return None; }🤖 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/object/field_get_set.rs` around lines 75 - 79, The GC header check in field_get_set should use the validated header reader instead of manually subtracting GC_HEADER_SIZE and dereferencing the header. Update the logic around the object-type validation to call try_read_gc_header (or the equivalent canonical helper) after is_plausible_heap_addr, and keep the existing GC_TYPE_OBJECT gate on the returned header. This ensures slab/small-buffer non-GC allocations are rejected before any header access while preserving the current field access flow.
🤖 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/number_format_options.rs`:
- Around line 87-94: Move the missing-currency TypeError check in
number_format_options::... so it runs immediately after reading currency and
before any access to currencyDisplay or currencySign. The current guard in the
currency style branch is still too late because the getter reads can throw
first; update the option-processing order in the currency-handling path so the
early throw happens before any other currency-related option validation or
property access.
In `@crates/perry-runtime/src/object/date_proto_thunks.rs`:
- Around line 381-408: The new Date.prototype.toLocaleString options path
forwards options, but the formatting pipeline still ignores timeZone and
timeZoneName, so Date output stays in the host default zone. Update the Intl
formatting flow in temporal_locale_string/date_collator.rs to thread these
options through the component extraction and formatting logic, and make sure
Date.prototype.toLocaleString via date_to_locale_string_opts produces output in
the requested zone instead of only rejecting timeZone for
Temporal.ZonedDateTime.
---
Outside diff comments:
In `@crates/perry-runtime/src/object/field_get_set.rs`:
- Around line 75-79: The GC header check in field_get_set should use the
validated header reader instead of manually subtracting GC_HEADER_SIZE and
dereferencing the header. Update the logic around the object-type validation to
call try_read_gc_header (or the equivalent canonical helper) after
is_plausible_heap_addr, and keep the existing GC_TYPE_OBJECT gate on the
returned header. This ensures slab/small-buffer non-GC allocations are rejected
before any header access while preserving the current field access flow.
🪄 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: f924f943-e96d-4ca3-9f77-da08662eec22
📒 Files selected for processing (6)
crates/perry-hir/src/lower/expr_call/url_date_instance.rscrates/perry-runtime/src/intl/date_collator.rscrates/perry-runtime/src/intl/number_format_options.rscrates/perry-runtime/src/object/date_proto_thunks.rscrates/perry-runtime/src/object/field_get_set.rscrates/perry-runtime/src/object/global_this/proto_methods.rs
…rencySign reads
Per CodeRabbit: the TypeError for `{ style: "currency" }` with no currency
was firing after the `currencyDisplay` and `currencySign` GetOption calls.
A proxy trap on `get currencyDisplay()` would throw a different error first,
breaking the spec-mandated observable read order. Move the check to
immediately after the `currency` read.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This was referenced Jul 16, 2026
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
1 participant
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
Fixes the 29 newly-failing test262 cases introduced by the overnight batch (#5789, #5793, #5783), grouped into three clusters.
Cluster A — 12 tests (
Temporal/**/from/order-of-operations.js)temporal_subclass_cellinfield_get_set.rsusedis_valid_obj_ptrto guard against non-heap pointers. On Linux (HEAP_MIN=0x1000) revocable Proxy ids in[0xF0000, 0x100000)pass that guard, and reading the GC header atproxy_id − 8segfaulted. Fixed by switching tois_plausible_heap_addr, which unconditionally rejects the entire handle band[0, 0x100000).Cluster B — 16 tests (Temporal
toLocaleStringregressions)Two sub-issues introduced by #5789:
Wrong per-type defaults: fix(temporal): preserve locale/options in toLocaleString, fix DTF-compatible defaults (#5580) #5789 unified all non-ZDT Temporal types to
{year, month, day}defaults. Restored ECMA-402 spec defaults:PlainDate→{y,m,d},PlainDateTime/Instant/ZDT→{y,m,d,h,min,sec},PlainTime→{h,min,sec},PlainYearMonth→{y,m},PlainMonthDay→{m,d}.Date args-dropping divergence: Tests comparing
Date.toLocaleString(locale, opts)with a Temporal type now diverge because fix(temporal): preserve locale/options in toLocaleString, fix DTF-compatible defaults (#5580) #5789 fixed Temporal args-dropping but leftDateon the HIR fold path (dropping args). Added a realDate.prototype.toLocaleStringthunk (date_to_locale_string_opts) that delegates totemporal_locale_stringwithPlainDateTimedefaults. Removedrecv_class == Some("Date")from the HIR fold so Date calls with args use the thunk.Cluster C — 1 test (
intl402/NumberFormat/constructor-order.js)NumberFormat({style:"currency", unit:"test"})threwRangeError(from the stricteris_well_formed_unit_identifieradded in #5783 rejecting"test") before theTypeErrorfor the missingcurrencyfield. Moved the currencyTypeErrorcheck ahead of the unit reads to restore spec-mandated observable order.Test plan
cargo test --release -p perry-runtime -p perry-hir -p perry-codegen -p perry-transform— all passlint,cargo-test,api-docs-drift,security-auditgreenCloses #5800
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Date.prototype.toLocaleString()behavior so calls with locale and options no longer take an incorrect fast-path and instead preserve provided arguments.toLocaleStringformatting when no options are supplied.Bug Fixes