Skip to content

Commit 9b6d12c

Browse files
proggeramlugRalphclaude
authored
fix(intl): #5581 — NumberFormat option read order, useGrouping/roundingIncrement validation, and format accessor (#5728)
* fix(intl): #5581 — NumberFormat option read order, useGrouping/roundingIncrement validation, and format accessor Closes 10 test262 intl402/NumberFormat cases by aligning the constructor's GetOption sequence and the `format` accessor shape with ECMA-402. SetNumberFormatDigitOptions / CreateNumberFormat: - Read `localeMatcher` first (it was never read), then keep the exact GetOption order the spec asserts: roundingIncrement, roundingMode, roundingPriority now read in sequence instead of roundingPriority jumping ahead of the others. (constructor-option-read-order, constructor-options-throwing-getters) - `roundingIncrement` is ToNumber-coerced (so `{ valueOf }` / string options work) and validated against the sanctioned increment set rather than a bare [1, 5000] range; a non-1 increment now requires the default fraction-digit rounding type (TypeError otherwise) with maxFrac == minFrac (RangeError otherwise). (constructor-roundingIncrement[-invalid]) - `useGrouping` follows GetStringOrBooleanOption exactly: boolean true → "always", any ToBoolean-false value (false/0/null/"") → false, the strings "true"/"false" fall back to the default, and only "min2"/"auto"/"always" are otherwise valid. (test-option-useGrouping[-extended]) format accessor: - `Intl.NumberFormat.prototype.format` is now a getter (name "get format", length 0) returning the instance's bound format function (name "", length 1), matching the ECMA-402 [[BoundFormat]] shape. Instances keep an own bound `format` for the hot dispatch path. (format/{length,name,prop-desc, no-instanceof,format-function-name}) - install_constructor gained a getters slice; only NumberFormat uses it. Verified against test262 @ 4249661 with Node v26.3.0: NumberFormat subset goes from 106 to 121 passing with no regressions; no parity changes in the existing Intl node-suite differential. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(intl): back NumberFormat [[BoundFormat]] with a hidden slot; cargo fmt CodeRabbit review follow-up: - The `format` getter now reads the bound format function from a hidden KEY_NF_BOUND_FORMAT slot instead of the public own `format` property, so `nf.format = 1` / `delete nf.format` can no longer corrupt what `Object.getOwnPropertyDescriptor(proto, "format").get.call(nf)` returns. The own `format` property is retained only for the native dispatch fast path. - Applied `cargo fmt`. (CodeRabbit's second finding — that `roundingIncrement !== 1` should resolve the default fraction width to 0/0 instead of throwing — does not match Node v26: `new Intl.NumberFormat("en", { roundingIncrement: 5 })` and `{ roundingIncrement: 5, maximumFractionDigits: 2 }` both throw RangeError there, matching this implementation.) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * style: cargo fmt the bound-format slot assignment Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Ralph <ralph@skelpo.com> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent de866a9 commit 9b6d12c

3 files changed

Lines changed: 184 additions & 54 deletions

File tree

‎crates/perry-runtime/src/intl.rs‎

