Skip to content

Commit

Permalink
Validate more about overrides on untyped methods (#17276)
Browse files Browse the repository at this point in the history
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:
#17274
  • Loading branch information
stroxler authored May 23, 2024
1 parent 0871c93 commit 25087fd
Show file tree
Hide file tree
Showing 4 changed files with 49 additions and 7 deletions.
23 changes: 16 additions & 7 deletions mypy/checker.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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.
Expand All @@ -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
Expand Down
6 changes: 6 additions & 0 deletions mypy/nodes.py
Original file line number Diff line number Diff line change
Expand Up @@ -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."""
Expand Down Expand Up @@ -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",
Expand Down
15 changes: 15 additions & 0 deletions test-data/unit/check-dynamic-typing.test
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
12 changes: 12 additions & 0 deletions test-data/unit/check-functions.test
Original file line number Diff line number Diff line change
Expand Up @@ -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

0 comments on commit 25087fd

Please sign in to comment.