Added configuration switch that enables reporting of import cycles.

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

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",

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'.

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;
+    }
+}

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();

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;

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');