From a982b02e64f6765bdeaa1b7795ea573b27260146 Mon Sep 17 00:00:00 2001 From: Eric Traut Date: Fri, 13 Dec 2019 21:22:56 -0800 Subject: [PATCH] Added new configuration option "reportDuplicateImports" that generates an error or warning when a symbol or module is imported more than once in a source file. --- client/schemas/pyrightconfig.schema.json | 6 ++ docs/configuration.md | 2 + server/src/analyzer/checker.ts | 59 ++++++++++++++++--- server/src/common/configOptions.ts | 11 ++++ server/src/common/diagnosticRules.ts | 1 + server/src/tests/checker.test.ts | 13 ++++ server/src/tests/samples/duplicateImports1.py | 15 +++++ 7 files changed, 100 insertions(+), 7 deletions(-) create mode 100644 server/src/tests/samples/duplicateImports1.py diff --git a/client/schemas/pyrightconfig.schema.json b/client/schemas/pyrightconfig.schema.json index bd32c901a..60b15ad4d 100644 --- a/client/schemas/pyrightconfig.schema.json +++ b/client/schemas/pyrightconfig.schema.json @@ -132,6 +132,12 @@ "title": "Controls reporting of local variables that are not accessed", "default": "none" }, + "reportDuplicateImport": { + "$id": "#/properties/reportDuplicateImport", + "$ref": "#/definitions/diagnostic", + "title": "Controls reporting of symbols or modules that are imported more than once", + "default": "none" + }, "reportOptionalSubscript": { "$id": "#/properties/reportOptionalSubscript", "$ref": "#/definitions/diagnostic", diff --git a/docs/configuration.md b/docs/configuration.md index b871a9cb1..96c7faaf1 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -56,6 +56,8 @@ The following settings control pyright’s diagnostic output (warnings or errors **reportUnusedVariable** [boolean or string, optional]: Generate or suppress diagnostics for a variable that is not accessed. The default value for this setting is 'none'. +**reportDuplicateImport** [boolean or string, optional]: Generate or suppress diagnostics for an imported symbol or module that is imported more than once. + **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/checker.ts b/server/src/analyzer/checker.ts index 28bfdbb9c..214abfe4a 100644 --- a/server/src/analyzer/checker.ts +++ b/server/src/analyzer/checker.ts @@ -19,22 +19,23 @@ import { Diagnostic, DiagnosticAddendum } from '../common/diagnostic'; import { DiagnosticRule } from '../common/diagnosticRules'; import { TextRange } from '../common/textRange'; import { AssertNode, AssignmentExpressionNode, AssignmentNode, AugmentedAssignmentNode, - BinaryOperationNode, CallNode, ClassNode, DelNode, ErrorNode, ExceptNode, ExpressionNode, - FormatStringNode, ForNode, FunctionNode, IfNode, ImportAsNode, ImportFromNode, IndexNode, - LambdaNode, ListComprehensionNode, MemberAccessNode, ModuleNode, NameNode, ParameterCategory, - ParseNode, ParseNodeType, RaiseNode, ReturnNode, SliceNode, StringListNode, SuiteNode, - TernaryNode, TupleNode, TypeAnnotationNode, UnaryOperationNode, UnpackNode, WhileNode, - WithNode, YieldFromNode, YieldNode } from '../parser/parseNodes'; + BinaryOperationNode, CallNode, ClassNode, DelNode, ErrorNode, ExceptNode, + FormatStringNode, ForNode, FunctionNode, IfNode, ImportAsNode, ImportFromAsNode, ImportFromNode, + IndexNode, LambdaNode, ListComprehensionNode, MemberAccessNode, ModuleNode, NameNode, + ParameterCategory, ParseNode, ParseNodeType, RaiseNode, ReturnNode, SliceNode, StringListNode, + SuiteNode, TernaryNode, TupleNode, TypeAnnotationNode, UnaryOperationNode, UnpackNode, + WhileNode, WithNode, YieldFromNode, YieldNode } from '../parser/parseNodes'; import { AnalyzerFileInfo } from './analyzerFileInfo'; import * as AnalyzerNodeInfo from './analyzerNodeInfo'; import { Declaration, DeclarationType } from './declaration'; +import { getTopLevelImports } from './importStatementUtils'; import * as ParseTreeUtils from './parseTreeUtils'; import { ParseTreeWalker } from './parseTreeWalker'; import { ScopeType } from './scope'; import { Symbol } from './symbol'; import * as SymbolNameUtils from './symbolNameUtils'; import { getLastTypedDeclaredForSymbol } from './symbolUtils'; -import { EvaluatorFlags, TypeEvaluator } from './typeEvaluator'; +import { TypeEvaluator } from './typeEvaluator'; import { ClassType, combineTypes, FunctionType, isAnyOrUnknown, isNoneOrNever, isTypeSame, NoneType, ObjectType, Type, TypeCategory, UnknownType } from './types'; import { containsUnknown, derivesFromClassRecursive, doForSubtypes, @@ -67,6 +68,8 @@ export class Checker extends ParseTreeWalker { // Perform a one-time validation of symbols in all scopes // defined in this module for things like unaccessed variables. this._validateSymbolTables(); + + this._reportDuplicateImports(); } walk(node: ParseNode) { @@ -1385,4 +1388,46 @@ export class Checker extends ParseTreeWalker { } } } + + private _reportDuplicateImports() { + const importStatements = getTopLevelImports(this._moduleNode); + + const importModuleMap = new Map(); + + importStatements.orderedImports.forEach(importStatement => { + if (importStatement.node.nodeType === ParseNodeType.ImportFrom) { + const symbolMap = new Map(); + + importStatement.node.imports.forEach(importFromAs => { + // Ignore duplicates if they're aliased. + if (!importFromAs.alias) { + const prevImport = symbolMap.get(importFromAs.name.value); + if (prevImport) { + this._evaluator.addDiagnostic( + this._fileInfo.diagnosticSettings.reportDuplicateImport, + DiagnosticRule.reportDuplicateImport, + `'${ importFromAs.name.value }' is imported more than once`, + importFromAs.name); + } else { + symbolMap.set(importFromAs.name.value, importFromAs); + } + } + }); + } else if (importStatement.subnode) { + // Ignore duplicates if they're aliased. + if (!importStatement.subnode.alias) { + const prevImport = importModuleMap.get(importStatement.moduleName); + if (prevImport) { + this._evaluator.addDiagnostic( + this._fileInfo.diagnosticSettings.reportDuplicateImport, + DiagnosticRule.reportDuplicateImport, + `'${ importStatement.moduleName }' is imported more than once`, + importStatement.subnode); + } else { + importModuleMap.set(importStatement.moduleName, importStatement.subnode); + } + } + } + }); + } } diff --git a/server/src/common/configOptions.ts b/server/src/common/configOptions.ts index c72ecc618..913b667d4 100644 --- a/server/src/common/configOptions.ts +++ b/server/src/common/configOptions.ts @@ -79,6 +79,9 @@ export interface DiagnosticSettings { // Report variable that is not accessed? reportUnusedVariable: DiagnosticLevel; + // Report symbol or module that is imported more than once? + reportDuplicateImport: DiagnosticLevel; + // Report attempts to subscript (index) an Optional type? reportOptionalSubscript: DiagnosticLevel; @@ -181,6 +184,7 @@ export function getDiagLevelSettings() { DiagnosticRule.reportUnusedClass, DiagnosticRule.reportUnusedFunction, DiagnosticRule.reportUnusedVariable, + DiagnosticRule.reportDuplicateImport, DiagnosticRule.reportOptionalSubscript, DiagnosticRule.reportOptionalMemberAccess, DiagnosticRule.reportOptionalCall, @@ -220,6 +224,7 @@ export function getStrictDiagnosticSettings(): DiagnosticSettings { reportUnusedClass: 'error', reportUnusedFunction: 'error', reportUnusedVariable: 'error', + reportDuplicateImport: 'error', reportOptionalSubscript: 'error', reportOptionalMemberAccess: 'error', reportOptionalCall: 'error', @@ -261,6 +266,7 @@ export function getDefaultDiagnosticSettings(): DiagnosticSettings { reportUnusedClass: 'none', reportUnusedFunction: 'none', reportUnusedVariable: 'none', + reportDuplicateImport: 'none', reportOptionalSubscript: 'none', reportOptionalMemberAccess: 'none', reportOptionalCall: 'none', @@ -520,6 +526,11 @@ export class ConfigOptions { configObj.reportUnusedVariable, DiagnosticRule.reportUnusedVariable, defaultSettings.reportUnusedVariable), + // Read the "reportDuplicateImport" entry. + reportDuplicateImport: this._convertDiagnosticLevel( + configObj.reportDuplicateImport, DiagnosticRule.reportDuplicateImport, + defaultSettings.reportDuplicateImport), + // Read the "reportMissingTypeStubs" entry. reportMissingTypeStubs: this._convertDiagnosticLevel( configObj.reportMissingTypeStubs, DiagnosticRule.reportMissingTypeStubs, diff --git a/server/src/common/diagnosticRules.ts b/server/src/common/diagnosticRules.ts index a97ae57f6..4b900fd87 100644 --- a/server/src/common/diagnosticRules.ts +++ b/server/src/common/diagnosticRules.ts @@ -22,6 +22,7 @@ export const enum DiagnosticRule { reportUnusedClass = 'reportUnusedClass', reportUnusedFunction = 'reportUnusedFunction', reportUnusedVariable = 'reportUnusedVariable', + reportDuplicateImport = 'reportDuplicateImport', reportOptionalSubscript = 'reportOptionalSubscript', reportOptionalMemberAccess = 'reportOptionalMemberAccess', reportOptionalCall = 'reportOptionalCall', diff --git a/server/src/tests/checker.test.ts b/server/src/tests/checker.test.ts index 023d84baa..39298e500 100644 --- a/server/src/tests/checker.test.ts +++ b/server/src/tests/checker.test.ts @@ -945,3 +945,16 @@ test('CallSite2', () => { const analysisResults = TestUtils.typeAnalyzeSampleFiles(['callSite2.py']); validateResults(analysisResults, 0); }); + +test('DuplicateImports1', () => { + const configOptions = new ConfigOptions('.'); + + // By default, optional diagnostics are ignored. + let analysisResults = TestUtils.typeAnalyzeSampleFiles(['duplicateImports1.py'], configOptions); + validateResults(analysisResults, 0); + + // Turn on errors. + configOptions.diagnosticSettings.reportDuplicateImport = 'error'; + analysisResults = TestUtils.typeAnalyzeSampleFiles(['duplicateImports1.py'], configOptions); + validateResults(analysisResults, 2); +}); diff --git a/server/src/tests/samples/duplicateImports1.py b/server/src/tests/samples/duplicateImports1.py new file mode 100644 index 000000000..3762ab7bd --- /dev/null +++ b/server/src/tests/samples/duplicateImports1.py @@ -0,0 +1,15 @@ +# This sample tests the duplicate import detection. + +import sys + +# This should generate an error because Any is duplicated +from typing import Any, Dict, Any + +# This should generate an error because sys is duplicated +import sys + + +a: Dict[Any, Any] +b = sys.api_version + +