diff --git a/.changeset/sg-regex-needs-kind-schema.md b/.changeset/sg-regex-needs-kind-schema.md new file mode 100644 index 00000000..1c753b23 --- /dev/null +++ b/.changeset/sg-regex-needs-kind-schema.md @@ -0,0 +1,5 @@ +--- +"@taskless/cli": patch +--- + +`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. diff --git a/packages/cli/assets/reference.json b/packages/cli/assets/reference.json index c167008e..c08c16f3 100644 --- a/packages/cli/assets/reference.json +++ b/packages/cli/assets/reference.json @@ -44,8 +44,8 @@ "id": "sg-regex-needs-kind", "engine": "sg", "enforcedBy": "validate", - "summary": "A `regex` needs a sibling `kind`, in `rule`, `constraints` and `utils`.", - "rationale": "A regex match with no kind to anchor it is ambiguous and slow: it is applied to every node rather than to the one shape the author meant. ast-grep accepts it, so the engine is not the thing that will tell you." + "summary": "Every `regex`, at any depth, needs a `kind` beside it: a sibling `kind`, or a sibling `any` whose every branch sets one. This covers `rule`, `constraints`, `utils` and every sub-rule beneath them (`any`, `all`, `not`, `has`, `inside`, `precedes`, `follows`).", + "rationale": "A regex match with no kind to anchor it is ambiguous and slow: it is applied to every node rather than to the one shape the author meant. ast-grep refuses it only at the top of `rule` and runs it in any sub-rule, so beyond that one spot the engine is not the thing that will tell you. The pairing is part of the rule schema Taskless publishes (`if`/`then` on every rule definition), so a schema-aware editor flags the same paths `validate` does." }, { "id": "sg-language-accepted", diff --git a/packages/cli/src/agent/create-sg-rule.md b/packages/cli/src/agent/create-sg-rule.md index d1d37ba5..4eb6baea 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 v14) +# Topic: create-sg-rule (CLI v%(CLI_VERSION)s / topic v15) %(ONE_COMMAND_PER_CALL)s @@ -163,8 +163,9 @@ whole rule. | a case didn't behave as expected | fix the rule pattern OR the test case | | the rule's scope excluded a case | move the fixture to a path `files:` admits | - A `regex` without an accompanying `kind` fails validation, the two - always travel together. + Every `regex`, at any depth, needs a `kind` beside it, or + validation fails. That includes `constraints`, `utils`, and every + `not`, `has`, `inside`, `any` and `all` sub-rule; see **Quirks**. Pass a directory above a rule and every rule beneath it is checked, one entry per rule in the report. `.taskless/rules/sg` covers every @@ -188,9 +189,9 @@ whole rule. ## Quirks -Each of these comes from a grammar or from ast-grep itself, so a rule -written carefully from the upstream reference still hits them. They -fail in one of two ways: +Each of these, except the first, comes from a grammar or from ast-grep +itself, so a rule written carefully from the upstream reference still +hits them. Those fail in one of two ways: - **Config error.** The scan exits 8 and names the problem. Loud, but it aborts config parsing, so every other `sg` rule in the project @@ -208,6 +209,7 @@ sometimes what the author meant. Read it and decide. | quirk | symptom | `validate` reports it | |------------------------------------------------|-------------------------|-----------------------------| +| a `regex` without a `kind`, at any depth | refused by `validate` | yes | | `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 | the `kind:` form only | @@ -217,6 +219,44 @@ sometimes what the author meant. Read it and decide. | `follows` or `precedes` without `stopBy: end` | silent | as a notice | | a `not` containing a bare metavariable | silent | no | +### Every `regex`, at any depth, needs a `kind` + +This one is a Taskless rule, wider than ast-grep's own check. ast-grep +refuses a bare `regex` only at the top of `rule` (a config error); in +any sub-rule it runs it against the text of every node it reaches, +which is ambiguous and slow. `validate` refuses a `regex` with no +`kind` beside it anywhere it appears: the top of `rule`, +`constraints.`, `utils`, `not`, `has`/`inside` (however deeply +nested), `precedes`/`follows`, and each `any`/`all` branch. + +A sibling `any:` whose every branch sets a `kind` counts, so a +metavariable that may capture one of several kinds keeps a single +regex. A callee is an `identifier` in `rpc()` and a +`member_expression` in `client.rpc()`: + +```yaml +rule: + pattern: "$CALLEE()" +constraints: + CALLEE: + any: + - kind: identifier + - kind: member_expression + regex: "(^|\\.)rpc$" +``` + +An `all:` with one branch that sets a `kind` counts too. A `kind` +inside `not`, `has` or another relation does not, because it describes +a different node. + +To find the kind, run ast-grep with `--debug-query=ast` on a sample of +the code and take the named node the regex should match. Under +`constraints`, that is the node the metavariable captures, which is +not always the obvious one: the method `$M` captures in `$OBJ.$M()` is +a `property_identifier` in TypeScript, not an `identifier`. Read it off +the dump rather than guessing, and if the metavariable can capture +several kinds, list them in a sibling `any:` as above. + ### `language:` copied from `detect --json` `detect --json` reports the *repository's* languages, in a different diff --git a/packages/cli/src/rules/constraints.ts b/packages/cli/src/rules/constraints.ts index e8a65b4c..9c4f7bb2 100644 --- a/packages/cli/src/rules/constraints.ts +++ b/packages/cli/src/rules/constraints.ts @@ -82,9 +82,9 @@ export const RULE_CONSTRAINTS = [ engine: "sg", enforcedBy: "validate", summary: - "A `regex` needs a sibling `kind`, in `rule`, `constraints` and `utils`.", + "Every `regex`, at any depth, needs a `kind` beside it: a sibling `kind`, or a sibling `any` whose every branch sets one. This covers `rule`, `constraints`, `utils` and every sub-rule beneath them (`any`, `all`, `not`, `has`, `inside`, `precedes`, `follows`).", rationale: - "A regex match with no kind to anchor it is ambiguous and slow: it is applied to every node rather than to the one shape the author meant. ast-grep accepts it, so the engine is not the thing that will tell you.", + "A regex match with no kind to anchor it is ambiguous and slow: it is applied to every node rather than to the one shape the author meant. ast-grep refuses it only at the top of `rule` and runs it in any sub-rule, so beyond that one spot the engine is not the thing that will tell you. The pairing is part of the rule schema Taskless publishes (`if`/`then` on every rule definition), so a schema-aware editor flags the same paths `validate` does.", }, { id: "sg-language-accepted", diff --git a/packages/cli/src/rules/verify.ts b/packages/cli/src/rules/verify.ts index 2e644786..c4d74928 100644 --- a/packages/cli/src/rules/verify.ts +++ b/packages/cli/src/rules/verify.ts @@ -3,9 +3,10 @@ import { readFile } from "node:fs/promises"; import { parse } from "yaml"; import { + astGrepRuleJsonSchema, astGrepRuleSchema, + REGEX_NEEDS_KIND, TASKLESS_REQUIRED_FIELDS, - findRegexWithoutKind, } from "../schemas/ast-grep-rule"; import { pathPrefixed, schemaLayer } from "../schemas/layer"; import { @@ -28,7 +29,6 @@ import { import { ensureTasklessDirectory } from "../filesystem/directory"; import { ruleFilePath } from "./engines"; import { RULE_TESTS_DIRECTORY, RULES_DIRECTORY } from "./layout"; -import astGrepJsonSchema from "../generated/ast-grep-rule-schema.json"; import { RULE_EXAMPLES } from "./verify-examples"; import { violate, type RuleViolation } from "./constraints"; import { isValidRuleId } from "./validate-id"; @@ -105,14 +105,14 @@ export interface VerifyResult { export function getSchemaPayload(): Record { return { - astGrepSchema: astGrepJsonSchema, + astGrepSchema: astGrepRuleJsonSchema, tasklessRequirements: { requiredFields: [...TASKLESS_REQUIRED_FIELDS], rules: [ { name: "regex-requires-kind", description: - "Rules using `regex` in any position must also specify `kind` at the same level. Without kind, regex matches against node text which is ambiguous and slow.", + "Every `regex`, at any depth (`rule`, `constraints`, `utils`, and every sub-rule beneath them), needs a `kind` beside it: a sibling `kind`, or a sibling `any` whose every branch sets `kind`. `astGrepSchema` states this as `if`/`then` on its rule definitions. Without kind, regex matches against node text which is ambiguous and slow.", }, ], }, @@ -127,7 +127,24 @@ function validateSchema(ruleData: unknown): LayerResult { // way whichever engine wrote it. `pathPrefixed` is the half that differs: // ast-grep's messages come from an upstream JSON Schema and are generic // ("expected string"), so the path is what makes them actionable. - return schemaLayer(astGrepRuleSchema.safeParse(ruleData), pathPrefixed); + const result = astGrepRuleSchema.safeParse(ruleData); + const layer = schemaLayer(result, pathPrefixed); + // The regex/kind pairing lives in the schema (see `astGrepRuleJsonSchema`), + // and its issues say which constraint they are, so they are attributed + // here rather than re-derived from their wording. + const violations: RuleViolation[] = []; + for (const issue of result.error?.issues ?? []) { + if ( + issue.code === "custom" && + issue.params?.constraintId === REGEX_NEEDS_KIND + ) { + violations.push({ + constraintId: REGEX_NEEDS_KIND, + message: pathPrefixed(issue), + }); + } + } + return { ...layer, violations }; } /** @@ -309,38 +326,6 @@ async function validateRequirements( } } - // Check regex-requires-kind in rule and other top-level rule containers - for (const key of ["rule", "constraints", "utils"]) { - const container = ruleData[key]; - if ( - container && - typeof container === "object" && - !Array.isArray(container) - ) { - if (key === "rule") { - for (const message of findRegexWithoutKind( - container as Record - )) { - violate(collected, "sg-regex-needs-kind", message); - } - } else { - // constraints/utils are Record - for (const [name, value] of Object.entries( - container as Record - )) { - if (value && typeof value === "object" && !Array.isArray(value)) { - for (const message of findRegexWithoutKind( - value as Record, - `${key}.${name}` - )) { - violate(collected, "sg-regex-needs-kind", message); - } - } - } - } - } - } - // `strictness:` at the top level is accepted by ast-grep and ignored, so the // pattern keeps narrowing exactly as before. Inside `rule:` the same key is a // schema error; up here only this check can say so. @@ -601,7 +586,7 @@ export async function verifyRule( const schemaResult: SchemaLayerResult = { valid: schemaErrors.length === 0, errors: schemaErrors, - violations: language.violations, + violations: [...(parsed.violations ?? []), ...language.violations], notices: [...language.notices, ...findSgTraps(ruleRecord)], }; diff --git a/packages/cli/src/schemas/ast-grep-rule.ts b/packages/cli/src/schemas/ast-grep-rule.ts index 5a86ed1d..af82e4ed 100644 --- a/packages/cli/src/schemas/ast-grep-rule.ts +++ b/packages/cli/src/schemas/ast-grep-rule.ts @@ -1,82 +1,284 @@ import { z } from "zod"; -import astGrepSchema from "../generated/ast-grep-rule-schema.json"; +import upstreamSchema from "../generated/ast-grep-rule-schema.json"; +import type { RuleConstraintId } from "../rules/constraints"; /** - * Zod schema for ast-grep rule validation, derived from the official - * ast-grep JSON Schema via `z.fromJSONSchema()`. This gives us full - * validation coverage matching the upstream spec. + * The `$defs` entries in ast-grep's schema that describe a rule object, and so + * the only places a `regex` can appear. `SerializableRule` is every plain rule + * (`rule`, `constraints.*`, `utils.*`, `any`/`all` items, `not`, a rule-form + * `stopBy`, `nthChild.ofRule`, utility arguments, rewriters); `Relation` is the + * same object with `stopBy` and `field` added (`has`, `inside`, `precedes`, + * `follows`, a fix's `expandStart`/`expandEnd`). + */ +const RULE_DEFINITIONS: readonly string[] = ["SerializableRule", "Relation"]; + +/** Our own `$defs` entry, prefixed so an upstream name can never collide. */ +const PINS_KIND_DEFINITION = "TasklessPinsKind"; + +/** The constraint the pairing reports under. See `rules/constraints.ts`. */ +export const REGEX_NEEDS_KIND = + "sg-regex-needs-kind" satisfies RuleConstraintId; + +/** + * What the pairing tells an author to do, shared by the JSON Schema + * description an editor shows and the message `validate` prints. + */ +export const REGEX_NEEDS_KIND_HINT = + 'Every "regex", at any depth, needs a "kind" beside it: a sibling `kind:`, ' + + "or a sibling `any:` whose every branch sets `kind:` (or an `all:` with a " + + "branch that does). Without one the regex is tested against the text of " + + "every node, which is ambiguous and slow. To find the kind, run ast-grep " + + "with `--debug-query=ast` on a sample of the code and take the named node " + + "the regex should match; under `constraints`, that is the node the " + + "metavariable captures."; + +type JsonObject = Record; + +function isObject(value: unknown): value is JsonObject { + return typeof value === "object" && value !== null && !Array.isArray(value); +} + +/** + * ast-grep's rule schema with the regex/kind pairing written into it. + * + * The upstream schema is vendored verbatim (`scripts/fetch-ast-grep-schema.ts` + * overwrites it on every upgrade), so the pairing is layered on here rather + * than edited into the generated file. Every rule-object definition gains + * `if: { required: [regex] }, then: TasklessPinsKind`, and `TasklessPinsKind` + * is the one statement of what anchors a regex: + * + * - a `kind` in the same object, or + * - an `any` whose every branch pins a kind, since whichever branch matched, + * the node has one of those kinds, or + * - an `all` with at least one branch that pins a kind, since every branch + * matched. + * + * `if`/`then` rather than 2020-12's `dependentSchemas`: both say the same + * thing, and `if`/`then` is the one YAML and JSON language servers have + * supported longest. + * + * zod cannot consume either keyword (`z.fromJSONSchema` throws on both), so + * {@link astGrepRuleSchema} is built from the upstream schema and mirrors this + * with a check. {@link findRegexWithoutKind} walks the data against this + * same schema, so the two report the same paths by construction. + */ +function withRegexNeedsKind(schema: JsonObject): JsonObject { + const augmented = structuredClone(schema); + const definitions = augmented.$defs; + if (!isObject(definitions)) { + throw new Error("ast-grep rule schema has no $defs"); + } + for (const name of RULE_DEFINITIONS) { + const definition = definitions[name]; + // A vendor upgrade that renames either definition, or starts using + // `if`/`then` itself, must fail loudly here rather than ship a schema + // that silently stopped saying this. + if (!isObject(definition)) { + throw new Error(`ast-grep rule schema has no $defs.${name}`); + } + if ("if" in definition || "then" in definition) { + throw new Error(`ast-grep rule schema already uses if/then on ${name}`); + } + definition.if = { required: ["regex"] }; + // A JSON Schema keyword on a plain data object, never awaited. + // eslint-disable-next-line unicorn/no-thenable + definition.then = { $ref: `#/$defs/${PINS_KIND_DEFINITION}` }; + } + definitions[PINS_KIND_DEFINITION] = { + description: `Taskless (${REGEX_NEEDS_KIND}): ${REGEX_NEEDS_KIND_HINT}`, + anyOf: [ + { required: ["kind"] }, + { + required: ["any"], + properties: { + any: { + type: "array", + minItems: 1, + items: { $ref: `#/$defs/${PINS_KIND_DEFINITION}` }, + }, + }, + }, + { + required: ["all"], + properties: { + all: { + type: "array", + contains: { $ref: `#/$defs/${PINS_KIND_DEFINITION}` }, + }, + }, + }, + ], + }; + return augmented; +} + +/** + * ast-grep's rule JSON Schema, plus the Taskless regex/kind pairing. * - * The raw JSON Schema is also embedded in `getSchemaPayload()`'s - * `astGrepSchema` field, for agent-facing consumption. There is no `--schema` - * CLI flag any more — it was removed CLI-wide in favor of embedding schema - * content in recipes — so nothing today reaches this via a command a user - * types; the comment used to say `rule verify --schema`, which named both a - * flag and a command form that no longer exist. + * This is the schema to hand anything schema-aware: an editor, or an agent + * reading `getSchemaPayload()`'s `astGrepSchema`. It flags a `regex` without + * a kind at exactly the paths `validate` refuses. */ -// eslint-disable-next-line @typescript-eslint/no-explicit-any, @typescript-eslint/no-unsafe-argument -export const astGrepRuleSchema = z.fromJSONSchema(astGrepSchema as any); +export const astGrepRuleJsonSchema = withRegexNeedsKind( + upstreamSchema as JsonObject +); /** - * Taskless-specific required fields, beyond what ast-grep requires. - * ast-grep only requires `rule` + `language`; Taskless also requires - * `id`, `severity`, and `message` for meaningful rule output. + * Whether a rule object fixes the kind of the node it matches, by the + * definition in `TasklessPinsKind` above. */ -export const TASKLESS_REQUIRED_FIELDS = [ - "id", - "language", - "severity", - "message", - "rule", -] as const; +function pinsKind(rule: JsonObject): boolean { + if ("kind" in rule) return true; + const { any, all } = rule; + if ( + Array.isArray(any) && + any.length > 0 && + any.every((branch) => isObject(branch) && pinsKind(branch)) + ) { + return true; + } + return ( + Array.isArray(all) && + all.some((branch) => isObject(branch) && pinsKind(branch)) + ); +} + +export interface RegexWithoutKind { + /** Where the offending rule object sits, as zod reports paths. */ + path: (string | number)[]; + message: string; +} + +const DEFINITION_PREFIX = "#/$defs/"; /** - * Check if a rule object uses `regex` anywhere without a sibling `kind`. - * Recursively walks the rule tree. + * Visit `value` against `schema`, reporting every rule object that uses + * `regex` without pinning a kind. + * + * Schema-directed rather than a hand-listed set of keys, so it reaches every + * position ast-grep's schema allows a rule (including `stopBy`, `nthChild`'s + * `ofRule`, utility arguments, a fix's `expandStart`/`expandEnd`, and + * rewriters) and cannot drift from the JSON Schema an editor applies. */ -export function findRegexWithoutKind( - object: Record, - path = "" -): string[] { - const errors: string[] = []; - - if ("regex" in object && !("kind" in object)) { - errors.push( - `${path || "rule"}: uses "regex" without a sibling "kind" field. Regex matches without kind are ambiguous and slow.` - ); +function visit( + definitions: JsonObject, + schema: unknown, + value: unknown, + path: (string | number)[], + found: Map +): void { + if (!isObject(schema)) return; + + let node = schema; + const reference = schema.$ref; + if ( + typeof reference === "string" && + reference.startsWith(DEFINITION_PREFIX) + ) { + const name = reference.slice(DEFINITION_PREFIX.length); + const resolved = definitions[name]; + if (!isObject(resolved)) return; + node = resolved; + if ( + RULE_DEFINITIONS.includes(name) && + isObject(value) && + "regex" in value && + !pinsKind(value) + ) { + // Keyed by path: an `anyOf` can reach the same object twice. + found.set(JSON.stringify(path), { path, message: REGEX_NEEDS_KIND_HINT }); + } } - // Recurse into known composite fields - for (const key of [ - "any", - "all", - "not", - "has", - "inside", - "precedes", - "follows", - ]) { - const value = object[key]; - if (Array.isArray(value)) { - for (const [index, item] of value.entries()) { - if (item && typeof item === "object" && !Array.isArray(item)) { - errors.push( - ...findRegexWithoutKind( - item as Record, - `${path || "rule"}.${key}[${String(index)}]` - ) - ); - } + for (const keyword of ["anyOf", "oneOf", "allOf"]) { + const branches = node[keyword]; + if (Array.isArray(branches)) { + for (const branch of branches) { + visit(definitions, branch, value, path, found); } - } else if (value && typeof value === "object" && !Array.isArray(value)) { - errors.push( - ...findRegexWithoutKind( - value as Record, - `${path || "rule"}.${key}` - ) - ); } } - return errors; + if (isObject(value)) { + const properties = isObject(node.properties) ? node.properties : {}; + for (const [key, child] of Object.entries(value)) { + const childSchema = + key in properties ? properties[key] : node.additionalProperties; + visit(definitions, childSchema, child, [...path, key], found); + } + } else if (Array.isArray(value)) { + for (const [index, item] of value.entries()) { + visit(definitions, node.items, item, [...path, index], found); + } + } +} + +/** + * Every rule object in a parsed ast-grep rule file that uses `regex` without + * a kind to anchor it: in `rule`, `constraints`, `utils`, and every composite + * and relational sub-rule beneath them, at any depth. + */ +export function findRegexWithoutKind(ruleFile: unknown): RegexWithoutKind[] { + const found = new Map(); + visit( + astGrepRuleJsonSchema.$defs as JsonObject, + astGrepRuleJsonSchema, + ruleFile, + [], + found + ); + return [...found.values()]; } + +/** ast-grep's rule schema as zod reads it, without the regex/kind pairing. */ +const upstreamRuleSchema = z + // eslint-disable-next-line @typescript-eslint/no-explicit-any, @typescript-eslint/no-unsafe-argument + .fromJSONSchema(upstreamSchema as any); + +/** + * Zod schema for ast-grep rule validation: the official ast-grep JSON Schema + * via `z.fromJSONSchema()`, plus the regex/kind pairing + * {@link astGrepRuleJsonSchema} adds. zod cannot read the `if`/`then` that + * states it there, so the pairing is checked here by hand. + * + * The two run side by side rather than as `upstream.superRefine(pairing)`, + * because zod skips a refinement once the schema it is attached to has + * failed: a rule with a mistyped `severity` would hide its regex problem + * until the next round, which is the round this check exists to save. So the + * outer schema accepts any input, and its one check reports the upstream + * schema's issues first, then one issue per path + * {@link findRegexWithoutKind} reports, each carrying `params.constraintId` + * so `validate` can attribute it to `sg-regex-needs-kind` without matching on + * wording. + */ +export const astGrepRuleSchema = z.unknown().superRefine((value, context) => { + const upstream = upstreamRuleSchema.safeParse(value); + for (const issue of upstream.error?.issues ?? []) { + // `continue: false` keeps zod's default for a schema's own issues, which + // `addIssue` would otherwise flip to `true`. + context.addIssue({ ...issue, continue: false }); + } + for (const { path, message } of findRegexWithoutKind(value)) { + context.addIssue({ + code: "custom", + path, + message, + continue: false, + params: { constraintId: REGEX_NEEDS_KIND }, + }); + } +}); + +/** + * Taskless-specific required fields, beyond what ast-grep requires. + * ast-grep only requires `rule` + `language`; Taskless also requires + * `id`, `severity`, and `message` for meaningful rule output. + */ +export const TASKLESS_REQUIRED_FIELDS = [ + "id", + "language", + "severity", + "message", + "rule", +] as const; diff --git a/packages/cli/test/ast-grep-rule-schema.test.ts b/packages/cli/test/ast-grep-rule-schema.test.ts new file mode 100644 index 00000000..9737cfdd --- /dev/null +++ b/packages/cli/test/ast-grep-rule-schema.test.ts @@ -0,0 +1,315 @@ +import { describe, expect, it } from "vitest"; +import { z } from "zod"; + +import upstreamSchema from "../src/generated/ast-grep-rule-schema.json"; +import { + astGrepRuleJsonSchema, + astGrepRuleSchema, + findRegexWithoutKind, +} from "../src/schemas/ast-grep-rule"; + +/** + * The regex/kind pairing (`sg-regex-needs-kind`), stated once in the JSON + * Schema and mirrored by the zod check `validate` runs. See taskless/cli#671. + */ + +const BASE = { + id: "probe", + language: "TypeScript", + severity: "error", + message: "probe", +}; + +/** The dotted paths `validate` would refuse, for a rule file body. */ +function refused(file: Record): string[] { + return findRegexWithoutKind({ ...BASE, ...file }).map(({ path }) => + path.join(".") + ); +} + +describe("findRegexWithoutKind", () => { + it("accepts a regex with a sibling kind", () => { + expect(refused({ rule: { kind: "identifier", regex: "^a$" } })).toEqual([]); + }); + + it("refuses a regex at the top of `rule`", () => { + expect(refused({ rule: { regex: "^a$" } })).toEqual(["rule"]); + }); + + it("refuses a regex inside `constraints.`", () => { + expect( + refused({ + rule: { pattern: "foo($A)" }, + constraints: { A: { regex: "^a$" } }, + }) + ).toEqual(["constraints.A"]); + }); + + it("refuses a regex inside `utils.`", () => { + expect( + refused({ + rule: { matches: "is-a" }, + utils: { "is-a": { regex: "^a$" } }, + }) + ).toEqual(["utils.is-a"]); + }); + + it("refuses a regex inside `not`", () => { + expect( + refused({ rule: { kind: "identifier", not: { regex: "^a$" } } }) + ).toEqual(["rule.not"]); + }); + + it("refuses a regex in nested `has` and `inside` sub-rules", () => { + expect( + refused({ + rule: { + kind: "call_expression", + has: { + field: "function", + kind: "member_expression", + has: { regex: "^rpc$" }, + }, + inside: { regex: "^async" }, + }, + }) + ).toEqual(["rule.has.has", "rule.inside"]); + }); + + it("refuses a regex in `precedes`, `follows`, and `any`/`all` items", () => { + expect( + refused({ + rule: { + kind: "identifier", + precedes: { regex: "a" }, + follows: { regex: "b" }, + any: [{ kind: "x" }, { regex: "c" }], + all: [{ kind: "x" }, { regex: "d" }], + }, + }) + ).toEqual(["rule.precedes", "rule.follows", "rule.any.1", "rule.all.1"]); + }); + + it("reaches every other rule position the schema allows", () => { + expect( + refused({ + rule: { + kind: "identifier", + has: { kind: "x", stopBy: { regex: "a" } }, + nthChild: { position: 1, ofRule: { regex: "b" } }, + matches: { "util-call": { ARG: { regex: "c" } } }, + }, + fix: { template: "", expandEnd: { regex: "d" } }, + rewriters: [{ id: "rw", rule: { regex: "e" } }], + }) + ).toEqual([ + "rule.has.stopBy", + "rule.nthChild.ofRule", + "rule.matches.util-call.ARG", + "fix.expandEnd", + "rewriters.0.rule", + ]); + }); + + describe("kind supplied through a sibling `any` or `all`", () => { + it("accepts a sibling `any` whose every branch pins a kind", () => { + expect( + refused({ + rule: { pattern: "foo($X)" }, + constraints: { + X: { + any: [{ kind: "identifier" }, { kind: "property_identifier" }], + regex: "^rpc$", + }, + }, + }) + ).toEqual([]); + }); + + it("refuses a sibling `any` with a branch that pins no kind", () => { + expect( + refused({ + rule: { pattern: "foo($X)" }, + constraints: { + X: { + any: [{ kind: "identifier" }, { pattern: "$A.$B" }], + regex: "^rpc$", + }, + }, + }) + ).toEqual(["constraints.X"]); + }); + + it("refuses an empty sibling `any`", () => { + expect(refused({ rule: { any: [], regex: "^a$" } })).toEqual(["rule"]); + }); + + it("accepts a sibling `all` with one branch that pins a kind", () => { + expect( + refused({ + rule: { + all: [{ pattern: "$A" }, { kind: "identifier" }], + regex: "a", + }, + }) + ).toEqual([]); + }); + + it("accepts kinds pinned through nested `any`/`all`", () => { + expect( + refused({ + rule: { + any: [ + { kind: "identifier" }, + { all: [{ has: { kind: "x" } }, { kind: "member_expression" }] }, + { any: [{ kind: "a" }, { kind: "b" }] }, + ], + regex: "^rpc", + }, + }) + ).toEqual([]); + }); + + it("does not count a kind inside `not` or a relation", () => { + expect( + refused({ + rule: { + all: [{ not: { kind: "x" } }, { has: { kind: "y" } }], + regex: "a", + }, + }) + ).toEqual(["rule"]); + }); + }); +}); + +describe("astGrepRuleSchema", () => { + it("reports every unanchored regex as an attributed issue", () => { + const result = astGrepRuleSchema.safeParse({ + ...BASE, + rule: { kind: "identifier", has: { regex: "a" } }, + constraints: { A: { regex: "b" } }, + }); + expect(result.success).toBe(false); + const issues = result.error?.issues ?? []; + expect(issues.map((issue) => issue.path.join("."))).toEqual([ + "rule.has", + "constraints.A", + ]); + for (const issue of issues) { + expect(issue.code).toBe("custom"); + expect(issue.message).toMatch(/--debug-query=ast/); + expect(issue.code === "custom" && issue.params).toEqual({ + constraintId: "sg-regex-needs-kind", + }); + } + }); + + // zod skips a refinement attached to a schema that already failed, so the + // pairing must not hang off the upstream schema: a rule with an unrelated + // mistake would hide its regex problem for a whole extra round. + it("still reports the pairing when another field fails the schema", () => { + const result = astGrepRuleSchema.safeParse({ + ...BASE, + severity: 3, + rule: { regex: "a" }, + }); + expect(result.success).toBe(false); + const issues = result.error?.issues ?? []; + expect( + issues.map((issue) => [ + issue.path.join("."), + issue.code, + issue.code === "custom" ? issue.params : undefined, + ]) + ).toEqual([ + ["severity", "invalid_union", undefined], + ["rule", "custom", { constraintId: "sg-regex-needs-kind" }], + ]); + }); + + it("still reports the pairing when the rule is missing a required field", () => { + const { language: _language, ...withoutLanguage } = BASE; + const result = astGrepRuleSchema.safeParse({ + ...withoutLanguage, + rule: { regex: "a" }, + }); + const issues = result.error?.issues ?? []; + expect(issues.map((issue) => issue.path.join("."))).toEqual([ + "language", + "rule", + ]); + const last = issues.at(-1); + expect(last?.code === "custom" && last.params).toEqual({ + constraintId: "sg-regex-needs-kind", + }); + }); + + it("reports upstream schema failures unchanged when no regex is involved", () => { + const upstream = z + // eslint-disable-next-line @typescript-eslint/no-explicit-any, @typescript-eslint/no-unsafe-argument + .fromJSONSchema(upstreamSchema as any); + for (const input of [ + { ...BASE, severity: 3, rule: { kind: "identifier" } }, + { ...BASE, rule: { kind: 3 } }, + { id: "probe", rule: { kind: "identifier" } }, + ]) { + const result = astGrepRuleSchema.safeParse(input); + expect(result.success).toBe(false); + expect(result.error?.issues).toEqual( + upstream.safeParse(input).error?.issues + ); + } + }); + + it("refuses input that is not a rule object at all", () => { + for (const input of [undefined, null, "rule", []]) { + expect(astGrepRuleSchema.safeParse(input).success).toBe(false); + } + }); + + it("returns the rule unchanged when it passes", () => { + const rule = { ...BASE, rule: { kind: "identifier", regex: "^a$" } }; + expect(astGrepRuleSchema.parse(rule)).toEqual(rule); + }); + + it("accepts a regex anchored by a sibling `any` of kinds", () => { + expect( + astGrepRuleSchema.safeParse({ + ...BASE, + rule: { any: [{ kind: "a" }, { kind: "b" }], regex: "x" }, + }).success + ).toBe(true); + }); +}); + +describe("astGrepRuleJsonSchema", () => { + const definitions = astGrepRuleJsonSchema.$defs as Record< + string, + Record + >; + + it("states the pairing on every rule definition", () => { + for (const name of ["SerializableRule", "Relation"]) { + expect(definitions[name]?.if).toEqual({ required: ["regex"] }); + expect(definitions[name]?.then).toEqual({ + $ref: "#/$defs/TasklessPinsKind", + }); + } + expect(definitions.TasklessPinsKind?.description).toMatch( + /sg-regex-needs-kind/ + ); + }); + + it("changes nothing else about the upstream schema", () => { + const stripped = structuredClone(astGrepRuleJsonSchema) as { + $defs: Record>; + }; + delete stripped.$defs.TasklessPinsKind; + for (const name of ["SerializableRule", "Relation"]) { + delete stripped.$defs[name]?.if; + delete stripped.$defs[name]?.then; + } + expect(stripped).toEqual(upstreamSchema); + }); +}); diff --git a/packages/cli/test/ast-grep-vendor-contract.test.ts b/packages/cli/test/ast-grep-vendor-contract.test.ts index f2a59bc1..c3488215 100644 --- a/packages/cli/test/ast-grep-vendor-contract.test.ts +++ b/packages/cli/test/ast-grep-vendor-contract.test.ts @@ -26,7 +26,7 @@ import { * ast-grep's observable behaviour, pinned. * * Everything here is a property of a **vendored third-party binary**, exact- - * pinned at `0.45.3` in `packages/cli/package.json`. Our own tests assert that + * pinned at `0.51.1` in `packages/cli/package.json`. Our own tests assert that * our code behaves correctly *given* these; this file asserts the givens, so an * ast-grep bump that changes one fails here — naming the assumption and the * code that rests on it — instead of surfacing downstream. @@ -1216,6 +1216,86 @@ withSg("ast-grep vendor contract", () => { * same features showed no difference at all, because neither bug fires * unless a metavariable is bound in the specific position the fix changed. */ + /** + * THE REGEX/KIND PAIRING, MEASURED AT 0.51.1. + * + * `sg-regex-needs-kind` is a Taskless rule, and `create-sg-rule.md` says + * under Quirks how far ast-grep's own check goes: it refuses a bare regex at + * the top of `rule` and runs one anywhere else. The recipe also tells + * authors that a sibling `any:` of kinds anchors a constraint's regex, and + * that the method `$OBJ.$M()` captures is a `property_identifier`. Each of + * those is ast-grep's behavior, so each is pinned here. + */ + describe("regex and kind", () => { + const callSource = [ + "rpc();", + "client.rpc();", + "other();", + "client.rpcx();", + "", + ].join("\n"); + + it("refuses a bare regex at the top of `rule` as a config error", () => { + const outcome = grammarFindings(' regex: "^rpc"', callSource); + expect(outcome.status).toBe(8); + expect(outcome.stderr).toContain( + "Rule must specify a set of AST kinds to match" + ); + }); + + it("runs a bare regex in a sub-rule, against any node's text", () => { + // Nothing upstream refuses this shape, which is why the house rule + // reaches past the top of `rule`. `rpc$` matches the callee whatever + // its kind: an identifier in one call, a member_expression in the other. + expect( + grammarMatches( + [ + " kind: call_expression", + ' has: { field: function, regex: "rpc$" }', + ].join("\n"), + callSource + ) + ).toEqual(["rpc()", "client.rpc()"]); + }); + + it("anchors a constraint's regex with a sibling `any:` of kinds", () => { + // The recipe's example verbatim: one regex, two kinds. + expect( + grammarMatches( + [ + ' pattern: "$CALLEE()"', + "constraints:", + " CALLEE:", + " any:", + " - kind: identifier", + " - kind: member_expression", + String.raw` regex: "(^|\\.)rpc$"`, + ].join("\n"), + callSource + ) + ).toEqual(["rpc()", "client.rpc()"]); + }); + + it("captures the method in `$OBJ.$M()` as a property_identifier", () => { + const [asIdentifier, asProperty] = [ + "identifier", + "property_identifier", + ].map((kind) => + [ + ' pattern: "$OBJ.$M()"', + "constraints:", + " M:", + ` kind: ${kind}`, + ' regex: "^rpc$"', + ].join("\n") + ); + expect(grammarMatches(asIdentifier ?? "", callSource)).toEqual([]); + expect(grammarMatches(asProperty ?? "", callSource)).toEqual([ + "client.rpc()", + ]); + }); + }); + 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/constraints.test.ts b/packages/cli/test/constraints.test.ts index 418995ae..412c6986 100644 --- a/packages/cli/test/constraints.test.ts +++ b/packages/cli/test/constraints.test.ts @@ -191,7 +191,7 @@ const VIOLATIONS: Record = { rule: { ...VALID_RULE, rule: { regex: "^eval$" } }, fixture: VALID_FIXTURE, }), - names: /uses "regex" without a sibling "kind" field/, + names: /rule: Every "regex", at any depth, needs a "kind" beside it/, }, "sg-language-accepted": { run: () => diff --git a/packages/cli/test/verify.test.ts b/packages/cli/test/verify.test.ts index 8768fbfa..76b61912 100644 --- a/packages/cli/test/verify.test.ts +++ b/packages/cli/test/verify.test.ts @@ -286,12 +286,13 @@ describe("verifyRule", () => { const result = await verifyRule(temporaryDirectory, "regex-no-kind"); - expect(result.requirements.valid).toBe(false); - expect(result.requirements.errors).toContainEqual( - expect.stringContaining("regex") + // The pairing is part of the schema now, so Layer 1 reports it. + expect(result.schema.valid).toBe(false); + expect(result.schema.errors).toContainEqual( + expect.stringMatching(/^rule: Every "regex", at any depth/) ); - expect(result.requirements.errors).toContainEqual( - expect.stringContaining("kind") + expect(result.schema.violations).toContainEqual( + expect.objectContaining({ constraintId: "sg-regex-needs-kind" }) ); });