diff --git a/.github/CODEOWNERS b/.github/CODEOWNERS index bdd391f2..41fe6452 100644 --- a/.github/CODEOWNERS +++ b/.github/CODEOWNERS @@ -28,4 +28,7 @@ /domains/pr-workflow/ @MetaMask/extension-platform @MetaMask/mobile-platform /domains/swaps/ @MetaMask/swaps-engineers /domains/testing/ @MetaMask/qa +/domains/typescript/ @MetaMask/extension-platform @MetaMask/mobile-platform +/domains/typescript/skills/*/repos/metamask-extension.md @MetaMask/extension-platform +/domains/typescript/skills/*/repos/metamask-mobile.md @MetaMask/mobile-platform /domains/ui/ @MetaMask/design-system-engineers diff --git a/domains/typescript/skills/avoid-any/repos/core.md b/domains/typescript/skills/avoid-any/repos/core.md new file mode 100644 index 00000000..4e0aa1f3 --- /dev/null +++ b/domains/typescript/skills/avoid-any/repos/core.md @@ -0,0 +1,60 @@ +--- +repo: core +parent: avoid-any +--- + +# `avoid-any` in core + +## The rule is a deliberate re-enable — and it does not travel with the code + +`@typescript-eslint/no-explicit-any` is `'error'` at `eslint.config.mjs:152`, in the `files: ['**/*.ts', '**/*.mts']` block (`:135`) that extends the shared config (`:136`). The comment above it says so outright: + +``` +// Enable rules that are disabled in `@metamask/eslint-config-typescript` +``` + +`@metamask/eslint-config-typescript@^15.0.0` (`package.json:70`) sets `no-explicit-any: 'off'` at `src/index.mjs:46`. The same is true of the majors extension and mobile run (`14.1.1:50`, `13.0.0:44`) — **the org's shared TypeScript config permits explicit `any`, and each repo bans it separately.** + +The consequence is for extraction, not for writing code here: **a package that leaves core and adopts `@metamask/eslint-config-typescript` without copying `eslint.config.mjs:152` silently permits explicit `any`.** Nothing warns at the split; the code lints clean in its new home and the rule is simply gone. Copy the line with the code. + +The block's glob is `**/*.ts` and `**/*.mts`. There are currently no `.tsx`, `.cts` or `.mts` sources under `packages/` (`main`, 2026-08-31), so nothing falls outside it — a first `.tsx` file would. + +## The absorbed case is unreported here too + +`src/index.mjs:69`–`:73` sets `no-unsafe-argument`, `no-unsafe-assignment`, `no-unsafe-call`, `no-unsafe-member-access` and `no-unsafe-return` all `'off'`, under `// Recommended rules that require type information`. `eslint.config.mjs` does not turn any of them back on. Those five are exactly the rules typescript-eslint ships for the absorbed case, so the parent skill's declared/absorbed split is this repo's literal configuration: **a written `any` fails CI; an absorbed `any` is reported by nothing.** + +Core is nonetheless the tightest of the three on the neighbouring rules. Unlike extension and mobile it carries **no ESLint-v9 reversion block**, so `no-floating-promises` and `no-unsafe-function-type` are both live from the shared config's `recommended` / `recommendedTypeChecked` (verified in `@typescript-eslint/eslint-plugin@8.54.0`, `dist/configs/flat/recommended.js`). Two gaps it shares with the others anyway: + +- `no-unsafe-enum-comparison` — the shared config disables it at `src/index.mjs:78`. +- `restrict-template-expressions` — on, at `src/index.mjs:171`–`:177`, but neither there nor here does anything set `allowAny`, whose default is `true` (`@typescript-eslint/eslint-plugin`, `dist/rules/restrict-template-expressions.js`, `defaultOptions`). `` `${value}` `` with an `any` operand is permitted. + +## Where the absorbed `any` enters here + +**Not through `.js`.** `allowJs` and `checkJs` appear in no tsconfig in the repo (`git grep -l 'allowJs\|checkJs' -- '*.json'` returns nothing on `main`). `.js` files exist — `eslint.config.mjs:128` has a `files: ['**/*.{js,cjs}']` block for them — but they are outside every TypeScript program, so a `.ts` file cannot import one without a declaration. **The route that carries most of extension's exposure does not exist here. Do not go looking for it.** + +**Through the root `types/` directory instead, and it reaches every package at once.** Ten shorthand `declare module` files live there, each typing its entire module as `any` with no `any` written and no diagnostic under `strict`: + +`types/@metamask/contract-metadata.d.ts` · `types/@metamask/eth-json-rpc-filters.d.ts` (`/subscriptionManager`) · `types/@metamask/ethjs-provider-http.d.ts` · `types/@metamask/ethjs-unit.d.ts` · `types/@metamask/metamask-eth-abis.d.ts` · `types/eth-ens-namehash.d.ts` · `types/eth-json-rpc-infura/src/createProvider.d.ts` · `types/ethereum-ens-network-map.d.ts` · `types/ethjs-query.d.ts` · `types/single-call-balance-checker-abi.d.ts` + +Package tsconfigs pull the directory in wholesale — `packages/network-controller/tsconfig.json` ends `"include": ["../../types", "../../tests", "./src", "./tests"]` — so all ten are in every package's program, not local to whichever package imports them. An ABI or a token-metadata blob entering a controller through one of these is `any` at the point of entry, and every precise signature downstream of it is absorbed. + +**And an absorbed `any` crosses package boundaries as a published type.** `tsconfig.base.json:6` sets `composite: true`, and the root `tsconfig.json` wires the packages together with project references. A dependent package sees its dependency through the emitted `.d.ts`, not through source — so an absorbed `any` on an exported signature is not a local imprecision, it is the interface every consuming package compiles against, and the extension and mobile bundles beyond them. + +## Two settings that are not `any` sources + +`tsconfig.base.json:12` sets `strict: true`, and it is the only `"strict"` key in any tsconfig in the repo — no package weakens `noImplicitAny`. `noUncheckedIndexedAccess` is not set, which widens indexing results rather than producing `any`; a replacement for one of the shorthand declarations typed `Record` will read as total when it is not. + +## Checking it + +```bash +git grep -nE "^declare module '[^']+';$" -- '*.d.ts' # shorthand = whole module is any +git grep -rn "eslint-disable.*no-explicit-any" # the declared any, countable +``` + +Neither finds the absorbed form. Probe it at the call site with `IsAny` — the probe, its controls, and the `declare module` composition that turns one of these shorthand declarations into a real type are in `tsc-blindspots`. + +## Open questions + +- Which of the ten shorthand declarations are actually imported, and from which packages? Settled by resolving importers per specifier, not by the file list. +- Do any package `src` files re-export a value sourced from one of them, publishing the `any` through a `.d.ts`? Settled by an `IsAny` probe against each package's built declaration output, not against its source. +- Whether every package tsconfig includes `../../types`, or only some. Checked here for `network-controller` only; settled by reading the `include` of each `packages/*/tsconfig.json`. diff --git a/domains/typescript/skills/avoid-any/repos/metamask-extension.md b/domains/typescript/skills/avoid-any/repos/metamask-extension.md new file mode 100644 index 00000000..cc954053 --- /dev/null +++ b/domains/typescript/skills/avoid-any/repos/metamask-extension.md @@ -0,0 +1,63 @@ +--- +repo: metamask-extension +parent: avoid-any +--- + +# `avoid-any` in metamask-extension + +## The rule is a re-enable, and the rules that would catch the absorbed case are off + +`@typescript-eslint/no-explicit-any` is `'error'` at `.eslintrc.js:162`. That line **restores** a rule the shared config switches off. Extension is on `@metamask/eslint-config-typescript@^14.1.1` (`package.json:620`), whose `src/index.mjs` sets: + +- `:50` — `@typescript-eslint/no-explicit-any: 'off'` +- `:73`–`:77` — `no-unsafe-argument`, `no-unsafe-assignment`, `no-unsafe-call`, `no-unsafe-member-access`, `no-unsafe-return`, all `'off'`, under the comment `// Recommended rules that require type information` + +Those five are the rules typescript-eslint ships for the absorbed case. Nothing in `.eslintrc.js` turns any of them back on. So the parent skill's declared/absorbed split is this repo's literal configuration: **a written `any` fails CI, and an absorbed `any` is reported by nothing at all.** + +## Three further rules off — extension's distinguishing gap + +The comment at `.eslintrc.js:272`–`:276` introduces a block of reversions — *"removing changes to our shared ESLint config made after version v9 … a temporary measure to get us to ESLint v9 compatible versions, at which point we can restore the intended rules"* — and the rules it governs run from `:277` to `:306`. Three of them bear on `any`: + +- **`no-floating-promises: 'off'` (`:284`)** — with the `no-unsafe-*` rules also off, a call whose return is absorbed `any` and a call that drops a real promise are the same unreported line. This rule is the last one that would have noticed the value was thenable. +- **`no-unsafe-enum-comparison: 'off'` (`:289`)** — comparing an enum member against a value that is `any` typechecks. Note this one is **not** extension-specific: the shared config already disables it at `src/index.mjs:82` (`// Recommended rules that we do not want to use`), so `:289` is redundant and the gap exists in mobile and core too. +- **`no-unsafe-function-type: 'off'` (`:291`)** — **this one is extension-only.** `Function` accepts any argument list and returns `any`, so `const f: Function` is an `any`-producing annotation with no `any` written. Mobile and core do not disable it, and it ships in typescript-eslint v8's `recommended` (verified in `@typescript-eslint/eslint-plugin@8.54.0`, `dist/configs/flat/recommended.js`). + +A fourth rule looks tightened and is not. `restrict-template-expressions` gets a local config at `:262` (`allowBoolean`, `allowNumber`) — the same option object the shared config already passes at `src/index.mjs:171`–`:177`. Neither sets `allowAny`, whose default is `true` (`@typescript-eslint/eslint-plugin`, `dist/rules/restrict-template-expressions.js`, `defaultOptions`). So `` `${value}` `` with an `any` operand is permitted. Same conclusion in core, by the same default; in mobile the rule is `'off'` outright. + +## Where the absorbed `any` enters here + +**The `.js` boundary is the largest of the three repos.** `tsconfig.json` sets `allowJs: true` and never sets `checkJs`. Inside its `include` (`app`, `development`, `shared`, `test`, `types`, `ui`) there are **1,182 `.js`/`.jsx` files against 7,373 `.ts`/`.tsx`** (`origin/main`, 2026-08-31). An untyped JS export's parameters and return are `any` at every TS call site, with zero diagnostics — verified by probe under `--strict` (`IsAny[0]>` resolves `true`, `tsc` exits 0). + +**Two bodyless ambient module declarations**, of 16 `declare module` lines in the repo: + +- `shared/lib/declare-modules.d.ts:1` — `declare module 'human-standard-token-abi';` +- `types/lavamoat__lavadome-core.d.ts:1` — `declare module '@lavamoat/lavadome-core';` + +A shorthand declaration types **every** import from that module as `any`, silently, under `strict` (verified by the same probe). Both files are inside `include`. + +**The 300 story files get neither `tsc` nor the rule, and the two exclusions compound.** `.eslintrc.js:26`–`:28` builds a `tsconfig` object from `tsconfig.json` with the TypeScript API — `ts.findConfigFile`, `ts.readConfigFile`, `ts.parseJsonConfigFileContent` — and `:148` uses `tsconfig.fileNames` as the `files` list for the block that carries `no-explicit-any`. So the rule's scope *is* the tsconfig program. `tsconfig.json` excludes `**/*.stories.ts` and `**/*.stories.tsx` (`:31`–`:32`) — **300 files on `origin/main`** — which puts them outside `fileNames` and therefore outside the rule as well. The repo says so itself, in the docblock at `.eslintrc.js:684`–`:691`: *"Storybook (JavaScript only) … This block is for overriding settings from the base config. It's JavaScript-only because the Storybook TypeScript files don't have the base config applied."* In those 300 files an explicit `any` is not an error and an absorbed `any` is not even typechecked. + +The same mechanism generalises: any `.ts`/`.tsx` file outside `tsconfig.json`'s `include` list is outside `no-explicit-any` too, silently, with no entry in `.eslintrc.js` naming it. Beyond the stories that is currently 3 files, all under `.devcontainer/` (7,379 `.ts`/`.tsx` on `origin/main`; 7,373 in the `include` dirs, 3 at repo root matching `*.ts`). The hole is real and small — it is the stories that carry it. + +The parent skill's own absorbed example (`shared/lib/token-util.ts`, ethers dynamic-method results) is extension code; it is the shape to expect wherever a precise signature sits downstream of ethers, a `.js` module, or one of the two shorthand declarations above. + +## Two settings that are not `any` sources, so they don't belong in this hunt + +- `useUnknownInCatchVariables: true` is set explicitly in `tsconfig.json` (already implied by the inherited `strict`). Catch bindings are `unknown`, so no `catch (e: any)` is needed. +- `noUncheckedIndexedAccess` is not set. That widens indexing results, not `any` — a replacement for a bodyless declaration typed `Record` will read as total when it is not. + +`strict` is inherited from `@tsconfig/node22` (`node_modules/@tsconfig/node22/tsconfig.json`), not declared locally — a local `tsconfig.json` edit that changed `extends` would drop `noImplicitAny` with nothing in the file mentioning it. + +## Checking it + +```bash +git grep -nE "^declare module '[^']+';$" -- '*.d.ts' # shorthand = whole module is any +git grep -rn "eslint-disable.*no-explicit-any" # the declared any, countable +``` + +The absorbed form is invisible to both. Probe it at the call site with `IsAny` — the probe, its controls, and the `declare module` composition that repairs one are in `tsc-blindspots`. + +## Open questions + +- How many of the 1,182 in-program `.js` files are actually imported from `.ts`? Settled by resolving each `.js` file's importers, not by the file count above. +- Do the two shorthand declarations still need to be shorthand, or do the packages now ship types? Settled by checking each package's `types`/`exports` field at the installed version. diff --git a/domains/typescript/skills/avoid-any/repos/metamask-mobile.md b/domains/typescript/skills/avoid-any/repos/metamask-mobile.md new file mode 100644 index 00000000..ccafe744 --- /dev/null +++ b/domains/typescript/skills/avoid-any/repos/metamask-mobile.md @@ -0,0 +1,67 @@ +--- +repo: metamask-mobile +parent: avoid-any +--- + +# `avoid-any` in metamask-mobile + +## The rule is a re-enable, and the rules that would catch the absorbed case are off + +`@typescript-eslint/no-explicit-any` is `'error'` at `.eslintrc.js:170`, inside the `files: ['*.{ts,tsx}']` override (`:155`) that extends `@metamask/eslint-config-typescript` (`:156`). That line **restores** a rule the shared config switches off. Mobile is on `^13.0.0` (`package.json:635`), whose `src/index.js` sets: + +- `:44` — `@typescript-eslint/no-explicit-any: 'off'` +- `:67`–`:71` — `no-unsafe-argument`, `no-unsafe-assignment`, `no-unsafe-call`, `no-unsafe-member-access`, `no-unsafe-return`, all `'off'` + +Those five are the rules typescript-eslint ships for the absorbed case, and nothing in `.eslintrc.js` turns any of them back on. So the parent skill's declared/absorbed split is this repo's literal configuration: **a written `any` fails CI, and an absorbed `any` is reported by nothing at all.** + +One thing mobile gets right that extension does not: the block's `files` is a plain glob, `'*.{ts,tsx}'`. Extension derives its equivalent list from the tsconfig program, so a file excluded from typechecking there loses `no-explicit-any` with it. Here the two scopes are independent — excluding a file from `tsconfig.json` does not remove the rule from it. + +## Three more rules off, and one that stays on + +The comment at `.eslintrc.js:186` introduces the same ESLint-v9 reversion block extension carries (*"a temporary measure to get us to ESLint v9 compatible versions"*); the rules it governs run from `:191` to `:223`. Three of them bear on `any`: + +- **`no-floating-promises: 'off'` (`:199`)** — with the `no-unsafe-*` rules also off, a call returning absorbed `any` and a call dropping a real promise are the same unreported line. +- **`no-unsafe-enum-comparison: 'off'` (`:206`)** — redundant with the shared config, which already disables it; the gap is not mobile-specific. +- **`restrict-template-expressions: 'off'` (`:221`)** — `` `${value}` `` with an `any` operand goes unremarked. Extension and core keep the rule on but leave `allowAny` at its default `true`, so the outcome is the same in all three; only the reason differs. + +`no-unsafe-function-type` is **not** disabled here, unlike extension. It ships in typescript-eslint v8's `recommended` and mobile is on `@typescript-eslint/eslint-plugin@^8.1.0` (`package.json:692`), so `const f: Function` — an `any`-producing annotation with no `any` written — is caught in mobile and not in extension. + +## The per-path override is the lever for tightening this + +`.eslintrc.js:471` scopes `no-floating-promises: 'error'` (`:628`) back on for `app/**/*-method-action-types*.ts`, under the Perps Core-alignment comment at `:459`. Whatever its motivation, it is the worked example: a team wanting the five `no-unsafe-*` rules on in their own directory does it with an `overrides` entry on their glob, not a repo-wide flip. + +## Where the absorbed `any` enters here + +**`app/declarations/index.d.ts` is the largest bodyless-declaration surface of the three repos** — **14 of its 38 `declare module` lines are shorthand**, and a shorthand declaration types every import from that module as `any`, silently, under `strict` (verified by probe: `IsAny` resolves `true`, `tsc` exits 0): + +- `:2`–`:5` — four `react-native-safe-area-context/src/*` deep imports +- `:9` `*.mp4` · `:11` `@metamask/react-native-payments/lib/js/__mocks__` · `:13` `react-native-fade-in-image` · `:15` `react-native-fast-crypto` · `:17` `react-native-minimizer` · `:19` `xhr2` +- `:307` `@metamask/react-native-search-api` · `:439`–`:440` `@tommasini/react-native-scrollable-tab-view` and its `/DefaultTabBar` · `:442` `react-native-tcp-socket` + +The same file also carries an **untyped ambient const** at `:302`–`:305` — `declare module '@metamask/react-native-actionsheet' { const ActionSheet; export default ActionSheet; }`. In an ambient context a missing annotation is `any` and `noImplicitAny` does not fire; the probe confirms it. This is the absorbed form wearing a declaration's clothes, and grepping for shorthand `declare module` will not find it. + +**The `.js` boundary exists but is narrow.** `tsconfig.json:6` sets `allowJs: true` and `checkJs` is never set. Under `app/` there are **237 `.js`/`.jsx` files against 12,797 `.ts`/`.tsx`** (`main`, 2026-08-31) — proportionally about a seventh of extension's exposure. `app/core/InpageBridgeWeb3.js` is in `exclude` (`tsconfig.json:94`), so it is outside the program entirely. + +**`lib: ["es2022"]` with no DOM (`tsconfig.json:5`) routes web-shaped code back into hand-written declarations.** A value with no ambient type either fails to compile or gets served by `app/declarations/`, which is where the shorthand list above lives. `index.d.ts:317` augmenting the global `Crypto` interface is this pressure showing up in the file. + +**`paths` maps 20 specifiers to a chosen declaration file** (`tsconfig.json:15`), several of them deep into `node_modules/**/dist/**/*.d.cts`, and one (`tsconfig.json:18`) to a local `app/declarations/@keystonehq/ur-decoder.d.ts`. The type at every call site of those imports is decided by `tsconfig.json` rather than by the package — so re-reading the import statement tells you nothing about what was resolved. + +## One setting that is not an `any` source + +`noUncheckedIndexedAccess` is not set. That widens indexing results, not `any`; a replacement for a shorthand declaration typed `Record` will read as total when it is not. + +## Checking it + +```bash +git grep -nE "^declare module '[^']+';$" -- '*.d.ts' # shorthand = whole module is any +git grep -nE "^\s+(const|let|var) [A-Za-z_$][\w$]*;$" -- '*.d.ts' # untyped ambient = also any +git grep -rn "eslint-disable.*no-explicit-any" # the declared any, countable +``` + +None of these finds the absorbed form in ordinary source. Probe it at the call site with `IsAny`; the probe, its controls, and the `declare module` composition that repairs one are in `tsc-blindspots`. + +## Open questions + +- Do any of the 20 `paths` targets resolve to `any` at their call sites? Settled by an `IsAny` probe on one import from each mapped specifier, not by reading the mapping. +- Which of the 14 shorthand declarations still need to be shorthand? Settled by checking each package's `types`/`exports` field at the installed version. +- Whether mobile's resolved `@typescript-eslint` version treats `no-unsafe-function-type` as `recommended` — `^8.1.0` was read from `package.json`, not from `yarn.lock`. Settled by reading the lockfile entry. diff --git a/domains/typescript/skills/avoid-any/skill.md b/domains/typescript/skills/avoid-any/skill.md new file mode 100644 index 00000000..dc2b1a65 --- /dev/null +++ b/domains/typescript/skills/avoid-any/skill.md @@ -0,0 +1,101 @@ +--- +name: avoid-any +description: >- + Handle `any` correctly — it is not a type but a directive that disables type + checking. Substitute by position (assignee → `unknown`, assigned → `never`). + Two narrow exceptions: a generic constraint, and a callback parameter caught + in a bivariant position between two fixed, irresolvable function-type + constraints — both declared with an inline eslint-disable. Also covers the + `any` that is never written down: a precise signature fed `any` at every call + site, which no lint rule and no compiler check can report. +maturity: experimental +--- + +# Avoid `any` + +Deepens the TypeScript guidance in `mms-coding-guidelines` — which already says "avoid `any`, prefer `unknown`" and links `MetaMask/contributor-docs` `docs/typescript.md`. This is the reasoning layer beneath that bullet: why `any` is dangerous, and how to replace it by position. Grounded in `docs/typescript.md` (§ Avoid `any`). + +## `any` is not a type — it is a directive that disables type checking + +The mental model matters more than the ESLint rule, because `@typescript-eslint/no-explicit-any` (already `error` in extension CI) does not stop the reasoning that reaches for `any`: + +- **`any` is not "the widest type" — that is `unknown`.** `any` is a compiler directive that _disables_ type checking for the value it annotates. +- **It suppresses every error about its assignee** — the equivalent of `@ts-ignore` on every use of that variable. The errors still affect the code; `any` only makes them invisible. +- **It subsumes what it touches.** Any type in a union, intersection, or property relationship with `any` becomes `any` — an unmitigated loss of type information. +- **It infects downstream code.** One `any` at a source (e.g. a library type that resolves to `any`) propagates silently through every consumer, converting compile-time errors into **silent runtime failures** — defeating the point of a statically-typed language. + +## Substitute `any` by position — assignee vs assigned + +Identify which side of an assignment the `any` sits on: + +- **Assignee** (a variable, parameter, or return that _receives_ a value — "it could be anything"): **try `unknown` first**, then narrow. `unknown` is the true universal supertype: everything is assignable to it, but it forces a type guard before use. `any` ↔ `unknown` are interchangeable in this position, so it is almost always a safe swap. + - 🚫 `type Fn = () => any; const xs: any[]` + - ✅ `type Fn = () => unknown; const xs: unknown[]` +- **Assigned** (a value that _flows into_ a slot): **try `never` first**, then widen to a subtype of the assignee's type. `unknown` cannot substitute here (it is only assignable to `unknown`); `never` is the bottom type, assignable to everything. + +## Acceptable exception 1 — generic *constraints* + +`any` is acceptable in a generic **constraint**, and only there. It bounds a type parameter without being assigned to a value, so it neither pollutes nor infects. + +```typescript +class BaseController< + ControllerName extends string, + ControllerState extends StateConstraint, + ControllerMessenger extends Messenger< + ControllerName, + ActionConstraint, + EventConstraint, + // eslint-disable-next-line @typescript-eslint/no-explicit-any + any + >, +> // ... +``` + +Three conditions attach: + +- **Declare it explicitly.** `no-explicit-any` is `error`, so a constraint `any` needs an inline `// eslint-disable-next-line @typescript-eslint/no-explicit-any` at the site — a deliberate, visible exception. +- **Constraints only — never a generic _argument_.** Passing `any` as an argument (`ControllerMessenger`) is 🚫 — that assigns `any` and infects. Constraint-vs-argument is the whole distinction. +- **Prefer a narrower constraint anyway.** Reach for `any` here only when the narrower bound is genuinely unavailable. + +## Acceptable exception 2 — a callback parameter between two irresolvable function-type constraints + +A callback's parameter may be `any` when all three hold: + +1. **Bivariant position** — the callback is _assignable to_ a wider function type **and** an _assignee of_ a narrower one. +2. **Irresolvable** — no concrete type satisfies both directions, because the wider function type is not a supertype of the narrower (`WideParam extends T extends NarrowParam` has no solution). +3. **Fixed** — neither constraint can be redesigned without breaking callers or losing accuracy. + +Under `--strictFunctionTypes` parameters are contravariant, so the callback's parameter must be a _supertype_ of the outer slot's (outward: `unknown` ✓, `never` ✗) **and** a _subtype_ of the incoming value's (inward: `never` ✓, `unknown` ✗). `any` is the only inhabitant of both the top and bottom of the assignability lattice, so it is the only escape. (Return types stay covariant — keep `unknown`.) + +🚫 If the constraints are **redesignable**, the contravariance error is a real design smell — fix it (usually by parametrizing with a generic), don't suppress: + +```typescript +declare const acceptGeneric: (handler: (event: E) => void) => void; +acceptGeneric(onA); // no `any` needed +``` + +✅ Only when the constraints are **fixed and irresolvable**: + +```typescript +// eslint-disable-next-line @typescript-eslint/no-explicit-any -- Bivariant position with irresolvable, fixed constraints +let bridge: (x: any) => void; +``` + +Like the generic-constraint case, this `any` is **not infectious** — it is scoped to one parameter position, and both constraint types re-impose their signatures at each use site. Annotate the `eslint-disable` with the criteria so a reviewer can check them. Caveat: the safety claim holds only if the constraint types are accurate — a fixed constraint that is itself imprecise (a library type that is `any` internally) still forces the bridge `any` but no longer preserves safety at the use sites. + +Canonical instance: a messenger `registerActionHandler` slot typed `(...args: any[]) => any` — strongly-typed handlers flow inward at registration, strongly-typed argument tuples outward at dispatch; `unknown[]` fails registration, `never[]` fails dispatch. It encodes rank-N polymorphism (`∀α. (α) => R`) that TypeScript cannot express directly. + +When `any` still seems unavoidable, prefer the narrower, greppable escape hatches: `as unknown as` as a documented last resort, or `@ts-expect-error` with a comment saying why (a TODO where a fix is planned), never `@ts-ignore`. An unneeded `@ts-expect-error` fails `tsc` with `Unused '@ts-expect-error' directive` while an unneeded `@ts-ignore` stays silent, and `@typescript-eslint/ban-ts-comment` bans `@ts-ignore` and requires a description on `@ts-expect-error`. To replace a member, assert the replacement instead of erasing the target: `target.method = stub as typeof target.method`, not `(target as any).method = stub`, so `tsc` still rejects a stub whose type does not overlap the original's. Never reach for `any` to unblock feature work "to fix later." + +## Declared `any` beats absorbed `any` — and only one of them is countable + +The two exceptions above are both *declared*: the `any` is written down, an inline disable sits next to it, and a reviewer can find it with `grep`. The dangerous case is the one where **no `any` appears anywhere** and the value is `any` regardless: + +- 🚫 **Absorbed.** A precise annotation on a parameter or return that receives `any` at every call site — `hexValueIsEmpty(value: string | null | undefined)` fed ethers dynamic-method results. `no-explicit-any` never fires, because no `any` was written. `tsc` never fires, because `any` satisfies every annotation. The file reads as checked and none of it is. +- ✅ **Declared.** `): Promise` with an inline disable and a linked issue, as `shared/lib/token-util.ts` does for the same ethers API. Nothing is safer at runtime, but the claim is now honest, greppable, and countable by CI. + +The absorbed form is strictly worse than the declared one, and it is what a JS→TS conversion produces by default: the writer annotates what the value *ought* to be, and `any` accepts the annotation without comment. + +**A conversion is where the boundary's type is chosen, so an absorbed `any` is a decision, not an inheritance.** With `checkJs` off the predecessor asserted nothing; the precise signature is new. "The `any` is pre-existing" is true of the library and false of the annotation next to it. + +Detect it with `IsAny` at the call sites rather than by reading the signature — the probe, its controls, and the `declare module` composition that turns an `any` into a confident `string` are in the `tsc-blindspots` skill. The fix is to type or validate the value where it enters, so the precise annotations downstream are earned. diff --git a/domains/typescript/skills/avoid-widening/skill.md b/domains/typescript/skills/avoid-widening/skill.md new file mode 100644 index 00000000..353f6a8d --- /dev/null +++ b/domains/typescript/skills/avoid-widening/skill.md @@ -0,0 +1,95 @@ +--- +name: avoid-widening +description: >- + Keep a type as narrow as the value it describes. An annotation wider than + what inference already knows throws information away, so annotate only to + add information. Keep immutable lookup tables literal with `as const + satisfies`, type built keys with template literal types and build them + `as const`, and widen on purpose only at a boundary where inference is + unsound. +maturity: experimental +--- + +# Avoid Widening + +Deepens `MetaMask/contributor-docs` [`docs/typescript.md`](https://github.com/MetaMask/contributor-docs/blob/0c297e8a01be7482cc3ff0d047e5e51069adc442/docs/typescript.md#L40-L42), *Avoid unintentionally widening an inferred type with a type annotation*: "Enforcing a wider type defeats the purpose of adding an explicit type declaration, as it _loses_ type information instead of adding it." `derive-types` covers where a type should come from. This skill covers how wide it should be, and `avoid-any` covers the annotation that turns checking off. + +## When To Use + +- Writing or reviewing a type annotation, a `satisfies` clause or an `as const`. +- A lookup table maps one set of names to another, such as trace names or action types. +- A collection is keyed by strings built from known parts, such as `` `${name}:${id}` ``. +- Proposing a type change in review, where a widening suggestion reads as a tightening. + +## Annotate to add information, never to repeat or lose it + +Compare the annotation with the type inference already produces. Equal adds nothing, and wider loses information. Only a narrower annotation, or a deliberate widening at a boundary (below), is worth writing. + +| declaration | resulting type | +|---|---| +| `const name = 'foo'` | `"foo"` | +| `const name: string = 'foo'` | `string`, widened | +| `const ids = [1, 2] as const` | `readonly [1, 2]` | +| `const ids: number[] = [1, 2]` | `number[]`, widened | + +To check a value against a type without widening it, use `satisfies` ([`docs/typescript.md` L134-L136](https://github.com/MetaMask/contributor-docs/blob/0c297e8a01be7482cc3ff0d047e5e51069adc442/docs/typescript.md#L134-L136)). The exception is a mutable object, array or class that code adds to later. There the narrowest type excludes the additions, so annotate with `:` unless the value is meant to be immutable ([L201-L205](https://github.com/MetaMask/contributor-docs/blob/0c297e8a01be7482cc3ff0d047e5e51069adc442/docs/typescript.md#L201-L205)). + +## Keep immutable lookup tables literal + +```typescript +// 🚫 The annotation widens every value to `TraceName`, so `NAMES[name]` no longer says which member it is. +const NAMES: Record = { + 'Feature Open': TraceName.FeatureOpen, + 'Feature Close': TraceName.FeatureClose, +}; + +// ✅ Checks every key and value, keeps each value's member type, and makes the table read-only. +const NAMES = { + 'Feature Open': TraceName.FeatureOpen, + 'Feature Close': TraceName.FeatureClose, +} as const satisfies Record; +``` + +- A plain object literal with neither widens its values to `TraceName`. +- `satisfies` alone already keeps each value's member type when the checked type is a union of literals, as an enum is. What `as const` adds is `readonly`, plus literal types for nested arrays and objects. It also marks the table as immutable, which is the condition under which the extensible-type exception above does not apply. + +## Type a built key by how it is built, and build it `as const` + +```typescript +// ✅ The map accepts only keys of this shape, so a key built from the wrong name does not compile. +const pending = new Map<`${FeatureTraceName}:${string}`, PendingSpan>(); + +const key = `${params.name}:${params.id}` as const; +pending.get(key); +``` + +- A template literal expression infers as `string` unless it is marked `as const` or contextually typed. So narrowing a collection's key type fails with TS2345 at each `get`, `set` and `delete` that passes an unmarked key. Change the key type and every expression that builds a key together. +- Measured with TypeScript 5.6.3 on [metamask-extension#46123 (preload Perps markets on unlock)](https://github.com/MetaMask/metamask-extension/pull/46123) at [`0118bcc256`](https://github.com/MetaMask/metamask-extension/blob/0118bcc25610de7238243c47d8ba37924251e017/app/scripts/controllers/perps/infrastructure.ts#L274-L306). Narrowing only the span map's key type produced five TS2345 errors, and adding `as const` to its two key expressions cleared them. + +## Widen on purpose only where inference is unsound, and only at a boundary + +When a library's types claim more than its runtime guarantees, the correct annotation is wider. luxon types `plus(duration)` as returning `this`, so a valid `DateTime` stays `DateTime` after `plus()`, although the result is invalid past year 275760. Declaring the return as `DateTimeMaybeValid` restores the distinction the library erased. + +That widening works only where the compiler has to use it: + +| form | forces the check? | +|---|---| +| `const x: DateTimeMaybeValid = start.plus(d)` | no, because control-flow analysis re-narrows `x` to its initializer | +| `start.plus(d) satisfies DateTimeMaybeValid` | no, because it checks assignability and keeps the inferred type | +| `start.plus(d) as DateTimeMaybeValid` | yes | +| a parameter or return typed `DateTimeMaybeValid` | yes | + +So the test is whether an annotation adds information inference lacks. Narrowing usually does. Widening does only where inference is wrong about the runtime. + +## Workflow + +1. Before writing `:`, hover or `tsc --noEmit` to see the inferred type, and write the annotation only if it is narrower, or a boundary widening of an unsound type. +2. For a table that code never adds to, use `as const satisfies `. For one that grows, annotate with `:`. +3. For a collection keyed by built strings, type the key as the template literal it is, and add `as const` to every expression that builds one. Typecheck before proposing the change, since the key type and the builders have to change together. +4. In review, run the proposed suggestion through `tsc` before posting it. A narrowing that leaves the builders unmarked fails the build. + +## Related + +- `derive-types` — take a type from its authoritative source instead of restating it. +- `avoid-any` — the annotation that disables checking. +- `tsc-blindspots` — divergence shapes the compiler cannot report, including a widened local. diff --git a/domains/typescript/skills/decompose-large-files/skill.md b/domains/typescript/skills/decompose-large-files/skill.md new file mode 100644 index 00000000..5c4addc2 --- /dev/null +++ b/domains/typescript/skills/decompose-large-files/skill.md @@ -0,0 +1,56 @@ +--- +name: decompose-large-files +description: Decompose a large file into coherent, independently-mergeable modules to improve modularity, maintainability, reviewability, and code organization — and to unblock incremental TS migration. Extract by coherent subject/domain cluster; never fragment for its own sake. +maturity: experimental +--- + +# Decompose Large Files By Coherent Units + +A file that has grown to thousands of lines is hard to review, hard to maintain, and — if it is still `.js` — hard to migrate to TypeScript in one pass. Decompose it by moving coherent subject/domain clusters into their own modules. The goal is **modularity, maintainability, reviewability, and code organization**; unblocking an incremental JS→TS migration is a direct benefit, because each extracted module becomes a small, independently-typable unit. + +Reference application: `metamask-extension` #41735 (`MetamaskController` decomposition, 9,260 → ~3,500 lines). + +## Why decompose first — even if the file could convert in one pass + +Documenting a large file's modularizable boundaries — one coherent unit per ticket — is worth doing **even if the file could somehow be converted and reviewed in a single PR**, because identifying those boundaries is the first logical step of _any_ migration process, human or AI. The boundary map is not throwaway scaffolding; it is the migration's own plan. + +And a single-pass conversion is impractical even for a capable AI — the binding constraint is the file's **context fan-in and fan-out** (upstream source types + downstream consumers), with its line count a minor factor (see `migration-context-cost`). Decomposing shrinks each unit's context to one subject plus its seam, which is what makes the conversion tractable and reviewable at all. + +## The decision that matters: what is a coherent unit? + +Extraction is worth it only when the extracted piece is a **coherent unit that can move independently**. Apply this judgment _before_ proposing any module: + +- **Coherent subject.** The cluster is about one thing — one domain, one lifecycle, one concern (phishing detection, metrics emission, badge rendering). A reader can state its responsibility in one sentence. +- **Independently mergeable.** It can be lifted behind a defined seam — injected dependencies, or a messenger action — without dragging half the file with it. Its coupling to shared state and to the composition root is small and nameable. +- **Not fragmentation.** Extraction for its own sake — splitting a cohesive routine across files, or pulling out a 20-line helper that only one caller uses and has no independent identity — makes the code _harder_ to follow, not easier. If pulling the piece out means the two halves must still change together, leave it inline. **Refactoring is a means to modularity, not an end; a change that raises the file count without raising coherence is a regression.** + +Some code should **stay** in the original file: the thin **composition / bootstrap root** that wires the modules together. Extracting the wiring itself fragments rather than clarifies — it is the one place the whole is assembled. The tell is code that references _everything_ (the central object plus the shared mutable state every cluster reads); relocating it behind a wide "params bag" just moves the tangle. + +## Identifying the boundaries is the key — extraction is optional + +**The key step is identifying the coherent boundaries.** Once you know them, you can convert the file to TypeScript in small, self-contained units — type and review one cluster at a time — instead of holding the whole file at once. That is true whether or not you physically move anything: the boundary map is what makes incremental conversion possible, and documenting it (one unit per ticket) is the deliverable. + +**Extraction — moving a unit into its own module — is optional.** It buys modularity, reviewability, and a bounded per-unit change surface, so it is often worth doing, but you convert in units because you identified the boundaries, not because you moved the code. Extract where the move adds value; leave a cluster in place where it doesn't. + +When you *do* extract a unit, it is one self-contained change — no separate "final deletion" or "integration" ticket: + +1. **Scaffold** the module (its own file/dir, TypeScript from the start). +2. **Port** the bodies in, unchanged in behavior. +3. **Define the seam** — inject the dependencies the module needs (or register its public methods as messenger actions) instead of reaching back into the file's globals. This is where the human judgment is. +4. **Rewire** the call sites to go through the seam. +5. **Delete the original** in the same change; leave no forwarding stub. Then grep the whole repo, not only the file, for every consumer of what the new module replaces. An event that no longer has a listener outside the moved code is dead, and a deletion candidate. +6. **Add a structural unit test** against a stub/mock of the seam — especially valuable when the original file had no tests. + +Steps 1, 2, 5, 6 are largely mechanical (codemod territory — `jscodeshift` on the source, `ts-morph` on the module). Step 3/4 is the part that needs a person. + +## Sizing and sequencing + +- **Size each unit S / M / L / XL** by body size × coupling to rewire. Ship one module (or one subject area) per PR — reviewer context window is usually the binding constraint, so a focused S/M PR merges where an XL one stalls. +- **Sequence lowest-coupling-first.** Extract the clusters with the smallest, cleanest seam first: they establish the pattern and shrink the file so later, more-entangled extractions are easier to see. Save the most coupled cluster (often the core lifecycle) for last, or leave it as the composition root. +- **Enforce the new boundary** so the file cannot silently re-absorb the module — an ESLint `import/no-restricted-paths` rule on the module directory (per #41735). + +## When NOT to decompose + +- The file is large but **already cohesive** — one subject, read top to bottom. Size alone is not a reason. +- The only available splits are **arbitrary** (by line count, or a catch-all `utils`) rather than by subject. That produces fragments, not modules. +- The extraction **cannot get a clean seam** — everything it touches is shared mutable state with the rest of the file. Fix the coupling first, or leave it inline and say so. diff --git a/domains/typescript/skills/derive-types/repos/core.md b/domains/typescript/skills/derive-types/repos/core.md new file mode 100644 index 00000000..b9d24dbb --- /dev/null +++ b/domains/typescript/skills/derive-types/repos/core.md @@ -0,0 +1,102 @@ +--- +repo: core +parent: derive-types +--- + +# Deriving in `core` + +Paths, line numbers and counts below are verified against `origin/main` (2026-08-31), not a working +tree — a local checkout can sit far enough behind that every line number in this file is wrong. + +`core` is where the authoritative types are *written*. Everything in this repo is a source that +`metamask-extension` and `metamask-mobile` derive from, so a loose or unexported type here becomes +a hand-written copy in two consumers. + +## Where the authoritative source lives + +Package granularity. `packages//src/index.ts` is the contract — read it before defining +anything. + +| Source | Where | +| --- | --- | +| Controller state, action union, event union, messenger | `packages//src/Controller.ts`, re-exported from `src/index.ts` | +| Per-method action types | `packages//src/Controller-method-action-types.ts` (generated, 88 files) | +| Get-state action and state-change event builders | `@metamask/base-controller` — `ControllerGetStateAction`, `ControllerStateChangeEvent` | +| Action/event extraction utilities | `@metamask/messenger` — `ExtractActionResponse`, `ExtractActionParameters`, `ExtractEventPayload`, `ActionHandler`, `MessengerActions`, `MessengerEvents` | +| Schema-backed types | `Infer` from `@metamask/superstruct` | + +## The generated action types are the source, not a convenience + +A controller lists its exposed methods in `MESSENGER_EXPOSED_METHODS` (94 files under `packages/` +do). The generator turns each into an action whose handler is an indexed access on the class: + +```typescript +// packages/accounts-controller/src/AccountsController-method-action-types.ts (generated) +export type AccountsControllerGetAccountAction = { + type: `AccountsController:getAccount`; + handler: AccountsController['getAccount']; +}; +``` + +`yarn messenger-action-types:check` runs inside the root `lint` script, so a hand-written +handler signature that disagrees with the method is a lint failure, not a silent drift. Add the +method name to `MESSENGER_EXPOSED_METHODS` and regenerate; never write the action type by hand. + +The state action and change event are derived the same way, from `base-controller` generics +rather than restated: + +```typescript +// packages/accounts-controller/src/AccountsController.ts +export type AccountsControllerGetStateAction = ControllerGetStateAction< + typeof controllerName, + AccountsControllerState +>; +``` + +## Prefer the messenger's own extractors + +`@metamask/messenger` exports `ExtractActionResponse` and `ExtractActionParameters`, +which do what the parent skill's `ReturnType` / `Parameters<...>` do but are +matched to the action shape. Reach for them first inside `core`. + +Measured caveat, so you know what you are adopting: outside `packages/messenger` these are almost +unused — `ExtractEventPayload` has one non-test call site (`AccountsController.ts:933`) and +`ExtractActionResponse` has none. They are exported and correct; they are not yet the prevailing +idiom, and `ReturnType` is what you will find in review. + +## Deriving across a package boundary + +`tsconfig.base.json` sets `composite: true`, and `tsconfig.packages.json` maps +`"@metamask/*": ["../*/src"]`. So a cross-package type import typechecks against the sibling's +**uncompiled source** — no build step stands between you and the authoritative type, and there is +no reason to copy a shape because "the other package isn't built." + +One edit makes a new cross-package derivation legal, and the rest is generated: add the package to +`dependencies` (or `devDependencies`) in `packages//package.json`, then run +`yarn lint:tsconfigs:fix`. `scripts/lint-tsconfigs/lint-package-tsconfigs.mts` derives the expected +`references` in both `tsconfig.json` and `tsconfig.build.json` from the manifest's dependencies and +writes them; `yarn lint:tsconfigs:all` enforces the result inside the root `lint` script. Do not +hand-maintain the `references` arrays. + +Those entries do not affect resolution — `paths` already did that. They order `tsc --build`. + +`packages/bridge-controller/src/types.ts` is the worked example: it derives against types imported +by package name from eleven sibling `@metamask/*` packages, and derives 13 more with +`Infer`. + +## Config that bears on derivation + +- `module` / `moduleResolution` are `Node16`, so package `exports` subpaths resolve natively. +- `strict: true`; `noUncheckedIndexedAccess` is **not** set. `State['someRecord'][key]` is the + value type, not `value | undefined` — a type derived out of a `Record` will claim more than the + runtime guarantees. +- `isolatedModules: true` — re-export derived types with `export type { … }`. +- `@typescript-eslint/no-explicit-any` is `error` (`eslint.config.mjs:152`). The legal escape from + that rule is a too-wide hand-written type, which the parent skill's cast-tax argument covers; + passing the lint rule is not evidence the type was derived. + +## Open + +- Whether `ExtractActionResponse` / `ExtractActionParameters` are intended to become the house + idiom or stay internal to `packages/messenger`. Settled by asking the messenger package owners, + or by a decision record in `MetaMask/decisions`. diff --git a/domains/typescript/skills/derive-types/repos/metamask-extension.md b/domains/typescript/skills/derive-types/repos/metamask-extension.md new file mode 100644 index 00000000..fed595e9 --- /dev/null +++ b/domains/typescript/skills/derive-types/repos/metamask-extension.md @@ -0,0 +1,106 @@ +--- +repo: metamask-extension +parent: derive-types +--- + +# Deriving in `metamask-extension` + +Most authoritative types this repo uses are **not in this repo**. `package.json` carries 134 +`@metamask/*` dependencies at caret ranges, resolving through `node_modules` to built declarations +(`dist/index.d.cts`). There is no `paths` mapping to sibling source. + +Paths, line numbers and counts below are verified against `origin/main` (2026-08-31), not a working +tree — a local checkout can sit far enough behind that every line number in this file is wrong. + +That is what makes deriving non-optional here. Upstream shapes change on a lockfile bump the +extension *performs* but does not *author*. A derived type fails the build at that bump, where you +can see it. A hand-written copy keeps compiling and is now wrong. + +## Where to look, in order + +1. `node_modules/@metamask/` — the published state, action and event types from `core`. +2. `app/scripts/controllers/` — 40 extension-local controllers, including 15 generated + `*-method-action-types.ts` files that derive `handler: Controller['method']`. +3. `shared/types/background.ts` and `ui/store/types.ts` — the already-derived spine below. + +## The spine is already derived — join it, don't re-derive it + +**Background state.** `shared/types/background.ts` builds the redux `metamask` slice type twice and +asserts the two agree: + +- `ControllerStatePropertiesEnumerated` — one indexed access per property into an upstream state + type (`transactions: TransactionControllerState['transactions']`, 255 rows in all). +- `ControllerStateTypesMerged` — the intersection of those same `*State` types. +- `FlattenedBackgroundStateProxy` — `IsEquivalent extends true ? … : never`. + +That last line is a drift alarm, not decoration. When an upstream package adds or removes a state +property, the two sides disagree and the whole redux slice type collapses to `never`, which fails +loudly across the UI. `IsEquivalent` lives in `shared/types/type-level-utils.ts`. + +**Redux state.** `ui/store/types.ts` derives the root from the store rather than declaring it: + +```typescript +type Store = ReturnType; +export type MetaMaskReduxState = ReturnType; +export type MetaMaskReduxDispatch = Store['dispatch']; +``` + +So UI types derive from `MetaMaskReduxState['metamask'][…]`, never from a fresh import of the +controller state — that path already carries the `IsEquivalent` guard. `ui/store/actions.ts` uses +`MetaMaskReduxState['metamask']` as the background-call return type in several places. +`configureStore`'s input type uses `Omit` to override the slices redux +infers as `never`, which is derive-then-override rather than restate. + +**Messenger clients.** `app/scripts/messenger-client-init/controller-list.ts` declares the +`MessengerClient` union over imported controller classes, then +`MessengerClientFlatState = AccountOrderController['state'] & AccountsController['state'] & …`. +`types.ts` derives the rest from the union — `MessengerClientName = MessengerClient['name']`, +`MessengerClientByName` as a mapped type, `MessengerClientPersistedState` from that. + +**Root messenger.** `app/scripts/lib/messenger.ts` derives the root action and event unions from +the factory table: + +```typescript +type ChildMessengers = ReturnType< + (typeof MESSENGER_FACTORIES)[keyof typeof MESSENGER_FACTORIES]['getMessenger'] +>; +export type RootMessengerActions = MessengerActions | DefaultActions; +``` + +Registering a factory in `MESSENGER_FACTORIES` +(`app/scripts/messenger-client-init/messengers/index.ts`) widens the root union with no hand edit. +`messenger-client-init/utils.ts` derives each init function's messenger parameters the same way, +via `ReturnType<(typeof MESSENGER_FACTORIES)[Name]['getMessenger']>`. + +## Config that bears on derivation + +- Extends `@tsconfig/node22`, which supplies `strict`, `skipLibCheck` and `module`/ + `moduleResolution: node16`. Strictness is inherited, not set locally — do not read its absence + from `tsconfig.json` as it being off. Node16 means subpath exports resolve, so + `@metamask/profile-sync-controller/auth` is a legal place to find an authoritative type + (`app/scripts/lib/state-utils.ts:1`). +- `noUncheckedIndexedAccess` is **not** set. Deriving an element type out of a `Record` gives the + value type, not `value | undefined`. +- `isolatedModules: true` — re-export derived types with `export type { … }`. +- `lib: ["DOM", "es2023"]`, `jsx: react`, `allowJs: true`. `allowJs` means a `typeof` derivation can + land on an untyped JS module and silently widen; check what the source file actually is. +- `import-x/no-restricted-paths` is `error` (`.eslintrc.js:838`, architectural zones per ADR 0021 + `modularize-routes`). Deriving across a zone boundary needs `import type` plus the documented + disable — `shared/types/background.ts` opens with exactly that, on the grounds that type imports + are stripped at runtime. +- `@typescript-eslint/no-explicit-any` is `error` (`.eslintrc.js:162`). `Record` + passes that rule and is the shape the parent skill's `getMetaMaskState` example was rejected for; + a clean lint run is not evidence a type was derived. + +## The generated action types are enforced here too + +The 15 `app/scripts/controllers/*-method-action-types.ts` files derive `handler: Controller['method']` +exactly as `core`'s do, and they run the same tool: `@metamask/messenger-cli` (published from +`core`'s `packages/messenger-cli`) supplies the `messenger-action-types` binary, and +`yarn messenger-action-types:check` is the last step of this repo's `lint` script. Regenerate with +`yarn messenger-action-types:generate`; never hand-edit an action's handler signature. Tooling also +knows these files are generated (`.eslintrc.js:872`, `oxfmt.config.mts:13`). + +That the extension and `core` share one generator is the practical form of this skill's cross-repo +point: the action types on both sides of the package boundary are produced from the same class +methods by the same binary, so the only way they can disagree is if someone writes one by hand. diff --git a/domains/typescript/skills/derive-types/repos/metamask-mobile.md b/domains/typescript/skills/derive-types/repos/metamask-mobile.md new file mode 100644 index 00000000..b450bff5 --- /dev/null +++ b/domains/typescript/skills/derive-types/repos/metamask-mobile.md @@ -0,0 +1,125 @@ +--- +repo: metamask-mobile +parent: derive-types +--- + +# Deriving in `metamask-mobile` + +Read via the GitHub API against `MetaMask/metamask-mobile` `main` on 2026-08-31, not from a local +checkout. Every path, line number and count below was re-verified against `main` at that date; +confirm before quoting a line number in review, and never verify one against a working tree. + +As in the extension, most authoritative types are upstream: `package.json` carries 132 +`@metamask/*` dependencies at caret ranges. Mobile and the extension sit on **different versions of +the same packages**, so a shape copied by hand here diverges from the same shape copied by hand +there, and neither repo can see the other drift. Deriving is what keeps both pinned to whatever +`core` actually says. + +## Where the authoritative source lives + +`app/core/Engine/types.ts` is the hub. Two hand-maintained maps sit at the top, and everything else +in the file is derived from them: + +- `MessengerClients` (line 854) — controller name to controller **class**, one row each. +- `EngineState` (line 979) — controller name to the upstream `*State` type, one row each. + +The rows are written by hand; the values are imported, not restated. So the shapes are already +authoritative — what is hand-maintained is the *membership* of the maps. + +Everything downstream derives: + +| Derived type | Line | Form | +| --- | --- | --- | +| `MessengerClientName` | 1060 | `keyof MessengerClients` | +| `MessengerClient` | 1065 | `MessengerClients[MessengerClientName]` | +| `MessengerClientsByName` | 1068 | mapped over `MessengerClientName` | +| `RequiredControllers` | 560 | `Omit` | +| `OptionalControllers` | 576 | `Pick` | +| `EngineContext` | 971 | `RequiredControllers & Partial` | +| `MessengerClientPersistedState` | 1188 | mapped, `MessengerClientsByName[Name]['state']` | +| `MessengerClientMessengersByName` | 1198 | `typeof MESSENGER_FACTORIES` | +| `Permissions` | 589-590 | `ReturnType[keyof …]` | + +`MESSENGER_FACTORIES` itself is at `app/core/Engine/messengers/index.ts`. + +## No completeness guard on the engine maps + +The extension asserts its two constructions of background state agree, and collapses the type to +`never` when they do not. Mobile has no equivalent: a GitHub code search for `IsEquivalent` across +this repo returns 0 hits (positive control — `MessengerClientsByName` returns 4 — so the search is +reaching the code). + +Consequence for derivation work here: adding a controller and forgetting its `EngineState` row +typechecks. Nothing fails. Derive *from* `EngineState` freely; do not assume `EngineState` is +complete because the build is green. + +## Redux runs the opposite direction from the extension + +`app/reducers/index.ts:66` declares `export interface RootState` by hand, with +`engine: { backgroundState: EngineState }` at line 71, and passes it *into* +`combineReducers` at line 193. The extension derives its root state out of the +configured store; mobile supplies its root state to the store. + +So when deriving a UI type here, `RootState['engine']['backgroundState'][…]` does reach the upstream +controller state types and is the right path. The rest of `RootState` — the ~24 non-engine slices +under `app/reducers/` — is a hand-written boundary, and a derivation that bottoms out there has +reached a declaration rather than a source. + +## The strongest derivation in the repo + +`app/messengers/ui-messenger.ts` derives the entire UI-facing action and event surface from +`GlobalActions` / `GlobalEvents` by transformation rather than by restatement — asynchronizing every +handler with `infer`, filtering to JSON-serializable actions with a conditional on +`Parameters`, and deriving the exclusion list off a const array +(`(typeof MESSENGERS_WITH_EXCLUSIONS)[number]['EXCLUDED_CAPABILITIES']['actions'][number]`). It is +also the only file across the three repos that uses `ExtractActionResponse` from +`@metamask/messenger`. Copy its shape when a consumer needs a *modified* view of an upstream union; +the alternative is re-declaring the union with the modification baked in. + +## `moduleResolution` is `node`, and that hides exported types + +`tsconfig.json` sets `"moduleResolution": "node"`, not `Node16`. Package `exports` subpaths +therefore do not resolve, and the repo shims each one it needs with an explicit `paths` entry into +`node_modules/@metamask//dist/**/*.d.cts` — `@metamask/keyring-api/v2`, +`@metamask/perps-controller/types`, `@metamask/delegation-controller/types` and others, under a +`// TODO: Remove these once we use Node16 module resolution.` comment. + +**Before concluding an authoritative type is not exported, check `paths`.** A type that lives behind +a subpath will not resolve until an entry exists, and the failure looks like the type not existing. +Adding the `paths` entry is the fix; hand-writing the shape because the import "doesn't work" is the +failure this skill is about. + +## Other config that bears on derivation + +- `strict: true` set explicitly; `noUncheckedIndexedAccess` **not** set, so an element type derived + out of a `Record` is the value type, not `value | undefined`. +- `lib: ["es2022"]` with **no `DOM`**. A derived type that transitively references a DOM type will + not resolve here even though the same derivation compiles in the extension, whose `lib` includes + `DOM`. This is the most common way a type that works upstream fails to land in mobile. +- `skipLibCheck: true` — errors inside upstream `.d.cts` files are not reported. The derived type is + still checked where you use it, so this suppresses noise rather than the signal you want. +- `isolatedModules: true` — re-export derived types with `export type { … }`. +- `target: esnext`, `module: commonjs`, `jsx: react-native`, `allowJs: true`. As in the extension, + `allowJs` means a `typeof` derivation can land on an untyped JS module and widen silently. +- `@typescript-eslint/no-explicit-any` is `error` (`.eslintrc.js:170`); a too-wide hand-written type + passes it. + +## The generated action types come from the same binary as `core`'s + +Generated `*-method-action-types.ts` files exist here (imported by +`app/core/Engine/controllers/rewards-controller/` and +`app/components/UI/Predict/controllers/PredictController.ts`, and recognized in `.eslintrc.js` and +`.eslintignore`), and they are enforced: `yarn messenger-action-types:check` is the second half of +this repo's `lint` script, running the `messenger-action-types` binary from +`@metamask/messenger-cli ^0.2.0` — the same tool, from `core`'s `packages/messenger-cli`, that +`core` and the extension run. Regenerate with `yarn messenger-action-types:generate`. + +So all three repos derive their action handlers from the same class methods with one shared +generator. A hand-written handler signature is the only thing that can put them out of step. + +## Open + +- The selector layer was not surveyed. Whether mobile's selectors derive from + `RootState['engine']['backgroundState']` or re-import controller state types directly is unknown, + and it decides which path this skill should point at for UI work. Settled by a sweep of + `app/selectors/`. diff --git a/domains/typescript/skills/derive-types/skill.md b/domains/typescript/skills/derive-types/skill.md new file mode 100644 index 00000000..9c67238b --- /dev/null +++ b/domains/typescript/skills/derive-types/skill.md @@ -0,0 +1,66 @@ +--- +name: derive-types +description: Derive types from authoritative sources (indexed access, `typeof`, `ReturnType`/`Parameters`, `Pick`/`Omit`, `Infer`) instead of hand-writing ad-hoc types that duplicate, run too wide or too narrow, and drift. +maturity: experimental +--- + +# Derive Types From Authoritative Sources + +Deepens the TypeScript guidance in `mms-coding-guidelines`, and is the structural counterpart to the contributor-docs rule *Prefer type inference over annotations and assertions* (`MetaMask/contributor-docs` `docs/typescript.md`). Where inference handles values, derivation handles types that reference other types. + +## Derive, don't re-declare + +When a type already exists at an authoritative source — a controller's state type, a function's return, a library's exported type, a schema/struct — **derive from it** rather than restating it. Derive with indexed access (`State['field']`), `typeof`, `ReturnType` / `Parameters` / `Awaited`, utility types (`Pick` / `Omit` / `Partial`), and `Infer`; let inference carry the rest. Inference covers an omitted return annotation, `as const` on a literal, and `satisfies` where a `:` annotation would widen the inferred type. How narrow the result should be, including literal lookup tables and built keys, is `avoid-widening`. Inferred and derived types stay "responsive to changes in code," while hand-written declarations "rely on hard-coding, making them brittle against code drift." + +Over a union, `Pick` and `Omit` work on `keyof` the whole union, which is only the keys every member shares, so `Omit` loses each member's own keys. Branch first with a distributive conditional type: `T extends unknown ? Omit : never` applies `Omit` to each member. + +An ad-hoc type — one hand-defined to describe a value an authoritative type already describes — carries three dangers: + +- **Duplication.** The same shape is stated twice; every reader reconciles them and every change touches both. +- **Incorrect in either direction.** A hand-written type is a _guess_ at the source's shape, and the guess can miss either way. Too wide, it admits values the authoritative type would reject, so invalid data still type-checks. Too narrow, it drops a case the source allows, such as a `| undefined`, and erases the compiler's record of why a runtime guard exists (see the five divergence shapes in `tsc-blindspots`). +- **Drift.** The source evolves; the copy does not. Because it is hand-written rather than derived, the compiler cannot flag the divergence — the bug surfaces at runtime, not at build. + +## A grounded example (`metamask-extension` #42583) + +A `wallet-services` module hand-rolled a messenger type, re-declaring each controller action's signature and **hand-copying its return shape** inline. + +🚫 Reinvents the controller's messenger and re-states its action returns: + +```typescript +type TokenResolutionMessenger = { + call( + action: 'AssetsContractController:getTokenStandardAndDetails', + address: string, + // … + ): Promise< + | { + balance?: string | number | bigint | { toString(radix?: number): string }; + decimals?: string | number | bigint | { toString(radix?: number): string }; + standard?: string; + symbol?: string; + } + | undefined + >; + // the other action's return is discarded entirely: + call(action: 'AssetsContractController:getBalancesInSingleCall' /* … */): Promise; +}; +``` + +✅ Derive each return from the controller's exported action type; don't hand-copy it: + +```typescript +import type { AssetsContractControllerGetTokenStandardAndDetailsAction } from '@metamask/assets-controllers'; + +// the action already types its own return — derive it +type TokenDetails = ReturnType< + AssetsContractControllerGetTokenStandardAndDetailsAction['handler'] +>; +``` + +The messenger itself should be a `Messenger` from `@metamask/messenger` parameterized with those exported action types, so every `call` signature comes from the controller rather than a hand-rolled overload. The hand-rolled version is worse than a plain duplicate: one return is hand-copied (already looser than the controller's real type), the other (`Promise`) discards the type entirely. + +The same PR also typed a dependency `getMetaMaskState: () => Record`, which forced every consumer to re-cast the shape by hand — including a `{ metamask: getMetaMaskState() } as never` double-cast. That downstream cast tax is what a too-wide type always imposes; deriving the dependency from the authoritative state type deletes it. Notably the same file _did_ derive one type correctly (`type Action = (typeof ACTIONS)[number]`), so the pattern was already in hand — the discipline is extending it to every referenced type. + +## Rule + +Before writing a type, ask where the value comes from and whether that source already types it. If it does, derive. Define a fresh type only when no authoritative source exists — a genuinely new shape at a boundary you own. Before defining, exhaust deriving: search the internal `@metamask/*` packages and the consuming repo for the authoritative source first. diff --git a/domains/typescript/skills/migration-context-cost/skill.md b/domains/typescript/skills/migration-context-cost/skill.md new file mode 100644 index 00000000..f5d7bd19 --- /dev/null +++ b/domains/typescript/skills/migration-context-cost/skill.md @@ -0,0 +1,26 @@ +--- +name: migration-context-cost +description: A file's JS→TS migration cost is driven mostly by context fan-in (upstream types it must read to derive from) and fan-out (downstream files that import it and must be updated), not by its line count. Scope and sequence migration tickets by it. +maturity: experimental +--- + +# TypeScript Migration Context Cost — Fan-In and Fan-Out + +A file's line count is a factor in how hard it is to convert to TypeScript, but usually not the dominant one. The dominant cost is the **context the conversion pulls in** — everything the converter, human or AI, must read or touch to do it correctly. + +## The two axes are different kinds of cost + +- **Fan-in (upstream source types) — a _reading_ cost.** To type the file's values correctly you must load every upstream module whose types the code should _derive_ from: controller state, function returns, library exports, schemas (see `derive-types`). You read these; you do not change them, and they are usually already TypeScript. A file that derives from many sources is expensive to hold in context even if it is short. +- **Fan-out (downstream consumers) — a _change_ cost.** Re-typing the file ripples to every module that imports it; each may need its own update. These are files you actually edit, so fan-out — together with the file itself — is the **change and review surface**. A widely-imported file has a large change surface even if it is small. + +The two are not the same kind of cost: fan-in is what you must _read_ to get the types right; fan-out is what you must _modify_. A 200-line hub imported by 100 files can be a larger migration than a self-contained 2,000-line leaf. + +## Why it matters even for AI + +Single-pass conversion of a high-fan-in/fan-out file is impractical even for a capable AI: it must hold the target file in context **and** read every upstream source-type file **and** edit every downstream consumer. That context load plus change surface — not the model's ability to read the file itself — is the binding constraint. + +## How to use it + +- **Scope tickets by context cost more than LOC.** Before sizing a migration ticket, estimate fan-in (how many upstream types it must derive from) and fan-out (`grep -rl` importer count). Size by the read-context plus the change surface, and treat line count as a minor factor. +- **Sequence low-fan-out first.** Convert leaf / low-fan-out files early — few downstream edits per PR. Files whose upstream types are already TypeScript are cheaper on the fan-in side and give later conversions more typed ground to derive from. +- **When the change surface won't fit one PR, reduce it first.** A hub with high fan-out is the case to decompose: split it into coherent units so each unit's fan-out — and thus each PR — is bounded (see `decompose-large-files`). Decomposition is one response to high context cost, not the only place the cost applies. diff --git a/domains/typescript/skills/tsc-blindspots/references/false-negatives.md b/domains/typescript/skills/tsc-blindspots/references/false-negatives.md new file mode 100644 index 00000000..fdb5e906 --- /dev/null +++ b/domains/typescript/skills/tsc-blindspots/references/false-negatives.md @@ -0,0 +1,349 @@ +# The standing blind spots — where `tsc` returns a false negative + +Defects the compiler cannot report, independent of any particular PR. Unlike the +restated-type class, these need no substitution to find: they are properties of +the language and the config, and they are present in every file. + +**Verified, not asserted.** The demonstration file below was typechecked against +`metamask-extension` at `7fafda0` with the repo's own `tsconfig.json`: + +``` +$ NODE_OPTIONS='--max-old-space-size=9216' npx tsc -p tsconfig.json --noEmit +$ echo $? +0 +``` + +Every block in it is wrong. `tsc` reports **zero errors** on all ten. + +--- + +## A. Unsoundness in the type system + +### 1. Index access is not `| undefined` + +```ts +const parts = host.split('.'); +return parts[9]; // typed `string`; `undefined` at runtime +``` + +`arr[i]` and `record[key]` are typed as if the element always exists. Easy to underestimate how much surface this covers: every `.split()[n]`, every +lookup table, every `find`-then-index is an instance. + +- **Flag:** `noUncheckedIndexedAccess` (not enabled in `metamask-extension`). +- **In review:** any index or dynamic key access on a path that can be empty or + short. `parts[parts.length - 1]` is only safe if the array is provably non-empty. + +### 2. Optional property vs. explicit `undefined` + +```ts +type PopupState = { currentPopupId?: number }; +const cleared: PopupState = { currentPopupId: undefined }; // accepted +``` + +`?:` means "may be absent"; without the strict flag it *also* accepts +present-and-undefined. Code that distinguishes the two (`'k' in obj`, +`Object.keys().length`, serialization that drops vs. writes `null`) breaks on a +distinction the type cannot express. + +- **Flag:** `exactOptionalPropertyTypes` (not enabled). +- **In review:** persisted state and message payloads, where absent and + `undefined` serialize differently. + +### 3. Method parameters are bivariant + +```ts +type MessageHandler = { handle(msg: { kind: 'booted' | 'connectivity' }): void }; +const narrow: MessageHandler = { handle(msg: { kind: 'booted' }) { … } }; // accepted +``` + +`strictFunctionTypes` makes function *properties* contravariant but exempts +**method shorthand** — deliberately, for DOM/array compatibility. So a handler +that only accepts a narrow subtype satisfies a wide handler type and receives +values it declared it would not. + +- **Fix:** declare callbacks as properties (`handle: (msg: …) => void`), which + *is* checked. +- **In review:** any interface with method-shorthand callbacks, especially + message/event handlers. + +### 4. Arrays are covariant + +```ts +const bases: Base[] = specials; // Special[] → Base[], accepted +bases.push(new Base()); // `specials` now holds a non-Special +``` + +- **In review:** a narrower array widened and then mutated. `readonly T[]` blocks it. + +### 5. Excess-property checking only fires on fresh literals + +```ts +const draft = { url: 'a', justification: 'b', reasosn: ['typo'] }; +const params: CreateParams = draft; // no error — not a fresh literal +``` + +Assigning the literal directly would catch the typo. Through a variable, the +extra property is silently ignored — and the intended one is missing. + +- **In review:** config/params objects built up in a variable before being passed. + This is how a misspelled option key survives to runtime. + +### 6. Structural typing erases domain distinctions + +```ts +type AccountAddress = string; +type TransactionHash = string; +fetchBalance(txHash); // accepted — both are `string` +``` + +Aliases are not nominal. Two semantically incompatible values are interchangeable +whenever their structure matches. + +- **Fix:** branded types, or a template-literal type where the format differs + (`Hex` = `` `0x${string}` `` genuinely does discriminate). +- **In review:** same-primitive parameters, especially adjacent ones in a + signature, where swapping the arguments would still compile. + +## B. Boundaries the compiler does not cross + +### 7. `any` absorbs any annotation + +```ts +declare function readPersisted(key: string): any; +const meta: VaultMeta = readPersisted('meta'); // asserted, never validated +meta.version.toFixed(2); // may be a string at runtime +``` + +An `any` satisfies every annotation silently. Sources: untyped dependencies, +`JSON.parse`, generics that default to `any`, `as any` — and the one that hides +best in a typed-looking file: **dynamic methods on an ethers `Contract`**, where +the ABI is runtime data so every call returns `any` while reading as an ordinary +typed `await`. This is a known source in `metamask-extension` +([#31973](https://github.com/MetaMask/metamask-extension/issues/31973)): +`shared/lib/token-util.ts` declares it as `Promise` with a disable comment, +which is the honest form. The dangerous form is letting it infer — nothing +annotates it, so it propagates into the module's return type unflagged. + +- **In review:** trace where a confidently-typed value *entered* the program. If + it entered as `any`, its type is a wish. +- **Also trace where an `any` *exits*.** The bullet above looks backwards from a + suspicious value; this looks forward from a module's public surface. Assign its + return to two impossible types, with a known-typed sibling as a control: + + ```ts + const { type, hash } = await resolveEnsToIpfsContentId(args); + const a: number = hash; // compiles ⇒ `hash` is any + const c: symbol = hash.whateverIWant.deeply; // compiles ⇒ ditto + const d: number = type; // MUST error ⇒ probe can discriminate + ``` + + If the nonsense compiles and the control errors, `any` is escaping into every + caller. **The control line is not optional** — without it, a probe that reports + nothing is indistinguishable from a probe that cannot fail. Run it under the + project's `tsconfig`, not a standalone `tsc` invocation, or missing ambient + declarations and `resolveJsonModule` will produce errors that are the harness + rather than the finding. +- **Precise signatures around an `any` source make it worse, not better.** A file + with a derived provider type and `hexValueIsEmpty(value: string | null | undefined)` + reads as a checked boundary while every value crossing it is unchecked. A + migration that adds those signatures is what creates the appearance. +- **To ask "is this specific value `any`", use `IsAny` rather than a nonsense + assignment.** The exit probe above traces a module's public surface; this answers + the question at any single site, and its verdict is a compile error rather than an + absence of one: + + ```ts + type IsAny = 0 extends 1 & T ? true : false; + + const resolverAddress = await registryContract.resolver(hash); + const a1: IsAny = true; // silent ⇒ it IS any + const known = 'x' as string; + const a3: IsAny = true; // MUST error ⇒ probe discriminates + ``` + + `0 extends 1 & T` holds only for `any`, because `1 & any` is `any` and every type + extends `any`. **Read the polarity carefully: silence is the finding here**, which + is the reverse of every other probe in this file — so the known-`string` control + is what separates "this value is `any`" from "the probe never ran." + + Verified against `metamask-extension` with the repo's own `tsconfig.json`: four + arms — an ethers dynamic method and an explicit `as any` both silent, a known + `string` and a laundered `string` (below) both `TS2322`. + +### 8. Ambient `declare module` is an unverified assertion + +```ts +declare module '@ensdomains/content-hash' { + const contentHash: { decode: (h: string) => string /* … */ }; + export default contentHash; +} +``` + +Hand-written module declarations are believed unconditionally — nothing compares +them to the package. Getting a return type wrong here is invisible forever, and +the declaration is **global**, so it also shadows any real types the package +later ships. + +- **In review:** verify the declaration against the installed package at runtime — + faster and more decisive than reading source: + + ```bash + node -e 'const m = require("@ensdomains/content-hash"); + console.log(Object.keys(m), "default:", typeof m.default); + console.log(typeof m.decode(m.encode("ipfs-ns", "Qm…")));' + ``` + + Check three things: **the exports exist**, **each declared signature's return + `typeof` matches**, and **whether `export default` is legitimate** — a CJS module + with `typeof m.default === 'undefined'` still warrants a default declaration *if* + `esModuleInterop` is on, and is a defect if it is not. Prefer `@types/*` or an + upstream PR over hand-writing. + +#### 7 + 8 compose: a declaration launders `any` into a confident type + +The two blind spots above are usually audited apart, and the defect lives in their +composition. §7 says an `any` propagates; §8 says a hand-written declaration is +believed. Put them in sequence and the propagation **stops** — replaced by a type +nobody checked, which every reader downstream then trusts: + +| hop | site | resulting type | who asserted it | +|---|---|---|---| +| 1 | `await resolverContract.contenthash(hash)` | `any` | ethers `readonly [key: string]: ContractFunction \| any` | +| 2 | `contentHash.getCodec(rawContentHash)` | **`string`** | a hand-written `declare module` | +| 3 | `return { type, hash: decoded }` | `{ type: string; hash: any }` | inferred from hop 2 | +| 4 | `` `https://${hash}.${type.slice(0, 4)}.${gateway}` `` | `.slice` on a `string` | inferred from hop 3 | + +Hop 2 is declared `(contentHash: string) => string` and **called with an `any`** — +so it neither rejects its input nor earns its output. By hop 4 the value is a URL +segment, and the only claim it was ever a string is a line a human wrote. + +- **Audit rule:** for every `declare module`, list its call sites and run `IsAny` on + each **argument**. A parameter declared `string` and passed `any` is the laundering + point, and it is where the annotation or the validator belongs — not at hop 4, + where the value already looks trustworthy. +- Verified in the demonstration run above: hop 1 silent under `IsAny` (it is `any`), + hop 2 `TS2322` (it is `string`), with the declaration block supplied locally so the + only variable between the two arms is the `declare module`. + +### 9. External data is asserted, not validated + +```ts +const chainId = (rpcResult as { chainId: string }).chainId.slice(2); +``` + +Every `as` on data crossing a boundary — RPC responses, `fetch().json()`, +`chrome.storage` reads, persisted state written by an *older version of the app* — +is a claim the compiler cannot evaluate. + +- **In review:** highest stakes for persisted state and migrations, where the real + input was produced by code that no longer exists. A runtime validator + (`@metamask/superstruct`, zod) is the only thing that actually checks. + +### 10. A JS caller is not checked at all + +With `checkJs` off (the default, and the case in `metamask-extension`), a type +written for a function whose callers are still `.js` is compared against no call +site, ever. See the restated-type class in the main skill — this is why that class +exists. + +## C. Config-level blind spots + +Check these before trusting a green build. Values shown are `metamask-extension` +at the time of writing. + +| Setting | Effect | Here | +|---|---|---| +| `skipLibCheck` | Errors *inside* `.d.ts` files are suppressed, including conflicts between library types | **`true`** (from `@tsconfig/node22`) | +| `exclude` | Excluded files are never typechecked | `**/*.stories.tsx`, `**/*.stories.ts` | +| `include` | Anything outside it is invisible to `tsc` | `app`, `development`, `shared`, `test`, `types`, `ui`, `*.ts` | +| `checkJs` | Off ⇒ `.js` callers unchecked | unset | +| `noEmit` + bundler | `tsc` never produces the shipped artifact; webpack/swc transpiles **without** typechecking, so a type error cannot break the build — only the separate `lint:tsc` job reports it | `noEmit: true` | +| `@ts-expect-error` / `@ts-ignore` | Point suppressions | grep before trusting a clean file, then read each hit: which diagnostic it hides, and whether the path under review reaches that line | + +The last row is worth stating plainly: **type errors do not break the build.** They +break a CI job. If that job is skipped, filtered, or its output is not read, the +types were never checked at all. + +--- + +## The demonstration file + +Drop this anywhere inside the `include` paths and typecheck. Zero errors is the +expected — and alarming — result. + +```ts +/* eslint-disable @typescript-eslint/no-explicit-any, @typescript-eslint/no-unused-vars */ + +// 1. Index access is not `| undefined` +function lastSegment(host: string): string { + const parts = host.split('.'); + return parts[9]; // typed `string`; `undefined` at runtime +} +export const seg = lastSegment('foo.eth').toUpperCase(); + +// 2. Record lookup claims the value always exists +declare const gateways: Record; +export const gw = gateways.definitelyNotAKey.url; + +// 3. Optional property accepts an explicit undefined +type PopupState = { currentPopupId?: number }; +const cleared: PopupState = { currentPopupId: undefined }; +export const idPlusOne = (cleared.currentPopupId ?? 0) + 1; + +// 4. Method-shorthand parameters are bivariant +type MessageHandler = { handle(msg: { kind: 'booted' | 'connectivity' }): void }; +const narrow: MessageHandler = { handle(msg: { kind: 'booted' }) {} }; +export { narrow }; + +// 5. Arrays are covariant +class Base {} +class Special extends Base { + special() { + return 1; + } +} +const specials: Special[] = [new Special()]; +const bases: Base[] = specials; +bases.push(new Base()); +export const boom = () => specials.map((s) => s.special()); + +// 6. Excess-property checking only fires on fresh literals +type CreateParams = { url: string; justification: string }; +const draft = { url: 'a', justification: 'b', reasosn: ['typo'] }; +export const params: CreateParams = draft; + +// 7. `any` absorbs any annotation +declare function readPersisted(key: string): any; +type VaultMeta = { version: number; storageKind: 'data' | 'split' }; +export const meta: VaultMeta = readPersisted('meta'); +export const ver = meta.version.toFixed(2); + +// 8. An ambient `declare module` is an unverified assertion +import contentHash from '@ensdomains/content-hash'; + +export const decoded: string = contentHash.decode('0x'); + +// 9. Structural typing erases domain distinctions +type AccountAddress = string; +type TransactionHash = string; +declare function fetchBalance(addr: AccountAddress): Promise; +declare const txHash: TransactionHash; +export const wrong = fetchBalance(txHash); + +// 10. A type assertion on external data is unchecked by construction +declare const rpcResult: unknown; +export const chainId = (rpcResult as { chainId: string }).chainId.slice(2); +``` + +## How to use the catalog in a review + +Don't run all ten as a checklist. Pick by what the diff touches: + +- **New indexing / destructuring** → 1, 2 +- **New message, event, or callback types** → 3, 5, 6 +- **New `declare module`, new dependency, `@types` change** → 8, then **7 + 8** on + its call sites — a declaration is audited against the package by default and + against its *arguments* almost never +- **Anything reading persisted state, storage, or an RPC response** → 7, 9 +- **A JS→TS conversion** → 10, plus the restated-type class in the main skill +- **Any PR whose safety argument is "CI is green"** → section C, first diff --git a/domains/typescript/skills/tsc-blindspots/references/metamask-extension.md b/domains/typescript/skills/tsc-blindspots/references/metamask-extension.md new file mode 100644 index 00000000..a4e56593 --- /dev/null +++ b/domains/typescript/skills/tsc-blindspots/references/metamask-extension.md @@ -0,0 +1,57 @@ +# Repo notes — metamask-extension + +Specifics for running the two-arm proof in `MetaMask/metamask-extension`. The +split against `repos/metamask-extension.md` is by kind rather than by repo: which +blind spots bite this repo, and where they live, go in the overlay; run mechanics +— heap size, probe placement, timings — stay here. + +## Typecheck invocation + +```bash +NODE_OPTIONS='--max-old-space-size=9216' npx tsc -p tsconfig.json --noEmit +``` + +`package.json`'s `lint:tsc` uses `--max-old-space-size=6144`, and CI's `Test lint` +job ran it over the full project at that heap and exited 0 (`main` at +`0269fae48a`, 2026-09-14). A local full run can still **OOM**, and the OOM exits +non-zero with no type diagnostics, so a naive exit-code check reads it as "errors +found." If it does, raise the heap and read the output. A full run takes roughly +3–5 minutes. + +## Where to put probes + +Anywhere under the `include` list — `app`, `development`, `shared`, `test`, +`types`, `ui`. `app/scripts/derive-probe/` works. Delete it afterwards; it is +inside the build's include paths. + +## What the compiler is *not* checking + +- **`checkJs` is unset** and there is no `// @ts-check` in `app/scripts/background.js`. + `background.js` is the sole caller of much of `app/scripts/lib/**`, so a type + written for those functions is validated against **nothing**. This is where + migration PRs accumulate silent divergence, and where this skill pays. +- `tsconfig.json` sets `lib: ["DOM", "es2023"]`, overriding the base. **`webworker` + is absent**, so service-worker globals (`clients`, `Clients`, `Client`) have no + authoritative type in scope — hand-declaring them is legitimate here. +- Strictness comes from `@tsconfig/node22` (`strict: true`), so `strictNullChecks` + is on and nullability divergences do surface in a probe. + +## Authoritative sources worth knowing + +| Looking for | Derive from | +|---|---| +| current chain id | `ReturnType` (`shared/lib/selectors/networks.ts`) → `Hex`, not `string` | +| the EIP-1193 provider | `ReturnType['provider']` — note the `\| undefined` | +| a controller method's params | `SomeController['methodName']` | +| persisted state root | `MetaMaskStorageStructure` (`shared/lib/stores/base-store.ts`) | +| a `browser.*` listener payload | `browser.WebRequest.OnErrorOccurredDetailsType` and siblings, from `webextension-polyfill` | +| offscreen message targets/events | the enums in `shared/constants/offscreen-communication.ts` | + +## Before "fixing" a `chrome.*` type error + +Read the declaration in `node_modules/@types/chrome/index.d.ts` first. Several +parameters are template-literal **string** types, not the enums they mirror — e.g. +`ContextFilter.contextTypes?: ` `` `${ContextType}`[] `` and +`CreateParameters.reasons: ` `` `${Reason}`[] ``. A plain string literal already +satisfies them, so swapping in `chrome.offscreen.Reason.X` is an unnecessary +runtime change, not a type fix. diff --git a/domains/typescript/skills/tsc-blindspots/references/worked-example.md b/domains/typescript/skills/tsc-blindspots/references/worked-example.md new file mode 100644 index 00000000..a7f1f978 --- /dev/null +++ b/domains/typescript/skills/tsc-blindspots/references/worked-example.md @@ -0,0 +1,92 @@ +# Worked example — a JS→TS migration PR + +[metamask-extension#44397](https://github.com/MetaMask/metamask-extension/pull/44397), +head `7fafda0`. 11 files converted, +153/−72, described as *"mostly mechanical +JS→TS with equivalent runtime logic"*, four files *"rename only"*. All CI green, +including `lint:tsc`. + +Twelve hand-written types. Nine had an authoritative source. Five disagreed with it. + +## Arm A + +``` +$ NODE_OPTIONS='--max-old-space-size=9216' npx tsc -p tsconfig.json --noEmit +$ echo $? +0 +``` + +Silent — so every diagnostic below is attributable to the substitution. + +## Arm B + +Six probe files, each substituting the derived type and calling it as the real +code does: + +``` +probe-1-get-obj-structure.ts(19,47): error TS2345: Argument of type + 'MetaMaskStorageStructure | undefined' is not assignable to parameter of type + 'Record'. +probe-2-set-current-popup-id.ts(22,21): error TS2345: Argument of type 'undefined' + is not assignable to parameter of type 'number'. +probe-2-set-current-popup-id.ts(29,21): error TS2345: Argument of type + 'number | undefined' is not assignable to parameter of type 'number'. +probe-3-ens-provider.ts(27,32): error TS18048: 'provider' is possibly 'undefined'. +probe-3-ens-provider.ts(44,14): error TS2322: Type + 'SwappableProxy> | undefined' is not + assignable to type 'HandWrittenEthProvider'. +probe-4-offscreen-message.ts(43,14): error TS2322: Types of property 'target' are + incompatible. Type 'string' is not assignable to type 'OffscreenCommunicationTarget'. +probe-6-chain-id-widening.ts(31,50): error TS2322: Type '"1"' is not assignable to + type '`0x${string}`'. +``` + +A seventh probe compiled the PR's *original* string literals with **zero** errors — +which is how the two unnecessary runtime changes below were established. + +## Findings + +| Hand-written | Authoritative source | Shape | +|---|---|---| +| `_setCurrentPopupId: ((id: number \| undefined) => void)` | `AppStateController['setCurrentPopupId']` → `(id: number) => void` | widening | +| `getCurrentChainId: () => string` | `ReturnType` → `` `0x${string}` `` | widening | +| `target: string` on a received message | the sender, TypeScript in-repo → `OffscreenCommunicationTarget` | widening | +| `EthProvider` (written twice, unshared) | `ReturnType['provider']` | duplication + dropped nullability | +| `obj: Record` | already in a JSDoc `@type` on the argument at the call site: `MetaMaskStorageStructure \| undefined` | placeholder + dropped nullability | + +Two of these had a consequence beyond tidiness: + +- The widened setter is what let `setter?.(undefined)` compile. Deriving it fails — + usefully, because it surfaces a genuine mismatch between a controller method's + declared parameter and how its callers actually use it. +- The dropped nullability sat in the same edit that deleted a `= {}` default + parameter, i.e. the guard that existed *for* the nullable case. (Traced: inert + today, because a fallback upstream guarantees an object by the time the path runs.) + +**Separately**, reading `@types/chrome` before trusting a type error found two +runtime changes the types never required: `contextTypes` and `reasons` are declared +as template-literal **string** types (`` `${ContextType}`[] ``, `` `${Reason}`[] ``), +so the original `['OFFSCREEN_DOCUMENT']` / `['IFRAME_SCRIPTING']` already compiled. +The PR replaced both with runtime enum lookups, and added a redundant cast. + +## Clearances — and one that mattered + +Five claims the probes **cleared**: + +- `Promise` as a provider `request` return looked unsound. It isn't: the + real `request` is generic in its result, so a call + site may legitimately fix `Result = string`. +- Two `declare module` blocks: neither package ships types, and no `@types/*` is + installed → no authoritative source exists, so hand-writing is correct. +- A hand-declared `clients?: { matchAll }`: the authoritative `Clients` lives in + `lib.webworker.d.ts`, which is not in this repo's `tsconfig.lib` → out of scope. +- Two dependency callbacks (`() => string`, `() => boolean`) matched their sources + exactly. +- A dropped argument (`_getPopup(id)` → `_getPopup()`) was a genuine no-op: the + base function declared no parameters, so the argument was already discarded. + +**The provider clearance is the reason Step 5 exists.** The first Arm B run +reported `TS18048: 'provider' is possibly 'undefined'` on that probe — an error on +the line the return-type claim lived on, which reads as confirmation if you count +exit codes. The nullability error fired one property *ahead* of the claim. Setting +it aside with `NonNullable<…>` and re-running showed the return type compiles +clean. Reported as a finding, it would have been wrong. diff --git a/domains/typescript/skills/tsc-blindspots/repos/core.md b/domains/typescript/skills/tsc-blindspots/repos/core.md new file mode 100644 index 00000000..1374ba68 --- /dev/null +++ b/domains/typescript/skills/tsc-blindspots/repos/core.md @@ -0,0 +1,233 @@ +--- +repo: core +parent: tsc-blindspots +--- + +# Blind spots — core + +What `MetaMask/core`'s configuration does **not** check. One of the parent skill's +classes does not exist here and two exist only here, so do not carry the extension +or mobile playbook across unchanged. + +Every line number and count below was read from `origin/main` +(`git show origin/main:`), not from a working tree. This matters more here +than in the other two: a local branch a few hundred commits behind reports a +different package count and different reference lists. + +## The JS boundary does not exist here — say so, do not omit it + +`tsconfig.base.json` does not set `allowJs`, and it is absent from the resolved +options for a package (`npx tsc -p packages/assets-controllers/tsconfig.json +--showConfig`). Independently, `packages/*/src` contains **0** `.js` files against +**2,052** `.ts`. + +So the parent skill's central premise — *"across a JavaScript boundary (`checkJs` +off) it checks nothing at all"* — has no instance in core. A review that reports +"the callers are still `.js`, so this type is validated against nothing" is wrong +here, and a `checkJs` finding copied from an extension review does not transfer. + +State this explicitly in a core review rather than leaving it out. + +## The repo-wide typecheck covers 14 of 98 packages + +The largest blind spot here, and it is not a compiler setting — it is which files +the compiler is pointed at. + +`lint` begins with `lint:tsc`, which is `tsc --build tsconfig.lint.json`, and CI +runs it as a matrix entry in `.github/workflows/lint-build-test.yml:91`. But +`tsconfig.lint.json` declares its own `references` — **14 entries** — and that is +the whole graph: `references` is not inherited through `extends`, so extending +`tsconfig.json` (99 entries, 98 unique) does not widen it. Checked with a two-config +fixture and `--showConfig`: a child declaring one reference over a base declaring +two resolves to **one**, and a child declaring none resolves to **no `references` +key at all** — overridden, never merged. Independently, exactly **14** of the **98** +packages have a `tsconfig.lint.json` for it to point at. The config says so itself: *"This configuration +incrementally enables repository-wide type checking."* + +The 14: `announcement-controller`, `app-metadata-controller`, `base-controller`, +`build-utils`, `client-controller`, `foundryup`, `local-node-utils`, `messenger`, +`messenger-cli`, `platform-api-docs`, `preferences-controller`, +`rate-limit-controller`, `stellar-quickstart-up`, `storage-service`. + +The other 84 are typechecked by the **Build** job instead +(`lint-build-test.yml:194-231`), which runs `ts-bridge` over a tsconfig generated +from the changed packages (`scripts/generate-partial-build-tsconfig.mts`), falling +back to `yarn build` over all 98. Different entry point, different config chain, +different scope per PR. + +**So "core is green" is a claim about one of two graphs, and neither is the whole +repo on every run.** Name the command you ran. Before reporting that a probe was +silent, confirm the package you probed is in the graph you invoked — a probe in one +of the 84 is not covered by `lint:tsc` at all. + +## Unchecked indexing — and it is not uniform across packages + +`tsconfig.base.json` does not set `noUncheckedIndexedAccess`, so `record[key]` and +`arr[i]` yield `T`, never `T | undefined`. But three configs opt in: + +| Config | Line | +|---|---| +| `packages/json-rpc-engine/tsconfig.json` | :9 | +| `packages/eth-json-rpc-provider/tsconfig.json` | :9 | +| `tsconfig.scripts.json` | :19 | + +That is **2 of the 98 package tsconfigs**, plus the scripts config. Read the +package's own file before concluding an index expression is unchecked — the answer +differs by directory, which no other MetaMask repo here requires you to check. + +The same expression appears both guarded and unguarded in two files of one package, +`packages/assets-controllers`: + +```ts +// packages/assets-controllers/src/TokenBalancesController.ts:590-592 (also :606-608) +const networkConfig = networkConfigurationsByChainId[chainId]; +const { networkClientId } = + networkConfig.rpcEndpoints[networkConfig.defaultRpcEndpointIndex]; + +// packages/assets-controllers/src/AccountTrackerController.ts:629-636 +return popularEvmChainIds + .map((hexChainId) => { + const networkConfig = networkConfigurationsByChainId[hexChainId]; + return networkConfig?.rpcEndpoints[ + networkConfig.defaultRpcEndpointIndex + ]?.networkClientId; + }) + .filter((id): id is NetworkClientId => id !== undefined); +``` + +Two index operations each, both typed non-nullable under `strict: true`. The second +site guards both with `?.` and then needs the type-predicate `.filter` to remove +the `undefined` it introduced by doing so; the first guards neither and destructures +straight through. `tsc` has no opinion about either, so consistency on this axis is +a review property in core, not a checked one. + +### A package's source is checked under stricter settings than its emitted types + +`packages/json-rpc-engine/tsconfig.json` sets `noUncheckedIndexedAccess` and +`exactOptionalPropertyTypes`; its `tsconfig.build.json` sets neither — it extends +`tsconfig.packages.build.json`, which does not. **0 of the build configs set +either.** So a package's source is checked under stricter settings than the `.d.ts` +consumers read is emitted under. + +**Open question:** whether that asymmetry changes any emitted signature — an +inferred return type of a function returning `arr[i]` is the candidate. I did not +test it. Settle it by building the package and diffing the emitted `dist/*.d.ts` +against one emitted with the flags added to `tsconfig.build.json`. + +## Bodiless ambient shims — verified `any` at live call sites + +Ten files under `types/` are a single line of the form `declare module 'x';` with no +body, which types the entire module `any`. Every package includes them — +`"include": ["../../types", "./src", "../../tests"]`. + +Two cases, needing different remedies: + +**1. The shim overrides a package that ships its own types.** +`@metamask/metamask-eth-abis` has `"types": "dist/index.d.ts"` in its +`package.json`, and `types/@metamask/metamask-eth-abis.d.ts` shadows it. Verified +with the parent skill's substitution method, replicating a package's arrangement +(`strict`, `module`/`moduleResolution` `Node16`, `target ES2020`, +`lib ES2020 + DOM`, core's `node_modules`): + +```ts +import { abiERC20 } from '@metamask/metamask-eth-abis'; +import contractMap from '@metamask/contract-metadata'; +type IsAny = 0 extends 1 & T ? true : false; +const abiIsAny: IsAny = true; +const mapIsAny: IsAny = true; +``` + +- With `types/**/*.d.ts` in `include`: **clean**, and flipping either to `false` + errors `TS2322` — so both are `any`. +- With `types/` removed from `include`: `abiIsAny` errors (`Type 'true' is not + assignable to type 'false'`) — the shipped declarations resolve and `abiERC20` is + **not** `any`. + +The probe ran against an older checkout, but both inputs match `main`: the shim file +is byte-identical, and `main` declares `@metamask/metamask-eth-abis@^3.1.1` against +the probed 3.1.1 with `types: dist/index.d.ts`. + +The shim is what makes it `any`, at these importers under +`packages/assets-controllers/src/` — `Standards/ERC20Standard.ts:6`, +`TokensController.ts:31`, `Standards/NftStandards/ERC1155/ERC1155Standard.ts:11`, +`Standards/NftStandards/ERC721/ERC721Standard.ts:11` — and outside that package at +`packages/bridge-controller/src/bridge-controller.ts:8` and +`packages/bridge-controller/src/utils/balance.ts:5`. This is parent §7 false +precision with a source you can delete: remove the shim. + +**2. The shim stands in for a package with no types.** +`@metamask/contract-metadata` has no `types`/`typings` field; with `types/` removed +the import is `TS7016`. Hand-writing is the parent's Step 2 case 7 — correct in +principle. The defect is that the shim asserts `any` rather than a shape, so +`contractMap` is `any` at `packages/assets-controllers/src/TokensController.ts:16`, +`.../TokenDetectionController.ts:10` and +`packages/client-utils/src/mappers/helpers/token-metadata.ts:1`. +`single-call-balance-checker-abi` is the same case, at +`.../AssetsContractController.ts:19`. + +Two gates that cannot see either case. `@typescript-eslint/no-explicit-any` is +`'error'` in `eslint.config.mjs`, under the comment *"Enable rules that are disabled +in `@metamask/eslint-config-typescript`"* — and no `any` is written anywhere, so it +has nothing to match. (That re-enable is not distinctive: extension and mobile do +the same, over the same shared default.) And `skipLibCheck` does not help either +way — a bodiless `declare module` is well-formed, so checking declarations finds +nothing wrong with it. + +## Project references — two entry points that resolve differently + +`composite: true` in `tsconfig.base.json`. Every package tsconfig lists its +dependencies under `references`, and every `tsconfig.build.json` references the +other packages' `tsconfig.build.json`. This is a class the other two repos do not +have, and the first thing to get right is which config you ran. + +**The `paths` mapping is not inherited by every entry point.** +`tsconfig.packages.json` sets `"@metamask/*": ["../*/src"]`, commented *"we ensure +that TypeScript resolves `@metamask/*` imports to the uncompiled source code."* + +| Config | Extends | Carries the `src` mapping | +|---|---|---| +| `packages/*/tsconfig.json` | `tsconfig.packages.json` | yes | +| `packages/*/tsconfig.build.json` | `tsconfig.packages.build.json` → `tsconfig.packages.json` | yes | +| `tsconfig.json` (root, `noEmit`) | `tsconfig.base.json` **directly** | **no** | + +So the root config is the one whose cross-package imports fall through to normal +`Node16` resolution and the package's `types` field, and the per-package configs — +lint *and* build alike — are the ones pointed at source. + +**Open question:** which file each entry point actually loads for a cross-package +import, since project references also redirect to declaration output. I did not +test it. Settle it before citing any typecheck as evidence about a cross-package +type: + +```bash +npx tsc -p packages//tsconfig.json --explainFiles | grep -i '' +npx tsc --build tsconfig.lint.json --verbose --traceResolution 2>&1 | grep -i '' +``` + +**Open question:** whether `tsc --build` here can report clean over an out-of-date +`dist/*.d.ts`. Both `build:types` and `lint:tsc` use `--build`, and `build:types` +already passes `--verbose`, which prints the up-to-date decision per project — read +that log rather than the exit code. + +What holds regardless of the resolution question: **consumers outside the repo read +`dist/*.d.ts`**, and no check inside core exercises that path from a consumer's +position. Mobile goes further and hardcodes paths *into* that `dist/` in its own +`tsconfig.json` (see the mobile overlay). A type correct against `src` and stale in +`dist` is invisible here and lands there. + +**Every typecheck that actually runs sets `skipLibCheck`.** Both +`tsconfig.packages.lint.json` and `tsconfig.packages.build.json` set it `true`, as +does the root `tsconfig.lint.json`. The per-package `tsconfig.json` — the one an +editor picks up for a file under `packages/*/src` — does not. So declarations are +checked in the editor and skipped by both CI paths, which is the wrong way round +for catching a bad `.d.ts`. + +## Probe note + +`lib` is `["ES2020", "DOM"]`, so a DOM-typed probe compiles here. It does not in +mobile. `module` and `moduleResolution` are `Node16`, matching extension and not +mobile — a probe is portable between core and extension, and is not portable to +mobile without rewriting its imports. + +Put the probe in a package that is in the graph you are running. In one of the 84 +packages outside `tsconfig.lint.json`, `yarn lint:tsc` will not read it. diff --git a/domains/typescript/skills/tsc-blindspots/repos/metamask-extension.md b/domains/typescript/skills/tsc-blindspots/repos/metamask-extension.md new file mode 100644 index 00000000..00cdcef0 --- /dev/null +++ b/domains/typescript/skills/tsc-blindspots/repos/metamask-extension.md @@ -0,0 +1,208 @@ +--- +repo: metamask-extension +parent: tsc-blindspots +--- + +# Blind spots — metamask-extension + +What this repo's configuration does **not** check, and what that means for the +parent skill's five divergence shapes. How to *run* the two-arm proof here — heap, +probe location, the authoritative-source table, the `chrome.*` caveat — is +[references/metamask-extension.md](../references/metamask-extension.md). + +Every line number and count below was read from `origin/main` +(`git show origin/main:`), not from a working tree. + +## `tsconfig.json` does not show you the strictness + +The local file names exactly one strictness flag, `useUnknownInCatchVariables`. +Everything else arrives through `"extends": "@tsconfig/node22/tsconfig.json"`. +Resolve it before concluding anything is off: + +```bash +npx tsc -p tsconfig.json --showConfig +``` + +That prints `strict`, `strictNullChecks`, `strictFunctionTypes`, `noImplicitAny`, +`alwaysStrict`, `strictBindCallApply`, `strictPropertyInitialization` and +`strictBuiltinIteratorReturn` all `true`, plus `target: es2022` and +`skipLibCheck: true` — none of which appear in `tsconfig.json`. + +**The failure this causes is under-reporting.** An auditor greps the local file for +`strict`, finds nothing, and downgrades a real nullability divergence to "tsc +wouldn't have caught it anyway." It would: `strictNullChecks` is on, so a dropped +`| undefined` (parent shape 2) *does* surface in a probe here. + +`noUncheckedIndexedAccess` is genuinely absent — the repo has three tsconfigs +(`tsconfig.json`, `development/webpack/tsconfig.webpack.json`, +`test/e2e/playwright/llm-workflow/tsconfig.json`) and none sets it. + +## 300 Storybook files are outside `tsc` *and* outside the lint rule + +The strongest blind spot in this repo, because it removes both checks at once and +neither absence is visible from the file you are reviewing. + +`tsconfig.json` excludes them, with the reason stated: + +```jsonc +"exclude": [ + // don't typecheck stories, as they don't yet pass the type checker. + "**/*.stories.tsx", + "**/*.stories.ts" +], +``` + +**300** `.stories.ts`/`.stories.tsx` files on `main`, all under `include` paths. + +The lint half follows from how the config is built. `.eslintrc.js:26-28` parses the +tsconfig with the TypeScript API — + +```js +const tsconfigPath = ts.findConfigFile('./', ts.sys.fileExists); +const { config } = ts.readConfigFile(tsconfigPath, ts.sys.readFile); +const tsconfig = ts.parseJsonConfigFileContent(config, ts.sys, './'); +``` + +— and `.eslintrc.js:148` scopes the block carrying `no-explicit-any` to +`files: tsconfig.fileNames.filter((f) => /\.tsx?$/u.test(f))`. **The rule's scope +is the tsconfig program.** A file excluded from the tsconfig is excluded from the +rule by construction. + +The repo documents the consequence at `.eslintrc.js:687-688`, in the comment above +the `**/*.stories.js` override: *"This block is for overriding settings from the +base config. It's JavaScript-only because the Storybook TypeScript files don't have +the base config applied."* The one block that does match `.stories.ts(x)` +(`.eslintrc.js:674-683`) sets only `color-no-hex` and `storybook/no-redundant-story-name` +— it does not restore anything. + +So in a `.stories.tsx` file: no `tsc`, no `no-explicit-any`, and no +`no-unsafe-*` (those are off repo-wide anyway, below). A hand-written type there is +unfalsifiable in the parent skill's Step 1 sense, and an `any` there is invisible to +every gate. Note the asymmetry — the 156 `.stories.js` files **do** get the base +config; only the TypeScript ones lost it. + +**For review:** a migration PR that adds or edits a `.stories.tsx` has had none of +its types checked. Say so rather than treating a green CI as coverage. + +## The JS boundary, sized + +`allowJs: true` and `checkJs` unset. The parent reference explains why +`app/scripts/background.js` matters; the number is the part worth knowing. Counts +under `include` (`app`, `development`, `shared`, `test`, `types`, `ui`) on `main`: + +| | Count | +|---|---| +| `.js` | 1,182 | +| `.ts` / `.tsx` | 7,374 (of which **300** are `.stories.ts(x)`, excluded above) | +| `.js` carrying `@ts-check` | **1** (`development/lib/build-type.js`) | + +Roughly one included file in seven asserts nothing and is checked against nothing. +A type hand-written for a function whose callers are all in that seventh is +unfalsifiable — probe it, but expect Arm B to stay silent, and report that as +*unconstrained* rather than as *cleared*. + +## Unchecked indexing, in one function + +`noUncheckedIndexedAccess` is off, so `record[key]` and `arr[i]` both yield `T`, +never `T | undefined`. `shared/lib/network.utils.ts:238-256` shows the whole class +in nineteen lines: + +```ts +const enabledEip155Networks = + enabledNetworkMap[KnownCaipNamespace.Eip155] ?? {}; // :243 guard the author wrote + +const chainIds = Object.entries(enabledEip155Networks) + .filter(([_chainId, isEnabled]) => isEnabled) + .map(([chainId, _isEnabled]) => chainId) as Hex[]; // :247 escape hatch on the key type + +return chainIds + .map((chainId) => networkConfigurationsByChainId[chainId]) + .filter((config) => config !== undefined) // :251 guard the author wrote + .map( + (config) => + config.rpcEndpoints[config.defaultRpcEndpointIndex].networkClientId, // :254 no guard + ); +``` + +The record lookup at `:250` is typed `NetworkConfiguration`, so the `!== undefined` +filter at `:251` is guarding against something the compiler says cannot happen — the +author supplied it from knowledge of the data. Two lines later the array index at +`:254` is dereferenced immediately with no equivalent guard, and the compiler asked +for neither. If `defaultRpcEndpointIndex` is ever out of range the throw is at +runtime and `tsc` is green. + +**For review:** a hand-written guard on an index expression is evidence the author +knew the lookup could miss. Ask why the sibling index in the same expression has +none. `shared/lib/selectors/networks.ts:88` is the same shape with the cast form — +`networkConfigurationsByChainId[chainId as Hex]` on a parameter declared `string`. + +## `skipLibCheck: true` — declarations are not checked + +Inherited from the base. Every `.d.ts` is exempt: the eleven files in `types/` and +every dependency's declarations. + +`types/lavamoat__lavadome-core.d.ts` is a bodiless `declare module +'@lavamoat/lavadome-core';`, which types the entire module `any`. The package ships +no types (no `types`/`typings` field, no `.d.ts` in the package), so hand-writing is +the parent's Step 2 case 7 — correct in principle. The defect is that the shim +asserts `any` where it could assert a shape. I found no `.ts`/`.tsx` importer under +`app`, `shared` or `ui`; a `.js` importer would be invisible to `tsc` regardless. + +## The lint layer changes what you can grep for + +`.eslintrc.js:162` sets `@typescript-eslint/no-explicit-any` to `'error'`, in the +override at `.eslintrc.js:148` scoped to the tsconfig program. + +**That is a re-enable, not a repo quirk.** The shared config +`@metamask/eslint-config-typescript` sets `no-explicit-any` to `'off'`, and all +three repos here turn it back on locally — extension at `.eslintrc.js:162`, mobile +at `.eslintrc.js:170`, core in `eslint.config.mjs` under the comment *"Enable rules +that are disabled in `@metamask/eslint-config-typescript`"*. Do not build a contrast +out of it. + +**Consequence for parent §7 (false precision):** you cannot find an absorbed `any` +by grepping for `any`, because writing one is a lint error. The rule pushes authors +to `as` instead, which is why the parent's "escape hatches are the tell" section is +the productive search here. Use `IsAny` at the call sites, not a grep. + +### The five `no-unsafe-*` rules are off in all three repos + +The shared config also sets `no-unsafe-argument`, `no-unsafe-assignment`, +`no-unsafe-call`, `no-unsafe-member-access` and `no-unsafe-return` to `'off'`, +across the majors each repo resolves (13.0.0 mobile, 14.1.1 extension, 15.0.0 core). + +Those are precisely the rules that report an `any` *flowing into* a typed position — +the thing `no-explicit-any` cannot see because no `any` was written. So the +repo-wide posture is: writing `any` is an error, and letting one arrive and spread +is unreported. That is the parent skill's §7 in configuration form, and it is why +this skill has work to do in all three repos. + +### Locally disabled rules + +`.eslintrc.js:278-296` disables a block, commented *"removing changes to our shared +ESLint config made after version v9 … TODO: Remove these modifications after the +ESLint v9 update"* — temporary, so a finding these would have caught is not a +decision to accept risk. + +| Rule | Line | Note | +|---|---|---| +| `no-floating-promises` | :284 | also off in mobile (`.eslintrc.js:199`) — shared posture, not local | +| `no-unsafe-enum-comparison` | :289 | **redundant** — the shared config already disables it in every resolved major | +| `no-unsafe-function-type` | :291 | the genuinely local disable | + +`no-unsafe-enum-comparison` being off — whether locally or via the shared config — +is what matters for this skill: it is the lint counterpart of the parent's first +divergence shape, so a migration that widens an enum-valued field to `string` and +compares it to an enum member is green in **both** `tsc` and lint. Lint silence is +not a clearance for that shape. + +## Probe note + +`incremental: true` with `tsBuildInfoFile: +node_modules/.cache/typescript/tsconfig.tsbuildinfo` means a cache persists between +Arm A and Arm B. **Open question:** whether that cache can mask a probe diagnostic +under `--noEmit`. I did not test it. If the two arms disagree in a way that does not +track the probe, delete that file and re-run before reporting anything. + +A probe file must be inside `include` **and** not match `**/*.stories.ts(x)`, or it +is silently outside the program. diff --git a/domains/typescript/skills/tsc-blindspots/repos/metamask-mobile.md b/domains/typescript/skills/tsc-blindspots/repos/metamask-mobile.md new file mode 100644 index 00000000..84a5bf44 --- /dev/null +++ b/domains/typescript/skills/tsc-blindspots/repos/metamask-mobile.md @@ -0,0 +1,170 @@ +--- +repo: metamask-mobile +parent: tsc-blindspots +--- + +# Blind spots — metamask-mobile + +**Scope of this file.** `tsconfig.json`, `.eslintrc.js` and `package.json` were read +from `main` via `gh api repos/MetaMask/metamask-mobile/contents/`, so every +config claim below is verified. The repo is **not checked out**, so there are **no +verified call sites** — statements about actual code are open questions with the +command that settles them. Add examples on the first real review; the extension and +core overlays show the form. + +## Strictness is local — the extension's audit trap does not apply + +`strict: true` is set in `tsconfig.json` itself, not inherited. A reader auditing +that file sees it, which is the opposite of extension, where `strict` arrives +through `@tsconfig/node22` and is invisible locally. + +Two flags that matter here are still not part of `strict`: + +- **`noUncheckedIndexedAccess` is not set.** Next section. +- **`skipLibCheck: true` is set**, so no `.d.ts` is checked — the same posture as + extension, and the same as every typecheck core actually runs. + +## Unchecked indexing + +`record[key]` and `arr[i]` yield `T`, never `T | undefined`. Under `strict: true` +this is the one nullability hole left open, and it is the one that reaches runtime — +as a property access on `undefined`, not as a type error. + +The source roots are `app/`, `tests/` and `scripts/` (from `include`; `app/**/*` is +the bulk). Find an index expression dereferenced immediately, and a hand-written +runtime guard on an index expression sitting beside one that has none: + +```bash +grep -rnE '\]\.[a-zA-Z]' app --include=*.ts --include=*.tsx | grep -v '\.test\.' +grep -rnE '\?\.\[|\]\?\.' app --include=*.ts --include=*.tsx | grep -v '\.test\.' +``` + +A guard the compiler did not ask for is evidence the author knew the lookup could +miss; the sibling index without one is the finding. Both core and extension carry +that exact pair on `networkConfigurationsByChainId[chainId]` followed by +`rpcEndpoints[defaultRpcEndpointIndex]`. **Open question:** whether it appears here. +Settle with `grep -rn "rpcEndpoints\[" app --include=*.ts`. + +## Hand-written `paths` into dependency `dist/` — a class the other two lack + +`tsconfig.json` maps roughly twenty subpath imports directly at declaration files +inside `node_modules`, with the reason stated: *"TODO: Remove these once we use +`Node16` module resolution."* `moduleResolution` is `node`, which cannot follow the +`exports` subpaths these packages publish, so the paths are written out by hand: + +```jsonc +"@metamask/json-rpc-engine/v2": ["node_modules/@metamask/json-rpc-engine/dist/v2/index.d.cts"], +"@metamask/keyring-api/v2": ["node_modules/@metamask/keyring-api/dist/v2/index.d.cts"], +"@metamask/perps-controller/types": ["node_modules/@metamask/perps-controller/dist/types/index.d.cts"], +``` + +Each is a hand-written claim about another package's build output — the same class +the parent skill is built around, one level up from a type to a file path. Two ways +it fails silently: + +1. **The path stops resolving.** A dependency reorganises `dist/`, and the mapping + points at nothing. Whether that surfaces as an error or as a fallback to normal + resolution is worth knowing before you trust a green run. +2. **The path resolves to a stale or wrong declaration.** These reach *into* core's + emitted `dist/`, which core's own checks do not exercise from a consumer's + position (see the core overlay). A type correct against core's `src` and stale in + its `dist` is invisible in core and lands here. + +**For review:** a PR touching one of these packages' subpath exports should be +checked against this list. **Open question:** whether any mapped path is currently +dead — settle with a loop over the `paths` values testing each file exists, then +`npx tsc -p tsconfig.json --traceResolution` for one of them. + +## No DOM lib — this changes how probes are written, not just what compiles + +`lib: ["es2022"]`, with `jsx: react-native`. Nothing from `lib.dom.d.ts` is in +scope from `lib`, so a probe borrowing a DOM type fails for a reason that has +nothing to do with the claim under test — the parent skill's Step 5 failure, and +here it is the default outcome of copying a probe from an extension review. + +`typeRoots` is unset, so every `@types/*` package is auto-included, and +`@types/node@^24` is a devDependency. Node-declared globals therefore have a +source; DOM-only ones (`document`, `window`, `HTMLElement`, `Event`) do not. +**Open question:** which specific globals resolve. Settle each in one line — a +scratch file inside the `include` paths, then run `tsc`: + +```ts +type _Probe = HTMLElement; // TS2304 if absent +type _Probe2 = typeof fetch; // TS2304 if absent +``` + +Do the same before using any DOM type in a real probe. Do not infer it from the +other two overlays; core has `DOM` in `lib` and extension has `DOM` plus `es2023`. + +## The JS boundary is live + +`allowJs: true` and **`checkJs` is not set**, so `.js` files are in the program and +checked against nothing — the same posture as extension. A hand-written type for a +function whose callers are all `.js` can drift indefinitely. + +`app/core/InpageBridgeWeb3.js` and `scripts/inpage-bridge/dist` are explicitly +excluded, so they are not even in the program. + +Size the boundary before weighting a finding, the way the extension overlay does: + +```bash +find app -name '*.js' | wc -l +find app \( -name '*.ts' -o -name '*.tsx' \) | wc -l +grep -rl '@ts-check' app --include=*.js | wc -l +``` + +## The lint layer + +`lint:tsc` is `tsc --project ./tsconfig.json` with a 12 GB heap — one whole-program +typecheck, unlike core, where the equivalent script covers a fraction of the repo. +`lint` runs `eslint '**/*.{js,ts,tsx}'` **by glob**, not by tsconfig program, so +mobile does not have extension's excluded-files-lose-the-rule problem. + +`.eslintrc.js:170` sets `@typescript-eslint/no-explicit-any` to `'error'` in the +`*.{ts,tsx}` block at `:155`. **That is a re-enable, not a repo quirk** — the shared +`@metamask/eslint-config-typescript` sets it `'off'`, and all three repos turn it +back on. Consequence: you cannot find an absorbed `any` by grepping for `any`. Use +`IsAny` at the call sites (parent §7), and read escape-hatch clusters as the index. + +The same shared config sets all five `no-unsafe-*` rules (`-argument`, +`-assignment`, `-call`, `-member-access`, `-return`) to `'off'` at major 13.0.0, +which mobile resolves. Those are the rules that report an `any` *flowing into* a +typed position — so writing `any` is an error here and letting one arrive and spread +is unreported. + +The `*.{ts,tsx}` block disables three rules that matter to this skill: + +| Rule | Line | +|---|---| +| `no-floating-promises` | :199 | +| `no-unsafe-enum-comparison` | :206 | +| `restrict-template-expressions` | :221 | + +`no-unsafe-enum-comparison` is the lint counterpart of the parent's divergence +shape 1 — a field widened from an enum to `string` and compared against an enum +member is green in **both** `tsc` and lint. Lint silence is not a clearance there. + +Two of the three are re-enabled in a narrow override: `.eslintrc.js:471` scopes +`files: ['app/**/*-method-action-types*.ts']`, and that block sets +`no-floating-promises` and `restrict-template-expressions` to `'error'` at +`:628-629`. That is generated-file territory, not a repo-wide restoration — do not +read it as either rule being on. + +## Probe note + +`module: commonjs` and `moduleResolution: node`, against `Node16` in both extension +and core; `jsx: react-native` against `react`. A probe is **not** portable from +those repos without rewriting its imports. `isolatedModules: true` additionally +requires `import type` / `export type` for type-only positions — a probe that +re-exports a type without `type` errors for the wrong reason, the parent's Step 5 +again. + +## Open questions, collected + +| Question | Settles it | +|---|---| +| Does the `record[k]` then `arr[i]` pair appear? | `grep -rn "rpcEndpoints\[" app --include=*.ts` | +| How big is the `.js` surface? | `find app -name '*.js' \| wc -l` | +| Which DOM-named globals resolve? | one-line `type _P = X;` probe per global | +| Is any `paths` mapping into `node_modules/**/dist` dead? | test each mapped file exists, then `--traceResolution` | +| Are there per-directory tsconfigs overriding the root? | `find . -name 'tsconfig*.json' -not -path '*/node_modules/*'` | diff --git a/domains/typescript/skills/tsc-blindspots/scripts/substitution-ab.sh b/domains/typescript/skills/tsc-blindspots/scripts/substitution-ab.sh new file mode 100755 index 00000000..32d76bd1 --- /dev/null +++ b/domains/typescript/skills/tsc-blindspots/scripts/substitution-ab.sh @@ -0,0 +1,51 @@ +#!/usr/bin/env bash +# +# Two-arm type proof: does a hand-written type agree with the authoritative one? +# +# substitution-ab.sh [probe-dest] +# +# repo checked out at the PR head, deps installed +# directory of probe-*.ts files (see skill.md Step 3) +# [probe-dest] where to stage them, relative to ; must sit inside +# the tsconfig `include` paths. Default: src/__type-probe__ +# +# Arm A must be silent. If it is not, stop — nothing in Arm B is attributable. +set -uo pipefail + +REPO=${1:?usage: substitution-ab.sh [probe-dest]} +PROBES=${2:?usage: substitution-ab.sh [probe-dest]} +DEST=${3:-src/__type-probe__} + +: "${NODE_OPTIONS:=--max-old-space-size=9216}" +export NODE_OPTIONS + +cd "$REPO" || exit 1 +[ -d "$PROBES" ] || { echo "no such probe dir: $PROBES" >&2; exit 1; } + +cleanup() { rm -rf "$REPO/$DEST"; } +trap cleanup EXIT INT TERM + +echo "=== Arm A — PR head as written (must be silent) ===" +A=$(npx tsc -p tsconfig.json --noEmit 2>&1) +A_STATUS=$? +if [ -n "$A" ]; then + echo "$A" + echo + echo "!! Arm A is NOT silent (exit $A_STATUS). The comparison is INCONCLUSIVE:" + echo "!! Arm B's diagnostics cannot be attributed to the substitution." + echo "!! Fix the baseline (toolchain, lockfile, heap, project scope) before reading Arm B." + exit 2 +fi +echo "0 diagnostics — baseline clean, Arm B is attributable." +echo + +echo "=== Arm B — same commit, derived types substituted ===" +mkdir -p "$DEST" +cp "$PROBES"/probe-*.ts "$DEST"/ 2>/dev/null || { + echo "no probe-*.ts found in $PROBES" >&2; exit 1; } + +npx tsc -p tsconfig.json --noEmit 2>&1 +echo +echo "=== Each diagnostic above is a disagreement the hand-written type concealed. ===" +echo "=== Before believing any of them: confirm the diagnostic is the one the ===" +echo "=== claim needs, not an earlier cause short-circuiting it (skill.md Step 5).===" diff --git a/domains/typescript/skills/tsc-blindspots/skill.md b/domains/typescript/skills/tsc-blindspots/skill.md new file mode 100644 index 00000000..75395b8a --- /dev/null +++ b/domains/typescript/skills/tsc-blindspots/skill.md @@ -0,0 +1,326 @@ +--- +name: tsc-blindspots +description: >- + Find the type defects `tsc` is structurally unable to report — a green build is + not evidence the types are correct. Covers the two classes: (1) hand-written + types that restate an authoritative source and disagree with it, caught by + substituting the derived type at a fixed commit and diffing `tsc` output; and + (2) the standing blind spots in the language and config — unchecked array/record + indexing, bivariant method parameters, covariant arrays, `any` absorption at + untyped boundaries, precise signatures fed `any` at every call site, ambient + `declare module` assertions that launder an `any` into a confident type, + excess-property checks that only fire on fresh literals, and external data + asserted rather than validated. Also audits typing edits that quietly change + runtime behavior: + stripped `| undefined`, deleted default parameters, literals swapped for runtime + enum lookups, calls made optional so a throw becomes a silent no-op. Use when + reviewing a JS→TS migration, a PR that hand-writes types for values that already + have them, a "rename-only" refactor, or any PR claiming a change is mechanical. + Triggers on /mms-tsc-blindspots, or on phrases like "validate this TypeScript + migration", "is this type right", "does this type match the real shape", + "why didn't CI catch this type", "derive vs define", and "what can tsc not + check". +maturity: experimental +--- + +# TypeScript compiler blind spots + +A hand-written type is a **claim about a value's shape**, and it compiles whether +or not the claim is true. `tsc` checks declarations for internal **consistency** — +never for **correspondence** to the source they restate. Across a JavaScript +boundary (`checkJs` off) it checks nothing at all. + +Those are the blind spots. This skill finds what is hiding in them. + +Two classes, two methods: + +| Class | What it is | Method | +|---|---|---| +| **Restated types** | A type hand-written to describe a value that already has an authoritative type | **Substitution A/B** — swap in the derived type, diff `tsc` output | +| **Standing blind spots** | Defects the language and config cannot report at all, in any codebase | **Targeted audit** — [references/false-negatives.md](references/false-negatives.md) | + +The second class is the one that surprises people: a file of genuinely broken +code can typecheck clean. The reference includes exactly that — a demonstration +file where every block is wrong and `tsc` reports zero errors. + +Companion to the authoring rule it enforces — *derive types from authoritative +sources instead of re-declaring them* in +[contributor-docs `docs/typescript.md`](https://github.com/MetaMask/contributor-docs/blob/main/docs/typescript.md). +This skill is the review-side proof; that document is the write-side guidance. + +## When to use + +- A **JS→TS migration** PR, or one whose body says *mechanical*, *rename-only*, or *no behavior change*. +- A PR that **hand-writes a type for a value that already has one** — a controller method's parameters, a selector's return, a message payload, a package's exported shape. +- A reviewer asks "is this type actually right?" and the answer so far is "it compiles." + +Out of scope: code correctness (use a normal review), runtime behavior (use e2e / +visual proof), and lint/format (CI owns those). + +## Prerequisites + +- The repo checked out at the PR head, dependencies installed, `tsc` runnable. +- Enough heap for a full typecheck on large repos (see Troubleshooting). +- A scratch directory inside the `tsconfig` `include` paths for probe files. + +## The core idea: two arms, one commit + +Both arms sit at the **same commit**. They differ by a *substitution*, not by a +ref — so there is no build, no rebase, and no merge boundary to confound. + +| | What it is | What it must show | +|---|---|---| +| **Arm A** | The PR exactly as written | **Silent.** Zero diagnostics | +| **Arm B** | Same tree + probes that use the *derived* type, exercised as the real code exercises it | Each new diagnostic = a disagreement the hand-written type concealed | + +**Arm A must be silent or the run is inconclusive.** If the untouched tree +already emits diagnostics, nothing in Arm B is attributable to the substitution — +"N errors in Arm B" is then a count, not a finding. Publish Arm A's result +verbatim as the delivery check. + +## Instructions + +### Step 1: Inventory every type the PR hand-wrote + +```bash +gh pr diff | grep -nE '^\+.*(type [A-Z]|interface [A-Z]|: (Record<|string|number|boolean|unknown|any)\b)' +``` + +List them. Each one is a claim you are about to test. + +**First, check each converted module is still referenced:** + +```bash +grep -rn "moduleName" --include=*.ts --include=*.js . | grep -v node_modules | grep -v '\.test\.' +``` + +If the only hits are the module's own definition and its test, the file is dead — +every type on it is unfalsifiable, because nothing constrains it and no divergence +can ever surface. This is the highest value-per-second check in a migration, and +it reorders the work: a dead module's conversion is a deletion candidate, not a +typing exercise. + +### Step 2: Find the authoritative source for each + +Work down this list — the first hit wins. In the worked example 9 of the 12 +hand-written types had a source, each found in under a minute: + +1. **The call site.** What is actually passed? In a JS caller, check for a JSDoc + `@type {import('…').Foo}` annotation on the variable — the answer is sometimes + literally already written down there. +2. **The class or method being wrapped** → `MyController['someMethod']`. +3. **A package already imported in the same file** — e.g. `webextension-polyfill` + defines every listener payload; if the file calls the API, the type is in reach. +4. **The sender**, for a message or event payload. If the sender is TypeScript, the + shape is derivable, not guessable. +5. **A selector's return** → `ReturnType`. +6. **`@types/*` for a platform API.** Read the actual declaration before "fixing" + a type error — many are template-literal *string* types (`` `${SomeEnum}`[] ``), + which already accept a plain string literal. +7. **No source exists** — an untyped dependency, a lib absent from `tsconfig.lib`, + a genuinely new boundary the repo owns. Hand-writing is then **correct**. Record + it as a cleared falsifier with the reason; do not report it as a finding. + +### Step 3: Write one probe per claim + +One file per claim, in a scratch dir inside the `include` paths. Each probe names +its authoritative source in a header comment and calls the derived type **the way +the real call site calls it**: + +```ts +// PROBE — src/thing.ts hand-wrote `setFoo: (id: number | undefined) => void`. +// Authoritative: FooController['setFoo'] (foo-controller.ts:120) — param is `number`. +// The real code calls it as below. +import type { FooController } from '../controllers/foo-controller'; + +declare const setFoo: FooController['setFoo']; + +export function asCalledByTheRealCode() { + setFoo(undefined); // thing.ts:105 +} +``` + +### Step 4: Run both arms + +```bash +./scripts/substitution-ab.sh +``` + +Or by hand — Arm A first, and stop if it is not silent. + +### Step 5: Isolate diagnostics that fire for the wrong reason + +A checker reports the *first* failure it reaches, so an unrelated earlier cause +can short-circuit the claim under test — and an exit-code read scores that as a +confirmation. **Assert on the specific diagnostic** (code + message + line), and +where an earlier cause intervenes, neutralise it and re-probe: + +```ts +const defined = value as NonNullable; // set nullability aside +// …now the return-type claim is the only thing left to fail +``` + +This is not hypothetical — see the worked example, where a claim that a return +type was unsound turned out **sound** once the nullability error ahead of it was +isolated. + +### Step 6: Report findings *and* clearances + +Give each claim a verdict, and say which ones the probes **cleared**. A +substitution sweep that only ever confirms is indistinguishable from one that +never isolated anything. + +### Severity: "pre-existing" is about the upstream `any`, not about the annotation + +A conversion PR invites two reflexes that both understate a false-precision +finding, and the underlying `any` is what makes each of them sound reasonable: + +- **"It's pre-existing."** Split the claim. The library's `any` is genuinely + inherited — ethers has returned `any` from dynamic contract methods since long + before this diff. The *annotation beside it* was written here: check `git log + --diff-filter=A -- ` and, for a `declare module`, whether the block itself + is added by this PR. If the file is new, nothing in it is pre-existing, because + with `checkJs` off the predecessor asserted nothing at all. A conversion is the + moment the boundary's type is **chosen**. +- **"It's a nit."** A nit is a finding that is minor *in itself*. A value crossing + a boundary unchecked, under a signature that says it was checked, is a type-safety + defect — the class this whole skill exists to surface. Scope and severity are + separate axes: a substantive finding a PR need not fix is still substantive, and + saying so costs one sentence. + +The remedy follows from which one it is. Annotating the hole and filing a TODO is +right for the inherited `any`; it is not a fix for false precision, because the +misleading signature stays exactly as it was. Type or validate the value where it +enters, so the precise annotations downstream are earned. + +## The five divergence shapes + +Five shapes to check for. Treat the list as a starting checklist, not a partition — +the first four each account for at least one divergence in the worked example, and +the fifth was found on a later pass over the same PR: + +1. **Widening** — `string` for a `Hex`/template-literal type, `string` for an enum, + `number | undefined` for `number`. Admits values the real type rejects; worst + when a guard downstream depends on the narrower form. A union ending in + `string & Record` reads as a narrowing and is not one: its last + member accepts every string, typos included. +2. **Dropped nullability** — the source says `| undefined`, the hand-written type + doesn't. Erases the compiler's record of why a runtime guard exists. +3. **Duplication** — the same shape written out in two files, unshared. Both copies + now need every future change. +4. **Placeholder** — `Record`, `any`, or `unknown` standing in for + a shape that is known. Pushes a cast to every use site. A circular import does + not justify one: `import type` the real type, which is erased from the emitted + JavaScript and adds no runtime edge to the module graph. +5. **False precision** — the inverse of a placeholder: the annotation is *narrower* + than what actually arrives. `hexValueIsEmpty(value: string | null | undefined)` + on a parameter fed `any` at every call site. `tsc` cannot report it, because + `any` satisfies every annotation, and `no-explicit-any` cannot either, because + no `any` was written. The narrower the type, the more confident the file reads + and the less any of it is checked. Find it with `IsAny` at the call sites, not + by reading the signature — see + [references/false-negatives.md](references/false-negatives.md) §7. + +## Escape hatches are the tell + +When a diff adds a hand-written type *and* an `as`, a `!`, a new `?.`, or an +`eslint-disable` in the same region, check whether the escape hatch exists to service +the type rather than the runtime. Count them — a cluster marks where to probe first. +Ask of each suppression what would fire on that exact line without it. `tsc` reports +an unused `@ts-expect-error` (TS2578) but not an unused `@ts-ignore`, and whether an +unused `eslint-disable` is reported depends on the repo's `reportUnusedDisableDirectives` +setting (`'error'` in extension and core, unset in mobile). + +## A typing change should not change runtime behavior + +The second axis, and the one whose defects reach runtime rather than staying in +the type layer. A migration PR is +allowed to add annotations; it is not allowed to change what the program *does*. +Four patterns to grep the diff for, all of which look like typing work: + +1. **A literal replaced by a runtime lookup.** `['IFRAME_SCRIPTING']` becoming + `[SomeApi.Reason.IFRAME_SCRIPTING]` adds a dependency on that object existing at + runtime. **Read the declaration first** — if the parameter is a template-literal + string type (`` `${SomeEnum}`[] ``), the literal already type-checked and the + swap bought nothing. +2. **A default parameter or fallback deleted.** `function f(x = {})` → `function f(x: T)` + removes a guard. Ask what the guard was *for*: a `| undefined` the new type just + dropped is the first candidate. Then check reachability rather than assuming either way. +3. **A call made optional.** `obj.method()` → `obj.method?.()`, added to satisfy a + hand-written `| undefined`, converts a **throw into a silent no-op**. The loud + failure was load-bearing; now the same state produces no signal at all. +4. **A widened local to keep a check alive.** `let name: string | undefined` on a + value the authoritative type calls `string`, so that an `=== undefined` branch + still compiles. If the runtime check is genuinely needed, the *input* type is + wrong — fix that instead of widening downstream. + +For each hit: state whether it is reachable, and say so plainly either way. "I +traced it and it is inert today" is a useful review finding. "This might be a bug" +is not. + +### Silent failure modes deserve their own pass + +Ask where a newly-introduced failure would *surface*. A change inside a +`try { … } catch { captureException(e); return; }` degrades a feature without +crashing — nothing goes red, no test fails, and the only signal is an error-tracker +entry nobody is watching. The same edit in a hot path would be caught in minutes. +Weight findings by observability, not just by likelihood: **an unlikely failure in a +swallowed path can outrank a likely one in a loud path.** + +## Why the build stays green regardless + +- The hand-written type **compiles by construction** — that is why it was written. +- With `checkJs` off, a type written for a function whose callers are still `.js` + is checked against **nothing** and can drift indefinitely. +- A value that arrives as `any` silently satisfies any annotation. + +So cite the green build as the *premise* of the finding, never as counter-evidence. + +## Examples + +**Worked example** — 12 hand-written types across a JS→TS migration PR, 5 confirmed +divergences and 5 cleared falsifiers, with the verbatim two-arm output: +[references/worked-example.md](references/worked-example.md). + +**Repo notes** for MetaMask Extension (heap, probe location, `checkJs` status): +[references/metamask-extension.md](references/metamask-extension.md). + +``` +User: "Validate this TS migration PR — is it really mechanical?" +Agent: inventories the 12 new types → finds the authoritative source for 9 → + writes 6 probes → Arm A silent, Arm B reports 6 diagnostics → isolates + one that fired for the wrong reason → reports 5 findings, 5 clearances. +``` + +## Troubleshooting + +### Arm A is not silent + +**Problem:** the untouched tree already emits diagnostics, so Arm B is unattributable. +**Fix:** pin the toolchain, install against the PR's own lockfile, raise the heap, or +narrow the project. If it cannot be made silent, the lane is **inconclusive** — say +so; do not report Arm B's count as findings. + +### `tsc` runs out of memory + +**Problem:** `FATAL ERROR: Ineffective mark-compacts near heap limit`. +**Fix:** raise the heap — `NODE_OPTIONS='--max-old-space-size=9216'`. Note the OOM +exits non-zero *without* type diagnostics, so a naive exit-code check reads it as +"errors found." Always look at the output, not just the status. + +### A probe errors, but not for the claimed reason + +**Problem:** the diagnostic is about an earlier property, not the claim. +**Fix:** neutralise the earlier cause (`NonNullable<…>`, a narrow assertion) and +re-run. If the claim then compiles clean, the claim was **wrong** — report it as cleared. + +### There is no authoritative source + +Not a failure. Hand-writing is correct where nothing defines the shape; record the +reason (package ships no types, lib not in `tsconfig.lib`, new boundary) so the next +reviewer doesn't re-litigate it. + +## Related + +- [contributor-docs `docs/typescript.md`](https://github.com/MetaMask/contributor-docs/blob/main/docs/typescript.md) — the write-side rule this proves. +- `unit-testing`, `integration-test` — for behavior claims; this skill proves *types*.