mirror of
https://github.com/abhigyanpatwari/GitNexus.git
synced 2026-09-15 23:32:49 +00:00
fix(scope-resolution): qualified-name keys for same-file method collisions
Review feedback from PR #980 reviewer flagged a BLOCKING correctness bug: when two classes in the same file define a method with the same simple name (e.g. class User: def save + class Document: def save), every d.save() CALLS edge silently resolved to User.save because the graph node lookup keyed only by (filePath, simpleName) and first-wins took User's method. Three-layer fix: 1. populateClassOwnedMembers now promotes a nested def's qualifiedName from `save` to `ClassName.save` when the def sits inside a class scope. Python's scopes.scm doesn't emit @declaration.qualified_name for methods, so without this the finalized SymbolDefinition carried only the simple name. 2. buildGraphNodeLookup adds a second key per node: (filePath, qualifiedName). For Method/Function nodes the qualifier is parsed deterministically out of the node id (`Method:file.py:User.save#N` → `User.save`), which is robust to Windows-style filePath colons. Simple-name key retained as a fallback for callers that don't know the qualifier. 3. resolveDefGraphId now tries the qualified key first, then falls back to the simple-name lookup. Also addresses the non-blocking review items: - scopeResolutionPhase.deps now includes `crossFile` so the Kahn's runner can't schedule scope-resolution before crossFile finishes writing heritage edges that buildMro consumes. - run.ts no longer mutates the finalized ScopeResolutionIndexes via `as` cast — spreads into a fresh object with the populated methodDispatch field instead. - Doc nits: scope-resolver.ts registry path + phase.ts Ring number. Test coverage: - New fixture test/fixtures/lang-resolution/python-same-file-method-collision with User.save + Document.save in one file and app.py calling both through typed receivers. - Three new integration assertions pin that u.save() and d.save() target the correct qualified node id. Fail before the fix, pass after. Confirmed by running once without populateClassOwnedMembers qualifier promotion — reproduces the original User.save-for-both bug. Verification: 194/194 test/integration/resolvers/python.test.ts pass both REGISTRY_PRIMARY_PYTHON=0 and =1. 523/523 related unit tests. tsc --noEmit clean.
This commit is contained in:
parent
349a3c3860
commit
8f848eafba
9 changed files with 187 additions and 26 deletions
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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}`);
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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<string, string>;
|
||||
|
||||
/**
|
||||
* Parse a qualified name out of a Function/Method node id.
|
||||
*
|
||||
* Node id format: `${label}:${filePath}:${qualifiedName}${arityTag}`,
|
||||
* where `arityTag` is `#<n>` (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<string, string>();
|
||||
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;
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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<ScopeResolutionOutput> = {
|
|||
// 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,
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -155,6 +155,23 @@ export function populateClassOwnedMembers(parsed: ParsedFile): void {
|
|||
const scopesById = new Map<ScopeId, ParsedFile['scopes'][number]>();
|
||||
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);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
|
|
|||
11
gitnexus/test/fixtures/lang-resolution/python-same-file-method-collision/app.py
vendored
Normal file
11
gitnexus/test/fixtures/lang-resolution/python-same-file-method-collision/app.py
vendored
Normal file
|
|
@ -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()
|
||||
22
gitnexus/test/fixtures/lang-resolution/python-same-file-method-collision/models.py
vendored
Normal file
22
gitnexus/test/fixtures/lang-resolution/python-same-file-method-collision/models.py
vendored
Normal file
|
|
@ -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
|
||||
|
|
@ -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');
|
||||
});
|
||||
});
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue