diff --git a/gitnexus/src/core/ingestion/scope-resolution/contract/scope-resolver.ts b/gitnexus/src/core/ingestion/scope-resolution/contract/scope-resolver.ts index cafc5693b..f721196b1 100644 --- a/gitnexus/src/core/ingestion/scope-resolution/contract/scope-resolver.ts +++ b/gitnexus/src/core/ingestion/scope-resolution/contract/scope-resolver.ts @@ -16,7 +16,8 @@ * 2. Export a thin entry point: * `runYourLangScopeResolution(input) = runScopeResolution(input, yourScopeResolver)`. * 3. Register the provider in - * `gitnexus/src/core/ingestion/emit-providers-registry.ts`. + * `gitnexus/src/core/ingestion/scope-resolution/pipeline/registry.ts` + * (the `SCOPE_RESOLVERS` map). * 4. Add `SupportedLanguages.YourLang` to `MIGRATED_LANGUAGES` in * `registry-primary-flag.ts`. * 5. Verify the resolver integration test at 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 e3dc44a4f..0843ca59a 100644 --- a/gitnexus/src/core/ingestion/scope-resolution/graph-bridge/ids.ts +++ b/gitnexus/src/core/ingestion/scope-resolution/graph-bridge/ids.ts @@ -22,7 +22,16 @@ import type { ScopeResolutionIndexes } from '../../model/scope-resolution-indexe import { generateId } from '../../../../lib/utils.js'; import { isLinkableLabel, type GraphNodeLookup } from '../graph-bridge/node-lookup.js'; -/** Look up a `SymbolDefinition` in the graph node lookup by file+name. */ +/** + * Look up a `SymbolDefinition` in the graph node lookup. + * + * Tries the fully-qualified name FIRST — that's the only correct key + * when two classes in the same file define a method with the same + * simple name (`class User: def save` + `class Document: def save`). + * Falls back to the simple name for definitions whose qualifier the + * lookup didn't capture (rare, but keeps cross-file simple-name + * resolution working). + */ export function resolveDefGraphId( filePath: string, def: { qualifiedName?: string }, @@ -30,6 +39,8 @@ export function resolveDefGraphId( ): string | undefined { const qn = def.qualifiedName; if (qn === undefined || qn.length === 0) return undefined; + const qualifiedHit = nodeLookup.get(`${filePath}::${qn}`); + if (qualifiedHit !== undefined) return qualifiedHit; const simpleName = qn.lastIndexOf('.') === -1 ? qn : qn.slice(qn.lastIndexOf('.') + 1); return nodeLookup.get(`${filePath}::${simpleName}`); } diff --git a/gitnexus/src/core/ingestion/scope-resolution/graph-bridge/node-lookup.ts b/gitnexus/src/core/ingestion/scope-resolution/graph-bridge/node-lookup.ts index 2b50e6fd4..bb69670a7 100644 --- a/gitnexus/src/core/ingestion/scope-resolution/graph-bridge/node-lookup.ts +++ b/gitnexus/src/core/ingestion/scope-resolution/graph-bridge/node-lookup.ts @@ -1,15 +1,21 @@ /** - * Build a `(filePath, simpleName) → graphNodeId` lookup over the - * graph's Function/Method/Class/Constructor nodes. + * Build a `(filePath, name) → graphNodeId` lookup over the graph's + * Function/Method/Class/Constructor nodes. Two keys per node: + * + * - simple name (`User` / `save`) — legacy fallback + * - qualified name when derivable from the node id (`User.save`) + * + * The qualified key is the authoritative one when two classes in the + * same file define a method with the same simple name + * (`class User: def save` + `class Document: def save`). Without it, + * the simple-name key collides and every `document.save()` CALLS edge + * would silently target `User.save`. Method node ids encode the + * qualifier (`Method:file.py:User.save#1`), so we parse it back out. * * Language-agnostic seam. Any language provider migrating to the * registry-primary path can consume this to translate scope-resolution * `SymbolDefinition.nodeId` values into the legacy graph-node ID * format that downstream consumers (queries, edges, MCP) expect. - * - * Next-consumer contract: a TypeScript or Java provider imports this - * module unchanged — the lookup is keyed by (filePath, name) which - * every language produces. */ import type { NodeLabel } from 'gitnexus-shared'; @@ -17,21 +23,53 @@ import type { KnowledgeGraph } from '../../../graph/types.js'; export type GraphNodeLookup = ReadonlyMap; +/** + * Parse a qualified name out of a Function/Method node id. + * + * Node id format: `${label}:${filePath}:${qualifiedName}${arityTag}`, + * where `arityTag` is `#` (or empty). Strips the known-length + * label + filePath prefix so colons inside `filePath` (Windows + * `C:\...`) don't break the parse. Returns `undefined` when the id + * doesn't match the expected shape. + */ +function parseQualifiedFromId(id: string, label: NodeLabel, filePath: string): string | undefined { + const prefix = `${label}:${filePath}:`; + if (!id.startsWith(prefix)) return undefined; + const suffix = id.slice(prefix.length); + if (suffix.length === 0) return undefined; + const hash = suffix.indexOf('#'); + return hash === -1 ? suffix : suffix.slice(0, hash); +} + export function buildGraphNodeLookup(graph: KnowledgeGraph): GraphNodeLookup { const lookup = new Map(); for (const node of graph.iterNodes()) { - const props = node.properties as { filePath?: string; name?: string }; + const props = node.properties as { + filePath?: string; + name?: string; + qualifiedName?: string; + }; if (props.filePath === undefined || props.name === undefined) continue; if (!isLinkableLabel(node.label)) continue; - // Keyed by (filePath, simpleName). Class kinds and method kinds - // share the same simple-name space within a file — a `class Foo` - // and `def Foo()` at the same level is disallowed by Python (and - // most languages), so a single key per (file, name) is unambiguous - // in practice. Method-vs-class disambiguation for resolved - // references happens earlier inside `MethodRegistry.lookup` - // (Step 1 + Step 2). - const key = `${props.filePath}::${props.name}`; - if (!lookup.has(key)) lookup.set(key, node.id); + + // Primary key: fully-qualified name when available. Class nodes + // carry `qualifiedName` in their properties (set by the parsing + // processor). Method/Function nodes do not, so derive the + // qualifier from the node id — that's where the parse-phase + // encoded it. + const qualified = + props.qualifiedName ?? parseQualifiedFromId(node.id, node.label, props.filePath); + if (qualified !== undefined && qualified.length > 0) { + const qKey = `${props.filePath}::${qualified}`; + if (!lookup.has(qKey)) lookup.set(qKey, node.id); + } + + // Fallback key: simple name. First-wins within a file — used when + // the caller doesn't know the qualifier (unqualified free-call + // fallback, cross-file resolution where MethodRegistry already + // disambiguated the owner). + const simpleKey = `${props.filePath}::${props.name}`; + if (!lookup.has(simpleKey)) lookup.set(simpleKey, node.id); } return lookup; } diff --git a/gitnexus/src/core/ingestion/scope-resolution/pipeline/phase.ts b/gitnexus/src/core/ingestion/scope-resolution/pipeline/phase.ts index 3129cf236..d050f0a82 100644 --- a/gitnexus/src/core/ingestion/scope-resolution/pipeline/phase.ts +++ b/gitnexus/src/core/ingestion/scope-resolution/pipeline/phase.ts @@ -1,7 +1,7 @@ /** * Phase: scopeResolution * - * Generic registry-primary resolution phase (RFC #909 Ring 4). + * Generic registry-primary resolution phase (RFC #909 Ring 3). * * For every language in `MIGRATED_LANGUAGES` (per-language flag set) * whose provider is registered in `SCOPE_RESOLVERS`: @@ -72,7 +72,15 @@ export const scopeResolutionPhase: PipelinePhase = { // already-existing Symbol nodes (Function/Method/Class). The legacy // `parse` phase still creates those nodes; we only replace the // import + call resolution layer. - deps: ['parse', 'structure'], + // + // Also depends on `crossFile` — we don't read crossFile's output + // directly (we have our own cross-file resolution), but crossFile + // writes EXTENDS edges that `buildMro` consumes via + // `iterRelationshipsByType('EXTENDS')`. Declaring the dep pins the + // ordering explicitly: without it, Kahn's runner could schedule + // scopeResolution before crossFile (both unblock after parse), and + // the MRO walk would miss heritage edges crossFile later adds. + deps: ['parse', 'crossFile', 'structure'], async execute( ctx: PipelineContext, diff --git a/gitnexus/src/core/ingestion/scope-resolution/pipeline/run.ts b/gitnexus/src/core/ingestion/scope-resolution/pipeline/run.ts index b8bbf2355..ff0974ae7 100644 --- a/gitnexus/src/core/ingestion/scope-resolution/pipeline/run.ts +++ b/gitnexus/src/core/ingestion/scope-resolution/pipeline/run.ts @@ -109,7 +109,7 @@ export function runScopeResolution( const nodeLookup = buildGraphNodeLookup(graph); const mroByClassDefId = provider.buildMro(graph, parsedFiles, nodeLookup); - const indexes = finalizeScopeModel(parsedFiles, { + const finalized = finalizeScopeModel(parsedFiles, { hooks: { resolveImportTarget: (targetRaw, fromFile) => provider.resolveImportTarget(targetRaw, fromFile, allFilePaths), @@ -118,11 +118,16 @@ export function runScopeResolution( }, }); - // Stitch the MRO into the finalized indexes (same pattern as before - // generalization — finalizeScopeModel builds an empty - // MethodDispatchIndex by design). - (indexes as { methodDispatch: typeof indexes.methodDispatch }).methodDispatch = - buildPopulatedMethodDispatch(mroByClassDefId); + // Replace the empty MethodDispatchIndex that finalizeScopeModel + // builds by design with the populated one derived from the + // language's MRO. Spread produces a fresh `ScopeResolutionIndexes` + // instead of mutating the finalized result through an `as` cast — + // downstream passes get an object whose readonly guarantees match + // the type system. + const indexes = { + ...finalized, + methodDispatch: buildPopulatedMethodDispatch(mroByClassDefId), + }; // Build the workspace resolution index ONCE — turns every // findOwnedMember / findExportedDef / classScopeByDefId lookup in diff --git a/gitnexus/src/core/ingestion/scope-resolution/scope/walkers.ts b/gitnexus/src/core/ingestion/scope-resolution/scope/walkers.ts index c9bbf74d7..fa9a443aa 100644 --- a/gitnexus/src/core/ingestion/scope-resolution/scope/walkers.ts +++ b/gitnexus/src/core/ingestion/scope-resolution/scope/walkers.ts @@ -155,6 +155,23 @@ export function populateClassOwnedMembers(parsed: ParsedFile): void { const scopesById = new Map(); for (const scope of parsed.scopes) scopesById.set(scope.id, scope); + // Promote a def's qualifiedName from `methodName` to `ClassName.methodName` + // when the def sits inside a class. Without this, two classes in the + // same file that share a method name collide at the graph-bridge lookup + // (`node-lookup.ts` keys by (filePath, qualifiedName) and falls back to + // simple name only). Python's `scopes.scm` doesn't emit + // `@declaration.qualified_name` for nested methods, so the finalized + // defs arrive here with simple names — we stamp the qualifier while + // we're already walking class scopes for ownerId. + const qualify = (def: SymbolDefinition, classDef: SymbolDefinition): void => { + const q = def.qualifiedName; + if (q === undefined || q.length === 0) return; + if (q.includes('.')) return; // already qualified (dotted) + const classQ = classDef.qualifiedName; + if (classQ === undefined || classQ.length === 0) return; + (def as { qualifiedName: string }).qualifiedName = `${classQ}.${q}`; + }; + for (const scope of parsed.scopes) { // Methods: function scope whose parent is a Class scope. Owner is // the parent's Class def. @@ -165,6 +182,7 @@ export function populateClassOwnedMembers(parsed: ParsedFile): void { if (classDef !== undefined) { for (const def of scope.ownedDefs) { (def as { ownerId?: string }).ownerId = classDef.nodeId; + qualify(def, classDef); } } } @@ -177,6 +195,7 @@ export function populateClassOwnedMembers(parsed: ParsedFile): void { for (const def of scope.ownedDefs) { if (def === classDef) continue; (def as { ownerId?: string }).ownerId = classDef.nodeId; + qualify(def, classDef); } } } diff --git a/gitnexus/test/fixtures/lang-resolution/python-same-file-method-collision/app.py b/gitnexus/test/fixtures/lang-resolution/python-same-file-method-collision/app.py new file mode 100644 index 000000000..397235f8f --- /dev/null +++ b/gitnexus/test/fixtures/lang-resolution/python-same-file-method-collision/app.py @@ -0,0 +1,11 @@ +from models import User, Document + + +def use_user() -> None: + u = User() + u.save() + + +def use_document() -> None: + d = Document() + d.save() diff --git a/gitnexus/test/fixtures/lang-resolution/python-same-file-method-collision/models.py b/gitnexus/test/fixtures/lang-resolution/python-same-file-method-collision/models.py new file mode 100644 index 000000000..42a251fe5 --- /dev/null +++ b/gitnexus/test/fixtures/lang-resolution/python-same-file-method-collision/models.py @@ -0,0 +1,22 @@ +""" +Two classes in one file each defining a method with the same simple +name. Exercises the node-lookup qualified-name key — without it, +both User.save and Document.save share the bucket `models.py::save` +and every `document.save()` CALLS edge silently resolves to User.save. +""" + + +class User: + def save(self) -> bool: + return True + + def load(self) -> None: + return None + + +class Document: + def save(self) -> bool: + return False + + def load(self) -> None: + return None diff --git a/gitnexus/test/integration/resolvers/python.test.ts b/gitnexus/test/integration/resolvers/python.test.ts index 72f23edeb..ce8fddc91 100644 --- a/gitnexus/test/integration/resolvers/python.test.ts +++ b/gitnexus/test/integration/resolvers/python.test.ts @@ -2231,3 +2231,49 @@ describe('Python Grandchild→Child→Parent — 3-level C3 MRO walk (SM-11)', ( expect(gpCall!.source).toBe('run'); }); }); + +// --------------------------------------------------------------------------- +// Same-file method-name collision across classes +// PR #980 review feedback — without a qualified-name key in the node lookup, +// User.save and Document.save share the bucket `models.py::save`, so every +// d.save() CALLS edge silently resolves to the first save() seen. +// --------------------------------------------------------------------------- + +describe('Python same-file method-name collision across classes', () => { + let result: PipelineResult; + + beforeAll(async () => { + result = await runPipelineFromRepo( + path.join(FIXTURES, 'python-same-file-method-collision'), + () => {}, + ); + }, 60000); + + it('u.save() resolves to User.save, not Document.save', () => { + const calls = getRelationships(result, 'CALLS'); + const saveCalls = calls.filter((c) => c.target === 'save'); + const fromUseUser = saveCalls.find((c) => c.source === 'use_user'); + expect(fromUseUser).toBeDefined(); + // targetId encodes qualifier: Method:models.py:User.save#0 + expect(fromUseUser!.rel.targetId).toContain('User.save'); + expect(fromUseUser!.rel.targetId).not.toContain('Document.save'); + }); + + it('d.save() resolves to Document.save, not User.save', () => { + const calls = getRelationships(result, 'CALLS'); + const saveCalls = calls.filter((c) => c.target === 'save'); + const fromUseDoc = saveCalls.find((c) => c.source === 'use_document'); + expect(fromUseDoc).toBeDefined(); + expect(fromUseDoc!.rel.targetId).toContain('Document.save'); + expect(fromUseDoc!.rel.targetId).not.toContain('User.save'); + }); + + it('exactly two CALLS edges to save() — one per class, no duplication to wrong target', () => { + const calls = getRelationships(result, 'CALLS'); + const saveCalls = calls.filter((c) => c.target === 'save'); + expect(saveCalls).toHaveLength(2); + const targets = saveCalls.map((c) => c.rel.targetId).sort(); + expect(targets[0]).toContain('Document.save'); + expect(targets[1]).toContain('User.save'); + }); +});