refactor(scope-resolution): clean up P2/P3 review residuals

P2:
- WASM dual-ownership invariant documented on ASTCache dispose:
  a Tree must live in AT MOST ONE disposing ASTCache. Native
  tree-sitter today is unaffected; WASM adoption would require
  tree.copy() or a non-disposing secondary cache.
- mro-processor C3 ordering test: pins EXTENDS-before-IMPLEMENTS
  parent grouping for classes with interleaved edge additions.
  Asserts exact MRO ['Base', 'Iface'] — a revert to single-loop
  insertion-order iteration would produce ['Iface', 'Base'] and
  fail loudly.
- cached-tree parity test: emitPythonScopeCaptures(src, path, T)
  returns identical CaptureMatch[] to emitPythonScopeCaptures(src,
  path). Pins the cache-hit path's correctness so a regression
  that silently returns stale captures would break the test.

P3:
- Dev-mode cache counters moved from captures.ts to cache-stats.ts.
  Production hot-path module no longer carries the module-global
  export surface; PROF gating behavior preserved.
- ParseOutput field rename astCache → scopeTreeCache. Clarifies
  that the surfaced cache is the persistent cross-phase one, not
  the chunk-local astCache parse-impl clears between chunks.
  Single consumer (scopeResolutionPhase) updated; no other readers.
- ASTCacheReader interface extracted. scopeResolutionPhase now
  reads the phase dep via a shared type instead of a hand-rolled
  inline structural shape that could drift from ASTCache's contract.
- graph.ts dual-index invariant enforced through writeRel/deleteRel
  private helpers instead of duplicated add/delete at 3 mutation
  sites. Adding a new mutation method only needs to call the
  helpers — forgetting to update one index becomes structurally
  impossible.

Tests: 382/382 unit (incl. 2 new), 191/191 python integration both
flag paths. tsc clean.
This commit is contained in:
Gergo Magyar 2026-04-20 17:33:04 +01:00
parent d4fd96fd79
commit 4cb46ef16a
10 changed files with 248 additions and 50 deletions

View file

