mirror of
https://github.com/abhigyanpatwari/GitNexus.git
synced 2026-09-10 22:43:40 +00:00
4 commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
Gergő Magyar
|
7468cc915b
|
fix(analyze): replace the hand-incremented schema version with a derived DDL fingerprint (#2798) (#2808)
* feat(schema): derive a fingerprint from the DDL this build creates `SCHEMA_FINGERPRINT` is a sha256 digest of the node and relation DDL that `runSchemaCreationQueries` actually executes, in the same shape as the existing `taintModelVersion` stamp (hex, sliced to 12). It exists because `INCREMENTAL_SCHEMA_VERSION` is hand-picked and has to *predict* whether an on-disk database matches this build's DDL. That number has collided with `main` eight times, twice exactly — and an exact clash is the quiet one, because the reuse gate is a strict `===`. `EMBEDDING_SCHEMA` is deliberately excluded: its `FLOAT[N]` width comes from `GITNEXUS_EMBEDDING_DIMS` at module load, so folding it in would make the digest a function of the environment rather than of code, and two runs of the same build under different env would thrash full rebuilds. Refs #2798 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * feat(storage): record the DDL fingerprint in RepoMeta `RepoMeta.schemaFingerprint` stores the digest of the DDL an index's tables were actually created from. It is the derived companion to `schemaVersion`, not its replacement: both are compared, and both must match. Absent means mismatch, deliberately. Grandfathering a missing fingerprint would let an incremental top-up stamp a fresh one onto a database whose DDL was never verified, permanently certifying exactly the wrong-shaped index the field exists to catch. The cost is one full rebuild per pre-existing index. The version ladder gains a note that its "re-check against origin/main before merge" ritual now only guards *semantic* bumps. v25, v26, v30, v31 and v34 all changed emitted ids, edges or wire formats while leaving the DDL byte-identical, and the fingerprint cannot see any of them — but DDL collisions no longer need renumbering. Refs #2798 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(analyze): gate index reuse on the DDL fingerprint, not just the version (#2798) `INCREMENTAL_SCHEMA_VERSION` is a hand-incremented integer that has to predict a derived fact: whether the on-disk DDL matches the code's DDL. It has collided with `main` eight times, and twice the collision was *exact*. An exact clash is the silent one. Two builds stamp the same number over different DDL, the `===` reuse gate reads the index as current, every `CREATE ... TABLE` is then skipped as "already exists" (suppressed in `runSchemaCreationQueries`), and the edges whose endpoint pair the live database cannot hold are dropped by `fallbackRelationshipInserts`' bare `catch`. The result is a wrong graph, with no error anywhere. Reuse now requires the version AND the DDL fingerprint to match, in both the pre-pipeline force-rebuild guard and the `isIncremental` predicate, and the fingerprint is stamped alongside the version at the end of a run. Both conditions are necessary. The fingerprint does not replace the integer: most entries in the version ladder change emitted ids, edges or wire formats while the DDL stays byte-identical, and a fingerprint-only gate would stop forcing rebuilds for all of them. What it does buy is that two branches picking the same number no longer need renumbering. The new branch sits above the `alreadyUpToDate` fast path for the same reason the version guard does — a clean tree at an unchanged commit would otherwise early-return before either check ran. Closes #2798 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(analyze): pin the DDL fingerprint gate and its two failure cases `schema-fingerprint.test.ts` pins the properties the gate rests on: the digest covers exactly the node and relation DDL that gets executed (recomputed from the exported lists, so adding a table or a FROM/TO pair without the fingerprint moving is impossible), it excludes the environment-derived embedding DDL, and it moves when any covered string moves. The two `incremental-orchestration` cases exercise the production path rather than modelling it: an index carrying the *current* version with a foreign fingerprint, and one with no fingerprint at all. Both were run against the pre-fix tree first and both failed there with `alreadyUpToDate === true` — the fast path swallowing the mismatch, which is the #2798 symptom exactly. `call-summary-schema-version.test.ts` widens its gate model to two equalities. The second argument defaults to the current fingerprint so all 33 existing version cases read unchanged, and a new case covers the collision, the legacy absence, and the semantic bump the fingerprint cannot see. Refs #2798 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(review-skill): point the schema-constant check at the fingerprint, not the deleted integer All four `gitnexus-review` SKILL.md mirrors told reviewers to verify `INCREMENTAL_SCHEMA_VERSION` "was bumped or regenerated". That constant no longer exists, so the instruction sent every future reviewer looking for something they could not find — and, worse, past its replacement. The check for graph DDL is now derived: `SCHEMA_FINGERPRINT` moves on its own, so the question is whether the diff changed a string in `NODE_SCHEMA_QUERIES` / `REL_SCHEMA_QUERIES`, and whether a newly added DDL array was folded into the fingerprint at all — the one way the derived gate can still be bypassed. What did NOT change is called out explicitly: the parse-store `SCHEMA_BUMP` and the bench fingerprint sets are still hand-maintained and still need the re-check-against-base ritual, and semantic changes that leave the DDL untouched fall outside the fingerprint entirely — those rely on the analyzer runner-identity receipt. Found by the review swarm's docs lane. The original plan for #2798 claimed no documentation mentioned the constant; that sweep covered five root docs and never looked at `.claude/skills/**` or the three mirrors. Refs #2798 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(migration): record the one-time rebuild the fingerprint switch costs Replacing `schemaVersion` with `schemaFingerprint` means every index written by an earlier GitNexus carries no fingerprint, reads as a mismatch, and is rebuilt once. That is deliberate — grandfathering absence would stamp a fresh fingerprint onto a database whose DDL was never verified — but until now it was undocumented, so a user's first post-upgrade analyze would announce a full re-analyze with nothing to explain it. MIGRATION.md already sets the precedent: PR #2363's meta.json → gitnexus.json rename was equally automatic and equally in need of an entry. This follows that shape, and is explicit about the parts that are easy to undersell: - the cost is per INDEX, and branch-scoped slots (#2106) each pay separately; on a large repository a full re-analyze is substantial, not a blip; - rollback is safe — an older binary sees no `schemaVersion` and forces its own rebuild, which is a cost, never a stale graph; - alternating between an old and a new binary rebuilds on every switch, because the end-of-run meta is written as a fresh literal so neither field survives the other's run. The retired ladder's per-version rationale is pointed at in git history rather than reproduced: `git show 561f913a3:.../repo-manager.ts`. That commit is an ancestor of origin/main, so the pointer survives this branch being squash-merged. Refs #2798 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(identity): cover workspace-linked packages in the analyzer dependency digest `dependencyNames` enumerated `dependencies`, `optionalDependencies` and `peerDependencies` only. `gitnexus-shared` is declared as a devDependency (`file:../gitnexus-shared`), and in a source-mode run the build root is the gitnexus package tree, which does not contain that sibling. So a change to gitnexus-shared moved neither `build.digest` nor `dependencyRuntime.digest`. That gap matters more since #2798 deleted `INCREMENTAL_SCHEMA_VERSION`. A DDL-affecting edit there is still caught by `SCHEMA_FINGERPRINT`, but a SEMANTIC-only edit — a new `REL_TYPES` member, say, where the relation table carries a bare `type STRING` column so no CREATE statement moves — was covered by nothing at all. Roughly thirty of the retired ladder's entries were exactly that change class, and the runner-identity receipt is what now carries them. Only checkout-local specifiers are added: `file:`, `link:`, `workspace:`, `portal:` and npm's bare local-path shorthands. Pulling in every devDependency was rejected — vitest, eslint and typescript would enter the digest and force a full re-analyze on unrelated tool bumps, which is worse than the hole. Scanning the linked sibling for the first time exposed a latent throw: `collectArtifacts` honoured `PRUNED_RUNTIME_DIRECTORIES` only for a real directory, so a SYMLINKED `node_modules` fell through to the payload branch and died with "Analyzer identity input is not a file". Worktree-style dev layouts and pnpm shared stores hit this immediately — verified in this worktree, where `gitnexus-shared/node_modules` is such a symlink. Pruning it loses nothing: packages beneath are still reached through `resolveDependencyPackageRoot`. Verified: a real `analyze` in this worktree succeeds with `packageCount` 259; editing the linked package's source moves the digest, bumping an installed registry devDependency does not, and removing the link moves it. `DEPENDENCY_RUNTIME_CANONICALIZATION` is deliberately not bumped — freshness compares digests, not the label, and the input-set change already moves them. Follow-up worth having: no fixture in the suite declares `devDependencies`, so this has no regression test yet. Refs #2798 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * refactor(analyze)!: delete INCREMENTAL_SCHEMA_VERSION, gate reuse on the DDL fingerprint alone The integer and its ~180-line version ladder are gone, along with `RepoMeta.schemaVersion`. Index reuse is now decided solely by `SCHEMA_FINGERPRINT`; a mismatch — including the absent stamp every pre-existing index carries — warns and forces a full re-analyze, which wipes and recreates the database so the tables are built from the current DDL. Deleting the integer is safe because it was already redundant: the runner-identity guard deep-compares the whole schema-v4 receipt, including a digest over the build tree, and forces a rebuild on ANY analyzer delta. Verified empirically — a comment-only edit to logger.ts, with the fingerprint byte identical, produced "runner identity changed ... forcing a full rebuild". The fingerprint is not thereby redundant. It fires where that guard cannot: a DDL-affecting change in `gitnexus-shared`, which is a workspace-linked devDependency and so sat outside both digests until the companion commit closed that gap. Review findings folded in, each correcting a line this rewrite itself introduced and never published: - B1: two assertions matched a log string the rewrite had renamed; both tests failed. They now assert what production emits. - B2: the pre-existing downgrade test perturbed `schemaVersion: 7`, a field this change deletes, so the spread carried a valid fingerprint, every guard passed, and the run legitimately took the fast path. It perturbs the fingerprint now, restoring the only integration coverage of the gate-above-the-fast-path ordering invariant. - N5: duplicate `schemaFingerprint` keys silently collapsed two assertions into one (TS1117). - N6: the absent-stamp message told non-git repositories their index was "built by an older GitNexus version" — on every run, about an index this exact build had just written. Non-git repos never record a fingerprint, and now the message says so. - N9: the on-disk stamp is shape-checked before being echoed, so a crafted gitnexus.json cannot push ANSI escapes through the CLI log. - N7: a test case that re-computed the same digest expression with its operands swapped, mislabelled as a randomness check on a module-level const. - N10: comments claiming the digest "cannot collide" (it is 48 bits), pointing at a vector-column gate that does not exist, and asserting storage/ is free of a core/ dependency two lines below a core/ value import. None of these were caught by `tsc -p tsconfig.json`, which covers src only, nor by eslint, where no-dupe-keys is off. `tsconfig.test.json` reports all three test defects and is not currently wired into CI. Refs #2798 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(schema): pin that the fingerprint covers every DDL statement init executes `SCHEMA_QUERIES` is the list `runSchemaCreationQueries` iterates — the DDL that actually runs. The fingerprint hashes only two of its three members, and until now no test imported `SCHEMA_QUERIES` at all, so nothing tied the two together. A fourth member appended to that array — the one literally named for what init executes — would have been invisible to the gate. Every existing test would still pass, because they all recompute the digest from the same two arrays the fingerprint already uses. An index whose gate passed would then run `initLbug` over the old database, where `runSchemaCreationQueries` suppresses "already exists", so the new table would never be created and its edges would be dropped by `fallbackRelationshipInserts`' bare catch. A wrong graph, no error — exactly the failure #2798 exists to end. The check is a pure predicate over (executed, fingerprinted, documented exclusions) rather than a positional `toEqual`, so `EMBEDDING_SCHEMA` is named as an exclusion with its reason — its FLOAT[N] width is environment-derived — rather than sitting in a list where a future reader might "fix" it by folding it in. It asserts both directions and is order-insensitive, leaving ordering to the digest assertion that already pins it. The negative case is pinned in CI rather than checked by hand once: the same predicate over a synthetic fourth member must report it. If a refactor ever makes the predicate vacuous, that case fails even though the positive one would not. Refs #2798 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(analyze): name the invariant the version deletion now rests on Deleting `INCREMENTAL_SCHEMA_VERSION` moved a load-bearing guarantee into an implicit one. Roughly thirty of the retired ladder's entries changed no DDL at all — node ids, wire formats, resolution tiers — and the fingerprint is structurally incapable of firing on any of them. Their only remaining cover is the analyzer runner-identity receipt, and nothing in the suite said so. This adds a table over the real `analyzerRunnerIdentitiesEqual` with a well-formed schema-v4 receipt: byte-identical reuses; an entrypoint-only difference reuses (CLI vs analyze worker); a moved build digest with unchanged DDL forces — that case IS the invariant, commented as such; and a dependency change, an ABI change, undefined, null, a schema-v3 legacy receipt, a missing build section and a non-sha256 digest all fail closed. The deleted `expect(INCREMENTAL_SCHEMA_VERSION).toBe(35)` pin is also worth naming: it failed CI on every bump by design, which is what made an author stop and think. Nothing replaced it. This does not restore that — a digest has no literal to pin — but it does make the mechanism that took over the job visible to the next person who reads the file. Refs #2798 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(spring): pin CLASS_SCHEMA's membership in the fingerprinted DDL set When `INCREMENTAL_SCHEMA_VERSION` went away, its sibling in basicblock-callee-ids-schema.test.ts got a replacement assertion tying BASICBLOCK_SCHEMA to the fingerprint's input set. This file's `>= 23` floor was deleted with nothing put in its place. The file still asserts CLASS_SCHEMA's CONTENT — that the `frameworkAnnotations` column exists — but not that CLASS_SCHEMA is part of what the digest covers, and the second is what makes an index built before that column carry a different fingerprint and get rebuilt. Mirrors the sibling so the two read the same way. Refs #2798 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(identity): stop a symlinked directory from aborting the whole analyze `collectArtifacts` fused two orthogonal facts into one condition: that four directory names never carry runtime payload, and that a symlink where a real directory was assumed falls through to the payload branch, where `snapshotReadableFile` stats the target, sees a directory, and throws "Analyzer identity input is not a file". The second was only fixed for those four names. Every other symlinked directory in a scanned package root still aborted the run — `dist -> build`, a vendored grammar link, anything inside a linked sibling checkout. Newly reachable, because making workspace-linked packages scannable pointed the scanner at a live checkout instead of an immutable registry tarball for the first time. Split along the actual seam: prune on the NAME alone, and give symlinks their own branch in the type dispatch, ahead of the payload branch. Link text is recorded rather than followed. Following was rejected on three grounds, each checked in source: the traversal is a stack with no visited set, so a self-referential link would recurse to `runtimeDepth` — which throws, trading one hard abort for another; `snapshotDirectory` rejects a symlink outright, so the directory guard could not accept one without a realpath rewrite of its canonical-path identity; and a link into an already-scanned tree double-counts against `runtimeEntries`/`runtimeBytes`, which also throw. The cost is stated in code: a link out of the package contributes its text, not its target's content. Links resolving to a regular file keep the existing content digest. The new `'unfollowed-symlink'` kind is threaded through every consumer, including the cache validator — which re-probes with `mode: 'link'`, since the readable-file probe resolves the target and would return null for exactly this kind, silently failing every warm validation. No canonicalization or cache-schema bump. Digest content changes only for trees that previously crashed: a delta scan over all 258 scanned roots of this install found no regular file bearing a pruned name and no symlink failing to resolve to a file, so `dependencyRuntime.digest` is byte-identical here. Six of the eight new tests fail against the unfixed tree with the exact production error; all eight pass after. Refs #2798 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * refactor(analyze): give the reuse gate a real seam and sanitize logs at the funnel Cleanup pass over the #2798 branch. Net -183 lines. The gate had no extracted predicate, so its own test asserted it by regex-matching run-analyze.ts SOURCE TEXT. That pinned production formatting: one pattern froze three back-to-back single-name imports from './lbug/schema.js', so merging them — the obvious tidy-up — failed a test named "still imports the DDL digest itself". `schemaFingerprintMismatch` and `isSchemaFingerprintShaped` now live in core/lbug/schema.ts beside the constant. Not in run-analyze.ts next to `pdgModeMismatch`, because storage/ must stay off the analyze pipeline and mcp/resources.ts is a plausible second consumer — the same reasoning that puts `cjkSegmentationModeMismatch` in core/search/. The regex block is gone; the test calls the predicate. The three imports are merged. ANSI sanitation moved from one field to the funnel. The per-field guard's own comment stated the general hazard — gitnexus.json is parsed with no runtime shape validation and the notice reaches console.log — while two sibling guards twelve lines away echoed `runnerIdentity.schemaVersion` and `cjkSegmentation` from that same file raw into the same log. `log()` now strips C0/C1 controls, covering all seven guard messages and any written later. Also: - Deleted a duplicate integration test. After the downgrade test was repointed at `schemaFingerprint` it became the same scenario as the new one, differing only by an extra log assertion — which is now folded into the survivor. Saves a fixture and two full pipeline runs per CI pass. - Replaced a 3-parameter set-difference helper with one set equality. Its doc was false at one call site (arguments semantically swapped) and it needed a fourth test purely to prove itself non-vacuous; set equality cannot go vacuous. - Removed ~115 lines of runner-identity table that duplicated analyzer-identity.test.ts. The three genuinely uncovered cases moved there, and the #2798 invariant — build digest moved while the DDL did not — now asserts against a REAL analyzer-build-tree edit rather than a hand-built literal, which is strictly stronger than what it replaces. - MIGRATION.md quoted a log line the code cannot emit; it was written before the placeholder changed. - Restored the rationale on the `capabilities` docstring, which a previous pass replaced with its consequence — leaving a maintainer reading "duplicated by hand" as a wart to fix by importing, which is what the original forbade. - Marked the `isIncremental` conjunct as belt-and-braces: `!options.force` short-circuits before it, so it cannot decide anything. Refs #2798 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * feat(analyze): force a rebuild when the vector column width changes `CodeEmbedding.embedding` is declared `FLOAT[EMBEDDING_DIMS]`, resolved from `GITNEXUS_EMBEDDING_DIMS` at module load. Nothing gated it. Flip the variable on a same-commit clean tree and no guard fired at all: `alreadyUpToDate` returned over a `FLOAT[384]` table while the process embedded at 768. The only reaction anywhere discards the embedding CACHE and re-embeds — into a column whose type it never revisits. This predates #2798; `INCREMENTAL_SCHEMA_VERSION` never covered dims either. It surfaced because the fingerprint work had to reason about why `EMBEDDING_SCHEMA` must stay OUT of the digest: its width is environment-derived, so folding it in would make the same build disagree with itself and thrash rebuilds. That exclusion is correct, and it leaves the width needing its own guard. Modelled on `cjkSegmentation`, the closest sibling: an env-resolved scalar stamped at write time and compared by a small exported predicate that forces on mismatch. `embeddingDimsMismatch` sits in core/lbug/schema.ts beside `EMBEDDING_DIMS`, so the query side can adopt it without importing the analyze pipeline — mcp/local/local-backend.ts already warns on a cjkSegmentation disagreement and has the identical claim here, since the query path embeds at the live width against a table of unknown width with no validation at all today. ABSENCE IS NOT A MISMATCH, deliberately. Forcing on it would be dead code: `embeddingDims` and `schemaFingerprint` ship together, and a missing fingerprint already forces exactly one rebuild — which is where this stamp lands. Absence also carries no signal here, unlike the fingerprint: a missing fingerprint means "DDL this build cannot vouch for" and ships WITH a DDL change, whereas a missing dims stamp means only "written before the field existed", and that run's table agreed with that run's width. Drift requires the env to change, which absence says nothing about. The `cjkSegmentation` trick of folding absence into the default was unavailable — there is no width that is safe to assume for an existing table — so the stamp is instead written unconditionally, giving absence exactly one meaning. Malformed values are not grandfathered: null, '384', NaN and objects all read as a mismatch and err toward a rebuild. Refs #2798 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * feat(mcp): warn when the served index's vector width differs from the query embedder's The analyze side now forces a rebuild when the vector column width changes. The query side had no equivalent: a serving process embeds a query at its own width and searches a table whose width was fixed when the index was built. Disagree and the user gets wrong or missing semantic results with nothing explaining why. Mirrors the cjkSegmentation drift warning immediately above it — same warnings[] array, same per-query recomputation, agent-visible in the tool response, and it warns rather than refuses. A width mismatch degrades the semantic lane only; keyword results are unaffected, so `partial` is deliberately not set. Compares against `getEmbeddingDims()` — the width the query embedder actually produces — NOT schema.ts's `EMBEDDING_DIMS`. The two diverge exactly when GITNEXUS_EMBEDDING_DIMS is set on a server that embeds LOCALLY: the query path ignores that variable and embeds at 384, so comparing against the env-derived constant would report drift on a lane that works fine. The recorded width is what the vector CAST actually binds. `embeddingDimsMismatch` is imported from core/lbug/schema.js rather than restated, so "absent is not a mismatch" cannot drift between the analyze and query sides. That predicate was placed in schema.ts precisely so this consumer could reach it without importing the analyze pipeline. Two gates keep it quiet when it would be noise: it fires only for a repo where this process actually produced a query vector, so an index analyzed without --embeddings (or a server whose embedder is unavailable) never carries it. An untrusted recorded value — meta.json is schema-less JSON — is reported as "an unrecognized width" rather than echoed. `loadMeta` is hoisted out of the neighbouring try so both diagnostics share one read and an invalid GITNEXUS_FTS_CJK_SEGMENTATION cannot take this one down with it. Refs #2798 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(identity): detect an npm-linked dev dependency the specifier cannot see `isLocallyLinkedSpecifier` admits a devDependency whose SPECIFIER is checkout-local. `npm link <pkg>` leaves the specifier a registry range while the node_modules entry symlinks to a checkout — locally linked, invisible to a specifier check, so a semantic-only edit there still moves neither digest. The obvious placement is unaffordable, measured rather than assumed: probing every dev-only name inside collectRuntimePackages costs 1998 resolutions, not the ~8 it looks like, because dependencyNames runs for every package in the BFS and published tarballs retain their devDependencies. Persisted path guards go 2221 -> 11050 (+398%), and every guard is re-probed on each warm validation — the path `status` takes. Scoped to the root package instead. The declared-intent half is untouched and still enumerated everywhere: it alone can emit the `<missing>` edge for a declared link whose checkout is absent, where resolution returns null and cannot distinguish that from an uninstalled dev tool. The new resolved-location half runs only when `parent.root === packageRoot`, resolves through the existing resolver so its path guards are recorded, and admits a name iff the realpath'd root carries no node_modules segment. Bounded against mis-fire by EXPANSION. "Not under node_modules" is a proxy for "checkout-local"; under a relocated pnpm virtual store every dev dep passes it and the whole dev tree folds into the receipt — against limits that THROW, so a legitimate install would abort. Measured here: uncapped, that shape takes 259 -> 347 packages and 2250 -> 3786 guards. The cap admits at most four and DROPS THE WHOLE CHANNEL on overflow rather than an arbitrary prefix, because the abort comes from the transitive payload of whichever trees get folded in — four of a mis-fired thirteen is still unbounded, and a sorted-prefix receipt would be arbitrary. Overflow falls back to the specifier-only receipt that ships today. Cost on this install: 259 packages unchanged, 13 dev names resolved, guards 2221 -> 2250 (+29, +1.3%). Verified against the real implementation, not just a replay: validation guards 16295 -> 16324, packageCount and artifactCount unchanged, and `dependencyRuntime.digest` byte-identical — so this forces no re-analysis for anyone. Each test fails on the defect it targets: disabling the channel kills the npm-link and cap cases; dropping the root-only scope makes the differential guard-count case fail at 2.8x guards; removing the specifier half kills the `<missing>` case. Refs #2798 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Gergo Magyar <gergomagyar0@gmail.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
azizur100389
|
bee3e82ab2
|
test(scope-resolution): guard closure identity invariants (#2748) | ||
|
Gergő Magyar
|
0ce7880290
|
fix(scope-resolution): a closure binding is a call SOURCE in every language, and function-local values carry their own identity (closes #2699) (#2718)
* test(scope-resolution): audit the consumers of file-scoped node ids (#2699 part A)
#2699 item 4 — "audit consumers that assume file-scoped ids" — after #2695/#2714
gave function-local CALLABLES position-bearing ids. Tests and findings only; no
production change. That split is deliberate: `impact` reports
`resolveDefGraphId` at CRITICAL with 23 DIRECT dependents across 7 modules
(every language MRO builder, both Spring attachers, C++ member lookup,
tryEmitEdge, emitReferencesViaLookup, buildGraphTargetIndex, emitFreeCallFallback,
emitReceiverBoundCalls, preEmitInheritanceEdges, emitDetectedInterfaceImplementations,
phpEmitUnresolvedReceiverEdges, emitRubyMixinEdges, emitRustTraitImplEdges,
emitDartHeritageEdges), so changing that key chain is its own change, not a
rider on an audit.
A2 — detect_changes: CONCERN RESOLVED, now pinned. The worry was that an id
containing `@row:col` re-keys whenever a declaration MOVES, making every edit
look like symbol churn. It cannot: `local-backend.ts` maps diff hunks to
symbols by LINE-RANGE OVERLAP (`n.startLine`/`n.endLine`) and merely REPORTS
`n.id`. Node identity never participates in the match. New structural test
asserts the WHERE clause never gains `n.id =` or `n.id IN`, keeps the one
legitimate id-shaped predicate (the `BasicBlock:` prefix exclusion, #2082 U7),
and confirms the id is returned rather than matched. Structural in the same
idiom as `detect-changes-worktree.test.ts`, and labelled as not proving runtime
behaviour.
A1 — ANSWERED, and the answer is that #2699 is NOT fully closed by items 1-3.
The fail-closed guard is gated on `isOverloadableCallable`
(Function | Method | Constructor), so a function-local VALUE never reaches it.
Measured on a fixture: a top-level `const handler` and a function-local
`const handler` still produce ONE node, `Const:v.ts:handler`. That is the
residual half of the issue's original complaint. Pinned as a KNOWN LIMIT with
its reason (widening identity to values re-keys ~14,700 build-time nodes to
change ~800 persisted ones — the decision recorded in `parse-worker.ts`), and
deliberately NOT fixed here.
A3 — id-persisting consumers, classified:
- detect_changes ................ SAFE (position-keyed; pinned by A2)
- MCP impact/context/trace ...... SAFE (resolve by name/uid at query time)
- bench fingerprints ............ SAFE (digest capture shape, not node ids)
- rust-captures golden .......... SAFE (digests captures, not ids)
- cfg pipeline-pdg snapshot ..... AT RISK by design — pins exact edge ids, so
it trips whenever attribution changes. That is the gate working; #2714
already exercised it.
- wiki / group-contract links ... NOT id-keyed on locals (locals are never
cross-file addressable, per the document-scoped contract of item 2).
Verified: tsc clean; 14/14 across the two touched files; `detect_changes`
reports 0 changed symbols (tests only).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0184RmD24KFJidYqpM7v3XjR
* fix(php): a closure binding is a call SOURCE, not only a TARGET (#2699 part B, S1)
A call made inside a closure binding was attributed to the ENCLOSING scope, so
the closure was a call TARGET but never a call SOURCE: impact(handler,
direction:"downstream") reported nothing even though the closure calls out.
Root cause, probe-measured rather than inferred. Instrumenting
pickCallerCallableDef (graph-bridge/ids.ts) to log every rejection reason shows
the closure's own scope EXISTS and its range DOES contain the call site, but its
ownedDefs is EMPTY, so the ":94" owned-callable filter drops it and attribution
falls through to the ":97" enclosing-scope fallback.
The reason is one missing query rule. javascript/query.ts pairs the binding name
with the closure via @declaration.function anchored on the INNER arrow node, so
anchor.range equals the @scope.function range and pass2AttachDeclarations
attaches the declaration to the CLOSURE's scope. No other language had that
rule — PHP, Rust, Kotlin, Ruby and Dart all captured named function
declarations only. That single omission is the entire empty-ownedDefs cause.
This ports the rule to PHP with the same anchor discipline (@declaration.function
on the inner anonymous_function / arrow_function, NOT on the
assignment_expression wrapper). PHP needs nothing else: it already declares
(anonymous_function) and (arrow_function) as @scope.function, so the rule alone
completes it.
Measured on a fixture: `$handler = function ($x) { return target($x); }` inside
outer() now emits
Function:src/a.php:outer.$handler@3:2 -> Function:src/a.php:target
where it previously emitted `outer -> target`.
The pinned test in closure-binding-labels.test.ts asserted the OLD, wrong
behaviour by design ("to catch that asymmetry changing in EITHER direction"), so
it is INVERTED here rather than deleted, per its own instruction. Its block
comment is corrected to record the measured root cause, including that Kotlin
and Ruby will need BOTH this rule AND a relaxed kind gate (their lambda_literal
/ do_block is @scope.block deliberately, #1757), and that Dart has no closure
scope at all.
Verification: closure-binding-labels 50/50; PHP resolver suites 221/221
(php, php-coverage, php-response-shapes). detect_changes {staged}: 1 changed
symbol (PHP_SCOPE_QUERY), 0 affected processes, risk LOW.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0184RmD24KFJidYqpM7v3XjR
* fix(rust): emit a node for a closure binding and make it a call SOURCE (#2699 part B, S3)
Rust was the one exception to #2687's "a closure bound to a name is a Function
node in every language": `let handler = || target(1);` produced NO graph node at
all, so the closure could be neither a call target nor a call source.
Needed BOTH query channels, which is the finding worth recording. Porting only
the scope-resolution rule (as S1 did for PHP) changed nothing measurable here,
because there was no node to attribute anything to:
- languages/rust/query.ts — closure-binding declaration, @declaration.function
on the INNER closure_expression so anchor.range aligns with the existing
(closure_expression) @scope.function. This is what gives the closure's own
scope a callable in ownedDefs, which is what stops pickCallerCallableDef
falling through to the enclosing fn.
- tree-sitter-queries.ts — @definition.function on the OUTER let_declaration.
This emits the Function NODE that Rust never had.
Note the deliberate anchor asymmetry between the two channels: the graph-node
channel anchors the WRAPPER (matching the existing
(lexical_declaration (variable_declarator ... (arrow_function))) rule), while
the scope-resolution channel anchors the INNER closure (to align with
@scope.function). Getting these backwards silently produces either no node or
an unattributable one, so both sites carry a comment saying so.
Measured on a fixture — `let handler = || target(1);` inside outer():
Function:src/a.rs:outer CALLS Function:src/a.rs:outer.handler@2:4
Function:src/a.rs:outer.handler@2:4 CALLS Function:src/a.rs:target
Previously the whole binding was absent and the call read as `outer -> target`.
The rule also covers `move` closures: the closure_expression node spans the
`move` keyword.
Verification: closure-binding-labels 50/50; rust.test.ts 192/192;
rust-coverage, rust-f70, rust-scope all pass; rust-captures-golden passes
UNCHANGED, so no golden regeneration was required. detect_changes {staged}:
2 changed symbols (RUST_SCOPE_QUERY, RUST_QUERIES), 0 affected processes,
risk LOW.
One caveat on the suite runs: this host times out `beforeAll` hooks at the
default 60s under load — rust.test.ts needed --hookTimeout=600000 to complete,
and a concurrent second vitest run starves worker startup entirely (every test
fails at ~5001ms). Both are host artifacts, not signal.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0184RmD24KFJidYqpM7v3XjR
* fix(kotlin,ruby): a closure binding is a call SOURCE, via a Block-scope callable boundary (#2699 part B, S2)
Kotlin and Ruby anchor a closure on a Block-kind scope — Kotlin lambda_literal
and Ruby do_block/block are @scope.block DELIBERATELY (#1757 smart casts), so
they must not be re-kinded. pickCallerCallableDef gated its child-scope walk on
kind === 'Function', so a closure there could never become a call SOURCE.
Both halves are required; neither alone changes anything:
1. kotlin/query.ts and ruby/query.ts gain the closure-binding declaration rule,
with @declaration.function on the INNER lambda_literal / block so its range
aligns with the @scope.block range (the anchor discipline documented in
javascript/query.ts). Without this the closure scope owns no callable def.
2. pickCallerCallableDef accepts a Block-kind child as a callable boundary when
the scope IS that callable's body. Without this the kind gate still rejects.
The alignment test in (2) is the part worth scrutiny. Relaxing the kind gate to
accept ANY Block owning a callable would be a real regression: a nested
`fun foo()` declared inside a block is owned by that block, so a call made at
BLOCK level — outside foo — would be misattributed to foo. Comparing the def's
declaration position against the scope's start position discriminates them: for
a closure the declaration and the scope sit on the SAME node, so the positions
match; for a nested function the block starts at `{` while the def starts at the
declaration, so they do not. Existing Function-kind behaviour is untouched, so
every already-working language is unaffected by construction.
The comparison is base-safe: scope-extractor.ts builds a def id as
`def:<filePath>#<startLine>:<startCol>:<type>:<name>` from the same Range a
scope carries, so both sides share one coordinate base. This is called out in
the helper's docblock because `defStartLine` nearby documents its own output as
1-based, which invites a wrong "fix" (#2377 is exactly this class of hazard).
Ruby's call forms are restricted to lambda/proc by name: an unrestricted
(call block: (block)) would match ANY method call taking a block, so
`mapped = items.map { |i| ... }` would wrongly declare `mapped` a callable.
Verified against the parser: 3 matches (->, lambda, proc), map excluded.
Separate #eq? patterns rather than one #match? alternation, which is a known
hazard on this tree-sitter line.
Measured on fixtures:
Kotlin Function:src/A.kt:outer.handler@2:4 CALLS Function:src/A.kt:target
Ruby Function:src/a.rb:outer.handler@4:2 CALLS Method:src/a.rb:target#1
previously `outer -> target` and `outer#0 -> target#1`.
The pinned Kotlin test asserted the old behaviour by design and is INVERTED, not
deleted. Ruby had NO pinned case, so a new one is added rather than inverted.
The describe title no longer claimed something false ("not yet a call SOURCE"
now holds only for Dart) and was retitled.
Verification: closure-binding-labels 51/51; kotlin.test.ts, kotlin-coverage,
ruby.test.ts, ruby-scope, ruby-namespaced all pass (478 passed / 1 expected
inversion before the test was flipped). impact on pickCallerCallableDef:
CRITICAL, 191 impacted, ONE d=1 (resolveCallerGraphId) — the return contract is
unchanged, so that dependent is unaffected. detect_changes {staged}: 5 changed
symbols, 2 affected processes (both EmitReferencesViaLookup, one of them the new
ScopeIsCallableBody step), risk medium.
Dart remains the last failing language: dart/query.ts declares no
@scope.function at all, and dart/captures.ts synthesizes one only from a
declaration WITH a body node, which an expression-bodied closure lacks.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0184RmD24KFJidYqpM7v3XjR
* fix(dart): give a closure binding a scope and a distinct identity (#2699 part B, S4)
Dart was the last language where a closure binding could not be a call SOURCE,
and fixing only that would have made the graph WORSE, not better. This lands
both halves together for that reason.
## The attribution half
A Dart closure had no scope at all. `dart/query.ts` declares no
@scope.function anywhere — Dart's function scopes are SYNTHESIZED in
`dart/captures.ts` from `declNode` + `findFunctionBody(declNode)`, and
`findFunctionBody` looked only at the next named SIBLING for a `function_body`.
A closure literal carries its body as a CHILD (`function_expression_body`), so
it matched nothing and no scope was produced.
`query.ts` gains the closure-binding declaration rule and `findFunctionBody`
understands the child form. Deliberately NO @scope.function is added to the
query: it would collide at identical range with the synthesized one, and
duplicate scope ids make `buildScopeTree` throw, which DROPS THE WHOLE FILE.
## The identity half, and why it is not optional
With attribution alone, two same-named closures in one file both keyed to the
bare `Function:a.dart:handler`. One node then appeared to call BOTH targets —
a CALLS edge present nowhere in the source. That is worse than the missing edge
it replaced, so S4 could not ship without this.
Root cause is not Dart-specific. `enclosingCallablePrefix` derives a SEMANTIC
relation — what encloses this callable — by SYNTACTIC ancestor walk. Dart parses
`int outer() { … }` as `function_signature` followed by `function_body` as
SIBLINGS, so the enclosing callable is never an ancestor of code inside it and
no membership set can fix that; the walk looks in the wrong direction.
This is what SCIP and real compilers avoid by construction. SCIP keeps a local
symbol opaque (`local <id>` — no name, no position, no chain) and models
containment as a SEPARATE `enclosing_symbol` field; its spec says the local/global
choice should follow ACCESSIBILITY, not the ability to name an enclosure. Dart's
own analyzer answers this from `Element.enclosingElement` in the element model,
never from AST ancestry. clang uses `name@offset` for a function-local; Kythe
uses a document-scoped VName plus a `childof` edge. Identity is positional and
opaque; enclosure is a relation.
`findSplitBodyCallableAncestor` is the narrow fix at that seam: a fallback used
ONLY when the ancestor walk finds nothing, recovering the callable from the
body's preceding sibling.
The sibling must be a BARE SIGNATURE, and that restriction is load-bearing —
"any preceding callable sibling" is WRONG and was caught regressing PHP during
this work. In `<?php function target($x) {…} $handler = function ($x) {…};` the
closure is at FILE level, so the ancestor walk correctly finds nothing, the
fallback runs, and an unrestricted version mis-qualified the file-level
`$handler` as `target.$handler`. A preceding sibling is only an ENCLOSING
callable when it cannot hold its own body.
`SPLIT_SIGNATURE_NODE_TYPES` is exactly that set and is DERIVED, not listed:
`LOCAL_SCOPE_BODY_NODE_TYPES` is already `FUNCTION_NODE_TYPES` minus the bare
signature types, so the difference between them IS the split-signature set
(`function_signature`, `method_signature` — verified at runtime). PHP's
`function_definition` carries a body and is in both, so it is excluded. No
language is named in shared code, and any future split-grammar language is
covered for free.
## Verification
Full resolver sweep — the gate that caught #2714's Rust regression — 2926
passed / 1 skipped / 0 failed across 51 files. closure-binding-labels 52/52;
dart.test.ts, dart-coverage, callable-id-lockstep, function-local-identity,
caller-identity-regression all pass (156/156 across 6 files).
impact on `enclosingCallablePrefix`: LOW, 5 impacted, 3 d=1 all inside
parse-worker. detect_changes {staged}: 5 changed symbols, 0 affected processes,
risk LOW.
Three existing Dart expectations FLIPPED rather than being deleted: Dart locals
now carry the same enclosing-callable + position identity every other language
got in #2695, so `local.dart:handler` became `local.dart:caller.handler@1:2`.
A new test pins the actual defect — two same-named closures staying DISTINCT
nodes — because the qualification assertions alone would not fail if the
fabricated edge returned.
One note for future work: an id-shape assertion here carries a call-site suffix
on indirect invocations (`…handler@3:2:5:9`) but not on direct calls. That is
the callable-value-flow pass keying its edge by invocation position, not part of
the node id.
Part B is now complete: PHP (S1), Rust (S3), Kotlin + Ruby (S2), Dart (S4).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0184RmD24KFJidYqpM7v3XjR
* fix(scope-resolution): close every deferred item on #2699 (A1 values, twin-list guard, schema bumps)
Clears the limitations this PR had been carrying rather than leaving them as
follow-ups.
## A1 — function-local VALUES now carry their own identity
This was #2699's ORIGINAL complaint and the one a callable-only gate could never
reach: a top-level `const handler` and a function-local `const handler`
collapsed onto ONE `Const:v.ts:handler`. #2695 restricted position-qualified
identity to Function|Method|Constructor because the collision that produced
wrong CALLS edges was between callables, and widening churned ids for symbols
the pruner mostly deletes. The churn is real and is accepted here deliberately.
Widening needed THREE gates aligned, not one:
- id-building — `parse-worker.ts` nestedCallablePrefix
- resolution — `ids.ts` position key
- registration — `node-lookup.ts` position-key registration
Missing the third would register no position key for values, so every lookup
misses and falls through silently. That is the #2714 failure mode: the caller
attaches to a node that does not exist and the edge is DROPPED, which looks like
"zero dangling edges" from outside. All three now route through ONE predicate,
`isPositionQualifiedLocalLabel`, rather than repeating the label set a third
time.
Only LOCALS move. The prefix comes from `enclosingCallablePrefix`, which returns
undefined when nothing encloses the declaration, so top-level and class-member
ids are untouched — verified by the full resolver sweep, where a leak onto class
members would have broken assertions in every language. `Property` is included
on purpose: a class field stays unqualified because the prefix walk boundaries
on class-likes, while an object-literal property inside a function is genuinely
local and would otherwise keep the old collision.
Measured: `Const:v.ts:handler` + `Const:v.ts:run.handler@3:2`, two distinct
nodes. The KNOWN LIMIT test is FLIPPED per its own former instruction ("this
test should be updated as part of it rather than deleted").
## Schema bumps — required by Part B, not just by A1
INCREMENTAL_SCHEMA_VERSION 20 -> 21, parse-cache SCHEMA_BUMP 27 -> 29.
SCHEMA_BUMP is 29, not 28, and that is the point of re-checking it against
origin/main at MERGE time rather than branch time. This branch cut at 27 and
bumped to 28; #2415 also bumped 27 -> 28 and merged first. The automated
main-merge onto this branch surfaced the collision — leaving it at 28 would have
shipped this whole change with NO parse-cache invalidation, so every warm cache
keeps replaying the pre-fix captures and ids. This is the third instance of that
collision recorded in parse-cache.ts (#2632/#2653 hit it at v21, and
#2653/#2654 hit INCREMENTAL_SCHEMA_VERSION the same way).
Part B already changed emitted node ids AND edges on files that did not
themselves change (Dart locals re-keyed, Rust gained a node it never emitted,
five languages gained closure-source attribution). A v20 index topped up
incrementally keeps serving the old attribution, and a warm parse cache replays
the old captures and ids verbatim. Shipping S1-S4 without these would have let
every existing index silently keep the pre-fix graph.
## Twin-list drift guard — the sixth instance in this family
`IMPLICIT_RECEIVERS` (gitnexus-shared lookup-core.ts) and `THIS_RECEIVERS`
(type-env.ts) spell the same concept in two packages, and nothing enforced
agreement — `$this` was added to the shared list in #2714 only because it was
already in the other. New structural test asserts set equality plus the ONE
deliberate asymmetry (`Me`, Visual Basic spelling, absent from the shared list
because no SupportedLanguages entry uses it) in BOTH directions, so re-adding it
there or dropping it here each fail loudly.
Structural rather than value-imported: both constants are module-private, and
exporting them purely to be testable would widen two public surfaces to satisfy
a test.
## Two false comments corrected
- `lookup-core.ts` said "see the drift guard noted in #2714", implying a guard
existed when it was only a deferred follow-up. It exists now, and the
comment points at it.
- `callable-id-lockstep.test.ts` claimed its regex "fails if any site
reconstructs the id". It matches ONE template spelling; a hand-rolled
concatenation still slips past. Now stated as a tripwire for the known
shape, not a proof.
## Skill learnings
Four entries appended to eval/workflow_bench/learnings.jsonl from this run: the
v9fs safe-writer failure, backticks silently terminating a query template
literal (hit three times), a module-level TDZ const that passes tsc and then
presents as N file failures with ZERO failing assertions, and concurrent vitest
runs starving worker startup so a whole suite fails at ~5001ms.
## Verification
Full resolver sweep 2926 passed / 1 skipped / 0 failed (51 files) — identical to
pre-A1, which is the evidence that only locals moved. All EIGHT bench gates PASS
with fingerprints UNCHANGED, so no regeneration was needed. function-local-identity,
callable-id-lockstep, receiver-twin-list-drift and closure-binding-labels 71/71.
tsc --noEmit clean.
detect_changes {staged}: 9 changed symbols, 14 affected processes, risk HIGH —
expected, and the reason the sweep above is the gate rather than a targeted list.
Every affected process routes through `resolveDefGraphId`, the key chain Part A
measured at CRITICAL with 23 direct dependents.
Deliberately NOT done: the SCIP end state (opaque `local <id>` plus an explicit
enclosure EDGE instead of containment encoded in the id string). It is a design
direction, not a limitation of this work, and it is INCOMPATIBLE with A1 — A1
widens chain-encoded identity, that removes chain encoding entirely. Bundling
both would re-key every local twice. Written up in the research notes.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0184RmD24KFJidYqpM7v3XjR
* test: update two assertions the #2699 changes correctly invalidated
Both failed on CI at
|
||
|
Gergő Magyar
|
4906daf27b
|
fix(scope-resolution): resolve calls through a closure-valued binding across languages (#2693) (#2695)
* fix(scope-resolution): resolve calls through a closure-valued binding (#2693)
`val f = { }; f()` emitted no CALLS edge in Kotlin or Swift, so `impact` on
such a symbol under-reported to zero — the same false all-clear as #2687.
The cause was not, as first suspected, that these languages fail to feed
`callable-value-flow`. They do: `synthesizeCallableFlowCaptures` is called
from 15 language capture modules, and Kotlin already resolves reassignment
through the pass (`var f = ::a; if (c) f = ::b; f(1)` reaches both targets).
Their captures are already exactly right — the seed names the binding as its
own callable, per the anonymous-callable convention in
callable-flow-captures.ts.
They died one layer later, at the `buildGraphTargetIndex` gate:
if (!isCallable(def) && providerTarget?.(def) !== true) continue;
`isCallable` is Function/Method/Constructor, but the scope-resolution layer
declares a closure binding with its VALUE label (Kotlin/Swift `Property`),
and `isCallableValueTarget` is implemented by exactly one provider — COBOL.
So the binding never entered `graphTargets`; `lexicalCallableLookup` then
returned `shadowed: true` with no targets, which also suppressed the
workspace-wide fallback, and the seed resolved to nothing.
Only the graph knows a value binding holds a callable — since #2687 it emits
a single `Function` node for one. So value bindings now resolve their graph
id first and are admitted on the label of the node they actually reach.
This is self-limiting: a genuine constant keeps its own Const/Property node,
so `resolveDefGraphId`'s qualified key hits before the label-agnostic
`simpleKey` fallback can reach a same-named callable. Only a binding whose
own value node was replaced by a callable one gets through.
No scope kind changes — Kotlin's `lambda_literal` stays `@scope.block`, so
#1757 smart-cast semantics are untouched by construction. The fix is
language-neutral: it discriminates on the graph node label, never on a
language name.
Dart is fixed separately; its root cause is independent.
* fix(dart): resolve calls through a closure-valued binding (#2693)
Dart needed more than the shared gate fix: neither of its closure-binding
forms could resolve, for two different reasons, and the plan's one-line
diagnosis turned out to be incomplete.
TOP-LEVEL `var f = (x) => x;`
A graph Function node already existed (#2687), but no `@declaration.*`
matched the binding, so scope resolution had no SymbolDefinition to attach
a flow seed to. Adding the declaration exposed a second problem: Dart's
`initialized_identifier` is FIELDLESS, so the shared field-based assignment
fallback (`left`/`name`/`value`/…) decomposed nothing and the binding still
emitted no flow captures at all. Kotlin's fieldless `assignment` node hit
exactly this and took the same remedy — a provider `extractAssignment`.
FUNCTION-LOCAL `void m() { var f = (x) => x; }`
Locals parse as `initialized_variable_definition`, which the top-level
graph-node rules are deliberately anchored under (program) to avoid, so a
local closure had no graph node at all — nothing for the widened
`buildGraphTargetIndex` gate to admit.
Both new rules are restricted to a `function_expression` value. Declaring
every Dart variable would mint defs and nodes repo-wide for no resolution
benefit; ordinary locals stay unindexed exactly as before. The top-level
declaration reuses the (program) anchor the graph-node query already relies
on, so class-body fields — which share `initialized_identifier_list` and are
already `@declaration.property` — are never matched twice.
Also drops the now-false note in tree-sitter-queries.ts claiming `f()` does
not resolve for Dart. That node is now the evidence that makes it resolve.
* docs(scope-resolution): document the callable-flow capture contract (#2693)
The module is 1200+ lines behind a nine-line docblock, and the only worked
example was C. Both root causes fixed in this series were "the contract was
discoverable only by reading the emitter":
- the anonymous-callable convention (a seed whose source is a closure takes
its DESTINATION's name) is what makes closure bindings resolvable at all,
and is the reason the widened target gate is correct;
- a fieldless binding node silently decomposes to nothing under the shared
assignment fallback, which cost Kotlin one debugging cycle in #2522 and
Dart another here;
- captures alone are never enough — the bound name also needs a
`@declaration.*` or there is no cell to key the seed on.
Records the cell/site model, both traps, and points at the fullest and
smallest worked examples.
Bumps INCREMENTAL_SCHEMA_VERSION 15 → 16 and the parse-cache SCHEMA_BUMP
22 → 23: this series emits NEW CALLS edges and new Dart Function nodes, and
the incremental write set only covers changed files, so an existing index
would keep reporting a zero blast radius for exactly the symbols the fix is
about.
* perf(scope-resolution): pre-filter value bindings in the callable target index (#2693)
Widening the `buildGraphTargetIndex` gate to consider VALUE bindings put the
hot loop on a much larger def population — value bindings outnumber callables
in real source — and the naive version paid full price per binding. Measured
on a synthetic 800-file corpus (8 value bindings per file, 1 of them a closure
binding), the widening cost 2.50-2.82x the pre-#2693 callable-only build.
Two wastes, both provable rather than guessed:
1. `definitionAnchorKey` ran for every def, including value bindings. The
anchor index is keyed by callable LABEL and the key is built from
`def.type`, so a value def can never hit it — and the key costs a regex
per def.
2. Every value binding paid the whole `resolveDefGraphId` key chain only to be
rejected. It need not: every qualified key that function tries embeds
`def.type`, so for a VALUE def those can only ever reach a value-labelled
node. Its one route to a callable is the label-agnostic
`simpleKey(filePath, simpleName)` fallback, which by construction requires
a callable node with the SAME file and simple name. So a value binding with
no such node cannot resolve to a callable, and one Set lookup decides it.
That set is derived in the graph walk the anchor index already performs, so it
costs no extra pass.
large_ms 7.79-8.37 -> 4.90-5.02 (1.61x faster)
widening_overhead 2.50-2.82 -> 1.45-1.50
The resolved target-set fingerprint is byte-identical across both, which is
the point: this is a cost change, not a behaviour change.
Adds bench/callable-value-flow/ (fingerprint + scaling + widening-overhead
gates) and wires it into ci-tests.yml beside the other build-free benches. The
overhead budget of 1.9 sits between the measured with-filter and without-filter
bands, so it cannot be met if the pre-filter is removed. Timings use the MIN of
15 warmed reps, not the median: the same build reported 1.65 idle and 2.03
under load, and a median-based gate would have to be loosened past the point of
detecting the regression it exists to catch.
`buildGraphTargetIndex` is exported for the bench; it is pure and not part of
the pass's public contract.
* test(scope-resolution): assert the declaration route does not double-emit (#2693)
Go, Python, C++ and TS/JS already resolved a closure-binding call through
their `@declaration.function` capture. The widened `buildGraphTargetIndex`
gate gives the same call a SECOND possible route, so each must still produce
exactly one edge.
`tryEmitEdge` dedups by key, but a collapsed key and a site-anchored key are
DIFFERENT keys — a real double-emit would show up as two ids for one call
site, not be silently collapsed. Asserting on edge ids rather than target ids
is what makes that visible.
* fix(scope-resolution): join value bindings to their callable node by POSITION (#2693)
Review found the first cut of this series minted FALSE CALLS edges. Admitting a
value binding whose *resolved* graph node is callable let `resolveDefGraphId`
fall through to its label-agnostic, first-write-wins
`simpleKey(filePath, simpleName)` and bind the name to ANY same-named callable
in the file.
The safety argument in the previous commit — "a genuine constant keeps its own
Const/Property node, so the qualified key hits first" — silently assumed
`def.type === node.label`. It does not hold:
- TypeScript declares `const` as `Variable` but emits a `Const` NODE, so the
qualified key misses even though the value node exists;
- Rust `let` bindings get no graph node at all, so the fallback is the only
route.
Reproduced, all previously emitting a fabricated caller:
const save = (x: number) => x * 2; // next to an unrelated Svc.save
-> Method:svc.ts:Svc.save#1 // Svc never instantiated
const handler = other; // shadowing a top-level handler
-> Function:app.ts:handler // unreachable from here
let handler = cb; // Rust
-> Function:main.rs:handler
Worse in Dart, where the same collision INVERTED the feature: the only edge went
to the class method and the closure's own node got none. The result was also
declaration-order dependent — two files differing only in declaration order got
different CALLS sets — and it propagated through argument-to-formal binding into
functions whose source never mentions the name.
A closure binding IS its callable node: same file, same line, same name. An
aliasing local is not. So the join is positional now — a file/line/name index
built in the graph walk `byAnchor` already performs — and value bindings never
run the key chain at all. That is both correct and cheaper:
large_ms 4.90-5.02 -> 4.37-4.63
widening_overhead 1.45-1.50 -> 1.43-1.58 (name-match design: 2.50-2.82)
with a byte-identical target-set fingerprint on the bench corpus.
Also from review:
- `Static` dropped from VALUE_BINDING_DEF_TYPES: `normalizeNodeLabel` has no
`static` case, so no def can carry that type — it was an entry no fixture
could ever exercise. The remaining set now documents why it deliberately
does NOT reuse `isOwnableValueLabel`, which is contracted to a different
consumer.
- Dart `final`/`const` top-level closures (static_final_declaration_list) and
every declarator after the first in a multi-name local now resolve; both
parse into shapes the earlier rules never reached.
- The bench source carried a literal NUL byte, so git recorded it as BINARY
and the only artifact pinning the target set was unreviewable in the PR
diff. It is written as an escape now. Its corpus also modelled `startLine`
as 1-based where graph nodes are 0-based, which would have stopped it
exercising the value-binding path at all.
- `call-summary-schema-version.test.ts` asserted `passesReuseGate(15)` is
true; the 15 to 16 bump made that false and the test RED. It now pins 16 as
current and 15 as rejected, matching the pattern every prior bump followed.
- The v23 parse-cache comment is at the top of the list, not mid-list.
Tests: the five collision cases above are new regression tests, each confirmed
failing against the previous commit. Also added Kotlin class-body closures (the
only case exercising the Method arm), Dart top-level `final`, Dart multi-name
locals, and a warm-parse-cache replay for Kotlin and Dart — the #2693 captures
are replayed verbatim, so a serialization change would surface only on a SECOND
analyze and every other test here runs cold. The previous negative tests were
vacuous: they paired names that did not collide (`maxSize` vs `size`), so the
pre-filter rejected them before the guard they were named after could run.
* docs(storage): fix the schema-version changelog blocks (#2693)
Two problems, one mine and one not.
MINE: the `INCREMENTAL_SCHEMA_VERSION` block is ASCENDING (v2 … v15), and I
inserted v16 above v15 rather than at the end — I had just moved the parse-cache
entry to the top of ITS block, which is descending, and applied the same habit
to a list ordered the other way. Moved to the end; both blocks are now
internally consistent.
NOT MINE: the parse-cache block carries TWO v21 entries, with v20 wedged between
them. Tracing it: #2632 (Spring DI facts) bumped 20 -> 21 and merged first;
#2653 (Java JLS local-class identities) had branched at 20, also bumped to 21,
and merged second — so it shipped with NO invalidation of its own. An index
already stamped 21 by the first change was treated as current by the second and
kept serving stale local-class identities from the warm cache.
Numbers left alone: both genuinely shipped as 21, and renumbering them now would
misstate what users' indexes actually contain. Instead the entry says so
explicitly, and points at the process fix — re-check the constant against
origin/main immediately before merging, not just when the branch is cut. The
identical collision hit INCREMENTAL_SCHEMA_VERSION in #2653/#2654, so this is a
recurring failure mode of concurrent PRs, not a one-off typo.
Comment-only; no constant changes value.
* feat(scope-resolution): resolve closure bindings in Ruby, Java, C#, PHP and JS/TS var (#2693)
Ruby, Java, C# and PHP already emitted correct callable-flow seeds and invokes.
What they lacked was the #2687 piece — a CALLABLE graph node at the binding,
which is what buildGraphTargetIndex joins to by position. PHP additionally had
no scope declaration for the bound name, so the flow pass had nothing to attach
its seed to.
ruby handler = ->(x) { x } handler.call(1) -> Function:a.rb:handler
java Function<..> handler = x->x handler.apply(1) -> Function:A.java:A.handler
csharp Func<int,int> handler = ... handler(1) -> Function:A.cs:A.handler
php $handler = fn($x) => $x $handler(1) -> Function:a.php:handler
Ruby and Java invoke through the callable-object protocol; C# and PHP call the
binding directly. Locals work in all four, and a binding whose name collides
with a same-named method resolves to the CLOSURE, not the method.
Two things the sweep caught:
JAVA TWIN. Anchoring the rule on the inner variable_declarator produced BOTH a
Function and a Property node — the exact double-indexing #2687 removed. The
parse-worker dedup keys on (definition node, name), and Java's value rule
anchors on field_declaration, so the keys never matched. Re-anchored on
field_declaration / local_variable_declaration.
JS/TS `var`. `var f = (x) => x` kept a Variable label while const/let got
Function, because `var` is a different grammar node (variable_declaration vs
lexical_declaration) that no closure rule covered. A call through the binding
still resolved via the declaration route, so the CALLS edge pointed at a
NON-callable node. Now consistent across const/let/var.
That last one flipped an existing assertion in const-function-twin.test.ts,
which expected `Variable` for a var-bound function-expression. Its comment
explained why — "var has no matching @definition.function pattern, so nothing
claims the name" — i.e. it documented the gap rather than defending it. The
property it was really protecting (an UNCLAIMED value node survives) now has
its own case with a non-function initializer, and the var-closure case asserts
the collapse to one node, which is also the twin guard for the new rule.
Known limits, both pre-existing and both failing safe:
- A PHP local closure whose name collides with a top-level function gets no
edge: both want id Function:<file>:<name>, so the closure never gets its own
node. This is the file-scoped node-identity convention — TypeScript, Python
and Dart collapse identically at base.
- TS/JS class-field arrows stay Property (Kotlin's equivalent emits Method).
They already resolve; changing the label risks the HAS_PROPERTY ownership
regression #2687 hit once.
The invalidation constants already bumped in this PR (INCREMENTAL_SCHEMA_VERSION
16, SCHEMA_BUMP 23) cover these additional languages; their notes now say so.
Tests: one case per newly-resolving language plus the PHP anonymous-function
form and the JS var form, in closure-binding-labels.test.ts. The file now spins
a worker pool per test across a dozen languages, so its timeout is raised
file-wide — a case that takes ~7s alone was exceeding the 30s default under
that contention.
* fix(ingestion): class-field closures are callable members in TS/JS (#2693)
A CALLS edge must target a callable node. `class A { handler = (x) => x }` emitted
a Property, so calling it produced `CALLS -> Property:A.ts:A.handler` — an edge
pointing at something the graph says is not callable. Same defect class as the
JS/TS `var` binding fixed in the previous commit, and the last place a closure
binding still carried a value label.
Kotlin already models its class-body closure as Method + HAS_METHOD; TS/JS now
match, so all three agree:
class-field closure -> Method + HAS_METHOD (CALLS target is callable)
plain class field -> Property + HAS_PROPERTY (unchanged, no CALLS)
Anchored on public_field_definition / field_definition — the same nodes the
property rules use — so the parse-worker dedup collapses the pair rather than
leaving a Method/Property twin, the failure the Java rule hit in the previous
commit.
ON MATCHING THE COMPILERS. This deliberately diverges from tsc and SCIP. The
TypeScript compiler classes `handler = () => {}` as a PropertyDeclaration
("a property declaration independently from what it's assigned to"), and SCIP
gives it a `.` term descriptor, the same suffix as any field — both call it a
property, and Kotlin's compiler likewise treats `val f = { }` as a property with
a function type. The divergence is intentional: GitNexus's Function/Method label
does not mean "tsc SymbolFlags", it means "this node can be the target of a
CALLS edge", which is the convention #2687 set for closure bindings in every
language. Modelling it the compiler's way would mean either dropping call
resolution for these members or emitting a separate node for the lambda and
flowing the property to it — the two-node shape #2687 removed. Recorded here so
the next reader does not "fix" it back.
Tests: TS and JS class-field arrows resolve to their Method node, plus a guard
that a NON-closure class field stays a Property — the closure rule must key on
the initializer, not on the field syntax.
* fix(php): keep the $ sigil on closure-binding nodes so locals stop colliding (#2693)
A PHP local closure whose name matched a file-level function got NO edge at all:
function save($x) { return $x; }
function run() {
$save = fn($x) => $x * 2;
return $save(1); // no CALLS edge
}
Both minted the id Function:<file>:save, so the closure's node was swallowed by
the function's and the positional join found nothing at the binding's line.
The fix is PHP's own semantics rather than a change to node identity across the
graph. PHP holds variables and functions in SEPARATE namespaces — $save and
save() cannot collide in the language — and the sigil is what separates them.
Dropping it was the bug. The node rule now captures the whole variable_name, so
the closure is Function:<file>:$save and the function stays Function:<file>:save.
languages/php/query.ts already keeps the sigil on property declarations for the
same reason, so this makes the two consistent.
The positional join normalises a leading $/@ on both sides, matching what the
scope layer and the callable-flow synthesizer already do, so the binding still
matches its own declaration while its NODE stays distinct.
local closure + same-named function -> Function:c.php:$save (the closure)
calling the real function -> Function:f.php:save (unchanged)
plain $max = 10 -> no node, no edge (unchanged)
WHAT THIS DOES NOT FIX. The general problem is wider than PHP: GitNexus node ids
are file-scoped, so a function-local symbol and a file-level one with the same
name collapse in TypeScript, Python and Dart too, and Java/C# only escape by
qualifying on the enclosing CLASS (so two same-named locals in different methods
still collide). SCIP solves it with a separate `local <id>` keyspace that is
document-scoped and never globally addressable. That is issue #2699 — it changes
persisted ids for every function-local symbol and needs its own invalidation, so
it is not bundled here. PHP is fixed on its own merits: the sigil belongs in the
identity regardless of how locals are eventually scoped.
* test(scope-resolution): pin the closure-binding caller-attribution limit (#2693)
Review of this PR found the new callable nodes are call TARGETS but never call
SOURCES: a call made INSIDE a closure binding is attributed to the enclosing
scope, so `impact(handler, direction:"downstream")` reports nothing even though
the closure calls out. Consistent across Kotlin, Dart, Ruby and PHP; TS/JS free
bindings are the exception because their arrow carries a @scope.function whose
range matches.
Not fixed here — pinned, so the boundary is visible instead of surprising, and
so a change in EITHER direction fails a test.
The cause is precise: `pickCallerCallableDef` (graph-bridge/ids.ts) finds the
caller by walking CHILD scopes whose range contains the call site, gated on
`child.kind === 'Function'`. A closure literal is a BLOCK scope in these
languages (Kotlin deliberately, #1757 smart casts), AND the binding's def is
owned by the enclosing scope rather than by the closure's scope — so neither
half of the link exists. Fixing it needs "callable boundary" decoupled from
scope `kind` plus an association between the closure scope and its binding.
That is a change to the caller anchor used by every call in the repo, which is
not something to land at the tail of this PR.
Also adds a unit suite for `buildGraphTargetIndex` itself, covering what the
integration tier cannot isolate: a binding is admitted only on POSITIONAL
evidence, a name-only match is rejected, a non-callable node at that position is
rejected, an ambiguous position claimed by two callables is rejected, and the
PHP dollar sigil normalises across the join while still not matching a
same-named function on another line. That last one closes the review's LOW —
the node/declaration name asymmetry now has an executable contract rather than
resting on a comment.
* docs(test): correct the per-language cause of the attribution limit (#2693)
The comment on the pinned attribution tests claimed "a closure literal is a
BLOCK scope in these languages". That is true for Kotlin (lambda_literal
@scope.block, #1757) and Ruby (do_block/block @scope.block) and FALSE for PHP:
anonymous_function and arrow_function are already @scope.function
(php/query.ts:61-62). Dart is a third case again — it has no scope over a
closure literal at all.
So the four languages fail at three different points, not one:
Kotlin, Ruby fail the `child.kind === 'Function'` gate
PHP passes that gate; its closure scope owns no callable def,
because the binding's def belongs to the enclosing scope
Dart has no child scope for the walk to consider
Worth correcting carefully rather than tidying: a follow-up plan re-stated this
comment instead of re-deriving it, and inherited the misdiagnosis — it proposed
"relax the kind gate" as required for all four, which is a no-op for PHP and
unreachable for Dart. A review caught it. The comment now states each language's
actual blocker and says why the distinction matters.
Comment-only; the three pinned tests are unchanged and still pass.
* fix(scope-resolution): an ordinary JS/TS `function` binds its own `this` (#2701)
`this.m()` inside a nested `function` resolved to the lexically enclosing
class, so it emitted a CALLS edge that does not exist at runtime — including
the exact `forEach(function () { this.m(); })` shape arrow functions were
introduced to avoid:
class D {
m() {}
build() { const h = function () { this.m(); }; return h; }
}
// CALLS: Function:D.ts:D.h -> Method:D.ts:D.m#0 FALSE
ECMA-262 gives an arrow `[[ThisMode]] = lexical`: it has no `this` binding in
its environment record, so the lookup passes through to the enclosing
environment. Every other function form binds `this` at call time. `tsc` draws
the same line by resolving `this` through `getThisContainer` with
`includeArrowFunctions = false`. That one rule is the whole fix.
Languages declare it; shared code never learns a language. The query files —
the one place that already names grammar nodes — tag every non-arrow function
form with `@receiver-owner.this`, which becomes `Scope.ownsReceivers`. A
receiver walk that reaches such a scope without finding the name stops there
instead of borrowing an enclosing scope's binding. Every other language leaves
the field unset and is bit-for-bit unchanged; a Kotlin lambda, which DOES
capture the enclosing `this`, still resolves (pinned as a test).
THREE GATES, ALL LOAD-BEARING. The false edge survived each one alone, which
is why the tests assert on the emitted edge rather than any single walk:
1. `Scope.ownsReceivers` stops BOTH receiver-type walks — `findReceiver
TypeBinding` here and its twin `lookupReceiverType` in gitnexus-shared's
`lookup-core`, which was resolving the receiver independently.
2. `LanguageTypeConfig.thisBoundaryNodeTypes` stops the type-env AST walk
that infers a receiver's type during capture.
3. `isReceiverOwnedButUnbound` makes `receiver-bound-calls` SUPPRESS the
site. Without it the member still resolved by NAME through `lookupCore`'s
lexical chain — the class-body scope binds `m` two scopes up — merely at
lower confidence. An owned-but-unbound receiver is a definitive negative,
not a miss, so it must not reach a receiver-blind fallback.
Also fixed: `function*(){}` as an expression was not a `@scope.function` at
all, so `this` inside one read as the enclosing method's.
WHAT THIS GIVES UP. The fix REMOVES edges, and some were correct:
`.bind(this)`, `.call(this)` and `forEach(fn, thisArg)` do make `this` the
instance at runtime. Their correctness is fixed at the CALL SITE, which no
scope-level rule can see, so the choice is between losing them and keeping
every detached-callback false positive. All three are pinned as tests
asserting the empty result, so changing the trade later is deliberate.
`this` in a static method also stops resolving to the INSTANCE member — that
edge was wrong in the other direction.
INVALIDATION. Both constants move, and the parse-cache one is not optional:
`ownsReceivers` lives on the cached `Scope`, and a warm cache replays scopes
without it — verified by probe that `--force` alone does NOT re-derive it, so
the fix silently did nothing until SCHEMA_BUMP moved. INCREMENTAL_SCHEMA_
VERSION 16 -> 17 (the incremental write set covers only changed files, so
unchanged TS/JS files would keep their fabricated `this` edges);
SCHEMA_BUMP 23 -> 24.
Verified against a built index, not by reading: all three false edges from the
issue gone, every correct edge kept, same result in JavaScript through its
separate grammar. 64 tests green across the new suite plus the closure-binding
and schema-version suites. The full suite's 36 failures are pre-existing
load-flakes — confirmed by A/B: `skip-git-cli` fails FOUR tests on a clean
HEAD versus three with this change, and `pipeline-pdg-streaming` passes in
isolation either way.
Refs #2701
* fix(ingestion): give function-local callables their own identity (#2699)
Graph node ids were file-scoped, so a local callable and a same-named
file-level one collapsed onto ONE node. That is a wrong answer, not a missing
one — the local call was attributed to the file-level symbol:
export function save(x) { return x; }
export function run() { const save = x => x * 2; return save(1); }
export function other() { const save = x => x * 3; return save(2); }
// ONE node Function:a.ts:save, and BOTH run and other pointed at it, so
// `impact` on the top-level save reported two callers that never call it.
A local's identity is now its enclosing-callable chain plus its own position —
`run.save@2:2`. The chain is for humans reading `impact`; the position is what
makes it correct. Names alone cannot express what ECMAScript actually
specifies, and the gap is the language's, not the grammar's: an environment
record is created per function AND per block, so an anonymous function has no
name to contribute and sibling blocks hold distinct bindings under the same
name. One positional rule settles both, with no conditionals and no
"disambiguate only when it looks ambiguous" heuristic — the ambiguity-flag
class of bug that bit #2514. SCIP reaches the same place with its
document-scoped `local <id>` keyspace.
Top-level functions and class methods are NOT locals and keep their ids
byte-for-byte. That is the bound on the churn: this touches only symbols that
are unreachable from outside their own document anyway.
RESOLUTION JOINS BY POSITION, NOT BY NAME. `resolveDefGraphId` matches a def
to its node on (file, label, line, simple name). A def and its node are the
same construct, so this needs no scope chain at all — which is the point:
re-deriving the chain in the resolver would be a second implementation that
could silently disagree with the first. A genuine tie (two callables on one
line) stores an AMBIGUOUS_POSITION tombstone and falls through to the existing
name keys rather than picking by source order. Without this the node ids were
already correct and calls STILL resolved to the file-level symbol — the fix is
only half a fix without it.
JS/TS GAIN BLOCK SCOPES. They emitted no `@scope.block` at all, so the
resolver could not tell two `const pick` in sibling branches apart. Giving
them distinct ids made that visible as DUPLICATE edges — each call resolving
to BOTH — which is worse than the collapse it replaced. `(statement_block)
@scope.block` supplies the missing environment record. The other half of the
ECMAScript rule was already implemented and waiting: `tsBindingScopeFor`
hoists `var` past blocks to the enclosing Function/Module while `let`/`const`
bind innermost, and its docblock already claimed "the innermost default covers
these" for block scopes that did not exist. All 82 scope-resolution test files
pass with blocks on.
Verified by probe, per case: two locals in different functions, a local inside
an ANONYMOUS function (`outer.fn@1:9.save@2:4`), sibling blocks resolving to
their own binding, `var` still hoisting out of its block, a nested named
`function` vs a file-level one, PHP composing with the `$` sigil from #2693,
and Python. Top-level/method ids unchanged, asserted directly.
Every assertion is on the EDGE, not on node existence. Ids are built twice and
independently — definition phase and caller attribution — and a one-character
disagreement makes the caller attach to a node that does not exist and the
edge vanish, with nothing thrown and no test failing. An edge assertion can
only pass if both phases agree.
INVALIDATION. INCREMENTAL_SCHEMA_VERSION 17 -> 18 and SCHEMA_BUMP 24 -> 25:
persisted node ids change for every function-local callable, and the cached
scope tree lacks block scopes. A top-up would leave unchanged files on the old
ids while changed files emit the new ones, splitting each symbol in two.
Bench fingerprint unchanged and both timing budgets pass. The one full-suite
failure (incremental-orchestration) passes in isolation — its log shows stale
init locks and WAL reclaim, i.e. LadybugDB contention under the parallel run.
Refs #2699
* perf(ingestion): emit block scopes only where they bind something (#2699)
Block scopes make `let`/`const` in sibling blocks distinct bindings, which is
what stopped a call in one branch resolving to both. Emitted naively — one
scope per `statement_block` — they also cost ~10% of analyze wall time, because
every scope-chain walk in every function then steps through levels that bind
nothing.
Two emit-side filters keep the semantics and drop the waste:
1. A block that IS a function body duplicates the enclosing Function scope.
Nothing can be declared between a function and its own body, so a binding
in either resolves identically — the inner scope is pure depth.
2. A block that declares no `let`/`const`/`class`/`function` binds nothing,
so it is transparent: a lookup finds nothing in it and walks to the
parent. `var` is deliberately excluded from that list — it hoists past the
block to the function, so a block containing only `var` still binds
nothing.
MEASURED, on a 762-file / 228k-line TypeScript corpus (gitnexus/src), min of 6
warmed reps with the cold first rep discarded:
block scopes emitted 19,389 -> 5,331 (-72%)
total scopes 35,942 -> 21,884 (-39%)
analyze wall time +9.8% -> +1.6-2.5% vs pre-#2699
peak RSS (whole tree) 2398MB -> 2434MB (+1.5%, inside run-to-run noise)
The filters themselves are free: scope emission over the same corpus measured
12.6s naive vs 12.5s filtered.
Wall-clock on a shared runner has a ±10% spread run to run, which is wider than
the effect being optimised, so the durable gate added here counts scopes
instead. `bench/scope-emission/measure.mjs --check` asserts an EXACT scope set
over a synthetic corpus that mixes the shapes the filters discriminate between
— function/method/arrow bodies, non-declaring if/else/for/while/try, blocks
that declare `const`, and a `var`-only block. Baseline is 2 block scopes per
module: only the two `if`/`else` branches that declare `const chosen`. If the
filters regress that number jumps immediately, in a way wall-clock CI could
never resolve from noise. Wired into the existing benchmarks job.
Behaviour is unchanged: 86 scope-resolution and identity test files, 1371
tests, all green — including the sibling-block case this could plausibly have
broken — and the callable-value-flow fingerprint is untouched.
Refs #2699
* test(bench): re-baseline the TS/JS scope-capture fingerprints for #2701
`bench/scope-capture` fingerprints the full capture set per language, and
#2701 added a `@receiver-owner.this` marker to every non-arrow function form
so a scope that BINDS its own `this` can terminate the receiver walk. That is
a capture-set change, so the TypeScript and JavaScript fingerprints moved and
the benchmarks job has been failing since that commit — I pushed it without
checking CI.
A fingerprint is a correctness gate, so this does not simply adopt the new
value. Verified first by diffing the capture-name HISTOGRAM over the same
fixture corpus against
|