From e723f3c2ee174738e2007a739991a280fafcaba9 Mon Sep 17 00:00:00 2001 From: azizur100389 Date: Thu, 30 Jul 2026 07:34:32 +0100 Subject: [PATCH 1/6] fix(scope-resolution): parse def coordinates after file paths (#2743) * fix(scope-resolution): parse def coordinates after file paths Anchor coordinate parsing to the known file path so coordinate-like path fragments and private symbol names cannot corrupt closure attribution. * fix(bench): use production definition ids --- .../bench/callable-value-flow/baselines.json | 4 +-- .../bench/callable-value-flow/measure.mjs | 2 +- .../scope-resolution/graph-bridge/ids.ts | 17 +++++------ .../passes/callable-value-flow.ts | 15 ++++++---- .../scope-resolution/utils/definition-id.ts | 24 +++++++++++++++ .../closure-review-findings.test.ts | 20 ++++++++++++- .../callable-value-target-index.test.ts | 8 ++--- .../scope-resolution/definition-id.test.ts | 30 +++++++++++++++++++ 8 files changed, 96 insertions(+), 24 deletions(-) create mode 100644 gitnexus/src/core/ingestion/scope-resolution/utils/definition-id.ts create mode 100644 gitnexus/test/unit/scope-resolution/definition-id.test.ts diff --git a/gitnexus/bench/callable-value-flow/baselines.json b/gitnexus/bench/callable-value-flow/baselines.json index c69d63c834..472cc5cd61 100644 --- a/gitnexus/bench/callable-value-flow/baselines.json +++ b/gitnexus/bench/callable-value-flow/baselines.json @@ -1,6 +1,6 @@ { - "_comment": "Baselines for bench/callable-value-flow/measure.mjs --check (#2693). `fingerprint` is an order-independent sha256 over every (defNodeId -> graphId) pair buildGraphTargetIndex resolves on the synthetic corpus; it is a CORRECTNESS gate, so drift means the callable-value target set moved and must be explained, never re-baselined to make CI green. The two budgets are timing gates and carry deliberate headroom for shared CI runners.", - "fingerprint": "70bebf6a26ff6fc9f231a0933678274b44c4883ddab5e719a61a9c77d6223e51", + "_comment": "Baselines for bench/callable-value-flow/measure.mjs --check (#2693). `fingerprint` is an order-independent sha256 over every (defNodeId -> graphId) pair buildGraphTargetIndex resolves on the synthetic corpus; it is a CORRECTNESS gate, so drift means the callable-value target set moved and must be explained, never re-baselined to make CI green. The fingerprint changed when the synthetic corpus adopted production-shaped def ids; target cardinality remains 4000 (3200 callable-only plus 800 value bindings). The two budgets are timing gates and carry deliberate headroom for shared CI runners.", + "fingerprint": "6599dda7d0ee5942e1995a1dcfb137c312eb8e4690bc82a8bbff429f08bd839d", "scaling_budget": 1.6, "_scaling_note": "(t_large/t_small)/(800/250). ~1.0 is linear; measured 1.14-1.16. The index build is one pass over defs plus map lookups, so a jump toward 3.x means someone made the per-def work depend on corpus size (e.g. a scan inside the loop).", "widening_overhead_budget": 1.9, diff --git a/gitnexus/bench/callable-value-flow/measure.mjs b/gitnexus/bench/callable-value-flow/measure.mjs index d8d7a23730..e0a5f320b2 100644 --- a/gitnexus/bench/callable-value-flow/measure.mjs +++ b/gitnexus/bench/callable-value-flow/measure.mjs @@ -89,7 +89,7 @@ function buildCorpus(fileCount) { return id; }; const addDef = (type, filePath, qualifiedName, line) => { - const nodeId = `${filePath}#${line}:0:${qualifiedName}`; + const nodeId = `def:${filePath}#${line}:0:${type}:${qualifiedName}`; defs.set(nodeId, { nodeId, type, filePath, qualifiedName }); }; 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 42dc67acc8..8770b9f3f8 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 f0240a2d3f..1f39bf3045 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 0000000000..93ee50013b --- /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 aed6d959b8..8879f7e57c 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 25674efcb5..6f301fe704 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 0000000000..af547b4714 --- /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(); + }); +}); From 9c24e3459e859b84bfe3dca6377c27960bf3c6a9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gerg=C5=91=20Magyar?= Date: Thu, 30 Jul 2026 17:08:00 +0100 Subject: [PATCH 2/6] fix(rust): qualify items by their enclosing mod chain so same-named ones stay distinct (#2742) (#2745) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(rust): let the qualified-call filter see inline modules The negative filter added in #2741 builds its set of known module names from FILE PATHS, so an inline `mod x { … }` — which appears in no path — was absent from it. Every module-qualified call into an inline module was therefore rejected before any candidate channel ran, which is a hole in that optimisation rather than in the resolution logic it guards. The per-pass index now unions the file-derived names with inline module names taken from the scope model: a `mod` declaration binds a `Namespace` def locally in the declaring scope, and that binding is the only place an inline module's name exists. Collected in the same walk that already builds the module → scope map, so it costs no extra pass. Found while fixing #2742, where a correctly resolved call into `mod inner { … }` still could not reach its target. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01J5nWP4BFeBSjbQ3uqAjtxV * fix(scope-resolution): try the namespace-prefixed node key before the bare one `resolveDefGraphId` looked up the plain `qualifiedName` key first and only retried with the `namespacePrefix`-qualified key afterwards. For the defs that carry a prefix the qualified name is a bare TAIL, so the plain key happily matched a same-named item at a different namespace depth in the same file and returned it before the more specific retry was ever reached. The namespace-prefixed key is strictly the more specific of the two, so it is now tried first. Where no such node exists the lookup falls through to exactly the previous order, which keeps the #1982 behaviour this retry was added for. Without this, a call into `mod inner { fn dispatch }` resolved to the correct definition and then mapped it onto the crate-root `fn dispatch` node — the self-loop #2742 describes. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01J5nWP4BFeBSjbQ3uqAjtxV * fix(rust): qualify items by their enclosing mod chain so same-named ones stay distinct (#2742) Node identity is `