diff --git a/README.md b/README.md index d43ab9b0e..bc50a98ca 100644 --- a/README.md +++ b/README.md @@ -78,7 +78,6 @@ To update to the latest version: Pyright is a work in progress. The following functionality is not yet finished. If you would like to contribute to any of these areas, contact the maintainers of the repo. -* Provide switch that reports circular import dependencies * Provide switch that treats instance variables and methods that begin with underscore as private * Validate that overridden methods in subclass have same signature as base class methods * Add support for f-strings diff --git a/client/schemas/pyrightconfig.schema.json b/client/schemas/pyrightconfig.schema.json index 368c442da..7f32bbaf7 100644 --- a/client/schemas/pyrightconfig.schema.json +++ b/client/schemas/pyrightconfig.schema.json @@ -79,6 +79,12 @@ "title": "Controls reporting of imports that cannot be resolved to type stub files", "default": "none" }, + "reportImportCycles": { + "$id": "#/properties/reportImportCycles", + "$ref": "#/definitions/diagnostic", + "title": "Controls reporting of module imports that create cycles in import graph", + "default": "none" + }, "reportOptionalSubscript": { "$id": "#/properties/reportOptionalSubscript", "$ref": "#/definitions/diagnostic", diff --git a/docs/configuration.md b/docs/configuration.md index 633051954..7d22db96d 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -34,6 +34,8 @@ The following settings control pyright's diagnostic output (warnings or errors). **reportMissingTypeStubs** [boolean or string, optional]: Generate or suppress diagnostics for imports that have no corresponding type stub file (either a typeshed file or a custom type stub). The type checker requires type stubs to do its best job at analysis. The default value for this setting is 'none', although pyright can do a much better job of static type checking if type stub files are provided for all imports. +**reportImportCycles** [boolean or string, optional]: Generate or suppress diagnostics for cyclical import chains. These are not errors in Python, but they do slow down type analysis and often hint at architectural layering issues. Generally, they should be avoided. The default value for this setting is 'none'. Note that there are import cycles in the typeshed stdlib typestub files that are ignored by this setting. + **reportOptionalSubscript** [boolean or string, optional]: Generate or suppress diagnostics for an attempt to subscript (index) a variable with an Optional type. The default value for this setting is 'none'. **reportOptionalMemberAccess** [boolean or string, optional]: Generate or suppress diagnostics for an attempt to access a member of a variable with an Optional type. The default value for this setting is 'none'. diff --git a/server/src/analyzer/circularDependency.ts b/server/src/analyzer/circularDependency.ts new file mode 100644 index 000000000..ee272367e --- /dev/null +++ b/server/src/analyzer/circularDependency.ts @@ -0,0 +1,53 @@ +/* +* circularDependency.ts +* Copyright (c) Microsoft Corporation. +* Licensed under the MIT license. +* Author: Eric Traut +* +* A list of file paths that are part of a circular dependency +* chain (i.e. a chain of imports). Since these are circular, there +* no defined "start", but this module helps normalize the start +* by picking the alphabetically-first module in the cycle. +*/ + +export class CircularDependency { + private _paths: string[] = []; + + appendPath(path: string) { + this._paths.push(path); + } + + getPaths() { + return this._paths; + } + + normalizeOrder() { + // Find the path that is alphabetically first and reorder + // based on that. + let firstIndex = 0; + this._paths.forEach((path, index) => { + if (path < this._paths[firstIndex]) { + firstIndex = index; + } + }); + + if (firstIndex !== 0) { + this._paths = this._paths.slice(firstIndex).concat( + this._paths.slice(0, firstIndex)); + } + } + + isEqual(circDependency: CircularDependency) { + if (circDependency._paths.length !== this._paths.length) { + return false; + } + + for (let i = 0; i < this._paths.length; i++) { + if (this._paths[i] !== circDependency._paths[i]) { + return false; + } + } + + return true; + } +} diff --git a/server/src/analyzer/program.ts b/server/src/analyzer/program.ts index de513e39c..2479e7ea8 100644 --- a/server/src/analyzer/program.ts +++ b/server/src/analyzer/program.ts @@ -17,6 +17,7 @@ import { FileDiagnostics } from '../common/diagnosticSink'; import { Duration } from '../common/timing'; import { ImportMap } from './analyzerFileInfo'; import { AnalyzerNodeInfo } from './analyzerNodeInfo'; +import { CircularDependency } from './circularDependency'; import { ImportType } from './importResult'; import { Scope } from './scope'; import { SourceFile } from './sourceFile'; @@ -397,6 +398,12 @@ export class Program { // Mark all files in the closure as finalized. Object.keys(closureMap).forEach(filePath => { + assert(!this._sourceFileMap[filePath].sourceFile.isAnalysisFinalized()); + + if (options.reportImportCycles) { + this._detectAndReportImportCycles(this._sourceFileMap[filePath]); + } + this._sourceFileMap[filePath].sourceFile.finalizeAnalysis(); }); @@ -413,6 +420,7 @@ export class Program { private _getNonFinalizedImportsRecursive(fileToAnalyze: SourceFileInfo, closureMap: { [path: string]: boolean }, analysisQueue: SourceFileInfo[], options: ConfigOptions, timeElapsedCallback: () => boolean): boolean { + // If the file is already finalized, no need to do any more work. if (fileToAnalyze.sourceFile.isAnalysisFinalized()) { return false; @@ -452,6 +460,64 @@ export class Program { return false; } + private _detectAndReportImportCycles(sourceFileInfo: SourceFileInfo, + dependencyChain: SourceFileInfo[] = [], + dependencyMap: { [path: string]: SourceFileInfo } = {}): boolean { + + // Don't bother checking for typestub files. + if (sourceFileInfo.sourceFile.isStubFile()) { + return false; + } + + // Don't bother checking files that are already finalized + // because they've already been searched. + if (sourceFileInfo.sourceFile.isAnalysisFinalized()) { + return false; + } + + const filePath = sourceFileInfo.sourceFile.getFilePath(); + if (dependencyMap[filePath]) { + // Look for chains at least two in length. A file that contains + // an "import . from X" will technically create a cycle with + // itself, but those are not interesting to report. + if (dependencyChain.length > 1 && sourceFileInfo === dependencyChain[0]) { + this._logImportCycle(dependencyChain); + return true; + } + return false; + } else { + // We use both a map (for fast lookups) and a list + // (for ordering information). + dependencyMap[filePath] = sourceFileInfo; + dependencyChain.push(sourceFileInfo); + + let reportedCycle = false; + for (const imp of sourceFileInfo.imports) { + if (this._detectAndReportImportCycles(imp, dependencyChain, dependencyMap)) { + reportedCycle = true; + } + } + + delete dependencyMap[filePath]; + dependencyChain.pop(); + + return reportedCycle; + } + } + + private _logImportCycle(dependencyChain: SourceFileInfo[]) { + const circDep = new CircularDependency(); + dependencyChain.forEach(sourceFileInfo => { + circDep.appendPath(sourceFileInfo.sourceFile.getFilePath()); + }); + + circDep.normalizeOrder(); + const firstFilePath = circDep.getPaths()[0]; + const firstSourceFile = this._sourceFileMap[firstFilePath]; + assert(firstSourceFile !== undefined); + firstSourceFile.sourceFile.addCircularDependency(circDep); + } + private _markFileDirtyRecursive(sourceFileInfo: SourceFileInfo, markMap: { [path: string]: boolean }) { let filePath = sourceFileInfo.sourceFile.getFilePath(); diff --git a/server/src/analyzer/sourceFile.ts b/server/src/analyzer/sourceFile.ts index a14181d82..223c68d17 100644 --- a/server/src/analyzer/sourceFile.ts +++ b/server/src/analyzer/sourceFile.ts @@ -23,6 +23,7 @@ import { ParseOptions, Parser, ParseResults } from '../parser/parser'; import { Token } from '../parser/tokenizerTypes'; import { AnalyzerFileInfo, ImportMap } from './analyzerFileInfo'; import { AnalyzerNodeInfo } from './analyzerNodeInfo'; +import { CircularDependency } from './circularDependency'; import { DefinitionProvider } from './definitionProvider'; import { HoverProvider } from './hoverProvider'; import { ImportResolver } from './importResolver'; @@ -54,6 +55,8 @@ export interface AnalysisJob { typeAnalysisLastPassDiagnostics: Diagnostic[]; typeAnalysisFinalDiagnostics: Diagnostic[]; + circularDependencies: CircularDependency[]; + typeAnalysisPassNumber: number; isTypeAnalysisPassNeeded: boolean; isTypeAnalysisFinalized: boolean; @@ -98,6 +101,8 @@ export class SourceFile { typeAnalysisLastPassDiagnostics: [], typeAnalysisFinalDiagnostics: [], + circularDependencies: [], + typeAnalysisPassNumber: 1, isTypeAnalysisPassNeeded: true, isTypeAnalysisFinalized: false @@ -152,6 +157,10 @@ export class SourceFile { return this._diagnosticVersion; } + isStubFile() { + return this._isStubFile; + } + // Returns a list of cached diagnostics from the latest analysis job. // If the prevVersion is specified, the method returns undefined if // the diagnostics haven't changed. @@ -168,6 +177,16 @@ export class SourceFile { this._analysisJob.semanticAnalysisDiagnostics, this._analysisJob.typeAnalysisFinalDiagnostics); + if (options.reportImportCycles !== 'none' && this._analysisJob.circularDependencies.length > 0) { + const category = options.reportImportCycles === 'warning' ? + DiagnosticCategory.Warning : DiagnosticCategory.Error; + + this._analysisJob.circularDependencies.forEach(cirDep => { + diagList.push(new Diagnostic(category, 'Cycle detected in import chain\n' + + cirDep.getPaths().map(path => ' ' + path).join('\n'))); + }); + } + if (this._isTypeshedStubFile) { if (options.reportTypeshedErrors === 'none') { return undefined; @@ -262,6 +281,14 @@ export class SourceFile { return undefined; } + // Adds a new circular dependency for this file but only if + // it hasn't already been added. + addCircularDependency(circDependency: CircularDependency) { + if (!this._analysisJob.circularDependencies.some(dep => dep.isEqual(circDependency))) { + this._analysisJob.circularDependencies.push(circDependency); + } + } + // Parse the file and update the state. Callers should wait for completion // (or at least cancel) prior to calling again. It returns true if a parse // was required and false if the parse information was up to date already. @@ -415,6 +442,7 @@ export class SourceFile { this._analysisJob.nextPhaseToRun = AnalysisPhase.TypeAnalysis; this._diagnosticVersion++; this._analysisJob.typeAnalysisPassNumber = 1; + this._analysisJob.circularDependencies = []; this._analysisJob.isTypeAnalysisPassNeeded = true; this._analysisJob.isTypeAnalysisFinalized = false; this._analysisJob.nextPhaseToRun = AnalysisPhase.TypeAnalysis; diff --git a/server/src/common/configOptions.ts b/server/src/common/configOptions.ts index 331809b25..34c4efe3c 100644 --- a/server/src/common/configOptions.ts +++ b/server/src/common/configOptions.ts @@ -85,6 +85,9 @@ export class ConfigOptions { // Report missing type stub files? reportMissingTypeStubs: DiagnosticLevel = 'none'; + // Report cycles in import graph? + reportImportCycles: DiagnosticLevel = 'none'; + // Report attempts to subscript (index) an Optional type? reportOptionalSubscript: DiagnosticLevel = 'none'; @@ -209,6 +212,10 @@ export class ConfigOptions { this.reportMissingTypeStubs = this._convertDiagnosticLevel( configObj.reportMissingTypeStubs, 'reportMissingTypeStubs', 'none'); + // Read the "reportImportCycles" entry. + this.reportImportCycles = this._convertDiagnosticLevel( + configObj.reportImportCycles, 'reportImportCycles', 'none'); + // Read the "reportOptionalSubscript" entry. this.reportOptionalSubscript = this._convertDiagnosticLevel( configObj.reportOptionalSubscript, 'reportOptionalSubscript', 'none');