diff --git a/gitnexus/src/core/ingestion/scope-resolution/graph-bridge/ids.ts b/gitnexus/src/core/ingestion/scope-resolution/graph-bridge/ids.ts index 42dc67acc..8770b9f3f 100644 --- a/gitnexus/src/core/ingestion/scope-resolution/graph-bridge/ids.ts +++ b/gitnexus/src/core/ingestion/scope-resolution/graph-bridge/ids.ts @@ -34,6 +34,7 @@ import { } from '../../utils/callable-labels.js'; import { templateConstraintsIdTag } from '../../utils/template-arguments.js'; import { parameterShapeIdTag } from '../../utils/method-props.js'; +import { definitionIdPosition } from '../utils/definition-id.js'; /** * Labels that may legitimately ANCHOR a CALLS/ACCESSES edge as the * source ("caller"). A Variable / Property can be the TARGET of an @@ -99,9 +100,9 @@ function scopeIsCallableBody( range: { startLine: number; startCol: number }, def: SymbolDefinition, ): boolean { - const m = def.nodeId.match(/#(\d+):(\d+):/); - if (m === null) return false; - return Number(m[1]) === range.startLine && Number(m[2]) === range.startCol; + const position = definitionIdPosition(def.nodeId, def.filePath); + if (position === undefined) return false; + return position.line === range.startLine && position.column === range.startCol; } /** Pick the callable that owns `atRange` when multiple overloads share a class scope. */ @@ -165,12 +166,8 @@ function pickCallerCallableDef( * Extract the 1-based declaration line from a scope-resolution def id. * Shape: `def:#::<...>`; `undefined` when it doesn't match. */ -function defStartLine(nodeId: string | undefined): number | undefined { - if (nodeId === undefined) return undefined; - const m = nodeId.match(/#(\d+):(\d+):/); - if (m === null) return undefined; - const line = Number(m[1]); - return Number.isFinite(line) ? line : undefined; +function defStartLine(nodeId: string | undefined, filePath: string): number | undefined { + return definitionIdPosition(nodeId, filePath)?.line; } /** @@ -221,7 +218,7 @@ export function resolveDefGraphId( // without either side having to model the scope chain. Node ids are // 0-based, def ids 1-based. An `AMBIGUOUS_POSITION` tombstone (two // callables on one line) falls through to the name-based keys below. - const line = defStartLine(def.nodeId); + const line = defStartLine(def.nodeId, filePath); if (line !== undefined && isPositionQualifiedLocalLabel(def.type)) { const simple = simpleNameOf(qn); const posHit = nodeLookup.get(positionKey(filePath, def.type, line - 1, simple)); diff --git a/gitnexus/src/core/ingestion/scope-resolution/passes/callable-value-flow.ts b/gitnexus/src/core/ingestion/scope-resolution/passes/callable-value-flow.ts index f0240a2d3..1f39bf304 100644 --- a/gitnexus/src/core/ingestion/scope-resolution/passes/callable-value-flow.ts +++ b/gitnexus/src/core/ingestion/scope-resolution/passes/callable-value-flow.ts @@ -26,6 +26,7 @@ import { simpleQualifiedName, } from '../graph-bridge/ids.js'; import { resolveInheritanceBaseInScope } from '../scope/walkers.js'; +import { definitionIdPosition } from '../utils/definition-id.js'; import { narrowOverloadCandidates } from './overload-narrowing.js'; export const MAX_CALLABLE_VALUE_TARGETS = 32; @@ -815,7 +816,7 @@ const withoutSigil = (name: string): string => name.replace(/^[$@]+/, ''); */ function valueBindingPositionKey(def: SymbolDefinition): string | undefined { if (!VALUE_BINDING_DEF_TYPES.has(def.type)) return undefined; - const line = def.nodeId.match(/#(\d+):(\d+):/)?.[1]; + const line = definitionIdPosition(def.nodeId, def.filePath)?.line; const name = simpleQualifiedName(def); if (line === undefined || name === undefined) return undefined; return `${def.filePath}\0${line}\0${withoutSigil(name)}`; @@ -921,8 +922,10 @@ function canonicalizeCallableDeclarations( } function definitionPosition(def: SymbolDefinition): string | undefined { - const match = def.nodeId.match(/#(\d+):(\d+):/); - return match === null ? undefined : `${def.filePath}\0${match[1]}\0${match[2]}`; + const position = definitionIdPosition(def.nodeId, def.filePath); + return position === undefined + ? undefined + : `${def.filePath}\0${position.line}\0${position.column}`; } function declarationSignatureCompatible( @@ -1003,7 +1006,7 @@ function buildGraphCallableIndexes(graph: KnowledgeGraph): GraphCallableIndexes } function definitionAnchorKey(def: SymbolDefinition): string | undefined { - const line = def.nodeId.match(/#(\d+):(\d+):/)?.[1]; + const line = definitionIdPosition(def.nodeId, def.filePath)?.line; const name = simpleName(def.qualifiedName); if (line === undefined || name === undefined) return undefined; return `${def.filePath}\0${def.type}\0${line}\0${name}`; @@ -1437,8 +1440,8 @@ function dedupeDefinitions(defs: readonly SymbolDefinition[]): SymbolDefinition[ } function definitionStartsAt(def: SymbolDefinition, line: number, column: number): boolean { - const match = def.nodeId.match(/#(\d+):(\d+):/); - return match !== null && Number(match[1]) === line && Number(match[2]) === column; + const position = definitionIdPosition(def.nodeId, def.filePath); + return position !== undefined && position.line === line && position.column === column; } function expandMemberTargets( diff --git a/gitnexus/src/core/ingestion/scope-resolution/utils/definition-id.ts b/gitnexus/src/core/ingestion/scope-resolution/utils/definition-id.ts new file mode 100644 index 000000000..93ee50013 --- /dev/null +++ b/gitnexus/src/core/ingestion/scope-resolution/utils/definition-id.ts @@ -0,0 +1,24 @@ +export interface DefinitionIdPosition { + readonly line: number; + readonly column: number; +} + +/** + * Extract coordinates from `def:#:::`. + * + * File paths are embedded verbatim and may themselves contain fragments such + * as `#12:34:`, while names may themselves contain `#` (TypeScript private + * members). Anchor on the known file path so neither side can be mistaken for + * the declaration separator. + */ +export function definitionIdPosition( + nodeId: string | undefined, + filePath: string, +): DefinitionIdPosition | undefined { + if (nodeId === undefined) return undefined; + const prefix = `def:${filePath}#`; + if (!nodeId.startsWith(prefix)) return undefined; + const match = /^(\d+):(\d+):/.exec(nodeId.slice(prefix.length)); + if (match === null) return undefined; + return { line: Number(match[1]), column: Number(match[2]) }; +} diff --git a/gitnexus/test/integration/closure-review-findings.test.ts b/gitnexus/test/integration/closure-review-findings.test.ts index aed6d959b..8879f7e57 100644 --- a/gitnexus/test/integration/closure-review-findings.test.ts +++ b/gitnexus/test/integration/closure-review-findings.test.ts @@ -18,12 +18,16 @@ import { DIST_WORKER_URL, distWorkerExists } from '../helpers/worker-parse.js'; vi.setConfig({ testTimeout: 90_000 }); const describeIfWorkerBuilt = distWorkerExists() ? describe : describe.skip; +const describeIfPathSupportsColon = + process.platform === 'win32' ? describe.skip : describeIfWorkerBuilt; /** Every CALLS edge in a one-file repo, as `src -> dst`, sorted. */ const callEdges = async (filename: string, source: string): Promise => { const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'gn-review-')); try { - fs.writeFileSync(path.join(dir, filename), source, 'utf-8'); + const filePath = path.join(dir, filename); + fs.mkdirSync(path.dirname(filePath), { recursive: true }); + fs.writeFileSync(filePath, source, 'utf-8'); const result = await runPipelineFromRepo(dir, () => {}, { workerPoolSize: 1, workerUrlForTest: DIST_WORKER_URL, @@ -85,6 +89,20 @@ describeIfWorkerBuilt( }, ); +describeIfPathSupportsColon('#2734 — def ids parse coordinates after the file path', () => { + it('a path containing #:: still attributes closure calls', async () => { + const edges = await callEdges( + 'src/bugs#12:34:test/a.php', + ' Function:src/bugs#12:34:test/a.php:target', + ]); + }); +}); + describeIfWorkerBuilt( '#2699 review P1-2 — widening identity to values must not touch class members', () => { diff --git a/gitnexus/test/unit/scope-resolution/callable-value-target-index.test.ts b/gitnexus/test/unit/scope-resolution/callable-value-target-index.test.ts index 25674efcb..6f301fe70 100644 --- a/gitnexus/test/unit/scope-resolution/callable-value-target-index.test.ts +++ b/gitnexus/test/unit/scope-resolution/callable-value-target-index.test.ts @@ -33,7 +33,7 @@ const graphOf = (nodes: readonly StubNode[]): KnowledgeGraph => { /** `line` is 1-based, matching the definition-id convention. */ const def = (type: string, filePath: string, qualifiedName: string, line: number) => ({ - nodeId: `${filePath}#${line}:0:${qualifiedName}`, + nodeId: `def:${filePath}#${line}:0:${type}:${qualifiedName}`, type, filePath, qualifiedName, @@ -69,7 +69,7 @@ describe('buildGraphTargetIndex — value bindings join by position', () => { // the same construct, so they share file, line and name. expect( targetsFor([node('Function', 'a.kt', 'handler', 3)], [def('Property', 'a.kt', 'handler', 3)]), - ).toEqual(['a.kt#3:0:handler => Function:a.kt:handler']); + ).toEqual(['def:a.kt#3:0:Property:handler => Function:a.kt:handler']); }); it('REJECTS a value binding that merely shares a name with a callable elsewhere', () => { @@ -106,12 +106,12 @@ describe('buildGraphTargetIndex — value bindings join by position', () => { [node('Function', 'a.php', '$save', 5), node('Function', 'a.php', 'save', 2)], [def('Variable', 'a.php', 'save', 5)], ), - ).toEqual(['a.php#5:0:save => Function:a.php:$save']); + ).toEqual(['def:a.php#5:0:Variable:save => Function:a.php:$save']); }); it('keeps ordinary callable defs, which never take the positional path', () => { expect( targetsFor([node('Function', 'a.ts', 'fn', 1)], [def('Function', 'a.ts', 'fn', 1)]), - ).toEqual(['a.ts#1:0:fn => Function:a.ts:fn']); + ).toEqual(['def:a.ts#1:0:Function:fn => Function:a.ts:fn']); }); }); diff --git a/gitnexus/test/unit/scope-resolution/definition-id.test.ts b/gitnexus/test/unit/scope-resolution/definition-id.test.ts new file mode 100644 index 000000000..af547b471 --- /dev/null +++ b/gitnexus/test/unit/scope-resolution/definition-id.test.ts @@ -0,0 +1,30 @@ +import { describe, expect, it } from 'vitest'; +import { definitionIdPosition } from '../../../src/core/ingestion/scope-resolution/utils/definition-id.js'; + +describe('definitionIdPosition', () => { + it('reads coordinates after a path containing a coordinate-like fragment (#2734)', () => { + expect( + definitionIdPosition( + 'def:src/bugs#12:34:test/a.php#4:2:Function:$handler', + 'src/bugs#12:34:test/a.php', + ), + ).toEqual({ line: 4, column: 2 }); + }); + + it('does not mistake a private-name hash for the path separator', () => { + expect(definitionIdPosition('def:src/a.ts#4:2:Method:#secret', 'src/a.ts')).toEqual({ + line: 4, + column: 2, + }); + }); + + it.each<[string | undefined, string]>([ + [undefined, 'src/plain.php'], + ['def:src/plain.php', 'src/plain.php'], + ['def:src/plain.php#x:2:Function:handler', 'src/plain.php'], + ['def:src/plain.php#4:x:Function:handler', 'src/plain.php'], + ['def:src/other.php#4:2:Function:handler', 'src/plain.php'], + ])('rejects malformed or mismatched definition id %s', (nodeId, filePath) => { + expect(definitionIdPosition(nodeId, filePath)).toBeUndefined(); + }); +});