fix(intl): #5581 — NumberFormat option read order, useGrouping/roundingIncrement validation, and format accessor#5728
Merged
Conversation
…ngIncrement 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>
|
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)
📝 WalkthroughWalkthroughThe PR updates ChangesIntl.NumberFormat wiring and options
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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
🤖 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.rs`:
- Around line 811-818: The NumberFormat bound format state is currently exposed
through the public own "format" property, which lets user mutations or deletion
affect [[BoundFormat]] and the prototype getter behavior. Update the
NumberFormat initialization and accessor path in intl.rs so
install_bound_instance_function no longer uses the public property as the
backing store; instead place the bound closure in an internal/hidden slot, have
number_format_format_getter_thunk return that slot, and keep the public "format"
property separate from the cached bound function.
In `@crates/perry-runtime/src/intl/number_format_options.rs`:
- Around line 184-198: In the number-format option validation logic inside
NumberFormatOptions, apply the roundingIncrement default-width adjustment before
the fraction-digit checks. When rounding_increment is not 1, ensure the resolved
minimumFractionDigits and maximumFractionDigits are forced to the same default
fraction width (so decimal/unit defaults become 0/0 instead of 0/3) before the
existing has_sd, rounding_priority, and equality validations run.
🪄 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: 693e9a80-ce58-47c7-9292-412289b042db
📒 Files selected for processing (3)
crates/perry-runtime/src/intl.rscrates/perry-runtime/src/intl/number_format.rscrates/perry-runtime/src/intl/number_format_options.rs
…o 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>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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.
Partially addresses #5581 (test262 intl402 NumberFormat gap). Aligns the
Intl.NumberFormatconstructor option handling and theformataccessor with ECMA-402, closing 10 test262 cases with no regressions.Changes
SetNumberFormatDigitOptions / CreateNumberFormat (
number_format_options.rs)localeMatcherfirst (it was never read at all), and keep the exact GetOption order the spec asserts —roundingIncrement,roundingMode,roundingPriorityare now read in sequence instead ofroundingPrioritybeing read ahead of significant-digit options.roundingIncrementisToNumber-coerced (so{ valueOf }and string options work) and validated against the sanctioned increment set{1,2,5,…,5000}rather than a bare[1,5000]range. A non-1 increment now requires the default fraction-digit rounding type (TypeErrorfor significant digits / non-autopriority) withmaximumFractionDigits == minimumFractionDigits(RangeErrorotherwise) — matching Node.useGroupingnow followsGetStringOrBooleanOptionprecisely: booleantrue→"always", anyToBoolean-false value (false/0/null/"") →false, the strings"true"/"false"fall back to the default, and only"min2"/"auto"/"always"are otherwise accepted.formataccessor (intl.rs,number_format.rs)Intl.NumberFormat.prototype.formatis now a getter (name"get format",length0) returning the instance's bound format function (name"",length1) — the ECMA-402[[BoundFormat]]shape. Instances keep an own boundformatfor the hot dispatch path (native objects resolve methods from own props), and the prototype getter is reflection-correct.install_constructorgained agettersslice; onlyNumberFormatpopulates it.Tests fixed (test262 @
4249661, Node v26.3.0)constructor-option-read-order(read order now matches; full pass still needs Proxy-trap support in native option reads — see below),constructor-options-throwing-gettersconstructor-roundingIncrement,constructor-roundingIncrement-invalidtest-option-useGrouping,test-option-useGrouping-extendedprototype/format/{length,name,prop-desc,no-instanceof,format-function-name}NumberFormat subset: 106 → 121 passing, 0 regressions. The existing
Intlnode-suite differential is unchanged.Not in scope (remaining #5581 work)
constructor-option-read-orderviapropertyBagObserveruses a Proxy; Perry's native option reads bypass Proxyget/hastraps (validated here with plain getters instead).ToObject/ array-like /HasProperty),formatRange/formatRangeToParts, currency sign placement, and CLDR-dependent locales (de/ja/ko/zh, non-Latin numbering systems).🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Intl.NumberFormatsoprototype.formatbehaves like an accessor, returning the correctly bound formatter function for the instance.Bug Fixes
localeMatcherto be read in the correctGetOptionorder, preserving expected deterministic behavior.useGroupingoption coercion and validation.roundingIncrement/roundingMode/roundingPriorityand related fraction digit constraints so invalid combinations consistently throw.