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 0843ca59a..5f12a3e6f 100644 --- a/gitnexus/src/core/ingestion/scope-resolution/graph-bridge/ids.ts +++ b/gitnexus/src/core/ingestion/scope-resolution/graph-bridge/ids.ts @@ -17,32 +17,46 @@ * migrate. */ -import type { ScopeId, SymbolDefinition } from 'gitnexus-shared'; +import type { NodeLabel, ScopeId, SymbolDefinition } from 'gitnexus-shared'; import type { ScopeResolutionIndexes } from '../../model/scope-resolution-indexes.js'; import { generateId } from '../../../../lib/utils.js'; -import { isLinkableLabel, type GraphNodeLookup } from '../graph-bridge/node-lookup.js'; +import { + isLinkableLabel, + qualifiedKey, + simpleKey, + type GraphNodeLookup, +} from '../graph-bridge/node-lookup.js'; /** * 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`). + * Tries the type-prefixed fully-qualified key 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`). + * - A top-level function and a class method share a simple name + * (`def save` + `class User: def save` — the Function's qualifier + * is just `save`, which would alias the Method's simple-key slot + * without the type prefix). + * * Falls back to the simple name for definitions whose qualifier the * lookup didn't capture (rare, but keeps cross-file simple-name - * resolution working). + * resolution working for languages that don't yet synthesize + * qualifiers). */ export function resolveDefGraphId( filePath: string, - def: { qualifiedName?: string }, + def: { qualifiedName?: string; type?: NodeLabel }, nodeLookup: GraphNodeLookup, ): 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; + if (def.type !== undefined) { + const qualifiedHit = nodeLookup.get(qualifiedKey(filePath, def.type, qn)); + if (qualifiedHit !== undefined) return qualifiedHit; + } const simpleName = qn.lastIndexOf('.') === -1 ? qn : qn.slice(qn.lastIndexOf('.') + 1); - return nodeLookup.get(`${filePath}::${simpleName}`); + return nodeLookup.get(simpleKey(filePath, simpleName)); } /** Derive the simple (unqualified) name of a def from its `qualifiedName`. */ 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 bb69670a7..db4c6024c 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 @@ -41,6 +41,25 @@ function parseQualifiedFromId(id: string, label: NodeLabel, filePath: string): s return hash === -1 ? suffix : suffix.slice(0, hash); } +/** + * Build a qualified-key string in a separate keyspace from simple-key + * strings. Prefix `` can't appear in a valid filePath on any OS, so + * no collision between the two keyspaces is possible. + * + * Includes the node label so a top-level `def save` (Function, + * qualifier = `save`) doesn't alias a class method `User.save` (Method, + * simple name = `save`) whose Function-typed qualifier would collapse + * to the same simple-key slot in a single map. + */ +export function qualifiedKey(filePath: string, label: NodeLabel, qualifiedName: string): string { + return `:${filePath}::${label}::${qualifiedName}`; +} + +/** Simple-name key (legacy fallback keyspace — no `` prefix). */ +export function simpleKey(filePath: string, name: string): string { + return `${filePath}::${name}`; +} + export function buildGraphNodeLookup(graph: KnowledgeGraph): GraphNodeLookup { const lookup = new Map(); for (const node of graph.iterNodes()) { @@ -52,15 +71,18 @@ export function buildGraphNodeLookup(graph: KnowledgeGraph): GraphNodeLookup { if (props.filePath === undefined || props.name === undefined) continue; if (!isLinkableLabel(node.label)) continue; - // 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. + // Primary key: fully-qualified name + label, in a separate + // keyspace from simple names. 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. Including + // the label avoids a collision when a free Function's qualifier + // happens to equal a Method's simple name (e.g. top-level + // `def save` vs `class User: def save`). const qualified = props.qualifiedName ?? parseQualifiedFromId(node.id, node.label, props.filePath); if (qualified !== undefined && qualified.length > 0) { - const qKey = `${props.filePath}::${qualified}`; + const qKey = qualifiedKey(props.filePath, node.label, qualified); if (!lookup.has(qKey)) lookup.set(qKey, node.id); } @@ -68,8 +90,8 @@ export function buildGraphNodeLookup(graph: KnowledgeGraph): GraphNodeLookup { // 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); + const sKey = simpleKey(props.filePath, props.name); + if (!lookup.has(sKey)) lookup.set(sKey, node.id); } return lookup; } diff --git a/gitnexus/src/core/ingestion/scope-resolution/workspace-index.ts b/gitnexus/src/core/ingestion/scope-resolution/workspace-index.ts index b8ce38d54..72428b2e3 100644 --- a/gitnexus/src/core/ingestion/scope-resolution/workspace-index.ts +++ b/gitnexus/src/core/ingestion/scope-resolution/workspace-index.ts @@ -26,16 +26,24 @@ export interface WorkspaceResolutionIndex { readonly classScopeByDefId: ReadonlyMap; /** Owner def `nodeId` → (simple-member-name → owned `SymbolDefinition`). - * Replaces `findOwnedMember`'s O(N × D) walk with O(1) lookup. */ + * Replaces `findOwnedMember`'s O(N × D) walk with O(1) lookup. + * Built from `parsed.localDefs` so class-owned members land in the + * right bucket via their `ownerId`. */ readonly memberByOwner: ReadonlyMap>; - /** File path → (simple-name → first matching `SymbolDefinition`). - * Replaces `findExportedDef`'s O(N × D) walk. */ + /** File path → (simple-name → first matching module-scope-owned + * `SymbolDefinition`). Backs `findExportedDef` — the lookup for + * `from mod import X` / `mod.X()` targets. Only defs directly + * owned by the file's `Module` scope are indexed here; methods, + * fields, and nested-function defs are NOT visible as file-level + * exports. First-seen-within-module wins. */ readonly defsByFileAndName: ReadonlyMap>; /** Workspace-wide simple-name fallback: simple-name → all matching - * Function/Method/Constructor defs. Backs the - * `findExportedDefByName` fallback scan. */ + * module-scope-owned Function/Method/Constructor defs. Backs the + * `findExportedDefByName` fallback scan. Class methods and nested + * functions are NOT eligible here — they are not import-visible + * callables. */ readonly callablesBySimpleName: ReadonlyMap; /** Module scope by file path — used by cross-file return-type @@ -64,38 +72,68 @@ export function buildWorkspaceResolutionIndex( if (cd !== undefined) classScopeByDefId.set(cd.nodeId, scope); } - // per-file by-simple-name index + callable fallback + // Module-export pass — populates the file-level export lookup + // and the workspace callable fallback with ONLY module-level + // defs. "Module-level" here means: defs owned by the module scope + // OR by any scope whose parent is the module scope (top-level + // class and function declarations live in their own Class / + // Function scopes whose parent is the module scope — not in + // moduleScope.ownedDefs). + // + // Methods (Function/Method defs whose owning scope's parent is a + // Class scope) and nested-function defs (parent is another + // Function scope) are intentionally excluded — they are not + // import-visible as `from mod import X` / `mod.X()` targets. + // Without this filter, a class method can win the file-level + // export lookup by parse order and produce silently wrong CALLS + // edges. let fileBucket = defsByFileAndName.get(parsed.filePath); if (fileBucket === undefined) { fileBucket = new Map(); defsByFileAndName.set(parsed.filePath, fileBucket); } + if (moduleScope !== undefined) { + const addExport = (def: SymbolDefinition): void => { + const simple = simpleQualifiedName(def); + if (simple === undefined) return; + // First-seen wins to match `findExportedDef` semantics. + if (!fileBucket!.has(simple)) fileBucket!.set(simple, def); + if (def.type === 'Function' || def.type === 'Method' || def.type === 'Constructor') { + let bucket = callablesBySimpleName.get(simple); + if (bucket === undefined) { + bucket = []; + callablesBySimpleName.set(simple, bucket); + } + bucket.push(def); + } + }; + // Defs directly owned by the module scope (rare — usually + // module-level variable assignments and re-exports). + for (const def of moduleScope.ownedDefs) addExport(def); + // Defs whose containing scope is a direct child of the module + // scope — top-level class declarations and top-level function + // declarations each get their own scope with parent = module. + for (const scope of parsed.scopes) { + if (scope.parent !== moduleScope.id) continue; + for (const def of scope.ownedDefs) addExport(def); + } + } + + // Member-by-owner pass — keyed on `ownerId`, so it must iterate + // `parsed.localDefs` (class-owned defs live in nested class scopes, + // not the module scope). Requires populateOwners to have run first. for (const def of parsed.localDefs) { + const ownerId = (def as { ownerId?: string }).ownerId; + if (ownerId === undefined) continue; const simple = simpleQualifiedName(def); if (simple === undefined) continue; - // First-seen wins to match `findExportedDef` semantics. - if (!fileBucket.has(simple)) fileBucket.set(simple, def); - - if (def.type === 'Function' || def.type === 'Method' || def.type === 'Constructor') { - let bucket = callablesBySimpleName.get(simple); - if (bucket === undefined) { - bucket = []; - callablesBySimpleName.set(simple, bucket); - } - bucket.push(def); - } - - // member-by-owner: requires populateOwners to have run first. - const ownerId = (def as { ownerId?: string }).ownerId; - if (ownerId !== undefined) { - let memberBucket = memberByOwner.get(ownerId); - if (memberBucket === undefined) { - memberBucket = new Map(); - memberByOwner.set(ownerId, memberBucket); - } - // First-seen wins to match `findOwnedMember` semantics. - if (!memberBucket.has(simple)) memberBucket.set(simple, def); + let memberBucket = memberByOwner.get(ownerId); + if (memberBucket === undefined) { + memberBucket = new Map(); + memberByOwner.set(ownerId, memberBucket); } + // First-seen wins to match `findOwnedMember` semantics. + if (!memberBucket.has(simple)) memberBucket.set(simple, def); } } diff --git a/gitnexus/test/fixtures/lang-resolution/python-module-export-vs-method-collision/app.py b/gitnexus/test/fixtures/lang-resolution/python-module-export-vs-method-collision/app.py new file mode 100644 index 000000000..0d43adc64 --- /dev/null +++ b/gitnexus/test/fixtures/lang-resolution/python-module-export-vs-method-collision/app.py @@ -0,0 +1,11 @@ +import mod +from mod import User + + +def use_module_export() -> None: + mod.save(1) + + +def use_method() -> None: + u = User() + u.save() diff --git a/gitnexus/test/fixtures/lang-resolution/python-module-export-vs-method-collision/mod.py b/gitnexus/test/fixtures/lang-resolution/python-module-export-vs-method-collision/mod.py new file mode 100644 index 000000000..68a296ba1 --- /dev/null +++ b/gitnexus/test/fixtures/lang-resolution/python-module-export-vs-method-collision/mod.py @@ -0,0 +1,21 @@ +""" +Class method declared BEFORE a top-level function with the same +simple name. Order matters for the workspace-resolution-index bug: +without the module-scope filter, `User.save` enters +`defsByFileAndName[mod.py]['save']` first and wins first-seen. Then +`mod.save(x)` silently binds to `User.save` instead of the free +function — the exact wrong-edge symptom Codex flagged. + +Assertions in the paired test pin the intended behavior: `mod.save` +resolves to the top-level Function, `u.save()` resolves to User.save +Method. +""" + + +class User: + def save(self) -> bool: + return True + + +def save(x: int) -> bool: + return x > 0 diff --git a/gitnexus/test/integration/resolvers/python.test.ts b/gitnexus/test/integration/resolvers/python.test.ts index ce8fddc91..c95793fc8 100644 --- a/gitnexus/test/integration/resolvers/python.test.ts +++ b/gitnexus/test/integration/resolvers/python.test.ts @@ -2277,3 +2277,57 @@ describe('Python same-file method-name collision across classes', () => { expect(targets[1]).toContain('User.save'); }); }); + +// --------------------------------------------------------------------------- +// Module export vs class method collision within the same file +// Codex review on PR #980 flagged: buildWorkspaceResolutionIndex feeds +// defsByFileAndName and callablesBySimpleName from parsed.localDefs (every +// def in the file, flat). A class method declared before a top-level +// function with the same simple name wins the file-level export lookup, +// so `mod.save(x)` silently binds to `User.save`. +// --------------------------------------------------------------------------- + +describe('Python module export vs method-name collision in same file', () => { + let result: PipelineResult; + + beforeAll(async () => { + result = await runPipelineFromRepo( + path.join(FIXTURES, 'python-module-export-vs-method-collision'), + () => {}, + ); + }, 60000); + + it('mod.save(x) resolves to the module-level Function, not User.save', () => { + const calls = getRelationships(result, 'CALLS'); + const saveCalls = calls.filter((c) => c.target === 'save'); + const fromModuleExport = saveCalls.find((c) => c.source === 'use_module_export'); + expect(fromModuleExport).toBeDefined(); + // Target must be the top-level Function save, not the User.save Method. + // Node id format: `Function:mod.py:save` vs `Method:mod.py:User.save#0`. + expect(fromModuleExport!.rel.targetId).toContain('Function:'); + expect(fromModuleExport!.rel.targetId).toContain('mod.py:save'); + expect(fromModuleExport!.rel.targetId).not.toContain('User.save'); + }); + + it('u.save() resolves to User.save Method via typed receiver', () => { + const calls = getRelationships(result, 'CALLS'); + const saveCalls = calls.filter((c) => c.target === 'save'); + const fromMethod = saveCalls.find((c) => c.source === 'use_method'); + expect(fromMethod).toBeDefined(); + expect(fromMethod!.rel.targetId).toContain('User.save'); + }); + + it('exactly two CALLS edges to save — one to the free function, one to the method', () => { + const calls = getRelationships(result, 'CALLS'); + const saveCalls = calls.filter((c) => c.target === 'save'); + expect(saveCalls).toHaveLength(2); + const targetIds = saveCalls.map((c) => c.rel.targetId).sort(); + // One Function target, one Method target. Exact shape pins the fix. + const hasFunctionTarget = targetIds.some( + (id) => id.startsWith('Function:') && !id.includes('User.save'), + ); + const hasMethodTarget = targetIds.some((id) => id.includes('User.save')); + expect(hasFunctionTarget).toBe(true); + expect(hasMethodTarget).toBe(true); + }); +});