[FIX] ICU plural evaluation in the Rust formatter, and the swift-computed-property detector - #420
Open
justin13888 wants to merge 10 commits into
Open
Conversation
The per-platform renderers never needed a plural-rule table: `xtask i18n` compiles an ICU plural into an Apple String Catalog variation or an Android `<plurals>`, and the platform's own CLDR data picks the arm at display time. The Rust runtime has no platform underneath it, which is why `format_message` refuses a plural outright instead of evaluating one. `capsule_i18n::plural` is the missing half: `Category`, `category(locale, n)` and `selectable(locale)` over the twelve language subtags of `locales/config.json`. Integer cardinals only — every plural in `locales/` selects on a count and `Value` has no decimal variant, so the CLDR operands reduce to `n = i`, `v = 0` and each rule is arithmetic on the absolute value. Rules and their selectable sets live in one row per language so a test can assert the pair is consistent; `xtask`'s second copy of the selectable table is re-derived from this one in a later commit. `selectable` returns `None` for an unknown language, which is what lets the generator keep failing loudly on a locale it has no rules for while the runtime falls back to `other`. The table is asserted cell by cell rather than derived, across the CLDR boundary counts, plus the properties that matter downstream: selection never leaves the selectable set, a region or script subtag resolves to its language, and negative counts use the absolute value (`i64::MIN` included).
Deploying capsule with
|
| Latest commit: |
8c191fb
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://9a57130d.capsule-22k.pages.dev |
| Branch Preview URL: | https://fix-i18n-plurals-and-swift-d.capsule-22k.pages.dev |
`format_message` substituted `{identifier}` and refused everything else: a
plural hit a `debug_assert!` and, in release, was copied through verbatim — the
user reading the message source. That refusal was the cheap half of `S-I7`; this
is the half it owed.
`{name, plural, …}` now evaluates, with `=N` exact arms, CLDR category arms, `#`
for the count, and nesting — an arm may hold `{name}` placeholders and further
plurals. Arm selection comes from `capsule_i18n::plural`, so the arm chosen is
the one the locale's rules select, and a category the message does not carry
falls back to `other`. That fallback is what makes the shipped catalogs render
at all: every translated plural is still an English `one`/`other` copy, so a
Russian `few` has nowhere else to go.
`Bundle::format` dropped `self.locale` before calling the formatter, which is
the API gap plural selection had to close. `format_message_in(locale, …)` is
new; `format_message` keeps its signature and means English, for a template that
came from nowhere in particular.
The refusal is narrowed, not removed. `select`, `selectordinal`, `offset:`, a
malformed plural, and nesting past 32 levels keep the assertion and the
verbatim pass-through, so the next construct this runtime cannot express is a
test failure rather than something a user reads. The two tests that pinned the
refusal are retargeted onto `select` rather than deleted — and the debug one
gains the `cfg(debug_assertions)` it always needed, without which
`cargo test --release` failed on it.
Three defects found reviewing this change before committing it:
- A stray `{` used to abandon the rest of the template, so one unbalanced brace
silently blanked every later argument. It is now one character of output and
scanning continues.
- Recursion was bounded only by the input. A public formatter that aborts the
process on a deeply nested template is not acceptable, so 32 levels is the
limit and past it is a refusal.
- A string count was trimmed for selection but not for `#`, so `" 1 "` could
choose one arm and print another. Both now read the same value.
The whole-catalog matrix asserts the acceptance property directly: every plural
in every one of the thirteen locales, at eight boundary counts, renders with no
braces, no `plural,`, and the count wherever the arm spells `#` — including the
keys a locale has not translated, which reach the reader through the source
catalog under the reader's own rules.
`xtask` held its own copy of which categories each language can select. It predates the runtime having any rules at all, and the moment the runtime gained them the two tables had to agree about thirteen locales — the shape that had already produced this slice's bug once. `android_plural_arms` and its test now read `capsule_i18n::plural::selectable`. The generator keeps failing loudly on a language with no rules, which is why `selectable` answers `None` rather than "only `other`": the runtime is happy to fall back, the generator must not guess. Two tests close the loop the merge opens. `the_selectable_set_is_exactly_what_selection_can_produce` pins both directions of every row: a category listed but unreachable is a dead `<item quantity=…>` the generator would emit, and a category reachable but unlisted is one it would drop and the runtime would then ask for. Russian is the interesting row — it never selects `other` for an integer, since its `other` is for fractions, yet every plural resource must carry the arm. `the_runtime_renders_what_the_android_resource_carries` is the agreement nothing checked before: over the real catalogs, in every locale, at the CLDR boundary counts, the sentence the runtime builds by substituting `#` equals the sentence Android builds from the `%d` format string the generator lowered — 1048 comparisons, in Android's own escaped spelling so `android_escape` is compared against itself.
…mple The detector's doc comment gives `case .places: "Places"` as the shape it was added to catch. Its literal pattern required a capital followed by a space, so that exact string — and every other single-word display string — could not be caught (#394). The space was there to keep `case .heic: "HEIC"` out; requiring a **lowercase letter** in position two does the same job and lets one capitalized word through. The trade is that prose whose second character is neither lowercase nor a space ("E-mail sent", "AI Insights") stops being caught. Measured across `capsule-swift/{App,Modules}` in every position this detector scans: zero strings are lost by the swap, one is gained. Four blind spots #394 lists close with it: - Only `case .foo:` inside the body was scanned. A literal returned explicitly, returned implicitly, or held as a dictionary value was invisible. All four shapes are scanned, keyed by absolute offset so an overlap is one finding. - Six property-name stems matched; `heading`, `text`, `summary` and `prompt` now do too. `value` deliberately still does not — `var rawValue: String` returns an identifier two dozen times in `CapsuleDomain`. - `var` was required, so `func hdrName(_:) -> String` was outside the gate. - `confirmationDialog` was watched by the interpolation regex and not the literal one, and `help`, `searchable`, `accessibilityValue`, `ContentUnavailableView` and `tabItem` by neither. Both regexes now build from one shared list, so they cannot disagree again. The widening finds exactly one string in the tree: "Dolby Vision", in `AssetInfoFormatting.hdrName(_:)`, whose own doc says it spells an HDR encoding "as its owner spells it". It is a trademark, spelled identically in every locale — the allowlist's stated category, and the same reason its sibling arms "HDR10" and "HLG" never tripped the gate. One allowlist line with its justification; no Swift source is touched.
The design doc said the runtime "currently handles literal text and `{name}`
interpolation" and listed plurals as follow-up. Both stopped being true when
`S-I7` landed plural evaluation, and a confident wrong answer in this document
is exactly what let Android ship raw ICU for as long as it did.
The runtime section now names the supported subset, the CLDR table behind arm
selection and the fact that `xtask` reads the same one, the `other` fallback and
why it is load-bearing today, and — spelled out rather than implied — what is
still refused: `select`, `selectordinal`, `offset:`, number and date skeletons,
and ICU apostrophe quoting, which no target implements. The codegen table's Rust
row and the future-work bullet follow.
`SLICES.md` records the second half of `S-I7` as landed, why the plural rules
are an in-house table rather than a crate, that `xtask`'s copy was the second
one, and the three non-plural defects the pre-commit review of the formatter
found. It also records one owed-CI item: the tests that pin release-build
behaviour are `cfg(not(debug_assertions))` and no CI job runs
`cargo test --release`, so they never execute.
`locales/README.md` tells a translator the one thing that changed for them:
every target evaluates a `plural` block now, and the constructs that still fail
the build.
Reviewing the previous commit's own diff: the new dictionary-value pattern
matched any `label: "Some text"` followed by a comma, which reads the
`defaultValue:` of `String(localized:defaultValue:comment:)` as a dictionary
entry. That argument is the English source text the ICU arguments hang off — the
migrated shape, deliberately never captured — so the guard would have failed a
call site that is correct.
The key must now begin with `.`, which is what a dictionary key in this codebase
looks like and what an argument label never does. Measured across
`capsule-swift/{App,Modules}`: the finding set is unchanged, still exactly
"Dolby Vision".
Also records the `func` form's own limit: the signature is read with
`\([^)]*\)`, so a parameter list containing a closure type is not matched. A
blind spot, but a narrowing one — it can never produce a false positive.
`render` asked `matching_brace` at every `{`, and `matching_brace` scans forward
from the brace it was given. On the matched path that is fine — it stops at the
partner. On the **unmatched** path it has to read the whole remainder to learn
there is no partner, and then the next unmatched brace reads the same remainder
again.
Measured against a copy of the shipped scan: bytes read are exactly n(n+1)/2,
and a template of n unmatched braces takes 22 ms at n=10 000, 104 ms at 20 000,
369 ms at 40 000 and 1.63 s at 80 000 — quadruples per doubling. `format_message`
is a `pub` entry point, so this is the same threat model `MAX_DEPTH` was capped
for two commits ago: a template that need not come from a catalog.
`brace_pairs` now pairs every brace in one stack pass, and `render` walks a
cursor over the result in source order, skipping the entries the recursive render
of a placeholder body consumed. `matching_brace` stays for `Arms::parse`, where
the arms are disjoint so its scans add up to a single pass, and its doc says why
the two coexist.
An unmatched brace was the case that could not be shortcut cheaply: "if pairing
fails here it fails later too" is false — in `{ {a} ` the outer brace has no
partner and the inner one does — so the fix has to be the real pairing, not a
watermark. That case is in the equivalence test.
Two pins, and they count bytes rather than watching a clock: a wall-clock budget
on this host would be either flaky or too loose to prove anything, while a
thread-local counter incremented by both scanners is deterministic and fails the
moment a per-brace rescan returns. Measured after: 100 000 bytes scanned for a
100 000-byte unmatched template (was ~5 x 10^9), and 118 000 for a 101 000-byte
template of 10 000 placeholders and 1 000 plurals.
Also makes the module doc and the refusal message say which malformed plural
does what: a plural whose arms do not parse is passed through verbatim, while one
whose arms parse but carry no `other` asserts and renders its first arm in CLDR
order. Those are different behaviours and the docs called both "malformed".
Two records described the formatter's fallback behaviour as a verbatim
pass-through, which is right for most of the refused set and wrong for the two
cases that matter most, because they are the ones a reader could actually hit.
- A plural whose arms parse but carry **no `other`** asserts and renders its
first arm in CLDR order. It is not passed through. `xtask i18n` refuses to emit
such a message, so it is unreachable from the catalogs; for a hand-written
template that slipped past the generator, some text beats showing the reader
ICU source. Both records now say so, and `i18n.md` gains the case at all.
- An unterminated `{` is **not** an assertion case. A lone brace in literal copy
("50% off {sale") cannot be told apart from a mistyped placeholder, and
asserting on it would panic every debug run over legitimate text. It is emitted
as an ordinary character and scanning continues past it.
Also corrects the owed-CI count in `S-I7`: three tests pin release-build
behaviour, not two, and names them and the issue (#428).
…list `swift_watches_every_api_position_in_both_regexes` exercised four of the fifteen entries in `SWIFT_TEXT_POSITIONS`. `searchable` and `tabItem` had no fixture and no call site anywhere in the tree, so a broken alternative in the shared list — a stray space, an empty alternative, a regex metacharacter — would have been silent in both regexes it is spliced into. Two derived tests replace the gap. `every_watched_api_position_is_a_bare _identifier` rejects an entry that is not a plain identifier, and rejects a duplicate. `every_watched_api_position_is_caught_in_both_regexes` builds its fixture *from* the list — one plain literal and one interpolated literal per entry, plus a helper name that merely ends in the entry — so a position added without a case cannot happen, which is the failure #394 records for `confirmationDialog`. Verified non-vacuous: appending `inspector Prompt` to the list fails the identifier test with that entry named. The hand-written test stays, and says why: a derived fixture proves each alternative matches, and the concrete one proves the real call shapes do, with the arguments SwiftUI actually puts after the string (`ContentUnavailableView("…", systemImage: …)`).
5 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.
Description
Two halves of one issue (#414). The Rust i18n runtime could not evaluate an ICU
plural — it refused one with a
debug_assert!and, in release, copied themessage source through to the user. And the
swift-computed-propertydetectorcould not catch the example its own doc comment gives as the shape it exists to
catch (#394).
Summary
Plural evaluation (
S-I7, the half it owed).capsule_i18n::plural(new): CLDR integer cardinal rules for the twelvelanguage subtags in
locales/config.json—Category,category(locale, n),selectable(locale). No new dependency;icu_pluralswould have meant aprovider, a data crate and a
dependencies.mdrow on a crate whose wholedependency list is
serde_json+tracing.format_message_in(locale, template, args)evaluates{name, plural, …}with=Nexact arms, CLDR category arms,#, and nesting.format_messagekeepsits signature and means English.
Bundle::formatwas droppingself.localebefore calling the formatter — that was the API gap, and it is closed.
other. That isload-bearing today: every translated plural is still an English
one/othercopy, so a Russian
fewhas nowhere else to go.select,selectordinal,offset:, a malformed plural, and nesting past 32 levels keep the assertionand the verbatim pass-through, so the next construct this runtime cannot
express is a test failure rather than something a user reads.
xtaskheld a second copy of the selectable-category table. It now readscapsule_i18n::plural::selectable, and two tests close the loop: the rules andthe selectable set are pinned equal in both directions, and — over the real
catalogs, in every locale, at eight boundary counts — the sentence the runtime
builds by substituting
#is asserted equal to the sentence Android buildsfrom the
%dformat string the generator lowered.The detector (#394).
single-word display string,
case .places: "Places"included. It is now "acapital, then a lowercase letter", which keeps
"HEIC","HDR10","HLG"and"key.fill"out just as well. Measured acrosscapsule-swift/{App,Modules}inevery position the detector scans: zero strings lost by the swap, one
gained.
only
case .foo:(explicit return, implicit return, dictionary value), fourmore name stems (
heading,text,summary,prompt— notvalue, which israwValue),func … -> Stringas well asvar, and six API positions thatwere in one regex or neither. Both Swift regexes now build from one shared
position list, so
confirmationDialogbeing in one and not the other cannotrecur.
"Dolby Vision", inAssetInfoFormatting.hdrName(_:), whose own doc says it names an HDR encoding"as its owner spells it". A trademark, spelled identically in every locale —
the allowlist's stated category, and why its siblings
"HDR10"and"HLG"never tripped the gate. One allowlist line. No file under
capsule-swift/is touched.
Validation
Run in the worktree, in this order.
cargo nextest run -p capsule-i18ncargo test -p capsule-i18n --releasef433d918):an_unrenderable_icu_construct_is_refused_in_debug_buildswas#[should_panic]with nocfg(debug_assertions), so it asserted a panic a release build cannot produce. Classifiedpre-existing, fixed here.cargo nextest run -p xtaskmise run i18n-guardmise run i18n-checkcheck-rust) — no generated file drifts; this change renders nothingmise run check-docsbun installincapsule-docs/first;node_modulesis absent from a fresh worktreemise run check-docs-truthmise run lint-check-mdmise run check-rustmise run test-rustcapsule-core --features ffi) + 160 (capsule-sdk --features ffi), 0 failed. Exit 0Adversarial read of the formatter before it was committed (one focused
sub-agent, differential and fuzz harness against the previous implementation)
found six things worth acting on. Three were defects, all fixed in the same
commit:
{abandoned the rest of the template, so onestray brace silently blanked every later argument — behaviour the previous
scanner did not have. It is now one character of output and scanning
continues.
stack overflow, which no caller can catch, through apubentry point.Capped at 32.
#did not, so" 1 "could select one arm and print another.The other three were test-quality: two tautological assertions and one
tautological test (
format_message(x) == format_message_in("en", x), which isthe definition of
format_message), all replaced with assertions that can fail.It also confirmed no memory-safety, UTF-8-boundary or overflow defects across
800 000 random templates, and checked the CLDR table against CLDR 46.
The plan's
xtaskcross-check was written, run, and found to assert a falseproperty — see decision 8.
Review-repair round (head
8c191fb4)Three findings from the orchestrator's reviewer, all closed; base merged forward
(
chore/merge-v1-head-397had moved to99dd4bc8) by merge commit, so thetwo-dot diff no longer shows a reverse
capsule-web/bun.lockhunk.cargo nextest run -p capsule-i18n -p xtaskcargo test -p capsule-i18n --releasemise run i18n-guardmise run lint-check-mdmise run check-rustFinding 1 (medium, O(n²) on the public entry point) — closed. Mechanism
reproduced first, against a copy of the shipped scan: bytes read are exactly
n(n+1)/2, and time quadruples per doubling (22 ms at n=10 000, 104 ms at 20 000,
369 ms at 40 000, 1.63 s at 80 000).
rendernow pairs every brace in one stackpass (
brace_pairs) and walks a cursor over the result;matching_bracestaysonly for
Arms::parse, where the arms are disjoint. The "if pairing fails here itfails later" shortcut is unsound —
{ {a}has an unmatched outer brace and amatched inner one — so the real pairing is the fix, and that case is in the
equivalence test. Pinned by byte count, not by clock: a thread-local counter in
both scanners, asserted at ≤4n and ≤8n. Measured after: 100 000 bytes for a
100 000-byte unmatched template (was ~5×10⁹) and 118 000 for a 101 000-byte
template of 10 000 placeholders plus 1 000 plurals.
Finding 2 (low, wrong test count in
S-I7) — closed. Three, not two, andthey are now named, with #428 cited.
Finding 3 (low, 4 of 15 API positions had a fixture) — closed. The fixture is
now derived from
SWIFT_TEXT_POSITIONS— one plain and one interpolated literalper entry, plus a helper name that merely ends in the entry — so a position
cannot be added without a case. A second test rejects an entry that is not a bare
identifier, or a duplicate. Verified non-vacuous: appending
inspector Prompttothe list fails it by name. The hand-written test stays, for the real call shapes a
derived fixture cannot express.
Risks and rollout
Pure-function rendering. No persisted state, no wire format, no generated file
changes, no new dependency.
Bundle::format's output changes for the elevenplural keys — from ICU source text to rendered text — and no Rust code resolves
one of those keys today, so no live output changes. Backout is a per-commit
git revert; the four commits are independent in that order.Residual risk: three tests in this diff are
cfg(not(debug_assertions))and noCI job runs
cargo test --release, so they are compiled out of every build CImakes. They were run by hand here. Filed as #428 and recorded in
S-I7asowed-CI;
mise.tomlis outside this lane's manifest.Known limitation, now written down rather than implied: ICU apostrophe quoting
(
'#'for a literal#) is not implemented here or in the ahead-of-timegenerators, so a literal
#,{or}in a plural arm cannot be escaped. Nocatalog message needs one, and an apostrophe away from ICU syntax ("couldn't")
is ordinary text under ICU's own rule.
Related Issues
Refs #414
Closes #394
Files #428 (owed-CI: no job runs
cargo test --release)Decisions taken
Unresolved review notes
None.
Contributor Checklist