diff --git a/.changeset/sg-regex-needs-kind-schema.md b/.changeset/sg-regex-needs-kind-schema.md index 1c753b23..4201da2b 100644 --- a/.changeset/sg-regex-needs-kind-schema.md +++ b/.changeset/sg-regex-needs-kind-schema.md @@ -3,3 +3,5 @@ --- `sg-regex-needs-kind` now lives in the ast-grep rule schema. The schema the CLI publishes states the pairing as `if`/`then` on every rule definition, and `validate` reports it from the schema layer with the same paths, so a schema-aware tool flags exactly what `validate` refuses. Every `regex`, at any depth, is covered: `rule`, `constraints`, `utils`, every `any`/`all`/`not`/`has`/`inside`/`precedes`/`follows` sub-rule, and the rarer positions (`stopBy`, `nthChild.ofRule`, utility arguments, `fix.expandStart`/`expandEnd`, rewriters). A sibling `any:` whose every branch sets a `kind` now anchors the regex, so a constraint no longer has to be split into one branch per kind, and an `all:` with one branch that sets a `kind` counts too. The message says how to find the kind, and it is reported even when another field also fails the schema. The `create-sg-rule` recipe (topic v15) lists the rule under Quirks. + +`validate` now notices a `has` or `inside` that declares no `stopBy`, as it already did for `follows` and `precedes`. Both default to looking one level (direct children, direct parent), so an exclusion such as `not: { inside: { kind: try_statement } }` silently stops excluding anything nested deeper. A relation that names a `field` is left alone, and `stopBy: neighbor` silences the notice where one level is what you mean. The `create-sg-rule` Quirks list has the matching entry. diff --git a/.taskless/rules/sg/prefer-node-process/prefer-node-process.yml b/.taskless/rules/sg/prefer-node-process/prefer-node-process.yml index 57ed91c3..32dc0185 100644 --- a/.taskless/rules/sg/prefer-node-process/prefer-node-process.yml +++ b/.taskless/rules/sg/prefer-node-process/prefer-node-process.yml @@ -44,6 +44,7 @@ rule: kind: program not: has: + stopBy: neighbor kind: import_statement has: stopBy: end @@ -64,6 +65,7 @@ rule: has: field: parameters has: + stopBy: neighbor any: - kind: required_parameter - kind: optional_parameter @@ -89,11 +91,13 @@ rule: - kind: statement_block - kind: program has: + stopBy: neighbor any: - any: - kind: lexical_declaration - kind: variable_declaration has: + stopBy: neighbor kind: variable_declarator has: field: name @@ -106,6 +110,7 @@ rule: - kind: lexical_declaration - kind: variable_declaration has: + stopBy: neighbor kind: variable_declarator has: field: name diff --git a/openspec/specs/cli-rule-validation/spec.md b/openspec/specs/cli-rule-validation/spec.md index 444d6941..68078584 100644 --- a/openspec/specs/cli-rule-validation/spec.md +++ b/openspec/specs/cli-rule-validation/spec.md @@ -478,6 +478,7 @@ The failure it prevents is not a local one: Vale reports this as `E201: has inva | -------------------------------------------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | `$$$` next to a comma | a pattern places a `$$$` metavariable directly beside a `,`, and that pattern is not an object pattern declaring `strictness: ast` | | a sibling relation that stops at punctuation | a `follows` or `precedes` relation declares no `stopBy` | +| a descent relation that looks one level | a `has` or `inside` relation declares neither `stopBy` nor `field` | | an awaited generic call hides its callee | a rule whose `language:` is `TypeScript` or `Tsx` keys a relation on the callee (`field: function`) with a `regex` anywhere beneath it, covering both a bare function (`identifier`) and a method (`member_expression`), and no alternative in that relation names an `await_expression` | A trap notice SHALL NOT fail the rule and SHALL NOT affect the exit code. Each shape has legitimate uses (a sibling relation that is meant to stop at the neighbour, a pattern meant to require at least one argument), so `validate` cannot know which one the author meant. A trap the author faces only as a notice is one they can decide about, and a trap that fails the rule is one they cannot write past. @@ -503,6 +504,12 @@ A trap that ast-grep itself refuses (a keyword or Markdown inline construct name - **THEN** the rule SHALL carry a notice naming `follows` and `stopBy: end` - **AND** the same relation declaring `stopBy: end` SHALL carry none +#### Scenario: A descent relation without stopBy draws a notice + +- **WHEN** an `sg` rule declares `not: { inside: { kind: try_statement } }` or `has: { kind: identifier }` with no `stopBy` +- **THEN** the rule SHALL carry a notice naming the relation, saying it checks only the direct parent or direct children, and naming `stopBy: end` +- **AND** the same relation declaring any `stopBy`, or naming a `field`, SHALL carry none + #### Scenario: A callee regex under TypeScript draws a notice - **WHEN** a `TypeScript` rule matches `kind: call_expression` with `has: { field: function, kind: identifier, regex: "^rpc$" }` diff --git a/packages/cli/src/agent/create-sg-rule.md b/packages/cli/src/agent/create-sg-rule.md index 4eb6baea..a380beb5 100644 --- a/packages/cli/src/agent/create-sg-rule.md +++ b/packages/cli/src/agent/create-sg-rule.md @@ -106,7 +106,8 @@ whole rule. - any `kind:` you read off a `--debug-query=cst` dump, - a rule that keys on a call's callee, if callers write `await f(...)`, - - a `follows` or `precedes`, or a `not` that reuses a metavariable. + - a `has`, `inside`, `follows` or `precedes` without `stopBy`, or + a `not` that reuses a metavariable. 6. **Write the tests.** Write at least two cases in each bucket under `.taskless/rules/sg//tests/`. These paths are the same ones the @@ -217,6 +218,7 @@ sometimes what the author meant. Read it and decide. | a keyword named as a `kind` | config error | yes | | a callee rule over `await f(...)` | silent | as a notice | | `follows` or `precedes` without `stopBy: end` | silent | as a notice | +| `has` or `inside` without `stopBy` | silent | as a notice | | a `not` containing a bare metavariable | silent | no | ### Every `regex`, at any depth, needs a `kind` @@ -408,7 +410,7 @@ rule: - kind: identifier regex: "^rpc$" - kind: await_expression - has: { kind: identifier, regex: "^rpc$" } + has: { kind: identifier, regex: "^rpc$", stopBy: neighbor } ``` The inner branch has to repeat whatever the plain branch matches. For @@ -422,6 +424,7 @@ both branches test that instead: - kind: await_expression has: kind: member_expression + stopBy: neighbor has: { field: property, kind: property_identifier, regex: "^rpc$" } ``` @@ -446,6 +449,31 @@ matches nothing, and the same rule with `stopBy: end` inside `follows` matches. If a sibling relation reports zero on input you believe should match, add `stopBy: end` before doubting the rest of the rule. +### `has` and `inside` look one level unless given `stopBy` + +Both default to `stopBy: neighbor`: `has` checks only the node's +direct children, and `inside` only its direct parent. The shape that +bites is an exclusion. Measured on TypeScript, with + +``` +rule: + pattern: fetch($$$) + not: + inside: { kind: try_statement } +``` + +a `fetch()` inside a `try` block is still reported, because its parent +is an `expression_statement`, and the `try_statement` sits two levels +above that, past the `statement_block`. Add `stopBy: end` inside `inside` and the exclusion +works. `has` fails the same way downward: in `foo(x)` the `x` is a +child of `arguments`, so `has: { kind: identifier, regex: "^x$" }` on +the call does not find it. + +`validate` notices each `has` and `inside` that declares no `stopBy`. +A relation that names a `field` is left alone, since a field already +picks one direct child. If the direct child or parent is what you +mean, say so with `stopBy: neighbor` and the notice goes away. + ### A `not` containing a bare metavariable excludes everything Writing "the name is not used in the initializer" as diff --git a/packages/cli/src/rules/sg-traps.ts b/packages/cli/src/rules/sg-traps.ts index 650f3c98..c0ced471 100644 --- a/packages/cli/src/rules/sg-traps.ts +++ b/packages/cli/src/rules/sg-traps.ts @@ -33,6 +33,7 @@ export function findSgTraps(rule: Record): string[] { notices.push( ...separatorTraps(node, path), ...siblingTraps(node, path), + ...descentTraps(node, path), ...(typescriptFamily ? calleeTraps(node, path) : []) ); }); @@ -126,6 +127,50 @@ function siblingTraps(node: Record, path: string): string[] { return notices; } +/** How far `has` and `inside` look under the default `stopBy: neighbor`. */ +const DESCENT = { + has: { + reach: "only the direct children", + missed: "a match nested any deeper is missed", + scope: "whole subtree", + }, + inside: { + reach: "only the direct parent", + missed: "an ancestor any further up is missed", + scope: "every ancestor", + }, +} as const; + +/** + * `has` or `inside` with no `stopBy`. The default `neighbor` checks only the + * direct children (`has`) or the direct parent (`inside`), so an exclusion such + * as `not: { inside: { kind: ... } }` stops excluding anything nested deeper, + * and a fixture written from the same assumption passes. + * + * A relation that names a `field` is left alone: the field already pins one + * particular parent-child link, which is exactly what the default looks at. + */ +function descentTraps(node: Record, path: string): string[] { + const notices: string[] = []; + for (const relation of ["has", "inside"] as const) { + const value = node[relation]; + if (value === null || typeof value !== "object" || Array.isArray(value)) { + continue; + } + if ("stopBy" in value || "field" in value) continue; + const { reach, missed, scope } = DESCENT[relation]; + // Under a negation the miss turns into a match: the exclusion quietly + // stops excluding, which is the form authors report hitting. + const negated = /(^|\.)not(\.|$)/.test(path) + ? `, so the \`not\` around it excludes nothing nested deeper` + : ""; + notices.push( + `${path}.${relation} declares no \`stopBy\`, so it checks ${reach}: ${missed}${negated}. Add \`stopBy: end\` inside \`${relation}\` to search the ${scope}, or \`stopBy: neighbor\` if stopping there is what you mean.` + ); + } + return notices; +} + /** * A callee matched by `regex` under `field: function`, in TypeScript or Tsx, * with no branch for the awaited form. The grammar parses diff --git a/packages/cli/test/ast-grep-vendor-contract.test.ts b/packages/cli/test/ast-grep-vendor-contract.test.ts index c3488215..2e5d531a 100644 --- a/packages/cli/test/ast-grep-vendor-contract.test.ts +++ b/packages/cli/test/ast-grep-vendor-contract.test.ts @@ -389,6 +389,21 @@ const grammarMatches = ( .filter((line) => line !== "") .map((line) => (JSON.parse(line) as { text: string }).text); +/** A `fetch()` excluded when inside a `try`, with `stopBy` text appended. */ +const tryExclusion = (stopBy: string): string => + [ + " pattern: fetch($$$)", + " not:", + ` inside: { kind: try_statement${stopBy} }`, + ].join("\n"); + +/** A call that has an `x` identifier, with `stopBy` text appended. */ +const hasX = (stopBy: string): string => + [ + " kind: call_expression", + ` has: { kind: identifier, regex: "^x$"${stopBy} }`, + ].join("\n"); + /** The call each finding matched, in file order — i.e. the arities accepted. */ const arityMatches = (body: string): string[] => grammarMatches(body, aritySource["src/a.ts"]); @@ -1163,7 +1178,7 @@ withSg("ast-grep vendor contract", () => { " - kind: identifier", ' regex: "^rpc$"', " - kind: await_expression", - ' has: { kind: identifier, regex: "^rpc$" }', + ' has: { kind: identifier, regex: "^rpc$", stopBy: neighbor }', ].join("\n"), awaitSource ) @@ -1187,6 +1202,7 @@ withSg("ast-grep vendor contract", () => { " - kind: await_expression", " has:", " kind: member_expression", + " stopBy: neighbor", ` ${property}`, ].join("\n"), [ @@ -1296,6 +1312,40 @@ withSg("ast-grep vendor contract", () => { }); }); + /** + * `has` and `inside` default to `stopBy: neighbor`: one level down, one + * level up. The create-sg-rule Quirks entry quotes both cases, and + * `findSgTraps` notices the shape because ast-grep runs it without a word. + */ + describe("has and inside look one level by default", () => { + const trySource = [ + "async function f() {", + " try {", + ' await fetch("a");', + " } catch {}", + ' fetch("b");', + "}", + "", + ].join("\n"); + + it("an inside exclusion misses an ancestor two levels up", () => { + expect(grammarMatches(tryExclusion(""), trySource)).toEqual([ + 'fetch("a")', + 'fetch("b")', + ]); + expect(grammarMatches(tryExclusion(", stopBy: end"), trySource)).toEqual([ + 'fetch("b")', + ]); + }); + + it("has misses a grandchild", () => { + expect(grammarMatches(hasX(""), "foo(x);\n")).toEqual([]); + expect(grammarMatches(hasX(", stopBy: end"), "foo(x);\n")).toEqual([ + "foo(x)", + ]); + }); + }); + describe("Haskell names a type synonym `type_synonym`", () => { // RENAMED AT 0.51.0, which moved tree-sitter-haskell from 0.23.1 to // 0.24.1. Upstream fixed a typo in the kind (`fix!: typo: 'synomym' -> diff --git a/packages/cli/test/sg-traps.test.ts b/packages/cli/test/sg-traps.test.ts index 4f19376c..607a650c 100644 --- a/packages/cli/test/sg-traps.test.ts +++ b/packages/cli/test/sg-traps.test.ts @@ -72,7 +72,7 @@ describe("$$$ beside a comma", () => { const notices = findSgTraps( typescript({ kind: "expression_statement", - has: { pattern: "foo($A, $$$)" }, + has: { pattern: "foo($A, $$$)", stopBy: "end" }, }) ); expect(notices).toHaveLength(1); @@ -110,6 +110,80 @@ describe("a sibling relation without stopBy", () => { }); }); +describe("a descent relation without stopBy", () => { + it("notices inside with no stopBy", () => { + const notices = findSgTraps( + typescript({ + kind: "call_expression", + not: { inside: { kind: "try_statement" } }, + }) + ); + expect(notices).toHaveLength(1); + expect(notices[0]).toContain("rule.not.inside"); + expect(notices[0]).toContain("direct parent"); + expect(notices[0]).toContain("the `not` around it excludes nothing"); + expect(notices[0]).toContain("stopBy: end"); + }); + + it("notices has with no stopBy", () => { + const notices = findSgTraps( + typescript({ + kind: "template_string", + has: { kind: "template_substitution" }, + }) + ); + expect(notices).toHaveLength(1); + expect(notices[0]).toContain("rule.has"); + expect(notices[0]).toContain("direct children"); + expect(notices[0]).not.toContain("`not`"); + expect(notices[0]).toContain("stopBy: end"); + }); + + it("finds one nested under constraints and utils", () => { + const notices = findSgTraps({ + ...typescript({ pattern: "$F()" }), + constraints: { F: { kind: "identifier", inside: { kind: "program" } } }, + utils: { u: { kind: "identifier", has: { kind: "identifier" } } }, + }); + expect(notices).toHaveLength(2); + expect(notices[0]).toContain("constraints.F.inside"); + expect(notices[1]).toContain("utils.u.has"); + }); + + it("is silent once stopBy is declared, end or neighbor", () => { + expect( + findSgTraps( + typescript({ + kind: "call_expression", + not: { inside: { kind: "try_statement", stopBy: "end" } }, + has: { kind: "arguments", stopBy: "neighbor" }, + }) + ) + ).toEqual([]); + }); + + // A field pins one parent-child link, which is what the default reads, so + // the author has already said how far to look. + it("is silent when the relation names a field", () => { + expect( + findSgTraps( + typescript({ + kind: "variable_declarator", + has: { field: "name", kind: "identifier" }, + }) + ) + ).toEqual([]); + expect( + findSgTraps( + typescript({ + kind: "statement_block", + inside: { field: "body", kind: "function_declaration" }, + }) + ) + ).toEqual([]); + }); +}); + describe("an awaited generic call hides its callee", () => { const plain = { kind: "call_expression", @@ -161,7 +235,7 @@ describe("an awaited generic call hides its callee", () => { { kind: "identifier", regex: "^rpc$" }, { kind: "await_expression", - has: { kind: "identifier", regex: "^rpc$" }, + has: { kind: "identifier", regex: "^rpc$", stopBy: "neighbor" }, }, ], }, @@ -190,6 +264,7 @@ describe("an awaited generic call hides its callee", () => { kind: "await_expression", has: { kind: "member_expression", + stopBy: "neighbor", has: { field: "property", kind: "property_identifier",