From a607798013c50fff936935fa43e078e0b4c7ee40 Mon Sep 17 00:00:00 2001 From: Vivswan Shah <58091053+Vivswan@users.noreply.github.com> Date: Tue, 22 Sep 2026 00:01:45 -0400 Subject: [PATCH] chore(lint): move the never-throw rule to a Biome GritQL plugin lint/never-throw.grit flags every throw under src/ that is neither a BUG: invariant nor a bare rethrow of the catch binding, and biome.json wires it with the one exempt file, src/cli/inputs.ts, where commander's argParser has no value form. The TypeScript walk in arch-lint.ts that carried the rule, its census, its empty ratchet, and the throws section of architecture.yml go; a declaration still holding that section fails as an unknown key. A fixture test lints a tree with the repository's own biome.json, so every throw class, both exemptions, and the exempt file still holding a throw outside the rule are pinned there. The compat-marker gate stays in TypeScript: Biome's GritQL cannot match comments, since they are trivia to it (biomejs/biome#10474). The generated gaps index is linted now, so an override turns the organizeImports assist off for it and it keeps its generator's import order. The shadow check also counts a namespace or an import-equals declaration named after the catch binding, and skips a name inside a type alias, an interface, or a type annotation, since nothing there holds a value. --- .github/scripts/arch-lint.ts | 227 +-------------- AGENTS.md | 2 +- architecture.yml | 11 - biome.json | 22 +- docs/reference/architecture.md | 6 +- lint/never-throw.grit | 55 ++++ test/architecture/architecture.test.ts | 222 ++------------- test/lint/never-throw.test.ts | 380 +++++++++++++++++++++++++ 8 files changed, 484 insertions(+), 441 deletions(-) create mode 100644 lint/never-throw.grit create mode 100644 test/lint/never-throw.test.ts diff --git a/.github/scripts/arch-lint.ts b/.github/scripts/arch-lint.ts index 9c11bb39..117c2d65 100644 --- a/.github/scripts/arch-lint.ts +++ b/.github/scripts/arch-lint.ts @@ -6,12 +6,10 @@ * type-only imports and re-exports -> edges too * `import("./x.js").T`, `import X = require("./x.js")` in type positions -> edges too * a computed `import(x)` or `require(x)` -> fails: the graph cannot follow it - * - * The same walk carries the never-throw rule the `throws` block of architecture.yml states beside its lists. */ -import { existsSync, readdirSync, readFileSync, statSync } from "node:fs"; -import { dirname, join, normalize, relative, resolve } from "node:path"; +import { existsSync, readdirSync, readFileSync } from "node:fs"; +import { dirname, join, relative, resolve } from "node:path"; import { inspect } from "node:util"; import { err, ok, Result } from "neverthrow"; import { type Node, parseSync } from "oxc-parser"; @@ -21,35 +19,22 @@ import { countNoun } from "../../src/text.js"; export const ARCHITECTURE_PATH = "architecture.yml"; -export interface Throws { - /** Files whose throw is a third party's contract (commander's argParser), each with its reason in the yaml. */ - readonly contracts: readonly string[]; - /** file -> its exact count of throws outside the rule. */ - readonly ratchet: Readonly>; -} - export interface Architecture { /** layer -> the src/ paths it owns (a `/` suffix means a directory). */ readonly layers: Readonly>; readonly exclude: readonly string[]; /** from -> the layers it may import. */ readonly edges: Readonly>; - readonly throws: Throws; } const PATHS = z.array(z.string()); -const COUNT = { error: "expected a whole number of throws" }; const ARCHITECTURE = z.strictObject({ layers: z.record(z.string(), PATHS), exclude: PATHS, edges: z.record(z.string(), PATHS), - throws: z.strictObject({ - contracts: PATHS, - ratchet: z.record(z.string(), z.int(COUNT).nonnegative(COUNT)), - }), }) satisfies z.ZodType; -/** `throws.ratchet["src/x.ts"]`, `throws.contracts[0]`: the yaml key as a reader would write it in code. */ +/** `layers.engine[0]`, `edges["plain-data"]`: the yaml key as a reader would write it in code. */ function keyPath(path: readonly PropertyKey[]): string { return path .map((segment, index) => @@ -82,8 +67,6 @@ function describeIssue(doc: unknown, issue: z.core.$ZodIssue): string[] { ]; } -/** The declaration, or every way the file fails to be one: yaml, shape, count type, and a `throws` path naming no - * file under src/. A count the lint cannot compare would otherwise silence the ratchet for that file. */ export function parseArchitecture(root: string): Result { const document = parseDocument(readFileSync(join(root, ARCHITECTURE_PATH), "utf8")); if (document.errors.length > 0) { @@ -100,35 +83,15 @@ export function parseArchitecture(root: string): Result return err(doc.error); } const parsed = ARCHITECTURE.safeParse(doc.value); - if (!parsed.success) { - return err(located(parsed.error.issues.flatMap((issue) => describeIssue(doc.value, issue)))); - } - const { contracts, ratchet } = parsed.data.throws; - const unknownFiles = [ - ...contracts.map((file, index) => [["contracts", index], file] as const), - ...Object.keys(ratchet).map((file) => [["ratchet", file], file] as const), - ] - .filter(([, file]) => !(normalize(file).startsWith("src/") && isFile(join(root, file)))) - .map( - ([path, file]) => - `${keyPath(["throws", ...path])} names no file under src/: ${inspect(file)}`, - ); - return unknownFiles.length > 0 ? err(located(unknownFiles)) : ok(parsed.data); + return parsed.success + ? ok(parsed.data) + : err(located(parsed.error.issues.flatMap((issue) => describeIssue(doc.value, issue)))); } function located(problems: readonly string[]): string[] { return problems.map((problem) => `${ARCHITECTURE_PATH}: ${problem}`); } -/** A path through a file (`src/x.ts/y.ts`) makes stat fail with ENOTDIR; that is as much "no file" as a missing one. */ -function isFile(path: string): boolean { - try { - return statSync(path).isFile(); - } catch { - return false; - } -} - /** For callers that render rather than lint (docs, tests): a malformed declaration is fatal to them. */ export function readArchitecture(root: string): Architecture { return parseArchitecture(root).match( @@ -145,31 +108,21 @@ function layerOf(arch: Architecture, path: string): string | undefined { )?.[0]; } -/** Every node under `value`, depth first, each with the state `carry` hands down from its nearest typed ancestor. */ -function* nodesOf( - value: unknown, - state: S, - carry: (node: Node, state: S) => S, -): Generator<{ node: Node; state: S }> { +function* nodesOf(value: unknown): Generator { if (Array.isArray(value)) { for (const item of value) { - yield* nodesOf(item, state, carry); + yield* nodesOf(item); } } else if (typeof value === "object" && value !== null) { - let inner = state; if ("type" in value && typeof value.type === "string") { - const node = value as Node; - yield { node, state }; - inner = carry(node, state); + yield value as Node; } for (const child of Object.values(value)) { - yield* nodesOf(child, inner, carry); + yield* nodesOf(child); } } } -const stateless = (): undefined => undefined; - /** The expression under the wrappers parentheses and TypeScript add: `(require)(x)`, `require!(x)`, * `(require as any)(x)`, `require(x)`. */ function unwrapped(expression: Node): Node { @@ -240,7 +193,7 @@ export function importSpecifiers(text: string, file: string): string[] { ); } const specifiers = new Set(); - for (const { node } of nodesOf(program, undefined, stateless)) { + for (const node of nodesOf(program)) { if (!isModuleLoad(node)) { continue; } @@ -326,156 +279,6 @@ export function lintArchitecture(root: string, arch = readArchitecture(root)): s return problems; } -type ThrowStatement = Extract; -/** The names a binding pattern declares; a destructuring key is not one of them. */ -function bindingNames(pattern: Node): string[] { - switch (pattern.type) { - case "Identifier": - return [pattern.name]; - case "ObjectPattern": - return pattern.properties.flatMap((property) => - bindingNames(property.type === "RestElement" ? property.argument : property.value), - ); - case "ArrayPattern": - return pattern.elements.flatMap((element) => (element ? bindingNames(element) : [])); - case "AssignmentPattern": - return bindingNames(pattern.left); - case "RestElement": - return bindingNames(pattern.argument); - default: - return []; - } -} - -/** Whether a block redeclares `name` with a value binding of its own: a variable, or anything declared under an - * `id` (a function, class, enum, or namespace). A type alias or interface lives in the type namespace, so - * `throw name` after one still throws the catch binding. */ -function redeclares(block: Extract, name: string): boolean { - return block.body.some((statement) => - statement.type === "VariableDeclaration" - ? statement.declarations.some((declaration) => bindingNames(declaration.id).includes(name)) - : statement.type !== "TSTypeAliasDeclaration" && - statement.type !== "TSInterfaceDeclaration" && - "id" in statement && - statement.id !== null && - typeof statement.id === "object" && - statement.id.type === "Identifier" && - statement.id.name === name, - ); -} - -/** The catch binding a throw may rethrow: the clause's own identifier, carried only through blocks and if-statements - * that do not redeclare it. A loop, a switch, a nested function, or anything else on the way drops it, so a throw - * there is judged on its own. */ -function carryRethrowable(node: Node, rethrowable: string | undefined): string | undefined { - if (node.type === "CatchClause") { - return node.param?.type === "Identifier" ? node.param.name : undefined; - } - return node.type === "IfStatement" || - (node.type === "BlockStatement" && rethrowable !== undefined && !redeclares(node, rethrowable)) - ? rethrowable - : undefined; -} - -/** `new X("BUG: ...")` or `new X(\`BUG: ${...}\`)`, X any Error class: a programming error no user can cause. */ -function isBugInvariant(argument: ThrowStatement["argument"]): boolean { - if (argument.type !== "NewExpression") { - return false; - } - const [first] = argument.arguments; - const head = - first?.type === "Literal" - ? first.value - : first?.type === "TemplateLiteral" - ? first.quasis[0]?.value.cooked - : undefined; - return typeof head === "string" && head.startsWith("BUG:"); -} - -export interface ThrowCensus { - bug: number; - rethrow: number; - contract: number; - /** Throws outside the rule, whether or not the ratchet lists them. */ - outside: number; -} - -const OUTSIDE_RULE = - "not a BUG: invariant, not a bare rethrow inside its catch clause, and the file is not in throws.contracts"; - -export function lintThrows( - root: string, - arch = readArchitecture(root), -): { problems: string[]; census: ThrowCensus } { - const census: ThrowCensus = { bug: 0, rethrow: 0, contract: 0, outside: 0 }; - const spared = new Set(arch.throws.contracts); - const sparedInUse = new Set(); - const outside = new Map(); - const problems: string[] = []; - for (const file of sourceFiles(root, arch)) { - const text = readFileSync(join(root, file), "utf8"); - const { program, errors } = parseSync(join(root, file), text); - if (errors.length > 0) { - problems.push(`${file} does not parse, so its throws are uncounted: ${errors[0]?.message}`); - continue; - } - for (const { node, state: rethrowable } of nodesOf(program, undefined, carryRethrowable)) { - if (node.type !== "ThrowStatement") { - continue; - } - if (isBugInvariant(node.argument)) { - census.bug += 1; - } else if (node.argument.type === "Identifier" && node.argument.name === rethrowable) { - census.rethrow += 1; - } else if (spared.has(file)) { - census.contract += 1; - sparedInUse.add(file); - } else { - census.outside += 1; - const line = text.slice(0, node.start).split("\n").length; - outside.set(file, [...(outside.get(file) ?? []), line]); - } - } - } - for (const [file, lines] of [...outside].sort()) { - const listed = arch.throws.ratchet[file]; - const sites = lines.map((line) => `${file}:${line}`).join(", "); - if (listed === undefined) { - problems.push( - ...lines.map( - (line) => - `${file}:${line} throws outside the rule: ${OUTSIDE_RULE}; return a Result, or add the file to throws.ratchet`, - ), - ); - } else if (lines.length > listed) { - problems.push( - `${file} throws ${lines.length} times outside the rule, throws.ratchet allows ${listed}: ${sites}; return a Result instead`, - ); - } else if (lines.length < listed) { - problems.push( - `${file} throws ${lines.length} times outside the rule, throws.ratchet lists ${listed}; lower it to ${lines.length}`, - ); - } - } - for (const file of Object.keys(arch.throws.ratchet).sort()) { - if (!outside.has(file)) { - problems.push( - `stale ratchet ${file}: no throw outside the rule remains; remove it from throws.ratchet`, - ); - } - } - for (const file of [...spared].sort()) { - if (!sparedInUse.has(file)) { - problems.push(`stale allowance throws.contracts ${file}: no throw remains there; remove it`); - } - } - return { problems, census }; -} - -export function describeThrowCensus({ bug, rethrow, contract, outside }: ThrowCensus): string { - return `throws: ${bug} BUG: invariants, ${rethrow} rethrows, ${contract} under a third-party contract, ${outside} outside the rule`; -} - /** A hyphen in a layer name is edge syntax to mermaid, so ids swap it for an underscore. */ export function renderArchitectureMermaid(arch: Architecture): string { const id = (layer: string): string => layer.replace(/-/g, "_"); @@ -491,11 +294,7 @@ export function renderArchitectureMermaid(arch: Architecture): string { if (import.meta.main) { const root = join(import.meta.dir, "..", ".."); const problems = parseArchitecture(root).match( - (arch) => { - const throws = lintThrows(root, arch); - console.log(`lint:arch: ${describeThrowCensus(throws.census)}`); - return [...lintArchitecture(root, arch), ...throws.problems]; - }, + (arch) => lintArchitecture(root, arch), (problems) => problems, ); if (problems.length > 0) { @@ -504,5 +303,5 @@ if (import.meta.main) { ); process.exit(1); } - console.log(`lint:arch: src/ imports and throws match ${ARCHITECTURE_PATH}`); + console.log(`lint:arch: src/ imports match ${ARCHITECTURE_PATH}`); } diff --git a/AGENTS.md b/AGENTS.md index b657eec9..68b6d0a0 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -43,7 +43,7 @@ Code is the source of truth: this section holds only the rules and the decisions - The import layering of `src/` is declared in `architecture.yml`; a new cross-layer import is a deliberate edit to that file. - A type a section module exposes is exported from its home module, or the bundled declarations cannot reach it and the package-smoke job fails. - New sections and endpoints ship with e2e scenarios, and `bun run test:e2e` runs green before they land. -- Errors are values: `throw` only for `BUG:` invariants (programming errors no user can cause), a bare rethrow in its `catch`, and where a third party's contract demands it (`architecture.yml` names the file and the reason). The arch lint enforces it; its ratchet only shrinks. +- Errors are values: `throw` only for `BUG:` invariants (programming errors no user can cause), a bare rethrow in its `catch`, and where a third party's contract demands it. The Biome plugin `lint/never-throw.grit` enforces it. - No backward-compatibility shims: a change that breaks an input, key, format, or behavior ships the break behind a major with a loud error naming the fix; a one-shot migration only when many files must move at once. A shim that stays anyway carries a `COMPAT(vN)` marker naming the major that deletes it. ### Decisions a reader would otherwise reverse diff --git a/architecture.yml b/architecture.yml index bbe6d53b..b2e382e7 100644 --- a/architecture.yml +++ b/architecture.yml @@ -46,14 +46,3 @@ edges: problem: [plain-data, schema, text] schema: [sections, types] upstream-gaps: [types] - -# The never-throw rule: errors are values, so a `throw` in src/ is a `BUG:` invariant (a programming error no user -# can cause), a bare rethrow sitting directly inside its catch clause, or spared here. `contracts` lists the files -# whose throw IS a third party's contract, each with its reason. `ratchet` lists every other throw by file with its -# exact count: the lint fails when this list and the tree disagree in either direction, and refuses a count it cannot -# compare. That a count only ever goes down is the review rule in AGENTS.md, not the lint's. -throws: - contracts: - # commander's argParser reports a refused value by throwing InvalidArgumentError; it has no value form. - - src/cli/inputs.ts - ratchet: {} diff --git a/biome.json b/biome.json index ce1f3574..0ca9d7d7 100644 --- a/biome.json +++ b/biome.json @@ -6,15 +6,11 @@ "useIgnoreFile": true }, "files": { - "includes": [ - "**", - "!lib", - "!src/upstream-gaps/index.ts", - "!.github/repo-platform-manifest.json" - ] + "includes": ["**", "!lib", "!.github/repo-platform-manifest.json"] }, "formatter": { "enabled": true, + "includes": ["**", "!src/upstream-gaps/index.ts"], "indentStyle": "space", "indentWidth": 2, "lineWidth": 100 @@ -52,6 +48,20 @@ } }, "overrides": [ + { + "includes": ["src/upstream-gaps/index.ts"], + "assist": { + "actions": { + "source": { + "organizeImports": "off" + } + } + } + }, + { + "includes": ["src/**", "!src/cli/inputs.ts"], + "plugins": ["./lint/never-throw.grit"] + }, { "includes": ["src/action/**"], "linter": { diff --git a/docs/reference/architecture.md b/docs/reference/architecture.md index dd69f014..8fd1c52b 100644 --- a/docs/reference/architecture.md +++ b/docs/reference/architecture.md @@ -10,11 +10,9 @@ The check is existence only: a caption-only box (`mode`, `rendered-file`) names The [module map](#the-module-map) at the end is generated from [architecture.yml](https://github.com/Vivswan/github-settings-as-code/blob/main/architecture.yml). The `lint:arch` script keeps that declaration equal to the import graph, so the map cannot show an edge the code does not draw. -The same lint enforces the never-throw rule: a function that can fail returns a neverthrow `Result` carrying a typed `Problem`, so an error is a value the caller handles. A section's `plan()`, `snapshot()`, and operation hooks carry a `SectionFailure` instead, which the engine loops match on by kind. +A Biome linter plugin, `lint/never-throw.grit`, enforces the never-throw rule: a function that can fail returns a neverthrow `Result` carrying a typed `Problem`, so an error is a value the caller handles. A section's `plan()`, `snapshot()`, and operation hooks carry a `SectionFailure` instead, which the engine loops match on by kind. -A `throw` is allowed as a `BUG:` invariant, as a bare rethrow inside its own `catch`, or where a third party's contract demands it, in a file the `throws` block of `architecture.yml` names with its reason. - -That block counts the remaining throws per file. The lint fails when the count and the tree disagree in either direction, so a converted throw lowers its file's count and the entry leaves the list once no throw remains; that a count only goes down is the review rule in AGENTS.md. +A `throw` is allowed as a `BUG:` invariant, as a bare rethrow inside its own `catch`, or where a third party's contract demands it, in a file the plugin's entry in `biome.json` exempts and the plugin's header names with its reason. ## The journey of one settings file diff --git a/lint/never-throw.grit b/lint/never-throw.grit new file mode 100644 index 00000000..d70eacd3 --- /dev/null +++ b/lint/never-throw.grit @@ -0,0 +1,55 @@ +// The never-throw rule of AGENTS.md as a Biome linter plugin: a `throw` under src/ is a BUG: invariant (a +// programming error no user can cause) or a bare rethrow of the catch binding; anything else is a failure the +// caller should receive as a Result. +// +// Reach and exemptions are the plugin's `overrides` entry in biome.json, with `includes` relative to the repository +// root; a top-level `plugins[].includes` matches the absolute path (biomejs/biome#11082), so the checkout's parent +// directories would decide the reach. The one exemption: +// src/cli/inputs.ts -> commander's argParser reports a refused value by throwing InvalidArgumentError; it has no value form +// +// GritQL has no scopes, so a second binding of the caught name anywhere in the catch body (a declaration, a +// destructuring, a nested function's parameter) flags every rethrow in that body, not only the shadowed one; the +// fix is to rename. A namespace or an `import x = require()` counts as a binding too. A name inside a type alias, an +// interface, or a type annotation does not, since nothing there holds a value; a function-type parameter anywhere else +// (a cast, a return type, a declare signature) still counts, and the fix is the same rename. The regex reads the literal's source text, so BUG: must open the string or template itself. + +`throw $arg` as $throw where { + not or { + and { + $arg <: JsNewExpression(arguments=JsCallArguments(args=[$first, ...])), + or { $first <: JsStringLiteralExpression(), $first <: JsTemplateExpression() }, + $first <: r"^[\"'`]BUG:[\s\S]*" + }, + and { + $arg <: JsIdentifierExpression(), + // A throw from a function nested in the catch body may run after the clause has ended, and one from a try + // nested there is caught or rethrown by that try, so the walk up from the throw to its clause stops at both. + $throw <: within JsCatchClause(declaration=JsCatchDeclaration(binding=$arg), body=$body) until or { + JsTryStatement(), + JsTryFinallyStatement(), + JsArrowFunctionExpression(), + JsFunctionExpression(), + JsFunctionDeclaration(), + JsMethodObjectMember(), + JsMethodClassMember(), + JsGetterObjectMember(), + JsSetterObjectMember(), + JsGetterClassMember(), + JsSetterClassMember(), + JsConstructorClassMember() + }, + not $body <: contains or { + JsIdentifierBinding() as $binding where { + not $binding <: within or { + TsTypeAnnotation(), + TsTypeAliasDeclaration(), + TsInterfaceDeclaration() + } + }, + TsModuleDeclaration(name=$binding), + TsImportEqualsDeclaration(id=$binding) + } where { $binding <: $arg } + } + }, + register_diagnostic(span=$throw, message="throw outside the never-throw rule: not a BUG: invariant (an Error whose message starts with BUG:) and not a bare rethrow of the catch binding; return a Result instead") +} diff --git a/test/architecture/architecture.test.ts b/test/architecture/architecture.test.ts index 4648d17b..a1a2e9f9 100644 --- a/test/architecture/architecture.test.ts +++ b/test/architecture/architecture.test.ts @@ -1,18 +1,16 @@ /** - * The verdict `bun run lint:arch` prints, so CI's test job carries the gate; every import form the scanner must read has a control, since a missed - * form would let a forbidden import pass, and every throw class the never-throw rule names has one, since a missed class would let a throw pass. + * The verdict `bun run lint:arch` prints, so CI's test job carries the gate; every import form the scanner must read + * has a control, since a missed form would let a forbidden import pass. */ import { describe, expect, test } from "bun:test"; -import { mkdirSync, writeFileSync } from "node:fs"; -import { dirname, join } from "node:path"; +import { writeFileSync } from "node:fs"; +import { join } from "node:path"; import { err } from "neverthrow"; import { ARCHITECTURE_PATH, - type Architecture, importSpecifiers, lintArchitecture, - lintThrows, parseArchitecture, readArchitecture, renderArchitectureMermaid, @@ -27,10 +25,6 @@ describe("architecture.yml against src/", () => { expect(lintArchitecture(ROOT)).toEqual([]); }); - test("lists exactly the throws outside the never-throw rule the tree holds", () => { - expect(lintThrows(ROOT, arch).problems).toEqual([]); - }); - test("a forbidden edge fails naming both files (negative control)", () => { const problems = lintArchitecture(ROOT, { ...arch, edges: { ...arch.edges, main: [] } }); expect(problems).toEqual([ @@ -129,220 +123,38 @@ describe("importSpecifiers", () => { }); }); -describe("lintThrows", () => { - /** A src/ tree of `files` under a root, linted against `throws` alone. */ - function lint(dir: string, files: Record, throws: Architecture["throws"]) { - for (const [path, text] of Object.entries(files)) { - mkdirSync(dirname(join(dir, path)), { recursive: true }); - writeFileSync(join(dir, path), text); - } - return lintThrows(dir, { layers: {}, edges: {}, exclude: ["src/**/*.test.ts"], throws }); - } - - const OUTSIDE = - "throws outside the rule: not a BUG: invariant, not a bare rethrow inside its catch clause, and the file is not in throws.contracts; return a Result, or add the file to throws.ratchet"; - - test("each class lands in the census; a throw outside the rule, a rethrow from another scope, and a file that does not parse are reported, and a type alias does not shadow the binding", () => - withTempDir("arch-lint-throws-", (dir) => { - const files = { - "src/bug.ts": [ - "export function bug(x: unknown): never {", - ' if (x === null) throw new Error("BUG: bug() was handed null");', - ` throw new RangeError(\`BUG: bug() was handed \${String(x)}\`);`, - "}", - ].join("\n"), - "src/rethrow.ts": [ - "export function rethrow(run: () => void, keep: boolean): void {", - " try {", - " run();", - " } catch (error) {", - " const { error: copy } = { error: 1 };", - " if (keep && copy === 1) {", - " throw error;", - " }", - " [1].forEach(() => {", - " throw error;", - " });", - " }", - ' const error = new Error("x");', - " throw error;", - "}", - ].join("\n"), - "src/shadow.ts": [ - "export function shadow(run: () => void, mode: number): void {", - " try {", - " run();", - " } catch (error) {", - " {", - ' const error = new Error("x");', - " throw error;", - " }", - " {", - " enum error {", - " Other = 1,", - " }", - " throw error;", - " }", - ' for (const error of [new Error("y")]) {', - " throw error;", - " }", - " switch (mode) {", - " default:", - " throw error;", - " }", - " {", - " type error = number;", - " throw error;", - " }", - " }", - "}", - ].join("\n"), - "src/spared.ts": 'export const spared = (): never => {\n throw new Error("x");\n};', - "src/plain.ts": 'export function plain(): never {\n throw "x";\n}', - "src/broken.ts": 'export function broken(): never {\n throw "x";', - "src/plain.test.ts": 'throw new Error("x");', - }; - expect(lint(dir, files, { contracts: ["src/spared.ts"], ratchet: {} })).toEqual({ - problems: [ - expect.stringMatching(/^src\/broken\.ts does not parse, so its throws are uncounted: /), - `src/plain.ts:2 ${OUTSIDE}`, - `src/rethrow.ts:10 ${OUTSIDE}`, - `src/rethrow.ts:14 ${OUTSIDE}`, - `src/shadow.ts:7 ${OUTSIDE}`, - `src/shadow.ts:13 ${OUTSIDE}`, - `src/shadow.ts:16 ${OUTSIDE}`, - `src/shadow.ts:20 ${OUTSIDE}`, - ], - census: { bug: 2, rethrow: 2, contract: 1, outside: 7 }, - }); - })); - - test.each<[string, number, string[]]>([ - ["equal to", 2, []], - [ - "under", - 1, - [ - "src/two.ts throws 2 times outside the rule, throws.ratchet allows 1: src/two.ts:2, src/two.ts:3; return a Result instead", - ], - ], - [ - "over", - 3, - ["src/two.ts throws 2 times outside the rule, throws.ratchet lists 3; lower it to 2"], - ], - ])( - "a ratchet count %s the file's count moves only by editing the list", - (_case, listed, problems) => - withTempDir("arch-lint-ratchet-", (dir) => { - const files = { - "src/two.ts": - 'export function two(a: boolean): never {\n if (a) throw new Error("x");\n throw new Error("y");\n}', - }; - expect(lint(dir, files, { contracts: [], ratchet: { "src/two.ts": listed } })).toEqual({ - problems, - census: { bug: 0, rethrow: 0, contract: 0, outside: 2 }, - }); - }), - ); - - test("a ratchet entry or a spared file with nothing left to spare is stale", () => - withTempDir("arch-lint-stale-", (dir) => { - const files = { - "src/clean.ts": - 'export function clean(): never {\n throw new Error("BUG: clean() ran");\n}', - }; - const throws = { - contracts: ["src/clean.ts", "src/gone.ts"], - ratchet: { "src/clean.ts": 1 }, - }; - expect(lint(dir, files, throws)).toEqual({ - problems: [ - "stale ratchet src/clean.ts: no throw outside the rule remains; remove it from throws.ratchet", - "stale allowance throws.contracts src/clean.ts: no throw remains there; remove it", - "stale allowance throws.contracts src/gone.ts: no throw remains there; remove it", - ], - census: { bug: 1, rethrow: 0, contract: 0, outside: 0 }, - }); - })); -}); - describe("parseArchitecture", () => { - /** A declaration with `throws` replaced, beside one src/ file and one test/ file (so a path that leaves src/ - * through `..` still names a real file), parsed from a temp root. */ - function parse(dir: string, throws: string) { - for (const file of ["src/x.ts", "test/x.ts"]) { - mkdirSync(dirname(join(dir, file)), { recursive: true }); - writeFileSync(join(dir, file), "export const x = 1;\n"); - } - writeFileSync( - join(dir, ARCHITECTURE_PATH), - `layers: {}\nexclude: []\nedges: {}\n${throws}`.replaceAll("|", "\n"), - ); + function parse(dir: string, document: string) { + writeFileSync(join(dir, ARCHITECTURE_PATH), `${document}\n`); return parseArchitecture(dir); } - test.each<[string, string, string]>([ - ["a word", "typo", "'typo'"], - ["a yaml NaN", ".nan", "NaN"], - ["a map", "{count: 1}", "{ count: 1 }"], - ["a quoted number", '"1"', "'1'"], - ["a fraction", "1.5", "1.5"], - ["a negative", "-1", "-1"], - ])("a ratchet count that is %s fails naming the key and the value", (_case, value, shown) => - withTempDir("arch-lint-parse-", (dir) => { - const throws = `throws:| contracts: []| ratchet:| src/x.ts: ${value}`; - expect(parse(dir, throws)).toEqual( - err([ - `${ARCHITECTURE_PATH}: throws.ratchet["src/x.ts"] is ${shown}; expected a whole number of throws`, - ]), - ); - }), - ); - test.each<[string, string, string[]]>([ - ["a missing ratchet", "throws:| contracts: []", ["throws.ratchet is missing"]], - [ - "a misspelled ratchet", - "throws:| contracts: []| ratchets: {}", - ["throws.ratchet is missing", "unknown key throws.ratchets"], - ], - ["a misspelled throws", "throw: {}", ["throws is missing", "unknown key throw"]], - [ - "a ratchet path that is no file", - "throws:| contracts: []| ratchet:| src/gone.ts: 1", - [`throws.ratchet["src/gone.ts"] names no file under src/: 'src/gone.ts'`], - ], - [ - "a spared path outside src/", - "throws:| contracts: [test/x.ts]| ratchet: {}", - ["throws.contracts[0] names no file under src/: 'test/x.ts'"], - ], [ - "a spared path through a file", - "throws:| contracts: [src/x.ts/y.ts]| ratchet: {}", - ["throws.contracts[0] names no file under src/: 'src/x.ts/y.ts'"], + "a missing key", + "layers: {}\nexclude: []\nedge: {}", + ["edges is missing", "unknown key edge"], ], [ - "a spared path that leaves src/ through a parent segment", - "throws:| contracts: [src/../test/x.ts]| ratchet: {}", - ["throws.contracts[0] names no file under src/: 'src/../test/x.ts'"], + "a layer whose paths are not a list", + "layers: {engine: src/engine/}\nexclude: []\nedges: {}", + ["layers.engine is 'src/engine/'; Invalid input: expected array, received string"], ], [ "an unresolved yaml alias", - "throws: {contracts: [], ratchet: *missing}", + "layers: {}\nexclude: []\nedges: *missing", ["Unresolved alias (the anchor must be set before the alias): missing"], ], [ "a yaml syntax error", - "throws: [", + "layers: {}\nexclude: []\nedges: [", [ - "Flow sequence in block collection must be sufficiently indented and end with a ] at line 4, column 10", + "Flow sequence in block collection must be sufficiently indented and end with a ] at line 4, column 1", ], ], - ])("%s fails naming the key", (_case, throws, problems) => + ])("%s fails naming the key", (_case, document, problems) => withTempDir("arch-lint-parse-", (dir) => { - expect(parse(dir, throws)).toEqual(err(problems.map((p) => `${ARCHITECTURE_PATH}: ${p}`))); + expect(parse(dir, document)).toEqual(err(problems.map((p) => `${ARCHITECTURE_PATH}: ${p}`))); }), ); }); diff --git a/test/lint/never-throw.test.ts b/test/lint/never-throw.test.ts new file mode 100644 index 00000000..83f076b1 --- /dev/null +++ b/test/lint/never-throw.test.ts @@ -0,0 +1,380 @@ +/** + * The plugin's verdict on every throw class, and its biome.json wiring, pinned by linting a fixture tree with the + * repository's own configuration; a pattern that silently stops matching would otherwise let a throw pass unseen. + */ + +import { describe, expect, test } from "bun:test"; +import { spawnSync } from "node:child_process"; +import { mkdirSync, readFileSync, writeFileSync } from "node:fs"; +import { dirname, join } from "node:path"; +import { ROOT } from "../root.js"; +import { withTempDir } from "../temp-dir.js"; + +const BIOME = join(ROOT, "node_modules", ".bin", "biome"); +/** The repository's own configuration and the plugin it names, copied so the fixture tree is the root its + * `overrides` globs are relative to; `vcs.useIgnoreFile` wants that root to be a git repository. The root sits + * under a `src/` parent, so a glob matching the absolute path would reach every excluded file. */ +const CONFIGURATION = Object.fromEntries( + ["biome.json", "lint/never-throw.grit"].map((path) => [ + path, + readFileSync(join(ROOT, path), "utf8"), + ]), +); +const MESSAGE = + "throw outside the never-throw rule: not a BUG: invariant (an Error whose message starts with BUG:) and not a bare rethrow of the catch binding; return a Result instead"; + +/** A line ending in `// outside` is one the plugin must flag; every other throw here is inside the rule. */ +const OUTSIDE = /\/\/ outside$/; + +const THROWS = ` +export function bug(x: unknown): never { + if (x === null) throw new Error("BUG: bug() was handed null"); + if (x === undefined) throw new Error("BUG: bug() was handed undefined", { cause: x }); + throw new RangeError(\`BUG: bug() was handed \${String(x)}\`); +} +export function rethrow(run: () => void, keep: boolean): void { + try { + run(); + } catch (error) { + if (keep) { + throw error; + } + run(); + throw error; + } +} +export function rethrowBeforeFinally(run: () => void): void { + try { + run(); + } catch (error) { + run(); + throw error; + } finally { + run(); + } +} +export const rethrowInArrow = (run: () => void): void => { + try { + run(); + } catch (error) { + run(); + throw error; + } +}; +export function wrapped(run: () => void): void { + try { + run(); + } catch (error) { + const failure = new Error("y", { cause: error }); + throw failure; // outside + } +} +export function shadowed(run: (cause?: unknown) => void): void { + try { + run(); + } catch (error) { + run(error); + { + const error = new Error("x"); + throw error; // outside + } + } +} +export function shadowedByLoop(run: (cause?: unknown) => void): void { + try { + run(); + } catch (error) { + run(error); + for (const error of [new Error("y")]) { + throw error; // outside + } + } +} +export function fromCallbacks(run: () => void, items: number[]): void { + try { + run(); + } catch (error) { + items.map((x) => { + if (x) throw error; // outside + return x; + }); + items.forEach(function (x) { + if (x > this.floor) throw error; // outside + }, { floor: 1 }); + items.map(async () => { + throw error; // outside + }); + } +} +export function afterTheCatch(run: () => void): void { + try { + run(); + } catch (error) { + run(); + throw error; + } + const error = new Error("x"); + throw error; // outside +} +export function unbound(run: () => void): never { + try { + run(); + } catch { + throw run; // outside + } + throw new Error("x"); // outside +} +export function notBug(): never { + throw new Error(\`not BUG: \${1}\`); // outside +} +export function bare(): never { + throw "x"; // outside +} +export function fromTheTryBody(error: unknown, run: () => void): unknown { + try { + throw error; // outside + } catch (error) { + run(); + return error; + } +} +export function bugByExpression(x: string): never { + if (x) throw new Error("BUG: sentinel".slice(5)); // outside + throw new Error("BUG: " + x); // outside +} +export function bugWithMoreArguments(x: string): never { + throw new Error("BUG: x", { cause: x }, 3); +} +export function shadowedByDestructuring(run: (cause?: unknown) => void): void { + try { + run(); + } catch (error) { + run(error); + { + const { error } = { error: new Error("x") }; + throw error; // outside + } + } +} +export function shadowedByAParameter(run: (cause?: unknown) => void, items: unknown[]): void { + try { + run(); + } catch (error) { + items.map((error) => { + throw error; // outside + }); + run(error); + throw error; // outside + } +} +export function shadowedByANamespace(run: (cause?: unknown) => void, mode: number): void { + try { + run(); + } catch (error) { + run(error); + switch (mode) { + default: { + namespace error { + export const other = 1; + } + throw error; // outside + } + } + } +} +export function shadowedByATypeAlias(run: (code?: unknown) => void, mode: number): void { + try { + run(); + } catch (error) { + switch (mode) { + default: { + type error = number; + const code: error = 0; + run(code); + throw error; + } + } + } +} +export function shadowedByATypeSignature(run: (code?: unknown) => void, mode: number): void { + try { + run(); + } catch (error) { + switch (mode) { + default: { + type Handler = (error: unknown) => void; + const handle: Handler = run; + handle(error); + throw error; + } + } + } +} +export function shadowedByAnAnnotation(run: (code?: unknown) => void): void { + try { + run(); + } catch (error) { + const handle: (error: unknown) => void = run; + handle(error); + throw error; + } +} +export function fromAMethod(run: () => void): { fail(): never } { + try { + run(); + } catch (error) { + return { + fail() { + throw error; // outside + }, + }; + } + return { + fail: () => { + throw new Error("BUG: unreachable"); + }, + }; +} +export function rethrowTwiceOnceFromACallback(run: () => void, fail: boolean): (() => never) | undefined { + try { + run(); + } catch (error) { + if (fail) throw error; + return () => { + throw error; // outside + }; + } + return undefined; +} +export function rethrowFromANestedCatch(run: () => void, cleanup: () => void): void { + try { + run(); + } catch (outer) { + try { + cleanup(); + } catch { + throw outer; // outside + } + try { + cleanup(); + } catch (inner) { + run(); + throw inner; + } + } +} +export function fromANestedTry(run: () => void): void { + try { + run(); + } catch (error) { + try { + throw error; // outside + } catch { + run(); + } + try { + run(); + } finally { + throw error; // outside + } + } +} +export function destructuredBinding(run: () => void, fail: boolean): void { + try { + run(); + } catch ({ message }) { + if (fail) throw { message }; // outside + throw message; // outside + } +} +export function rethrowInsideACallbackCatch(run: (cause?: unknown) => void): () => void { + try { + run(); + } catch (error) { + run(error); + return () => { + try { + run(); + } catch (error) { + run(); + throw error; + } + }; + } + return run; +} +export function hidden(): never { + throw new Error('prefix "BUG: hidden'); // outside +} +`.trimStart(); + +const OUTSIDE_THE_RULE = 'export function outside(): never {\n throw new Error("x");\n}\n'; +const CONTRACT_FILE = "src/cli/inputs.ts"; + +interface Diagnostic { + category: string; + message: string; + location: { path: string; start: { line: number } }; +} + +function lint(tmp: string, files: Record): Record { + const dir = join(tmp, "src", "checkout"); + for (const [path, text] of Object.entries({ ...CONFIGURATION, ...files })) { + mkdirSync(dirname(join(dir, path)), { recursive: true }); + writeFileSync(join(dir, path), text); + } + spawnSync("git", ["init", "--quiet"], { cwd: dir }); + const run = spawnSync(BIOME, ["lint", "--reporter=json", "--no-errors-on-unmatched", "."], { + cwd: dir, + encoding: "utf8", + }); + if (run.stdout === "") { + throw new Error(`biome wrote no report: ${run.stderr}`); + } + const { diagnostics } = JSON.parse(run.stdout) as { diagnostics: Diagnostic[] }; + // One entry per line; when a line also carries a built-in rule's diagnostic (a throw inside finally trips + // noUnsafeFinally too), the plugin's wins, since the plugin is what this test pins. + const byLine: Record = {}; + for (const d of diagnostics) { + const key = `${d.location.path}:${d.location.start.line}`; + if (!(key in byLine) || d.category === "plugin") { + byLine[key] = `${d.category}: ${d.message}`; + } + } + return byLine; +} + +describe("the never-throw plugin", () => { + test("flags exactly the throws outside the rule, and nothing outside src/ or in the contract file", () => + withTempDir("never-throw-", (dir) => { + const expected = Object.fromEntries( + THROWS.split("\n").flatMap((line, index) => + OUTSIDE.test(line) ? [[`src/throws.ts:${index + 1}`, `plugin: ${MESSAGE}`]] : [], + ), + ); + expect( + lint(dir, { + "src/throws.ts": THROWS, + [CONTRACT_FILE]: OUTSIDE_THE_RULE, + "src/cli/beside-the-contract.ts": OUTSIDE_THE_RULE, + // The generated index is exempt from the formatter only, so the rule still reaches it. + "src/upstream-gaps/index.ts": OUTSIDE_THE_RULE, + "test/src/fixture.ts": OUTSIDE_THE_RULE, + ".github/scripts/beside-the-tree.ts": OUTSIDE_THE_RULE, + }), + ).toEqual({ + ...expected, + "src/cli/beside-the-contract.ts:2": `plugin: ${MESSAGE}`, + "src/upstream-gaps/index.ts:2": `plugin: ${MESSAGE}`, + }); + })); + + test("the contract file still throws outside the rule, so its exemption is not stale", () => + withTempDir("never-throw-", (dir) => { + const flagged = lint(dir, { + "src/cli/under-the-rule.ts": readFileSync(join(ROOT, CONTRACT_FILE), "utf8"), + }); + expect(Object.keys(flagged)).not.toEqual([]); + expect(new Set(Object.values(flagged))).toEqual(new Set([`plugin: ${MESSAGE}`])); + })); +});