From 20942b3389842c636b15591c6db09151acb00d61 Mon Sep 17 00:00:00 2001 From: Eric Traut Date: Wed, 19 Jun 2024 08:52:40 +0200 Subject: [PATCH] Added support for detecting mismatched property types in base classes used for multiple inheritance. This addresses #8167. (#8175) --- .../pyright-internal/src/analyzer/checker.ts | 364 +++++++++++++----- .../src/tests/samples/methodOverride3.py | 128 +++--- .../src/tests/typeEvaluator3.test.ts | 2 +- 3 files changed, 333 insertions(+), 161 deletions(-) diff --git a/packages/pyright-internal/src/analyzer/checker.ts b/packages/pyright-internal/src/analyzer/checker.ts index 4bcd87d38..f7991196a 100644 --- a/packages/pyright-internal/src/analyzer/checker.ts +++ b/packages/pyright-internal/src/analyzer/checker.ts @@ -5954,7 +5954,15 @@ export class Checker extends ParseTreeWalker { ); } } else { - // TODO - check types of property methods fget, fset, fdel. + this._validateMultipleInheritancePropertyOverride( + overriddenClassAndSymbol.classType, + childClassType, + overriddenType, + overrideType, + overrideSymbol, + memberName, + errorNode + ); } } else { // This check can be expensive, so don't perform it if the corresponding @@ -6051,26 +6059,156 @@ export class Checker extends ParseTreeWalker { } if (diag && overrideDecl && overriddenDecl) { - diag.addRelatedInfo( - LocAddendum.baseClassOverriddenType().format({ - baseClass: this._evaluator.printType(convertToInstance(overriddenClassAndSymbol.classType)), - type: this._evaluator.printType(overriddenType), - }), - overriddenDecl.uri, - overriddenDecl.range - ); - - diag.addRelatedInfo( - LocAddendum.baseClassOverridesType().format({ - baseClass: this._evaluator.printType(convertToInstance(overrideClassAndSymbol.classType)), - type: this._evaluator.printType(overrideType), - }), - overrideDecl.uri, - overrideDecl.range + this._addMultipleInheritanceRelatedInfo( + diag, + overriddenClassAndSymbol.classType, + overriddenType, + overriddenDecl, + overrideClassAndSymbol.classType, + overrideType, + overrideDecl ); } } + private _addMultipleInheritanceRelatedInfo( + diag: Diagnostic, + overriddenClass: ClassType, + overriddenType: Type, + overriddenDecl: Declaration, + overrideClass: ClassType, + overrideType: Type, + overrideDecl: Declaration + ) { + diag.addRelatedInfo( + LocAddendum.baseClassOverriddenType().format({ + baseClass: this._evaluator.printType(convertToInstance(overriddenClass)), + type: this._evaluator.printType(overriddenType), + }), + overriddenDecl.uri, + overriddenDecl.range + ); + + diag.addRelatedInfo( + LocAddendum.baseClassOverridesType().format({ + baseClass: this._evaluator.printType(convertToInstance(overrideClass)), + type: this._evaluator.printType(overrideType), + }), + overrideDecl.uri, + overrideDecl.range + ); + } + + private _validateMultipleInheritancePropertyOverride( + overriddenClassType: ClassType, + overrideClassType: ClassType, + overriddenSymbolType: Type, + overrideSymbolType: Type, + overrideSymbol: Symbol, + memberName: string, + errorNode: ParseNode + ) { + const propMethodInfo: [string, (c: ClassType) => FunctionType | undefined][] = [ + ['fget', (c) => c.fgetInfo?.methodType], + ['fset', (c) => c.fsetInfo?.methodType], + ['fdel', (c) => c.fdelInfo?.methodType], + ]; + + propMethodInfo.forEach((info) => { + const diagAddendum = new DiagnosticAddendum(); + const [methodName, methodAccessor] = info; + const baseClassPropMethod = methodAccessor(overriddenSymbolType as ClassType); + const subclassPropMethod = methodAccessor(overrideSymbolType as ClassType); + + // Is the method present on the base class but missing in the subclass? + if (baseClassPropMethod) { + const baseClassMethodType = partiallySpecializeType(baseClassPropMethod, overriddenClassType); + + if (isFunction(baseClassMethodType)) { + if (!subclassPropMethod) { + // The method is missing. + diagAddendum.addMessage( + LocAddendum.propertyMethodMissing().format({ + name: methodName, + }) + ); + + const decls = overrideSymbol.getDeclarations(); + + if (decls.length > 0) { + const lastDecl = decls[decls.length - 1]; + const diag = this._evaluator.addDiagnostic( + DiagnosticRule.reportIncompatibleMethodOverride, + LocMessage.propertyOverridden().format({ + name: memberName, + className: overriddenClassType.details.name, + }) + diagAddendum.getString(), + errorNode + ); + + const origDecl = baseClassMethodType.details.declaration; + if (diag && origDecl) { + this._addMultipleInheritanceRelatedInfo( + diag, + overriddenClassType, + overriddenSymbolType, + origDecl, + overrideClassType, + overrideSymbolType, + lastDecl + ); + } + } + } else { + const subclassMethodType = partiallySpecializeType(subclassPropMethod, overrideClassType); + + if (isFunction(subclassMethodType)) { + if ( + !this._evaluator.validateOverrideMethod( + baseClassMethodType, + subclassMethodType, + overrideClassType, + diagAddendum.createAddendum() + ) + ) { + diagAddendum.addMessage( + LocAddendum.propertyMethodIncompatible().format({ + name: methodName, + }) + ); + const decl = subclassMethodType.details.declaration; + + if (decl && decl.type === DeclarationType.Function) { + const diag = this._evaluator.addDiagnostic( + DiagnosticRule.reportIncompatibleMethodOverride, + LocMessage.propertyOverridden().format({ + name: memberName, + className: overriddenClassType.details.name, + }) + diagAddendum.getString(), + errorNode + ); + + const origDecl = baseClassMethodType.details.declaration; + if (diag && origDecl) { + this._addMultipleInheritanceRelatedInfo( + diag, + overriddenClassType, + overriddenSymbolType, + origDecl, + overrideClassType, + overrideSymbolType, + decl + ); + } + } + } + } + } + } + } + }); + } + // Validates that any overloaded methods are consistent in how they // are decorated. For example, if the first overload is not marked @final // but subsequent ones are, an error should be reported. @@ -6572,93 +6710,14 @@ export class Checker extends ParseTreeWalker { ); } } else { - const baseClassType = baseClass; - const propMethodInfo: [string, (c: ClassType) => FunctionType | undefined][] = [ - ['fget', (c) => c.fgetInfo?.methodType], - ['fset', (c) => c.fsetInfo?.methodType], - ['fdel', (c) => c.fdelInfo?.methodType], - ]; - - propMethodInfo.forEach((info) => { - const diagAddendum = new DiagnosticAddendum(); - const [methodName, methodAccessor] = info; - const baseClassPropMethod = methodAccessor(baseType as ClassType); - const subclassPropMethod = methodAccessor(overrideType as ClassType); - - // Is the method present on the base class but missing in the subclass? - if (baseClassPropMethod) { - const baseClassMethodType = partiallySpecializeType(baseClassPropMethod, baseClassType); - if (isFunction(baseClassMethodType)) { - if (!subclassPropMethod) { - // The method is missing. - diagAddendum.addMessage( - LocAddendum.propertyMethodMissing().format({ - name: methodName, - }) - ); - const decls = overrideSymbol.getDeclarations(); - if (decls.length > 0) { - const lastDecl = decls[decls.length - 1]; - const diag = this._evaluator.addDiagnostic( - DiagnosticRule.reportIncompatibleMethodOverride, - LocMessage.propertyOverridden().format({ - name: memberName, - className: baseClassType.details.name, - }) + diagAddendum.getString(), - getNameNodeForDeclaration(lastDecl) ?? lastDecl.node - ); - - const origDecl = baseClassMethodType.details.declaration; - if (diag && origDecl) { - diag.addRelatedInfo( - LocAddendum.overriddenMethod(), - origDecl.uri, - origDecl.range - ); - } - } - } else { - const subclassMethodType = partiallySpecializeType(subclassPropMethod, childClassType); - if (isFunction(subclassMethodType)) { - if ( - !this._evaluator.validateOverrideMethod( - baseClassMethodType, - subclassMethodType, - childClassType, - diagAddendum.createAddendum() - ) - ) { - diagAddendum.addMessage( - LocAddendum.propertyMethodIncompatible().format({ - name: methodName, - }) - ); - const decl = subclassMethodType.details.declaration; - if (decl && decl.type === DeclarationType.Function) { - const diag = this._evaluator.addDiagnostic( - DiagnosticRule.reportIncompatibleMethodOverride, - LocMessage.propertyOverridden().format({ - name: memberName, - className: baseClassType.details.name, - }) + diagAddendum.getString(), - decl.node.name - ); - - const origDecl = baseClassMethodType.details.declaration; - if (diag && origDecl) { - diag.addRelatedInfo( - LocAddendum.overriddenMethod(), - origDecl.uri, - origDecl.range - ); - } - } - } - } - } - } - } - }); + this._validatePropertyOverride( + baseClass, + childClassType, + baseType, + overrideType, + overrideSymbol, + memberName + ); } } else { // This check can be expensive, so don't perform it if the corresponding @@ -6859,6 +6918,103 @@ export class Checker extends ParseTreeWalker { } } + private _validatePropertyOverride( + baseClassType: ClassType, + childClassType: ClassType, + baseType: Type, + childType: Type, + overrideSymbol: Symbol, + memberName: string + ) { + const propMethodInfo: [string, (c: ClassType) => FunctionType | undefined][] = [ + ['fget', (c) => c.fgetInfo?.methodType], + ['fset', (c) => c.fsetInfo?.methodType], + ['fdel', (c) => c.fdelInfo?.methodType], + ]; + + propMethodInfo.forEach((info) => { + const diagAddendum = new DiagnosticAddendum(); + const [methodName, methodAccessor] = info; + const baseClassPropMethod = methodAccessor(baseType as ClassType); + const subclassPropMethod = methodAccessor(childType as ClassType); + + // Is the method present on the base class but missing in the subclass? + if (baseClassPropMethod) { + const baseClassMethodType = partiallySpecializeType(baseClassPropMethod, baseClassType); + + if (isFunction(baseClassMethodType)) { + if (!subclassPropMethod) { + // The method is missing. + diagAddendum.addMessage( + LocAddendum.propertyMethodMissing().format({ + name: methodName, + }) + ); + + const decls = overrideSymbol.getDeclarations(); + + if (decls.length > 0) { + const lastDecl = decls[decls.length - 1]; + const diag = this._evaluator.addDiagnostic( + DiagnosticRule.reportIncompatibleMethodOverride, + LocMessage.propertyOverridden().format({ + name: memberName, + className: baseClassType.details.name, + }) + diagAddendum.getString(), + getNameNodeForDeclaration(lastDecl) ?? lastDecl.node + ); + + const origDecl = baseClassMethodType.details.declaration; + if (diag && origDecl) { + diag.addRelatedInfo(LocAddendum.overriddenMethod(), origDecl.uri, origDecl.range); + } + } + } else { + const subclassMethodType = partiallySpecializeType(subclassPropMethod, childClassType); + + if (isFunction(subclassMethodType)) { + if ( + !this._evaluator.validateOverrideMethod( + baseClassMethodType, + subclassMethodType, + childClassType, + diagAddendum.createAddendum() + ) + ) { + diagAddendum.addMessage( + LocAddendum.propertyMethodIncompatible().format({ + name: methodName, + }) + ); + const decl = subclassMethodType.details.declaration; + + if (decl && decl.type === DeclarationType.Function) { + const diag = this._evaluator.addDiagnostic( + DiagnosticRule.reportIncompatibleMethodOverride, + LocMessage.propertyOverridden().format({ + name: memberName, + className: baseClassType.details.name, + }) + diagAddendum.getString(), + decl.node.name + ); + + const origDecl = baseClassMethodType.details.declaration; + if (diag && origDecl) { + diag.addRelatedInfo( + LocAddendum.overriddenMethod(), + origDecl.uri, + origDecl.range + ); + } + } + } + } + } + } + } + }); + } + // Performs checks on a function that is located within a class // and has been determined not to be a property accessor. private _validateMethod(node: FunctionNode, functionType: FunctionType, classNode: ClassNode) { diff --git a/packages/pyright-internal/src/tests/samples/methodOverride3.py b/packages/pyright-internal/src/tests/samples/methodOverride3.py index 64d768789..9c40919b4 100644 --- a/packages/pyright-internal/src/tests/samples/methodOverride3.py +++ b/packages/pyright-internal/src/tests/samples/methodOverride3.py @@ -7,96 +7,78 @@ from typing import Generic, Iterable, ParamSpec, TypeVar class A1: - def func1(self, a: int) -> str: - ... + def func1(self, a: int) -> str: ... class A2: - def func1(self, a: int, b: int = 3) -> str: - ... + def func1(self, a: int, b: int = 3) -> str: ... # This should generate an error because func1 is incompatible. -class ASub(A1, A2): - ... +class ASub(A1, A2): ... class B1: - def func1(self) -> int: - ... + def func1(self) -> int: ... class B2: - def func1(self) -> float: - ... + def func1(self) -> float: ... -class BSub(B1, B2): - ... +class BSub(B1, B2): ... class C1: - def func1(self) -> float: - ... + def func1(self) -> float: ... class C2: - def func1(self) -> int: - ... + def func1(self) -> int: ... # This should generate an error because func1 is incompatible. -class CSub(C1, C2): - ... +class CSub(C1, C2): ... class D1: - def func1(self, a: int) -> None: - ... + def func1(self, a: int) -> None: ... class D2: - def func1(self, b: int) -> None: - ... + def func1(self, b: int) -> None: ... # This should generate an error because func1 is incompatible. -class DSub(D1, D2): - ... +class DSub(D1, D2): ... _T_E = TypeVar("_T_E") class E1(Generic[_T_E]): - def func1(self, a: _T_E) -> None: - ... + def func1(self, a: _T_E) -> None: ... class E2(Generic[_T_E]): - def func1(self, a: _T_E) -> None: - ... + def func1(self, a: _T_E) -> None: ... -class ESub(E1[int], E2[int]): - ... +class ESub(E1[int], E2[int]): ... _T_F = TypeVar("_T_F") class F1(Generic[_T_F]): - def do_stuff(self) -> Iterable[_T_F]: - ... + def do_stuff(self) -> Iterable[_T_F]: ... class F2(F1[_T_F]): - def do_stuff(self) -> Iterable[_T_F]: - ... + def do_stuff(self) -> Iterable[_T_F]: ... -class F3(F1[_T_F]): - ... +class F3(F1[_T_F]): ... class FSub1(F3[int], F2[int]): @@ -116,43 +98,77 @@ _R = TypeVar("_R") class G1(Generic[_P, _R]): - def f(self, *args: _P.args, **kwargs: _P.kwargs) -> _R: - ... + def f(self, *args: _P.args, **kwargs: _P.kwargs) -> _R: ... - def g(self) -> _R: - ... + def g(self) -> _R: ... class G2(G1[_P, _R]): # This should generate an error because f is missing ParamSpec parameters. - def f(self) -> _R: - ... + def f(self) -> _R: ... - def g(self, *args: _P.args, **kwargs: _P.kwargs) -> _R: - ... + def g(self, *args: _P.args, **kwargs: _P.kwargs) -> _R: ... class G3(G1[[], _R]): - def f(self) -> _R: - ... + def f(self) -> _R: ... - def g(self) -> _R: - ... + def g(self) -> _R: ... class G4(G1[[int, int], str]): - def f(self, a: int, b: int, /) -> str: - ... + def f(self, a: int, b: int, /) -> str: ... - def g(self) -> str: - ... + def g(self) -> str: ... class G5(G1[[], str]): # This should generate an error because the specialized # signature of f in the base class has no positional parameters. - def f(self, a: int, b: int) -> str: - ... + def f(self, a: int, b: int) -> str: ... - def g(self) -> str: - ... + def g(self) -> str: ... + + +class H1: + @property + def prop1(self) -> int: + return 3 + + @property + def prop2(self) -> int: + return 3 + + @prop2.setter + def prop2(self, val: int) -> None: + pass + + @property + def prop3(self) -> int: + return 3 + + @prop3.setter + def prop3(self, val: int) -> None: + pass + + +class H2: + @property + def prop1(self) -> str: + return "" + + @property + def prop2(self) -> int: + return 3 + + @property + def prop3(self) -> int: + return 3 + + @prop3.setter + def prop3(self, val: str) -> None: + pass + + +# This should generate three errors: prop1, prop2 and prop3. +class H(H2, H1): ... diff --git a/packages/pyright-internal/src/tests/typeEvaluator3.test.ts b/packages/pyright-internal/src/tests/typeEvaluator3.test.ts index 8f48d9457..f7660e287 100644 --- a/packages/pyright-internal/src/tests/typeEvaluator3.test.ts +++ b/packages/pyright-internal/src/tests/typeEvaluator3.test.ts @@ -910,7 +910,7 @@ test('MethodOverride3', () => { configOptions.diagnosticRuleSet.reportIncompatibleMethodOverride = 'error'; analysisResults = TestUtils.typeAnalyzeSampleFiles(['methodOverride3.py'], configOptions); - TestUtils.validateResults(analysisResults, 5); + TestUtils.validateResults(analysisResults, 8); }); test('MethodOverride4', () => {