From b92873b2dd1ca9e950109338e4ce0f640035eead Mon Sep 17 00:00:00 2001 From: Eric Traut Date: Sat, 11 May 2019 19:28:23 -0700 Subject: [PATCH] Added "reportConstantRedfinition" config option. --- client/schemas/pyrightconfig.schema.json | 6 ++++ docs/configuration.md | 2 ++ server/src/analyzer/symbol.ts | 4 +++ server/src/analyzer/symbolUtils.ts | 8 +++++ server/src/analyzer/typeAnalyzer.ts | 33 ++++++++++++++++--- server/src/common/configOptions.ts | 7 ++++ server/src/tests/samples/constant1.py | 41 ++++++++++++++++++++++++ server/src/tests/typeAnalyzer.test.ts | 13 ++++++++ 8 files changed, 109 insertions(+), 5 deletions(-) create mode 100644 server/src/tests/samples/constant1.py diff --git a/client/schemas/pyrightconfig.schema.json b/client/schemas/pyrightconfig.schema.json index fe02dd2d1..10a8a5cc7 100644 --- a/client/schemas/pyrightconfig.schema.json +++ b/client/schemas/pyrightconfig.schema.json @@ -151,6 +151,12 @@ "title": "Controls reporting of private variables and functions used outside of the owning class or module", "default": "none" }, + "reportConstantRedefinition": { + "$id": "#/properties/reportConstantRedefinition", + "$ref": "#/definitions/diagnostic", + "title": "Controls reporting of attempts to redefine variables that are in all-caps", + "default": "none" + }, "reportIncompatibleMethodOverride": { "$id": "#/properties/reportIncompatibleMethodOverride", "$ref": "#/definitions/diagnostic", diff --git a/docs/configuration.md b/docs/configuration.md index dcd55d1fe..00e9c148d 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -58,6 +58,8 @@ The following settings control pyright's diagnostic output (warnings or errors). **reportPrivateUsage** [boolean or string, optional]: Generate or suppress diagnostics for uses of private variables or functions outside of the class or module that declares them. Private variables and functions, by convention, are named starting with a single underscoe (“_”) character. The default value for this setting is 'none'. +**reportConstantRedefinition** [boolean or string, optional]: Generate or suppress diagnostics for attempts to redefine variables whose names are all-caps with underscores and numerals. The default value for this setting is 'none'. + **reportIncompatibleMethodOverride** [boolean or string, optional]: Generate or suppress diagnostics for methods that override a method of the same name in a base class in an incompatible manner (different number of parameters, different parameter tyeps, or a different return type). The default value for this setting is 'none'. **reportInvalidStringEscapeSequence** [boolean or string, optional]: Generate or suppress diagnostics for invalid escape sequences used within string literals. The Python specification indicates that such sequences will generate a syntax error in future versions. The default value for this setting is 'warning'. diff --git a/server/src/analyzer/symbol.ts b/server/src/analyzer/symbol.ts index 476755b4b..657555429 100644 --- a/server/src/analyzer/symbol.ts +++ b/server/src/analyzer/symbol.ts @@ -35,6 +35,10 @@ export interface Declaration { // Declared type (if specified) of the symbol. declaredType?: Type; + // Is the declaration considered "constant" (i.e. + // reassignment is not permitted)? + isConstant?: boolean; + // The file and range within that file that // contains the declaration. path: string; diff --git a/server/src/analyzer/symbolUtils.ts b/server/src/analyzer/symbolUtils.ts index c21a1621b..925d0de6c 100644 --- a/server/src/analyzer/symbolUtils.ts +++ b/server/src/analyzer/symbolUtils.ts @@ -7,6 +7,9 @@ * Static methods that apply to symbols or symbol names. */ +const _constantRegEx = /^[A-Z0-9_]+$/; +const _underscoreOnlyRegEx = /^[_]+$/; + export class SymbolUtils { // Private symbol names start with a single underscore. static isPrivateName(name: string) { @@ -21,4 +24,9 @@ export class SymbolUtils { name.startsWith('__') && name.endsWith('__'); } + + // Constants are all-caps with possible numbers and underscores. + static isConstantName(name: string) { + return !!name.match(_constantRegEx) && !name.match(_underscoreOnlyRegEx); + } } diff --git a/server/src/analyzer/typeAnalyzer.ts b/server/src/analyzer/typeAnalyzer.ts index 48f5f98cb..2a8d1541b 100644 --- a/server/src/analyzer/typeAnalyzer.ts +++ b/server/src/analyzer/typeAnalyzer.ts @@ -2217,10 +2217,11 @@ export class TypeAnalyzer extends ParseTreeWalker { // A local helper function that creates a new declaration. let createDeclaration = () => { - let declaration: Declaration = { + const declaration: Declaration = { category: srcType instanceof FunctionType ? SymbolCategory.Method : SymbolCategory.Variable, node: node.memberName, + isConstant: SymbolUtils.isConstantName(node.memberName.nameToken.value), path: this._fileInfo.filePath, range: convertOffsetsToRange(node.memberName.start, node.memberName.end, this._fileInfo.lines) }; @@ -2260,8 +2261,18 @@ export class TypeAnalyzer extends ParseTreeWalker { } this._addDeclarationToSymbol(symbol, createDeclaration(), node); - AnalyzerNodeInfo.setDeclarations(node.memberName, - TypeUtils.getPrimaryDeclarationsForSymbol(symbol)!); + const primaryDecls = TypeUtils.getPrimaryDeclarationsForSymbol(symbol)!; + AnalyzerNodeInfo.setDeclarations(node.memberName, primaryDecls); + + // Check for an attempt to overwrite a constant member variable. + const primaryDecl = primaryDecls ? primaryDecls[0] : undefined; + if (primaryDecl && primaryDecl.isConstant && srcExprNode) { + if (node.memberName !== primaryDecl.node) { + this._addDiagnostic(this._fileInfo.configOptions.reportConstantRedefinition, + `'${ node.memberName.nameToken.value }' is constant and cannot be redefined`, + node.memberName); + } + } } else { // Is the target a property? const prevDeclarations = memberInfo.symbol.getDeclarations(); @@ -2484,6 +2495,7 @@ export class TypeAnalyzer extends ParseTreeWalker { const declaration: Declaration = { category: SymbolCategory.Variable, node: target, + isConstant: SymbolUtils.isConstantName(name.value), path: this._fileInfo.filePath, range: convertOffsetsToRange(name.start, name.end, this._fileInfo.lines) }; @@ -2588,10 +2600,11 @@ export class TypeAnalyzer extends ParseTreeWalker { this._assignTypeToExpression(target.valueExpression, destType, srcExpr); } else if (target instanceof UnpackExpressionNode) { if (target.expression instanceof NameNode) { - let name = target.expression.nameToken; - let declaration: Declaration = { + const name = target.expression.nameToken; + const declaration: Declaration = { category: SymbolCategory.Variable, node: target.expression, + isConstant: SymbolUtils.isConstantName(name.value), path: this._fileInfo.filePath, range: convertOffsetsToRange(name.start, name.end, this._fileInfo.lines) }; @@ -2700,12 +2713,14 @@ export class TypeAnalyzer extends ParseTreeWalker { // Determine if there's a declared type for this symbol. let declaredType: Type | undefined = declaration ? declaration.declaredType : undefined; + let primaryDecl: Declaration | undefined; const symbolWithScope = this._currentScope.lookUpSymbolRecursive(nameValue); if (symbolWithScope) { const primaryDecls = TypeUtils.getPrimaryDeclarationsForSymbol(symbolWithScope.symbol); if (primaryDecls) { declaredType = primaryDecls[0].declaredType!; + primaryDecl = primaryDecls[0]; } } else { // We should never get here. @@ -2727,6 +2742,14 @@ export class TypeAnalyzer extends ParseTreeWalker { } } + if (primaryDecl && primaryDecl.isConstant && srcExpressionNode) { + if (nameNode !== primaryDecl.node) { + this._addDiagnostic(this._fileInfo.configOptions.reportConstantRedefinition, + `'${ nameValue }' is constant and cannot be redefined`, + nameNode); + } + } + this._addTypeSourceToNameNode(nameNode, destType, declaration); if (declaration) { diff --git a/server/src/common/configOptions.ts b/server/src/common/configOptions.ts index d4a8c18f4..4a609562c 100644 --- a/server/src/common/configOptions.ts +++ b/server/src/common/configOptions.ts @@ -125,6 +125,9 @@ export class ConfigOptions { // the owning class or module? reportPrivateUsage: DiagnosticLevel = 'none'; + // Report attempts to redefine variables that are in all-caps. + reportConstantRedefinition: DiagnosticLevel = 'none'; + // Report usage of method override that is incomatlble with // the base class method of the same name? reportIncompatibleMethodOverride: DiagnosticLevel = 'none'; @@ -301,6 +304,10 @@ export class ConfigOptions { this.reportPrivateUsage = this._convertDiagnosticLevel( configObj.reportPrivateUsage, 'reportPrivateUsage', 'none'); + // Read the "reportConstantRedefinition" entry. + this.reportConstantRedefinition = this._convertDiagnosticLevel( + configObj.reportConstantRedefinition, 'reportConstantRedefinition', 'none'); + // Read the "reportIncompatibleMethodOverride" entry. this.reportIncompatibleMethodOverride = this._convertDiagnosticLevel( configObj.reportIncompatibleMethodOverride, 'reportIncompatibleMethodOverride', 'none'); diff --git a/server/src/tests/samples/constant1.py b/server/src/tests/samples/constant1.py new file mode 100644 index 000000000..a3998ec68 --- /dev/null +++ b/server/src/tests/samples/constant1.py @@ -0,0 +1,41 @@ +# This sample tests the "reportConstantRedefinition" feature. + +ALL_CAPS_123_ = 234 +# This should generate an error if the feature is enabled. +ALL_CAPS_123_ = 233 + +_ = 234 +# This should not be considered a constant +_ = 234 + + +a = True + +def foo(): + LOCALVAR = 3 + + if a: + # This should generate an error if the feature is enabled. + LOCALVAR = 23 + + +from typing import TYPE_CHECKING +# This should generate an error if the feature is enabled. +TYPE_CHECKING = True + + +class Foo(object): + CONST_VAR = 3 + + # This should generate an error if the feature is enabled. + CONST_VAR = 4 + + def __init__(self): + self.HELLO = '3' + + + def foo(self): + # This should generate an error if the feature is enabled. + self.HELLO = '324' + + diff --git a/server/src/tests/typeAnalyzer.test.ts b/server/src/tests/typeAnalyzer.test.ts index 867b7581b..8a7159fcf 100644 --- a/server/src/tests/typeAnalyzer.test.ts +++ b/server/src/tests/typeAnalyzer.test.ts @@ -270,6 +270,19 @@ test('Private1', () => { validateResults(analysisResults, 3); }); +test('Constant1', () => { + const configOptions = new ConfigOptions('.'); + + // By default, optional diagnostics are ignored. + let analysisResults = TestUtils.typeAnalyzeSampleFiles(['constant1.py'], configOptions); + validateResults(analysisResults, 0); + + // Turn on errors. + configOptions.reportPrivateUsage = 'error'; + analysisResults = TestUtils.typeAnalyzeSampleFiles(['constant1.py'], configOptions); + validateResults(analysisResults, 5); +}); + test('Tuples1', () => { let analysisResults = TestUtils.typeAnalyzeSampleFiles(['tuples1.py']);