Skip to content

Commit

Permalink
Fixed a bug that led to incorrect import resolution in cases where mu…
Browse files Browse the repository at this point in the history 
…ltiple import resolution paths have partial overlapping paths and some include namespace packages and others do not. This addresses #5089.
  • Loading branch information
@msfterictraut
msfterictraut committed May 11, 2023
1 parent 7888d1c commit 43a6a8f
Show file tree
Hide file tree
Showing 3 changed files with 139 additions and 34 deletions.
78 changes: 46 additions & 32 deletions packages/pyright-internal/src/analyzer/importResolver.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1578,41 +1578,38 @@ export class ImportResolver {
}

if (newImport.isImportFound) {
// Prefer found over not found.
if (!bestImportSoFar.isImportFound) {
return newImport;
}

// Prefer traditional over namespace imports.
if (bestImportSoFar.isNamespacePackage && !newImport.isNamespacePackage) {
return newImport;
// Prefer traditional packages over namespace packages.
const soFarIndex = bestImportSoFar.resolvedPaths.findIndex((path) => !!path);
const newIndex = newImport.resolvedPaths.findIndex((path) => !!path);
if (soFarIndex !== newIndex) {
if (soFarIndex < 0) {
return newImport;
} else if (newIndex < 0) {
return bestImportSoFar;
}
return soFarIndex < newIndex ? bestImportSoFar : newImport;
}

// Prefer local packages.
if (bestImportSoFar.importType === ImportType.Local && !bestImportSoFar.isNamespacePackage) {
return bestImportSoFar;
}
if (newImport.importType === ImportType.Local && !newImport.isNamespacePackage) {
// Prefer found over not found.
if (!bestImportSoFar.isImportFound) {
return newImport;
}

// If both are namespace imports, select the one that resolves the symbols.
if (
bestImportSoFar.isNamespacePackage &&
newImport.isNamespacePackage &&
moduleDescriptor.importedSymbols
) {
if (!this._isNamespacePackageResolved(moduleDescriptor, bestImportSoFar.implicitImports)) {
if (this._isNamespacePackageResolved(moduleDescriptor, newImport.implicitImports)) {
return newImport;
}
if (bestImportSoFar.isNamespacePackage && newImport.isNamespacePackage) {
if (moduleDescriptor.importedSymbols) {
if (!this._isNamespacePackageResolved(moduleDescriptor, bestImportSoFar.implicitImports)) {
if (this._isNamespacePackageResolved(moduleDescriptor, newImport.implicitImports)) {
return newImport;
}

// Prefer the namespace package that has an __init__.py(i) file present
// in the final directory over one that does not.
if (bestImportSoFar.isInitFilePresent && !newImport.isInitFilePresent) {
return bestImportSoFar;
} else if (!bestImportSoFar.isInitFilePresent && newImport.isInitFilePresent) {
return newImport;
// Prefer the namespace package that has an __init__.py(i) file present
// in the final directory over one that does not.
if (bestImportSoFar.isInitFilePresent && !newImport.isInitFilePresent) {
return bestImportSoFar;
} else if (!bestImportSoFar.isInitFilePresent && newImport.isInitFilePresent) {
return newImport;
}
}
}
}
Expand All @@ -1635,10 +1632,27 @@ export class ImportResolver {
if (bestImportSoFar.resolvedPaths.length > newImport.resolvedPaths.length) {
return newImport;
}
} else if (newImport.isPartlyResolved && bestImportSoFar.isNamespacePackage && !newImport.isNamespacePackage) {
// Always prefer a traditional over namespace import even
// if the traditional import is only partly resolved.
return newImport;
} else if (newImport.isPartlyResolved) {
// If the new import is a traditional package but only partly resolves
// the import but the best import so far is a namespace package, we need
// to consider whether the best import so far also resolves the first part
// of the import with a traditional package. Using the example "import a.b.c.d"
// and the symbol ~ to represent a namespace package, consider the following
// cases:
// bestSoFar: a/~b/~c/~d new: a Result: bestSoFar wins
// bestSoFar: ~a/~b/~c/~d new: a Result: new wins
// bestSoFar: a/~b/~c/~d new: a/b Result: new wins
const soFarIndex = bestImportSoFar.resolvedPaths.findIndex((path) => !!path);
const newIndex = newImport.resolvedPaths.findIndex((path) => !!path);

if (soFarIndex !== newIndex) {
if (soFarIndex < 0) {
return newImport;
} else if (newIndex < 0) {
return bestImportSoFar;
}
return soFarIndex < newIndex ? bestImportSoFar : newImport;
}
}

return bestImportSoFar;
Expand Down
5 changes: 4 additions & 1 deletion packages/pyright-internal/src/analyzer/importResult.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,10 @@ export interface ImportResult {
isPartlyResolved: boolean;

// True if the import refers to a namespace package (a
// folder without an __init__.py(i) file at every level).
// folder without an __init__.py(i) file at the last level).
// To determine if any intermediate level is a namespace
// package, look at the resolvedPaths array. Namespace package
// entries will have an empty string for the resolvedPath.
isNamespacePackage: boolean;

// True if there is an __init__.py(i) file in the final
Expand Down
90 changes: 89 additions & 1 deletion packages/pyright-internal/src/tests/importResolver.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -474,7 +474,7 @@ test('import side by side file sub under lib folder', () => {
assert(!importResult.isImportFound);
});

test('dont walk up the root', () => {
test("don't walk up the root", () => {
const files = [
{
path: combinePaths('/', 'file1.py'),
Expand All @@ -486,6 +486,94 @@ test('dont walk up the root', () => {
assert(!importResult.isImportFound);
});

test('nested namespace package 1', () => {
const files = [
{
path: combinePaths('/', 'packages1', 'a', 'b', 'c', 'd.py'),
content: 'def f(): pass',
},
{
path: combinePaths('/', 'packages1', 'a', '__init__.py'),
content: '',
},
{
path: combinePaths('/', 'packages2', 'a', '__init__.py'),
content: '',
},
];

const importResult = getImportResult(files, ['a', 'b', 'c', 'd'], (config) => {
config.defaultExtraPaths = [combinePaths('/', 'packages1'), combinePaths('/', 'packages2')];
});
assert(importResult.isImportFound);
});

test('nested namespace package 2', () => {
const files = [
{
path: combinePaths('/', 'packages1', 'a', 'b', 'c', 'd.py'),
content: 'def f(): pass',
},
{
path: combinePaths('/', 'packages1', 'a', 'b', 'c', '__init__.py'),
content: '',
},
{
path: combinePaths('/', 'packages2', 'a', 'b', 'c', '__init__.py'),
content: '',
},
];

const importResult = getImportResult(files, ['a', 'b', 'c', 'd'], (config) => {
config.defaultExtraPaths = [combinePaths('/', 'packages1'), combinePaths('/', 'packages2')];
});
assert(importResult.isImportFound);
});

test('nested namespace package 3', () => {
const files = [
{
path: combinePaths('/', 'packages1', 'a', 'b', 'c', 'd.py'),
content: 'def f(): pass',
},
{
path: combinePaths('/', 'packages2', 'a', '__init__.py'),
content: '',
},
];

const importResult = getImportResult(files, ['a', 'b', 'c', 'd'], (config) => {
config.defaultExtraPaths = [combinePaths('/', 'packages1'), combinePaths('/', 'packages2')];
});
assert(!importResult.isImportFound);
});

test('nested namespace package 4', () => {
const files = [
{
path: combinePaths('/', 'packages1', 'a', 'b', '__init__.py'),
content: '',
},
{
path: combinePaths('/', 'packages1', 'a', 'b', 'c.py'),
content: 'def f(): pass',
},
{
path: combinePaths('/', 'packages2', 'a', '__init__.py'),
content: '',
},
{
path: combinePaths('/', 'packages2', 'a', 'b', '__init__.py'),
content: '',
},
];

const importResult = getImportResult(files, ['a', 'b', 'c'], (config) => {
config.defaultExtraPaths = [combinePaths('/', 'packages1'), combinePaths('/', 'packages2')];
});
assert(!importResult.isImportFound);
});

function getImportResult(
files: { path: string; content: string }[],
nameParts: string[],
Expand Down

0 comments on commit 43a6a8f

Please sign in to comment.