From 4634309d2a3a5a62d29e71e9165048e4adf544d8 Mon Sep 17 00:00:00 2001 From: Eric Traut Date: Tue, 3 Nov 2020 15:46:32 -0800 Subject: [PATCH] Fixed inconsistency in type narrowing for `isinstance` and `isclass` calls. Previously, the narrowing logic used the target class(es) if the source expression was of type Any but did not do the same when the source expression was a union type that included Any but all other subtypes were eliminated. --- .../src/analyzer/typeEvaluator.ts | 66 ++++++++++--------- .../src/tests/samples/isinstance1.py | 27 ++++++++ .../src/tests/typeEvaluator2.test.ts | 6 ++ 3 files changed, 69 insertions(+), 30 deletions(-) create mode 100644 packages/pyright-internal/src/tests/samples/isinstance1.py diff --git a/packages/pyright-internal/src/analyzer/typeEvaluator.ts b/packages/pyright-internal/src/analyzer/typeEvaluator.ts index 608d95dd6..9a2464267 100644 --- a/packages/pyright-internal/src/analyzer/typeEvaluator.ts +++ b/packages/pyright-internal/src/analyzer/typeEvaluator.ts @@ -12996,42 +12996,48 @@ export function createTypeEvaluator(importLookup: ImportLookup, evaluatorOptions return filteredTypes.map((t) => ObjectType.create(t)); }; - if (isInstanceCheck && isObject(effectiveType)) { - const filteredType = filterType(effectiveType.classType); - return combineTypes(filteredType); - } else if (!isInstanceCheck && isClass(effectiveType)) { - const filteredType = filterType(effectiveType); - return combineTypes(filteredType); - } else if (effectiveType.category === TypeCategory.Union) { - let remainingTypes: Type[] = []; + const anyOrUnknownSubstitutions: Type[] = []; + const anyOrUnknown: Type[] = []; - 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 (isInstanceCheck && isObject(t)) { - remainingTypes = remainingTypes.concat(filterType(t.classType)); - } else if (!isInstanceCheck && isClass(t)) { - remainingTypes = remainingTypes.concat(filterType(t)); + const filteredType = doForSubtypes(effectiveType, (subtype) => { + if (isInstanceCheck && isObject(subtype)) { + return combineTypes(filterType(subtype.classType)); + } else if (!isInstanceCheck && isClass(subtype)) { + return combineTypes(filterType(subtype)); + } else if (isPositiveTest && isAnyOrUnknown(subtype)) { + // If this is a positive test and the effective type is Any or + // Unknown, we can assume that the type matches one of the + // specified types. + if (isInstanceCheck) { + anyOrUnknownSubstitutions.push( + combineTypes(classTypeList.map((classType) => ObjectType.create(classType))) + ); } else { - // All other types are never instances of a class. - if (!isPositiveTest) { - remainingTypes.push(t); - } + anyOrUnknownSubstitutions.push(combineTypes(classTypeList)); } - }); - return combineTypes(remainingTypes); - } else if (isInstanceCheck && isPositiveTest && isAnyOrUnknown(effectiveType)) { - // If this is a positive test for isinstance and the effective - // type is Any or Unknown, we can assume that the type matches - // one of the specified types. - type = combineTypes(classTypeList.map((classType) => ObjectType.create(classType))); + anyOrUnknown.push(subtype); + return undefined; + } + + return isPositiveTest ? undefined : subtype; + }); + + // If the result is Any/Unknown and contains no other subtypes and + // we have substitutions for Any/Unknown, use those instead. We don't + // want to apply this if the filtering produced something other than + // Any/Unknown. For example, if the statement is "isinstance(x, list)" + // and the type of x is "List[str] | int | Any", the result should be + // "List[str]", not "List[str] | List[Unknown]". + if (isNever(filteredType) && anyOrUnknownSubstitutions.length > 0) { + return combineTypes(anyOrUnknownSubstitutions); } - // Return the original type. - return type; + if (anyOrUnknown.length > 0) { + return combineTypes([filteredType, ...anyOrUnknown]); + } + + return filteredType; } // Attempts to narrow a type (make it more constrained) based on an "in" or diff --git a/packages/pyright-internal/src/tests/samples/isinstance1.py b/packages/pyright-internal/src/tests/samples/isinstance1.py new file mode 100644 index 000000000..4ce2f1231 --- /dev/null +++ b/packages/pyright-internal/src/tests/samples/isinstance1.py @@ -0,0 +1,27 @@ +# This sample tests basic type narrowing behavior for +# the isinstance call. + +from typing import Any, List, Literal, Union + + +def func1(x: Union[List[str], int]): + if isinstance(x, list): + t1: Literal["List[str]"] = reveal_type(x) + else: + t2: Literal["int"] = reveal_type(x) + + +def func2(x: Any): + if isinstance(x, list): + t1: Literal["list[Unknown]"] = reveal_type(x) + else: + t2: Literal["Any"] = reveal_type(x) + + +def func3(x): + if isinstance(x, list): + t1: Literal["list[Unknown]"] = reveal_type(x) + else: + t2: Literal["Unknown"] = reveal_type(x) + + diff --git a/packages/pyright-internal/src/tests/typeEvaluator2.test.ts b/packages/pyright-internal/src/tests/typeEvaluator2.test.ts index 8d966b546..706902aa2 100644 --- a/packages/pyright-internal/src/tests/typeEvaluator2.test.ts +++ b/packages/pyright-internal/src/tests/typeEvaluator2.test.ts @@ -102,6 +102,12 @@ test('NewType3', () => { TestUtils.validateResults(analysisResults, 4); }); +test('isInstance1', () => { + const analysisResults = TestUtils.typeAnalyzeSampleFiles(['isinstance1.py']); + + TestUtils.validateResults(analysisResults, 0); +}); + test('isInstance2', () => { const analysisResults = TestUtils.typeAnalyzeSampleFiles(['isinstance2.py']);