Added diagnostic for TypeVar or generic class with type args being used as a second argument for isinstance or issubclass. These will raise a TypeError exception at runtime.

2024-09-11 16:06:39 +03:00 · 2020-07-11 10:58:34 -07:00 · 2020-07-11 10:58:34 -07:00 · f937bb5c2d
commit f937bb5c2d
parent 5fca2d35a7
8 changed files with 222 additions and 38 deletions
--- a/server/src/analyzer/checker.ts
+++ b/server/src/analyzer/checker.ts
@ -314,7 +314,7 @@ export class Checker extends ParseTreeWalker {
    }

    visitCall(node: CallNode): boolean {
-        this._validateIsInstanceCallNecessary(node);
+        this._validateIsInstanceCall(node);

        if (ParseTreeUtils.isWithinDefaultParamInitializer(node) && !this._fileInfo.isStubFile) {
            this._evaluator.addDiagnostic(
@ -432,7 +432,9 @@ export class Checker extends ParseTreeWalker {
        let adjYieldType = yieldType;
        const iteratorType = this._evaluator.getBuiltInType(node, 'Iterator');
        if (yieldType && iteratorType.category === TypeCategory.Class) {
-            adjYieldType = ObjectType.create(ClassType.cloneForSpecialization(iteratorType, [yieldType]));
+            adjYieldType = ObjectType.create(
+                ClassType.cloneForSpecialization(iteratorType, [yieldType], /* isTypeArgumentExplicit */ false)
+            );
        } else {
            adjYieldType = UnknownType.create();
        }
@ -1168,8 +1170,9 @@ export class Checker extends ParseTreeWalker {
    }

    // Validates that a call to isinstance or issubclass are necessary. This is a
-    // common source of programming errors.
-    private _validateIsInstanceCallNecessary(node: CallNode) {
+    // common source of programming errors. Also validates that arguments passed
+    // to isinstance or issubclass won't generate exceptions.
+    private _validateIsInstanceCall(node: CallNode) {
        if (
            node.leftExpression.nodeType !== ParseNodeType.Name ||
            (node.leftExpression.value !== 'isinstance' && node.leftExpression.value !== 'issubclass') ||
@ -1178,15 +1181,6 @@ export class Checker extends ParseTreeWalker {
            return;
        }

-        // If this call is within an assert statement, we'll ignore it.
-        let curNode: ParseNode | undefined = node;
-        while (curNode) {
-            if (curNode.nodeType === ParseNodeType.Assert) {
-                return;
-            }
-            curNode = curNode.parent;
-        }
-
        const callName = node.leftExpression.value;
        const isInstanceCheck = callName === 'isinstance';

@ -1207,6 +1201,75 @@ export class Checker extends ParseTreeWalker {
            return;
        }

+        // Create a helper function that determines whether the specified
+        // type is valid for the isinstance or issubclass call.
+        const isSupportedTypeForIsInstance = (type: Type) => {
+            let isSupported = true;
+
+            doForSubtypes(type, (subtype) => {
+                switch (subtype.category) {
+                    case TypeCategory.Any:
+                    case TypeCategory.Unknown:
+                    case TypeCategory.Unbound:
+                        break;
+
+                    case TypeCategory.Class:
+                        // If it's a class, make sure that it has not been given explicit
+                        // type arguments. This will result in a TypeError exception.
+                        if (subtype.isTypeArgumentExplicit) {
+                            isSupported = false;
+                        }
+                        break;
+
+                    default:
+                        isSupported = false;
+                        break;
+                }
+                return undefined;
+            });
+
+            return isSupported;
+        };
+
+        let isValidType = true;
+        if (
+            arg1Type.category === TypeCategory.Object &&
+            ClassType.isBuiltIn(arg1Type.classType, 'Tuple') &&
+            arg1Type.classType.typeArguments
+        ) {
+            isValidType = !arg1Type.classType.typeArguments.some((typeArg) => !isSupportedTypeForIsInstance(typeArg));
+        } else {
+            isValidType = isSupportedTypeForIsInstance(arg1Type);
+        }
+
+        if (!isValidType) {
+            const diag = new DiagnosticAddendum();
+            diag.addMessage(Localizer.DiagnosticAddendum.typeVarNotAllowed());
+
+            this._evaluator.addDiagnostic(
+                this._fileInfo.diagnosticRuleSet.reportGeneralTypeIssues,
+                DiagnosticRule.reportGeneralTypeIssues,
+                isInstanceCheck
+                    ? Localizer.Diagnostic.isInstanceInvalidType().format({
+                          type: this._evaluator.printType(arg1Type, /* expandTypeAlias */ false),
+                      }) + diag.getString()
+                    : Localizer.Diagnostic.isSubclassInvalidType().format({
+                          type: this._evaluator.printType(arg1Type, /* expandTypeAlias */ false),
+                      }) + diag.getString(),
+                node.arguments[1]
+            );
+        }
+
+        // If this call is within an assert statement, we won't check whether
+        // it's unnecessary.
+        let curNode: ParseNode | undefined = node;
+        while (curNode) {
+            if (curNode.nodeType === ParseNodeType.Assert) {
+                return;
+            }
+            curNode = curNode.parent;
+        }
+
        // Several built-in classes don't follow the normal class hierarchy
        // rules, so we'll avoid emitting false-positive diagnostics if these
        // are used.
--- a/server/src/analyzer/typeEvaluator.ts
+++ b/server/src/analyzer/typeEvaluator.ts
@ -1814,7 +1814,11 @@ export function createTypeEvaluator(importLookup: ImportLookup, printTypeFlags:
        let dictType = getBuiltInType(node, 'Dict');
        if (dictType.category === TypeCategory.Class) {
            dictType = ObjectType.create(
-                ClassType.cloneForSpecialization(dictType, [getBuiltInObject(node, 'str'), AnyType.create()])
+                ClassType.cloneForSpecialization(
+                    dictType,
+                    [getBuiltInObject(node, 'str'), AnyType.create()],
+                    /* isTypeArgumentExplicit */ false
+                )
            );
        }
        symbolTable.set('__dataclass_fields__', Symbol.createWithType(SymbolFlags.ClassMember, dictType));
@ -2402,7 +2406,9 @@ export function createTypeEvaluator(importLookup: ImportLookup, printTypeFlags:
            if (index === unpackIndex) {
                const listType = getBuiltInType(expr, 'List');
                if (listType.category === TypeCategory.Class) {
-                    targetType = ObjectType.create(ClassType.cloneForSpecialization(listType, [targetType]));
+                    targetType = ObjectType.create(
+                        ClassType.cloneForSpecialization(listType, [targetType], /* isTypeArgumentExplicit */ false)
+                    );
                }
            }

@ -3785,7 +3791,13 @@ export function createTypeEvaluator(importLookup: ImportLookup, printTypeFlags:
            // isn't used in most cases, but it is supported by the language.
            const builtInTupleType = getBuiltInType(node, 'Tuple');
            if (builtInTupleType.category === TypeCategory.Class) {
-                indexType = convertToInstance(ClassType.cloneForSpecialization(builtInTupleType, indexTypeList));
+                indexType = convertToInstance(
+                    ClassType.cloneForSpecialization(
+                        builtInTupleType,
+                        indexTypeList,
+                        /* isTypeArgumentExplicit */ false
+                    )
+                );
            } else {
                indexType = UnknownType.create();
            }
@ -3913,7 +3925,9 @@ export function createTypeEvaluator(importLookup: ImportLookup, printTypeFlags:
                }
            }

-            type = convertToInstance(ClassType.cloneForSpecialization(builtInTupleType, tupleTypes));
+            type = convertToInstance(
+                ClassType.cloneForSpecialization(builtInTupleType, tupleTypes, /* isTypeArgumentExplicit */ false)
+            );
        }

        return { type, node };
@ -4128,7 +4142,10 @@ export function createTypeEvaluator(importLookup: ImportLookup, printTypeFlags:
                    } else if (className === 'auto' && argList.length === 0) {
                        type = getBuiltInObject(errorNode, 'int');
                    }
-                } else if (ClassType.hasAbstractMethods(callType)) {
+                } else if (
+                    baseTypeResult.type.category === TypeCategory.Class &&
+                    ClassType.hasAbstractMethods(callType)
+                ) {
                    // If the class is abstract, it can't be instantiated.
                    const abstractMethods = getAbstractMethods(callType);

@ -6947,7 +6964,9 @@ export function createTypeEvaluator(importLookup: ImportLookup, printTypeFlags:
        const builtInIteratorType = getTypingType(node, isAsync ? 'AsyncGenerator' : 'Generator');

        if (builtInIteratorType && builtInIteratorType.category === TypeCategory.Class) {
-            type = ObjectType.create(ClassType.cloneForSpecialization(builtInIteratorType, [elementType]));
+            type = ObjectType.create(
+                ClassType.cloneForSpecialization(builtInIteratorType, [elementType], /* isTypeArgumentExplicit */ false)
+            );
        }

        return { type, node };
@ -7358,7 +7377,7 @@ export function createTypeEvaluator(importLookup: ImportLookup, printTypeFlags:
            typeArgTypes.push(AnyType.create(true));
        }

-        const specializedType = ClassType.cloneForSpecialization(classType, typeArgTypes);
+        const specializedType = ClassType.cloneForSpecialization(classType, typeArgTypes, typeArgs !== undefined);

        return specializedType;
    }
@ -8410,7 +8429,13 @@ export function createTypeEvaluator(importLookup: ImportLookup, printTypeFlags:
                // Create a Tuple[X, ...] type.
                const tupleType = getTypingType(node, 'Tuple');
                if (tupleType && tupleType.category === TypeCategory.Class) {
-                    return ObjectType.create(ClassType.cloneForSpecialization(tupleType, [type, AnyType.create(true)]));
+                    return ObjectType.create(
+                        ClassType.cloneForSpecialization(
+                            tupleType,
+                            [type, AnyType.create(true)],
+                            /* isTypeArgumentExplicit */ false
+                        )
+                    );
                }

                return UnknownType.create();
@ -8421,7 +8446,9 @@ export function createTypeEvaluator(importLookup: ImportLookup, printTypeFlags:
                const strType = getBuiltInObject(node, 'str');

                if (dictType.category === TypeCategory.Class && strType.category === TypeCategory.Object) {
-                    return ObjectType.create(ClassType.cloneForSpecialization(dictType, [strType, type]));
+                    return ObjectType.create(
+                        ClassType.cloneForSpecialization(dictType, [strType, type], /* isTypeArgumentExplicit */ false)
+                    );
                }

                return UnknownType.create();
@ -8792,7 +8819,11 @@ export function createTypeEvaluator(importLookup: ImportLookup, printTypeFlags:
                            typeArgs.push(generatorTypeArgs[1]);
                        }
                        awaitableReturnType = ObjectType.create(
-                            ClassType.cloneForSpecialization(asyncGeneratorType, typeArgs)
+                            ClassType.cloneForSpecialization(
+                                asyncGeneratorType,
+                                typeArgs,
+                                /* isTypeArgumentExplicit */ false
+                            )
                        );
                    }
                } else if (
@ -8808,7 +8839,9 @@ export function createTypeEvaluator(importLookup: ImportLookup, printTypeFlags:
        if (!awaitableReturnType) {
            const awaitableType = getTypingType(node, 'Awaitable');
            if (awaitableType && awaitableType.category === TypeCategory.Class) {
-                awaitableReturnType = ObjectType.create(ClassType.cloneForSpecialization(awaitableType, [returnType]));
+                awaitableReturnType = ObjectType.create(
+                    ClassType.cloneForSpecialization(awaitableType, [returnType], /* isTypeArgumentExplicit */ false)
+                );
            } else {
                awaitableReturnType = UnknownType.create();
            }
@ -8868,7 +8901,11 @@ export function createTypeEvaluator(importLookup: ImportLookup, printTypeFlags:
                    const generatorType = getTypingType(node, 'Generator');
                    if (generatorType && generatorType.category === TypeCategory.Class) {
                        inferredReturnType = ObjectType.create(
-                            ClassType.cloneForSpecialization(generatorType, [inferredReturnType])
+                            ClassType.cloneForSpecialization(
+                                generatorType,
+                                [inferredReturnType],
+                                /* isTypeArgumentExplicit */ false
+                            )
                        );
                    } else {
                        inferredReturnType = UnknownType.create();
@ -10918,7 +10955,7 @@ export function createTypeEvaluator(importLookup: ImportLookup, printTypeFlags:
            }
        });

-        const specializedClass = ClassType.cloneForSpecialization(classType, typeArgTypes);
+        const specializedClass = ClassType.cloneForSpecialization(classType, typeArgTypes, typeArgs !== undefined);

        return specializedClass;
    }
@ -10957,7 +10994,11 @@ export function createTypeEvaluator(importLookup: ImportLookup, printTypeFlags:
        if (nameType.category === TypeCategory.Class) {
            let classType = nameType;
            if (typeArguments) {
-                classType = ClassType.cloneForSpecialization(classType, typeArguments);
+                classType = ClassType.cloneForSpecialization(
+                    classType,
+                    typeArguments,
+                    /* isTypeArgumentExplicit */ false
+                );
            }

            return ObjectType.create(classType);
@ -11235,7 +11276,13 @@ export function createTypeEvaluator(importLookup: ImportLookup, printTypeFlags:
                    if (declaration.intrinsicType === 'Iterable[str]') {
                        const iterableType = getBuiltInType(declaration.node, 'Iterable');
                        if (iterableType.category === TypeCategory.Class) {
-                            return ObjectType.create(ClassType.cloneForSpecialization(iterableType, [strType]));
+                            return ObjectType.create(
+                                ClassType.cloneForSpecialization(
+                                    iterableType,
+                                    [strType],
+                                    /* isTypeArgumentExplicit */ false
+                                )
+                            );
                        }
                    }

@ -11243,7 +11290,11 @@ export function createTypeEvaluator(importLookup: ImportLookup, printTypeFlags:
                        const dictType = getBuiltInType(declaration.node, 'Dict');
                        if (dictType.category === TypeCategory.Class) {
                            return ObjectType.create(
-                                ClassType.cloneForSpecialization(dictType, [strType, AnyType.create()])
+                                ClassType.cloneForSpecialization(
+                                    dictType,
+                                    [strType, AnyType.create()],
+                                    /* isTypeArgumentExplicit */ false
+                                )
                            );
                        }
                    }
@ -11834,7 +11885,11 @@ export function createTypeEvaluator(importLookup: ImportLookup, printTypeFlags:
        }

        // Strip the type arguments off the dest protocol if they are provided.
-        const genericDestType = ClassType.cloneForSpecialization(destType, undefined);
+        const genericDestType = ClassType.cloneForSpecialization(
+            destType,
+            undefined,
+            /* isTypeArgumentExplicit */ false
+        );
        const genericDestTypeVarMap = new TypeVarMap();

        let typesAreConsistent = true;
--- a/server/src/analyzer/typeUtils.ts
+++ b/server/src/analyzer/typeUtils.ts
@ -1053,7 +1053,13 @@ export function getDeclaredGeneratorYieldType(functionType: FunctionType, iterat

        if (generatorTypeArgs && generatorTypeArgs.length >= 1 && iteratorType.category === TypeCategory.Class) {
            // The yield type is the first type arg. Wrap it in an iterator.
-            return ObjectType.create(ClassType.cloneForSpecialization(iteratorType, [generatorTypeArgs[0]]));
+            return ObjectType.create(
+                ClassType.cloneForSpecialization(
+                    iteratorType,
+                    [generatorTypeArgs[0]],
+                    /* isTypeArgumentExplicit */ false
+                )
+            );
        }

        // If the return type isn't a Generator, assume that it's the
@ -1338,7 +1344,7 @@ function _specializeClassType(
        return classType;
    }

-    return ClassType.cloneForSpecialization(classType, newTypeArgs);
+    return ClassType.cloneForSpecialization(classType, newTypeArgs, /* isTypeArgumentExplicit */ false);
 }

 // Converts a type var type into the most specific type
--- a/server/src/analyzer/types.ts
+++ b/server/src/analyzer/types.ts
@ -290,6 +290,10 @@ export interface ClassType extends TypeBase {
    // some or all of the type parameters.
    typeArguments?: Type[];

+    // If type arguments are present, were they explicit (i.e.
+    // provided explicitly in the code)?
+    isTypeArgumentExplicit?: boolean;
+
    skipAbstractClassTest: boolean;

    // Some types can be further constrained to have
@ -321,20 +325,27 @@ export namespace ClassType {
    export function cloneForSpecialization(
        classType: ClassType,
        typeArguments: Type[] | undefined,
+        isTypeArgumentExplicit: boolean,
        skipAbstractClassTest = false
    ): ClassType {
        const newClassType = create(classType.details.name, classType.details.flags, classType.details.typeSourceId);
+
        newClassType.details = classType.details;
        newClassType.typeArguments = typeArguments;
+        newClassType.isTypeArgumentExplicit = isTypeArgumentExplicit;
+
        if (classType.literalValue !== undefined) {
            newClassType.literalValue = classType.literalValue;
        }
+
        if (classType.typeAliasInfo !== undefined) {
            newClassType.typeAliasInfo = classType.typeAliasInfo;
        }
+
        if (skipAbstractClassTest) {
            newClassType.skipAbstractClassTest = true;
        }
+
        return newClassType;
    }

--- a/server/src/localization/localize.ts
+++ b/server/src/localization/localize.ts
@ -330,6 +330,10 @@ export namespace Localizer {
        export const invalidIdentifierChar = () => getRawString('Diagnostic.invalidIdentifierChar');
        export const invalidTokenChars = () =>
            new ParameterizedString<{ text: string }>(getRawString('Diagnostic.invalidTokenChars'));
+        export const isInstanceInvalidType = () =>
+            new ParameterizedString<{ type: string }>(getRawString('Diagnostic.isInstanceInvalidType'));
+        export const isSubclassInvalidType = () =>
+            new ParameterizedString<{ type: string }>(getRawString('Diagnostic.isSubclassInvalidType'));
        export const keyRequiredDeleted = () =>
            new ParameterizedString<{ name: string }>(getRawString('Diagnostic.keyRequiredDeleted'));
        export const keyValueInSet = () => getRawString('Diagnostic.keyValueInSet');
@ -747,6 +751,7 @@ export namespace Localizer {
            new ParameterizedString<{ name: string }>(getRawString('DiagnosticAddendum.typeVarIsCovariant'));
        export const typeVarIsInvariant = () =>
            new ParameterizedString<{ name: string }>(getRawString('DiagnosticAddendum.typeVarIsInvariant'));
+        export const typeVarNotAllowed = () => getRawString('DiagnosticAddendum.typeVarNotAllowed');
        export const useDictInstead = () => getRawString('DiagnosticAddendum.useDictInstead');
        export const useListInstead = () => getRawString('DiagnosticAddendum.useListInstead');
        export const useTupleInstead = () => getRawString('DiagnosticAddendum.useTupleInstead');
--- a/server/src/localization/package.nls.en-us.json
+++ b/server/src/localization/package.nls.en-us.json
@ -136,6 +136,8 @@
        "instanceMethodSelfParam": "Instance methods should take a \"self\" parameter",
        "invalidIdentifierChar": "Invalid character in identifier",
        "invalidTokenChars": "Invalid character in token \"{text}\"",
+        "isInstanceInvalidType": "Second argument to \"isinstance\" must be a class or tuple of classes",
+        "isSubclassInvalidType": "Second argument to \"issubclass\" must be a class or tuple of classes",
        "keyRequiredDeleted": "\"{name}\" is a required key and cannot be deleted",
        "keyValueInSet": "Key/value pairs are not allowed within a set",
        "lambdaReturnTypeUnknown": "Return type of lambda is unknown",
@ -356,6 +358,7 @@
        "typeVarIsContravariant": "TypeVar \"{name}\" is contravariant",
        "typeVarIsCovariant": "TypeVar \"{name}\" is covariant",
        "typeVarIsInvariant": "TypeVar \"{name}\" is invariant",
+        "typeVarNotAllowed": "TypeVar or generic type with type arguments is not allowed",
        "useDictInstead": "Use Dict[T1, T2] instead",
        "useListInstead": "Use List[T] instead",
        "useTupleInstead": "Use Tuple[T1, ..., Tn] instead",
--- a/server/src/tests/checker.test.ts
+++ b/server/src/tests/checker.test.ts
@ -1092,6 +1092,24 @@ test('NewType2', () => {
    validateResults(analysisResults, 1);
 });

+test('isInstance1', () => {
+    const analysisResults = TestUtils.typeAnalyzeSampleFiles(['isInstance1.py']);
+
+    validateResults(analysisResults, 0);
+});
+
+test('isInstance2', () => {
+    const analysisResults = TestUtils.typeAnalyzeSampleFiles(['isInstance2.py']);
+
+    validateResults(analysisResults, 1);
+});
+
+test('isInstance3', () => {
+    const analysisResults = TestUtils.typeAnalyzeSampleFiles(['isInstance3.py']);
+
+    validateResults(analysisResults, 2);
+});
+
 test('UnnecessaryIsInstance1', () => {
    const configOptions = new ConfigOptions('.');

@ -1104,12 +1122,6 @@ test('UnnecessaryIsInstance1', () => {
    validateResults(analysisResults, 4);
 });

-test('UnnecessaryIsInstance2', () => {
-    const analysisResults = TestUtils.typeAnalyzeSampleFiles(['unnecessaryIsInstance2.py']);
-
-    validateResults(analysisResults, 1);
-});
-
 test('UnnecessaryIsSubclass1', () => {
    const configOptions = new ConfigOptions('.');

--- a/server/src/tests/samples/isinstance3.py
+++ b/server/src/tests/samples/isinstance3.py
@ -0,0 +1,29 @@
+# This sample tests the logic that validates the second parameter to
+# an isinstance or issubclass call and ensures that it's a class or
+# tuple of classes.
+
+
+from typing import Generic, TypeVar, Union
+
+
+_T = TypeVar("_T")
+
+
+class A(Generic[_T]):
+    pass
+
+
+a = A()
+
+if isinstance(a, A):
+    pass
+
+# This should generate an error because generic types with
+# subscripts are not allowed.
+if isinstance(a, A[str]):
+    pass
+
+# This should generate an error because unions are not
+# allowed.
+if issubclass(a, Union[A, int]):
+    pass