Add ECMA-402 Intl namespace with ICU-first data strategy (#542)#590
Merged
Conversation
Full implementation of the ECMA-402 Internationalization API: Intl.Locale, Intl.Collator, Intl.NumberFormat, Intl.DateTimeFormat, Intl.PluralRules, Intl.RelativeTimeFormat, Intl.ListFormat, Intl.DisplayNames, Intl.Segmenter, Intl.DurationFormat, plus Intl.getCanonicalLocales and Intl.supportedValuesOf. Data strategy mirrors Temporal timezone: system ICU library (macOS libicucore, Linux libicui18n, Windows icu.dll shared with Temporal) as primary, embedded CLDR resource as fallback. Shared units in source/shared/ (ICU.pas, BCP47.pas, IntlTypes.pas, IntlICU.pas, IntlCLDRData.pas, IntlLocaleResolver.pas) provide platform-independent Intl infrastructure. Temporal refactored to use shared ICU handle. Existing locale stubs on String.prototype wired to ICU formatters. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
Contributor
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughAdds full ECMA-402 Intl: ICU loader/operations, embedded CLDR generator and resource, BCP47 and locale resolver, TGoccia Intl builtin and value types, String locale methods, Temporal ICU refactor, build/system docs, and tests. ChangesIntl Implementation
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
|
Contributor
Suite TimingTest Runner (interpreted: 9,214 passed; bytecode: 9,214 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: 🟢 187 improved · 🔴 25 regressed · 195 unchanged · avg +4.0% arraybuffer.js — Interp: 🟢 10, 4 unch. · avg +8.8% · Bytecode: 🟢 10, 🔴 1, 3 unch. · avg +3.3%
arrays.js — Interp: 🟢 10, 9 unch. · avg +4.6% · Bytecode: 🟢 18, 1 unch. · avg +9.3%
async-await.js — Interp: 🟢 3, 3 unch. · avg +6.1% · Bytecode: 🟢 3, 3 unch. · avg +2.3%
async-generators.js — Interp: 🟢 1, 1 unch. · avg +5.4% · Bytecode: 🟢 1, 1 unch. · avg +2.8%
base64.js — Interp: 🟢 8, 2 unch. · avg +2.5% · Bytecode: 🟢 2, 🔴 7, 1 unch. · avg -3.8%
classes.js — Interp: 🟢 7, 🔴 5, 19 unch. · avg +0.4% · Bytecode: 🟢 18, 13 unch. · avg +4.3%
closures.js — Interp: 🟢 9, 2 unch. · avg +8.3% · Bytecode: 🟢 9, 2 unch. · avg +6.5%
collections.js — Interp: 🟢 7, 5 unch. · avg +3.3% · Bytecode: 🟢 3, 🔴 6, 3 unch. · avg -0.3%
csv.js — Interp: 🟢 2, 11 unch. · avg +1.5% · Bytecode: 🟢 13 · avg +11.9%
destructuring.js — Interp: 🟢 14, 8 unch. · avg +4.2% · Bytecode: 🟢 8, 14 unch. · avg +2.7%
fibonacci.js — Interp: 🟢 5, 3 unch. · avg +3.8% · Bytecode: 🟢 6, 2 unch. · avg +11.0%
float16array.js — Interp: 🟢 19, 🔴 1, 12 unch. · avg +3.8% · Bytecode: 🟢 11, 🔴 6, 15 unch. · avg -0.8%
for-of.js — Interp: 🟢 5, 2 unch. · avg +3.8% · Bytecode: 🟢 5, 2 unch. · avg +7.1%
generators.js — Interp: 🟢 3, 1 unch. · avg +5.1% · Bytecode: 🟢 3, 1 unch. · avg +4.1%
iterators.js — Interp: 🟢 10, 🔴 6, 26 unch. · avg +1.1% · Bytecode: 🟢 30, 🔴 2, 10 unch. · avg +4.9%
json.js — Interp: 🟢 4, 16 unch. · avg +2.4% · Bytecode: 🟢 20 · avg +10.0%
jsx.jsx — Interp: 🟢 9, 🔴 1, 11 unch. · avg +3.1% · Bytecode: 🟢 16, 5 unch. · avg +7.0%
modules.js — Interp: 🟢 1, 8 unch. · avg +2.3% · Bytecode: 🟢 9 · avg +13.4%
numbers.js — Interp: 🟢 5, 6 unch. · avg +4.1% · Bytecode: 🟢 9, 2 unch. · avg +7.0%
objects.js — Interp: 🟢 6, 1 unch. · avg +6.1% · Bytecode: 🟢 7 · avg +11.7%
promises.js — Interp: 12 unch. · avg +0.4% · Bytecode: 🟢 6, 6 unch. · avg +2.8%
regexp.js — Interp: 🟢 7, 4 unch. · avg +2.5% · Bytecode: 🟢 5, 🔴 6 · avg -2.3%
strings.js — Interp: 🟢 14, 5 unch. · avg +7.1% · Bytecode: 🟢 15, 4 unch. · avg +6.5%
tsv.js — Interp: 🟢 1, 🔴 1, 7 unch. · avg -1.7% · Bytecode: 🟢 8, 1 unch. · avg +10.8%
typed-arrays.js — Interp: 🟢 10, 🔴 4, 8 unch. · avg +12.7% · Bytecode: 🟢 11, 🔴 8, 3 unch. · avg -3.9%
uint8array-encoding.js — Interp: 🟢 3, 🔴 7, 8 unch. · avg -19.3% · Bytecode: 🟢 12, 🔴 3, 3 unch. · avg +6.0%
weak-collections.js — Interp: 🟢 14, 1 unch. · avg +34.3% · Bytecode: 🟢 10, 🔴 3, 2 unch. · avg +28.8%
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 (+527 / -0)Newly passing (527):
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 |
toLocaleUpperCase/toLocaleLowerCase and localeCompare now only route through ICU when a locale argument is explicitly provided. Without an explicit locale, they fall back to the non-locale variants, preserving NUL character handling and avoiding context-sensitive casing differences (e.g. Greek final sigma) that changed observable behavior. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Locale methods (toLocaleUpperCase, toLocaleLowerCase, localeCompare) now always route through ICU using the default locale, matching ECMA-402. NUL character handling fixed by using Move with explicit length instead of PAnsiChar on ICU casemap output. Greek final-sigma test updated to expect the correct Unicode SpecialCasing behavior. DefaultLocale now uses uloc_getDefault for the system locale instead of returning the first available locale alphabetically. supportedLocalesOf added to all 10 Intl constructors. NumberFormat validates required currency/unit options and digit range constraints per spec. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- IntlICU: fix uloc_getDefault missing call parentheses and nil check, fix icsCase collator sensitivity to use UCOL_PRIMARY + UCOL_CASE_LEVEL instead of wrong UCOL_PRIMARY alone, check ucol_setAttribute status - IntlNumberFormat: fix tautological digit range validation that never rejected values > 100 / > 21 - BCP47: reject lone "x" without subtags, reject empty subtags from consecutive hyphens, reject duplicate variants and duplicate extension singletons per RFC 5646, lowercase private-use in NormalizeBcp47Case - ICU: reset Linux UCHandle per loop iteration, unload on finalization - IntlLocaleResolver: BestAvailableLocale singleton-skip loop per ECMA-402 §9.2.2 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Strings containing embedded NUL characters (U+0000) were silently truncated at the NUL boundary when passed to ICU C APIs, causing option values like "base\0cookie" to be accepted as "base". Per ECMA-402, these must throw RangeError. Added ContainsNulCharacter validator to IntlTypes and wired NUL rejection into BCP47 tag parsing, all 10 Intl constructors' option reading, Locale constructor properties, RelativeTimeFormat unit arguments, and supportedLocalesOf localeMatcher handling. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
# Conflicts: # docs/decision-log.md
Tests that assert ICU-specific output (grouping separators, currency symbols, plural rule selection, segmenter decomposition, final sigma) now use test.runIf(hasICU) or conditional expects so they skip cleanly on CI runners where libicu is unavailable. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
On Linux, ICU functions are split across libicui18n.so (formatting) and libicuuc.so (locale utilities). TryGetICULibraryHandle returned only the libicui18n handle, so uloc_* symbols from libicuuc were never found — making ICU appear unavailable on Linux even when both libraries loaded. Added ICUGetProcAddress that resolves from both handles on Linux. Updated IntlICU and Temporal TimeZone to use it. Reverted test.runIf(hasICU) guards — tests now assert unconditionally since ICU will be available on all CI platforms. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…matters
scripts/generate-intl-data.js downloads CLDR 45.0 JSON data from npm,
extracts plural rules, number symbols/patterns, list patterns, relative
time patterns, display names, currency data, likely subtags, and alias
tables for 20 locales, packs into a 1.1 MB binary resource with the same
container format as timezone data, and generates Generated.IntlData.res
via fpcres.
IntlCLDRData.pas now reads the embedded resource on first access and
provides binary-search lookups with locale subtag fallback. Each value
type falls back to CLDR data when ICU is unavailable:
- NumberFormat: locale-specific grouping/decimal/currency/percent symbols
- ListFormat: CLDR conjunction/disjunction/unit templates
- PluralRules: evaluates zero/one/two categories from CLDR rule data
- Segmenter: UTF-8 grapheme/word/sentence splitting via TextSemantics
- RelativeTimeFormat: CLDR future/past templates with {0} substitution
- DisplayNames: CLDR language/territory/script name lookups
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
CLDR resource loading now reads via TResourceStream on each lookup call, matching Temporal's TryGetEmbeddedTimeZoneFile pattern. Removed module- level CachedResource, CachedResourceLoaded, CachedHeader, and EnsureResourceLoaded. Each lookup function reads the resource, parses the header, finds the section, and extracts data in a single call chain. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
IntlCLDRData.pas was in source/shared/ with {$I Shared.inc}, so
GOCCIA_INTL_EMBEDDED_CLDR was never defined and all lookup functions
compiled as stubs returning False. Moved to source/units/ as
Goccia.Intl.CLDRData.pas with {$I Goccia.inc}, exactly matching
Goccia.Temporal.TimeZoneData.pas.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Goccia.Intl.CLDRData was only referenced from IntlLocaleResolver (a shared unit). With production smart linking (-CX -XX), the resource from Generated.IntlData was not pulled into the binary. Adding the unit to Goccia.Builtins.Intl uses clause ensures it participates in the engine compilation chain, matching how Goccia.Temporal.TimeZoneData is referenced from Goccia.Temporal.TimeZone. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
generate-intl-data.js now sorts key-value entries alphabetically before packing, matching the binary search assumption in TryGetKeyValue. Without sorting, keys like "end" and "middle" in list-patterns were unreachable — only keys that happened to land at the binary search midpoint were found. NumberFormat CLDR fallback now maps common currency codes (USD→$, EUR→€, GBP→£, JPY/CNY→¥) to symbols when TryGetCurrencyInfo returns no data, instead of using the raw currency code as the symbol. toLocaleLowerCase test accepts both ς (ICU final-sigma) and σ (non-ICU) since CLDR does not include context-sensitive case mapping rules. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ndle significantDigits validation used > 0 as the guard, allowing 0 through when ECMA-402 requires [1, 21]. Changed to >= 0 with explicit < 1 check. ICU library handle now unloaded in finalization on all platforms, matching the UCHandle cleanup pattern on Linux. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…plate EmbeddedResourceReader.pas: extract HasBytesAvailable, ReadUInt32LE, TryUInt32ToInteger, CopyStringFromBytes from both TimeZoneData and CLDRData into a shared unit — eliminates four duplicate functions. Resource caching: both Temporal TimeZoneData and Intl CLDRData now cache the resource buffer on first read. Temporal reads 723 KB on every timezone lookup; Intl reads 1.1 MB per format() call (4-5 symbol lookups). Caching eliminates ~4.4 MB allocation per format() call. Goccia.Intl.Helpers.pas: extract FormatPartsToArray (was duplicated in NumberFormat and DateTimeFormat) and ReadValidatedStringOption / TryReadStringOption (NUL-validated option reading boilerplate repeated ~47 times across 10 value types). IntlICU EnsureLoaded: check-before-lock guard avoids critical section enter/leave on the hot path after ICU is loaded. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
coderabbitai
Bot
added
documentation
![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
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
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.Temporal.TimeZoneData.pas (1)
231-257:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSynchronize the embedded-resource cache initialization.
CachedTZResourceandCachedTZResourceLoadedare process-wide mutable globals, but this cold path reads and writes them without any synchronization. Two callers can race through the first-load path and publish/read the managedTBytesstate concurrently, which makes the cache nondeterministic once timezone lookups happen from more than one thread.🤖 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.Temporal.TimeZoneData.pas` around lines 231 - 257, The cache access must be synchronized: introduce a global lock (e.g. CachedTZResourceLock: TCriticalSection) and use double-checked locking around CachedTZResource/CachedTZResourceLoaded so only the assignment/publish is protected but the resource I/O is not held under the lock; specifically, before creating TResourceStream check CachedTZResourceLoaded under the lock (or check once without lock then Enter the lock and re-check), if still not loaded perform the Stream read into a local ABuffer, then Enter the lock, re-check CachedTZResourceLoaded, and if still false assign CachedTZResource := ABuffer and set CachedTZResourceLoaded := True, finally Leave the lock and ensure Stream is freed in a try/finally; this ensures safe publication of the managed TBytes and prevents races on CachedTZResource/CachedTZResourceLoaded.
🟠 Major comments (27)
scripts/test262_compatibility_roadmap.json-165-205 (1)
165-205:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd the new Intl statuses to the roadmap allowlist.
These entries now use
implemented/in-progress, buteligibleStatusesstill only admitssupported,active, andcompat. Any consumer that keys off the allowlist will keep treating the newly implemented Intl features as non-eligible until the schema is updated or these entries are mapped back onto an existing eligible status.Also applies to: 1271-1272
🤖 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 `@scripts/test262_compatibility_roadmap.json` around lines 165 - 205, The roadmap's allowlist validation currently only accepts "supported", "active", and "compat" via the eligibleStatuses configuration but new Intl entries use "implemented" and "in-progress"; update the schema/allowlist by adding "implemented" and "in-progress" to eligibleStatuses (or add a mapping layer that treats those values as eligible) so keys such as "Intl.DateTimeFormat", "Intl.DurationFormat", and "Intl.Era-monthcode" are recognized as eligible; ensure the change also covers the duplicate occurrences noted around the other lines (the other mention at lines referenced in the review).scripts/generate-intl-data.js-208-305 (1)
208-305:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftEmbedded fallback data is missing required Intl variants and categories.
The generated payload only includes long-form list patterns, the base relative-time fields, three
DisplayNamescategories, and currency fraction metadata. On builds that rely on embedded CLDR, that leaves no data forIntl.ListFormatshort/narrow,Intl.RelativeTimeFormatshort/narrow,Intl.DisplayNamestypes likecurrency/calendar/dateTimeField, or localized currency symbol/name rendering. That’s a real behavior gap for the ICU-less fallback path this PR is introducing.Also applies to: 401-422
🤖 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 `@scripts/generate-intl-data.js` around lines 208 - 305, The embedded CLDR extraction is incomplete: update extractListPatterns to also pull short and narrow variants (e.g., listPattern-type-short / -narrow) and include their pair/start/middle/end entries; update extractRelativeTimePatterns to extract short and narrow forms (look for relativeTime-type-short and relativeTime-type-narrow in dateFields.json and include their future/past/relative-* entries alongside the long forms); expand extractDisplayNames to return additional categories/types (include currency, calendar, dateTimeField and any types under localeDisplayNames that the runtime needs, not just languages/territories/scripts); and enhance extractCurrencyData to include localized currency names/symbols (pull from cldr-numbers-modern main/*/currencies.json or other currency name sources) so Intl currency display works. Locate these changes in the functions extractListPatterns, extractRelativeTimePatterns, extractDisplayNames, and extractCurrencyData and add the extra variant/category parsing and merge into the returned result objects.source/shared/ICU.pas-27-31 (1)
27-31:⚠️ Potential issue | 🟠 Major | ⚡ Quick winThe Linux ICU version range is too narrow and excludes current production distributions.
This loader checks only ICU versions 70–76. Current distributions ship:
- RHEL 9 (enterprise stable): version 67 — below minimum
- Arch Linux & openSUSE Tumbleweed: version 78 — above maximum
- Debian testing/unstable: version 78 — above maximum
Users on these distros will see ICU unavailable even when it is installed, unless unversioned
libicui18n.so/libicuuc.sosymlinks exist. Widen the range or discover installed sonames dynamically instead of hardcoding the bounds.🤖 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/shared/ICU.pas` around lines 27 - 31, The hardcoded ICU range (ICU_VERSION_MIN / ICU_VERSION_MAX) is too narrow and excludes common distro sonames; update ICU_VERSION_MIN to 67 and ICU_VERSION_MAX to 78 (and keep ICU_I18N_BASE / ICU_UC_BASE as fallbacks) or, preferably, replace the hard bounds with a dynamic discovery: attempt to dlopen the unversioned ICU_I18N_BASE / ICU_UC_BASE first and if that fails iterate available sonames (e.g., try libicui18n.so.78 down to .67) until a successful load; ensure the code that references ICU_VERSION_MIN/ICU_VERSION_MAX or the loader logic uses the new widened range or the dynamic probe instead.source/units/Goccia.Values.StringObjectValue.pas-2009-2027 (1)
2009-2027:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift
localeComparedoes not forward theoptionsparameter to ICU collation settings.The third argument (
optionsobject) is never accessed or parsed. Instead, the ICU call hard-codes sensitivity toicsVariant(quaternary) andignorePunctuationtoFalse. This prevents proper handling of ECMA-402 options like{ sensitivity: "base" },{ ignorePunctuation: true },usage,numeric,caseFirst, andcollation, which should be read from the options parameter and mapped to corresponding ICU collation strengths and settings.🤖 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.StringObjectValue.pas` around lines 2009 - 2027, In TGocciaStringObjectValue.StringLocaleCompare, read and parse the third argument (AArgs.GetElement(2)) when present (and not TGocciaUndefinedLiteralValue) instead of hard-coding icsVariant and False; extract options fields (sensitivity, ignorePunctuation, usage, numeric, caseFirst, collation) from that object, map sensitivity -> ICU strength (e.g. "base"->icsPrimary, "accent"->icsSecondary, "case"->icsTertiary, "variant"->icsVariant), set ignorePunctuation to the corresponding boolean, enable numeric collation if numeric=true, apply caseFirst mapping to ICU case ordering, and pass the resulting strength/flags/locale/collation into TryICUCompareStrings (replace the fixed icsVariant/False values). Locate this logic in StringLocaleCompare and update how ICUResult parameters are constructed before calling TryICUCompareStrings.source/shared/BCP47.pas-294-300 (1)
294-300:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd per-subtag validation for private-use subtags and normalize case in canonicalization.
The parser accepts private-use tails verbatim without validating that each subtag is 1–8 alphanumerics per RFC 5646. Tags like
x-abcdefghi(9 chars) oren-x-abc def(space) would incorrectly validate. Additionally, the canonicalizer returns private-use-only tags without case normalization: line 432 returnsATag.PrivateUsedirectly instead of using the normalized result fromNormalizeBcp47Case, sox-FOO-Barstays uppercase instead of becomingx-foo-bar.Split each private-use tail by hyphens, validate each segment as 1–8 alphanumerics during parsing, store the lowercase form, and ensure the private-use-only branch at line 432 applies case normalization before returning.
Also applies to: 366–375
🤖 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/shared/BCP47.pas` around lines 294 - 300, When handling private-use tags in the parser (the branch that checks IsPrivateUsePrefix(Subtags[0]) and sets Result.PrivateUse/Result.IsValid), split the tail (everything after the 'x' prefix) by '-' and validate each subtag is 1–8 characters and contains only ASCII alphanumerics; if any subtag fails, treat the tag as invalid/Exit; when storing Result.PrivateUse store the normalized lowercase form for each subtag (e.g., join the validated lowercased segments with '-'). Repeat the same per-subtag validation+lowercasing for the other private-use parsing branch referenced in the comment (the similar block around SubtagCount handling). Finally, in the canonicalization/return path that currently returns ATag.PrivateUse directly, return the case-normalized private-use string produced by calling NormalizeBcp47Case (or the same per-subtag lowercased value) so private-use-only tags are returned in lowercase form.source/shared/IntlICU.pas-701-738 (1)
701-738:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftApply number options to the ICU formatter before formatting.
The function receives
TIntlNumberFormatOptionswith 15 fields (Currency, CurrencyDisplay, CurrencySign, UnitIdentifier, UnitDisplay, Notation, CompactDisplay, SignDisplay, UseGrouping, RoundingMode, TrailingZeroDisplay, MinimumIntegerDigits, MinimumFractionDigits, MaximumFractionDigits) but only usesStyle. The remaining options are silently ignored. ICU's legacyunum_*APIs support these settings viaunum_setAttribute(for digit ranges, grouping, sign display) andunum_setTextAttribute(for currency code), which are already loaded inIntlFunctionsbut never called afterUnumOpen.🤖 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/shared/IntlICU.pas` around lines 701 - 738, TryICUFormatNumber currently only sets ICU style and never applies the other TIntlNumberFormatOptions fields; update the function to call the appropriate IntlFunctions UnumSetAttribute and UnumSetTextAttribute APIs after creating Formatter (and before UnumFormatDouble) to apply options such as UseGrouping, MinimumIntegerDigits, MinimumFractionDigits, MaximumFractionDigits, RoundingMode, SignDisplay and any currency/unit text via UnumSetTextAttribute (and numeric flags via UnumSetAttribute); reference the option names (e.g., AOptions.UseGrouping, AOptions.MinimumIntegerDigits, AOptions.MaximumFractionDigits, AOptions.RoundingMode, AOptions.SignDisplay, AOptions.Currency) and the ICU wrapper functions (IntlFunctions.UnumSetAttribute, IntlFunctions.UnumSetTextAttribute) so the formatter honors all provided fields before formatting.source/shared/IntlICU.pas-768-830 (1)
768-830:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftComponent-based
DateTimeFormatoptions are dropped on the ICU path.When
dateStyleandtimeStyleare both unset, this falls back toUDAT_MEDIUMfor both instead of honoring component options likeyear,month,day,hour,minute,second, andtimeZoneName. Calls such as{ year: "numeric" }will therefore format as a medium date+time string rather than the requested field set.To fix this, use the ICU DateTimePatternGenerator API: convert ECMA-402 component options to a skeleton string (e.g.,
year="numeric"→"y",month="short"→"MMM",day="numeric"→"d"), then callgetBestPattern(skeleton)to generate a localized pattern forSimpleDateFormat.🤖 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/shared/IntlICU.pas` around lines 768 - 830, TryICUFormatDateTime currently falls back to UDAT_MEDIUM when both AOptions.DateStyle and AOptions.TimeStyle are unset, losing component-based options (year/month/day/hour/...). Instead, when dateStyle/timeStyle are idtsNone build an ICU skeleton from AOptions component fields (e.g., year="numeric"→"y", month="short"→"MMM", day="numeric"→"d", hour/minute/second and timeZoneName) and use the ICU DateTimePatternGenerator APIs to call getBestPattern(skeleton) to produce a localized SimpleDateFormat pattern, then create the formatter with that pattern (via the IntlFunctions pattern/generator wrappers) and format; update TryICUFormatDateTime to branch: if both styles are unset use the skeleton→getBestPattern path, otherwise keep the existing DateTimeStyleToICU path, and ensure timezone handling (TzPtr/TzLen) is passed into the pattern-based formatter creation and closing (IntlFunctions.*).source/shared/IntlICU.pas-741-752 (1)
741-752:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftImplement semantic field extraction using ICU's FormattedValue APIs for
formatToPartscompliance.Both
TryICUFormatNumberToParts(lines 741–752) andTryICUFormatDateTimeToParts(lines 833–844) collapse the formatted result into a single{ type: "literal", value: ... }part, losing all semantic information (integer, group, currency, year, month, etc.). ECMA-402formatToPartsimplementations require field-by-field semantic breakdown.ICU exposes this capability through modern FormattedValue APIs (
FormattedNumber,FormattedDateTime) vianextPosition(ConstrainedFieldPosition)iteration (available since ICU 64.0). The current codebase only binds legacy ICU C functions (unum_formatDouble,udat_format) withFieldPosition, which are insufficient for complete field extraction.To fix: Add Pascal bindings for modern ICU formatting APIs and iterate field positions to populate the semantic part types.
🤖 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/shared/IntlICU.pas` around lines 741 - 752, The current TryICUFormatNumberToParts and TryICUFormatDateTimeToParts return a single "literal" part; replace this with real field extraction by adding Pascal bindings for ICU's modern FormattedValue APIs (FormattedNumber / FormattedDateTime and ConstrainedFieldPosition / nextPosition) and using them to iterate semantic field ranges; for TryICUFormatNumberToParts call the FormattedNumber formatter, loop nextPosition to get each field name and substring, map ICU field identifiers to our TIntlFormatPartArray.PartType values (integer, group, fraction, currency, etc.), and append parts with correct Value and PartType; do the analogous binding+iteration for TryICUFormatDateTimeToParts to extract year/month/day/hour/minute/second and literal separators instead of collapsing into one literal.tests/built-ins/Intl/nul-character-validation.js-8-157 (1)
8-157:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd per-constructor guards for optional Intl APIs.
This suite only checks
typeof Intl !== "undefined"but directly references optional constructors likeIntl.DisplayNames,Intl.Segmenter,Intl.DurationFormat,Intl.ListFormat,Intl.RelativeTimeFormat, andIntl.Locale. These are not guaranteed to exist on all Intl-supporting runtimes. Other Intl test files in this repository use per-constructor guards (e.g.,describe.runIf(isIntl && typeof Intl.DisplayNames !== "undefined")) for exactly these optional APIs. Without similar guards, this file will crash with a runtime error before reaching the NUL-validation assertions if any optional constructor is missing. Split tests into per-constructor guarded blocks to skip cleanly instead of failing the entire suite.🤖 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/nul-character-validation.js` around lines 8 - 157, The test suite currently only checks isIntl but directly calls optional APIs (Intl.DisplayNames, Intl.Segmenter, Intl.DurationFormat, Intl.ListFormat, Intl.RelativeTimeFormat, Intl.Locale and methods like Intl.Collator.supportedLocalesOf and Intl.getCanonicalLocales) which can be undefined; split or wrap the tests that reference each optional constructor/method in per-constructor guards using describe.runIf(isIntl && typeof Intl.DisplayNames !== "undefined") (and similarly for Intl.Segmenter, Intl.DurationFormat, Intl.ListFormat, Intl.RelativeTimeFormat, Intl.Locale) and guard calls to Intl.Collator.supportedLocalesOf and Intl.getCanonicalLocales with typeof checks so each group is skipped if the specific API is missing rather than crashing the whole suite.source/units/Goccia.Values.IntlCollator.pas-111-124 (1)
111-124:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftForward numeric, caseFirst, collation, and usage options to the comparison function.
These options are parsed from the constructor (lines 111-124) and returned in
resolvedOptions()(lines 232-237), but the two compare paths—BoundCompare(lines 198-199) andIntlCollatorCompare(lines 217-218)—only pass sensitivity and ignorePunctuation toTryICUCompareStrings. This means numeric, caseFirst, collation, and usage have no effect on sort order, making calls like{ numeric: true }or{ caseFirst: "upper" }observably incorrect and non-compliant with ECMA-402.🤖 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.IntlCollator.pas` around lines 111 - 124, The compare paths are only passing FSensitivity and FIgnorePunctuation to TryICUCompareStrings, so FNumeric, FCaseFirst, FCollation and FUsage are never used; update the calls in BoundCompare and IntlCollatorCompare to forward FNumeric, FCaseFirst, FCollation and FUsage along with FSensitivity and FIgnorePunctuation to TryICUCompareStrings, and if necessary extend TryICUCompareStrings' signature/implementation to accept and apply these four options when invoking ICU string comparison; reference the parsed fields FSensitivity, FIgnorePunctuation, FNumeric, FCaseFirst, FCollation and FUsage and the functions BoundCompare, IntlCollatorCompare, and TryICUCompareStrings.source/units/Goccia.Values.IntlNumberFormat.pas-247-254 (1)
247-254:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCLDR fallback depends on the process decimal separator.
FormatFloatuses the currentFormatSettings, but the code later searches for'.'unconditionally. On hosts whose decimal separator is',', the fallback misparses fractional output when ICU is unavailable.Based on learnings: locale-independent numeric formatting sites in this repo must pin
FS.DecimalSeparator := '.'right afterFS := DefaultFormatSettings.🤖 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 247 - 254, The code builds RawStr via FormatFloat(FormatSpec, AbsValue) but then looks for '.' unconditionally (DotPos := Pos('.', RawStr)), which fails when the current DefaultFormatSettings.DecimalSeparator is ','; to fix, create and use a locale-independent FormatSettings instance (e.g. FS := DefaultFormatSettings; FS.DecimalSeparator := '.'); pass that FS into FormatFloat (use FormatFloat(FormatSpec, AbsValue, FS)) so RawStr always uses '.' and the subsequent DotPos/FracPart/IntPart parsing works correctly; update usages in IntlNumberFormat-related functions (FormatSpec, RawStr, DotPos handling) accordingly.source/units/Goccia.Values.IntlNumberFormat.pas-271-272 (1)
271-272:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
useGroupingis stored but never applied in the fallback formatter.The CLDR path always inserts grouping separators, so
resolvedOptions().useGroupingcan disagree withformat()output on non-ICU builds.🤖 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 271 - 272, The fallback formatter always applies grouping by calling InsertGroupingSeparator(IntPart, GroupSep), causing resolvedOptions().useGrouping to be inaccurate; update the formatting logic (the routine that builds IntPart — e.g., where IntPart := InsertGroupingSeparator(IntPart, GroupSep) is called) to only insert GroupSep when the instance's useGrouping flag is true (check the formatter's stored useGrouping property) and ensure resolvedOptions().useGrouping reflects that same flag so format() output and resolvedOptions() remain consistent.source/units/Goccia.Values.IntlListFormat.pas-92-105 (1)
92-105:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftOnly arrays are consumed; other iterables format as an empty list.
ExtractStringArrayreturns[]for every non-TGocciaArrayValue, soSet, generators, and custom iterables silently format as''instead of going through the spec’s iterable consumption path.Also applies to: 280-287
🤖 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.IntlListFormat.pas` around lines 92 - 105, The ExtractStringArray function currently treats only TGocciaArrayValue as iterable and returns an empty array for other iterable types; modify ExtractStringArray (and the duplicate logic around the other occurrence) to detect and consume any iterable TGocciaValue (e.g., types implementing the iterable interface or a TGocciaIterableValue) by enumerating its elements rather than only TGocciaArrayValue, converting each element to its string literal (Element.ToStringLiteral.Value) and building the TStringArray from that enumeration so Sets, generators and custom iterables are handled the same as arrays.source/units/Goccia.Builtins.Intl.pas-644-665 (1)
644-665:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
supportedLocalesOf()becomes empty on CLDR-only builds.The available-locale set is sourced only from ICU. If ICU loading fails but the embedded CLDR fallback still powers the constructors, every
supportedLocalesOf()call returns[], which no longer matches actual Intl support.🤖 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.Builtins.Intl.pas` around lines 644 - 665, The code only populates AvailableTags when TryICUGetAvailableLocales succeeds, so when ICU fails supportedLocalesOf returns empty even though CLDR-backed constructors (Canonical) exist; change the logic in the block around TryICUGetAvailableLocales/AvailableTags so that if TryICUGetAvailableLocales returns false you populate AvailableTags from the embedded CLDR canonical list (use the existing Canonical array or a CLDR fallback source, canonicalizing as needed with TryICUCanonicalizeLocale or equivalent) before calling BestAvailableLocale; ensure AvailableTags ends up containing CLDR-derived tags so BestAvailableLocale(Canonical[I]) can find matches when ICU is absent.source/units/Goccia.Values.IntlListFormat.pas-311-324 (1)
311-324:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift
formatToParts()hardcodes comma literals instead of the locale pattern.This builder discards the actual ICU/CLDR formatting pattern and always emits
', 'between elements. That makesformatToParts()disagree withformat()for conjunction/disjunction and for locales whose list separators are not commas.🤖 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.IntlListFormat.pas` around lines 311 - 324, The loop in formatToParts currently inserts a hardcoded ', ' literal (creating TGocciaStringLiteralValue with ', ') which breaks locale-specific list formatting; replace that hardcoded literal with the locale-derived separator/pattern from the list formatter used by Intl.ListFormat (do not create a constant ', '). Locate the list formatting code (the method implementing formatToParts / the class that builds parts) and, instead of inserting TGocciaStringLiteralValue.Create(', '), call the existing list/ICU pattern helper (or add a small helper) that returns the correct literal for the current position (e.g., GetListLiteralForPosition or the formatter instance used by format()) and pass that string into TGocciaStringLiteralValue.Create; keep creating the 'literal' PartObj but use the locale pattern value so formatToParts matches format() for conjunctions/disjunctions and non-comma locales.source/units/Goccia.Values.IntlPluralRules.pas-262-271 (1)
262-271:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift
selectRange()currently ignores the start category.This path computes
StartCat, then discards it and always returnsEndCat. Plural-range resolution is locale-specific, so ranges will be misclassified whenever ICU range support is unavailable.🤖 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.IntlPluralRules.pas` around lines 262 - 271, selectRange currently computes StartCat then ignores it and always returns EndCat; update selectRange to perform ECMA-402 plural-range resolution when ICU range support is unavailable by combining StartCat and EndCat instead of returning EndCat directly. Specifically, after obtaining StartCat and EndCat via TryICUSelectPlural/SelectPluralWithCLDR, call or implement a plural-range resolution helper (e.g., SelectPluralRangeWithCLDR or similar) that takes StartCat, EndCat and PR.FLocale (and PluralTypeStringToEnum(PR.FType) as needed) and use its returned category when creating the TGocciaStringLiteralValue result instead of always using EndCat. Ensure symbols referenced: selectRange, TryICUSelectPlural, SelectPluralWithCLDR, StartCat, EndCat, TGocciaStringLiteralValue.Create.source/units/Goccia.Values.IntlDisplayNames.pas-72-85 (1)
72-85:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUnknown
typevalues silently fall back to language names.
DisplayTypeStringToEnumdefaults toidntLanguage, and the constructor never rejects unsupportedtypestrings. Invalid input should throw, not succeed and route lookups through the language code path.Also applies to: 121-126
🤖 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.IntlDisplayNames.pas` around lines 72 - 85, DisplayTypeStringToEnum currently maps any unknown AValue to idntLanguage, allowing invalid "type" strings to silently succeed; change DisplayTypeStringToEnum to raise an exception (e.g. raise EArgumentException.CreateFmt(...)) when AValue is not one of the known tokens ('region','script','calendar','currency','dateTimeField','language'), and update the TIntlDisplayNames constructor (the Create method that accepts the type string) to call DisplayTypeStringToEnum and propagate the exception so unsupported type strings are rejected at construction; also apply the same validation/error behavior to the similar conversion routine found around lines 121-126 so no unknown values fall back silently.source/units/Goccia.Values.IntlRelativeTimeFormat.pas-106-142 (1)
106-142:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftFallback formatting ignores both
numericandstyle.The CLDR path only keys on
locale+unit, so when ICU is unavailablenumeric: 'auto'never produces lexical forms andstyle: 'short'/'narrow'still formats like the default long style.resolvedOptions()reports these options, but the output cannot reflect them.Also applies to: 260-265
🤖 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 106 - 142, FormatRelativeTimeWithCLDR currently only looks up CLDR patterns by locale+unit so it ignores numeric and style; update the lookup flow so FormatRelativeTimeWithCLDR (and TryGetRelativeTimePattern) first attempt to retrieve a pattern keyed by locale+unit+style+numeric (e.g., long/short/narrow and numeric=auto/always), if that fails then for numeric='auto' try lexical forms (yesterday/today/tomorrow) before falling back, and if style is short/narrow try those pattern variants (Pattern.Short / Pattern.Narrow or equivalent) before using Pattern.Long; ensure you pass the style and numeric flags into TryGetRelativeTimePattern (or add parameters) and only fallback to FloatToStr(AValue) after all style/numeric-aware lookups are exhausted.source/units/Goccia.Values.IntlDateTimeFormat.pas-259-299 (1)
259-299:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftThe non-ICU path does not implement
DateTimeFormatbehavior.When ICU formatting is unavailable,
format()returns raw milliseconds,formatToParts()returns[], andformatRange()just concatenates the two numbers. That breaks the advertised CLDR fallback path on platforms without ICU.🤖 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.IntlDateTimeFormat.pas` around lines 259 - 299, The three methods IntlDateTimeFormatFormat, IntlDateTimeFormatFormatToParts, and IntlDateTimeFormatFormatRange currently return raw millis/empty arrays/concatenated numbers when TryICUFormatDateTime* fails; instead implement the CLDR fallback path: when TryICUFormatDateTime fails call the library's CLDR-based formatter (using DTF.FResolvedOptions and DTF.FLocale) to produce the locale-aware formatted string in IntlDateTimeFormatFormat, produce the correct sequence of parts in IntlDateTimeFormatFormatToParts (use the same part-generation routine used by the ICU path or a new CLDR parts generator and then return FormatPartsToArray(Parts)), and implement FormatRange to follow the Range formatting algorithm (format each end via the CLDR fallback and apply the range bridging rules) rather than concatenating numbers; update code around TryICUFormatDateTime, TryICUFormatDateTimeToParts, FormatPartsToArray, and the constructors TGocciaStringLiteralValue/TGocciaArrayValue to return the proper locale-aware outputs.source/units/Goccia.Intl.CLDRData.pas-482-553 (1)
482-553:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd parent-locale fallback when resolving plural rules.
This path only probes exact keys like
ALocale + '/one'. In the embedded CLDR fallback, locale variants that inherit plural rules from a parent locale will returnFalseeven when the parent data exists, soPluralRulesbecomes unnecessarily unsupported for tags likeen-USorpt-BR.🤖 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.CLDRData.pas` around lines 482 - 553, TryGetPluralRules currently only queries exact keys for ALocale (e.g., ALocale + '/one'), so implement a parent-locale fallback loop inside TryGetPluralRules: set a localeToTry variable initialized to ALocale and in a loop attempt TryGetKeyValue(Resource, DataOffset, DataLength, localeToTry + '/zero'|'one'|'two'|'few'|'many'|'other') to populate ARules; if any category is found set FoundAny and break out of the loop; if nothing is found and localeToTry contains a '-' remove the last subtag (e.g., change 'en-US' -> 'en') and retry, stopping when no '-' remains (or you reach a defined root). Keep using TryGetSectionData and TryGetKeyValue as the lookup functions and preserve existing ARules assignment logic when a parent provides the rules.source/units/Goccia.Intl.CLDRData.pas-576-581 (1)
576-581:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftImplement the embedded date-pattern lookup before merging.
TryGetDatePatternis part of the public fallback surface but currently always returnsFalse. That means anyDateTimeFormatpath that lands on embedded CLDR cannot obtain patterns at all.🤖 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.CLDRData.pas` around lines 576 - 581, TryGetDatePattern currently always returns False; implement it to look up the embedded CLDR date/time pattern for ALocale given ADateStyle and ATimeStyle, set APattern to the found pattern and return True, otherwise return False. Map the TIntlDateTimeStyle combination to the CLDR pattern key(s) used in this unit, search the embedded CLDR data/fallbacks for ALocale (including parent/fallback locales), populate APattern with the matched pattern string and return True; if no pattern is found leave APattern empty and return False. Ensure the implementation uses the same locale lookup/fallback logic as other functions in this unit so the public fallback surface behaves consistently.source/units/Goccia.Values.IntlDurationFormat.pas-144-199 (1)
144-199:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftThe
*Displayoptions are parsed but never affect formatting.
ReadOptions()storesyearsDisplay/monthsDisplay/… andresolvedOptions()exposes them, but neitherformat()norformatToParts()ever reads those fields. Right now instances with{ hoursDisplay: 'always' }and{ hoursDisplay: 'auto' }format identically, so the object advertises option support it does not implement.Also applies to: 369-408
🤖 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.IntlDurationFormat.pas` around lines 144 - 199, FormatDurationFallback currently ignores the per-unit display options (yearsDisplay, monthsDisplay, etc.) that ReadOptions/resolvedOptions parsed; update FormatDurationFallback (and its nested AddPart) to consult the corresponding display option for each unit and only skip zero-valued units when the unit's display option is not 'always' (i.e., if display is 'always' include zeros, if 'auto' or unspecified skip zeros), by passing the unit's display setting into AddPart (or reading it from a supplied options record) and using that to determine whether to exit on AValue = 0; apply the same change to the analogous code segment at lines ~369-408 so format() / formatToParts() honor the stored yearsDisplay/monthsDisplay/... options.source/units/Goccia.Values.IntlLocale.pas-101-138 (1)
101-138:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftRecompute
FTagandFBaseNamefrom the resolved locale components.
FBaseNameis frozen toCanonicalbefore extensions are stripped, and the later option overrides only mutateFLanguage/FRegion/FCalendar/… without rebuilding the serialized tag. That leaves the object internally inconsistent: e.g.baseNamecan still include-u-...extensions, andtoString()can keep returning the pre-override tag while the getters expose the overridden components.🤖 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.IntlLocale.pas` around lines 101 - 138, The object leaves FTag/FBaseName unchanged after option overrides, causing mismatch between serialized tag and component getters; after applying ReadValidatedStringOption changes (affecting FLanguage, FScript, FRegion, FCalendar, FCaseFirst, FCollation, FHourCycle, FNumberingSystem, FNumeric) you must rebuild the locale identifier and base name from those resolved components: assemble the core subtags (FLanguage, FScript, FRegion, any variants) into a baseName without Unicode extensions, assemble a -u- extension chunk from calendar/caseFirst/collation/hourCycle/numberingSystem/numeric when present, concatenate them, then pass the result through CanonicalizeUnicodeLocaleId (or the existing BCP47/CANONICAL helper) and assign the result to both FBaseName and FTag so toString()/getters are consistent with the overrides; update the code around ParseBcp47Tag, ReadValidatedStringOption and the option-handling block to perform this rebuild after options are applied.source/units/Goccia.Values.IntlDurationFormat.pas-221-260 (1)
221-260:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject unsupported option values instead of storing them verbatim.
FStyleand the per-unit display fields accept any string here, but the formatter only special-cases'digital'. A typo like{ style: 'digitial' }leaves the instance in an invalid state,resolvedOptions()echoes that invalid value, and formatting silently falls back to the generic text path.🤖 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.IntlDurationFormat.pas` around lines 221 - 260, The code currently stores any string values for FStyle and per-unit display fields (FYearsDisplay, FMonthsDisplay, FWeeksDisplay, FDaysDisplay, FHoursDisplay, FMinutesDisplay, FSecondsDisplay, FMillisecondsDisplay, FMicrosecondsDisplay, FNanosecondsDisplay, and FNumberingSystem) directly from AOptions, which allows invalid typos to persist; instead, validate each incoming option against the allowed set for that option (per the Intl.DurationFormat spec) and reject unknown values by calling ThrowRangeError (use the existing SErrorIntlInvalidOption message format) rather than assigning them; reuse the same pattern already used for FStyle’s nul check and use the GetProperty/ToStringLiteral extraction you have, but add an allowed-values lookup (or case/if checks) for each property before assigning to the corresponding field.source/units/Goccia.Values.IntlSegmenter.pas-146-153 (1)
146-153:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPopulate
inputwith the original segmented string, not the current segment.Every segment object currently reports
input === segment, so callers lose the original source text entirely. That breakssegment().containing(...)and iterator results for any multi-segment input.Possible fix
-function CreateSegmentObject(const ASeg: TIntlSegment; const AIsWordGranularity: Boolean): TGocciaObjectValue; +function CreateSegmentObject(const ASeg: TIntlSegment; const AInput: string; + const AIsWordGranularity: Boolean): TGocciaObjectValue; begin Result := TGocciaObjectValue.Create(TGocciaObjectValue.SharedObjectPrototype); Result.AssignProperty('segment', TGocciaStringLiteralValue.Create(ASeg.Segment)); Result.AssignProperty('index', TGocciaNumberLiteralValue.Create(ASeg.Index)); - Result.AssignProperty('input', TGocciaStringLiteralValue.Create(ASeg.Segment)); + Result.AssignProperty('input', TGocciaStringLiteralValue.Create(AInput)); if AIsWordGranularity then Result.AssignProperty('isWordLike', TGocciaBooleanLiteralValue.Create(ASeg.IsWordLike)); end;🤖 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.IntlSegmenter.pas` around lines 146 - 153, The segment object currently sets its 'input' property to ASeg.Segment (the single segment) — change CreateSegmentObject so 'input' contains the original source string instead: replace the AssignProperty call that creates 'input' using ASeg.Segment with one that uses the ASeg property that holds the full original input (e.g. ASeg.Input or ASeg.OriginalText depending on the TIntlSegment field name), i.e. Result.AssignProperty('input', TGocciaStringLiteralValue.Create(ASeg.Input)); thereby preserving the original segmented string for methods like segment().containing and iterators.source/units/Goccia.Values.IntlSegmenter.pas-324-336 (1)
324-336:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFree the temporary iterator in
containing().
Iteris class-typed here, so it will not be auto-released. EachSegments.prototype.containing()call currently leaks aTGocciaIntlSegmentIteratorValue.Possible fix
Iter := TGocciaIntlSegmentIteratorValue.Create(Segs.FLocale, Segs.FGranularity, UnicodeString(Segs.FText)); - for I := 0 to Length(Iter.FSegments) - 1 do - begin - Seg := Iter.FSegments[I]; - if (Idx >= Seg.Index) and (Idx < Seg.Index + Length(Seg.Segment)) then + try + for I := 0 to Length(Iter.FSegments) - 1 do begin - Result := CreateSegmentObject(Seg, Segs.FGranularity = 'word'); - Exit; + Seg := Iter.FSegments[I]; + if (Idx >= Seg.Index) and (Idx < Seg.Index + Length(Seg.Segment)) then + begin + Result := CreateSegmentObject(Seg, Segs.FText, Segs.FGranularity = 'word'); + Exit; + end; end; + finally + Iter.Free; end;Based on learnings: class-typed owners in this codebase are not auto-released and need explicit cleanup via
try..finally+Free.🤖 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.IntlSegmenter.pas` around lines 324 - 336, The temporary TGocciaIntlSegmentIteratorValue instance (Iter) created in containing() is not freed and leaks; wrap the creation and loop in a try..finally and call Iter.Free in the finally block so Iter is always released, keeping the same logic that uses Iter.FSegments and CreateSegmentObject and returning TGocciaUndefinedLiteralValue.UndefinedValue when not found.source/units/Goccia.Values.IntlSegmenter.pas-551-556 (1)
551-556:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
[Symbol.iterator]on the iterator prototype must return the iterator itself, not callnext().Currently,
[Symbol.iterator]is bound toIntlSegmentIteratorNext, which returns a{ value, done }record and advances the iterator. This violates the iterator protocol—iterator[Symbol.iterator]()should return the iterator itself to maintain self-iterable behavior.Fix
+function TGocciaIntlSegmentIteratorValue.IntlSegmentIteratorSymbolIterator( + const AArgs: TGocciaArgumentsCollection; const AThisValue: TGocciaValue): TGocciaValue; +begin + if not (AThisValue is TGocciaIntlSegmentIteratorValue) then + ThrowTypeError('Segment Iterator [Symbol.iterator] called on non-iterator'); + Result := AThisValue; +end; + Members.AddSymbolMethod( TGocciaSymbolValue.WellKnownIterator, '[Symbol.iterator]', - IntlSegmentIteratorNext, + IntlSegmentIteratorSymbolIterator, 0, [pfConfigurable, pfWritable]);🤖 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.IntlSegmenter.pas` around lines 551 - 556, The [Symbol.iterator] binding on the iterator prototype is wrong: it currently points to IntlSegmentIteratorNext (which advances and returns {value, done}) but must instead return the iterator itself; replace the AddSymbolMethod call that binds TGocciaSymbolValue.WellKnownIterator/'[Symbol.iterator]' to IntlSegmentIteratorNext with a new method (e.g. IntlSegmentIteratorSymbolIterator) that simply returns the iterator object (this/self) without advancing, and register that new method with the same attributes (0 args, [pfConfigurable, pfWritable]); update references to TGocciaSymbolValue.WellKnownIterator/'[Symbol.iterator]' to use the new function name so the iterator is self-iterable.
🟡 Minor comments (3)
tests/built-ins/Intl/Collator/constructor.js-8-8 (1)
8-8:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winGuard this suite on
Intl.Collatoravailability too.Line 8 only checks
Intl; in partial-Intl environments this can hard-fail instead of skipping.Suggested patch
- describe.runIf(isIntl)("Intl.Collator constructor", () => { + describe.runIf(isIntl && typeof Intl.Collator !== "undefined")("Intl.Collator constructor", () => {🤖 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/Collator/constructor.js` at line 8, The test suite is only guarded by isIntl and can fail in partial-Intl environments; update the guard used on describe.runIf to ensure Intl.Collator exists (e.g., require isIntl && typeof Intl.Collator === "function" or a small helper like isIntlCollator) so the "Intl.Collator constructor" suite is skipped when Collator is unavailable; modify the call at describe.runIf(isIntl) to use this combined predicate referencing Intl.Collator.tests/built-ins/Intl/NumberFormat/prototype/format.js-8-8 (1)
8-8:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winGate this suite on
Intl.NumberFormatexistence.Line 8 currently gates only on
Intl; ifNumberFormatis missing, the suite fails instead of being skipped.Suggested patch
- describe.runIf(isIntl)("Intl.NumberFormat.prototype.format", () => { + describe.runIf(isIntl && typeof Intl.NumberFormat !== "undefined")("Intl.NumberFormat.prototype.format", () => {🤖 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/NumberFormat/prototype/format.js` at line 8, The suite is gated only by isIntl but should also require Intl.NumberFormat; update the runIf predicate on the describe (the describe.runIf(...) call wrapping "Intl.NumberFormat.prototype.format") to check both Intl and NumberFormat existence (e.g., use isIntl && typeof Intl.NumberFormat === "function" or a small helper like hasIntlNumberFormat) so the suite is skipped when Intl.NumberFormat is missing rather than run and fail.source/units/Goccia.Values.IntlCollator.pas-193-196 (1)
193-196:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove the arity check that rejects fewer than 2 arguments.
ECMA-402 specifies that
Intl.Collator.prototype.compareshould convert arguments to strings viaToString(), with missing arguments defaulting toundefined(which converts to the string"undefined"). The explicit check at lines 193–196 and 212–215 that throwsTypeErrorfor fewer than 2 arguments violates this specification. Remove these checks and allow arguments to pass through the normal string-conversion path so thatcollator.compare("a")succeeds rather than failing.🤖 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.IntlCollator.pas` around lines 193 - 196, Remove the explicit arity check that throws via ThrowTypeError when AArgs.Length < 2 in Intl.Collator.prototype.compare; instead allow the normal ToString conversion path for missing args. Concretely, delete the conditional that checks AArgs.Length and the ThrowTypeError call, and keep the subsequent calls that obtain values via AArgs.GetElement(0).ToStringLiteral.Value and AArgs.GetElement(1).ToStringLiteral.Value so missing elements are converted to "undefined" per ToString semantics (ensure Str1 and Str2 are produced solely from those GetElement(...)→ToStringLiteral.Value calls). Also remove the equivalent arity-check/ThrowTypeError elsewhere in the same compare implementation so both checks no longer reject fewer-than-2-argument calls.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 48f9c0ac-9de4-435c-8741-ab9dae208180
⛔ Files ignored due to path filters (2)
source/generated/Generated.IntlData.pasis excluded by!**/generated/**source/generated/Generated.IntlData.resis excluded by!**/generated/**
📒 Files selected for processing (54)
docs/build-system.mddocs/built-ins-intl.mddocs/built-ins.mddocs/decision-log.mddocs/language-tables.mddocs/test262.mdscripts/generate-intl-data.jsscripts/test262_compatibility_roadmap.jsonsource/shared/BCP47.passource/shared/EmbeddedResourceReader.passource/shared/ICU.passource/shared/IntlICU.passource/shared/IntlLocaleResolver.passource/shared/IntlTypes.passource/units/Goccia.Builtins.Intl.passource/units/Goccia.Builtins.TestingLibrary.passource/units/Goccia.Engine.passource/units/Goccia.Error.Messages.passource/units/Goccia.Intl.CLDRData.passource/units/Goccia.Intl.Helpers.passource/units/Goccia.Temporal.TimeZone.passource/units/Goccia.Temporal.TimeZoneData.passource/units/Goccia.Values.IntlCollator.passource/units/Goccia.Values.IntlDateTimeFormat.passource/units/Goccia.Values.IntlDisplayNames.passource/units/Goccia.Values.IntlDurationFormat.passource/units/Goccia.Values.IntlListFormat.passource/units/Goccia.Values.IntlLocale.passource/units/Goccia.Values.IntlNumberFormat.passource/units/Goccia.Values.IntlPluralRules.passource/units/Goccia.Values.IntlRelativeTimeFormat.passource/units/Goccia.Values.IntlSegmenter.passource/units/Goccia.Values.StringObjectValue.passource/units/Goccia.inctests/built-ins/Intl/Collator/constructor.jstests/built-ins/Intl/Collator/prototype/compare.jstests/built-ins/Intl/DateTimeFormat/constructor.jstests/built-ins/Intl/DisplayNames/constructor.jstests/built-ins/Intl/DurationFormat/constructor.jstests/built-ins/Intl/ListFormat/constructor.jstests/built-ins/Intl/ListFormat/prototype/format.jstests/built-ins/Intl/Locale/constructor.jstests/built-ins/Intl/NumberFormat/constructor.jstests/built-ins/Intl/NumberFormat/prototype/format.jstests/built-ins/Intl/PluralRules/constructor.jstests/built-ins/Intl/PluralRules/prototype/select.jstests/built-ins/Intl/RelativeTimeFormat/constructor.jstests/built-ins/Intl/Segmenter/constructor.jstests/built-ins/Intl/Segmenter/prototype/segment.jstests/built-ins/Intl/getCanonicalLocales.jstests/built-ins/Intl/nul-character-validation.jstests/built-ins/Intl/supportedValuesOf.jstests/built-ins/Intl/toStringTag.jstests/built-ins/String/prototype/toLocaleLowerCase.js
Comment on lines
+1105
to
+1139
| function TryICUFormatRelativeTime(const ALocale: string; AValue: Double; | ||
| AUnit: TIntlRelativeTimeUnit; ANumeric: TIntlRelativeTimeNumeric; | ||
| out AFormatted: string): Boolean; | ||
| var | ||
| Status: TICUErrorCode; | ||
| Reldatefmt: Pointer; | ||
| Buffer: array[0..FORMAT_BUFFER_CAPACITY - 1] of WideChar; | ||
| ResultLen: LongInt; | ||
| LocaleAnsi: AnsiString; | ||
| begin | ||
| Result := False; | ||
| AFormatted := ''; | ||
|
|
||
| if not EnsureLoaded then | ||
| Exit; | ||
|
|
||
| if not Assigned(IntlFunctions.UreldatefmtOpen) or | ||
| not Assigned(IntlFunctions.UreldatefmtClose) or | ||
| not Assigned(IntlFunctions.UreldatefmtFormatNumeric) then | ||
| Exit; | ||
|
|
||
| LocaleAnsi := AnsiString(ALocale); | ||
| Status := ICU_SUCCESS; | ||
| Reldatefmt := IntlFunctions.UreldatefmtOpen(PAnsiChar(LocaleAnsi), | ||
| nil, URELDATEFMT_STYLE_LONG, 0, Status); | ||
| if not ICUSucceeded(Status) or not Assigned(Reldatefmt) then | ||
| Exit; | ||
|
|
||
| try | ||
| FillChar(Buffer, SizeOf(Buffer), 0); | ||
| Status := ICU_SUCCESS; | ||
| ResultLen := IntlFunctions.UreldatefmtFormatNumeric(Reldatefmt, | ||
| AValue, RelativeTimeUnitToICU(AUnit), | ||
| @Buffer[0], FORMAT_BUFFER_CAPACITY, Status); | ||
| if not ICUSucceeded(Status) or (ResultLen <= 0) then |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Which ICU relative-time API should be used for numeric: "auto" values such as "yesterday"/"tomorrow", and how are short/narrow widths selected?
💡 Result:
The ICU API for handling relative-time formatting with numeric: "auto" values like "yesterday" and "tomorrow" is RelativeDateTimeFormatter.[1][2][3][4] This class provides methods to format relative dates using text styles when possible (e.g., format(1.0, RelativeUnit.DAYS) produces "tomorrow"), falling back to numeric styles otherwise, which corresponds to the "auto" behavior in Intl.RelativeTimeFormat.[1][3] For short and narrow widths, use the Style parameter when constructing the formatter: RelativeDateTimeFormatter.Style.SHORT (abbreviations when possible, e.g., "3 sec. ago") or Style.NARROW (single letters when possible, e.g., "3s ago"). Pass the desired Style to factory methods like getInstance(locale, style).[4][5][3] These styles apply to both numeric patterns and absolute relative units like yesterday/tomorrow, with data sourced from CLDR relativeTimePatterns distinguished by style (long/short/narrow).[6][3]
Citations:
- 1: https://unicode-org.github.io/icu-docs/apidoc/released/icu4j/com/ibm/icu/text/RelativeDateTimeFormatter.html
- 2: https://unicode-org.github.io/icu-docs/apidoc/dev/icu4c/classicu_1_1RelativeDateTimeFormatter.html
- 3: https://unicode-org.github.io/icu-docs/apidoc/dev/icu4j/com/ibm/icu/text/RelativeDateTimeFormatter.html
- 4: https://developer.android.com/reference/android/icu/text/RelativeDateTimeFormatter.Style
- 5: https://unicode-org.github.io/icu-docs/apidoc/released/icu4j/com/ibm/icu/text/RelativeDateTimeFormatter.Style.html
- 6: https://unicode-org.github.io/cldr/ldml/tr35-dates.html
🏁 Script executed:
# Check the full function implementation to see if ANumeric is used
fd "IntlICU.pas" --type f -x cat -n {} | sed -n '1105,1139p'Repository: frostney/GocciaScript
Length of output: 1454
🏁 Script executed:
# Search for ANumeric usage in the entire function and related code
rg "ANumeric" source/shared/IntlICU.pas -B 5 -A 15Repository: frostney/GocciaScript
Length of output: 1246
🏁 Script executed:
# Check for other relative time formatting functions in the file
rg "TryICUFormatRelativeTime|UreldatefmtFormat|Ureldatefmt" source/shared/IntlICU.pas -A 2Repository: frostney/GocciaScript
Length of output: 2049
🏁 Script executed:
# Check the enum/constant definitions for RelativeTimeNumeric and styles
rg "TIntlRelativeTimeNumeric|RelativeTimeNumeric|URELDATEFMT_STYLE" source/shared/IntlICU.pas -B 2 -A 5Repository: frostney/GocciaScript
Length of output: 1240
🏁 Script executed:
# Search for any references to non-numeric relative time formatting
rg "ureldatefmt_format[^N]|Ureldatefmt(?!Numeric)" source/shared/IntlICU.pasRepository: frostney/GocciaScript
Length of output: 348
🏁 Script executed:
# Check the entire variable/constant declarations for relative time
rg "TIntlRelativeTimeNumeric|relativeTime|Relative" source/shared/IntlICU.pas -B 2 -A 2Repository: frostney/GocciaScript
Length of output: 822
🏁 Script executed:
# Search for ureldatefmt function declarations more broadly
rg "Ureldatefmt" source/shared/IntlICU.pas | head -20Repository: frostney/GocciaScript
Length of output: 946
Use ureldatefmt_format() instead of ureldatefmt_formatNumeric() to support the numeric parameter and enable text output like "yesterday".
The current implementation calls ureldatefmt_formatNumeric(), which always produces numeric output regardless of the numeric parameter setting (e.g., "in 1 day" instead of "tomorrow"). To support numeric: "auto" (which produces text for common relative units), add a binding for ureldatefmt_format() and use it to pass the ANumeric parameter as the numeric mode (URELDATEFMT_NUMERIC_AUTO or URELDATEFMT_NUMERIC_ALWAYS).
Additionally, the width style is hard-coded to URELDATEFMT_STYLE_LONG in the ureldatefmt_open() call. To support short/narrow styles, parameterize the width based on a style input parameter.
🤖 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/shared/IntlICU.pas` around lines 1105 - 1139, The code currently calls
IntlFunctions.UreldatefmtFormatNumeric which forces numeric output; change the
implementation of TryICUFormatRelativeTime to call a new binding
IntlFunctions.UreldatefmtFormat (binding must be added) and pass the ANumeric
value mapped to ICU numeric modes (URELDATEFMT_NUMERIC_AUTO /
URELDATEFMT_NUMERIC_ALWAYS) so the numeric parameter controls auto/text vs
numeric output; also stop hard-coding URELDATEFMT_STYLE_LONG in the
UreldatefmtOpen call—add a style parameter (or map an existing style input) and
pass that into IntlFunctions.UreldatefmtOpen instead of URELDATEFMT_STYLE_LONG.
Reference the existing symbols TryICUFormatRelativeTime,
IntlFunctions.UreldatefmtOpen, IntlFunctions.UreldatefmtFormatNumeric,
IntlFunctions.UreldatefmtFormat and RelativeTimeUnitToICU when making these
changes.
Owner
Author
There was a problem hiding this comment.
Tracked in #599. The ureldatefmt_formatNumeric vs ureldatefmt_format gap is a known incomplete feature — the current implementation always uses numeric output. Adding ureldatefmt_format for numeric: "auto" and parameterizing the width style are part of the RelativeTimeFormat conformance work.
Contributor
There was a problem hiding this comment.
@frostney, understood — thanks for the context. Acknowledged that the ureldatefmt_format / numeric: "auto" gap and the width-style parameterization are intentionally deferred conformance work tracked in #599. I'll avoid re-flagging these in future reviews.
✏️ Learnings added
Learnt from: frostney
Repo: frostney/GocciaScript PR: 590
File: source/shared/IntlICU.pas:1105-1139
Timestamp: 2026-05-10T13:54:57.343Z
Learning: In frostney/GocciaScript, `TryICUFormatRelativeTime` in `source/shared/IntlICU.pas` intentionally always calls `ureldatefmt_formatNumeric` (always numeric output) and hard-codes `URELDATEFMT_STYLE_LONG`. Supporting `numeric: "auto"` (via `ureldatefmt_format`) and parameterizing the width style are known gaps tracked in issue `#599` (RelativeTimeFormat conformance follow-up). Do not flag these as bugs during review of this file.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: frostney
Repo: frostney/GocciaScript PR: 53
File: souffle/Souffle.Wasm.Emitter.pas:1467-1477
Timestamp: 2026-03-09T17:21:38.193Z
Learning: In this codebase (Frostney/GocciaScript), for Pascal/Delphi (FPC) files using AnsiString with {$mode delphi} {H+}, treat String as byte-indexed. Length(str) returns byte count and Byte(str[I]) is a no-op cast. Do not replace per-byte copy loops like Data[I-1] := Byte(AStr[I]) with TEncoding.GetBytes, since TEncoding is a Delphi RTL feature unavailable in FPC's SysUtils and would break the build. Apply this guidance to all .pas files under the project; ensure manual per-byte copying logic remains correct for UTF-8 content encoded in AnsiString, and avoid introducing RTL dependencies not supported by FPC.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 63
File: units/Goccia.Runtime.Operations.pas:0-0
Timestamp: 2026-03-11T01:50:11.071Z
Learning: Replace hardcoded string literals 'now', 'toJSON', and 'timeOrigin' with the new constants from Goccia.Constants.PropertyNames (PROP_NOW, PROP_TO_JSON, PROP_TIME_ORIGIN). Apply this refactor across the repository wherever these literals are used, including but not limited to units/Goccia.Runtime.Operations.pas and units/Goccia.Builtins.Performance.pas. Use a project-wide search (e.g., regex or code navigation) to identify all occurrences, update to the respective constants, and adjust imports or uses accordingly. Ensure all references compile and behavior remains unchanged.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 110
File: units/Goccia.Runtime.Operations.pas:0-0
Timestamp: 2026-03-23T11:25:00.381Z
Learning: In this repo’s GocciaScript Pascal code, when implementing thenable detection/callability checks (e.g., around Await/unwrap logic), always read the thenable’s `then` property using `Goccia.Constants.PropertyNames.PROP_THEN` rather than a string literal. If the value being inspected is a `TSouffleRecord`, unwrap the retrieved `then` property via `UnwrapToGocciaValue` before testing whether it is callable (e.g., in `TGocciaRuntimeOperations.AwaitValue`).
Learnt from: frostney
Repo: frostney/GocciaScript PR: 200
File: REPL.dpr:0-0
Timestamp: 2026-04-07T17:12:07.324Z
Learning: When reviewing timing/metrics code in the GocciaScript repo (e.g., REPL.dpr, ScriptLoader, TestRunner, BenchmarkRunner, and TGocciaEngine.Execute), follow the established timing convention: capture `StartTime` before the JSX pre-pass (`TGocciaJSXTransformer.Transform`) and `StringListToLFText`, then capture `LexEnd` after `ScanTokens`. The `Lex` duration (`LexEnd - StartTime`) is intended to include the JSX pre-pass because it is treated as part of the source→tokens pipeline. Do not flag this ordering as a bug and do not recommend moving `StartTime` to after the JSX transform, as it would break the bucket semantics.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 209
File: units/Goccia.FFI.DynamicLibrary.pas:7-21
Timestamp: 2026-04-08T13:56:59.262Z
Learning: In Free Pascal (FPC), `TLibHandle` is defined in the `System` unit (`TLibHandle = PtrInt`), not in `Dynlibs`. Since `System` is implicitly available in all Pascal units, referencing `TLibHandle` in the interface section (e.g., as a private field type of a class declared in the interface/`interface` section) does not require `Dynlibs` to be listed in the interface `uses` clause. Only ensure `Dynlibs` is included in the implementation `uses` clause when you actually use `LoadLibrary`, `GetProcAddress`, or `FreeLibrary`. Therefore, do not flag a missing `Dynlibs` entry in the interface `uses` clause when the interface section references only `TLibHandle`.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 242
File: AGENTS.md:396-400
Timestamp: 2026-04-09T11:50:01.452Z
Learning: In .pas code, avoid unsafe Int64→Double conversions/workarounds that trigger FPC 3.2.2 miscompilations. Specifically, do not rely on `Double(Int64Var)` when it performs bit reinterpretation (Delphi-mode front-end bug on affected versions/platforms). Also do not use arithmetic patterns like `Int64Var * 1.0` / `Int64Var * Double` that can produce incorrect results near ±2^31 on AArch64. Prefer value-safe conversions via implicit assignment (e.g., `var D: Double; D := SomeInt64`) or implicit promotion through function/constructor parameter passing (e.g., `TGocciaNumberLiteralValue.Create(SomeInt64)`).
Learnt from: frostney
Repo: frostney/GocciaScript PR: 253
File: units/Goccia.Values.Iterator.Concat.pas:84-88
Timestamp: 2026-04-10T12:44:59.067Z
Learning: In frostney/GocciaScript (GocciaScript), `TGocciaFunctionBase(callable).Call(args, thisValue)` is the project-standard way to invoke callable values. During code review, do not flag `TGocciaFunctionBase(...)` casts at call sites as unsafe and do not recommend replacing them with an alternative “generic callable path”—for this codebase, this cast-and-`Call` pattern is the intended generic callable invocation mechanism.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 285
File: units/Goccia.Lexer.pas:207-213
Timestamp: 2026-04-12T15:46:28.683Z
Learning: In this Frostney/GocciaScript project (Free Pascal / FPC, including files under units/), do not recommend adding explicit `{$B-}` directives (either inside units or in `Goccia.inc`) to “fix” short-circuit evaluation safety. `{$B-}` is already FPC’s default in all compiler modes (`fpc`, `delphi`, `objfpc`). Treat missing `{$B-}` as expected and do not flag it as a maintainability or safety issue. Only note `{$B-}` overrides if an explicit `{$B+}` is present in the file or via a highly visible project-level `fpc.cfg` setting.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 296
File: units/Goccia.REPL.Formatter.pas:15-29
Timestamp: 2026-04-13T16:53:02.191Z
Learning: For Pascal units in frostney/GocciaScript, enforce `uses`-clause grouping order as follows (with blank lines between groups): (1) System units (e.g., `SysUtils`), (2) Third-party units, then (3) Project units (e.g., `Goccia.*`). Specifically, `StringBuffer` (from `StringBuffer.pas`) must be treated as a Third-party unit and placed in the Third-party group. Do not report a `uses` ordering violation when `StringBuffer` appears between the System group and the `Goccia.*` group, since `./format.pas` and the Lefthook pre-commit hook enforce this grouping.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 302
File: docs/contributing/code-style.md:31-37
Timestamp: 2026-04-14T13:36:54.107Z
Learning: When reviewing Free Pascal (FPC) sources in this project, do not flag the compiler directive `{H+}` as a syntax error or as an inconsistency caused by a missing `$`. In FPC, `{H+}` (short-form) is valid and equivalent to `{$H+}` for enabling long/AnsiStrings. This project’s own `Goccia.inc` and documentation intentionally use `{H+}`, so the reviewer should only consider it an issue if it’s not valid FPC syntax for the surrounding context.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 308
File: units/Goccia.VM.pas:0-0
Timestamp: 2026-04-15T15:41:55.194Z
Learning: In this repo’s Pascal code, when handling “assigning to a read-only” errors, use the exact suggestion type `SSuggestReadOnlyProperty`, paired with `SErrorCannotAssignReadOnly`. Keep this mapping consistent everywhere it applies (e.g., for `ObjectValue`, `ClassValue`, and similar value/descriptor assignments), so the same read-only assignment diagnostic always yields the same suggestion/supported error combination.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 333
File: source/shared/OrderedMap.Test.pas:387-401
Timestamp: 2026-04-18T03:14:07.382Z
Learning: For Free Pascal (FPC) in Delphi mode (`{$mode delphi}`) used in this repo (Pascal `.pas` files), do not treat a bare indexed-property read used as a standalone statement (e.g., `Map[999];`) as a compile error. In this mode, the getter call is executed and its result is silently discarded; if the getter raises an exception, it still propagates to the surrounding `except` handler as expected. For readability/portability, assigning the result to a dummy local (e.g., `Dummy := Map[999];`) is preferred, but bare indexed-property reads should remain unflagged as long as they are valid under FPC Delphi mode.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 402
File: source/units/Goccia.Values.TemporalDuration.pas:176-178
Timestamp: 2026-04-25T22:50:52.352Z
Learning: In this Free Pascal (FPC) codebase (GocciaScript), do not use `Abs(Int64Var)` in range-guard comparisons when the input could be `Low(Int64)` (-2^63), because `Abs(Low(Int64))` overflows in FPC and wraps back to a negative value, causing range checks to silently pass. Instead, use explicit signed-bound comparisons, e.g. `(AVal <= -Int64(BOUND)) or (AVal >= BOUND)`. When forming `-BOUND` and `BOUND` might be implicitly typed narrower than `Int64`, cast to `Int64` before negation (e.g., `-Int64(BOUND)`) so the negation stays in range. Apply this pattern everywhere you see `Abs(Int64)` used inside a comparison guard.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 402
File: source/units/Goccia.Values.ArrayValue.pas:755-767
Timestamp: 2026-04-26T14:02:20.855Z
Learning: In frostney/GocciaScript, do not re-flag calls to `Trunc()` for missing finite-number guards (`IsNaN`/`IsInfinity`) when the code runs under `TGocciaEngine.Create` where `exInvalidOp` is masked (engine `Create`). Under this masking, `Trunc()` on `NaN` or `±Infinity` is safe and yields IEEE indeterminate `Low(Int64)` instead of raising `EInvalidOp`. Also, rely on the existing guard pattern `Len <> Trunc(Len)` to catch non-finite inputs: `NaN <> <anything>` evaluates to `true`, and `±Infinity <> Low(Int64)` evaluates to `true`, which triggers `ThrowRangeError`. Therefore, in this codebase, `Trunc()` calls are considered already protected when paired with the `Len <> Trunc(Len)`-style check; only flag missing guards if that combined protection (or equivalent non-finite handling) is not present.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 415
File: source/units/Goccia.Values.Primitives.pas:242-306
Timestamp: 2026-04-27T11:24:27.552Z
Learning: In the frostney/GocciaScript codebase (Free Pascal 3.2.2), `FormatSettings`/`DefaultFormatSettings` is declared as `Absolute DefaultFormatSettings` and therefore shares the same memory. For every new `TFormatSettings` construction site that creates a locale-independent formatter (i.e., any code that sets up `TFormatSettings` for JSON/TOML/YAML/etc. parsing/serialization), immediately pin `FS.DecimalSeparator := '.'` right after `FS := DefaultFormatSettings`. This pin is required for consistency: `Str()` is a compiler intrinsic that always uses `'.'`, but `TryStrToFloat(..., FS)` and `FloatToStr(..., FS)` use the provided `FS` record—so you must explicitly set `FS.DecimalSeparator` rather than relying on defaults. Do not treat this assignment as redundant; enforce it at every `TFormatSettings` usage site that relies on locale-independent decimal parsing/formatting.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 471
File: source/units/Goccia.VM.pas:0-0
Timestamp: 2026-04-30T12:40:36.331Z
Learning: When implementing iterator-to-array (or similar consumption) in Pascal units, wrap both consumption paths (e.g., typed iterator values and any raw iterator object) in try/except so that on abrupt completion you close iterators using CloseIteratorPreservingError / CloseRawIteratorPreservingError. After closing, re-raise the original error so the thrown exception matches ES §7.4.10 step 5 semantics (i.e., close must not replace the original error).
Learnt from: frostney
Repo: frostney/GocciaScript PR: 493
File: source/units/Goccia.ScriptLoader.Globals.Test.pas:159-172
Timestamp: 2026-05-02T00:42:01.543Z
Learning: When reviewing code that uses `TGocciaRuntime` in `frostney/GocciaScript`, account for its engine-ownership semantics. The default `TGocciaRuntime.Create(AEngine, ARuntimeGlobals)` overload is **non-owning**: the caller retains ownership of `AEngine` and may (and must) free it separately after `TGocciaRuntime` is freed. Treat a “double-free” as a false positive if the code calls the non-owning overload and frees both `Runtime` and `Engine` independently. Engine ownership is transferred only when an overload explicitly includes `AOwnsEngine: Boolean` with `True` (e.g., `Create(AEngine, AOwnsEngine)` or `Create(AEngine, ARuntimeGlobals, AOwnsEngine)` where `AOwnsEngine=True`); in that case, the caller must not free `AEngine` separately.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 490
File: source/units/Goccia.VM.pas:4867-4939
Timestamp: 2026-05-02T12:47:39.953Z
Learning: In frostney/GocciaScript (Goccia VM), all VM private-key helper APIs must be called with an explicit brand token to prevent cross-class private-name aliasing. Use the tokenized forms only:
- Call `NormalizeBytecodePrivateKey(AName, PrivateBrandToken)` and `BytecodePrivateBrandKey(AKey, PrivateBrandToken)`.
- Derive `PrivateBrandToken` via `BytecodePrivateTokenForKey(AKey, Fallback)` or from the owning class/receiver value (e.g., `AClassValue.PrivateBrandToken` / `BytecodePrivateReceiverBrandToken`).
Tokenless forms were removed (commit 99994d2c), so any call sites passing only keys/names without the brand token should be rejected during review.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 506
File: source/app/GocciaScriptLoaderBare.dpr:0-0
Timestamp: 2026-05-03T18:43:29.776Z
Learning: In the GocciaScript codebase (Free Pascal/Delphi), follow the canonical “acquire-then-protect” pattern for resource management: (1) assign the result of a constructor/creation call to the resource variable first, then (2) immediately open the surrounding `try..finally`, and only call `Resource.Free` in the `finally`. Don’t require a pre-nil guard like `Resource := nil` *when* the `try` block opens only after the assignment has completed. Rationale: if the constructor raises, execution skips the inner `try..finally` that contains the `Free`, so `Resource.Free` is never reached before the variable has been assigned. Only add pre-nil/initialization when the `try` block is entered before the assignment (i.e., `try` wraps the assignment).
Learnt from: frostney
Repo: frostney/GocciaScript PR: 527
File: source/units/Goccia.Values.FunctionValue.Test.pas:110-123
Timestamp: 2026-05-05T23:39:55.120Z
Learning: In frostney/GocciaScript's Pascal tests, TGocciaThrowValue carries only Value: TGocciaValue and has no Kind or TGocciaThrowValueKind. To assert a thrown error is a TypeError, read E.Value as TGocciaObjectValue, call GetProperty(PROP_NAME) to obtain the error's name, cast to TGocciaStringLiteralValue, and compare .Value against TYPE_ERROR_NAME. Do not use E.Kind or similar enum-based checks since they do not exist on TGocciaThrowValue.
WindowsICU.Handle now comes from the shared ICU.pas loader, which already frees the library in its own finalization. Removed the redundant UnloadLibrary call that would double-release the DLL. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ocale safety Segmenter: segment objects now carry the full original input string instead of the individual segment text. Iterator [Symbol.iterator] returns self instead of calling next(). containing() frees the temporary iterator via try/finally. DisplayNames: unknown type strings now throw RangeError instead of silently defaulting to language lookups. NumberFormat CLDR fallback: uses locale-invariant FormatSettings for FormatFloat so decimal separator is always '.' regardless of host locale. Respects useGrouping option — skips grouping when false. Collator: compare no longer rejects <2 arguments. Missing args convert to "undefined" per ECMA-402 ToString semantics. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
4 tasks
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.Locale,Intl.Collator,Intl.NumberFormat,Intl.DateTimeFormat,Intl.PluralRules,Intl.RelativeTimeFormat,Intl.ListFormat,Intl.DisplayNames,Intl.Segmenter,Intl.DurationFormat, plusIntl.getCanonicalLocales,Intl.supportedValuesOf, andsupportedLocalesOfon all constructors.libicucore, Linuxlibicui18n, Windowsicu.dllshared with Temporal) as primary, embedded CLDR resource as fallback. Cross-platform ICU handle extracted tosource/shared/ICU.pas— Temporal refactored to use the shared handle instead of private DLL loading.source/shared/(ICU.pas,BCP47.pas,IntlTypes.pas,IntlICU.pas,IntlCLDRData.pas,IntlLocaleResolver.pas) provide platform-independent Intl infrastructure with pure Pascal types. Engine units insource/units/(Goccia.Builtins.Intl.pas+ 10Goccia.Values.Intl*.pasvalue types) bridge to the GocciaScript object model.String.prototype(localeCompare,toLocaleUpperCase,toLocaleLowerCase) wired to ICU — always uses locale-aware casing with NUL-safe output via explicit-lengthMoveinstead ofPAnsiChar.supportedLocalesOfper ECMA-402 (fixes test262staging/sm/extensions/quote-string-for-nul-character.js).xwithout subtags, and NUL characters.currency/unitoptions and digit range constraints per spec.UCOL_CASE_LEVELfor case sensitivity.GOCCIA_INTL_EMBEDDED_CLDR/GOCCIA_INTL_NO_EMBEDDED_CLDRfollows the exact Temporal pattern.Testing
docs/built-ins-intl.mdwith full API referencedocs/built-ins.md,docs/build-system.md,docs/language-tables.md,docs/test262.md,docs/decision-log.mdscripts/test262_compatibility_roadmap.json— Intl status changed fromexcluded-by-language-designtoimplemented