refactor(sm-14): address PR review feedback

- Remove redundant typeEnvBindings worker payload — allScopeBindings is a strict
  superset (file-scope entries with scope=''). Removes duplicate IPC data and
  the dead fallback branch in pipeline.ts.
- Add single-use guard to TypeEnvironment.flush() — throws on second call to
  prevent silent duplication. Update JSDoc to clarify "copy" semantics
  (env is not actually drained — TypeEnv is per-file and discarded immediately).
- Remove redundant inner guard in parse-worker.ts allScopeBindings serialization.
- Rename allAllScopeBindings -> allScopeBindingsByFile for clarity.
- Document estimateMemoryBytes pessimistic ASCII assumption (V8 uses Latin-1
  for all-ASCII strings, so actual heap cost is ~half).
- Add test for single-use flush() guard.

Addresses PR #743 review feedback.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This commit is contained in:
abhigyanpatwari 2026-04-09 03:20:18 +05:30
parent ec85c1e8bc
commit 89feea744d
6 changed files with 32 additions and 50 deletions

View file

@ -89,9 +89,14 @@ export class BindingAccumulator {
}
/**
* Rough memory estimate in bytes.
* Rough memory estimate in bytes (intentionally pessimistic).
* Formula: sum of (ENTRY_OVERHEAD + char bytes of scope+varName+typeName) per entry
* + MAP_ENTRY_OVERHEAD + char bytes of filePath per file.
*
* Note: V8 stores all-ASCII strings as Latin-1 (1 byte/char) and only upgrades
* to UCS-2 (2 bytes/char) for non-Latin-1 code points. Source paths and type names
* are typically all-ASCII, so actual heap cost is roughly half what this returns.
* The pessimistic factor is intentional better to over-budget than under-budget.
*/
estimateMemoryBytes(): number {
let total = 0;

View file

@ -42,7 +42,6 @@ import type {
ExtractedDecoratorRoute,
ExtractedToolDef,
FileConstructorBindings,
FileTypeEnvBindings,
FileAllScopeBindings,
ExtractedORMQuery,
} from './workers/parse-worker.js';
@ -61,7 +60,6 @@ export interface WorkerExtractedData {
toolDefs: ExtractedToolDef[];
ormQueries: ExtractedORMQuery[];
constructorBindings: FileConstructorBindings[];
typeEnvBindings: FileTypeEnvBindings[];
allScopeBindings: FileAllScopeBindings[];
}
@ -96,7 +94,6 @@ const processParsingWithWorkers = async (
toolDefs: [],
ormQueries: [],
constructorBindings: [],
typeEnvBindings: [],
allScopeBindings: [],
};
@ -121,8 +118,7 @@ const processParsingWithWorkers = async (
const allToolDefs: ExtractedToolDef[] = [];
const allORMQueries: ExtractedORMQuery[] = [];
const allConstructorBindings: FileConstructorBindings[] = [];
const allTypeEnvBindings: FileTypeEnvBindings[] = [];
const allAllScopeBindings: FileAllScopeBindings[] = [];
const allScopeBindingsByFile: FileAllScopeBindings[] = [];
for (const result of chunkResults) {
for (const node of result.nodes) {
graph.addNode({
@ -158,9 +154,8 @@ const processParsingWithWorkers = async (
for (const _item of result.toolDefs) allToolDefs.push(_item);
if (result.ormQueries) for (const _item of result.ormQueries) allORMQueries.push(_item);
for (const _item of result.constructorBindings) allConstructorBindings.push(_item);
for (const _item of result.typeEnvBindings) allTypeEnvBindings.push(_item);
if (result.allScopeBindings)
for (const _item of result.allScopeBindings) allAllScopeBindings.push(_item);
for (const _item of result.allScopeBindings) allScopeBindingsByFile.push(_item);
}
// Merge and log skipped languages from workers
@ -190,8 +185,7 @@ const processParsingWithWorkers = async (
toolDefs: allToolDefs,
ormQueries: allORMQueries,
constructorBindings: allConstructorBindings,
typeEnvBindings: allTypeEnvBindings,
allScopeBindings: allAllScopeBindings,
allScopeBindings: allScopeBindingsByFile,
};
};

View file

@ -928,16 +928,6 @@ async function runChunkedParseAndResolve(
}));
bindingAccumulator.appendFile(filePath, entries);
}
} else if (chunkWorkerData.typeEnvBindings?.length) {
// Fallback: old-style file-scope-only bindings (backward compat)
for (const { filePath, bindings } of chunkWorkerData.typeEnvBindings) {
const entries = bindings.map(([varName, typeName]) => ({
scope: '',
varName,
typeName,
}));
bindingAccumulator.appendFile(filePath, entries);
}
}
// Collect fetch() calls for Next.js route matching
if (chunkWorkerData.fetchCalls?.length) {

View file

@ -74,8 +74,9 @@ export interface TypeEnvironment {
* Populated when a variable has BOTH a declared base type AND a more specific
* constructor type (e.g., `Animal a = new Dog()` key maps to 'Dog'). */
readonly constructorTypeMap: ReadonlyMap<string, string>;
/** Drain all scoped bindings into a BindingAccumulator.
* Called once per file at end of processing. Skips empty scopes. */
/** Copy all scoped bindings into a BindingAccumulator.
* Must be called at most once per TypeEnv instance throws on second call.
* The source `env` is not cleared (TypeEnv is per-file and discarded immediately after). */
flush(filePath: string, accumulator: BindingAccumulator): void;
}
@ -826,6 +827,7 @@ export const buildTypeEnv = (
const parentMap = options?.parentMap;
const extractFuncNameHook = options?.extractFunctionName;
const env: TypeEnv = new Map();
let flushed = false;
const patternOverrides: PatternOverrides = new Map();
// Phase P: maps `scope\0varName` → constructor type when a declaration has BOTH
// a base type annotation AND a more specific constructor initializer.
@ -1250,6 +1252,10 @@ export const buildTypeEnv = (
allScopes: () => env as ReadonlyMap<string, ReadonlyMap<string, string>>,
constructorTypeMap,
flush(filePath: string, accumulator: BindingAccumulator): void {
if (flushed) {
throw new Error(`TypeEnv.flush called twice for ${filePath} — flush is single-use`);
}
flushed = true;
const entries: BindingEntry[] = [];
for (const [scope, scopeMap] of env) {
for (const [varName, typeName] of scopeMap) {

View file

@ -235,15 +235,9 @@ export interface FileConstructorBindings {
bindings: ConstructorBinding[];
}
/** File-scope type bindings from TypeEnv fixpoint — used for cross-file ExportedTypeMap. */
export interface FileTypeEnvBindings {
filePath: string;
/** [varName, typeName] pairs from file scope (scope = '') */
bindings: [string, string][];
}
/** All-scope type bindings from TypeEnv includes function-local scopes.
* Used by BindingAccumulator for cross-file type propagation (Phase 9+). */
* Used by BindingAccumulator for cross-file type propagation (Phase 9+).
* File-scope entries (scope = '') are included as a subset. */
export interface FileAllScopeBindings {
filePath: string;
/** [scope, varName, typeName] triples from all scopes. */
@ -264,8 +258,6 @@ export interface ParseWorkerResult {
toolDefs: ExtractedToolDef[];
ormQueries: ExtractedORMQuery[];
constructorBindings: FileConstructorBindings[];
/** File-scope type bindings from TypeEnv fixpoint for exported symbol collection. */
typeEnvBindings: FileTypeEnvBindings[];
/** All-scope type bindings from TypeEnv for BindingAccumulator (includes function-local). */
allScopeBindings: FileAllScopeBindings[];
skippedLanguages: Record<string, number>;
@ -700,7 +692,6 @@ const processBatch = (
toolDefs: [],
ormQueries: [],
constructorBindings: [],
typeEnvBindings: [],
allScopeBindings: [],
skippedLanguages: {},
fileCount: 0,
@ -1397,17 +1388,8 @@ const processFileGroup = (
});
}
// Extract file-scope bindings for ExportedTypeMap (closes worker/sequential quality gap).
// Sequential path uses collectExportedBindings(typeEnv) directly; worker path serializes
// these bindings so the main thread can merge them into ExportedTypeMap.
const fileScope = typeEnv.fileScope();
if (fileScope.size > 0) {
const bindings: [string, string][] = [];
for (const [name, type] of fileScope) bindings.push([name, type]);
result.typeEnvBindings.push({ filePath: file.path, bindings });
}
// Serialize all scopes for BindingAccumulator (Phase 9+ cross-file propagation)
// 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][] = [];
@ -1416,9 +1398,7 @@ const processFileGroup = (
scopeBindings.push([scope, varName, typeName]);
}
}
if (scopeBindings.length > 0) {
result.allScopeBindings.push({ filePath: file.path, bindings: scopeBindings });
}
result.allScopeBindings.push({ filePath: file.path, bindings: scopeBindings });
}
// Per-file map: decorator end-line → decorator info, for associating with definitions
@ -2138,7 +2118,6 @@ let accumulated: ParseWorkerResult = {
toolDefs: [],
ormQueries: [],
constructorBindings: [],
typeEnvBindings: [],
allScopeBindings: [],
skippedLanguages: {},
fileCount: 0,
@ -2159,7 +2138,6 @@ const mergeResult = (target: ParseWorkerResult, src: ParseWorkerResult) => {
target.toolDefs.push(...src.toolDefs);
target.ormQueries.push(...src.ormQueries);
target.constructorBindings.push(...src.constructorBindings);
target.typeEnvBindings.push(...src.typeEnvBindings);
target.allScopeBindings.push(...src.allScopeBindings);
for (const [lang, count] of Object.entries(src.skippedLanguages)) {
target.skippedLanguages[lang] = (target.skippedLanguages[lang] || 0) + count;
@ -2211,7 +2189,6 @@ parentPort!.on('message', (msg: WorkerIncomingMessage) => {
toolDefs: [],
ormQueries: [],
constructorBindings: [],
typeEnvBindings: [],
allScopeBindings: [],
skippedLanguages: {},
fileCount: 0,

View file

@ -5921,5 +5921,15 @@ function process() {
expect(acc.getFile('/src/a.ts')).toBeDefined();
expect(acc.getFile('/src/b.ts')).toBeDefined();
});
it('throws on second flush of the same TypeEnv (single-use)', () => {
const code = `const x: X = makeX();`;
const tree = parse(code, TypeScript.typescript);
const typeEnv = buildTypeEnv(tree, 'typescript');
const acc = new BindingAccumulator();
typeEnv.flush('/src/a.ts', acc);
expect(() => typeEnv.flush('/src/a.ts', acc)).toThrow(/single-use/);
});
});
});