From 1ba05987955acad2c2bc60fc477cedabd1127377 Mon Sep 17 00:00:00 2001 From: Colby McHenry Date: Sat, 31 Oct 2026 11:16:29 -0500 Subject: [PATCH] fix(python): a script's import of a module in a folder above it links again PR #1921 made Python imports resolve from the package roots a project has, and stopped binding by any file whose path ends in the module's name. A folder that is neither a root nor the importer's own is skipped on purpose, so this common script layout lost its links: redis's modules/vector-sets/tests/*.py do from test import TestCase, and modules/vector-sets/test.py is the runner whose folder is first on sys.path. No root says so. On redis, main lost 137 true edges (69 imports, 68 calls) that 1.6.2 had. As a last step, resolvePythonModule now walks the importer's ancestors for the module, nearest first and passing over packages. A name like test or utils may just as well be the stdlib's, so what proves the module is what the importer takes from it: every name must be one the file defines at module level. A plain import or a star import takes no name, so it proves nothing and stays unresolved, as on main. A module a root provides still answers first. Reading the importer's own from-imports keeps this inside the resolver, so every caller gets one answer: context.resolveModuleFile finds the module too, and the outside-the-project check from PR #3109 does not count it as outside. A sync follows it. Such a module binds only while it defines those names, so an edit can link or unlink importers though no file appeared or vanished. An edited module in a folder that is neither a package nor a root, with Python folders below it, now re-opens the edges into it and the failures naming it. Any other edit re-opens nothing. Validation: - 9 new tests in python-module-resolution.test.ts, 2 of them on sync. Without the fallback the 4 that expect a link fail, and the 5 that expect none pass, as they should. Here all 9 pass. Taking out each guard (every name, plain imports, packages passed over, the nearest module with the names, the re-open on edit) fails exactly its own test. - A/B against main on 14 Python repos plus express and gin, 741,560 edges: 173 added, 0 removed, 0 moved, 44 extends upgraded from exact-match to import, all in redis. All 137 edges 1.6.2 had and main lost are back. The other 36 added are the file-level imports of test.py, as a from-import from the same folder gets. The controls' full dumps are byte-identical. - A trace of the fallback found every candidate it could see on those repos and on whole django, polar's server, ansible and cpython: redis's test.py from its 36 test files, all proven, and nothing else, so no wrong edge and no rejected candidate. No plain or star import had a candidate, so leaving those unresolved costs nothing on these repos. - Index cost on redis, median of 3 interleaved: 12.0 s on main, 10.7 s here, which is noise. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 2 +- __tests__/python-module-resolution.test.ts | 209 +++++++++++++++++++++ src/index.ts | 10 + src/resolution/import-resolver.ts | 61 +++++- src/resolution/index.ts | 7 +- 5 files changed, 286 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a10c140b0f..752cc21558 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -414,7 +414,7 @@ and adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). - A call on a variable or attribute named after a class, like `client` or `nodelist`, now prefers the class it spells whole, `Client` or `NodeList`, over another that only shares a word with it, like `RedisCacheClient`. - A call through an attribute of an imported value, like flask's `app.jinja_env.filters.keys()`, no longer links to the value's own `jinja_env`: the method called is `keys`. Re-index to update an existing project. - A call through a class name, like `Logger.log()`, or through a variable named after one, like `logger.log()`, now links to that class's own method or the one it inherits, such as Django's `form.is_valid()` reaching `BaseForm`, instead of the same-named method of a class whose name merely contains it, such as `FileLogger`. These calls also no longer link to a class a test declares inside one of its functions, or, from your own code, to a class in a test suite. Re-index to update an existing project. Thanks @inth3shadows. -- Python imports now resolve the way Python finds modules: from your project's package roots — the repository root, `src/` layouts, each service with its own `pyproject.toml`, `setup.cfg` or `setup.py`, and the package directories those declare — so `from shop.cart import total` reaches `src/shop/cart.py`, `from pkg import name` reaches what the package's `__init__.py` defines or re-exports (respecting `__all__`), and `import json` or `import requests` no longer links to a same-named file nested in a test fixture, an example, a vendored copy or another package. After a module, package or `__init__.py` is added or removed, a package's `__init__.py` is edited, or a build config is edited, the next sync brings Python imports back in line with a fresh index; the file watcher doesn't react to a build-config edit by itself, so run `codegraph sync` after one. Re-index Python projects after upgrading. (#1704) +- Python imports now resolve the way Python finds modules: from your project's package roots — the repository root, `src/` layouts, each service with its own `pyproject.toml`, `setup.cfg` or `setup.py`, and the package directories those declare — so `from shop.cart import total` reaches `src/shop/cart.py`, `from pkg import name` reaches what the package's `__init__.py` defines or re-exports (respecting `__all__`), and `import json` or `import requests` no longer links to a same-named file nested in a test fixture, an example, a vendored copy or another package. A script can still import a module from a folder above it, like `from test import TestCase` in a `tests/` folder with the runner `test.py` one level up, when that module defines every name the script takes from it. After a module, package or `__init__.py` is added or removed, a package's `__init__.py` is edited, or a build config is edited, the next sync brings Python imports back in line with a fresh index; the file watcher doesn't react to a build-config edit by itself, so run `codegraph sync` after one. Re-index Python projects after upgrading. (#1704) - An Express route keeps its named handler when a middleware before it contains an arrow function. `rateLimit({ keyGenerator: (req) => req.ip })` or `(req, res, next) => next()` no longer hides the handler at the end of the route (re-index to update an existing project). - A TypeScript type re-exported through a barrel (`export type { Foo } from`, `export { type Foo } from`, `export type * from`) now links to its real declaration instead of a same-named type elsewhere in the project, and import lists with comments, string names, or a JSDoc `@import` are read correctly. Re-index to update an existing project. - In TypeScript, a file that re-exports a whole module's types, like `export type * from './types'` or `export type * as Types from './types'`, now parses cleanly. Before, the parser couldn't read that line, so a barrel made of re-exports was reported as producing no symbols and `codegraph status` listed it among the files it could not parse. Re-index a project that uses it after upgrading. Thanks @gavinwilliams for the report and @danusha2345 for the pull request. (#2474) diff --git a/__tests__/python-module-resolution.test.ts b/__tests__/python-module-resolution.test.ts index 99a0a1ec8d..dae97f2ac6 100644 --- a/__tests__/python-module-resolution.test.ts +++ b/__tests__/python-module-resolution.test.ts @@ -736,6 +736,170 @@ def main(): }); }); + // A runner's folder is first on `sys.path` for the tests it loads from a + // folder below it (redis's modules/vector-sets/test.py and tests/). No root + // says so, so the names the importer takes are what prove the module. + describe('a module in a folder above a script', () => { + const runner = `class TestCase: + def run(self): + return 1 + + +def generate_vector(n): + return [0] * n +`; + + it('binds the names it imports from a module in a parent folder', async () => { + write({ + 'modules/vs/test.py': runner, + 'modules/vs/tests/with_a.py': `from test import TestCase, generate_vector + + +class WithA(TestCase): + pass + + +def go(): + return generate_vector(3) + + +def inherited(): + WithA.run(None) +`, + }); + cg = await CodeGraph.init(tempDir, { index: true }); + + expect(targets('go', 'modules/vs/tests/with_a.py', 'calls')).toEqual(['generate_vector@modules/vs/test.py']); + expect(targets('with_a.py', 'modules/vs/tests/with_a.py', 'imports').sort()).toEqual([ + 'TestCase@modules/vs/test.py', 'generate_vector@modules/vs/test.py', 'test.py@modules/vs/test.py', + ]); + // The check for a base from outside the project agrees: this one isn't. + expect(targets('inherited', 'modules/vs/tests/with_a.py', 'calls')).toEqual(['run@modules/vs/test.py']); + }); + + it('walks up more than one folder, and takes the nearest module that has the names', async () => { + write({ + 'tools/helper.py': 'def assist(n):\n return n\n', + 'tools/sub/helper.py': 'def other(n):\n return n\n', + 'tools/sub/deep/er/run.py': `from helper import assist + + +def go(): + return assist(1) +`, + 'lib/a/common.py': 'def shared():\n return 1\n', + 'lib/common.py': 'def shared():\n return 2\n', + 'lib/a/b/use.py': `from common import shared + + +def go2(): + return shared() +`, + }); + cg = await CodeGraph.init(tempDir, { index: true }); + + expect(targets('go', 'tools/sub/deep/er/run.py', 'calls')).toEqual(['assist@tools/helper.py']); + expect(targets('go2', 'lib/a/b/use.py', 'calls')).toEqual(['shared@lib/a/common.py']); + }); + + it('binds nothing when the module lacks a name the importer takes from it', async () => { + write({ + // Named like the stdlib's: only the name it lacks keeps the import off it. + 'tools/time.py': 'def clock():\n return 0\n', + 'tools/sub/wait.py': `from time import sleep + + +def nap(): + sleep(1) +`, + 'tools/helper.py': 'def assist(n):\n return n\n', + 'tools/sub/partly.py': `from helper import assist, missing + + +def go(): + return assist(missing) +`, + }); + cg = await CodeGraph.init(tempDir, { index: true }); + + expect(targets('nap', 'tools/sub/wait.py', 'calls')).toEqual([]); + expect(targets('wait.py', 'tools/sub/wait.py', 'imports')).toEqual([]); + expect(targets('partly.py', 'tools/sub/partly.py', 'imports')).toEqual([]); + }); + + it('binds nothing for a plain import, which takes no names to prove the module', async () => { + write({ + 'tools/utils.py': 'def assist(n):\n return n\n', + 'tools/sub/plain.py': `import utils + + +def go(): + return utils.assist(1) +`, + }); + cg = await CodeGraph.init(tempDir, { index: true }); + + expect(targets('go', 'tools/sub/plain.py', 'calls')).toEqual([]); + expect(targets('plain.py', 'tools/sub/plain.py', 'imports')).toEqual([]); + }); + + it('binds nothing through a star import, whose names it cannot read', async () => { + write({ + 'tools/helper.py': 'def assist(n):\n return n\n', + 'tools/sub/star.py': `from helper import * + + +def go(): + return assist(1) +`, + 'other/assist.py': 'def other(n):\n return n\n', + }); + cg = await CodeGraph.init(tempDir, { index: true }); + + expect(targets('star.py', 'tools/sub/star.py', 'imports')).toEqual([]); + }); + + it('looks in no sibling folder and no package above the script', async () => { + write({ + 'tools/other/helper.py': 'def assist(n):\n return n\n', + 'tools/sub/run.py': `from helper import assist + + +def go(): + return assist(1) +`, + 'pkg/__init__.py': '', + 'pkg/shared.py': 'def share(n):\n return n\n', + 'pkg/scripts/run.py': `from shared import share + + +def go2(): + return share(1) +`, + }); + cg = await CodeGraph.init(tempDir, { index: true }); + + expect(targets('run.py', 'tools/sub/run.py', 'imports')).toEqual([]); + expect(targets('run.py', 'pkg/scripts/run.py', 'imports')).toEqual([]); + }); + + it('leaves a module a root provides to the root', async () => { + write({ + 'helper.py': 'def assist(n):\n return "root"\n', + 'tools/helper.py': 'def assist(n):\n return "above"\n', + 'tools/sub/run.py': `from helper import assist + + +def go(): + return assist(1) +`, + }); + cg = await CodeGraph.init(tempDir, { index: true }); + + expect(targets('go', 'tools/sub/run.py', 'calls')).toEqual(['assist@helper.py']); + }); + }); + describe('sync', () => { /** Every Python calls/imports edge, as a sorted list of readable triples. */ function pythonEdges(graph: CodeGraph): string[] { @@ -950,6 +1114,51 @@ def main(): )); }); + describe('a module in a folder above a script', () => { + const importer = `from helper import assist + + +def go(): + return assist(1) +`; + + it('links it when the module is added, and unlinks it when removed', async () => { + write({ 'tools/sub/run.py': importer, 'tools/other.py': 'def other():\n return 0\n' }); + cg = await CodeGraph.init(tempDir, { index: true }); + expect(pythonEdges(cg).join('\n')).not.toContain('assist'); + + write({ 'tools/helper.py': 'def assist(n):\n return n\n' }); + await cg.sync(); + let fresh = await freshEdges(); + expect(fresh).toContain('calls go@tools/sub/run.py -> function:assist@tools/helper.py'); + expect(pythonEdges(cg)).toEqual(fresh); + + fs.rmSync(path.join(tempDir, 'tools/helper.py')); + await cg.sync(); + fresh = await freshEdges(); + expect(fresh.join('\n')).not.toContain('assist'); + expect(pythonEdges(cg)).toEqual(fresh); + }); + + it('follows an edit that gives the module the name, or takes it away', async () => { + write({ 'tools/sub/run.py': importer, 'tools/helper.py': 'def other():\n return 0\n' }); + cg = await CodeGraph.init(tempDir, { index: true }); + expect(pythonEdges(cg).join('\n')).not.toContain('assist'); + + write({ 'tools/helper.py': 'def assist(n):\n return n\n' }); + await cg.sync(); + let fresh = await freshEdges(); + expect(fresh).toContain('calls go@tools/sub/run.py -> function:assist@tools/helper.py'); + expect(pythonEdges(cg)).toEqual(fresh); + + write({ 'tools/helper.py': 'def other():\n return 0\n' }); + await cg.sync(); + fresh = await freshEdges(); + expect(fresh.join('\n')).not.toContain('assist'); + expect(pythonEdges(cg)).toEqual(fresh); + }); + }); + it('re-opens nothing on a sync that changes nothing', async () => { write(subpackageRepo); cg = await CodeGraph.init(tempDir, { index: true }); diff --git a/src/index.ts b/src/index.ts index 56ea79d9ed..99e0eaeb59 100644 --- a/src/index.ts +++ b/src/index.ts @@ -1654,6 +1654,16 @@ export class CodeGraph { this.resolver.clearCaches(); for (const f of this.orchestrator.resurrectPythonPackageImporters(this.resolver.getPythonPackageImporters(editedInits))) reopened.add(f); } + // An edit can give a module above a script the names its importers + // take from it, or take them away. + const scriptModules = this.resolver.getPythonScriptModuleScope( + (result.changedFilePaths ?? []).filter((f) => pythonFilesBefore.has(f) && after.has(f)) + ); + if (scriptModules.targetFiles.length > 0) { + this.resolver.clearCaches(); + for (const f of this.orchestrator.resurrectResolutionEdgesTouching('python', scriptModules)) reopened.add(f); + } + // Resolve the narrow set now, the way the changed files' own // references were resolved above, rather than leaving a handful of // refs to the orphan sweep and its whole-graph synthesis pass. diff --git a/src/resolution/import-resolver.ts b/src/resolution/import-resolver.ts index aca107fe72..ebd4d93c7e 100644 --- a/src/resolution/import-resolver.ts +++ b/src/resolution/import-resolver.ts @@ -891,7 +891,39 @@ function resolvePythonModule( if (found !== null && found !== hit) return null; found = hit; } - return found; + return found ?? pythonModuleAboveScript(specifier, relPath, fromFile, importerDir, index, context); +} + +/** + * The module in a folder above a script that no root provides: `from test + * import TestCase` in `modules/vs/tests/with_a.py`, with the runner + * `modules/vs/test.py` whose folder is first on `sys.path`. Nothing in the tree + * says that folder is a root, and a name like `test` or `utils` may just as + * well be the stdlib's, so what proves the module is what the importer takes + * from it: every name must be one the file defines at module level. A plain + * `import utils` or a star import takes no name, so it proves nothing and stays + * unresolved. Nearest folder first; a package is no root, so it is passed over. + */ +function pythonModuleAboveScript( + specifier: string, + relPath: string, + fromFile: string, + importerDir: string, + index: PythonModuleIndex, + context: ResolutionContext +): string | null { + const taken = context.getImportMappings(fromFile, 'python') + .filter((imp) => imp.source === specifier && !imp.isNamespace) + .map((imp) => imp.exportedName); + if (taken.length === 0) return null; + for (let dir = parentDir(importerDir); dir; dir = parentDir(dir)) { + if (index.packageDirs.has(dir)) continue; + const file = pythonModuleUnder(dir, relPath, index); + if (!file) continue; + const defined = new Set(context.getNodesInFile(file).filter(isPythonModuleLevelDef).map((n) => n.name)); + if (taken.every((name) => defined.has(name))) return file; + } + return null; } /** @@ -960,6 +992,33 @@ export function pythonReopenScope( return { targetFiles, sourceDirs: [...sourceDirs], moduleLeaves }; } +/** + * What an edit of these Python files can move through + * {@link pythonModuleAboveScript}: such a module binds only while it defines + * the names its importers take, so an edit links or unlinks them though no file + * appeared or vanished. Only a module in a folder that is neither a package nor + * a root, with Python folders below it, can be one, so any other edit re-opens + * nothing. + */ +export function pythonScriptModuleScope( + context: ResolutionContext, + editedFiles: string[] +): { targetFiles: string[]; sourceDirs: string[]; moduleLeaves: string[] } { + const index = pythonModuleIndex(context); + const targetFiles: string[] = []; + const moduleLeaves: string[] = []; + for (const raw of editedFiles) { + const file = raw.replace(/\\/g, '/'); + const dir = parentDir(file); + if (!PYTHON_MODULE_FILE.test(file) || !dir || index.packageDirs.has(dir) || + index.projectRoots.has(dir) || index.localRoots.has(dir)) continue; + if (![...index.dirs].some((d) => d.startsWith(`${dir}/`))) continue; + targetFiles.push(file); + moduleLeaves.push(file.slice(dir.length + 1, -'.py'.length)); + } + return { targetFiles, sourceDirs: [], moduleLeaves }; +} + /** * The Python files that import one of the packages whose `__init__.py` is in * `initFiles`, each with the local names bound from it and what each name diff --git a/src/resolution/index.ts b/src/resolution/index.ts index 01da3d4836..27286e60a2 100644 --- a/src/resolution/index.ts +++ b/src/resolution/index.ts @@ -42,7 +42,7 @@ import { matchShopifyThemeFile } from './shopify-themes'; import { isJsSuperCallLanguage, isJsSuperCallSite, matchJsSuperCall } from './js-super-calls'; import { CLASS_EXPRESSION_REF_LANGUAGES, isCommonJsExportedClass, isJsClassExpression, writtenIn, type ClassExpressionIndex } from './js-class-expressions'; import { matchRubyConstantPathRef } from './ruby-constant-paths'; -import { resolveViaImport, resolvePhpImportedStaticCall, resolvePhpQualifiedClassRef, resolveTsQualifiedTypeRef, resolveJvmImport, extractImportMappings, extractReExports, loadCppIncludeDirs, isPhpIncludePathRef, isCobolCopybookRef, isNixPathImportRef, isDartImportRef, isLuaRequireRef, isJsPathImportRef, isBoundToOutOfRepoImport, reachesAmbientModule, importBindingTail, clearImportResolverMemos, resolveImportPath, isExternalImport, pythonImportsInScope, ESM_IMPORT_LANGUAGES, jsModuleFile, pythonRootFingerprint, pythonReopenScope, pythonPackageImporters, moduleDefaultExport, isJsFamilyFile, isModuleBindingName } from './import-resolver'; +import { resolveViaImport, resolvePhpImportedStaticCall, resolvePhpQualifiedClassRef, resolveTsQualifiedTypeRef, resolveJvmImport, extractImportMappings, extractReExports, loadCppIncludeDirs, isPhpIncludePathRef, isCobolCopybookRef, isNixPathImportRef, isDartImportRef, isLuaRequireRef, isJsPathImportRef, isBoundToOutOfRepoImport, reachesAmbientModule, importBindingTail, clearImportResolverMemos, resolveImportPath, isExternalImport, pythonImportsInScope, ESM_IMPORT_LANGUAGES, jsModuleFile, pythonRootFingerprint, pythonReopenScope, pythonScriptModuleScope, pythonPackageImporters, moduleDefaultExport, isJsFamilyFile, isModuleBindingName } from './import-resolver'; import { ResolverPool, minRefsForPool, shouldEngageAdaptively } from './resolver-pool'; import { resolveAliasBinding } from './alias-binding'; import { detectFrameworks, getResolvingFrameworks } from './frameworks'; @@ -3157,6 +3157,11 @@ export class ReferenceResolver { return pythonReopenScope(this.diskContext(), changedFiles); } + /** What a sync must re-open after these Python files were edited (see `pythonScriptModuleScope`). */ + getPythonScriptModuleScope(editedFiles: string[]): ReturnType { + return pythonScriptModuleScope(this.context, editedFiles); + } + /** Importers of Python packages whose `__init__.py` was edited, with the names they bind (see `pythonPackageImporters`). */ getPythonPackageImporters(initFiles: string[]): Map> { return pythonPackageImporters(this.context, initFiles);