@ -17,6 +17,24 @@ export const createKnowledgeGraph = (): KnowledgeGraph => {
// docs/plans/2026-04-20-002-perf-parse-heritage-mro-plan.md (Unit 1).
const relationshipsByType = new Map<RelationshipType, Map<string, GraphRelationship>>();
// Private helpers that encode the dual-index invariant in one place.
// All mutation paths (addRelationship, removeRelationship, removeNode)
// go through these — adding a new mutation method only needs to call
// writeRel / deleteRel, not remember to touch both maps.
const writeRel = (rel: GraphRelationship): void => {
relationshipMap.set(rel.id, rel);
let bucket = relationshipsByType.get(rel.type);
if (bucket === undefined) {
bucket = new Map();
relationshipsByType.set(rel.type, bucket);
}
bucket.set(rel.id, rel);
};
const deleteRel = (rel: GraphRelationship): void => {
relationshipMap.delete(rel.id);
relationshipsByType.get(rel.type)?.delete(rel.id);
};
const addNode = (node: GraphNode) => {
if (!nodeMap.has(node.id)) {
nodeMap.set(node.id, node);
@ -25,13 +43,7 @@ export const createKnowledgeGraph = (): KnowledgeGraph => {
const addRelationship = (relationship: GraphRelationship) => {
if (relationshipMap.has(relationship.id)) return;
relationshipMap.set(relationship.id, relationship);
let bucket = relationshipsByType.get(relationship.type);
if (bucket === undefined) {
bucket = new Map();
relationshipsByType.set(relationship.type, bucket);
}
bucket.set(relationship.id, relationship);
writeRel(relationship);
};
/**
@ -42,12 +54,9 @@ export const createKnowledgeGraph = (): KnowledgeGraph => {
nodeMap.delete(nodeId);
// Remove all relationships involving this node — clean up both
// indexes in lockstep so the per-type buckets never drift.
for (const [relId, rel] of relationshipMap) {
for (const rel of relationshipMap.values()) {
if (rel.sourceId === nodeId || rel.targetId === nodeId) {
relationshipMap.delete(relId);
relationshipsByType.get(rel.type)?.delete(relId);
deleteRel(rel);
}
}
return true;
@ -60,8 +69,7 @@ export const createKnowledgeGraph = (): KnowledgeGraph => {
const removeRelationship = (relationshipId: string): boolean => {
const rel = relationshipMap.get(relationshipId);
if (rel === undefined) return false;
relationshipMap.delete(relationshipId);
relationshipsByType.get(rel.type)?.delete(relationshipId);
deleteRel(rel);
return true;
};

View file

@ -1,8 +1,24 @@
import { LRUCache } from 'lru-cache';
import Parser from 'tree-sitter';
/**
* Minimal structural shape consumers need when reading Trees back
* through a phase-dependency boundary. Declared here so phases that
* receive ASTCache via `getPhaseOutput<...>` don't hand-roll their
* own inline structural types that silently drift when ASTCache's
* contract changes.
*
* Typed as `unknown` at the Tree boundary because consumers on the
* other side of the phase-output map don't share tree-sitter's type
* graph (e.g. COBOL's standalone processor).
*/
export interface ASTCacheReader {
get(filePath: string): unknown;
clear(): void;
}
// Define the interface for the Cache
export interface ASTCache {
export interface ASTCache extends ASTCacheReader {
get: (filePath: string) => Parser.Tree | undefined;
set: (filePath: string, tree: Parser.Tree) => void;
clear: () => void;
@ -17,8 +33,20 @@ export const createASTCache = (maxSize: number = 50): ASTCache => {
max: effectiveMax,
dispose: (tree) => {
try {
// NOTE: web-tree-sitter has tree.delete(); native tree-sitter trees are GC-managed.
// Keep this try/catch so we don't crash on either runtime.
// NOTE: web-tree-sitter has tree.delete(); native tree-sitter
// trees are GC-managed and .delete is absent (no-op here).
//
// Single-owner invariant (load-bearing under WASM): a given
// Parser.Tree reference must live in AT MOST ONE ASTCache
// that disposes. The parse-phase chunk-local cache clears
// between chunks; the cross-phase `scopeTreeCache` (also an
// ASTCache today) holds the same Tree by reference. Under
// native tree-sitter this is benign (dispose is a no-op).
// If/when GitNexus adopts web-tree-sitter for sequential
// parsing, the cross-phase cache must either (a) skip
// writing Trees that are already owned by a disposing cache,
// or (b) use tree.copy() per entry. Failing to pick one
// will hand freed memory to scope-resolution.
(tree as unknown as { delete?: () => void }).delete?.();
} catch (e) {
console.warn('Failed to delete tree from WASM memory', e);

View file

@ -0,0 +1,32 @@
/**
* Dev-mode counters for the cross-phase scope-captures parse cache.
*
* Gated by `PROF_SCOPE_RESOLUTION=1`. In production the module-level
* `PROF` constant is `false` and V8 folds every increment site into
* dead code, so the hot path in `captures.ts` stays branch-free.
*
* Extracted from `captures.ts` so the production hot-path module
* doesn't carry a module-global counter and its reset/export surface.
*/
export const PROF = process.env.PROF_SCOPE_RESOLUTION === '1';
let CACHE_HITS = 0;
let CACHE_MISSES = 0;
export function recordCacheHit(): void {
if (PROF) CACHE_HITS++;
}
export function recordCacheMiss(): void {
if (PROF) CACHE_MISSES++;
}
export function getPythonCaptureCacheStats(): { hits: number; misses: number } {
return { hits: CACHE_HITS, misses: CACHE_MISSES };
}
export function resetPythonCaptureCacheStats(): void {
CACHE_HITS = 0;
CACHE_MISSES = 0;
}

View file

@ -22,21 +22,7 @@ import { splitImportStatement } from './import-decomposer.js';
import { getPythonParser, getPythonScopeQuery } from './query.js';
import { synthesizeReceiverTypeBinding } from './receiver-binding.js';
import { computePythonArityMetadata } from './arity-metadata.js';
// Dev-mode counters for the parse-cache hit-rate. Gated by
// `PROF_SCOPE_RESOLUTION=1` to keep the hot path branch-free in
// production. Surfaced via `getPythonCaptureCacheStats()` so
// benchmarks / debug scripts can verify the cache is being used.
const PROF = process.env.PROF_SCOPE_RESOLUTION === '1';
let CACHE_HITS = 0;
let CACHE_MISSES = 0;
export function getPythonCaptureCacheStats(): { hits: number; misses: number } {
return { hits: CACHE_HITS, misses: CACHE_MISSES };
}
export function resetPythonCaptureCacheStats(): void {
CACHE_HITS = 0;
CACHE_MISSES = 0;
}
import { recordCacheHit, recordCacheMiss } from './cache-stats.js';
export function emitPythonScopeCaptures(
sourceText: string,
@ -51,8 +37,10 @@ export function emitPythonScopeCaptures(
let tree = cachedTree as ReturnType<ReturnType<typeof getPythonParser>['parse']> | undefined;
if (tree === undefined) {
tree = getPythonParser().parse(sourceText);
if (PROF) CACHE_MISSES++;
} else if (PROF) CACHE_HITS++;
recordCacheMiss();
} else {
recordCacheHit();
}
const rawMatches = getPythonScopeQuery().matches(tree.rootNode);
const out: CaptureMatch[] = [];

View file

@ -22,6 +22,7 @@
export { PYTHON_SCOPE_QUERY } from './query.js';
export { emitPythonScopeCaptures } from './captures.js';
export { getPythonCaptureCacheStats, resetPythonCaptureCacheStats } from './cache-stats.js';
export { interpretPythonImport, interpretPythonTypeBinding } from './interpret.js';
export { pythonMergeBindings } from './merge-bindings.js';
export { pythonArityCompatibility } from './arity.js';

View file

@ -109,13 +109,15 @@ export async function runChunkedParseAndResolve(
bindingAccumulator: BindingAccumulator;
resolutionContext: ReturnType<typeof createResolutionContext>;
usedWorkerPool: boolean;
/** AST cache populated by the sequential parse path. Empty when
/** Cross-phase tree-sitter Tree cache populated by the sequential
* parse path. Distinct from the chunk-local `astCache` used inside
* the parse loop (that one is cleared between chunks). Empty when
* every chunk ran via the worker pool (workers can't return native
* tree-sitter Trees across the MessageChannel). Downstream phases
* (e.g. scope-resolution) read from this to skip re-parsing the
* same source. See plan
* (scope-resolution) read from this to skip re-parsing the same
* source. See plan
* docs/plans/2026-04-20-002-perf-parse-heritage-mro-plan.md (Unit 4). */
astCache: ASTCache;
scopeTreeCache: ASTCache;
}> {
const ctx = createResolutionContext();
const symbolTable = ctx.model.symbols;
@ -617,6 +619,6 @@ export async function runChunkedParseAndResolve(
// sequential path already parsed. Survives chunk boundaries; the
// chunk-local `astCache` above is intentionally NOT exposed
// because parse-impl clears it between chunks.
astCache: scopeTreeCache,
scopeTreeCache,
};
}

View file

@ -65,14 +65,22 @@ export interface ParseOutput {
*/
readonly usedWorkerPool: boolean;
/**
* AST cache populated by the sequential parse path. Empty entries
* for files that ran through the worker pool (workers can't return
* native tree-sitter Trees across the MessageChannel). Downstream
* phases (scope-resolution) read from this to skip re-parsing —
* cache miss is safe and falls back to a fresh parse. See plan
* Cross-phase tree-sitter Tree cache populated by the sequential
* parse path. Separate from the chunk-local `astCache` used *inside*
* the parse phase (which is cleared between chunks) — this one
* survives the whole phase and hands Trees to scope-resolution so
* it can skip a second parse.
*
* Empty entries for files that ran through the worker pool
* (workers can't return native tree-sitter Trees across the
* MessageChannel). Cache miss is safe — consumers fall back to a
* fresh parse. See plan
* docs/plans/2026-04-20-002-perf-parse-heritage-mro-plan.md (Unit 4).
*
* Disposed by `scopeResolutionPhase` (the sole consumer) via
* `scopeTreeCache.clear()` after its extract loop finishes.
*/
readonly astCache: ASTCache;
readonly scopeTreeCache: ASTCache;
}
export const parsePhase: PipelinePhase<ParseOutput> = {

View file

@ -36,6 +36,7 @@ import { readFileContents } from '../../filesystem-walker.js';
import { runScopeResolution } from './run.js';
import { SCOPE_RESOLVERS } from './registry.js';
import { isDev } from '../../utils/env.js';
import type { ASTCacheReader } from '../../ast-cache.js';
export interface ScopeResolutionOutput {
/** True when at least one language ran. */
@ -82,9 +83,7 @@ export const scopeResolutionPhase: PipelinePhase<ScopeResolutionOutput> = {
// skip a second tree-sitter parse. Cache miss is safe (re-parses).
// Worker-mode parses leave the cache empty for those files; they
// also fall back to a fresh parse — no correctness impact.
const { astCache } = getPhaseOutput<{
astCache: { get(path: string): unknown; clear(): void };
}>(deps, 'parse');
const { scopeTreeCache } = getPhaseOutput<{ scopeTreeCache: ASTCacheReader }>(deps, 'parse');
let totalFiles = 0;
let totalImports = 0;
@ -117,7 +116,7 @@ export const scopeResolutionPhase: PipelinePhase<ScopeResolutionOutput> = {
{
graph: ctx.graph,
files,
treeCache: astCache,
treeCache: scopeTreeCache,
onWarn: (msg) => {
if (isDev) console.warn(`[scope-resolution:${lang}] ${msg}`);
},
@ -148,7 +147,7 @@ export const scopeResolutionPhase: PipelinePhase<ScopeResolutionOutput> = {
// never read them, and tree-sitter Trees hold native-heap memory
// under WASM runtimes. ASTCache.clear() fires the LRU dispose
// handler which calls tree.delete?.() on each retained Tree.
astCache.clear();
scopeTreeCache.clear();
if (!anyRan) return NOOP_OUTPUT;

View file

@ -1740,4 +1740,58 @@ describe('computeMRO', () => {
}
});
});
// ---- PHM parent-order pinning ------------------------------------------
//
// PHM Unit 2 split buildAdjacency's single forEachRelationship into three
// typed iterations (EXTENDS, IMPLEMENTS, HAS_METHOD). Parent enumeration
// now runs ALL EXTENDS edges before ANY IMPLEMENTS edges. For classes
// with parents added in interleaved order, this re-orders `parentMap`
// and any C3 linearization that consumes it.
//
// Python (single EXTENDS model) and Java/C# (resolveCsharpJava partitions
// by edge type regardless of order) are unaffected in practice. This
// test pins the new behavior so a future "simplification" back to a
// single loop would surface as a deliberate change rather than a silent
// semantic drift.
describe('PHM: interleaved EXTENDS + IMPLEMENTS parent ordering', () => {
it('class methods win regardless of the order EXTENDS/IMPLEMENTS edges were added', () => {
const graph = createKnowledgeGraph();
// C extends Base (class) AND implements Iface (interface). Edges
// added in INTERLEAVED order: IMPLEMENTS first, then EXTENDS.
// Under the old single-loop adjacency, parentMap[C] would be
// [IfaceId, BaseId]. Under the new grouped adjacency,
// parentMap[C] is [BaseId, IfaceId] (EXTENDS bucket first).
//
// For resolveCsharpJava, class-method-wins is invariant to parent
// order — both produce the same winner. This test encodes that
// invariant, guarding the behavioral claim that 'Java/C# are
// unaffected' in the PHM commit message.
addClass(graph, 'Base', 'java');
addClass(graph, 'C', 'java');
addClass(graph, 'Iface', 'java', 'Interface');
addMethod(graph, 'Base', 'greet');
addMethod(graph, 'Iface', 'greet', 'Interface');
addMethod(graph, 'C', 'greet');
// Add IMPLEMENTS BEFORE EXTENDS to exercise interleaving.
addImplements(graph, 'C', 'Iface');
addExtends(graph, 'C', 'Base');
const result = computeMRO(graph);
const cId = generateId('Class', 'C');
const entry = result.entries.find((e) => e.classId === cId);
expect(entry).toBeDefined();
const mro = entry!.mro;
// Grouped iteration yields EXTENDS parents first. This pin fails
// loudly if a future refactor reverts the typed-bucket iteration
// to a single full-graph scan and restores insertion-order
// semantics.
// Grouped EXTENDS-before-IMPLEMENTS iteration produces this exact
// MRO for C: [Base, Iface]. A single-loop reversion would yield
// [Iface, Base] (IMPLEMENTS added first in this test).
expect(mro).toEqual(['Base', 'Iface']);
});
});
});

View file

@ -0,0 +1,78 @@
/**
* Parity guard for the cross-phase tree cache (PHM Unit 5).
*
* `emitPythonScopeCaptures(src, path)` re-parses internally;
* `emitPythonScopeCaptures(src, path, cachedTree)` skips the parse. The
* two paths MUST return identical `CaptureMatch[]`. A future change
* that (a) mutates Trees before caching, (b) conditionally branches
* the capture query on cached vs fresh Trees, or (c) leaks state
* through module-level caches would break this — and no other test
* today asserts the equivalence.
*
* Keeps the wins from the cache-hit path honest.
*/
import { describe, it, expect } from 'vitest';
import {
emitPythonScopeCaptures,
resetPythonCaptureCacheStats,
getPythonCaptureCacheStats,
} from '../../../../src/core/ingestion/languages/python/index.js';
import { getPythonParser } from '../../../../src/core/ingestion/languages/python/query.js';
const FIXTURE = `
from typing import List
class Base:
def greet(self) -> str:
return "hi"
class Child(Base):
def shout(self, items: List[str]) -> None:
for item in items:
print(item.upper())
def top(c: Child) -> None:
c.greet()
c.shout([])
`;
function normalizeCaptures(caps: readonly Record<string, unknown>[]): unknown[] {
// CaptureMatch is a Record<tag, Capture>. Compare by structural JSON
// so Node references don't create false negatives.
return caps.map((m) => {
const out: Record<string, unknown> = {};
for (const [tag, cap] of Object.entries(m)) {
const c = cap as { range?: unknown; text?: unknown };
out[tag] = { range: c.range, text: c.text };
}
return out;
});
}
describe('emitPythonScopeCaptures cache-hit parity', () => {
it('returns identical captures whether cachedTree is supplied or not', () => {
const fresh = emitPythonScopeCaptures(FIXTURE, 'fixture.py');
const tree = getPythonParser().parse(FIXTURE);
const cached = emitPythonScopeCaptures(FIXTURE, 'fixture.py', tree);
expect(cached).toHaveLength(fresh.length);
expect(normalizeCaptures(cached)).toEqual(normalizeCaptures(fresh));
});
it('counters stay at zero baseline after reset regardless of whether PROF is active', () => {
// The PROF gate is evaluated at module load, so we can't toggle
// counters on mid-test. What we CAN assert deterministically is
// that reset zeros the counters and repeated reads yield the same
// zeroed snapshot (counter API shape invariant).
resetPythonCaptureCacheStats();
expect(getPythonCaptureCacheStats()).toEqual({ hits: 0, misses: 0 });
// Running the emit path should not mutate the counters unless PROF
// was on at module load. Whichever state, calling reset again must
// return to zero.
const tree = getPythonParser().parse(FIXTURE);
emitPythonScopeCaptures(FIXTURE, 'fixture.py', tree);
emitPythonScopeCaptures(FIXTURE, 'fixture.py');
resetPythonCaptureCacheStats();
expect(getPythonCaptureCacheStats()).toEqual({ hits: 0, misses: 0 });
});
});