diff --git a/scripts/__tests__/check-i18n-dead-keys.test.ts b/scripts/__tests__/check-i18n-dead-keys.test.ts index 2295af3110..a3e805b8d3 100644 --- a/scripts/__tests__/check-i18n-dead-keys.test.ts +++ b/scripts/__tests__/check-i18n-dead-keys.test.ts @@ -4,7 +4,7 @@ import os from 'node:os'; import path from 'node:path'; import { fileURLToPath } from 'node:url'; -import { sweep, textFootprint } from '../check-i18n-dead-keys.mjs'; +import { propertyChainProbe, sweep, textFootprint } from '../check-i18n-dead-keys.mjs'; /** * objectui#4658 — the behaviour test for `scripts/check-i18n-dead-keys.mjs`, @@ -216,6 +216,192 @@ describe('textFootprint()', () => { }); }); +/** + * objectui#6666 — the property-chain leg. + * + * A consumer that imports a locale PACK OBJECT and reads it by property access + * spells neither a `t()` call nor the dotted key, so BOTH of the gate's legs + * were blind to it and the key landed in CONFIRMED — the tier documented as + * the safest thing to delete — with a shipping screen rendering it. + * + * What is pinned below is not "the leg detects things" but that it + * DISCRIMINATES: a key a pack-object consumer really reads is found, and a key + * with no reader at all is still reported CONFIRMED. A leg that demoted + * everything would pass a detection-only test while destroying the top tier, + * which is the failure mode this file exists to make impossible to ship. + */ + +/** A pack shaped like the real bootstrap case: a namespace read through a + * local binding, siblings that nobody reads, and the two shapes the leg's own + * boundaries turn on (a two-segment key, and a leaf that PREFIXES a longer + * sibling leaf). */ +const PACK_READER_EN = `const en = { + splash: { + steps: { connecting: 'Connecting', loadingConfig: 'Loading configuration', connect: 'Connect' }, + failure: { unreachable: 'Server unreachable', giveUp: 'Giving up' }, + }, + short: { ok: 'OK' }, +} as const; +export default en; +`; + +/** The LoadingScreen shape: imports the pack object, binds a namespace to a + * local, reads leaves off it. No `t()`/`tt()` call anywhere, so the AST pass + * visits nothing; the dotted key is never spelled, so the full-key probe + * finds nothing. `response.ok` is the two-segment trap — the chain of + * `short.ok` is exactly `.ok`, and it is present in this source. */ +const PACK_PROPERTY_READER = ` +import { en as enLocale } from '${I18N_PKG}'; +export function Splash(response: { ok: boolean }) { + const strings = enLocale.splash; + if (!response.ok) return null; + return [strings.steps.connecting, strings.steps.loadingConfig]; +} +`; + +function packReaderRoot() { + return repoWith({ + 'packages/i18n/src/locales/en.ts': PACK_READER_EN, + 'packages/x/src/Splash.tsx': PACK_PROPERTY_READER, + }); +} + +describe('propertyChainProbe()', () => { + it('drops the leading namespace segment and keeps the dot', () => { + expect(propertyChainProbe('ns.group.leaf')).toBe('.group.leaf'); + expect(propertyChainProbe('ns.a.b.c')).toBe('.a.b.c'); + }); + + it('returns null below three segments — the leg must NOT apply to two-segment keys', () => { + // A two-segment key's chain is a single generic word (`.ok`, `.no`, + // `.empty`). Probing on it would demote most of the pack on incidental + // property accesses and hollow out CONFIRMED instead of correcting it. + // Two-segment keys are checked against the enumerated importer list in the + // script header by hand — see objectui#6662, which did exactly that. + expect(propertyChainProbe('ns.leaf')).toBeNull(); + expect(propertyChainProbe('leaf')).toBeNull(); + }); +}); + +describe('the property-chain leg discriminates (objectui#6666)', () => { + it('POSITIVE control: a key read only by property access is no longer CONFIRMED', () => { + const { confirmed, needsReview } = sweep(packReaderRoot()); + expect(confirmed).not.toContain('splash.steps.connecting'); + expect(confirmed).not.toContain('splash.steps.loadingConfig'); + const entry = needsReview.find((f) => f.key === 'splash.steps.connecting'); + expect(entry, 'splash.steps.connecting should be in needsReview').toBeDefined(); + expect(entry!.hits).toEqual(['packages/x/src/Splash.tsx (via property chain)']); + }); + + it('NEGATIVE control: a key with no reader at all is STILL CONFIRMED', () => { + // The half that makes this a discriminator rather than a blanket + // demotion. These two live in the same pack, under a sibling namespace of + // the one the consumer binds, and nothing reads them by any route. + const { confirmed } = sweep(packReaderRoot()); + expect(confirmed).toContain('splash.failure.unreachable'); + expect(confirmed).toContain('splash.failure.giveUp'); + }); + + it('does not demote a two-segment key whose one-word chain IS present in source', () => { + // `short.ok`'s chain would be `.ok`, and `PACK_PROPERTY_READER` spells + // `response.ok`. If the leg ever starts applying below three segments this + // is the assertion that catches it. + const { confirmed } = sweep(packReaderRoot()); + expect(confirmed).toContain('short.ok'); + }); + + it('does not demote a leaf merely because a LONGER sibling leaf is read', () => { + // `splash.steps.connect`'s chain `.steps.connect` is a prefix of the + // `.steps.connecting` the consumer actually reads. Without the + // property-boundary check, reading one leaf would demote the other. + const { confirmed } = sweep(packReaderRoot()); + expect(confirmed).toContain('splash.steps.connect'); + }); + + it('does not shrink the CONFIRMED tier to nothing', () => { + // The blunt guard against "make the tool conservative by demoting + // everything": that would pass every detection assertion above while + // making the strongest tier meaningless. + const { confirmed } = sweep(packReaderRoot()); + expect(confirmed.length).toBeGreaterThan(0); + }); +}); + +describe('textFootprint() marks a chain-only hit so the report cannot mislead', () => { + it('suffixes a file the full key does not appear in', () => { + const result = textFootprint(packReaderRoot(), ['splash.steps.connecting']); + expect(result.get('splash.steps.connecting')).toEqual([ + 'packages/x/src/Splash.tsx (via property chain)', + ]); + }); + + it('reports a file plainly when the literal key appears in it, even if the chain also does', () => { + // The literal spelling is the stronger evidence and needs no explanation; + // a suffix there would send the reader looking for a property access that + // is not the reason the file matched. + const root = repoWith({ + 'packages/i18n/src/locales/en.ts': PACK_READER_EN, + 'packages/x/src/config.ts': `export const C = [{ labelKey: 'splash.steps.connecting' }];`, + }); + expect(textFootprint(root, ['splash.steps.connecting']).get('splash.steps.connecting')).toEqual([ + 'packages/x/src/config.ts', + ]); + }); +}); + +describe('both control groups from the card, measured on THIS repository (objectui#6666)', () => { + const repoRoot = path.resolve(path.dirname(fileURLToPath(import.meta.url)), '../..'); + + /** + * Assembled from segments rather than written as dotted strings ON PURPOSE, + * and it must stay that way. `textFootprint()` greps the whole repo + * including `scripts/`, so a dotted key spelled here would make THIS FILE a + * textual hit for it — the negative controls below would stop being + * reader-less because the test asserting they are reader-less mentioned + * them. `check-i18n-dead-keys.mjs` records the same trap on + * `textFootprint()` itself, where an earlier draft self-polluted a real key. + * Joining on the segment boundary keeps BOTH probes' spellings out of this + * file: neither the dotted key nor its property chain occurs contiguously. + */ + const key = (group: string, leaf: string) => ['console', group, leaf].join('.'); + + /** Read by `packages/app-shell/src/chrome/LoadingScreen.tsx` through a local + * binding — the five the card measured, plus two more the leg turned up + * that the card did not list (the same file reads them the same way). */ + const READ_BY_PROPERTY_ACCESS = [ + key('loadingSteps', 'connecting'), + key('loadingSteps', 'loadingConfig'), + key('loadingSteps', 'preparingWorkspace'), + key('error', 'connectionFailed'), + key('error', 'checkServer'), + key('actions', 'retry'), + key('actions', 'retrying'), + ]; + + /** Sibling keys under the same namespace with no reader by any route. */ + const READ_BY_NOBODY = [key('error', 'serverUnreachable'), key('error', 'timeout')]; + + it('POSITIVE: every property-access-read key names LoadingScreen.tsx as a hit', () => { + const found = textFootprint(repoRoot, READ_BY_PROPERTY_ACCESS); + for (const k of READ_BY_PROPERTY_ACCESS) { + expect( + found.get(k), + `${k} is rendered by LoadingScreen.tsx through a local binding, and the property-chain leg ` + + 'is the only probe that can see that read', + ).toContain('packages/app-shell/src/chrome/LoadingScreen.tsx (via property chain)'); + } + }); + + it('NEGATIVE: keys nothing reads still have no textual footprint at all', () => { + // The half that keeps the leg honest on the real tree. If someone + // "hardens" it into a blanket demotion this is what fails. If it ever + // fails honestly — a real reader for one of these appeared — the fix is to + // pick a still-reader-less sibling, never to loosen the assertion. + const found = textFootprint(repoRoot, READ_BY_NOBODY); + for (const k of READ_BY_NOBODY) expect(found.get(k), `${k} must have no reader`).toEqual([]); + }); +}); + describe('the collapse guard lives in the CLI block, not in sweep() itself', () => { it('sweep() runs against a small synthetic fixture without throwing', () => { // Unlike the CLI entry point (which exits 1 below ~2000 keys on the REAL diff --git a/scripts/check-i18n-dead-keys.mjs b/scripts/check-i18n-dead-keys.mjs index 5e88c7aed4..851663da37 100644 --- a/scripts/check-i18n-dead-keys.mjs +++ b/scripts/check-i18n-dead-keys.mjs @@ -94,6 +94,85 @@ * look at it before deleting, which is exactly what "AST-dead" alone cannot * tell you. * + * ## The property-chain leg (objectui#6666) + * + * The full-key probe above has a blind spot with a name. A consumer that + * imports a locale PACK OBJECT and reads it by property access never spells + * the dotted key at all: the namespace segment is bound to a local variable, + * so the source says `myPack.someGroup.someLeaf` where the pack key is + * `topNamespace.someGroup.someLeaf`. The AST pass sees nothing either — there + * is no `t()`/`tt()` call to classify. Both legs are blind at once, and the + * key lands in CONFIRMED, the tier documented as the safest thing to delete, + * while a shipping screen renders it. (That example is synthetic on purpose; + * naming a real key here would make this file a textual hit for it — same + * trap `textFootprint()`'s own note below records.) + * + * So alongside the full dotted key, `textFootprint()` also probes the key's + * PROPERTY CHAIN: the key minus its leading namespace segment, leading dot + * kept (`propertyChainProbe()`). That is precisely the text a property-access + * reader DOES spell, whatever it named the variable holding the pack. + * + * Three boundaries, each load-bearing: + * + * 1. THREE SEGMENTS OR MORE, never two. A two-segment key's chain is a + * single word (`.ok`, `.no`, `.empty`) that matches far too much to be + * evidence of anything, and demoting on it would hollow out the top tier + * instead of correcting it. `propertyChainProbe()` returns `null` below + * three segments, so the leg does not apply there at all. For two-segment + * keys the sound check is the enumerated importer list below, read by + * hand — which is what objectui#6662 used. + * 2. The match must END at a property boundary: the next character may not + * continue an identifier. Without it a chain would also match a LONGER + * sibling leaf it merely prefixes, demoting a key on evidence about a + * different one. + * 3. Recall over precision where the two still collide — the same direction + * `dynamicHeads` above is deliberately wrong in. Two keys under different + * namespaces can share a chain, so a reader of one demotes both. For a + * DELETION sweep that is the right way to be wrong. To keep that from + * reading as a lie, a hit found ONLY by this probe is reported with a + * ` (via property chain)` suffix: the human reading the report then looks + * for a property access instead of grepping the dotted key, finding + * nothing, and concluding the tool is broken. + * + * ## The pack-object importers, enumerated (objectui#6666) + * + * The leg is only as good as the class it covers, so the class is written down + * rather than left to memory. Re-derive it — the specifier is kept off the + * `from '...'` shape here on purpose, so this comment is not itself read as an + * import edge: + * + * grep -rn --include=*.ts --include=*.tsx -E \ + * "import \{[^}]*\b(en|zh|builtInLocales|ja|ko|de|fr|es|pt|ru|ar)\b[^}]*\}" \ + * packages apps examples e2e \ + * | grep "@object-ui/i18n" | grep -v '^packages/i18n/' + * + * NON-TEST importers — the ones that can keep a SHIPPED key alive (19 matches + * today, of which these four): + * + * - `packages/app-shell/src/chrome/LoadingScreen.tsx` — the case this leg was + * built for. Bootstrap-critical UI: it must render BEFORE i18n loads, which + * is exactly the server-down boot it exists to explain, so it deliberately + * does not call `useObjectTranslation` (its own comment says so). It reads + * its pack subtree through a local binding. Covered BY DESIGN. + * - `packages/app-shell/src/console/ai/outboundAgentText.ts` — indexes its + * pack subtree DYNAMICALLY (`ai?.[key]`), so NEITHER leg sees the read: no + * dotted key, no property chain, no call site. Its four keys land in + * NEEDS-REVIEW anyway, but only because those property names happen to + * appear as string literals in the `OutboundAgentTextKey` union a few lines + * above them — BY LUCK, NOT BY DESIGN. Do NOT read this file as covered by + * the leg. Replace that union with anything generated and all four keys + * drop to CONFIRMED with a live consumer still reading them. + * - `packages/plugin-grid/demo/main.tsx` and + * `packages/plugin-grid/demo/bulk-actions.tsx` — whole-pack `resources` + * wiring only, no per-key property reads: nothing for the leg to see and + * nothing at risk. + * + * The rest of the matches are TEST-only importers, deliberately not listed one + * by one. A key read only by a test is not a key a user can see, so a test + * importer never establishes liveness — it only ever adds a textual footprint, + * which the full-key probe already catches. The re-derivation returns both + * sets; the split is the point, not the count. + * * ## Usage * * node scripts/check-i18n-dead-keys.mjs # report, exit 0 always @@ -129,9 +208,58 @@ const TEXT_SWEEP_SKIP_DIRS = new Set([ * hit inside it is not evidence of a reference and must not count as one. */ const LOCALES_DIR = 'packages/i18n/src/locales/'; +/** Appended to a hit path that only the property-chain probe matched, so the + * report never sends a reader grepping for a dotted key that file does not + * contain. A hit the full-key probe also found is reported unsuffixed — the + * literal spelling is the stronger evidence and needs no explanation. */ +const PROPERTY_CHAIN_HIT_SUFFIX = ' (via property chain)'; + +/** Characters that would continue a JS identifier, and therefore mean a + * property-chain match landed in the MIDDLE of a longer property name. */ +const IDENTIFIER_CHAR = /[A-Za-z0-9_$]/; + +/** + * The property-chain probe for `key`: the key minus its leading namespace + * segment, leading dot kept (`ns.group.leaf` -> `.group.leaf`). + * + * This is the text a consumer that imported the PACK OBJECT and reads it by + * property access actually spells — the namespace segment is bound to a local + * variable, so the dotted key never appears. See the header section + * "The property-chain leg" for why both other legs are blind to that shape. + * + * @returns {string | null} `null` for keys of fewer than three segments, whose + * chain would be a single generic word (`.ok`, `.no`) rather than evidence. + * Callers must treat `null` as "the leg does not apply", never as "no match". + */ +export function propertyChainProbe(key) { + const parts = key.split('.'); + if (parts.length < 3) return null; + return `.${parts.slice(1).join('.')}`; +} + +/** + * Whether `probe` occurs in `content` ending at a property boundary — i.e. at + * least one occurrence is NOT immediately followed by an identifier character. + * + * `.group.leaf` is a substring of `.group.leafExtended`, so a plain + * `includes()` would demote a key on a reader of a longer sibling leaf. Every + * occurrence is checked, not just the first: one line can hold both shapes. + */ +function occursAtPropertyBoundary(content, probe) { + for (let from = 0; ; from += 1) { + const at = content.indexOf(probe, from); + if (at === -1) return false; + const next = content[at + probe.length]; + if (next === undefined || !IDENTIFIER_CHAR.test(next)) return true; + from = at; + } +} + /** * Whether the literal dotted string of each `key` in `keys` occurs anywhere in - * the repo outside `LOCALES_DIR`. + * the repo outside `LOCALES_DIR`, OR — for keys of three or more segments — + * its property chain does (`propertyChainProbe()`; header section "The + * property-chain leg"). * * One `grep -rF` pass over the whole tree, not one subprocess per candidate — * a repo this size makes per-key greps the slow path. `-F` (fixed string) is @@ -144,15 +272,24 @@ const LOCALES_DIR = 'packages/i18n/src/locales/'; * target key here and self-polluted that key's report entry). * * @returns {Map} key -> repo-relative file paths that - * mention it outside the locale packs (deduped, sorted). A key absent from - * the map, or mapped to `[]`, has no textual footprint at all. + * mention it outside the locale packs (deduped, sorted). A path matched only + * by the property-chain probe carries `PROPERTY_CHAIN_HIT_SUFFIX`. A key + * absent from the map, or mapped to `[]`, has no textual footprint at all. */ export function textFootprint(root, keys) { const result = new Map(keys.map((key) => [key, []])); if (keys.length === 0) return result; + // One probe set per key: the literal dotted key always, plus its property + // chain once the key is three segments deep. Both go into the SAME grep pass + // — the whole point of the patterns file is that a repo this size makes + // per-key greps the slow path, and that argument does not change because + // there are now up to twice as many fixed strings in it. + const probes = keys.map((key) => ({ key, chain: propertyChainProbe(key) })); + const patterns = [...new Set(probes.flatMap(({ key, chain }) => (chain === null ? [key] : [key, chain])))]; + const patternsFile = join(mkdtempSync(join(tmpdir(), 'i18n-dead-keys-')), 'patterns.txt'); - writeFileSync(patternsFile, keys.join('\n') + '\n'); + writeFileSync(patternsFile, patterns.join('\n') + '\n'); const args = [ '-rFn', // recursive, fixed-string, line-numbered @@ -177,6 +314,12 @@ export function textFootprint(root, keys) { } rmSync(dirname(patternsFile), { recursive: true, force: true }); + // key -> (file -> whether the LITERAL dotted key was seen in that file). The + // flag decides the suffix at the end: a file that also spells the key + // literally is reported plain, even if another line in it only matched the + // chain, because the literal spelling is the stronger evidence. + const perKey = new Map(keys.map((key) => [key, new Map()])); + for (const line of output.split('\n')) { if (!line) continue; // `file:line:content` — content itself may contain further `:`, so split @@ -188,12 +331,20 @@ export function textFootprint(root, keys) { const content = line.slice(secondColon + 1); const relFile = relative(root, file).split('\\').join('/'); if (relFile.startsWith(LOCALES_DIR)) continue; - for (const key of keys) { - if (content.includes(key)) result.get(key).push(relFile); + for (const { key, chain } of probes) { + const literal = content.includes(key); + if (!literal && !(chain !== null && occursAtPropertyBoundary(content, chain))) continue; + const files = perKey.get(key); + files.set(relFile, (files.get(relFile) ?? false) || literal); } } - for (const [key, files] of result) result.set(key, [...new Set(files)].sort()); + for (const [key, files] of perKey) { + result.set( + key, + [...files].map(([file, literal]) => (literal ? file : `${file}${PROPERTY_CHAIN_HIT_SUFFIX}`)).sort(), + ); + } return result; }