From 25087fdbb72d1495e6903d171dee999c47ba09fd Mon Sep 17 00:00:00 2001 From: Steven Troxler Date: Thu, 23 May 2024 01:59:06 -0400 Subject: [PATCH] Validate more about overrides on untyped methods (#17276) This commit fixes #9618 by making MyPy always complain if a method overrides a base class method marked as `@final`. In the process, it also adds a few additional validations: - Always verify the `@override` decorator, which ought to be pretty backward-compatible for most projects assuming that strict override checks aren't enabled by default (and it appears to me that `--enable-error-code explicit-override` is off by default) - Verify that the method signature is compatible (which in practice means only arity and argument name checks) *if* the `--check-untyped-defs` flag is set; it seems unlikely that a user would want mypy to validate the bodies of untyped functions but wouldn't want to be alerted about incompatible overrides. Note: I did also explore enabling the signature compatibility check for all code, which in principle makes sense. But the mypy_primer results indicated that there would be backward compability issues because too many libraries rely on us not validating this: https://github.com/python/mypy/pull/17274 --- mypy/checker.py | 23 ++++++++++++++++------- mypy/nodes.py | 6 ++++++ test-data/unit/check-dynamic-typing.test | 15 +++++++++++++++ test-data/unit/check-functions.test | 12 ++++++++++++ 4 files changed, 49 insertions(+), 7 deletions(-) diff --git a/mypy/checker.py b/mypy/checker.py index 3daf64daaac4..6da537fad5cb 100644 --- a/mypy/checker.py +++ b/mypy/checker.py @@ -1004,7 +1004,7 @@ def _visit_func_def(self, defn: FuncDef) -> None: """Type check a function definition.""" self.check_func_item(defn, name=defn.name) if defn.info: - if not defn.is_dynamic() and not defn.is_overload and not defn.is_decorated: + if not defn.is_overload and not defn.is_decorated: # If the definition is the implementation for an # overload, the legality of the override has already # been typechecked, and decorated methods will be @@ -1913,9 +1913,17 @@ def check_method_override( Return a list of base classes which contain an attribute with the method name. """ # Check against definitions in base classes. + check_override_compatibility = defn.name not in ( + "__init__", + "__new__", + "__init_subclass__", + "__post_init__", + ) and (self.options.check_untyped_defs or not defn.is_dynamic()) found_method_base_classes: list[TypeInfo] = [] for base in defn.info.mro[1:]: - result = self.check_method_or_accessor_override_for_base(defn, base) + result = self.check_method_or_accessor_override_for_base( + defn, base, check_override_compatibility + ) if result is None: # Node was deferred, we will have another attempt later. return None @@ -1924,7 +1932,10 @@ def check_method_override( return found_method_base_classes def check_method_or_accessor_override_for_base( - self, defn: FuncDef | OverloadedFuncDef | Decorator, base: TypeInfo + self, + defn: FuncDef | OverloadedFuncDef | Decorator, + base: TypeInfo, + check_override_compatibility: bool, ) -> bool | None: """Check if method definition is compatible with a base class. @@ -1945,10 +1956,8 @@ def check_method_or_accessor_override_for_base( if defn.is_final: self.check_if_final_var_override_writable(name, base_attr.node, defn) found_base_method = True - - # Check the type of override. - if name not in ("__init__", "__new__", "__init_subclass__", "__post_init__"): - # Check method override + if check_override_compatibility: + # Check compatibility of the override signature # (__init__, __new__, __init_subclass__ are special). if self.check_method_override_for_base_with_name(defn, name, base): return None diff --git a/mypy/nodes.py b/mypy/nodes.py index 21051ffa4d0b..e52618fcdae6 100644 --- a/mypy/nodes.py +++ b/mypy/nodes.py @@ -616,6 +616,9 @@ def deserialize(cls, data: JsonDict) -> OverloadedFuncDef: # NOTE: res.info will be set in the fixup phase. return res + def is_dynamic(self) -> bool: + return all(item.is_dynamic() for item in self.items) + class Argument(Node): """A single argument in a FuncItem.""" @@ -938,6 +941,9 @@ def deserialize(cls, data: JsonDict) -> Decorator: dec.is_overload = data["is_overload"] return dec + def is_dynamic(self) -> bool: + return self.func.is_dynamic() + VAR_FLAGS: Final = [ "is_self", diff --git a/test-data/unit/check-dynamic-typing.test b/test-data/unit/check-dynamic-typing.test index 0dc05a7a0ea1..21fd52169ff5 100644 --- a/test-data/unit/check-dynamic-typing.test +++ b/test-data/unit/check-dynamic-typing.test @@ -756,6 +756,21 @@ main:5: note: def f(self, x: A) -> None main:5: note: Subclass: main:5: note: def f(self, x: Any, y: Any) -> None +[case testInvalidOverrideArgumentCountWithImplicitSignature4] +# flags: --check-untyped-defs +import typing +class B: + def f(self, x: A) -> None: pass +class A(B): + def f(self, x, y): + x() +[out] +main:6: error: Signature of "f" incompatible with supertype "B" +main:6: note: Superclass: +main:6: note: def f(self, x: A) -> None +main:6: note: Subclass: +main:6: note: def f(self, x: Any, y: Any) -> Any + [case testInvalidOverrideWithImplicitSignatureAndClassMethod1] class B: @classmethod diff --git a/test-data/unit/check-functions.test b/test-data/unit/check-functions.test index 3aecbe065c27..fe01590c6c71 100644 --- a/test-data/unit/check-functions.test +++ b/test-data/unit/check-functions.test @@ -3228,3 +3228,15 @@ class A: reveal_type(A.f) # N: Revealed type is "__main__.something_callable" reveal_type(A().f) # N: Revealed type is "builtins.str" [builtins fixtures/property.pyi] + +[case testFinalOverrideOnUntypedDef] +from typing import final + +class Base: + @final + def foo(self): + pass + +class Derived(Base): + def foo(self): # E: Cannot override final attribute "foo" (previously declared in base class "Base") + pass