mirror of
https://github.com/abhigyanpatwari/GitNexus.git
synced 2026-10-03 02:21:44 +00:00
fix(scope-resolution): filter export index to module-level defs + label-prefixed qualified key
Codex adversarial review on PR #980 flagged that buildWorkspaceResolutionIndex feeds defsByFileAndName and callablesBySimpleName from parsed.localDefs — the flat set of every def in the file including methods, fields, and nested functions. findExportedDef / findExportedDefByName treat those maps as file-level exports, so `mod.save()` could silently bind to User.save whenever a method's simple name appeared first in parse order. Plan: docs/plans/2026-04-21-001-fix-workspace-index-module-scope-only-plan.md Fix layers: 1. workspace-index.ts: split the single parsed.localDefs loop into two passes: - Module-export pass: iterate moduleScope.ownedDefs PLUS ownedDefs of every child scope whose parent is the module scope. Top-level class and function declarations each live in their own scope with parent=module, not in moduleScope.ownedDefs directly, so the "parent === moduleScope.id" walk is required to reach them. Methods (scope.parent === Class scope) and nested functions (scope.parent === another Function scope) are excluded. - Member-by-owner pass: keeps iterating parsed.localDefs since that map is keyed on ownerId and correctly saw class-owned defs before this change. 2. graph-bridge/node-lookup.ts: qualified keys now live in a separate keyspace (`<q>:filePath::<label>::<qualifiedName>`) and include the node label. Without the label prefix, a top-level `def save` (Function, qualifier `save`) would collide with a class method `User.save` (Method, simple name `save`) in the same simple-key slot because the Function's qualifier happens to equal the Method's simple name. The label differentiates them. 3. graph-bridge/ids.ts: resolveDefGraphId uses the new type-prefixed qualified key when def.type is set. Simple-name fallback retained for languages that don't yet synthesize qualifiers on their defs. Test fixture: python-module-export-vs-method-collision places `class User: def save` BEFORE top-level `def save` — parse order that exposes the bug (class method enters the index first). Three new integration assertions: - `mod.save(x)` resolves to the module-level Function, not User.save - `u.save()` resolves to User.save Method - Exactly two CALLS edges to `save` exist, one per intended target Fixture confirmed failing before the workspace-index fix (bug reproduced), passing after. Verification: 197/197 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
8f848eafba
commit
a0bce3a8cf
6 changed files with 206 additions and 46 deletions
|
|
@ -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`. */
|
||||
|
|
|
|||
|
|
@ -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 `<q>` 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 `<q>:${filePath}::${label}::${qualifiedName}`;
|
||||
}
|
||||
|
||||
/** Simple-name key (legacy fallback keyspace — no `<q>` prefix). */
|
||||
export function simpleKey(filePath: string, name: string): string {
|
||||
return `${filePath}::${name}`;
|
||||
}
|
||||
|
||||
export function buildGraphNodeLookup(graph: KnowledgeGraph): GraphNodeLookup {
|
||||
const lookup = new Map<string, string>();
|
||||
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;
|
||||
}
|
||||
|
|
|
|||
|
|
@ -26,16 +26,24 @@ export interface WorkspaceResolutionIndex {
|
|||
readonly classScopeByDefId: ReadonlyMap<string, Scope>;
|
||||
|
||||
/** 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<string, ReadonlyMap<string, SymbolDefinition>>;
|
||||
|
||||
/** 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<string, ReadonlyMap<string, SymbolDefinition>>;
|
||||
|
||||
/** 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<string, readonly SymbolDefinition[]>;
|
||||
|
||||
/** 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);
|
||||
}
|
||||
}
|
||||
|
||||
|
|
|
|||
11
gitnexus/test/fixtures/lang-resolution/python-module-export-vs-method-collision/app.py
vendored
Normal file
11
gitnexus/test/fixtures/lang-resolution/python-module-export-vs-method-collision/app.py
vendored
Normal file
|
|
@ -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()
|
||||
21
gitnexus/test/fixtures/lang-resolution/python-module-export-vs-method-collision/mod.py
vendored
Normal file
21
gitnexus/test/fixtures/lang-resolution/python-module-export-vs-method-collision/mod.py
vendored
Normal file
|
|
@ -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
|
||||
|
|
@ -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);
|
||||
});
|
||||
});
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue