From f844bd2f1c80fbb7f7783d3996dc792ee04dfa59 Mon Sep 17 00:00:00 2001 From: Eric Traut Date: Sun, 26 Jul 2020 10:32:21 -0700 Subject: [PATCH] Added new diagnostic rule "reportIncompatibleVariableOverride" which is similar to "reportIncompatibleMethodOverride" except that it reports incompatible overrides of variables (non-methods). --- client/schemas/pyrightconfig.schema.json | 6 ++ docs/configuration.md | 4 +- server/src/analyzer/checker.ts | 38 ++++++++++ server/src/common/configOptions.ts | 15 ++++ server/src/common/diagnosticRules.ts | 1 + server/src/localization/localize.ts | 3 + .../src/localization/package.nls.en-us.json | 2 + server/src/tests/checker.test.ts | 13 ++++ server/src/tests/samples/classes5.py | 70 +++++++++++++++++++ 9 files changed, 151 insertions(+), 1 deletion(-) create mode 100644 server/src/tests/samples/classes5.py diff --git a/client/schemas/pyrightconfig.schema.json b/client/schemas/pyrightconfig.schema.json index 80de57aab..aead6f077 100644 --- a/client/schemas/pyrightconfig.schema.json +++ b/client/schemas/pyrightconfig.schema.json @@ -255,6 +255,12 @@ "title": "Controls reporting of method overrides in subclasses that redefine the method in an incompatible way", "default": "none" }, + "reportIncompatibleVariableOverride": { + "$id": "#/properties/reportIncompatibleVariableOverride", + "$ref": "#/definitions/diagnostic", + "title": "Controls reporting of overrides in subclasses that redefine a variable in an incompatible way", + "default": "none" + }, "reportInvalidStringEscapeSequence": { "$id": "#/properties/reportInvalidStringEscapeSequence", "$ref": "#/definitions/diagnostic", diff --git a/docs/configuration.md b/docs/configuration.md index 20f8b1287..28abd7a73 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -88,7 +88,9 @@ The following settings control pyright’s diagnostic output (warnings or errors **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 types, or a different return type). 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 (wrong number of parameters, incompatible parameter types, or incompatible return type). The default value for this setting is 'none'. + +**reportIncompatibleVariableOverride** [boolean or string, optional]: Generate or suppress diagnostics for class variable declarations that override a symbol of the same name in a base class with a type that is incompatible with the base class symbol 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/checker.ts b/server/src/analyzer/checker.ts index bbc7de97f..45e0be0d4 100644 --- a/server/src/analyzer/checker.ts +++ b/server/src/analyzer/checker.ts @@ -1729,6 +1729,11 @@ export class Checker extends ParseTreeWalker { // Get the symbol type defined in this class. const typeOfSymbol = this._evaluator.getEffectiveTypeOfSymbol(symbol); + // If the type of the override symbol isn't known, stop here. + if (isAnyOrUnknown(typeOfSymbol)) { + return; + } + // Get the symbol defined in the base class. const baseClassAndSymbol = lookUpClassMember(classType, name, ClassMemberLookupFlags.SkipOriginalClass); @@ -1822,6 +1827,39 @@ export class Checker extends ParseTreeWalker { } } } + } else { + // This check can be expensive, so don't perform it if the corresponding + // rule is disabled. + if (this._fileInfo.diagnosticRuleSet.reportIncompatibleVariableOverride !== 'none') { + // Verify that the override type is assignable to (same or narrower than) + // the declared type of the base symbol. + if (!this._evaluator.canAssignType(baseClassSymbolType, typeOfSymbol, diagAddendum)) { + const decls = symbol.getDeclarations(); + if (decls.length > 0) { + const lastDecl = decls[decls.length - 1]; + if (lastDecl) { + const diag = this._evaluator.addDiagnostic( + this._fileInfo.diagnosticRuleSet.reportIncompatibleVariableOverride, + DiagnosticRule.reportIncompatibleVariableOverride, + Localizer.Diagnostic.symbolOverridden().format({ + name, + className: baseClassAndSymbol.classType.details.name, + }) + diagAddendum.getString(), + lastDecl.node + ); + + const origDecl = getLastTypedDeclaredForSymbol(baseClassAndSymbol.symbol); + if (diag && origDecl) { + diag.addRelatedInfo( + Localizer.DiagnosticAddendum.overriddenSymbol(), + origDecl.path, + origDecl.range + ); + } + } + } + } + } } }); } diff --git a/server/src/common/configOptions.ts b/server/src/common/configOptions.ts index f65a95924..a58fb1797 100644 --- a/server/src/common/configOptions.ts +++ b/server/src/common/configOptions.ts @@ -142,6 +142,10 @@ export interface DiagnosticRuleSet { // the base class method of the same name? reportIncompatibleMethodOverride: DiagnosticLevel; + // Report usage of variable override that is incompatible with + // the base class symbol of the same name? + reportIncompatibleVariableOverride: DiagnosticLevel; + // Report usage of invalid escape sequences in string literals? reportInvalidStringEscapeSequence: DiagnosticLevel; @@ -231,6 +235,7 @@ export function getDiagLevelDiagnosticRules() { DiagnosticRule.reportPrivateUsage, DiagnosticRule.reportConstantRedefinition, DiagnosticRule.reportIncompatibleMethodOverride, + DiagnosticRule.reportIncompatibleVariableOverride, DiagnosticRule.reportInvalidStringEscapeSequence, DiagnosticRule.reportUnknownParameterType, DiagnosticRule.reportUnknownArgumentType, @@ -287,6 +292,7 @@ export function getOffDiagnosticRuleSet(): DiagnosticRuleSet { reportPrivateUsage: 'none', reportConstantRedefinition: 'none', reportIncompatibleMethodOverride: 'none', + reportIncompatibleVariableOverride: 'none', reportInvalidStringEscapeSequence: 'none', reportUnknownParameterType: 'none', reportUnknownArgumentType: 'none', @@ -339,6 +345,7 @@ export function getBasicDiagnosticRuleSet(): DiagnosticRuleSet { reportPrivateUsage: 'none', reportConstantRedefinition: 'none', reportIncompatibleMethodOverride: 'none', + reportIncompatibleVariableOverride: 'none', reportInvalidStringEscapeSequence: 'warning', reportUnknownParameterType: 'none', reportUnknownArgumentType: 'none', @@ -391,6 +398,7 @@ export function getStrictDiagnosticRuleSet(): DiagnosticRuleSet { reportPrivateUsage: 'error', reportConstantRedefinition: 'error', reportIncompatibleMethodOverride: 'error', + reportIncompatibleVariableOverride: 'error', reportInvalidStringEscapeSequence: 'error', reportUnknownParameterType: 'error', reportUnknownArgumentType: 'error', @@ -857,6 +865,13 @@ export class ConfigOptions { defaultSettings.reportIncompatibleMethodOverride ), + // Read the "reportIncompatibleVariableOverride" entry. + reportIncompatibleVariableOverride: this._convertDiagnosticLevel( + configObj.reportIncompatibleVariableOverride, + DiagnosticRule.reportIncompatibleVariableOverride, + defaultSettings.reportIncompatibleVariableOverride + ), + // Read the "reportInvalidStringEscapeSequence" entry. reportInvalidStringEscapeSequence: this._convertDiagnosticLevel( configObj.reportInvalidStringEscapeSequence, diff --git a/server/src/common/diagnosticRules.ts b/server/src/common/diagnosticRules.ts index 9405a97ff..e39491152 100644 --- a/server/src/common/diagnosticRules.ts +++ b/server/src/common/diagnosticRules.ts @@ -37,6 +37,7 @@ export const enum DiagnosticRule { reportPrivateUsage = 'reportPrivateUsage', reportConstantRedefinition = 'reportConstantRedefinition', reportIncompatibleMethodOverride = 'reportIncompatibleMethodOverride', + reportIncompatibleVariableOverride = 'reportIncompatibleVariableOverride', reportInvalidStringEscapeSequence = 'reportInvalidStringEscapeSequence', reportUnknownParameterType = 'reportUnknownParameterType', reportUnknownArgumentType = 'reportUnknownArgumentType', diff --git a/server/src/localization/localize.ts b/server/src/localization/localize.ts index ddd08c62e..b4f75eb21 100644 --- a/server/src/localization/localize.ts +++ b/server/src/localization/localize.ts @@ -472,6 +472,8 @@ export namespace Localizer { new ParameterizedString<{ name: string }>(getRawString('Diagnostic.symbolIsUndefined')); export const symbolIsPossiblyUnbound = () => new ParameterizedString<{ name: string }>(getRawString('Diagnostic.symbolIsPossiblyUnbound')); + export const symbolOverridden = () => + new ParameterizedString<{ name: string; className: string }>(getRawString('Diagnostic.symbolOverridden')); export const tupleInAnnotation = () => getRawString('Diagnostic.tupleInAnnotation'); export const tupleSizeMismatch = () => new ParameterizedString<{ expected: number; received: number }>( @@ -656,6 +658,7 @@ export namespace Localizer { export const overloadCallName = () => new ParameterizedString<{ name: string }>(getRawString('DiagnosticAddendum.overloadCallName')); export const overriddenMethod = () => getRawString('DiagnosticAddendum.overriddenMethod'); + export const overriddenSymbol = () => getRawString('DiagnosticAddendum.overriddenSymbol'); export const overrideParamCount = () => new ParameterizedString<{ baseCount: number; overrideCount: number }>( getRawString('DiagnosticAddendum.overrideParamCount') diff --git a/server/src/localization/package.nls.en-us.json b/server/src/localization/package.nls.en-us.json index 2f60e8e01..f4ddbf23b 100644 --- a/server/src/localization/package.nls.en-us.json +++ b/server/src/localization/package.nls.en-us.json @@ -228,6 +228,7 @@ "symbolIsUnbound": "\"{name}\" is unbound", "symbolIsUndefined": "\"{name}\" is not defined", "symbolIsPossiblyUnbound": "\"{name}\" is possibly unbound", + "symbolOverridden": "\"{name}\" overrides symbol of same name in class \"{className}\"", "tupleInAnnotation": "Tuple expression not allowed in type annotation", "tupleSizeMismatch": "Tuple size mismatch: expected {expected} but received {received}", "typeAbstract": "Cannot instantiate abstract class \"{type}\"", @@ -323,6 +324,7 @@ "noOverloadAssignable": "No overloaded function matches type \"{type}\"", "overloadCallName": "Calling function \"{name}\"", "overriddenMethod": "Overridden method", + "overriddenSymbol": "Overridden symbol", "overrideParamCount": "Parameter count mismatch; base method has {baseCount}, but override has {overrideCount}", "overrideParamName": "Parameter {index} name mismatch: base parameter is named \"{baseName}\", override parameter is named \"{overrideName}\"", "overrideParamType": "Parameter {index} type mismatch: base parameter is type \"{baseType}\", override parameter is type \"{overrideType}\"", diff --git a/server/src/tests/checker.test.ts b/server/src/tests/checker.test.ts index 034b19a58..5a1561953 100644 --- a/server/src/tests/checker.test.ts +++ b/server/src/tests/checker.test.ts @@ -1001,6 +1001,19 @@ test('Classes4', () => { validateResults(analysisResults, 0); }); +test('Classes5', () => { + const configOptions = new ConfigOptions('.'); + + // By default, optional diagnostics are ignored. + let analysisResults = TestUtils.typeAnalyzeSampleFiles(['classes5.py'], configOptions); + validateResults(analysisResults, 0); + + // Turn on errors. + configOptions.diagnosticRuleSet.reportIncompatibleVariableOverride = 'error'; + analysisResults = TestUtils.typeAnalyzeSampleFiles(['classes5.py'], configOptions); + validateResults(analysisResults, 4); +}); + test('Mro1', () => { const analysisResults = TestUtils.typeAnalyzeSampleFiles(['mro1.py']); diff --git a/server/src/tests/samples/classes5.py b/server/src/tests/samples/classes5.py new file mode 100644 index 000000000..cef7ae99c --- /dev/null +++ b/server/src/tests/samples/classes5.py @@ -0,0 +1,70 @@ +# This sample tests the reportIncompatibleVariableOverride +# configuration option. + +from typing import Union +from abc import ABC, abstractmethod + +class ParentClass(ABC): + var1: int + var2: str + var3: Union[int, str] + var4: int + var5: int + + @property + def property1(self) -> int: + return 1 + + @property + def property2(self) -> int: + return 1 + + @property + def property3(self) -> int: + return 1 + + @property + def property4(self) -> Union[str, int]: + return 1 + + @property + @abstractmethod + def property5(self) -> int: + raise NotImplementedError() + + +class Subclass(ParentClass): + # This should generate an error because the type is incompatible. + var1: str + + var2: str + + var3: int + + # This should generate an error because the type is incompatible. + var4 = "" + + var5 = 5 + + # This should generate an error because a + # property object is different from a simple + # variable. + property1: int + + @property + def property2(self) -> int: + return 3 + + # This should generate an error because it is + # an incompatible property. + @property + def property3(self) -> str: + return "" + + @property + def property4(self) -> int: + return 1 + + @property + def property5(self) -> int: + return 4