Skip to content

Commit

Permalink
Improved type narrowing in the negative case for isinstance when th…
Browse files Browse the repository at this point in the history 
…e filter type (the second argument) is `type[T]` (where `T` is a type variable) and the first argument is of type `T`. In this case, we can eliminate (filter) `T` in the negative case. This addresses #6088. (#6091)

Co-authored-by: Eric Traut <erictr@microsoft.com>
  • Loading branch information
@erictraut @msfterictraut
erictraut and msfterictraut authored Oct 6, 2023
1 parent 66a0576 commit eda2b65
Show file tree
Hide file tree
Showing 4 changed files with 96 additions and 48 deletions.
1 change: 1 addition & 0 deletions packages/pyright-internal/src/analyzer/checker.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3660,6 +3660,7 @@ export class Checker extends ParseTreeWalker {
const filterIsSuperclass = isIsinstanceFilterSuperclass(
this._evaluator,
varType,
varType,
filterType,
filterType,
isInstanceCheck
Expand Down
99 changes: 55 additions & 44 deletions packages/pyright-internal/src/analyzer/typeGuards.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1234,36 +1234,40 @@ function getIsInstanceClassTypes(argType: Type): (ClassType | TypeVarType | None

export function isIsinstanceFilterSuperclass(
evaluator: TypeEvaluator,
varType: ClassType,
varType: Type,
concreteVarType: ClassType,
filterType: Type,
concreteFilterType: ClassType,
isInstanceCheck: boolean
) {
if (isTypeVar(filterType)) {
return isTypeSame(convertToInstance(filterType), varType);
}

// If the filter type represents all possible subclasses
// of a type, we can't make any statements about its superclass
// relationship with varType.
// relationship with concreteVarType.
if (concreteFilterType.includeSubclasses) {
return false;
}

if (isTypeVar(filterType)) {
return false;
}

if (ClassType.isDerivedFrom(varType, concreteFilterType)) {
if (ClassType.isDerivedFrom(concreteVarType, concreteFilterType)) {
return true;
}

if (isInstanceCheck) {
if (ClassType.isProtocolClass(concreteFilterType) && evaluator.assignType(concreteFilterType, varType)) {
if (
ClassType.isProtocolClass(concreteFilterType) &&
evaluator.assignType(concreteFilterType, concreteVarType)
) {
return true;
}
}

// Handle the special case where the variable type is a TypedDict and
// we're filtering against 'dict'. TypedDict isn't derived from dict,
// but at runtime, isinstance returns True.
if (ClassType.isBuiltIn(concreteFilterType, 'dict') && ClassType.isTypedDictClass(varType)) {
if (ClassType.isBuiltIn(concreteFilterType, 'dict') && ClassType.isTypedDictClass(concreteVarType)) {
return true;
}

Expand Down Expand Up @@ -1313,9 +1317,9 @@ function narrowTypeForIsInstance(
// and returns the list of types the varType could be after
// applying the filter.
const filterClassType = (
varType: ClassType,
unexpandedType: Type,
constraints: TypeCondition[] | undefined,
varType: Type,
concreteVarType: ClassType,
conditions: TypeCondition[] | undefined,
negativeFallbackType: Type
): Type[] => {
const filteredTypes: Type[] = [];
Expand All @@ -1330,13 +1334,14 @@ function narrowTypeForIsInstance(
const filterIsSuperclass = isIsinstanceFilterSuperclass(
evaluator,
varType,
concreteVarType,
filterType,
concreteFilterType,
isInstanceCheck
);
const filterIsSubclass = isIsinstanceFilterSubclass(
evaluator,
varType,
concreteVarType,
concreteFilterType,
isInstanceCheck
);
Expand All @@ -1353,7 +1358,7 @@ function narrowTypeForIsInstance(
if (
filterIsSubclass &&
filterIsSuperclass &&
!ClassType.isSameGenericClass(varType, concreteFilterType)
!ClassType.isSameGenericClass(concreteVarType, concreteFilterType)
) {
isClassRelationshipIndeterminate = true;
}
Expand All @@ -1362,11 +1367,11 @@ function narrowTypeForIsInstance(
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(addConditionToType(varType, constraints));
filteredTypes.push(addConditionToType(concreteVarType, conditions));
} else if (filterIsSubclass) {
if (
evaluator.assignType(
varType,
concreteVarType,
filterType,
/* diag */ undefined,
/* destTypeVarContext */ undefined,
Expand Down Expand Up @@ -1397,7 +1402,7 @@ function narrowTypeForIsInstance(
populateTypeVarContextBasedOnExpectedType(
evaluator,
unspecializedFilterType,
varType,
concreteVarType,
typeVarContext,
/* liveTypeVarScopes */ undefined,
errorNode.start
Expand All @@ -1412,23 +1417,23 @@ function narrowTypeForIsInstance(
}
}

filteredTypes.push(addConditionToType(specializedFilterType, constraints));
filteredTypes.push(addConditionToType(specializedFilterType, conditions));
}
} else if (
allowIntersections &&
!ClassType.isFinal(varType) &&
!ClassType.isFinal(concreteVarType) &&
!ClassType.isFinal(concreteFilterType)
) {
// The two types appear to have no relation. It's possible that the
// two types are protocols or the program is expecting one type to
// be a mix-in class used with the other. In this case, we'll
// synthesize a new class type that represents an intersection of
// the two types.
const className = `<subclass of ${varType.details.name} and ${concreteFilterType.details.name}>`;
const className = `<subclass of ${concreteVarType.details.name} and ${concreteFilterType.details.name}>`;
const fileInfo = getFileInfo(errorNode);

// The effective metaclass of the intersection is the narrower of the two metaclasses.
let effectiveMetaclass = varType.details.effectiveMetaclass;
let effectiveMetaclass = concreteVarType.details.effectiveMetaclass;
if (concreteFilterType.details.effectiveMetaclass) {
if (
!effectiveMetaclass ||
Expand All @@ -1447,21 +1452,24 @@ function narrowTypeForIsInstance(
ParseTreeUtils.getTypeSourceId(errorNode),
/* declaredMetaclass */ undefined,
effectiveMetaclass,
varType.details.docString
concreteVarType.details.docString
);
newClassType.details.baseClasses = [ClassType.cloneAsInstantiable(varType), concreteFilterType];
newClassType.details.baseClasses = [
ClassType.cloneAsInstantiable(concreteVarType),
concreteFilterType,
];
computeMroLinearization(newClassType);

newClassType = addConditionToType(newClassType, concreteFilterType.condition) as ClassType;

if (
isTypeVar(unexpandedType) &&
!unexpandedType.details.isParamSpec &&
unexpandedType.details.constraints.length === 0
isTypeVar(varType) &&
!varType.details.isParamSpec &&
varType.details.constraints.length === 0
) {
newClassType = addConditionToType(newClassType, [
{
typeVarName: TypeVarType.getNameWithScope(unexpandedType),
typeVarName: TypeVarType.getNameWithScope(varType),
constraintIndex: 0,
isConstrainedTypeVar: false,
},
Expand All @@ -1470,10 +1478,10 @@ function narrowTypeForIsInstance(

let newClassInstanceType = ClassType.cloneAsInstance(newClassType);

if (varType.condition) {
if (concreteVarType.condition) {
newClassInstanceType = addConditionToType(
newClassInstanceType,
varType.condition
concreteVarType.condition
) as ClassType;
}

Expand All @@ -1488,33 +1496,35 @@ function narrowTypeForIsInstance(
} else if (isTypeVar(filterType) && TypeBase.isInstantiable(filterType)) {
// Handle the case where the filter type is Type[T] and the unexpanded
// subtype is some instance type, possibly T.
if (isInstanceCheck && TypeBase.isInstance(unexpandedType)) {
if (isTypeVar(unexpandedType) && isTypeSame(convertToInstance(filterType), unexpandedType)) {
if (isInstanceCheck && TypeBase.isInstance(varType)) {
if (isTypeVar(varType) && isTypeSame(convertToInstance(filterType), varType)) {
// If the unexpanded subtype is T, we can definitively filter
// in both the positive and negative cases.
if (isPositiveTest) {
filteredTypes.push(unexpandedType);
filteredTypes.push(varType);
} else {
foundSuperclass = true;
}
} else {
if (isPositiveTest) {
filteredTypes.push(convertToInstance(filterType));
} else {
// If the unexpanded subtype is some other instance, we can't
// filter anything because it might be an instance.
filteredTypes.push(unexpandedType);
filteredTypes.push(varType);
isClassRelationshipIndeterminate = true;
}
}
} else if (!isInstanceCheck && TypeBase.isInstantiable(unexpandedType)) {
if (isTypeVar(unexpandedType) && isTypeSame(filterType, unexpandedType)) {
} else if (!isInstanceCheck && TypeBase.isInstantiable(varType)) {
if (isTypeVar(varType) && isTypeSame(filterType, varType)) {
if (isPositiveTest) {
filteredTypes.push(unexpandedType);
filteredTypes.push(varType);
}
} else {
if (isPositiveTest) {
filteredTypes.push(filterType);
} else {
filteredTypes.push(unexpandedType);
filteredTypes.push(varType);
isClassRelationshipIndeterminate = true;
}
}
Expand All @@ -1524,12 +1534,12 @@ function narrowTypeForIsInstance(
if (isInstanceCheck) {
let isCallable = false;

if (isClass(varType)) {
if (TypeBase.isInstantiable(unexpandedType)) {
if (isClass(concreteVarType)) {
if (TypeBase.isInstantiable(varType)) {
isCallable = true;
} else {
isCallable = !!lookUpClassMember(
varType,
concreteVarType,
'__call__',
ClassMemberLookupFlags.SkipInstanceVariables
);
Expand All @@ -1538,7 +1548,7 @@ function narrowTypeForIsInstance(

if (isCallable) {
if (isPositiveTest) {
filteredTypes.push(unexpandedType);
filteredTypes.push(varType);
} else {
foundSuperclass = true;
}
Expand Down Expand Up @@ -1686,8 +1696,8 @@ function narrowTypeForIsInstance(
if (isClassInstance(subtype) && !isSubtypeTypeObject) {
return combineTypes(
filterClassType(
ClassType.cloneAsInstantiable(subtype),
convertToInstance(unexpandedSubtype),
ClassType.cloneAsInstantiable(subtype),
getTypeCondition(subtype),
negativeFallback
)
Expand All @@ -1712,7 +1722,7 @@ function narrowTypeForIsInstance(
} else {
if (isInstantiableClass(subtype)) {
return combineTypes(
filterClassType(subtype, unexpandedSubtype, getTypeCondition(subtype), negativeFallback)
filterClassType(unexpandedSubtype, subtype, getTypeCondition(subtype), negativeFallback)
);
}

Expand All @@ -1721,8 +1731,8 @@ function narrowTypeForIsInstance(
if (objectType && isClassInstance(objectType)) {
return combineTypes(
filterClassType(
ClassType.cloneAsInstantiable(objectType),
convertToInstantiable(unexpandedSubtype),
ClassType.cloneAsInstantiable(objectType),
getTypeCondition(subtype),
negativeFallback
)
Expand Down Expand Up @@ -2265,6 +2275,7 @@ function narrowTypeForClassComparison(
!ClassType.isSameGenericClass(concreteSubtype, classType) &&
!isIsinstanceFilterSuperclass(
evaluator,
subtype,
concreteSubtype,
classType,
classType,
Expand Down
2 changes: 1 addition & 1 deletion packages/pyright-internal/src/tests/samples/typeNarrowingIsinstance16.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,4 +16,4 @@ def baz(self, other: object):
reveal_type(other, expected_text="Self@ClassA")

if isinstance(other, (int, type(self))):
reveal_type(other, expected_text="Self@ClassA | int")
reveal_type(other, expected_text="ClassA | int | Self@ClassA")
42 changes: 39 additions & 3 deletions packages/pyright-internal/src/tests/samples/typeNarrowingIsinstance2.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,49 @@
# on "self" and other bound TypeVars.


class Base:
from typing import Self, TypeVar


class ClassA:
def get_value(self) -> int:
if isinstance(self, Derived):
if isinstance(self, ChildB):
return self.calculate()
return 7


class Derived(Base):
class ChildB(ClassA):
def calculate(self) -> int:
return 2 * 2


TC = TypeVar("TC")


class ClassC:
@classmethod
def test(cls: type[TC], id: int | TC):
if isinstance(id, cls):
reveal_type(id, expected_text="TC@test")
else:
reveal_type(id, expected_text="int")


TD = TypeVar("TD", bound="ClassD")


class ClassD:
@classmethod
def test(cls: type[TD], id: int | TD):
if isinstance(id, cls):
reveal_type(id, expected_text="ClassD*")
else:
reveal_type(id, expected_text="int")


class ClassE:
@classmethod
def test(cls: type[Self], id: int | Self):
if isinstance(id, cls):
reveal_type(id, expected_text="ClassE")
else:
reveal_type(id, expected_text="int")

0 comments on commit eda2b65

Please sign in to comment.