Intl.NumberFormat: fix percent formatRange scaling twice on approximately path - #36362
Open
robobun wants to merge 5 commits into
Open
Intl.NumberFormat: fix percent formatRange scaling twice on approximately path#36362robobun wants to merge 5 commits into
robobun wants to merge 5 commits into
Conversation
…tely path With style:"percent", formatRange(a, a) (and endpoints that round equal) rendered "~5,000%" for 0.5 instead of "~50%": ICU's NumberRangeFormatterImpl::formatApproximately re-runs preProcess on an already-scaled quantity, applying scale/100 a second time (ICU-23110, fixed upstream in ICU 78.1). The fix is a build-time ICU patch applied in oven-sh/WebKit#374. This commit bumps WEBKIT_VERSION to that PR's preview build and adds coverage: for en/de/ja/ar, the numeric parts of formatRangeToParts(a, a) must equal formatToParts(a), and the collapsed range string must contain format(a). Covers double, BigInt, string, and compact-notation inputs, plus a distinct-range sanity check. macOS is unaffected (Apple's system ICU already renders correctly).
Contributor
WalkthroughChangesWebKit preview dependency
Intl.NumberFormat regression coverage
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
Collaborator
Author
|
Status: waiting on oven-sh/WebKit#374 CI to publish The initial WebKit run failed on Android/FreeBSD because Repro: bun -e 'console.log(new Intl.NumberFormat("en",{style:"percent"}).formatRange(0.5,0.5))'
# bun: ~5,000% node: ~50% |
Collaborator
Author
|
Updated 5:26 AM PT - Jul 29th, 2026
❌ @autofix-ci[bot], your commit 8687fc2 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 36362That installs a local version of the PR into your bun-36362 --bun |
The previous preview sha was superseded after installing `patch` in the Android/FreeBSD Dockerfiles (ubuntu:24.04 does not ship it and those images had no patch step before).
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
expect({a, b, range}.range) reads the property before expect() sees the
wrapper, so the loop context never appears in the failure message.
Use toEqual with expect.stringContaining so the diff shows which (a, b)
pair produced the mismatch.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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 `@scripts/build/deps/webkit.ts`:
- Line 6: Update the WEBKIT_VERSION constant to reference a currently published
merged WebKit SHA or release tag, replacing the unavailable
autobuild-preview-pr-374-46c558e6 value so the default prebuilt download
succeeds.
In `@test/js/web/intl/intl.test.ts`:
- Around line 107-108: Update the distinct-endpoint range assertion near
nf.formatRange(0.5, 0.6) to validate the structured output rather than substring
containment. Use formatRangeToParts() and compare its range-related parts with
formatToParts() results, or explicitly assert the expected startRange and
endRange parts; apply the same structural validation to the compact-notation
check.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 26a9ebbe-8679-4ae1-8630-70f1554359c8
📒 Files selected for processing (2)
scripts/build/deps/webkit.tstest/js/web/intl/intl.test.ts
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.
Problem
With
style: "percent",Intl.NumberFormat.prototype.formatRange(andformatRangeToParts) applies the ×100 scale twice whenever the range collapses to a single approximate value (equal endpoints, or endpoints that round to the same formatted number):Reproduces in every locale and through every input path (double, BigInt, string); compact notation shows the same ×100 (
formatRange(15, 15)→~150K%, Node~1.5K%). Distinct ranges,style: "unit" / "currency" / "decimal"collapses, and macOS are all correct.Cause
ICU-23110.
NumberRangeFormatterImpl::formatcallsformatterImpl1.preProcess(data.quantity1, ...)(which applies the skeleton'sscale/100), then on theUNUM_IDENTITY_FALLBACK_APPROXIMATELYpath hands the already-scaleddata.quantity1toformatApproximately, which callsfApproximatelyFormatter.preProcess(data.quantity1, ...)again. The interveningresetExponent()only clears compact/scientific exponent state, not the multiplier, so 0.5 → 50 → 5000.JSC builds the range formatter from the same skeleton as
format()(" percent scale/100") and passes raw inputs tounumrf_formatDoubleRange/unumrf_formatDecimalRange, which is also what V8 does. Node is correct only because it ships ICU ≥ 78. Bun bundles ICU 75.1 on Linux/musl/Android/FreeBSD and 73.2 on Windows; macOS links Apple'slibicucore, which already renders correctly.Fix
oven-sh/WebKit#374 backports unicode-org/icu@757be359 (first released in ICU 78.1) as a build-time patch applied in every Dockerfile that builds ICU. The patch takes a copy of
data.quantity1before the firstpreProcessand feeds that copy toformatApproximately, so the approximately formatter starts from the original quantity.This PR:
WEBKIT_VERSIONto that PR's preview build;test/js/web/intl/intl.test.ts: foren/de/ja/ar, the numeric parts offormatRangeToParts(a, a)must equalformatToParts(a), and the collapsed range string must containformat(a). Covers double, BigInt, string, and compact-notation inputs, plus a distinct-range sanity check.Verification
Built ICU 75.1 with and without the backport and drove
unumrf_formatDoubleRangedirectly:The new test block:
Expected to contain: "50%"/Received: "~5,000%", all four locales, BigInt/string/compact);Note: the automated fail-before/fail-after gate does not apply here because the code change is a prebuilt-dependency bump (
scripts/build/deps/webkit.ts), notsrc/**; the WebKit preview build is what makes the new tests pass. This PR's CI build step will fail until the oven-sh/WebKit#374 preview release (autobuild-preview-pr-374-9cfb1f57) is published; once it merges theWEBKIT_VERSIONhere will be updated to the resulting autobuild sha.no test proof · iteration 3 · Platform-specific test-only change; deferring to CI.