From e2e3a5c9ffebec01850cd1d54c2c21883f5bc83c Mon Sep 17 00:00:00 2001 From: Eric Traut Date: Thu, 16 Mar 2023 09:52:25 -0600 Subject: [PATCH] Added new diagnostic check `reportImplicitOverride`. This addresses https://github.com/microsoft/pyright/issues/4788. --- docs/configuration.md | 5 +- .../pyright-internal/src/analyzer/checker.ts | 57 ++++++++++++++----- .../src/common/configOptions.ts | 7 +++ .../src/common/diagnosticRules.ts | 1 + .../src/localization/localize.ts | 8 ++- .../src/localization/package.nls.en-us.json | 3 +- .../src/tests/samples/override2.py | 23 ++++++++ .../src/tests/typeEvaluator5.test.ts | 16 ++++++ packages/vscode-pyright/package.json | 11 ++++ .../schemas/pyrightconfig.schema.json | 6 ++ 10 files changed, 119 insertions(+), 18 deletions(-) create mode 100644 packages/pyright-internal/src/tests/samples/override2.py diff --git a/docs/configuration.md b/docs/configuration.md index c7e949533..a0e8f0c37 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -180,6 +180,8 @@ The following settings control pyright’s diagnostic output (warnings or errors **reportMatchNotExhaustive** [boolean or string, optional]: Generate or suppress diagnostics for a `match` statement that does not provide cases that exhaustively match against all potential types of the target expression. The default value for this setting is `"none"`. + **reportImplicitOverride** [boolean or string, optional]: Generate or suppress diagnostics for overridden methods in a class that are missing an explicit `@override` decorator. + **reportShadowedImports** [boolean or string, optional]: Generate or suppress diagnostics for files that are overriding a module in the stdlib. The default value for this setting is `"none"`. ## Execution Environment Options @@ -337,7 +339,6 @@ The following table lists the default severity levels for each diagnostic rule w | reportMissingTypeArgument | "none" | "none" | "error" | | reportOverlappingOverload | "none" | "none" | "error" | | reportPrivateUsage | "none" | "none" | "error" | -| reportShadowedImports | "none" | "none" | "none" | | reportTypeCommentUsage | "none" | "none" | "error" | | reportUnknownArgumentType | "none" | "none" | "error" | | reportUnknownLambdaType | "none" | "none" | "error" | @@ -357,9 +358,11 @@ The following table lists the default severity levels for each diagnostic rule w | reportUntypedFunctionDecorator | "none" | "none" | "error" | | reportUntypedNamedTuple | "none" | "none" | "error" | | reportCallInDefaultInitializer | "none" | "none" | "none" | +| reportImplicitOverride | "none" | "none" | "none" | | reportImplicitStringConcatenation | "none" | "none" | "none" | | reportMissingSuperCall | "none" | "none" | "none" | | reportPropertyTypeMismatch | "none" | "none" | "none" | +| reportShadowedImports | "none" | "none" | "none" | | reportUninitializedInstanceVariable | "none" | "none" | "none" | | reportUnnecessaryTypeIgnoreComment | "none" | "none" | "none" | | reportUnusedCallResult | "none" | "none" | "none" | diff --git a/packages/pyright-internal/src/analyzer/checker.ts b/packages/pyright-internal/src/analyzer/checker.ts index 82243ee01..c11054077 100644 --- a/packages/pyright-internal/src/analyzer/checker.ts +++ b/packages/pyright-internal/src/analyzer/checker.ts @@ -5258,7 +5258,7 @@ export class Checker extends ParseTreeWalker { return; } - let foundOverride = false; + let firstOverride: ClassMember | undefined; for (const baseClass of classType.details.baseClasses) { if (!isClass(baseClass)) { @@ -5279,22 +5279,27 @@ export class Checker extends ParseTreeWalker { continue; } - foundOverride = true; + firstOverride = firstOverride ?? baseClassAndSymbol; + this._validateBaseClassOverride(baseClassAndSymbol, symbol, typeOfSymbol, classType, name); } - if (!foundOverride) { + if (!firstOverride) { // If this is a method decorated with @override, validate that there // is a base class method of the same name. - this._validateOverrideDecorator(typeOfSymbol); + this._validateOverrideDecoratorNotPresent(typeOfSymbol); + } else { + this._validateOverrideDecoratorPresent(typeOfSymbol, firstOverride); } }); } - // Determines whether the type is a function or overloaded function with an @override - // decorator. In this case, an error is reported because no base class has declared - // a method of the same name. - private _validateOverrideDecorator(overrideType: Type) { + private _validateOverrideDecoratorPresent(overrideType: Type, baseMember: ClassMember) { + // Skip this check if disabled. + if (this._fileInfo.diagnosticRuleSet.reportImplicitOverride === 'none') { + return; + } + let overrideFunction: FunctionType | undefined; if (isFunction(overrideType)) { @@ -5303,11 +5308,7 @@ export class Checker extends ParseTreeWalker { overrideFunction = OverloadedFunctionType.getImplementation(overrideType); } - if ( - !overrideFunction || - !FunctionType.isOverridden(overrideFunction) || - !overrideFunction.details.declaration - ) { + if (!overrideFunction?.details.declaration || FunctionType.isOverridden(overrideFunction)) { return; } @@ -5315,7 +5316,35 @@ export class Checker extends ParseTreeWalker { this._evaluator.addDiagnostic( this._fileInfo.diagnosticRuleSet.reportGeneralTypeIssues, DiagnosticRule.reportGeneralTypeIssues, - Localizer.Diagnostic.overrideNotFound().format({ name: funcNode.name.value }), + Localizer.Diagnostic.overrideDecoratorMissing().format({ + name: funcNode.name.value, + className: this._evaluator.printType(convertToInstance(baseMember.classType)), + }), + funcNode.name + ); + } + + // Determines whether the type is a function or overloaded function with an @override + // decorator. In this case, an error is reported because no base class has declared + // a method of the same name. + private _validateOverrideDecoratorNotPresent(overrideType: Type) { + let overrideFunction: FunctionType | undefined; + + if (isFunction(overrideType)) { + overrideFunction = overrideType; + } else if (isOverloadedFunction(overrideType)) { + overrideFunction = OverloadedFunctionType.getImplementation(overrideType); + } + + if (!overrideFunction?.details.declaration || !FunctionType.isOverridden(overrideFunction)) { + return; + } + + const funcNode = overrideFunction.details.declaration.node; + this._evaluator.addDiagnostic( + this._fileInfo.diagnosticRuleSet.reportGeneralTypeIssues, + DiagnosticRule.reportGeneralTypeIssues, + Localizer.Diagnostic.overriddenMethodNotFound().format({ name: funcNode.name.value }), funcNode.name ); } diff --git a/packages/pyright-internal/src/common/configOptions.ts b/packages/pyright-internal/src/common/configOptions.ts index 0a0b128fe..52480f95f 100644 --- a/packages/pyright-internal/src/common/configOptions.ts +++ b/packages/pyright-internal/src/common/configOptions.ts @@ -310,6 +310,9 @@ export interface DiagnosticRuleSet { // Report files that match stdlib modules. reportShadowedImports: DiagnosticLevel; + + // Report missing @override decorator. + reportImplicitOverride: DiagnosticLevel; } export function cloneDiagnosticRuleSet(diagSettings: DiagnosticRuleSet): DiagnosticRuleSet { @@ -405,6 +408,7 @@ export function getDiagLevelDiagnosticRules() { DiagnosticRule.reportUnnecessaryTypeIgnoreComment, DiagnosticRule.reportMatchNotExhaustive, DiagnosticRule.reportShadowedImports, + DiagnosticRule.reportImplicitOverride, ]; } @@ -490,6 +494,7 @@ export function getOffDiagnosticRuleSet(): DiagnosticRuleSet { reportUnnecessaryTypeIgnoreComment: 'none', reportMatchNotExhaustive: 'none', reportShadowedImports: 'none', + reportImplicitOverride: 'none', }; return diagSettings; @@ -571,6 +576,7 @@ export function getBasicDiagnosticRuleSet(): DiagnosticRuleSet { reportUnnecessaryTypeIgnoreComment: 'none', reportMatchNotExhaustive: 'none', reportShadowedImports: 'none', + reportImplicitOverride: 'none', }; return diagSettings; @@ -652,6 +658,7 @@ export function getStrictDiagnosticRuleSet(): DiagnosticRuleSet { reportUnnecessaryTypeIgnoreComment: 'none', reportMatchNotExhaustive: 'error', reportShadowedImports: 'none', + reportImplicitOverride: 'none', }; return diagSettings; diff --git a/packages/pyright-internal/src/common/diagnosticRules.ts b/packages/pyright-internal/src/common/diagnosticRules.ts index 99885d440..0da7d6926 100644 --- a/packages/pyright-internal/src/common/diagnosticRules.ts +++ b/packages/pyright-internal/src/common/diagnosticRules.ts @@ -81,4 +81,5 @@ export enum DiagnosticRule { reportUnnecessaryTypeIgnoreComment = 'reportUnnecessaryTypeIgnoreComment', reportMatchNotExhaustive = 'reportMatchNotExhaustive', reportShadowedImports = 'reportShadowedImports', + reportImplicitOverride = 'reportImplicitOverride', } diff --git a/packages/pyright-internal/src/localization/localize.ts b/packages/pyright-internal/src/localization/localize.ts index 462dee571..7682aa6ae 100644 --- a/packages/pyright-internal/src/localization/localize.ts +++ b/packages/pyright-internal/src/localization/localize.ts @@ -629,8 +629,12 @@ export namespace Localizer { new ParameterizedString<{ name: string }>(getRawString('Diagnostic.overloadWithImplementation')); export const overloadWithoutImplementation = () => new ParameterizedString<{ name: string }>(getRawString('Diagnostic.overloadWithoutImplementation')); - export const overrideNotFound = () => - new ParameterizedString<{ name: string }>(getRawString('Diagnostic.overrideNotFound')); + export const overriddenMethodNotFound = () => + new ParameterizedString<{ name: string }>(getRawString('Diagnostic.overriddenMethodNotFound')); + export const overrideDecoratorMissing = () => + new ParameterizedString<{ name: string; className: string }>( + getRawString('Diagnostic.overrideDecoratorMissing') + ); export const paramAfterKwargsParam = () => getRawString('Diagnostic.paramAfterKwargsParam'); export const paramAlreadyAssigned = () => new ParameterizedString<{ name: string }>(getRawString('Diagnostic.paramAlreadyAssigned')); diff --git a/packages/pyright-internal/src/localization/package.nls.en-us.json b/packages/pyright-internal/src/localization/package.nls.en-us.json index adff29f9b..a4e6858a3 100644 --- a/packages/pyright-internal/src/localization/package.nls.en-us.json +++ b/packages/pyright-internal/src/localization/package.nls.en-us.json @@ -303,7 +303,8 @@ "overloadReturnTypeMismatch": "Overload {prevIndex} for \"{name}\" overlaps overload {newIndex} and returns an incompatible type", "overloadWithImplementation": "\"{name}\" is marked as overload, but it includes an implementation", "overloadWithoutImplementation": "\"{name}\" is marked as overload, but no implementation is provided", - "overrideNotFound": "Method \"{name}\" is marked as override, but no base method of same name is present", + "overriddenMethodNotFound": "Method \"{name}\" is marked as override, but no base method of same name is present", + "overrideDecoratorMissing": "Method \"{name}\" is not marked as override but is overriding a method in class \"{className}\"", "paramAfterKwargsParam": "Parameter cannot follow \"**\" parameter", "paramAlreadyAssigned": "Parameter \"{name}\" is already assigned", "paramAnnotationMissing": "Type annotation is missing for parameter \"{name}\"", diff --git a/packages/pyright-internal/src/tests/samples/override2.py b/packages/pyright-internal/src/tests/samples/override2.py new file mode 100644 index 000000000..4ee6e17d5 --- /dev/null +++ b/packages/pyright-internal/src/tests/samples/override2.py @@ -0,0 +1,23 @@ +# This sample tests the reportImplicitOverride diagnostic check +# (strict enforcement of PEP 698). + +from typing_extensions import override + + +class Base: + @override + def __init__(self): + pass + + def method1(self): + pass + + +class Child(Base): + # This should generate an error if reportImplicitOverride is enabled. + def __init__(self): + pass + + # This should generate an error if reportImplicitOverride is enabled. + def method1(self): + pass diff --git a/packages/pyright-internal/src/tests/typeEvaluator5.test.ts b/packages/pyright-internal/src/tests/typeEvaluator5.test.ts index 7f4d0bb31..e718bf279 100644 --- a/packages/pyright-internal/src/tests/typeEvaluator5.test.ts +++ b/packages/pyright-internal/src/tests/typeEvaluator5.test.ts @@ -150,6 +150,22 @@ test('Override1', () => { TestUtils.validateResults(analysisResults, 2); }); +test('Override1', () => { + const analysisResults = TestUtils.typeAnalyzeSampleFiles(['override1.py']); + TestUtils.validateResults(analysisResults, 2); +}); + +test('Override2', () => { + const configOptions = new ConfigOptions('.'); + + const analysisResults1 = TestUtils.typeAnalyzeSampleFiles(['override2.py'], configOptions); + TestUtils.validateResults(analysisResults1, 0); + + configOptions.diagnosticRuleSet.reportImplicitOverride = 'error'; + const analysisResults2 = TestUtils.typeAnalyzeSampleFiles(['override2.py'], configOptions); + TestUtils.validateResults(analysisResults2, 2); +}); + test('TypeVarDefault1', () => { const analysisResults = TestUtils.typeAnalyzeSampleFiles(['typeVarDefault1.py']); TestUtils.validateResults(analysisResults, 12); diff --git a/packages/vscode-pyright/package.json b/packages/vscode-pyright/package.json index 55847c0d5..14ee38ac8 100644 --- a/packages/vscode-pyright/package.json +++ b/packages/vscode-pyright/package.json @@ -856,6 +856,17 @@ "warning", "error" ] + }, + "reportImplicitOverride": { + "type": "string", + "description": "Diagnostics for overridden methods that do not include an `@override` decorator.", + "default": "none", + "enum": [ + "none", + "information", + "warning", + "error" + ] } } }, diff --git a/packages/vscode-pyright/schemas/pyrightconfig.schema.json b/packages/vscode-pyright/schemas/pyrightconfig.schema.json index 6845f9551..b5d8287a5 100644 --- a/packages/vscode-pyright/schemas/pyrightconfig.schema.json +++ b/packages/vscode-pyright/schemas/pyrightconfig.schema.json @@ -526,6 +526,12 @@ "title": "Controls reporting of shadowed imports of stdlib modules", "default": "none" }, + "reportImplicitOverride": { + "$id": "#/properties/reportImplicitOverride", + "$ref": "#/definitions/diagnostic", + "title": "Controls reporting overridden methods that are missing an `@override` decorator", + "default": "none" + }, "extraPaths": { "$id": "#/properties/extraPaths", "type": "array",