From 9565c2e53aa50a15130c6b015f69230b1e592ac6 Mon Sep 17 00:00:00 2001 From: Jakob Heuser Date: Mon, 12 Oct 2026 14:53:35 -0700 Subject: [PATCH] fix(cli): make create-sg-rule's Quirks workarounds hold for real code A post-merge review of #488 found the recipe's own workarounds reintroduced the silent failures the section warns about. All measured on ast-grep 0.45.3. - The `readonly` workaround (`^readonly\b` on `required_parameter`) matched 1 of 4 real shapes. It missed `private readonly x` and a decorated parameter, whose text starts before the keyword, and `readonly x?`, which is an `optional_parameter`. The new regex covers all four and keeps out a `readonly` type and a parameter named `readonly`. - The awaited-call workaround and the repro both used a `regex` with no sibling `kind`, which `verify` refuses (`sg-regex-needs-kind`). Both now carry `kind: identifier`. The recipe also shows the method form: under `await`, `client.rpc()` wraps a `member_expression`, and the identifier branch found 0 of 2. - Step 5 claimed every quirk "verifies clean", but `verify` does report the `language:` spelling and a TypeScript rule whose `files:` names `.tsx`. The table gains a column saying which quirks `verify` reports. The vendor contract now pins the claims it previously took on trust: Tsx parses awaited generics the same way, `!`/`void`/`typeof` wrap the callee in `unary_expression` and `yield` does not, and both workarounds match exactly what the recipe says. The header reads "measured at" rather than "added at", since a bump did not introduce these. `typescriptMatches` and `arityMatches` were the third copy of the same parse chain; both now go through `grammarMatches`. The topic moves to v7, because v6 already shipped in the nightly, and the pending changeset says so. --- .changeset/sg-rule-quirks.md | 2 +- packages/cli/src/agent/create-sg-rule.md | 84 +++++--- packages/cli/test/agent-extensions.test.ts | 1 + .../cli/test/ast-grep-vendor-contract.test.ts | 189 ++++++++++++++---- 4 files changed, 214 insertions(+), 62 deletions(-) diff --git a/.changeset/sg-rule-quirks.md b/.changeset/sg-rule-quirks.md index fd15efb5..82cba1b6 100644 --- a/.changeset/sg-rule-quirks.md +++ b/.changeset/sg-rule-quirks.md @@ -2,4 +2,4 @@ "@taskless/cli": patch --- -`taskless agent create-sg-rule` now collects its ast-grep authoring traps into one **Quirks** section. Each entry gives the symptom (a config error that aborts the scan, or a rule that silently matches nothing), a minimal repro, and the workaround. Two new entries: a keyword such as `readonly` is not a valid `kind` (read kinds from `--debug-query=ast`, not the CST dump), and in TypeScript `await f(...)` parses as a call of the await expression, so a rule keyed on the callee silently skips every awaited generic call. +`taskless agent create-sg-rule` now collects its ast-grep authoring traps into one **Quirks** section, and the recipe moves to topic v7. Each entry gives the symptom (a config error that aborts the scan, or a rule that silently matches nothing), a minimal repro, and the workaround, and the table says which ones `verify` reports. Two new entries: a keyword such as `readonly` is not a valid `kind` (read kinds from `--debug-query=ast`, not the CST dump), and in TypeScript and Tsx `await f(...)` parses as a call of the await expression, so a rule keyed on the callee silently skips every awaited generic call. Both workarounds cover the common shapes (`private readonly x`, optional and decorated parameters, method callees) and pass `verify`. diff --git a/packages/cli/src/agent/create-sg-rule.md b/packages/cli/src/agent/create-sg-rule.md index 24563a50..7af08e8d 100644 --- a/packages/cli/src/agent/create-sg-rule.md +++ b/packages/cli/src/agent/create-sg-rule.md @@ -1,4 +1,4 @@ -# Topic: create-sg-rule (CLI v%(CLI_VERSION)s / topic v6) +# Topic: create-sg-rule (CLI v%(CLI_VERSION)s / topic v7) ## You are here This is `create-sg-rule`. It helps you write an ast-grep rule: a check @@ -100,10 +100,11 @@ whole rule. inside a line. 5. **Check the rule against Quirks.** Read the **Quirks** section below - against what you just wrote, before writing any tests. Every entry - there verifies clean and then either fails the whole scan or matches - less than it should, and a test file written from the same - assumption passes anyway. Look again at: + against what you just wrote, before writing any tests. Most entries + there get past `verify` and then either fail the whole scan or match + less than they should, and a test file written from the same + assumption passes anyway. The table says which ones `verify` does + catch. Look again at: - a `$$$` next to a comma (the separator trap), - any `kind:` you read off a `--debug-query=cst` dump, - a rule that keys on a call's callee, if callers write @@ -193,18 +194,19 @@ fail in one of two ways: When a rule matches less than it should, check this list before doubting the rest of the rule. Each entry was measured against the -ast-grep this CLI ships (v%(AST_GREP_VERSION)s). - -| quirk | symptom | -|------------------------------------------------|-------------------------| -| `language:` copied from `detect --json` | config error, or notice | -| `TypeScript` declared over `.tsx` files | silent | -| an inline construct named in a `Markdown` rule | config error, or silent | -| `$$$` next to a comma | silent | -| a keyword named as a `kind` | config error | -| a callee rule over `await f(...)` | silent | -| `follows` or `precedes` without `stopBy: end` | silent | -| a `not` containing a bare metavariable | silent | +ast-grep this CLI ships (v%(AST_GREP_VERSION)s). The last column says +whether `verify` reports it; where it says no, only you will notice. + +| quirk | symptom | `verify` reports it | +|------------------------------------------------|-------------------------|-----------------------------| +| `language:` copied from `detect --json` | config error, or notice | yes | +| `TypeScript` declared over `.tsx` files | silent | only if `files:` names .tsx | +| an inline construct named in a `Markdown` rule | config error, or silent | no | +| `$$$` next to a comma | silent | no | +| a keyword named as a `kind` | config error | no | +| a callee rule over `await f(...)` | silent | no | +| `follows` or `precedes` without `stopBy: end` | silent | no | +| a `not` containing a bare metavariable | silent | no | ### `language:` copied from `detect --json` @@ -217,7 +219,9 @@ Workaround: copy the spelling from the list in step 4. ### `Tsx` and `TypeScript` are two parsers, not aliases A rule that declares `TypeScript` does not read `.tsx` files at all, -so over a `.tsx` tree it reports nothing and exits 0. Workaround: +so over a `.tsx` tree it reports nothing and exits 0. `verify` catches +it only when the rule's `files:` globs name `.tsx`; with no `files:` +it cannot tell. Workaround: declare `Tsx` for `.tsx` files. A check that must cover both extensions is two rules, one per parser. @@ -294,14 +298,25 @@ keyword, not only `readonly`. Workaround: read kinds off `--debug-query=ast` instead, which prints named nodes only. A name that appears there is a usable kind; one that appears only in the CST dump is not. To match on the keyword, match -its named parent and test the parent's text: +its named parent and test the parent's text. Cover every shape the +keyword appears in: after a decorator or an accessibility modifier +(`private readonly x`), and on an optional parameter (`readonly x?`), +which is an `optional_parameter`, not a `required_parameter`: ```yaml rule: - kind: required_parameter - regex: "^readonly\\b" + any: + - kind: required_parameter + regex: "^(?:@\\S+\\s+)*(?:(?:public|private|protected)\\s+)?readonly\\s" + - kind: optional_parameter + regex: "^(?:@\\S+\\s+)*(?:(?:public|private|protected)\\s+)?readonly\\s" ``` +Anchoring at the start keeps a `readonly` type (`x: readonly string[]`) +and a parameter named `readonly` out. A decorator whose arguments +contain whitespace (`@Inject(A, B)`) defeats the `\S+`; add a fixture +for it if your codebase has one. + ### An awaited generic call hides its callee ```ts @@ -314,7 +329,7 @@ With ```yaml rule: kind: call_expression - has: { field: function, regex: "^rpc$" } + has: { field: function, kind: identifier, regex: "^rpc$" } ``` only the second call matches. The TypeScript grammar parses the first @@ -333,7 +348,7 @@ So any rule keyed on the callee silently skips every awaited generic call. The same happens in `Tsx`, and with `!`, `void` and `typeof` in place of `await`, where the callee becomes a `unary_expression`. `yield` is not affected. Workaround: accept the wrapped form as a -second branch. +second branch, matching the same callee inside it. ```yaml rule: @@ -341,14 +356,29 @@ rule: has: field: function any: - - regex: "^rpc$" + - kind: identifier + regex: "^rpc$" - kind: await_expression has: { kind: identifier, regex: "^rpc$" } ``` -Add a `unary_expression` branch the same way if those prefixes matter. -The wrapped match spans the `await` too, so a `fix:` on this rule -rewrites it along with the call. +The inner branch has to repeat whatever the plain branch matches. For +a method, `await client.rpc()` wraps a `member_expression`, so +both branches test that instead: + +```yaml + any: + - kind: member_expression + has: { field: property, kind: property_identifier, regex: "^rpc$" } + - kind: await_expression + has: + kind: member_expression + has: { field: property, kind: property_identifier, regex: "^rpc$" } +``` + +Add a `unary_expression` branch the same way if `!`, `void` or +`typeof` matter. The wrapped match spans the `await` too, so a `fix:` +on this rule rewrites it along with the call. ### `follows` and `precedes` need `stopBy: end` to cross punctuation diff --git a/packages/cli/test/agent-extensions.test.ts b/packages/cli/test/agent-extensions.test.ts index aa5e592a..e098dcf5 100644 --- a/packages/cli/test/agent-extensions.test.ts +++ b/packages/cli/test/agent-extensions.test.ts @@ -570,6 +570,7 @@ describe("create-sg-rule names the tree-sitter traps under Quirks", () => { it("says an awaited generic call hides its callee", async () => { const result = await runCli(["agent", "create-sg-rule", "-d", cwd]); + expect(result.exitCode).toBe(0); expect(result.stdout).toContain( "### An awaited generic call hides its callee" ); diff --git a/packages/cli/test/ast-grep-vendor-contract.test.ts b/packages/cli/test/ast-grep-vendor-contract.test.ts index 253dcccc..ca86c172 100644 --- a/packages/cli/test/ast-grep-vendor-contract.test.ts +++ b/packages/cli/test/ast-grep-vendor-contract.test.ts @@ -372,29 +372,51 @@ const statusAt = (severity: string) => }) ).status; -/** The call each finding matched, in file order — i.e. the arities accepted. */ -const arityMatches = (body: string): string[] => - scan(project({ rules: { arity: arityRule(body) }, sources: aritySource })) - .stdout.split("\n") - .filter((line) => line !== "") - .map((line) => (JSON.parse(line) as { text: string }).text); - -/** Scan one TypeScript rule, `rule:` body verbatim, over one source file. */ -const typescriptFindings = (body: string, source: string) => +/** + * Scan one rule, `rule:` body verbatim, over one source file. `TypeScript` + * reads `src/a.ts` and `Tsx` reads `src/a.tsx`, since each parser claims only + * its own extension. + */ +const grammarFindings = ( + body: string, + source: string, + language: "TypeScript" | "Tsx" = "TypeScript" +) => scan( project({ - rules: { arity: arityRule(body) }, - sources: { "src/a.ts": source }, + rules: { + grammar: [ + "id: grammar", + `language: ${language}`, + "severity: error", + "message: grammar", + "note: n", + "rule:", + body, + "", + ].join("\n"), + }, + sources: { + [language === "Tsx" ? "src/a.tsx" : "src/a.ts"]: source, + }, }) ); -/** The text each TypeScript finding matched, in file order. */ -const typescriptMatches = (body: string, source: string): string[] => - typescriptFindings(body, source) +/** The text each finding matched, in file order. */ +const grammarMatches = ( + body: string, + source: string, + language: "TypeScript" | "Tsx" = "TypeScript" +): string[] => + grammarFindings(body, source, language) .stdout.split("\n") .filter((line) => line !== "") .map((line) => (JSON.parse(line) as { text: string }).text); +/** The call each finding matched, in file order — i.e. the arities accepted. */ +const arityMatches = (body: string): string[] => + grammarMatches(body, aritySource["src/a.ts"]); + /** A rule whose fixtures all pass. */ const passingProject = () => project({ @@ -1058,33 +1080,60 @@ withSg("ast-grep vendor contract", () => { }); /** - * TWO TYPESCRIPT GRAMMAR TRAPS, ADDED AT 0.45.3. + * TWO TYPESCRIPT GRAMMAR TRAPS, MEASURED AT 0.45.3. * - * Both were hit while authoring rules on a real project, and - * `create-sg-rule.md` lists both under Quirks with a workaround each. These - * are the measurements behind that prose: if a grammar bump fixes either, the - * recipe is describing a parser that no longer exists, and this goes red. + * Long-standing grammar behavior, not something a bump introduced: both were + * first measured at 0.45.3, while authoring rules on a real project. + * `create-sg-rule.md` lists both under Quirks with a workaround each, and + * every claim that section makes about them is pinned here, workarounds + * included. If a grammar bump changes either, the recipe is describing a + * parser that no longer exists, and this goes red. */ describe("TypeScript keywords and awaited generic calls", () => { - const parameterSource = - "class C { constructor(readonly x: number, y: number) {} }\n"; + const parameterSource = [ + "class A { constructor(readonly a: number) {} }", + "class B { constructor(private readonly b: number) {} }", + "class C { constructor(@Inject() readonly c: number) {} }", + "class D { constructor(readonly d?: number) {} }", + "class E { constructor(x: readonly string[]) {} }", + "class F { constructor(readonly: number) {} }", + "class G { constructor(public g: number) {} }", + "", + ].join("\n"); it("rejects an anonymous keyword token as a kind", () => { // The CST dump prints `readonly` as a node, which is what invites this. // `kind:` takes named nodes only, so it is exit 8 and every other rule's // report goes with it. - const outcome = typescriptFindings(" kind: readonly", parameterSource); + const outcome = grammarFindings(" kind: readonly", parameterSource); expect(outcome.status).toBe(8); expect(outcome.stderr).toContain("Kind `readonly` is invalid"); }); - it("matches the keyword through its named parent's text", () => { + it("matches every readonly parameter shape the recipe names, and no other", () => { + // The recipe's workaround verbatim. An earlier `^readonly\b` over + // `required_parameter` alone matched only A: an accessibility modifier or + // a decorator comes first in the node's text, and `readonly d?` is an + // `optional_parameter`. E and F are the false positives the anchor keeps + // out. + const pattern = String.raw`^(?:@\\S+\\s+)*(?:(?:public|private|protected)\\s+)?readonly\\s`; expect( - typescriptMatches( - ' kind: required_parameter\n regex: "^readonly\\\\b"', + grammarMatches( + [ + " any:", + " - kind: required_parameter", + ` regex: "${pattern}"`, + " - kind: optional_parameter", + ` regex: "${pattern}"`, + ].join("\n"), parameterSource ) - ).toEqual(["readonly x: number"]); + ).toEqual([ + "readonly a: number", + "private readonly b: number", + "@Inject() readonly c: number", + "readonly d?: number", + ]); }); const awaitSource = [ @@ -1094,28 +1143,65 @@ withSg("ast-grep vendor contract", () => { "", ].join("\n"); + const calleeRule = [ + " kind: call_expression", + ' has: { field: function, kind: identifier, regex: "^rpc$" }', + ].join("\n"); + it("parses `await f()` as a call of the await expression", () => { // The silent one: the generic awaited call is simply absent from the // findings, exit 0. + expect(grammarMatches(calleeRule, awaitSource)).toEqual([ + 'rpc("y")', + 'rpc("z")', + ]); + }); + + it("parses it the same way under Tsx", () => { + expect(grammarMatches(calleeRule, awaitSource, "Tsx")).toEqual([ + 'rpc("y")', + 'rpc("z")', + ]); + }); + + it("hides the callee behind `!`, `void` and `typeof` too, but not `yield`", () => { + const source = [ + "function* g() {", + ' !rpc("a");', + ' void rpc("b");', + ' typeof rpc("c");', + ' yield rpc("d");', + "}", + "", + ].join("\n"); + expect(grammarMatches(calleeRule, source)).toEqual(['rpc("d")']); expect( - typescriptMatches( - ' kind: call_expression\n has: { field: function, regex: "^rpc$" }', - awaitSource + grammarMatches( + [ + " kind: call_expression", + " has: { field: function, kind: unary_expression }", + ].join("\n"), + source ) - ).toEqual(['rpc("y")', 'rpc("z")']); + ).toEqual([ + '!rpc("a")', + 'void rpc("b")', + 'typeof rpc("c")', + ]); }); it("recovers the awaited generic call with an await_expression branch", () => { - // The wrapped match spans the `await`, which the recipe warns a `fix:` - // will rewrite along with the call. + // The recipe's workaround verbatim. The wrapped match spans the `await`, + // which the recipe warns a `fix:` will rewrite along with the call. expect( - typescriptMatches( + grammarMatches( [ " kind: call_expression", " has:", " field: function", " any:", - ' - regex: "^rpc$"', + " - kind: identifier", + ' regex: "^rpc$"', " - kind: await_expression", ' has: { kind: identifier, regex: "^rpc$" }', ].join("\n"), @@ -1123,6 +1209,41 @@ withSg("ast-grep vendor contract", () => { ) ).toEqual(['await rpc("x")', 'rpc("y")', 'rpc("z")']); }); + + it("recovers an awaited generic method call with a member_expression branch", () => { + // The identifier branch alone finds none of these: under `await`, a + // method callee is a member_expression, and the recipe says so. + const property = + 'has: { field: property, kind: property_identifier, regex: "^rpc$" }'; + expect( + grammarMatches( + [ + " kind: call_expression", + " has:", + " field: function", + " any:", + " - kind: member_expression", + ` ${property}`, + " - kind: await_expression", + " has:", + " kind: member_expression", + ` ${property}`, + ].join("\n"), + [ + "async function f() {", + ' await this.rpc("x");', + ' await client.rpc("y");', + ' client.rpc("z");', + "}", + "", + ].join("\n") + ) + ).toEqual([ + 'await this.rpc("x")', + 'await client.rpc("y")', + 'client.rpc("z")', + ]); + }); }); /**