diff --git a/biome.json b/biome.json index 04e0d019..f01efd90 100644 --- a/biome.json +++ b/biome.json @@ -59,7 +59,7 @@ } }, { - "includes": ["src/**", "!src/cli/inputs.ts"], + "includes": ["src/**"], "plugins": ["./lint/never-throw.grit"] }, { diff --git a/lint/never-throw.grit b/lint/never-throw.grit index ca5d038f..0fbe1379 100644 --- a/lint/never-throw.grit +++ b/lint/never-throw.grit @@ -1,12 +1,13 @@ -// 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. +// The never-throw rule of AGENTS.md as a Biome linter plugin: a `throw` or a `Promise.reject()` 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 rule covers src/** only; .github/scripts/ stays exempt by policy, since a // build script may throw on a user-caused failure. -// src/cli/inputs.ts -> exempt: commander's argParser can only refuse a value by throwing InvalidArgumentError +// src/cli/inputs.ts -> one `// biome-ignore lint/plugin:` suppression on the InvalidArgumentError throw: commander's +// argParser can only refuse a value by throwing // // GritQL has no scopes, so a second binding of the caught name anywhere in the catch body flags every rethrow in that // body, not only the shadowed one; the fix is to rename. The regex reads the literal's source text, so BUG: must open @@ -16,7 +17,14 @@ // a function-type parameter in a cast, return type, or signature -> a binding // a name in a type alias, an interface, or a type annotation -> not one: nothing there holds a value -`throw $arg` as $throw where { +or { + `throw $arg` as $site, + `Promise.reject($arg)` as $site, + `Promise.reject<$_>($arg)` as $site, + // No argument: a rejection with no reason is never a BUG: invariant or a rethrow. + `Promise.reject()` as $site where { $arg = `undefined` }, + `Promise.reject<$_>()` as $site where { $arg = `undefined` } +} where { not or { and { $arg <: JsNewExpression(arguments=JsCallArguments(args=[$first, ...])), @@ -27,7 +35,7 @@ $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 { + $site <: within JsCatchClause(declaration=JsCatchDeclaration(binding=$arg), body=$body) until or { JsTryStatement(), JsTryFinallyStatement(), JsArrowFunctionExpression(), @@ -54,5 +62,5 @@ } 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") + register_diagnostic(span=$site, message="throw or Promise.reject 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/src/cli/inputs.ts b/src/cli/inputs.ts index ed0472e8..d34fd093 100644 --- a/src/cli/inputs.ts +++ b/src/cli/inputs.ts @@ -174,6 +174,7 @@ function accumulate(value: string, previous?: string): string { export function once(flag: string): (value: string, previous?: string) => string { return (value, previous) => { if (previous !== undefined) { + // biome-ignore lint/plugin: commander's argParser can only refuse a value by throwing InvalidArgumentError throw new InvalidArgumentError(`--${flag} takes one value and was given more than once`); } return value; diff --git a/src/sections/registry.ts b/src/sections/registry.ts index fee58734..3844e7d6 100644 --- a/src/sections/registry.ts +++ b/src/sections/registry.ts @@ -224,24 +224,26 @@ const byKeyErased: { [K in SectionKey]: SectionModule } = byKey; * module property read per call -> handlers read `this`, and a test stubs the module while the engine holds this */ function refusingForeignContexts(module: SectionModule): SectionModule { - const refusal = (ctx: PlanContext, handler: "plan" | "snapshot"): Error | null => + const foreign = (ctx: PlanContext, handler: "plan" | "snapshot"): string | null => ctx.section === module.key ? null - : new Error( - `${module.key}.${handler}() was given the context built for section "${ctx.section}"; build it from this module: ` + - `${handler}Context(sectionModule("${module.key}"), api, repo${handler === "plan" ? "" : ", onMissingPermission"})`, - ); + : `${module.key}.${handler}() was given the context built for section "${ctx.section}"; build it from this module: ` + + `${handler}Context(sectionModule("${module.key}"), api, repo${handler === "plan" ? "" : ", onMissingPermission"})`; return { ...module, plan: (ctx, desired) => { - const error = refusal(ctx, "plan"); - return error === null ? module.plan(ctx, desired) : Promise.reject(error); + const problem = foreign(ctx, "plan"); + return problem === null + ? module.plan(ctx, desired) + : Promise.reject(new Error(`BUG: ${problem}`)); }, ...(hasSnapshot(module) ? { snapshot: (ctx: SnapshotContext) => { - const error = refusal(ctx, "snapshot"); - return error === null ? module.snapshot(ctx) : Promise.reject(error); + const problem = foreign(ctx, "snapshot"); + return problem === null + ? module.snapshot(ctx) + : Promise.reject(new Error(`BUG: ${problem}`)); }, } : {}), diff --git a/test/lint/never-throw.test.ts b/test/lint/never-throw.test.ts index 05764268..d0707238 100644 --- a/test/lint/never-throw.test.ts +++ b/test/lint/never-throw.test.ts @@ -11,7 +11,7 @@ import { withTempDir } from "../temp-dir.js"; import { lint } from "./fixture-lint.js"; 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"; + "throw or Promise.reject 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$/; @@ -296,13 +296,31 @@ export function rethrowInsideACallbackCatch(run: (cause?: unknown) => void): () export function hidden(): never { throw new Error('prefix "BUG: hidden'); // outside } +export function rejections(ok: boolean, error: unknown): Promise { + if (ok) return Promise.reject(new Error("BUG: rejections() was handed ok")); + if (error === null) return Promise.reject(new Error("x")); // outside + if (error === 0) return Promise.reject(); // outside + if (error === 1) return Promise.reject(); // outside + if (error === 2) return Promise.reject(new Error("x")); // outside + if (error === 3) return Promise.reject(new Error("BUG: rejections() was handed 3")); + return Promise.reject(error); // outside +} +export function rejectsTheCatchBinding(run: () => Promise): Promise { + try { + return run(); + } catch (error) { + return Promise.reject(error); + } +} `.trimStart(); const OUTSIDE_THE_RULE = 'export function outside(): never {\n throw new Error("x");\n}\n'; +/** The one throw the rule admits by suppression: commander's argParser can only refuse a value by throwing. */ const CONTRACT_FILE = "src/cli/inputs.ts"; +const SUPPRESSION = /^\s*\/\/ biome-ignore lint\/plugin: .*\n/m; describe("the never-throw plugin", () => { - test("flags exactly the throws outside the rule, and nothing outside src/ or in the contract file", () => + test("flags exactly the throws and rejections outside the rule, and nothing outside src/", () => withTempDir("never-throw-", (dir) => { const expected = Object.fromEntries( THROWS.split("\n").flatMap((line, index) => @@ -313,7 +331,6 @@ describe("the never-throw plugin", () => { 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/generated/upstream-gaps.ts": OUTSIDE_THE_RULE, "test/src/fixture.ts": OUTSIDE_THE_RULE, @@ -321,17 +338,17 @@ describe("the never-throw plugin", () => { }), ).toEqual({ ...expected, - "src/cli/beside-the-contract.ts:2": `plugin: ${MESSAGE}`, + [`${CONTRACT_FILE}:2`]: `plugin: ${MESSAGE}`, "src/generated/upstream-gaps.ts:2": `plugin: ${MESSAGE}`, }); })); - test("the contract file still throws outside the rule, so its exemption is not stale", () => + test("the contract file's one suppression covers a throw the rule would flag, so it 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}`])); + const contract = readFileSync(join(ROOT, CONTRACT_FILE), "utf8"); + expect(contract.match(new RegExp(SUPPRESSION.source, "gm"))).toHaveLength(1); + expect(lint(dir, { [CONTRACT_FILE]: contract })).toEqual({}); + const flagged = lint(dir, { [CONTRACT_FILE]: contract.replace(SUPPRESSION, "") }); + expect(Object.values(flagged)).toEqual([`plugin: ${MESSAGE}`]); })); });