Repository navigation
fix(test): notice when an sg rule's globs reach no file - #487
Conversation
An ast-grep fixture is a pathless snippet, so a rule's globs never apply to it. A rule scoped to a directory that does not exist passed `test` while `check` never ran it. `test` now asks ast-grep, through `--inspect entity` over the same walk `check` performs, whether the rule applies to any file, and attaches a notice when it applies to none.
|
@claude /review |
|
Claude finished @theCodeDrift's task in 4m 31s —— View job Review: fix(test): notice when an sg rule's globs reach no fileReview mode: incremental — read 0 prior review thread(s) before reviewing.
SummaryThe approach is sound and well-isolated: asking ast-grep's own Two [New] findings posted inline, both performance-oriented, no correctness bugs found:
Smaller things noticed, not worth blocking on:
Changeset ( |
| /** | ||
| * Whether ast-grep applies the rule to at least one file in the project. | ||
| * | ||
| * Three answers, because a walk that did not finish is neither: `unknown` is | ||
| * what lets the caller stay silent rather than report an empty scope it never | ||
| * measured. | ||
| * | ||
| * Stops at the first file in scope: the answer is a yes/no, and the rest of the | ||
| * walk, including running the rule over every file, would only confirm it. | ||
| */ | ||
| function reachesAnyFile( |
There was a problem hiding this comment.
[New] Performance: one full-project walk per scoped rule, run sequentially for every rule in the test target.
describeEmptySgScope spawns a fresh whole-project sg scan -r <rule> --inspect entity for every rule that declares files:/ignores:. The worst case — the glob matches nothing — is also the case this feature exists to catch, and it's the one case where the walk can't stop early at "first file in scope": ast-grep has to visit every file in the project before --inspect entity can confirm zero matches.
Two things compound this:
runOverPathincommands/verify.ts(for (const rule of rules) { results.push(await run(cwd, rule)); }) runs rules sequentially, not in parallel, so N scoped rules means N full sequential walks.checkanswers "is this rule in scope" once per run, for every rule, via a single assembled-config scan. This feature re-derives the same answer per rule via its own dedicated walk, so a project with e.g. 30 ast-grep rules scoped to subdirectories-not-yet-written (explicitly called out in the PR description as a deliberate, expected pattern) pays 30 full-tree walks on everytaskless test, wherecheckpays one.
Not necessarily a blocker — the PR already made a deliberate "costs little in the common case" tradeoff — but worth confirming this was measured against a repo of realistic size/rule-count, since the expensive path is exactly the one the feature is meant to exercise.
There was a problem hiding this comment.
Measured rather than assumed. Declining.
On this repo (298 TypeScript files in the walk), one empty-scope walk takes ~65 ms, about the same as running a real rule over every file. taskless test .taskless/rules/sg over all 9 rules, 7 of them scoped, takes 0.34–0.39 s without the walk and 0.43–0.44 s with it, discounting one 0.63 s outlier. Every one of those globs reaches a file, so each walk stops at the first hit.
The full walk only happens when a glob matches nothing, and that run also prints a notice for every such rule. Thirty rules scoped ahead of their code means thirty notices to act on. The extra latency on top of that is not where the cost lands.
The walk can't be batched into one scan either. --inspect entity reports a per-file appliedRuleCount without naming the rules, so with more than one rule loaded the zero can't be attributed to a rule. check never answers "is this rule in scope". It only reports findings, and a rule with no findings looks exactly like a rule that never ran, which is the bug.
| notices: collectNotices([ | ||
| ...verification.notices, | ||
| await describeEmptySgScope(cwd, ruleId), | ||
| ]), |
There was a problem hiding this comment.
[New] Minor: these two awaits run sequentially where they could overlap.
await describeEmptySgScope(cwd, ruleId) (a full-project walk) completes before collectSgFixtureFindings(cwd, ruleId) (one scan --stdin per fixture snippet) even starts, since both are plain awaits evaluated in object-literal order. Running them via Promise.all would let the per-rule cost of this PR's new walk overlap with work test was already doing, rather than adding on top of it. Minor given the scan already dominates; flagging alongside the broader performance note on sg-scope.ts.
There was a problem hiding this comment.
Declining. On a scoped rule whose glob reaches a file, which is every scoped rule in this repo, the walk exits at the first hit and the whole feature adds ~50 ms across 7 rules. Overlapping it with the fixture scans would save part of that. That's not enough to justify reordering how the result object is built.
taskless testpassed an ast-grep rule whosefiles:glob matched nothing in the project, andcheckthen never ran it. Nothing said so. An sg fixture is a pathlessvalid:/invalid:snippet, so the globs never apply to it, and a mistyped glob looked exactly like a clean codebase.testnow asks ast-grep whether the rule applies to any file, and attaches a notice when it applies to none:How
sg scan -r <rule> --inspect entityreportsappliedRuleCountper file. That count folds in language,files:,ignores:,.gitignore, and the same walk flagscheckuses (sgWalkArgv([])). A second matcher would be a second opinion that could disagree withcheck.-r, not the assembled config, so a malformed sibling rule cannot make this rule's scope read as empty.Choices
files:orignores:are measured. A rule scoped bylanguage:alone, in a project with no code in that language, is honestly clean.testonly.verifystays cheap and never walks the project.Tests
Seven end-to-end cases in
verify-test-commands.test.ts. Each one was mutation-checked: I broke the behavior it guards and confirmed it went red.files:matches nothing → noticeignores:excludes everything → notice namingignores:.gitignore.taskless/→ noticecheck's.taskless/exclusionfiles:/ignores:gate is removedFixes #486