Fixed a bug that results in incorrect type narrowing when using `isin…

…stance` with an instance of a generic class as the first argument and a concrete subclass as the filter type. This addresses #8672. (#8675)
microsoft · Aug 6, 2024 · c1df659 · c1df659
1 parent 35ab773
commit c1df659
Show file tree

Hide file tree

Showing 3 changed files with 52 additions and 12 deletions.
diff --git a/packages/pyright-internal/src/analyzer/typeGuards.ts b/packages/pyright-internal/src/analyzer/typeGuards.ts
@@ -89,6 +89,7 @@ import {
     isUnboundedTupleClass,
     lookUpClassMember,
     lookUpObjectMember,
+    makeTypeVarsFree,
     mapSubtypes,
     MemberAccessFlags,
     specializeTupleClass,
@@ -1355,6 +1356,17 @@ function narrowTypeForIsInstanceInternal(
 
     expandedTypes = evaluator.expandPromotionTypes(errorNode, expandedTypes);
 
+    const convertVarTypeToFree = (varType: Type): Type => {
+        // If this is a TypeIs check, type variables should remain bound.
+        if (isTypeIsCheck) {
+            return varType;
+        }
+
+        // If this is an isinstance or issubclass check, the type variables
+        // should be converted to "free" type variables.
+        return makeTypeVarsFree(varType, ParseTreeUtils.getTypeVarScopesForNode(errorNode));
+    };
+
     // Filters the varType by the parameters of the isinstance
     // and returns the list of types the varType could be after
     // applying the filter.
@@ -1443,7 +1455,7 @@ function narrowTypeForIsInstanceInternal(
                     } else if (filterIsSubclass) {
                         if (
                             evaluator.assignType(
-                                convertToInstance(concreteVarType),
+                                convertToInstance(convertVarTypeToFree(concreteVarType)),
                                 convertToInstance(concreteFilterType),
                                 /* diag */ undefined,
                                 /* destConstraints */ undefined,
@@ -1597,7 +1609,7 @@ function narrowTypeForIsInstanceInternal(
                         }
                     } else if (
                         evaluator.assignType(
-                            concreteVarType,
+                            convertVarTypeToFree(concreteVarType),
                             filterType,
                             /* diag */ undefined,
                             /* destConstraints */ undefined,
@@ -1653,7 +1665,7 @@ function narrowTypeForIsInstanceInternal(
 
                 if (filterMetaclass && isInstantiableClass(filterMetaclass)) {
                     let isMetaclassOverlap = evaluator.assignType(
-                        metaclassType,
+                        convertVarTypeToFree(metaclassType),
                         ClassType.cloneAsInstance(filterMetaclass)
                     );
 
@@ -1711,7 +1723,7 @@ function narrowTypeForIsInstanceInternal(
             for (const filterType of filterTypes) {
                 const concreteFilterType = evaluator.makeTopLevelTypeVarsConcrete(filterType);
 
-                if (evaluator.assignType(varType, convertToInstance(concreteFilterType))) {
+                if (evaluator.assignType(convertVarTypeToFree(varType), convertToInstance(concreteFilterType))) {
                     // If the filter type is a Callable, use the original type. If the
                     // filter type is a callback protocol, use the filter type.
                     if (isFunction(filterType)) {
@@ -1730,7 +1742,7 @@ function narrowTypeForIsInstanceInternal(
                     return false;
                 }
 
-                return evaluator.assignType(varType, convertToInstance(concreteFilterType));
+                return evaluator.assignType(convertVarTypeToFree(varType), convertToInstance(concreteFilterType));
             })
         ) {
             filteredTypes.push(unexpandedType);

diff --git a/packages/pyright-internal/src/tests/samples/typeNarrowingIsinstance1.py b/packages/pyright-internal/src/tests/samples/typeNarrowingIsinstance1.py
@@ -1,7 +1,17 @@
 # This sample exercises the type analyzer's isinstance type narrowing logic.
 
 from types import NoneType
-from typing import Any, Generic, Iterable, Iterator, Protocol, Sized, TypeVar, Union, runtime_checkable
+from typing import (
+    Any,
+    Generic,
+    Iterable,
+    Iterator,
+    Protocol,
+    Sized,
+    TypeVar,
+    Union,
+    runtime_checkable,
+)
 
 S = TypeVar("S")
 T = TypeVar("T")
@@ -233,3 +243,21 @@ def func13(x: object | type[object]) -> None:
 def func14(x: Iterable[T]):
     if isinstance(x, Iterator):
         reveal_type(x, expected_text="Iterator[T@func14]")
+
+
+class Base15(Generic[T]):
+    value: T
+
+
+class Child15(Base15[int]):
+    value: int
+
+
+def func15(x: Base15[T]):
+    if isinstance(x, Child15):
+        # This should generate an error. It's here just to ensure that
+        # this code branch isn't marked unreachable.
+        reveal_type(x, expected_text="Never")
+
+        reveal_type(x, expected_text="Child15")
+        reveal_type(x.value, expected_text="int")
diff --git a/packages/pyright-internal/src/tests/typeEvaluator1.test.ts b/packages/pyright-internal/src/tests/typeEvaluator1.test.ts
@@ -288,12 +288,6 @@ test('TypeNarrowing7', () => {
     TestUtils.validateResults(analysisResults, 0);
 });
 
-test('TypeNarrowingIsinstance1', () => {
-    const analysisResults = TestUtils.typeAnalyzeSampleFiles(['typeNarrowingIsinstance1.py']);
-
-    TestUtils.validateResults(analysisResults, 8);
-});
-
 test('TypeNarrowingAssert1', () => {
     const analysisResults = TestUtils.typeAnalyzeSampleFiles(['typeNarrowingAssert1.py']);
 
@@ -372,6 +366,12 @@ test('TypeNarrowingEnum2', () => {
     TestUtils.validateResults(analysisResults, 2);
 });
 
+test('TypeNarrowingIsinstance1', () => {
+    const analysisResults = TestUtils.typeAnalyzeSampleFiles(['typeNarrowingIsinstance1.py']);
+
+    TestUtils.validateResults(analysisResults, 9);
+});
+
 test('TypeNarrowingIsinstance2', () => {
     const analysisResults = TestUtils.typeAnalyzeSampleFiles(['typeNarrowingIsinstance2.py']);