mirror of
https://github.com/abhigyanpatwari/GitNexus.git
synced 2026-09-09 22:33:39 +00:00
fix(SM-14): address PR #743 BindingAccumulator review findings
Addresses the 5 findings from PR #743 review comment 4211636245. Critical (R1) — Strip function-scope bindings from worker IPC - parse-worker.ts previously serialized typeEnv.allScopes() over the worker IPC boundary on every batch, pushing ~4.9 MB of function-scope bindings (e.g. `handleRequest@15 → db: Database`) into the accumulator with zero downstream consumers. The only reader is the ExportedTypeMap enrichment loop in pipeline.ts, which calls fileScopeEntries() — the `scope = ''` subset only. - Narrowed parse-worker.ts to use typeEnv.fileScope() and emit [varName, typeName] pairs. FileAllScopeBindings.bindings type narrowed from [string, string, string][] to [string, string][]. - pipeline.ts adapter updated to unpack the new two-element tuples and construct BindingEntry with scope: '' hardcoded. - Sequential path (call-processor.ts → typeEnv.flush()) is UNCHANGED and still writes all scopes — preserves a working sample of higher- quality bindings for Phase 9 prototyping without IPC cost. - Phase 9 reversion path documented inline on both FileAllScopeBindings and the pipeline adapter: change fileScope() → allScopes(), widen the tuple back to 3 elements, done. Field name `allScopeBindings` kept to keep that revert mechanically trivial. Medium #1 (R2) — Worker-vs-sequential quality asymmetry - BindingAccumulator class JSDoc now documents that entries are NOT homogeneous in resolution quality: sequential path has SymbolTable + importedBindings access (Tier 2 cross-file propagation); worker path has Tier 0 + local Tier 1 only. Phase 9 consumers that trust every entry equally will silently produce worse results for large repos (worker path dominant) than for small ones. Medium #2 (R3) — Integration test for ExportedTypeMap enrichment - Added a 4-scenario test suite mocking KnowledgeGraph nodes and running the exact enrichment loop from pipeline.ts:1082-1110 inline: (a) Exported Function node → enriched (b) Non-exported Variable → filtered out (isExported gate) (c) Exported Const node → enriched (d) No matching graph node → silently skipped via continue - Locks in the `{Label}:{filePath}:{name}` node-ID format contract. If the ID format drifts for any language, this test fires. Low #1 (R4) — Storage split for O(n_file_scope) reads - BindingAccumulator now stores two parallel maps: _allByFile: Map<string, BindingEntry[]> — full entry list _fileScopeByFile: Map<string, [string, string][]> — scope='' fast path - appendFile iterates input once, populates both maps synchronously. fileScopeEntries becomes O(1) map lookup + O(n_file_scope) return — no longer walks function-scope entries to filter. - 4 new tests: mixed scopes correctness, only-function-scope file still visible via files()/fileCount, multiple appends accumulate consistently, 1001-entry performance guard. Low #2 (R5) — Duplicate iteration logic - Resolved as a side effect of R1: after the worker narrowing, the parse-worker loop iterates `fileScope()` (flat map) and typeEnv.flush() iterates `allScopes()` (nested map). Different data shapes — no common helper to extract. Swift CI gap (R7) - Acknowledged as out of scope. Not SM-14 specific — 95 Swift tests skipped across the broader test file. Verification - `tsc --noEmit` clean - 3103 unit tests pass (+10 new scenarios in binding-accumulator.test.ts) - 1766 resolver integration tests pass - Zero regressions Plan: docs/plans/2026-04-09-004-fix-sm14-binding-accumulator-review-findings-plan.md Review: https://github.com/abhigyanpatwari/GitNexus/pull/743#issuecomment-4211636245
This commit is contained in:
Gergo Magyar
parent
47edb754d1
commit
803631fef7
4 changed files with 440 additions and 54 deletions
|
|
@ -1,6 +1,39 @@
|
|||
/**
|
||||
* BindingAccumulator — read-append-only accumulator that collects TypeEnv
|
||||
* bindings across all files in the GitNexus analyzer pipeline.
|
||||
*
|
||||
* **Quality asymmetry between execution paths (PR #743 review):** Entries in
|
||||
* this accumulator are NOT homogeneous in resolution quality. The pipeline
|
||||
* feeds bindings through two paths:
|
||||
*
|
||||
* - **Sequential path** (`call-processor.ts` → `typeEnv.flush()`): files
|
||||
* processed on the main thread have access to the full `SymbolTable` and
|
||||
* `importedBindings` at build time, so their bindings benefit from Tier 2
|
||||
* cross-file propagation (e.g. an imported constructor's return type
|
||||
* flows into the variable binding).
|
||||
*
|
||||
* - **Worker path** (`parse-worker.ts` → IPC tuple → pipeline adapter): files
|
||||
* processed in worker threads run without the main-thread `SymbolTable`
|
||||
* and without `importedBindings`, so they can only produce Tier 0
|
||||
* (annotation-declared) and local Tier 1 (same-file constructor
|
||||
* inference) bindings. Cross-file type flow is not visible to the worker.
|
||||
*
|
||||
* Implication: Phase 9 consumers that trust every accumulator entry equally
|
||||
* will silently produce worse results for large repos (worker path dominates)
|
||||
* than for small ones (sequential path dominates). The asymmetry is
|
||||
* structural — workers cannot see the SymbolTable without either shipping a
|
||||
* copy over IPC or synchronizing after parse. If Phase 9 needs homogeneous
|
||||
* quality, it should either (a) tag entries with their tier at insert time
|
||||
* so consumers can filter, or (b) post-process worker-path entries through a
|
||||
* follow-up resolution pass once the main-thread SymbolTable is complete.
|
||||
*
|
||||
* **PR #743 review — IPC narrowing:** The worker path currently only
|
||||
* serializes file-scope (`scope = ''`) entries through the IPC boundary.
|
||||
* Function-scope bindings are stripped at `parse-worker.ts` to avoid paying
|
||||
* a ~4.9 MB live memory cost for data that has no current consumer. The
|
||||
* sequential path's `flush()` still writes all scopes (file-scope and
|
||||
* function-scope). See `FileAllScopeBindings` JSDoc in `parse-worker.ts`
|
||||
* for the Phase 9 reversion path.
|
||||
*/
|
||||
|
||||
export interface BindingEntry {
|
||||
|
|
@ -13,7 +46,18 @@ const ENTRY_OVERHEAD = 64; // bytes per entry (object overhead + property refs)
|
|||
const MAP_ENTRY_OVERHEAD = 80; // bytes per file entry in the map
|
||||
|
||||
export class BindingAccumulator {
|
||||
private readonly _map = new Map<string, BindingEntry[]>();
|
||||
// PR #743 review (Low finding #1): storage is split into two parallel
|
||||
// maps so fileScopeEntries() is O(n_file_scope) instead of O(n_total).
|
||||
// - _allByFile holds every BindingEntry (used by getFile, memory estimate).
|
||||
// - _fileScopeByFile caches the flat [varName, typeName] view of the
|
||||
// `scope === ''` subset, populated at insert time so reads are O(1) map
|
||||
// lookup + O(n_file_scope) array return. Both maps carry the same key
|
||||
// set modulo the `scope === ''` precondition: _allByFile has a key as
|
||||
// soon as any entry is appended; _fileScopeByFile only has a key once a
|
||||
// file-scope entry arrives. Code that iterates via files() uses
|
||||
// _allByFile so files with only function-scope entries remain visible.
|
||||
private readonly _allByFile = new Map<string, BindingEntry[]>();
|
||||
private readonly _fileScopeByFile = new Map<string, [string, string][]>();
|
||||
private _totalBindings = 0;
|
||||
private _finalized = false;
|
||||
|
||||
|
|
@ -28,13 +72,25 @@ export class BindingAccumulator {
|
|||
if (entries.length === 0) {
|
||||
return;
|
||||
}
|
||||
const existing = this._map.get(filePath);
|
||||
if (existing !== undefined) {
|
||||
// All-scope store.
|
||||
const existingAll = this._allByFile.get(filePath);
|
||||
if (existingAll !== undefined) {
|
||||
for (const e of entries) {
|
||||
existing.push(e);
|
||||
existingAll.push(e);
|
||||
}
|
||||
} else {
|
||||
this._map.set(filePath, entries.slice());
|
||||
this._allByFile.set(filePath, entries.slice());
|
||||
}
|
||||
// File-scope fast-path store. Populated lazily on first file-scope entry.
|
||||
let existingFileScope = this._fileScopeByFile.get(filePath);
|
||||
for (const e of entries) {
|
||||
if (e.scope === '') {
|
||||
if (existingFileScope === undefined) {
|
||||
existingFileScope = [];
|
||||
this._fileScopeByFile.set(filePath, existingFileScope);
|
||||
}
|
||||
existingFileScope.push([e.varName, e.typeName]);
|
||||
}
|
||||
}
|
||||
this._totalBindings += entries.length;
|
||||
}
|
||||
|
|
@ -46,36 +102,31 @@ export class BindingAccumulator {
|
|||
|
||||
/** Get all bindings for a file, or undefined if the file is unknown. */
|
||||
getFile(filePath: string): readonly BindingEntry[] | undefined {
|
||||
return this._map.get(filePath);
|
||||
return this._allByFile.get(filePath);
|
||||
}
|
||||
|
||||
/**
|
||||
* Get only scope='' (file-level) entries as [varName, typeName] tuples.
|
||||
* Backward-compatible with the old workerTypeEnvBindings pattern.
|
||||
* Returns an empty array for an unknown file.
|
||||
*
|
||||
* O(1) map lookup + O(n_file_scope) array construction — does NOT walk
|
||||
* function-scope entries. See the `_fileScopeByFile` field comment for
|
||||
* the storage split rationale (PR #743 review Low finding #1).
|
||||
*/
|
||||
fileScopeEntries(filePath: string): [string, string][] {
|
||||
const entries = this._map.get(filePath);
|
||||
if (entries === undefined) {
|
||||
return [];
|
||||
}
|
||||
const result: [string, string][] = [];
|
||||
for (const e of entries) {
|
||||
if (e.scope === '') {
|
||||
result.push([e.varName, e.typeName]);
|
||||
}
|
||||
}
|
||||
return result;
|
||||
const cached = this._fileScopeByFile.get(filePath);
|
||||
return cached ?? [];
|
||||
}
|
||||
|
||||
/** Iterate over all file paths in insertion order. */
|
||||
files(): IterableIterator<string> {
|
||||
return this._map.keys();
|
||||
return this._allByFile.keys();
|
||||
}
|
||||
|
||||
/** Number of distinct files with at least one binding. */
|
||||
get fileCount(): number {
|
||||
return this._map.size;
|
||||
return this._allByFile.size;
|
||||
}
|
||||
|
||||
/** Total number of binding entries across all files. */
|
||||
|
|
@ -100,7 +151,7 @@ export class BindingAccumulator {
|
|||
*/
|
||||
estimateMemoryBytes(): number {
|
||||
let total = 0;
|
||||
for (const [filePath, entries] of this._map) {
|
||||
for (const [filePath, entries] of this._allByFile) {
|
||||
total += MAP_ENTRY_OVERHEAD + filePath.length * 2;
|
||||
for (const e of entries) {
|
||||
total += ENTRY_OVERHEAD + (e.scope.length + e.varName.length + e.typeName.length) * 2;
|
||||
|
|
|
|||
|
|
@ -918,11 +918,16 @@ async function runChunkedParseAndResolve(
|
|||
});
|
||||
}),
|
||||
]);
|
||||
// Collect all-scope bindings into BindingAccumulator
|
||||
// Collect file-scope bindings into BindingAccumulator.
|
||||
// PR #743 review: the worker IPC payload now carries only file-scope
|
||||
// entries (`scope = ''` hardcoded here). See the FileAllScopeBindings
|
||||
// JSDoc in parse-worker.ts for the rationale and Phase 9 reversion
|
||||
// path — the field name is retained to keep that future revert
|
||||
// mechanically trivial.
|
||||
if (chunkWorkerData.allScopeBindings?.length) {
|
||||
for (const { filePath, bindings } of chunkWorkerData.allScopeBindings) {
|
||||
const entries = bindings.map(([scope, varName, typeName]) => ({
|
||||
scope,
|
||||
const entries = bindings.map(([varName, typeName]) => ({
|
||||
scope: '',
|
||||
varName,
|
||||
typeName,
|
||||
}));
|
||||
|
|
|
|||
|
|
@ -237,11 +237,34 @@ export interface FileConstructorBindings {
|
|||
|
||||
/** All-scope type bindings from TypeEnv — includes function-local scopes.
|
||||
* Used by BindingAccumulator for cross-file type propagation (Phase 9+).
|
||||
* File-scope entries (scope = '') are included as a subset. */
|
||||
*
|
||||
* **PR #743 review follow-up:** Despite the name, this field carries only
|
||||
* file-scope entries (`scope = ''`) after the Critical finding on worker
|
||||
* IPC payload cost. The worker previously serialized all scopes via
|
||||
* `typeEnv.allScopes()`, but function-scope bindings (e.g.
|
||||
* `handleRequest@15 → db: Database`) had no current consumer — the only
|
||||
* reader is `fileScopeEntries()` in the ExportedTypeMap enrichment loop.
|
||||
* Narrowing to `typeEnv.fileScope()` recovers the ~4.9 MB live memory
|
||||
* delta and shrinks the IPC payload proportionally to function-scope
|
||||
* binding density.
|
||||
*
|
||||
* **Phase 9 reversion:** when a downstream consumer of function-scope
|
||||
* bindings lands, revert by (a) changing the loop in `runParseJob` below
|
||||
* from `typeEnv.fileScope()` back to `typeEnv.allScopes()`, (b) emitting
|
||||
* three-element tuples `[scope, varName, typeName]`, (c) widening this
|
||||
* `bindings` field back to `[string, string, string][]`, and (d) updating
|
||||
* the pipeline adapter in `pipeline.ts` to unpack three elements. The
|
||||
* sequential path's `flush()` is unchanged and still writes all scopes
|
||||
* into the accumulator — it preserves a working sample of higher-quality
|
||||
* bindings for Phase 9 prototyping today.
|
||||
*
|
||||
* Field is not renamed pending that reversion to keep downstream merges
|
||||
* across `ParseWorkerResult` / `ProcessingResult` / pipeline adapter
|
||||
* mechanically trivial. */
|
||||
export interface FileAllScopeBindings {
|
||||
filePath: string;
|
||||
/** [scope, varName, typeName] triples from all scopes. */
|
||||
bindings: [string, string, string][];
|
||||
/** [varName, typeName] pairs from the file scope only. */
|
||||
bindings: [string, string][];
|
||||
}
|
||||
|
||||
export interface ParseWorkerResult {
|
||||
|
|
@ -1388,15 +1411,21 @@ const processFileGroup = (
|
|||
});
|
||||
}
|
||||
|
||||
// Serialize all scopes for BindingAccumulator (file-scope entries are included
|
||||
// with scope = '' and consumed by ExportedTypeMap enrichment in pipeline.ts).
|
||||
const allScopes = typeEnv.allScopes();
|
||||
if (allScopes.size > 0) {
|
||||
const scopeBindings: [string, string, string][] = [];
|
||||
for (const [scope, scopeMap] of allScopes) {
|
||||
for (const [varName, typeName] of scopeMap) {
|
||||
scopeBindings.push([scope, varName, typeName]);
|
||||
}
|
||||
// Serialize file-scope bindings for BindingAccumulator. These feed the
|
||||
// ExportedTypeMap enrichment loop in pipeline.ts — the only current
|
||||
// consumer of worker-path binding data.
|
||||
//
|
||||
// PR #743 review Critical finding: we previously serialized all scopes
|
||||
// (`typeEnv.allScopes()`), which pushed ~4.9 MB of function-scope
|
||||
// bindings across the IPC boundary on every worker batch with zero
|
||||
// downstream readers. Narrowing to `fileScope()` recovers that cost.
|
||||
// See the `FileAllScopeBindings` JSDoc above for the Phase 9 reversion
|
||||
// path when a function-scope consumer lands.
|
||||
const fileScope = typeEnv.fileScope();
|
||||
if (fileScope.size > 0) {
|
||||
const scopeBindings: [string, string][] = [];
|
||||
for (const [varName, typeName] of fileScope) {
|
||||
scopeBindings.push([varName, typeName]);
|
||||
}
|
||||
result.allScopeBindings.push({ filePath: file.path, bindings: scopeBindings });
|
||||
}
|
||||
|
|
|
|||
|
|
@ -152,26 +152,27 @@ describe('BindingAccumulator', () => {
|
|||
it('deserializes allScopeBindings from worker into accumulator', () => {
|
||||
const acc = new BindingAccumulator();
|
||||
|
||||
// Simulated worker output (allScopeBindings format: [scope, varName, typeName])
|
||||
// Simulated worker output — PR #743 review follow-up:
|
||||
// After narrowing the worker IPC payload to file-scope only, the
|
||||
// emitted tuple shape is [varName, typeName]. Function-scope entries
|
||||
// are stripped at the parse-worker boundary; the sequential path's
|
||||
// flush() still writes all scopes via its own code path.
|
||||
const workerBindings = [
|
||||
{
|
||||
filePath: 'src/service.ts',
|
||||
bindings: [
|
||||
['', 'config', 'Config'] as [string, string, string],
|
||||
['handleRequest@15', 'db', 'Database'] as [string, string, string],
|
||||
['handleRequest@15', 'result', 'QueryResult'] as [string, string, string],
|
||||
],
|
||||
bindings: [['config', 'Config'] as [string, string]],
|
||||
},
|
||||
{
|
||||
filePath: 'src/utils.ts',
|
||||
bindings: [['', 'logger', 'Logger'] as [string, string, string]],
|
||||
bindings: [['logger', 'Logger'] as [string, string]],
|
||||
},
|
||||
];
|
||||
|
||||
// Pipeline deserialization logic (mirrors pipeline.ts)
|
||||
// Pipeline deserialization logic (mirrors pipeline.ts adapter):
|
||||
// two-element tuples → BindingEntry with hard-coded scope: ''.
|
||||
for (const { filePath, bindings } of workerBindings) {
|
||||
const entries = bindings.map(([scope, varName, typeName]) => ({
|
||||
scope,
|
||||
const entries: BindingEntry[] = bindings.map(([varName, typeName]) => ({
|
||||
scope: '',
|
||||
varName,
|
||||
typeName,
|
||||
}));
|
||||
|
|
@ -180,21 +181,321 @@ describe('BindingAccumulator', () => {
|
|||
acc.finalize();
|
||||
|
||||
expect(acc.fileCount).toBe(2);
|
||||
expect(acc.totalBindings).toBe(4);
|
||||
expect(acc.totalBindings).toBe(2);
|
||||
|
||||
// fileScopeEntries backward compat (what ExportedTypeMap enrichment uses)
|
||||
// fileScopeEntries — what the ExportedTypeMap enrichment loop uses.
|
||||
expect(acc.fileScopeEntries('src/service.ts')).toEqual([['config', 'Config']]);
|
||||
expect(acc.fileScopeEntries('src/utils.ts')).toEqual([['logger', 'Logger']]);
|
||||
|
||||
// All-scope access (what Phase 9 will use)
|
||||
// Every entry produced by the worker path has scope === '' after the
|
||||
// IPC narrowing — locks the contract in place.
|
||||
const serviceEntries = acc.getFile('src/service.ts');
|
||||
expect(serviceEntries).toHaveLength(3);
|
||||
const dbEntry = serviceEntries!.find((e) => e.varName === 'db');
|
||||
expect(dbEntry).toEqual({
|
||||
scope: 'handleRequest@15',
|
||||
varName: 'db',
|
||||
typeName: 'Database',
|
||||
expect(serviceEntries).toHaveLength(1);
|
||||
expect(serviceEntries![0]).toEqual({
|
||||
scope: '',
|
||||
varName: 'config',
|
||||
typeName: 'Config',
|
||||
});
|
||||
});
|
||||
|
||||
it('worker IPC payload contains ONLY file-scope entries (narrowing guard)', () => {
|
||||
// PR #743 review Critical finding: function-scope bindings were being
|
||||
// serialized over worker IPC with no consumer, costing ~4.9 MB. The
|
||||
// worker now uses typeEnv.fileScope() instead of typeEnv.allScopes(),
|
||||
// so `handleRequest@15 → db: Database` never crosses the IPC boundary.
|
||||
//
|
||||
// This test simulates a TypeEnvironment that HAD both file-scope and
|
||||
// function-scope bindings (as would be produced by a realistic file),
|
||||
// then asserts the worker IPC payload contains only the file-scope
|
||||
// ones. If a future change accidentally re-broadens the worker loop
|
||||
// to `allScopes()`, this assertion fires.
|
||||
const simulatedFileScope = new Map<string, string>([
|
||||
['config', 'Config'],
|
||||
['db', 'Database'],
|
||||
]);
|
||||
// Function-scope entries that must NOT appear in the worker payload.
|
||||
const simulatedFunctionScope = new Map<string, string>([
|
||||
['localRequest', 'Request'],
|
||||
['localUser', 'User'],
|
||||
]);
|
||||
|
||||
// Mirror the parse-worker loop (post-narrowing shape):
|
||||
// const fileScope = typeEnv.fileScope();
|
||||
// for (const [varName, typeName] of fileScope) {
|
||||
// scopeBindings.push([varName, typeName]);
|
||||
// }
|
||||
const workerPayload: [string, string][] = [];
|
||||
for (const [varName, typeName] of simulatedFileScope) {
|
||||
workerPayload.push([varName, typeName]);
|
||||
}
|
||||
|
||||
// Verify: the simulated function-scope variables are never pushed.
|
||||
const allVarNames = workerPayload.map(([v]) => v);
|
||||
expect(allVarNames).toEqual(['config', 'db']);
|
||||
expect(allVarNames).not.toContain('localRequest');
|
||||
expect(allVarNames).not.toContain('localUser');
|
||||
|
||||
// Sanity: simulatedFunctionScope exists so the test is not trivially
|
||||
// vacuous — it documents what the old allScopes() path would have
|
||||
// emitted and what the new fileScope() path deliberately excludes.
|
||||
expect(simulatedFunctionScope.size).toBe(2);
|
||||
|
||||
// Round-trip through the accumulator with the pipeline adapter shape.
|
||||
const acc = new BindingAccumulator();
|
||||
const entries: BindingEntry[] = workerPayload.map(([varName, typeName]) => ({
|
||||
scope: '',
|
||||
varName,
|
||||
typeName,
|
||||
}));
|
||||
acc.appendFile('src/service.ts', entries);
|
||||
acc.finalize();
|
||||
|
||||
const stored = acc.getFile('src/service.ts');
|
||||
expect(stored).toHaveLength(2);
|
||||
// All accumulator entries from the worker path have scope === ''.
|
||||
for (const entry of stored!) {
|
||||
expect(entry.scope).toBe('');
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
// -------------------------------------------------------------------------
|
||||
// PR #743 review Low finding #1: fileScopeEntries() must be O(n_file_scope),
|
||||
// not O(n_total). Storage is split into _allByFile + _fileScopeByFile so
|
||||
// reads skip function-scope entries entirely.
|
||||
// -------------------------------------------------------------------------
|
||||
|
||||
describe('storage split (fast-path fileScopeEntries)', () => {
|
||||
it('mixed file-scope and function-scope input: fileScopeEntries ignores function-scope', () => {
|
||||
const acc = new BindingAccumulator();
|
||||
acc.appendFile('src/a.ts', [
|
||||
{ scope: '', varName: 'file1', typeName: 'T1' },
|
||||
{ scope: 'fn@10', varName: 'local1', typeName: 'L1' },
|
||||
{ scope: '', varName: 'file2', typeName: 'T2' },
|
||||
{ scope: 'fn@20', varName: 'local2', typeName: 'L2' },
|
||||
{ scope: 'fn@30', varName: 'local3', typeName: 'L3' },
|
||||
]);
|
||||
|
||||
// fileScopeEntries returns exactly the two file-scope entries,
|
||||
// preserving insertion order.
|
||||
expect(acc.fileScopeEntries('src/a.ts')).toEqual([
|
||||
['file1', 'T1'],
|
||||
['file2', 'T2'],
|
||||
]);
|
||||
|
||||
// getFile still returns all 5 entries (mixed scopes preserved).
|
||||
expect(acc.getFile('src/a.ts')).toHaveLength(5);
|
||||
});
|
||||
|
||||
it('only-function-scope file: fileScopeEntries returns [] but files() still lists it', () => {
|
||||
const acc = new BindingAccumulator();
|
||||
acc.appendFile('src/only-fn.ts', [
|
||||
{ scope: 'fn@5', varName: 'x', typeName: 'X' },
|
||||
{ scope: 'fn@10', varName: 'y', typeName: 'Y' },
|
||||
]);
|
||||
|
||||
expect(acc.fileScopeEntries('src/only-fn.ts')).toEqual([]);
|
||||
expect(acc.getFile('src/only-fn.ts')).toHaveLength(2);
|
||||
expect([...acc.files()]).toContain('src/only-fn.ts');
|
||||
expect(acc.fileCount).toBe(1);
|
||||
});
|
||||
|
||||
it('multiple appends accumulate in both maps consistently', () => {
|
||||
const acc = new BindingAccumulator();
|
||||
acc.appendFile('src/a.ts', [
|
||||
{ scope: '', varName: 'x', typeName: 'X' },
|
||||
{ scope: 'fn@1', varName: 'y', typeName: 'Y' },
|
||||
]);
|
||||
acc.appendFile('src/a.ts', [
|
||||
{ scope: '', varName: 'z', typeName: 'Z' },
|
||||
{ scope: 'fn@2', varName: 'w', typeName: 'W' },
|
||||
]);
|
||||
|
||||
expect(acc.fileScopeEntries('src/a.ts')).toEqual([
|
||||
['x', 'X'],
|
||||
['z', 'Z'],
|
||||
]);
|
||||
expect(acc.getFile('src/a.ts')).toHaveLength(4);
|
||||
expect(acc.totalBindings).toBe(4);
|
||||
});
|
||||
|
||||
it('performance guard: fileScopeEntries does not walk function-scope entries', () => {
|
||||
const acc = new BindingAccumulator();
|
||||
// 1 file-scope entry + 1000 function-scope entries.
|
||||
const entries: BindingEntry[] = [{ scope: '', varName: 'shared', typeName: 'Shared' }];
|
||||
for (let i = 0; i < 1000; i++) {
|
||||
entries.push({
|
||||
scope: `fn${i}@${i * 10}`,
|
||||
varName: `local${i}`,
|
||||
typeName: 'Local',
|
||||
});
|
||||
}
|
||||
acc.appendFile('src/big.ts', entries);
|
||||
|
||||
// fileScopeEntries returns the single file-scope pair without
|
||||
// iterating the 1000 function-scope entries — this is the O(1) cache
|
||||
// lookup behavior guaranteed by the storage split.
|
||||
const result = acc.fileScopeEntries('src/big.ts');
|
||||
expect(result).toHaveLength(1);
|
||||
expect(result[0]).toEqual(['shared', 'Shared']);
|
||||
// Sanity: getFile still sees everything.
|
||||
expect(acc.getFile('src/big.ts')).toHaveLength(1001);
|
||||
});
|
||||
});
|
||||
|
||||
// -------------------------------------------------------------------------
|
||||
// PR #743 review Medium finding #2: No integration test for the sequential
|
||||
// path → accumulator → ExportedTypeMap enrichment loop at pipeline.ts
|
||||
// lines 1082-1110. This test mirrors that loop inline with a minimal
|
||||
// KnowledgeGraph-shaped mock, locking in the node-ID format contract
|
||||
// (Function:{filePath}:{name}, Variable:..., Const:...). If the ID format
|
||||
// drifts for any language, this test fires.
|
||||
// -------------------------------------------------------------------------
|
||||
|
||||
describe('ExportedTypeMap enrichment (integration)', () => {
|
||||
/**
|
||||
* Minimal graph-node shape mirroring what the enrichment loop reads.
|
||||
* Matches the relevant subset of `GraphNode` in graph/types.ts — this
|
||||
* test does not depend on the full graph module.
|
||||
*/
|
||||
interface MockGraphNode {
|
||||
id: string;
|
||||
label: string; // 'Function' | 'Variable' | 'Const' | ...
|
||||
name: string;
|
||||
filePath: string;
|
||||
isExported: boolean;
|
||||
}
|
||||
|
||||
/**
|
||||
* Inline reimplementation of the enrichment loop from
|
||||
* `pipeline.ts:1082-1110`. Kept inline so this test asserts the
|
||||
* current contract — if the pipeline code is refactored, this test
|
||||
* must be updated alongside it. That coupling is intentional: the
|
||||
* purpose is to lock in the node-ID format assumption.
|
||||
*/
|
||||
function runEnrichmentLoop(
|
||||
bindingAccumulator: BindingAccumulator,
|
||||
nodesById: Map<string, MockGraphNode>,
|
||||
exportedTypeMap: Map<string, Map<string, string>>,
|
||||
): void {
|
||||
if (bindingAccumulator.fileCount === 0) return;
|
||||
for (const filePath of bindingAccumulator.files()) {
|
||||
for (const [name, type] of bindingAccumulator.fileScopeEntries(filePath)) {
|
||||
// Try Function, Variable, Const ID formats in priority order —
|
||||
// mirrors the pipeline loop exactly.
|
||||
const candidateIds = [
|
||||
`Function:${filePath}:${name}`,
|
||||
`Variable:${filePath}:${name}`,
|
||||
`Const:${filePath}:${name}`,
|
||||
];
|
||||
let matchedNode: MockGraphNode | undefined;
|
||||
for (const id of candidateIds) {
|
||||
const node = nodesById.get(id);
|
||||
if (node !== undefined) {
|
||||
matchedNode = node;
|
||||
break;
|
||||
}
|
||||
}
|
||||
if (matchedNode === undefined) continue;
|
||||
if (!matchedNode.isExported) continue;
|
||||
let fileMap = exportedTypeMap.get(filePath);
|
||||
if (fileMap === undefined) {
|
||||
fileMap = new Map();
|
||||
exportedTypeMap.set(filePath, fileMap);
|
||||
}
|
||||
fileMap.set(name, type);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
it('enriches exportedTypeMap with an exported Function node', () => {
|
||||
const acc = new BindingAccumulator();
|
||||
acc.appendFile('src/utils.ts', [
|
||||
{ scope: '', varName: 'helper', typeName: '(arg: string) => User' },
|
||||
]);
|
||||
acc.finalize();
|
||||
|
||||
const nodesById = new Map<string, MockGraphNode>([
|
||||
[
|
||||
'Function:src/utils.ts:helper',
|
||||
{
|
||||
id: 'Function:src/utils.ts:helper',
|
||||
label: 'Function',
|
||||
name: 'helper',
|
||||
filePath: 'src/utils.ts',
|
||||
isExported: true,
|
||||
},
|
||||
],
|
||||
]);
|
||||
const exportedTypeMap = new Map<string, Map<string, string>>();
|
||||
|
||||
runEnrichmentLoop(acc, nodesById, exportedTypeMap);
|
||||
|
||||
expect(exportedTypeMap.get('src/utils.ts')?.get('helper')).toBe('(arg: string) => User');
|
||||
});
|
||||
|
||||
it('skips non-exported Variable nodes', () => {
|
||||
const acc = new BindingAccumulator();
|
||||
acc.appendFile('src/app.ts', [{ scope: '', varName: 'dbClient', typeName: 'Database' }]);
|
||||
acc.finalize();
|
||||
|
||||
const nodesById = new Map<string, MockGraphNode>([
|
||||
[
|
||||
'Variable:src/app.ts:dbClient',
|
||||
{
|
||||
id: 'Variable:src/app.ts:dbClient',
|
||||
label: 'Variable',
|
||||
name: 'dbClient',
|
||||
filePath: 'src/app.ts',
|
||||
isExported: false, // NOT exported
|
||||
},
|
||||
],
|
||||
]);
|
||||
const exportedTypeMap = new Map<string, Map<string, string>>();
|
||||
|
||||
runEnrichmentLoop(acc, nodesById, exportedTypeMap);
|
||||
|
||||
// Non-exported → enrichment loop's isExported check filters it out.
|
||||
expect(exportedTypeMap.has('src/app.ts')).toBe(false);
|
||||
});
|
||||
|
||||
it('enriches exportedTypeMap with an exported Const node', () => {
|
||||
const acc = new BindingAccumulator();
|
||||
acc.appendFile('src/config.ts', [{ scope: '', varName: 'API_URL', typeName: 'string' }]);
|
||||
acc.finalize();
|
||||
|
||||
const nodesById = new Map<string, MockGraphNode>([
|
||||
[
|
||||
'Const:src/config.ts:API_URL',
|
||||
{
|
||||
id: 'Const:src/config.ts:API_URL',
|
||||
label: 'Const',
|
||||
name: 'API_URL',
|
||||
filePath: 'src/config.ts',
|
||||
isExported: true,
|
||||
},
|
||||
],
|
||||
]);
|
||||
const exportedTypeMap = new Map<string, Map<string, string>>();
|
||||
|
||||
runEnrichmentLoop(acc, nodesById, exportedTypeMap);
|
||||
|
||||
expect(exportedTypeMap.get('src/config.ts')?.get('API_URL')).toBe('string');
|
||||
});
|
||||
|
||||
it('silently skips accumulator entries with no matching graph node', () => {
|
||||
const acc = new BindingAccumulator();
|
||||
acc.appendFile('src/missing.ts', [{ scope: '', varName: 'ghost', typeName: 'Ghost' }]);
|
||||
acc.finalize();
|
||||
|
||||
// Empty graph — no nodes at any of the candidate IDs.
|
||||
const nodesById = new Map<string, MockGraphNode>();
|
||||
const exportedTypeMap = new Map<string, Map<string, string>>();
|
||||
|
||||
// Must not throw; enrichment loop's `continue` path fires for every
|
||||
// unmatched entry.
|
||||
expect(() => runEnrichmentLoop(acc, nodesById, exportedTypeMap)).not.toThrow();
|
||||
expect(exportedTypeMap.has('src/missing.ts')).toBe(false);
|
||||
});
|
||||
});
|
||||
});
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue