From 90467942051606c783726f94cac141a2ccd183c2 Mon Sep 17 00:00:00 2001 From: Eric Traut Date: Wed, 21 Oct 2020 09:59:36 -0700 Subject: [PATCH] Removed recently-added "reportInvalidTypeVarUse" diagnostic rule and associated checks. After further discussion, we decided that there are legitimate cases where a TypeVar can appear only once in a function signature. --- docs/configuration.md | 2 - .../pyright-internal/src/analyzer/checker.ts | 51 ------------------- .../src/analyzer/parseTreeUtils.ts | 13 ----- .../src/common/configOptions.ts | 14 ----- .../src/common/diagnosticRules.ts | 1 - .../src/tests/samples/typeVar5.py | 50 ------------------ .../src/tests/typeEvaluator2.test.ts | 6 --- packages/vscode-pyright/package.json | 11 ---- .../schemas/pyrightconfig.schema.json | 6 --- 9 files changed, 154 deletions(-) delete mode 100644 packages/pyright-internal/src/tests/samples/typeVar5.py diff --git a/docs/configuration.md b/docs/configuration.md index d870fa516..a811ff6a5 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -112,8 +112,6 @@ The following settings control pyright’s diagnostic output (warnings or errors **reportMissingTypeArgument** [boolean or string, optional]: Generate or suppress diagnostics when a generic class is used without providing explicit or implicit type arguments. The default value for this setting is 'none'. -**reportInvalidTypeVarUse** [boolean or string, optional]: Generate or suppress diagnostics when a TypeVar is used inappropriately (e.g. if a TypeVar appears only once) within a generic function signature. The default value for this setting is 'none'. - **reportCallInDefaultInitializer** [boolean or string, optional]: Generate or suppress diagnostics for function calls within a default value initialization expression. Such calls can mask expensive operations that are performed at module initialization time. The default value for this setting is 'none'. **reportUnnecessaryIsInstance** [boolean or string, optional]: Generate or suppress diagnostics for 'isinstance' or 'issubclass' calls where the result is statically determined to be always true or always false. Such calls are often indicative of a programming error. The default value for this setting is 'none'. diff --git a/packages/pyright-internal/src/analyzer/checker.ts b/packages/pyright-internal/src/analyzer/checker.ts index 11aa35c14..bf976be9f 100644 --- a/packages/pyright-internal/src/analyzer/checker.ts +++ b/packages/pyright-internal/src/analyzer/checker.ts @@ -344,8 +344,6 @@ export class Checker extends ParseTreeWalker { this._scopedNodes.push(node); - this._validateFunctionTypeVarUsage(node); - if (functionTypeResult && functionTypeResult.decoratedType.category === TypeCategory.OverloadedFunction) { const overloads = functionTypeResult.decoratedType.overloads; if (overloads.length > 1) { @@ -794,55 +792,6 @@ export class Checker extends ParseTreeWalker { return false; } - // Verifies that each local type variable is used more than once. - private _validateFunctionTypeVarUsage(node: FunctionNode) { - // Skip this check entirely if it's disabled. - if (this._fileInfo.diagnosticRuleSet.reportInvalidTypeVarUse === 'none') { - return; - } - - const localTypeVarUsage = new Map(); - - const nameWalker = new ParseTreeUtils.NameNodeWalker((nameNode) => { - const nameType = this._evaluator.getType(nameNode); - if (nameType && isTypeVar(nameType)) { - if (nameType.scopeId === node.id) { - if (!localTypeVarUsage.has(nameType.details.name)) { - localTypeVarUsage.set(nameType.details.name, [nameNode]); - } else { - localTypeVarUsage.get(nameType.details.name)!.push(nameNode); - } - } - } - }); - - // Find all of the local type variables in signature. - node.parameters.forEach((param) => { - const annotation = param.typeAnnotation || param.typeAnnotationComment; - if (annotation) { - nameWalker.walk(annotation); - } - }); - - if (node.returnTypeAnnotation) { - nameWalker.walk(node.returnTypeAnnotation); - } - - // Report errors for all local type variables that appear only once. - localTypeVarUsage.forEach((nameNodes) => { - if (nameNodes.length === 1) { - this._evaluator.addDiagnostic( - this._fileInfo.diagnosticRuleSet.reportInvalidTypeVarUse, - DiagnosticRule.reportInvalidTypeVarUse, - Localizer.Diagnostic.typeVarUsedOnlyOnce().format({ - name: nameNodes[0].value, - }), - nameNodes[0] - ); - } - }); - } - private _validateOverloadConsistency( node: FunctionNode, functionType: FunctionType, diff --git a/packages/pyright-internal/src/analyzer/parseTreeUtils.ts b/packages/pyright-internal/src/analyzer/parseTreeUtils.ts index 1f36ea02a..1069e1426 100644 --- a/packages/pyright-internal/src/analyzer/parseTreeUtils.ts +++ b/packages/pyright-internal/src/analyzer/parseTreeUtils.ts @@ -1034,16 +1034,3 @@ export function isAssignmentToDefaultsFollowingNamedTuple(callNode: ParseNode): return false; } - -// This simple parse tree walker calls a callback function -// for each NameNode it encounters. -export class NameNodeWalker extends ParseTreeWalker { - constructor(private _callback: (node: NameNode) => void) { - super(); - } - - visitName(node: NameNode) { - this._callback(node); - return true; - } -} diff --git a/packages/pyright-internal/src/common/configOptions.ts b/packages/pyright-internal/src/common/configOptions.ts index 0d944e02f..2bcd7d470 100644 --- a/packages/pyright-internal/src/common/configOptions.ts +++ b/packages/pyright-internal/src/common/configOptions.ts @@ -192,9 +192,6 @@ export interface DiagnosticRuleSet { // Report usage of generic class without explicit type arguments? reportMissingTypeArgument: DiagnosticLevel; - // Report improper usage of type variables within function signatures? - reportInvalidTypeVarUse: DiagnosticLevel; - // Report usage of function call within default value // initialization expression? reportCallInDefaultInitializer: DiagnosticLevel; @@ -280,7 +277,6 @@ export function getDiagLevelDiagnosticRules() { DiagnosticRule.reportUnknownVariableType, DiagnosticRule.reportUnknownMemberType, DiagnosticRule.reportMissingTypeArgument, - DiagnosticRule.reportInvalidTypeVarUse, DiagnosticRule.reportCallInDefaultInitializer, DiagnosticRule.reportUnnecessaryIsInstance, DiagnosticRule.reportUnnecessaryCast, @@ -342,7 +338,6 @@ export function getOffDiagnosticRuleSet(): DiagnosticRuleSet { reportUnknownVariableType: 'none', reportUnknownMemberType: 'none', reportMissingTypeArgument: 'none', - reportInvalidTypeVarUse: 'none', reportCallInDefaultInitializer: 'none', reportUnnecessaryIsInstance: 'none', reportUnnecessaryCast: 'none', @@ -400,7 +395,6 @@ export function getBasicDiagnosticRuleSet(): DiagnosticRuleSet { reportUnknownVariableType: 'none', reportUnknownMemberType: 'none', reportMissingTypeArgument: 'none', - reportInvalidTypeVarUse: 'none', reportCallInDefaultInitializer: 'none', reportUnnecessaryIsInstance: 'none', reportUnnecessaryCast: 'none', @@ -458,7 +452,6 @@ export function getStrictDiagnosticRuleSet(): DiagnosticRuleSet { reportUnknownVariableType: 'error', reportUnknownMemberType: 'error', reportMissingTypeArgument: 'error', - reportInvalidTypeVarUse: 'error', reportCallInDefaultInitializer: 'none', reportUnnecessaryIsInstance: 'error', reportUnnecessaryCast: 'error', @@ -1027,13 +1020,6 @@ export class ConfigOptions { defaultSettings.reportMissingTypeArgument ), - // Read the "reportInvalidTypeVarUse" entry. - reportInvalidTypeVarUse: this._convertDiagnosticLevel( - configObj.reportInvalidTypeVarUse, - DiagnosticRule.reportInvalidTypeVarUse, - defaultSettings.reportInvalidTypeVarUse - ), - // Read the "reportCallInDefaultInitializer" entry. reportCallInDefaultInitializer: this._convertDiagnosticLevel( configObj.reportCallInDefaultInitializer, diff --git a/packages/pyright-internal/src/common/diagnosticRules.ts b/packages/pyright-internal/src/common/diagnosticRules.ts index 775e406ff..37abf86ee 100644 --- a/packages/pyright-internal/src/common/diagnosticRules.ts +++ b/packages/pyright-internal/src/common/diagnosticRules.ts @@ -49,7 +49,6 @@ export enum DiagnosticRule { reportUnknownVariableType = 'reportUnknownVariableType', reportUnknownMemberType = 'reportUnknownMemberType', reportMissingTypeArgument = 'reportMissingTypeArgument', - reportInvalidTypeVarUse = 'reportInvalidTypeVarUse', reportCallInDefaultInitializer = 'reportCallInDefaultInitializer', reportUnnecessaryIsInstance = 'reportUnnecessaryIsInstance', reportUnnecessaryCast = 'reportUnnecessaryCast', diff --git a/packages/pyright-internal/src/tests/samples/typeVar5.py b/packages/pyright-internal/src/tests/samples/typeVar5.py deleted file mode 100644 index be2ce68eb..000000000 --- a/packages/pyright-internal/src/tests/samples/typeVar5.py +++ /dev/null @@ -1,50 +0,0 @@ -# This sample tests the reporting of incorrect TypeVar usage within -# a generic function. A TypeVar must appear at least twice to be -# considered legitimate. - -# pyright: reportInvalidTypeVarUse=true - -from typing import Dict, Generic, List, TypeVar - - -_T = TypeVar("_T") -_S = TypeVar("_S") - - -class Foo(Generic[_T]): - def m1(self, v1: _T) -> None: - ... - - # This should generate an error because _S - # is a local typeVar and appears only once. - def m2(self, v1: _S) -> None: - ... - - # This should generate an error because _S - # is a local typeVar and appears only once. - def m3(self, v1: _T) -> _S: - ... - - -# This should generate an error because _T -# is a local typeVar and appears only once. -def f1(self, v1: _T) -> None: - ... - - -def f2(self, v1: _T, v2: List[_T]) -> None: - ... - - -def f3(self, v1: _T) -> _T: - ... - - -def f4(self) -> Dict[_T, _T]: - ... - - -# This should generate an error because _T -# is a local typeVar and appears only once. -def f5(self) -> List[_T]: - ... diff --git a/packages/pyright-internal/src/tests/typeEvaluator2.test.ts b/packages/pyright-internal/src/tests/typeEvaluator2.test.ts index 6883c6fda..e9ac7c1ff 100644 --- a/packages/pyright-internal/src/tests/typeEvaluator2.test.ts +++ b/packages/pyright-internal/src/tests/typeEvaluator2.test.ts @@ -945,12 +945,6 @@ test('TypeVar4', () => { TestUtils.validateResults(analysisResults, 4); }); -test('TypeVar5', () => { - const analysisResults = TestUtils.typeAnalyzeSampleFiles(['typeVar5.py']); - - TestUtils.validateResults(analysisResults, 4); -}); - test('Annotated1', () => { const configOptions = new ConfigOptions('.'); diff --git a/packages/vscode-pyright/package.json b/packages/vscode-pyright/package.json index 40d5d82ae..21f02bf5d 100644 --- a/packages/vscode-pyright/package.json +++ b/packages/vscode-pyright/package.json @@ -477,17 +477,6 @@ "error" ] }, - "reportInvalidTypeVarUse": { - "type": "string", - "description": "Diagnostics for improper use of type variables in a function signature.", - "default": "none", - "enum": [ - "none", - "information", - "warning", - "error" - ] - }, "reportCallInDefaultInitializer": { "type": "string", "description": "Diagnostics for function calls within a default value initialization expression. Such calls can mask expensive operations that are performed at module initialization time.", diff --git a/packages/vscode-pyright/schemas/pyrightconfig.schema.json b/packages/vscode-pyright/schemas/pyrightconfig.schema.json index f8b696325..b0aa5a3dd 100644 --- a/packages/vscode-pyright/schemas/pyrightconfig.schema.json +++ b/packages/vscode-pyright/schemas/pyrightconfig.schema.json @@ -321,12 +321,6 @@ "title": "Controls reporting generic class reference with missing type arguments", "default": "none" }, - "reportInvalidTypeVarUse": { - "$id": "#/properties/reportInvalidTypeVarUse", - "$ref": "#/definitions/diagnostic", - "title": "Controls reporting improper use of type variables within function signatures", - "default": "none" - }, "reportCallInDefaultInitializer": { "$id": "#/properties/reportCallInDefaultInitializer", "$ref": "#/definitions/diagnostic",