Implemented new "reportUnnecessaryIsInstance" check, which reports cases where isinstance is used where it's possible to determine that the result is always true or false.

client/schemas/pyrightconfig.schema.json

@@ -240,6 +240,12 @@
       "title": "Controls reporting usage of function calls within a default value initializer expression",
       "default": "none"
     },
+    "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",
+      "default": "none"
+    },
     "pythonVersion": {
       "$id": "#/properties/pythonVersion",
       "type": "string",

docs/configuration.md

@@ -88,6 +88,8 @@ 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'.
+
 
 ## Execution Environment Options
 Pyright allows multiple “execution environments” to be defined for different portions of your source tree. For example, a subtree may be designed to run with different import search paths or a different version of the python interpreter than the rest of the source base.

server/src/analyzer/typeAnalyzer.ts

@@ -43,9 +43,9 @@ import { SymbolUtils } from './symbolUtils';
 import { TypeConstraintBuilder } from './typeConstraint';
 import { TypeConstraintUtils } from './typeConstraintUtils';
 import { AnyType, ClassType, ClassTypeFlags, FunctionParameter, FunctionType,
-    FunctionTypeFlags, ModuleType, NoneType, ObjectType,
-    OverloadedFunctionType, PropertyType, Type, TypeVarType,
-    UnboundType, UnionType, UnknownType } from './types';
+    FunctionTypeFlags, ModuleType, NeverType, NoneType,
+    ObjectType, OverloadedFunctionType, PropertyType, Type,
+    TypeVarType, UnboundType, UnionType, UnknownType } from './types';
 import { ClassMemberLookupFlags, TypeUtils } from './typeUtils';
 
 interface AliasMapEntry {
@@ -575,6 +575,8 @@ export class TypeAnalyzer extends ParseTreeWalker {
             this._currentScope.setAlwaysRaises();
         }
 
+        this._validateIsInstanceCallNecessary(node);
+
         if (this._defaultValueInitializerExpression && !this._fileInfo.isStubFile) {
             this._addDiagnostic(
                 this._fileInfo.diagnosticSettings.reportCallInDefaultInitializer,
@@ -1508,6 +1510,121 @@ export class TypeAnalyzer extends ParseTreeWalker {
         return false;
     }
 
+    // Validates that a call to isinstance is necessary. This is a
+    // common source of programming errors.
+    private _validateIsInstanceCallNecessary(node: CallExpressionNode) {
+        if (this._fileInfo.diagnosticSettings.reportUnnecessaryIsInstance === 'none') {
+            return;
+        }
+
+        if (!(node.leftExpression instanceof NameNode) ||
+                node.leftExpression.nameToken.value !== 'isinstance' ||
+                node.arguments.length !== 2) {
+            return;
+        }
+
+        const arg0Type = this._getTypeOfExpression(node.arguments[0].valueExpression);
+        if (arg0Type.isAny()) {
+            return;
+        }
+
+        const arg1Type = this._getTypeOfExpression(node.arguments[1].valueExpression);
+
+        const classTypeList: ClassType[] = [];
+        if (arg1Type instanceof ClassType) {
+            classTypeList.push(arg1Type);
+        } else if (arg1Type instanceof ObjectType) {
+            // The isinstance call supports a variation where the second
+            // parameter is a tuple of classes.
+            const objClass = arg1Type.getClassType();
+            if (objClass.isBuiltIn() && objClass.getClassName() === 'Tuple' && objClass.getTypeArguments()) {
+                objClass.getTypeArguments()!.forEach(typeArg => {
+                    if (typeArg instanceof ClassType) {
+                        classTypeList.push(typeArg);
+                    } else {
+                        return;
+                    }
+                });
+            }
+        } else {
+            return;
+        }
+
+        const finalizeFilteredTypeList = (types: Type[]): Type => {
+            return TypeUtils.combineTypes(types);
+        };
+
+        const filterType = (varType: ClassType): ObjectType[] => {
+            let filteredTypes: ClassType[] = [];
+
+            for (let filterType of classTypeList) {
+                const filterIsSuperclass = varType.isDerivedFrom(filterType);
+                const filterIsSubclass = filterType.isDerivedFrom(varType);
+
+                if (filterIsSuperclass) {
+                    // If the variable type is a subclass of the isinstance
+                    // filter, we haven't learned anything new about the
+                    // variable type.
+                    filteredTypes.push(varType);
+                } else if (filterIsSubclass) {
+                    // If the variable type is a superclass of the isinstance
+                    // filter, we can narrow the type to the subclass.
+                    filteredTypes.push(filterType);
+                }
+            }
+
+            return filteredTypes.map(t => new ObjectType(t));
+        };
+
+        let filteredType: Type;
+        if (arg0Type instanceof ObjectType) {
+            let remainingTypes = filterType(arg0Type.getClassType());
+            filteredType = finalizeFilteredTypeList(remainingTypes);
+        } else if (arg0Type instanceof UnionType) {
+            let remainingTypes: Type[] = [];
+            let foundAnyType = false;
+
+            arg0Type.getTypes().forEach(t => {
+                if (t.isAny()) {
+                    foundAnyType = true;
+                }
+
+                if (t instanceof ObjectType) {
+                    remainingTypes = remainingTypes.concat(
+                        filterType(t.getClassType()));
+                }
+            });
+
+            filteredType = finalizeFilteredTypeList(remainingTypes);
+
+            // If we found an any or unknown type, all bets are off.
+            if (foundAnyType) {
+                return;
+            }
+        } else {
+            return;
+        }
+
+        const getTestType = () => {
+            const objTypeList = classTypeList.map(t => new ObjectType(t));
+            return TypeUtils.combineTypes(objTypeList);
+        };
+
+        if (filteredType instanceof NeverType) {
+            this._addDiagnostic(
+                this._fileInfo.diagnosticSettings.reportUnnecessaryIsInstance,
+                `Unnecessary isinstance call: '${ arg0Type.asString() }' ` +
+                    `is never instance of '${ getTestType().asString() }'`,
+                node);
+        } else if (filteredType.isSame(arg0Type)) {
+            this._addDiagnostic(
+                this._fileInfo.diagnosticSettings.reportUnnecessaryIsInstance,
+                `Unnecessary isinstance call: '${ arg0Type.asString() }' ` +
+                    `is always instance of '${ getTestType().asString() }'`,
+                node);
+        }
+    }
+
     // Handles some special-case assignment statements that are found
     // within the typings.pyi file.
     private _handleTypingStubAssignment(node: AssignmentNode): boolean {

server/src/common/configOptions.ts

@@ -131,6 +131,10 @@ export interface DiagnosticSettings {
     // Report usage of function call within default value
     // initialization expression?
     reportCallInDefaultInitializer: DiagnosticLevel;
+
+    // Report calls to isinstance that are statically determined
+    // to always be true or false.
+    reportUnnecessaryIsInstance: DiagnosticLevel;
 }
 
 export function cloneDiagnosticSettings(
@@ -175,7 +179,8 @@ export function getDiagLevelSettings() {
         'reportUnknownParameterType',
         'reportUnknownVariableType',
         'reportUnknownMemberType',
-        'reportCallInDefaultInitializer'
+        'reportCallInDefaultInitializer',
+        'reportUnnecessaryIsInstance'
     ];
 }
 
@@ -209,7 +214,8 @@ export function getStrictDiagnosticSettings(): DiagnosticSettings {
         reportUnknownParameterType: 'error',
         reportUnknownVariableType: 'error',
         reportUnknownMemberType: 'error',
-        reportCallInDefaultInitializer: 'none'
+        reportCallInDefaultInitializer: 'none',
+        reportUnnecessaryIsInstance: 'error'
     };
 
     return diagSettings;
@@ -245,7 +251,8 @@ export function getDefaultDiagnosticSettings(): DiagnosticSettings {
         reportUnknownParameterType: 'none',
         reportUnknownVariableType: 'none',
         reportUnknownMemberType: 'none',
-        reportCallInDefaultInitializer: 'none'
+        reportCallInDefaultInitializer: 'none',
+        reportUnnecessaryIsInstance: 'none'
     };
 
     return diagSettings;
@@ -569,7 +576,12 @@ export class ConfigOptions {
             // Read the "reportCallInDefaultInitializer" entry.
             reportCallInDefaultInitializer: this._convertDiagnosticLevel(
                 configObj.reportCallInDefaultInitializer, 'reportCallInDefaultInitializer',
-                defaultSettings.reportCallInDefaultInitializer)
+                defaultSettings.reportCallInDefaultInitializer),
+
+            // Read the "reportUnnecessaryIsInstance" entry.
+            reportUnnecessaryIsInstance: this._convertDiagnosticLevel(
+                configObj.reportUnnecessaryIsInstance, 'reportUnnecessaryIsInstance',
+                defaultSettings.reportUnnecessaryIsInstance)
         };
 
         // Read the "venvPath".

server/src/tests/samples/typeConstraint5.py

@@ -0,0 +1,14 @@
+# This sample tests the type constraint logic for "continue"
+# statements within a loop.
+
+from typing import List, Optional
+
+def foo(args: List[Optional[int]]):
+    for arg in args:
+        if arg is None:
+            continue
+
+        # This should not generate an error because
+        # arg is known to be an int at this point.
+        print(arg.bit_length())
+    

server/src/tests/samples/unnecessaryIsInstance1.py

@@ -0,0 +1,21 @@
+# This sample tests unnecessary isinstance error reporting.
+
+from typing import Union
+
+def foo(p1: int, p2: Union[int, str]):
+    a = isinstance(p2, str)
+
+    b = isinstance(p2, (int, float))
+
+    # This should generate an error because this is always true.
+    c = isinstance(p2, (float, dict, int, str))
+
+    # This should generate an error because this is always false.
+    d = isinstance(p1, float)
+
+    e = isinstance(p2, (float, dict, int))
+
+    # This should generate an error because this is always true.
+    f = isinstance(p1, int)
+
+    

server/src/tests/typeAnalyzer.test.ts

@@ -115,6 +115,18 @@ test('TypeConstraint3', () => {
     validateResults(analysisResults, 1);
 });
 
+test('TypeConstraint4', () => {
+    let analysisResults = TestUtils.typeAnalyzeSampleFiles(['typeConstraint4.py']);
+
+    validateResults(analysisResults, 2);
+});
+
+test('TypeConstraint5', () => {
+    let analysisResults = TestUtils.typeAnalyzeSampleFiles(['typeConstraint5.py']);
+
+    validateResults(analysisResults, 0);
+});
+
 test('CircularBaseClass', () => {
     let analysisResults = TestUtils.typeAnalyzeSampleFiles(['circularBaseClass.py']);
 
@@ -487,3 +499,15 @@ test('NewType1', () => {
 
     validateResults(analysisResults, 1);
 });
+
+test('UnnecessaryIsInstance1', () => {
+    const configOptions = new ConfigOptions('.');
+
+    let analysisResults = TestUtils.typeAnalyzeSampleFiles(['unnecessaryIsInstance1.py'], configOptions);
+    validateResults(analysisResults, 0);
+
+    // Turn on errors.
+    configOptions.diagnosticSettings.reportUnnecessaryIsInstance = 'error';
+    analysisResults = TestUtils.typeAnalyzeSampleFiles(['unnecessaryIsInstance1.py'], configOptions);
+    validateResults(analysisResults, 3);
+});