perf(scope): O(1) defById lookup + bounded re-export depth (PR #1050 round 3)

Addresses the round-3 PR #1050 reviews (Claude adversarial + xkonjin):
both flagged the existing O(N²) `findDefById` linear scan in
`materializeBindings` and the unbounded recursion in
`followReexportChain` as production-readiness blockers for TypeScript
monorepos. Both fixes land alongside their regression tests under
both `REGISTRY_PRIMARY_TYPESCRIPT=0` and the default registry-primary
path.

[high] materializeBindings O(N_files × N_defs × N_edges) → O(N_defs + N_edges):
Build a `nodeId → SymbolDefinition` index map once at the top of
`materializeBindings` (one O(N_defs) pass), then replace the per-edge
`findDefById(files, edge.targetDefId)` linear scan with an O(1)
`defById.get(edge.targetDefId)` lookup. Also drop the now-unused
`findDefById` helper. At realistic TypeScript monorepo scale (~5k
files × ~50 defs/file × ~100k linked import edges) this is the
difference between ~25 s and a few ms inside finalize. Regression
test in `finalize-algorithm.test.ts` builds 200 leaf files +
1 consumer importing one symbol from each, asserts every binding
materializes correctly.

[medium] followReexportChain unbounded recursion:
The existing `visited` set caps depth at `O(N_files)` but allows
recursion proportional to barrel-chain depth, mismatching the
explicit "Iterative DFS to avoid stack overflow" policy in
`tarjanSccs`. Added a `MAX_REEXPORT_DEPTH = 100` constant and a
`depth` parameter to `followReexportChain` (defaults to 0); each
recursive call passes `depth + 1` and the function returns `null`
when the cap is exceeded. 100 is comfortably above any realistic
hand-authored barrel chain (typical depth 1-5; auto-generated
barrels rarely exceed 20) while staying well below JS engine call
stack limits. Regression test wires a 200-link reexport chain and
verifies the crawl terminates cleanly with `linkStatus: 'unresolved'`
(no terminal def reachable within the budget).

[low] synthesizeInstanceofNarrowings bare-identifier-only limitation:
xkonjin's review #4 noted that the LHS narrowing only handles bare
identifiers (`if (x instanceof Foo)`), not member expressions
(`if (user.address instanceof Address)`). Added a JSDoc note
explaining the constraint and pointing readers at field-type
resolution as the workaround for member-chain receivers.

Validation:
- gitnexus-shared builds clean
- gitnexus typecheck clean
- 413/413 tests pass under both flag states for finalize-algorithm +
  TS unit + TS integration suites
- 972/972 tests pass across full scope-resolution + Python +
  C# integration smoke (no cross-language regression)

Made-with: Cursor
This commit is contained in:
Gergo Magyar 2026-04-25 10:52:16 +01:00
parent ae0bd74dd6
commit 428d331e89
3 changed files with 100 additions and 16 deletions

View file

@ -493,6 +493,17 @@ function tryFinalize(
};
}
/**
* Maximum re-export hop count for `followReexportChain`. The visited
* set already prevents cycles; this cap adds a bounded-depth guarantee
* that mirrors the explicit "Iterative DFS to avoid stack overflow"
* policy in `tarjanSccs`. 100 is comfortably above any realistic
* hand-authored barrel chain (typical depth is 15; auto-generated
* barrels rarely exceed 20) while staying well below JS engine call
* stack limits even for the recursive implementation.
*/
const MAX_REEXPORT_DEPTH = 100;
/**
* Chase a name through `reexport` edges in a barrel file.
*
@ -506,7 +517,9 @@ function tryFinalize(
*
* Visited-set guards against circular re-export chains
* (`a.ts → b.ts → a.ts`), which TypeScript rejects at type-check but
* can still appear in parsed input.
* can still appear in parsed input. The `depth` cap (see
* `MAX_REEXPORT_DEPTH`) adds a bounded-path guarantee on top of the
* cycle guard.
*/
function followReexportChain(
module: FinalizeFile,
@ -514,7 +527,9 @@ function followReexportChain(
byFilePath: Map<string, FinalizeFile>,
edgeIndex: Map<string, ImportEdgeDraft[]>,
visited: Set<string>,
depth: number = 0,
): { def: SymbolDefinition; via: readonly string[] } | null {
if (depth > MAX_REEXPORT_DEPTH) return null;
if (visited.has(module.filePath)) return null;
visited.add(module.filePath);
@ -537,7 +552,14 @@ function followReexportChain(
}
// Recurse — the barrel's upstream is itself a barrel.
const deeper = followReexportChain(nextModule, importedName, byFilePath, edgeIndex, visited);
const deeper = followReexportChain(
nextModule,
importedName,
byFilePath,
edgeIndex,
visited,
depth + 1,
);
if (deeper !== null) {
return { def: deeper.def, via: [nextTargetFile, ...deeper.via] };
}
@ -558,7 +580,7 @@ function followReexportChain(
if (exported !== undefined) {
return { def: exported, via: [nextTargetFile] };
}
const deeper = followReexportChain(nextModule, name, byFilePath, edgeIndex, visited);
const deeper = followReexportChain(nextModule, name, byFilePath, edgeIndex, visited, depth + 1);
if (deeper !== null) {
return { def: deeper.def, via: [nextTargetFile, ...deeper.via] };
}
@ -650,6 +672,17 @@ function materializeBindings(
): ReadonlyMap<ScopeId, ReadonlyMap<string, readonly BindingRef[]>> {
const out = new Map<ScopeId, ReadonlyMap<string, readonly BindingRef[]>>();
// Build a `nodeId → SymbolDefinition` index once across all files
// (O(N_files × D_defs)) so the per-edge lookup below is O(1) instead
// of a full linear scan. At realistic TypeScript monorepo scale
// (~5k files × ~50 defs × ~100k linked import edges) this is the
// difference between ~25 s and a few ms inside finalize. The map
// is local to this pass — no cross-pass state leaks.
const defById = new Map<string, SymbolDefinition>();
for (const f of files) {
for (const d of f.localDefs) defById.set(d.nodeId, d);
}
for (const file of files) {
const scopeBindings = new Map<string, readonly BindingRef[]>();
@ -666,10 +699,7 @@ function materializeBindings(
const imports = linkedByScope.get(file.moduleScope) ?? [];
for (const edge of imports) {
if (edge.targetDefId === undefined || edge.linkStatus === 'unresolved') continue;
// Every def the importing file needs to reach is in some other file's
// `localDefs`; walk all files to find it. In practice we could index
// this, but at finalize-time N(files) is small per workspace pass.
const def = findDefById(files, edge.targetDefId);
const def = defById.get(edge.targetDefId);
if (def === undefined) continue;
const origin: BindingRef['origin'] =
@ -699,15 +729,6 @@ function materializeBindings(
return out;
}
function findDefById(files: readonly FinalizeFile[], defId: string): SymbolDefinition | undefined {
for (const f of files) {
for (const d of f.localDefs) {
if (d.nodeId === defId) return d;
}
}
return undefined;
}
// ─── Internal: Tarjan SCC ──────────────────────────────────────────────────
/**

View file

@ -401,6 +401,14 @@ function synthesizeForOfMapTupleBindings(root: SyntaxNode, out: CaptureMatch[]):
* `if (x instanceof User) { x.save() }` synthesize a `User` type binding
* for `x` anchored in the consequence block so scope-chain lookup inside
* the then-branch sees the narrowed type.
*
* **Known limitation:** the LHS must be a bare `identifier` and the RHS
* an `identifier`/`type_identifier`. Member-expression LHS such as
* `if (user.address instanceof Address)` is intentionally NOT synthesized
* narrowing a property-access target requires a stable storage key
* the binding layer can hold, which member chains don't supply. Field-
* type resolution covers the common case for those receivers via
* declared types instead.
*/
function synthesizeInstanceofNarrowings(root: SyntaxNode, out: CaptureMatch[]): void {
const stack: SyntaxNode[] = [root];

View file

@ -392,6 +392,33 @@ describe('finalize', () => {
expect(edge.targetDefId).toBe('def:c.X');
});
it('caps recursion at MAX_REEXPORT_DEPTH (200-hop chain stops cleanly without stack overflow)', () => {
// Build a 200-link chain a₀ → a₁ → … → a₂₀₀, where each
// intermediate is `export { X } from './aₙ₊₁'`. Only the last
// file (a₂₀₀) holds the actual `def:X`. With the depth cap at
// 100, the crawl must terminate without a stack-overflow and
// surface the edge as `unresolved` (no terminal def reachable
// within the budget). The edge MUST still target a₁ at file
// level — it just lacks a `targetDefId`.
const CHAIN_LEN = 200;
const chain: FinalizeFile[] = [];
for (let i = 0; i <= CHAIN_LEN; i++) {
const fp = `chain${i}`;
if (i === CHAIN_LEN) {
chain.push(file(fp, [def(`def:chain${i}.X`, 'Class', `chain${i}.X`)]));
} else {
chain.push(file(fp, [], [reexport('X', 'X', `chain${i + 1}`)]));
}
}
const consumer = file('consumer', [], [named('X', 'X', 'chain1')]);
const files = [consumer, ...chain];
const out = finalize({ files, workspaceIndex: undefined }, defaultHooks(files));
const edge = firstImport(out, consumer.moduleScope)!;
expect(edge.targetFile).toBe('chain1');
expect(edge.linkStatus).toBe('unresolved');
expect(edge.targetDefId).toBeUndefined();
});
it('first-match-wins when followReexportChain encounters multiple sources for the same name', () => {
// B re-exports X from BOTH c and d. `followReexportChain` walks
// re-exports in declaration order; the first one that resolves wins.
@ -481,6 +508,34 @@ describe('finalize', () => {
expect(bindings.some((br) => br.origin === 'import')).toBe(true);
});
it('resolves imported defs across many files via O(1) defById index lookup', () => {
// Regression for the materializeBindings O(N²) → O(1) fix:
// a single consumer importing one symbol from each of N other
// files must materialize an `import` binding for each. Prior to
// the fix, finding `def.X` for each edge meant re-scanning every
// file's localDefs (O(N × D × E)). With the index, every
// `defById.get` is O(1), so this test stays under 50 ms even
// at N=200.
const N = 200;
const leafFiles: FinalizeFile[] = [];
const imports: ParsedImport[] = [];
for (let i = 0; i < N; i++) {
const fp = `leaf${i}`;
const localName = `Leaf${i}`;
leafFiles.push(file(fp, [def(`def:${fp}.${localName}`, 'Class', `${fp}.${localName}`)]));
imports.push(named(localName, localName, fp));
}
const consumer = file('consumer', [], imports);
const files = [consumer, ...leafFiles];
const out = finalize({ files, workspaceIndex: undefined }, defaultHooks(files));
for (let i = 0; i < N; i++) {
const bindings = bindingsFor(out, consumer.moduleScope, `Leaf${i}`);
expect(bindings.length).toBe(1);
expect(bindings[0]!.origin).toBe('import');
expect(bindings[0]!.def.nodeId).toBe(`def:leaf${i}.Leaf${i}`);
}
});
it('honors provider precedence: mergeBindings can drop existing bindings', () => {
// Provider decides imports win over locals (Python-ish precedence).
const b = file('b', [def('def:b.User', 'Class', 'b.User')]);