From 7be19b6a52dba7f938cf75587ca09de891f25669 Mon Sep 17 00:00:00 2001 From: Eric Traut Date: Thu, 24 Oct 2019 23:22:03 -0700 Subject: [PATCH] Expanded isinstance support (for both the reportUnnecessaryIsInstance switch and type constraints) to also support issubclass. --- client/schemas/pyrightconfig.schema.json | 2 +- docs/configuration.md | 2 +- server/src/analyzer/binder.ts | 3 +- server/src/analyzer/typeAnalyzer.ts | 44 +++++++++++++------ server/src/analyzer/typeConstraint.ts | 42 ++++++++++++------ server/src/analyzer/typeUtils.ts | 25 +++++++++++ server/src/common/configOptions.ts | 2 +- server/src/tests/samples/typeConstraint2.py | 23 +++++++++- .../tests/samples/unnecessaryIsSubclass1.py | 24 ++++++++++ server/src/tests/typeAnalyzer.test.ts | 14 +++++- 10 files changed, 148 insertions(+), 33 deletions(-) create mode 100644 server/src/tests/samples/unnecessaryIsSubclass1.py diff --git a/client/schemas/pyrightconfig.schema.json b/client/schemas/pyrightconfig.schema.json index cee61452f..2f9440dd6 100644 --- a/client/schemas/pyrightconfig.schema.json +++ b/client/schemas/pyrightconfig.schema.json @@ -249,7 +249,7 @@ "reportUnnecessaryIsInstance": { "$id": "#/properties/reportUnnecessaryIsInstance", "$ref": "#/definitions/diagnostic", - "title": "Controls reporting calls to 'isinstance' where the result is statically determined to be always true or false", + "title": "Controls reporting calls to 'isinstance' or 'issubclass' where the result is statically determined to be always true or false", "default": "none" }, "reportUnnecessaryCast": { diff --git a/docs/configuration.md b/docs/configuration.md index ea8e2fc41..b7730dcef 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -92,7 +92,7 @@ The following settings control pyright’s diagnostic output (warnings or errors **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' 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'. +**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'. **reportUnnecessaryCast** [boolean or string, optional]: Generate or suppress diagnostics for 'cast' calls that are statically determined to be unnecessary. Such calls are sometimes indicative of a programming error. The default value for this setting is 'none'. diff --git a/server/src/analyzer/binder.ts b/server/src/analyzer/binder.ts index 527df1278..921f45079 100644 --- a/server/src/analyzer/binder.ts +++ b/server/src/analyzer/binder.ts @@ -1324,7 +1324,8 @@ export class Binder extends ParseTreeWalker { case ParseNodeType.Call: { return expression.leftExpression.nodeType === ParseNodeType.Name && - expression.leftExpression.nameToken.value === 'isinstance' && + (expression.leftExpression.nameToken.value === 'isinstance' || + expression.leftExpression.nameToken.value === 'issubclass') && expression.arguments.length === 2; } } diff --git a/server/src/analyzer/typeAnalyzer.ts b/server/src/analyzer/typeAnalyzer.ts index f9867400b..fe17d53fb 100644 --- a/server/src/analyzer/typeAnalyzer.ts +++ b/server/src/analyzer/typeAnalyzer.ts @@ -1547,7 +1547,7 @@ export class TypeAnalyzer extends ParseTreeWalker { return moduleType; } - // Validates that a call to isinstance is necessary. This is a + // Validates that a call to isinstance or issubclass are necessary. This is a // common source of programming errors. private _validateIsInstanceCallNecessary(node: CallExpressionNode) { if (this._fileInfo.diagnosticSettings.reportUnnecessaryIsInstance === 'none') { @@ -1564,12 +1564,21 @@ export class TypeAnalyzer extends ParseTreeWalker { } if (node.leftExpression.nodeType !== ParseNodeType.Name || - node.leftExpression.nameToken.value !== 'isinstance' || + (node.leftExpression.nameToken.value !== 'isinstance' && + node.leftExpression.nameToken.value !== 'issubclass') || node.arguments.length !== 2) { return; } - const arg0Type = this._getTypeOfExpression(node.arguments[0].valueExpression); + const callName = node.leftExpression.nameToken.value; + const isInstanceCheck = callName === 'isinstance'; + const arg0Type = TypeUtils.doForSubtypes( + this._getTypeOfExpression(node.arguments[0].valueExpression), + subtype => { + + return TypeUtils.transformTypeObjectToClass(subtype); + }); + if (isAnyOrUnknown(arg0Type)) { return; } @@ -1580,7 +1589,7 @@ export class TypeAnalyzer extends ParseTreeWalker { if (arg1Type.category === TypeCategory.Class) { classTypeList.push(arg1Type); } else if (arg1Type.category === TypeCategory.Object) { - // The isinstance call supports a variation where the second + // The isinstance and issubclass call supports a variation where the second // parameter is a tuple of classes. const objClass = arg1Type.classType; if (ClassType.isBuiltIn(objClass, 'Tuple') && ClassType.getTypeArguments(objClass)) { @@ -1600,7 +1609,7 @@ export class TypeAnalyzer extends ParseTreeWalker { return combineTypes(types); }; - const filterType = (varType: ClassType): ObjectType[] => { + const filterType = (varType: ClassType): (ObjectType[] | ClassType[]) => { const filteredTypes: ClassType[] = []; for (const filterType of classTypeList) { @@ -1619,13 +1628,20 @@ export class TypeAnalyzer extends ParseTreeWalker { } } + if (!isInstanceCheck) { + return filteredTypes; + } + return filteredTypes.map(t => ObjectType.create(t)); }; let filteredType: Type; - if (arg0Type.category === TypeCategory.Object) { + if (isInstanceCheck && arg0Type.category === TypeCategory.Object) { const remainingTypes = filterType(arg0Type.classType); filteredType = finalizeFilteredTypeList(remainingTypes); + } else if (!isInstanceCheck && arg0Type.category === TypeCategory.Class) { + const remainingTypes = filterType(arg0Type); + filteredType = finalizeFilteredTypeList(remainingTypes); } else if (arg0Type.category === TypeCategory.Union) { let remainingTypes: Type[] = []; let foundAnyType = false; @@ -1635,9 +1651,10 @@ export class TypeAnalyzer extends ParseTreeWalker { foundAnyType = true; } - if (t.category === TypeCategory.Object) { - remainingTypes = remainingTypes.concat( - filterType(t.classType)); + if (isInstanceCheck && t.category === TypeCategory.Object) { + remainingTypes = remainingTypes.concat(filterType(t.classType)); + } else if (!isInstanceCheck && t.category === TypeCategory.Class) { + remainingTypes = remainingTypes.concat(filterType(t)); } }); @@ -1656,19 +1673,20 @@ export class TypeAnalyzer extends ParseTreeWalker { return combineTypes(objTypeList); }; + const callType = isInstanceCheck ? 'instance' : 'subclass'; if (filteredType.category === TypeCategory.Never) { this._addDiagnostic( this._fileInfo.diagnosticSettings.reportUnnecessaryIsInstance, DiagnosticRule.reportUnnecessaryIsInstance, - `Unnecessary isinstance call: '${ printType(arg0Type) }' ` + - `is never instance of '${ printType(getTestType()) }'`, + `Unnecessary ${ callName } call: '${ printType(arg0Type) }' ` + + `is never ${ callType } of '${ printType(getTestType()) }'`, node); } else if (isTypeSame(filteredType, arg0Type)) { this._addDiagnostic( this._fileInfo.diagnosticSettings.reportUnnecessaryIsInstance, DiagnosticRule.reportUnnecessaryIsInstance, - `Unnecessary isinstance call: '${ printType(arg0Type) }' ` + - `is always instance of '${ printType(getTestType()) }'`, + `Unnecessary ${ callName } call: '${ printType(arg0Type) }' ` + + `is always ${ callType } of '${ printType(getTestType()) }'`, node); } } diff --git a/server/src/analyzer/typeConstraint.ts b/server/src/analyzer/typeConstraint.ts index ff55fc835..f34f1c86e 100644 --- a/server/src/analyzer/typeConstraint.ts +++ b/server/src/analyzer/typeConstraint.ts @@ -269,12 +269,14 @@ export class TypeConstraintBuilder { } } else if (testExpression.nodeType === ParseNodeType.Call) { if (testExpression.leftExpression.nodeType === ParseNodeType.Name && - testExpression.leftExpression.nameToken.value === 'isinstance' && + (testExpression.leftExpression.nameToken.value === 'isinstance' || + testExpression.leftExpression.nameToken.value === 'issubclass') && testExpression.arguments.length === 2) { // Make sure the first parameter is a supported expression type // and the second parameter is a valid class type or a tuple // of valid class types. + const isInstanceCheck = testExpression.leftExpression.nameToken.value === 'isinstance'; const arg0Expr = testExpression.arguments[0].valueExpression; const arg1Expr = testExpression.arguments[1].valueExpression; @@ -284,8 +286,10 @@ export class TypeConstraintBuilder { // Create a shared lambda for creating the actual type constraint. const createIsInstanceTypeConstraint = (classList: ClassType[]) => { const originalType = typeEvaluator(arg0Expr); - const positiveType = this._transformTypeForIsInstanceExpression(originalType, classList, true); - const negativeType = this._transformTypeForIsInstanceExpression(originalType, classList, false); + const positiveType = this._transformTypeForIsInstanceExpression( + originalType, classList, isInstanceCheck, true); + const negativeType = this._transformTypeForIsInstanceExpression( + originalType, classList, isInstanceCheck, false); const trueConstraint = new TypeConstraint(arg0Expr, positiveType); const falseConstraint = new TypeConstraint(arg0Expr, negativeType); return { @@ -412,15 +416,19 @@ export class TypeConstraintBuilder { }); } - // Represents an "isinstance" check, potentially constraining a + // Represents an "isinstance" or "issubclass" check, potentially constraining a // union type. private static _transformTypeForIsInstanceExpression(type: Type, classTypeList: ClassType[], - isPositiveTest: boolean): Type { + isInstanceCheck: boolean, isPositiveTest: boolean): Type { + + const effectiveType = TypeUtils.doForSubtypes(type, subtype => { + return TypeUtils.transformTypeObjectToClass(subtype); + }); // Filters the varType by the parameters of the isinstance // and returns the list of types the varType could be after // applying the filter. - const filterType = (varType: ClassType): ObjectType[] => { + const filterType = (varType: ClassType): (ObjectType[] | ClassType[]) => { const filteredTypes: ClassType[] = []; let foundSuperclass = false; @@ -455,6 +463,10 @@ export class TypeConstraintBuilder { filteredTypes.push(varType); } + if (!isInstanceCheck) { + return filteredTypes; + } + return filteredTypes.map(t => ObjectType.create(t)); }; @@ -462,20 +474,24 @@ export class TypeConstraintBuilder { return combineTypes(types); }; - if (type.category === TypeCategory.Object) { - const filteredType = filterType(type.classType); + if (isInstanceCheck && effectiveType.category === TypeCategory.Object) { + const filteredType = filterType(effectiveType.classType); return finalizeFilteredTypeList(filteredType); - } else if (type.category === TypeCategory.Union) { + } else if (!isInstanceCheck && effectiveType.category === TypeCategory.Class) { + const filteredType = filterType(effectiveType); + return finalizeFilteredTypeList(filteredType); + } else if (effectiveType.category === TypeCategory.Union) { let remainingTypes: Type[] = []; - type.subtypes.forEach(t => { + effectiveType.subtypes.forEach(t => { if (isAnyOrUnknown(t)) { // Any types always remain for both positive and negative // checks because we can't say anything about them. remainingTypes.push(t); - } else if (t.category === TypeCategory.Object) { - remainingTypes = remainingTypes.concat( - filterType(t.classType)); + } else if (isInstanceCheck && t.category === TypeCategory.Object) { + remainingTypes = remainingTypes.concat(filterType(t.classType)); + } else if (!isInstanceCheck && t.category === TypeCategory.Class) { + remainingTypes = remainingTypes.concat(filterType(t)); } else { // All other types are never instances of a class. if (!isPositiveTest) { diff --git a/server/src/analyzer/typeUtils.ts b/server/src/analyzer/typeUtils.ts index f0dc553f7..274405312 100644 --- a/server/src/analyzer/typeUtils.ts +++ b/server/src/analyzer/typeUtils.ts @@ -561,6 +561,31 @@ export function canBeTruthy(type: Type): boolean { return true; } +// If the type is a concrete class X described by the object Type[X], +// returns X. Otherwise returns the original type. +export function transformTypeObjectToClass(type: Type): Type { + if (type.category !== TypeCategory.Object) { + return type; + } + + const classType = type.classType; + if (!ClassType.isBuiltIn(classType, 'Type')) { + return type; + } + + // If it's a generic Type, we can't get the class. + if (!classType.typeArguments || classType.typeArguments.length < 1) { + return type; + } + + const typeArg = classType.typeArguments[0]; + if (typeArg.category !== TypeCategory.Object) { + return type; + } + + return typeArg.classType;; +} + // None is always falsy. All other types are generally truthy // unless they are objects that support the __bool__ or __len__ // methods. diff --git a/server/src/common/configOptions.ts b/server/src/common/configOptions.ts index 2d6a3d937..1a3ae5529 100644 --- a/server/src/common/configOptions.ts +++ b/server/src/common/configOptions.ts @@ -139,7 +139,7 @@ export interface DiagnosticSettings { // initialization expression? reportCallInDefaultInitializer: DiagnosticLevel; - // Report calls to isinstance that are statically determined + // Report calls to isinstance or issubclass that are statically determined // to always be true or false. reportUnnecessaryIsInstance: DiagnosticLevel; diff --git a/server/src/tests/samples/typeConstraint2.py b/server/src/tests/samples/typeConstraint2.py index 6484a4e4e..4ec01afe7 100644 --- a/server/src/tests/samples/typeConstraint2.py +++ b/server/src/tests/samples/typeConstraint2.py @@ -1,9 +1,10 @@ # This sample exercises the type analyzer's isintance type constraint logic. from collections import defaultdict -from typing import DefaultDict, Optional, Union, Any +from typing import DefaultDict, Optional, Type, Union, Any class UnrelatedClass: + class_var1: int def __init__(self) -> None: self.property: None = None @@ -12,10 +13,12 @@ class UnrelatedSubclass(UnrelatedClass): self.property2: None = None class SuperClass: + class_var1: int def __init__(self) -> None: self.property: None = None class MyClass1(SuperClass): + class_var2: int def __init__(self) -> None: self.property2: None = None @@ -32,13 +35,29 @@ def f(instance: Union[SuperClass, UnrelatedClass]) -> None: # 'property2' is not a known member of 'UnrelatedClass' print(instance.property2) else: - print(instance.property) + print(instance.property) # This should generate two errors: # 'property2' is not a known member of 'SuperClass' # 'property2' is not a known member of 'UnrelatedClass' print(instance.property2) +def g(cls: Union[Type[SuperClass], Type[UnrelatedClass]]) -> None: + if issubclass(cls, (MyClass1, UnrelatedSubclass, Any)): + print(cls.class_var1) + + # This should generate two errors: + # 'property2' is not a known member of 'SuperClass' + # 'property2' is not a known member of 'UnrelatedClass' + print(cls.class_var2) + else: + print(cls.class_var1) + + # This should generate two errors: + # 'property2' is not a known member of 'SuperClass' + # 'property2' is not a known member of 'UnrelatedClass' + print(cls.class_var2) + # This code should analyze without any errors. class TestClass1: diff --git a/server/src/tests/samples/unnecessaryIsSubclass1.py b/server/src/tests/samples/unnecessaryIsSubclass1.py new file mode 100644 index 000000000..f84e7f695 --- /dev/null +++ b/server/src/tests/samples/unnecessaryIsSubclass1.py @@ -0,0 +1,24 @@ +# This sample tests unnecessary issubclass error reporting. + +from typing import Union, Type + +def foo(p1: Type[int], p2: Union[Type[int], Type[str]]): + a = issubclass(p2, str) + + b = issubclass(p2, (int, float)) + + # This should generate an error because this is always true. + c = issubclass(p2, (float, dict, int, str)) + + # This should generate an error because this is always false. + d = issubclass(p1, float) + + e = issubclass(p2, (float, dict, int)) + + # This should generate an error because this is always true. + f = issubclass(p1, int) + + # This should not generate an error because it's within an assert. + assert issubclass(p1, int) + + \ No newline at end of file diff --git a/server/src/tests/typeAnalyzer.test.ts b/server/src/tests/typeAnalyzer.test.ts index 57657397d..49834bfce 100644 --- a/server/src/tests/typeAnalyzer.test.ts +++ b/server/src/tests/typeAnalyzer.test.ts @@ -110,7 +110,7 @@ test('TypeConstraint1', () => { test('TypeConstraint2', () => { const analysisResults = TestUtils.typeAnalyzeSampleFiles(['typeConstraint2.py']); - validateResults(analysisResults, 4); + validateResults(analysisResults, 8); }); test('TypeConstraint3', () => { @@ -620,6 +620,18 @@ test('UnnecessaryIsInstance1', () => { validateResults(analysisResults, 3); }); +test('UnnecessaryIsSubclass1', () => { + const configOptions = new ConfigOptions('.'); + + let analysisResults = TestUtils.typeAnalyzeSampleFiles(['unnecessaryIsSubclass1.py'], configOptions); + validateResults(analysisResults, 0); + + // Turn on errors. + configOptions.diagnosticSettings.reportUnnecessaryIsInstance = 'error'; + analysisResults = TestUtils.typeAnalyzeSampleFiles(['unnecessaryIsSubclass1.py'], configOptions); + validateResults(analysisResults, 3); +}); + test('Unbound1', () => { const analysisResults = TestUtils.typeAnalyzeSampleFiles(['unbound1.py']);