Lines changed: 88 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -61,12 +61,13 @@ pub(crate) use number_format::{
6161
decimal_msd_exponent, format_number_instance, grouping_enabled, increment_decimal,
6262
intl_object_from_value, nf_coerce_number, nf_load, nf_resolved_default,
6363
number_format_bound_format_thunk, number_format_bound_resolved_options_thunk,
64-
number_format_bound_to_parts_thunk, number_format_format_object, number_format_format_thunk,
65-
number_format_resolved_options_object, number_format_resolved_options_thunk,
66-
number_format_to_parts_thunk, number_instance_parts, number_parts_from_resolved,
67-
parts_to_js_array, push_grouped_integer, push_sign, push_style_suffix, round_integer_to_place,
68-
round_mode_code, round_to_fraction, round_to_significant, rounding_up, set_round_ctx,
69-
significant_count, strip_leading_zeros, this_intl_object, trim_fraction, NfResolved,
64+
number_format_bound_to_parts_thunk, number_format_format_getter_thunk,
65+
number_format_format_object, number_format_resolved_options_object,
66+
number_format_resolved_options_thunk, number_format_to_parts_thunk, number_instance_parts,
67+
number_parts_from_resolved, parts_to_js_array, push_grouped_integer, push_sign,
68+
push_style_suffix, round_integer_to_place, round_mode_code, round_to_fraction,
69+
round_to_significant, rounding_up, set_round_ctx, significant_count, strip_leading_zeros,
70+
this_intl_object, trim_fraction, NfResolved,
7071
};
7172
pub(crate) use number_format_options::{
7273
configure_number_format, is_well_formed_currency_code, is_well_formed_unit_identifier,
@@ -148,6 +149,11 @@ const KEY_NF_ROUNDING_INCREMENT: &str = "__intlNfRoundingIncrement";
148149
const KEY_NF_ROUNDING_MODE: &str = "__intlNfRoundingMode";
149150
const KEY_NF_ROUNDING_PRIORITY: &str = "__intlNfRoundingPriority";
150151
const KEY_NF_TRAILING_ZERO: &str = "__intlNfTrailingZero";
152+
// Hidden [[BoundFormat]] slot. The bound format function is also installed as an
153+
// own `format` property for the native dispatch fast path, but the prototype
154+
// `format` getter reads it from here so user mutation/deletion of the public
155+
// property can't corrupt what the accessor returns.
156+
const KEY_NF_BOUND_FORMAT: &str = "__intlNfBoundFormat";
151157

152158
fn undefined() -> f64 {
153159
f64::from_bits(crate::value::TAG_UNDEFINED)
@@ -306,25 +312,31 @@ fn get_string_option_enum(options: f64, key: &str, allowed: &[&str], default: &s
306312
fn get_use_grouping_option(options: f64, default: &str) -> String {
307313
let value = get_option_value(options, "useGrouping");
308314
let js = JSValue::from_bits(value.to_bits());
315+
// GetStringOrBooleanOption(options, "useGrouping",
316+
// «"min2","auto","always"», "always", false, fallback):
317+
// 2. undefined → fallback.
309318
if js.is_undefined() {
310319
return default.to_string();
311320
}
312-
if js.is_bool() {
313-
return if js.as_bool() { "always" } else { "false" }.to_string();
321+
// 3. The boolean `true` → trueValue ("always").
322+
if js.is_bool() && js.as_bool() {
323+
return "always".to_string();
314324
}
315-
// Strings (and other coercibles) follow the WellFormedUnicodeString path.
325+
// 4. Any value whose ToBoolean is false (false, 0, null, "") → falseValue,
326+
// stored as the sentinel "false" (resolvedOptions surfaces it as `false`).
327+
if crate::value::js_is_truthy(value) == 0 {
328+
return "false".to_string();
329+
}
330+
// 5-8. ToString the (truthy) value. The strings "true"/"false" map back to
331+
// the fallback; only the sanctioned grouping strings are otherwise valid.
316332
let s = if js.is_any_string() {
317333
string_from_string_value(value).unwrap_or_default()
318-
} else if js.is_null() {
319-
// `null` coerces to the string "null" → not in the allow-list → RangeError.
320-
"null".to_string()
321334
} else {
322335
value_to_string(value)
323336
};
324337
match s.as_str() {
338+
"true" | "false" => default.to_string(),
325339
"min2" | "auto" | "always" => s,
326-
"true" => "always".to_string(),
327-
"false" => "false".to_string(),
328340
other => throw_range_error(&format!(
329341
"Value {other} out of range for Intl.NumberFormat options property useGrouping"
330342
)),
@@ -797,12 +809,27 @@ fn make_instance(closure: *const ClosureHeader, kind: &str, locales: f64, option
797809
match kind {
798810
KIND_NUMBER => {
799811
configure_number_format(obj, &locale, options);
800-
install_bound_instance_function(
812+
// The bound format function is the [[BoundFormat]] slot: ECMA-402
813+
// gives it an empty `name` ("") and length 1. It is installed as an
814+
// own `format` property so `nf.format(x)` dispatches without the
815+
// prototype accessor (native objects resolve methods from own
816+
// props), and is also stashed in the hidden KEY_NF_BOUND_FORMAT slot
817+
// that the prototype `format` getter reads — so mutating or deleting
818+
// the public property can't corrupt what the accessor returns.
819+
let format_fn = install_bound_instance_function(
801820
obj,
802821
"format",
803822
number_format_bound_format_thunk as *const u8,
804823
1,
805824
);
825+
if !format_fn.is_null() {
826+
crate::object::set_bound_native_closure_name(format_fn, "");
827+
set_internal_field(
828+
obj,
829+
KEY_NF_BOUND_FORMAT,
830+
js_nanbox_pointer(format_fn as i64),
831+
);
832+
}
806833
install_bound_instance_function(
807834
obj,
808835
"formatToParts",
@@ -1150,10 +1177,10 @@ fn install_bound_instance_function(
11501177
name: &str,
11511178
func_ptr: *const u8,
11521179
arity: u32,
1153-
) {
1180+
) -> *mut ClosureHeader {
11541181
let closure = crate::closure::js_closure_alloc(func_ptr, 1);
11551182
if closure.is_null() {
1156-
return;
1183+
return closure;
11571184
}
11581185
crate::closure::js_register_closure_arity(func_ptr, arity);
11591186
crate::closure::js_closure_set_capture_f64(closure, 0, js_nanbox_pointer(obj as i64));
@@ -1171,6 +1198,7 @@ fn install_bound_instance_function(
11711198
);
11721199
set_field(obj, name, js_nanbox_pointer(closure as i64));
11731200
set_builtin_attrs(obj, name, PropertyAttrs::new(true, false, true));
1201+
closure
11741202
}
11751203

11761204
extern "C" fn number_format_constructor_thunk(closure: *const ClosureHeader, rest: f64) -> f64 {
@@ -1306,6 +1334,7 @@ fn install_constructor(
13061334
ctor_ptr: *const u8,
13071335
ctor_length: u32,
13081336
methods: &[(&str, *const u8, u32)],
1337+
getters: &[(&str, *const u8)],
13091338
) {
13101339
let ctor = crate::closure::js_closure_alloc(ctor_ptr, 0);
13111340
if ctor.is_null() {
@@ -1326,12 +1355,43 @@ fn install_constructor(
13261355
);
13271356

13281357
let ctor_value = js_nanbox_pointer(ctor as i64);
1329-
let proto = js_object_alloc(0, 4);
1358+
// Generous inline capacity so installing methods plus an accessor getter and
1359+
// the toStringTag symbol never bumps `field_count` past the physical slot
1360+
// count (which would expose an overflow slot — keys_array.rs #4099).
1361+
let proto = js_object_alloc(0, 16);
13301362
set_field(proto, "constructor", ctor_value);
13311363
set_builtin_attrs(proto, "constructor", PropertyAttrs::new(true, false, true));
13321364
for (method, ptr, arity) in methods.iter().copied() {
13331365
install_function(proto, method, ptr, arity, arity, false);
13341366
}
1367+
// Accessor properties (e.g. `get Intl.NumberFormat.prototype.format`): a
1368+
// getter-only descriptor on the prototype so reflection
1369+
// (`Object.getOwnPropertyDescriptor(proto, key).get`) sees a function whose
1370+
// name is `"get <key>"` and length 0. Instances still carry an own bound
1371+
// method for the hot dispatch path (native objects resolve from own props).
1372+
for (getter_name, ptr) in getters.iter().copied() {
1373+
let closure = crate::closure::js_closure_alloc(ptr, 0);
1374+
if closure.is_null() {
1375+
continue;
1376+
}
1377+
crate::closure::js_register_closure_arity(ptr, 0);
1378+
crate::object::set_bound_native_closure_name(closure, &format!("get {getter_name}"));
1379+
crate::object::set_builtin_closure_length(closure as usize, 0);
1380+
crate::object::set_builtin_property_attrs(
1381+
closure as usize,
1382+
"name".to_string(),
1383+
PropertyAttrs::new(false, false, true),
1384+
);
1385+
crate::object::set_builtin_property_attrs(
1386+
closure as usize,
1387+
"length".to_string(),
1388+
PropertyAttrs::new(false, false, true),
1389+
);
1390+
let getter_bits = js_nanbox_pointer(closure as i64).to_bits();
1391+
unsafe {
1392+
crate::object::install_builtin_getter(proto, getter_name, getter_bits);
1393+
}
1394+
}
13351395
set_proto_to_string_tag(proto, &format!("Intl.{name}"));
13361396
let proto_value = js_nanbox_pointer(proto as i64);
13371397
crate::closure::closure_set_dynamic_prop(ctor as usize, "prototype", proto_value);
@@ -1384,7 +1444,6 @@ pub fn install_intl_namespace(ns_obj: *mut ObjectHeader) {
13841444
number_format_constructor_thunk as *const u8,
13851445
0,
13861446
&[
1387-
("format", number_format_format_thunk as *const u8, 1),
13881447
(
13891448
"formatToParts",
13901449
number_format_to_parts_thunk as *const u8,
@@ -1396,6 +1455,8 @@ pub fn install_intl_namespace(ns_obj: *mut ObjectHeader) {
13961455
0,
13971456
),
13981457
],
1458+
// `format` is an accessor (getter) per ECMA-402, not a plain method.
1459+
&[("format", number_format_format_getter_thunk as *const u8)],
13991460
);
14001461
install_constructor(
14011462
ns_obj,
@@ -1421,6 +1482,7 @@ pub fn install_intl_namespace(ns_obj: *mut ObjectHeader) {
14211482
0,
14221483
),
14231484
],
1485+
&[],
14241486
);
14251487
install_constructor(
14261488
ns_obj,
@@ -1435,6 +1497,7 @@ pub fn install_intl_namespace(ns_obj: *mut ObjectHeader) {
14351497
0,
14361498
),
14371499
],
1500+
&[],
14381501
);
14391502
install_constructor(
14401503
ns_obj,
@@ -1449,6 +1512,7 @@ pub fn install_intl_namespace(ns_obj: *mut ObjectHeader) {
14491512
0,
14501513
),
14511514
],
1515+
&[],
14521516
);
14531517
install_constructor(
14541518
ns_obj,
@@ -1464,6 +1528,7 @@ pub fn install_intl_namespace(ns_obj: *mut ObjectHeader) {
14641528
0,
14651529
),
14661530
],
1531+
&[],
14671532
);
14681533
install_constructor(
14691534
ns_obj,
@@ -1479,6 +1544,7 @@ pub fn install_intl_namespace(ns_obj: *mut ObjectHeader) {
14791544
0,
14801545
),
14811546
],
1547+
&[],
14821548
);
14831549
install_constructor(
14841550
ns_obj,
@@ -1498,6 +1564,7 @@ pub fn install_intl_namespace(ns_obj: *mut ObjectHeader) {
14981564
0,
14991565
),
15001566
],
1567+
&[],
15011568
);
15021569
install_constructor(
15031570
ns_obj,
@@ -1517,6 +1584,7 @@ pub fn install_intl_namespace(ns_obj: *mut ObjectHeader) {
15171584
0,
15181585
),
15191586
],
1587+
&[],
15201588
);
15211589
install_constructor(
15221590
ns_obj,
@@ -1531,5 +1599,6 @@ pub fn install_intl_namespace(ns_obj: *mut ObjectHeader) {
15311599
0,
15321600
),
15331601
],
1602+
&[],
15341603
);
15351604
}

‎crates/perry-runtime/src/intl/number_format.rs‎

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -751,12 +751,15 @@ pub(crate) fn intl_object_from_value(
751751
obj
752752
}
753753

754-
pub(crate) extern "C" fn number_format_format_thunk(
755-
_closure: *const ClosureHeader,
756-
value: f64,
757-
) -> f64 {
754+
/// `get Intl.NumberFormat.prototype.format` — the ECMA-402 accessor. Validates
755+
/// that `this` is an initialized NumberFormat (TypeError otherwise) and returns
756+
/// the instance's bound format function ([[BoundFormat]]). It reads the hidden
757+
/// KEY_NF_BOUND_FORMAT slot (set at construction, name `""`, length 1) rather
758+
/// than the public own `format` property, so user mutation/deletion of that
759+
/// property can't change what the accessor returns.
760+
pub(crate) extern "C" fn number_format_format_getter_thunk(_closure: *const ClosureHeader) -> f64 {
758761
let obj = this_intl_object("format", KIND_NUMBER);
759-
number_format_format_object(obj, value)
762+
get_field(obj, KEY_NF_BOUND_FORMAT)
760763
}
761764

762765
pub(crate) extern "C" fn number_format_bound_format_thunk(

0 commit comments

Comments
 (0)