Expanded isinstance support (for both the reportUnnecessaryIsInstance switch and type constraints) to also support issubclass.

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": {

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'.
 

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;
             }
         }

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);
         }
     }

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) {

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.

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;
 

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:

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)
+
+    

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']);