diff --git a/packages/cli/src/agent/create-sg-rule.txt b/packages/cli/src/agent/create-sg-rule.txt index 2f41a63e..727e4220 100644 --- a/packages/cli/src/agent/create-sg-rule.txt +++ b/packages/cli/src/agent/create-sg-rule.txt @@ -1,4 +1,4 @@ -# Topic: create-sg-rule (CLI v%(CLI_VERSION)s / topic v3) +# Topic: create-sg-rule (CLI v%(CLI_VERSION)s / topic v4) ## You are here This is `create-sg-rule`. It helps you write an ast-grep rule: a check @@ -218,6 +218,53 @@ whole rule. - Only after the user confirms, fetch `%(TASKLESS_CLI)s agent create-remote-rule` and follow it. Do not call the service silently. +## Two ways a relational rule matches nothing + +Both of these parse, verify, run, and exit 0. Neither reports anything +ever, which reads exactly like a clean codebase. + +**`follows` and `precedes` need `stopBy: end` to cross punctuation.** +A sibling relation walks the immediate siblings, and in most grammars +the separators are nodes too. Between two array elements sits a `,`, so +the previous element is not the previous SIBLING. Measured on +TypeScript, `[10, 10, 30]` with + +``` +rule: + kind: number + follows: { kind: number } +``` + +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. + +**A `not` containing a bare metavariable excludes everything.** Writing +"the name is not used in the initializer" as + +``` +rule: + kind: variable_declarator + has: { field: name, pattern: $A } + not: + has: { pattern: $A, stopBy: end } +``` + +matches nothing at all, because `$A` is bound to the name node and that +node is its own descendant: the `not` finds it inside every declarator +and rejects all of them. Scope the negation to the part you meant, in +this case the value: + +``` + not: + has: + field: value + has: { pattern: $A, stopBy: end } +``` + +The general shape: when a `not` searches the same subtree that bound the +metavariable, it always finds it. + ## Important Notes - Do NOT make any HTTP requests to taskless.io on this path. diff --git a/packages/cli/test/agent-extensions.test.ts b/packages/cli/test/agent-extensions.test.ts index bed1b750..ba8963d5 100644 --- a/packages/cli/test/agent-extensions.test.ts +++ b/packages/cli/test/agent-extensions.test.ts @@ -453,3 +453,41 @@ describe("the GitHub-owner constraint is guarded twice", () => { } ); }); + +/** + * The two shapes that verify clean and report nothing forever. Both cost real + * time to diagnose while building the matching-semantics differential, and + * neither was written down anywhere. + */ +describe("create-sg-rule warns about silently inert relational rules", () => { + let cwd: string; + + beforeEach(async () => { + cwd = await mkdtemp(join(tmpdir(), "taskless-sg-inert-")); + }); + + afterEach(async () => { + await rm(cwd, { recursive: true, force: true }); + }); + + it("says a sibling relation needs stopBy to cross punctuation", async () => { + const result = await runCli(["agent", "create-sg-rule", "-d", cwd]); + expect(result.exitCode).toBe(0); + expect(result.stdout).toContain("stopBy: end` to cross punctuation"); + // The reason, not just the fix: the separator is a node too. + expect(result.stdout).toContain("the separators are nodes too"); + }); + + it("says a not over the binding subtree excludes everything", async () => { + const result = await runCli(["agent", "create-sg-rule", "-d", cwd]); + expect(result.stdout).toContain("excludes everything"); + expect(result.stdout).toContain("its own descendant"); + }); + + it("frames both as reporting nothing rather than erroring", async () => { + // The dangerous property: zero findings is what a clean codebase looks + // like, so neither failure announces itself. + const result = await runCli(["agent", "create-sg-rule", "-d", cwd]); + expect(result.stdout).toContain("reads exactly like a clean codebase"); + }); +}); diff --git a/packages/cli/test/ast-grep-vendor-contract.test.ts b/packages/cli/test/ast-grep-vendor-contract.test.ts index c4351cc5..f9f85ca6 100644 --- a/packages/cli/test/ast-grep-vendor-contract.test.ts +++ b/packages/cli/test/ast-grep-vendor-contract.test.ts @@ -285,6 +285,46 @@ const arityRule = (body: string) => "", ].join("\n"); +/** Findings from one semantics rule over one source file, text and message. */ +const semanticsFindings = (body: string, source: string) => + scan( + project({ + rules: { semantics: semanticsRule(body) }, + sources: { "src/a.ts": source }, + }) + ) + .stdout.split("\n") + .filter((line) => line !== "") + .map((line) => JSON.parse(line) as { text: string; message: string }); + +/** The statement `nthChild` selects at `position`, over three siblings. */ +const nthChildAt = (position: number) => + semanticsFindings( + [ + " kind: expression_statement", + " nthChild:", + ` position: ${String(position)}`, + " ofRule: { pattern: $S }", + ].join("\n"), + "function f() { a(); b(); c(); }\n" + ).map((match) => match.text); + +/** + * A rule whose `message` is a bare metavariable, so a leaked binding is + * visible in the rendered output rather than only in the match. + */ +const semanticsRule = (body: string) => + [ + "id: semantics", + "language: TypeScript", + "severity: error", + "message: $A", + "note: n", + "rule:", + body, + "", + ].join("\n"); + /** A Markdown rule whose `rule:` body is given verbatim, already indented. */ const markdownRule = (body: string) => [ @@ -972,6 +1012,84 @@ withSg("ast-grep vendor contract", () => { }); }); + /** + * Two matching-semantics fixes from the 0.41.0 to 0.45.2 upgrade, pinned + * because they are the class no local check catches: a valid, unchanged rule + * matching a DIFFERENT set of nodes, with no error and no warning. + * + * Both cases come from the upstream pull requests rather than from guessing + * at the constructs. That distinction mattered: hand-written rules over the + * same features showed no difference at all, because neither bug fires + * unless a metavariable is bound in the specific position the fix changed. + */ + describe("metavariable bindings do not leak", () => { + it("counts nthChild positions from 1, not from 0", () => { + // Pinned because the vendored schema says otherwise. `NthChildSimple` + // describes the plain-number form as "A number indicating the precise + // element index" with `minimum: 0`, which reads as 0-indexed and would + // make the test below off by one, selecting `c();` while claiming + // `b();`. + // + // Measured against the binary: position 0 matches nothing at all, and + // 1, 2, 3 select the first, second and third. The behaviour is CSS-like + // and the schema's own description is misleading, so the ordering the + // next test depends on is asserted here rather than assumed. + expect(nthChildAt(0)).toEqual([]); + expect(nthChildAt(1)).toEqual(["a();"]); + expect(nthChildAt(2)).toEqual(["b();"]); + expect(nthChildAt(3)).toEqual(["c();"]); + }); + + it("counts every sibling when nthChild's ofRule binds a metavariable", () => { + // ast-grep/ast-grep#2677. `ofRule` reused one environment across all + // siblings, so the first match committed `$S` and every later sibling + // failed the consistency check and went uncounted. The rule then matched + // NOTHING at 0.41.0 while looking correct. + // + // Measured across the upgrade: 0.41.0 found 0, 0.45.2 finds `b();`. + // Swapping `pattern: $S` for a non-binding `kind:` was the workaround, + // which is what identified binding as the cause. + const matches = semanticsFindings( + [ + " kind: expression_statement", + " nthChild:", + " position: 2", + " ofRule: { pattern: $S }", + ].join("\n"), + "function f() { a(); b(); c(); }\n" + ); + expect(matches.map((m) => m.text)).toEqual(["b();"]); + }); + + it("leaves a metavariable unbound when a negated not rejected it", () => { + // ast-grep/ast-grep#2676. A `not` must contribute no bindings, since a + // successful negation means the inner rule did NOT match. It passed the + // live environment to its inner matcher, so a failed candidate left its + // bindings behind, and relational rules reuse one environment across + // candidates. + // + // The subtle part, and the reason this is asserted on the MESSAGE: the + // finding itself is identical across the upgrade. Same file, same range, + // same rule. Only the binding differs, so a differential comparing + // locations would report no change. At 0.41.0 `$A` rendered as `foo`, + // leaked from `return foo;`; here it must be empty. + const matches = semanticsFindings( + [ + " kind: expression_statement", + " pattern: target;", + " follows:", + " not:", + " pattern: return $A", + " stopBy: end", + ].join("\n"), + "function f() { bar(); return foo; target; }\n" + ); + expect(matches).toHaveLength(1); + expect(matches[0]?.text).toBe("target;"); + expect(matches[0]?.message.trim()).toBe(""); + }); + }); + describe("the `sg` alias prints a deprecation banner on stderr", () => { // DEPRECATED AT 0.45.0. `AST_GREP_BINARY.binaryNames` puts `ast-grep` // first, but the resolver reverses that list at its link-based tiers, so