mirror of
https://github.com/abhigyanpatwari/GitNexus.git
synced 2026-08-28 05:25:25 +00:00
4906daf27b
* 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
|
||
|---|---|---|
| .. | ||
| actions | ||
| claude-canary-runtime | ||
| gitnexus-review-runtime | ||
| ISSUE_TEMPLATE | ||
| prompts | ||
| scripts | ||
| workflows | ||
| actionlint.yaml | ||
| CODEOWNERS | ||
| dependabot.yml | ||
| FUNDING.yml | ||
| PULL_REQUEST_TEMPLATE.md | ||
| release-drafter.yml | ||
| release.yml | ||
| vendored-grammars.json | ||
| zizmor.yml | ||