Fixed a bug that leads to a false positive error in the reportIncompatibleMethodOverride check when a child class is overriding an overloaded method in the base class and one or more of the overloads doesn't apply because the self or cls parameter is explicitly annotated in a way that's not applicable to the child class. This addresses https://github.com/microsoft/pyright/issues/5288. (#5302)

Co-authored-by: Eric Traut <erictr@microsoft.com>
This commit is contained in:
Eric Traut 2023-06-14 15:52:11 -07:00 committed by GitHub
parent 7bfe3153ce
commit b0c4716f7b
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
4 changed files with 182 additions and 15 deletions

View File

@ -5328,6 +5328,7 @@ export class Checker extends ParseTreeWalker {
!this._evaluator.validateOverrideMethod(
overriddenType,
overrideFunction,
/* baseClass */ undefined,
diagAddendum,
/* enforceParamNameMatch */ true
)
@ -5613,6 +5614,7 @@ export class Checker extends ParseTreeWalker {
!this._evaluator.validateOverrideMethod(
baseType,
overrideType,
childClassType,
diagAddendum,
enforceParamNameMatch
)
@ -5768,6 +5770,7 @@ export class Checker extends ParseTreeWalker {
!this._evaluator.validateOverrideMethod(
baseClassMethodType,
subclassMethodType,
childClassType,
diagAddendum.createAddendum()
)
) {

View File

@ -24197,6 +24197,7 @@ export function createTypeEvaluator(importLookup: ImportLookup, evaluatorOptions
function validateOverrideMethod(
baseMethod: Type,
overrideMethod: FunctionType | OverloadedFunctionType,
baseClass: ClassType | undefined,
diag: DiagnosticAddendum,
enforceParamNames = true
): boolean {
@ -24235,9 +24236,19 @@ export function createTypeEvaluator(importLookup: ImportLookup, evaluatorOptions
// For a non-overloaded method overriding an overloaded method, the
// override must match all of the overloads.
if (isFunction(overrideMethod)) {
return OverloadedFunctionType.getOverloads(baseMethod).every((overload) =>
validateOverrideMethodInternal(overload, overrideMethod, diag?.createAddendum(), enforceParamNames)
);
return OverloadedFunctionType.getOverloads(baseMethod).every((overload) => {
// If the override isn't applicable for this base class, skip the check.
if (baseClass && !isOverrideMethodApplicable(overload, baseClass)) {
return true;
}
return validateOverrideMethodInternal(
overload,
overrideMethod,
diag?.createAddendum(),
enforceParamNames
);
});
}
// For an overloaded method overriding an overloaded method, the overrides
@ -24246,8 +24257,15 @@ export function createTypeEvaluator(importLookup: ImportLookup, evaluatorOptions
let previousMatchIndex = -1;
let overrideOverloadIndex = 0;
const baseOverloads = OverloadedFunctionType.getOverloads(baseMethod);
for (const overrideOverload of OverloadedFunctionType.getOverloads(overrideMethod)) {
const matchIndex = OverloadedFunctionType.getOverloads(baseMethod).findIndex((baseOverload) => {
const matchIndex = baseOverloads.findIndex((baseOverload) => {
// If the override isn't applicable for this base class, skip the check.
if (baseClass && !isOverrideMethodApplicable(baseOverload, baseClass)) {
return false;
}
return validateOverrideMethodInternal(
baseOverload,
overrideOverload,
@ -24276,12 +24294,57 @@ export function createTypeEvaluator(importLookup: ImportLookup, evaluatorOptions
return true;
}
// Determines whether the override method is compatible with the base method.
// If enforceParamNames is true, the parameter names of non-positional-only
// parameters are enforced. If exemptSelfClsParam is true, the "self" and "cls"
// parameters are exempted from type checks. This is normally the case except
// with overloaded method overrides where the "self" or "cls" parameter type
// must be honored to differentiate between overloads.
// Determines whether a child class override is applicable to a parent
// class method signature. This is important in cases where the parent
// class defines an overload where some of the overload signatures supply
// explicit type annotations for the "self" or "cls" parameter and some
// of these do not apply to the child class.
function isOverrideMethodApplicable(baseMethod: FunctionType, childClass: ClassType): boolean {
if (
!FunctionType.isInstanceMethod(baseMethod) &&
!FunctionType.isClassMethod(baseMethod) &&
!FunctionType.isConstructorMethod(baseMethod)
) {
return true;
}
const baseParamDetails = getParameterListDetails(baseMethod);
if (baseParamDetails.params.length === 0) {
return true;
}
const baseParamType = baseParamDetails.params[0].param;
if (baseParamType.category !== ParameterCategory.Simple || !baseParamType.hasDeclaredType) {
return true;
}
// If this is a self or cls parameter, determine whether the override
// class can be assigned to the base parameter type. If not, then this
// override doesn't apply. This is important for overloads where the
// base class contains some overload signatures that are not applicable
// to the child class.
const childSelfOrClsType = FunctionType.isInstanceMethod(baseMethod)
? ClassType.cloneAsInstance(childClass)
: childClass;
return assignType(
baseParamType.type,
childSelfOrClsType,
/* diag */ undefined,
/* destTypeVarContext */ undefined,
/* srcTypeVarContext */ undefined,
AssignTypeFlags.SkipSolveTypeVars
);
}
// Determines whether the override method is compatible with the overridden method.
// This is used both for parent/child overrides and implicit overrides for peer
// classes in a multi-inheritance case. If enforceParamNames is true, the parameter
// names of non-positional-only parameters are enforced. If exemptSelfClsParam
// is true, the "self" and "cls" parameters are exempted from type checks.
// This is normally the case except with overloaded method overrides where the
// "self" or "cls" parameter type must be honored to differentiate between overloads.
function validateOverrideMethodInternal(
baseMethod: FunctionType,
overrideMethod: FunctionType,
@ -24310,8 +24373,7 @@ export function createTypeEvaluator(importLookup: ImportLookup, evaluatorOptions
diag?.addMessage(Localizer.DiagnosticAddendum.overrideNotClassMethod());
canOverride = false;
}
}
if (FunctionType.isInstanceMethod(baseMethod)) {
} else if (FunctionType.isInstanceMethod(baseMethod)) {
if (!FunctionType.isInstanceMethod(overrideMethod)) {
diag?.addMessage(Localizer.DiagnosticAddendum.overrideNotInstanceMethod());
canOverride = false;
@ -24526,14 +24588,14 @@ export function createTypeEvaluator(importLookup: ImportLookup, evaluatorOptions
paramInfo.source === ParameterSource.KeywordOnly &&
paramInfo.param.category === ParameterCategory.Simple
);
const overrideWkOnlyParams = overrideParamDetails.params.filter(
const overrideKwOnlyParams = overrideParamDetails.params.filter(
(paramInfo) =>
paramInfo.source === ParameterSource.KeywordOnly &&
paramInfo.param.category === ParameterCategory.Simple
);
baseKwOnlyParams.forEach((paramInfo) => {
const overrideParamInfo = overrideWkOnlyParams.find((pi) => paramInfo.param.name === pi.param.name);
const overrideParamInfo = overrideKwOnlyParams.find((pi) => paramInfo.param.name === pi.param.name);
if (!overrideParamInfo && overrideParamDetails.kwargsIndex === undefined) {
diag?.addMessage(
@ -24583,7 +24645,7 @@ export function createTypeEvaluator(importLookup: ImportLookup, evaluatorOptions
// Verify that any keyword-only parameters added by the overload are compatible
// with the **kwargs in the base.
overrideWkOnlyParams.forEach((paramInfo) => {
overrideKwOnlyParams.forEach((paramInfo) => {
const baseParamInfo = baseKwOnlyParams.find((pi) => paramInfo.param.name === pi.param.name);
if (!baseParamInfo) {

View File

@ -584,6 +584,7 @@ export interface TypeEvaluator {
validateOverrideMethod: (
baseMethod: Type,
overrideMethod: FunctionType | OverloadedFunctionType,
baseClass: ClassType | undefined,
diag: DiagnosticAddendum,
enforceParamNames?: boolean
) => boolean;

View File

@ -102,3 +102,104 @@ class Child1_6(Parent1[bytes]):
def m1(self, x: bool | bytes) -> int | float | bytes:
return x
class Parent2(Generic[_T]):
@overload
def method1(self: "Parent2[int]", x: list[int]) -> list[int]:
...
@overload
def method1(self, x: str) -> dict[str, str]:
...
def method1(self, x: Any) -> Any:
...
@overload
def method2(self: "Parent2[int]", x: list[int]) -> list[int]:
...
@overload
def method2(self, x: str) -> dict[str, str]:
...
@overload
def method2(self, x: int) -> int:
...
def method2(self, x: Any) -> Any:
...
@overload
@classmethod
def method3(cls: "type[Parent2[int]]", x: list[int]) -> list[int]:
...
@overload
@classmethod
def method3(cls, x: str) -> dict[str, str]:
...
@classmethod
def method3(cls, x: Any) -> Any:
...
@overload
@classmethod
def method4(cls: "type[Parent2[int]]", x: list[int]) -> list[int]:
...
@overload
@classmethod
def method4(cls, x: str) -> dict[str, str]:
...
@overload
@classmethod
def method4(cls, x: int) -> int:
...
@classmethod
def method4(cls, x: Any) -> Any:
...
class Child2_1(Parent2[str]):
def method1(self, x: str) -> dict[str, str]:
...
class Child2_2(Parent2[str]):
@overload
def method2(self, x: str) -> dict[str, str]:
...
@overload
def method2(self, x: int) -> int:
...
def method2(self, x: Any) -> Any:
...
class Child2_3(Parent2[str]):
@classmethod
def method3(cls, x: str) -> dict[str, str]:
...
class Child2_4(Parent2[str]):
@overload
@classmethod
def method4(cls, x: str) -> dict[str, str]:
...
@overload
@classmethod
def method4(cls, x: int) -> int:
...
@classmethod
def method4(cls, x: Any) -> Any:
...