mirror of
https://github.com/abhigyanpatwari/GitNexus.git
synced 2026-10-05 02:43:32 +00:00
fix(scope-resolution): drive module export index from moduleScope.bindings
Codex round-2 adversarial review flagged that the workspace-index module-export pass iterated every def in every direct-child scope of the module, including class-body Variable defs like `class User: MAX_USERS = 100`. `defsByFileAndName[file][MAX_USERS]` silently aliased to the class attribute. Latent today because Python doesn't emit ACCESSES edges for `mod.NAME` member access, but the index-layer leak would surface the moment reference capture widens. Plan: docs/plans/2026-04-21-002-fix-codex-round2-scope-resolution-plan.md Drive the module-export index from the extractor invariant instead of a scope-kind → allowed-label switch: moduleScope.bindings already contains exactly the names visible at module level — top-level class/function declarations, module-level variable assignments, imports. Class methods, class-body attributes, and nested-function defs bind to their containing (Class or Function) scope, not the module, so they're naturally excluded. Filter to `BindingRef.origin === 'local'` so imports and wildcard re-exports stay out of the index (matches the pre-fix invariant when the source was `parsed.localDefs`). No per-kind predicates, no scope-kind / def-kind enumeration, no two-pass merge between moduleScope.ownedDefs and direct-child scope walks — one loop, language-agnostic. Codex also flagged `propagateImportedReturnTypes` as potentially broken for function-local imports, but scope-dump probing showed the finalize algorithm puts `from svc import get_user` into the MODULE scope's finalized bindings even when declared inside a function, so the existing module-scope propagation already handles the case. The new python-function-local-import-chain integration test pins that working behavior as a regression guard; no code change required. Coverage: - test/unit/scope-resolution/workspace-index.test.ts (new, 5 tests) — directly asserts the index shape. The "excludes class-body Variable defs" test fails without this fix and passes after (confirmed via stash-pop probe). - test/integration/resolvers/python.test.ts — 4 new integration assertions across two describe blocks (python-class-attr-export-leak, python-function-local-import-chain) pin end-to-end invariants. - Two new fixtures under test/fixtures/lang-resolution/. Verification: 201/201 test/integration/resolvers/python.test.ts both REGISTRY_PRIMARY_PYTHON=0 and =1. 528/528 related unit tests (was 523). tsc clean.
This commit is contained in:
parent
a0bce3a8cf
commit
e27843774c
7 changed files with 305 additions and 35 deletions
|
|
@ -72,50 +72,43 @@ export function buildWorkspaceResolutionIndex(
|
|||
if (cd !== undefined) classScopeByDefId.set(cd.nodeId, scope);
|
||||
}
|
||||
|
||||
// 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).
|
||||
// Module-export pass — use the module scope's own `bindings` map
|
||||
// as the source of truth for "what names this module exports".
|
||||
// The scope extractor populates moduleScope.bindings with exactly
|
||||
// the names visible at module level: top-level class/function
|
||||
// declarations, module-level variable assignments, imports, etc.
|
||||
// Filtering to `origin === 'local'` keeps only locally-defined
|
||||
// names (not imports or wildcard re-exports brought in from
|
||||
// elsewhere), which matches the pre-fix invariant that
|
||||
// defsByFileAndName was built from `parsed.localDefs`.
|
||||
//
|
||||
// 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.
|
||||
// Class methods, class-body attributes, and nested-function defs
|
||||
// are NOT in moduleScope.bindings — they're bound at their
|
||||
// containing (Class or Function) scope — so they're naturally
|
||||
// excluded, no per-kind filter required.
|
||||
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);
|
||||
for (const [, refs] of moduleScope.bindings) {
|
||||
for (const ref of refs) {
|
||||
if (ref.origin !== 'local') continue;
|
||||
const def = ref.def;
|
||||
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);
|
||||
}
|
||||
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);
|
||||
}
|
||||
}
|
||||
|
||||
|
|
|
|||
11
gitnexus/test/fixtures/lang-resolution/python-class-attr-export-leak/app.py
vendored
Normal file
11
gitnexus/test/fixtures/lang-resolution/python-class-attr-export-leak/app.py
vendored
Normal file
|
|
@ -0,0 +1,11 @@
|
|||
import mod
|
||||
|
||||
|
||||
def use_class_attr() -> int:
|
||||
# Would silently bind to User.MAX_USERS without the fix.
|
||||
return mod.MAX_USERS
|
||||
|
||||
|
||||
def use_helper() -> int:
|
||||
# Happy-path guard: legitimate top-level function export must still resolve.
|
||||
return mod.helper()
|
||||
21
gitnexus/test/fixtures/lang-resolution/python-class-attr-export-leak/mod.py
vendored
Normal file
21
gitnexus/test/fixtures/lang-resolution/python-class-attr-export-leak/mod.py
vendored
Normal file
|
|
@ -0,0 +1,21 @@
|
|||
"""
|
||||
Class-body attribute (`User.MAX_USERS`) that MUST NOT leak into the
|
||||
module's export index. `from mod import MAX_USERS` / `mod.MAX_USERS`
|
||||
should find nothing — there is no top-level `MAX_USERS` at module
|
||||
scope.
|
||||
|
||||
Also includes a top-level `def helper()` as a happy-path guard: the
|
||||
narrowing fix must not over-narrow and drop legitimate module-level
|
||||
function exports.
|
||||
"""
|
||||
|
||||
|
||||
class User:
|
||||
MAX_USERS = 100
|
||||
|
||||
def save(self) -> bool:
|
||||
return True
|
||||
|
||||
|
||||
def helper() -> int:
|
||||
return 42
|
||||
9
gitnexus/test/fixtures/lang-resolution/python-function-local-import-chain/app.py
vendored
Normal file
9
gitnexus/test/fixtures/lang-resolution/python-function-local-import-chain/app.py
vendored
Normal file
|
|
@ -0,0 +1,9 @@
|
|||
def do_work() -> bool:
|
||||
# Function-local import — pythonImportOwningScope pins `get_user`
|
||||
# to the function scope, not the module scope. The cross-file
|
||||
# return-type propagation pass must mirror the return type into
|
||||
# THIS scope's typeBindings, or `u.save()` misses its edge.
|
||||
from svc import get_user
|
||||
|
||||
u = get_user()
|
||||
return u.save()
|
||||
16
gitnexus/test/fixtures/lang-resolution/python-function-local-import-chain/svc.py
vendored
Normal file
16
gitnexus/test/fixtures/lang-resolution/python-function-local-import-chain/svc.py
vendored
Normal file
|
|
@ -0,0 +1,16 @@
|
|||
"""
|
||||
Provider module for the function-local-import propagation test.
|
||||
`get_user` returns a `User` instance; the importer calls
|
||||
`u = get_user(); u.save()` from INSIDE a function body, so the
|
||||
`from svc import get_user` binding lives on the function scope, not
|
||||
the module scope.
|
||||
"""
|
||||
|
||||
|
||||
class User:
|
||||
def save(self) -> bool:
|
||||
return True
|
||||
|
||||
|
||||
def get_user() -> User:
|
||||
return User()
|
||||
|
|
@ -2331,3 +2331,83 @@ describe('Python module export vs method-name collision in same file', () => {
|
|||
expect(hasMethodTarget).toBe(true);
|
||||
});
|
||||
});
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
// Class-body attribute leak into module export index
|
||||
// Codex round-2 review on PR #980: defsByFileAndName indexes ALL defs
|
||||
// owned by every child scope of the module, including class-body defs
|
||||
// (e.g. `User.MAX_USERS`). `mod.MAX_USERS` / `from mod import MAX_USERS`
|
||||
// can silently bind to a class attribute that's not a module export.
|
||||
// ---------------------------------------------------------------------------
|
||||
|
||||
describe('Python class-body attribute does NOT leak into module export index', () => {
|
||||
let result: PipelineResult;
|
||||
|
||||
beforeAll(async () => {
|
||||
result = await runPipelineFromRepo(
|
||||
path.join(FIXTURES, 'python-class-attr-export-leak'),
|
||||
() => {},
|
||||
);
|
||||
}, 60000);
|
||||
|
||||
it('mod.MAX_USERS does not resolve to User.MAX_USERS as a module export', () => {
|
||||
// Any edge sourced from `use_class_attr` must NOT target a node
|
||||
// that represents `User.MAX_USERS`. Under the bug, CALLS/USES/
|
||||
// ACCESSES could silently bind to the class attribute.
|
||||
const edges = [
|
||||
...getRelationships(result, 'CALLS'),
|
||||
...getRelationships(result, 'USES'),
|
||||
...getRelationships(result, 'ACCESSES'),
|
||||
];
|
||||
const fromConsumer = edges.filter((e) => e.source === 'use_class_attr');
|
||||
for (const edge of fromConsumer) {
|
||||
expect(edge.rel.targetId).not.toContain('User.MAX_USERS');
|
||||
}
|
||||
});
|
||||
|
||||
it('mod.helper() still resolves to the top-level Function (happy-path guard)', () => {
|
||||
// Regression guard: the narrowing fix must not drop legitimate
|
||||
// top-level function exports. Without this, the fix would over-
|
||||
// narrow and break normal `mod.helper()` calls.
|
||||
const calls = getRelationships(result, 'CALLS');
|
||||
const helperCall = calls.find((c) => c.source === 'use_helper' && c.target === 'helper');
|
||||
expect(helperCall).toBeDefined();
|
||||
expect(helperCall!.rel.targetId).toContain('mod.py:helper');
|
||||
});
|
||||
});
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
// Function-local import + cross-file return-type propagation
|
||||
// Codex round-2 flagged this as potentially broken, but empirically the
|
||||
// finalize-algorithm hoists the `from svc import get_user` binding to
|
||||
// the app.py module scope (observed via indexes.bindings dump), so
|
||||
// `propagateImportedReturnTypes`'s module-scope pass already handles
|
||||
// it. These assertions pin that working behavior as a regression
|
||||
// guard against any future change to binding-scope routing.
|
||||
// ---------------------------------------------------------------------------
|
||||
|
||||
describe('Python function-local import feeds chained receiver-bound call', () => {
|
||||
let result: PipelineResult;
|
||||
|
||||
beforeAll(async () => {
|
||||
result = await runPipelineFromRepo(
|
||||
path.join(FIXTURES, 'python-function-local-import-chain'),
|
||||
() => {},
|
||||
);
|
||||
}, 60000);
|
||||
|
||||
it('emits CALLS edge do_work -> get_user (free call, baseline sanity)', () => {
|
||||
const calls = getRelationships(result, 'CALLS');
|
||||
const getUserCall = calls.find((c) => c.source === 'do_work' && c.target === 'get_user');
|
||||
expect(getUserCall).toBeDefined();
|
||||
expect(getUserCall!.rel.targetId).toContain('svc.py:get_user');
|
||||
});
|
||||
|
||||
it('emits CALLS edge do_work -> User.save via function-local-scoped import return-type', () => {
|
||||
const calls = getRelationships(result, 'CALLS');
|
||||
const saveCall = calls.find((c) => c.source === 'do_work' && c.target === 'save');
|
||||
expect(saveCall).toBeDefined();
|
||||
// Target must be the User.save Method in svc.py.
|
||||
expect(saveCall!.rel.targetId).toContain('User.save');
|
||||
});
|
||||
});
|
||||
|
|
|
|||
140
gitnexus/test/unit/scope-resolution/workspace-index.test.ts
Normal file
140
gitnexus/test/unit/scope-resolution/workspace-index.test.ts
Normal file
|
|
@ -0,0 +1,140 @@
|
|||
/**
|
||||
* Directly assert the shape of `WorkspaceResolutionIndex` — in
|
||||
* particular that `defsByFileAndName` and `callablesBySimpleName`
|
||||
* filter class-body attributes and nested-function locals out of the
|
||||
* file-level export keyspace.
|
||||
*
|
||||
* The equivalent integration-level assertions in
|
||||
* `test/integration/resolvers/python.test.ts` (see the
|
||||
* `python-class-attr-export-leak` fixture) cover the downstream
|
||||
* edge-emission path. This unit test pins the index shape directly
|
||||
* because the downstream consumer in Python today doesn't emit an
|
||||
* ACCESSES edge for `mod.NAME` member access, so the leak would be
|
||||
* latent at the index layer until a future capture path makes it
|
||||
* visible. That's precisely when a unit-level pin is most valuable.
|
||||
*/
|
||||
|
||||
import { describe, it, expect } from 'vitest';
|
||||
import { extractParsedFile } from '../../../src/core/ingestion/scope-extractor-bridge.js';
|
||||
import { pythonScopeResolver } from '../../../src/core/ingestion/languages/python/scope-resolver.js';
|
||||
import { buildWorkspaceResolutionIndex } from '../../../src/core/ingestion/scope-resolution/workspace-index.js';
|
||||
|
||||
function parsePython(source: string, filePath: string) {
|
||||
const parsed = extractParsedFile(
|
||||
pythonScopeResolver.languageProvider,
|
||||
source,
|
||||
filePath,
|
||||
() => {},
|
||||
);
|
||||
if (parsed === undefined) throw new Error('scope extraction failed');
|
||||
return parsed;
|
||||
}
|
||||
|
||||
describe('buildWorkspaceResolutionIndex — module-export filter', () => {
|
||||
it('keeps top-level class and function defs', () => {
|
||||
const parsed = parsePython(
|
||||
`
|
||||
class User:
|
||||
def save(self) -> bool:
|
||||
return True
|
||||
|
||||
def helper() -> int:
|
||||
return 42
|
||||
`,
|
||||
'mod.py',
|
||||
);
|
||||
const index = buildWorkspaceResolutionIndex([parsed]);
|
||||
const fileBucket = index.defsByFileAndName.get('mod.py');
|
||||
expect(fileBucket).toBeDefined();
|
||||
expect(fileBucket!.get('User')?.type).toBe('Class');
|
||||
expect(fileBucket!.get('helper')?.type).toBe('Function');
|
||||
});
|
||||
|
||||
it('excludes class-body Variable defs from defsByFileAndName', () => {
|
||||
// Python's scope extractor captures `MAX_USERS = 100` inside a
|
||||
// class body as `Variable:MAX_USERS` in the Class scope's
|
||||
// ownedDefs. Without the scope-defining-def filter, this entry
|
||||
// would leak into defsByFileAndName['mod.py']['MAX_USERS'] and
|
||||
// `mod.MAX_USERS` / `from mod import MAX_USERS` would silently
|
||||
// resolve to the class attribute.
|
||||
const parsed = parsePython(
|
||||
`
|
||||
class User:
|
||||
MAX_USERS = 100
|
||||
`,
|
||||
'mod.py',
|
||||
);
|
||||
const index = buildWorkspaceResolutionIndex([parsed]);
|
||||
const fileBucket = index.defsByFileAndName.get('mod.py');
|
||||
expect(fileBucket).toBeDefined();
|
||||
expect(fileBucket!.get('MAX_USERS')).toBeUndefined();
|
||||
// Positive-case invariant: the Class def itself is still exported.
|
||||
expect(fileBucket!.get('User')?.type).toBe('Class');
|
||||
});
|
||||
|
||||
it('excludes class methods from defsByFileAndName', () => {
|
||||
// A method lives in a Function scope whose parent is the Class
|
||||
// scope (not the Module), so it shouldn't be reachable through
|
||||
// the direct-child filter at all. Guard against a regression to
|
||||
// the earlier "method wins module-export slot" bug.
|
||||
const parsed = parsePython(
|
||||
`
|
||||
class User:
|
||||
def save(self) -> bool:
|
||||
return True
|
||||
`,
|
||||
'mod.py',
|
||||
);
|
||||
const index = buildWorkspaceResolutionIndex([parsed]);
|
||||
const fileBucket = index.defsByFileAndName.get('mod.py');
|
||||
expect(fileBucket).toBeDefined();
|
||||
// `save` is a method — NOT a module export.
|
||||
expect(fileBucket!.get('save')).toBeUndefined();
|
||||
expect(fileBucket!.get('User')?.type).toBe('Class');
|
||||
});
|
||||
|
||||
it('excludes class methods from callablesBySimpleName', () => {
|
||||
const parsed = parsePython(
|
||||
`
|
||||
class User:
|
||||
def save(self) -> bool:
|
||||
return True
|
||||
|
||||
def save(x: int) -> int:
|
||||
return x
|
||||
`,
|
||||
'mod.py',
|
||||
);
|
||||
const index = buildWorkspaceResolutionIndex([parsed]);
|
||||
const saves = index.callablesBySimpleName.get('save') ?? [];
|
||||
// Only the top-level `def save(x)` is a module-level callable.
|
||||
// The `User.save` method lives under a Class scope and must not
|
||||
// appear in the workspace callable fallback.
|
||||
expect(saves).toHaveLength(1);
|
||||
expect(saves[0].qualifiedName).toBe('save');
|
||||
});
|
||||
|
||||
it('keeps memberByOwner populated for class methods (unchanged contract)', () => {
|
||||
// Regression guard: the narrowing of defsByFileAndName must NOT
|
||||
// collaterally drop class-method entries from memberByOwner.
|
||||
// findOwnedMember relies on this for receiver-bound dispatch.
|
||||
const parsed = parsePython(
|
||||
`
|
||||
class User:
|
||||
def save(self) -> bool:
|
||||
return True
|
||||
`,
|
||||
'mod.py',
|
||||
);
|
||||
pythonScopeResolver.populateOwners(parsed);
|
||||
const index = buildWorkspaceResolutionIndex([parsed]);
|
||||
// User's nodeId is derivable from its ownedDefs — find the Class
|
||||
// scope's Class def and look up 'save' under its nodeId.
|
||||
const classScope = parsed.scopes.find((s) => s.kind === 'Class');
|
||||
const classDef = classScope?.ownedDefs.find((d) => d.type === 'Class');
|
||||
expect(classDef).toBeDefined();
|
||||
const members = index.memberByOwner.get(classDef!.nodeId);
|
||||
expect(members).toBeDefined();
|
||||
expect(members!.get('save')?.type).toBe('Function');
|
||||
});
|
||||
});
|
||||
Loading…
Add table
Reference in a new issue