fix(scope): address TypeScript review follow-ups

Made-with: Cursor
This commit is contained in:
Gergo Magyar 2026-04-25 17:44:30 +01:00
parent eeacd57822
commit 17cfe11daa
8 changed files with 147 additions and 26 deletions

View file

@ -194,7 +194,8 @@ export function finalize(input: FinalizeInput, hooks: FinalizeHooks): FinalizeOu
graph.set(file.filePath, new Set());
}
for (const [fromFile, drafts] of edgeIndex) {
const edges = graph.get(fromFile)!;
const edges = graph.get(fromFile);
if (edges === undefined) continue;
for (const d of drafts) {
if (d.targetFile !== null && byFilePath.has(d.targetFile)) {
edges.add(d.targetFile);
@ -231,7 +232,8 @@ export function finalize(input: FinalizeInput, hooks: FinalizeHooks): FinalizeOu
progressed = false;
iterations++;
for (const filePath of scc.files) {
const drafts = edgeIndex.get(filePath)!;
const drafts = edgeIndex.get(filePath);
if (drafts === undefined) continue;
for (const draft of drafts) {
if (draft.finalized !== null) continue;
const finalized = tryFinalize(draft, byFilePath, reexportClosures);
@ -245,7 +247,8 @@ export function finalize(input: FinalizeInput, hooks: FinalizeHooks): FinalizeOu
// Any drafts still not finalized within this SCC hit the cap → unresolved.
for (const filePath of scc.files) {
const drafts = edgeIndex.get(filePath)!;
const drafts = edgeIndex.get(filePath);
if (drafts === undefined) continue;
for (const draft of drafts) {
if (draft.finalized !== null) continue;
draft.finalized = {
@ -259,10 +262,14 @@ export function finalize(input: FinalizeInput, hooks: FinalizeHooks): FinalizeOu
// ── Phase 4: collect finalized `ImportEdge[]` per module scope, preserving
// input order within each file, and wildcard-expand where applicable.
for (const file of input.files) {
const drafts = edgeIndex.get(file.filePath)!;
const drafts = edgeIndex.get(file.filePath);
if (drafts === undefined) continue;
const finalized: ImportEdge[] = [];
for (const d of drafts) {
const edge = d.finalized!;
const edge = d.finalized;
if (edge === null) {
throw new Error(`Invariant violated: import edge was not finalized for ${file.filePath}`);
}
if (d.source.kind === 'wildcard' && edge.linkStatus !== 'unresolved') {
// Produce one `wildcard-expanded` ImportEdge per exported name.
const expanded = expandWildcard(edge, byFilePath, hooks, input.workspaceIndex);
@ -581,7 +588,10 @@ function buildReexportClosures(
// singletons settle in one pass; cyclic SCCs run a bounded fixpoint.
for (const scc of subSccs) {
if (!scc.isCycle) {
populateFileClosure(scc.files[0]!, byFilePath, edgeIndex, closures);
const filePath = scc.files[0];
if (filePath !== undefined) {
populateFileClosure(filePath, byFilePath, edgeIndex, closures);
}
continue;
}
// Cap = |SCC| + 1. With first-wins precedence each name needs at
@ -624,7 +634,8 @@ function populateFileClosure(
edgeIndex: ReadonlyMap<string, ImportEdgeDraft[]>,
closures: Map<string, Map<string, ReexportClosureEntry>>,
): boolean {
const myClosure = closures.get(filePath)!;
const myClosure = closures.get(filePath);
if (myClosure === undefined) return false;
const before = myClosure.size;
const drafts = edgeIndex.get(filePath);
if (drafts === undefined) return false;
@ -871,7 +882,8 @@ function tarjanSccs(graph: ReadonlyMap<string, ReadonlySet<string>>): FinalizedS
entered: false,
});
while (iterStack.length > 0) {
const frame = iterStack[iterStack.length - 1]!;
const frame = iterStack[iterStack.length - 1];
if (frame === undefined) break;
if (!frame.entered) {
frame.entered = true;
@ -889,7 +901,10 @@ function tarjanSccs(graph: ReadonlyMap<string, ReadonlySet<string>>): FinalizedS
const scc: string[] = [];
let selfInCycle = false;
while (true) {
const w = stack.pop()!;
const w = stack.pop();
if (w === undefined) {
throw new Error(`Invariant violated: Tarjan stack exhausted at ${frame.node}`);
}
onStack.delete(w);
scc.push(w);
// A single-file self-loop counts as a cycle.
@ -904,8 +919,16 @@ function tarjanSccs(graph: ReadonlyMap<string, ReadonlySet<string>>): FinalizedS
iterStack.pop();
// Propagate lowlink to parent.
if (iterStack.length > 0) {
const parent = iterStack[iterStack.length - 1]!;
lowlink.set(parent.node, Math.min(lowlink.get(parent.node)!, lowlink.get(frame.node)!));
const parent = iterStack[iterStack.length - 1];
if (parent !== undefined) {
lowlink.set(
parent.node,
Math.min(
requiredNumber(lowlink, parent.node, 'lowlink'),
requiredNumber(lowlink, frame.node, 'lowlink'),
),
);
}
}
continue;
}
@ -918,10 +941,24 @@ function tarjanSccs(graph: ReadonlyMap<string, ReadonlySet<string>>): FinalizedS
entered: false,
});
} else if (onStack.has(child)) {
lowlink.set(frame.node, Math.min(lowlink.get(frame.node)!, index.get(child)!));
lowlink.set(
frame.node,
Math.min(
requiredNumber(lowlink, frame.node, 'lowlink'),
requiredNumber(index, child, 'index'),
),
);
}
}
}
return sccs;
}
function requiredNumber(map: ReadonlyMap<string, number>, key: string, label: string): number {
const value = map.get(key);
if (value === undefined) {
throw new Error(`Invariant violated: missing Tarjan ${label} for ${key}`);
}
return value;
}

View file

@ -207,6 +207,10 @@ export const typescriptProvider = defineLanguage({
interpretTypeBinding: interpretTsTypeBinding,
bindingScopeFor: tsBindingScopeFor,
importOwningScope: tsImportOwningScope,
// Merge precedence is decided from BindingRef origin + declaration
// space only. The central finalizer already calls this per (scope,
// name), so the Scope object itself intentionally does not affect
// TypeScript declaration merging.
mergeBindings: (_scope, bindings) => typescriptMergeBindings(bindings),
receiverBinding: tsReceiverBinding,
arityCompatibility: typescriptArityCompatibility,

View file

@ -192,9 +192,10 @@ function decomposeNamedSpecifier(
// alias: identifier? (only when `as` is present)
// plus an optional `type` keyword token in front (per-specifier type-only)
//
// tree-sitter-typescript exposes `name` and `alias` as named fields —
// prefer them over positional children to tolerate grammar churn.
const nameNode = spec.childForFieldName('name') ?? findFirstIdentifier(spec);
// tree-sitter-typescript exposes `name` and `alias` as named fields.
// If `name` is absent, fail closed rather than guessing positionally:
// binding the alias as the imported name would invert the edge.
const nameNode = spec.childForFieldName('name');
const aliasNode = spec.childForFieldName('alias');
if (nameNode === null) return null;
const name = nameNode.text;
@ -296,7 +297,7 @@ function decomposeReexportSpecifier(
source: string,
stmtNode: SyntaxNode,
): CaptureMatch | null {
const nameNode = spec.childForFieldName('name') ?? findFirstIdentifier(spec);
const nameNode = spec.childForFieldName('name');
const aliasNode = spec.childForFieldName('alias');
if (nameNode === null) return null;
const name = nameNode.text;
@ -404,15 +405,6 @@ function stripQuotes(raw: string): string {
return trimmed;
}
function findFirstIdentifier(node: SyntaxNode): SyntaxNode | null {
for (let i = 0; i < node.namedChildCount; i++) {
const c = node.namedChild(i);
if (c === null) continue;
if (c.type === 'identifier' || c.type === 'property_identifier') return c;
}
return null;
}
function buildImportMatch(stmtNode: SyntaxNode, spec: ImportSpec): CaptureMatch {
const m: Record<string, Capture> = {
'@import.statement': nodeToCapture('@import.statement', stmtNode),

View file

@ -76,6 +76,9 @@
* 9. **Intersection types on parameters** (`(a: A & B)`) treated
* as opaque (no strip); overload narrowing on intersections
* won't match.
* 10. **`instanceof` member-expression narrowing** only bare
* identifiers are narrowed (`user instanceof User`). Member paths
* such as `user.address instanceof Address` remain unresolved.
*
* Shadow-harness corpus parity on `test/integration/resolvers/
* typescript.test.ts` is the authoritative signal for which of these

View file

@ -58,7 +58,9 @@ import { SupportedLanguages } from 'gitnexus-shared';
* so this set also controls what gets silenced in the legacy DAG.
*
* Add a language here ONLY after shadow parity 99% fixtures / 98%
* corpus per RFC §6.4. The parity CI gate will block the PR otherwise.
* corpus per RFC §6.4. TypeScript is temporarily accepted under the
* Ring 3 CI parity gate while corpus-level shadow-mode wiring is tracked
* separately for this migration.
*
* The set is intentionally a static TypeScript literal (not a JSON import,
* not an env lookup) so CI can discover it via `tsx` without a build step

View file

@ -382,6 +382,13 @@ describe('emitTsScopeCaptures — type bindings', () => {
expect(m!['@type-binding.type'].text).toBe('User');
});
it('documents limitation: member-expression `instanceof` narrowing is not synthesized', () => {
const m = findMatch('if (user.address instanceof Address) { user.address.save(); }', (t) =>
t.includes('@type-binding.assertion'),
);
expect(m).toBeUndefined();
});
it('captures class field annotations', () => {
// Field-level @type-binding.annotation and @declaration.property fire
// as separate matches (different query patterns), not combined on one

View file

@ -18,6 +18,7 @@ import {
} from '../../../../src/core/ingestion/languages/typescript/simple-hooks.js';
import { synthesizeTsReceiverBinding } from '../../../../src/core/ingestion/languages/typescript/receiver-binding.js';
import { typescriptMergeBindings } from '../../../../src/core/ingestion/languages/typescript/merge-bindings.js';
import { typescriptProvider } from '../../../../src/core/ingestion/languages/typescript.js';
import { typescriptArityCompatibility } from '../../../../src/core/ingestion/languages/typescript/arity.js';
import { computeTsArityMetadata } from '../../../../src/core/ingestion/languages/typescript/arity-metadata.js';
import { getTsParser } from '../../../../src/core/ingestion/languages/typescript/query.js';
@ -518,6 +519,27 @@ describe('typescriptMergeBindings — declaration merging (multi-space)', () =>
});
});
describe('typescriptProvider.mergeBindings adapter', () => {
const binding = (origin: BindingRef['origin'], nodeId: string, type: NodeLabel): BindingRef =>
({
origin,
def: { nodeId, type },
}) as BindingRef;
it('is scope-id independent because finalize calls it per (scope, name)', () => {
const merge = typescriptProvider.mergeBindings;
if (merge === undefined) throw new Error('typescriptProvider.mergeBindings missing');
const importBinding = binding('import', 'I', 'Class');
const localBinding = binding('local', 'L', 'Class');
const scopeA = fakeScope({ kind: 'Module', id: 'module-a' as ScopeId });
const scopeB = fakeScope({ kind: 'Module', id: 'module-b' as ScopeId });
expect(merge(scopeA, [importBinding, localBinding])).toEqual([localBinding]);
expect(merge(scopeB, [importBinding, localBinding])).toEqual([localBinding]);
});
});
// ─── typescriptArityCompatibility ─────────────────────────────────────────
describe('typescriptArityCompatibility', () => {

View file

@ -8,11 +8,13 @@
import { describe, it, expect } from 'vitest';
import { emitTsScopeCaptures } from '../../../../src/core/ingestion/languages/typescript/captures.js';
import { splitImportStatement } from '../../../../src/core/ingestion/languages/typescript/import-decomposer.js';
import { interpretTsImport } from '../../../../src/core/ingestion/languages/typescript/interpret.js';
import {
resolveTsImportTarget,
type TsResolveContext,
} from '../../../../src/core/ingestion/languages/typescript/import-target.js';
import type { SyntaxNode } from '../../../../src/core/ingestion/utils/ast-helpers.js';
import type { ParsedImport, WorkspaceIndex } from 'gitnexus-shared';
import { SupportedLanguages } from 'gitnexus-shared';
@ -24,6 +26,27 @@ function importsFor(src: string): ParsedImport[] {
.filter((p): p is ParsedImport => p !== null);
}
function mockNode(
type: string,
text: string,
fields: Record<string, SyntaxNode | null> = {},
children: readonly SyntaxNode[] = [],
startIndex = 0,
): SyntaxNode {
return {
type,
text,
startIndex,
startPosition: { row: 0, column: startIndex },
endPosition: { row: 0, column: startIndex + text.length },
get namedChildCount() {
return children.length;
},
namedChild: (index: number) => children[index] ?? null,
childForFieldName: (name: string) => fields[name] ?? null,
} as unknown as SyntaxNode;
}
describe('interpretTsImport — static imports', () => {
it('named: `import { X } from "./a"`', () => {
const [imp, ...rest] = importsFor('import { X } from "./a";');
@ -131,6 +154,22 @@ describe('interpretTsImport — static imports', () => {
});
});
it('fails closed when an import specifier is missing its `name` field', () => {
const source = mockNode('string', '"./m"');
const alias = mockNode('identifier', 'Alias', {}, [], 12);
const spec = mockNode('import_specifier', 'Missing as Alias', { alias }, [alias]);
const named = mockNode('named_imports', '{ Missing as Alias }', {}, [spec]);
const clause = mockNode('import_clause', '{ Missing as Alias }', {}, [named]);
const stmt = mockNode(
'import_statement',
'import { Missing as Alias } from "./m";',
{ source },
[clause, source],
);
expect(splitImportStatement(stmt)).toHaveLength(0);
});
it('preserves the module path as written (no quote stripping leftovers)', () => {
const [imp] = importsFor("import X from '@scope/pkg';");
expect(imp?.targetRaw).toBe('@scope/pkg');
@ -188,6 +227,21 @@ describe('interpretTsImport — re-exports', () => {
const imps = importsFor('const X = 1; export { X };');
expect(imps).toHaveLength(0);
});
it('fails closed when a re-export specifier is missing its `name` field', () => {
const source = mockNode('string', '"./m"');
const alias = mockNode('identifier', 'Alias', {}, [], 12);
const spec = mockNode('export_specifier', 'Missing as Alias', { alias }, [alias]);
const clause = mockNode('export_clause', '{ Missing as Alias }', {}, [spec]);
const stmt = mockNode(
'export_statement',
'export { Missing as Alias } from "./m";',
{ source },
[clause, source],
);
expect(splitImportStatement(stmt)).toHaveLength(0);
});
});
describe('interpretTsImport — dynamic imports', () => {