diff --git a/gitnexus/src/core/ingestion/workers/callable-id.ts b/gitnexus/src/core/ingestion/workers/callable-id.ts new file mode 100644 index 000000000..75d6bfcdd --- /dev/null +++ b/gitnexus/src/core/ingestion/workers/callable-id.ts @@ -0,0 +1,62 @@ +/** + * The id rules for a callable nested inside another callable (#2699). + * + * Extracted from `parse-worker.ts` for one reason: **three** phases there + * build these ids independently — the definition phase + * (`callableOwnQualifiedName`), the caller-attribution phase + * (`findEnclosingFunctionId`), and the worker-path node-id derivation in + * `processFileGroup`. An id they compute differently is not a test failure; + * the caller attaches to a node that does not exist, so the edge is dropped + * rather than reported. "Zero dangling edges" is what that looks like from + * outside, which is why the divergence #2714 fixed went unnoticed. + * + * These functions are pure and free of module-scope side effects, unlike + * `parse-worker.ts`, which posts a `ready` message to `parentPort` at import + * and therefore cannot be value-imported by a unit test at all. That is what + * makes the rule testable rather than merely commented. + * + * See `parse-worker.ts`'s `enclosingCallablePrefix` for how the prefix passed + * in here is derived, and why only genuinely nested callables get one. + */ + +import type { SyntaxNode } from '../utils/ast-helpers.js'; + +/** + * A function-local callable's own name segment: its name plus its declaration + * position. + * + * The name chain alone is not enough, and the gap is the language's, not the + * grammar's: ECMAScript creates an environment record per function AND per + * block, so sibling blocks in one function hold genuinely different bindings — + * + * function outer(a) { + * if (a) { const pick = …; return pick(1); } // one binding + * else { const pick = …; return pick(2); } // a DIFFERENT binding + * } + * + * — and both are `outer.pick` by name. Putting a block token in the qualifier + * would tag every local inside any `if`, the common case, and buy nothing over + * putting the position on the declaration itself: a declaration's own position + * is unique across every environment record it could belong to, without the + * qualifier having to enumerate them. One rule, no conditionals, O(1). + * + * Applied ONLY to locals. Top-level functions and class methods keep their + * bare/class-qualified ids, which is what keeps this off the symbols other + * files, saved queries and stored references actually address. + */ +export const localIdentity = (node: SyntaxNode, name: string): string => + `${name}@${node.startPosition.row}:${node.startPosition.column}`; + +/** + * The qualified name of a callable nested inside another callable — THE single + * definition of that rule, shared by all three id-building phases. + * + * A comment asking three call sites to stay in step is exactly the invariant + * that rots; routing them through one function makes divergence require + * deleting a call rather than editing a duplicated expression. + */ +export const nestedCallableQualifiedName = ( + prefix: string, + node: SyntaxNode, + name: string, +): string => `${prefix}.${localIdentity(node, name)}`; diff --git a/gitnexus/src/core/ingestion/workers/parse-worker.ts b/gitnexus/src/core/ingestion/workers/parse-worker.ts index 3c8716020..18bdd5bc0 100644 --- a/gitnexus/src/core/ingestion/workers/parse-worker.ts +++ b/gitnexus/src/core/ingestion/workers/parse-worker.ts @@ -1,4 +1,5 @@ import { parentPort, threadId, workerData } from 'node:worker_threads'; +import { localIdentity, nestedCallableQualifiedName } from './callable-id.js'; import Parser from 'tree-sitter'; import JavaScript from 'tree-sitter-javascript'; import TypeScript from 'tree-sitter-typescript'; @@ -767,28 +768,12 @@ function getMethodInfo( * and keep their existing ids byte-for-byte, which is what bounds the id churn * this change forces. * - * `localIdentity` below completes it. The name chain alone is not enough, and - * the gap is the language's, not the grammar's: ECMAScript creates an - * environment record per function AND per block, so sibling blocks in one - * function hold genuinely different bindings — - * - * function outer(a) { - * if (a) { const pick = …; return pick(1); } // one binding - * else { const pick = …; return pick(2); } // a DIFFERENT binding - * } - * - * — and both are `outer.pick` by name. Putting a block token in the qualifier - * would tag every local inside any `if`, the common case, and buy nothing over - * putting the position on the declaration itself: a declaration's own position - * is unique across every environment record it could belong to, without the - * qualifier having to enumerate them. One rule, no conditionals, O(1). - * - * Applied ONLY to locals. Top-level functions and class methods keep their - * bare/class-qualified ids, which is what keeps this off the symbols other - * files, saved queries and stored references actually address. + * `localIdentity` completes it, and both it and the shared + * `nestedCallableQualifiedName` rule now live in `./callable-id.ts`: this + * module posts a `ready` message to `parentPort` at import, so a unit test + * cannot value-import it, and a rule three phases must agree on has to be + * testable rather than merely commented (#2714). */ -const localIdentity = (node: SyntaxNode, name: string): string => - `${name}@${node.startPosition.row}:${node.startPosition.column}`; /** * Boundary for the enclosing-callable walk (#2699). @@ -873,9 +858,9 @@ const callableOwnQualifiedName = ( if (cached !== undefined) return cached; const efnResult = provider.methodExtractor?.extractFunctionName?.(fnNode, filePath); - // An anonymous callable has no name of its own, so it IS its position — - // `localIdentity` supplies the same suffix the local branch below appends, - // and the two must not stack. + // An anonymous callable has no name of its own, so it IS its position: the + // `ownName === null` branch below carries the position INSTEAD of a name, + // never in addition to one, so the two spellings cannot stack. const ownName = efnResult?.funcName ?? genericFuncName(fnNode) ?? null; const prefix = enclosingCallablePrefix(fnNode, filePath, provider); @@ -884,12 +869,11 @@ const callableOwnQualifiedName = ( ? cachedFindEnclosingClassInfo(fnNode, filePath, provider.resolveEnclosingOwner) : null; const owner = prefix ?? classInfo?.className; - const localName = localIdentity(fnNode, ownName ?? 'fn'); const result = prefix !== undefined - ? `${prefix}.${localName}` + ? nestedCallableQualifiedName(prefix, fnNode, ownName ?? 'fn') : ownName === null - ? localName + ? localIdentity(fnNode, 'fn') : owner ? `${owner}.${ownName}` : ownName; @@ -947,15 +931,16 @@ const findEnclosingFunctionId = ( const nestedPrefix = enclosingCallablePrefix(current, filePath, provider); const ownerName = nestedPrefix ?? classInfo?.className ?? standaloneMethodInfo?.receiverType ?? undefined; - // Lockstep with the definition phase. `callableOwnQualifiedName` appends - // `localIdentity` to a nested callable's OWN name segment, under exactly - // this condition (`prefix !== undefined`). Omitting it here made the two - // phases derive different ids for the same callable — and the failure is - // silent: the caller id names a node that does not exist, so the edge is - // dropped rather than reported. The condition is deliberately identical - // to the definition phase's so the two cannot diverge again. - const ownSegment = nestedPrefix !== undefined ? localIdentity(current, funcName) : funcName; - const qualifiedName = ownerName ? `${ownerName}.${ownSegment}` : ownSegment; + // Lockstep with the other two id-building phases — see + // `nestedCallableQualifiedName`, which is the shared rule. When a + // nested prefix exists it IS `ownerName`, so this branch and the + // owner branch below cannot disagree about which prefix applies. + const qualifiedName = + nestedPrefix !== undefined + ? nestedCallableQualifiedName(nestedPrefix, current, funcName) + : ownerName + ? `${ownerName}.${funcName}` + : funcName; // Include # suffix to match definition-phase Method/Constructor IDs. // Use the same MethodExtractor (getMethodInfo) as the definition phase. // When same-arity collisions exist, also append ~type1,type2. @@ -2328,7 +2313,7 @@ const processFileGroup = ( qualifiedTypeName !== undefined ? qualifiedTypeName : nestedCallablePrefix !== undefined && definitionNode - ? `${nestedCallablePrefix}.${localIdentity(definitionNode, nodeName)}` + ? nestedCallableQualifiedName(nestedCallablePrefix, definitionNode, nodeName) : enclosingClassInfo ? `${enclosingClassInfo.className}.${nodeName}` : nodeName; diff --git a/gitnexus/test/unit/callable-id-lockstep.test.ts b/gitnexus/test/unit/callable-id-lockstep.test.ts new file mode 100644 index 000000000..d22266418 --- /dev/null +++ b/gitnexus/test/unit/callable-id-lockstep.test.ts @@ -0,0 +1,77 @@ +/** + * #2699 / #2714 — the nested-callable id rule has ONE definition. + * + * Three phases in `parse-worker.ts` build the id of a callable nested inside + * another callable, independently: the definition phase + * (`callableOwnQualifiedName`), the caller-attribution phase + * (`findEnclosingFunctionId`), and the worker-path node-id derivation in + * `processFileGroup`. They must agree byte-for-byte, and when they do not the + * failure is SILENT — the caller id names a node that does not exist, so the + * edge is dropped rather than reported. "Zero dangling edges" is what that + * looks like from the outside, which is why it went unnoticed. + * + * Caller attribution really did omit the position suffix until #2714. The fix + * routed all three through `nestedCallableQualifiedName`; this file pins both + * halves of that — the rule's contract, and the fact that no call site has + * re-inlined it. + * + * The rule reads only `startPosition` off the node, so a positional stub is a + * complete input here; parsing real source would add a tree-sitter dependency + * without testing anything more of this function. + * + * The rules live in `callable-id.ts` rather than `parse-worker.ts` precisely so + * this file can exist: parse-worker posts a `ready` message to `parentPort` at + * import, so value-importing it from a unit test throws before any test runs. + */ +import { readFileSync } from 'node:fs'; +import { fileURLToPath } from 'node:url'; +import { describe, expect, it } from 'vitest'; +import { nestedCallableQualifiedName } from '../../src/core/ingestion/workers/callable-id.js'; +import type { SyntaxNode } from '../../src/core/ingestion/utils/ast-helpers.js'; + +const nodeAt = (row: number, column: number): SyntaxNode => + ({ startPosition: { row, column } }) as unknown as SyntaxNode; + +describe('nestedCallableQualifiedName — the shared nested-callable id rule', () => { + it('qualifies by the enclosing callable AND the declaration position', () => { + expect(nestedCallableQualifiedName('run', nodeAt(3, 2), 'save')).toBe('run.save@3:2'); + }); + + it('takes the position from the node, never from the name', () => { + // Guards against a "fix" that formats the suffix from anything but the + // declaration site — the position is what makes the id unique. + expect(nestedCallableQualifiedName('outer', nodeAt(12, 9), 'fn')).toBe('outer.fn@12:9'); + }); + + it('separates same-named siblings in different blocks', () => { + // The case names alone cannot express (#2699): two `pick` bindings in the + // if/else arms of one function are genuinely different bindings, and both + // are `outer.pick` by name. + const first = nestedCallableQualifiedName('outer', nodeAt(2, 4), 'pick'); + const second = nestedCallableQualifiedName('outer', nodeAt(5, 4), 'pick'); + + expect(first).not.toBe(second); + }); + + it('carries a multi-level chain verbatim in the prefix', () => { + expect(nestedCallableQualifiedName('A.outer.mid', nodeAt(7, 0), 'inner')).toBe( + 'A.outer.mid.inner@7:0', + ); + }); +}); + +describe('no call site re-inlines the rule', () => { + it('parse-worker.ts contains no inlined `.${localIdentity(...)}` template', () => { + // The structural half. The unit assertions above would still pass if a + // fourth phase appeared and spelled the rule out by hand — which is + // exactly how the divergence #2714 fixed came to exist. This fails if any + // site reconstructs the id instead of calling the shared function. + const source = readFileSync( + fileURLToPath(new URL('../../src/core/ingestion/workers/parse-worker.ts', import.meta.url)), + 'utf8', + ); + const inlined = source.match(/\}\.\$\{localIdentity\(/g) ?? []; + + expect(inlined).toEqual([]); + }); +});