fix(intl): repair RelativeTimeFormat output#692
Merged
Conversation
- thread RelativeTimeFormat style through ICU bindings - repair en/pl numeric relative output using ICU plural and number formatting - validate RelativeTimeFormat options and resolved numbering system order - add focused RelativeTimeFormat format regressions Closes #599
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds a narrow relative-time style, threads style into ICU relative-time formatter calls, appends numbering-system tokens to number skeletons, adds locale helpers to parse ChangesRelative-time style and numbering-system support
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
Contributor
Suite TimingTest Runner (interpreted: 9,850 passed; bytecode: 9,850 passed)
MemoryGC rows aggregate the main thread plus all worker thread-local GCs. Test runner worker shutdown frees thread-local heaps in bulk; that shutdown reclamation is not counted as GC collections or collected objects.
Benchmarks (interpreted: 407; bytecode: 407)
MemoryGC rows aggregate the main thread plus all worker thread-local GCs. Benchmark runner performs explicit between-file collections, so collection and collected-object counts can be much higher than the test runner.
Measured on ubuntu-latest x64. |
Contributor
Benchmark Results407 benchmarks Interpreted: 🟢 341 improved · 🔴 21 regressed · 45 unchanged · avg +7.9% arraybuffer.js — Interp: 🟢 10, 🔴 1, 3 unch. · avg +7.3% · Bytecode: 🟢 14 · avg +98.6%
arrays.js — Interp: 🟢 16, 3 unch. · avg +6.6% · Bytecode: 🟢 19 · avg +88.3%
async-await.js — Interp: 🟢 5, 1 unch. · avg +8.6% · Bytecode: 🟢 6 · avg +94.8%
async-generators.js — Interp: 🟢 1, 1 unch. · avg +9.9% · Bytecode: 🟢 2 · avg +98.5%
base64.js — Interp: 🟢 3, 🔴 7 · avg -0.1% · Bytecode: 🟢 10 · avg +111.9%
classes.js — Interp: 🟢 27, 4 unch. · avg +8.0% · Bytecode: 🟢 31 · avg +77.5%
closures.js — Interp: 🟢 11 · avg +10.2% · Bytecode: 🟢 11 · avg +92.0%
collections.js — Interp: 🟢 11, 1 unch. · avg +11.1% · Bytecode: 🟢 12 · avg +96.6%
csv.js — Interp: 🟢 10, 3 unch. · avg +8.2% · Bytecode: 🟢 13 · avg +89.3%
destructuring.js — Interp: 🟢 20, 2 unch. · avg +8.3% · Bytecode: 🟢 22 · avg +87.4%
fibonacci.js — Interp: 🟢 8 · avg +10.2% · Bytecode: 🟢 8 · avg +95.2%
float16array.js — Interp: 🟢 29, 3 unch. · avg +7.7% · Bytecode: 🟢 32 · avg +73.7%
for-of.js — Interp: 🟢 7 · avg +4.4% · Bytecode: 🟢 7 · avg +94.5%
generators.js — Interp: 🟢 4 · avg +10.0% · Bytecode: 🟢 4 · avg +82.8%
iterators.js — Interp: 🟢 42 · avg +8.8% · Bytecode: 🟢 42 · avg +84.6%
json.js — Interp: 🟢 20 · avg +11.9% · Bytecode: 🟢 20 · avg +80.8%
jsx.jsx — Interp: 🟢 19, 2 unch. · avg +7.8% · Bytecode: 🟢 21 · avg +90.2%
modules.js — Interp: 🟢 7, 2 unch. · avg +8.3% · Bytecode: 🟢 9 · avg +105.0%
numbers.js — Interp: 🟢 10, 1 unch. · avg +10.1% · Bytecode: 🟢 11 · avg +90.2%
objects.js — Interp: 🟢 7 · avg +7.2% · Bytecode: 🟢 7 · avg +90.2%
promises.js — Interp: 🟢 7, 🔴 2, 3 unch. · avg +3.9% · Bytecode: 🟢 12 · avg +87.4%
regexp.js — Interp: 🟢 2, 🔴 7, 2 unch. · avg -1.8% · Bytecode: 🟢 11 · avg +100.3%
strings.js — Interp: 🟢 18, 1 unch. · avg +9.7% · Bytecode: 🟢 19 · avg +94.7%
tsv.js — Interp: 🟢 4, 5 unch. · avg +6.1% · Bytecode: 🟢 9 · avg +92.3%
typed-arrays.js — Interp: 🟢 20, 2 unch. · avg +18.6% · Bytecode: 🟢 22 · avg +75.9%
uint8array-encoding.js — Interp: 🟢 11, 🔴 2, 5 unch. · avg +2.6% · Bytecode: 🟢 18 · avg +95.2%
weak-collections.js — Interp: 🟢 12, 🔴 2, 1 unch. · avg +1.7% · Bytecode: 🟢 15 · avg +112.1%
Deterministic profile diffDeterministic profile diff: no significant changes. Measured on ubuntu-latest x64. Benchmark ranges compare cached main-branch min/max ops/sec with the PR run; overlapping ranges are treated as unchanged noise. Percentage deltas are secondary context. |
Contributor
test262 Conformance
Areas closest to 100%
Per-test deltas (+27 / -0)Newly passing (27):
Steady-state failures are non-blocking; regressions vs the cached main baseline (lower total pass count, or any PASS → non-PASS transition) fail the conformance gate. Measured on ubuntu-latest x64, bytecode mode. Areas grouped by the first two test262 path components; minimum 25 attempted tests, areas already at 100% excluded. Δ vs main compares against the most recent cached |
coderabbitai
Bot
added
bug
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
source/units/Goccia.Values.IntlRelativeTimeFormat.pas (1)
472-525:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon’t short-circuit
numeric: "auto"through a numeric-only repair path.
TryFormatRelativeTimeAutoNameonly knows English long-form literals, whileTryFormatRelativeTimeWithLocalePattern*always emits numeric phrases. With the current ordering,numeric: "auto"loses ICU’s locale/style-specific named forms: Polish falls back to numeric output for-1/0/1, and Englishshort/narrowstill returns the long literals fromTryFormatRelativeTimeAutoName.💡 Minimal safe direction
- if (NumericValue = irtnAuto) and + if (NumericValue = irtnAuto) and (StyleValue = irtsLong) and TryFormatRelativeTimeAutoName(NumValue, RTF.FLocale, UnitValue, Formatted) then Result := TGocciaStringLiteralValue.Create(Formatted) - else if TryFormatRelativeTimeWithLocalePattern(NumValue, IsPast, RTF.FLocale, + else if (NumericValue = irtnAlways) and + TryFormatRelativeTimeWithLocalePattern(NumValue, IsPast, RTF.FLocale, RTF.FNumberingSystem, UnitValue, StyleValue, Formatted) then Result := TGocciaStringLiteralValue.Create(Formatted)- if (NumericValue = irtnAuto) and + if (NumericValue = irtnAuto) and (StyleValue = irtsLong) and TryFormatRelativeTimeAutoName(NumValue, RTF.FLocale, UnitValue, Formatted) then begin ... end; - if TryFormatRelativeTimePartsWithLocalePattern(NumValue, IsPast, RTF.FLocale, + if (NumericValue = irtnAlways) and + TryFormatRelativeTimePartsWithLocalePattern(NumValue, IsPast, RTF.FLocale, RTF.FNumberingSystem, UnitValue, StyleValue, Parts) then Exit(FormatPartsToArray(Parts));Also applies to: 794-799, 836-848
🤖 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 `@source/units/Goccia.Values.IntlRelativeTimeFormat.pas` around lines 472 - 525, The code is short-circuiting numeric:"auto" through the numeric-only path by calling TryFormatRelativeTimeAutoName too early; change the flow so that any TryFormatRelativeTimeWithLocalePattern* functions are invoked first for numeric:"auto" and only if they produce no output fall back to TryFormatRelativeTimeAutoName, and when falling back ensure TryFormatRelativeTimeAutoName is only used for English long-form cases (locale starts with 'en' and style == long) to avoid returning English long literals for other locales or short/narrow styles.
🧹 Nitpick comments (3)
tests/built-ins/Intl/RelativeTimeFormat/prototype/format.js (1)
8-8: ⚡ Quick winConsider adding Polish locale test coverage.
The implementation adds locale-pattern formatters for both English and Polish (as noted in the review stack context), but the test suite only covers English. Adding a test case for Polish would verify that the Polish-specific formatting logic works correctly.
📝 Example Polish test case
+ test("formats Polish relative time with locale-specific patterns", () => { + const rtf = new Intl.RelativeTimeFormat("pl", { numeric: "always" }); + + expect(rtf.format(-1, "day")).toBe("1 dzień temu"); + expect(rtf.format(-2, "day")).toBe("2 dni temu"); + expect(rtf.format(1, "day")).toBe("za 1 dzień"); + });Note: Verify the exact expected Polish output strings with the implementation.
🤖 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 `@tests/built-ins/Intl/RelativeTimeFormat/prototype/format.js` at line 8, Add a Polish locale test to the Intl.RelativeTimeFormat.prototype.format suite: inside the existing describe.runIf block for "Intl.RelativeTimeFormat.prototype.format", create a test that constructs new Intl.RelativeTimeFormat('pl', { numeric: 'auto' }) and calls its format for a few units/values (e.g., 1 day, -1 day, 2 hours) asserting the expected Polish strings; reference the existing test patterns in the file to mirror structure and assertions so you cover locale-specific pluralization and word forms for 'pl' alongside the existing English cases.source/units/Goccia.Intl.Helpers.pas (1)
90-99: ⚡ Quick winConsider documenting that this strips the entire Unicode extension.
LocaleWithoutUnicodeExtensionremoves everything from-u-onward, not just specific keywords. For example,"en-US-u-ca-gregory-nu-latn"becomes"en-US", losing both thecaandnuextensions. If the intent is to strip all Unicode extensions when reconciling options, this is correct; otherwise, a more selective approach might be needed. Adding a comment clarifying this behavior would help future maintainers.🤖 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 `@source/units/Goccia.Intl.Helpers.pas` around lines 90 - 99, The function LocaleWithoutUnicodeExtension currently removes everything from the first "-u-" onward (i.e., it strips the entire Unicode extension, so "en-US-u-ca-gregory-nu-latn" becomes "en-US"); update the code by adding a concise comment above LocaleWithoutUnicodeExtension that documents this behavior (that it strips the entire Unicode extension) and notes the alternative (implement selective keyword removal if only certain keys like "ca" or "nu" should be removed) so future maintainers understand the intended behavior and can change it if needed.source/units/Goccia.Values.IntlNumberFormat.pas (1)
583-600: ⚖️ Poor tradeoffReconciliation logic strips the entire Unicode extension, not just the
nukeyword.When an explicit
numberingSystemoption is provided (lines 587-589), the code callsLocaleWithoutUnicodeExtension(FLocale)if the locale'snuextension doesn't match (or is absent). This removes the entire-u-extension, which could discard other Unicode extension keywords like-ca-(calendar) or-hc-(hour cycle).For example:
- Input:
locale = "en-US-u-ca-gregory-nu-latn",options = { numberingSystem: "arab" }- Result:
FLocalebecomes"en-US"(losing bothcaandnu)If this behavior is intentional (to avoid ambiguity when passing the locale to ICU and also using the skeleton
numbering-system/arabtoken), consider adding a comment explaining why the entire extension is stripped. If other extensions should be preserved, a more selective removal approach would be needed.🤖 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 `@source/units/Goccia.Values.IntlNumberFormat.pas` around lines 583 - 600, The reconciliation currently calls LocaleWithoutUnicodeExtension(FLocale) which strips the entire -u- extension (losing other keywords like ca/hc); change the logic so when FNumberingSystem is set but doesn't match the locale's 'nu' keyword you remove only the 'nu' key from FLocale instead of the whole Unicode extension. Implement or use a helper that removes just the 'nu' keyword (e.g., RemoveUnicodeExtensionKeyword(FLocale, 'nu')) and call that in the branches that currently invoke LocaleWithoutUnicodeExtension; keep using TryGetUnicodeLocaleExtensionKeyword, IsSupportedNumberingSystem, FNumberingSystem and FLocale to detect and update the locale.
🤖 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 `@source/shared/IntlICU.pas`:
- Around line 220-221: The constants UDAT_RELATIVE_YEARS and
UDAT_RELATIVE_QUARTERS are inverted relative to ICU4C; update them so
UDAT_RELATIVE_QUARTERS = 6 and UDAT_RELATIVE_YEARS = 7 (or alternatively change
the mapping in RelativeTimeUnitToICU so irtuYear maps to 7 and irtuQuarter maps
to 6) to match ICU4C's URelativeDateTimeUnit values, and verify the enum cases
irtuYear and irtuQuarter in RelativeTimeUnitToICU are producing the corrected
ICU values.
In `@source/units/Goccia.Values.IntlRelativeTimeFormat.pas`:
- Around line 657-708: The constructor currently strips or ignores the requested
numbering system so FNumberingSystem only affects resolvedOptions() but not
actual formatting; ensure the chosen numbering system is applied to both the ICU
path and the fallback number-splitting path: when calling
TryICUFormatRelativeTime* (and other ICU helpers) pass a locale that includes
the -u-nu- extension for FNumberingSystem or otherwise supply FNumberingSystem
to the ICU call so ICU uses that numbering system, and in the fallback splitter
use FNumberingSystem when building number parts (instead of
DefaultNumberFormatOptions/'latn'); update logic around
FLocale/LocaleWithoutUnicodeExtension, TryGetLocaleNumberingSystemExtension, and
DefaultNumberFormatOptions so that FNumberingSystem is consistently used for
formatting, not just in resolvedOptions().
---
Outside diff comments:
In `@source/units/Goccia.Values.IntlRelativeTimeFormat.pas`:
- Around line 472-525: The code is short-circuiting numeric:"auto" through the
numeric-only path by calling TryFormatRelativeTimeAutoName too early; change the
flow so that any TryFormatRelativeTimeWithLocalePattern* functions are invoked
first for numeric:"auto" and only if they produce no output fall back to
TryFormatRelativeTimeAutoName, and when falling back ensure
TryFormatRelativeTimeAutoName is only used for English long-form cases (locale
starts with 'en' and style == long) to avoid returning English long literals for
other locales or short/narrow styles.
---
Nitpick comments:
In `@source/units/Goccia.Intl.Helpers.pas`:
- Around line 90-99: The function LocaleWithoutUnicodeExtension currently
removes everything from the first "-u-" onward (i.e., it strips the entire
Unicode extension, so "en-US-u-ca-gregory-nu-latn" becomes "en-US"); update the
code by adding a concise comment above LocaleWithoutUnicodeExtension that
documents this behavior (that it strips the entire Unicode extension) and notes
the alternative (implement selective keyword removal if only certain keys like
"ca" or "nu" should be removed) so future maintainers understand the intended
behavior and can change it if needed.
In `@source/units/Goccia.Values.IntlNumberFormat.pas`:
- Around line 583-600: The reconciliation currently calls
LocaleWithoutUnicodeExtension(FLocale) which strips the entire -u- extension
(losing other keywords like ca/hc); change the logic so when FNumberingSystem is
set but doesn't match the locale's 'nu' keyword you remove only the 'nu' key
from FLocale instead of the whole Unicode extension. Implement or use a helper
that removes just the 'nu' keyword (e.g., RemoveUnicodeExtensionKeyword(FLocale,
'nu')) and call that in the branches that currently invoke
LocaleWithoutUnicodeExtension; keep using TryGetUnicodeLocaleExtensionKeyword,
IsSupportedNumberingSystem, FNumberingSystem and FLocale to detect and update
the locale.
In `@tests/built-ins/Intl/RelativeTimeFormat/prototype/format.js`:
- Line 8: Add a Polish locale test to the
Intl.RelativeTimeFormat.prototype.format suite: inside the existing
describe.runIf block for "Intl.RelativeTimeFormat.prototype.format", create a
test that constructs new Intl.RelativeTimeFormat('pl', { numeric: 'auto' }) and
calls its format for a few units/values (e.g., 1 day, -1 day, 2 hours) asserting
the expected Polish strings; reference the existing test patterns in the file to
mirror structure and assertions so you cover locale-specific pluralization and
word forms for 'pl' alongside the existing English cases.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6701716f-97a4-40ee-b32b-0d6f420733cb
📒 Files selected for processing (8)
source/shared/IntlICU.passource/shared/IntlTypes.passource/units/Goccia.Intl.Helpers.passource/units/Goccia.Values.IntlNumberFormat.passource/units/Goccia.Values.IntlRelativeTimeFormat.pastests/built-ins/Intl/NumberFormat/prototype/format.jstests/built-ins/Intl/RelativeTimeFormat/prototype/format.jstests/built-ins/Intl/RelativeTimeFormat/prototype/formatToParts.js
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
Intl.RelativeTimeFormat.prototype.formatandformatToPartsfor localized numeric relative output, includingnumeric: "auto", plural-sensitive units, grouped number parts, negative zero, and non-finite value rejection.stylethrough the ICU binding, correct ICU relative quarter/year unit constants, and add an ICU plural/number-format backed repair path for the host ICU relative formatter behavior.Testing
./build/GocciaTestRunner tests/built-ins/Intl/RelativeTimeFormat --no-progress --exit-on-first-failurebun scripts/run_test262_suite.ts --suite-dir /tmp/goccia-test262-pinned --categories intl402 --filter 'intl402/RelativeTimeFormat/prototype/format/*' --mode bytecode --jobs 1 --output=/tmp/rtf-format.jsonbun scripts/run_test262_suite.ts --suite-dir /tmp/goccia-test262-pinned --categories intl402 --filter 'intl402/RelativeTimeFormat/prototype/formatToParts/*' --mode bytecode --jobs 1 --output=/tmp/rtf-format-parts.jsonbun scripts/run_test262_suite.ts --suite-dir /tmp/goccia-test262-pinned --categories intl402 --filter 'intl402/RelativeTimeFormat/prototype/resolvedOptions/*' --mode bytecode --jobs 1 --output=/tmp/rtf-resolved.jsonbun scripts/run_test262_suite.ts --suite-dir /tmp/goccia-test262-pinned --categories intl402 --filter 'intl402/RelativeTimeFormat/**' --mode bytecode --jobs 1 --output=/tmp/rtf-all.jsonnow passes 67/80; remaining failures are constructor/prototype/locale support outside this formatting fix../build.pas testrunner && ./build/GocciaTestRunner testswas run; it still reports the existing FFI fixture load failures for./fixtures/ffi/libfixture.dylib.Intl.RelativeTimeFormat; this fixes conformance of existing behavior.